Repository navigation
fix(soak): match .exe on windows tool-install early-exit - #1028
Merged
Merged
Conversation
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 always re-downloaded and re-extracted (and wedges on a restricted/offline runner). Mirror the same .exe resolution in the early-exit. Resolves the remaining greptile P1 from aubepkg#1020.
📝 WalkthroughWalkthroughThe asset-install flow now detects an existing Windows ChangesWindows asset installation
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 1
🤖 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 `@scripts/soak/external-tools.mts`:
- Around line 284-290: Restrict the `.exe` fallback in the installer’s
destination resolution to `process.platform === 'win32'`, so non-Windows
platforms never reuse or rename an adjacent executable. Apply the same Windows
guard to the post-extraction check around the corresponding destination and
command-name updates, keeping first and repeated installs consistent.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5173ddb2-d600-4da3-adb5-e92ebe061cbd
📒 Files selected for processing (1)
scripts/soak/external-tools.mts
Comment on lines
+284
to
+290
| // Windows archives land as `<bin>.exe`; resolve the same suffix the | ||
| // post-extract path does so a second --install is a no-op instead of a | ||
| // forced re-download (which wedges on a restricted/offline runner). | ||
| if (!existsSync(destBin) && existsSync(`${destBin}.exe`)) { | ||
| destBin = `${destBin}.exe` | ||
| binName = `${binName}.exe` | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict .exe resolution to Windows.
This branch is described as Windows-specific, but it currently runs on every platform. If a non-Windows installation has <destBin>.exe alongside the expected binary, the installer will reuse it and change the linked command name unexpectedly. Add process.platform === 'win32' here and apply the same guard to the post-extraction check at Lines 301-306 (or extract both checks into one helper) to keep first and repeated installs consistent.
🤖 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 284 - 290, Restrict the `.exe`
fallback in the installer’s destination resolution to `process.platform ===
'win32'`, so non-Windows platforms never reuse or rename an adjacent executable.
Apply the same Windows guard to the post-extraction check around the
corresponding destination and command-name updates, keeping first and repeated
installs consistent.
Contributor
Greptile SummaryThis PR fixes a Windows-only idempotency regression in
Confidence Score: 5/5Safe to merge — a 7-line addition that mirrors existing, already-tested logic to the early-exit block, with no new behavioral paths introduced for POSIX. The new block is a direct structural copy of the post-extract No files require special attention. Important Files Changed
Reviews (1): Last reviewed commit: "fix(soak): match .exe on windows tool-in..." | Re-trigger Greptile |
Contributor
Author
|
@jdx You're merging quick, I have another follow up but related to robustness (updating a lib) |
jdalton
added a commit
to jdalton/nub
that referenced
this pull request
Jul 15, 2026
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 aubepkg/aube#1028.
colinhacks
added a commit
to nubjs/nub
that referenced
this pull request
Jul 24, 2026
…d tooling (#442) * build: pin rust 1.95.0 toolchain, bump rust-version to 1.95 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(). * perf(pm): mark rare branches in hot install loops with 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. * build: switch toolchain pin to nightly-2026-07-04 (7-day soak) 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. * build: enable cargo min-publish-age with a 7-day soak 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. * build: add 7-day soak manager, taze, pinned external tools 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. * ci: gate soak + tool pins, route installs through socket firewall, add 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. * fix(soak): make update-deps dry-run actually dry 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. * fix(soak): timeout stalled external-tool downloads A hung release or registry response would stall CI indefinitely; abort after 120s (generous for the largest pinned binary on a slow runner). * fix(soak): harden gates and installer against review findings - 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 * build: manage .dockerignore with untracked, gate docker prebake pins 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). * ci: zizmor hygiene across all workflows 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). * build: pin pnpm 11.8 + npm 12 via the tool rack, dogfood dep installs 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. * fix(soak): shim self-clobber, windows .exe resolve, date guards; add 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). * refactor(soak): perfectionist pass — dead code out, tests isolated, docker gates wired Mirrors the shared-script pass (dead assertRepoRoot/'required' knob out, hasSocketToken inlined, shared extractArchive, single manifest parse, manifest-resolved shim targets with idempotence + clobber regression tests, tmpdir-isolated rack tests, SOAK_DAYS-derived fixtures, SOCKET_SECURITY_KEY prose). nub-side: .dockerignore joins the docker-smoke and docker workflow path filters so context changes actually build the images; new dockerignore tests assert the managed section, preserved custom rules, honored whitelist, and that no recursive dotfile rule can reach the load-bearing nested .cargo config; CI gate defers to the package.json scripts; zizmor.yml uses the fleet checkout pin; ignore-comment and permissions placement normalized (zizmor still reports zero findings); package.json em dashes restored to literals * feat(soak): enforce the toolchain nightly soak, add the soak skill rust-toolchain.toml carries a machine-read '# adopted: YYYY-MM-DD' line and soak.mts enforces adopted >= nightly date + SOAK_DAYS as a fifth checked surface (stable pins pass freely; missing or impossible dates are findings). .claude/skills/soak/SKILL.md documents the system pointing at scripts/soak/: the one-place window change (edit SOAK_DAYS, run soak:fix), the committed-only opt-out (SOAK_DAYS = 0 — deliberately no env bypass), per-package dated excludes, per-tool soakBypass, and the nightly bump procedure. * docs(soak): align skill with claude prompting and skill-authoring guidance * test(soak): cover cli paths, fixers, download token gating, installer guards * refactor(soak): dedupe the rust-toolchain.toml path, complete the header inventory * fix(ci): fixture anchor, zizmor scope, docker whitelist, sfw cargo bypass - tests/node-matrix/fixtures/package.json: workspaces:[] boundary so the new root pnpm-workspace.yaml + root engines can't redirect matrix legs to the highest installed node (proved: node 20 leg ran 26.4 without it) - zizmor.yml: scan .github/ only — vendor/aube ships aube's workflow files, which GitHub never executes outside the root .github/ - .dockerignore: whitelist tests/docker-smoke (untracked's test* pattern excluded the smoke.sh the docker-smoke Dockerfiles COPY) - ci.yml: run the .mts soak scripts with a type-stripping node on compat legs, pin pnpm/npm only on the node-24 leg, and exec real cargo past the sfw shim (the proxy mangles the registry-mock tests: ECONNRESET) * fix(soak): anchor the pnpm soak in tools/ — a root workspace yaml redirects the test matrix pnpm only honors minimumReleaseAge from a workspace yaml (.npmrc's minimum-release-age is silently ignored — proven: fresh npm@12.0.0 resolves through npmrc but is blocked by the yaml with ERR_PNPM_NO_MATURE_MATCHING_VERSION). But a workspace yaml beside the pre-existing root pnpm-lock.yaml marks the REPO ROOT as a workspace for nub itself: every in-repo invocation then inherits the root package.json engines (>=22.15) and redirects to the highest installed node, which un-skipped the tier-test harness gates and failed the 18.19/20.11/22.13/ 22.15 matrix legs (main nightly green; branch bisected to the yaml). Same shape as aube's docs/ anchor: the root is npm-only (npm ci, package-lock.json, .npmrc min-release-age; root pnpm-lock deleted and gitignored), and the pnpm domain — catalog, minimumReleaseAge, taze — lives in tools/ where the workspace boundary sits below anything the tests walk. Verified: the three failing integration tests pass under a Node 20.11 PATH again, and all soak gates stay green. * fix(ci): reachable rust-toolchain pin, windows-safe tar extraction - 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 * ci: exact version comments on docker action hash pins (ref-version-mismatch) * fix(soak): use System32 bsdtar on windows — Git Bash's GNU tar can't read zip * fix(soak): windows handles forward to the rack — symlinks strand SEA 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. * fix(deps): strip .exe from win32 tool handle base name * chore: encode PR home + push fork as a checked remote map * fix(deps): close the renovate soak bypass — explicit minimumReleaseAge, parity-checked * fix: portable awk comment in commit-msg hook — C-style broke BSD awk * test: pin commit-msg hook behavior — clean stderr, trailer strip, blank collapse * fix(soak): match .exe on windows tool-install early-exit 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 aubepkg/aube#1028. * build: drop the repo-root nightly pin, soak cargo at update time A repo-root rust-toolchain.toml outranks `rustup default` in rustup's toolchain precedence, and dtolnay/rust-toolchain only sets the default. Pinning nightly there therefore redirected every version-pinned CI job -- including the MSRV Check legs that compile on 1.93.0 and 1.95.0 -- and built the released binaries on nightly rather than stable. min-publish-age is the only thing that needed nightly, and it only matters where versions are chosen. Request it per invocation there: update-deps.mts now runs `cargo +nightly update`. Every CI job and every shipped binary builds on stable again, and the MSRV legs verify their declared floors. Drops the now-unreachable toolchain-soak checker and its tests; the remaining surfaces are unchanged. * ci: clear zizmor workflow-hygiene findings - Correct the actions/checkout@93cb6efe pin comments from the moving major tag "v5" to the exact immutable tag v5.0.1 the SHA resolves to (comment only; no SHA changed). Clears 53 ref-version-mismatch findings. - Suppress impostor-commit and ref-version-mismatch on the dtolnay/rust-toolchain pins; the repo force-pushes its version tags, so pinned SHAs go unreachable and cannot be pinned to a stable tag. Both SHAs verified as genuine upstream commits. - Add persist-credentials: false to the two verify-install.yml checkout steps flagged by artipacked; neither job pushes to the repo. - Suppress dangerous-triggers on trunk-red.yml; it is a report-only workflow_run notifier that never checks out or runs PR-controlled code. --------- Co-authored-by: Colin McDonnell <3084745+colinhacks@users.noreply.github.com>
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Follow-up to #1020. The pre-download idempotency guard in
installAssetTool(scripts/soak/external-tools.mts) checked only the suffix-lessdestBin, while the post-extract path resolves<bin>.exefor Windows archives. So on Windows a secondaubedev-tool--installalways re-downloaded and re-extracted the archive — and on a restricted/offline runner (cached rack, no network) that forced re-download fails outright.This mirrors the same
.exeresolution into the early-exit block, so a repeat install is a no-op on Windows as it already is on POSIX.Why
Resolves the remaining unresolved greptile P1 on #1020 ("Idempotency check misses
.exeon Windows",external-tools.mts:288). The sibling P1 (".ziphandle name doubles the.exeextension") was already handled by the mergedlinkHandle, which strips the suffix before building the PATH handle.Testing
node --test scripts/soak/external-tools.test.mts— 13/13 passnode scripts/soak/external-tools.mts --check— cleanA dedicated unit test isn't added:
RACK_DIR/BIN_DIRare frozen fromXDG_DATA_HOMEat import andinstallAssetToolisn't exported, so exercising the guard would need a subprocess + env-override harness disproportionate to a 4-line Windows-only mirror of already-present logic (lines 296–299).Note:
scripts/soak/is mirrored into the nub repo (onlypaths.mtsdiffers); this one-liner should be mirrored there too.This PR was generated by Claude.
Summary by CodeRabbit
.exeextension.