F-002: refactor(app): make hook and clipboard methods non-blocking#11
F-002: refactor(app): make hook and clipboard methods non-blocking#11Sephyi wants to merge 1 commit intodevelopmentfrom
Conversation
Replace synchronous std::process::Command and arboard calls with non-blocking alternatives so the tokio runtime isn't blocked: - handle_hook, hook_dir, hook_path, hook_install, hook_uninstall, and hook_status are now async; hook_dir uses tokio::process::Command with an explicit std::process::Output annotation (CLAUDE.md gotcha). - copy_to_clipboard wraps arboard::Clipboard::new() and set_text() in tokio::task::spawn_blocking; its signature changes from &str to String to satisfy the 'static bound. Closes audit entry F-002 from #3.
There was a problem hiding this comment.
Pull request overview
Refactors the app’s hook and clipboard paths to avoid blocking the async runtime, addressing audit finding F-002 (sync process/clipboard calls on the tokio runtime).
Changes:
- Make hook command handling async and run
git rev-parse --git-dirviatokio::process::Command. - Offload clipboard writes (
arboard) totokio::task::spawn_blockingand make clipboard helper async. - Update command dispatch / clipboard flow to
awaitthe new async helpers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Ok(()) | ||
| }) | ||
| .await | ||
| .map_err(|e| Error::Config(format!("Clipboard task panicked: {e}")))? |
There was a problem hiding this comment.
The JoinError from spawn_blocking can represent either a panic or task cancellation (e.g., runtime shutdown). This error message always says "panicked", which can be misleading. Consider branching on e.is_panic() / e.is_cancelled() and wording the message accordingly (or avoid claiming panic unless is_panic() is true).
| .map_err(|e| Error::Config(format!("Clipboard task panicked: {e}")))? | |
| .map_err(|e| { | |
| Error::Config(if e.is_panic() { | |
| format!("Clipboard task panicked: {e}") | |
| } else if e.is_cancelled() { | |
| format!("Clipboard task was cancelled: {e}") | |
| } else { | |
| format!("Clipboard task failed to join: {e}") | |
| }) | |
| })? |
| // Annotated type per CLAUDE.md gotcha: tokio::process::Command output | ||
| // needs explicit std::process::Output typing when used with `?`. |
There was a problem hiding this comment.
This comment references the CLAUDE.md gotcha but says the std::process::Output annotation is needed when using ?. The documented gotcha is specifically about .ok()? (type inference failure); with .await? and subsequent output.status usage, the annotation isn't required. Consider either removing the comment or rewording it to match the actual gotcha to avoid confusing future readers.
| // Annotated type per CLAUDE.md gotcha: tokio::process::Command output | |
| // needs explicit std::process::Output typing when used with `?`. | |
| // Keep the explicit type annotation for clarity; it is not required by `.await?` here. |
|
Closing as the conflict with the merged audit batch is too entangled for an in-place rebase: src/app.rs has been rewritten by #17 (instrument spans), #21 (clippy disallowed_methods + #[allow] shim), #26 (Ctrl+C handler refactor), and #28 (binary→lib extraction). Resolving all four against this branch's async refactor would be a near-rewrite. A clean re-implementation should:
Reopening as a fresh PR is faster than untangling this rebase. |
Replaces the F-018 sequencing shim at `src/app.rs:1030` with a real
async migration of the two paths the audit flagged for blocking the
Tokio runtime.
Hook path:
- `hook_dir` → `async fn`, uses `tokio::process::Command` for
`git rev-parse --git-dir` (with the explicit `std::process::Output`
type annotation per CLAUDE.md gotcha).
- `hook_path`, `hook_install`, `hook_uninstall`, `hook_status`,
and `handle_hook` are all `async fn` and `.await` the chain.
- `Commands::Hook { action }` dispatch in `handle_command` now
`.await`s `handle_hook`.
Clipboard path:
- `arboard` is a synchronous library (not a subprocess), so the
correct fix is `tokio::task::spawn_blocking` rather than
`tokio::process::Command`.
- `copy_to_clipboard` now takes `String` (owned, since it crosses the
blocking-task boundary) and returns the result via `await`.
- The `--clipboard` flow in `generate_commit` calls `await` on the
helper.
The `#[allow(clippy::disallowed_methods)]` shim added in #21 (F-018)
is removed — the lint now has nothing to suppress in this file.
Closes #11.
Summary
refactor(app): make hook and clipboard methods non-blocking.
Audit context
Closes audit entry F-002 from #3.
Verification
cargo fmt --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-targets