feat(harden): AST-based no-panic baseline detector (#187 PR 8a)#228
Conversation
`cargo xtask no-panic baseline` walks every tracked production source file
in the workspace (`crates/*/src/**/*.rs`), parses each via `syn`, and
classifies every panic-family call site by exact identity:
family selector_kind selector_callee where it matches
───────────── ───────────── ─────────────── ─────────────────
unwrap method_call unwrap .unwrap() on Option/Result
expect method_call expect .expect("...") on Option/Result
panic macro panic | panic_any panic!() / panic_any()
unreachable macro unreachable unreachable!()
todo macro todo todo!()
unimplemented macro unimplemented unimplemented!()
index syntax [] a[b] (Index trait)
Each finding is keyed by (path, family, selector_kind, selector_callee,
snippet) and counted; the deduplicated entries land in
policy/no-panic-baseline.json. AST-based detection avoids the regex
failure modes the planner flagged: `.lock().unwrap()` is correctly
attributed to its own call site, macro context is unambiguous, and
cfg-gated test blocks are skipped via attribute walking.
Test code is excluded from the production baseline per
docs/NO_PANIC_POLICY.md via three overlapping mechanisms:
- File-path exclusion: `tests/`, `benches/`, `examples/` directories.
- File-name exclusion: `tests.rs` and `*_tests.rs` files.
- AST attribute walk: `#[cfg(test)]` on `mod`/`impl`, `#[test]`/`#[bench]`
on `fn`. The visitor pushes/pops a depth counter as it enters/exits
these scopes; findings inside are tagged and filtered before
grouping.
This catches three classes of test code: dedicated test files, in-source
test modules, and `#[test]` functions inline in production files.
Current production surface (initial baseline):
files_scanned=50 files_with_findings=25
production_sites=59 entries=28 (dropped 1547 test-code sites)
unwrap: 48 index: 7 expect: 4
Concentrations are unsurprising: `.lock().unwrap()` patterns in
engine/parallel/ (Mutex poisoning is unhandled today), `serde_json::Value`
indexing in state/execution_state/mod.rs, and plan-side `.expect("pkg
exists")` invariants. The conversion sweep is out of scope for #187;
PR 8b wires the no-new-debt check and the release CI gate, and
subsequent PRs address the debt.
policy/no-panic-baseline.json new, marked linguist-generated=true
.gitattributes new, linguist-generated rule
policy/generated-allowlist.toml receipt for the new artifact
docs/NO_PANIC_POLICY.md updated: JSON not TOML; scope section
xtask/Cargo.toml syn 2 (full,visit,extra-traits)
+ proc-macro2 (span-locations)
xtask/src/no_panic.rs new — detector + visitor + writer
xtask/src/main.rs new `no-panic baseline` subcommand
CHANGELOG.md Added section entry
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Droid finished @EffortlessSteven's task —— View job Droid review complete. Validation result: No actionable findings. The AST-based no-panic baseline detector (PR 8a / #187) is sound. Inspected surfaces:
Checks performed:
Why no comments: Residual risk: Validation signal:
No review comments posted (no actionable findings). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef10758ae4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .arg(workspace_root) | ||
| .arg("ls-files") | ||
| .arg("-z") | ||
| .arg("crates/*/src/**/*.rs") |
There was a problem hiding this comment.
Scan top-level crate sources
Observed: this pathspec only matches files below a subdirectory of src, so files such as crates/shipper-cli/src/lib.rs, crates/shipper-core/src/lib.rs, and every single-file support crate are never scanned; git ls-files 'crates/*/src/**/*.rs' returns 62 files here, while git ls-files 'crates/*/src/*.rs' shows 86 tracked source files including the omitted top-level ones. Once the follow-up check uses this baseline, new production unwrap/panic! sites in those omitted files can be added without being reported, which defeats the no-new-debt gate. Include both crates/*/src/*.rs and recursive children, or use a path walk rooted at each crate's src.
Useful? React with 👍 / 👎.
| let family: Option<&'static str> = match name.as_str() { | ||
| "panic" | "panic_any" => Some("panic"), | ||
| "unreachable" => Some("unreachable"), | ||
| "todo" => Some("todo"), | ||
| "unimplemented" => Some("unimplemented"), | ||
| _ => None, |
There was a problem hiding this comment.
Handle panic_any as a function call
Observed: panic_any is listed here with macro invocations, but the Rust API is std::panic::panic_any(...), which syn represents as an ExprCall, not an ExprMacro; there is no panic_any! macro for this branch to catch. A new production panic_any(...) call will therefore be absent from the generated baseline/check results even though the policy explicitly lists panic_any() as part of the panic family. Add an ExprCall visitor for path calls whose last segment is panic_any.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
LGTM — no issues found. The implementation is sound: the AST-based detector correctly classifies panic-family call sites, the three-layer test-code exclusion is precise, file enumeration uses a hardcoded git pattern with no user input, no unsafe code is introduced, and the baseline correctly reflects 59 production sites across 28 grouped entries. Security review found no vulnerabilities.
The PR 8a baseline detector added .gitattributes (linguist-generated marker for policy/no-panic-baseline.json) but did not receipt it in non-rust-allowlist.toml. CI's blocking-allowlist check on .github/ workflows/ci.yml flagged it as 1 unreceipted file and failed the Policy job. Mirrors the .gitignore entry directly above it.
|
Droid finished @EffortlessSteven's task —— View job Droid Phase 2 validation complete. Validation result: 4 findings confirmed across
Inspected surfaces: Why no security issues: This PR adds an xtask-only tool with Post-processing: 4/4 validated review comments posted to PR #228. Security analysis performed. |
| baseline.counts.files_with_findings, | ||
| baseline.counts.total_call_sites, | ||
| baseline.counts.total_entries, | ||
| dropped_test_sites, |
There was a problem hiding this comment.
[P1] Incomplete file enumeration — pathspec misses top-level crate sources
The enumerate_source_files function uses crates/*/src/**/*.rs as a git pathspec. This pattern requires at least one subdirectory under src/, so it excludes files directly in crates/*/src/ such as crates/shipper-cli/src/lib.rs, crates/shipper-core/src/lib.rs, or any crates/*/src/main.rs. Verified locally: git ls-files 'crates/*/src/**/*.rs' returns 62 files versus 86 from git ls-files 'crates/*/src/*.rs' — a 24-file gap. Any top-level crate source files with panic-family calls will be missing from the baseline entirely, allowing new unwrap/panic! sites in those files to bypass the no-new-debt gate when PR 8b lands.
Fix direction: Change the enumeration to use two git ls-files calls (one for crates/*/src/*.rs, one for crates/*/src/**/*.rs) and deduplicate, or walk crates/*/src/ as a directory tree.
| ); | ||
| for (family, n) in &baseline.counts.by_family { | ||
| println!(" {family}: {n}"); | ||
| } | ||
| println!("wrote {}", BASELINE_PATH); | ||
| Ok(()) | ||
| } | ||
|
|
||
| // ─── File enumeration ────────────────────────────────────────────────────── | ||
|
|
||
| fn enumerate_source_files(workspace_root: &Path) -> Result<Vec<PathBuf>> { | ||
| let output = Command::new("git") | ||
| .arg("-C") | ||
| .arg(workspace_root) | ||
| .arg("ls-files") | ||
| .arg("-z") | ||
| .arg("crates/*/src/**/*.rs") |
There was a problem hiding this comment.
[P2] panic_any is listed as a macro but is actually a function call — never detected
std::panic::panic_any(...) is a function call, not a macro — syn represents it as ExprCall, not ExprMacro. The visit_expr_call method is not implemented on PanicVisitor, so panic_any(...) invocations are silently undetected despite being listed in the panic family in the policy doc.
Fix direction: Implement visit_expr_call on PanicVisitor and check if the callee path's last segment is panic_any.
| ); | ||
| for (family, n) in &baseline.counts.by_family { | ||
| println!(" {family}: {n}"); | ||
| } | ||
| println!("wrote {}", BASELINE_PATH); | ||
| Ok(()) | ||
| } | ||
|
|
||
| // ─── File enumeration ────────────────────────────────────────────────────── | ||
|
|
||
| fn enumerate_source_files(workspace_root: &Path) -> Result<Vec<PathBuf>> { | ||
| let output = Command::new("git") | ||
| .arg("-C") | ||
| .arg(workspace_root) | ||
| .arg("ls-files") | ||
| .arg("-z") | ||
| .arg("crates/*/src/**/*.rs") |
There was a problem hiding this comment.
[P3] cfg attribute test detection does not recurse beyond one level of any()/all()/not() nesting
In attr_implies_test, when the outer parse_nested_meta callback encounters any/all/not, it recurses via meta.parse_nested_meta(inner). However, the inner callback does not itself check for and recurse into further any/all/not calls. A pattern like #[cfg(any(all(test, feature = "x"), other))] would have its inner test missed because the inner callback never descends into the all(...).
Fix direction: In the inner callback, add the same if meta.path.is_ident("any") || meta.path.is_ident("all") || meta.path.is_ident("not") check that exists in the outer callback, and recurse.
| @@ -1,6 +1,6 @@ | |||
| # No-Panic Policy | |||
There was a problem hiding this comment.
[P1] Inconsistent file extension — TOML vs JSON
Line 14 of docs/NO_PANIC_POLICY.md reads receipted in \policy/no-panic-baseline.toml`but the actual baseline file ispolicy/no-panic-baseline.json. All other references in the same document correctly use .json`.
…#190) (#235) Refactors three safe-by-construction indexing operations to their idiomatic Rust equivalents, removing them from the no-panic baseline. All three were already provably safe — the panic surface was statically unreachable — but the new forms make that obvious and let the check recognise them as resolved. ops/cargo/mod.rs is_valid_package_name: replace the `name.is_empty()` guard + `chars: Vec<char>` collection + `chars[0]` indexing with a direct `chars.next()` returning Option<char>. Drops the Vec allocation as a small bonus. runtime/environment/mod.rs normalize_version: replace `.collect::<Vec<_>>()` + `.len() >= 2` guard + `[1]` with `.nth(1)`. Reduces a five-line function to one expression. plan/chunking/mod.rs chunk_by_max_concurrent: replace the hand-rolled index/while loop with the standard `slice::chunks(batch_size)`. Drops the `if items.is_empty()` guard since `[T]::chunks` on an empty slice is already a no-op. Baseline state: Before: 28 entries, 59 production sites by family: expect=4 index=7 unwrap=48 After: 25 entries, 55 production sites by family: expect=4 index=3 unwrap=48 `cargo xtask no-panic check` against the prior baseline correctly reported `resolved=3`, identifying each site by exact identity (path, family, selector_kind, selector_callee, snippet). After regenerating the baseline, `check` reports clean. This is the first burn-down validating the no-panic loop end-to-end. The issue's chosen target (shipper-duration) had zero production debt — all 53 sites flagged in the issue body were in test code, which the detector already excludes from the production baseline via the `*_tests.rs` / `#[cfg(test)]` filtering landed in #228. The real debt lives in shipper-core, so the burn-down moved there. Tests: cargo test -p shipper-core --lib passes (1900/1900).
Summary
Splits #187 into two PRs per the no-surprise-enforcement pattern. This PR (8a) ships the detector + baseline only; PR 8b will add the verify mode (
cargo xtask no-panic check) and wire the release CI gate.cargo xtask no-panic baselinewalks tracked production source viasyn, classifies every panic-family call site, groups by exact identity, and writespolicy/no-panic-baseline.json(markedlinguist-generated=true)..lock().unwrap(), macro context, and cfg-gated test blocks are classified correctly.tests//benches//examples/), filename (tests.rs/*_tests.rs), and AST attribute walk (#[cfg(test)]/#[test]/#[bench]).Initial baseline (committed)
```
no-panic baseline: files_scanned=50 files_with_findings=25
production_sites=59 entries=28
(dropped 1547 test-code sites)
unwrap: 48 index: 7 expect: 4
```
Distribution is concentrated where you'd expect —
.lock().unwrap()inengine/parallel/(Mutex poisoning is unhandled today),serde_json::Valueindexing instate/execution_state/, and a few plan-side.expect(\"pkg exists\")invariants. The 28 grouped entries are reviewable end-to-end.Why the data shape changed from the issue plan
docs/NO_PANIC_POLICY.md.test_codefield, but with 1,547 test-code sites versus 59 production sites, a mixed baseline would be 96% noise. The detector still walks all of it (so the cfg(test)/test-fn attribute decisions are honoured), then drops test findings before serialization.Edge cases — decisions baked in
.lock().unwrap()chained on a method.unwrap()positionpanic!()in amatcharmunreachable!()would be preferred per policy, but the planner decided it's still a panic family member until refactored)vec[i]with bounds-checkediindex)expect("msg").b.expect("msg")tempdir().expect(\"tempdir\")in#[test] fn#[test]triggers the cfg-test depth countertempdir().expect(\"tempdir\")in atests.rsfiletests.rs(no#[test]attr)vec![...],assert_eq!(...)).unwrap()inside is invisible. Documented limitation.Test plan
cargo build --workspace --all-targets— cleancargo clippy --workspace --all-targets -- -D warnings— clean (new ratchet activations from PR 7: Clippy 1.94/1.95 ratchets #191 also clean againstxtask/src/no_panic.rs)cargo fmt --all -- --check— cleancargo xtask no-panic baseline— runs in <2s, produces 28 entries, exits 0cargo xtask check-generated --mode advisory—policy/no-panic-baseline.jsonis receipted via the new[[file]]entry ingenerated-allowlist.tomlcargo xtask check-file-policy --mode advisory— covered by existingpolicy/**glob innon-rust-allowlist.tomlcargo xtask policy-report— all 7 areas cleanWhat lands in PR 8b (follow-up)
cargo xtask no-panic check— compare current vs baseline, fail on any new entrycargo xtask policy-reportshipper-release.yml) callingno-panic checkbefore publishattrs_imply_testandis_excluded_dir(deferred to PR 8b since the detector's behaviour is exercised by the baseline run)Refs #187. Closes nothing yet — #187 stays open until PR 8b lands.