chore(dev): make pre-tag-check.sh mirror the lint job; fix stale local-CI docs#6793
Open
TheHypnoo wants to merge 1 commit into
Open
chore(dev): make pre-tag-check.sh mirror the lint job; fix stale local-CI docs#6793TheHypnoo wants to merge 1 commit into
TheHypnoo wants to merge 1 commit into
Conversation
…tale local-CI docs pre-tag-check.sh opens by claiming it runs "the same fast lint gates that the Tests workflow's `lint` and `api-docs-drift` jobs run". That drifted: it ran fmt, a docs-fence lint and cargo check, while the lint job also gates on the 2000-line file cap, the GC store-site inventory, the address-classification audit, the benchmark harness tests and public artifact freshness — plus the clippy job added in #6754. Those are exactly the gates CLAUDE.md files under "CI gates that surprise people", and none were reachable locally from one command. - --quick is now every lint-job gate that compiles nothing (~10s), in the same order as test.yml so drift between the two is visible on sight. - The default tier adds clippy and cargo-deny alongside the existing cargo check. cargo-deny is optional: a missing tool skips cleanly rather than failing a contributor who lacks it. The gap-snapshot self-test is likewise guarded for checkouts predating #6755. - CONTRIBUTING.md's "Running the full CI suite locally" told contributors the parity suite "needs Node 22". Node is the oracle the suite diffs against, so its version is a correctness input — and Node 22 is the pin that classified 14 tests as node_fail and dropped them from the gate for months (#6364). The docs recommended the configuration that caused the incident. Rewritten around the script, with the pin explained. No new dependency: this is the existing script and the existing gates wired together. A justfile would have meant every contributor installing `just` to run scripts they can already run.
📝 WalkthroughWalkthroughThe pre-tag check script now uses separate non-compiling and compile-dependent tiers, with additional validation gates and optional ChangesPre-tag validation workflow
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@CONTRIBUTING.md`:
- Around line 102-106: Update the prerequisites table in CONTRIBUTING.md to
reference the Node.js version pinned in `.node-version` instead of prescribing
Node.js 22. Keep the surrounding guidance consistent with `.node-version` as the
authoritative contributor baseline.
- Around line 108-109: The contribution policy must require the mandatory
release metadata alongside the changelog fragment for PRs touching crates/.
Update the guidance near the changelog.d requirement to explicitly require a
Cargo.toml patch-version increment, a matching CLAUDE.md Current Version update,
and a new top-level CHANGELOG.md version block, or reconcile the wording so both
policies are consistent.
- Around line 82-92: Update the gate descriptions around
./scripts/pre-tag-check.sh --quick in CONTRIBUTING.md: state that --quick
excludes Clippy, clarify that the default run also includes the docs linter, and
note that cargo deny is skipped when unavailable. Keep the command examples and
remaining gate descriptions accurate.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ff83eed-1173-40fa-b3ed-5aff22523fd7
📒 Files selected for processing (2)
CONTRIBUTING.mdscripts/pre-tag-check.sh
Comment on lines
+82
to
+92
| Most red PRs come from gates that `cargo build` never looks at — a file | ||
| that crossed the 2000-line cap, fmt drift, a clippy deny-level lint, an | ||
| unbarriered GC store site. One command runs all of them: | ||
|
|
||
| ```bash | ||
| cargo build --release # All crates | ||
| ./run_parity_tests.sh # Perry vs Node parity (needs Node 22) | ||
| ./scripts/pre-tag-check.sh --quick | ||
| ``` | ||
|
|
||
| That is every gate in the `lint` job that doesn't compile anything, so it | ||
| costs seconds. Drop `--quick` to add `cargo check`, clippy, and cargo-deny; | ||
| the script prints every failure in one pass rather than one per push. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the quick/default gate descriptions.
--quick does not run Clippy, and the default run also adds the docs linter; cargo deny is skipped when unavailable. Update this text so contributors choose the intended gate.
Proposed wording
-Most red PRs come from gates that `cargo build` never looks at — a file
-that crossed the 2000-line cap, fmt drift, a clippy deny-level lint, an
-unbarriered GC store site. One command runs all of them:
+Most red PRs come from gates that `cargo build` never looks at — a file
+that crossed the 2000-line cap, fmt drift, or an unbarriered GC store site.
+Run the fast, non-compiling subset with:
-That is every gate in the `lint` job that doesn't compile anything, so it
-costs seconds. Drop `--quick` to add `cargo check`, clippy, and cargo-deny;
+That is every gate in the `lint` job that doesn't compile anything, so it
+costs seconds. Drop `--quick` to add the docs linter, `cargo check`, and
+Clippy; it also runs `cargo deny` when installed.📝 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.
Suggested change
| Most red PRs come from gates that `cargo build` never looks at — a file | |
| that crossed the 2000-line cap, fmt drift, a clippy deny-level lint, an | |
| unbarriered GC store site. One command runs all of them: | |
| ```bash | |
| cargo build --release # All crates | |
| ./run_parity_tests.sh # Perry vs Node parity (needs Node 22) | |
| ./scripts/pre-tag-check.sh --quick | |
| ``` | |
| That is every gate in the `lint` job that doesn't compile anything, so it | |
| costs seconds. Drop `--quick` to add `cargo check`, clippy, and cargo-deny; | |
| the script prints every failure in one pass rather than one per push. | |
| Most red PRs come from gates that `cargo build` never looks at — a file | |
| that crossed the 2000-line cap, fmt drift, or an unbarriered GC store site. | |
| Run the fast, non-compiling subset with: | |
🤖 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 `@CONTRIBUTING.md` around lines 82 - 92, Update the gate descriptions around
./scripts/pre-tag-check.sh --quick in CONTRIBUTING.md: state that --quick
excludes Clippy, clarify that the default run also includes the docs linter, and
note that cargo deny is skipped when unavailable. Keep the command examples and
remaining gate descriptions accurate.
Comment on lines
+102
to
+106
| **Use the Node version pinned in `.node-version`, not whatever you have.** | ||
| Node is the oracle the suite diffs against, so its version is a correctness | ||
| input: on a Node too old for a test, *node* fails, the harness classifies the | ||
| test `node_fail`, and it is dropped from the run instead of going red. CI sat | ||
| on Node 22 while the suite grew Node 24/26 features and hid 14 tests that way. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the conflicting Node 22 prerequisite.
The prerequisites table still prescribes Node.js 22, while this section says Node 22 silently skips parity coverage. Make the table reference .node-version so contributors cannot select a non-authoritative baseline.
🤖 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 `@CONTRIBUTING.md` around lines 102 - 106, Update the prerequisites table in
CONTRIBUTING.md to reference the Node.js version pinned in `.node-version`
instead of prescribing Node.js 22. Keep the surrounding guidance consistent with
`.node-version` as the authoritative contributor baseline.
Comment on lines
+108
to
+109
| Any PR touching `crates/` also needs a `changelog.d/<PR-number>-<slug>.md` | ||
| fragment; see [changelog.d/README.md](changelog.d/README.md). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Reconcile the fragment policy with mandatory release metadata.
This .md change lacks the required Cargo.toml patch bump, CLAUDE.md Current Version update, and new top-level CHANGELOG.md version block. A fragment-only requirement does not satisfy that policy; add the metadata changes or align the policies before merge.
As per coding guidelines, “For every change landing on main, increment the patch version in Cargo.toml and the Current Version line in CLAUDE.md, [and] add a new top-level version block to CHANGELOG.md.”
🤖 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 `@CONTRIBUTING.md` around lines 108 - 109, The contribution policy must require
the mandatory release metadata alongside the changelog fragment for PRs touching
crates/. Update the guidance near the changelog.d requirement to explicitly
require a Cargo.toml patch-version increment, a matching CLAUDE.md Current
Version update, and a new top-level CHANGELOG.md version block, or reconcile the
wording so both policies are consistent.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third of the base-hygiene changes from studying oxc (after #6754 and #6755). Their
just readyis a single "will CI pass?" command; Perry already had the pieces, but nothing tied them together and the docs pointed the wrong way.What was actually wrong
scripts/pre-tag-check.shopens by claiming it runs "the same fast lint gates that the Tests workflow'slintandapi-docs-driftjobs run". That stopped being true a while ago. It ran fmt, a docs-fence lint, andcargo check— while thelintjob also gates on the 2000-line file cap, the GC store-site inventory, the address-classification audit, the benchmark harness tests, and public-artifact freshness, plus the clippy job added in #6754. Those are exactly the gates CLAUDE.md files under "CI gates that surprise people", and none of them were reachable locally from one command.CONTRIBUTING.mdwas worse than incomplete. Its "Running the full CI suite locally" section told contributors./run_parity_tests.sh # (needs Node 22). Node is the oracle the suite diffs against, so its version is a correctness input — and Node 22 is precisely the pin that classified 14 tests asnode_failand silently dropped them from the gate for months (#6364). The docs were recommending the configuration that caused the incident.The change
pre-tag-check.shnow mirrors thelintjob, split so the cheap half is the habitual one:--quick(~10s) — every lint-job gate that compiles nothing: fmt, benchmark harness tests, public-artifact freshness, file-size cap, GC store-site inventory, address-classification audit, and (once test(parity): bidirectional snapshot ratchet for the gap suite (#797) #6755 lands) the gap-snapshot self-test, which is guarded so this script also works on a checkout predating it. Kept in the same order astest.ymlso drift between the two is visible on sight.cargo check --release,cargo clippy, andcargo deny check. cargo-deny is optional: missing tool skips cleanly rather than failing a contributor who lacks it.--thorough— unchanged.The existing all-failures-then-exit behavior is preserved, so one run surfaces everything instead of trickling one failure per push.
CONTRIBUTING.md's section is rewritten around it, with the Node pin explained as a correctness input rather than a toolchain detail, and thechangelog.d/fragment requirement spelled out.No new dependency: this is the existing script and the existing gates, wired together. A
justfilewould have meant every contributor installingjustto run scripts they can already run.Validation
--quickrun end-to-end: 7 gates execute in CI order, exit 1 on failure, unknown flags still exit 2. It immediately reproduced thepublic benchmark evidence freshnessfailure that is currently red onmain— which is the point: that gate went red in #6754 and only surfaced after pushing.Two things it surfaced, not fixed here
main's lint job is red. The freshness check fingerprints the rootCargo.toml, so chore(workspace): workspace lint policy, clippy + cargo-deny CI gates, dep unification #6754's[workspace.lints]/[workspace.dependencies]edits invalidated the public benchmark artifact. It needs./benchmarks/run_public_baseline.shre-run on the benchmark machine and committed. It matters more than one red check: the step aborts the job before the file-size, GC store-site, and address-classification gates run, so those are currently not executing on any PR.buffer/from.rs10→9,object_ops_frozen.rs8→7,symbol/constructors.rs3→2,thread.rs13→10. Sites were fixed without recording it. The audit passes (it is a ratchet) and prints the fix,python3 scripts/addr_class_inventory.py --write-baseline; left out of this PR to keep the diff to tooling. Worth noting it is the same "improvements must be recorded" problem test(parity): bidirectional snapshot ratchet for the gap suite (#797) #6755 solves for the gap suite.Summary by CodeRabbit
Documentation
Chores