Preserve actionable initialize errors after CLI process exit#702
Preserve actionable initialize errors after CLI process exit#702shuofengzhang wants to merge 2 commits intoanthropics:mainfrom
Conversation
16e3002 to
ec7291b
Compare
|
@claude review |
| if isinstance(e, ProcessError) and self._last_execution_error: | ||
| pending_error = Exception(self._last_execution_error) |
There was a problem hiding this comment.
🔴 ProcessError is downgraded to a bare Exception on line 262: pending_error = Exception(self._last_execution_error). This means callers using the documented except ProcessError as e: pattern (README line 263) will not catch this error, and the structured exit_code/stderr attributes are lost. Should be ProcessError(self._last_execution_error, exit_code=e.exit_code, stderr=e.stderr).
Extended reasoning...
What the bug is
On line 262 of query.py, when a ProcessError is caught and _last_execution_error is set, the code creates a new bare Exception to replace it:
if isinstance(e, ProcessError) and self._last_execution_error:
pending_error = Exception(self._last_execution_error)This discards the ProcessError type and its structured attributes (exit_code, stderr), replacing them with a plain Exception carrying only the error message string.
How it propagates
The pending_error is stored in pending_control_results[request_id] (line 267). When _send_control_request() retrieves this result, it checks isinstance(result, Exception) and raises it (lines 434-435). This propagates through initialize() and ultimately to the caller of query(). The caller receives a bare Exception instead of the expected ProcessError.
Why existing code doesn't prevent it
The test added by this PR (test_initialize_uses_error_during_execution_result_text) catches with except Exception as e:, which catches all exception types indiscriminately. If the test used except ProcessError as e: — the pattern documented in the README — it would fail, exposing the type downgrade.
Step-by-step proof
- CLI emits a
resultmessage withsubtype="error_during_execution"containing"No conversation found with session ID ab2c985b" _read_messages()stores this inself._last_execution_error- CLI then exits non-zero, causing
transport.read_messages()to raiseProcessError("Command failed with exit code 1", exit_code=1, stderr="Check stderr output for details") - The
except Exception as e:block catches thisProcessError - Since
isinstance(e, ProcessError)is True and_last_execution_erroris set, line 262 createspending_error = Exception("No conversation found with session ID ab2c985b")— a bareException - This bare
Exceptionis stored inpending_control_resultsand later raised by_send_control_request() - A caller doing
except ProcessError as e:(as documented in README line 263) will NOT catch this — it falls through as an unhandledException - Even if caught with a broader handler,
e.exit_codeande.stderrare unavailable since plainExceptionhas no such attributes
Impact
This breaks the public API contract for error handling. ProcessError is part of the SDK's public API (exported in __init__.py, documented in the README). Any SDK consumer following the recommended error handling pattern will experience unhandled exceptions in the specific scenario this PR aims to improve.
Fix
Replace line 262 with:
pending_error = ProcessError(self._last_execution_error, exit_code=e.exit_code, stderr=e.stderr)This preserves the actionable error message (the PR's goal) while maintaining the correct exception type and structured attributes. The test should also be updated to catch ProcessError specifically to prevent future regressions.
What changed
resultmessages withsubtype="error_during_execution"inQuery._read_messagesand store the CLI-provided error text.ProcessError, propagate the captured CLI error text to pending control requests (such asinitialize) instead of the generic process-exit placeholder.TestInitializeErrorPropagation) that reproduces the failure mode and verifies the propagated exception contains the real CLI error text.Why
error_during_executionresult on the message stream and then exit non-zero.ProcessError("Check stderr output for details") that overwrote pending control requests, so callers waiting oninitialize()lost the actionable root-cause message.Insight / Why this matters
The core issue is a cross-channel ordering bug:
resultmessage (error_during_execution) on the message stream.initialize()is waiting on the control-response channel, not message iteration.ProcessError, and that generic error replaced the pending control result.This is easy to miss because both error signals exist, but only one is observed by initialize callers. Preserving the CLI-emitted execution error keeps diagnostics aligned with actual root cause.
Practical gain / Why this matters
Testing
pytest -q tests/test_query.py -k "initialize_uses_error_during_execution_result_text"pytest -qRisk analysis
ProcessErrorfollows a knownerror_during_executionresult.