A legacy (no-digest) dict over a non-blank assistant row adopted the whole
decoded DB row: tool_calls / reasoning* / codex_* were overwritten with the
stored JSON (which still holds the escaped lone surrogate the sanitizer just
fixed, re-injecting it into the provider payload) and live-only fields were
popped. Resumed dicts (_rows_to_conversation stamps _row_id without a
digest) and compaction clones hit this path. Adopt content only, as before
this stack, via a content-only canonical handled like the metadata-only one.
_insert_message_rows dropped a clone's parent digest but only the flush
path restamped it, so clones made by archive_and_compact / replace /
rotation handoff / import reached the legacy path and the first live edit
after a clone was not persisted. Stamp the stored-row digest inside
_insert_message_rows (one batched SELECT, cold paths only; the flush path
statement count is unchanged) and drop the duplicate call in
append_messages_batch.
Define the _db_row_snapshot / _canonical_row keys once in
agent/message_metadata.py and import them everywhere instead of repeating
the literals.
_sanitize_surrogates ran the surrogate regex over every string leaf of the
outbound messages and kwargs on each request. Surrogates are never ASCII and
str.isascii() is an O(1) flag check, so gate the scan on it; same gate on
_strip_non_ascii. 50 KB ASCII leaf: 1017 us -> 0.1 us; output unchanged.
Partial salvage of #83839: kept the isascii fast-path idea as a single gate
at the top of _sanitize_surrogates (main refactored to a generic fix-callable
walk, so the PR's per-site hunks no longer apply); dropped the monkeypatch
"never invokes regex" tests because they are change-detectors.
(cherry picked from commit 36d2575a12e2ffae1a3286208f269fb3d92acbf5)
The image-rejection recovery no longer switches the session to
text-only or strips history; it records the (provider, model) and
build_api_request strips images from that model's requests only. Three
places still described the old behaviour:
- recover_before_classification docstring and the adjacent comment said
"switch session to text-only" / "mark session vision-unsupported".
- TestStripImagesDropsStaleApiContent's rationale claimed the strip runs
on persistent history and that leaving api_content would replay
rejected images every turn because the recovery gates on
_image_rejecting_models — false on both counts: current callers pass
per-call clones. Reworded as the generic helper contract (a rewritten
persisted row must drop its sidecar) with the no-op note.
- _strip_images_from_messages docstring, same fix.
Comments and docstrings only; no code change.
Two branches in turn_recovery.py carried the same isinstance +
_strip_images_from_messages guard and the same "Provider rejected a
corrupted image" notice: the new pre-classification corrupt branch and
the FailoverReason.image_corrupt branch. Both now call one module
helper, _strip_request_images_and_retry(agent, api_messages) -> bool,
so the strip-this-attempt-only policy lives in one place.
_IMAGE_REJECTION_PHRASES was rebound as unsupported + corrupt, which
made _looks_like_image_content_rejection silently cover corrupt payloads
and forced the recovery to test the corrupt list a second time to undo
that. The name stays (external references) but is now the
unsupported-only tuple; _IMAGE_CORRUPT_PHRASES is disjoint, and the
recovery asks the two questions explicitly:
`_corrupt or (model not yet rejected and unsupported)`.
Tests: the phrase-isolation matcher asks the same disjoint question the
recovery does; the image_corrupt source-contract check looks for the
helper call instead of the inlined strip.
init_agent seeds `_image_rejecting_models = set()` at the same site as
`_force_ascii_payload`, which the adjacent sanitize_outbound_kwargs already
reads as a plain attribute. The getattr/isinstance guards and the lazy
re-create in recover_before_classification implied the attribute could be
missing or mistyped on a real agent; it cannot, and defensive fallbacks for
impossible states hide wiring bugs instead of surfacing them. Every test
fixture that reaches these paths seeds the attribute already.
message_sanitization.image_model_key duplicated vision_message_prep's
_provider_model_key — the (provider, model) key the same mixin already uses
for its per-model vision bookkeeping. Two keying functions for the same
concept drift: one normalised the provider (.strip().lower()), the other did
not, so a provider spelled "OpenAI" in one place and "openai" in another
would have been tracked as two models. Key `_image_rejecting_models` on the
existing helper and delete the duplicate (and its __all__ entry).
No import cycle: vision_message_prep imports only lazy_forward,
tool_dispatch_helpers and utils, none of which import message_sanitization
or turn_recovery.
recover_before_classification matches _IMAGE_REJECTION_PHRASES, which
mixes two kinds of body: capability rejections ("does not support
images", "only text content type is supported", ...) and bad-payload
rejections. After the per-model tracking landed, BOTH added the
(provider, model) to agent._image_rejecting_models, so one truncated
screenshot rejected with "failed to decode image" made every later
request to that model text-only for the rest of the session, even
though the model can see fine.
Split the bad-payload phrases into _IMAGE_CORRUPT_PHRASES:
- "image data you provided does not represent a valid image"
(ChatGPT-account Codex backend)
- "failed to decode image" (Kimi / Moonshot and other
OpenAI-compatible providers)
_IMAGE_REJECTION_PHRASES stays the union so the turn still recovers on
them. For a corrupt match the recovery now strips the current attempt
only and retries iff something was stripped (mirroring the existing
image_corrupt branch), and leaves the model unmarked; only a capability
rejection records the model.
One test: a "failed to decode image" body strips the wire copy, keeps
history, and leaves _image_rejecting_models empty so the next request
to the same model carries its images.
The first head stored a single rejecting (provider, model) and kept the
turn-global `_vision_supported` as the recovery guard. In a fallback
chain that fails: model A rejects images and retries text-only, a later
error activates model B, the restart rebuilds api_messages from history
so B receives the images, and when B rejects them too the branch is
skipped because `_vision_supported` is already False — the request
falls through to generic error handling. Recording B also overwrote A,
so A was no longer treated as text-only on later turns.
`_image_rejecting_models` is now a set of every rejecting model, and it
is also the guard: each model's first rejection runs the recovery and a
repeat rejection from the same model still falls through, so the retry
cannot loop. image_model_key() names the key in one place.
Adds a test for the two-model sequence (fails on the previous head) and
one pinning that a repeat rejection from the same model does not retry.
Thanks to @ehz0ah for the review.
(cherry picked from commit 225fd76ccf90cd3ec509a9327f725ac02d996f24)
When a provider 4xx's on image content, recover_before_classification
ran _strip_images_from_messages on the canonical `messages` list and
reset _db_flush_scan_prefix. Since #117569 that function pops
_db_persisted on every rewritten dict, so the next flush rewrote those
rows: every image in the session — and every image-only message, which
the stripper deletes outright — was removed from state.db for good.
The rejection describes what the CURRENT model accepts, not what the
conversation holds. An automatic fallback to a text-only provider, or a
single /model switch, was enough to erase images the user had sent to a
vision model, and switching back found them gone. It is the same failure
as the ASCII strip in #117802, on the image path; neither open fix for
that issue touches this branch.
Keep the repair on the send path, where the per-call copy already lives
(_clone_message_for_send exists so send-path rewrites never reach the
persisted transcript, #80498):
- record the rejecting (provider, model) on the agent, a session-scoped
flag initialised beside _force_ascii_payload;
- strip the in-flight api_messages copy for the immediate retry;
- build_api_request calls strip_images_for_rejecting_model() on each
attempt's api_messages BEFORE provider conversion. The stripper knows
Hermes's own part types; a converted payload would slip past it
(Bedrock Converse image blocks carry no `type`). Keyed on the model,
so one that accepts images gets them again.
_strip_images_from_messages itself is unchanged, so its role-alternation
and sidecar guarantees still hold on the wire copy. The notice no longer
claims "text-only mode for this session" (_vision_supported resets every
turn) or that images were stripped from history.
(cherry picked from commit cbdb184c27f915ab138b2087f878aed7fcc7c76b)
sanitize_outbound_kwargs detached the agent.tools alias with copy.deepcopy
before the ASCII strip. The repo's send-path detach idiom is the structural
clone _clone_message_for_send (dicts/lists recursively, immutable leaves
shared), which is what every other outbound copy uses and is cheaper on
JSON-shaped, acyclic payloads. It is sufficient here because
_sanitize_structure only rebinds str leaves inside dict/list containers, so
the clone fully isolates the canonical tool schemas. Imported lazily inside
the function (conversation_loop imports this module) exactly as
turn_finalizer does. Drops the now-unused `import copy`.
api_kwargs["tools"] is rebuilt from agent.tools on every attempt
(_build_api_kwargs_for_mode: tools_for_api = agent.tools; transports set
api_kwargs["tools"] = tools without copying), so the list usually aliases
the canonical tool schemas. The ASCII retry path then runs
sanitize_outbound_kwargs with _force_ascii_payload set, and its in-place
strip rewrote agent.tools for the rest of the session.
Move the guard to the chokepoint: when the flag is set and tools IS
agent.tools, deepcopy before stripping. The deepcopy the contributor pick
added inside _recover_unicode_encode_error only protected the failed
request's kwargs, which are discarded before the retry; drop it and have
recovery skip an aliased tools list entirely (request-local lists are
still stripped for the diagnostic message).
The kept test now exercises the chokepoint directly: with the flag set on
kwargs whose tools aliases agent.tools, agent.tools must be byte-stable
afterwards. Verified red against the previous chokepoint.
The _db_persisted marker asserts that a message dict's persisted row is
durable as written; any in-place mutation must pop it or the flush scan
identity-skips the dict and state.db keeps the stale row forever. Six
mutation sites violated the contract:
- micro_compaction._merge_adjacent_user_turns rewrote content on a
carried-forward dict after superseding a stale micro marker. On the
archive-failure path nothing re-stamps, so the merged text never
reached state.db.
- repair_message_sequence passes mutated stamped survivors in place:
_merge_assistant_into (tool_calls union, content join,
reasoning_content carry), _prune_unanswered_tool_calls (tool_calls
rewrite), _merge_consecutive_users (content join).
- sanitize_tool_call_arguments rewrote corrupted/blank
function.arguments and prepended the corruption marker onto stamped
resumed rows, leaving the corrupt bytes durable and self-perpetuating
across resumes.
- _sanitize_messages (surrogate and non-ASCII recovery) and
_strip_images_from_messages (image-rejection recovery) rewrote live
dicts on the recovery path.
Each site now pops the marker when a persisted field actually changes,
and the agent-aware callers (repair_message_sequence_with_cursor,
turn_iteration_prep, turn_recovery) invalidate the bounded flush-scan
prefix so repaired rows are rewritten on the next flush.
Balanced-but-misnested argument JSON ({"a": [{"b": 1}, {"c": 2}}]}, the reporter's
deepseek-v4-flash shape where the "]" of an array of objects is dropped and the
neighbouring "}" closes in its place) has equal delimiter counts, so the closer
arithmetic had nothing to append and the call degraded to "{}" — silently losing
the tool call. The string-aware scan from #115096 now rewrites the text: a closer
that matches a deeper opener gets the missing inner closers inserted in front of
it, and the remaining stack is appended in order. Anything ending inside a string
still returns "{}" (unrecoverable content is never guessed).
Tests trimmed to invariants: string-aware balance and stack-order close (from
#115096), the five reporter shapes, and the streaming assembler seam
(_StreamingCall._assemble_tool_calls) repairing instead of flagging truncation,
with the mid-string-cut control still flagged.
Part of #115061 (JSON-repair half; the plugin button-callback half is a
separate design change the reporter agreed to file standalone).
Truncated tool-call arguments whose string values contain braces or brackets
("}", "if (x) {") were miscounted by the naive count-based repair: the
delimiter inside the value either cancelled a real unclosed brace or demanded
one closer too many, and the candidate never parsed. Closers were also
appended grouped (all } before all ]) instead of in stack order, so a
truncated array of objects ({"items": [{"n": 1}, {"n": 2) could not be
closed at all. Scan the prefix string-aware and append the closers the open
stack actually needs.
A tool call whose function.name violates the provider pattern
^[A-Za-z0-9_-]{1,64}$ — OpenAI's synthetic `multi_tool_use.parallel`, or a
whole shell command a weak fallback model put into `name` (372 chars) — is
persisted once and then 400s every later request on a strict endpoint, so
the session silently pins itself to the lenient fallback model (#51944).
`agent/message_sanitization.py::coerce_tool_name` is now the single owner of
the coercion (valid → identity, invalid runs → `_`, cut at 64, empty →
fallback); the Codex Responses adapter uses it in place of its private copy,
and the pre-call sanitizer's nameless-call repair becomes
`_repair_invalid_tool_call_names`, so both outbound builders that already
call `sanitize_api_messages` — the main loop (turn_request_assembly) and the
iteration-limit summary (chat_completion_helpers) — send valid names. Dict
tool calls are rewritten copy-on-write, so the summary path's shallow message
copy never edits persisted history; tool results follow through
`_realign_tool_result_names`. Deterministic, so identical stored bytes always
render identical wire bytes (prompt-cache prefix stays stable).
Co-authored-by: liuhao1024 <sunsky.lau@gmail.com>
The summary path had grown a verbatim copy of turn_api_request's 3-line
surrogate/ASCII chokepoint — the same drift class this PR removes for the
hand-rolled kwargs builder. Move the two lines and the #50959 rationale into
`message_sanitization.sanitize_outbound_kwargs` and call it from both sites,
so the next sanitizer step added to the main loop cannot miss the summary.
Tighten two comments: "same kwargs builder" (cache_control redecoration is
not re-applied here) and a `_summary_text` note that is true for all three
summary branches, not just the chat one.
Some OpenAI-compatible gateways fronting Gemini backends emit the native
uppercase finish reasons (STOP, MAX_TOKENS) instead of the lowercase
OpenAI contract values. Every downstream comparison in Hermes uses
lowercase literals, so an uppercase reason silently fell through: a
clean STOP completion missed the stop handling and a MAX_TOKENS
truncation never entered the length-recovery path.
Adds normalize_finish_reason() as the single owner in
agent/message_sanitization.py (case fold + alias map: max_tokens->length,
end->stop, function_call->tool_calls) and wires it at both wire-intake
choke points: ChatCompletionsTransport.normalize_response and the
streaming chunk-capture loop in chat_completion_helpers. Non-string and
empty values pass through unchanged so existing 'or "stop"' defaults
and the Poolside int-reason path keep their behavior.
For each issue anchor present in BASE 63279301bc non-test .py and absent on HEAD, the BASE comment/docstring block was re-attached at the HEAD location of the code it explained (matched by the distinctive code line / enclosing def). Sentences already covered by an existing HEAD comment were deduped; the issue number always survives. Insert-only: no code lines changed.
The preflight trigger charged reasoning/reasoning_content on every assistant message while the tail-budget walks charged newest-turn-only (#73624), so reasoning-heavy codex_responses sessions fired compaction forever while the walk protected everything (middle_window_tokens=0, no_progress every turn, each attempt a full aux summarization).
Wire truth: the codex_responses input builder never ships the text thinking keys (encrypted codex_reasoning_items carry the chain and were already charged unconditionally by both sides), so the trigger overcounted reality; echo-back chat-completions families (DeepSeek/Kimi/MiMo thinking mode) replay stored reasoning_content on every turn, so there the walk undercounted. New single wire-truth predicate message_sanitization.stale_thinking_reaches_wire() now drives BOTH sides: trigger estimates exclude stale thinking on non-echo routes; tail/prune walks charge it on echo routes.
Also: reasoning/reasoning_content double-count fixed in both estimators (wire ships at most one; +53% overcount vs provider prompt_tokens per issue comment), and the commit-layer no_progress path now arms the structural no-op backoff so an unchanged-transcript compaction cannot re-fire every turn (defense in depth; overlaps the #96775 re-entry class).
A 413 is a byte-size error, but the recovery loop scored compression
progress with estimate_messages_tokens_rough, which deliberately prices
every image at a flat per-image token cost (so screenshots don't trigger
premature compaction). When the payload is image-dominated that check can
never pass: in the reporting session two vision_analyze results were
5,627,202 bytes (96.6% of the request body) but ~3K of the ~80K token
estimate, so every attempt reported no_progress, the budget burned, and
the session wedged permanently with 'max compression attempts (3)
reached' at 13% context usage.
Post-#97160, the 413 path already routes into compaction and compaction's
historical-media aging genuinely frees the image bytes — but the
token-scored yardstick could not see the megabytes it freed. Add
serialized_messages_bytes() (exact serialized payload size, measured
identically before and after each pass — a measurement, not an estimate)
and score the 413 progress check with it. Tokens remain for status
display only; the context-overflow branch keeps its token yardstick,
because that error IS a token-budget error.
Images are never evicted from live history outside compaction (cache
invariant); the original strip-from-history mechanism in this PR was
superseded by #97160's compaction-time aging and is dropped in salvage.
Salvaged from #88960. Fixes#47339.
_strip_images_from_messages() deleted any non-tool message whose content
became empty after image removal. An assistant message whose content was
entirely images but which carried tool_calls was therefore dropped,
orphaning its paired tool responses — providers reject the next request
with unmatched tool_call_id errors (HTTP 400). Replace such messages
with the plaintext placeholder instead, exactly like tool-role messages.
Adds a regression test covering the assistant + tool_calls +
image-only-content case.
Closes#40463
`api_content` is the byte-stability sidecar from #67274: it holds the exact
bytes previously sent for a message, and every turn substitutes it back into
`content` when building `api_messages`. `drop_stale_api_content` exists so a
content rewrite cannot be replayed from it — its own docstring states the
contract, and names the historical image strip as one of the callers:
Replaying the pre-rewrite sidecar would resend exactly what the rewrite
removed, so it must be dropped — the cost is one cache boundary miss,
never wrong content.
`_strip_images_from_messages` never drops it. The image-rejection recovery in
`conversation_loop` runs it over the persistent history, not just the per-call
copy:
agent._vision_supported = False
_imgs_removed = _strip_images_from_messages(messages) # history
if isinstance(api_messages, list):
_strip_images_from_messages(api_messages)
and `api_messages` are copies (`api_msg = msg.copy()`), so the history message
keeps its sidecar. The strip is therefore undone on the very next turn.
Reproduced with the real functions:
history content after strip : [{'type': 'text', 'text': 'look'}]
sidecar still present : True
NEXT TURN sends : 'look<IMAGE BYTES SENT LAST TURN>'
This is worse than a one-turn glitch, because the recovery cannot fire again:
it is gated on `getattr(agent, "_vision_supported", True)` and just set that
False. So on every subsequent turn the sidecar re-injects the images, the
text-only endpoint rejects them again, and the branch that would strip them is
disabled — the session stays wedged on a 4xx it already knew how to fix.
Drop the sidecar on each message the strip rewrites, inside the function so
every caller is covered. Messages with no images keep theirs, so only the
rewritten message pays a cache boundary — the tradeoff the invariant
prescribes. The two sibling recovery paths, `_sanitize_messages_surrogates`
and `_sanitize_messages_non_ascii`, are already safe: both walk every string
field on the message and so scrub the sidecar in passing. This one only
touches `content`.
tests/run_agent/test_image_rejection_fallback.py: new
TestStripImagesDropsStaleApiContent — the rewritten message loses its sidecar,
the next turn does not resend the stripped images, the tool-placeholder rewrite
is covered too, and untouched messages keep their sidecar. All four fail on
main. 53 passed across the image-rejection and api_content-sidecar suites; 307
passed across the sanitization/image/sidecar/replay agent tests (8 failures in
test_image_routing.py / test_save_url_image.py are pre-existing and fail
identically on clean main).
Match Responses/Codex tool-call aliases across execution, repair, sanitization, replay, and duplicate handling so valid parallel results are not replaced by unavailable stubs.\n\nFixes #93251
Review follow-up (W1): the pre-send transcript sanitizer
(agent_runtime_helpers.sanitize_tool_call_arguments) runs on the
PERSISTED messages list before every api_messages build and rewrites any
json.loads-failing argument string to "{}" in the transcript, prepending
a corruption marker to the paired tool result. That in-transcript repair
is deliberate (the stored turn must be replayable next call), but it
destroys the model's original bytes — for a truncated write_file call
those bytes are the user's streamed file content (#80498), and they
previously survived only as an 80-char log preview.
Until a sidecar-preservation design exists, make the bytes recoverable:
both destruction sites (the transcript sanitizer's WARNING and
_repair_tool_call_arguments' unrepairable-path WARNING) now log the full
original argument string bounded at 100KB instead of 80 chars. Corrupted
calls are rare; an oversized WARNING is a fair price for the only copy
of real user content.
#48879 closed the tool-call sequence on interrupt inside finalize_turn so a
/stop after a tool no longer persists a `tool` tail that the next user message
turns into a `tool -> user` role-alternation violation (which strict providers
like Gemini/Claude react to by hallucinating a continuation and ignoring prior
context — what users see as "lost context after stop").
But the retry-wait, error-handling, and post-error retry-wait interrupt aborts
in conversation_loop return early and never reach finalize_turn, so they still
persisted and returned a raw `tool` tail. Interrupting during provider
backoff/rate-limiting (common under heavy work) hit exactly this path.
Extract the close into a shared close_interrupted_tool_sequence helper and apply
it at every interrupt abort (finalize_turn + the three early returns) so the
whole bug class is fixed, not just the one site.
PR #35718 added a per-slot "cumulative-resend" latch to the universal
streaming tool-call accumulator to fix DeepSeek / Baidu Qianfan (#35592).
The latch fires when a delta is a strict superset of the accumulated
buffer (len(_new) > len(_prev) and _new.startswith(_prev)) and then
REPLACES the buffer instead of appending.
That superset test is not an unambiguous cumulative signature. A normal
incremental stream can emit a single fragment that restates an already-
accumulated prefix — trivially common in large code-patch arguments with
repeated lines / indentation — which trips the latch and clobbers the
accumulated buffer, corrupting the tool call. Observed in the wild on
Anthropic Opus (the primary model) building a large patch: corrupted /
short arguments → finish_reason='length' dead-end → session killed.
A guessing heuristic that can silently clobber a tool-call buffer has no
place on the path every provider and model shares. Reverting restores the
known-good plain `+=` accumulator. The #35592 narrow provider bug should
be re-addressed provider-gated so it is structurally impossible to touch
Anthropic / OpenAI incremental streams, rather than via a heuristic on the
shared path.
Reverts ca03486b6.
DeepSeek / Baidu Qianfan stream tool-call arguments in cumulative mode:
each chunk resends the full arguments-so-far instead of the new fragment.
The stream accumulator blindly concatenated arg deltas with +=, turning
that into '{...}{...}{...}', which failed json.loads and got nuked to '{}'
— a silently corrupted tool call (#35592). Worse on multi-param tools
(search_files, session_search, memory replace) because longer args take
more chunks, giving more resend opportunities.
- Per-slot cumulative latch in the stream accumulator: a delta that is a
strict superset of the accumulated buffer marks the slot cumulative and
replaces (not appends); exact duplicates are dropped only after latching.
Incremental fragments are untouched (default += path).
- Backstop _collapse_repeated_json_arguments() in the repair pipeline
collapses pure identical-resend buffers (K exact repeats of a valid-JSON
unit) for providers that resend the complete object from chunk 1. Only
reached after json.loads already failed, so compliant single objects are
never touched.
Not a gateway or DeepSeek-model bug — any OpenAI-wire provider in
cumulative streaming mode is affected.
Pull the 10 pure sanitization/repair helpers (\_sanitize_surrogates,
\_sanitize_structure_surrogates, \_sanitize_messages_surrogates,
\_escape_invalid_chars_in_json_strings, \_repair_tool_call_arguments,
\_strip_non_ascii, \_sanitize_messages_non_ascii, \_sanitize_tools_non_ascii,
\_strip_images_from_messages, \_sanitize_structure_non_ascii) and the
\_SURROGATE_RE constant out of run_agent.py into a new module.
These are stateless byte-walking helpers with no AIAgent dependency.
Backward compatibility: run_agent re-exports every name via a single
import block, so existing 'from run_agent import _sanitize_surrogates'
imports in tests and cli.py keep working unchanged. Same pattern the
file already uses for _summarize_user_message_for_log (codex_responses_adapter).
run_agent.py: 16077 -> 15682 lines (-395).
