From 51837b9e562805a327638a873d124955086ef4e2 Mon Sep 17 00:00:00 2001 From: Brett Date: Wed, 15 Apr 2026 21:27:38 -0500 Subject: [PATCH 1/6] =?UTF-8?q?refactor:=20enforce=20check=5Fx=20=E2=86=92?= =?UTF-8?q?=20CheckStatus=20convention=20across=20all=20source=20checks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tier 1: Document the Source Check Convention in CLAUDE.md — check_x() returns CheckStatus (not CheckResult), run() is the sole CheckResult constructor using self.id()/self.group()/self.layer(). Tier 2: Apply the convention to all 16 Rust source checks. Each check_x() function now returns CheckStatus directly, eliminating ID/group/layer string literal triplication. Net -132 lines. Tier 3: Add convention_check_x_returns_check_status_not_check_result integration test that greps source check files for the old pattern and fails if any check_x() still returns CheckResult. --- CLAUDE.md | 18 ++++++ src/checks/source/rust/env_flags.rs | 65 +++++++------------ src/checks/source/rust/error_types.rs | 6 +- src/checks/source/rust/exit_codes.rs | 49 ++++++--------- src/checks/source/rust/global_flags.rs | 47 +++++--------- src/checks/source/rust/headless_auth.rs | 51 ++++++--------- src/checks/source/rust/naked_println.rs | 46 ++++++-------- src/checks/source/rust/no_color.rs | 43 +++++-------- src/checks/source/rust/no_pager.rs | 55 ++++++---------- src/checks/source/rust/output_clamping.rs | 55 ++++++---------- src/checks/source/rust/process_exit.rs | 46 ++++++-------- src/checks/source/rust/structured_output.rs | 51 ++++++--------- src/checks/source/rust/timeout_flag.rs | 55 ++++++---------- src/checks/source/rust/try_parse.rs | 47 ++++++-------- src/checks/source/rust/tty_detection.rs | 69 ++++++++------------- src/checks/source/rust/unwrap.rs | 43 +++++-------- tests/integration.rs | 32 ++++++++++ 17 files changed, 323 insertions(+), 455 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 84dfcee..39edfcc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -64,6 +64,24 @@ Key decisions already made: - Feature flag is `tree-sitter-rust`, not `language-rust` - Edition 2024, dual MIT/Apache-2.0 license +## Source Check Convention + +Every source check follows this structure: + +- **Struct** implements `Check` trait with `id()`, `group()`, `layer()`, `applicable()`, `run()` +- **`check_x()` helper** takes `(source: &str, file: &str)` and returns `CheckStatus` (not `CheckResult`) — this is the + unit-testable core +- **`run()` is the sole `CheckResult` constructor** — uses `self.id()`, `self.group()`, `self.layer()` to build the + result. Never hardcode ID/group/layer string literals in `check_x()` or anywhere outside `run()` +- **Tests call `check_x()`** and match on `CheckStatus` directly, not `result.status` + +This prevents ID triplication (the same string literal in `id()`, `run()`, and `check_x()`) and ensures the `Check` +trait is the single source of truth for check metadata. + +For cross-language pattern helpers, use `source::has_pattern_in()` / `source::find_pattern_matches_in()` / +`source::has_string_literal_in()` with a `Language` parameter — do not write private per-language helpers in individual +check files. + ## Dogfooding Safety Behavioral checks spawn the target binary as a child process. When dogfooding (`anc check .`), the target IS diff --git a/src/checks/source/rust/env_flags.rs b/src/checks/source/rust/env_flags.rs index 7269624..cfc2bf4 100644 --- a/src/checks/source/rust/env_flags.rs +++ b/src/checks/source/rust/env_flags.rs @@ -44,8 +44,7 @@ impl Check for EnvFlagsCheck { for (path, parsed_file) in parsed.iter() { let file_str = path.display().to_string(); - let result = check_env_flags(&parsed_file.source, &file_str); - match &result.status { + match &check_env_flags(&parsed_file.source, &file_str) { CheckStatus::Warn(evidence) => { has_clap_attrs = true; all_warn_evidence.push(evidence.clone()); @@ -69,10 +68,10 @@ impl Check for EnvFlagsCheck { }; Ok(CheckResult { - id: "p6-env-flags".to_string(), + id: self.id().to_string(), label: "Agentic flags have env backing".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } @@ -81,32 +80,20 @@ impl Check for EnvFlagsCheck { /// Check a single source string for agentic flags missing `env = "..."`. /// /// Kept public(crate) for unit testing with inline source strings. -pub(crate) fn check_env_flags(source: &str, file: &str) -> CheckResult { +pub(crate) fn check_env_flags(source: &str, file: &str) -> CheckStatus { let missing = find_agentic_flags_missing_env(source, file); // If we found no agentic arg attributes at all, check if there are *any* arg attributes. if missing.found_agentic == 0 { let has_any_arg = has_arg_attributes(source); if !has_any_arg { - return CheckResult { - id: "p6-env-flags".to_string(), - label: "Agentic flags have env backing".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, - status: CheckStatus::Skip("No clap #[arg(...)] attributes found".to_string()), - }; + return CheckStatus::Skip("No clap #[arg(...)] attributes found".to_string()); } // Has arg attributes but none are agentic — that's still a skip for this file - return CheckResult { - id: "p6-env-flags".to_string(), - label: "Agentic flags have env backing".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, - status: CheckStatus::Skip("No agentic flags found".to_string()), - }; + return CheckStatus::Skip("No agentic flags found".to_string()); } - let status = if missing.locations.is_empty() { + if missing.locations.is_empty() { CheckStatus::Pass } else { let evidence = missing @@ -121,14 +108,6 @@ pub(crate) fn check_env_flags(source: &str, file: &str) -> CheckResult { .collect::>() .join("\n"); CheckStatus::Warn(evidence) - }; - - CheckResult { - id: "p6-env-flags".to_string(), - label: "Agentic flags have env backing".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, - status, } } @@ -215,8 +194,8 @@ struct Cli { verbose: bool, } "#; - let result = check_env_flags(source, "src/cli.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_env_flags(source, "src/cli.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -231,9 +210,9 @@ struct Cli { quiet: bool, } "#; - let result = check_env_flags(source, "src/cli.rs"); - assert!(matches!(result.status, CheckStatus::Warn(_))); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_env_flags(source, "src/cli.rs"); + assert!(matches!(status, CheckStatus::Warn(_))); + if let CheckStatus::Warn(evidence) = &status { assert!(evidence.contains("output")); assert!(evidence.contains("quiet")); assert!(evidence.contains("missing env")); @@ -247,8 +226,8 @@ fn main() { println!("Hello, world!"); } "#; - let result = check_env_flags(source, "src/main.rs"); - assert!(matches!(result.status, CheckStatus::Skip(_))); + let status = check_env_flags(source, "src/main.rs"); + assert!(matches!(status, CheckStatus::Skip(_))); } #[test] @@ -263,8 +242,8 @@ struct Cli { config: Option, } "#; - let result = check_env_flags(source, "src/cli.rs"); - assert!(matches!(result.status, CheckStatus::Skip(_))); + let status = check_env_flags(source, "src/cli.rs"); + assert!(matches!(status, CheckStatus::Skip(_))); } #[test] @@ -279,9 +258,9 @@ struct Cli { verbose: bool, } "#; - let result = check_env_flags(source, "src/cli.rs"); - assert!(matches!(result.status, CheckStatus::Warn(_))); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_env_flags(source, "src/cli.rs"); + assert!(matches!(status, CheckStatus::Warn(_))); + if let CheckStatus::Warn(evidence) = &status { assert!(evidence.contains("verbose")); assert!(!evidence.contains("output")); } @@ -296,8 +275,8 @@ struct Cli { timeout: Option, } "#; - let result = check_env_flags(source, "src/cli.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_env_flags(source, "src/cli.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] diff --git a/src/checks/source/rust/error_types.rs b/src/checks/source/rust/error_types.rs index 1ec6d38..8221f0f 100644 --- a/src/checks/source/rust/error_types.rs +++ b/src/checks/source/rust/error_types.rs @@ -55,10 +55,10 @@ impl Check for ErrorTypesCheck { }; Ok(CheckResult { - id: "p4-error-types".to_string(), + id: self.id().to_string(), label: "Structured error types".to_string(), - group: CheckGroup::P4, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } diff --git a/src/checks/source/rust/exit_codes.rs b/src/checks/source/rust/exit_codes.rs index 01535c0..abf405c 100644 --- a/src/checks/source/rust/exit_codes.rs +++ b/src/checks/source/rust/exit_codes.rs @@ -40,8 +40,7 @@ impl Check for ExitCodesCheck { for (path, parsed_file) in parsed.iter() { let file_str = path.display().to_string(); - let result = check_exit_codes(&parsed_file.source, &file_str); - if let CheckStatus::Warn(evidence) = result.status { + if let CheckStatus::Warn(evidence) = check_exit_codes(&parsed_file.source, &file_str) { all_evidence.push(evidence); } } @@ -53,10 +52,10 @@ impl Check for ExitCodesCheck { }; Ok(CheckResult { - id: "p4-exit-codes".to_string(), + id: self.id().to_string(), label: "Exit codes use named constants".to_string(), - group: CheckGroup::P4, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } @@ -65,10 +64,10 @@ impl Check for ExitCodesCheck { /// Check a single source string for raw integer exit codes. /// /// Kept public(crate) for unit testing with inline source strings. -pub(crate) fn check_exit_codes(source: &str, file: &str) -> CheckResult { +pub(crate) fn check_exit_codes(source: &str, file: &str) -> CheckStatus { let violations = find_raw_exit_codes(source, file); - let status = if violations.is_empty() { + if violations.is_empty() { CheckStatus::Pass } else { let evidence = violations @@ -77,14 +76,6 @@ pub(crate) fn check_exit_codes(source: &str, file: &str) -> CheckResult { .collect::>() .join("\n"); CheckStatus::Warn(evidence) - }; - - CheckResult { - id: "p4-exit-codes".to_string(), - label: "Exit codes use named constants".to_string(), - group: CheckGroup::P4, - layer: CheckLayer::Source, - status, } } @@ -144,8 +135,8 @@ fn main() -> anyhow::Result<()> { Ok(()) } "#; - let result = check_exit_codes(source, "src/main.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_exit_codes(source, "src/main.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -163,8 +154,8 @@ fn main() { process::exit(EXIT_SUCCESS); } "#; - let result = check_exit_codes(source, "src/main.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_exit_codes(source, "src/main.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -176,9 +167,9 @@ fn main() { process::exit(1); } "#; - let result = check_exit_codes(source, "src/main.rs"); - assert!(matches!(result.status, CheckStatus::Warn(_))); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_exit_codes(source, "src/main.rs"); + assert!(matches!(status, CheckStatus::Warn(_))); + if let CheckStatus::Warn(evidence) = &status { assert!(evidence.contains("process::exit(1)")); assert!(evidence.contains("src/main.rs")); } @@ -191,9 +182,9 @@ fn bail() { std::process::exit(2); } "#; - let result = check_exit_codes(source, "src/lib.rs"); - assert!(matches!(result.status, CheckStatus::Warn(_))); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_exit_codes(source, "src/lib.rs"); + assert!(matches!(status, CheckStatus::Warn(_))); + if let CheckStatus::Warn(evidence) = &status { assert!(evidence.contains("std::process::exit(2)")); } } @@ -210,8 +201,8 @@ fn main() { process::exit(0); } "#; - let result = check_exit_codes(source, "src/main.rs"); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_exit_codes(source, "src/main.rs"); + if let CheckStatus::Warn(evidence) = &status { assert_eq!(evidence.lines().count(), 2); } else { panic!("Expected Warn"); @@ -227,8 +218,8 @@ fn main() { process::exit(code); } "#; - let result = check_exit_codes(source, "src/main.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_exit_codes(source, "src/main.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] diff --git a/src/checks/source/rust/global_flags.rs b/src/checks/source/rust/global_flags.rs index fa5c48f..d6f103f 100644 --- a/src/checks/source/rust/global_flags.rs +++ b/src/checks/source/rust/global_flags.rs @@ -47,8 +47,7 @@ impl Check for GlobalFlagsCheck { for (path, parsed_file) in parsed.iter() { let file_str = path.display().to_string(); - let result = check_global_flags(&parsed_file.source, &file_str); - match &result.status { + match &check_global_flags(&parsed_file.source, &file_str) { CheckStatus::Warn(evidence) => { has_subcommands = true; all_warn_evidence.push(evidence.clone()); @@ -73,10 +72,10 @@ impl Check for GlobalFlagsCheck { }; Ok(CheckResult { - id: "p6-global-flags".to_string(), + id: self.id().to_string(), label: "Agentic flags are global".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } @@ -85,25 +84,19 @@ impl Check for GlobalFlagsCheck { /// Check a single source string for non-global agentic flags. /// /// Kept public(crate) for unit testing with inline source strings. -pub(crate) fn check_global_flags(source: &str, file: &str) -> CheckResult { +pub(crate) fn check_global_flags(source: &str, file: &str) -> CheckStatus { // Step 1: Detect if the project uses clap subcommands. let has_subcommands = has_pattern(source, "Subcommand") || has_pattern(source, "#[command(subcommand)]"); if !has_subcommands { - return CheckResult { - id: "p6-global-flags".to_string(), - label: "Agentic flags are global".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, - status: CheckStatus::Skip("No subcommands detected".to_string()), - }; + return CheckStatus::Skip("No subcommands detected".to_string()); } // Step 2: Find all clap field attributes and check agentic flags. let missing = find_non_global_agentic_flags(source, file); - let status = if missing.is_empty() { + if missing.is_empty() { CheckStatus::Pass } else { let evidence = missing @@ -117,14 +110,6 @@ pub(crate) fn check_global_flags(source: &str, file: &str) -> CheckResult { .collect::>() .join("\n"); CheckStatus::Warn(evidence) - }; - - CheckResult { - id: "p6-global-flags".to_string(), - label: "Agentic flags are global".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, - status, } } @@ -175,8 +160,8 @@ struct Cli { output: Option, } "#; - let result = check_global_flags(source, "src/cli.rs"); - assert!(matches!(result.status, CheckStatus::Skip(_))); + let status = check_global_flags(source, "src/cli.rs"); + assert!(matches!(status, CheckStatus::Skip(_))); } #[test] @@ -199,8 +184,8 @@ enum Commands { Check, } "#; - let result = check_global_flags(source, "src/cli.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_global_flags(source, "src/cli.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -223,9 +208,9 @@ enum Commands { Check, } "#; - let result = check_global_flags(source, "src/cli.rs"); - assert!(matches!(result.status, CheckStatus::Warn(_))); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_global_flags(source, "src/cli.rs"); + assert!(matches!(status, CheckStatus::Warn(_))); + if let CheckStatus::Warn(evidence) = &status { assert!(evidence.contains("output")); assert!(evidence.contains("quiet")); } @@ -248,8 +233,8 @@ enum Commands { Check, } "#; - let result = check_global_flags(source, "src/cli.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_global_flags(source, "src/cli.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] diff --git a/src/checks/source/rust/headless_auth.rs b/src/checks/source/rust/headless_auth.rs index 3c1f850..4a50d18 100644 --- a/src/checks/source/rust/headless_auth.rs +++ b/src/checks/source/rust/headless_auth.rs @@ -52,8 +52,7 @@ impl Check for HeadlessAuthCheck { let mut has_headless_flag = false; for (_path, parsed_file) in parsed.iter() { - let result = check_headless_auth(&parsed_file.source); - match &result.status { + match &check_headless_auth(&parsed_file.source) { CheckStatus::Skip(_) => { // No auth code in this file } @@ -81,10 +80,10 @@ impl Check for HeadlessAuthCheck { }; Ok(CheckResult { - id: "p1-headless-auth".to_string(), + id: self.id().to_string(), label: "Headless auth supported".to_string(), - group: CheckGroup::P1, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } @@ -94,17 +93,11 @@ impl Check for HeadlessAuthCheck { /// /// Searches function definitions via ast-grep to find auth-related identifiers. /// This avoids false positives from comments, string literals, and constant arrays. -pub(crate) fn check_headless_auth(source: &str) -> CheckResult { +pub(crate) fn check_headless_auth(source: &str) -> CheckStatus { let has_auth = has_auth_functions(source); if !has_auth { - return CheckResult { - id: "p1-headless-auth".to_string(), - label: "Headless auth supported".to_string(), - group: CheckGroup::P1, - layer: CheckLayer::Source, - status: CheckStatus::Skip("no auth code found".to_string()), - }; + return CheckStatus::Skip("no auth code found".to_string()); } // Check for --no-browser or --headless flag in clap arg definitions @@ -113,7 +106,7 @@ pub(crate) fn check_headless_auth(source: &str) -> CheckResult { || source.contains("no_browser") || source.contains("headless")); - let status = if has_flag { + if has_flag { CheckStatus::Pass } else { CheckStatus::Warn( @@ -121,14 +114,6 @@ pub(crate) fn check_headless_auth(source: &str) -> CheckResult { Agents need a way to authenticate without a browser." .to_string(), ) - }; - - CheckResult { - id: "p1-headless-auth".to_string(), - label: "Headless auth supported".to_string(), - group: CheckGroup::P1, - layer: CheckLayer::Source, - status, } } @@ -172,8 +157,8 @@ fn main() { println!("Hello, world!"); } "#; - let result = check_headless_auth(source); - assert!(matches!(result.status, CheckStatus::Skip(_))); + let status = check_headless_auth(source); + assert!(matches!(status, CheckStatus::Skip(_))); } #[test] @@ -191,8 +176,8 @@ struct Cli { no_browser: bool, } "#; - let result = check_headless_auth(source); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_headless_auth(source); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -210,8 +195,8 @@ struct Cli { headless: bool, } "#; - let result = check_headless_auth(source); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_headless_auth(source); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -229,9 +214,9 @@ struct Cli { verbose: bool, } "#; - let result = check_headless_auth(source); - assert!(matches!(result.status, CheckStatus::Warn(_))); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_headless_auth(source); + assert!(matches!(status, CheckStatus::Warn(_))); + if let CheckStatus::Warn(evidence) = &status { assert!(evidence.contains("no --no-browser")); } } @@ -246,8 +231,8 @@ fn parse_token(s: &str) -> Token { Token::new(s) } "#; - let result = check_headless_auth(source); - assert!(matches!(result.status, CheckStatus::Skip(_))); + let status = check_headless_auth(source); + assert!(matches!(status, CheckStatus::Skip(_))); } #[test] diff --git a/src/checks/source/rust/naked_println.rs b/src/checks/source/rust/naked_println.rs index 20c4bf0..362cb46 100644 --- a/src/checks/source/rust/naked_println.rs +++ b/src/checks/source/rust/naked_println.rs @@ -46,8 +46,8 @@ impl Check for NakedPrintlnCheck { continue; } - let result = check_naked_println(&parsed_file.source, &file_str); - if let CheckStatus::Warn(evidence) = result.status { + if let CheckStatus::Warn(evidence) = check_naked_println(&parsed_file.source, &file_str) + { all_evidence.push(evidence); } } @@ -59,10 +59,10 @@ impl Check for NakedPrintlnCheck { }; Ok(CheckResult { - id: "p7-naked-println".to_string(), + id: self.id().to_string(), label: "No naked println!/print! outside output modules".to_string(), - group: CheckGroup::P7, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } @@ -71,7 +71,7 @@ impl Check for NakedPrintlnCheck { /// Check a single source string for `println!` and `print!` calls. /// /// Kept public(crate) for unit testing with inline source strings. -pub(crate) fn check_naked_println(source: &str, file: &str) -> CheckResult { +pub(crate) fn check_naked_println(source: &str, file: &str) -> CheckStatus { let mut println_matches = find_pattern_matches(source, PRINTLN_PATTERN); let mut print_matches = find_pattern_matches(source, PRINT_PATTERN); @@ -85,7 +85,7 @@ pub(crate) fn check_naked_println(source: &str, file: &str) -> CheckResult { let mut all_matches = println_matches; all_matches.append(&mut print_matches); - let status = if all_matches.is_empty() { + if all_matches.is_empty() { CheckStatus::Pass } else { let evidence = all_matches @@ -94,14 +94,6 @@ pub(crate) fn check_naked_println(source: &str, file: &str) -> CheckResult { .collect::>() .join("\n"); CheckStatus::Warn(evidence) - }; - - CheckResult { - id: "p7-naked-println".to_string(), - label: "No naked println!/print! outside output modules".to_string(), - group: CheckGroup::P7, - layer: CheckLayer::Source, - status, } } @@ -117,8 +109,8 @@ fn main() -> anyhow::Result<()> { Ok(()) } "#; - let result = check_naked_println(source, "src/main.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_naked_println(source, "src/main.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -128,9 +120,9 @@ fn main() { println!("Hello, world!"); } "#; - let result = check_naked_println(source, "src/main.rs"); - assert!(matches!(result.status, CheckStatus::Warn(_))); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_naked_println(source, "src/main.rs"); + assert!(matches!(status, CheckStatus::Warn(_))); + if let CheckStatus::Warn(evidence) = &status { assert!(evidence.contains("println!")); assert!(evidence.contains("src/main.rs")); } @@ -143,9 +135,9 @@ fn render() { print!("loading..."); } "#; - let result = check_naked_println(source, "src/render.rs"); - assert!(matches!(result.status, CheckStatus::Warn(_))); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_naked_println(source, "src/render.rs"); + assert!(matches!(status, CheckStatus::Warn(_))); + if let CheckStatus::Warn(evidence) = &status { assert!(evidence.contains("print!")); assert!(evidence.contains("src/render.rs")); } @@ -159,8 +151,8 @@ fn main() { eprintln!("error: {}", msg); } "#; - let result = check_naked_println(source, "src/main.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_naked_println(source, "src/main.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -172,8 +164,8 @@ fn main() { print!("three"); } "#; - let result = check_naked_println(source, "src/lib.rs"); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_naked_println(source, "src/lib.rs"); + if let CheckStatus::Warn(evidence) = &status { assert_eq!(evidence.lines().count(), 3); } else { panic!("Expected Warn"); diff --git a/src/checks/source/rust/no_color.rs b/src/checks/source/rust/no_color.rs index bf9ec8a..55f3b73 100644 --- a/src/checks/source/rust/no_color.rs +++ b/src/checks/source/rust/no_color.rs @@ -41,8 +41,7 @@ impl Check for NoColorSourceCheck { let mut found_any = false; for (_path, parsed_file) in parsed.iter() { - let result = check_no_color(&parsed_file.source, ""); - if matches!(result.status, CheckStatus::Pass) { + if matches!(check_no_color(&parsed_file.source, ""), CheckStatus::Pass) { found_any = true; break; } @@ -59,10 +58,10 @@ impl Check for NoColorSourceCheck { }; Ok(CheckResult { - id: "p6-no-color".to_string(), + id: self.id().to_string(), label: "Respects NO_COLOR".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } @@ -71,7 +70,7 @@ impl Check for NoColorSourceCheck { /// Check a single source string for NO_COLOR references. /// /// Kept public(crate) for unit testing with inline source strings. -pub(crate) fn check_no_color(source: &str, file: &str) -> CheckResult { +pub(crate) fn check_no_color(source: &str, file: &str) -> CheckStatus { let found_env_var = has_pattern(source, r#"std::env::var("NO_COLOR")"#) || has_pattern(source, r#"env::var("NO_COLOR")"#) || has_pattern(source, r#"std::env::var_os("NO_COLOR")"#) @@ -83,21 +82,13 @@ pub(crate) fn check_no_color(source: &str, file: &str) -> CheckResult { || found_clap_env || has_string_literal_in(source, "NO_COLOR", Language::Rust); - let status = if found_any { + if found_any { CheckStatus::Pass } else { CheckStatus::Fail(format!( "{file}: No reference to NO_COLOR found. CLIs must respect the NO_COLOR convention. \ See https://no-color.org/" )) - }; - - CheckResult { - id: "p6-no-color".to_string(), - label: "Respects NO_COLOR".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, - status, } } @@ -129,8 +120,8 @@ fn setup_color() { } } "#; - let result = check_no_color(source, "src/output.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_no_color(source, "src/output.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -144,8 +135,8 @@ fn setup_color() { } } "#; - let result = check_no_color(source, "src/output.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_no_color(source, "src/output.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -157,8 +148,8 @@ struct Cli { no_color: bool, } "#; - let result = check_no_color(source, "src/cli.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_no_color(source, "src/cli.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -168,9 +159,9 @@ fn main() { println!("Hello, world!"); } "#; - let result = check_no_color(source, "src/main.rs"); - assert!(matches!(result.status, CheckStatus::Fail(_))); - if let CheckStatus::Fail(evidence) = &result.status { + let status = check_no_color(source, "src/main.rs"); + assert!(matches!(status, CheckStatus::Fail(_))); + if let CheckStatus::Fail(evidence) = &status { assert!(evidence.contains("NO_COLOR")); assert!(evidence.contains("no-color.org")); } @@ -185,8 +176,8 @@ fn check_color() { std::env::var(COLOR_ENV).ok(); } "#; - let result = check_no_color(source, "src/color.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_no_color(source, "src/color.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] diff --git a/src/checks/source/rust/no_pager.rs b/src/checks/source/rust/no_pager.rs index a8ce19c..185d4f9 100644 --- a/src/checks/source/rust/no_pager.rs +++ b/src/checks/source/rust/no_pager.rs @@ -41,8 +41,7 @@ impl Check for NoPagerCheck { let mut has_no_pager_flag = false; for (_path, parsed_file) in parsed.iter() { - let result = check_no_pager(&parsed_file.source); - match &result.status { + match &check_no_pager(&parsed_file.source) { CheckStatus::Pass => { // Either no pager or has --no-pager flag if source_has_pager_code(&parsed_file.source) { @@ -68,10 +67,10 @@ impl Check for NoPagerCheck { }; Ok(CheckResult { - id: "p6-no-pager".to_string(), + id: self.id().to_string(), label: "No pager blocking agents".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } @@ -93,23 +92,17 @@ fn source_has_pager_code(source: &str) -> bool { } /// Check a single source string for pager code and --no-pager flag. -pub(crate) fn check_no_pager(source: &str) -> CheckResult { +pub(crate) fn check_no_pager(source: &str) -> CheckStatus { let has_pager = source_has_pager_code(source); if !has_pager { - return CheckResult { - id: "p6-no-pager".to_string(), - label: "No pager blocking agents".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, - status: CheckStatus::Pass, - }; + return CheckStatus::Pass; } // Check for --no-pager flag let has_flag = source.contains("no-pager") || source.contains("no_pager"); - let status = if has_flag { + if has_flag { CheckStatus::Pass } else { CheckStatus::Warn( @@ -117,14 +110,6 @@ pub(crate) fn check_no_pager(source: &str) -> CheckResult { Pagers block agents; provide --no-pager to disable." .to_string(), ) - }; - - CheckResult { - id: "p6-no-pager".to_string(), - label: "No pager blocking agents".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, - status, } } @@ -139,8 +124,8 @@ fn main() { println!("Hello, world!"); } "#; - let result = check_no_pager(source); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_no_pager(source); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -154,8 +139,8 @@ fn setup_pager(no_pager: bool) { } } "#; - let result = check_no_pager(source); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_no_pager(source); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -167,9 +152,9 @@ fn setup() { Pager::new().setup(); } "#; - let result = check_no_pager(source); - assert!(matches!(result.status, CheckStatus::Warn(_))); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_no_pager(source); + assert!(matches!(status, CheckStatus::Warn(_))); + if let CheckStatus::Warn(evidence) = &status { assert!(evidence.contains("no --no-pager")); } } @@ -183,8 +168,8 @@ fn show_output(text: &str) { Command::new("less").spawn().expect("spawn less"); } "#; - let result = check_no_pager(source); - assert!(matches!(result.status, CheckStatus::Warn(_))); + let status = check_no_pager(source); + assert!(matches!(status, CheckStatus::Warn(_))); } #[test] @@ -196,8 +181,8 @@ fn show_output(text: &str) { Command::new("more").spawn().expect("spawn more"); } "#; - let result = check_no_pager(source); - assert!(matches!(result.status, CheckStatus::Warn(_))); + let status = check_no_pager(source); + assert!(matches!(status, CheckStatus::Warn(_))); } #[test] @@ -213,8 +198,8 @@ fn show_output(text: &str, no_pager: bool) { } } "#; - let result = check_no_pager(source); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_no_pager(source); + assert_eq!(status, CheckStatus::Pass); } #[test] diff --git a/src/checks/source/rust/output_clamping.rs b/src/checks/source/rust/output_clamping.rs index 8dca77a..0d7a85d 100644 --- a/src/checks/source/rust/output_clamping.rs +++ b/src/checks/source/rust/output_clamping.rs @@ -52,8 +52,7 @@ impl Check for OutputClampingCheck { for (path, parsed_file) in parsed.iter() { let file_str = path.display().to_string(); - let result = check_output_clamping(&parsed_file.source, &file_str); - match &result.status { + match &check_output_clamping(&parsed_file.source, &file_str) { CheckStatus::Warn(evidence) => { has_list_patterns = true; list_evidence.push(evidence.clone()); @@ -78,10 +77,10 @@ impl Check for OutputClampingCheck { }; Ok(CheckResult { - id: "p7-output-clamping".to_string(), + id: self.id().to_string(), label: "List output is clamped".to_string(), - group: CheckGroup::P7, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } @@ -90,7 +89,7 @@ impl Check for OutputClampingCheck { /// Check a single source string for list patterns and clamping. /// /// Kept public(crate) for unit testing with inline source strings. -pub(crate) fn check_output_clamping(source: &str, file: &str) -> CheckResult { +pub(crate) fn check_output_clamping(source: &str, file: &str) -> CheckStatus { // Step 1: Look for list/iteration patterns let mut list_locations = Vec::new(); @@ -103,20 +102,14 @@ pub(crate) fn check_output_clamping(source: &str, file: &str) -> CheckResult { } if list_locations.is_empty() { - return CheckResult { - id: "p7-output-clamping".to_string(), - label: "List output is clamped".to_string(), - group: CheckGroup::P7, - layer: CheckLayer::Source, - status: CheckStatus::Skip("No list output patterns detected".to_string()), - }; + return CheckStatus::Skip("No list output patterns detected".to_string()); } // Step 2: Check if clamping exists anywhere in the same source let has_clamping = CLAMP_PATTERNS.iter().any(|pat| has_pattern(source, pat)) || CLAMP_STRINGS.iter().any(|s| source.contains(s)); - let status = if has_clamping { + if has_clamping { CheckStatus::Pass } else { let evidence = list_locations @@ -132,14 +125,6 @@ pub(crate) fn check_output_clamping(source: &str, file: &str) -> CheckResult { .collect::>() .join("\n"); CheckStatus::Warn(evidence) - }; - - CheckResult { - id: "p7-output-clamping".to_string(), - label: "List output is clamped".to_string(), - group: CheckGroup::P7, - layer: CheckLayer::Source, - status, } } @@ -169,8 +154,8 @@ fn main() { eprintln!("result: {x}"); } "#; - let result = check_output_clamping(source, "src/main.rs"); - assert!(matches!(result.status, CheckStatus::Skip(_))); + let status = check_output_clamping(source, "src/main.rs"); + assert!(matches!(status, CheckStatus::Skip(_))); } #[test] @@ -180,9 +165,9 @@ fn list_items(items: &[Item]) -> Vec { items.iter().map(|i| i.name.clone()).collect::>() } "#; - let result = check_output_clamping(source, "src/list.rs"); - assert!(matches!(result.status, CheckStatus::Warn(_))); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_output_clamping(source, "src/list.rs"); + assert!(matches!(status, CheckStatus::Warn(_))); + if let CheckStatus::Warn(evidence) = &status { assert!(evidence.contains("list pattern without clamping")); } } @@ -194,8 +179,8 @@ fn list_items(items: &[Item]) -> Vec { items.iter().take(100).map(|i| i.name.clone()).collect::>() } "#; - let result = check_output_clamping(source, "src/list.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_output_clamping(source, "src/list.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -209,8 +194,8 @@ fn parse_args() { let limit = matches.get_one::("--limit"); } "#; - let result = check_output_clamping(source, "src/list.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_output_clamping(source, "src/list.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -222,8 +207,8 @@ fn print_all(items: Vec) { } } "#; - let result = check_output_clamping(source, "src/printer.rs"); - assert!(matches!(result.status, CheckStatus::Warn(_))); + let status = check_output_clamping(source, "src/printer.rs"); + assert!(matches!(status, CheckStatus::Warn(_))); } #[test] @@ -234,8 +219,8 @@ fn list_results(items: &[Item]) -> Vec { items.iter().map(|i| i.name.clone()).collect::>() } "#; - let result = check_output_clamping(source, "src/list.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_output_clamping(source, "src/list.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] diff --git a/src/checks/source/rust/process_exit.rs b/src/checks/source/rust/process_exit.rs index 888a99d..b4ece48 100644 --- a/src/checks/source/rust/process_exit.rs +++ b/src/checks/source/rust/process_exit.rs @@ -41,8 +41,8 @@ impl Check for ProcessExitCheck { continue; } - let result = check_process_exit(&parsed_file.source, &file_str); - if let CheckStatus::Fail(evidence) = result.status { + if let CheckStatus::Fail(evidence) = check_process_exit(&parsed_file.source, &file_str) + { all_evidence.push(evidence); } } @@ -54,10 +54,10 @@ impl Check for ProcessExitCheck { }; Ok(CheckResult { - id: "p4-process-exit".to_string(), + id: self.id().to_string(), label: "No process::exit outside main".to_string(), - group: CheckGroup::P4, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } @@ -66,7 +66,7 @@ impl Check for ProcessExitCheck { /// Check a single source string for `process::exit()` calls. /// /// Kept public(crate) for unit testing with inline source strings. -pub(crate) fn check_process_exit(source: &str, file: &str) -> CheckResult { +pub(crate) fn check_process_exit(source: &str, file: &str) -> CheckStatus { let mut all_matches = Vec::new(); for pattern_str in PATTERNS { @@ -77,7 +77,7 @@ pub(crate) fn check_process_exit(source: &str, file: &str) -> CheckResult { all_matches.extend(matches); } - let status = if all_matches.is_empty() { + if all_matches.is_empty() { CheckStatus::Pass } else { let evidence = all_matches @@ -86,14 +86,6 @@ pub(crate) fn check_process_exit(source: &str, file: &str) -> CheckResult { .collect::>() .join("\n"); CheckStatus::Fail(evidence) - }; - - CheckResult { - id: "p4-process-exit".to_string(), - label: "No process::exit outside main".to_string(), - group: CheckGroup::P4, - layer: CheckLayer::Source, - status, } } @@ -108,8 +100,8 @@ fn handle_error(e: Error) -> Result<()> { Err(e.into()) } "#; - let result = check_process_exit(source, "src/lib.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_process_exit(source, "src/lib.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -122,9 +114,9 @@ fn bail(msg: &str) { process::exit(1); } "#; - let result = check_process_exit(source, "src/lib.rs"); - assert!(matches!(result.status, CheckStatus::Fail(_))); - if let CheckStatus::Fail(evidence) = &result.status { + let status = check_process_exit(source, "src/lib.rs"); + assert!(matches!(status, CheckStatus::Fail(_))); + if let CheckStatus::Fail(evidence) = &status { assert!(evidence.contains("process::exit")); assert!(evidence.contains("src/lib.rs")); } @@ -137,9 +129,9 @@ fn bail() { std::process::exit(1); } "#; - let result = check_process_exit(source, "src/util.rs"); - assert!(matches!(result.status, CheckStatus::Fail(_))); - if let CheckStatus::Fail(evidence) = &result.status { + let status = check_process_exit(source, "src/util.rs"); + assert!(matches!(status, CheckStatus::Fail(_))); + if let CheckStatus::Fail(evidence) = &status { assert!(evidence.contains("std::process::exit")); } } @@ -156,8 +148,8 @@ fn main() { } "#; // Inner function always reports — it's run() that filters main.rs - let result = check_process_exit(source, "src/main.rs"); - assert!(matches!(result.status, CheckStatus::Fail(_))); + let status = check_process_exit(source, "src/main.rs"); + assert!(matches!(status, CheckStatus::Fail(_))); } #[test] @@ -173,8 +165,8 @@ fn bail_b() { process::exit(2); } "#; - let result = check_process_exit(source, "src/commands.rs"); - if let CheckStatus::Fail(evidence) = &result.status { + let status = check_process_exit(source, "src/commands.rs"); + if let CheckStatus::Fail(evidence) = &status { assert_eq!(evidence.lines().count(), 2); } else { panic!("Expected Fail"); diff --git a/src/checks/source/rust/structured_output.rs b/src/checks/source/rust/structured_output.rs index 6a0bfc9..ffc38db 100644 --- a/src/checks/source/rust/structured_output.rs +++ b/src/checks/source/rust/structured_output.rs @@ -37,8 +37,7 @@ impl Check for StructuredOutputCheck { let mut has_output_format = false; for (_path, parsed_file) in parsed.iter() { - let result = check_structured_output(&parsed_file.source); - match &result.status { + match &check_structured_output(&parsed_file.source) { CheckStatus::Skip(_) => { // No clap in this file } @@ -66,27 +65,21 @@ impl Check for StructuredOutputCheck { }; Ok(CheckResult { - id: "p2-structured-output".to_string(), + id: self.id().to_string(), label: "Structured output type exists".to_string(), - group: CheckGroup::P2, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } } /// Check a single source string for OutputFormat enum. -pub(crate) fn check_structured_output(source: &str) -> CheckResult { +pub(crate) fn check_structured_output(source: &str) -> CheckStatus { let has_clap = source.contains("clap") || source.contains("#[derive(Parser)]"); if !has_clap { - return CheckResult { - id: "p2-structured-output".to_string(), - label: "Structured output type exists".to_string(), - group: CheckGroup::P2, - layer: CheckLayer::Source, - status: CheckStatus::Skip("no clap dependency detected".to_string()), - }; + return CheckStatus::Skip("no clap dependency detected".to_string()); } let has_output_format = has_pattern(source, "enum OutputFormat { $$$BODY }") @@ -94,7 +87,7 @@ pub(crate) fn check_structured_output(source: &str) -> CheckResult { || has_pattern(source, "enum Format { $$$BODY }") || has_pattern(source, "pub enum Format { $$$BODY }"); - let status = if has_output_format { + if has_output_format { CheckStatus::Pass } else { CheckStatus::Warn( @@ -102,14 +95,6 @@ pub(crate) fn check_structured_output(source: &str) -> CheckResult { structured output (e.g., --output json) for agent consumption." .to_string(), ) - }; - - CheckResult { - id: "p2-structured-output".to_string(), - label: "Structured output type exists".to_string(), - group: CheckGroup::P2, - layer: CheckLayer::Source, - status, } } @@ -124,8 +109,8 @@ fn main() { println!("Hello, world!"); } "#; - let result = check_structured_output(source); - assert!(matches!(result.status, CheckStatus::Skip(_))); + let status = check_structured_output(source); + assert!(matches!(status, CheckStatus::Skip(_))); } #[test] @@ -146,8 +131,8 @@ struct Cli { output: OutputFormat, } "#; - let result = check_structured_output(source); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_structured_output(source); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -166,8 +151,8 @@ struct Cli { format: Format, } "#; - let result = check_structured_output(source); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_structured_output(source); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -181,9 +166,9 @@ struct Cli { verbose: bool, } "#; - let result = check_structured_output(source); - assert!(matches!(result.status, CheckStatus::Warn(_))); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_structured_output(source); + assert!(matches!(status, CheckStatus::Warn(_))); + if let CheckStatus::Warn(evidence) = &status { assert!(evidence.contains("OutputFormat")); } } @@ -198,9 +183,9 @@ struct Cli { name: String, } "#; - let result = check_structured_output(source); + let status = check_structured_output(source); // Has clap (via derive(Parser)) but no output format - assert!(matches!(result.status, CheckStatus::Warn(_))); + assert!(matches!(status, CheckStatus::Warn(_))); } #[test] diff --git a/src/checks/source/rust/timeout_flag.rs b/src/checks/source/rust/timeout_flag.rs index 6304e93..dea6285 100644 --- a/src/checks/source/rust/timeout_flag.rs +++ b/src/checks/source/rust/timeout_flag.rs @@ -40,8 +40,7 @@ impl Check for TimeoutFlagCheck { let mut has_timeout_flag = false; for (_path, parsed_file) in parsed.iter() { - let result = check_timeout_flag(&parsed_file.source); - match &result.status { + match &check_timeout_flag(&parsed_file.source) { CheckStatus::Skip(_) => { // No network code in this file } @@ -69,27 +68,21 @@ impl Check for TimeoutFlagCheck { }; Ok(CheckResult { - id: "p6-timeout".to_string(), + id: self.id().to_string(), label: "Timeout flag for network ops".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } } /// Check a single source string for network code and timeout flag. -pub(crate) fn check_timeout_flag(source: &str) -> CheckResult { +pub(crate) fn check_timeout_flag(source: &str) -> CheckStatus { let has_network = NETWORK_INDICATORS.iter().any(|ind| source.contains(ind)); if !has_network { - return CheckResult { - id: "p6-timeout".to_string(), - label: "Timeout flag for network ops".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, - status: CheckStatus::Skip("no network code detected".to_string()), - }; + return CheckStatus::Skip("no network code detected".to_string()); } // Check for --timeout flag in source @@ -99,7 +92,7 @@ pub(crate) fn check_timeout_flag(source: &str) -> CheckResult { || source.contains("long = \"timeout\"") || source.contains("long(\"timeout\")")); - let status = if has_flag { + if has_flag { CheckStatus::Pass } else { CheckStatus::Warn( @@ -107,14 +100,6 @@ pub(crate) fn check_timeout_flag(source: &str) -> CheckResult { Agents need to bound execution time for network operations." .to_string(), ) - }; - - CheckResult { - id: "p6-timeout".to_string(), - label: "Timeout flag for network ops".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, - status, } } @@ -129,8 +114,8 @@ fn main() { println!("Hello, world!"); } "#; - let result = check_timeout_flag(source); - assert!(matches!(result.status, CheckStatus::Skip(_))); + let status = check_timeout_flag(source); + assert!(matches!(status, CheckStatus::Skip(_))); } #[test] @@ -150,8 +135,8 @@ fn fetch(url: &str, timeout: u64) { // --timeout used here } "#; - let result = check_timeout_flag(source); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_timeout_flag(source); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -164,9 +149,9 @@ fn fetch(url: &str) { let resp = client.get(url).send(); } "#; - let result = check_timeout_flag(source); - assert!(matches!(result.status, CheckStatus::Warn(_))); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_timeout_flag(source); + assert!(matches!(status, CheckStatus::Warn(_))); + if let CheckStatus::Warn(evidence) = &status { assert!(evidence.contains("no --timeout")); } } @@ -180,8 +165,8 @@ async fn fetch() { let client = Client::new(); } "#; - let result = check_timeout_flag(source); - assert!(matches!(result.status, CheckStatus::Warn(_))); + let status = check_timeout_flag(source); + assert!(matches!(status, CheckStatus::Warn(_))); } #[test] @@ -191,8 +176,8 @@ fn fetch(url: &str) -> String { ureq::get(url).call().into_string() } "#; - let result = check_timeout_flag(source); - assert!(matches!(result.status, CheckStatus::Warn(_))); + let status = check_timeout_flag(source); + assert!(matches!(status, CheckStatus::Warn(_))); } #[test] @@ -208,8 +193,8 @@ struct Cli { timeout: u64, } "#; - let result = check_timeout_flag(source); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_timeout_flag(source); + assert_eq!(status, CheckStatus::Pass); } #[test] diff --git a/src/checks/source/rust/try_parse.rs b/src/checks/source/rust/try_parse.rs index f69cc11..55cc3e6 100644 --- a/src/checks/source/rust/try_parse.rs +++ b/src/checks/source/rust/try_parse.rs @@ -39,8 +39,7 @@ impl Check for TryParseCheck { for (path, parsed_file) in parsed.iter() { let file_str = path.display().to_string(); - let result = check_try_parse(&parsed_file.source, &file_str); - if let CheckStatus::Warn(evidence) = result.status { + if let CheckStatus::Warn(evidence) = check_try_parse(&parsed_file.source, &file_str) { all_evidence.push(evidence); } } @@ -52,10 +51,10 @@ impl Check for TryParseCheck { }; Ok(CheckResult { - id: "p4-try-parse".to_string(), + id: self.id().to_string(), label: "No .parse().unwrap()".to_string(), - group: CheckGroup::P4, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } @@ -64,7 +63,7 @@ impl Check for TryParseCheck { /// Check a single source string for `.parse().unwrap()` and `.parse().expect()`. /// /// Kept public(crate) for unit testing with inline source strings. -pub(crate) fn check_try_parse(source: &str, file: &str) -> CheckResult { +pub(crate) fn check_try_parse(source: &str, file: &str) -> CheckStatus { let mut all_matches = Vec::new(); for pattern_str in PATTERNS { @@ -75,7 +74,7 @@ pub(crate) fn check_try_parse(source: &str, file: &str) -> CheckResult { all_matches.extend(matches); } - let status = if all_matches.is_empty() { + if all_matches.is_empty() { CheckStatus::Pass } else { let evidence = all_matches @@ -84,14 +83,6 @@ pub(crate) fn check_try_parse(source: &str, file: &str) -> CheckResult { .collect::>() .join("\n"); CheckStatus::Warn(evidence) - }; - - CheckResult { - id: "p4-try-parse".to_string(), - label: "No .parse().unwrap()".to_string(), - group: CheckGroup::P4, - layer: CheckLayer::Source, - status, } } @@ -106,8 +97,8 @@ fn parse_port(s: &str) -> Result { s.parse() } "#; - let result = check_try_parse(source, "src/config.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_try_parse(source, "src/config.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -118,8 +109,8 @@ fn parse_port(s: &str) -> anyhow::Result { Ok(port) } "#; - let result = check_try_parse(source, "src/config.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_try_parse(source, "src/config.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -129,9 +120,9 @@ fn main() { let port: u16 = args[1].parse().unwrap(); } "#; - let result = check_try_parse(source, "src/main.rs"); - assert!(matches!(result.status, CheckStatus::Warn(_))); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_try_parse(source, "src/main.rs"); + assert!(matches!(status, CheckStatus::Warn(_))); + if let CheckStatus::Warn(evidence) = &status { assert!(evidence.contains("parse().unwrap()")); assert!(evidence.contains("src/main.rs")); } @@ -145,8 +136,8 @@ fn main() { let port: u16 = args[1].parse().expect("bad port"); } "#; - let result = check_try_parse(source, "src/main.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_try_parse(source, "src/main.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -158,8 +149,8 @@ fn main() { let timeout: u64 = args[3].parse().expect("bad timeout"); } "#; - let result = check_try_parse(source, "src/main.rs"); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_try_parse(source, "src/main.rs"); + if let CheckStatus::Warn(evidence) = &status { // Only .parse().unwrap() lines are flagged, not .expect() assert_eq!(evidence.lines().count(), 2); } else { @@ -180,8 +171,8 @@ fn parse_port(s: &str) -> u16 { } } "#; - let result = check_try_parse(source, "src/config.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_try_parse(source, "src/config.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] diff --git a/src/checks/source/rust/tty_detection.rs b/src/checks/source/rust/tty_detection.rs index e634f30..64895d6 100644 --- a/src/checks/source/rust/tty_detection.rs +++ b/src/checks/source/rust/tty_detection.rs @@ -55,8 +55,7 @@ impl Check for TtyDetectionCheck { let mut has_tty = false; for (_path, parsed_file) in parsed.iter() { - let result = check_tty_detection(&parsed_file.source); - match &result.status { + match &check_tty_detection(&parsed_file.source) { CheckStatus::Skip(_) => { // No color code in this file } @@ -87,10 +86,10 @@ impl Check for TtyDetectionCheck { }; Ok(CheckResult { - id: "p6-tty-detection".to_string(), + id: self.id().to_string(), label: "TTY detection for color output".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } @@ -108,42 +107,24 @@ fn source_has_tty_detection(source: &str) -> bool { } /// Check a single source string for TTY detection. -pub(crate) fn check_tty_detection(source: &str) -> CheckResult { +pub(crate) fn check_tty_detection(source: &str) -> CheckStatus { let has_tty = source_has_tty_detection(source); if has_tty { - return CheckResult { - id: "p6-tty-detection".to_string(), - label: "TTY detection for color output".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, - status: CheckStatus::Pass, - }; + return CheckStatus::Pass; } let has_color = source_has_color_code(source); if !has_color { - return CheckResult { - id: "p6-tty-detection".to_string(), - label: "TTY detection for color output".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, - status: CheckStatus::Skip("no color/formatting code detected".to_string()), - }; + return CheckStatus::Skip("no color/formatting code detected".to_string()); } - CheckResult { - id: "p6-tty-detection".to_string(), - label: "TTY detection for color output".to_string(), - group: CheckGroup::P6, - layer: CheckLayer::Source, - status: CheckStatus::Warn( - "Color/ANSI code detected but no TTY detection found. \ - Use IsTerminal or is_terminal() to avoid corrupting piped output." - .to_string(), - ), - } + CheckStatus::Warn( + "Color/ANSI code detected but no TTY detection found. \ + Use IsTerminal or is_terminal() to avoid corrupting piped output." + .to_string(), + ) } #[cfg(test)] @@ -157,8 +138,8 @@ fn main() { println!("Hello, world!"); } "#; - let result = check_tty_detection(source); - assert!(matches!(result.status, CheckStatus::Skip(_))); + let status = check_tty_detection(source); + assert!(matches!(status, CheckStatus::Skip(_))); } #[test] @@ -173,8 +154,8 @@ fn setup_color() { } } "#; - let result = check_tty_detection(source); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_tty_detection(source); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -189,8 +170,8 @@ fn setup() { } } "#; - let result = check_tty_detection(source); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_tty_detection(source); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -202,9 +183,9 @@ fn display(msg: &str) { println!("{}", msg.green()); } "#; - let result = check_tty_detection(source); - assert!(matches!(result.status, CheckStatus::Warn(_))); - if let CheckStatus::Warn(evidence) = &result.status { + let status = check_tty_detection(source); + assert!(matches!(status, CheckStatus::Warn(_))); + if let CheckStatus::Warn(evidence) = &status { assert!(evidence.contains("TTY detection")); } } @@ -217,8 +198,8 @@ fn display(msg: &str) { print!("\x1b[32m{msg}\x1b[0m"); } "#; - let result = check_tty_detection(source); - assert!(matches!(result.status, CheckStatus::Warn(_))); + let status = check_tty_detection(source); + assert!(matches!(status, CheckStatus::Warn(_))); } #[test] @@ -234,8 +215,8 @@ fn main() { } } "#; - let result = check_tty_detection(source); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_tty_detection(source); + assert_eq!(status, CheckStatus::Pass); } #[test] diff --git a/src/checks/source/rust/unwrap.rs b/src/checks/source/rust/unwrap.rs index 8fa3a1f..3231c26 100644 --- a/src/checks/source/rust/unwrap.rs +++ b/src/checks/source/rust/unwrap.rs @@ -36,8 +36,7 @@ impl Check for UnwrapCheck { for (path, parsed_file) in parsed.iter() { let file_str = path.display().to_string(); - let result = check_unwrap(&parsed_file.source, &file_str); - if let CheckStatus::Fail(evidence) = result.status { + if let CheckStatus::Fail(evidence) = check_unwrap(&parsed_file.source, &file_str) { all_evidence.push(evidence); } } @@ -49,10 +48,10 @@ impl Check for UnwrapCheck { }; Ok(CheckResult { - id: "code-unwrap".to_string(), + id: self.id().to_string(), label: "No .unwrap() in source".to_string(), - group: CheckGroup::CodeQuality, - layer: CheckLayer::Source, + group: self.group(), + layer: self.layer(), status, }) } @@ -61,13 +60,13 @@ impl Check for UnwrapCheck { /// Check a single source string for `.unwrap()` calls. /// /// Kept public(crate) for unit testing with inline source strings. -pub(crate) fn check_unwrap(source: &str, file: &str) -> CheckResult { +pub(crate) fn check_unwrap(source: &str, file: &str) -> CheckStatus { let mut matches = find_pattern_matches(source, PATTERN); for m in &mut matches { m.file = file.to_string(); } - let status = if matches.is_empty() { + if matches.is_empty() { CheckStatus::Pass } else { let evidence = matches @@ -76,14 +75,6 @@ pub(crate) fn check_unwrap(source: &str, file: &str) -> CheckResult { .collect::>() .join("\n"); CheckStatus::Fail(evidence) - }; - - CheckResult { - id: "code-unwrap".to_string(), - label: "No .unwrap() in source".to_string(), - group: CheckGroup::CodeQuality, - layer: CheckLayer::Source, - status, } } @@ -100,8 +91,8 @@ fn main() -> anyhow::Result<()> { Ok(()) } "#; - let result = check_unwrap(source, "src/main.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_unwrap(source, "src/main.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -111,9 +102,9 @@ fn main() { let config = load_config().unwrap(); } "#; - let result = check_unwrap(source, "src/main.rs"); - assert!(matches!(result.status, CheckStatus::Fail(_))); - if let CheckStatus::Fail(evidence) = &result.status { + let status = check_unwrap(source, "src/main.rs"); + assert!(matches!(status, CheckStatus::Fail(_))); + if let CheckStatus::Fail(evidence) = &status { assert!(evidence.contains("unwrap")); assert!(evidence.contains("src/main.rs")); } @@ -128,8 +119,8 @@ fn main() { let c = baz().unwrap(); } "#; - let result = check_unwrap(source, "src/lib.rs"); - if let CheckStatus::Fail(evidence) = &result.status { + let status = check_unwrap(source, "src/lib.rs"); + if let CheckStatus::Fail(evidence) = &status { assert_eq!(evidence.lines().count(), 3); } else { panic!("Expected Fail"); @@ -145,8 +136,8 @@ fn main() -> anyhow::Result<()> { Ok(()) } "#; - let result = check_unwrap(source, "src/main.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_unwrap(source, "src/main.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] @@ -157,8 +148,8 @@ fn main() -> anyhow::Result<()> { Ok(()) } "#; - let result = check_unwrap(source, "src/main.rs"); - assert_eq!(result.status, CheckStatus::Pass); + let status = check_unwrap(source, "src/main.rs"); + assert_eq!(status, CheckStatus::Pass); } #[test] diff --git a/tests/integration.rs b/tests/integration.rs index 22a6d31..696df95 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -652,3 +652,35 @@ fn test_broken_python_fixture() { "broken-python fixture should trigger code-bare-except check" ); } + +/// Convention enforcement: check_x() functions must return CheckStatus, not CheckResult. +/// +/// The Check trait's run() method is the sole constructor of CheckResult. If check_x() +/// returns CheckResult, the ID/group/layer fields are duplicated as string literals +/// instead of derived from self.id()/self.group()/self.layer(). This test greps the +/// source to catch regressions. +#[test] +fn convention_check_x_returns_check_status_not_check_result() { + use std::process::Command; + + // Find all pub(crate) fn check_ functions in source check files and verify + // none of them return CheckResult. + let output = Command::new("rg") + .args([ + "--no-filename", + "-c", + r"pub\(crate\) fn check_.*-> CheckResult", + "src/checks/source/", + ]) + .output() + .expect("rg command failed"); + + let stdout = String::from_utf8_lossy(&output.stdout); + let match_count: usize = stdout.lines().filter_map(|l| l.trim().parse::().ok()).sum(); + + assert_eq!( + match_count, 0, + "Found {match_count} check_x() functions returning CheckResult instead of CheckStatus. \ + See CLAUDE.md 'Source Check Convention' — check_x() must return CheckStatus." + ); +} From 1fd6afdbc96977fd010cb575599f8d2fa8339ec7 Mon Sep 17 00:00:00 2001 From: Brett Date: Wed, 15 Apr 2026 21:28:54 -0500 Subject: [PATCH 2/6] style: format convention enforcement test --- tests/integration.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/integration.rs b/tests/integration.rs index 696df95..dce6a8c 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -676,7 +676,10 @@ fn convention_check_x_returns_check_status_not_check_result() { .expect("rg command failed"); let stdout = String::from_utf8_lossy(&output.stdout); - let match_count: usize = stdout.lines().filter_map(|l| l.trim().parse::().ok()).sum(); + let match_count: usize = stdout + .lines() + .filter_map(|l| l.trim().parse::().ok()) + .sum(); assert_eq!( match_count, 0, From 076f917cb5edecf347d9c50080958acb3df81c90 Mon Sep 17 00:00:00 2001 From: Brett Date: Thu, 16 Apr 2026 11:23:00 -0500 Subject: [PATCH 3/6] fix: enforcement test independence from ripgrep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrite convention_check_x_returns_check_status_not_check_result as pure Rust using std::fs. The previous implementation shelled out to rg which is not available in GitHub Actions runners, causing the test to panic with "No such file or directory" and blocking all CI runs. The new implementation: - Uses CARGO_MANIFEST_DIR for a stable absolute path (no CWD fragility) - Walks src/checks/source/ recursively with only std::fs - Catches all visibility modifiers (pub, pub(crate), pub(super), private) - Handles multi-line signatures by accumulating text until the opening { - Reports specific file:line:code references on failure Also corrects CLAUDE.md Source Check Convention claims: - check_x() signature is (source: &str) or (source: &str, file: &str) - "Every" → "Most" with noted exceptions for output_module.rs and error_types.rs which use different helper shapes --- CLAUDE.md | 7 ++-- tests/integration.rs | 86 +++++++++++++++++++++++++++++++------------- 2 files changed, 66 insertions(+), 27 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 39edfcc..0f6dcf5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -66,11 +66,12 @@ Key decisions already made: ## Source Check Convention -Every source check follows this structure: +Most source checks follow this structure (a few legacy helpers in `output_module.rs` and `error_types.rs` use +different helper shapes but still satisfy the core contract that `run()` is the sole `CheckResult` constructor): - **Struct** implements `Check` trait with `id()`, `group()`, `layer()`, `applicable()`, `run()` -- **`check_x()` helper** takes `(source: &str, file: &str)` and returns `CheckStatus` (not `CheckResult`) — this is the - unit-testable core +- **`check_x()` helper** takes `(source: &str)` (or `(source: &str, file: &str)` when evidence needs file location + context) and returns `CheckStatus` (not `CheckResult`) — this is the unit-testable core - **`run()` is the sole `CheckResult` constructor** — uses `self.id()`, `self.group()`, `self.layer()` to build the result. Never hardcode ID/group/layer string literals in `check_x()` or anywhere outside `run()` - **Tests call `check_x()`** and match on `CheckStatus` directly, not `result.status` diff --git a/tests/integration.rs b/tests/integration.rs index dce6a8c..77dfb7a 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -657,33 +657,71 @@ fn test_broken_python_fixture() { /// /// The Check trait's run() method is the sole constructor of CheckResult. If check_x() /// returns CheckResult, the ID/group/layer fields are duplicated as string literals -/// instead of derived from self.id()/self.group()/self.layer(). This test greps the -/// source to catch regressions. +/// instead of derived from self.id()/self.group()/self.layer(). This test walks the +/// source to catch regressions. Pure Rust — no external tooling required. #[test] fn convention_check_x_returns_check_status_not_check_result() { - use std::process::Command; - - // Find all pub(crate) fn check_ functions in source check files and verify - // none of them return CheckResult. - let output = Command::new("rg") - .args([ - "--no-filename", - "-c", - r"pub\(crate\) fn check_.*-> CheckResult", - "src/checks/source/", - ]) - .output() - .expect("rg command failed"); + use std::ffi::OsStr; + use std::fs; + use std::path::Path; - let stdout = String::from_utf8_lossy(&output.stdout); - let match_count: usize = stdout - .lines() - .filter_map(|l| l.trim().parse::().ok()) - .sum(); + let root = Path::new(env!("CARGO_MANIFEST_DIR")).join("src/checks/source"); + assert!( + root.is_dir(), + "source checks dir not found: {}", + root.display() + ); - assert_eq!( - match_count, 0, - "Found {match_count} check_x() functions returning CheckResult instead of CheckStatus. \ - See CLAUDE.md 'Source Check Convention' — check_x() must return CheckStatus." + let mut violations: Vec = Vec::new(); + visit_dir(&root, &mut |path, contents| { + // Match `fn check_(...) -> CheckResult` allowing any visibility + // modifier and either single-line or multi-line signatures. + // We search for `fn check_` then look ahead for `-> CheckResult` before the + // next `{` (end of signature). + for (i, line) in contents.lines().enumerate() { + if let Some(idx) = line.find("fn check_") { + // Collect signature text until we hit `{` (end of signature) + let mut sig = String::new(); + let rest = &line[idx..]; + sig.push_str(rest); + if !sig.contains('{') { + // signature continues on following lines + for cont in contents.lines().skip(i + 1) { + sig.push(' '); + sig.push_str(cont); + if cont.contains('{') { + break; + } + } + } + let sig_end = sig.find('{').map(|e| &sig[..e]).unwrap_or(&sig); + if sig_end.contains("-> CheckResult") { + violations.push(format!("{}:{}: {}", path.display(), i + 1, line.trim())); + } + } + } + }); + + assert!( + violations.is_empty(), + "Found {} check_x() function(s) returning CheckResult instead of CheckStatus. \ + See CLAUDE.md 'Source Check Convention' — check_x() must return CheckStatus \ + so run() can use self.id()/self.group()/self.layer() as the sole source of truth.\n\n\ + Violations:\n{}", + violations.len(), + violations.join("\n") ); + + fn visit_dir(dir: &Path, f: &mut F) { + for entry in fs::read_dir(dir).expect("read_dir") { + let entry = entry.expect("entry"); + let path = entry.path(); + if path.is_dir() { + visit_dir(&path, f); + } else if path.extension() == Some(OsStr::new("rs")) { + let contents = fs::read_to_string(&path).expect("read_to_string"); + f(&path, &contents); + } + } + } } From 4d4e7e5628d414805d4c45131e41dd565722c24f Mon Sep 17 00:00:00 2001 From: Brett Date: Thu, 16 Apr 2026 11:28:43 -0500 Subject: [PATCH 4/6] fix(clippy): collapse nested if into match guard in no_pager MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit clippy 1.95 flags the nested `if` inside `CheckStatus::Pass =>` as collapsible. Use a match guard instead. Local toolchain was 1.94 so the lint didn't fire pre-push — ran into this as a CI-only failure. --- src/checks/source/rust/no_pager.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/checks/source/rust/no_pager.rs b/src/checks/source/rust/no_pager.rs index 185d4f9..a020c23 100644 --- a/src/checks/source/rust/no_pager.rs +++ b/src/checks/source/rust/no_pager.rs @@ -42,12 +42,10 @@ impl Check for NoPagerCheck { for (_path, parsed_file) in parsed.iter() { match &check_no_pager(&parsed_file.source) { - CheckStatus::Pass => { - // Either no pager or has --no-pager flag - if source_has_pager_code(&parsed_file.source) { - has_pager = true; - has_no_pager_flag = true; - } + // Pass + pager code present means both are true: has pager, has --no-pager flag + CheckStatus::Pass if source_has_pager_code(&parsed_file.source) => { + has_pager = true; + has_no_pager_flag = true; } CheckStatus::Warn(_) => { has_pager = true; From 8a627d651dcf22218a397281513261717faa5098 Mon Sep 17 00:00:00 2001 From: Brett Date: Thu, 16 Apr 2026 11:35:39 -0500 Subject: [PATCH 5/6] chore(hooks): sync local stable toolchain in pre-push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run `rustup update stable --no-self-update` before the other checks so local clippy/rustc match what CI will use. CI reinstalls stable on every run, so if the local toolchain is behind, a newer lint can fail CI while pre-push reports green — exactly what happened with the collapsible_match lint that landed in clippy 1.95. Fast no-op when already current; takes a few seconds on first push after a new stable release. --- CLAUDE.md | 7 ++++--- scripts/hooks/pre-push | 10 ++++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 0f6dcf5..4293308 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -100,9 +100,10 @@ agentnative. Two rules prevent recursive fork bombs: ## CI and Quality -**Pre-push hook:** `scripts/hooks/pre-push` mirrors CI exactly: fmt, clippy with `-Dwarnings`, test, cargo-deny, and a -Windows compatibility check. Tracked in git and activated via `core.hooksPath`. After cloning, run: `git config -core.hooksPath scripts/hooks` +**Pre-push hook:** `scripts/hooks/pre-push` mirrors CI exactly: runs `rustup update stable` to sync the local +toolchain, then fmt, clippy with `-Dwarnings`, test, cargo-deny, and a Windows compatibility check. The toolchain sync +prevents "local clippy older than CI clippy" false greens — CI reinstalls stable every run, so the local hook does too. +Tracked in git and activated via `core.hooksPath`. After cloning, run: `git config core.hooksPath scripts/hooks` **Windows compatibility:** Only `libc` belongs in `[target.'cfg(unix)'.dependencies]`. All SIGPIPE/signal code must be inside `#[cfg(unix)]` blocks. The pre-push hook checks this statically. diff --git a/scripts/hooks/pre-push b/scripts/hooks/pre-push index 1bfaf92..22831e2 100755 --- a/scripts/hooks/pre-push +++ b/scripts/hooks/pre-push @@ -15,6 +15,16 @@ fail() { echo -e " ${RED}✗${RESET} $1"; exit 1; } echo -e "${BOLD}Running local CI checks...${RESET}" +# 0. Sync local stable toolchain to match CI (prevents "local clippy older than CI" gaps). +# CI runs `rustup toolchain install stable` on every run, so it always has the latest +# stable components. If local stable is behind, a lint newer than the local version can +# fail CI while the pre-push hook reports green. Running `rustup update stable` here +# closes the gap — it's a fast no-op when already current. +if command -v rustup &>/dev/null; then + rustup update stable --no-self-update &>/dev/null || true + pass "toolchain ($(rustc --version 2>/dev/null | awk '{print $2}' || echo unknown))" +fi + # 1. Format check cargo fmt -- --check 2>/dev/null || fail "cargo fmt -- --check" pass "fmt" From 381d5e1c36793a73df551c875f57f402692f165e Mon Sep 17 00:00:00 2001 From: Brett Date: Thu, 16 Apr 2026 11:45:03 -0500 Subject: [PATCH 6/6] chore(supply-chain): pin Rust toolchain to version with commit SHA MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pin rust-toolchain.toml to a specific release instead of floating `stable`. Rustup verifies component SHA256s from the distribution manifest — the version pin is effectively a SHA pin. Trailing comment documents the rustc commit SHA for audit, mirroring the GitHub Actions pin pattern (action@ # vN.N.N). Both local and CI now read rust-toolchain.toml and install identical bits. Removes the pre-push rustup-update hook introduced earlier in this branch — pinning is the right fix, not runtime sync. Policy: bump the channel only via reviewed PR, after the new stable has aged ≥7 days. This matches the UV_EXCLUDE_NEWER / bun minimumReleaseAge / npm_config_min_release_age quarantine already in dotfiles, and guards against the `tj-actions/changed-files`-class supply-chain attack on the toolchain itself. --- CLAUDE.md | 13 +++++++++---- rust-toolchain.toml | 6 +++++- scripts/hooks/pre-push | 14 +++++--------- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4293308..88a6d7f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -100,10 +100,15 @@ agentnative. Two rules prevent recursive fork bombs: ## CI and Quality -**Pre-push hook:** `scripts/hooks/pre-push` mirrors CI exactly: runs `rustup update stable` to sync the local -toolchain, then fmt, clippy with `-Dwarnings`, test, cargo-deny, and a Windows compatibility check. The toolchain sync -prevents "local clippy older than CI clippy" false greens — CI reinstalls stable every run, so the local hook does too. -Tracked in git and activated via `core.hooksPath`. After cloning, run: `git config core.hooksPath scripts/hooks` +**Toolchain pin:** `rust-toolchain.toml` pins the channel to a specific `X.Y.Z` version with a trailing comment naming +the rustc commit SHA. Rustup reads this file on every `cargo` invocation — both local and CI snap to identical bits. +Rustup verifies component SHA256s from the distribution manifest, so the version pin is effectively a SHA pin (the +manifest is the toolchain's "lockfile"). Bumping the toolchain is a reviewed PR that updates `rust-toolchain.toml`; no +runtime `rustup update` anywhere. Policy: bump only after a new stable has aged ≥7 days (supply-chain quarantine). + +**Pre-push hook:** `scripts/hooks/pre-push` mirrors CI exactly: fmt, clippy with `-Dwarnings`, test, cargo-deny, and a +Windows compatibility check. Tracked in git and activated via `core.hooksPath`. After cloning, run: `git config +core.hooksPath scripts/hooks` **Windows compatibility:** Only `libc` belongs in `[target.'cfg(unix)'.dependencies]`. All SIGPIPE/signal code must be inside `#[cfg(unix)]` blocks. The pre-push hook checks this statically. diff --git a/rust-toolchain.toml b/rust-toolchain.toml index 73cb934..2473baf 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,3 +1,7 @@ +# Supply-chain pin: rustc version is immutable; rustup verifies component SHA256s +# from the distribution manifest (the "lockfile" for a toolchain release). +# Trailing comment documents the commit SHA for audit. +# Bump via reviewed PR only after a new stable has aged ≥7 days. [toolchain] -channel = "stable" +channel = "1.94.1" # rustc e408947bfd200af42db322daf0fadfe7e26d3bd1, released 2026-03-25 components = ["rustfmt", "clippy"] diff --git a/scripts/hooks/pre-push b/scripts/hooks/pre-push index 22831e2..a9d2349 100755 --- a/scripts/hooks/pre-push +++ b/scripts/hooks/pre-push @@ -15,15 +15,11 @@ fail() { echo -e " ${RED}✗${RESET} $1"; exit 1; } echo -e "${BOLD}Running local CI checks...${RESET}" -# 0. Sync local stable toolchain to match CI (prevents "local clippy older than CI" gaps). -# CI runs `rustup toolchain install stable` on every run, so it always has the latest -# stable components. If local stable is behind, a lint newer than the local version can -# fail CI while the pre-push hook reports green. Running `rustup update stable` here -# closes the gap — it's a fast no-op when already current. -if command -v rustup &>/dev/null; then - rustup update stable --no-self-update &>/dev/null || true - pass "toolchain ($(rustc --version 2>/dev/null | awk '{print $2}' || echo unknown))" -fi +# The toolchain is pinned in rust-toolchain.toml — both local and CI read the same file +# and install the same version. Rustup verifies component SHA256s from the distribution +# manifest, so the pin is effectively a SHA pin. No `rustup update` step here: bumping +# the toolchain is a reviewed PR that updates rust-toolchain.toml. +pass "toolchain ($(rustc --version 2>/dev/null | awk '{print $2}' || echo unknown))" # 1. Format check cargo fmt -- --check 2>/dev/null || fail "cargo fmt -- --check"