Repository navigation
[3328 by Claude Opus 5.5] Honor nonzero Claude exits before accepting streamed success - #3329
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…ed even if it streamed a successful result line first. The regression fails on current main and passes with the fix, and all 14 related suites pass. Root cause: `parseStreamJsonOutput` set success from the exit code, but `processResultLine` then overwrote it with `!is_error`. `processDockerResult` passed that along unchanged, so the PR follow-up finished as "completed" with no commit, and the retry never ran. Fix - `packages/core/src/claude/claudeHelpers.ts`: after parsing, a known nonzero exit forces `success = false`. The result text, conversation log, token usage, session id and cost are all still kept. A new helper, `hasKnownNonzeroExit`, treats a missing (null) exit code as unknown, so those runs behave exactly as before. - `packages/core/src/agents/impl/utils/dockerResultProcessor.ts`: - When this case happens, the error is now `Claude process exited with code <N> after reporting a successful result`, followed by stderr if there is any. Without this, the agent's success summary would have been shown as the error. - The summary is still kept as the summary. - Timeout, cost-cap, watchdog and runtime-crash runs keep their existing termination reason and error message. - To stay under CI's zero-warning lint on changed core files, I moved the termination/error logic into a helper, `resolveExecutionOutcome`. The function was already over the complexity limit (26 against a max of 20) before my change. The behaviour for every other case is the same as before. How the failure reaches the retry: `success: false` with no termination reason makes `getPostExecutionDisposition` return `failed`. The PR job then throws `Agent execution failed: …`, which is the normal failed-attempt path that the automatic retry already handles. I didn't change attempt counts, backoff, cancellation, permissions or the UI. Regression test: `test/claudeNonzeroExitResult.test.ts` (8 tests, using the real parser and result processor) - Exits 137, 143 and 1 with a successful result line: the parser and the processor both report failure, the diagnostics are kept, and the PR disposition is `failed`. - The error message includes stderr when there is some. - Timeout plus exit 143 is still `timeout` / `partial`, and cost cap plus 137 is still `cost_cap`. - Exit 0 with a successful result still succeeds (`complete`); an error result fails with exit 0 or 137; a null exit keeps whatever the result line says. Results - Against current main (the two source files swapped back temporarily): 4 of 8 tests fail. The 137, 143 and 1 cases fail on `true !== false`, and the stderr case gets an `undefined` error. The four control tests pass. - With the fix, through `scripts/run-test-suite.mjs`, all 14 files pass: | Suite | Passed | | --- | ---: | | claudeNonzeroExitResult | 8/8 | | claude | 8/8 | | claudeAnalysisOutcome | 5/5 | | claudeContinuedAnswer | 8/8 | | claudeExtensionEnvironment | 1/1 | | claudeNativeGoal | 20/20 | | claudeRateLimitPropagation | 1/1 | | partialExecution | 19/19 | | prCommentAutomaticRetry | 17/17 | | prCommentTaskStateFinalizers | 8/8 | | retryHandler | 121/121 | | core sqliteRetry | 46/46 | | agentToolPolicy | 22/22 | | issueUsageLimitRetry | 16/16 | - `eslint --max-warnings 0` on the two changed core files and `tsc --noEmit` for `packages/core` are both clean. There's no visible change, so I didn't create any previews.
|
/review llm-codex-astra Independent read-only review of exact head 3bae49f for #3328. Root live validation observed a real Claude container exit137 while its streamed result made the worker record success/completed without a commit or automatic retry. Root also reproduced known nonzero exits being overwritten in the real compiled parser and result processor. Assess correctness of the known-nonzero exit boundary through the PR failure/retry disposition; preservation of terminal output/usage/summary diagnostics; unchanged valid zero-exit last-result ordering; explicit timeout, cost-cap, watchdog/runtime-crash and rate-limit policies; and useful error propagation. Check regression coverage and scope of the outcome-helper extraction. Do not change code, permissions, credentials, retry budgets or release artifacts. Root will independently validate this head and repeat the controlled live retry after integration. |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex-astraOverall EvaluationMerge-ready within the supplied scope, conditional on pending checks passing. The fix enforces process failure after stream parsing and keeps the outcome-helper extraction focused. ✅ Nonzero exits remain failures — The final parser guard prevents streamed success from overriding a known nonzero exit. The result adapter preserves failure, and ordinary failures reach the PR post-processing throw that enables queue retry. ✅ Diagnostics remain intact — Terminal results, usage, session information, conversation history, and summaries survive the override. The processor supplies an exit-specific error and includes available stderr. ✅ Existing policies remain explicit — Zero-exit result ordering is unchanged. Timeout, cost-cap, watchdog, guarded runtime-crash recovery, and rate-limit exception handling retain their existing control paths. This is a static review of the supplied diff and context; no commands or tests were run. The linked issue description is excerpted, so unseen requirements were not assessed. The supplied current checks show no failures and five pending checks. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Cover result ordering and policy combinationsExtend S2: 🟢 Confirm actual queue retryThe new regression reaches ScoreThe localized fix addresses the reported failure while preserving diagnostics and existing termination policies. Regression coverage is meaningful; pending checks and the planned live retry remain to complete validation. Score: 9/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
AI Implementation Summary
Closes #3328
Branch:
3328/claude-opus-5-5-honor-nonzero-claude-exit-20261023-0453-q8mCommits: ✅ Changes committed (3bae49f)
AI Processing Completed
Execution Details:
Summary:
A Claude run that exits nonzero (137, 143, 1) is now reported as failed even if it streamed a successful result line first. The regression fails on current main and passes with the fix, and all 14 related suites pass.
Root cause:
parseStreamJsonOutputset success from the exit code, butprocessResultLinethen overwrote it with!is_error.processDockerResultpassed that along unchanged, so the PR follow-up finished as "completed" with no commit, and the retry never ran.Fix
packages/core/src/claude/claudeHelpers.ts: after parsing, a known nonzero exit forcessuccess = false. The result text, conversation log, token usage, session id and cost are all still kept. A new helper,hasKnownNonzeroExit, treats a missing (null) exit code as unknown, so those runs behave exactly as before.packages/core/src/agents/impl/utils/dockerResultProcessor.ts:Claude process exited with code <N> after reporting a successful result, followed by stderr if there is any. Without this, the agent's success summary would have been shown as the error.resolveExecutionOutcome. The function was already over the complexity limit (26 against a max of 20) before my change. The behaviour for every other case is the same as before.How the failure reaches the retry:
success: falsewith no termination reason makesgetPostExecutionDispositionreturnfailed. The PR job then throwsAgent execution failed: …, which is the normal failed-attempt path that the automatic retry already handles. I didn't change attempt counts, backoff, cancellation, permissions or the UI.Regression test:
test/claudeNonzeroExitResult.test.ts(8 tests, using the real parser and result processor)failed.timeout/partial, and cost cap plus 137 is stillcost_cap.complete); an error result fails with exit 0 or 137; a null exit keeps whatever the result line says.Results
true !== false, and the stderr case gets anundefinederror. The four control tests pass.scripts/run-test-suite.mjs, all 14 files pass:eslint --max-warnings 0on the two changed core files andtsc --noEmitforpackages/coreare both clean.There's no visible change, so I didn't create any previews.
Detailed Logs:
0de238cb-463c-4a7b-9097-3068b89cec30Log files stored at:
/usr/src/app/logs/executions/issue-3328-2026-10-23T05-00-00-629Z-conversation.json/usr/src/app/logs/executions/issue-3328-2026-10-23T05-00-00-629Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3328.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!