Repository navigation
fix(panicbot): canonical PA rule IDs from panic-attack; test the --parallel wrapper and drain - #645
hyperpolymath wants to merge 4 commits into
Conversation
Groundwork for adding panicbot to the rust.yml CI matrix, where it has never been tested or linted: `cargo clippy -D warnings` failed on `MinSeverity::from_str` shadowing the std trait method (should_implement_trait), and rustfmt reported drift in seven files. MinSeverity now implements std::str::FromStr (callers use `.parse()`); the rest is `cargo fmt`. No behaviour change: 80 tests pass before and after. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01PpQVLrCbURpoXtrdvy8V3B
…n CI panicbot's translator numbered panic-attack categories with its own sequence: 20 of its 25 rule IDs disagreed with panic-attack's WeakPointCategory::rule_id (UnsafeCode PA001 vs PA004, CommandInjection PA003 vs PA009, HardcodedSecret PA004 vs PA017, ...). Hypatia deduplicates findings by these IDs and follows panic-attack's numbering (e.g. its `PA009-shell-unquoted-var` pattern), so panicbot filed findings under the wrong rules. - Vendor panic-attack's published table (data/canonical-rule-ids.json at 5175a68) into bots/panicbot/canon/, pinned by sha256 in pin.toml. - Correct the translator's rule IDs from that table. tests/canonical_rule_ids_test.rs checks every category against it; the five panic-attack categories panicbot does not translate yet (UnboundedAllocation, LicensePolicy, Advisory, BannedDep, UnknownSource) are listed explicitly, so a new upstream category fails the test until someone decides how to map it. - Fix-script headers cited six wrong PA codes (documentation only: routing is by category name). They now cite `PAnnn (Category)` from the table, and Hypatia pattern IDs are back-quoted so the two namespaces stay apart. tests/panic-attack-rule-ids-test.sh checks the pin digest and every reference (`--upstream` compares with panic-attack main). - Add panicbot to the rust.yml matrix (it was built, never tested or linted, in CI) and the new shell test to e2e.yml. Verified: panicbot 82 tests, clippy -D warnings, fmt; a reintroduced old ID (ResourceLeak PA009) fails canonical_rule_ids_test; a wrong header, an unquoted Hypatia ID and an edited table each fail the shell test; actionlint and check-lock-sync clean. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01PpQVLrCbURpoXtrdvy8V3B
Section 8 drove count/fold_counts directly, so dropping IN_JOB=1 from run_entry, or the fold_counts after the post-loop wait, left the suite green while --parallel summaries read zero (CodeRabbit on #622). Section 9 extracts the real run_entry and drain from dispatch-runner.sh, as section 4 extracts record_outcome, and runs five stubbed entries at --parallel 1 and 2. The main loop itself stays behind the automation quarantine (exit 78); this test does not bypass it. Both mutations above now fail the suite (41 checks). Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01PpQVLrCbURpoXtrdvy8V3B
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 32 minutes. View limit details
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. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@coderabbitai review Generated by Claude Code |
Rate Limit Exceeded
|
|
This PR has two red checks that aren't caused by its diff:
Generated by Claude Code |
|
There's a third red check that this PR doesn't cause: Main's workflows now call four reusables at Fixing this means deciding whether the metadatastician owner is trusted, so it belongs in the standards allowlist rather than in a gitbot-fleet PR. No fix exists yet. Generated by Claude Code |
|
A fourth red check doesn't come from this PR: Generated by Claude Code |
…#649) ## Summary `build · test · clippy (idris-dependents)` has failed on main since #628, and on every PR since (for example #645). 1. **Compile error.** `ready_mock()` in `tests/scan_tests.rs` predates the `responses` field on `Mock`, so the test crate failed with E0063 before any test ran. The field is now initialised empty, as the file's other two `Mock` literals already do. 2. **Two tests the compile error had hidden.** In `tests/scan_edge_tests.rs`, both tests encoded behaviour from before raw files returning HTTP 404/410/451 became *skipped URLs*. That contract is specified in the README, in the `get_file` doc comment, and in `scan_tests::unavailable_files_are_logged_reported_and_skipped_in_both_raw_fetch_paths`. - `raw_content_fetches_retry_rate_limits_and_propagate_hard_failures` expected a raw 404 to fail the run. Its hard-failure case now uses HTTP 500, which still propagates as `HTTP 500 for <url>`. The 404 skip stays covered by `scan_tests`. - `report_json_keeps_public_field_names_and_confidence_spelling` now includes the public `skippedUrls` field. Only test code changes; the product behaviour is untouched. ## Validation - I reproduced E0063 on a clean checkout of main. - All 79 idris-dependents tests pass across every test binary. - `cargo clippy --all-targets -D warnings` and `cargo fmt --check` are clean. - The first commit's message says "all tests pass". That was wrong when written: the two edge-test failures had been cut from the output I checked. The second commit fixes them and says so. --- _Generated by [Claude Code](https://claude.ai/code/session_01PpQVLrCbURpoXtrdvy8V3B)_ --------- Co-authored-by: Claude <[email protected]>
|
I've merged main into this branch (
Main is red on all of these. This PR still needs:
Generated by Claude Code |
|
Summary
Two follow-ups from the outcome-verdict work (#616, #622, #623) and from panic-attack#232.
1. panicbot filed findings under the wrong PA rules
panicbot's translator used its own numbering, and 20 of its 25 rule IDs disagreed with panic-attack's canonical
WeakPointCategory::rule_id:Only PA021–PA025 agreed. Hypatia deduplicates findings by these IDs, and its patterns follow panic-attack's numbering (for example
PA009-shell-unquoted-var). So every panicbot finding in PA001–PA020 was deduplicated against the wrong rule.The fix:
data/canonical-rule-ids.jsonat 5175a68, is vendored intobots/panicbot/canon/and pinned by sha256 inpin.toml. This follows the rhodibot canon pattern.tests/canonical_rule_ids_test.rschecks every category against it.PAnnn (Category), and Hypatia pattern IDs are back-quoted so the two namespaces can't be confused.tests/panic-attack-rule-ids-test.shchecks the pin digest and every reference. Its--upstreamflag compares the pin against panic-attack main.rust.ymlmatrix: it was built, but never tested or linted, in CI. Making it clippy-clean meant implementingFromStrforMinSeverityand applying rustfmt, in a separate, behaviour-neutral commit. The new shell test runs ine2e.yml.2. The
--parallelwrapper and drain had no test (CodeRabbit on #622/#623)Section 8 of
tests/outcome-verdict-test.shdrovecount/fold_countsdirectly. New section 9 extracts the runner's realrun_entryand its post-loopwait; fold_countsblock, the same way section 4 extractsrecord_outcome. It then runs five stubbed entries at--parallel 1and2.The main loop itself is not run. It sits behind the automation quarantine (exit 78, which per the owner ruling stays), and this test does not bypass that.
Validation
cargo clippy --all-targets -D warningsandcargo fmt --checkare clean.canonical_rule_ids_test.panic-attack-rule-ids-test.sh.IN_JOB=1fromrun_entry, orfold_countsfrom the drain, fails section 9.outcome-verdict-test.shpasses 41/41. Its ShellCheck findings are identical to main's.automation-quarantine,dispatch-pathsande2etests pass.actionlint1.7.7 is clean onrust.ymlande2e.yml, andcheck-lock-sync.shis clean.Deliberately not here
I didn't add
--checkpredicates tofix-heredoc-install.shandfix-todo-markers.sh, even though their recipes have the most outcome-log rows after the two that already have predicates.Whether they should stay routed as
auto_executefixers is a design question. It goes to a tracking issue instead.Generated by Claude Code