fix(code-unwrap): cfg(not(test)) polarity; add missing negative-case tests#80
Merged
Merged
Conversation
…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
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 #4 execution.
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.
11 tasks
brettdavies
added a commit
that referenced
this pull request
Jun 4, 2026
…le (#81) ## Summary PR #80 review surfaced that the three pre-existing Rust fixtures lag the main crate by one edition. This PR aligns them and documents the convention so future fixtures don't drift again. `tests/fixtures/broken-rust/`, `tests/fixtures/perfect-rust/`, and `tests/fixtures/source-only/` were on `edition = "2021"`. Main is `edition = "2024"`. The audits parse fixture sources via tree-sitter and never invoke `cargo build`, so the drift is harmless today, but a fixture lagging the main crate is a silent skew that could mask audit regressions on edition-specific syntax in future. ### Why not Cargo workspace inheritance? Cargo's `field.workspace = true` requires the sub-crate to be a workspace member. The fixtures here are intentionally NOT workspace members. Making them members would have `cargo build` from the root compile every fixture, and would apply workspace-level lints, dependencies, and profile overrides to them, changing what the audits see. Inheritance was investigated and rejected; documenting the convention in `AGENTS.md` is the practical alternative. ## Type of Change - [x] `chore`: Maintenance tasks (dependencies, config, etc.) ## Related Issues/Stories - Origin: surfaced during PR #80 review. - See `AGENTS.md > Testing > Test fixtures` for the new convention. ## Testing - [x] Existing tests - [x] All tests passing **Test Summary:** - `cargo test`: 847 passed, 2 ignored (same baseline as PR #80 merge, no regression). - `cargo clippy --all-targets -- -Dwarnings`: clean. - Integration tests touching the three bumped fixtures (`test_broken_fixture`, `test_perfect_fixture`, `test_source_only_fixture`, `test_binary_only_fixture`): all 4 pass. ## Files Modified **Modified:** - `tests/fixtures/broken-rust/Cargo.toml`: edition 2021 → 2024. - `tests/fixtures/perfect-rust/Cargo.toml`: edition 2021 → 2024. - `tests/fixtures/source-only/Cargo.toml`: edition 2021 → 2024. - `AGENTS.md`: new "Test fixtures" subsection under "Testing" documenting the lockstep edition rule and the workspace-inheritance non-option. **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
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
Closes a P0 correctness regression in
code-unwrapsurfaced 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, treatsnot(...)identically toany(...)andall(...): it descends into the inner argument list without polarity tracking and returnstruewhenever the bare identifiertestappears 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: boolparameter throughcfg_args_contain_test. The bare-test branch returnstrueonly at even parity (!negated); at odd parity (under anot(...)) the predicate means production code and the walker keeps scanning siblings in case another predicate resolves to test-gated. The recursive call fornot(...)flips polarity;any(...)andall(...)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 throughnot(...)wrappers;not(test),any(not(test), unix), andall(not(test), feature = "x")correctly classify as production. Polarity-pinning tests cover the double-negation case (not(not(test))), thenot(any(test))shape,cfg_attr(test, …)non-gating, and the inner-attribute one-shot reset asymmetry.Type of Change
fix: Bug fix (non-breaking change which fixes an issue)Related Issues/Stories
docs/plans/2026-06-03-004-fix-pr77-code-unwrap-cfg-not-test-polarity-plan.mddocs/plans/2026-06-03-001-refactor-audit-philosophy-structure-over-vocabulary-plan.mdTesting
Test Summary:
cargo test: 846 passed, 2 ignored (8 suites). 9 new unit tests insrc/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.anc audit .):code-unwrapflags one production unwrap atsrc/audits/behavioral/json_errors.rs:192(trimmed.chars().next().unwrap()). The unwrap has no cfg gate; it surfaces because PR feat(audits): role-based JSON error envelope validation #79 added it and the audit wasn't re-dogfooded after that merge. Verified identical behavior on pre-fixdevviagit stash+ rebuild, ruling out a regression from this PR. Owned by plan ci: add workflows and rulesets to main #5 (PR feat(audits): role-based JSON error envelope validation #79 follow-up) or addressable as a one-line fix in a separate PR.anc audit ~/dev/xurl-rs):code-unwrapcontinues topass. 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: threadnegated: boolthroughcfg_args_contain_test; flip polarity onnot(...)recursion (preserve onany/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:
Renamed:
Deleted:
Breaking Changes
The audit becomes strictly more accurate. Callers that previously relied on
#[cfg(not(test))]exempting production code fromcode-unwrapwere doing so accidentally, since that path was an unintended consequence of PR #77's missing polarity tracking.Deployment Notes
Checklist