fix(cli): coerce ServerSupervisor exit code to number — prevents TypeError on Node.js v24 (#3748) - #3750
Conversation
…Error on Node.js v24 (#3748) Node.js v24 added strict type checking to process.exit() and throws TypeError [ERR_INVALID_ARG_TYPE] when given a non-number. The spawn 'error' event passes err.code (e.g. 'ENOENT') — a string, not a number — via `err.code ?? -1` (nullish coalescing doesn't help since 'ENOENT' is not null/undefined). handleExit() now normalises the code to a number at the top; the 'error' callback passes -1 unconditionally.
There was a problem hiding this comment.
Code Review
This pull request addresses a Node.js v24 compatibility issue where process.exit() throws an error if passed a string instead of a number. The ServerSupervisor.handleExit method was updated to normalize exit codes, and a corresponding unit test was added. The review feedback suggests wrapping the global stubbing of process.exit in a try...finally block within the unit test to guarantee the original function is restored even if assertions fail.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const exits: Array<number | string | undefined> = []; | ||
| const origExit = process.exit.bind(process); | ||
| // @ts-ignore | ||
| process.exit = (code?: number | string) => exits.push(code); | ||
|
|
||
| const supervisor = new ServerSupervisor({ | ||
| serverPath: "/fake/server.js", | ||
| env: {}, | ||
| maxRestarts: 0, | ||
| }); | ||
| // Simulates the 'error' event on child spawn failure: err.code = 'ENOENT' (string, not number). | ||
| // maxRestarts=0 → restartCount(0) >= maxRestarts(0) → process.exit() is called immediately. | ||
| supervisor.startedAt = Date.now() - 100; | ||
| supervisor.handleExit("ENOENT" as any); | ||
|
|
||
| // @ts-ignore | ||
| process.exit = origExit; |
There was a problem hiding this comment.
Stubbing process.exit globally without a try...finally block is risky. If any assertion fails or an unexpected error is thrown during the test execution, process.exit will remain stubbed. This can cause subsequent tests to behave unpredictably or prevent the test runner from exiting properly. Wrapping the test logic in a try...finally block ensures that process.exit is always restored to its original implementation.
| const exits: Array<number | string | undefined> = []; | |
| const origExit = process.exit.bind(process); | |
| // @ts-ignore | |
| process.exit = (code?: number | string) => exits.push(code); | |
| const supervisor = new ServerSupervisor({ | |
| serverPath: "/fake/server.js", | |
| env: {}, | |
| maxRestarts: 0, | |
| }); | |
| // Simulates the 'error' event on child spawn failure: err.code = 'ENOENT' (string, not number). | |
| // maxRestarts=0 → restartCount(0) >= maxRestarts(0) → process.exit() is called immediately. | |
| supervisor.startedAt = Date.now() - 100; | |
| supervisor.handleExit("ENOENT" as any); | |
| // @ts-ignore | |
| process.exit = origExit; | |
| const exits: Array<number | string | undefined> = []; | |
| const origExit = process.exit; | |
| try { | |
| // @ts-ignore | |
| process.exit = (code?: number | string) => { exits.push(code); }; | |
| const supervisor = new ServerSupervisor({ | |
| serverPath: "/fake/server.js", | |
| env: {}, | |
| maxRestarts: 0, | |
| }); | |
| // Simulates the 'error' event on child spawn failure: err.code = 'ENOENT' (string, not number). | |
| // maxRestarts=0 → restartCount(0) >= maxRestarts(0) → process.exit() is called immediately. | |
| supervisor.startedAt = Date.now() - 100; | |
| supervisor.handleExit("ENOENT" as any); | |
| } finally { | |
| process.exit = origExit; | |
| } |
| const { ServerSupervisor } = await import("../../bin/cli/runtime/processSupervisor.mjs"); | ||
|
|
||
| const exits: Array<number | string | undefined> = []; | ||
| const origExit = process.exit.bind(process); |
There was a problem hiding this comment.
SUGGESTION: Narrow the stub result type and assert the expected exit code
exits is typed to allow strings, and the test only asserts that the value is a number. Use number[] and assert exits[0] === 1 so the regression pins both Node.js v24 compatibility and the intended failure exit code.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Other Observations (not in diff)No additional unchanged-code issues identified. Files Reviewed (3 files)
Fix these issues in Kilo Cloud Reviewed by nex-n2-pro:free · 372,926 tokens |
…Error on Node.js v24 (diegosouzapw#3748) (diegosouzapw#3750) Node.js v24 added strict type checking to process.exit() and throws TypeError [ERR_INVALID_ARG_TYPE] when given a non-number. The spawn 'error' event passes err.code (e.g. 'ENOENT') — a string, not a number — via `err.code ?? -1` (nullish coalescing doesn't help since 'ENOENT' is not null/undefined). handleExit() now normalises the code to a number at the top; the 'error' callback passes -1 unconditionally.
Closes #3748
Summary
Node.js v24 throws
TypeError [ERR_INVALID_ARG_TYPE]: The 'code' argument must be of type numberwhenprocess.exit()receives a string. The spawnerrorevent passeserr.code(e.g.'ENOENT') viaerr.code ?? -1— nullish coalescing doesn't help since'ENOENT'is neithernullnorundefined.Root cause in
processSupervisor.mjs:this.handleExit(err.code ?? -1, err)→ passes'ENOENT'(string)process.exit(code || 0)/process.exit(code ?? 1)→ both receive a string → TypeError on Node.js v24Fix:
errorcallback now passes-1unconditionally (err.codeis an OS error string, not a meaningful exit code)handleExit()normalisescodeto a number at the top:const exitCode = typeof code === 'number' ? code : nullprocess.exit()calls useexitCodewith safe numeric defaultsRegression Test
tests/unit/cli-process-supervisor.test.ts— new test callssupervisor.handleExit('ENOENT')directly (mimicking the spawn error path) and asserts thatprocess.exitreceives anumber, not a string. Confirmed failing before fix, passing after.