test: commit gap-suite runner + fix stale CLAUDE.md /tmp pointer#5222
Merged
Conversation
Add scripts/run_gap_tests.sh — a committed runner for the gap suite (test-files/test_gap_*.ts, 235 tests), replacing the out-of-repo /tmp/run_gap_tests.sh that CLAUDE.md pointed at. It is a thin wrapper over run_parity_tests.sh --filter test_gap_, so it reuses the one canonical normalizer, skip-list, and output cap (seed of the single-normalizer work), and gates on "no NEW failures vs known_failures.json" rather than run_parity_tests.sh's loose <80%-aggregate exit. Wire it into a new smoke-parity CI job. The gap suite is the highest-signal- per-second test Perry has and had no committed runner and no PR gate — a single-feature regression could merge green. The job is INFORMATIONAL for now (continue-on-error): the first runs surface which gap tests fail on the Linux image under node 26 so they can be triaged into known_failures.json; once curated + green, a follow-up drops continue-on-error and branch protection makes it required. Uses node 26 (the node-suite baseline oracle) and only allow-listed actions (no sccache), so it is not blocked by the org action allow-list. Also fix the stale CLAUDE.md parity-status line (28 -> 235 tests, /tmp -> scripts/run_gap_tests.sh). No source changes; no existing job altered.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesGap test smoke gate
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 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 @.github/workflows/test.yml:
- Around line 284-295: Replace all mutable version tags with full commit SHAs
for the four GitHub Actions used in the smoke-parity job in the test.yml
workflow file. Update actions/checkout from `@v6` to its commit SHA,
dtolnay/rust-toolchain from `@stable` to its commit SHA, Swatinem/rust-cache from
`@v2` to its commit SHA, and actions/setup-node from `@v6` to its commit SHA. Each
action should be specified in the format owner/repo@<full-commit-sha> to ensure
reproducibility and prevent supply-chain drift.
In `@scripts/run_gap_tests.sh`:
- Around line 50-60: The script uses fixed temporary filenames like
/tmp/gap_all_fails.txt, /tmp/gap_known.txt, and /tmp/gap_new.txt which can
conflict when multiple script instances run concurrently. Replace all three
fixed temp filenames with dynamically generated unique names using mktemp or by
incorporating a process-specific identifier (such as $$) into each filename.
Update the assignments where these temp files are created from $REPORT, $KNOWN,
and the comm command output, and ensure the wc -l command references the correct
updated variable name for /tmp/gap_all_fails.txt.
🪄 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: 301df9eb-f909-4158-8e87-a5913c9fbe0e
📒 Files selected for processing (3)
.github/workflows/test.ymlCLAUDE.mdscripts/run_gap_tests.sh
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)
124-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin
mozilla-actions/sccache-actionto a full commit SHA.Line 125, Line 206, and Line 402 use a mutable tag (
@v0.0.10). For CI supply-chain integrity and reproducibility, this needsowner/repo@<40-char-sha>pinning.Suggested change
- - name: Start sccache - uses: mozilla-actions/sccache-action@v0.0.10 + - name: Start sccache + uses: mozilla-actions/sccache-action@<full-commit-sha>Also applies to: 205-207, 401-403
🤖 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 @.github/workflows/test.yml around lines 124 - 126, The mozilla-actions/sccache-action action is pinned to a mutable version tag (`@v0.0.10`) at three locations in the workflow file, which compromises reproducibility and supply-chain security. Replace the mutable tag `@v0.0.10` with a full 40-character commit SHA for the mozilla-actions/sccache-action action at all three affected locations: .github/workflows/test.yml lines 125, 205-207, and 401-403. Use the same commit SHA across all three locations to ensure consistent behavior.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In @.github/workflows/test.yml:
- Around line 124-126: The mozilla-actions/sccache-action action is pinned to a
mutable version tag (`@v0.0.10`) at three locations in the workflow file, which
compromises reproducibility and supply-chain security. Replace the mutable tag
`@v0.0.10` with a full 40-character commit SHA for the
mozilla-actions/sccache-action action at all three affected locations:
.github/workflows/test.yml lines 125, 205-207, and 401-403. Use the same commit
SHA across all three locations to ensure consistent behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7070c0c8-861a-47aa-ba29-e1f0a32ed83b
📒 Files selected for processing (2)
.github/workflows/test.ymlCLAUDE.md
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
Member
Author
|
Good catch — fixed in the latest commit. |
run_gap_tests.sh wrote its failure lists to fixed /tmp/gap_*.txt names, so concurrent runs (a second PR, local + CI on the same box, or the upcoming node-suite-guard alongside) could clobber each other and produce a false gate result. Allocate a run-scoped dir with mktemp -d and rm -rf it on EXIT.
…ls: false Two fixes to the informational gap gate: - Oracle node 26 -> 22. The gap suite is byte-diffed live against `node --experimental-strip-types`, and it is already green under node 22 (the legacy parity job). Node 26 introduced version-sensitive diffs (v8 / perf_hooks / process internals — e.g. test_gap_node_v8, test_gap_v8_2, test_gap_perfhooks, test_gap_process_*) that are not Perry regressions and would pollute the triage list. The node-suite regression guard keeps node 26 for its frozen pass-count baseline — a separate mechanism. - persist-credentials: false on checkout. The job is read-only (build + test), so it should not leave the GITHUB_TOKEN in the local git config (CodeRabbit / least privilege).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)
319-334:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin
smoke-parityactions to full commit SHAs.Line 319 through Line 334 still use mutable refs (
@v6,@stable,@v2), which leaves this job open to supply-chain drift. Please pin all four actions in this job to immutable SHAs.#!/bin/bash # Verify mutable action refs specifically within smoke-parity job. # Expected after fix: no output. awk ' /^ smoke-parity:/ {in_job=1} in_job && /^ [a-zA-Z0-9_-]+:/ && $1 != "smoke-parity:" {in_job=0} in_job {print NR ":" $0} ' .github/workflows/test.yml \ | rg -n 'uses:\s*[^@[:space:]]+@(v[0-9]+|stable|main|master)$'🤖 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 @.github/workflows/test.yml around lines 319 - 334, Pin all four actions in the smoke-parity job to immutable full commit SHAs instead of mutable version references. Replace the `@v6` reference in actions/checkout, the `@stable` reference in dtolnay/rust-toolchain, the `@v2` reference in Swatinem/rust-cache, and the `@v6` reference in actions/setup-node with their respective full commit SHA values to prevent supply-chain drift and ensure reproducible builds.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In @.github/workflows/test.yml:
- Around line 319-334: Pin all four actions in the smoke-parity job to immutable
full commit SHAs instead of mutable version references. Replace the `@v6`
reference in actions/checkout, the `@stable` reference in
dtolnay/rust-toolchain, the `@v2` reference in Swatinem/rust-cache, and the
`@v6` reference in actions/setup-node with their respective full commit SHA
values to prevent supply-chain drift and ensure reproducible builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e759d216-e2aa-4101-92bd-411b33cc243a
📒 Files selected for processing (1)
.github/workflows/test.yml
… runner The smoke-parity job was continue-on-error (never blocking) and started all-red (60 untriaged pre-existing gaps), so on every PR it cost a full release build + 237-test run while enforcing nothing. Drop it; this PR now just commits the gap runner and fixes the stale CLAUDE.md /tmp pointer. A real gap gate should land later with a green baseline and as a required check in one move.
TheHypnoo
changed the title
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
2 participants
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.
What
scripts/run_gap_tests.sh— a runner for the gap suite (test-files/test_gap_*.ts). It is a thin wrapper overrun_parity_tests.sh --filter test_gap_, so it reuses the one canonical normalizer, skip-list, and per-test output cap, builds the compiler itself, and gates locally on no new untriaged failures vstest-parity/known_failures.json.CLAUDE.mdparity-status line: it pointed at an out-of-repo/tmp/run_gap_tests.sh(a file that was never in the repo) and an outdated test count.Why
CLAUDE.mdtold contributors to run/tmp/run_gap_tests.sh, which doesn't exist — the gap suite, the highest-signal-per-second test Perry has, had no committed runner. This gives everyone a real, in-repo command.What changed from the original PR
Dropped the
smoke-parityCI job. As designed it wascontinue-on-error(never blocking) and started ~all-red (60 untriaged pre-existing gaps on the Linux/node-22 image), so on every PR it cost a full release build + 237-test run (~57 min) while enforcing nothing and emitting noise — not worth the per-PR cost in that form. A real gap gate should land later with a green baseline and as a required check in one move (triage the baseline first), tracked separately.No source changes; no existing CI job altered.