diff --git a/CLAUDE.md b/CLAUDE.md index 84dfcee..88a6d7f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -64,6 +64,25 @@ Key decisions already made: - Feature flag is `tree-sitter-rust`, not `language-rust` - Edition 2024, dual MIT/Apache-2.0 license +## Source Check Convention + +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)` (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` + +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 @@ -81,6 +100,12 @@ agentnative. Two rules prevent recursive fork bombs: ## CI and Quality +**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` 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 1bfaf92..a9d2349 100755 --- a/scripts/hooks/pre-push +++ b/scripts/hooks/pre-push @@ -15,6 +15,12 @@ fail() { echo -e " ${RED}✗${RESET} $1"; exit 1; } echo -e "${BOLD}Running local CI checks...${RESET}" +# 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" pass "fmt" 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..a020c23 100644 --- a/src/checks/source/rust/no_pager.rs +++ b/src/checks/source/rust/no_pager.rs @@ -41,14 +41,11 @@ 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 { - 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; - } + match &check_no_pager(&parsed_file.source) { + // 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; @@ -68,10 +65,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 +90,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 +108,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 +122,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 +137,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 +150,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 +166,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 +179,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 +196,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..77dfb7a 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -652,3 +652,76 @@ 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 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::ffi::OsStr; + use std::fs; + use std::path::Path; + + let root = Path::new(env!("CARGO_MANIFEST_DIR")).join("src/checks/source"); + assert!( + root.is_dir(), + "source checks dir not found: {}", + root.display() + ); + + 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); + } + } + } +}