Skip to content

fix: make production panic controls falsifiable - #176

Merged
hyperpolymath merged 2 commits into
mainfrom
fix/issue-171-panic-surface
Aug 9, 2026
Merged

fix: make production panic controls falsifiable#176
hyperpolymath merged 2 commits into
mainfrom
fix/issue-171-panic-surface

Conversation

@hyperpolymath

Copy link
Copy Markdown
Owner

What changed

  • make a single production panic!() or .unwrap() a PanicPath finding;
  • use the canonical test-path classifier for Rust panic-surface exclusions;
  • exclude tests/, benches/, #[cfg(test)] functions/modules, #[should_panic] bodies, and build.rs from the production panic surface;
  • remove the unsupported Kanren inference that any Rust .unwrap() is protected by a Result boundary;
  • add paired precision/recall regression controls.

Why

Issue #171's original zero-finding criterion could pass both a correct exclusion and an exclusion that accidentally hid production code. The previous analyzer also counted explicit panic sites without emitting a finding, required more than five unwrap/expect calls, and suppressed unwrap findings based only on the presence of unwraps in a Rust file.

The new fixture proves all three directions: one production explicit panic is visible, one production None::<()>.unwrap() is visible, and test/benchmark/#[should_panic]/build.rs panic code produces zero active findings.

Validation

cargo test passes all library, integration, end-to-end, readiness, and doc tests (381 library tests passed; 3 regression tests remain intentionally ignored).

Closes #171.

@hyperpolymath
hyperpolymath force-pushed the fix/issue-171-panic-surface branch from 7d3cf4b to b6b13d1 Compare August 9, 2026 09:29

@gitar-bot gitar-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This PR is blocked due to unresolved code review findings.

Configure merge blocking · Maintainers can dismiss this review.

@gitar-bot

gitar-bot Bot commented Aug 9, 2026

Copy link
Copy Markdown

Note

Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime.
Learn more

Code Review 👍 Approved with suggestions 0 resolved / 2 findings

Refactors Rust production panic-surface analysis to correctly flag unwrap and panic calls while excluding test and benchmark paths. Consider addressing brittle byte-matching for #[should_panic] and #[cfg(test)] attributes to prevent bypasses from whitespace or macro variants.

Auto-approved and auto-merge armed: No blocking issues found.
Please see Auto-approve Docs for details on setting custom approval criteria. — merges when pipeline and required approvals pass.

💡 Edge Case: should_panic stripping keys on attribute alone, enabling evasion

📄 src/assail/analyzer.rs:1679-1693

strip_should_panic_items_rs blanks the body of any item preceded by #[should_panic regardless of whether the function is actually a test (#[test]). Because rustc only warns (not errors) on a #[should_panic] attribute placed on a non-test fn, a production function annotated #[should_panic] fn f() { panic!() } — accidentally or via macro expansion — is silently removed from the production panic surface, producing a false negative for the recall contract this PR establishes. Consider only stripping when a #[test] attribute is also present on the same item, or documenting the accepted limitation.

💡 Quality: should_panic/cfg matching is brittle to attribute whitespace/variants

📄 src/assail/analyzer.rs:1686 📄 src/assail/analyzer.rs:1552 📄 src/assail/analyzer.rs:1609-1611

The byte matchers require the exact literals #[should_panic and #[cfg( with no interior whitespace, so formatter output or forms like #[ should_panic ], #[cfg_attr(test, should_panic)], or # [cfg(test)] are not recognized and their bodies are left in the production panic surface. This is a precision gap (extra findings) rather than a correctness bug and fails open, but is worth a normalizing pre-pass or a documented assumption that input is rustfmt-canonical.

🤖 Prompt for agents
Code Review: Refactors Rust production panic-surface analysis to correctly flag unwrap and panic calls while excluding test and benchmark paths. Consider addressing brittle byte-matching for `#[should_panic]` and `#[cfg(test)]` attributes to prevent bypasses from whitespace or macro variants.

1. 💡 Edge Case: should_panic stripping keys on attribute alone, enabling evasion
   Files: src/assail/analyzer.rs:1679-1693

   `strip_should_panic_items_rs` blanks the body of any item preceded by `#[should_panic` regardless of whether the function is actually a test (`#[test]`). Because rustc only warns (not errors) on a `#[should_panic]` attribute placed on a non-test fn, a production function annotated `#[should_panic] fn f() { panic!() }` — accidentally or via macro expansion — is silently removed from the production panic surface, producing a false negative for the recall contract this PR establishes. Consider only stripping when a `#[test]` attribute is also present on the same item, or documenting the accepted limitation.

2. 💡 Quality: should_panic/cfg matching is brittle to attribute whitespace/variants
   Files: src/assail/analyzer.rs:1686, src/assail/analyzer.rs:1552, src/assail/analyzer.rs:1609-1611

   The byte matchers require the exact literals `#[should_panic` and `#[cfg(` with no interior whitespace, so formatter output or forms like `#[ should_panic ]`, `#[cfg_attr(test, should_panic)]`, or `# [cfg(test)]` are not recognized and their bodies are left in the production panic surface. This is a precision gap (extra findings) rather than a correctness bug and fails open, but is worth a normalizing pre-pass or a documented assumption that input is rustfmt-canonical.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Important

Your trial ends in 1 day — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

Was this helpful? React with 👍 / 👎 | Gitar

@hyperpolymath
hyperpolymath marked this pull request as ready for review August 9, 2026 09:31
@gitar-bot gitar-bot Bot added the gitar-approved Added by Gitar label Aug 9, 2026

@gitar-bot gitar-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gitar has auto-approved this PR and enabled auto-merge (configure)

@gitar-bot

gitar-bot Bot commented Aug 9, 2026

Copy link
Copy Markdown

Note

Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime.
Learn more

Code Review 👍 Approved with suggestions 0 resolved / 2 findings

Refactors Rust production panic-surface analysis to correctly flag unwrap and panic calls while excluding test and benchmark paths. Consider addressing brittle byte-matching for #[should_panic] and #[cfg(test)] attributes to prevent bypasses from whitespace or macro variants.

Auto-approved and auto-merge armed: No blocking issues found.
Please see Auto-approve Docs for details on setting custom approval criteria. — merges when pipeline and required approvals pass.

💡 Edge Case: should_panic stripping keys on attribute alone, enabling evasion

📄 src/assail/analyzer.rs:1679-1693

strip_should_panic_items_rs blanks the body of any item preceded by #[should_panic regardless of whether the function is actually a test (#[test]). Because rustc only warns (not errors) on a #[should_panic] attribute placed on a non-test fn, a production function annotated #[should_panic] fn f() { panic!() } — accidentally or via macro expansion — is silently removed from the production panic surface, producing a false negative for the recall contract this PR establishes. Consider only stripping when a #[test] attribute is also present on the same item, or documenting the accepted limitation.

💡 Quality: should_panic/cfg matching is brittle to attribute whitespace/variants

📄 src/assail/analyzer.rs:1686 📄 src/assail/analyzer.rs:1552 📄 src/assail/analyzer.rs:1609-1611

The byte matchers require the exact literals #[should_panic and #[cfg( with no interior whitespace, so formatter output or forms like #[ should_panic ], #[cfg_attr(test, should_panic)], or # [cfg(test)] are not recognized and their bodies are left in the production panic surface. This is a precision gap (extra findings) rather than a correctness bug and fails open, but is worth a normalizing pre-pass or a documented assumption that input is rustfmt-canonical.

🤖 Prompt for agents
Code Review: Refactors Rust production panic-surface analysis to correctly flag unwrap and panic calls while excluding test and benchmark paths. Consider addressing brittle byte-matching for `#[should_panic]` and `#[cfg(test)]` attributes to prevent bypasses from whitespace or macro variants.

1. 💡 Edge Case: should_panic stripping keys on attribute alone, enabling evasion
   Files: src/assail/analyzer.rs:1679-1693

   `strip_should_panic_items_rs` blanks the body of any item preceded by `#[should_panic` regardless of whether the function is actually a test (`#[test]`). Because rustc only warns (not errors) on a `#[should_panic]` attribute placed on a non-test fn, a production function annotated `#[should_panic] fn f() { panic!() }` — accidentally or via macro expansion — is silently removed from the production panic surface, producing a false negative for the recall contract this PR establishes. Consider only stripping when a `#[test]` attribute is also present on the same item, or documenting the accepted limitation.

2. 💡 Quality: should_panic/cfg matching is brittle to attribute whitespace/variants
   Files: src/assail/analyzer.rs:1686, src/assail/analyzer.rs:1552, src/assail/analyzer.rs:1609-1611

   The byte matchers require the exact literals `#[should_panic` and `#[cfg(` with no interior whitespace, so formatter output or forms like `#[ should_panic ]`, `#[cfg_attr(test, should_panic)]`, or `# [cfg(test)]` are not recognized and their bodies are left in the production panic surface. This is a precision gap (extra findings) rather than a correctness bug and fails open, but is worth a normalizing pre-pass or a documented assumption that input is rustfmt-canonical.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Important

Your trial ends in 1 day — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

Was this helpful? React with 👍 / 👎 | Gitar

@hyperpolymath
hyperpolymath merged commit 887ca86 into main Aug 9, 2026
6 of 7 checks passed
@hyperpolymath
hyperpolymath deleted the fix/issue-171-panic-surface branch August 9, 2026 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gitar-approved Added by Gitar

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exclude tests and benches from panic-surface analysis (false-positive class)

1 participant