fix(audits): exempt #[cfg(test)]-gated items from code-unwrap - #77
Merged
Conversation
`code-unwrap` flagged every `.unwrap()` inside inline `#[cfg(test)] mod
tests { ... }`, gated helper functions, and gated `impl` blocks even
though `--include-tests` was off by default. Only the `tests/` directory
and `*_test.rs` filenames were excluded; the canonical inline-test-module
convention was mis-classified as production code.
Replace the flat `$RECV.unwrap()` ast-grep pattern with a tree-sitter
walk that tracks whether the current node sits inside a
`#[cfg(test)]`-gated item. Attributes are sibling nodes in
tree-sitter-rust, not children, so the walk propagates a one-shot
"next sibling is cfg-test-gated" flag that consumes on the very next
sibling regardless of kind — a `#[cfg(test)] use foo;` correctly
consumes the flag without gating subsequent items.
The cfg-args parser recognises a bare `test` predicate at any nesting
under `any(...)` / `all(...)` / `not(...)` while rejecting
`feature = "test"` (string literals are skipped), `testing_only` /
`test_helper` (full-identifier match only), and unrelated predicates
like `cfg(unix)`. Threading `project.include_tests` into the audit
preserves the existing `--include-tests` opt-in for callers that do
want to audit test code.
brettdavies
added a commit
that referenced
this pull request
Jun 3, 2026
…llow-up plans Adds five plans surfaced by the post-merge adversarial review of PRs #73, #76, #77, and #79: - 2026-06-03-001 (umbrella, P1): structure-over-vocabulary audit philosophy. Spec is canonical; closed-vocabulary lookups (COMMON_VERBS, STANDARD_VERBS, DISCRIMINANT_FIELD_NAMES, SUCCESS_VALUE_STRINGS) are failed quality proxies; per-PR redesigns of p6-consistent-naming and p2-must-json-errors land under this plan. - 2026-06-03-002 (P1): PR #73 follow-up. Probe-failure now Warns instead of silent Pass, leaf non-verb requires a successful probe, drop emit from COMMON_VERBS, add 4 negative-case tests. - 2026-06-03-003 (P1): PR #76 follow-up. Scorecard transparency for .anc.toml domain_verbs (using_domain_verbs flag + domain_match_count + evidence string showing built-in vs domain ratio), trim 7 platform-specific verbs out of built-in STANDARD_VERBS, document the social-CLI .anc.toml pattern. - 2026-06-03-004 (P0): PR #77 follow-up. cfg(not(test)) is treated identically to cfg(test) in the new cfg-args parser, silently exempting production-only code from code-unwrap. Thread polarity through the recursive call site, add 8 negative-case tests, correct an inner-attribute comment. - 2026-06-03-005 (P1): PR #79 follow-up. Close three role-based-validator bypasses: numeric discriminant on info-coded values, success synonyms outside the 8-string closed set, and nested-recursion harvesting roles from a success-coded outer envelope. Add 6 adversarial tests. Plans 002, 003, and 005 are tactical stopgaps that stop active misclassification while plan 001 owns the long-term redesign. Plan 004 is a correctness bug fix and ships independently.
11 tasks
brettdavies
added a commit
that referenced
this pull request
Jun 4, 2026
…tests (#80) ## Summary Closes a P0 correctness regression in `code-unwrap` surfaced by the adversarial review of merged PR #77. PR #77 (commit 817d6fa) 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 into the inner argument list 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 audit, a regression confirmed both by reading the parser and by an injected adversarial test that panicked under the buggy logic. This PR threads a `negated: bool` parameter through `cfg_args_contain_test`. The bare-test branch returns `true` only at even parity (`!negated`); at odd parity (under a `not(...)`) the predicate means production code and the walker keeps scanning siblings in case another predicate resolves to test-gated. The recursive call for `not(...)` flips polarity; `any(...)` and `all(...)` preserve it. Double-negation (`not(not(test))`) correctly restores even parity. Also corrects the inner-attribute comment block (which claimed sticky propagation across all siblings while the code actually does one-shot reset) and pins the intentional asymmetry with a named test. Per plan #4, the diff is surgical: only the parser function, the inner-attribute comment, and the test module are touched. ## Changelog ### Fixed - `code-unwrap`: `#[cfg(not(test))]` no longer silently exempts production-only code from the audit. The cfg-args parser now tracks polarity through `not(...)` wrappers; `not(test)`, `any(not(test), unix)`, and `all(not(test), feature = "x")` correctly classify as production. Polarity-pinning tests cover the double-negation case (`not(not(test))`), the `not(any(test))` shape, `cfg_attr(test, …)` non-gating, and the inner-attribute one-shot reset asymmetry. ## Type of Change - [x] `fix`: Bug fix (non-breaking change which fixes an issue) ## Related Issues/Stories - Plan: `docs/plans/2026-06-03-004-fix-pr77-code-unwrap-cfg-not-test-polarity-plan.md` - Origin: adversarial review of merged PR #77 (commit 817d6fa) - Umbrella plan (audit philosophy, supersedes plans #2 and #5): `docs/plans/2026-06-03-001-refactor-audit-philosophy-structure-over-vocabulary-plan.md` ## Testing - [x] Unit tests added/updated - [x] All tests passing **Test Summary:** - `cargo test`: 846 passed, 2 ignored (8 suites). 9 new unit tests in `src/audits/source/rust/unwrap.rs`: `cfg_not_test_does_not_exempt_production_unwrap`, `cfg_any_not_test_unix_does_not_exempt`, `cfg_all_not_test_feature_does_not_exempt`, `cfg_any_test_or_not_feature_still_exempts`, `cfg_attr_test_does_not_exempt`, `inner_attribute_one_shot_consumes_on_use_not_fn`, `mod_tests_without_gate_does_not_exempt`, `cfg_not_not_test_does_exempt`, `cfg_not_any_test_does_not_exempt`. - `cargo clippy --all-targets -- -Dwarnings`: clean. - Dogfood (`anc audit .`): `code-unwrap` flags one production unwrap at `src/audits/behavioral/json_errors.rs:192` (`trimmed.chars().next().unwrap()`). The unwrap has no cfg gate; it surfaces because PR #79 added it and the audit wasn't re-dogfooded after that merge. Verified identical behavior on pre-fix `dev` via `git stash` + rebuild, ruling out a regression from this PR. Owned by plan #5 (PR #79 follow-up) or addressable as a one-line fix in a separate PR. - Dogfood (`anc audit ~/dev/xurl-rs`): `code-unwrap` continues to `pass`. xurl-rs has zero `#[cfg(not(test))]` items, so the polarity fix is a no-op against that target. ## Files Modified **Modified:** - `src/audits/source/rust/unwrap.rs`: thread `negated: bool` through `cfg_args_contain_test`; flip polarity on `not(...)` recursion (preserve on `any`/`all`); rewrite the inner-attribute comment block to match one-shot reset semantics and name the pinning test; add 9 negative-case unit tests. **Created:** - None. **Renamed:** - None. **Deleted:** - None. ## Breaking Changes - [x] No breaking changes The audit becomes strictly more accurate. Callers that previously relied on `#[cfg(not(test))]` exempting production code from `code-unwrap` were doing so accidentally, since that path was an unintended consequence of PR #77's missing polarity tracking. ## Deployment Notes - [x] No special deployment steps required ## Checklist - [x] Code follows project conventions and style guidelines - [x] Commit messages follow Conventional Commits - [x] Self-review of code completed - [x] Tests added/updated and passing - [x] No new warnings or errors introduced - [x] Changes are backward compatible
11 tasks
brettdavies
added a commit
that referenced
this pull request
Jun 4, 2026
…ue (#82) ## Summary Addresses the `json_errors.rs:192` finding from PR #80's dogfood run: the `.unwrap()` on `trimmed.chars().next()` inside `is_type_id_value` was provably safe (an `is_empty()` guard ten lines earlier guarantees at least one char), but the `code-unwrap` source audit pattern-matches on `.unwrap()` call expressions and surfaced it during self-dogfood. Replaces the `.unwrap()` with a `let-else` that returns `false`, consistent with the function's "return false on any invalid input" pattern. Adds a comprehensive unit test (`is_type_id_value_validates_json_shapes`) covering all five non-string JSON value kinds (Number, Bool, Null, Array, Object), three valid identifier shapes (kebab-case, SCREAMING_SNAKE, snake_case), and three invalid string shapes (whitespace, period-separated, leading digit). ## Changelog ### Fixed - Fix `code-unwrap` self-dogfood finding at `json_errors.rs:192`: replace `.unwrap()` with a `let-else` returning `false` in `is_type_id_value`. ## Type of Change - [x] `fix`: Bug fix (non-breaking change which fixes an issue) ## Related Issues/Stories - PR #80 (dogfood surfaced `json_errors.rs:192` as the one remaining `code-unwrap` finding) - PR #77 (introduced the `code-unwrap` audit) - PR #79 (introduced the `.unwrap()` that this PR removes) ## Testing - [x] Unit tests added/updated - [x] All tests passing **Test Summary:** - `cargo test`: 848 passed, 2 ignored (8 suites). +1 new test `is_type_id_value_validates_json_shapes` in `src/audits/behavioral/json_errors.rs`. - `cargo clippy --all-targets -- -Dwarnings`: clean. - `cargo fmt --check`: clean. ## Files Modified **Modified:** - `src/audits/behavioral/json_errors.rs`: replace `.unwrap()` with `let-else` at line 192; add `is_type_id_value_validates_json_shapes` unit test. **Created:** - None. **Renamed:** - None. **Deleted:** - None. ## Breaking Changes - [x] No breaking changes ## Deployment Notes - [x] No special deployment steps required ## Checklist - [x] Code follows project conventions and style guidelines - [x] Commit messages follow Conventional Commits - [x] Self-review of code completed - [x] Tests added/updated and passing - [x] No new warnings or errors introduced - [x] Changes are backward compatible
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
code-unwrapflagged every.unwrap()inside inline#[cfg(test)] mod tests { ... }, gated helper functions, and gatedimplblocks even though--include-testswas off by default. The pre-existing filter only excluded thetests/directory and*_test.rsfilenames, so the canonical Rust inline-test-module convention was mis-classified as production code and inflated false-positive counts on every Rust project that audits its own crate.This PR replaces the flat
$RECV.unwrap()ast-grep pattern with a tree-sitter walk that tracks whether the current node sits inside a#[cfg(test)]-gated item. Attributes are sibling nodes in tree-sitter-rust (not children), so the walk propagates a one-shot "next sibling is cfg-test-gated" flag that consumes on the very next sibling regardless of kind: a#[cfg(test)] use foo;correctly consumes the flag without gating subsequent items.The cfg-args parser recognises a bare
testpredicate at any nesting underany(...)/all(...)/not(...)while rejectingfeature = "test"(string literals are skipped),testing_only/test_helper(full-identifier match only), and unrelated predicates likecfg(unix). Threadingproject.include_testsinto the audit preserves the existing--include-testsopt-in for callers that do want to audit test code.Verified on
xurl-rs(the motivating repo): the audit drops fromfailwith ~50 evidence lines topass.Changelog
Fixed
.unwrap()calls inside#[cfg(test)]-gated items (inlinemod tests, gated helper functions, gatedimplblocks) fromcode-unwrapby default;--include-testsrestores the prior behaviour.Type of Change
fix: Bug fix (non-breaking change which fixes an issue)Related Issues/Stories
docs/plans/2026-06-02-001-feat-anc-100-percent-compliance-plan.mdU1 (xurl-rs side)Testing
Test Summary:
src/audits/source/rust/unwrap.rscovering: inlinemod testsexemption, nested gating,#[cfg(test)] usenot gating later items,cfg(any(test, feature = "x")),cfg(all(test, unix)),feature = "test"not gating,test_helper/cfg(unix)not gating, gatedimplblocks, and--include-testsregression.cargo test→ 810 passing, 2 ignored.cargo clippy --all-targets -- -D warningsclean.xurl-rs:code-unwrapflips fromfail(~50 evidence lines) topass.Files Modified
Modified:
src/audits/source/rust/unwrap.rs— tree-sitter walk, cfg-args parser, expanded test suite.Breaking Changes
The audit becomes strictly more permissive by default; callers that depend on the old behaviour can pass
--include-tests.