Skip to content

Commit 1930c5e

Browse files
qing-antclaude
andcommitted
review: replace ProcessError with result error text instead of suppressing
Mirrors the TypeScript SDK (Query.ts readMessages): when the CLI exits non-zero after emitting result(is_error=True), replace the generic "Command failed with exit code 1" with the structured error text the CLI already reported, instead of suppressing the exception entirely. This preserves the try/except contract that existing consumers may rely on (an exception is still raised), while fixing #913's "unactionable message" complaint. Suppression silently changed iteration semantics for consumers who detect failure via try/except around `query()`; replacing the message keeps that contract intact and aligns the two SDKs. Tracking flag `_got_error_result: bool` becomes `_last_error_result_text: str | None`, holding the text from `result.errors` (joined) or the subtype as fallback. Reset condition broadened from `user`-only to "any non-result, non-session_state_changed message" to match the TS SDK's `lastErrorResultText` reset. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 5808f22 commit 1930c5e

2 files changed

Lines changed: 168 additions & 73 deletions

File tree

src/claude_agent_sdk/_internal/query.py

Lines changed: 36 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,11 @@ def __init__(
129129

130130
# Track first result for proper stream closure with SDK MCP servers
131131
self._first_result_event = anyio.Event()
132-
self._got_error_result = False
132+
# Set to the result's error text when the most recent message is a
133+
# result with is_error=True. Used to replace the generic "exit code 1"
134+
# ProcessError with the structured error the CLI already reported.
135+
# Mirrors the TypeScript SDK's `lastErrorResultText` (Query.ts).
136+
self._last_error_result_text: str | None = None
133137

134138
# SessionStore mirroring (set via set_transcript_mirror_batcher)
135139
self._transcript_mirror_batcher: TranscriptMirrorBatcher | None = None
@@ -296,11 +300,22 @@ async def _read_messages(self) -> None:
296300
if self._transcript_mirror_batcher is not None:
297301
await self._transcript_mirror_batcher.flush()
298302
self._first_result_event.set()
299-
self._got_error_result = bool(message.get("is_error"))
300-
elif msg_type == "user":
301-
# New turn observed — a ProcessError now is a fresh crash,
302-
# not the expected exit from a prior error result.
303-
self._got_error_result = False
303+
if message.get("is_error"):
304+
errors = message.get("errors") or []
305+
self._last_error_result_text = "; ".join(errors) or str(
306+
message.get("subtype", "unknown error")
307+
)
308+
else:
309+
self._last_error_result_text = None
310+
elif not (
311+
msg_type == "system"
312+
and message.get("subtype") == "session_state_changed"
313+
):
314+
# Anything other than the post-turn session_state_changed
315+
# marker means the conversation moved on; a ProcessError
316+
# now is a fresh crash, not the expected exit from a prior
317+
# error result. Mirrors the TypeScript SDK's reset logic.
318+
self._last_error_result_text = None
304319

305320
# Regular SDK messages go to the stream
306321
await self._message_send.send(message)
@@ -315,20 +330,26 @@ async def _read_messages(self) -> None:
315330
if request_id not in self.pending_control_results:
316331
self.pending_control_results[request_id] = e
317332
event.set()
318-
if isinstance(e, ProcessError) and self._got_error_result:
319-
# CLI exits non-zero after emitting an error result
320-
# (error_max_turns, error_during_execution, ...). The consumer
321-
# already received the structured ResultMessage; don't follow
322-
# it with a redundant bare Exception.
333+
# When the CLI emits a result with is_error=True (e.g.
334+
# error_max_turns, error_during_execution) it then exits non-zero
335+
# on purpose, for shell-script consumers. The trailing ProcessError
336+
# carries no information beyond "exit code 1" — replace it with the
337+
# structured error the CLI already reported so the exception is
338+
# actionable. Mirrors the TypeScript SDK (Query.ts readMessages).
339+
if isinstance(e, ProcessError) and self._last_error_result_text is not None:
340+
error_text = (
341+
f"Claude Code returned an error result: "
342+
f"{self._last_error_result_text}"
343+
)
323344
logger.debug(
324-
"CLI exited with code %s after error result; "
325-
"treating as clean termination",
345+
"Replacing ProcessError (exit code %s) with result error text",
326346
e.exit_code,
327347
)
328348
else:
349+
error_text = str(e)
329350
logger.error(f"Fatal error in message reader: {e}")
330-
# Put error in stream so iterators can handle it
331-
await self._message_send.send({"type": "error", "error": str(e)})
351+
# Put error in stream so iterators can handle it
352+
await self._message_send.send({"type": "error", "error": error_text})
332353
finally:
333354
# Flush any remaining transcript mirror entries before closing so
334355
# an early stdout EOF or transport error doesn't drop entries

tests/test_query.py

Lines changed: 132 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -955,9 +955,9 @@ async def _test():
955955
class TestProcessExitAfterErrorResult:
956956
"""Regression tests for #913: when the CLI emits a result message with
957957
is_error=True (e.g. subtype=error_max_turns) and then exits non-zero,
958-
the SDK should treat that as clean termination — the consumer already
959-
received the structured ResultMessage and shouldn't see a redundant
960-
bare Exception."""
958+
the trailing ProcessError carries no information beyond "exit code 1".
959+
Replace it with the structured error text the CLI already reported so
960+
the exception is actionable. Mirrors the TypeScript SDK (Query.ts)."""
961961

962962
def _make_transport_then_raise(self, messages, exc):
963963
mock_transport = AsyncMock()
@@ -975,20 +975,31 @@ async def mock_receive():
975975
mock_transport.is_ready = Mock(return_value=True)
976976
return mock_transport
977977

978-
def test_process_error_after_error_result_is_suppressed(self):
978+
def _error_result(self, subtype="error_max_turns", errors=None, **overrides):
979+
msg = {
980+
"type": "result",
981+
"subtype": subtype,
982+
"is_error": True,
983+
"num_turns": 1,
984+
"session_id": "s",
985+
"duration_ms": 1,
986+
"duration_api_ms": 1,
987+
"total_cost_usd": 0.0,
988+
}
989+
if errors is not None:
990+
msg["errors"] = errors
991+
msg.update(overrides)
992+
return msg
993+
994+
def test_process_error_after_error_result_uses_result_error_text(self):
979995
async def _test():
980996
transport = self._make_transport_then_raise(
981997
messages=[
982-
{
983-
"type": "result",
984-
"subtype": "error_max_turns",
985-
"is_error": True,
986-
"num_turns": 60,
987-
"session_id": "s",
988-
"duration_ms": 1,
989-
"duration_api_ms": 1,
990-
"total_cost_usd": 0.0,
991-
}
998+
self._error_result(
999+
subtype="error_max_turns",
1000+
errors=["Reached maximum number of turns (60)"],
1001+
num_turns=60,
1002+
)
9921003
],
9931004
exc=ProcessError(
9941005
"Command failed with exit code 1", exit_code=1, stderr=""
@@ -998,16 +1009,71 @@ async def _test():
9981009
await q.start()
9991010

10001011
received = []
1001-
async for msg in q.receive_messages():
1002-
received.append(msg)
1012+
with pytest.raises(
1013+
Exception,
1014+
match=r"Claude Code returned an error result: "
1015+
r"Reached maximum number of turns \(60\)",
1016+
):
1017+
async for msg in q.receive_messages():
1018+
received.append(msg)
10031019
await q.close()
10041020

10051021
assert len(received) == 1
10061022
assert received[0]["subtype"] == "error_max_turns"
10071023

10081024
anyio.run(_test)
10091025

1010-
def test_process_error_without_result_still_raises(self):
1026+
def test_process_error_after_error_result_falls_back_to_subtype(self):
1027+
"""When the result has no errors[] (older CLI / minimal payload), the
1028+
improved message falls back to the subtype so it's still actionable."""
1029+
1030+
async def _test():
1031+
transport = self._make_transport_then_raise(
1032+
messages=[self._error_result(subtype="error_during_execution")],
1033+
exc=ProcessError(
1034+
"Command failed with exit code 1", exit_code=1, stderr=""
1035+
),
1036+
)
1037+
q = Query(transport=transport, is_streaming_mode=True)
1038+
await q.start()
1039+
1040+
with pytest.raises(
1041+
Exception,
1042+
match=r"Claude Code returned an error result: error_during_execution",
1043+
):
1044+
async for _ in q.receive_messages():
1045+
pass
1046+
await q.close()
1047+
1048+
anyio.run(_test)
1049+
1050+
def test_process_error_after_error_result_joins_multiple_errors(self):
1051+
async def _test():
1052+
transport = self._make_transport_then_raise(
1053+
messages=[
1054+
self._error_result(
1055+
subtype="error_during_execution",
1056+
errors=["tool timed out", "ENOENT: missing file"],
1057+
)
1058+
],
1059+
exc=ProcessError(
1060+
"Command failed with exit code 1", exit_code=1, stderr=""
1061+
),
1062+
)
1063+
q = Query(transport=transport, is_streaming_mode=True)
1064+
await q.start()
1065+
1066+
with pytest.raises(
1067+
Exception,
1068+
match=r"tool timed out; ENOENT: missing file",
1069+
):
1070+
async for _ in q.receive_messages():
1071+
pass
1072+
await q.close()
1073+
1074+
anyio.run(_test)
1075+
1076+
def test_process_error_without_result_keeps_original_message(self):
10111077
async def _test():
10121078
transport = self._make_transport_then_raise(
10131079
messages=[],
@@ -1025,7 +1091,7 @@ async def _test():
10251091

10261092
anyio.run(_test)
10271093

1028-
def test_process_error_after_success_result_still_raises(self):
1094+
def test_process_error_after_success_result_keeps_original_message(self):
10291095
async def _test():
10301096
transport = self._make_transport_then_raise(
10311097
messages=[
@@ -1058,22 +1124,13 @@ async def _test():
10581124

10591125
anyio.run(_test)
10601126

1061-
def test_process_error_after_error_then_success_result_still_raises(self):
1062-
"""The flag tracks the *most recent* result, not a sticky latch."""
1127+
def test_process_error_after_error_then_success_result_keeps_original(self):
1128+
"""Tracks the *most recent* result, not a sticky latch."""
10631129

10641130
async def _test():
10651131
transport = self._make_transport_then_raise(
10661132
messages=[
1067-
{
1068-
"type": "result",
1069-
"subtype": "error_during_execution",
1070-
"is_error": True,
1071-
"num_turns": 1,
1072-
"session_id": "s",
1073-
"duration_ms": 1,
1074-
"duration_api_ms": 1,
1075-
"total_cost_usd": 0.0,
1076-
},
1133+
self._error_result(subtype="error_during_execution"),
10771134
{
10781135
"type": "result",
10791136
"subtype": "success",
@@ -1102,23 +1159,49 @@ async def _test():
11021159

11031160
anyio.run(_test)
11041161

1105-
def test_process_error_after_error_result_then_new_turn_still_raises(self):
1106-
"""A new user turn invalidates the 'expecting imminent exit' state from
1107-
a prior turn's error result; a crash mid-new-turn must propagate."""
1162+
def test_session_state_changed_after_error_result_preserves_replacement(self):
1163+
"""The CLI emits a post-turn `system: session_state_changed(idle)`
1164+
marker after the result and before exit. It must not reset the
1165+
tracking flag — the conversation hasn't moved on."""
11081166

11091167
async def _test():
11101168
transport = self._make_transport_then_raise(
11111169
messages=[
1170+
self._error_result(
1171+
subtype="error_max_turns",
1172+
errors=["Reached maximum number of turns (10)"],
1173+
),
11121174
{
1113-
"type": "result",
1114-
"subtype": "error_during_execution",
1115-
"is_error": True,
1116-
"num_turns": 1,
1175+
"type": "system",
1176+
"subtype": "session_state_changed",
1177+
"state": "idle",
11171178
"session_id": "s",
1118-
"duration_ms": 1,
1119-
"duration_api_ms": 1,
1120-
"total_cost_usd": 0.0,
11211179
},
1180+
],
1181+
exc=ProcessError(
1182+
"Command failed with exit code 1", exit_code=1, stderr=""
1183+
),
1184+
)
1185+
q = Query(transport=transport, is_streaming_mode=True)
1186+
await q.start()
1187+
1188+
with pytest.raises(
1189+
Exception, match=r"Claude Code returned an error result"
1190+
):
1191+
async for _ in q.receive_messages():
1192+
pass
1193+
await q.close()
1194+
1195+
anyio.run(_test)
1196+
1197+
def test_new_turn_after_error_result_keeps_original_message(self):
1198+
"""A new user turn invalidates the 'expecting imminent exit' state from
1199+
a prior turn's error result; a crash mid-new-turn must surface as-is."""
1200+
1201+
async def _test():
1202+
transport = self._make_transport_then_raise(
1203+
messages=[
1204+
self._error_result(subtype="error_during_execution"),
11221205
{
11231206
"type": "user",
11241207
"message": {"role": "user", "content": "next turn"},
@@ -1142,25 +1225,13 @@ async def _test():
11421225

11431226
anyio.run(_test)
11441227

1145-
def test_pending_control_requests_fail_fast_on_suppressed_exit(self):
1146-
"""Even when the ProcessError is suppressed for the message stream,
1147-
in-flight control requests must still fail fast (process is dead;
1148-
no control_response will ever arrive)."""
1228+
def test_pending_control_requests_fail_fast_on_replaced_error(self):
1229+
"""In-flight control requests must still fail fast (process is dead;
1230+
no control_response will ever arrive) regardless of message replacement."""
11491231

11501232
async def _test():
11511233
transport = self._make_transport_then_raise(
1152-
messages=[
1153-
{
1154-
"type": "result",
1155-
"subtype": "error_max_turns",
1156-
"is_error": True,
1157-
"num_turns": 1,
1158-
"session_id": "s",
1159-
"duration_ms": 1,
1160-
"duration_api_ms": 1,
1161-
"total_cost_usd": 0.0,
1162-
}
1163-
],
1234+
messages=[self._error_result(subtype="error_max_turns")],
11641235
exc=ProcessError(
11651236
"Command failed with exit code 1", exit_code=1, stderr=""
11661237
),
@@ -1172,8 +1243,11 @@ async def _test():
11721243
q.pending_control_responses["req_1"] = event
11731244

11741245
await q.start()
1175-
async for _ in q.receive_messages():
1176-
pass
1246+
with pytest.raises(
1247+
Exception, match=r"Claude Code returned an error result"
1248+
):
1249+
async for _ in q.receive_messages():
1250+
pass
11771251
await q.close()
11781252

11791253
assert event.is_set()

0 commit comments

Comments
 (0)