Codex/pr533 update recovery hardening - #803
Conversation
📝 WalkthroughWalkthroughThe update flow adds npm cache ownership checks, process-tree installer execution, bounded cleanup, identity-verified proxy recovery, sanitized persisted state, and trusted recovery package validation. Tests and CLI documentation cover the new behavior. ChangesUpdate safety and recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment Warning |
|
Closing. This pull request contains no work by its author. The head commit here is byte-identical to a branch that already exists in this repository and was authored by the maintainer. All six of these pull requests have the same shape: fork the repository, push the upstream branches back unchanged, and open them as incoming contributions.
Every listed SHA resolves to the same commit on the upstream branch of the same name. The commit authors inside them are This is not a rebase, a resubmission of stalled work, or a fork that drifted. It burns maintainer review time and CI minutes on a diff that is already in the tree, and it presents other people's commits under a new author's pull request. Repository access is being revoked for this account. |
![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: b0434ea584
ℹ️ 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".
| treeExited = result.status === 0 | ||
| && result.signal === null | ||
| && posixTreeAfterRootExit !== "running"; |
There was a problem hiding this comment.
When the installer exits nonzero without leaving any child process behind (for example, npm install returns 1 after the proxy was stopped), this assignment still forces treeExited to false solely because result.status !== 0. The callers check !treeExited before their normal failure handling/recovery (bin/ocx.mjs exits 75, and the GUI worker skips recoverFailedGuiUpdate), so ordinary clean installer failures are misclassified as unsafe cleanup and leave the proxy/tray recovery path unreachable. Base treeExited on the process-tree inspection result, and let the status decide success vs. recoverable failure separately.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 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 `@bin/ocx.mjs`:
- Around line 246-250: Update the cleanup-failure messages in bin/ocx.mjs (lines
246-250) and src/update/index.ts (lines 271-275) to include the installer
result’s timeout, interruption signal, and status details before exiting. Use
res.timedOut, res.interruptedSignal, and res.status in the ocx updater, and
r.timedOut, r.interruptedSignal, and r.status in the launcher, keeping both
messages consistent while preserving the existing cleanup warning and exit code.
In `@src/update/index.ts`:
- Around line 264-269: The installer timeout is duplicated as raw values,
preventing the CLI deadline and GUI worker deadline from being validated
together. In src/update/index.ts#L264-L269, add a module-level named
installer-timeout constant and use it for runProcessTreeCommand; in
src/update/index.ts#L374-L379, derive the timeout failure text from that
constant. In tests/update-stop-first.test.ts#L120-L137, parse the CLI constant
and assert UPDATE_TIMEOUT_MS is at least 60,000 ms longer, and update the
literal timeout assertion at line 162.
In `@src/update/install-process.mjs`:
- Around line 291-296: Update both consumers of cleanupPromise in
terminateInstallerProcessTree and runProcessTreeCommand to handle rejections by
converting them to the module’s existing fail-closed cleanup result. Add
rejection handling to the detached then chain and ensure the awaited
cleanupPromise always resolves with the failure result, so signal handlers are
removed and runProcessTreeCommand does not propagate cleanup exceptions.
In `@src/update/job.ts`:
- Around line 127-148: The recovery-scan contract is duplicated in job.ts
instead of sourced from recovery-tree-scan.mjs. In src/update/job.ts lines
127-148, remove the local isPathInside, hasTrustedRecoveryOwner, and
hasTrustedRecoveryPermissions definitions; export and declare those symbols from
recovery-tree-scan.mjs and recovery-tree-scan.d.mts, then import them in job.ts.
In src/update/job.ts lines 42, 53-55, import DEFAULT_MAX_RECOVERY_TREE_ENTRIES
and DEFAULT_MAX_RECOVERY_TREE_SCAN_MS and derive MAX_NPM_RECOVERY_TREE_ENTRIES
and MAX_NPM_RECOVERY_TREE_SCAN_MS from them, preserving the existing worker
payload and spawnSync timeout usage.
- Around line 714-720: Update the restart path around readPidForRestart to
invoke the injected RestartIo.killProxyFn when available, falling back to the
imported killProxy otherwise, matching recoverFailedGuiUpdate. Add a focused
regression test alongside the existing restartAfterUpdateForTests tests that
verifies the injected function observes or blocks the verified old-PID kill
path.
- Around line 505-528: Update runLoggedProcessTreeCommand to detect result.error
from runProcessTreeCommand and append its message to the job log via updateJob,
preserving the existing stdout and stderr logging. Ensure spawn failures such as
missing binaries expose the underlying error to downstream worker and dashboard
output while retaining the existing InstallerCommandResult fields.
- Around line 324-331: Expand sanitizeUpdateJobText to redact Windows
user-profile paths such as C:\Users\<name>\..., including paths not containing
the existing npm/cache markers, while preserving the current POSIX and
specialized path sanitization. Update the UID/GID pattern to also redact
lowercase space-separated forms such as “uid 1001”. Add a focused regression
case beside the existing update-job tests verifying Windows command and log text
does not persist the username.
- Around line 1246-1266: Update the failure handling around the result-status
branch in the update job so status === 0 with treeExited === false is reported
as a successful install with unconfirmed process shutdown, while retaining the
fail-closed recovery decision and skip log. Avoid labeling this case as “update
command failed (0)”, preserve the existing failure behavior for nonzero
statuses, and add a focused regression test in the existing update-job tests
covering the specified result shape and asserting both the skip log and new
error text.
- Around line 256-276: Bound recovery candidates before expensive validation: in
the retired-directory collection around inspectNpmRecoveryPackage, first sort
matching directory names by cheap filesystem mtime, retain at most
MAX_NPM_RECOVERY_CANDIDATES, then call inspectNpmRecoveryPackage only for that
subset before probing. Update the relevant update-job test to verify
inspection/spawned scan count is also limited to the candidate cap, not merely
that two launchers are probed.
In `@src/update/npm-cache-preflight.d.mts`:
- Around line 7-24: Split NpmCachePreflightOptions into entry-point-specific
option types: keep access, lstat, readdir, realpath, and now only on the options
used by findForeignOwnedNpmCacheEntry, while scanNpmCacheOwnership and
checkNpmCacheOwnership accept a narrow type containing only options they
forward, such as cache configuration and scan limits. Update the walk function
declaration to use the filesystem-hook type and preserve the existing worker
payload behavior.
In `@src/update/npm-cache-preflight.mjs`:
- Around line 146-199: Update scanNpmCacheOwnership so the parent spawnSync
timeout exceeds the worker’s maxDurationMs by a small margin, using a shared
SCAN_SPAWN_MARGIN_MS constant declared with the other scan constants. Keep
maxDurationMs in the worker payload unchanged, and apply the margin only to the
parent timeout so the worker can report its own budget expiration.
- Around line 97-111: Update the accessMode selection in the npm cache preflight
check to use only constants.R_OK for non-directory entries, while retaining
constants.R_OK | constants.W_OK | constants.X_OK for directories. Revise the
non-directory error message to mention readability only, and add a regression
test covering a readable read-only cache file.
In `@tests/update-job.test.ts`:
- Around line 155-170: Set explicit extended timeouts on both recovery tests,
including the test identified by findNpmRecoveryLauncher and the additional test
around the related recovery flow. Append the timeout using the existing test
framework syntax, choosing a value consistent with neighboring subprocess-based
tests (such as 15,000 ms), without changing their assertions or setup.
In `@tests/update-npm-cache-preflight.test.ts`:
- Around line 249-270: Update the fake blocking npm executable created by the
“force-kills an npm cache lookup that ignores SIGTERM” test so it does not
depend on resolving node through PATH. Use the guaranteed Bun interpreter in the
shebang, or otherwise invoke Bun explicitly while preserving the existing
blocking and SIGTERM-ignoring behavior and the ETIMEDOUT assertion.
- Around line 272-299: Update the test “fails closed before shutdown when npm
cannot resolve its cache” to track scan-worker invocation rather than stubbing
`lstat`, since `checkNpmCacheOwnership` delegates scanning through `spawn`.
Configure the existing `spawn` mock to record calls and assert that no scan
invocation occurs when cache resolution fails, while preserving the current
failure result and formatted-message assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5d877796-67dd-4902-ad10-e5bf0e26f835
📒 Files selected for processing (23)
bin/ocx.mjsdocs-site/src/content/docs/ja/reference/cli.mddocs-site/src/content/docs/ko/reference/cli.mddocs-site/src/content/docs/reference/cli.mddocs-site/src/content/docs/ru/reference/cli.mddocs-site/src/content/docs/zh-cn/reference/cli.mddocs/adr/0001-gui-update-worker.mdsrc/config.tssrc/update/index.tssrc/update/install-process.d.mtssrc/update/install-process.mjssrc/update/job.tssrc/update/npm-cache-preflight.d.mtssrc/update/npm-cache-preflight.mjssrc/update/recovery-tree-scan.d.mtssrc/update/recovery-tree-scan.mjstests/config.test.tstests/ocx-launcher-source.test.tstests/update-install-process.test.tstests/update-job.test.tstests/update-npm-cache-preflight.test.tstests/update-stop-first.test.tstests/windows-deploy-close-regressions.test.ts
| if (!res.treeExited) { | ||
| console.error("\nopencodex: installer process-tree cleanup could not be confirmed; automatic recovery is unsafe until those processes exit."); | ||
| console.error(` The proxy is stopped. Once no '${npm}' installer processes remain, run 'ocx start' or re-run 'ocx update'.${trayBeforeUpdate.restoreOnFailure ? " The Windows tray also remains stopped; run 'ocx tray start' after the installer processes exit." : ""}`); | ||
| process.exit(INSTALLER_TREE_CLEANUP_FAILED_EXIT_CODE); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Both updaters hide the timeout and interruption cause behind the tree-cleanup message. runProcessTreeCommand sets treeExited = process.platform === "win32" && knownGroupExited whenever cleanup ran (src/update/install-process.mjs lines 319-324), so on Linux and macOS a timeout or a forwarded signal always lands in the !treeExited branch. Both updaters exit there before the timedOut and interruptedSignal branches can report the cause.
bin/ocx.mjs#L246-L250: includeres.timedOut,res.interruptedSignal, andres.statusin the cleanup-failure message beforeprocess.exit(INSTALLER_TREE_CLEANUP_FAILED_EXIT_CODE).src/update/index.ts#L271-L275: apply the same message change usingr.timedOut,r.interruptedSignal, andr.status, so the CLI and the launcher report the identical cause.
📍 Affects 2 files
bin/ocx.mjs#L246-L250(this comment)src/update/index.ts#L271-L275
🤖 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 `@bin/ocx.mjs` around lines 246 - 250, Update the cleanup-failure messages in
bin/ocx.mjs (lines 246-250) and src/update/index.ts (lines 271-275) to include
the installer result’s timeout, interruption signal, and status details before
exiting. Use res.timedOut, res.interruptedSignal, and res.status in the ocx
updater, and r.timedOut, r.interruptedSignal, and r.status in the launcher,
keeping both messages consistent while preserving the existing cleanup warning
and exit code.
| const r = await runProcessTreeCommand(target.bin, cmdArgs, { | ||
| stdio: installStdio, | ||
| encoding: installStdio === "pipe" ? "utf8" : undefined, | ||
| timeout: 180000, | ||
| timeoutMs: 180000, | ||
| windowsHide: true, | ||
| shell: target.shell, | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
src/update/index.ts has no named installer-deadline constant, so the value is duplicated and the CLI path escapes the nesting invariant. bin/ocx.mjs declares NPM_INSTALL_TIMEOUT_MS, but the CLI updater repeats the raw value in the option and again in the failure text, and the test can therefore only verify the launcher.
src/update/index.ts#L264-L269: replacetimeoutMs: 180000with a module-level named constant, for exampleINSTALL_TIMEOUT_MS = 180_000.src/update/index.ts#L374-L379: derive the"timed out after …s"text from that constant instead of the hardcoded180s.tests/update-stop-first.test.ts#L120-L137: parse the new CLI constant as well and assertUPDATE_TIMEOUT_MS >= cliInstallTimeoutMs + 60_000, so the GUI worker deadline is proven to outlive both nested installer deadlines. Update the literal assertion"timeoutMs: 180000"at line 162 in the same change.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
📍 Affects 2 files
src/update/index.ts#L264-L269(this comment)src/update/index.ts#L374-L379tests/update-stop-first.test.ts#L120-L137
🤖 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 `@src/update/index.ts` around lines 264 - 269, The installer timeout is
duplicated as raw values, preventing the CLI deadline and GUI worker deadline
from being validated together. In src/update/index.ts#L264-L269, add a
module-level named installer-timeout constant and use it for
runProcessTreeCommand; in src/update/index.ts#L374-L379, derive the timeout
failure text from that constant. In tests/update-stop-first.test.ts#L120-L137,
parse the CLI constant and assert UPDATE_TIMEOUT_MS is at least 60,000 ms
longer, and update the literal timeout assertion at line 162.
| void cleanupPromise.then(treeExited => { | ||
| if (treeExited) return; | ||
| try { child.kill("SIGKILL"); } catch { /* best-effort root cleanup */ } | ||
| reportCleanupFailure({ status: null, signal: null }); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A rejected cleanupPromise escapes runProcessTreeCommand and aborts the update mid-flight.
Two places consume cleanupPromise without a rejection handler. Line 291 uses void cleanupPromise.then(...), and line 320 uses await cleanupPromise.
terminateInstallerProcessTree can reject. It calls inspect(pid) at lines 169 and 183 outside any try, and inspect may be a caller-supplied inspectProcessGroup override or inspectPsProcessGroup, which calls spawnSync("/bin/ps", ...) and throws synchronously on some failures instead of returning a result object. A rejection at line 320 propagates out of runProcessTreeCommand. Two things then break: the forwarded signal handlers registered at line 305 are never removed, because line 337 is skipped, and the awaiting caller in src/update/index.ts line 264 or bin/ocx.mjs line 240 receives an exception at a point where the proxy is already stopped and no recovery guidance is printed. Line 291 additionally produces an unhandled rejection.
Map a rejection to the fail-closed result the module already uses for unproven cleanup.
🔒 Proposed fix to keep cleanup failures inside the result contract
- cleanupPromise = terminateInstallerProcessTree(child.pid, {
- terminationGraceMs,
- forceWaitMs,
- isOriginalLeader: () => child.exitCode === null && child.signalCode === null,
- inspectProcessGroup: inspect,
- });
+ cleanupPromise = terminateInstallerProcessTree(child.pid, {
+ terminationGraceMs,
+ forceWaitMs,
+ isOriginalLeader: () => child.exitCode === null && child.signalCode === null,
+ inspectProcessGroup: inspect,
+ // An inspection failure cannot prove the tree exited; report it fail-closed.
+ }).catch(() => false);
void cleanupPromise.then(treeExited => {With the .catch in place, the await cleanupPromise at line 320 always resolves and line 337 always runs.
Also applies to: 318-336
🤖 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 `@src/update/install-process.mjs` around lines 291 - 296, Update both consumers
of cleanupPromise in terminateInstallerProcessTree and runProcessTreeCommand to
handle rejections by converting them to the module’s existing fail-closed
cleanup result. Add rejection handling to the detached then chain and ensure the
awaited cleanupPromise always resolves with the failure result, so signal
handlers are removed and runProcessTreeCommand does not propagate cleanup
exceptions.
| function isPathInside(parent: string, child: string): boolean { | ||
| const fromParent = relative(parent, child); | ||
| return fromParent !== "" | ||
| && fromParent !== ".." | ||
| && !fromParent.startsWith(`..${sep}`) | ||
| && !isAbsolute(fromParent); | ||
| } | ||
|
|
||
| function hasTrustedRecoveryOwner(uid: number): boolean { | ||
| const currentUid = process.getuid?.(); | ||
| // Root-owned global installations are trusted and otherwise unrecoverable by | ||
| // an unprivileged GUI process; reject packages planted by every other account. | ||
| return currentUid === undefined || uid === currentUid || uid === 0; | ||
| } | ||
|
|
||
| function hasTrustedRecoveryPermissions(stat: { uid: number; mode: number }): boolean { | ||
| if (!hasTrustedRecoveryOwner(stat.uid)) return false; | ||
| // POSIX group/other write bits make even a trusted-owner entry mutable by a | ||
| // different local account. Windows ACLs are not represented by these bits; | ||
| // npm failed-root recovery is already disabled there when tree exit is unknown. | ||
| return process.getuid === undefined || (stat.mode & 0o022) === 0; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make src/update/recovery-tree-scan.mjs the single source of truth for the recovery-scan contract.
src/update/job.ts re-declares both halves of the scan contract that src/update/recovery-tree-scan.mjs already defines: the three trust predicates and the traversal budgets. The shared root cause is that line 42 imports only RECOVERY_TREE_SCAN_WORKER_ARG from that module, so every other shared value was copied instead of imported. The two copies then form one trust boundary and one budget, with two places to change and no compiler link between them.
src/update/job.ts#L127-L148: remove the localisPathInside,hasTrustedRecoveryOwner, andhasTrustedRecoveryPermissionscopies. Export the originals fromsrc/update/recovery-tree-scan.mjs(lines 8-24), declare them insrc/update/recovery-tree-scan.d.mts, and import them here. If one copy is hardened and the other is not,inspectNpmRecoveryPackagecan accept a candidate that the hardened rule rejected, andprobeNpmRecoveryLauncherthen executes it.src/update/job.ts#L42-L42: extend the import to also pullDEFAULT_MAX_RECOVERY_TREE_ENTRIESandDEFAULT_MAX_RECOVERY_TREE_SCAN_MS.src/update/job.ts#L53-L55: defineMAX_NPM_RECOVERY_TREE_ENTRIESandMAX_NPM_RECOVERY_TREE_SCAN_MSfrom those imported defaults instead of repeating50_000and5_000. The parent passes these values both as the worker payload and as thespawnSynctimeout (lines 171-176), so divergence changes the effective budget on only one side and silently contradictsdocs/adr/0001-gui-update-worker.mdline 30.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
📍 Affects 1 file
src/update/job.ts#L127-L148(this comment)src/update/job.ts#L42-L42src/update/job.ts#L53-L55
🤖 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 `@src/update/job.ts` around lines 127 - 148, The recovery-scan contract is
duplicated in job.ts instead of sourced from recovery-tree-scan.mjs. In
src/update/job.ts lines 127-148, remove the local isPathInside,
hasTrustedRecoveryOwner, and hasTrustedRecoveryPermissions definitions; export
and declare those symbols from recovery-tree-scan.mjs and
recovery-tree-scan.d.mts, then import them in job.ts. In src/update/job.ts lines
42, 53-55, import DEFAULT_MAX_RECOVERY_TREE_ENTRIES and
DEFAULT_MAX_RECOVERY_TREE_SCAN_MS and derive MAX_NPM_RECOVERY_TREE_ENTRIES and
MAX_NPM_RECOVERY_TREE_SCAN_MS from them, preserving the existing worker payload
and spawnSync timeout usage.
| try { | ||
| const retired = readdirSync(scopeRoot, { encoding: "utf8" }) | ||
| .filter(name => name.startsWith(prefix) && /^[A-Za-z0-9]{8}$/.test(name.slice(prefix.length))) | ||
| .map(name => inspectNpmRecoveryPackage(join(scopeRoot, name), expectedVersion)) | ||
| .filter((candidate): candidate is ValidatedNpmLauncher => candidate !== null) | ||
| .sort((a, b) => b.mtimeMs - a.mtimeMs); | ||
| candidates.push(...retired); | ||
| } catch { | ||
| // The current package can still be a valid recovery candidate when the scope | ||
| // directory itself cannot be enumerated. | ||
| } | ||
|
|
||
| const launchers: string[] = []; | ||
| for (const candidate of candidates.slice(0, MAX_NPM_RECOVERY_CANDIDATES)) { | ||
| try { | ||
| // `--version` loads the launcher's static imports, resolves the bundled Bun | ||
| // binary, and imports the complete CLI graph without starting or stopping a proxy. | ||
| if (await probeLauncher(candidate.launcher)) launchers.push(candidate.launcher); | ||
| } catch { /* an un-runnable candidate is equivalent to a partial package */ } | ||
| } | ||
| return launchers; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Bound candidate inspection, not only candidate probing.
Line 259 calls inspectNpmRecoveryPackage for every matching .opencodex-XXXXXXXX directory. Line 269 applies MAX_NPM_RECOVERY_CANDIDATES only afterwards, to the probe loop.
Failure mode, with exact cost: each inspectNpmRecoveryPackage call reaches line 204, which calls hasTrustedRecoveryTree. That runs a blocking spawnSync (line 163) bounded by MAX_NPM_RECOVERY_TREE_SCAN_MS = 5_000 ms and by a 50,000-entry walk of a full node_modules tree. npm accumulates retired sibling directories across failed installs, so with N retired copies the update worker blocks for up to (N + 1) * 5 seconds inside recovery, on the same thread that must still restore the proxy. Recovery already runs after a failed install, when the proxy is down, so this directly extends the outage.
This also contradicts docs/adr/0001-gui-update-worker.md line 47-48, which states that "Candidate inspection and restart are bounded to at most two candidates". Only restart is bounded today.
Concrete fix: order candidates by a cheap directory mtime, cut to the limit, and only then run the full validation.
🐛 Proposed fix
const candidates: ValidatedNpmLauncher[] = [];
const current = inspectNpmRecoveryPackage(packageRoot, expectedVersion);
if (current) candidates.push(current);
try {
- const retired = readdirSync(scopeRoot, { encoding: "utf8" })
- .filter(name => name.startsWith(prefix) && /^[A-Za-z0-9]{8}$/.test(name.slice(prefix.length)))
- .map(name => inspectNpmRecoveryPackage(join(scopeRoot, name), expectedVersion))
- .filter((candidate): candidate is ValidatedNpmLauncher => candidate !== null)
- .sort((a, b) => b.mtimeMs - a.mtimeMs);
- candidates.push(...retired);
+ // Order by a cheap lstat first. Each full inspection spawns a bounded but
+ // blocking tree-scan worker, so never run more than the candidate budget.
+ const ordered = readdirSync(scopeRoot, { encoding: "utf8" })
+ .filter(name => name.startsWith(prefix) && /^[A-Za-z0-9]{8}$/.test(name.slice(prefix.length)))
+ .map(name => {
+ const path = join(scopeRoot, name);
+ try { return { path, mtimeMs: lstatSync(path).mtimeMs }; } catch { return null; }
+ })
+ .filter((entry): entry is { path: string; mtimeMs: number } => entry !== null)
+ .sort((a, b) => b.mtimeMs - a.mtimeMs)
+ .slice(0, MAX_NPM_RECOVERY_CANDIDATES);
+ for (const entry of ordered) {
+ if (candidates.length >= MAX_NPM_RECOVERY_CANDIDATES) break;
+ const candidate = inspectNpmRecoveryPackage(entry.path, expectedVersion);
+ if (candidate) candidates.push(candidate);
+ }
} catch {Note that tests/update-job.test.ts lines 308-338 asserts only probed length 2, so it passes today with four full inspections. Extend that test to count inspections, for example by asserting the number of spawned scan workers.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@src/update/job.ts` around lines 256 - 276, Bound recovery candidates before
expensive validation: in the retired-directory collection around
inspectNpmRecoveryPackage, first sort matching directory names by cheap
filesystem mtime, retain at most MAX_NPM_RECOVERY_CANDIDATES, then call
inspectNpmRecoveryPackage only for that subset before probing. Update the
relevant update-job test to verify inspection/spawned scan count is also limited
to the candidate cap, not merely that two launchers are probed.
| try { | ||
| const accessMode = stat.isDirectory() | ||
| ? constants.R_OK | constants.W_OK | constants.X_OK | ||
| : constants.R_OK | constants.W_OK; | ||
| access(path, accessMode); | ||
| } catch (error) { | ||
| return { | ||
| kind: "error", | ||
| path, | ||
| reason: stat.isDirectory() | ||
| ? `npm cache directory is not readable, writable, and searchable (${errorCode(error)})` | ||
| : `npm cache entry is not readable and writable (${errorCode(error)})`, | ||
| }; | ||
| } | ||
| if (!stat.isDirectory()) continue; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Show npm cacache content file modes and simulate the access gate.
set -uo pipefail
cache="$(npm config get cache 2>/dev/null)"
echo "cache=$cache"
if [ -d "$cache/_cacache/content-v2" ]; then
find "$cache/_cacache/content-v2" -type f -printf '%m %p\n' 2>/dev/null | head -20
echo "--- files without user write bit ---"
find "$cache/_cacache/content-v2" -type f ! -writable 2>/dev/null | wc -l
fiRepository: lidge-jun/opencodex
Length of output: 183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target file ---'
sed -n '1,180p' src/update/npm-cache-preflight.mjs
printf '%s\n' '--- related tests ---'
sed -n '1,230p' tests/update-npm-cache-preflight.test.ts
printf '%s\n' '--- call sites ---'
rg -n -C 5 'checkNpmCacheOwnership|findForeignOwnedNpmCacheEntry|npm-cache-preflight' src bin testsRepository: lidge-jun/opencodex
Length of output: 34873
🌐 Web query:
npm cacache content-v2 files read-only mode doctor new cacache sets content files to be read-only
💡 Result:
Cacache is designed to ensure data integrity by storing content files in a read-only state [1]. Because cacache is a content-addressable cache where files are identified by their integrity hashes (such as SHA-512), the stored content is immutable [1][2]. The only valid operation on existing content is deletion, which requires write permissions on the parent directory rather than the file itself [1]. Consequently, cacache marks these content files as read-only to signal to the operating system that they should not be modified [1]. The reference to "doctor: new cacache sets content files to be read-only" specifically refers to a change in the npm CLI and the cacache library (introduced around 2017) to formalize this read-only behavior for content files within the content-v2 storage directory [1][3]. The npm doctor command, which checks for potential issues in an npm installation, was updated to account for this behavior [3][4]. npm doctor verifies that the cache directory and its contents are accessible, and it ensures that permissions are correctly configured so that npm can properly manage the cache without encountering errors related to these read-only files [4].
Citations:
- 1: npm/cacache@4131196
- 2: https://deepwiki.com/npm/cacache
- 3: npm/npm@662a15a
- 4: https://docs.npmjs.com/cli/v10/commands/npm-doctor/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository npm version/configuration references ---'
rg -n -i -C 3 'npm|cacache|content-v2' package.json package-lock.json npm-shrinkwrap.json .npmrc README.md 2>/dev/null || true
printf '%s\n' '--- isolated access(2)-equivalent permission probe ---'
python3 - <<'PY'
import os
import pathlib
import tempfile
with tempfile.TemporaryDirectory() as root:
path = pathlib.Path(root) / "content"
path.write_bytes(b"cache")
os.chmod(path, 0o444)
print(f"mode={oct(path.stat().st_mode & 0o777)}")
print(f"readable={os.access(path, os.R_OK)}")
print(f"writable={os.access(path, os.W_OK)}")
print(f"read_write_gate={os.access(path, os.R_OK | os.W_OK)}")
PYRepository: lidge-jun/opencodex
Length of output: 4294
Do not require write access on npm cache files
npm cacache stores content-v2 blobs as read-only files. Deletion requires write access to the containing directory. At src/update/npm-cache-preflight.mjs:97-111, W_OK can therefore reject a healthy cache and abort updates at src/update/index.ts:173-176 and bin/ocx.mjs:142-145. Use constants.R_OK for non-directory entries, keep R_OK | W_OK | X_OK for directories, update the file error message, and add a regression test for a read-only cache file.
🤖 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 `@src/update/npm-cache-preflight.mjs` around lines 97 - 111, Update the
accessMode selection in the npm cache preflight check to use only constants.R_OK
for non-directory entries, while retaining constants.R_OK | constants.W_OK |
constants.X_OK for directories. Revise the non-directory error message to
mention readability only, and add a regression test covering a readable
read-only cache file.
| function parseOwnershipIssue(output, cachePath) { | ||
| try { | ||
| const parsed = JSON.parse(output); | ||
| if (parsed === null) return null; | ||
| if (!parsed || typeof parsed !== "object" || typeof parsed.path !== "string") throw new Error("invalid issue"); | ||
| if (parsed.kind === "foreign-owner" && Number.isInteger(parsed.actualUid)) return parsed; | ||
| if (parsed.kind === "error" && typeof parsed.reason === "string") return parsed; | ||
| } catch { /* convert malformed worker output into a fail-closed result below */ } | ||
| return { | ||
| kind: "error", | ||
| path: resolve(cachePath), | ||
| reason: "npm cache inspection returned invalid output", | ||
| }; | ||
| } | ||
|
|
||
| /** Run the blocking filesystem walk out-of-process so its wall-clock deadline is enforceable. */ | ||
| function scanNpmCacheOwnership(cachePath, expectedUid, options) { | ||
| const maxEntries = positiveInteger(options.maxEntries, DEFAULT_MAX_CACHE_ENTRIES); | ||
| const maxDurationMs = positiveInteger(options.maxDurationMs, DEFAULT_CACHE_SCAN_TIMEOUT_MS); | ||
| const scanSpawn = options.scanSpawn ?? spawnSync; | ||
| let result; | ||
| try { | ||
| result = scanSpawn( | ||
| options.scanBin ?? process.execPath, | ||
| [options.scanScript ?? fileURLToPath(import.meta.url), CACHE_SCAN_WORKER_ARG], | ||
| { | ||
| encoding: "utf8", | ||
| input: JSON.stringify({ cachePath, expectedUid, maxEntries, maxDurationMs }), | ||
| timeout: maxDurationMs, | ||
| killSignal: "SIGKILL", | ||
| windowsHide: true, | ||
| shell: false, | ||
| }, | ||
| ); | ||
| } catch (error) { | ||
| return { | ||
| kind: "error", | ||
| path: resolve(cachePath), | ||
| reason: `could not inspect npm cache (${errorCode(error)})`, | ||
| }; | ||
| } | ||
| if (result.status !== 0) { | ||
| const timedOut = result.status === null || errorCode(result.error) === "ETIMEDOUT"; | ||
| return { | ||
| kind: "error", | ||
| path: resolve(cachePath), | ||
| reason: timedOut | ||
| ? `npm cache inspection exceeded its ${maxDurationMs}ms time budget` | ||
| : `could not inspect npm cache (${result.error ? errorCode(result.error) : `status ${result.status}`})`, | ||
| }; | ||
| } | ||
| const output = Buffer.isBuffer(result.stdout) ? result.stdout.toString("utf8") : String(result.stdout ?? ""); | ||
| return parseOwnershipIssue(output, cachePath); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The scan timeout and the in-worker time budget use the same value, so the worker rarely reports its own budget message.
Line 164 computes maxDurationMs, line 173 passes it into the worker payload, and line 174 sets the spawnSync timeout to that same maxDurationMs. The parent deadline and the child deadline expire at the same instant, and the parent also pays process spawn time. The parent therefore usually kills the worker with SIGKILL before the worker can print its own "exceeded its Nms time budget" result. The outcome text is identical in both paths, so behavior stays correct, but the child budget is effectively dead code and the scan can be cut off before it finishes an otherwise-in-budget walk.
Give the parent a small margin over the child budget.
♻️ Proposed margin between the two deadlines
- timeout: maxDurationMs,
+ // The worker enforces `maxDurationMs` itself; give the parent a margin so
+ // spawn latency does not preempt an in-budget walk.
+ timeout: maxDurationMs + SCAN_SPAWN_MARGIN_MS,Declare the margin with the other constants:
const SCAN_SPAWN_MARGIN_MS = 2_000;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function parseOwnershipIssue(output, cachePath) { | |
| try { | |
| const parsed = JSON.parse(output); | |
| if (parsed === null) return null; | |
| if (!parsed || typeof parsed !== "object" || typeof parsed.path !== "string") throw new Error("invalid issue"); | |
| if (parsed.kind === "foreign-owner" && Number.isInteger(parsed.actualUid)) return parsed; | |
| if (parsed.kind === "error" && typeof parsed.reason === "string") return parsed; | |
| } catch { /* convert malformed worker output into a fail-closed result below */ } | |
| return { | |
| kind: "error", | |
| path: resolve(cachePath), | |
| reason: "npm cache inspection returned invalid output", | |
| }; | |
| } | |
| /** Run the blocking filesystem walk out-of-process so its wall-clock deadline is enforceable. */ | |
| function scanNpmCacheOwnership(cachePath, expectedUid, options) { | |
| const maxEntries = positiveInteger(options.maxEntries, DEFAULT_MAX_CACHE_ENTRIES); | |
| const maxDurationMs = positiveInteger(options.maxDurationMs, DEFAULT_CACHE_SCAN_TIMEOUT_MS); | |
| const scanSpawn = options.scanSpawn ?? spawnSync; | |
| let result; | |
| try { | |
| result = scanSpawn( | |
| options.scanBin ?? process.execPath, | |
| [options.scanScript ?? fileURLToPath(import.meta.url), CACHE_SCAN_WORKER_ARG], | |
| { | |
| encoding: "utf8", | |
| input: JSON.stringify({ cachePath, expectedUid, maxEntries, maxDurationMs }), | |
| timeout: maxDurationMs, | |
| killSignal: "SIGKILL", | |
| windowsHide: true, | |
| shell: false, | |
| }, | |
| ); | |
| } catch (error) { | |
| return { | |
| kind: "error", | |
| path: resolve(cachePath), | |
| reason: `could not inspect npm cache (${errorCode(error)})`, | |
| }; | |
| } | |
| if (result.status !== 0) { | |
| const timedOut = result.status === null || errorCode(result.error) === "ETIMEDOUT"; | |
| return { | |
| kind: "error", | |
| path: resolve(cachePath), | |
| reason: timedOut | |
| ? `npm cache inspection exceeded its ${maxDurationMs}ms time budget` | |
| : `could not inspect npm cache (${result.error ? errorCode(result.error) : `status ${result.status}`})`, | |
| }; | |
| } | |
| const output = Buffer.isBuffer(result.stdout) ? result.stdout.toString("utf8") : String(result.stdout ?? ""); | |
| return parseOwnershipIssue(output, cachePath); | |
| } | |
| function parseOwnershipIssue(output, cachePath) { | |
| try { | |
| const parsed = JSON.parse(output); | |
| if (parsed === null) return null; | |
| if (!parsed || typeof parsed !== "object" || typeof parsed.path !== "string") throw new Error("invalid issue"); | |
| if (parsed.kind === "foreign-owner" && Number.isInteger(parsed.actualUid)) return parsed; | |
| if (parsed.kind === "error" && typeof parsed.reason === "string") return parsed; | |
| } catch { /* convert malformed worker output into a fail-closed result below */ } | |
| return { | |
| kind: "error", | |
| path: resolve(cachePath), | |
| reason: "npm cache inspection returned invalid output", | |
| }; | |
| } | |
| /** Run the blocking filesystem walk out-of-process so its wall-clock deadline is enforceable. */ | |
| function scanNpmCacheOwnership(cachePath, expectedUid, options) { | |
| const maxEntries = positiveInteger(options.maxEntries, DEFAULT_MAX_CACHE_ENTRIES); | |
| const maxDurationMs = positiveInteger(options.maxDurationMs, DEFAULT_CACHE_SCAN_TIMEOUT_MS); | |
| const scanSpawn = options.scanSpawn ?? spawnSync; | |
| let result; | |
| try { | |
| result = scanSpawn( | |
| options.scanBin ?? process.execPath, | |
| [options.scanScript ?? fileURLToPath(import.meta.url), CACHE_SCAN_WORKER_ARG], | |
| { | |
| encoding: "utf8", | |
| input: JSON.stringify({ cachePath, expectedUid, maxEntries, maxDurationMs }), | |
| // The worker enforces `maxDurationMs` itself; give the parent a margin so | |
| // spawn latency does not preempt an in-budget walk. | |
| timeout: maxDurationMs + SCAN_SPAWN_MARGIN_MS, | |
| killSignal: "SIGKILL", | |
| windowsHide: true, | |
| shell: false, | |
| }, | |
| ); | |
| } catch (error) { | |
| return { | |
| kind: "error", | |
| path: resolve(cachePath), | |
| reason: `could not inspect npm cache (${errorCode(error)})`, | |
| }; | |
| } | |
| if (result.status !== 0) { | |
| const timedOut = result.status === null || errorCode(result.error) === "ETIMEDOUT"; | |
| return { | |
| kind: "error", | |
| path: resolve(cachePath), | |
| reason: timedOut | |
| ? `npm cache inspection exceeded its ${maxDurationMs}ms time budget` | |
| : `could not inspect npm cache (${result.error ? errorCode(result.error) : `status ${result.status}`})`, | |
| }; | |
| } | |
| const output = Buffer.isBuffer(result.stdout) ? result.stdout.toString("utf8") : String(result.stdout ?? ""); | |
| return parseOwnershipIssue(output, cachePath); | |
| } |
🤖 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 `@src/update/npm-cache-preflight.mjs` around lines 146 - 199, Update
scanNpmCacheOwnership so the parent spawnSync timeout exceeds the worker’s
maxDurationMs by a small margin, using a shared SCAN_SPAWN_MARGIN_MS constant
declared with the other scan constants. Keep maxDurationMs in the worker payload
unchanged, and apply the margin only to the parent timeout so the worker can
report its own budget expiration.
| test("finds a validated npm-retired launcher when the current package is partial", async () => { | ||
| const scopeRoot = join(dir, "global", "@bitkyc08"); | ||
| const currentRoot = join(scopeRoot, "opencodex"); | ||
| const retiredRoot = join(scopeRoot, ".opencodex-Ab12Cd34"); | ||
| for (const [root, name, version] of [ | ||
| [currentRoot, "@bitkyc08/opencodex", "2.7.41"], | ||
| [retiredRoot, "@bitkyc08/opencodex", "2.7.40"], | ||
| ] as const) { | ||
| mkdirSync(join(root, "bin"), { recursive: true }); | ||
| writeFileSync(join(root, "package.json"), JSON.stringify({ name, version })); | ||
| writeFileSync(join(root, "bin", "ocx.mjs"), "#!/usr/bin/env node\n"); | ||
| } | ||
|
|
||
| expect(await findNpmRecoveryLauncher(join(currentRoot, "bin", "ocx.mjs"), "2.7.40")) | ||
| .toBe(realpathSync(join(retiredRoot, "bin", "ocx.mjs"))); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Set explicit timeouts on the recovery tests that spawn real subprocesses.
These two tests omit the probeLauncher argument, so findNpmRecoveryLaunchers uses the default probeNpmRecoveryLauncher (src/update/job.ts lines 232-238). Each accepted candidate then costs real subprocess work:
- one blocking
spawnSynctree-scan worker per inspected candidate (src/update/job.tsline 163), each bounded by 5_000 ms; - one
runProcessTreeCommandlauncher probe per candidate (src/update/job.tsline 233), each bounded by 10_000 ms.
For the test at line 155, both roots are valid, so the flow spawns two scan workers plus two --version probes. Bun's default per-test timeout is 5_000 ms, which is below the single-probe budget alone. On a loaded CI runner these tests can fail for timing reasons rather than for a behavior regression, and that noise lands on the exact security-sensitive path this PR adds.
The neighbouring tests already do this: line 356 uses }, 5_000) and tests/update-install-process.test.ts lines 165, 197, 230 use }, 15_000).
💚 Proposed fix
expect(await findNpmRecoveryLauncher(join(currentRoot, "bin", "ocx.mjs"), "2.7.40"))
.toBe(realpathSync(join(retiredRoot, "bin", "ocx.mjs")));
- });
+ }, 30_000); expect(await findNpmRecoveryLaunchers(join(currentRoot, "bin", "ocx.mjs"), "2.7.40"))
.toEqual([realpathSync(join(retiredRoot, "bin", "ocx.mjs"))]);
- });
+ }, 30_000);Also applies to: 290-306
🤖 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 `@tests/update-job.test.ts` around lines 155 - 170, Set explicit extended
timeouts on both recovery tests, including the test identified by
findNpmRecoveryLauncher and the additional test around the related recovery
flow. Append the timeout using the existing test framework syntax, choosing a
value consistent with neighboring subprocess-based tests (such as 15,000 ms),
without changing their assertions or setup.
| test("force-kills an npm cache lookup that ignores SIGTERM", () => { | ||
| const uid = process.getuid?.(); | ||
| if (uid === undefined) return; | ||
| const blockingNpm = join(dir, "blocking-npm.mjs"); | ||
| writeFileSync( | ||
| blockingNpm, | ||
| "#!/usr/bin/env node\nprocess.on('SIGTERM', () => {});\nsetInterval(() => {}, 60_000);\n", | ||
| ); | ||
| chmodSync(blockingNpm, 0o755); | ||
|
|
||
| const startedAt = Date.now(); | ||
| const result = checkNpmCacheOwnership({ | ||
| getuid: () => uid, | ||
| lookupTimeoutMs: 250, | ||
| npmBin: blockingNpm, | ||
| }); | ||
| expect(Date.now() - startedAt).toBeLessThan(2_000); | ||
| expect(result).toMatchObject({ | ||
| ok: false, | ||
| reason: "could not resolve the npm cache (ETIMEDOUT)", | ||
| }); | ||
| }, 5_000); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This test needs node on PATH, which a Bun-only environment may not provide.
Line 255 writes the fake npm binary with the shebang #!/usr/bin/env node, and line 263 passes it as npmBin while checkNpmCacheOwnership spawns with shell: false. The kernel then resolves node through PATH. If the CI image ships Bun only, spawnSync fails with ENOENT, the result reason becomes "could not resolve the npm cache (ENOENT)", and the assertion on line 268 fails for an environment reason rather than a behavior regression.
Point the shebang at the interpreter that is guaranteed to exist, or spawn it explicitly.
🔧 Proposed fix to remove the `node` dependency
writeFileSync(
blockingNpm,
- "#!/usr/bin/env node\nprocess.on('SIGTERM', () => {});\nsetInterval(() => {}, 60_000);\n",
+ `#!${process.execPath}\nprocess.on('SIGTERM', () => {});\nsetInterval(() => {}, 60_000);\n`,
);As per path instructions "Runtime is Bun-native TypeScript (no separate compile step). Flag Node-only APIs that break under Bun".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("force-kills an npm cache lookup that ignores SIGTERM", () => { | |
| const uid = process.getuid?.(); | |
| if (uid === undefined) return; | |
| const blockingNpm = join(dir, "blocking-npm.mjs"); | |
| writeFileSync( | |
| blockingNpm, | |
| "#!/usr/bin/env node\nprocess.on('SIGTERM', () => {});\nsetInterval(() => {}, 60_000);\n", | |
| ); | |
| chmodSync(blockingNpm, 0o755); | |
| const startedAt = Date.now(); | |
| const result = checkNpmCacheOwnership({ | |
| getuid: () => uid, | |
| lookupTimeoutMs: 250, | |
| npmBin: blockingNpm, | |
| }); | |
| expect(Date.now() - startedAt).toBeLessThan(2_000); | |
| expect(result).toMatchObject({ | |
| ok: false, | |
| reason: "could not resolve the npm cache (ETIMEDOUT)", | |
| }); | |
| }, 5_000); | |
| test("force-kills an npm cache lookup that ignores SIGTERM", () => { | |
| const uid = process.getuid?.(); | |
| if (uid === undefined) return; | |
| const blockingNpm = join(dir, "blocking-npm.mjs"); | |
| writeFileSync( | |
| blockingNpm, | |
| `#!${process.execPath}\nprocess.on('SIGTERM', () => {});\nsetInterval(() => {}, 60_000);\n`, | |
| ); | |
| chmodSync(blockingNpm, 0o755); | |
| const startedAt = Date.now(); | |
| const result = checkNpmCacheOwnership({ | |
| getuid: () => uid, | |
| lookupTimeoutMs: 250, | |
| npmBin: blockingNpm, | |
| }); | |
| expect(Date.now() - startedAt).toBeLessThan(2_000); | |
| expect(result).toMatchObject({ | |
| ok: false, | |
| reason: "could not resolve the npm cache (ETIMEDOUT)", | |
| }); | |
| }, 5_000); |
🤖 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 `@tests/update-npm-cache-preflight.test.ts` around lines 249 - 270, Update the
fake blocking npm executable created by the “force-kills an npm cache lookup
that ignores SIGTERM” test so it does not depend on resolving node through PATH.
Use the guaranteed Bun interpreter in the shebang, or otherwise invoke Bun
explicitly while preserving the existing blocking and SIGTERM-ignoring behavior
and the ETIMEDOUT assertion.
Source: Path instructions
| test("fails closed before shutdown when npm cannot resolve its cache", () => { | ||
| let inspected = false; | ||
| const result = checkNpmCacheOwnership({ | ||
| platform: "linux", | ||
| getuid: () => 501, | ||
| spawn: (() => ({ | ||
| status: null, | ||
| stdout: "", | ||
| stderr: "", | ||
| error: Object.assign(new Error("timed out"), { code: "ETIMEDOUT" }), | ||
| pid: 1, | ||
| output: [], | ||
| signal: "SIGTERM", | ||
| })) as never, | ||
| lstat: () => { | ||
| inspected = true; | ||
| throw new Error("must not inspect without a resolved cache path"); | ||
| }, | ||
| }); | ||
| expect(result).toMatchObject({ | ||
| ok: false, | ||
| expectedUid: 501, | ||
| reason: "could not resolve the npm cache (ETIMEDOUT)", | ||
| }); | ||
| expect(inspected).toBe(false); | ||
| if (result.ok !== false) throw new Error("expected lookup failure"); | ||
| expect(formatNpmCacheOwnershipFailure(result)).toContain("before stopping the proxy"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The inspected assertion cannot fail, so this test does not prove the claimed behavior.
The test stubs lstat on line 286 and asserts inspected === false on line 296. checkNpmCacheOwnership never calls lstat. It delegates the walk to the out-of-process worker and forwards only cachePath, expectedUid, maxEntries, and maxDurationMs (see src/update/npm-cache-preflight.mjs lines 168-179). The hook would stay unused even if the function scanned the cache after a failed lookup. Assert on the scan spawn instead, because that is the observable the code actually uses.
💚 Proposed fix to assert on the real observable
})) as never,
- lstat: () => {
- inspected = true;
- throw new Error("must not inspect without a resolved cache path");
- },
+ scanSpawn: (() => {
+ inspected = true;
+ throw new Error("must not inspect without a resolved cache path");
+ }) as never,
});As per path instructions "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("fails closed before shutdown when npm cannot resolve its cache", () => { | |
| let inspected = false; | |
| const result = checkNpmCacheOwnership({ | |
| platform: "linux", | |
| getuid: () => 501, | |
| spawn: (() => ({ | |
| status: null, | |
| stdout: "", | |
| stderr: "", | |
| error: Object.assign(new Error("timed out"), { code: "ETIMEDOUT" }), | |
| pid: 1, | |
| output: [], | |
| signal: "SIGTERM", | |
| })) as never, | |
| lstat: () => { | |
| inspected = true; | |
| throw new Error("must not inspect without a resolved cache path"); | |
| }, | |
| }); | |
| expect(result).toMatchObject({ | |
| ok: false, | |
| expectedUid: 501, | |
| reason: "could not resolve the npm cache (ETIMEDOUT)", | |
| }); | |
| expect(inspected).toBe(false); | |
| if (result.ok !== false) throw new Error("expected lookup failure"); | |
| expect(formatNpmCacheOwnershipFailure(result)).toContain("before stopping the proxy"); | |
| }); | |
| test("fails closed before shutdown when npm cannot resolve its cache", () => { | |
| let inspected = false; | |
| const result = checkNpmCacheOwnership({ | |
| platform: "linux", | |
| getuid: () => 501, | |
| spawn: (() => ({ | |
| status: null, | |
| stdout: "", | |
| stderr: "", | |
| error: Object.assign(new Error("timed out"), { code: "ETIMEDOUT" }), | |
| pid: 1, | |
| output: [], | |
| signal: "SIGTERM", | |
| })) as never, | |
| scanSpawn: (() => { | |
| inspected = true; | |
| throw new Error("must not inspect without a resolved cache path"); | |
| }) as never, | |
| }); | |
| expect(result).toMatchObject({ | |
| ok: false, | |
| expectedUid: 501, | |
| reason: "could not resolve the npm cache (ETIMEDOUT)", | |
| }); | |
| expect(inspected).toBe(false); | |
| if (result.ok !== false) throw new Error("expected lookup failure"); | |
| expect(formatNpmCacheOwnershipFailure(result)).toContain("before stopping the proxy"); | |
| }); |
🤖 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 `@tests/update-npm-cache-preflight.test.ts` around lines 272 - 299, Update the
test “fails closed before shutdown when npm cannot resolve its cache” to track
scan-worker invocation rather than stubbing `lstat`, since
`checkNpmCacheOwnership` delegates scanning through `spawn`. Configure the
existing `spawn` mock to record calls and assert that no scan invocation occurs
when cache resolution fails, while preserving the current failure result and
formatted-message assertions.
Source: Path instructions
2 participants
Summary
Verification
Checklist
Summary by CodeRabbit
New Features
Documentation