diff --git a/src/query/agent_loop_compat.py b/src/query/agent_loop_compat.py index a192d45e..c04458e9 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 0394211c..8a56d19d 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 c6f1cc21..98ce5f9b 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 7cf894e3..7c4c0e0d 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 4b9b626a..58700b42 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 166001e8..cb9b445f 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 06f2d097..554966a8 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 4b795028..7b11796d 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 83261767..ec7a9520 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' });