From a11286a082c1f70a937dcb1a32ab89d93a96bf89 Mon Sep 17 00:00:00 2001 From: Brett Date: Wed, 3 Jun 2026 19:01:05 -0500 Subject: [PATCH 1/3] fix(code-unwrap): cfg(not(test)) polarity; add missing negative-case tests PR #77 introduced a tree-sitter walk that exempts .unwrap() calls inside #[cfg(test)]-gated items. The cfg-args parser, however, treats not(...) identically to any(...) and all(...): it descends without polarity tracking and returns true whenever the bare identifier `test` appears at any depth. The effect was that #[cfg(not(test))] on production-only code silently exempted that code from the code-unwrap audit. The fix threads a `negated: bool` parameter through cfg_args_contain_test. The bare-test branch returns true only at even parity (!negated); at odd parity the predicate means production code and continues scanning sibling predicates. The recursive call for not(...) flips polarity; any(...) / all(...) preserve it. Also corrects the inner-attribute comment block (which claimed sticky propagation while the code does one-shot reset) and adds 9 negative-case unit tests covering cfg(not(test)), cfg(any(not(test), unix)), cfg(all(not(test), feature = "x")), cfg(any(test, not(feature = "real"))) still-exempts regression, cfg_attr, mod-tests-without-gate, inner-attribute one-shot consumption, cfg(not(not(test))) double-negation, and cfg(not(any(test))) polarity-pin. Plan: docs/plans/2026-06-03-004-fix-pr77-code-unwrap-cfg-not-test-polarity-plan.md --- src/audits/source/rust/unwrap.rs | 228 +++++++++++++++++++++++++++++-- 1 file changed, 215 insertions(+), 13 deletions(-) diff --git a/src/audits/source/rust/unwrap.rs b/src/audits/source/rust/unwrap.rs index 9075157..759581e 100644 --- a/src/audits/source/rust/unwrap.rs +++ b/src/audits/source/rust/unwrap.rs @@ -120,11 +120,21 @@ fn walk<'a>( for child in node.children() { let kind = child.kind(); if kind.as_ref() == "attribute_item" || kind.as_ref() == "inner_attribute_item" { - // An inner attribute (`#![cfg(test)]` at the head of a mod body) - // gates the enclosing item — i.e. *every* sibling of the inner - // attribute inherits the gate. Conservatively, treat - // `inner_attribute_item` as if it gated all following siblings - // here, which covers the canonical position at the top of a mod. + // Both outer (`#[cfg(test)]`) and inner (`#![cfg(test)]`) + // attribute kinds are handled uniformly: when the attribute + // resolves to `cfg(test)`, set the one-shot `next_is_cfg_test` + // flag so the very next sibling (and only that one) inherits + // the gate. + // + // This is *not* canonical Rust semantics for inner attributes — + // a real `#![cfg(test)]` at the head of a mod body gates every + // sibling in that mod body — but the asymmetry is intentional + // here. Fixing it would require hoisting the gate to the + // enclosing `mod_item` or making the flag sticky across + // siblings; both are out of scope for this walker. The + // resulting behavior (only the next sibling is gated by an + // inner attribute) is pinned by + // `inner_attribute_one_shot_consumes_on_use_not_fn`. if attribute_text_is_cfg_test(child.text().as_ref()) { next_is_cfg_test = true; } @@ -195,7 +205,7 @@ fn attribute_text_is_cfg_test(attr: &str) -> bool { let Some(args) = rest.strip_prefix('(').and_then(|s| s.strip_suffix(')')) else { return false; }; - cfg_args_contain_test(args) + cfg_args_contain_test(args, false) } /// Walk the textual argument list of a `cfg(...)` attribute looking for a bare @@ -203,7 +213,16 @@ fn attribute_text_is_cfg_test(attr: &str) -> bool { /// and only matches full identifiers (so `testing_only` / `test_helper` / /// `cfg(unix)` are rejected). Handles arbitrary nesting under `any(...)`, /// `all(...)`, and `not(...)`. -fn cfg_args_contain_test(args: &str) -> bool { +/// +/// `negated` tracks the polarity inherited from enclosing `not(...)` wrappers: +/// at even parity (`negated = false`) a bare `test` means the item is test-gated; +/// at odd parity (`negated = true`) it means the item is *non*-test-gated +/// (production-only). Only `not(...)` flips polarity on recursion; `any(...)` +/// and `all(...)` preserve it. This is conservative: returning `true` from any +/// recursion claims "this attribute resolves to test-gated"; sibling predicates +/// inside `any(...)` / `all(...)` cannot revoke that claim, so a production +/// item with `cfg(any(not(test), unix))` correctly returns `false`. +fn cfg_args_contain_test(args: &str, negated: bool) -> bool { let bytes = args.as_bytes(); let mut i = 0; while i < bytes.len() { @@ -237,18 +256,25 @@ fn cfg_args_contain_test(args: &str) -> bool { j += 1; } let next = bytes.get(j).copied(); - if ident == "test" { - // Bare `test` predicate: must be followed by a separator - // (`,`, `)`, or end-of-args), not `=` (would make it a key) - // and not `(` (would make it a nested call, e.g. `test(...)`). - if next.is_none() || matches!(next, Some(b',') | Some(b')')) { + if ident == "test" && (next.is_none() || matches!(next, Some(b',') | Some(b')'))) { + // Bare `test` predicate, followed by a separator (`,`, `)`, + // or end-of-args). At even parity (`!negated`) the item is + // test-gated. At odd parity (under a `not(...)`) the + // predicate means production code; keep scanning sibling + // predicates in case another one resolves to test-gated. + if !negated { return true; } + continue; } else if (ident == "any" || ident == "all" || ident == "not") && next == Some(b'(') && let Some(inner) = balanced_parens(&args[j..]) { - if cfg_args_contain_test(inner) { + let recurse_negated = match ident { + "not" => !negated, + _ => negated, // `any` and `all` preserve polarity. + }; + if cfg_args_contain_test(inner, recurse_negated) { return true; } i = j + inner.len() + 2; // skip past matching ')' @@ -553,6 +579,182 @@ impl S { assert_eq!(status, AuditStatus::Pass); } + #[test] + fn cfg_not_test_does_not_exempt_production_unwrap() { + // Regression guard for the PR #77 polarity bug: `cfg(not(test))` + // gates *production-only* code (compiled when NOT testing) and must + // NOT be treated as cfg(test). The unwrap below is production and + // must flag. + let source = r#" +#[cfg(not(test))] +fn production_only() { + foo().unwrap(); +} +"#; + let status = audit_unwrap_with(source, "src/lib.rs", false); + if let AuditStatus::Fail(evidence) = &status { + assert_eq!(evidence.lines().count(), 1); + assert!(evidence.contains("foo().unwrap()")); + } else { + panic!("expected Fail, got {status:?}"); + } + } + + #[test] + fn cfg_any_not_test_unix_does_not_exempt() { + // `any(not(test), unix)` compiles in (production AND any platform) + // OR (any compilation AND unix). Either way the item compiles in + // production builds — the unwrap is production code. + let source = r#" +#[cfg(any(not(test), unix))] +fn x() { + foo().unwrap(); +} +"#; + let status = audit_unwrap_with(source, "src/lib.rs", false); + if let AuditStatus::Fail(evidence) = &status { + assert_eq!(evidence.lines().count(), 1); + assert!(evidence.contains("foo().unwrap()")); + } else { + panic!("expected Fail, got {status:?}"); + } + } + + #[test] + fn cfg_all_not_test_feature_does_not_exempt() { + // `all(not(test), feature = "x")` is production-only under feature + // `x`. The unwrap is production code. + let source = r#" +#[cfg(all(not(test), feature = "x"))] +fn x() { + foo().unwrap(); +} +"#; + let status = audit_unwrap_with(source, "src/lib.rs", false); + if let AuditStatus::Fail(evidence) = &status { + assert_eq!(evidence.lines().count(), 1); + assert!(evidence.contains("foo().unwrap()")); + } else { + panic!("expected Fail, got {status:?}"); + } + } + + #[test] + fn cfg_any_test_or_not_feature_still_exempts() { + // Polarity fix must not break the positive case: `any(test, ...)` + // still has a bare `test` predicate at even parity. The unwrap is + // gated. + let source = r#" +#[cfg(any(test, not(feature = "real")))] +mod tests { + fn t() { foo().unwrap(); } +} +"#; + let status = audit_unwrap_with(source, "src/lib.rs", false); + assert_eq!(status, AuditStatus::Pass); + } + + #[test] + fn cfg_attr_test_does_not_exempt() { + // `cfg_attr(test, ...)` applies the inner attributes conditionally; + // the item itself compiles unconditionally. The walk must NOT treat + // `cfg_attr` as a gate. The unwrap is production code. + let source = r#" +#[cfg_attr(test, allow(unused))] +fn x() { + foo().unwrap(); +} +"#; + let status = audit_unwrap_with(source, "src/lib.rs", false); + if let AuditStatus::Fail(evidence) = &status { + assert_eq!(evidence.lines().count(), 1); + assert!(evidence.contains("foo().unwrap()")); + } else { + panic!("expected Fail, got {status:?}"); + } + } + + #[test] + fn inner_attribute_one_shot_consumes_on_use_not_fn() { + // Pins the intentional one-shot semantics for inner attributes: + // `#![cfg(test)]` followed by `use foo;` and then `fn production` + // gates only the `use` (which is not item-like) — the `fn` slips + // through. This is *not* canonical Rust semantics; the walker is + // deliberately one-shot for both outer and inner attributes and + // accepts the asymmetry. See the comment block at the top of + // `walk` for the rationale. + let source = r#" +#![cfg(test)] +use foo; + +fn production() { + foo().unwrap(); +} +"#; + let status = audit_unwrap_with(source, "src/lib.rs", false); + if let AuditStatus::Fail(evidence) = &status { + assert_eq!(evidence.lines().count(), 1); + assert!(evidence.contains("foo().unwrap()")); + } else { + panic!("expected Fail, got {status:?}"); + } + } + + #[test] + fn mod_tests_without_gate_does_not_exempt() { + // A module *named* `tests` without a `#[cfg(test)]` gate is + // production code; the walker uses the gate, not the name. The + // unwrap inside must flag. + let source = r#" +mod tests { + fn helper() { + foo().unwrap(); + } +} +"#; + let status = audit_unwrap_with(source, "src/lib.rs", false); + if let AuditStatus::Fail(evidence) = &status { + assert_eq!(evidence.lines().count(), 1); + assert!(evidence.contains("foo().unwrap()")); + } else { + panic!("expected Fail, got {status:?}"); + } + } + + #[test] + fn cfg_not_not_test_does_exempt() { + // `not(not(test))` is semantically equivalent to `test` (even + // parity restored). The polarity tracker must recognize this. The + // unwrap inside is test code and must NOT flag. + let source = r#" +#[cfg(not(not(test)))] +mod tests { + fn t() { foo().unwrap(); } +} +"#; + let status = audit_unwrap_with(source, "src/lib.rs", false); + assert_eq!(status, AuditStatus::Pass); + } + + #[test] + fn cfg_not_any_test_does_not_exempt() { + // `not(any(test))` is semantically equivalent to `not(test)` — + // production-only. The unwrap inside is production code. + let source = r#" +#[cfg(not(any(test)))] +fn x() { + foo().unwrap(); +} +"#; + let status = audit_unwrap_with(source, "src/lib.rs", false); + if let AuditStatus::Fail(evidence) = &status { + assert_eq!(evidence.lines().count(), 1); + assert!(evidence.contains("foo().unwrap()")); + } else { + panic!("expected Fail, got {status:?}"); + } + } + #[test] fn include_tests_flag_disables_exemption() { // Regression guard for U1's contract: every cfg(test)-gated unwrap From 93dd9faffa35fa7e6755b66bf183fca9109c995a Mon Sep 17 00:00:00 2001 From: Brett Date: Wed, 3 Jun 2026 19:18:36 -0500 Subject: [PATCH 2/3] test(code-unwrap): add cfg-test edge-cases fixture and integration test Wires the polarity-fix parser to a real on-disk project so the full parse -> walk -> AuditResult -> scorecard pipeline is exercised, not just the parser in isolation. The fixture covers four cfg shapes in one src/lib.rs: production code (no gate, must flag), #[cfg(not(test))] production-only (PR #80 regression guard, must flag), #[cfg(test)] helper (must exempt), and items inside #[cfg(test)] mod tests (must exempt). The integration test pins expected evidence to the fixture's current line numbers (20, 25 flagged; 30, 37 exempt). A header comment in the fixture tells future maintainers to update the test if items are reordered. Closes the "no fixture" gap surfaced during plan #004 execution. --- tests/fixtures/cfg-test-edge-cases/Cargo.toml | 4 ++ tests/fixtures/cfg-test-edge-cases/src/lib.rs | 41 +++++++++++ tests/integration.rs | 72 +++++++++++++++++++ 3 files changed, 117 insertions(+) create mode 100644 tests/fixtures/cfg-test-edge-cases/Cargo.toml create mode 100644 tests/fixtures/cfg-test-edge-cases/src/lib.rs diff --git a/tests/fixtures/cfg-test-edge-cases/Cargo.toml b/tests/fixtures/cfg-test-edge-cases/Cargo.toml new file mode 100644 index 0000000..83b6811 --- /dev/null +++ b/tests/fixtures/cfg-test-edge-cases/Cargo.toml @@ -0,0 +1,4 @@ +[package] +name = "cfg-test-edge-cases" +version = "0.1.0" +edition = "2021" diff --git a/tests/fixtures/cfg-test-edge-cases/src/lib.rs b/tests/fixtures/cfg-test-edge-cases/src/lib.rs new file mode 100644 index 0000000..005b469 --- /dev/null +++ b/tests/fixtures/cfg-test-edge-cases/src/lib.rs @@ -0,0 +1,41 @@ +// Fixture for the code-unwrap audit's cfg-gate exemption logic. Exercises the +// full parse -> walk -> AuditResult -> scorecard pipeline for the four cfg +// shapes that matter: +// +// 1. Production code (no gate): MUST be flagged. +// 2. `#[cfg(not(test))]` production-only items: MUST be flagged. This is the +// regression guard for the PR #80 polarity fix. +// 3. `#[cfg(test)]`-gated functions: MUST be exempt. +// 4. `#[cfg(test)] mod`-gated items: MUST be exempt. +// +// The integration test in tests/integration.rs asserts exactly which lines +// surface in evidence; do not reorder items or add unrelated unwraps without +// updating the expected line numbers there. + +fn maybe() -> Result { + Ok(42) +} + +pub fn production_path() { + let _ = maybe().unwrap(); +} + +#[cfg(not(test))] +pub fn production_only_path() { + let _ = maybe().unwrap(); +} + +#[cfg(test)] +pub fn test_only_helper() { + let _ = maybe().unwrap(); +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn unit() { + let _ = maybe().unwrap(); + } +} diff --git a/tests/integration.rs b/tests/integration.rs index 1b98cc1..9d64a04 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -349,6 +349,78 @@ fn test_perfect_fixture() { assert_eq!(error_count, 0, "perfect-rust fixture should have 0 errors"); } +#[test] +fn test_cfg_test_edge_cases_fixture_flags_only_production_unwraps() { + // End-to-end coverage of the code-unwrap audit's cfg-gate exemption logic + // through the full parse -> walk -> AuditResult -> scorecard pipeline. + // Unit tests in src/audits/source/rust/unwrap.rs cover the parser in + // isolation; this test wires the parser to a real project on disk and + // checks that the scorecard reflects the polarity-correct behavior fixed + // in PR #80. + // + // Expected evidence (pinned to the fixture's current layout — see + // tests/fixtures/cfg-test-edge-cases/src/lib.rs): + // - line 20: production_path() (no cfg gate) + // - line 25: production_only_path() (#[cfg(not(test))]) + // Exempted (must NOT appear in evidence): + // - line 30: test_only_helper() (#[cfg(test)]) + // - line 37: tests::unit() (inside #[cfg(test)] mod) + let path = fixture_path("cfg-test-edge-cases"); + + let assert = cmd().args(["audit", &path, "--output", "json"]).assert(); + let output = assert.get_output().stdout.clone(); + let json_str = String::from_utf8(output).expect("stdout should be valid UTF-8"); + let parsed: serde_json::Value = + serde_json::from_str(&json_str).expect("output should be valid JSON"); + + let code_unwrap = parsed["results"] + .as_array() + .expect("results should be an array") + .iter() + .find(|r| r["id"].as_str() == Some("code-unwrap")) + .expect("results should include a code-unwrap row"); + + assert_eq!( + code_unwrap["status"].as_str(), + Some("fail"), + "code-unwrap must fail on the fixture (two production unwraps): {code_unwrap}", + ); + + let evidence = code_unwrap["evidence"] + .as_str() + .expect("code-unwrap fail must carry an evidence string"); + let lines: Vec<&str> = evidence.lines().collect(); + assert_eq!( + lines.len(), + 2, + "evidence must list exactly two unwraps (production_path + cfg(not(test)) production_only_path), got: {evidence}", + ); + + let has_production_path = lines + .iter() + .any(|l| l.contains("/lib.rs:20:") && l.contains("maybe().unwrap()")); + let has_production_only_path = lines + .iter() + .any(|l| l.contains("/lib.rs:25:") && l.contains("maybe().unwrap()")); + assert!( + has_production_path, + "evidence must flag production_path at lib.rs:20: {evidence}", + ); + assert!( + has_production_only_path, + "evidence must flag the cfg(not(test)) production_only_path at lib.rs:25 (PR #80 regression guard): {evidence}", + ); + + // The two cfg(test) cases live at lib.rs:30 and lib.rs:37 in the fixture; + // neither must appear in evidence. + for forbidden in [":30:", ":37:"] { + assert!( + !evidence.contains(forbidden), + "cfg(test)-exempt line {forbidden} leaked into evidence: {evidence}", + ); + } +} + // ── Bare invocation test ────────────────────────────────────────── #[test] From 7245ec24a2199648441a1ebb2f069504c1918a1c Mon Sep 17 00:00:00 2001 From: Brett Date: Wed, 3 Jun 2026 19:24:45 -0500 Subject: [PATCH 3/3] chore(fixtures): align cfg-test-edge-cases fixture to edition 2024 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Matches the main crate's edition (Cargo.toml:4). The audit parses the fixture's source via tree-sitter and does not invoke cargo build, so the edition does not affect audit behavior; this is a consistency cleanup. The three pre-existing Rust fixtures (broken-rust, perfect-rust, source-only) remain on edition 2021 — pre-dating main's bump and out of scope for this PR. --- tests/fixtures/cfg-test-edge-cases/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fixtures/cfg-test-edge-cases/Cargo.toml b/tests/fixtures/cfg-test-edge-cases/Cargo.toml index 83b6811..7416de1 100644 --- a/tests/fixtures/cfg-test-edge-cases/Cargo.toml +++ b/tests/fixtures/cfg-test-edge-cases/Cargo.toml @@ -1,4 +1,4 @@ [package] name = "cfg-test-edge-cases" version = "0.1.0" -edition = "2021" +edition = "2024"