fix(update): gate the npm cache before shutdown and stop persisting vendor output (replaces #557) - #1207
Merged
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
This was referenced Aug 7, 2026
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 897318a72f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (inaccessibleByMode(stat)) { | ||
| return { ok: false, reason: "cache_entry_inaccessible" }; | ||
| } | ||
| if (!stat.isDirectory()) continue; |
There was a problem hiding this comment.
When npm config get cache points to a regular file, this branch treats the root like an ordinary nested file and the inspection eventually returns cache_accessible. npm then fails with ENOTDIR while creating <cache>/_cacache, but only after the update path has stopped the proxy—the exact outage this preflight is intended to prevent. Distinguish the depth-zero entry and fail the preflight unless the resolved cache root is a directory.
Useful? React with 👍 / 👎.
| if (stat.isSymbolicLink()) return false; | ||
| const ownerBits = stat.mode & 0o700; | ||
| if (stat.isDirectory()) return (ownerBits & 0o700) !== 0o700; | ||
| return (ownerBits & 0o400) === 0; |
There was a problem hiding this comment.
For regular files this check only requires owner-read permission, so an existing cache index bucket owned by the current user but mode 0400 passes the preflight. cacache appends to existing _cacache/index-v5 buckets; reproducing a second cache write with such a bucket produces EACCES, which means the subsequent install can still fail after the proxy has been stopped. Check write permission for cache files npm may mutate, rather than treating readability alone as sufficient.
Useful? React with 👍 / 👎.
Comment on lines
+9
to
+11
| const WORKER_TIMEOUT_MS = 10_000; | ||
| const NPM_CONFIG_TIMEOUT_MS = 5_000; | ||
| const INSPECTION_TIMEOUT_MS = 7_500; |
There was a problem hiding this comment.
A successful npm config get cache may consume almost its 5-second allowance, after which inspection is intentionally allowed another 7.5 seconds, but the parent kills the worker after only 10 seconds total. On a slow disk or filesystem, the worker is therefore terminated before inspection can return its intended inspection_incomplete success, and every update is blocked with worker_timeout. Make the outer timeout at least cover the combined phases, or pass a shared remaining deadline to both.
Useful? React with 👍 / 👎.
| ]); | ||
|
|
||
| /** npm prints `npm ERR! code EACCES`; anchor on that position rather than scanning free text. */ | ||
| const NPM_CODE_RECORD = /^\s*npm\s+ERR!\s+code\s+([A-Z][A-Z0-9_]{2,})\s*$/gm; |
There was a problem hiding this comment.
This pattern only accepts the legacy npm ERR! code EACCES form, while npm 11 emits records such as npm error code EACCES. Consequently none of the allowlisted codes are retained for failures under current npm, and because raw output is deliberately discarded the dashboard record loses its only specific diagnostic and reports just an exit status and byte count. Keep the match anchored, but accept both npm record prefixes.
Useful? React with 👍 / 👎.
| // and we never follow them — so its owner is irrelevant and must not abort the update. | ||
| // This has to come BEFORE the ownership check: a foreign-owned but never-followed link is | ||
| // exactly the false positive that made the previous attempt at this feature unusable. | ||
| if (stat.isSymbolicLink()) continue; |
There was a problem hiding this comment.
Skipping every nested symlink also skips structural paths that npm itself follows, such as a symlinked <cache>/_cacache. If that link targets a non-directory or an inaccessible cache tree, this function returns cache_accessible, but npm follows the link and fails after the proxy has been stopped; for example, _cacache linked to a regular file passes here and npm cache verify fails with EEXIST. Continue ignoring package links under _npx/node_modules, but resolve or reject symlinks in cache storage paths that npm will traverse.
Useful? React with 👍 / 👎.
lidge-jun
force-pushed
the
codex/260807-routed-reasoning-effort
branch
from
August 7, 2026 10:56
f179645 to
32b01c2
Compare
lidge-jun
force-pushed
the
codex/260807-npm-cache-preflight
branch
from
August 7, 2026 11:02
ec3a31d to
ef89b72
Compare
lidge-jun
force-pushed
the
codex/260807-routed-reasoning-effort
branch
from
August 7, 2026 11:34
32b01c2 to
392179e
Compare
lidge-jun
force-pushed
the
codex/260807-npm-cache-preflight
branch
2 times, most recently
from
August 7, 2026 16:47
46aec35 to
913579e
Compare
…replacement, round 2)
Audit found four defects in the first cut, two of which would have shipped a
feature worse than the bug it fixes.
BUDGET EXHAUSTION IS NOT FAILURE. `inspectNpmCacheDirectory` returned
`{ok:false, reason:"inspection_limit"}` when it ran out of entries, depth, or
time. A mature npm cache legitimately holds hundreds of thousands of entries —
the auditor measured 256,322 on their machine and watched the real preflight
reject it in 1.13s. Every one of those users would have been locked out of
updating. "We ran out of budget looking" now returns `ok:true` with
`inspection_incomplete`: we inspected a bounded prefix, found nothing wrong, and
let the update proceed.
NESTED SYMLINKS ARE SKIPPED BEFORE THE OWNERSHIP CHECK. npm creates symlinks
constantly below `_npx`, `node_modules`, and `.bin`. We never follow them, so
their owner is irrelevant — but the ownership check ran first and aborted the
update on a foreign-owned link. A symlinked cache ROOT is still rejected: we
cannot vouch for where the install writes.
SANITIZATION SURVIVES WRAPPED PATHS. npm and the OS wrap long paths, and the
line-bound regexes let `C:\Users\<newline>Jane Doe\...` through with the
username intact. Redaction now runs on a newline-collapsed copy and additionally
covers `%USERPROFILE%`-class expansions, `$HOME`, UNC shares, and `/root`.
GATE ORDERING IS TESTED BY BEHAVIOR. The existing checks compared source-string
positions, so they would stay green if the gate were unreachable or disconnected
from the stop. `runGuiUpdateWorker` now takes injectable preflight and install
seams, and a new test asserts the install command is never called when the
preflight fails.
The symlink test also needed a real seam: `!stat.isDirectory()` skips a link
anyway, so removing the ownership-ordering rule left every assertion green. An
injected `uidOf` binds the assertion to ownership specifically. Each of the four
fixes was confirmed to fail its test when reverted.
…t wrapped paths (round 3)
Audit round 2 found the previous commit's headline fix was inert.
THE PROTOCOL REJECTED ITS OWN SUCCESS. `inspectNpmCacheDirectory` started
returning `{ok: true, reason: "inspection_incomplete"}` for a bounded-but-clean
scan, but `parseWorkerOutput` cross-checked the flag against a single literal —
`parsed.ok !== (parsed.reason === "cache_accessible")` — so the pass was
discarded as `worker_output_malformed`. Every large cache still failed, now with
a misleading reason. The cross-check is worth keeping (a worker must not claim
success with a failure reason), so it is now a set. Verified against this
machine's real 256k-entry cache: `{"ok":true,"reason":"inspection_incomplete"}`.
`inspection_limit` became unreachable and is removed.
WRAPPED PATHS STILL LEAKED. Rejoining wrapped lines was the wrong shape: joining
aggressively enough to catch a wrap inside the username also merged genuinely
separate log entries, and joining conservatively enough to keep them apart let
`C:\Us<newline>ers\Zoe [Admin]+` through. Redaction now runs against a
newline-stripped scan copy with an index map back to the original, so the match
never depends on where the wrap landed, and an absolute-Windows-path backstop
covers any run that cannot be resolved into a known profile shape.
THE GATE TEST NEVER REACHED THE GATE. It asserted on a source checkout, where
`checkForUpdate` aborts long before the npm branch — so it proved nothing about
the pre-flight. `runGuiUpdateWorker` now also accepts `checkForUpdateFn` and
`integrityFn`, and the test forces the npm installer, asserts the pre-flight
actually ran, asserts the install spy did not, and asserts the abort message
names the pre-flight.
Two source-position tests were updated to match the new seam strings. They
remain non-behavioral; the new injected test is the one that proves ordering.
Known limitation, deliberately not fixed here: a cache root that is itself a
symlink is still rejected, though symlinking ~/.npm to another volume is
legitimate. Resolving the root target safely is a separate change.
…linked cache root (round 4) INDENTED CONTINUATIONS STILL LEAKED. The scan copy stripped CR/LF but kept the whitespace that follows a wrap, so `Us` + newline + two spaces + `ers` never reformed into the keyword and the profile rules did not fire. Three real leaks went through the persistence boundary with the account name intact, including a non-ASCII one. The scan now consumes the break and its indentation, and the match set gains a UNC backstop alongside the absolute-Windows-path one. Regression inputs are the auditor's exact cases: a wrap inside `Users` behind a UNC share, a wrap inside `Documents and Settings`, and a wrapped POSIX path with a Korean username. A SYMLINKED CACHE ROOT IS NO LONGER REJECTED. Pointing ~/.npm at another volume is ordinary npm configuration, and refusing it was the same class of false positive as failing on a large cache — it blocks an update for a user whose setup is fine, which this change's own rule says is worse than the defect. The root is now resolved once via realpath and the target inspected; nested symlinks are still never followed, and an unresolvable root remains a hard stop. Both fixes confirmed to fail their tests when reverted.
… it (round 5) The scan copy removed every line boundary, so the `[^\r\n]*` backstops ran to the end of the text: one redacted path consumed every following log entry. Privacy was intact; the diagnostics were destroyed. The persisted log is what a user reads when an update fails, so eating it is its own kind of damage. Boundary sentinels are now inserted, but only where the next line starts a new log entry rather than continuing a path. Marking every boundary would have been equally wrong — it blocks the reconstruction that catches a username split across a wrap. The test is structural: a continuation carries a separator (or follows one), a new entry is a label with none. That distinction is what lets `Mary Jane van der Berg\Documents\...` still reconstruct while `KEEP diagnostic code E42` survives untouched. Regression asserts both halves: the username is gone AND the following diagnostic line is still present.
… continuations (round 6) The separator heuristic could not work, and the audit proved it with two inputs that fail in opposite directions: C:\Users\Z / " oë [Admin]+" continuation with NO separator -> leaked ...\Users\Jane\x / "npm ERR! /usr/…" new record WITH a separator -> swallowed Nothing in the text distinguishes those two cases, so any rule keyed on separators trades one failure for the other. Four shapes were tried before this one: rejoin-aggressively (merged unrelated entries), rejoin-conservatively (leaked the username), strip-all-boundaries (swallowed the diagnostics), and sentinel-on-heuristic (both of the above, depending on the input). The redaction is now line-aware with one carry bit. A line is redacted normally; if it ENDS on an incomplete profile prefix — an unclosed account segment, or a split keyword like `...\Documents and Set` — the next line is treated as that account name's continuation and redacted whole. This is deliberately asymmetric. It can redact a following line that was actually unrelated, costing one line of diagnostics. The alternative costs somebody's account name, and this boundary exists precisely so that never happens. The keyword-prefix set is generated from the keywords rather than hand-written, so a wrap at any offset inside `Documents and Settings` is covered without enumerating them. Regressions now assert both directions: the username is gone, and an unrelated following record — with a separator in it — survives.
Six rounds of redaction, six new leaks. A wrap inside the keyword, inside the account name, an indented continuation, three consecutive wraps, an empty continuation line — each fix surfaced the next case, and the last two attempts started breaking cases they had previously fixed. That is not a tuning problem. The leak surface is whatever npm chooses to print and however the terminal wraps it, and no redactor gets to see the original line structure. Taking the auditor's second recommendation: - `runLoggedCommand` no longer persists stdout/stderr. It records exit status or signal, any recognized npm error codes (a fixed vocabulary, not user text), and a withheld-byte count. Detailed output stays ephemeral. - The persistence boundary replaces any multi-line value wholesale with a line count and a note. Single-line structured fields keep the precise redaction, which is what makes `command` and `error` still readable. The cost is real and worth naming: a user reading a failed update job now sees which step failed, how it exited, and any npm error code, but not the installer's own message. That is a genuine diagnostic loss. It buys a boundary that cannot leak an account name regardless of what npm prints, which the previous six versions could not promise. The auditor's five leaking inputs are kept as regressions. They now pass structurally rather than by pattern-matching.
…ingle-line path leaks (round 8)
Three more findings, all real.
THE "FIXED VOCABULARY" WAS A SHAPE PATTERN. `E[A-Z]{3,}` matches `ERROR`, so
`npm ERR! path C:\Users\ERROR\.npm` re-emitted the username as a "code" — the
summary leaking exactly what withholding the output was meant to protect. It is
now an explicit Set of recognized npm/libc codes, extracted only from npm's
canonical `code <CODE>` position rather than scanned out of free text.
SINGLE-LINE PATHS STILL LEAKED. The multi-line path is withheld wholesale, but
single-line values keep precise redaction, and three rules there stopped at the
first space — so `\\server\home$\Jane Doe\...` and `D:\Profiles\Mary Jane\...`
kept the surname. Path segments legitimately contain spaces; those runs now
continue across them and stop at a delimiter that cannot appear mid-path. The
UNC rule also consumed only `server\share`, leaving the account segment behind
for later rules that could no longer recognize it.
BYTE COUNT WAS A CODE-UNIT COUNT. `Buffer.byteLength(..., "utf8")` now, which
matters for the non-ASCII output this feature exists around.
… does not (round 9) Eight rounds of redaction failed in both directions at once, and the audit proved it with one input each: D:\Profiles\Mary O'Connor\... leaked — an apostrophe was a terminator installed at C:\x and then ... over-redacted — a path run has no reliable end Both come from the same mistake: guessing which characters belong to a path in text we did not produce. No amount of pattern work fixes that, because the adversary is npm's output format and the terminal's wrapping. The boundary now asks a question it can answer — is this value KNOWN safe? — and withholds everything else. Safe means: built from our own vocabulary, one line, no absolute path of any form (drive letter, UNC, POSIX root, `~user`, environment expansion), plus two explicitly recognized shapes, a package-manager invocation and our release URL. Verified against every leaking input the audit produced across nine rounds — all withheld — while the values a user actually needs survive intact: the command, the queue and version lines, the exit/code/size summary, and the restart diagnostics. The previous redactors are deleted rather than left beside the new check. Two competing notions of "safe" in one file is how the earlier rounds kept reintroducing each other's bugs.
…held error text (round 10) The previous "allow-list" defaulted to `return true`, which makes it a denylist wearing an allowlist's name — and the audit walked straight through the exception carved out for our own endpoint: `probe /healthz?path=/Users/Jane-Doe` passed. Three changes, following the audit's provenance recommendation: FIELD-SCOPED. Only `command`, `error`, `log`, and `releaseNotesUrl` go through the check. The rest of the record is a closed vocabulary — statuses, channels, installers, versions, timestamps — and running a text check over those only risked mangling values that were never a disclosure route. RENDERED, NOT FILTERED. `releaseNotesUrl` is compared against the module constant rather than pattern-matched, so a URL-shaped value cannot smuggle a path. `command` is rebuilt from a recognized shape: a known tool, fixed subcommands and flags, our own package spec, and `<path>` placeholders for absolute arguments. Anything else is withheld — content alone cannot tell `npm install Mary-Jane` from a package argument. ERROR TEXT IS DESCRIBED, NOT COPIED. Every site that interpolated an `Error.message` now calls `withheldSummary()`, which reports the error's type, its code when it is a recognized one, and a byte count. The message itself is kept only when it passes the same path test — so `spawn denied` and `ETIMEDOUT` still reach the user, and a message carrying a path does not. Verified against every attack input from rounds 5-9, including the six that defeated round 9, while the diagnostics a user needs survive: the queue line, the command shape, the exit/code/size summary, and the restart trace.
… provenance code (round 11) The audit caught something worse than a bug: I built a provenance mechanism and never wired it up. `ownText`, `sanitizePersistedUpdateText`, `isBrandedSafe`, and `stripBrand` were all unreferenced, so the boundary was still doing content inspection while the commit message described branding. Dead scaffolding that describes a guarantee the code does not provide is worse than no scaffolding — it makes the next reader believe the guarantee holds. All of it is deleted. THE REAL LEAK IT WAS HIDING: `withheldSummary` kept any message that carried no path. That sounds reasonable and is wrong — `spawn denied for Jane Doe` has no path in it and still names a person. There is no test on message CONTENT that separates a diagnostic from an identity, so message text no longer crosses the boundary at all. The record keeps the error's type, a recognized code, and a byte count. Also closed: - A raw `err.message` catch in the GUI worker that never went through any check. - `error.code` was surfaced on an arbitrary uppercase shape; it must now be in the explicit NPM_ERROR_CODES set, since a code can be attacker-shaped too. - The npm code extractor is anchored to a complete canonical line (`^npm ERR! code <CODE>$`, multiline) rather than matching `code` anywhere in free text — closing `npm ERR! path code EACCES\private`. Cost, stated plainly: a user no longer sees npm's own error text. They see which step failed, the error type, a recognized code, the command shape, and how much output was withheld. Confirmed by ablation that the new regression fails when the summary is replaced with the raw message.
…on at entry (round 12)
Two more channels, both external text reaching disk through a field that looked
like ours.
`Error.name` IS EXTERNAL. It is writable, so `error.name = "Jane Doe"` put the
caller's chosen string into the persisted record even with the message withheld.
The summary now states a fixed classification — `Error` or the primitive type —
rather than repeating anything we were handed.
`/healthz` VERSION IS EXTERNAL. That endpoint is answered by whatever holds the
port, and the restart-evidence reasons interpolate its `version` into a
persisted field. A responder returning `{version: "Jane Doe"}` persisted it. The
value is now validated as semver where it ENTERS — in the probe — rather than
where it is logged, so every downstream consumer gets a version or nothing.
Validating at entry rather than at each log site is the point: there are four
places that interpolate this value, and a check at the boundary cannot be
forgotten by the fifth.
…ound 13) Shape validation was not enough: `2.7.41-JaneDoe` is valid semver, so the mismatch reason echoed it straight into a persisted field. `/healthz` is answered by whatever holds the port, which makes its version external input no matter how well-formed it looks. Mismatch reasons now state THAT the reported version did not match and name only the version we expected — which is ours. On a match the reported value equals the expectation by definition, so the trusted one is rendered instead. This closes the last channel the audit's persistence inventory found. Regression drives the hostile value from ingress through to the evidence reason and asserts the name is absent while our own version still appears.
… 14) Withholding the whole stream was too blunt. A user whose update fails deserves to know why, and `exit 1 · 359 bytes withheld` tells them nothing. The insight I missed for thirteen rounds: npm's failure output is STRUCTURED, not prose. It prints `npm error <field> <value>`, one field per line. That means the useful parts can be read BY NAME instead of reconstructed from text — which is what made every redaction attempt fail, since it had to guess where a path started and ended. Kept fields, each because its value cannot be a local path: `code`, `syscall`, `errno`, `notarget`, and the HTTP-status lines (`404`, `401`, `403`, `409`, `429`) whose value is a registry URL. Explicitly not kept: `path`, `dest`, `file`, `stack`, the bare `Error:` line, and the debug-log location — every one of those is a filesystem path by definition. Each kept value still passes the path test before use, is length-capped, and `code` must additionally be in the recognized vocabulary. Convention is not a guarantee. Node exceptions get the same treatment: `syscall` and `errno` are named properties, shape-validated (a short lowercase identifier, an integer), so an error summary now reads `Error EACCES · syscall: mkdir · errno: -13` instead of a byte count. Measured against real npm failures: before exit 1 · 366 bytes withheld after exit 1 · code: E404 · 404: The requested resource '…' could not be found before exit 1 · 359 bytes withheld after exit 1 · code: EACCES · syscall: mkdir · errno: -13 before exit 1 · 208 bytes withheld after exit 1 · code: ETARGET · notarget: No matching version found for left-pad@99.99.99 The regression drives a real EACCES dump containing `/Users/Jane Doe/...` and asserts the cause survives while the account name and paths do not. Ablation confirmed.
Allowlisting the field name and leaving its value free-form just moved the leak
one level in. `npm error syscall janedoe` was kept verbatim, because `syscall`
was a recognized field and nothing ever checked what followed it.
Every kept field is now RENDERED from a validated value:
code must be in the recognized npm/libc vocabulary
syscall must be in an explicit POSIX syscall set — a shape check accepts `janedoe`
errno must parse as an integer, and is re-rendered from the parsed number
notarget reduced to `no matching version for <pkg>@<version>`, or the bare fact;
the surrounding prose is never borrowed
HTTP 4xx parsed as a URL, rendering only the registry HOST — the path can name a
private scope and userinfo is a credential
Node exceptions use the same syscall vocabulary rather than the shape check.
This also fixes a bug the audit found in the previous round: the HTTP diagnostic
never actually worked, because the raw line contains `https:/` and the path test
read that as a drive letter. Parsing the URL fixes the false positive and the
disclosure risk in one move.
Verified against every forged input the audit produced — `syscall janedoe`,
`errno JaneDoe`, `notarget ... Jane Doe`, mixed-case `NpM ErRoR`, a private
scope in the URL path, and userinfo — while the real failures stay legible:
exit 1 · code: EACCES · syscall: mkdir · errno: -13
exit 1 · code: E404 · 404: HTTP 404 from registry.npmjs.org
exit 1 · code: ETARGET · notarget: no matching version for left-pad@99.99.99
Ablation confirmed: restoring the shape check fails the new test.
…(round 16) Two things I treated as "safe shapes" that were not. A PACKAGE SPEC IS NOT A SAFE SHAPE. `name@version` also matches `jane.doe@example.com` and `JaneDoe@2.7.41`, so extracting "the spec" from a `notarget` line was itself a disclosure channel. This updater resolves exactly one package, so a spec is echoed only when it IS ours; everything else reports the bare fact. A HOSTNAME IS NOT A SAFE SHAPE. `^[\w.-]+$` accepts `janedoe.example`, a numeric host, and a punycode host. Knowing whether a 404 came from the public registry or somewhere else is the useful part, and that fits in an allowlist — four known registry hosts. Anything else reports the status alone. Also fixes the spec matcher itself: `PKG` is scoped (`@bitkyc08/opencodex`), so it needed escaping and a boundary that works with a leading `@` — `\b` does not. Verified our own spec is kept while an email is not. Every attack input from this round is a regression test.
Pinning the package NAME to our own still left the VERSION free: `@bitkyc08/opencodex@99.99.99-JaneDoe` is a valid-looking spec, and a semver prerelease identifier can encode anything. That is the same lesson the `/healthz` version taught in round 13 — I applied it there and not here. There is no trusted resolved version available at this call site, so the spec is not rendered at all. `code: ETARGET · notarget: no matching version` already tells a user their requested version does not exist, which is the diagnostic that matters. Both attack inputs are regression tests.
lidge-jun
force-pushed
the
codex/260807-npm-cache-preflight
branch
from
August 7, 2026 17:15
913579e to
821a845
Compare
lidge-jun
changed the base branch from
codex/260807-routed-reasoning-effort
to
dev
August 7, 2026 17:26
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.
Summary
Replaces #557. Stacked on #1197.
#557 should be closed in favor of this PR. It is 18 commits past its merge base while
devis 1,220 commits beyond that point, its CI is red (Windowsupdate-job"must not spawn" failures, a macOS hang in its own preflight test until 30-minute cancellation), and ~30 review threads are unresolved. The useful idea is one gate; the rest of its 24-file, +2351/-64 diff assumes a tree that no longer exists. This is 13 files built from currentdev.The two defects
The proxy stopped before we knew the install could succeed.
bin/ocx.mjschecks the version, shuts down, and only then installs;src/update/index.tsruns a registry-integrity preflight, not a cache-access one. A foreign-owned or unreadable nested cache entry produced a failed install after the proxy was already down. A bounded cache inspection now gates all three entry points, before the irreversible stop.Update output was persisted verbatim.
runLoggedCommandstored npm's stdout/stderr into a file on disk, carrying local paths and account names.Two things worth reading before reviewing
The preflight must not block a legitimate cache. The first cut rejected any cache it could not finish inspecting — and a mature npm cache legitimately holds hundreds of thousands of entries (256,322 on the review machine, rejected in 1.13s). A preflight that blocks ordinary users is worse than the bug it prevents, so budget/depth/deadline exhaustion returns pass. Same reasoning for a foreign-owned nested symlink (npm creates those constantly under
_npx, and we never follow them) and for a cache root symlinked to another volume (ordinary npm configuration).Redaction was the wrong tool for the log leak, and it took thirteen review rounds to accept that. Every pattern-based attempt failed in one of two directions — a username survived a wrap, or a whole diagnostic line was swallowed — because the leak surface is whatever npm prints and however the terminal wrapped it. Some inputs that defeated intermediate versions:
C:\Us+ wrap +ers\Zoë [Admin]+D:\Profiles\Mary O'Connorinstalled at C:\x and then rebuilt 42 modulesspawn denied for Jane Doeerror.name = "Jane Doe"Error.nameis writable/healthzreturningversion: "2.7.41-JaneDoe"So the boundary stopped inspecting content. Raw vendor output is not persisted; error text is described (type, allowlisted code, byte count) rather than copied; the command is rendered from recognized parts with
<path>placeholders; the release URL must equal the module constant; the health version is validated at ingress and never echoed back. The npm code allowlist is an explicitSetanchored to^npm ERR! code <CODE>$— an earlierE[A-Z]{3,}shape matchedC:\Users\ERROR\.npmand re-emitted the username as a "code".Diagnostics are preserved, and this is the part worth reviewing closely. An intermediate version withheld the whole stream, which made a failed update undebuggable. The way out was noticing that npm's failure output is structured, not prose — it prints
npm error <field> <value>, one field per line — so the useful parts can be read by NAME rather than reconstructed from text.exit 1 · code: EACCES · syscall: mkdir · errno: -13exit 1 · code: E404 · 404: HTTP 404 from registry.npmjs.orgexit 1 · code: ETARGET · notarget: no matching versionEvery element is a vocabulary member, a re-rendered primitive, or a fixed phrase — never borrowed text.
codemust be in a recognized npm/libc set,syscallin an explicit POSIX set,errnois re-rendered fromNumber(), the registry host is an allowlist of four, andnotargetis a fixed phrase. Node exceptionsyscall/errnoget the same treatment; the message andnameare never copied.That last part matters because shapes kept failing. A path shape, a code shape, a syscall shape, a hostname shape, a package-spec shape, a semver shape — each looked safe and each turned out to be an encoding channel (
janedoeis a valid syscall shape;jane.doe@example.comis a valid package spec;99.99.99-JaneDoeis valid semver). Only vocabularies and fixed phrases held.Verification
The full
prepushgate ran and passed on push. No frontend files are touched.Real-machine check:
runNpmCachePreflight()against this machine's 256k-entry cache returns{"ok":true,"reason":"inspection_incomplete"}.Every fix is ablation-confirmed — reverted, watched the matching test fail, restored. That includes one case where the ablation did not fail and the test had to be rewritten against an injected
uidOfseam, because!isDirectory()skipped the symlink regardless.This work failed independent audit thirteen times, and every round found a real defect — including a headline fix that was inert (the protocol parser rejected its own success reason, so the large-cache fix never reached the caller) and a provenance mechanism I wrote and never wired up. The rounds are in the commit history rather than squashed, because the failure modes are the useful part.
Known follow-up: the configured hostname is still persisted in restart diagnostics. It is user configuration and operationally useful; classifying it as sensitive is separate policy work.
Checklist