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.
This commit is contained in:
teknium1
2026-09-23 17:23:40 +00:00
committed by Teknium
parent 224e7b673e
commit 9d6e4e72a4
9 changed files with 275 additions and 26 deletions

View File

@@ -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 ():

View File

@@ -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') })
})

View File

@@ -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<string, unknown>) => Promise<unknown>
let submitted = false
host.request = async (method: string, params: Record<string, unknown> = {}) => {
const result = (await request(method, params)) as Record<string, unknown>
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<string, unknown>) => Promise<unknown>
host.request = async (method: string, params: Record<string, unknown> = {}) => {
const result = (await request(method, params)) as Record<string, unknown>
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

View File

@@ -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<null
// the transcript, so the tombstone — not the message count — is the only
// evidence; a tombstone identical to the pre-submit one is an older turn's.
const failure = retainedGroupTurnError(state)
const died = failure !== null && (messages.length > 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)

View File

@@ -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

View File

@@ -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<MockSe
return
}
if (userText.includes(PROVIDER_FAILURE_TRIGGER)) {
if (userText.includes(TOOL_THEN_FAILURE_TRIGGER) && !messages.some(message => 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' } }))

View File

@@ -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()

View File

@@ -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,
]

View File

@@ -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