fix(api_server): robust Responses turn-start detection - #70695
Conversation
|
Thanks for targeting the shared turn-boundary helper. The current detector is all-or-nothing at Problems
Suggested changes
This is an automated hermes-sweeper review. |
…m strip Address hermes-sweeper review of PR NousResearch#70695: 1. Premise fix — the agent core never prepends its private system prompt into the stored result["messages"]; the prompt is prepended only to the wire copy (api_messages in conversation_loop ~L1065). So the stored transcript has no leading private system message to strip. 2. Correctness fix — drop the role=='system' strip entirely. A leading system message in result["messages"] can only be client-supplied history (the Responses input parser accepts any role, api_server ~L3919/3950), so stripping by role would delete legitimate client state. It now lives in 'prior' and is matched, never skipped. Keep the useful part of the PR: semantic (not byte-equal) prefix matching so in-place history reshaping (api_content sidecar stamps, alternation repair) does not defeat turn-start detection and re-trigger the phantom-function_call / history-doubling bugs (NousResearch#68257). Reverse-anchor + final-assistant fallbacks retained for the compression/unrecoverable-prefix paths. Add two behavior-contract E2E regressions driving the real POST /v1/responses path: (a) api_content in-place stamp on a prior row must not double history or replay a phantom function_call; (b) client-supplied leading system history round-trips and is never stripped.
e73cf49 to
8f8482e
Compare
|
Addressed the sweeper review and rebased the PR onto current Changes made:
Verification on current main:
The PR now consists of one commit on current main and changes only |
Field confirmation: the in-place-reshaping trigger is the one that fires in production (v0.20.3)Independent confirmation from a self-hosted deployment, offered as evidence for the Environment: hermes-agent v0.20.3 ( The second bullet of this PR is the trigger, and it reproduces on current mainOf the three mismatch triggers listed here, the one I observe is in-place reshaping — not the Minimal repro against the storage path, run against installed v0.20.3, no mocks: from gateway.platforms.api_server import APIServerAdapter as A
prior = [
{"role": "user", "content": "turn 1"},
{"role": "assistant", "content": "", "tool_calls": [
{"id": "call_1", "type": "function",
"function": {"name": "bash", "arguments": "{}"}}]},
{"role": "tool", "tool_call_id": "call_1", "content": "ok"},
{"role": "assistant", "content": "done"},
]
user_message = "turn 2"
agent_messages = [dict(m) for m in prior]
agent_messages.append({"role": "user", "content": user_message})
agent_messages.append({"role": "assistant", "content": "done 2"})
# A. untouched -> correct
res = {"messages": [dict(m) for m in agent_messages]}
assert A._response_messages_turn_start_index(prior, user_message, res) == 5
assert len(A._build_response_conversation_history(list(prior), user_message, res, "done 2")) == 6
# B. ONE prefix message reshaped in place (here: trailing-space normalisation)
mutated = [dict(m) for m in agent_messages]
mutated[3] = {**mutated[3], "content": "done "}
res2 = {"messages": mutated}
assert A._response_messages_turn_start_index(prior, user_message, res2) == 0 # prefix miss
out = A._build_response_conversation_history(list(prior), user_message, res2, "done 2")
assert len(out) == 11 # expected 6Stored transcript in case B — the whole prefix is re-appended after the current user turn:
Production numbers (observed on 0.19.0, before I updated)Turn-start log lines for one conversation, each entry one chained request: Alongside, Cross-checked against ground truth: for that session I repaired one snapshot by hand (403 → 57, rebuilt from Downstream consequence worth noting: it manufactures duplicate
|
What does this PR do?
/v1/responseshas two reported misbehaviours that share a single root cause ingateway/platforms/api_server.py::_response_messages_turn_start_index:outputarray replays the previous turn'sfunction_call/function_call_outputitems.The helper located the current turn's start by a strict byte-for-byte prefix equality (
agent_messages[:len(prior)] == prior) and, on any mismatch, returned0. A0start makes_extract_output_items(start_index=0)walk the entire transcript and re-emit every historicaltool_calls+ tool message as this-turn output (phantomfunction_calls), and makes_build_response_conversation_historyfall through toprior + current_user + full_transcript(doubling).The strict equality breaks under several non-exceptional conditions, so this is a bug class, not a single trigger:
systemmessage to the transcript (the Exponential conversation history doubling in Responses API (_build_response_conversation_history) #68257 trigger);api_contentsidecar stamping inconversation_loop.py, provider cache markers — none of which change meaning but all of which defeat==;result['_compressed']from fix(api): persist compressed transcripts in ResponseStore to stop re-compression loops #69306, but the output side had no such fallback).The fix makes
_response_messages_turn_start_indexrobust instead of all-or-nothing: skip leading privatesystemmessages → tolerant semantic prefix match (compare only meaningful fields, ignore sidecar stamps) → reverse-anchor on the lastusermessage equal to this turn's input → last resort trust only the final assistant message and never replay unattributable history tool calls. The suffix-only path (genuine current-turn tool calls, no prior) still returns0. Because every call site funnels through this one helper, the fix benefits output extraction, storage, and streaming (_turn_transcript_messages) at once.Relationship to #68282: #68282 strips the leading
systemmessage on the storage path only and explicitly does not touch the output-side offset. This PR fixes the shared helper itself, covering all three mismatch triggers (system-prefix + in-place reshaping + compression) and also the output-side phantomfunction_calls that #68282 leaves in place. Non-competing (same function, complementary scope) — happy to defer to maintainers; #68282's system-stripping intent is subsumed here.Invariants respected: no mid-conversation prompt/tool/system mutation (prompt caching preserved), strict role alternation, no synthetic user messages.
Related Issue
Fixes #68257
Type of Change
Changes Made
gateway/platforms/api_server.py: rewrote_response_messages_turn_start_index— skip leading privatesystemmessages; tolerant semantic prefix match againstprior + current_userthenprior; reverse-anchor fallback on the last matchingusermessage; final-assistant last resort; never replay unattributable history tool calls.gateway/platforms/api_server.py:_build_response_conversation_historystrips leading privatesystemmessages so stored history round-trips cleanly asprioron the next chained request.tests/gateway/test_api_server.py: addedTestTurnStartRobustness(6 behavior-contract regressions).How to Test
python -m pytest tests/gateway/test_api_server.py -o 'addopts=' -q→ 232 passed (6 new + 226 existing, no regressions).main(RED) and pass with this change (GREEN).main, passes here):Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/gateway/test_api_server.py) and all tests pass (232 passed)Documentation & Housekeeping
docs/, docstrings) — N/A (internal helper, no public-facing config/doc surface)cli-config.yaml.exampleif I added/changed config keys — N/A (no config keys)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A (behavior fix within existing helper)Related issues
function_callreplay in the area introduced by fix(gateway): avoid duplicated Responses history (salvage #18995) #21185.Generated with AI assistance.