From 9d6e4e72a45544f55b2db33e92ec8247950f8b7c Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 23 Sep 2026 17:23:40 +0000 Subject: [PATCH] fix(desktop): a member that fails after pre-tool text is reported, not posted as its reply Independent-review follow-up for the group room failed-turn fix. - Group room poll: a retained error newer than the pre-submit snapshot (turn start replaces it with a fresh started_at) is this turn's failure and wins over transcript text. The core closer writes no failed-turn row behind a tool row, so "said X, called a tool, provider 401" left X as the newest assistant row and the room posted it, dropped the 401 and re-drove the member to the round cap (live repro on the PR head). - Both pickers end the turn at a failed_turn row instead of scanning past it; with the retained error gone (backend restart) the row's notice is reported through the failed path instead of a silent pass / dropped stranded marker. The stranded harvest treats a retained error as the stranded turn's own the same way. - REST cold-load/paging (/api/sessions/{id}/messages, /messages/around) type legacy untyped notice rows like session.resume does, via one read-side helper in agent/turn_failure_copy.py. - Gateway closer: the fresh-session closure test now asserts the row's display_kind through a real SessionDB round-trip (red without the run_turn.py stamp). - E2E: mock trigger that says text + calls a tool, then 401s; spec asserts the room reports the error and never posts that text. --- agent/turn_failure_copy.py | 10 ++ .../e2e/group-member-backend-failure.spec.ts | 41 ++++++- .../plugins/hermes-bots/group-turns.test.ts | 101 ++++++++++++++++++ .../src/plugins/hermes-bots/group-turns.ts | 65 +++++++---- hermes_cli/web_routers/sessions.py | 6 ++ tests-js/scripts/mock-server.ts | 25 ++++- .../gateway/test_failure_writer_ownership.py | 8 +- .../test_session_message_page_owner.py | 37 +++++++ tui_gateway/session_history.py | 8 +- 9 files changed, 275 insertions(+), 26 deletions(-) diff --git a/agent/turn_failure_copy.py b/agent/turn_failure_copy.py index e33f259223..1686f28c63 100644 --- a/agent/turn_failure_copy.py +++ b/agent/turn_failure_copy.py @@ -48,6 +48,16 @@ PARTIAL_FAILED_TURN_NOTICE = ( FAILED_TURN_DISPLAY_KIND = "failed_turn" +def untyped_failed_turn_display_kind(role: Any, content: Any) -> Optional[str]: + """``FAILED_TURN_DISPLAY_KIND`` for a boundary row persisted before the closers typed it + (exact notice text, so a real reply quoting it stays a reply); read-side only.""" + if role == "assistant" and isinstance(content, str) and content.strip() in ( + FAILED_TURN_NOTICE, PARTIAL_FAILED_TURN_NOTICE, + ): + return FAILED_TURN_DISPLAY_KIND + return None + + def failed_turn_notice(turn_messages: Any) -> str: """Boundary copy for a failed turn: never claim "not processed" when a tool may have run.""" for row in turn_messages or (): diff --git a/apps/desktop/e2e/group-member-backend-failure.spec.ts b/apps/desktop/e2e/group-member-backend-failure.spec.ts index 9d03acf1ed..aa670da52d 100644 --- a/apps/desktop/e2e/group-member-backend-failure.spec.ts +++ b/apps/desktop/e2e/group-member-backend-failure.spec.ts @@ -1,4 +1,8 @@ -import { PROVIDER_FAILURE_TRIGGER } from '../../../tests-js/scripts/mock-server' +import { + PROVIDER_FAILURE_TRIGGER, + TOOL_THEN_FAILURE_TEXT, + TOOL_THEN_FAILURE_TRIGGER +} from '../../../tests-js/scripts/mock-server' import { type MockBackendFixture, setupMockBackend, waitForAppReady } from './fixtures' import { expect, test } from './test' @@ -107,3 +111,38 @@ test('a member whose backend fails the turn is reported at once, not read as bus console.log('RETAINED FAILURE: activity =', await activity.textContent(), 'room =', JSON.stringify(room)) await page.screenshot({ path: test.info().outputPath('retained-failure-after.png') }) }) + +// The member spoke and called a tool before the provider failed. The session +// ends on the tool row (no failed-turn boundary behind it) and only the +// retained 401 says the turn died: the text written before the tool call must +// not be posted into the room as the member's reply. +test('a member that fails after pre-tool text is reported, and that text is not its reply', async () => { + test.setTimeout(240_000) + const page = fixture!.page + const groupComposer = await createRoom(page) + + await groupComposer.fill(`@programmer ${TOOL_THEN_FAILURE_TRIGGER}`) + await groupComposer.press('Enter') + + const activity = page.getByRole('button', { name: /^Activity/ }) + const groupTab = page.getByRole('tab', { name: new RegExp(`${ROOM} Close`) }) + + await expect(async () => { + if ((await groupTab.getAttribute('aria-selected')) !== 'true') { + await groupTab.click() + } + + await expect(activity).toContainText('Programmer hit an error', { timeout: 5_000 }) + }).toPass({ timeout: 150_000 }) + + const room = await page.evaluate(name => { + const rooms = JSON.parse(localStorage.getItem('hermes.plugin.hermes-bots.group-chats') || '{}') + + return { log: (rooms[name]?.log || []).map((entry: { text?: string }) => entry.text || ''), stranded: Object.keys(rooms[name]?.stranded || {}) } + }, ROOM) + + expect(room.log.some((text: string) => text.includes(TOOL_THEN_FAILURE_TEXT))).toBe(false) + expect(room.stranded).toEqual([]) + console.log('TOOL THEN FAILURE: activity =', await activity.textContent(), 'log =', JSON.stringify(room.log)) + await page.screenshot({ path: test.info().outputPath('tool-then-failure-after.png') }) +}) diff --git a/apps/desktop/src/plugins/hermes-bots/group-turns.test.ts b/apps/desktop/src/plugins/hermes-bots/group-turns.test.ts index 3069a3c4cc..30e7561ef0 100644 --- a/apps/desktop/src/plugins/hermes-bots/group-turns.test.ts +++ b/apps/desktop/src/plugins/hermes-bots/group-turns.test.ts @@ -20,6 +20,10 @@ vi.mock('@hermes/plugin-sdk', async () => { return pluginSdkMock(host) }) +// agent/turn_failure_copy.py::PARTIAL_FAILED_TURN_NOTICE +const PARTIAL_NOTICE = + 'This turn did not complete. Some actions may already have run; verify their effects before resending.' + interface Room { chat: typeof groupChat gateway: ScriptedGateway @@ -368,6 +372,51 @@ describe('session-gone classification', () => { } }) + // The member spoke, called a tool, then the provider failed. The core closer + // writes no boundary behind a tool row, so the retained error is the only + // evidence; with a boundary but the retained error gone (backend restarted) + // the turn still failed. Either way the pre-tool text is not the reply. + it.each([ + ['the retained provider error', [], 'HTTP 401: invalid_api_key'], + [ + 'the failed-turn notice once the retained error is gone', + [{ content: PARTIAL_NOTICE, display_kind: 'failed_turn', role: 'assistant' }], + null + ] + ])('reports a turn that failed after pre-tool text with %s', async (_label, boundary, retained) => { + let now = 1_000_000 + const clock = vi.spyOn(Date, 'now').mockImplementation(() => (now += 60_000)) + + const room = await loadRoom({ + turn: () => [ + { content: 'Let me check the repo first.', role: 'assistant' }, + { content: 'ok', role: 'tool' }, + ...boundary + ] + }) + + const request = host.request as (method: string, params?: Record) => Promise + let submitted = false + + host.request = async (method: string, params: Record = {}) => { + const result = (await request(method, params)) as Record + + submitted = submitted || method === 'prompt.submit' + + return method === 'session.resume' && submitted && retained + ? { ...result, inflight: { error: retained, status: 'error', streaming: false } } + : result + } + + try { + await expect(room.turns.runGroupChatMemberTurn('Room', LOCAL_MEMBER, 'hi', 't1', [])).rejects.toThrow( + retained ?? PARTIAL_NOTICE + ) + } finally { + clock.mockRestore() + } + }) + // A turn that dies BEFORE its prompt is committed (agent-init failure, // no-agent refusal) leaves a retained `{ status: 'error' }` and a transcript // that never grew — the failure must still surface instead of the poll @@ -1296,6 +1345,58 @@ describe('stranded harvest', () => { ]) }) + // A late turn that spoke, called a tool and then hit a provider failure: the + // pre-tool text is not a late reply, whether the retained error or only the + // failed-turn row (retained error gone) says the turn failed. + it.each([ + ['the retained provider error', [], 'HTTP 401: invalid_api_key'], + [ + 'the failed-turn notice alone', + [{ content: PARTIAL_NOTICE, display_kind: 'failed_turn', role: 'assistant' }], + null + ] + ])('reports a late turn that failed after pre-tool text with %s', async (_label, boundary, retained) => { + const room = await loadRoom() + const activity = await import('./group-activity') + + room.chat.updateGroupChat('Broke', current => { + current.sessions = { research: 'sid-research' } + current.stranded = { research: 0 } + + return current + }) + room.gateway.sessions.set('sid-research', { + messages: [ + { content: roomPrompt('Broke'), role: 'user' }, + { content: 'Let me check the repo first.', role: 'assistant' }, + { content: 'ok', role: 'tool' }, + ...boundary + ], + profile: 'research', + runtime: 'rt-research', + stored: 'sid-research', + title: 'Group: Broke' + }) + + if (retained) { + const request = host.request as (method: string, params?: Record) => Promise + + host.request = async (method: string, params: Record = {}) => { + const result = (await request(method, params)) as Record + + return method === 'session.resume' + ? { ...result, inflight: { error: retained, status: 'error', streaming: false } } + : result + } + } + + await room.turns.harvestStrandedGroupReply('Broke', { name: 'research', title: '' }) + + expect(log(room, 'Broke')).toHaveLength(0) + expect(room.chat.$groupChats.get().Broke.stranded?.research).toBeUndefined() + expect(activity.$groupActivity.get().Broke?.events.map(event => event.kind)).toEqual(['failed']) + }) + it('never re-submits into a member the harvest just confirmed is still running', async () => { // research is confirmed busy on exactly its first two session.resume calls // — the number of harvest-only touches the FIXED code makes across two diff --git a/apps/desktop/src/plugins/hermes-bots/group-turns.ts b/apps/desktop/src/plugins/hermes-bots/group-turns.ts index 97bed58b41..6dc142ad37 100644 --- a/apps/desktop/src/plugins/hermes-bots/group-turns.ts +++ b/apps/desktop/src/plugins/hermes-bots/group-turns.ts @@ -51,6 +51,11 @@ interface GroupTurnTranscriptMessage { text?: string } +/** What a finished turn left behind: the member's reply, or the notice of the + * `failed_turn` row Hermes closed it with (the member never answered), or + * null when no assistant row landed. */ +type GroupTurnPick = { failedNotice: string } | null | string + /** #94376: pick the reply a finished turn should surface among the messages * appended since `before`. Scans newest-first and prefers the last * substantive (non-pass) assistant answer over a trailing pass — a Codex @@ -58,8 +63,10 @@ interface GroupTurnTranscriptMessage { * synthetic "(pass)" to the nudge itself, which must not hide the answer. * When only pass text exists in range, returns the newest (last * chronological) one rather than the oldest. Returns null only when no - * assistant message appears in that range. */ -function pickGroupTurnReply(messages: GroupTurnTranscriptMessage[], before: number): null | string { + * assistant message appears in that range. A failed-turn boundary ends the + * scan: text the member wrote before the tool call that preceded the + * provider failure is not its reply. */ +function pickGroupTurnReply(messages: GroupTurnTranscriptMessage[], before: number): GroupTurnPick { let passText: null | string = null for (let i = messages.length - 1; i >= before; i--) { @@ -79,7 +86,7 @@ function pickGroupTurnReply(messages: GroupTurnTranscriptMessage[], before: numb const replyText = String(text).trim() if (failedTurnBoundaryRow(msg)) { - continue + return passText ?? { failedNotice: replyText } } if (isGroupPassText(replyText)) { @@ -101,14 +108,17 @@ function pickGroupTurnReply(messages: GroupTurnTranscriptMessage[], before: numb * stopping where an outside writer takes the session over. Newest-first * (`pickGroupTurnReply`) would post a CLI answer written after the late reply * as the turn reply — and the external-write mirror posts it again. Only - * passes in range → the last pass; no anchor row → scan from `before`. */ -function pickStrandedGroupTurnReply(messages: GroupTurnTranscriptMessage[], before: number): null | string { + * passes in range → the last pass; no anchor row → scan from `before`. A + * failed-turn boundary anywhere in the turn makes it a failure, even after + * text the member wrote before its last tool call. */ +function pickStrandedGroupTurnReply(messages: GroupTurnTranscriptMessage[], before: number): GroupTurnPick { const anchor = messages.findIndex( (msg, i) => i >= before && msg?.role === 'user' && groupTranscriptRowText(msg).startsWith(GROUP_PROMPT_HEADER_PREFIX) ) let passText: null | string = null + let reply: null | string = null for (let i = anchor === -1 ? before : anchor; i < messages.length; i++) { const msg = messages[i] @@ -122,7 +132,11 @@ function pickStrandedGroupTurnReply(messages: GroupTurnTranscriptMessage[], befo break } - if (msg?.role !== 'assistant' || !text || failedTurnBoundaryRow(msg)) { + if (failedTurnBoundaryRow(msg)) { + return { failedNotice: text } + } + + if (msg?.role !== 'assistant' || !text) { continue } @@ -132,10 +146,10 @@ function pickStrandedGroupTurnReply(messages: GroupTurnTranscriptMessage[], befo continue } - return text + reply ??= text } - return passText + return reply ?? passText } /** A clarify question blocking inside a member's session, as `session.resume` @@ -1053,26 +1067,33 @@ async function pollGroupMemberTurn(context: GroupTurnPollContext): Promise before || JSON.stringify(state?.inflight) !== context.leftover) + // Turn start replaces the snapshot (with a fresh `started_at`), so a + // retained error unlike the pre-submit one is THIS turn's: the member did + // not finish, and text it wrote before a tool call is not its reply. + const failedThisTurn = failure !== null && JSON.stringify(state?.inflight) !== context.leftover + const died = failedThisTurn || (failure !== null && messages.length > before) if ((messages.length > before || died) && done) { - const replyText = messages.length > before ? pickGroupTurnReply(messages, before) : null + const pick = messages.length > before && !failedThisTurn ? pickGroupTurnReply(messages, before) : null - if (replyText !== null) { + if (typeof pick === 'string') { recordGroupActivity(context.group, { - kind: isGroupPassText(replyText) ? 'passed' : 'replied', + kind: isGroupPassText(pick) ? 'passed' : 'replied', member: groupMemberKey(member), thread }) - return replyText + return pick } // The turn died on our prompt: surface the gateway's retained error // through the failed-turn path (activity row + roster badge) instead of - // reading the silence as a pass or sitting out the deadline. - if (failure !== null) { - throw new Error(failure) + // reading the silence as a pass or sitting out the deadline. A failed-turn + // row whose error is gone (backend restarted since) still failed. + const error = failure ?? pick?.failedNotice ?? null + + if (error !== null) { + throw new Error(error) } recordGroupActivity(context.group, { @@ -1329,12 +1350,20 @@ export async function harvestStrandedGroupReply(group: string, member: GroupMemb const messages = Array.isArray(state?.messages) ? state.messages : [] // A transcript that never grew is not proof of nothing: a turn that dies // before its prompt is committed leaves only the retained error behind. - const reply = messages.length > strandedBefore ? pickStrandedGroupTurnReply(messages, strandedBefore) : null + // The retained error is the stranded turn's own (the next turn start would + // have replaced it): text written before a failed tool step is no reply. + const retained = retainedGroupTurnError(state) + + const pick = + retained === null && messages.length > strandedBefore ? pickStrandedGroupTurnReply(messages, strandedBefore) : null + + const reply = typeof pick === 'string' ? pick : null + const failedNotice = typeof pick === 'string' ? null : (pick?.failedNotice ?? null) if (reply === null) { // The late turn died instead of answering: say so where the user looks // (activity row + roster badge) rather than consuming the marker silently. - const failure = retainedGroupTurnError(state) + const failure = retained ?? failedNotice if (failure !== null) { const reason = groupFailureReason(failure) diff --git a/hermes_cli/web_routers/sessions.py b/hermes_cli/web_routers/sessions.py index e406017718..97a852b22a 100644 --- a/hermes_cli/web_routers/sessions.py +++ b/hermes_cli/web_routers/sessions.py @@ -554,10 +554,16 @@ def _with_tool_call_labels(message: dict) -> dict: def _project_for_display(messages: list) -> list: from agent.compaction_display import project_compaction_message_for_display from agent.context_compressor import is_compaction_summary_message + from agent.turn_failure_copy import untyped_failed_turn_display_kind projected_messages = [] for message in messages: message = _with_tool_call_labels(message) + # Same read-side typing as session.resume (tui_gateway/session_history.py). + failed_turn = not message.get("display_kind") and untyped_failed_turn_display_kind( + message.get("role"), message.get("content")) + if failed_turn: + message = {**message, "display_kind": failed_turn} if not is_compaction_summary_message(message): projected_messages.append(message) continue diff --git a/tests-js/scripts/mock-server.ts b/tests-js/scripts/mock-server.ts index d689190483..72323aa427 100644 --- a/tests-js/scripts/mock-server.ts +++ b/tests-js/scripts/mock-server.ts @@ -360,6 +360,19 @@ const TASK_PANEL_RESUME_SCRIPT: ScriptedTurn[] = [ export const PROVIDER_FAILURE_TRIGGER = 'E2E_PROVIDER_FAILURE_TRIGGER' export const PROVIDER_FAILURE_MESSAGE = 'E2E invalid_api_key: the mock refused this completion on purpose' +/** + * The same provider failure one step later: the first completion says + * TOOL_THEN_FAILURE_TEXT and calls a tool, the completion after the tool + * result is the 401. That pre-tool text is not the member's reply. + */ +export const TOOL_THEN_FAILURE_TRIGGER = 'E2E_TOOL_THEN_PROVIDER_401' +export const TOOL_THEN_FAILURE_TEXT = 'Let me note the plan before answering.' + +const TOOL_THEN_FAILURE_TURN: ScriptedTurn = { + text: TOOL_THEN_FAILURE_TEXT, + toolCalls: [{ name: 'todo', args: { todos: [{ id: '1', content: 'Answer the room', status: 'in_progress' }] } }], +} + const BLOCKING_CLARIFY_TURN: ScriptedTurn = { text: '', toolCalls: [{ name: 'clarify', args: { question: BLOCKING_CLARIFY_QUESTION, choices: ['Yes', 'No'] } }], @@ -700,7 +713,17 @@ export function startMockServer(options: MockServerOptions = {}): Promise message?.role === 'tool')) { + if (stream) { + streamScriptedTurn(res, model, TOOL_THEN_FAILURE_TURN) + } else { + nonStreamingScriptedTurn(res, model, TOOL_THEN_FAILURE_TURN) + } + + return + } + + if (userText.includes(PROVIDER_FAILURE_TRIGGER) || userText.includes(TOOL_THEN_FAILURE_TRIGGER)) { res.writeHead(401, { 'Content-Type': 'application/json' }) res.end(JSON.stringify({ error: { code: 'invalid_api_key', message: PROVIDER_FAILURE_MESSAGE, type: 'invalid_request_error' } })) diff --git a/tests/gateway/test_failure_writer_ownership.py b/tests/gateway/test_failure_writer_ownership.py index 86839736a4..71a946350f 100644 --- a/tests/gateway/test_failure_writer_ownership.py +++ b/tests/gateway/test_failure_writer_ownership.py @@ -5,7 +5,7 @@ import subprocess import sys from pathlib import Path -from agent.turn_failure_copy import PARTIAL_FAILED_TURN_NOTICE +from agent.turn_failure_copy import FAILED_TURN_DISPLAY_KIND, PARTIAL_FAILED_TURN_NOTICE def test_gateway_failure_writer_preserves_accepted_turn_identity(tmp_path): @@ -228,8 +228,10 @@ def test_fresh_session_agent_flushed_failed_turn_is_closed(tmp_path): response="x", agent_failed_early=True, hidden_reasoning_incomplete=False, is_context_overflow_failure=False, ) - roles = [m["role"] for m in db.get_messages(sid) if m["role"] != "session_meta"] - assert roles == ["user", "assistant"] + rows = [m for m in db.get_messages(sid) if m["role"] != "session_meta"] + assert [m["role"] for m in rows] == ["user", "assistant"] + # Typed like the core closer's row, or Desktop reads the boundary as the model's reply. + assert rows[-1]["display_kind"] == FAILED_TURN_DISPLAY_KIND assert store.transcript_tail_role(sid) == "assistant" db.close() diff --git a/tests/hermes_cli/test_session_message_page_owner.py b/tests/hermes_cli/test_session_message_page_owner.py index 3e43acdfcd..5d453f83ab 100644 --- a/tests/hermes_cli/test_session_message_page_owner.py +++ b/tests/hermes_cli/test_session_message_page_owner.py @@ -47,3 +47,40 @@ def test_message_pages_identify_the_serving_profile(tmp_path, monkeypatch, servi assert [row["content"] for row in older["messages"] + tail["messages"]] == [ f"message-{index}" for index in range(199) ] + + +def test_message_pages_type_untyped_failed_turn_rows(tmp_path, monkeypatch): + """Desktop cold-loads and pages through REST, not ``session.resume``: a failed-turn boundary + written before the closers typed it must reach it as ``failed_turn``, not model text.""" + from agent.turn_failure_copy import FAILED_TURN_DISPLAY_KIND, FAILED_TURN_NOTICE, PARTIAL_FAILED_TURN_NOTICE + from hermes_state import SessionDB + + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setattr(Path, "home", lambda: tmp_path) + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr("hermes_state.DEFAULT_DB_PATH", home / "state.db") + db = SessionDB(db_path=home / "state.db") + try: + db.create_session(session_id="s", source="desktop") + db.append_messages_batch("s", [ + {"role": "user", "content": "a"}, + {"role": "assistant", "content": FAILED_TURN_NOTICE}, + {"role": "user", "content": "b"}, + {"role": "assistant", "content": PARTIAL_FAILED_TURN_NOTICE}, + {"role": "user", "content": "c"}, + {"role": "assistant", "content": f"Quoting Hermes: {FAILED_TURN_NOTICE}"}, + ]) + finally: + db.close() + + from hermes_cli.web_routers.sessions import manage_router + + app = FastAPI() + app.include_router(manage_router) + with TestClient(app) as client: + rows = client.get("/api/sessions/s/messages").json()["messages"] + + assert [row.get("display_kind") for row in rows] == [ + None, FAILED_TURN_DISPLAY_KIND, None, FAILED_TURN_DISPLAY_KIND, None, None, + ] diff --git a/tui_gateway/session_history.py b/tui_gateway/session_history.py index 540f3a16dc..b01a4129a3 100644 --- a/tui_gateway/session_history.py +++ b/tui_gateway/session_history.py @@ -7,7 +7,6 @@ import re from .method_ctx import bind_module from agent.prompt_builder import STEER_DISPLAY_KIND -from agent.turn_failure_copy import FAILED_TURN_DISPLAY_KIND, FAILED_TURN_NOTICE, PARTIAL_FAILED_TURN_NOTICE # Discord routing note (gateway/run_inbound.py::discord_triggering_note) persisted as user # ``content`` by gateways before the authored-text fix; presentation-only heal for those rows. @@ -180,8 +179,11 @@ _AUTO_CONTINUE_NOTE_PREFIX = "[System note: Your previous turn was interrupted m def _legacy_display_kind(role: str, text: str) -> str | None: """Display type of a synthetic row persisted untyped: new rows are typed at turn start (``persist_user_display_kind``); this prefix sniff migrates rows already on disk (a turn killed mid-run never reached the stamp).""" - if role == "assistant" and text.strip() in (FAILED_TURN_NOTICE, PARTIAL_FAILED_TURN_NOTICE): - return FAILED_TURN_DISPLAY_KIND # failed-turn boundary written before it was typed + # Imported functions are not rebound onto server.py (method_ctx.bind_module): import here. + from agent.turn_failure_copy import untyped_failed_turn_display_kind + + if failed_turn := untyped_failed_turn_display_kind(role, text): + return failed_turn return "auto_continue" if role == "user" and text.lstrip().startswith(_AUTO_CONTINUE_NOTE_PREFIX) else None