fix(kimi-code): exit 1 when a headless (-p) turn fails#1483
Conversation
🦋 Changeset detectedLatest commit: ca7a1b4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![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: 64d8cf456c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // code 0. Setting the exit code here makes that drain-exit report failure | ||
| // too. main's .catch() sets the same code as a belt-and-suspenders for the | ||
| // case where cleanup completes but a later await in .catch() drains. | ||
| process.exitCode = 1; |
There was a problem hiding this comment.
When runPrompt is called directly and the caller handles the rejection, as the existing apps/kimi-code/test/cli/run-prompt.test.ts rejection cases do, this mutates the real process.exitCode even when a fake io.process is supplied and nothing restores it. That leaves the Vitest worker/process marked as failed after an asserted rejection, so the CLI test suite can exit with status 1 despite passing assertions; keep this side effect in the entrypoint or make direct callers/tests restore it.
Useful? React with 👍 / 👎.
Headless (`kimi -p`) failures could exit with code 0 when the event loop drained during the shutdown cleanup (e.g. telemetry's unref'd retry backoff when the network is blocked), because the rejection never reached the process.exit(1) call. Set the failure exit code before any await in both the run-prompt catch and the main catch, and keep the cleanup timeout ref'd so the loop stays alive long enough for the rejection to propagate.
wbxl2000
force-pushed
the
fix/headless-exit-code
branch
from
64d8cf4 to
ca7a1b4
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Headless (`kimi -p`) failures could exit with code 0 when the event loop drained during the shutdown cleanup (e.g. telemetry's unref'd retry backoff when the network is blocked), because the rejection never reached the process.exit(1) call. Set the failure exit code before any await in both the run-prompt catch and the main catch, and keep the cleanup timeout ref'd so the loop stays alive long enough for the rejection to propagate.
1 participant
Related Issue
No linked issue. Found while investigating two failed benchmark runs of
kimi -pthat exited cleanly but scored 0.Problem
Headless (
kimi -p) runs could exit with code 0 when a turn fails, instead of code 1. To a caller (e.g. the benchmark harness) this looks like a successful run, so the failure goes unnoticed.Root cause. When a turn fails, the error has to traverse an async chain to reach the
process.exit(1)call:Node only keeps the process alive while the event loop has a ref'd handle; a pending promise does not count. The cleanup's
awaits (notably telemetry's unref'd retry backoff when the network is blocked) can leave the loop with no ref'd handle, so Node drains and exits with the default code 0 beforeprocess.exit(1)ever runs.There are two distinct drain points:
runPrompt'sfinally { await cleanupPromptRun() }drains. The error never propagates out of thefinally, somain's.catch()never runs..catch(), but.catch()'sawait logStartupFailure(...)drains beforeprocess.exit(1).This was nondeterministic: it depended on whether a ref'd handle happened to survive until the chain completed (e.g. whether the telemetry endpoint was reachable).
What changed
src/cli/run-prompt.ts— makeraceWithTimeout's cleanup timeout ref'd. This keeps the event loop alive during cleanup (bounded bytimeoutMs), so Scenario B can't drain mid-cleanup; the rejection propagates tomain's.catch()normally.src/main.ts— setprocess.exitCode = 1synchronously at the start of.catch(), before anyawait. Covers Scenario C: even if the loop drains duringawait logStartupFailure(...), the drain-exit reports failure.test/cli/main.test.ts— assert the failure exit code is set before awaiting startup-failure logging.An earlier iteration also set
process.exitCode = 1in acatchinsiderunPrompt, but that mutated globalprocess.exitCodefor any direct caller (including the existingrun-prompt.test.tsrejection cases). Since the ref'd cleanup timeout already prevents Scenario B, that belt-and-suspenders write was redundant and was removed.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.