diff --git a/backend/apps/agents/core/error_classify.py b/backend/apps/agents/core/error_classify.py index 0d9cb883..8b78a394 100644 --- a/backend/apps/agents/core/error_classify.py +++ b/backend/apps/agents/core/error_classify.py @@ -61,6 +61,20 @@ P_SUBSCRIPTION_STATE_PATTERNS = re.compile( AUTH_RESUME_WAIT_CAP = 120 +# A codex/GPT subscription token rotates every 1-2 minutes; anything shorter than the window just +# retries into the same expiry. +CODEX_ROTATION_RESUME_WAIT = 75 + +P_CODEX_ROTATION_PATTERNS = re.compile( + r"(?:\[?codex/|\bcx/|\bgpt-[0-9])" + r".*?" + r"(?:authentication\s+token\s+(?:is|has)\s+expired|token\s+expired|\b401\b)" + r"|(?:authentication\s+token\s+(?:is|has)\s+expired|token\s+expired|\b401\b)" + r".*?" + r"(?:\[?codex/|\bcx/|\bgpt-[0-9])", + re.IGNORECASE | re.DOTALL, +) + @typechecked def auth_resume_wait(exc: BaseException, attempt: int, extra_text: str = "") -> Optional[int]: @@ -95,6 +109,12 @@ def auth_resume_wait(exc: BaseException, attempt: int, extra_text: str = "") -> hinted = parse_retry_after(exc, extra_text) if hinted is not None: return min(hinted + 5, AUTH_RESUME_WAIT_CAP) + # Codex tokens rotate on a 1-2 minute cadence, so a 20s resume lands back INSIDE the same + # window and spends the one attempt on a failure that was always going to fail. This is the + # turn-level twin of the ENG-361 wait: without it that fix never gets a say, because this + # retry runs first (drill C5/D6, 2026-08-20). + if P_CODEX_ROTATION_PATTERNS.search(combined): + return CODEX_ROTATION_RESUME_WAIT return 20 diff --git a/backend/apps/agents/manager/run/empty_finish.py b/backend/apps/agents/manager/run/empty_finish.py index 521fb672..98fdb005 100644 --- a/backend/apps/agents/manager/run/empty_finish.py +++ b/backend/apps/agents/manager/run/empty_finish.py @@ -47,12 +47,22 @@ def maybe_nudge_empty_finish(session: AgentSession, session_id: str) -> bool: if getattr(session, "pending_continuation", False): return False if not turn_finished_empty(session): + # A turn that produced NOTHING at all (no text, no tool call) leaves the same Done pill + # over an empty chat, and the tail-walk above can't see it: with nothing persisted the + # tail is still the user's own message. Nudging it would re-send a prompt the model just + # refused, so say so instead of ending mute. + if p_turn_produced_nothing(session): + p_surface_exhausted(session, session_id) return False if session.empty_finish_nudges >= NUDGE_HARD_CAP: p_surface_exhausted(session, session_id) return False p_tool_calls = p_count_tool_calls(session) if session.empty_finish_nudges >= 1 and p_tool_calls <= session.empty_finish_progress_mark: + # The nudge bought no new work, so re-nudging would ping-pong a model with nothing left. + # Refusing is right; ending the ask in SILENCE is not, and that is what the user actually + # reports as "the agent just stopped" (Haik's poke storms). + p_surface_exhausted(session, session_id) return False session.empty_finish_progress_mark = p_tool_calls # At high context the silent quit is usually the model choking on the prompt itself, so @@ -126,6 +136,20 @@ def p_surface_exhausted(session: AgentSession, session_id: str) -> None: P_ANSWER_TOOL_MARKERS = ("openswarm-ui", "ShowUI", "AskUI", "AskUserQuestion") +@typechecked +def p_turn_produced_nothing(session: AgentSession) -> bool: + """True when the model returned an empty hand: no reply, no tool work, nothing the user can + read. Thinking does not count, because a collapsed reasoning trace is not an answer, and a + thinking-only end_turn is the exact shape of the quits users report.""" + msgs: List = [ + m for m in get_branch_messages(session) + if not getattr(m, "hidden", False) and getattr(m, "role", "") != "thinking" + ] + if not msgs or getattr(msgs[-1], "role", "") != "user": + return False + return not any(getattr(m, "role", "") in ("assistant", "tool_call") for m in msgs) + + @typechecked def p_count_tool_calls(session: AgentSession) -> int: return sum(1 for m in get_branch_messages(session) if getattr(m, "role", "") == "tool_call") diff --git a/backend/tests/test_assistant_message.py b/backend/tests/test_assistant_message.py index d0cac41f..cf12b8af 100644 --- a/backend/tests/test_assistant_message.py +++ b/backend/tests/test_assistant_message.py @@ -46,7 +46,14 @@ async def test_first_token_expiry_heals_silently(monkeypatch): with patch.object(assistant_message.ws_manager, "send_to_session", new=fake_send): await assistant_message.handle_assistant_message( p_asst([TextBlock(text=txt)]), session, session.id, turn, thinking, {}, {}) - assert not any(m.role == "system" for m in session.messages), "no banner on the first expiry" + # ENG-361 amended this contract: the codex retry now waits ~75s to clear the rotation window, + # and a minute of nothing reads as a hang, so ONE slim "retrying automatically" notice is + # expected. What must never appear is the reconnect BANNER, because there is nothing for the + # user to do. The distinction is the whole point: a notice informs, a banner assigns homework. + p_sys = [m for m in session.messages if m.role == "system"] + assert len(p_sys) == 1, "exactly one slim notice, not a wall" + assert "no action needed" in p_sys[0].content.lower() + assert "reconnect" not in p_sys[0].content.lower(), "never demand a reconnect on the first expiry" assert not any(m.role == "assistant" for m in session.messages) assert "agent:auth_error" not in events assert session.auth_retry_used is True diff --git a/backend/tests/test_auth_resume.py b/backend/tests/test_auth_resume.py index 77066c13..6aff8970 100644 --- a/backend/tests/test_auth_resume.py +++ b/backend/tests/test_auth_resume.py @@ -91,3 +91,28 @@ def test_the_recovery_ledger_counts_auth_resumes(): src = inspect.getsource(TurnRunner) assert "auth-resume" in src.split("record_recovery", 1)[0] or "p_auth_retry_attempt" in src.split("record_recovery", 1)[1].split(")", 2)[1], \ "a survived auth blip must land in the near-miss ledger" + + +def test_codex_rotation_resume_waits_past_the_rotation_window(): + """A codex token rotates every 1-2 minutes, so the turn-level resume has to clear the window. + At 20s it retried into the same expiry and spent the single attempt for nothing, which meant + the ENG-361 self-heal downstream never got a say (drill D6/C5, 2026-08-20).""" + from backend.apps.agents.core.error_classify import CODEX_ROTATION_RESUME_WAIT + for text in ( + "[codex/gpt-5.6] API Error: 401 authentication token is expired", + "cx/gpt-5.4: token expired", + "API Error 401 on gpt-5.6-terra: authentication token has expired", + ): + assert auth_resume_wait(Exception(text), 0) == CODEX_ROTATION_RESUME_WAIT, text + assert CODEX_ROTATION_RESUME_WAIT > 60, "must outlast the rotation window" + + +def test_non_codex_auth_failures_keep_the_short_resume(): + """Negative control: only the rotating lane pays the long wait; a plain bad key must still + come back fast, or every Anthropic 401 gets a minute of dead air.""" + for text in ( + "API Error: 401 authentication_error invalid x-api-key", + "Request failed: 401 Unauthorized", + "Invalid bearer token", + ): + assert auth_resume_wait(Exception(text), 0) == 20, text diff --git a/backend/tests/test_empty_finish.py b/backend/tests/test_empty_finish.py index aa90bf9d..43d6b6ab 100644 --- a/backend/tests/test_empty_finish.py +++ b/backend/tests/test_empty_finish.py @@ -215,3 +215,73 @@ def test_vanishing_quit_on_repeat_session_is_claimed(): assert turn_finished_empty(s) is False, "first-time session: user tail stays unclaimed" s.empty_finish_total = 1 assert turn_finished_empty(s) is True, "repeat session: the vanishing quit is claimed" + + +def p_system_lines(session) -> list: + return [m for m in session.messages if m.role == "system"] + + +def test_stalled_continuation_still_tells_the_user() -> None: + """The anti-ping-pong guard is right to refuse a second nudge, but the ask must not end in + silence: that Done-pill-over-tool-rows is exactly what users report as 'it just stopped' + (drill D3/C4, 2026-08-20).""" + s = p_session(("user", "audit the repo"), + ("tool_call", {"tool": "Bash", "input": {}}), + ("tool_result", {"text": "ok"})) + assert maybe_nudge_empty_finish(s, "sid") is True + s.pending_continuation = False + # The nudge bought nothing at all. + assert maybe_nudge_empty_finish(s, "sid") is False + assert s.empty_finish_nudges == 1, "still no second nudge" + assert len(p_system_lines(s)) == 1, "but the user is told once" + + # And it stays once, however many stalled quits follow. + s.pending_continuation = False + assert maybe_nudge_empty_finish(s, "sid") is False + assert len(p_system_lines(s)) == 1 + + +def test_turn_that_produced_nothing_is_not_silent() -> None: + """No text, no tool call, nothing persisted: the tail-walk cannot see it, so the honest line + is the only thing standing between the user and an empty Done pill (drill D4/C3/C5).""" + s = p_session(("user", "read all 16 files and report the magic word")) + assert maybe_nudge_empty_finish(s, "sid") is False, "nudging would re-send a refused prompt" + assert len(p_system_lines(s)) == 1, "the user gets an honest line instead of silence" + + +def test_a_working_turn_is_never_given_the_exhausted_line() -> None: + """Negative control: a turn that actually answered must stay clean.""" + s = p_session(("user", "hi"), ("assistant", "here is your answer")) + assert maybe_nudge_empty_finish(s, "sid") is False + assert p_system_lines(s) == [], "an answered turn earns no card" + + +def test_a_turn_still_holding_tool_work_is_not_called_empty_handed() -> None: + """Negative control for the produced-nothing seal: tool work exists, so this is the ordinary + nudge path, not the empty-handed one.""" + s = p_session(("user", "task"), + ("tool_call", {"tool": "Read", "input": {}}), + ("tool_result", {"text": "x"})) + assert maybe_nudge_empty_finish(s, "sid") is True, "ordinary silent quit still nudges" + assert p_system_lines(s) == [], "and says nothing yet, because work may still land" + + +def test_the_honest_lines_survive_the_frontends_jargon_filter() -> None: + """The seal only works if the user can SEE the line. MessageBubble.tsx deliberately swallows + raw subprocess/API dumps rendered as system messages, so an honest note that happens to match + those patterns would be added by the backend and then silently dropped by the UI: the exact + silence this whole issue is about, just moved one layer up. Kept here rather than in a .tsx + test so it lives beside the text it guards and cannot drift from it.""" + import re + from backend.apps.agents.manager.run.empty_finish import EXHAUSTED_NOTE + + # Mirrors the swallow test in frontend/src/app/pages/AgentChat/bubbles/MessageBubble.tsx. + p_swallowed = re.compile( + r'Command failed with exit code|API Error:|invalid_request_error' + r'|"type"\s*:\s*"error"|Check stderr output', + re.IGNORECASE, + ) + assert not p_swallowed.search(EXHAUSTED_NOTE), ( + "the exhausted note would be swallowed by the UI's dev-jargon filter" + ) + assert EXHAUSTED_NOTE.strip(), "an empty note renders as nothing at all" diff --git a/backend/tests/test_handle_run_error.py b/backend/tests/test_handle_run_error.py index 1e2bd695..01e06fd5 100644 --- a/backend/tests/test_handle_run_error.py +++ b/backend/tests/test_handle_run_error.py @@ -10,7 +10,7 @@ from backend.apps.agents.manager.run.handle_run_error import handle_run_error from backend.apps.agents.manager.streaming.state import TurnState -def p_drive_error(monkeypatch, exc, stderr=None): +def p_drive_error(monkeypatch, exc, stderr=None, session=None): events = [] async def fake_send(session_id, event, data): @@ -20,7 +20,10 @@ def p_drive_error(monkeypatch, exc, stderr=None): # Diagnostics are fire-and-forget network; keep the tests offline. import backend.apps.service.client as service_client monkeypatch.setattr(service_client, "submit_diagnostic", lambda payload: None, raising=True) - session = AgentSession(name="t", model="sonnet", dashboard_id="d") + # Passing a session back in drives a SECOND failure on the same ask, which is the only way to + # test a once-per-ask budget: a fresh session would silently hand it a fresh budget too. + if session is None: + session = AgentSession(name="t", model="sonnet", dashboard_id="d") asyncio.run(handle_run_error(exc, session, session.id, TurnState(), stderr or [])) return session, events @@ -113,9 +116,29 @@ def test_out_of_credits_does_not_respawn_the_cli(monkeypatch): assert session.needs_fresh_session is False -def test_auth_failure_does_not_respawn_the_cli(monkeypatch): +def test_auth_failure_self_heals_once_then_stops_respawning(monkeypatch): + # ENG-361 amended the older "never respawn on auth" rule: every sub lane now gets ONE silent + # self-heal before any card, and a fresh CLI is exactly how the stale token gets dropped. The + # rule that still matters is that it happens ONCE; a credential that fails twice is genuinely + # dead, and respawning forever would just hide it behind an endless retry. session, _ = p_drive_error(monkeypatch, Exception("401 invalid authentication credentials")) + assert session.needs_fresh_session is True, "one rebuild is the heal" + assert session.auth_retry_used is True + assert not [m for m in session.messages if m.role == "system"], "no card on the first expiry" + + session.needs_fresh_session = False + session.pending_continuation = False + p_drive_error(monkeypatch, Exception("401 invalid authentication credentials"), session=session) + assert session.needs_fresh_session is False, "the budget is spent; stop respawning" + assert [m for m in session.messages if m.role == "system"], "the second failure is honest" + + +def test_missing_credential_never_burns_a_retry(monkeypatch): + # Negative control: a config problem retries identically, so it must card immediately instead + # of spending a rebuild and a wait on a request that cannot succeed. + session, _ = p_drive_error(monkeypatch, Exception("No credentials for provider: claude (401)")) assert session.needs_fresh_session is False + assert [m for m in session.messages if m.role == "system"], "straight to the honest card" def test_rate_limit_does_not_respawn_the_cli(monkeypatch): diff --git a/backend/tests/test_streaming_harness.py b/backend/tests/test_streaming_harness.py index 8133f8a4..e3eb115b 100644 --- a/backend/tests/test_streaming_harness.py +++ b/backend/tests/test_streaming_harness.py @@ -5,6 +5,7 @@ coverage), so it pins the observable contract: streamed text lands as an assista tool calls are recorded, and the turn completes.""" import asyncio +import os import pytest import claude_agent_sdk @@ -181,6 +182,14 @@ def test_loop_builds_direct_anthropic_key_env(monkeypatch): import backend.apps.agents.agent_manager as am import backend.apps.agents.manager.run.RunOptions as run_opts + # Supply the packaged shape this strip exists for: a bundle site-packages entry that must be + # dropped, next to the debugger dir that must survive. + monkeypatch.setenv( + "PYTHONPATH", + os.pathsep.join(["/Applications/OpenSwarm.app/Contents/Resources/python-env/lib/site-packages", + "/opt/openswarm/debugger"]), + ) + settings = AppSettings(anthropic_api_key="sk-ant-test123", connection_mode="own_key") monkeypatch.setattr(am, "load_settings", lambda: settings, raising=True) monkeypatch.setattr(run_opts, "load_settings", lambda: settings, raising=True) @@ -208,6 +217,13 @@ def test_loop_builds_direct_anthropic_key_env(monkeypatch): asyncio.run(mgr.run_agent_loop(session.id, "hi")) env = captured["options"].env + # PYTHONPATH rides along stripped of site-packages (ENG-347: the bundle shadowed agent venvs). + # The loop only forwards a PYTHONPATH the ambient shell actually set, so the fixture below + # supplies one; without it this asserted whatever the developer's terminal happened to export + # and failed on any machine that exported nothing. + p_pp = env.pop("PYTHONPATH", None) + assert p_pp is not None and "site-packages" not in p_pp + assert "debugger" in p_pp, "the debugger injection dir is the one entry that survives" # Direct key, no 9router proxy; the CLI's own auto-memory is force-disabled on every spawn (ENG-222). assert env == {"ANTHROPIC_API_KEY": "sk-ant-test123", "CLAUDE_CODE_DISABLE_AUTO_MEMORY": "1"} diff --git a/frontend/src/app/pages/AgentChat/bubbles/MessageBubble.tsx b/frontend/src/app/pages/AgentChat/bubbles/MessageBubble.tsx index 8b44d022..58f6df89 100644 --- a/frontend/src/app/pages/AgentChat/bubbles/MessageBubble.tsx +++ b/frontend/src/app/pages/AgentChat/bubbles/MessageBubble.tsx @@ -887,6 +887,7 @@ const MessageBubble: React.FC = React.memo((props) => { const { body: sysBody, note: sysNote } = extractPlatformNote(rawSysText); const sysText = sysNote || sysBody; // A raw subprocess/API failure ("Command failed with exit code 1", API Error JSON) is dev jargon, and the same failure is already shown as a friendly card on the assistant side. Swallow just that stderr dump so the user sees one calm card, not jargon beneath it. + // Widen this at your peril: the silent-quit seal's honest lines ride the same system role, and swallowing one turns a stopped agent back into an unexplained Done pill. backend/tests/test_empty_finish.py pins that they survive. if (/Command failed with exit code|API Error:|invalid_request_error|"type"\s*:\s*"error"|Check stderr output/i.test(sysText)) { return null; }