feat(harden): no-panic check + release CI gate (#187 PR 8b)#229
Conversation
`cargo xtask no-panic check` re-runs the AST detector landed in PR 8a,
compares the fresh result against `policy/no-panic-baseline.json` keyed
by (path, family, selector_kind, selector_callee, snippet), and reports
the diff in four buckets:
new entries new panic-family debt; fails the gate
count increases same site, more occurrences; fails the gate
resolved entries debt that no longer exists; reported, never fails
count decreases same site, fewer occurrences; reported, never fails
`--mode advisory` writes the report to target/policy/no-panic-report.json
without bailing — used by `cargo xtask policy-report` which now includes
no-panic as an eighth advisory area in its unified output. `--mode
blocking` (the CLI default for the `check` subcommand) exits non-zero on
any violation. A new `policy-gate` job in `.github/workflows/release.yml`
runs `cargo xtask no-panic check --mode blocking` plus
`check-lint-policy` before `publish-crates-io`, so a tag push cannot
publish if either ledger has drifted since the SHA the baseline was
generated at.
The detector now ships with 15 unit tests covering:
- attribute classification: #[test], #[bench], cfg(test), cfg(any(
test, ...)), cfg(all(..., test)), and explicitly cfg(not(test))
which is *not* test code despite mentioning `test` (regression
fixed during PR 8b — the previous heuristic recursed into not(...)
and misclassified production-only sites)
- directory exclusions: tests/, benches/, examples/
- filename exclusions: tests.rs and *_tests.rs
- production-source negative cases (engine/mod.rs, lib.rs,
testable_seam.rs)
`scan_and_group` is now a shared helper called by both `baseline()`
(write to disk) and `check()` (compare to disk). That keeps the
production-vs-test classification identical at baseline-time and
check-time — drift would have been a subtle source of false-positive
"new debt" findings.
The baseline file did not change as part of this PR (current_entries
matches baseline_entries; 0 new, 0 resolved, 0 count_increase, 0
count_decrease).
xtask/src/no_panic.rs shared scan_and_group helper,
check(mode) with diff + report writer,
attr_implies_test fix for cfg(not(test)),
15 unit tests
xtask/src/main.rs new `no-panic check` subcommand with
--mode flag (default Blocking)
xtask/src/policy_report.rs no-panic registered as 8th area,
`violations` added to headline priority
.github/workflows/release.yml
new `policy-gate` job; publish-crates-io
depends on msrv-gate AND policy-gate
CHANGELOG.md Added entry; #187 closed
|
Droid finished @EffortlessSteven's task —— View job Droid Review — Phase 2 Validation CompletePR #229 ( Result: ✅ Approved — zero findings Validation performed:
Why no comments: Residual risk: Validation signal:
|
|
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 selected for processing (5)
✨ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e623abaeac
ℹ️ 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".
| } else if meta.path.is_ident("any") || meta.path.is_ident("all") { | ||
| let _ = meta.parse_nested_meta(|inner| { |
There was a problem hiding this comment.
Recurse through nested cfg test predicates
Observed in the changed attr_implies_test logic: the parser now only checks direct children of any(...)/all(...), so test-only blocks behind nested predicates such as #[cfg(all(unix, any(test, feature = "foo")))] mod tests { ... } are classified as production. With the new release no-panic gate, unwraps in those test-only modules become false violations or get baselined as production debt, despite docs/NO_PANIC_POLICY.md excluding #[cfg(test)] subtrees. Fix direction: make the any/all predicate walk recursive while still treating not(test) as non-test, and validate with an added nested-any/all unit test.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request implements the "No-panic check" and release CI gate. It introduces the cargo xtask no-panic check command, which compares a fresh scan of panic-family call sites against a stored baseline in policy/no-panic-baseline.json. The implementation includes logic to categorize changes as new debt, resolved entries, or count fluctuations, and supports both advisory and blocking modes. Additionally, the PR refactors the scanner to share logic between baseline generation and checking, fixes a regression in cfg(not(test)) classification, and adds unit tests for the detector's exclusion rules. I have no feedback to provide.
) #189 was filed with a richer lane-routing proposal (e.g. moving cross-platform from every-PR to labeled+nightly). That work is deferred because: - Current PR wall-clock is ~24-28 min (recent CI runs from this rollout), close to the original 25-min target without aggressive routing. - Shipper has Windows-specific code paths (path handling, process spawning, line endings). Moving Windows/macOS/aarch64 builds to nightly means platform regressions surface a day later instead of inside the PR — that is a real loss for a release-pipeline product. - Coverage-removing routing changes are hard to undo: contributors stop expecting platform signal, regressions accumulate, then "fixing" the lanes becomes a multi-PR cleanup. So #189 lands as a documentation pass: capture the actual current state of .github/workflows/ so future routing decisions have an accurate baseline. Changes to docs/ci/test-evidence-lanes.md: - Replace the conflated "Always-On" table with a complete workflow inventory: 10 workflows, ~28 jobs across them, columns for trigger / lane / required-for-merge. - Per-job lane map for ci.yml (the load-bearing PR workflow), with predicates, observed wall-clock, and what each job proves. Previous doc was missing fuzz-smoke, cross-platform, release-build, crypto- proptests-heavy, and policy entirely; conflated `lint` into separate `fmt`/`clippy` rows. - Policy gates section now lists all eight xtask checks (file-policy, generated, executable, dependency-surface, workflow, process, network, lint-policy) plus the release-time no-panic check, with the PR each was introduced in. - Advisory/Routed section adds droid-review and droid (was missing). - Scheduled section corrects mutation to "Sunday 04:00 UTC" (was "nightly") and adds droid-security-scan. - Release Proof: full release.yml job table including the policy-gate job added in #229. - New "Routing Changes Deferred to Follow-Up PRs" section enumerates concrete movements with rationale: release-build to release-only, path-filtered fuzz-smoke, split cross-platform so only Linux is every-PR. No workflow files changed. Coverage-removing routing decisions deserve their own focused PRs. CHANGELOG.md Added entry under Documentation docs/ci/test-evidence-lanes.md ~180 lines rewritten
Summary
Closes #187 by wiring the verify mode and release-time enforcement around the baseline detector that landed in PR 8a (#228).
cargo xtask no-panic checkre-runs the AST detector, compares fresh findings topolicy/no-panic-baseline.jsonkeyed by(path, family, selector_kind, selector_callee, snippet), and reports diffs in four buckets:--mode advisorywritestarget/policy/no-panic-report.jsonwithout bailing. Used bycargo xtask policy-report, which now includes no-panic as an eighth advisory area in the unified output.--mode blocking(the CLI default forcheck) bails non-zero on any violation.policy-gatejob in.github/workflows/release.ymlrunscargo xtask no-panic check --mode blockingandcargo xtask check-lint-policy.publish-crates-iodepends on bothmsrv-gateandpolicy-gate, so a tag push cannot publish if either ledger has drifted since the SHA the baseline was generated at.scan_and_groupis now a shared helper called by bothbaseline()andcheck(). Keeps the production-vs-test classification identical at baseline-time and check-time; drift between the two would have been a subtle false-positive source.#[test]/#[bench]/#[cfg(test)]/#[cfg(any(test, ...))]/#[cfg(all(..., test))]/#[cfg(not(test))]classifications and thetests.rs/*_tests.rs/tests//benches//examples/filename and directory exclusions.Regression caught during implementation
The previous
attrs_imply_testrecursed intocfg(not(test))and misclassified it as test code. That heuristic was technically a "any mention oftestanywhere in the cfg" rule, which inverts the meaning ofcfg(not(test))(production-only code). Fixed by walking onlyany(...)andall(...), notnot(...). Theattrs_cfg_not_test_is_not_testunit test guards against re-regression.PR 8a's baseline was generated before the fix, but the production source happened to contain no
cfg(not(test))sites, so the baseline file did not change as part of PR 8b. (Verified:current_entries == baseline_entries == 28with 0 new, 0 resolved, 0 count_increase, 0 count_decrease.)Test plan
cargo build -p xtask— cleancargo test -p xtask— 15/15 passingcargo clippy --workspace --all-targets -- -D warnings— cleancargo fmt --all -- --check— cleancargo xtask no-panic check— clean (28/28 entries match baseline)cargo xtask no-panic check --mode advisory— writes report, returns 0 even with synthetic violationscargo xtask policy-report— 8 areas, all cleanpolicy-gatejob hopefully not triggered since this isn't a tag push — but every other check should pass)Followups
.lock().unwrap()inengine/parallel/(Mutex poisoning unhandled) andserde_json::Valueindexing instate/execution_state/. First debt burn-down (per the rollout doc) will reduce that count.policy-gatejob's release-side enforcement is the first gate that runs ONLY on tag push. PR-time CI doesn't run it; a future PR can mirrorcargo xtask no-panic check --mode blockinginto the PR-time Policy job so drift is caught earlier.Closes #187.