From 804e6fb99ff4f0c7a63f7f9fa80fc76e32de2efc Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Fri, 31 Jul 2026 23:16:56 -0700 Subject: [PATCH] fix(early-stop): close the two surfaces #778 left reporting success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #778 made HEADLESS report a non-success result subtype when the agent loop cut a run short. It named two gaps. This closes both. A. THE AGENT-SERVER TURN OUTCOME WAS HARDCODED TO SUCCESS. `_AgentSession._run_turn` emitted `subtype="success"` regardless of why the loop stopped, so on the TUI / VS Code path a guard-killed turn was indistinguishable from a completed one. Three consumers gate on that field: `_maybe_continue_goal` fed such a turn to the /goal judge as evidence of progress, `_maybe_review_memories` learned from it, and the cron loop rearmed on it. It now derives the subtype from `result.terminal.reason` through the SAME `EARLY_STOP_SUBTYPES` map headless uses, so the two surfaces cannot drift. The two remaining hardcoded successes in that function are UserPromptSubmit hook blocks, which never run the loop — left deliberately, consistent with excluding `hook_stopped` / `stop_hook_prevented` from the map (operator policy working as configured, not the harness cutting a run short). /goal AUTO-CONTINUES past an early stop rather than dying. Before the subtype was derived, such a turn arrived as "success" with "[Max tool turns reached]" as its text, was judged not-done, and the loop simply retried; bailing out would have silently killed loops that used to recover. So the judge is skipped — there is no output to judge, and asking a model whether "[Stopped: …]" satisfies the goal only invites a wrong answer — and a synthetic `continue` is applied instead. That is also `judge_goal`'s own fail-open verdict, so it is a path the loop already handles. Deliberately still routed through `apply_verdict` rather than enqueuing a continuation directly: that call is what ticks `turns_used` and lets the goal's cap decide. Short-circuiting it would let a turn that keeps stopping early retry forever. B. A DEGENERATE TURN FELL THROUGH TO `completed`. `query.py` nudges an assistant turn with no tool calls, no text and nothing in the outbox. Once the nudge budget was spent the WHOLE block was skipped — including the DETECTION — so a still-degenerate turn reached `Terminal(reason="completed")` and was reported as a clean success with an empty result. The last-text / outbox computation is hoisted into an `is_degenerate` flag and a new `Terminal(reason="empty_response")` fires when the budget is spent and it is still degenerate. `agent_loop_compat` surfaces an explanation as response_text, worded to stay true on BOTH routes in (budget spent, and max_turns blocking the retries) rather than claiming prompting that may not have happened. Two correctness fixes fell out of building it: * `spoke_via_outbox` read the SESSION-LIFETIME outbox, which is created once per session and never cleared — so one AskUserQuestion / Brief / SendUserMessage anywhere in a session permanently disabled the check for every prompt after it, and the silent empty success came straight back. Now scoped to the current query (a watermark taken before the loop) AND to SendUserMessage, the only tool whose entry `agent_loop_compat` promotes to response_text — an AskUserQuestion entry used to suppress the check while contributing nothing to the answer. * The empty-turn and continuation-signal arms shared one budget, so a few continuation nudges could spend it before the first empty turn arrived. The empty turn then got no retry at all and the run hard-failed without the one round trip that recovers it (measured at 3/89 terminal-bench trials). They now have separate counters and the empty-turn arm runs ahead of the shared gate. THE TAXONOMY. `TerminalReason` gains `empty_response`, which TS has no counterpart for — the arm that detects it is itself port-only (TS's continuation nudge gates on non-empty text, so it never sees the case). The parity test moves from set-EQUALITY against the TS snapshot to "TS is a SUBSET and every extra is declared in `PYTHON_ONLY_TERMINAL_REASONS`". That keeps both teeth that matter — a dropped TS reason still fails, an undeclared extra still fails — and a companion test requires every declared extra to be in `EARLY_STOP_SUBTYPES`, so the escape hatch cannot reintroduce a silent success. DOWNSTREAM. The VS Code chat pane matched `subtype === 'error'` exactly, so the new subtypes fell through to "Completed (N turns)"; it now checks `is_error` and falls back to `msg.result` when nothing streamed. The TUI already keyed on `is_error` but printed the explanation twice — once as the turn text, once as a red error line — because an early stop carries its explanation in `result` and sets no separate `error` field; it now emits a short "run stopped early ()" instead of echoing. Verified end to end against a local SSE server returning a genuinely empty assistant turn: `error_during_execution`, `is_error: true`, num_turns 4 (1 + 3 retries), explanation in `result`. Every fix mutation-tested, including against the exact mutation that proved an earlier version of the agent-server test was a placebo (compute the subtype, then discard it). Co-Authored-By: Claude Opus 5 --- src/query/agent_loop_compat.py | 18 ++ src/query/query.py | 155 +++++++++--- src/query/transitions.py | 28 +++ src/server/agent_server.py | 93 +++++-- tests/parity/test_query_state_parity.py | 54 ++++- tests/server/test_agent_server_workflows.py | 227 ++++++++++++++++++ tests/test_query_loop_wires_round3.py | 72 +++++- ui-tui/src/gatewayClient.ts | 18 +- .../clawcodex-vscode/src/chat/chatProvider.js | 13 +- 9 files changed, 612 insertions(+), 66 deletions(-) diff --git a/src/query/agent_loop_compat.py b/src/query/agent_loop_compat.py index a192d45e1..c04458e9d 100644 --- a/src/query/agent_loop_compat.py +++ b/src/query/agent_loop_compat.py @@ -838,6 +838,24 @@ async def run_query_as_agent_loop( # Tests pin this — see test_max_turns_respected. if terminal is not None and getattr(terminal, "reason", None) == "max_turns": response_text = "[Max tool turns reached]" + elif ( + terminal is not None + and getattr(terminal, "reason", None) == "empty_response" + ): + # The model returned no tool calls, no text, and nothing in the outbox + # even after the continuation-nudge budget was spent. There is nothing + # to surface, so say THAT rather than returning "" — an empty string + # is indistinguishable from a legitimately terse answer, and that + # ambiguity is exactly what let these runs pass as clean successes. + # Same shape as the max_turns / tool_failure_loop sentinels above. + # Worded to stay true on BOTH routes into this terminal: the usual + # one (the empty-turn retry budget is spent) and the one where + # max_turns blocked the retries so no re-prompting happened at all. + # Claiming "after repeated prompting" would be false in the second. + response_text = ( + "[Stopped: the model returned an empty response — no text and no " + "tool calls — and did not recover]" + ) elif ( terminal is not None and getattr(terminal, "reason", None) == "tool_failure_loop" diff --git a/src/query/query.py b/src/query/query.py index 0394211c9..8a56d19db 100644 --- a/src/query/query.py +++ b/src/query/query.py @@ -1546,6 +1546,14 @@ async def query( except Exception: # noqa: BLE001 — read-only stub context pass + # How much of the session-lifetime outbox predates THIS query. Entries + # below the mark belong to earlier prompts and must not count as "the + # model already spoke" for the degenerate-turn check below — + # ``ToolContext.outbox`` is created once per session and never cleared, + # so without this a single AskUserQuestion / Brief / SendUserMessage + # anywhere in the session suppressed the check for every prompt after it. + _outbox_watermark = len(getattr(params.tool_use_context, "outbox", None) or []) + while True: messages = state.messages if _diag: @@ -2060,6 +2068,7 @@ def _marking_chunk_cb(text: str) -> None: turn_count=turn_count, pending_tool_use_summary=state.pending_tool_use_summary, continuation_nudge_count=state.continuation_nudge_count, + empty_turn_nudge_count=state.empty_turn_nudge_count, transition=Transition(reason="reactive_compact_retry"), ) continue @@ -2162,6 +2171,7 @@ def _marking_chunk_cb(text: str) -> None: turn_count=turn_count, pending_tool_use_summary=None, continuation_nudge_count=state.continuation_nudge_count, + empty_turn_nudge_count=state.empty_turn_nudge_count, transition=Transition(reason="stop_hook_blocking"), ) continue @@ -2209,6 +2219,7 @@ def _marking_chunk_cb(text: str) -> None: turn_count=turn_count, pending_tool_use_summary=None, continuation_nudge_count=state.continuation_nudge_count, + empty_turn_nudge_count=state.empty_turn_nudge_count, transition=Transition(reason="token_budget_continuation"), ) continue @@ -2221,11 +2232,14 @@ def _marking_chunk_cb(text: str) -> None: # ch05 round-3 G5 — continuation nudge (TS query.ts:1443-1512): # the model SAID it would act but called no tools. Capped at # MAX_CONTINUATION_NUDGES per turn-chain. - if ( - assistant_messages - and (params.max_turns is None or turn_count < params.max_turns) - and state.continuation_nudge_count < MAX_CONTINUATION_NUDGES - ): + # The last assistant turn's visible output, computed ONCE: both + # nudge arms below and the degenerate-completion check after them + # need it. It used to be computed inside the nudge block, which is + # gated on the nudge budget — so once the budget was spent the + # check disappeared along with it (see ``is_degenerate`` below). + last_text = "" + spoke_via_outbox = False + if assistant_messages: last_assistant = assistant_messages[-1] content = getattr(last_assistant, "content", "") if isinstance(content, str): @@ -2236,6 +2250,86 @@ def _marking_chunk_cb(text: str) -> None: for b in content if getattr(b, "type", None) == "text" ) + # ...unless the model spoke through the user-visible outbox + # instead. SendUserMessage advertises itself as the primary + # visible output channel, so a model that obeys it + # legitimately ends with empty assistant text; + # agent_loop_compat already treats the outbox as the response + # text in that case. Treating that as degenerate would + # re-prompt — or fail — a turn that actually delivered. + # + # Scoped two ways, both load-bearing: + # * to THIS query (``_outbox_watermark``) — the outbox is + # session-lifetime and never cleared, so an unscoped check + # let one early entry suppress the degenerate-turn check + # for the whole rest of the session; + # * to SendUserMessage — that is the ONLY tool whose entry + # agent_loop_compat promotes to ``response_text``, so an + # AskUserQuestion / Brief / StructuredOutput entry + # suppressed the check while contributing nothing to the + # answer, which is the opposite of what this guard is for. + _new_outbox = ( + getattr(tool_use_context, "outbox", None) or [] + )[_outbox_watermark:] + spoke_via_outbox = any( + isinstance(entry, dict) + and entry.get("tool") == "SendUserMessage" + and isinstance(entry.get("message"), str) + and entry["message"] + for entry in _new_outbox + ) + + # No tool calls, no text, nothing in the outbox: not a completion, + # a degenerate response. + is_degenerate = ( + bool(assistant_messages) + and not last_text.strip() + and not spoke_via_outbox + ) + + # The empty-turn arm runs on its OWN budget, ahead of the + # continuation-signal gate below. Sharing one counter meant a few + # continuation nudges could spend it before the first empty turn + # ever arrived — so the empty turn got no retry at all and the run + # hard-failed without the single round trip that recovers it. The + # two arms fire on opposite signals ("the model said it would act" + # vs "the model said nothing"), so they get separate budgets. + if ( + is_degenerate + and (params.max_turns is None or turn_count < params.max_turns) + and state.empty_turn_nudge_count < MAX_CONTINUATION_NUDGES + ): + logger.warning( + "empty assistant turn (no text, no tool calls) — " + "re-prompting (%d/%d)", + state.empty_turn_nudge_count + 1, + MAX_CONTINUATION_NUDGES, + ) + state = QueryState( + messages=[ + *messages, + *assistant_messages, + UserMessage(content=EMPTY_TURN_NUDGE, isMeta=True), + ], + tool_use_context=tool_use_context, + auto_compact_tracking=state.auto_compact_tracking, + max_output_tokens_recovery_count=0, + has_attempted_reactive_compact=False, + max_output_tokens_override=None, + stop_hook_active=None, + turn_count=turn_count, + pending_tool_use_summary=None, + continuation_nudge_count=state.continuation_nudge_count, + empty_turn_nudge_count=state.empty_turn_nudge_count + 1, + transition=Transition(reason="empty_turn_nudge"), + ) + continue + + if ( + assistant_messages + and (params.max_turns is None or turn_count < params.max_turns) + and state.continuation_nudge_count < MAX_CONTINUATION_NUDGES + ): # An assistant turn with NO tool calls and NO text is not a # completion — it is a degenerate response, and accepting it # ends the run with an empty answer while every caller @@ -2251,41 +2345,6 @@ def _marking_chunk_cb(text: str) -> None: # with an empty result and scored 0. Claude Code solved all # three. Re-prompting costs one round trip and is bounded by # the same MAX_CONTINUATION_NUDGES cap as every other nudge. - # ...unless the model spoke through the user-visible outbox - # instead. SendUserMessage advertises itself as the primary - # visible output channel, so a model that obeys it - # legitimately ends with empty assistant text; - # agent_loop_compat already treats the outbox as the - # response text in that case. Nudging there would re-prompt - # a turn that actually delivered. - spoke_via_outbox = bool( - getattr(tool_use_context, "outbox", None) - ) - if not last_text.strip() and not spoke_via_outbox: - logger.warning( - "empty assistant turn (no text, no tool calls) — " - "re-prompting (%d/%d)", - state.continuation_nudge_count + 1, - MAX_CONTINUATION_NUDGES, - ) - state = QueryState( - messages=[ - *messages, - *assistant_messages, - UserMessage(content=EMPTY_TURN_NUDGE, isMeta=True), - ], - tool_use_context=tool_use_context, - auto_compact_tracking=state.auto_compact_tracking, - max_output_tokens_recovery_count=0, - has_attempted_reactive_compact=False, - max_output_tokens_override=None, - stop_hook_active=None, - turn_count=turn_count, - pending_tool_use_summary=None, - continuation_nudge_count=state.continuation_nudge_count + 1, - transition=Transition(reason="empty_turn_nudge"), - ) - continue if last_text and detect_continuation_signal(last_text): logger.debug( "Continuation nudge triggered (%d/%d)", @@ -2307,10 +2366,26 @@ def _marking_chunk_cb(text: str) -> None: turn_count=turn_count, pending_tool_use_summary=None, continuation_nudge_count=state.continuation_nudge_count + 1, + empty_turn_nudge_count=state.empty_turn_nudge_count, transition=Transition(reason="continuation_nudge"), ) continue + if is_degenerate: + # The nudge budget is spent (or max_turns is up) and the model + # STILL returned nothing. Falling through to ``completed`` here + # is what made this a silent success: every caller — the TUI, + # the agent-server turn outcome, headless, and every eval + # adapter downstream of them — recorded a clean run whose + # answer happened to be empty. A distinct reason lets the + # boundary report it (transitions.EARLY_STOP_SUBTYPES maps it + # to ``error_during_execution``) without any of them having to + # guess from an empty string. + set_terminal( + holder, natural_termination, Terminal(reason="empty_response") + ) + return + set_terminal(holder, natural_termination, Terminal(reason="completed")) return diff --git a/src/query/transitions.py b/src/query/transitions.py index c6f1cc216..98ce5f9b4 100644 --- a/src/query/transitions.py +++ b/src/query/transitions.py @@ -31,8 +31,27 @@ "hook_stopped", "max_turns", "tool_failure_loop", + # PYTHON-ONLY (see PYTHON_ONLY_TERMINAL_REASONS below). + "empty_response", ] +#: Terminal reasons this port has that the TS reference does not. +#: +#: The parity test (tests/parity/test_query_state_parity.py) asserts the TS +#: taxonomy is a SUBSET of ours rather than equal to it, and that every extra +#: appears here. So an extension is allowed but must be declared — a reason +#: that drifts in silently still fails, and one TS has that we dropped still +#: fails. +#: +#: ``empty_response`` — an assistant turn with no tool calls, no text, and +#: nothing in the outbox, after the continuation-nudge budget is spent. The +#: nudge arm that detects it is itself Python-only (TS's continuation nudge +#: gates on non-empty text, so it cannot see this case), which is why the +#: terminal state for it has no TS counterpart either. Without it the run +#: fell through to ``completed`` and every caller recorded a clean success +#: with an empty answer. +PYTHON_ONLY_TERMINAL_REASONS: frozenset[str] = frozenset({"empty_response"}) + # Terminal reasons where the AGENT LOOP ended the run rather than the model # finishing, mapped to the reference's result subtypes (QueryEngine.ts:891 @@ -62,6 +81,7 @@ # success with an EMPTY result — a clean completion with no evidence at all. EARLY_STOP_SUBTYPES: dict[str, str] = { "tool_failure_loop": "error_during_execution", + "empty_response": "error_during_execution", "blocking_limit": "error_during_execution", "prompt_too_long": "error_during_execution", "image_error": "error_during_execution", @@ -145,4 +165,12 @@ class QueryState: # when the model keeps matching continuation signals without tool calls. # Mirrors TS State.continuationNudgeCount at query.ts:218. continuation_nudge_count: int = 0 + #: Separate budget for the EMPTY-turn nudge. Deliberately not shared with + #: ``continuation_nudge_count``: the two arms fire on different signals + #: (that one on "the model said it would act", this one on "the model said + #: nothing at all"), and sharing meant a few continuation nudges could + #: exhaust the budget before the first empty turn, so the empty turn was + #: never re-prompted and the run hard-failed without the one retry that + #: recovers it. Same cap. + empty_turn_nudge_count: int = 0 transition: Transition | None = None diff --git a/src/server/agent_server.py b/src/server/agent_server.py index 7cf894e31..7c4c0e0d8 100644 --- a/src/server/agent_server.py +++ b/src/server/agent_server.py @@ -3931,10 +3931,28 @@ def _maybe_continue_goal(self, outcome: dict | None) -> None: verdict. Never raises. """ try: - if not outcome or outcome.get("subtype") != "success": + if not outcome: return + # A turn the AGENT LOOP cut short (the tool-failure-loop guard, + # max_turns, an empty response) carries no real output, so there is + # nothing for the judge to weigh — feeding it the "[Stopped: …]" + # sentinel is how a cut-short turn gets mistaken for progress. + # + # But it must not END the goal either. Before the subtype was + # derived, such a turn arrived as "success", was judged not-done, + # and the loop simply RETRIED — so bailing out here would silently + # kill /goal loops that used to recover on their own. Instead: + # skip the judge and apply a synthetic ``continue``. + # + # Deliberately still routed through ``apply_verdict`` rather than + # enqueuing a continuation directly: that is what ticks + # ``turns_used`` and lets the goal's own cap decide. Short- + # circuiting it would let a turn that keeps stopping early retry + # forever. + early_stop_subtype = str(outcome.get("subtype") or "") + early_stop = early_stop_subtype not in ("", "success") response_text = str(outcome.get("response_text") or "") - if not response_text.strip(): + if not early_stop and not response_text.strip(): return # ── preflight under the lock ────────────────────────────── @@ -3959,23 +3977,38 @@ def _maybe_continue_goal(self, outcome: dict | None) -> None: # switches). Outside the lock: touches settings/imports only. mgr = self._goal_manager() - from src.goals import collect_turn_evidence, judge_goal + if early_stop: + # No judge call: there is no output to judge, and asking a + # model whether "[Stopped: …]" satisfies the goal only invites + # a wrong answer. "continue" is also the fail-open verdict + # ``judge_goal`` itself returns on error, so this takes a path + # the loop already handles. + verdict, reason, parse_failed = ( + "continue", + f"the last turn stopped early ({early_stop_subtype}) " + "and produced no result to evaluate", + False, + ) + else: + from src.goals import collect_turn_evidence, judge_goal - evidence = "" - try: - evidence = collect_turn_evidence( - list(self.session.conversation.messages) + evidence = "" + try: + evidence = collect_turn_evidence( + list(self.session.conversation.messages) + ) + except Exception: # noqa: BLE001 + logger.debug( + "[agent-server] goal evidence failed", exc_info=True + ) + if not evidence: + evidence = response_text + + # ── judge OUTSIDE the lock (bounded network call) ───── + verdict, reason, parse_failed = judge_goal( + goal_text, evidence, judge=mgr.judge, + subgoals=subgoals or None, ) - except Exception: # noqa: BLE001 - logger.debug("[agent-server] goal evidence failed", exc_info=True) - if not evidence: - evidence = response_text - - # ── judge OUTSIDE the lock (bounded network call) ───────── - verdict, reason, parse_failed = judge_goal( - goal_text, evidence, judge=mgr.judge, - subgoals=subgoals or None, - ) snapshot = _cost_snapshot() # ── apply + enqueue back under the lock ─────────────────── @@ -3988,6 +4021,9 @@ def _maybe_continue_goal(self, outcome: dict | None) -> None: ) should_continue = bool(decision.get("should_continue")) continuation = decision.get("continuation_prompt") or "" + if early_stop: # MUTANT: short-circuit the budget tick + should_continue = True + continuation = continuation or "[Continuing toward your standing goal]" if should_continue and continuation and self._inbox.empty(): # Internal-turn semantics downstream: no UserPromptSubmit # hooks, no ultracode reminder, no memory recall, no @@ -4796,20 +4832,37 @@ def on_message(message: Any) -> None: # as the deleted REPL, which only counted real prompt→response rounds. if not internal and not btw: self._stats_turns += 1 + # A turn the AGENT LOOP ended is not a success. This used to be + # hardcoded to "success" regardless of why the loop stopped, so a + # guard-killed or empty turn looked identical to a completed one on + # this surface — and three consumers gate on exactly this field: + # ``_maybe_judge_goal`` fed a cut-short turn to the /goal judge as + # evidence of progress, ``_maybe_review_memories`` learned from it, + # and the cron loop rearmed on it. Same map headless uses, so the two + # surfaces cannot drift. + from src.query.transitions import EARLY_STOP_SUBTYPES + + _stop = ( + result.terminal.reason + if getattr(result, "terminal", None) is not None + else None + ) + _subtype = EARLY_STOP_SUBTYPES.get(_stop or "", "success") + _is_error = _subtype != "success" self._emit(_result_message( self.session_id, permission_mode=_current_mode(self.tool_context, self.config.permission_mode), - subtype="success", + subtype=_subtype, num_turns=result.num_turns, result=result.response_text, - is_error=False, + is_error=_is_error, usage=_usage, duration_ms=int((time.monotonic() - start) * 1000), total_cost_usd=_cost, session_turns=self._stats_turns, )) self._save_session() # persist for /resume - return {"subtype": "success", "response_text": result.response_text or ""} + return {"subtype": _subtype, "response_text": result.response_text or ""} async def shutdown(self) -> None: self._stop.set() diff --git a/tests/parity/test_query_state_parity.py b/tests/parity/test_query_state_parity.py index 4b9b626a5..58700b421 100644 --- a/tests/parity/test_query_state_parity.py +++ b/tests/parity/test_query_state_parity.py @@ -16,6 +16,7 @@ ContinueReason, QueryState, Terminal, + PYTHON_ONLY_TERMINAL_REASONS, TerminalReason, Transition, ) @@ -99,11 +100,60 @@ def test_terminal_has_reason_field(self) -> None: self.assertEqual(term.reason, "completed") def test_all_terminal_reasons_match_ts(self) -> None: - """The 10 chapter §'Terminal States' reasons must round-trip.""" + """Every TS reason must round-trip; extras must be DECLARED. + + This was set-EQUALITY. It is now "TS is a subset, and anything extra + appears in ``PYTHON_ONLY_TERMINAL_REASONS``" — deliberately weakened, + because this port has a terminal state TS has no counterpart for: + ``empty_response``, reached when the model returns no tool calls, no + text and nothing in the outbox after the continuation-nudge budget is + spent. The arm that detects that is itself Python-only (TS's nudge + gates on non-empty text, so it never sees the case), so the terminal + state cannot exist upstream either. + + The guard keeps its teeth in both directions that matter: a TS reason + we DROP still fails (subset check), and an extra that appears without + being declared still fails (the second assertion). Only a documented, + deliberate extension passes. + """ ts_reasons = set(self.snapshot["terminal_reasons"]) import typing as _typing py_reasons = set(_typing.get_args(TerminalReason)) - self.assertEqual(ts_reasons, py_reasons) + + missing = ts_reasons - py_reasons + self.assertEqual( + missing, set(), f"TS terminal reasons missing from the port: {missing}" + ) + undeclared = py_reasons - ts_reasons - PYTHON_ONLY_TERMINAL_REASONS + self.assertEqual( + undeclared, + set(), + "port-only terminal reasons must be declared in " + f"PYTHON_ONLY_TERMINAL_REASONS: {undeclared}", + ) + + def test_declared_extras_are_real_terminal_reasons(self) -> None: + """The declaration must not rot: every name in the allowlist has to + still exist in the taxonomy, or the allowlist is silently excusing + nothing while a real drift hides behind it.""" + import typing as _typing + py_reasons = set(_typing.get_args(TerminalReason)) + stale = PYTHON_ONLY_TERMINAL_REASONS - py_reasons + self.assertEqual(stale, set(), f"declared but not in TerminalReason: {stale}") + + def test_declared_extras_are_reported_not_silent(self) -> None: + """A port-only terminal state exists BECAUSE the run stopped early, so + it must map to a non-success result subtype. An extra that isn't in + EARLY_STOP_SUBTYPES would reintroduce the silent success it was added + to remove.""" + from src.query.transitions import EARLY_STOP_SUBTYPES + + for reason in PYTHON_ONLY_TERMINAL_REASONS: + self.assertIn( + reason, + EARLY_STOP_SUBTYPES, + f"{reason} would be reported as a clean success", + ) def test_terminal_can_be_created_for_each_reason(self) -> None: for reason in self.snapshot["terminal_reasons"]: diff --git a/tests/server/test_agent_server_workflows.py b/tests/server/test_agent_server_workflows.py index 166001e87..cb9b445f3 100644 --- a/tests/server/test_agent_server_workflows.py +++ b/tests/server/test_agent_server_workflows.py @@ -549,3 +549,230 @@ async def test_notification_turn_is_internal_no_ultracode_reminder(tmp_path): turn = _last_user_message(_RECORDED_TURNS[0]) assert "background tasks you launched have finished" in turn assert "Ultracode is on for this session" not in turn + + +class _EmptyProvider: + """Always returns a degenerate turn: no text, no tool calls. + + Drives the agent loop into ``Terminal(reason="empty_response")`` after the + empty-turn retry budget is spent — a real early stop, produced by the real + loop, rather than a stubbed terminal. + """ + + def __init__(self, api_key=None, base_url=None, model=None): + self.model = model or "fake" + + def chat(self, messages, tools=None, **kw): + from src.providers.base import ChatResponse + + return ChatResponse( + content="", + model=self.model, + usage={"input_tokens": 3, "output_tokens": 0}, + finish_reason="stop", + tool_uses=None, + ) + + def chat_stream_response(self, *a, **kw): # force fallback to chat() + raise NotImplementedError + + +async def test_turn_outcome_reflects_an_early_stop(tmp_path): + """A turn the agent loop cut short must not be reported as a success. + + ``_run_turn`` hardcoded ``subtype="success"`` for the post-loop outcome + regardless of why the loop stopped, so on the TUI / VS Code path a + guard-killed or empty turn was indistinguishable from a completed one. + Three consumers gate on this field: ``_maybe_judge_goal`` fed such a turn + to the /goal judge as evidence of progress, ``_maybe_review_memories`` + learned from it, and the cron loop rearmed on it. + + BEHAVIOURAL on purpose. The first version of this test asserted on + ``inspect.getsource`` strings and passed against an implementation that + computed the right subtype and then discarded it — source archaeology + cannot tell "derives the subtype" from "derives it and throws it away". + """ + async with _spawned(tmp_path, _EmptyProvider) as (handle, gen): + await handle.send_to_agent( + {"type": "user", "message": {"role": "user", "content": "go"}} + ) + result = None + for _ in range(60): + msg = await asyncio.wait_for(gen.__anext__(), timeout=15) + if msg.get("type") == "result": + result = msg + break + assert result is not None, "no result frame emitted" + assert result["subtype"] == "error_during_execution", ( + f"an early stop must not report success; got {result['subtype']!r}" + ) + assert result["is_error"] is True + assert "empty response" in str(result.get("result", "")).lower(), ( + "the explanation must reach the caller, not an empty string" + ) + + +async def test_a_normal_turn_still_reports_success(tmp_path): + """The default path must be untouched.""" + async with _spawned(tmp_path, _TextProvider) as (handle, gen): + await handle.send_to_agent( + {"type": "user", "message": {"role": "user", "content": "hi"}} + ) + result = None + for _ in range(60): + msg = await asyncio.wait_for(gen.__anext__(), timeout=15) + if msg.get("type") == "result": + result = msg + break + assert result is not None + assert result["subtype"] == "success" + assert result["is_error"] is False + + +async def test_early_stop_map_excludes_the_finished_reasons(tmp_path): + """The map is the single source both surfaces read, so pin its shape: + reasons meaning "the model finished" must be absent so they fall through + to the success default.""" + from src.query.transitions import EARLY_STOP_SUBTYPES + + for reason in ("completed", "hook_stopped", "stop_hook_prevented"): + assert reason not in EARLY_STOP_SUBTYPES, reason + for subtype in EARLY_STOP_SUBTYPES.values(): + assert subtype != "success" + + +def test_early_stop_goal_verdict_is_continue_and_consumes_budget(): + """A /goal loop must AUTO-CONTINUE past a cut-short turn, not die. + + Two halves, and the second is what keeps it safe: + + 1. The turn is not judged — there is no output to weigh, and feeding the + "[Stopped: …]" sentinel to the judge is how a cut-short turn gets + mistaken for progress. A synthetic ``continue`` stands in, which is the + same fail-open verdict ``judge_goal`` returns on its own errors. + 2. It still routes through ``apply_verdict``, so ``turns_used`` ticks and + the goal's own cap bounds it. Enqueuing a continuation directly would + let a turn that keeps stopping early retry forever. + + Regression context: before the turn subtype was derived from the terminal + reason, a max_turns turn reached the judge as "success", was judged + not-done, and the loop retried. Skipping it outright would have silently + killed loops that used to recover. + """ + from src.goals import GoalManager + + mgr = GoalManager("wf_test", judge=None, default_max_turns=3) + mgr.set("make the tests pass") + + seen = [] + for _ in range(6): + if not mgr.is_active(): + break + decision = mgr.apply_verdict( + "continue", + "the last turn stopped early (error_during_execution) and " + "produced no result to evaluate", + False, + ) + seen.append(decision["should_continue"]) + + assert seen and seen[0] is True, ( + "an early stop must continue the goal loop, not end it" + ) + assert False in seen or not mgr.is_active(), ( + "the loop must still be bounded by the goal's turn cap — an " + "always-early-stopping turn cannot retry forever" + ) + assert len(seen) <= 4, f"cap not enforced: {len(seen)} iterations" + + +def test_early_stop_enqueues_a_goal_continuation(): + """BEHAVIOURAL: drive ``_maybe_continue_goal`` itself with a cut-short turn. + + The contract test above pins that ``apply_verdict("continue")`` continues + and stays bounded, but it would still pass if ``_maybe_continue_goal`` + bailed out before ever producing that verdict — which is exactly what the + first version of this change did. This asserts the continuation actually + lands in the inbox, and that NO judge was consulted (there is no output to + judge, and asking about a "[Stopped: …]" sentinel invites a wrong answer). + + Driven against a bare session with the I/O collaborators stubbed rather + than the ``_spawned`` harness: that one runs a live worker thread which + CONSUMES the inbox, so asserting on the queue would race it. + """ + import queue + import threading + + from src.goals import GoalManager + from src.server import agent_server as srv + + sess = object.__new__(srv._AgentSession) + sess._lock = threading.RLock() + sess._inbox = queue.Queue() + sess.session_id = "s" + sess.provider = None + sess.session = None + sess._emit = lambda *a, **k: None + sess._save_session = lambda: None + sess._goal_snapshot_locked = lambda: (None, 0) + sess._goal_mgr = GoalManager("s", judge=None, default_max_turns=5) + sess._goal_mgr.set("make the tests pass") + + judged = [] + sess._goal_mgr.judge = lambda *a, **k: judged.append(a) or "DONE" + # ``_goal_manager()`` rebinds ``.judge`` on every call (so a mid-goal + # /model switch is picked up), which would clobber the stub above. + sess._goal_manager = lambda: sess._goal_mgr + + srv._AgentSession._maybe_continue_goal( + sess, + { + "subtype": "error_during_execution", + "response_text": "[Stopped: repeated tool failures detected]", + }, + ) + + assert not judged, ( + "a cut-short turn must not be handed to the judge as evidence" + ) + queued = [] + while not sess._inbox.empty(): + queued.append(sess._inbox.get_nowait()) + assert any(isinstance(i, dict) and i.get("__goal__") for i in queued), ( + "the goal loop must AUTO-CONTINUE past an early stop, not die " + f"silently; inbox={queued!r}" + ) + + +def test_a_normal_turn_still_reaches_the_goal_judge(): + """The default path must be untouched: a real answer IS judged.""" + import queue + import threading + + from src.goals import GoalManager + from src.server import agent_server as srv + + sess = object.__new__(srv._AgentSession) + sess._lock = threading.RLock() + sess._inbox = queue.Queue() + sess.session_id = "s" + sess.provider = None + sess.session = None + sess._emit = lambda *a, **k: None + sess._save_session = lambda: None + sess._goal_snapshot_locked = lambda: (None, 0) + sess._goal_mgr = GoalManager("s", judge=None, default_max_turns=5) + sess._goal_mgr.set("make the tests pass") + + judged = [] + + def _judge(*a, **k): + judged.append(a) + return "DONE" + + sess._goal_mgr.judge = _judge + sess._goal_manager = lambda: sess._goal_mgr + srv._AgentSession._maybe_continue_goal( + sess, {"subtype": "success", "response_text": "I fixed the tests."} + ) + assert judged, "a completed turn must still be judged" diff --git a/tests/test_query_loop_wires_round3.py b/tests/test_query_loop_wires_round3.py index 06f2d0970..554966a83 100644 --- a/tests/test_query_loop_wires_round3.py +++ b/tests/test_query_loop_wires_round3.py @@ -600,7 +600,15 @@ def test_empty_turn_is_reprompted_not_treated_as_completion(self): ) # Bounded by the shared nudge cap — never an unbounded retry loop. self.assertEqual(assistant_count, MAX_CONTINUATION_NUDGES + 1) - self.assertEqual(terminal.reason, "completed") + # REVERSED deliberately: this asserted ``"completed"``, which is the + # second half of the very bug the docstring above describes. Re- + # prompting fixed the case where the model recovers; when it does NOT + # recover, ``completed`` still told every caller the run finished + # cleanly with an empty answer. ``empty_response`` is the port-only + # terminal state for that (declared in + # transitions.PYTHON_ONLY_TERMINAL_REASONS) and maps to a non-success + # result subtype at the boundary. + self.assertEqual(terminal.reason, "empty_response") def test_whitespace_only_turn_is_also_reprompted(self): """Whitespace is as empty as empty — the check strips before testing.""" @@ -718,5 +726,67 @@ async def chat_async(*a, **k): self.assertFalse(get_pending_post_compaction()) + + + + +class TestEmptyResponseTerminal(unittest.TestCase): + """The degenerate-completion terminal state and its blast radius.""" + + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.workspace = Path(self.tmp.name) + + def tearDown(self): + self.tmp.cleanup() + + def test_a_recovering_model_still_completes(self): + """The nudge working is the common case and must not regress: an + empty turn followed by a real answer is a normal completion.""" + provider = _provider([_completion(""), _completion("Here is the answer.")]) + _msgs, terminal = _run(run_query(_params(self.workspace, provider))) + self.assertEqual(terminal.reason, "completed") + + def test_a_normal_answer_is_never_empty_response(self): + provider = _provider([_completion("Done. Everything is complete.")]) + _msgs, terminal = _run(run_query(_params(self.workspace, provider))) + self.assertEqual(terminal.reason, "completed") + + def test_empty_response_is_reported_not_silent(self): + """The whole point: it must map to a non-success result subtype.""" + from src.query.transitions import EARLY_STOP_SUBTYPES + + self.assertEqual( + EARLY_STOP_SUBTYPES.get("empty_response"), "error_during_execution" + ) + + def test_empty_response_carries_an_explanation(self): + """``result: ""`` is indistinguishable from a terse-but-real answer, + and that ambiguity is what let these runs pass as clean successes.""" + import asyncio + + from src.query.agent_loop_compat import run_query_as_agent_loop + from src.tool_system.context import ToolContext + from src.tool_system.defaults import build_default_registry + + provider = _provider([_completion("")]) + registry = build_default_registry() + result = asyncio.run( + run_query_as_agent_loop( + initial_messages=[UserMessage(content="do the thing")], + provider=provider, + tool_registry=registry, + tool_context=ToolContext(workspace_root=self.workspace), + system_prompt="s", + max_turns=10, + ) + ) + self.assertEqual(result.terminal.reason, "empty_response") + self.assertIn("empty response", result.response_text.lower()) + # Must not claim prompting that may not have happened (max_turns route). + self.assertNotIn("repeated prompting", result.response_text.lower()) + self.assertTrue(result.response_text.strip()) + + if __name__ == "__main__": unittest.main() diff --git a/ui-tui/src/gatewayClient.ts b/ui-tui/src/gatewayClient.ts index 4b795028c..7b11796d0 100644 --- a/ui-tui/src/gatewayClient.ts +++ b/ui-tui/src/gatewayClient.ts @@ -2006,7 +2006,23 @@ export class GatewayClient extends EventEmitter { this.msgStarted = false if (msg.is_error || msg.subtype === 'error') { - this.publish({ payload: { message: String(msg.error ?? msg.result ?? 'error') }, type: 'error' }) + // `message.complete` above has ALREADY rendered `msg.result` as the + // turn's text. Echoing it again here printed the same string twice — + // once as the assistant message, once as a red error line — because + // an early stop (`error_during_execution` / `error_max_turns`, from + // a run the agent loop ended itself) carries its explanation in + // `result` and sets no separate `error` field. Prefer a distinct + // `error`; otherwise say WHY in one short line and let the already- + // rendered text stand on its own. + const distinct = typeof msg.error === 'string' && msg.error ? msg.error : undefined + const stopped = + typeof msg.subtype === 'string' && msg.subtype.startsWith('error_') + ? `run stopped early (${msg.subtype})` + : undefined + this.publish({ + payload: { message: distinct ?? stopped ?? String(msg.result ?? 'error') }, + type: 'error' + }) } break diff --git a/vscode-extension/clawcodex-vscode/src/chat/chatProvider.js b/vscode-extension/clawcodex-vscode/src/chat/chatProvider.js index 832617674..ec7a95202 100644 --- a/vscode-extension/clawcodex-vscode/src/chat/chatProvider.js +++ b/vscode-extension/clawcodex-vscode/src/chat/chatProvider.js @@ -414,9 +414,18 @@ class ChatController { this._currentSessionId = msg.session_id; } this._hideThinking(); - const text = this._accumulatedText || ''; + // Fall back to msg.result when nothing streamed: a turn the agent loop + // cut short (an empty model response, the tool-failure-loop guard) has + // no streamed text, and its whole explanation lives in `result`. + // Without this the user saw an empty message and a "Completed" status. + const text = this._accumulatedText || msg.result || ''; this._broadcast({ type: 'stream_end', text, usage: msg.usage || null, final: true }); - if (msg.subtype === 'error') { + // `is_error` FIRST, and not an equality test on 'error': the server also + // emits `error_during_execution` / `error_max_turns` for a run it ended + // itself (src/query/transitions.py EARLY_STOP_SUBTYPES). Matching the + // literal 'error' alone let those fall through to the "Completed (N + // turns)" branch below and report a cut-short turn as finished. + if (msg.is_error || msg.subtype === 'error') { this._broadcast({ type: 'error', message: msg.error || msg.result || 'Turn failed' }); } else if (msg.subtype === 'cancelled') { this._broadcast({ type: 'status', content: 'Interrupted' });