Skip to content

F-018: chore: add clippy.toml with disallowed-methods / disallowed-macros#21

Open
Sephyi wants to merge 1 commit intodevelopmentfrom
audit/f-018-clippy-toml
Open

F-018: chore: add clippy.toml with disallowed-methods / disallowed-macros#21
Sephyi wants to merge 1 commit intodevelopmentfrom
audit/f-018-clippy-toml

Conversation

@Sephyi
Copy link
Copy Markdown
Owner

@Sephyi Sephyi commented Apr 22, 2026

Summary

chore: add clippy.toml with disallowed-methods / disallowed-macros.

Audit context

Closes audit entry F-018 from #3.

Verification

  • cargo fmt --check
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test --all-targets

Note: one pre-existing test porcelain_exits_within_timeout_with_no_staged_changes is a known macOS cold-start flake that reproduces on unmodified development — unrelated to this change.

Add a minimal, pragmatic `clippy.toml` that catches two common
anti-patterns at lint time:

- `disallowed-methods` blocks `std::process::Command::new`, pushing
  async call-sites toward `tokio::process::Command` or
  `spawn_blocking`. Synchronous contexts (integration tests, CLI
  bootstrap before the runtime starts) opt out locally via
  `#[allow(clippy::disallowed_methods)]` with a comment.
- `disallowed-macros` blocks `std::dbg` so leftover debug scaffolding
  can't ship; `tracing::{debug,trace}` is the durable replacement.

Existing legitimate sync `Command` uses are annotated rather than
rewritten:

- `src/app.rs::hook_dir` runs in the synchronous handle_hook bootstrap
  path; F-002 will migrate it off sync Command, and the allow here
  documents the coordination so this PR doesn't block on F-002.
- `tests/history.rs` and `tests/porcelain.rs` set a module-level
  `#![allow(clippy::disallowed_methods)]` because integration tests
  legitimately shell out to git / the commitbee binary synchronously.

Closes audit entry F-018 from #3.
Copilot AI review requested due to automatic review settings April 22, 2026 19:51
@Sephyi Sephyi added the audit Codebase audit cleanup (issue #3) label Apr 22, 2026
@Sephyi Sephyi self-assigned this Apr 22, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a repository-wide clippy.toml configuration to enforce avoidance of blocking process spawning in async contexts and prevent shipping leftover debug macros, aligning with audit finding F-018.

Changes:

  • Introduces clippy.toml with disallowed-methods (blocking std::process::Command::new) and disallowed-macros (blocking dbg!).
  • Adds #[allow(clippy::disallowed_methods)] in select integration tests and in the hook path to avoid the new lint blocking existing code.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
clippy.toml Adds Clippy configuration for disallowed methods/macros tied to audit F-018.
src/app.rs Locally suppresses disallowed_methods for the git hook directory lookup.
tests/history.rs Suppresses disallowed_methods for git-fixture setup in history integration tests.
tests/porcelain.rs Suppresses disallowed_methods for porcelain contract tests that shell out to git/commitbee.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/app.rs
Comment on lines +993 to 1000
// `hook_dir` is called from a synchronous `handle_hook` path (CLI
// bootstrap, no tokio runtime live yet). F-002 will migrate the hook /
// clipboard paths off sync `Command`; until then, allow the lint here
// so the new clippy.toml rule doesn't block unrelated PRs.
#[allow(clippy::disallowed_methods)]
let output = std::process::Command::new("git")
.args(["rev-parse", "--git-dir"])
.output()?;
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The justification comment is inaccurate: this code runs under the Tokio runtime (main is #[tokio::main] and Commands::Hook is handled inside app.run().await), so std::process::Command here can still block a runtime worker thread. Either update the comment to reflect the actual execution context and why blocking is acceptable temporarily, or (preferably) move the blocking call behind spawn_blocking / tokio::process::Command so the new lint remains meaningful in production code paths.

Suggested change
// `hook_dir` is called from a synchronous `handle_hook` path (CLI
// bootstrap, no tokio runtime live yet). F-002 will migrate the hook /
// clipboard paths off sync `Command`; until then, allow the lint here
// so the new clippy.toml rule doesn't block unrelated PRs.
#[allow(clippy::disallowed_methods)]
let output = std::process::Command::new("git")
.args(["rev-parse", "--git-dir"])
.output()?;
let output = if tokio::runtime::Handle::try_current().is_ok() {
tokio::task::block_in_place(|| {
std::process::Command::new("git")
.args(["rev-parse", "--git-dir"])
.output()
})?
} else {
std::process::Command::new("git")
.args(["rev-parse", "--git-dir"])
.output()?
};

Copilot uses AI. Check for mistakes.
Comment thread tests/history.rs
Comment on lines +5 to +8
// Integration tests are synchronous and legitimately use `std::process::Command`
// to build git fixtures in tempdirs; the `disallowed_methods` rule in
// clippy.toml targets async-context misuse, which does not apply here.
#![allow(clippy::disallowed_methods)]
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file contains multiple #[tokio::test] async fn ... tests that call std::process::Command::new("git"). With a crate-level #![allow(clippy::disallowed_methods)], the new lint is silenced even for these async contexts, which defeats the purpose of the rule and makes the leading comment (“tests are synchronous”) incorrect. Prefer narrowing the allow to the specific sync helpers/call sites (with a short justification), or switch those async test command invocations to tokio::process::Command / spawn_blocking.

Suggested change
// Integration tests are synchronous and legitimately use `std::process::Command`
// to build git fixtures in tempdirs; the `disallowed_methods` rule in
// clippy.toml targets async-context misuse, which does not apply here.
#![allow(clippy::disallowed_methods)]
// Keep `clippy::disallowed_methods` enabled for this file. If a synchronous
// helper needs `std::process::Command` to build git fixtures in a tempdir,
// allow it narrowly at that specific helper or call site with a short
// justification rather than silencing the lint for async tests as well.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audit Codebase audit cleanup (issue #3)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants