Conversation
Adds a GitHub Actions job that runs the cross-crate dead-code analysis configured in hawk.toml on PRs touching Rust. main currently has 8784 findings, so the job analyzes both the base commit and the PR and fails only on findings the PR adds, reporting them as inline annotations and in the job summary. scripts/rust-hawk.ts wraps the procedure from tools/hawk/README.md (scaffold the bun_bin analysis root, run cargo hawk check, revert), builds cargo-hawk from the pinned oven-sh/hawk rev that understands this config, and implements the report comparison. The rust problem matcher is widened to accept hawk's bracketed lint codes. Six hawk.toml overrides that pointed at enum variants removed in #36833 are dropped; hawk was reporting them as unknown items.
WalkthroughAdded a Bun-based Hawk CLI and GitHub Actions workflow. The workflow analyzes base and PR revisions, compares JSON findings, and reports newly introduced diagnostics. Documentation, toolchain guidance, matcher behavior, and Hawk overrides were updated. ChangesHawk analysis
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 74: Hawk tooling commands currently invoke the normal Bun binary instead
of the required debug build. Update package.json lines 74-74,
tools/hawk/README.md lines 15-18, .github/workflows/hawk.yml lines 98-98,
107-110, 115-118, and 125-125, and .github/workflows/CLAUDE.md lines 60-60 to
use the corresponding “bun bd” form for every Hawk installation, configuration,
analysis, reporting, and documented command.
In `@scripts/rust-hawk.ts`:
- Around line 65-68: Strengthen readReport to validate the complete Hawk report
contract: require schema_version to be a safe integer and validate every
required field and numeric/string representation in each diagnostic before
returning the report. Wrap file-read and JSON-parse failures with the report
path, original cause, and a concrete command remedy, and add regression coverage
for malformed reports, including missing or invalid schema_version and
diagnostic fields.
- Around line 211-226: Restructure the scaffold lifecycle around the main flow
so the try/finally begins before the conditional git apply, ensuring cleanup
runs if patching or later setup fails. Register SIGTERM with the same
non-default handling as SIGINT, and make cleanup failures from the reverse git
apply or rootRsPath removal update the final status to nonzero instead of being
ignored; preserve successful cleanup and cargo status behavior.
- Line 40: Update the scaffoldPatch path in scripts/rust-hawk.ts to resolve
analysis-root.patch from repoRoot, producing an absolute path before passing it
to git apply; keep the existing tools/hawk location unchanged.
- Around line 123-125: Update the introduced-finding logic in the loop over
headByKey to append only entries after the existing base count, slicing each
entries array from baseCounts.get(key) ?? 0 rather than appending all entries.
Add an automated regression test covering a key occurring once in the base and
twice in the head, asserting that exactly one instance is reported.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74c445e8-d940-413b-bbad-aef82eac4928
📒 Files selected for processing (7)
.github/rust-matcher.json.github/workflows/CLAUDE.md.github/workflows/hawk.ymlhawk.tomlpackage.jsonscripts/rust-hawk.tstools/hawk/README.md
💤 Files with no reviewable changes (1)
- hawk.toml
| "rust:check": "cargo check --workspace --keep-going", | ||
| "rust:check-all": "bun scripts/rust-check-all.ts", | ||
| "rust:clippy": "cargo clippy --workspace --no-deps --keep-going", | ||
| "rust:hawk": "bun scripts/rust-hawk.ts", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Run Hawk tooling through the Bun debug build.
These changed commands use normal Bun invocation. They can run a release binary instead of the required debug build.
package.json#L74-L74: change therust:hawkscript to use thebun bdform.tools/hawk/README.md#L15-L18: document the correspondingbun bdcommands..github/workflows/hawk.yml#L98-L98: run the temporary Hawk installer throughbun bd..github/workflows/hawk.yml#L107-L110: run install, configuration, and base analysis throughbun bd..github/workflows/hawk.yml#L115-L118: run install, configuration, and PR analysis throughbun bd..github/workflows/hawk.yml#L125-L125: run report comparison throughbun bd..github/workflows/CLAUDE.md#L60-L60: document thebun bdinstallation command.
As per coding guidelines, “Run Bun commands and tests through the debug build (bun bd ...).”
📍 Affects 4 files
package.json#L74-L74(this comment)tools/hawk/README.md#L15-L18.github/workflows/hawk.yml#L98-L98.github/workflows/hawk.yml#L107-L110.github/workflows/hawk.yml#L115-L118.github/workflows/hawk.yml#L125-L125.github/workflows/CLAUDE.md#L60-L60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 74, Hawk tooling commands currently invoke the normal
Bun binary instead of the required debug build. Update package.json lines 74-74,
tools/hawk/README.md lines 15-18, .github/workflows/hawk.yml lines 98-98,
107-110, 115-118, and 125-125, and .github/workflows/CLAUDE.md lines 60-60 to
use the corresponding “bun bd” form for every Hawk installation, configuration,
analysis, reporting, and documented command.
Source: Coding guidelines
| const repoRoot = process.cwd(); | ||
| const cargoTomlPath = join(repoRoot, "src", "bun_bin", "Cargo.toml"); | ||
| const rootRsPath = join(repoRoot, "src", "bun_bin", "hawk_root.rs"); | ||
| const scaffoldPatch = join("tools", "hawk", "analysis-root.patch"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use an absolute path for analysis-root.patch.
Line 40 passes a relative file path to git apply. Build this path from repoRoot so the operation does not depend on the child process working directory.
Proposed fix
-const scaffoldPatch = join("tools", "hawk", "analysis-root.patch");
+const scaffoldPatch = join(repoRoot, "tools", "hawk", "analysis-root.patch");As per coding guidelines, “Use absolute paths in file operations.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const scaffoldPatch = join("tools", "hawk", "analysis-root.patch"); | |
| const scaffoldPatch = join(repoRoot, "tools", "hawk", "analysis-root.patch"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/rust-hawk.ts` at line 40, Update the scaffoldPatch path in
scripts/rust-hawk.ts to resolve analysis-root.patch from repoRoot, producing an
absolute path before passing it to git apply; keep the existing tools/hawk
location unchanged.
Source: Coding guidelines
| function readReport(path: string): Report { | ||
| const report = JSON.parse(readFileSync(path, "utf8")); | ||
| if (!Array.isArray(report?.diagnostics)) throw new Error(`${path} is not a hawk JSON report`); | ||
| return report; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the complete Hawk report contract.
Line 67 only validates that diagnostics is an array. Two reports containing {"diagnostics":[]} pass the schema comparison because both schema_version values are undefined, and the command exits successfully.
Validate schema_version as a safe integer and validate every diagnostic field before comparison. Wrap read and JSON parse failures with the report path, cause, and a command remedy. Add malformed-report regression coverage.
As per coding guidelines, “Validate numeric and string representations at every boundary” and “Error messages must identify the failed resource, violated constraint, rejected value, cause, and concrete remedy.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/rust-hawk.ts` around lines 65 - 68, Strengthen readReport to validate
the complete Hawk report contract: require schema_version to be a safe integer
and validate every required field and numeric/string representation in each
diagnostic before returning the report. Wrap file-read and JSON-parse failures
with the report path, original cause, and a concrete command remedy, and add
regression coverage for malformed reports, including missing or invalid
schema_version and diagnostic fields.
Source: Coding guidelines
| for (const [key, entries] of headByKey) { | ||
| if (entries.length > (baseCounts.get(key) ?? 0)) introduced.push(...entries); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Append only the introduced diagnostic instances.
Line 124 appends every head entry when a key count increases. If base contains one occurrence and head contains two, the command reports two introduced findings instead of one.
Append only the entries beyond the base count. Add a regression test for this one-to-two occurrence case.
Proposed fix
for (const [key, entries] of headByKey) {
- if (entries.length > (baseCounts.get(key) ?? 0)) introduced.push(...entries);
+ const baseCount = baseCounts.get(key) ?? 0;
+ if (entries.length > baseCount) introduced.push(...entries.slice(baseCount));
}As per coding guidelines, “Every behavioral change must include an automated regression test in the same change.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const [key, entries] of headByKey) { | |
| if (entries.length > (baseCounts.get(key) ?? 0)) introduced.push(...entries); | |
| } | |
| for (const [key, entries] of headByKey) { | |
| const baseCount = baseCounts.get(key) ?? 0; | |
| if (entries.length > baseCount) introduced.push(...entries.slice(baseCount)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/rust-hawk.ts` around lines 123 - 125, Update the introduced-finding
logic in the loop over headByKey to append only entries after the existing base
count, slicing each entries array from baseCounts.get(key) ?? 0 rather than
appending all entries. Add an automated regression test covering a key occurring
once in the base and twice in the head, asserting that exactly one instance is
reported.
Source: Coding guidelines
| const alreadyScaffolded = readFileSync(cargoTomlPath, "utf8").includes("bun_hawk_root"); | ||
| const createdRootRs = !existsSync(rootRsPath); | ||
| if (!alreadyScaffolded && run(["git", "apply", scaffoldPatch]) !== 0) { | ||
| console.error(`[hawk] failed to apply ${scaffoldPatch}`); | ||
| process.exit(1); | ||
| } | ||
| if (createdRootRs) writeFileSync(rootRsPath, "fn main() {}\n"); | ||
|
|
||
| // Ctrl-C goes to cargo too; let it die and fall through to the revert below. | ||
| process.on("SIGINT", () => {}); | ||
| let status = 1; | ||
| try { | ||
| status = run(["cargo", "hawk", "check", ...args], { env: { BUN_CODEGEN_DIR: codegenDir } }); | ||
| } finally { | ||
| if (!alreadyScaffolded) run(["git", "apply", "--reverse", scaffoldPatch]); | ||
| if (createdRootRs) rmSync(rootRsPath, { force: true }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Complete scaffold cleanup on every failure path.
If line 217 fails after git apply succeeds, execution never enters the try block and leaves Cargo.toml patched. SIGTERM also uses the default process termination path, so the finally block does not run. Line 225 ignores a failed reverse patch application.
Enter try before applying the patch. Handle SIGTERM like SIGINT. Return a nonzero result when reverse application or file removal fails.
As per coding guidelines, “Every error, abort, and timeout path must complete the operation” and “Never swallow failures.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/rust-hawk.ts` around lines 211 - 226, Restructure the scaffold
lifecycle around the main flow so the try/finally begins before the conditional
git apply, ensuring cleanup runs if patching or later setup fails. Register
SIGTERM with the same non-default handling as SIGINT, and make cleanup failures
from the reverse git apply or rootRsPath removal update the final status to
nonzero instead of being ignored; preserve successful cleanup and cargo status
behavior.
Source: Coding guidelines
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I also checked whether the scaffolding in scripts/rust-hawk.ts could be left applied on early failure — git apply and writeFileSync(rootRsPath) run before the try/finally, but the write is a 15-byte file into an existing directory and the CI checkout is ephemeral, so this was ruled out as a practical concern.
Extended reasoning...
The scaffolding-cleanup ordering was raised by a finder and refuted: the window between git apply succeeding and entering the try block is a single writeFileSync of fn main() {}\n into src/bun_bin/ (which must already exist for the patch to have applied), and the SIGINT handler is only needed once the long-running cargo hawk check starts. Locally a mid-window failure would leave the patch applied, but that is recoverable and not a CI concern.
| for (const [key, entries] of headByKey) { | ||
| if (entries.length > (baseCounts.get(key) ?? 0)) introduced.push(...entries); | ||
| } |
There was a problem hiding this comment.
🟡 When head count for a key exceeds base count, introduced.push(...entries) pushes all head entries for that key rather than only the excess — asymmetric with the fixed counter just above, which correctly computes the delta. If an item already has 2 dead cfg alternatives on base and a PR adds a 3rd, the job reports "3 finding(s) introduced" with 3 annotations instead of 1 (exit code is still correct). Fix: introduced.push(...entries.slice(baseCounts.get(key) ?? 0)).
Extended reasoning...
What the bug is
diffReports in scripts/rust-hawk.ts builds the introduced list by iterating headByKey and, for any key whose head count exceeds its base count, pushing every head entry for that key:
for (const [key, entries] of headByKey) {
if (entries.length > (baseCounts.get(key) ?? 0)) introduced.push(...entries);
}This is asymmetric with the fixed computation immediately above it, which correctly counts only the delta:
for (const [key, count] of baseCounts) {
const remaining = headByKey.get(key)?.length ?? 0;
if (remaining < count) fixed += count - remaining;
}Why the multi-entry case matters
The comment on diagnosticKey explicitly documents that counting (rather than set membership) is used because "An item compiled from several cfg alternatives yields one entry per alternative, hence counting." So the multi-entry-per-key case is the very scenario the counting scheme was designed to handle — and the introduced side doesn't follow through on it.
Step-by-step proof
Suppose bun_foo::bar (function) is dead on both a Windows and a macOS cfg alternative on base, and a PR adds a Linux alternative that is also dead:
- Base report: 2 diagnostics with key
hawk::dead_public|bun_foo|bar|function→baseCounts.get(key) = 2. - Head report: 3 diagnostics with the same key →
headByKey.get(key).length = 3. entries.length (3) > baseCount (2)is true, sointroduced.push(...entries)pushes all 3 entries.- The loop then prints 3
error[hawk::dead_public]:lines (3 inline PR annotations via the rust matcher) and the summary reportshawk: 3 finding(s) introduced. - In reality only 1 new finding was introduced. The
fixedside would have correctly reported1in the mirror scenario (3→2).
Why nothing prevents it
There is no dedup or delta step downstream: introduced.length is used verbatim for the console line, the GITHUB_STEP_SUMMARY header, and the bullet list, and each entry is printed as its own error[...] block. The exit code (introduced.length > 0 ? 1 : 0) remains correct — there IS at least one new finding — so pass/fail is unaffected.
Impact
Inflated "N finding(s) introduced" count and duplicate inline annotations on the PR when a PR adds a cfg alternative to an item that is already flagged on base. This is an advisory check and the trigger scenario is narrow, so the practical impact is limited to noise/confusion for the PR author, not incorrect gating.
Fix
Push only the excess entries. Since same-key entries are indistinguishable by identity, an arbitrary slice is fine:
for (const [key, entries] of headByKey) {
const baseCount = baseCounts.get(key) ?? 0;
if (entries.length > baseCount) introduced.push(...entries.slice(baseCount));
}Alternatively, keep printing all locations for context but compute the summary count from the delta rather than introduced.length.
This branch has not been deployed
2 participants
What
Runs the cross-crate dead-code analysis set up in #36184 (hawk,
hawk.toml) in GitHub Actions on every PR that touches Rust, sopubitems a PR adds that nothing in the shipped binary reaches on any of the 11 targets (or that are more visible than needed) show up on the PR as a failed check with inline annotations, instead of waiting for the next manual sweep.Why a base/PR comparison
main is not hawk-clean. A full run on
827475ereports 8784 diagnostics: 1222dead_public, 855unnecessary_public, 6701unnecessary_restricted_visibility, plus 6unknown_itemconfig warnings (#36184 did the narrowing and left the dead-public deletions for later, and the tree has moved since). So-D warningswould be red on every PR. Instead the job analyzes the base commit (HEAD^1of the merge commit) and the PR, and fails only on findings present in the PR and not in the base.The comparison keys a finding on lint + crate + item path + item kind (counted, since cfg alternatives of one item produce one entry each). hawk's own
identity.idembeds file:line, which would flag every finding below an unrelated edit as new. Moving or renaming an already-flagged item does count as new; that seemed acceptable for an advisory check.Changes
.github/workflows/hawk.yml: clippy.yml's setup, then buildscargo-hawk, analyzes base and PR, and runs the comparison. Not added tomerge_group; this is an advisory check, not a required one, so a PR that intentionally adds code ahead of its caller (stacked PRs) still shows red but stays mergeable. For code that is dead on purpose, an[[override]]withlevel = "expect"inhawk.tomlsilences it and hawk later reports the override as unfulfilled once the code gains a user.scripts/rust-hawk.ts(bun run rust:hawk): the procedure fromtools/hawk/README.mdas a script, used locally and by CI. Appliestools/hawk/analysis-root.patchand an emptyhawk_root.rsfor the duration of the run and reverts them (the extra crate-type cannot live in the tree because it would disable fat LTO on the shipped staticlib).installbuildscargo-hawk;diff base.json head.jsondoes the comparison and prints new findings in rustc format. It operates on the current directory so CI can run the PR's copy of the script against the base checkout (which, for this PR, does not have the script yet).bunbranch of oven-sh/hawk (c9ca301).hawk.tomlusestargets,cargo-profileand[[root-marker]], which upstream releases do not have (multi-target is Analyze and fix across a configured list of compilation targets astral-sh/hawk#150, still open), and the fork's one prebuilt release predates the last two. The driver links rustc internals, so the job builds it against the workspace's pinned nightly (rustc-devcomponent; about 15s)..github/workflows/CLAUDE.mdgets the corresponding toolchain-bump step..github/rust-matcher.json: the first pattern only acceptederror[E0xxx]:; hawk's codes look likeerror[hawk::dead_public]:. Widened to any bracketed code; rustc and clippy lines match exactly as before. The matcher is registered only for the comparison step so annotations never come from compiler output of either analysis.hawk.toml: drops six overrides forbun_platformdarwin::Categoryvariants that Remove dead code from platform/darwin, webcrypto, sqlite, NodeVM, ast #36833 deleted; hawk reports them ashawk::unknown_itemtoday, and this job would have flagged the PR that added them.Cost
One analysis is a
cargo checkof the workspace per target, 11 targets, production and test surfaces: 21.5 min on a 16-core box at an average of only 2.7 cores busy (the critical path isbun_runtime), soubuntu-latestshould not be dramatically slower. hawk re-instruments every workspace crate on each run, so the PR analysis does not reuse the base one; expect roughly 45 min per job, running alongside the Buildkite pipeline. Target dir is about 4 GB. If that turns out to be too much, caching the base report keyed by base sha would roughly halve it; left out of this PR.Verification
Base run on
827475e(this branch's merge base): 8784 diagnostics, as above.Appended an unused
pub fntosrc/csrf/lib.rsand re-ran through the script (scaffolding applied and reverted cleanly, JSON on stdout parsed).bun run rust:hawk diffon the two reports:exit code 1. The "6 fixed" are the stale overrides removed in this PR (the second run used the updated
hawk.toml), which also exercises config diagnostics flowing through the comparison. Identical reports, and reports differing only in line numbers, produce0 introducedand exit 0.The
Hawkworkflow run on this PR exercises the job itself (hawk.ymlis in its own trigger paths); it should finish with0 finding(s) introduced, 6 fixed.no test proof · iteration 0 · docs-only change; test-proof not applicable