fix(cli): address PR review — guard NaN signal code, scrub marker via constant

- runReexec: os.constants.signals[signal] can be undefined for a signal name
  absent on the platform; 128 + undefined = NaN and process.exit(NaN) coerces to
  0, masking a signal-abort as success. Guard with a nonzero fallback. Add a
  cross-platform wiring test (SIGSTKFLT, absent on macOS) asserting a nonzero,
  non-NaN code.
- dev.ts: scrub the handshake marker from the user command's env in
  runChildProcess using the exported REEXEC_MARKER_ENV constant instead of a
  hardcoded string, so a rename can't desync it. This also covers the
  Windows-direct spawn path (the wrapper script only runs on POSIX).
This commit is contained in:
Bilal Godil 2026-06-16 15:59:16 -07:00
parent ac9271f487
commit 405a2b34bf
3 changed files with 39 additions and 10 deletions

View File

@ -9,7 +9,7 @@ import { resolveConfigFilePathOption } from "../lib/config-file-path.js";
import { devEnvStatePath, ensureLocalDashboardSecret, readDevEnvState, recordLocalDashboardProcess } from "../lib/dev-env-state.js";
import { CliError } from "../lib/errors.js";
import { cliVersion } from "../lib/own-package.js";
import { maybeReexecToLatest } from "../lib/self-update.js";
import { maybeReexecToLatest, REEXEC_MARKER_ENV } from "../lib/self-update.js";
type ChildCommand = {
command: string,
@ -693,8 +693,6 @@ const childEnv = { ...process.env };
delete childEnv.HEXCLAVE_DEV_APP_COMMAND_PARENT_PID;
delete childEnv.HEXCLAVE_DEV_APP_COMMAND;
delete childEnv.HEXCLAVE_DEV_APP_COMMAND_ARGS_JSON;
// Internal to the npx auto-update handshake; never meant for the user's command.
delete childEnv.HEXCLAVE_CLI_REEXEC_MARKER;
const child = spawn(command, args, {
env: childEnv,
@ -751,14 +749,20 @@ child.on("error", (error) => {
`;
function runChildProcess(command: ChildCommand, env: NodeJS.ProcessEnv): Promise<number> {
// The npx auto-update handshake marker is internal to the parent<->child
// re-exec; never leak it into the user's command. Scrub it here (rather than in
// the wrapper script) so it uses the exported constant and covers both the
// Windows-direct and POSIX-wrapper spawn paths.
const childEnv = { ...env };
delete childEnv[REEXEC_MARKER_ENV];
return new Promise((resolvePromise, reject) => {
const child = process.platform === "win32"
? spawn(command.command, command.args, { stdio: "inherit", env })
? spawn(command.command, command.args, { stdio: "inherit", env: childEnv })
: spawn(process.execPath, ["-e", APP_COMMAND_WRAPPER_SCRIPT], {
detached: true,
stdio: "inherit",
env: {
...env,
...childEnv,
[APP_COMMAND_WRAPPER_PARENT_PID_ENV_VAR]: String(process.pid),
[APP_COMMAND_WRAPPER_COMMAND_ENV_VAR]: command.command,
[APP_COMMAND_WRAPPER_ARGS_ENV_VAR]: JSON.stringify(command.args),

View File

@ -336,4 +336,24 @@ describe("maybeReexecToLatest fallback wiring", () => {
await promise;
expect(exitSpy).toHaveBeenCalledWith(1);
});
it("propagates a nonzero, non-NaN code when killed by a signal missing from os.constants", async () => {
const exitSpy = vi.spyOn(process, "exit").mockImplementation(((() => {
throw new Error("__exit__");
}) as never));
const child = mockNpxChild({ writeMarker: false });
const promise = maybeReexecToLatest({ forwardArgs: ["dev"] });
// SIGSTKFLT is absent from os.constants.signals on macOS (present on Linux).
// Either way the abort must surface as a real nonzero code — never NaN (which
// process.exit coerces to 0, masking the abort as success).
child.emit("close", null, "SIGSTKFLT");
await promise;
expect(exitSpy).toHaveBeenCalledTimes(1);
const code = exitSpy.mock.calls[0][0];
expect(typeof code).toBe("number");
expect(Number.isNaN(code as number)).toBe(false);
expect(code as number).toBeGreaterThan(0);
});
});

View File

@ -138,11 +138,16 @@ function runReexec(invocation: NpxInvocation, markerFile: string | null): Promis
child.on("close", (code, signal) => {
cleanup();
if (signal != null) {
// Killed by a signal we forwarded (e.g. Ctrl-C). Report it with the
// conventional 128 + signal-number exit code so the caller can both
// recognize the abort and propagate a sensible code.
const signalNumber = osConstants.signals[signal];
resolvePromise({ exited: true, code: 128 + signalNumber, signal });
// Killed by a signal (a Ctrl-C we forwarded, or an external kill). Report
// the conventional 128 + signal-number exit code so the caller can both
// recognize the abort and propagate a sensible code. `os.constants.signals`
// doesn't list every NodeJS.Signals name on every platform, so the lookup
// can be undefined at runtime (the type says otherwise) — `128 + undefined`
// is NaN and `process.exit(NaN)` coerces to 0, masking the abort as
// success. Fall back to a generic nonzero code in that case.
const signalNumber = osConstants.signals[signal] as number | undefined;
const code = signalNumber != null ? 128 + signalNumber : 1;
resolvePromise({ exited: true, code, signal });
return;
}
resolvePromise({ exited: true, code: code ?? 1, signal: null });