Skip to content

build: 7-day soak across toolchain and deps, cold_path() hints, pinned tooling#442

Open
jdalton wants to merge 30 commits into
nubjs:mainfrom
jdalton:build/rust-195-cold-path
Open

build: 7-day soak across toolchain and deps, cold_path() hints, pinned tooling#442
jdalton wants to merge 30 commits into
nubjs:mainfrom
jdalton:build/rust-195-cold-path

Conversation

@jdalton

@jdalton jdalton commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Gives nub one supply-chain safety rule — a 7-day "soak" — applied everywhere a dependency or tool can enter the repo: when a new version of anything is published, we wait 7 days before adopting it, so a compromised release gets caught by the ecosystem before we ever install it. Also brings workflow-security hardening (zizmor, 267 findings to zero), pinned-by-checksum external tooling, Socket Firewall in CI, and core::hint::cold_path() branch hints in the vendored install engine.

The soak, explained

"Soak time" = the minimum age a release must reach before we install it. Ours is 7 days, defined once in scripts/soak/constants.mts (SOAK_DAYS = 7); every config file below derives from or is checked against it:

Surface File What it controls
Rust toolchain rust-toolchain.toml Builds use a dated nightly that was ≥ 7 days old at adoption
Rust crates .cargo/config.toml cargo's -Zmin-publish-age skips crate versions younger than 7 days
npm deps tools/pnpm-workspace.yaml minimumReleaseAge: 10080 (same rule, in minutes)
npm CLI parity .npmrc min-release-age=7 for npm ≥ 11.17
Version bumps tools/taze.config.mts taze only proposes versions ≥ 7 days old
Renovate PRs .github/renovate.json minimumReleaseAge: "7 days", explicit — Renovate commits updated lockfiles, and -Zmin-publish-age skips already-locked versions, so Renovate itself is the only place this path can be soaked

scripts/soak/soak.mts --check fails CI if any of those drift; --fix rewrites them. The pnpm soak surfaces live in tools/ (not the repo root): a root pnpm-workspace.yaml marks the repo root as a workspace root, and nub's own workspace detection then redirects the node-version test matrix through the root engines — the exact false-green the matrix exists to catch. The root stays npm-only; a tracked fixture anchor plus a harness guard keep the matrix honest as defense-in-depth. Skipping the soak for one package requires a dated # published: YYYY-MM-DD | removable: YYYY-MM-DD comment above the exclude pin (removable = published + 7d), and the gate fails once the window passes — soaked pins must come off. taze itself is exact-pinned in package.json (npm can't parse the catalog: protocol and wpt-worker runs a root npm ci); the workspace catalog entry is the reference, and the gate fails if the two versions drift.

A repo skill (.claude/skills/soak/SKILL.md) documents the procedures — change the window, opt out, add an exclusion, bump the nightly — so agent sessions don't re-derive them; it follows Anthropic's skill-authoring + prompting guidance and links it for future edits.

Workflow security: 267 zizmor findings to zero

zizmor (a GitHub Actions security linter) is now a CI gate (zizmor.yml), so all 21 existing workflows were brought to zero findings first:

  • persist-credentials: false on every checkout that doesn't push (the homebrew-tap bump keeps its credentialed checkout, with an inline ignore explaining why).
  • Explicit permissions: blocks — workflow-level {} plus per-job scopes matching only what each job's steps actually do.
  • Every template-injection site (28 in release.yml alone) moved to env-var indirection: ${{ matrix.foo }} inside a shell script becomes a step-level env: var referenced as "$FOO", so a crafted value can't execute as shell.
  • Deliberate residuals carry inline # zizmor: ignore[rule] reason comments (e.g. caches on gate-only jobs that never feed published artifacts).

No behavior changed — each job kept every permission its steps use.

Pinned tools, Socket Firewall, and the docker context

external-tools.json pins pnpm 11.8.0, npm 12.0.0, sfw, zizmor, agentshield, and skillspector by exact version + sha512 SRI checksum; scripts/soak/external-tools.mts is the only install path (download → verify → rack → PATH symlink). npm-packaged tools get their deps installed by nub itself first (dogfooding), prod-only with scripts skipped.

CI installs the pinned pnpm + npm, then writes firewall shims: bash wrappers named npm/pnpm/cargo/… that route the real command through sfw, wrapping the pinned binaries directly. SOCKET_SECURITY_KEY is the only credential env var — not set yet, which is fine: sfw's free tier is keyless, and the enterprise tier engages automatically when the secret lands.

untracked 1.6.4 (pinned via the same catalog rule) maintains a generated section in .dockerignore to keep docker build contexts small; a package.json whitelist rescues the load-bearing .cargo configs (crates/nub-native/.cargo routes in-container build output to the shared target dir — the docker-smoke builders depend on it).

Toolchain + cold-path hints

rust-toolchain.toml pins nightly-2026-07-04 (newest nightly ≥ 7 days old at adoption; nightly is what activates -Zmin-publish-age), with rust-version moving 1.88 → 1.95 as the stable floor. core::hint::cold_path() (stable since 1.95) marks rare branches in the vendored install engine's hot loops — the per-file link fallbacks, lockfile-parser bails, semver cache misses, and tar validation errors — so the optimizer keeps the common path tight. Hints only; no behavior change.

Testing + review trail
  • 63 node:test unit tests cover the soak checkers, fixers, parsers, and CLIs (pnpm run test:scripts, also in the CI gate). Review findings live on as regression cases (impossible calendar dates, expired bypasses, flow-style exclude lists, catalog drift, the --npm --cargo selector bug), and the security-relevant paths are pinned by tests: the downloader sends GITHUB_TOKEN to github.com only and rejects any checksum mismatch, the sfw flavor flips on SOCKET_SECURITY_KEY, and both CLIs round-trip end-to-end through their entrypoints.
  • Full cargo test green. The two registry-mock test failures first blamed on main turned out to be the Socket Firewall shim MITM-proxying cargo's local-registry mocks; the Test job now sets SFW_SHIM_ACTIVE_CARGO=1 so the shim execs the real cargo for the mock-registry suites while staying active for everything else.
  • Every review-bot finding was fixed, replied to, and resolved — including the standout: writing a shim through its symlink handle overwrote the pinned binary with the shim script, which then executed itself forever. The shim writer now removes the handle first, and a CI step asserts pnpm/npm resolve through the shims on every run.
  • Static skill scanning flagged two real rough edges in skill docs, both hardened (an rsync --delete that could empty a vendor tree on a failed staging step; a cache-wipe recipe that now fails loudly on an empty path substitution).
Hardening rounds after opening (CI + review driven)
  • Windows CI, four quirks deep: tar parses C:\… as a remote host (extract cwd-relative); Git Bash's GNU tar shadows System32 bsdtar and can't read zip (invoke %SystemRoot%\System32\tar.exe); a copied handle strands pnpm's SEA launcher from its dist/ siblings (handles now forward to the absolute rack target via .cmd + bash shims); and the forwarder must strip .exe from the handle base — pwsh resolves pnpm to pnpm.exe first, and a bash script wearing that name is "not a valid application for this OS platform".
  • zizmor follow-ups: scan scoped to .github/ (the vendored aube workflows are inert); 24 dtolnay/rust-toolchain sites repinned — the old SHA was genuine but left dangling by an upstream force-push (impostor-commit finding); docker action pins carry exact version comments (ref-version-mismatch).
  • commit-msg hook: a C-style comment in an awk END block broke BSD awk on every commit, silently skipping the blank-line collapse while the visible half (agent-trailer strip) kept working. Fixed, and the hook's behavior is now pinned by tests that assert clean stderr on every case.
  • Remote map as code: scripts/soak/remotes.mts declares where PRs live vs where branches push (--check/--fix/--print), so tooling reads it instead of rediscovering it.

jdalton added 2 commits July 11, 2026 13:20
Exact stable pin via rust-toolchain.toml so all four workspaces (nub,
nub-native, nub-phantom, vendor/aube) build with one toolchain. 1.95
is the floor for core::hint::cold_path().
core::hint::cold_path() (stable since rust 1.95) annotates the rare
arm so the optimizer sinks it off the hot path. Sites: the per-file
materialize loops' reflink/hardlink fallbacks and link-error returns
(project-local and GVS staging), the pnpm-lock byte-cursor subset
parser's decline-to-serde bails, the thread-local semver range and
version cache misses, and the per-entry tar walk's validation
rejects. No behavior change; hints only.
@vercel

vercel Bot commented Jul 11, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nub Ready Ready Preview, Comment Jul 15, 2026 7:59pm

Request Review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — two minor observations, both non-blocking.

Reviewed changes — pins an exact stable toolchain (rust-toolchain.toml, 1.95.0), bumps every workspace rust-version to 1.95, and adds core::hint::cold_path() codegen hints to rare arms of hot install loops in the vendored aube engine. Hints only, no behavior change.

  • Pin the toolchain to 1.95.0 — new repo-root rust-toolchain.toml fixes channel = "1.95.0" plus rustfmt/clippy so all four workspaces build on one compiler.
  • Bump rust-version to 1.95Cargo.toml, crates/nub-native/Cargo.toml, crates/nub-phantom/Cargo.toml (1.88 → 1.95) and vendor/aube/Cargo.toml (1.93 → 1.95). 1.95.0 is the floor for cold_path().
  • Mark cold branches with cold_path() — link fallbacks in materialize.rs, decline-to-serde/indent-mismatch bails in the pnpm subset parser (subset.rs), semver cache misses in semver_util.rs, and tar-walk validation rejects in tarball.rs.

I verified the load-bearing claim: core::hint::cold_path() is stable since 1.95.0 (const fn), and 1.95.0 is a real stable release (16 Apr 2026). The placements are sound — the cache-miss arms in semver_util.rs are documented as trending to a ~1.0 hit rate after the first BFS layer, so marking the miss path cold is justified.

ℹ️ Stale MSRV in the vendored aube docs

Bumping vendor/aube/Cargo.toml from rust-version = "1.93" to "1.95" leaves the vendored aube's own agent docs claiming MSRV 1.93. Those docs aren't in this diff, but this change is what makes them stale — and the aube-parity doctrine keeps the vendored tree self-consistent so it stays conceivably upstreamable, so the drift is worth closing in the same PR.

Technical details
# Stale MSRV in vendored aube docs after rust-version bump

## Affected sites
- `vendor/aube/.rules:6``msrv: 1.93` (mirrored by `vendor/aube/AGENTS.md`)
- `vendor/aube/.rules:190``edition 2024, latest stable toolchain, msrv 1.93 (...)`

## Required outcome
- The vendored aube MSRV documentation reflects 1.95, matching `vendor/aube/Cargo.toml`'s `rust-version`.

## Open questions for the human
- Whether to update the aube-side docs here or leave them for a dedicated vendor-sync commit. Either is defensible; the risk of leaving them is a future upstream diff carrying a contradictory MSRV.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread rust-toolchain.toml Outdated
Adopt the newest nightly at least 7 days old as of 2026-07-11; bump
the pin under the same soak rule. rust-version stays 1.95 as the
stable msrv floor. Verified on nightly-2026-07-04: clippy
--all-targets -D warnings clean across nub and vendor/aube.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The repo-root pin now propagates a nightly toolchain into the release build, so shipped binaries compile on nightly. Worth an explicit decision before merging.

Reviewed changes — since the prior pullfrog review, the single incremental change swaps the repo-root toolchain pin from stable 1.95.0 to nightly-2026-07-04 (with a 7-day-soak adoption comment). The rust-version bumps to 1.95 and the cold_path() hints are unchanged from the prior review.

  • Switch the toolchain pin to nightlyrust-toolchain.toml now pins channel = "nightly-2026-07-04" instead of stable 1.95.0; the comment documents a "newest nightly ≥7 days old" adoption/bump policy.

⚠️ Shipped release binaries now build on nightly

Every CI workflow installs dtolnay/rust-toolchain@... # stable, but the repo-root rust-toolchain.toml overrides that for all cargo invocations. The per-platform release build runs at repo root, so the 8-platform binaries published to npm now compile on nightly-2026-07-04 rather than stable. Nightly carries weaker stability and soundness guarantees than stable; making it the compiler for distributed artifacts is a posture change worth an explicit, conscious decision rather than a side effect of the dev-toolchain pin.

Technical details
# Release binaries compile on the pinned nightly

## Affected sites
- `rust-toolchain.toml:8``channel = "nightly-2026-07-04"` at repo root applies to all cargo invocations.
- `.github/workflows/release.yml:466-471``cargo build --release -p nub-cli --features embed-runtime --target <t>` / `cross build --release ...` build the shipped binaries at repo root, so the pin applies. `cross` reads `rust-toolchain.toml` and provisions the pinned toolchain in-container, so the container path is not an escape.
- `.github/workflows/release.yml:266` and every other workflow — `dtolnay/rust-toolchain@... # stable` installs stable but the pin supersedes it for cargo.

## Required outcome
- A deliberate decision on whether production release binaries should build on a pinned nightly, and if so, that intent recorded where the release toolchain is chosen (not only in the dev-tooling comment).

## Open questions for the human
- Is nightly intended to apply to the release build, or only to local/CI dev builds? If the latter, the release job could pin an explicit stable toolchain (e.g. a scoped `+stable` / `RUSTUP_TOOLCHAIN`), decoupling ship-toolchain from dev-toolchain.

ℹ️ rust-version = 1.95 is no longer verified by any build

Nothing in the tree requires nightly to compile — the stated justification (core::hint::cold_path()) is stable since 1.95.0, and there are no #![feature(...)] gates anywhere. With the toolchain pinned to nightly and rust-version declaring a stable-1.95 floor, no build actually exercises stable 1.95, so a nightly-only construct or a stable-1.95 regression would pass CI undetected. The declared MSRV becomes aspirational rather than enforced.

Technical details
# MSRV floor is decoupled from the build toolchain

## Affected sites
- `Cargo.toml:95`, `crates/nub-native/Cargo.toml:34`, `crates/nub-phantom/Cargo.toml:22`, `vendor/aube/Cargo.toml:23``rust-version = "1.95"`.
- `rust-toolchain.toml:8` — build toolchain is `nightly-2026-07-04`, not stable 1.95.

## Required outcome
- Either an MSRV-verification signal (a job that builds on stable 1.95 / `rust-version`), or an explicit acknowledgement that `rust-version` is a documentation-only floor while the build tracks nightly.

## Open questions for the human
- Is a nightly toolchain actually needed, given `cold_path()` is stable on 1.95? If not, pinning stable 1.95 keeps ship-toolchain, dev-toolchain, and the declared MSRV consistent — which was the state at the prior review.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

Crate versions published less than 7 days ago are pubtime-incompatible
and the resolver skips them unless already in Cargo.lock. Unstable
cargo feature, active via the nightly pinned in rust-toolchain.toml;
stable cargo ignores the [unstable] table. Unignore the root
.cargo/config.toml the same way as the nub-native one.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — the incremental change is correct; one observation ties it to the still-open nightly-posture discussion.

Reviewed changes — since the prior pullfrog review, the single incremental change adds a repo-root .cargo/config.toml enabling cargo's min-publish-age unstable feature and the .gitignore allow-entries needed to track it. The nightly pin, rust-version bumps, and cold_path() hints are unchanged from the prior review.

  • Enable a min-publish-age supply-chain soak — new .cargo/config.toml sets [unstable] min-publish-age = true and [registry] global-min-publish-age = "7 days", so cargo's resolver skips crate versions published in the last 7 days unless already in Cargo.lock, mirroring the PM's minimumReleaseAge discipline.
  • Track the root .cargo/ past the broad ignore.gitignore adds !.cargo/ and !.cargo/config.toml, matching the existing crates/nub-native/.cargo/ allow-entry so the broad .* rule doesn't drop the config.

I verified the config against the Cargo Book's unstable-features page: [unstable] min-publish-age = true is the correct enabling key, global-min-publish-age accepts the "N days" format, and resolver.incompatible-publish-age defaults to "deny" (ignore too-new versions unless already locked) — so the comment's claims are accurate and the file is well-formed.

ℹ️ The soak config makes the nightly pin load-bearing, reinforcing the open nightly-posture question

min-publish-age is a nightly-only cargo feature — the [unstable] table is silently ignored on stable cargo. Before this commit, the prior review noted nothing in the tree actually required nightly (cold_path() is stable on 1.95). This commit changes that: the build config now genuinely depends on nightly, so reverting to stable 1.95 would silently no-op the soak rather than fail. This doesn't introduce a bug — it strengthens the premise of the two still-open findings from the prior review (shipped binaries building on nightly, and rust-version = 1.95 not being verified by any build), which remain human decisions.

Technical details
# min-publish-age couples the supply-chain soak to the nightly pin

## Affected sites
- `.cargo/config.toml:7-8``[unstable] min-publish-age = true` is nightly-only; ignored by stable cargo.
- `rust-toolchain.toml:8``channel = "nightly-2026-07-04"` is what makes the `[unstable]` key active.

## Required outcome
- No code change required — the config is correct. This is a note that the nightly pin is now load-bearing for the soak, not only a policy convenience for the (stable) `cold_path()` hints.

## Open questions for the human
- The prior review's open questions still stand and are now coupled: if release binaries were to pin stable to decouple ship-toolchain from dev-toolchain, the `min-publish-age` soak would silently stop applying to that build. Worth deciding whether the soak should hold on the release path specifically.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

@jdalton

jdalton commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

On the nightly-posture question: deliberate. The dated-nightly pin (7-day soak on toolchain adoption) is intentionally load-bearing for the -Zmin-publish-age soak; decoupling release builds to stable would silently drop the soak there, so the release path keeps the pinned nightly. rust-version=1.95 stays the declared stable floor. Tracking: rust-lang/cargo#15973 (now rust-lang/cargo#17009) — revisit the pin when min-publish-age stabilizes.

jdalton added 2 commits July 11, 2026 14:30
scripts/soak/constants.mts is the one source for the release-age
window; soak.mts parity-checks and fixes every surface (.cargo
min-publish-age, pnpm-workspace.yaml minimumReleaseAge in minutes,
.npmrc min-release-age in days, taze maturityPeriod via direct
import) and enforces lockstep between catalog entries and
catalog-shadowed exact pins. Version-pinned soak excludes require a
'# published: | removable:' date annotation and expired pins are
pruned by --fix. update-deps.mts bumps npm (taze) and cargo through
the same window, refusing a non-rustup cargo that would silently
skip the [unstable] soak. external-tools.json pins external security
tools exact with sha512 SRI; external-tools.mts is the only install
path; paths.mts is the single declaration of every location the
scripts touch. taze is exact-pinned in package.json (npm cannot parse
the catalog: protocol and wpt-worker runs a root npm ci) with the
catalog entry as the enforced reference.
…d zizmor

docs-links runs soak.mts --check and external-tools.mts --check; the
test job installs sfw and its package-manager shims before pnpm
install. The SOCKET_SECURITY_KEY repo secret (not yet set) feeds
SOCKET_API_KEY + SOCKET_API_TOKEN; until it lands sfw runs its
keyless free tier. zizmor.yml lints workflow files on PRs.
The taze config sets write: true, so a dry run must pass --no-write
explicitly or it rewrites package.json; also suppress the ni install
prompt with --no-install.
@jdalton jdalton changed the title build: pin rust 1.95.0 toolchain and adopt cold_path() branch hints build: 7-day soak across toolchain and deps, cold_path() hints, pinned tooling Jul 11, 2026
A hung release or registry response would stall CI indefinitely;
abort after 120s (generous for the largest pinned binary on a slow
runner).
- reject flow-style minimumReleaseAgeExclude (invisible to the block
  parser = unvalidated bypass) and parse entries with trailing
  comments; catalog parity tolerates blank lines in the block
- entrypoint guard uses realpath + pathToFileURL so symlinked or
  space-containing checkouts don't silently no-op the gate
- linkHandle force-removes dangling handles (EEXIST wedge)
- extract .zip release assets via bsdtar and verify the binary landed
- GITHUB_TOKEN only sent to github.com, never the npm registry
- npm-purl installs key completion off the extracted manifest so an
  interrupted extract self-heals
- update-deps runs every requested ecosystem and reports spawn errors
- sfw install/shims skip cleanly on windows (POSIX-only mechanism)
- Socket credential is SOCKET_SECURITY_KEY everywhere — no
  SOCKET_API_* aliasing in CI env or the enterprise-tier check

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The socket firewall doesn't actually take effect on the Windows CI leg, so pnpm install there runs unfirewalled without any signal. Worth addressing before merge.

Reviewed changes — since the prior pullfrog review (1012a75), this run covers the two new commits that add the supply-chain soak machinery and route CI installs through a firewall. The nightly pin, rust-version bumps, .cargo/config.toml, and cold_path() hints are unchanged.

  • Add the scripts/soak/ cooldown manager — one canonical SOAK_DAYS in constants.mts; soak.mts parity-gates .cargo/config.toml / .npmrc / pnpm-workspace.yaml / taze.config.mts against it, enforces annotated-exclude expiry, and checks catalog↔package.json lockstep.
  • Add pinned external security toolingexternal-tools.json pins sfw, zizmor, agentshield, skillspector to exact versions with per-platform sha512; external-tools.mts downloads, SRI-verifies, installs, and writes sfw firewall shims for npm/yarn/pnpm/pip/uv/cargo.
  • Wire the firewall + gates into CIci.yml installs the sfw shims and prepends the rack bin to $GITHUB_PATH before pnpm install, and feeds SOCKET_API_KEY/SOCKET_API_TOKEN from a repo secret; the docs job runs the soak + tool-pin --check gates.
  • Add the zizmor workflow-security lint — new zizmor.yml with permissions: {} at workflow scope and persist-credentials: false.
  • Add taze as a soaked dependency updater — devDep + catalog entry; update-deps.mts runs taze then pnpm install, and cargo update only via the rustup shim so the min-publish-age soak applies.

The prior review's inline thread on the # stable CI / nightly / MSRV interaction was addressed by the author's "Deliberate" reply and resolved, so those posture questions are settled and not re-raised here.

ℹ️ Nitpicks

  • scripts/soak/update-deps.mts:64-73 — passing both --npm and --cargo makes !onlyCargo and !onlyNpm both false, so neither ecosystem updates and the script exits 0. Reject the combination or treat the flags as additive selectors.
  • .github/workflows/ci.yml:547-549SOCKET_API_KEY/SOCKET_API_TOKEN are set at job scope, so the Socket secret is ambient to cargo build/cargo test (which run third-party build scripts) and the side-Node curl steps too. Scoping to just the shim + install steps would cut the blast radius.
  • scripts/soak/external-tools.mts:154-161,203-210 — the tar -xzf extraction has no path-traversal/entry-type guard, unlike this repo's own vendor/aube/crates/aube-store/src/tarball.rs. Defense-in-depth only (requires a prior SRI/pin compromise), but the asymmetry is worth a note.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread .github/workflows/ci.yml Outdated
Comment thread scripts/soak/constants.mts
Comment thread scripts/soak/external-tools.mts
Comment thread scripts/soak/external-tools.mts
untracked 1.6.4 (catalog-shadowed pin, lockstep-enforced) maintains a
generated exclusion section in .dockerignore between markers
(pnpm run dockerignore:update), shrinking the docker build context
for the from-source images; the package.json whitelist rescues the
load-bearing .cargo configs (crates/nub-native/.cargo routes
in-container build output to the shared target dir). external-tools
--check gains a docker-prebake parity gate (null surface here — nub's
docker images are product artifacts, not dev-tool prebakes).

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in the fix commits. Two prior findings remain open (both latent).

Reviewed changes — this run covers the three fix commits pushed in response to the prior review, which harden the soak gates and the external-tool installer. The delta introduces no new issues.

  • Skip the sfw firewall cleanly on WindowsinstallTool and writeShims short-circuit on win32 with a log line, so the POSIX-only shim mechanism is an explicit, visible gap rather than a silent fail-open on the Windows CI leg.
  • Scope the GITHUB_TOKEN to GitHub hostsdownload() attaches the authorization header only when the URL host is github.com, so the token is no longer sent to the npm registry on the purl path.
  • Make update-deps flags additive and the dry run actually dry — passing both --npm and --cargo now runs both, and --dry-run overrides the taze config's write: true with --no-write --no-install.
  • Harden the installer — download timeout (AbortSignal.timeout), .zip extraction plus a missing-binary check, dangling-symlink handling, and a manifest-based completion marker so an interrupted npm extract self-heals.
  • Tighten the soak parser — reject flow-style minimumReleaseAgeExclude: [...] (invisible to the block parser), tolerate trailing comments on exclude entries, handle blank lines inside the catalog: block, and make isMain detection use realpath + pathToFileURL.

The two ⚠️/ℹ️ threads the fix commits addressed (Windows firewall skip, GITHUB_TOKEN host-gating) are resolved. Two informational threads remain open — addDaysIso throwing on a shape-valid-but-invalid date, and checkPins not flagging an expired soakBypass. Both are latent (no current data exercises either), so they don't block merge, but they're left open since the code doesn't yet address them.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

jdalton added 2 commits July 11, 2026 15:20
267 findings to zero at the default persona: persist-credentials:
false on every checkout that doesn't push (the homebrew-tap bump
keeps its credentialed checkout, inline-ignored with the reason);
workflow-level permissions: {} with per-job scopes matched to what
each job's steps actually use; all template-injection sites moved to
env-var indirection (28 in release.yml alone, zero behavior change);
adhoc package installs documented as already env-pinned; deliberate
residuals carry inline zizmor ignores with per-site verified reasons
(cache-poisoning on gate-only jobs, secrets-inherit where docker.yml
declares no workflow_call secrets).
Same mechanism as the sibling repos: SRI-pinned pnpm/npm in
external-tools.json (npm 12.0.0 carries a dated soakBypass
annotation), CI installs both before the firewall shims, shims wrap
the pinned binaries directly, and npm-packaged external tools
materialize runtime deps through nub itself first (pnpm/npm
fallbacks), prod-only with scripts skipped. Also hardens two skill
docs flagged by static skill scanning: the vendor-sync rsync
--delete now guards against an empty staging tree, and the bench
cache-wipe recipe fails loudly on an empty cache-path substitution.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The new pnpm/npm firewall-shim wiring overwrites the pinned pnpm/npm wrapper it depends on, so pnpm/npm cannot resolve through the shims on the Linux and macOS test legs. One inline finding; worth fixing before merge.

Reviewed changes — since the prior pullfrog review (7e422e3), this run covers the two new commits: workflow-security hardening across all workflows, and pinning pnpm/npm into the tool rack so CI installs run through the pinned binaries + the socket firewall.

  • Harden every workflow against zizmor findings — workflow-level permissions: {}, per-job least-privilege scopes, persist-credentials: false on all checkouts, SHA-pinned actions, and env-var indirection for every ${{ github.* }}/${{ steps.* }} interpolation in run: steps (values now read from a step env: block), with justified inline zizmor: ignore on the gate-only cache and vendored-template cases.
  • Pin pnpm 11.8 + npm 12 via the tool rackexternal-tools.json adds a per-platform pnpm SEA (plus a darwin-x64 registry .tgz fallback) and a single npm registry tarball with a soakBypass; external-tools.mts factors the registry-tarball install into a shared installNpmTarball and materializes tool deps through PM_DEP_INSTALLERS (dogfooding nub install --prod first, then pnpm/npm).
  • Route CI through the pinned PMs — the test job installs pnpm/npm/sfw and writes the firewall shims in one call, then asserts pnpm --version/npm --version resolve.

The zizmor hygiene commit is mechanical and correct — the env-var-indirection rewrites preserve the original interpolated values and the inline ignores are individually justified. The prior two latent threads (addDaysIso on an invalid date, checkPins not flagging an expired soakBypass) are unchanged by these commits and remain open.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread scripts/soak/external-tools.mts
…unit tests

writeShims wrote THROUGH the rack symlink handle, overwriting the
pinned binary with the shim body — which then exec'd itself forever.
Remove the handle before writing. Windows .zip archives resolve
<bin>.exe before failing, and a failed extract cleans its dir so
retries re-extract. Annotation and soakBypass dates must be real
calendar dates (finding, not a RangeError crash), and an expired
soakBypass is flagged like any other soaked pin. node:test unit
tests cover the checkers, fixers, and parsers (pnpm run
test:scripts; also in the CI gate).

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — the single new commit 0bf3643 relocates the pnpm-side soak domain out of the repo root into tools/ and makes the repo root npm-managed, so a root workspace yaml no longer marks the repo as a nub workspace and redirects the test matrix's older-Node legs.

  • Move the pnpm-soak domain into tools/ — new tools/package.json (taze: catalog:), tools/pnpm-workspace.yaml (catalog + minimumReleaseAge), and tools/pnpm-lock.yaml; taze.config.mts moves alongside and its constants.mts import becomes ../scripts/....
  • Make the repo root npm-managed — the root pnpm-lock.yaml is deleted and gitignored, the root package-lock.json gains the untracked dev tree, and the CI test/rust jobs plus release.yml swap the root pnpm install --frozen-lockfile for npm ci (and setup-node cache pnpmnpm), matching the PM wpt-worker's root npm ci already uses.
  • Repoint the soak surfacespaths.mts moves SURFACES.workspaceYaml/tazeConfig under tools/ and sets NPM_PKG_DIR to tools/, so the catalog↔package.json parity gate reads the relocated files.

I verified the split is internally consistent. The only remaining root pnpm install --frozen-lockfile (ci.yml:936) is scoped to working-directory: site, so there is no orphaned root pnpm install; the docs job runs pnpm run soak/tools:check/test:scripts as plain node-script execution (no install needed, and every surface they read is tracked); and the root runtime deps still install for the data-format loader tests via the new root npm ci. The checkCatalogParity gate reads tools/package.json whose taze: catalog: spec correctly no-ops.

The incremental range-diff is heavily polluted by reorderings (the whole pnpm-lock delete/re-add and some function-order churn in soak.mts/update-deps.mts) but the substantive net change is only the tools/ relocation. All prior pullfrog threads are resolved. No new issues.

Pullfrog  | View workflow run | Using Claude Opus𝕏

- dtolnay/rust-toolchain@29eef336 is a real upstream commit but
  DANGLING: dtolnay force-pushes the stable branch each release, so the
  old head is unreachable from any ref and the new zizmor gate's online
  impostor-commit audit rejects it (24 sites). Repin to the current
  stable head 4be7066a.
- tar on Windows parses an absolute C:\ archive path as a remote
  host:path ("Cannot connect to C") — extract with a cwd-relative
  archive name, which unblocks the pinned-pnpm install on the
  windows-latest leg

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — the single new commit 5b3524e is a CI/build fix: it re-pins the dtolnay/rust-toolchain action to a reachable stable head and makes the external-tool archive extraction work on Windows.

  • Re-pin dtolnay/rust-toolchain to a reachable commit — every @29eef336 usage across 24 sites in 9 workflows moves to @4be7066a (comment still # stable). The old SHA is dangling — dtolnay force-pushes the stable branch each release, so it's unreachable from any ref and the new zizmor impostor-commit audit rejects it.
  • Extract archives with a cwd-relative name on WindowsextractArchive now passes the bare asset name with cwd: destDir instead of an absolute path, because tar on Windows parses C:\... as a remote host:path and fails, which was blocking the pinned-pnpm install on the windows-latest leg.

I verified both load-bearing claims. 4be7066a is a real GPG-verified dtolnay commit (2026-06-30, message toolchain: stable) on dtolnay/rust-toolchain, so the re-pin points at a legitimate current stable head. The tar change is behavior-preserving on every platform: the only two callers pass plat.asset and the literal package.tgz — plain filenames with no directory components — and the archive is written at path.join(destDir, asset), so the cwd-relative name resolves to the same file and extracts into the same directory (rmSync still uses the absolute path).

The incremental range-diff renders the toolchain change as noisy add/remove pairs, but the commit diff confirms it is a clean one-line SHA replacement per site. All prior pullfrog threads are resolved. No new issues.

Pullfrog  | View workflow run | Using Claude Opus𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — the single new commit 872c0a0 refines the Windows path of extractArchive so the pinned-pnpm install extracts correctly on the windows-latest leg.

  • Invoke System32 bsdtar explicitly on WindowsextractArchive now runs %SystemRoot%\System32\tar.exe (fallback C:\Windows\System32\tar.exe) on win32 instead of a bare tar, because Git Bash's PATH shadows the System32 bsdtar with GNU tar, which can't read the pnpm SEA .zip. The relative-name-with-cwd extraction from the prior fix is preserved, and non-Windows platforms still use bare tar unchanged.

I verified the change is behavior-preserving off Windows (the tarBin ternary yields 'tar'), and that both callers plus the test still pass plain filenames (plat.asset, 'package.tgz', 'a.tar.gz') with no directory components, so the cwd: destDir addressing resolves to the written archive. Using an absolute executable path with spawnSync and no shell launches the binary directly, which is correct. This is only fully provable on the windows-latest test leg.

Pullfrog  | View workflow run | Using Claude Opus𝕏

…siblings

pnpm's Windows SEA launcher resolves dist/pnpm.mjs relative to the
invoked handle's directory, so a symlinked/copied bin handle crashes
with MODULE_NOT_FOUND. On win32 the handle is now a forwarding pair
(.cmd for cmd/pwsh + extensionless bash shim for Git Bash) that invokes
the absolute rack target; registry-tarball tools forward through node
directly instead of a bash-only wrapper pwsh can't run.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in the Windows rack-handle rework — one minor test-portability nit inline, non-blocking.

Reviewed changes — the single new commit c000bb5 reworks how the tool rack materializes its BIN_DIR handles on Windows, so the pinned pnpm/npm installs resolve on the windows-latest test leg.

  • Forward Windows handles to the rack target instead of symlinkinglinkHandle now writes a .cmd (for cmd/pwsh) plus an extensionless bash forwarder (for Git Bash) on win32, dispatching directly to a SEA .exe or via node for a non-.exe entry script. A symlinked/copied handle strands a Windows SEA's dist/ siblings, so the absolute rack target is addressed instead.
  • Link the npm-tarball bin through linkHandle on WindowsinstallNpmTarball calls linkHandle(binAbs, name) directly on win32 rather than writing a bash-only <name>-wrapper (which would strand under pwsh); the POSIX path keeps the -wrapper + linkHandle shape unchanged.

The rework is coherent: writeShims/rackRealFor and the sfw install all early-return on win32, so the now-absent <name>-wrapper on Windows can't break shim resolution (shims never run there), and the CI assert step resolves the .cmd via PATHEXT under pwsh after BIN_DIR is prepended to $GITHUB_PATH. The non-Windows path is untouched. This is only fully provable on the windows-latest test leg.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

rmSync(bin)
const bin2 = plantRackBinary('fakea', '2.0.0')
linkHandle(bin2, 'fakea')
assert.equal(readFileSync(path.join(BIN_DIR, 'fakea'), 'utf8').includes('real-binary'), true)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ This test isn't win32-guarded, unlike the two writeShims tests below (lines 55, 72). The new win32 branch in linkHandle writes a bash-forwarder handle (exec node "...") rather than a symlink to the binary, so this includes('real-binary') assertion fails when test:scripts runs on a Windows host. Harmless in CI (the docs job runs it on ubuntu-latest only), but it breaks a local Windows dev run — worth the same t.skip('...') guard for consistency.

Technical details
# `linkHandle` test lacks the win32 skip its siblings have

## Affected sites
- `scripts/soak/rack.test.mts:32-42` — the `linkHandle replaces existing and DANGLING handles` test reads `BIN_DIR/fakea` and asserts it contains `real-binary`; on `win32` the handle is now a `#!/usr/bin/env bash\nexec node "<target>"` forwarder, so the assertion fails.
- `scripts/soak/external-tools.mts:238-258` — the new `win32` branch (this commit) that makes the handle a forwarder rather than a symlink-to-content.

## Required outcome
- The test either skips on `win32` (matching lines 55 and 72) or asserts the forwarder content on `win32`, so a local Windows `pnpm test:scripts` stays green.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in the Windows handle-name fix — one prior test-portability nit remains open (non-blocking).

Reviewed changes — the single new commit 744f1ba is a Windows-only fix that strips a caller-supplied .exe suffix from the tool-rack handle base name so the pinned pnpm/npm resolve on the windows-latest test leg.

  • Strip .exe from the win32 handle base — a Windows SEA install resolves binName to pnpm.exe, so linkHandle previously wrote the handle as pnpm.exe (a bash text file wearing the .exe suffix) plus pnpm.exe.cmd. pwsh resolves pnpm to pnpm.exe first via PATHEXT, hits the text file, and fails with "not a valid application for this OS platform." The base is now name.replace(/\.exe$/, ''), so the handle becomes pnpm + pnpm.cmd and pwsh resolves through the .cmd.

The change is behavior-preserving off Windows (the whole block is gated on process.platform === 'win32'). The rmSync calls key off the stripped base (base, ${base}.cmd, ${base}.exe), and the raw-name handle is still force-removed above, so a prior .exe-named handle is cleaned up. On the win32 leg, writeShims/rackRealFor/the sfw install all early-return, and the CI assert step resolves pnpm --version/npm --version through the .cmd — which is exactly what the strip enables. Only fully provable on the windows-latest test leg.

The one open thread from the prior review (rack.test.mts:41 missing a win32 skip guard, matching its two writeShims siblings) is untouched by this commit and remains open. It's a local-Windows-dev-only test failure — CI runs test:scripts on ubuntu-latest only — so it does not block merge.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found in the incremental change. One prior non-blocking thread remains open.

Reviewed changes — the single new commit 49610a3 adds a dev-machine remote-map guard: a standalone script declaring where this repo's PRs open versus where branches push, plus unit tests. It is not wired into any CI gate or soak surface.

  • Declare the two GitHub homes as law — new scripts/soak/remotes.mts fixes PR_HOME (nubjs/nub, the origin PRs open on) and PUSH_HOME (jdalton/nub, the fork branches push to). --check verifies a clone's remotes match, --fix adds or repoints only the fork remote (origin is report-only, never auto-rewritten), and --print emits the map as JSON.
  • Cover the pure core — new scripts/soak/remotes.test.mts exercises both arms of normalizeGitHubRepo (https / scp-ssh / ssh://, with and without .git, non-github rejected), parseRemotes, and checkRemotes drift classification; remotes:check/remotes:fix package.json scripts drive the CLI.

I verified the load-bearing pieces. The normalizeGitHubRepo regex requires an exact owner/repo pair, so a truncated scp URL (git@github.com:aube.git) correctly returns null, and checkRemotes compares case-insensitively while ignoring undeclared extra remotes. The --check path self-skips under CI (unless --fix), the --fix path touches only the fork remote and writes an SSH URL, and the test suite is hermetic — it imports only node: builtins plus the local module and exercises the pure functions with no git spawn, so the test:scripts glob picks it up safely on the CI docs job.

One prior open thread — the rack.test.mts win32 skip-guard nit — is untouched by this commit and left open. It's a local-Windows-dev-only test failure (CI runs test:scripts on ubuntu-latest), so it doesn't block merge.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in the incremental change. One prior non-blocking thread remains open.

Reviewed changes — since the prior pullfrog review (49610a3, whose own commit was already reviewed), the one new commit ff97dbf closes the last dependency path the local soak surfaces couldn't reach: Renovate updates.

  • Gate renovate.json against the soak window — new checkRenovateConfig/fixRenovateConfig in scripts/soak/soak.mts demand an explicit top-level minimumReleaseAge of "7 days" (an extends: preset value doesn't count, since a preset can drift without a commit here); registered as a fixer-backed surface and exported alongside the other checks.
  • Carry the window in renovate.json — the config gains top-level "minimumReleaseAge": "7 days" plus "internalChecksFilter": "strict", so Renovate actually holds a too-fresh PR back rather than only annotating it.
  • Register the surface and document itpaths.mts adds SURFACES.renovateJson, and the soak skill's description + surface table list renovate.json.
  • Cover both functions — two hermetic tests assert preset-inheritance-is-drift and that --fix sets the window, preserves other keys, and is idempotent.

The rationale is sound: Renovate bumps manifests and lockfiles server-side, and cargo's min-publish-age skips already-locked versions, so a Renovate PR would otherwise sidestep every local soak surface. Top-level minimumReleaseAge cascades to all enabledManagers per the Renovate schema, and internalChecksFilter: "strict" is the load-bearing half that makes the age check block rather than advise. Verified locally: node scripts/soak/soak.mts --check exits 0 with the renovate surface included, and all 22 soak.test.mts cases pass, including the two new renovate tests and main --check against the tracked repo state.

The one prior thread — the rack.test.mts:41 win32 skip-guard nit — is untouched by this commit and remains open. It's a local-Windows-dev-only test failure (CI runs test:scripts on ubuntu-latest), so it doesn't block merge.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in the incremental change. One prior non-blocking thread remains open.

Reviewed changes — since the prior pullfrog review (ff97dbf), the two new commits fix a portability bug in the commit-msg hook and pin its behavior with tests.

  • Fix the BSD-awk breakage in the commit-msg hook.githooks/commit-msg drops the END { /* ... */ } block whose C-style comment broke BSD awk, silently skipping the trailing-blank collapse. The rewrite relies on trailing blanks never being flushed at EOF, so the collapse-runs-to-one and drop-trailing semantics are preserved without an END block.
  • Pin the hook's behavior — new hermetic scripts/soak/commit-msg-hook.test.mts asserts exit 0 and empty stderr across five cases (agent-trailer strip, human co-author preserved, blank-run collapse plus trailing-drop, subject-only leaves no trailing blank, hook present at the core.hooksPath location). Every case asserts an empty stderr, so a tool erroring mid-pipeline is a failure even when the message still comes out right — which is the incident this pins.

The awk rewrite is behavior-preserving: blank lines accumulate a counter and a run is emitted as at most one blank only before the next non-blank line, so EOF-trailing blanks are dropped as intended (traced against the feat: subject\n\n\n\nbody line\n\n\nfeat: subject\n\nbody line\n case). The tests are contract-named and comprehensive without bloat.

The one prior open thread — the rack.test.mts:41 win32 skip-guard nit — is untouched by these commits and remains open. It's a local-Windows-dev-only test failure (CI runs test:scripts on ubuntu-latest), so it doesn't block merge.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

The post-extract path resolves <bin>.exe for windows archives, but the
pre-download idempotency guard checked only the suffix-less destBin, so a
second --install on windows re-downloaded and re-extracted (and wedges on
a restricted/offline runner). Mirror the same .exe resolution in the
early-exit. Mirrors jdx/aube#1028.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in the incremental change. One prior non-blocking thread remains open.

Reviewed changes — the single new commit 003447a makes a repeat Windows tool install a no-op instead of forcing a re-download, closing the last idempotency gap in the pinned-tool installer.

  • Resolve <bin>.exe in the pre-download early-exitinstallAssetTool now checks for a <bin>.exe variant before deciding to download, mirroring the identical resolution the post-extract path already does. On Windows a tool installed as <bin>.exe was invisible to the "already installed" check, so a second --install re-downloaded — which wedges on a restricted/offline runner.

The change is sound and behavior-preserving off Windows. The new block is logically identical to the post-extract block from d947ec8, and passing the .exe-suffixed binName into linkHandle is safe: on win32 linkHandle strips the trailing .exe from the handle base and sets viaExe = true, so the .cmd/bash forwarders dispatch to the .exe directly — exactly matching a fresh install. On non-Windows platforms no <bin>.exe exists, so the block is a no-op. Only fully provable on the windows-latest test leg.

The one prior open thread — the rack.test.mts:41 win32 skip-guard nit — is untouched by this commit and remains open. It's a local-Windows-dev-only test failure (CI runs test:scripts on ubuntu-latest), so it doesn't block merge.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant