test(compat): pinned-Node builtin-module compatibility matrix - #7074
Open
jdalton wants to merge 31 commits into
Open
test(compat): pinned-Node builtin-module compatibility matrix#7074jdalton wants to merge 31 commits into
jdalton wants to merge 31 commits into
Conversation
Ports the nub soak/security stack (nubjs/nub#442) with the wheelhouse shim workarounds baked in: - scripts/soak/: soak window parity gate + fixer (SOAK_DAYS=7 in constants.mts is the single source), soaked dependency updater (taze for npm, rustup cargo for crates), and the external-tools installer (SRI-verified rack + PATH handles + sfw firewall shims). - Soak surfaces: .npmrc min-release-age, tools/pnpm-workspace.yaml minimumReleaseAge (pnpm domain isolated in tools/ so the root stays npm-only), tools/taze.config.mts (imports SOAK_DAYS), .cargo/config.toml min-publish-age (nightly-only; inert on stable), .github/dependabot.yml cooldown per update block (the renovate-check equivalent, reworked for dependabot). - external-tools.json: exact pins + sha512 SRI for pnpm 11.8.0, npm 12, sfw-free/-enterprise 1.13.1, zizmor 1.26.1, agentshield 1.4.0, skillspector @2eb84478. - sfw shims carry the known workarounds: per-command recursion sentinel (not PATH-strip), fail-open when sfw is absent, symlink clobber guard, rack-pinned pnpm/npm resolution. - zizmor workflow (hash-pinned action, gate at high) with a documented starting config: official actions ref-pin, six existing third-party actions grandfathered pending a digest-pin sweep, cache-poisoning (49 Low-confidence findings) disabled, release-packages.yml excessive-permissions scoped-ignored pending a job-level split. - security-audit.yml gains always-run soak-gate, agent-scan (AgentShield over .claude/), and skills-scan (NVIDIA SkillSpector over .claude/skills/, static --no-llm path) jobs. - .claude/skills/soak/SKILL.md documents the workflow.
The zizmorcore/zizmor-action run hit startup_failure (repo Actions allowlist), and the rack binary is the better shape anyway: one pin source (external-tools.json), and local tools:install audits with the exact bits CI uses. Token-only-when-nonempty works around zizmor treating an empty --gh-token as real and then fatally erroring.
1.14.0 (published 2026-07-23) fixes two things the shims care about: sfw's own diagnostics now go to stderr (stdout stays transparent for callers capturing `pnpm --version` through a shim), and the child env gains a NO_PROXY loopback exemption (localhost,127.0.0.1,::1) so locally-mocked registries are never proxied. Inside the 7-day window until 2026-07-30, so both pins carry the dated soakBypass annotation; tools:check will demand its removal once the window clears — prune the two annotations then.
…aked releases All three cleared the 7-day window (zizmor 1.28.0 published 07-21, pnpm 11.15.1 07-19, npm 12.0.1 07-10), so no bypass annotations. pnpm 11.16/11.17 and taze 19.16.0 are still soaking; skillspector upstream (2.4/2.5) is entirely inside the window — follow-up bumps once cleared. Gate re-verified: zizmor 1.28.0 with the shipped config reports 0 findings at high across .github/.
… bot PR The soak gates fail closed by design: the day a soakBypass window clears, tools:check goes red until the two annotation lines come off. Failing closed is right; making a human notice is not. So: - external-tools.mts gains --fix (npm run tools:fix): prunes soakBypass annotations whose removable date has passed, then re-runs the checks. Valid-but-expired only — malformed dates stay findings for a human. - soak-autofix.yml (daily cron + dispatch) runs soak:fix + tools:fix, and when anything changed commits to bot/soak-autofix and opens (or force-updates) a PR. Note in-workflow: PRs opened with the default github.token don't trigger CI; set the optional SOAK_AUTOFIX_TOKEN secret to make the bot PRs run checks like any other. Verified end-to-end with a planted expired annotation: --fix prunes it, check returns green, and the fixer is idempotent (unit-tested).
…flows checkout v7.0.1 (3d3c42e5) + setup-node v7.0.0 (82076278) across soak-autofix / zizmor / security-audit — both releases cleared the 7-day window (07-20 / 07-14). Also splits the PATH export in agent-scan (SC2155). The legacy workflow fleet stays on ref pins per the documented digest-pin sweep in .github/zizmor.yml.
… unsafe An EXPIRED soakBypass / exclude pin means the version has fully soaked: the bypass no longer bypasses anything and the pin stays SRI-verified. Failing closed on that turned a no-risk cosmetic state into a red required check that flips overnight with zero code change — the exact noise that trains people to admin-bypass (and the model wheelhouse deliberately avoids: informational + auto-drop). Now: expired-but-VALID annotations are warnings (exit 0), surfaced by staleBypasses / staleExcludes and pruned by --fix + the daily soak-autofix workflow. Missing, malformed, or wrong-arithmetic annotations stay hard failures — unauditable IS unsafe. This also defuses the 2026-07-30 expiry of this PR's own sfw annotations.
- platformKey(): detect musl via the loader heuristic — the -musl pnpm pins were dead keys and a musl host silently installed glibc bits; tools with no -musl pin now fail loud instead. - RUSTUP_CARGO honors CARGO_HOME (custom cargo homes reported the rustup shim as missing). - parseExcludeEntries: tolerate a trailing comment on the minimumReleaseAgeExclude key line — previously the block never opened and every entry beneath escaped validation. - checkCatalogParity: malformed package.json is a Finding, not a crash. - soak-autofix workflow: main-ref guard (dispatch on a topic branch can't force-push the bot branch), concurrency group, and fixer exit status captured + re-raised AFTER the mechanical commit instead of '|| true' masking runtime failures. - sfw shims: fail-open is no longer silent-open — one stderr line when sfw is missing (never on the sentinel re-entry path). - GITHUB_TOKEN on the CI install steps (github.com release fetches). - schematic YYYY-MM-DD example dates in the yaml + skill (the concrete examples were expired copy-paste bait); em-dashes restored in external-tools.json (ensure_ascii artifact).
Wheelhouse lesson: enterprise sfw defaults to BLOCK for non-registry hosts, which breaks ordinary dev flows (API calls, git clones) the day a SOCKET_SECURITY_KEY lands. Free tier hardcodes ignore and disregards the var, so setting it unconditionally is always safe.
- checkDockerPrebake: parse the rustup install line's argument list instead of substring-matching the msrv (a multi-toolchain install line false-failed the check). - RUSTUP_CARGO resolves cargo.exe on win32. - soak-autofix: lease-checked force push (fetch the bot branch, then --force-with-lease) so a concurrent actor's commits are never clobbered.
…ost comment Same drift pullfrog flagged on the nub twin: the skill still said the gates "fail closed when a bypass window clears" — expired-but-valid annotations warn and get pruned by soak:fix / the soak-autofix workflow; invalid annotations are what fail. The shim comment now claims only what the source shows about SFW_UNKNOWN_HOST_ACTION (the enterprise config parses it; inert for free).
Greptile P1 on the aube twin: the pruners and stale lists accepted any valid-ISO annotation whose removable date had passed — including one whose removable was WRONG (earlier than published + SOAK_DAYS). Such an annotation must surface as the hard check failure it is; treating it as soaked would silently delete a bypass whose real window may still be open. All four surfaces (staleExcludes, fixWorkspaceYaml, staleBypasses, pruneExpiredSoakBypasses) now require the arithmetic to hold before an annotation counts as stale or prunable; regression tests cover the wrong-math-expired case.
The nub node-18 compat leg died on `download failed 500` — the first authed fetch of a PUBLIC sfw release asset after GITHUB_TOKEN was added to the step env. Whether that 500 was token-induced (an Actions token against a cross-org public asset endpoint) or a transient GitHub blip, one attempt was too brittle: download() now retries without auth when an authed fetch fails (public assets need no credential), and once more after 2s on a 5xx. Regression test pins the fallback dropping the Authorization header.
Adversarial self-review of the renovate/npmrc/yaml fixers, prompted by
the 20-line diff my own soak:fix produced on aube's renovate.json:
- fixRenovateConfig rewrote the WHOLE file via JSON.parse +
re-stringify, collapsing hand-written single-line arrays and
reformatting unrelated packageRules (aube's decmpfs musl hold among
them). It is now a targeted text edit: only the minimumReleaseAge
line changes, every other byte is preserved. A regression test
asserts exactly one changed line and that the decmpfs rule survives
verbatim.
- The insert path produced INVALID JSON for a minimal `{}` config
(`{,\n ...}`); guarded and covered by a test.
- fixNpmrc / fixWorkspaceYaml matched trailing `\s*$` under /m — `\s`
matches newlines, so the replacement swallowed blank lines after the
key. Now `[ \t]*$`; verified soak:fix is a no-op on a clean tree.
- checkRenovateConfig now also requires `internalChecksFilter: strict`.
Without it renovate's default flexible mode raises updates that have
NOT cleared minimumReleaseAge — the window silently stops biting.
- The no-pinned-asset error names the musl case and lists the pinned
platforms: sfw ships no musl asset, so an alpine runner hits this,
and the old message gave nothing to act on.
Verified alongside: decmpfs stays at 0.1.0 under `cargo update` (the
`=0.1.0` requirement holds, so the soak updater cannot smuggle in the
musl-breaking 0.1.2), and `--force-with-lease` correctly rejects a
concurrent update even when the preceding fetch fails, and still
creates the branch on a first run.
Adversarial self-review of the renovate/npmrc/yaml fixers, prompted by
the 20-line diff my own soak:fix produced on aube's renovate.json:
- fixRenovateConfig rewrote the WHOLE file via JSON.parse +
re-stringify, collapsing hand-written single-line arrays and
reformatting unrelated packageRules (aube's decmpfs musl hold among
them). It is now a targeted text edit: only the minimumReleaseAge
line changes, every other byte is preserved. A regression test
asserts exactly one changed line and that the decmpfs rule survives
verbatim.
- The insert path produced INVALID JSON for a minimal `{}` config
(`{,\n ...}`); guarded and covered by a test.
- fixNpmrc / fixWorkspaceYaml matched trailing `\s*$` under /m — `\s`
matches newlines, so the replacement swallowed blank lines after the
key. Now `[ \t]*$`; verified soak:fix is a no-op on a clean tree.
- checkRenovateConfig now also requires `internalChecksFilter: strict`.
Without it renovate's default flexible mode raises updates that have
NOT cleared minimumReleaseAge — the window silently stops biting.
- The no-pinned-asset error names the musl case and lists the pinned
platforms: sfw ships no musl asset, so an alpine runner hits this,
and the old message gave nothing to act on.
Verified alongside: decmpfs stays at 0.1.0 under `cargo update` (the
`=0.1.0` requirement holds, so the soak updater cannot smuggle in the
musl-breaking 0.1.2), and `--force-with-lease` correctly rejects a
concurrent update even when the preceding fetch fails, and still
creates the branch on a first run.
Auditing a sibling fleet repo (abitious) for compatibility surfaced an unguarded bypass: npm >= 11.17 has its OWN exclude surface, `min-release-age-exclude[]=<spec>`, parallel to pnpm's `minimumReleaseAgeExclude` block — and the gate validated only the pnpm side. `min-release-age-exclude[]=lodash@1.2.3` was therefore an unvalidated, never-expiring hole in exactly the rule the yaml side enforces. checkNpmrc now applies the same law to .npmrc: bare names and `@scope/*` globs are standing trust (the shape real repos use for trusted scopes, so this is not a churn tax), while a VERSION-PINNED entry needs the `# published: | removable:` annotation with correct arithmetic and real calendar dates. Tests cover trusted-glob, unannotated, correct, wrong-arithmetic, and impossible-date cases.
Verified rather than assumed, and the assumption was wrong: cargo treats
an [unstable] key it does not implement as a WARNING ("unused config key
`unstable.min-publish-age`") and exits 0. Measured on nightly
2026-03-21, which has no such -Z — so `cargo +nightly update` on a
merely-OLD nightly resolved every crate with NO window at all while the
run reported success. The tooling was claiming a protection it had not
applied.
updateCargo now captures stderr and treats that warning as a hard
failure: the lockfile changes are unsoaked, so say so and exit nonzero
with the fix (`rustup update nightly`). The detector is an exported,
unit-tested predicate pinning cargo's exact wording.
perry rides stable, where the key is expected to be inert, so there the
same detection downgrades to an explicit note naming dependabot cooldown
as the enforcing surface for cargo deps — no silent no-op either way.
…env bypass
Re-measured on a current nightly (2026-07-27, cargo 1.99.0-nightly): the
`-Z min-publish-age` feature IS implemented there and the window visibly
bites — it holds a too-fresh release back ("available: v0.2.189,
published 7 days ago"). Both measurements are now recorded in the
comment and the skill, since the OLD nightly (2026-03-21) is the
evidence that a stale toolchain skips the window silently.
Running the real updater surfaced the other half of the contract: the
window can make re-resolution IMPOSSIBLE, not just conservative. When a
requirement's only candidate is inside the window (aube today:
`clap_usage = "^4"`, whose 4.0.0 shipped 3 days ago) cargo fails the
whole update — correct behavior, but its own help line advertises
`CARGO_RESOLVER_INCOMPATIBLE_PUBLISH_AGE=allow`, a blanket env-var
bypass this design deliberately does not have. The updater now detects
that failure and prints ordered options (wait it out, repin so a soaked
version satisfies the requirement, or adopt the fresh release as a
reviewable commit) with an explicit warning against the env bypass.
Predicate is exported and unit-tested against cargo's real wording.
An independent hostile review of the three sibling PRs found real defects, including one where my own test had verified only the safe half of the case: - soak-autofix no longer force-pushes at all. `git fetch origin $BRANCH` UPDATES the remote-tracking ref (actions/checkout leaves the default wildcard refspec), so the following --force-with-lease took its lease against whatever another actor had just pushed and overwrote it — the classic fetch-before-lease anti-pattern, and the inline comment asserting "a concurrent actor's commits are never clobbered" was false. Demonstrated: a human commit onto the open autofix PR was discarded by the next scheduled run. My earlier test only covered the fetch-FAILS path (which is genuinely safe, rejecting with "stale info"). The step now stashes the fixes, bases the work on the existing bot branch when there is one, and plain-pushes: human commits survive by construction, an empty re-run exits 0 instead of pushing a no-op commit, and a genuine conflict fails loudly instead of being resolved by deletion. - fixWorkspaceYaml's prune set must EQUAL staleExcludes' warn set: it was missing the VERSION_PIN_RE guard, so a bare-name / `@scope/*` standing-trust entry sitting under an expired annotation line was deleted by --fix, silently re-arming the soak for a whole scope inside a bot commit advertised as touching only annotation lines. - download() retry semantics split by meaning: 401/403/404 with a token means the credential is the problem (retry unauthenticated), >=500 is transient (retry with the SAME auth). Dropping auth on 5xx made a private asset 404 on the retry, report a bogus "download failed 404", and never be able to succeed. The SRI is verified either way, so no retry can substitute a different artifact.
- fixRenovateConfig still matched a trailing `\s*$`, which under /m eats the NEWLINES after the value: replacing through it silently deleted the blank line that followed. That is the exact defect the same commit fixed in fixNpmrc and fixWorkspaceYaml, kept in the third fixer. Verified with a config carrying a blank line after the key: it disappeared before, survives now. - checkPins now rejects a soakBypass whose `version` is not the version actually pinned. Bump a pin and leave the annotation behind and the ledger vouches for a release that is no longer installed — "1.13.1 was adopted early" while 1.14.0 ships unreviewed. A mismatch is unauditable, so it is a hard finding, not a stale-annotation warning. - soak-autofix.yml's header still described the gates as failing closed on a cleared window; the fourth and last sibling of that stale premise. Expired is a warning, invalid still fails, and the workflow's job is convergence rather than rescue. - Two paths were changed without a test covering them, both added: the multi-arg `rustup toolchain install 1.91.0 1.93.0` case that motivated replacing the substring msrv match (only the negative case was covered), and the `>= 500` retry branch that the retry commit is named for (the existing test exercises only the auth fallback).
… a real npm CLI needs Compiling Socket Firewall (sfw — a TLS-MITM proxy CLI with undici, node-forge, iovalkey, zod, … in its graph) end-to-end surfaced four independent blockers. Fixed here: 1. auto-optimize feature skew (driver.rs / freshness.rs): the perry binary's baked-in cross-feature list tracks the branch it was BUILT from, but the auto-optimize cargo build resolves against the checkout on disk. One unknown `perry-runtime/<feat>` failed the whole resolve, and the silent prebuilt fallback linked without the routed ext-pump entrypoints — undefined-js_* errors two stages from the cause. New retain_workspace_declared_features() drops names the checkout's perry-runtime / perry-stdlib don't declare (features table + optional deps, fail-open on unreadable manifests) before the build stamp is computed, and the cargo-failure fallback now says what the consequence and remedy are. 2. perry-ext-zlib zstd surface: undici's web-fetch content decoding references js_zlib_create_zstd_decompress unconditionally, but only perry-stdlib's `compression` module carried the zstd codecs — and routing node:zlib to the ext archive strips that feature. Port the full surface (create factories, sync/async one-shots, streaming write-codec via zstd::stream::write) so the routed archive is self-sufficient. 3. class X extends DOMException (codegen + runtime): undici probes DOMException inheritability at module load (websocketerror.js), and the name was neither in the builtin-parent list nor backed by a subclass initializer — the compiled binary died at startup with 'DOMException is not a function'. Add js_dom_exception_subclass_init (stamps message/name/code onto the subclass instance) wired through both the explicit super() lowering and the implicit-ctor NativeInstanceBase chain walk. 4. panic-runtime dedup for prebuilt (panic=unwind) wrappers co-linked with a panic=abort auto-optimized stdlib (strip_dedup.rs): the name-containment rule never nominated the wrapper's panic_unwind member (stdlib bundles panic_abort under a different name), and the localize pass severed the std-cgu → panic_unwind __rust_drop_panic edge that abort stdlibs cannot re-provide. Nominate panic_unwind in the nosharedeps fixed-point (protected exactly when the stdlib can't cover it), and skip localizing panic symbols a sibling member still references. Allocator shims stay always-localized: leaving the wrapper's system-malloc shim global beats the runtime's mimalloc at link and breaks pointer classification (silent console loss). With these, sfw and sfw-free compile, link, and run as native arm64 binaries straight from their TypeScript entrypoints.
…ll form ECMA-262 22.2.4: `RegExp(pattern, flags)` without `new` constructs exactly like `new RegExp`, with the identity shortcut `RegExp(re)` → `re`. The globalThis sentinel fell through to the noop thunk and returned undefined — which is how lodash's module init died: runInContext rebinds the global (`var RegExp = context.RegExp`) and builds `reIsNative` through the call form, so the immediately following `reIsNative.test(...)` threw 'Cannot read properties of undefined'. New regexp_constructor_call_thunk (arity 2) mirrors the dynamic-new RegExp arm in class_registry/construct.rs; without the regex-engine feature it keeps the old noop behavior. Unblocks sfw-registry (lodash via registry/proxy-request.ts).
…own bindings Port the socket-registry fleet's upstream-reference technique to well_known_bindings.toml: every third-party npm package with a bundled native wrapper records which upstream release it ports (version, tarball sha256, repo, git ref) and when it was last reviewed against it (ported-at, date). The lock-step rule — ported-at == version — means a pin bump can't outrun the wrapper review it demands. Enforced twice: the toml parser inside perry refuses to load a skewed table, and scripts/binding_pins.mjs --check gates it in CI (offline). Node builtins (node-builtin=true), aliases (alias-of), and perry-owned packages are exempt from pinning; a distinct package sharing a wrapper crate (redis, iovalkey) still carries its own pin. scripts/binding_pins.mjs provisions pins from the npm registry (--set/--backfill), gates them offline (--check), flags newly-soaked upstream releases (--check --refresh --soak-days N), and materializes an upstream at its pin for port review (--materialize). All 34 third-party bindings backfilled at their current latest. Also adds an iovalkey binding — the Valkey fork of ioredis (valkey-io/iovalkey), a distinct npm package served by the existing perry-ext-ioredis surface — wired through NATIVE_MODULES, the codegen module-normalization + Redis-constructor lists, and stdlib feature gating, and annotates every NATIVE_MODULES entry. 758 tests pass; binding_pins.mjs --check green.
Breadth sweep over every require("module").builtinModules entry, both
import forms (M and node:M), against a pinned SRI-verified Node oracle
(26.5.1). Compares Perry's export-SHAPE fingerprint (sorted name:typeof
over the namespace + default export typeof) to Node's. Systematic form
of tracker PerryTS#812; behavioral parity stays in the node-suite.
- external-tools.json: pin tools.node 26.5.1 with per-platform sha512 SRI
(darwin/linux/win, arm64+x64), each cross-checked vs SHASUMS256.txt.
scripts/soak/external-tools.mts learns a node-dist release type so the
pin gate stays green without treating nodejs.org as a github release.
- scripts/node_compat_matrix.mjs: resolver (download+verify+cache under
.cache/node-pin/) + matrix runner. Modes: default table, --check gate,
--update-baseline, and fast-loop selectors --module/--method/--only.
- test-parity/node-compat-matrix.baseline.json: real darwin-arm64 baseline
(58 builtins; 21 both-forms match, 35 shape-diff, 0 perry-unresolved,
0 prefix-parity divergences) + a curated skip list.
- .github/workflows/node-compat-matrix.yml: own nightly --check job.
- docs + CLAUDE.md + llms.txt: fast-loop-first usage and how to bump the pin.
Findings recorded (not fixed here, infra PR): sea/sqlite/test/test/reporters
are node:-prefix-only in Node 26 yet Perry resolves the bare form too;
trace_events matches Node fully but is absent from the manifest module lists.
📝 WalkthroughWalkthroughChangesThe pull request adds a seven-day supply-chain soak system, pinned external security tooling, a Node builtin compatibility matrix, upstream provenance validation for native bindings, and compiler/runtime fixes for DOMException, zstd, RegExp, iovalkey, feature skew, and panic-runtime linking. Supply-chain soak enforcement
Pinned security tooling
Node compatibility matrix
Compiler and runtime compatibility fixes
Binding provenance pins
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 14 UNAVAILABLE: Connection dropped 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: 16
🧹 Nitpick comments (14)
docs/src/testing/node-compat-matrix.md (1)
43-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWorth documenting that the baseline is platform-scoped.
--checkcompares against a baseline whose header records a singleplatform; the nightly job runs on a different host than the committeddarwin-arm64baseline. Please state which platform the committed baseline is authoritative for (see the consolidated comment).🤖 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 `@docs/src/testing/node-compat-matrix.md` around lines 43 - 53, Update the node compatibility matrix documentation near the baseline and --check description to state that the committed baseline is platform-scoped and authoritative for darwin-arm64, as recorded by its header; note that the nightly comparison must account for platform differences.crates/perry-api-manifest/src/entries.rs (1)
94-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate entries are now documented rather than removed.
"dns"/"dns/promises"already appear at Lines 78-79. The new comments make the duplication intentional-looking; if nothing keys off list position, dropping the duplicates is cleaner than annotating them.🤖 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 `@crates/perry-api-manifest/src/entries.rs` around lines 94 - 95, Remove the duplicate "dns" and "dns/promises" entries from the manifest list in entries.rs, keeping the earlier occurrences unchanged. Delete their associated explanatory comments rather than documenting intentional duplication.scripts/node_compat_matrix.mjs (1)
111-141: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFlag options that consume a value don't validate it.
--module/--method/--only/--node-versionall doargv[++i]without checking the operand exists.--node-versionat the end of argv setsnodeVersion = undefined(silently falls back to the pin), and--modulewith no operand silently means "all modules" — the opposite of the user's intent for a selector-scoped--update-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 `@scripts/node_compat_matrix.mjs` around lines 111 - 141, Update the argument parsing loop to validate that --module, --method, --only, and --node-version each have a following operand before consuming argv[++i]. When an operand is missing, emit the existing invalid-argument style error, set args.mode to help, and avoid applying a selector or node-version fallback; preserve current parsing for present operands..github/workflows/zizmor.yml (1)
6-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPath filter misses the installer this job depends on.
The job installs zizmor through
scripts/soak/external-tools.mts+external-tools.json, but neither path triggers it, so a pin bump or installer change lands unexercised. Consider adding both paths (and this workflow's own file is already covered by.github/**).♻️ Proposed fix
pull_request: - paths: ['.github/**'] + paths: + - '.github/**' + - 'external-tools.json' + - 'scripts/soak/external-tools.mts'🤖 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/zizmor.yml around lines 6 - 7, Update the pull_request paths filter in the zizmor workflow to include both scripts/soak/external-tools.mts and external-tools.json, while retaining the existing .github/** pattern..github/zizmor.yml (1)
32-38: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winBlanket
cache-poisoning: disablealso silences future high-confidence findings.The rationale is release workflows publishing artifacts, but disabling the rule globally means a genuinely poisonable new workflow never surfaces. Prefer
ignore:scoped to the release workflow filenames so the rule keeps working everywhere else.🤖 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/zizmor.yml around lines 32 - 38, Replace the global cache-poisoning disable in zizmor configuration with an ignore list scoped to the existing release workflow filenames. Preserve the intended suppression for current Swatinem/rust-cache findings in artifact-publishing workflows while allowing cache-poisoning checks to remain enabled for all other workflows and future high-confidence findings.scripts/soak/external-tools.mts (2)
688-692: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--installwith no operand throws an opaque error.
argv[++i]!isundefinedwhen--installis last, producingunknown tool undefined. A guard makes the CLI misuse legible.🤖 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/soak/external-tools.mts` around lines 688 - 692, Update the argument loop handling `--install` to validate that an operand exists before incrementing and passing it to `installTool`; when `--install` is the final argument, emit a clear CLI misuse error instead of allowing `undefined` to reach `installTool`. Preserve the existing installation flow for valid operands.
152-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
staleBypassesandpruneExpiredSoakBypassesare the same predicate twice.Both walk tools, skip missing/malformed dates, skip wrong arithmetic, then test
removable < today— including duplicated comments. Extracting the predicate keeps the two from diverging (a divergence here means the autofix workflow warns about something it can't prune, or vice versa).♻️ Sketch
+function isExpiredBypass(pin: ToolPin, today: string): boolean { + const b = pin.soakBypass + if (!b || !isValidIsoDate(b.published) || !isValidIsoDate(b.removable)) { + return false + } + // Wrong-arithmetic annotations are a hard checkPins failure, never + // stale/prunable — a too-early removable must not read as "cleared". + return b.removable === addDaysIso(b.published, SOAK_DAYS) && b.removable < today +}🤖 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/soak/external-tools.mts` around lines 152 - 206, Extract the shared soak-bypass eligibility and expiration predicate from staleBypasses and pruneExpiredSoakBypasses into a reusable helper, including date validation, arithmetic validation, and removable < today checks. Update both functions to call that helper while preserving their existing name collection and deletion behavior, and remove the duplicated predicate logic and comments.scripts/soak/external-tools.test.mts (2)
286-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
writeShims.The generated shim is the highest-risk artifact in this module (it intercepts every
npm/pnpm/cargoinvocation) and is entirely untested. A POSIX-only test can write shims into a tempBIN_DIRand assert the three paths: rack-pinned resolution, sentinel re-entry, and the not-found exit code — which currently exits 1 rather than 127 (see the comment onexternal-tools.mtsLines 611-620).🤖 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/soak/external-tools.test.mts` around lines 286 - 298, Add POSIX-only coverage for writeShims in the external-tools tests using a temporary BIN_DIR. Exercise and assert rack-pinned resolution, sentinel re-entry, and missing-tool behavior, including the expected not-found exit code of 127; isolate and clean up the temporary shim directory.
103-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrift assertion depends on the real Dockerfile's hex prefix.
docker.replace(/sha=[0-9a-f]{8}/, 'sha=deadbeef')is a no-op if the first pinned hash already begins withdeadbeef, and the test would then assert on unmodified input. Mutating a known-position byte (e.g. appending/prefixing via a captured group) removes that coupling.🤖 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/soak/external-tools.test.mts` around lines 103 - 120, The Docker drift assertion in the test for checkDockerPrebake must always modify the Dockerfile instead of relying on its existing hash value. Update the replacement to capture the sha assignment and deterministically alter a known hash character or append a marker, ensuring the input differs even when the original prefix is deadbeef.scripts/soak/soak.mts (1)
401-422: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
checkTazeConfigis substring-only, so a commented-out or broken config still passes.
body.includes('maturityPeriod')matches inside comments and cannot see whether the import actually resolves. Given this is the one surface that can't be value-asserted, consider dynamically importing the config and assertingmaturityPeriod === SOAK_DAYSinstead of grepping.🤖 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/soak/soak.mts` around lines 401 - 422, Update checkTazeConfig to dynamically import and validate the taze configuration, asserting that maturityPeriod equals SOAK_DAYS and that the SOAK_DAYS import resolves from the expected constants module. Replace the substring checks so commented-out or broken configurations produce findings, while preserving the existing Finding[] output and messages.scripts/soak/soak.test.mts (1)
32-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: derive the numeric fixtures from
SOAK_DAYS/SOAK_MINUTES.
"7 days",min-release-age=7,10080, andremovable: 2020-01-08hardcode a 7-day window, so bumpingSOAK_DAYSreddens these tests for reasons unrelated to the logic under test. If the literals are a deliberate tripwire, a one-line comment saying so would save the next reader the trip.Also applies to: 109-109
🤖 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/soak/soak.test.mts` around lines 32 - 54, Derive the fixture values in the soak tests from the existing SOAK_DAYS/SOAK_MINUTES symbols instead of hardcoding the seven-day values, updating the Cargo, npmrc, YAML, and removable-date fixtures consistently. If the literals are intentionally fixed as tripwires, add a concise comment documenting that intent.package.json (1)
3-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin a Node floor for the direct
.mtsexecution.All six scripts run TypeScript through
nodewith no flag, which requires native type stripping (unflagged only on Node 22.18+/23.6+). Please confirm the repo's.node-versionclears that bar and consider declaringengines.nodeso a stale local Node fails with a clear message instead of a syntax error.As per coding guidelines, "Use the Node version pinned in the repository's
.node-versionfor CI and gap tests".#!/bin/bash fd -H -t f '^\.node-version$' --exec cat fd -H -t f '^package\.json$' -d 2 --exec jq -r '{name, engines}'🤖 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` around lines 3 - 11, Verify that the repository’s .node-version specifies a Node release supporting unflagged .mts type stripping, and update package.json to declare a matching engines.node minimum. Ensure the six direct Node script commands and CI/gap-test configuration use the pinned version consistently, so unsupported local Node versions fail with a clear engine requirement.crates/perry/src/commands/compile/well_known.rs (2)
541-556: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
alias_of_targets_existdoesn't verify the target actually carries provenance.The test only checks
lookup_well_known(target).is_some(). It doesn't assert the target has anupstreampin (or resolve through a chain of aliases to one), so a future alias pointing at another unpinned alias would silently violate the "aliases share their target's provenance" invariant documented onalias_of(lines 43-47) with no test failure.🤖 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 `@crates/perry/src/commands/compile/well_known.rs` around lines 541 - 556, Extend the alias validation in alias_of_targets_exist to follow each alias_of chain and assert that it ultimately reaches a binding with an upstream pin. Preserve the existing missing-target assertion, and ensure chained aliases are accepted only when their resolved target carries provenance.
209-271: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo mutual-exclusivity check between
node_builtin/alias_ofandupstream.The parser happily accepts an entry where
node-builtin = true(oralias-of = "...") and an[bindings.<x>.upstream]table are both present, even though the doc comments onnode_builtin/alias_of(lines 38-47) state these rows "carry no npm provenance pin" / "share that binding's provenance instead of carrying its own pin." Nothing currently rejects a contradictory row.♻️ Proposed validation
+ if node_builtin && upstream.is_some() { + return Err(format!( + "[bindings.{}] is node-builtin but also declares an [upstream] pin", + pkg_name + )); + } + if alias_of.is_some() && upstream.is_some() { + return Err(format!( + "[bindings.{}] is alias-of another binding but also declares an [upstream] pin", + pkg_name + )); + } + out.insert(🤖 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 `@crates/perry/src/commands/compile/well_known.rs` around lines 209 - 271, Validate the parsed binding entry before inserting WellKnownBinding so node_builtin or alias_of cannot coexist with an upstream pin. Reject entries where node_builtin is true or alias_of is present while upstream is Some, using the existing package name in a descriptive error; leave valid standalone node-builtin, alias, and upstream entries unchanged.
🤖 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/security-audit.yml:
- Around line 71-74: Update the three new jobs, including soak-gate and the jobs
at the other referenced blocks, to declare explicit job-level permissions
containing only contents: read. First check the workflow header for existing
permissions, but retain the explicit blocks on each job regardless so the new
surface satisfies the enforced permission policy.
- Around line 129-137: Update the skillspector loop in the workflow to guard
against a missing or empty .claude/skills directory before iterating, preventing
the unmatched glob from being passed as a literal path. Preserve the existing
skillspector scan behavior for each actual skill directory.
In `@changelog.d/7021-real-npm-cli-compile-gaps.md`:
- Around line 1-7: Update the changelog entry to describe five blockers instead
of four, and add the affected code paths or components for each fix, including
the relevant runtime, zlib, DOMException, panic-runtime, and RegExp areas. Add
concise validation notes documenting how each change was verified, while
preserving the existing root-cause details and structure.
In `@crates/perry-runtime/src/event_target.rs`:
- Around line 337-358: Update js_dom_exception_subclass_init to root this_value,
message, and name using RuntimeHandleScope before calling
optional_string_from_value or set_event_field. Store NaN-boxed handles rather
than raw pointers, and reload rewritten values from the scope before deriving
name_string and before each subsequent field write so moving GC remains safe.
In `@crates/perry-runtime/src/object/global_this/ctor_thunks.rs`:
- Around line 47-63: The RegExp construction path must reuse an existing
RegExp’s internal source when explicit flags are provided. Update the logic
around is_regex_pointer and js_regexp_new to detect the RegExp pointer before
js_string_coerce, retrieve its stored source, and pass that source with the
supplied flags; preserve the existing fast return when flags are undefined.
- Around line 53-64: Update the regexp constructor flow around
js_string_coerce(pattern) to store the coerced pattern in a precise shadow-stack
root as a NaN-boxed value before coercing flags. After js_string_coerce(flags),
reload the potentially rewritten pattern pointer from that root and pass it to
js_regexp_new; do not keep the raw pattern pointer across the flag coercion.
In `@crates/perry/src/commands/compile/optimized_libs/freshness.rs`:
- Around line 210-225: Update the optional-dependency collector around
collect_optional to first gather dependency names referenced by dep:<name>
entries in the manifest’s features table, then exclude those names when
inserting optional dependencies into names. Apply this suppression consistently
to both top-level and target-specific dependency tables while preserving
collection of optional dependencies whose implicit features remain enabled.
In `@external-tools.json`:
- Around line 213-219: The SkillSpector git pin is abbreviated and duplicated
without validation. In external-tools.json lines 213-219, replace version
2eb84478 with the full 40-character commit SHA and update checkPins to require
/^[0-9a-f]{40}$/ for release === 'uv-project' entries. In
.github/workflows/security-audit.yml lines 133-135, use that same full SHA for
the NVIDIA/skillspector git source, preferably loading it from
external-tools.json so the manifest remains the single pin source.
In `@scripts/binding_pins.mjs`:
- Around line 152-173: Update fetchJson and sha256OfTarball to use an
AbortSignal timeout with a finite request deadline, passing the signal to each
registry fetch so stalled npm requests fail promptly. Preserve the existing HTTP
status validation and response-processing behavior, and apply the same timeout
handling to all calls in both functions.
In `@scripts/node_compat_matrix.mjs`:
- Around line 613-618: The baseline check must validate scope before comparing
cells: update checkAgainstBaseline to fail fast or report a skip when
base.platform differs from matrix.platform or base.nodeVersion differs from
matrix.version. In .github/workflows/node-compat-matrix.yml lines 78-83, run the
gate on a runner matching the committed baseline or use a platform-specific
Linux baseline. In test-parity/node-compat-matrix.baseline.json lines 18-19,
retain platform and nodeVersion as authoritative scope fields and use a
platform-specific filename if multiple baselines are introduced.
- Around line 620-632: Update the regression comparison loop in the
matrix-checking logic to safely read status from prev[form] when a baseline form
is missing, matching the defensive handling already used for
prev.unprefixed?.status. Preserve the existing severity comparison and
regression reporting for forms present in both baseline and current entries.
- Around line 232-256: Update the verification branch around isPinned and
platPin so any version matching pin.version requires a platform pin. Fail
immediately with a clear error when pin.platforms[key] is absent, and only use
the SHASUMS256.txt fallback for genuinely non-pinned versions; preserve the
existing SRI verification for configured platform pins.
In `@scripts/soak/external-tools.mts`:
- Around line 112-116: Update the SRI validation in the integrities loop to
require a complete base64-encoded SHA-512 digest: enforce the expected
86-character payload followed by “==” rather than accepting any nonempty base64
text. Preserve rejection of malformed SRI values and ensure truncated pins such
as “sha512-AAAA” fail before download.
- Around line 269-272: Update the pinned toolchain validation around the channel
extraction and dockerBody check to parse rustup/toolchain install command
arguments rather than using dockerBody.includes. Detect the channel regardless
of argument position or whether it is followed by additional arguments or
end-of-line, while preserving the existing warning when the pinned channel is
absent.
In `@scripts/soak/soak.mts`:
- Line 58: Correct the `fix:` guidance in `checkCargoConfig` so it does not
claim `--fix` can resolve the `saw: '(missing)'` case while `fixCargoConfig`
only replaces existing `global-min-publish-age` lines. Either remove the `--fix`
hint for missing keys or update `fixCargoConfig` to append the key with
appropriate `[registry]`-aware insertion, matching `fixNpmrc`; keep the existing
replacement behavior for drift cases.
- Around line 517-546: Update the pruning logic around the drop set in the soak
output fixer so that when all entries under minimumReleaseAgeExclude are
removed, the now-empty key line is removed as well. Preserve the key when any
exclude entry remains, and continue removing only expired valid pins and their
annotations.
---
Nitpick comments:
In @.github/workflows/zizmor.yml:
- Around line 6-7: Update the pull_request paths filter in the zizmor workflow
to include both scripts/soak/external-tools.mts and external-tools.json, while
retaining the existing .github/** pattern.
In @.github/zizmor.yml:
- Around line 32-38: Replace the global cache-poisoning disable in zizmor
configuration with an ignore list scoped to the existing release workflow
filenames. Preserve the intended suppression for current Swatinem/rust-cache
findings in artifact-publishing workflows while allowing cache-poisoning checks
to remain enabled for all other workflows and future high-confidence findings.
In `@crates/perry-api-manifest/src/entries.rs`:
- Around line 94-95: Remove the duplicate "dns" and "dns/promises" entries from
the manifest list in entries.rs, keeping the earlier occurrences unchanged.
Delete their associated explanatory comments rather than documenting intentional
duplication.
In `@crates/perry/src/commands/compile/well_known.rs`:
- Around line 541-556: Extend the alias validation in alias_of_targets_exist to
follow each alias_of chain and assert that it ultimately reaches a binding with
an upstream pin. Preserve the existing missing-target assertion, and ensure
chained aliases are accepted only when their resolved target carries provenance.
- Around line 209-271: Validate the parsed binding entry before inserting
WellKnownBinding so node_builtin or alias_of cannot coexist with an upstream
pin. Reject entries where node_builtin is true or alias_of is present while
upstream is Some, using the existing package name in a descriptive error; leave
valid standalone node-builtin, alias, and upstream entries unchanged.
In `@docs/src/testing/node-compat-matrix.md`:
- Around line 43-53: Update the node compatibility matrix documentation near the
baseline and --check description to state that the committed baseline is
platform-scoped and authoritative for darwin-arm64, as recorded by its header;
note that the nightly comparison must account for platform differences.
In `@package.json`:
- Around line 3-11: Verify that the repository’s .node-version specifies a Node
release supporting unflagged .mts type stripping, and update package.json to
declare a matching engines.node minimum. Ensure the six direct Node script
commands and CI/gap-test configuration use the pinned version consistently, so
unsupported local Node versions fail with a clear engine requirement.
In `@scripts/node_compat_matrix.mjs`:
- Around line 111-141: Update the argument parsing loop to validate that
--module, --method, --only, and --node-version each have a following operand
before consuming argv[++i]. When an operand is missing, emit the existing
invalid-argument style error, set args.mode to help, and avoid applying a
selector or node-version fallback; preserve current parsing for present
operands.
In `@scripts/soak/external-tools.mts`:
- Around line 688-692: Update the argument loop handling `--install` to validate
that an operand exists before incrementing and passing it to `installTool`; when
`--install` is the final argument, emit a clear CLI misuse error instead of
allowing `undefined` to reach `installTool`. Preserve the existing installation
flow for valid operands.
- Around line 152-206: Extract the shared soak-bypass eligibility and expiration
predicate from staleBypasses and pruneExpiredSoakBypasses into a reusable
helper, including date validation, arithmetic validation, and removable < today
checks. Update both functions to call that helper while preserving their
existing name collection and deletion behavior, and remove the duplicated
predicate logic and comments.
In `@scripts/soak/external-tools.test.mts`:
- Around line 286-298: Add POSIX-only coverage for writeShims in the
external-tools tests using a temporary BIN_DIR. Exercise and assert rack-pinned
resolution, sentinel re-entry, and missing-tool behavior, including the expected
not-found exit code of 127; isolate and clean up the temporary shim directory.
- Around line 103-120: The Docker drift assertion in the test for
checkDockerPrebake must always modify the Dockerfile instead of relying on its
existing hash value. Update the replacement to capture the sha assignment and
deterministically alter a known hash character or append a marker, ensuring the
input differs even when the original prefix is deadbeef.
In `@scripts/soak/soak.mts`:
- Around line 401-422: Update checkTazeConfig to dynamically import and validate
the taze configuration, asserting that maturityPeriod equals SOAK_DAYS and that
the SOAK_DAYS import resolves from the expected constants module. Replace the
substring checks so commented-out or broken configurations produce findings,
while preserving the existing Finding[] output and messages.
In `@scripts/soak/soak.test.mts`:
- Around line 32-54: Derive the fixture values in the soak tests from the
existing SOAK_DAYS/SOAK_MINUTES symbols instead of hardcoding the seven-day
values, updating the Cargo, npmrc, YAML, and removable-date fixtures
consistently. If the literals are intentionally fixed as tripwires, add a
concise comment documenting that intent.
🪄 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: 19d42eb8-1945-463e-9293-5645c22cd77f
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktools/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (57)
.cargo/config.toml.claude/skills/soak/SKILL.md.github/dependabot.yml.github/workflows/node-compat-matrix.yml.github/workflows/security-audit.yml.github/workflows/soak-autofix.yml.github/workflows/test.yml.github/workflows/zizmor.yml.github/zizmor.yml.gitignore.npmrcCLAUDE.mdchangelog.d/6912-security-tooling-soak.mdchangelog.d/7021-real-npm-cli-compile-gaps.mdchangelog.d/7031-binding-upstream-lockstep.mdchangelog.d/7034-node-compat-matrix.mdcrates/perry-api-manifest/src/entries.rscrates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/lower_call/builtin.rscrates/perry-codegen/src/lower_call/native_module_dispatch.rscrates/perry-codegen/src/lower_call/new_helpers.rscrates/perry-codegen/src/runtime_decls/strings_part2.rscrates/perry-ext-zlib/Cargo.tomlcrates/perry-ext-zlib/src/stream.rscrates/perry-runtime/src/event_target.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/global_this/ctor_thunks.rscrates/perry-runtime/src/object/global_this/populate.rscrates/perry/src/commands/compile/optimized_libs.rscrates/perry/src/commands/compile/optimized_libs/driver.rscrates/perry/src/commands/compile/optimized_libs/freshness.rscrates/perry/src/commands/compile/optimized_libs/tests.rscrates/perry/src/commands/compile/strip_dedup.rscrates/perry/src/commands/compile/well_known.rscrates/perry/src/commands/stdlib_features.rscrates/perry/well_known_bindings.tomldocs/src/SUMMARY.mddocs/src/native-libraries/upstream-pins.mddocs/src/testing/node-compat-matrix.mdexternal-tools.jsonllms.txtpackage.jsonscripts/binding_pins.mjsscripts/node_compat_matrix.mjsscripts/soak/constants.mtsscripts/soak/external-tools.mtsscripts/soak/external-tools.test.mtsscripts/soak/paths.mtsscripts/soak/soak.mtsscripts/soak/soak.test.mtsscripts/soak/update-deps.mtsscripts/soak/update-deps.test.mtstest-parity/node-compat-matrix.baseline.jsontest-parity/node-compat-matrix.skip.jsontools/package.jsontools/pnpm-workspace.yamltools/taze.config.mts
Comment on lines
+71
to
+74
| soak-gate: | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 10 | ||
| steps: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Give the new jobs explicit permissions: blocks.
.github/zizmor.yml grandfathers existing excessive-permissions debt while stating new surface is enforced; these three jobs are new surface. All of them only need contents: read. Verify whether a workflow-level permissions: already constrains them:
#!/bin/bash
sed -n '1,20p' .github/workflows/security-audit.ymlAlso applies to: 92-95, 120-123
🤖 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/security-audit.yml around lines 71 - 74, Update the three
new jobs, including soak-gate and the jobs at the other referenced blocks, to
declare explicit job-level permissions containing only contents: read. First
check the workflow header for existing permissions, but retain the explicit
blocks on each job regardless so the new surface satisfies the enforced
permission policy.
Comment on lines
+129
to
+137
| run: | | ||
| set -euo pipefail | ||
| for skill in .claude/skills/*/; do | ||
| echo "::group::skillspector ${skill}" | ||
| uv tool run --python 3.12 \ | ||
| --from git+https://github.com/NVIDIA/skillspector@2eb84478 \ | ||
| skillspector scan "${skill}" --no-llm | ||
| echo "::endgroup::" | ||
| done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unmatched glob turns into a literal path.
Without nullglob, an empty (or absent) .claude/skills/ makes the loop run once with the literal string .claude/skills/*/, and set -e fails the job with a confusing skillspector error. Guard the directory instead.
🐛 Proposed fix
set -euo pipefail
+ shopt -s nullglob
+ skills=(.claude/skills/*/)
+ if [ ${`#skills`[@]} -eq 0 ]; then
+ echo "no skills to scan"; exit 0
+ fi
- for skill in .claude/skills/*/; do
+ for skill in "${skills[@]}"; do📝 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
| run: | | |
| set -euo pipefail | |
| for skill in .claude/skills/*/; do | |
| echo "::group::skillspector ${skill}" | |
| uv tool run --python 3.12 \ | |
| --from git+https://github.com/NVIDIA/skillspector@2eb84478 \ | |
| skillspector scan "${skill}" --no-llm | |
| echo "::endgroup::" | |
| done | |
| run: | | |
| set -euo pipefail | |
| shopt -s nullglob | |
| skills=(.claude/skills/*/) | |
| if [ ${`#skills`[@]} -eq 0 ]; then | |
| echo "no skills to scan"; exit 0 | |
| fi | |
| for skill in "${skills[@]}"; do | |
| echo "::group::skillspector ${skill}" | |
| uv tool run --python 3.12 \ | |
| --from git+https://github.com/NVIDIA/skillspector@2eb84478 \ | |
| skillspector scan "${skill}" --no-llm | |
| echo "::endgroup::" | |
| done |
🤖 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/security-audit.yml around lines 129 - 137, Update the
skillspector loop in the workflow to guard against a missing or empty
.claude/skills directory before iterating, preventing the unmatched glob from
being passed as a literal path. Preserve the existing skillspector scan behavior
for each actual skill directory.
Comment on lines
+1
to
+7
| **Compile fixes from taking a real npm CLI (Socket Firewall) to a native binary** — four independent blockers, each fixed at its own layer: | ||
|
|
||
| - **Auto-optimize feature skew**: cross-features the on-disk checkout's `perry-runtime`/`perry-stdlib` don't declare are now dropped (with a warning) instead of failing the whole cargo resolve and silently falling back to a link that's missing the routed ext entrypoints. The cargo-failure fallback message now explains the consequence and remedy. | ||
| - **`perry-ext-zlib` zstd surface**: `zlib.createZstdCompress`/`createZstdDecompress`, the zstd one-shots, and the streaming write-codec are now implemented in the ext wrapper, so routing `node:zlib` no longer strips the only zstd implementation out of the link (undici's web-fetch content decoding references it unconditionally). | ||
| - **`class X extends DOMException`**: new `js_dom_exception_subclass_init` wired through both the explicit `super()` lowering and the implicit-ctor chain walk — undici's module-init inheritability probe no longer aborts startup with `DOMException is not a function`. | ||
| - **panic-runtime dedup**: prebuilt (panic=unwind) wrapper staticlibs co-linked with a panic=abort auto-optimized stdlib no longer die on `__rust_drop_panic` — the `panic_unwind` member is nominated for the nosharedeps fixed-point (kept only when the stdlib can't cover it), and panic symbols referenced by a sibling member are no longer localized. Allocator shims remain always-localized (a global wrapper malloc shim would break runtime pointer classification). | ||
| - **`RegExp` call form via a rebound global**: `var R = globalThis.RegExp; R(src)` now constructs (with the spec's `RegExp(re)` identity shortcut) instead of returning `undefined` from the noop thunk — lodash's `runInContext` module init relied on exactly this. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add affected paths and validation notes.
This fragment has root-cause detail, but omits paths and validation outcomes; also change “four” to “five” blockers. As per coding guidelines, “put root-cause details, paths, and validation notes there.”
🤖 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 `@changelog.d/7021-real-npm-cli-compile-gaps.md` around lines 1 - 7, Update the
changelog entry to describe five blockers instead of four, and add the affected
code paths or components for each fix, including the relevant runtime, zlib,
DOMException, panic-runtime, and RegExp areas. Add concise validation notes
documenting how each change was verified, while preserving the existing
root-cause details and structure.
Source: Coding guidelines
Comment on lines
+337
to
+358
| pub extern "C" fn js_dom_exception_subclass_init(this_value: f64, message: f64, name: f64) -> f64 { | ||
| let Some(exception) = value_as_ptr::<ObjectHeader>(this_value) else { | ||
| return undefined_value(); | ||
| }; | ||
| let message_ptr = optional_string_from_value(message, b""); | ||
| let name_ptr = optional_string_from_value(name, b"Error"); | ||
| let name_string = unsafe { | ||
| let len = (*name_ptr).byte_len as usize; | ||
| let data = (name_ptr as *const u8).add(std::mem::size_of::<StringHeader>()); | ||
| String::from_utf8_lossy(std::slice::from_raw_parts(data, len)).into_owned() | ||
| }; | ||
| set_event_field( | ||
| exception, | ||
| b"message", | ||
| crate::value::js_nanbox_string(message_ptr as i64), | ||
| ); | ||
| set_event_field( | ||
| exception, | ||
| b"name", | ||
| crate::value::js_nanbox_string(name_ptr as i64), | ||
| ); | ||
| set_event_field(exception, b"code", dom_exception_code(&name_string)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Root this, message, and name across GC-capable operations.
optional_string_from_value and set_event_field can allocate, but exception, message_ptr, and name_ptr are raw pointers rather than GC roots. A moving collection can invalidate them before later field writes, corrupting memory or crashing. Hold NaN-boxed handles in RuntimeHandleScope and reload rewritten values before each reuse.
As per coding guidelines, preserve GC safety with precise shadow-stack roots and reference rewriting. Based on learnings, raw Rust pointer locals are not GC roots across allocating operations.
🤖 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 `@crates/perry-runtime/src/event_target.rs` around lines 337 - 358, Update
js_dom_exception_subclass_init to root this_value, message, and name using
RuntimeHandleScope before calling optional_string_from_value or set_event_field.
Store NaN-boxed handles rather than raw pointers, and reload rewritten values
from the scope before deriving name_string and before each subsequent field
write so moving GC remains safe.
Sources: Coding guidelines, Learnings
Comment on lines
+47
to
+63
| if flags_undefined && pattern_value.is_pointer() { | ||
| let addr = (pattern.to_bits() & 0x0000_FFFF_FFFF_FFFF) as usize; | ||
| if crate::regex::is_regex_pointer(addr as *const u8) { | ||
| return pattern; | ||
| } | ||
| } | ||
| let pattern_ptr = if pattern_value.is_undefined() { | ||
| std::ptr::null_mut() | ||
| } else { | ||
| crate::builtins::js_string_coerce(pattern) | ||
| }; | ||
| let flags_ptr = if flags_undefined { | ||
| std::ptr::null_mut() | ||
| } else { | ||
| crate::builtins::js_string_coerce(flags) | ||
| }; | ||
| let re = crate::regex::js_regexp_new(pattern_ptr, flags_ptr); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the existing RegExp’s internal source when flags are supplied.
RegExp(/a/i, "g") must construct from source "a" and flags "g". This path string-coerces the RegExp instead, yielding its display form ("/a/i"), so it constructs a regex with the wrong source. Handle RegExp pointers before js_string_coerce and pass their stored source to js_regexp_new.
#!/bin/bash
set -euo pipefail
fd -t f . crates/perry-runtime/src |
xargs rg -n -C3 'fn js_regexp_new|is_regex_pointer|source.*regexp|regexp.*source'🤖 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 `@crates/perry-runtime/src/object/global_this/ctor_thunks.rs` around lines 47 -
63, The RegExp construction path must reuse an existing RegExp’s internal source
when explicit flags are provided. Update the logic around is_regex_pointer and
js_regexp_new to detect the RegExp pointer before js_string_coerce, retrieve its
stored source, and pass that source with the supplied flags; preserve the
existing fast return when flags are undefined.
Comment on lines
+620
to
+632
| for (const [name, cur] of Object.entries(matrix.modules)) { | ||
| const prev = base.modules[name] | ||
| if (!prev) continue // new module (e.g. after a node bump) — not a regression | ||
| for (const form of ['unprefixed', 'prefixed']) { | ||
| const wasSev = SEVERITY[prev[form].status] ?? 0 | ||
| const nowSev = SEVERITY[cur[form].status] ?? 0 | ||
| if (nowSev > wasSev) { | ||
| regressions.push(`${name} [${form}]: ${prev[form].status} -> ${cur[form].status}`) | ||
| } | ||
| } | ||
| if (prev.prefixParity === true && cur.prefixParity === false) { | ||
| regressions.push(`${name}: prefix-parity broke (M and node:M now diverge)`) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unguarded prev[form].status can crash --check.
Line 637 defensively uses prev.unprefixed?.status, but Line 624 dereferences prev[form] directly. A baseline entry missing a form (hand-edited, or written by an older/partial schema) throws a TypeError instead of reporting a comparison result.
🛡️ Proposed fix
for (const form of ['unprefixed', 'prefixed']) {
- const wasSev = SEVERITY[prev[form].status] ?? 0
- const nowSev = SEVERITY[cur[form].status] ?? 0
+ const wasSev = SEVERITY[prev[form]?.status] ?? 0
+ const nowSev = SEVERITY[cur[form]?.status] ?? 0
if (nowSev > wasSev) {
- regressions.push(`${name} [${form}]: ${prev[form].status} -> ${cur[form].status}`)
+ regressions.push(`${name} [${form}]: ${prev[form]?.status} -> ${cur[form]?.status}`)
}
}📝 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
| for (const [name, cur] of Object.entries(matrix.modules)) { | |
| const prev = base.modules[name] | |
| if (!prev) continue // new module (e.g. after a node bump) — not a regression | |
| for (const form of ['unprefixed', 'prefixed']) { | |
| const wasSev = SEVERITY[prev[form].status] ?? 0 | |
| const nowSev = SEVERITY[cur[form].status] ?? 0 | |
| if (nowSev > wasSev) { | |
| regressions.push(`${name} [${form}]: ${prev[form].status} -> ${cur[form].status}`) | |
| } | |
| } | |
| if (prev.prefixParity === true && cur.prefixParity === false) { | |
| regressions.push(`${name}: prefix-parity broke (M and node:M now diverge)`) | |
| } | |
| for (const [name, cur] of Object.entries(matrix.modules)) { | |
| const prev = base.modules[name] | |
| if (!prev) continue // new module (e.g. after a node bump) — not a regression | |
| for (const form of ['unprefixed', 'prefixed']) { | |
| const wasSev = SEVERITY[prev[form]?.status] ?? 0 | |
| const nowSev = SEVERITY[cur[form]?.status] ?? 0 | |
| if (nowSev > wasSev) { | |
| regressions.push(`${name} [${form}]: ${prev[form]?.status} -> ${cur[form]?.status}`) | |
| } | |
| } | |
| if (prev.prefixParity === true && cur.prefixParity === false) { | |
| regressions.push(`${name}: prefix-parity broke (M and node:M now diverge)`) | |
| } |
🤖 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/node_compat_matrix.mjs` around lines 620 - 632, Update the regression
comparison loop in the matrix-checking logic to safely read status from
prev[form] when a baseline form is missing, matching the defensive handling
already used for prev.unprefixed?.status. Preserve the existing severity
comparison and regression reporting for forms present in both baseline and
current entries.
Comment on lines
+112
to
+116
| for (const sri of integrities) { | ||
| if (!/^sha512-[A-Za-z0-9+/]+={0,2}$/.test(sri)) { | ||
| out.push(`${name}: integrity is not a sha512 SRI: ${sri}`) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
SRI regex accepts truncated digests.
sha512-AAAA passes this gate (your own test at external-tools.test.mts Line 282 relies on that shape reaching download). A base64 sha512 is always 86 chars + ==; validating the length turns a fat-fingered/truncated pin into a gate failure instead of a runtime integrity mismatch.
🛡️ Proposed fix
for (const sri of integrities) {
- if (!/^sha512-[A-Za-z0-9+/]+={0,2}$/.test(sri)) {
+ if (!/^sha512-[A-Za-z0-9+/]{86}==$/.test(sri)) {
out.push(`${name}: integrity is not a sha512 SRI: ${sri}`)
}
}📝 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
| for (const sri of integrities) { | |
| if (!/^sha512-[A-Za-z0-9+/]+={0,2}$/.test(sri)) { | |
| out.push(`${name}: integrity is not a sha512 SRI: ${sri}`) | |
| } | |
| } | |
| for (const sri of integrities) { | |
| if (!/^sha512-[A-Za-z0-9+/]{86}==$/.test(sri)) { | |
| out.push(`${name}: integrity is not a sha512 SRI: ${sri}`) | |
| } | |
| } |
🤖 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/soak/external-tools.mts` around lines 112 - 116, Update the SRI
validation in the integrities loop to require a complete base64-encoded SHA-512
digest: enforce the expected 86-character payload followed by “==” rather than
accepting any nonempty base64 text. Preserve rejection of malformed SRI values
and ensure truncated pins such as “sha512-AAAA” fail before download.
Comment on lines
+269
to
+272
| const channel = /^channel\s*=\s*"([^"]+)"/m.exec(toolchainToml)?.[1] | ||
| if (channel && !dockerBody.includes(`toolchain install ${channel} `)) { | ||
| out.push(`docker prebake: image does not pre-install the pinned toolchain ${channel}`) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The toolchain-channel check still uses the substring match you replaced above.
dockerBody.includes(\toolchain install ${channel} `)requires the channel to be the *first* arg and to be followed by a space — exactly the fragility called out in the comment at Lines 230-232.rustup toolchain install 1.93.0 stable --profile minimal`, or a channel at end-of-line, false-fails. Parse the arg list instead.
♻️ Proposed fix
- const installedToolchains = [...dockerBody.matchAll(/toolchain install ([^\\\n]+)/g)]
- .flatMap(m => m[1]!.trim().split(/\s+/))
- .filter(a => /^\d/.test(a))
+ const installArgs = [...dockerBody.matchAll(/toolchain install ([^\\\n]+)/g)].flatMap(m =>
+ m[1]!.trim().split(/\s+/),
+ )
+ const installedToolchains = installArgs.filter(a => /^\d/.test(a))
@@
- if (channel && !dockerBody.includes(`toolchain install ${channel} `)) {
+ if (channel && !installArgs.includes(channel)) {
out.push(`docker prebake: image does not pre-install the pinned toolchain ${channel}`)
}📝 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
| const channel = /^channel\s*=\s*"([^"]+)"/m.exec(toolchainToml)?.[1] | |
| if (channel && !dockerBody.includes(`toolchain install ${channel} `)) { | |
| out.push(`docker prebake: image does not pre-install the pinned toolchain ${channel}`) | |
| } | |
| const channel = /^channel\s*=\s*"([^"]+)"/m.exec(toolchainToml)?.[1] | |
| if (channel && !installArgs.includes(channel)) { | |
| out.push(`docker prebake: image does not pre-install the pinned toolchain ${channel}`) | |
| } |
🤖 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/soak/external-tools.mts` around lines 269 - 272, Update the pinned
toolchain validation around the channel extraction and dockerBody check to parse
rustup/toolchain install command arguments rather than using
dockerBody.includes. Detect the channel regardless of argument position or
whether it is followed by additional arguments or end-of-line, while preserving
the existing warning when the pinned channel is absent.
| what: 'cargo min-publish-age window', | ||
| saw: age ?? '(missing)', | ||
| wanted, | ||
| fix: `set [registry] global-min-publish-age = "${wanted}" (or run --fix)`, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
fix: text over-promises: --fix cannot add a missing cargo key.
fixCargoConfig is a pure replace on an existing global-min-publish-age line, so when the key is absent (the saw: '(missing)' path) --fix is a no-op while the finding tells the user to run it. Either scope the hint to the drift case or append the key like fixNpmrc does.
♻️ Option: append the key when absent (mirrors fixNpmrc)
export function fixCargoConfig(body: string): string {
+ if (!/^global-min-publish-age\s*=/m.test(body)) {
+ return body
+ }
return body.replace(
/^(global-min-publish-age\s*=\s*)"[^"]*"/m,
`$1"${SOAK_DAYS} days"`,
)
}…and drop (or run --fix) from the missing-key branch of checkCargoConfig, or implement [registry]-aware insertion.
Also applies to: 494-499
🤖 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/soak/soak.mts` at line 58, Correct the `fix:` guidance in
`checkCargoConfig` so it does not claim `--fix` can resolve the `saw:
'(missing)'` case while `fixCargoConfig` only replaces existing
`global-min-publish-age` lines. Either remove the `--fix` hint for missing keys
or update `fixCargoConfig` to append the key with appropriate `[registry]`-aware
insertion, matching `fixNpmrc`; keep the existing replacement behavior for drift
cases.
Comment on lines
+517
to
+546
| // Prune expired pins together with their annotation line. | ||
| const today = todayIso() | ||
| const lines = out.split('\n') | ||
| const drop = new Set<number>() | ||
| for (const entry of parseExcludeEntries(out)) { | ||
| // Prune only WELL-FORMED cleared annotations (same rule as | ||
| // staleExcludes): a wrong-arithmetic removable already in the past | ||
| // must surface as a check failure, not vanish silently. | ||
| // VERSION_PIN_RE too: the prune set must EQUAL the warn set | ||
| // (staleExcludes). Without it a bare-name / `@scope/*` standing-trust | ||
| // entry that merely sits under an expired annotation line was deleted | ||
| // by --fix — silently re-arming the soak for a whole scope, in a bot | ||
| // commit whose review story is "only annotation lines are touched". | ||
| if ( | ||
| entry.annotation && | ||
| VERSION_PIN_RE.test(entry.name) && | ||
| isValidIsoDate(entry.annotation.published) && | ||
| entry.annotation.removable === addDaysIso(entry.annotation.published, SOAK_DAYS) && | ||
| entry.annotation.removable < today | ||
| ) { | ||
| drop.add(entry.line - 1) | ||
| if (ANNOTATION_RE.test(lines[entry.line - 2]?.trim() ?? '')) { | ||
| drop.add(entry.line - 2) | ||
| } | ||
| } | ||
| } | ||
| if (drop.size > 0) { | ||
| out = lines.filter((_, i) => !drop.has(i)).join('\n') | ||
| } | ||
| return out |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Pruning the last exclude leaves an orphaned minimumReleaseAgeExclude: key.
Only entry lines (and their annotations) are dropped; the key line itself is never removed. If every pin under it has soaked, --fix / the soak-autofix workflow rewrites the yaml to a bare minimumReleaseAgeExclude: with no items, i.e. a null-valued pnpm setting — committed unattended by the bot. Drop the key when its block becomes empty.
🔧 Sketch: drop the key line when no entries survive
if (drop.size > 0) {
+ // If the block is now empty, drop the key line too — a bare
+ // `minimumReleaseAgeExclude:` is a null-valued pnpm setting.
+ const entries = parseExcludeEntries(out)
+ if (entries.every(e => drop.has(e.line - 1))) {
+ const keyIdx = lines.findIndex(l => /^minimumReleaseAgeExclude:\s*(?:#.*)?$/.test(l))
+ if (keyIdx !== -1) {
+ drop.add(keyIdx)
+ }
+ }
out = lines.filter((_, i) => !drop.has(i)).join('\n')
}📝 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
| // Prune expired pins together with their annotation line. | |
| const today = todayIso() | |
| const lines = out.split('\n') | |
| const drop = new Set<number>() | |
| for (const entry of parseExcludeEntries(out)) { | |
| // Prune only WELL-FORMED cleared annotations (same rule as | |
| // staleExcludes): a wrong-arithmetic removable already in the past | |
| // must surface as a check failure, not vanish silently. | |
| // VERSION_PIN_RE too: the prune set must EQUAL the warn set | |
| // (staleExcludes). Without it a bare-name / `@scope/*` standing-trust | |
| // entry that merely sits under an expired annotation line was deleted | |
| // by --fix — silently re-arming the soak for a whole scope, in a bot | |
| // commit whose review story is "only annotation lines are touched". | |
| if ( | |
| entry.annotation && | |
| VERSION_PIN_RE.test(entry.name) && | |
| isValidIsoDate(entry.annotation.published) && | |
| entry.annotation.removable === addDaysIso(entry.annotation.published, SOAK_DAYS) && | |
| entry.annotation.removable < today | |
| ) { | |
| drop.add(entry.line - 1) | |
| if (ANNOTATION_RE.test(lines[entry.line - 2]?.trim() ?? '')) { | |
| drop.add(entry.line - 2) | |
| } | |
| } | |
| } | |
| if (drop.size > 0) { | |
| out = lines.filter((_, i) => !drop.has(i)).join('\n') | |
| } | |
| return out | |
| // Prune expired pins together with their annotation line. | |
| const today = todayIso() | |
| const lines = out.split('\n') | |
| const drop = new Set<number>() | |
| for (const entry of parseExcludeEntries(out)) { | |
| // Prune only WELL-FORMED cleared annotations (same rule as | |
| // staleExcludes): a wrong-arithmetic removable already in the past | |
| // must surface as a check failure, not vanish silently. | |
| // VERSION_PIN_RE too: the prune set must EQUAL the warn set | |
| // (staleExcludes). Without it a bare-name / `@scope/*` standing-trust | |
| // entry that merely sits under an expired annotation line was deleted | |
| // by --fix — silently re-arming the soak for a whole scope, in a bot | |
| // commit whose review story is "only annotation lines are touched". | |
| if ( | |
| entry.annotation && | |
| VERSION_PIN_RE.test(entry.name) && | |
| isValidIsoDate(entry.annotation.published) && | |
| entry.annotation.removable === addDaysIso(entry.annotation.published, SOAK_DAYS) && | |
| entry.annotation.removable < today | |
| ) { | |
| drop.add(entry.line - 1) | |
| if (ANNOTATION_RE.test(lines[entry.line - 2]?.trim() ?? '')) { | |
| drop.add(entry.line - 2) | |
| } | |
| } | |
| } | |
| if (drop.size > 0) { | |
| // If the block is now empty, drop the key line too — a bare | |
| // `minimumReleaseAgeExclude:` is a null-valued pnpm setting. | |
| const entries = parseExcludeEntries(out) | |
| if (entries.every(e => drop.has(e.line - 1))) { | |
| const keyIdx = lines.findIndex(l => /^minimumReleaseAgeExclude:\s*(?:#.*)?$/.test(l)) | |
| if (keyIdx !== -1) { | |
| drop.add(keyIdx) | |
| } | |
| } | |
| out = lines.filter((_, i) => !drop.has(i)).join('\n') | |
| } | |
| return out |
🤖 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/soak/soak.mts` around lines 517 - 546, Update the pruning logic
around the drop set in the soak output fixer so that when all entries under
minimumReleaseAgeExclude are removed, the now-empty key line is removed as well.
Preserve the key when any exclude entry remains, and continue removing only
expired valid pins and their annotations.
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.
A breadth harness that measures Perry's
node:*builtin surface against a pinned, SRI-verified Node — everyrequire('module').builtinModulesentry, both thenode:Mand bareMimport forms. Infra only; touches no compiler code. The systematic version of tracker #812 (42-module behavioral matrix).Why pinned-binary, not source-checkout
Measuring runtime behavior needs a runnable Node; Node source is ~GB and would have to be built. So the oracle is the official nodejs.org dist tarball, pinned in
external-tools.jsonwith sha512 SRI (the repo's existing pin convention, as used for pnpm) — currently 26.5.1, independent of the.node-versiongap-suite oracle. All six platform tarballs were independently verified (sha256 vs upstream SHASUMS256, sha512 recomputed).What it checks, per module × per form
Export-shape fingerprint (sorted
name:typeofoverimport * as ns+default's typeof) — Perry vs the oracle. Shape, not deep behavior (behavioral cases stay in the node-suite). Three axes: does Perry resolve it, does the shape match, and doesM≡node:M.Fast loop (the reach-for-it command)
Documented fast-loop-first in
CLAUDE.md+llms.txt+docs/src/testing/node-compat-matrix.mdso it's discoverable.Real baseline (darwin-arm64, Node 26.5.1)
58 builtins probed · 21 both-forms exact-shape match · 35 shape-diff (partial surfaces, recorded) · 0 perry-unresolved · 0 prefix-parity divergences (the
M≡node:Minvariant holds everywhere — no Perry prefix bugs).Findings recorded, NOT fixed (infra PR — surfaced for follow-up):
sea,sqlite,test,test/reportersarenode:-prefix-only in Node 26 (bare throws) yet Perry also resolves the unprefixed form — a leniency divergence.trace_eventsmatches Node fully both forms but is absent fromperry-api-manifest::{NATIVE_MODULES,NODE_SUBMODULES}(works-but-unclaimed).Validation
cargo build --release -p perryok;cargo test -p perry --bin perry758 pass; soak-script tests 49 pass;external-tools.mts --check8 pins valid. Selector/--check-slice/method-subset-refusal/partial---update-baselineall validated. CI jobnode-compat-matrix.ymlruns--checkin its own job. Nocrates/changes.Summary by CodeRabbit
New Features
Bug Fixes
DOMExceptionsubclass behavior.RegExp()calls.Security & Reliability
Documentation