Preserve reasoning and replay tool history across turns - #2470
Conversation
Inside the MCP loop, keep <think> content as reasoning_content on the
assistant tool-call message instead of dropping it: preserved-thinking
models (Kimi K2/K3 family) condition each tool round on their prior
reasoning and degrade when it is stripped. Also omit the content field
entirely when nothing visible remains, since some OpenAI-compatible
backends reject empty text next to tool_calls with a 400.
Across turns, the MCP flow now opts into replayToolHistory in
prepareMessagesWithFiles: past assistant turns are expanded from their
persisted tool updates into assistant/tool message pairs (grouped back
into rounds, update uuid as tool_call_id, outputs capped at 8k chars),
with reasoning re-attached as reasoning_content instead of inline
<think> text. Previously history was flattened to plain {role, content},
so models saw no evidence of tools they had just used and would deny
having them. The plain completion path is unchanged.
Verified with scripts/reasoning-replay-harness.ts, which sends the same
tool-using conversation in the old flat shape and both new shapes to the
first 10 router models: no model that accepts the old shape rejects the
new ones (2 runs, 0 regressions).
Requests now stream like prod and repeat per model/scenario (sequential reps, models in parallel), reporting median time-to-first-token and chunk-approximated tokens/sec alongside the acceptance verdict.
…e budget Derive nine-character alphanumeric tool_call_ids from the persisted update uuids, since Mistral-family chat templates reject other shapes. Replay calls that have no persisted Result or Error as an explicit interruption error instead of fabricating an empty successful output, which an aborted run can otherwise leave behind. Cap the cumulative expanded replay at 100k chars spent newest-first, with older turns falling back to the flat shape, so long tool-heavy histories cannot outgrow a context window they previously fit. Document that replayed arguments are best-effort, as only top-level primitive params are persisted by design.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce6265ad02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // S3: attach reasoning_content to the first tool-call assistant message, | ||
| // mirroring what runMcpFlow sends between rounds of a live turn. | ||
| const inloop: ChatMessage[] = replay.map((m) => | ||
| m.role === "assistant" && "tool_calls" in m && m.tool_calls?.[0]?.id === "call-1" |
There was a problem hiding this comment.
When this harness builds S3, prepareMessagesWithFiles has already normalized the stored ID call-1 to call10000, so this predicate never matches and S3 is byte-for-byte identical to S2. As a result, the advertised compatibility check never sends reasoning_content on a tool-call assistant message, allowing the harness to report the in-loop shape as shippable without testing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, this predicate went stale in ce6265a when the replay ids got normalized, so the S3 runs between that commit and 56382cf were effectively duplicates of S2. Fixed in 56382cf when the scenarios were restructured (the predicate now matches the normalized id call10000). The final harness runs used the fix: S3's payload is 1106 chars vs 1025 for S2 (the delta is exactly the added reasoning_content property), and every reachable model accepted it.
The tool-less chat_completions path now re-attaches persisted reasoning (inline think blocks and message.reasoning) as reasoning_content on past assistant turns, so preserved-thinking models keep their chain in ordinary conversations too. Tool replay stays off there since the path never declares tools. Per review: the cross-turn reasoning echo is gated on the model's supportsReasoning flag in both flows (matching how reasoning_effort is forwarded), so strict non-reasoning backends never see the nonstandard field, and attachReasoning payloads now spend the same newest-first 100k replay budget as tool history, falling back to the flat shape when exhausted. The in-loop echo remains ungated because it only fires when the model emitted reasoning in that same turn. The harness gains tool-less scenarios (flat vs reasoning_content): across the first 10 router models, no model that accepts the flat shape rejects the reasoning shape.
gary149
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56382cfe68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
| } | ||
| } | ||
| replayed.push(finalMessage); |
There was a problem hiding this comment.
When a completed MCP turn is reused in a later request, every reconstructed assistant message containing tool_calls is emitted without reasoning, while this final append places all extracted reasoning_content after every tool result. That differs from the live shape in runMcpFlow, where reasoning is attached to the assistant tool-call message; for multi-round histories, preserved-thinking models therefore receive the reasoning at the wrong conversational boundary, defeating the cross-turn preservation this replay is intended to provide. Associate each reasoning segment with its corresponding replayed tool-call message rather than only the final answer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The recovered reasoning is attached to the final message because that is where it actually came from. message.content persists only the last loop iteration's text (FinalAnswer is the last round's content), so the think blocks extracted here were produced together with the final answer, after the tool results. Per-round reasoning is never persisted (in-loop stream updates are compressed to lengths in storage), so there is no reasoning segment available to associate with the replayed tool-call messages; moving this one onto them would misattribute it to the wrong boundary in the other direction. Within a live turn, the in-loop echo in runMcpFlow keeps each round's reasoning on its own tool-call message, which is the case the preserved-thinking docs require. Added a code comment in c04f2e9 making this explicit.
Only the last loop iteration's text is persisted, so the recovered reasoning belongs with the final answer; per-round reasoning is never stored and the live in-loop echo covers that case instead.
When the newest turn alone exceeded the 100k replay budget it fell back to flat without consuming the budget, so an older, smaller turn could still expand: rich tool history for a stale turn while the turn being continued from was plain prose, inverting the newest-first invariant. Once any turn falls back to flat, every older turn now does too, with a regression test where the newest turn alone exceeds the budget.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f773dd3ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const finalMessage: AssistantReplayMessage = { | ||
| role: "assistant", | ||
| content: visible, | ||
| ...(includeReasoning && reasoning.length > 0 ? { reasoning_content: reasoning } : {}), |
There was a problem hiding this comment.
When a provider streams reasoning before its first tool_calls delta, this reasoning did not originate from the final assistant message. Fresh evidence contrary to the prior thread is that runMcpFlow.ts:604-608 emits such reasoning while sawToolCall is still false, and the finalizer at src/routes/conversation/[id]/+server.ts:543-563 preserves that pre-tool stream once tool updates exist. Moving every recovered <think> block onto this final message therefore places tool-round reasoning at the wrong boundary on the next turn; preserved-thinking models need each segment associated with the following replayed tool-call message.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right, and this corrects my earlier reply on the other thread: I claimed content persists only the last loop iteration's text, but the FinalAnswer handler merges the pre-tool stream into content when tool updates exist, so recovered think blocks can indeed belong to earlier tool rounds. Exact per-segment attribution is still not recoverable from old messages (stream updates are compressed to lengths, so there are no offsets tying think blocks to round boundaries), so the fix in d7fcbfb makes it recoverable going forward: the live loop now persists each round's reasoning on the round's first Call update (optional field, additive, no migration), and replay re-attaches it to that round's assistant tool-call message, deduping it out of the final message's blocks. Messages persisted before the field existed keep the previous behavior of carrying recovered reasoning on the final message, which is bounded imprecision rather than data loss.
| let messagesOpenAI: OpenAI.Chat.Completions.ChatCompletionMessageParam[] = | ||
| await prepareMessagesWithFiles(messages, imageProcessor, isMultimodal ?? model.multimodal); | ||
| await prepareMessagesWithFiles(messages, imageProcessor, isMultimodal ?? model.multimodal, { | ||
| attachReasoning: Boolean(model.supportsReasoning), |
There was a problem hiding this comment.
On self-hosted installations, reasoningOverrides[model.id] can explicitly enable reasoning for a model whose static supportsReasoning flag is false, and src/routes/conversation/[id]/+server.ts:700-705 consequently forwards the selected reasoning_effort. This call nevertheless disables attachReasoning solely from the static flag, so those enabled models receive past <think> text inline rather than the required reasoning_content, defeating the preservation behavior for the exact force-enabled configuration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The gate now honors the per-user override in both directions: reasoningOverrides[model.id] is threaded through both flows alongside reasoning_effort (new reasoningOverride context field) and wins over the static supportsReasoning flag, which remains the fallback when no override is set. A force-enabled model on self-hosted now gets reasoning_content history, and a force-disabled flagged model stops receiving it. Fixed in d7fcbfb.
…override Review follow-ups on the replay shape: The FinalAnswer handler merges the pre-tool stream into content when tools ran, so recovered think blocks can belong to earlier tool rounds, not just the final answer. The live loop now persists each round's reasoning on the round's first Call update (optional field, absent on old messages), and replay re-attaches it to that round's assistant tool-call message, deduping it out of the final message's blocks. Old conversations keep the previous behavior of attaching recovered reasoning to the final message. The cross-turn reasoning gate previously read only the static supportsReasoning flag, while reasoning_effort forwarding also honors the per-user reasoningOverrides setting, which can force-enable a model on self-hosted installs or force-disable a flagged one. The override is now threaded through both flows and wins in both directions, with the capability flag as fallback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7fcbfb473
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| replayed.push({ | ||
| role: "assistant", | ||
| tool_calls: callsInRound.map((u) => ({ | ||
| id: idByUuid.get(u.uuid) ?? u.uuid, | ||
| type: "function" as const, |
There was a problem hiding this comment.
When a provider streams visible text before emitting tool_calls (for example, “I'll check that”), runMcpFlow.ts:607-613 persists that text before the Call update and the finalizer in +server.ts:547-563 retains it in message.content. Replay nevertheless omits content from this tool-call assistant message and later puts the entire visible string on the final assistant message, moving the preamble from before tool execution to after the tool results. Partition the persisted stream using its update ordering and attach the pre-call portion here so future turns receive the same conversational boundaries as the live request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and traced precisely: message.content, once tools ran, is the model's own streamed tokens only (round preamble(s) + final answer, chronologically concatenated by the FinalAnswer merge in +server.ts), never the tool output text. Replay had no per-round slot to reclaim a preamble from, so it all landed on the final message after the tool results. Fixed in 2040703: the live loop now persists each round's pre-tool visible text on the round's first Call update (new optional content field on MessageToolCallUpdate, mirroring the existing reasoning field), and replay re-attaches it to that round's assistant message, removing it from the final message the same way round reasoning already is (prefix-match since content order is strictly chronological, with an indexOf fallback for defensiveness). Messages persisted before this field existed keep the old behavior of the preamble landing on the final message.
Some OpenAI-compatible providers stream reasoning under reasoning_text rather than reasoning or reasoning_content. Read it in the MCP loop, in both stream adapters of the plain flow, and in the harness.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24ec859226
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const speedRuns = okRuns.filter((r) => r.ttftMs !== undefined && r.genTokens > 1); | ||
| stats.push({ | ||
| scenario: scenario.name, | ||
| ok: okRuns.length > 0, |
There was a problem hiding this comment.
When one of the two repetitions rejects a scenario but the other succeeds—for example, because router requests reach provider variants with different schema validation—okRuns.length > 0 marks the scenario as accepted. The regression check can therefore print SHIPPABLE even though the new payload fails for half of the sampled requests; require all repetitions to succeed, or report partial failures separately rather than treating any success as compatibility.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, this was too lenient. Fixed in 2040703: a scenario now requires every repetition to pass (okCount === repCount) to count as ok; a partial pass prints as FLAKY(n/total) instead of being rounded up to OK. The SHIPPABLE gate already read off this same .ok field, so tightening it here automatically tightens the ship criterion without further changes.
Vendor research across the top router models: DeepSeek documents a hard 400 when reasoning_content is not passed back on tool-call turns (Flash was the only Required-verdict model missing the flag; Pro already had it), and the Qwen3.6 generation is trained for preserve_thinking with documented accuracy loss in tool flows when reasoning is omitted. gemma-4 stays unflagged deliberately: Google requires stripping thoughts across completed turns.
The flag now controls the cross-turn reasoning_content echo in addition to the thinking-effort dropdown, so the skill's flagging decision needs the vendor's preserved-thinking guidance: flag models whose vendors require or recommend passing reasoning back (Kimi, MiniMax, DeepSeek V4, GLM, Qwen3.6), and never flag models whose vendors require stripping historical thoughts (Gemma family), even when they accept an effort knob.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1c5f1dc23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| resolved[i] = entry.replay; | ||
| } else { | ||
| exhausted = true; | ||
| resolved[i] = [entry.flat]; |
There was a problem hiding this comment.
When cumulative replay exceeds 100,000 characters, this fallback restores the original flat message, including any inline <think> blocks. This bypasses the capability gate: preserved-thinking models lose reasoning_content, while models such as Gemma that require historical thoughts to be stripped receive those thoughts inline. Use a reasoning-stripped flat fallback rather than entry.flat.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. entry.flat was the raw message.content, so a budget cutoff would leak the original unstripped block as visible content regardless of which model it went to, including models like Gemma whose vendor requires historical thoughts stripped unconditionally. Fixed in 2040703: the fallback is now -stripped (a new stripThink helper) in both the tool-replay and reasoning-only branches, so degrading under budget pressure only ever means losing the enrichment (tool_calls/reasoning_content), never leaking raw reasoning that shouldn't be there. Added a test with two ~60k-reasoning turns forcing the older one to fall back, asserting no raw tag and no reasoning_content on the fallback message.
| } | ||
| ); | ||
| const assistantToolMessage: ChatCompletionMessageParam = { | ||
| const reasoningForToolMsg = thinkParts.join("\n").trim(); |
There was a problem hiding this comment.
When a provider's reasoning begins or ends with whitespace, this trim() changes the value before it is echoed as reasoning_content and persisted for later replay. Providers that require the complete, unmodified reasoning payload can therefore reject the next tool round or condition on a corrupted trace; use trimming only to test whether the value is empty, not to construct the echoed value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 2040703, on both ends: runMcpFlow's in-loop echo (thinkParts.join("\n").trim()) and prepareFiles' splitReasoning/replay lookup were both trimming the value that gets echoed and persisted, not just testing it for emptiness. trim() is now only used as a truthiness check; the joined/persisted/echoed value keeps its exact bytes, with whitespace-only parts still filtered out of the parts array so nothing empty gets echoed. Added a test with deliberate leading/trailing spaces inside a block asserting the exact string survives in reasoning_content while a separate whitespace-only block is dropped.
…k leak Four review follow-ups on the replay shape: Reasoning was trimmed before being echoed and persisted, both in the live loop (runMcpFlow) and in cross-turn replay (splitReasoning). Vendors documenting preserved thinking can require the payload sent back unmodified or use it for cache matching, so trimming must only decide whether a value counts as empty, never change what gets echoed. Whitespace-only parts are still filtered out; surviving parts keep their exact bytes. Visible preamble text streamed before a round's tool calls (e.g. "Let me check that.") was being moved onto the final answer on replay, after the tool results, since only reasoning had a per-round slot to reclaim it from. The live loop now persists each round's preamble on the round's first Call update (optional content field, mirroring the existing reasoning field), and replay re-attaches it to that round's message, deduping it out of the final message the same way reasoning already was. The replay-budget fallback used the raw message.content when a turn didn't fit, which defeats the point for models whose vendor requires historical thoughts stripped (e.g. Gemma): they'd get raw <think> text back regardless of budget, on top of never losing anything for preserved-thinking models either. The fallback is now the <think>-stripped shape in both the tool-replay and reasoning-only paths. Also tightened scripts/reasoning-replay-harness.ts: a scenario counted as compatible if any single repetition succeeded, silently rounding up a real half-failure rate. It now requires every repetition to pass and reports partial failures as FLAKY(n/total) instead of OK. 6 new unit tests (16 total in prepareFiles.spec.ts, up from 10); full server/SSR suite: 518/519 pass, the one failure is the pre-existing CORS test unrelated to this branch.
…tream The pre-tool preamble was persisted untrimmed while replay compares it against trim-normalized visible text, so a preamble starting with newlines (the common case after a think block) failed the dedup match and replayed twice. Visible text is now trimmed on both sides; reasoning stays byte-exact. Whitespace-only reasoning deltas were dropped from the think merge even mid-block, losing paragraph breaks from the echoed trace. They are now appended whenever a think block is already open; non-blank text is still required to open one, so stray whitespace cannot create empty blocks.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 120ffdb1bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // (paragraph breaks are part of the byte-exact trace); non-blank | ||
| // text is only required to OPEN a block, so stray leading | ||
| // whitespace can't create empty think blocks. | ||
| if (deltaReasoning.length > 0 && (thinkOpen || deltaReasoning.trim().length > 0)) { |
There was a problem hiding this comment.
When a provider streams leading reasoning whitespace as its own delta before the first nonblank reasoning delta, this guard drops that chunk because thinkOpen is still false, so the later reasoningForToolMsg no longer contains the complete byte-exact trace required for replay. Fresh evidence beyond the prior trimming fix is this chunk-level condition, which still discards whitespace solely because it arrived in a separate initial chunk; buffer such chunks until the first nonblank reasoning delta instead of dropping them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, and a sharper catch than the earlier trim fix covered: a leading whitespace-only reasoning delta (thinkOpen still false, so it couldn't satisfy the non-blank-to-open condition) was being dropped outright rather than just deferred. Fixed in 64425f0 by buffering whitespace-only deltas in a pendingReasoningWhitespace accumulator and flushing it into the block once a non-blank delta opens it, so those bytes survive into the persisted trace instead of vanishing.
…aw tool args
External review (discussed with Codex) confirmed the design and flagged
concrete gaps. This addresses the five agreed pre-merge items, then three
more found by /review on the resulting diff, then one final read-boundary
hardening Codex caught on a second pass.
Five agreed items:
- Plain flow (endpointOai.ts) previously fell through to raw
message.content when attachReasoning was false, leaking inline <think>
text to models like Gemma whose vendor requires historical thoughts
stripped. Now always routes through splitReasoning first.
- Cross-turn reasoning_content is now gated on message.routerMetadata.model
matching the current turn's resolved model: under the "omni" router
alias, per-message routing can mix producers within one conversation
with no user action, and reasoning is conditioned on its own producer.
Tool call/result replay stays unconditional; only reasoning_content is
gated. Applies in both the MCP flow (candidateModelId) and the plain
flow (model.id, already resolved by the time endpointOai.ts runs,
whether invoked directly or as a router candidate).
- Original provider tool-call id and raw JSON arguments string are now
persisted per call (originalId, argumentsRaw) and used on replay:
arguments prefer the raw string over reserializing sanitized primitive
parameters, which could under-represent nested objects/arrays down to
{}. tool_call_id stays unconditionally the normalized one regardless.
- scripts/reasoning-replay-harness.ts adds a semantic proof pair
(N1-nonce-flat/N2-nonce-replay): a fabricated tool result carries a fact
absent from visible content, so only real replay can answer the
follow-up. Shape acceptance alone was previously the only signal.
- Harness pinned to the models this PR's vendor research found a
documented preservation policy for, plus two controls, replacing an
unstable "first N from /models" sample.
Three more from /review:
- A malformed/truncated arguments string from the model was persisted
unconditionally; replay would then prefer that invalid JSON over the
valid sanitized fallback, risking a 400 that kills the whole
continuation on providers that validate the field. Added
isValidJsonObject (rejects malformed JSON and non-object JSON alike)
gating persistence at write time.
- The pinned Gemma/Llama controls were tested against a forced
attachReasoning:true shape production never actually sends them (both
are correctly unflagged), proving provider tolerance instead of policy
correctness. buildScenarios is now parameterized per model's real
supportsReasoning flag for the cross-turn scenarios; S3-inloop stays
unconditioned since the in-loop echo it models is evidence-based and
ungated in production for every model.
- The nonce semantic gate could be masked: if one rep succeeded but
lacked the nonce while another rep transport-failed, the scenario's
overall ok flipped false and suppressed the real semantic-failure
signal from the first rep. Gate is now independent of overall ok.
Debugged during harness verification, not review findings: the first
nonce run showed Kimi-K3 and both Qwen3.6 models failing. Kimi-K3's own
reasoning_content explicitly quoted the tool result's nonce, then refused
to repeat it — the fixture named the field "internal_reference", which
its safety tuning read as "not for the user"; renamed to "station_id".
The Qwen models hit finish_reason:"length" — 120 max_tokens was too
tight once a model reasons about a lookup before answering; raised to
400. Neither was a replay failure.
One more from a second Codex pass: write-time validation only protects
this one write path going forward. Replay now independently validates
argumentsRaw at its own read boundary before trusting it, falling back
to sanitized parameters otherwise.
31 unit tests (28 in prepareFiles.spec.ts, 3 new in
toolInvocation.spec.ts), typecheck and lint clean. Harness re-verified
live against the router: 0 semantic failures across all 10 pinned
models including Kimi-K3, with payload sizes now visibly differing by
supportsReasoning, confirming the per-model gate engages.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2530e4f184
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const producer = message.routerMetadata?.model; | ||
| return !producer || !currentProducerModel || producer === currentProducerModel; |
There was a problem hiding this comment.
When a conversation's pinned model is removed, ModelSwitch.svelte lets the user PATCH the conversation to another model, but historical messages from the old pinned model have no routerMetadata.model. This predicate therefore treats them as same-producer and can attach the retired model's reasoning to the replacement model, potentially corrupting its continuation or causing a strict preserved-thinking backend to reject it. Missing producer metadata is not proof of a match after a model switch, so suppress or clear historical reasoning in that flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 64425f0. Traced it to the PATCH /conversation/:id handler (the 'this model is no longer available' recovery banner): it only updates conv.model and never touches historical messages, so after a switch, old messages from the retired model still have no routerMetadata.model (that field is only ever stamped for the omni router alias, never for a pinned single model) - meaning the producer gate's '!producer = same producer' default, which is correct for the common pinned-model case, was silently wrong specifically in this switch flow. Fix: when the PATCH detects an actual model change, it now backfills routerMetadata.model with the retiring model's id onto every assistant message that doesn't already carry producer metadata, before the new model is set. That gives the gate something real to compare against going forward.
| const regressed = Object.entries(FAMILIES).some(([base, dependents]) => { | ||
| const baseStat = stats.find((s) => s.scenario === base); | ||
| return Boolean( | ||
| baseStat?.ok && dependents.some((name) => !stats.find((s) => s.scenario === name)?.ok) | ||
| ); |
There was a problem hiding this comment.
When a replay scenario returns HTTP success but fails its expected answer checks (for example, omits 18 or 80), runOne records coherent: false, yet this regression predicate compares only ok. The model is consequently shown as OK(incoherent) and the harness can still print SHIPPABLE, allowing a semantically degraded replay payload to pass the experiment; gate dependent scenarios on coherence when the coherent baseline passes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and this was a real gap independent of the N1/N2 nonce work: coherent was computed and displayed but never gated anything. Fixed in 64425f0 - the regression check now also flags a dependent scenario that succeeded at the HTTP level but answered incoherently, when the baseline it's compared against was itself coherent (an already-incoherent baseline is excluded, since it says nothing about whether replay made things worse).
| const availableIds = new Set((modelsJson.data ?? []).map((m) => m.id)); | ||
| const models = PINNED_MODELS.filter((id) => availableIds.has(id)); |
There was a problem hiding this comment.
When /models contains none of the pinned IDs, such as after model churn or against a misconfigured endpoint, this filter produces an empty cohort. The subsequent run has zero regressions and zero semantic failures, so the script prints SHIPPABLE and exits successfully without sending a single test request; treat models.length === 0 as an indeterminate/failing run instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 64425f0 - the harness now exits 1 with an explicit error if every pinned model is absent from /models, instead of falling through to zero regressions/zero semantic failures and printing SHIPPABLE with no requests ever sent.
… model switch, harness gating gaps Buffer leading whitespace-only reasoning deltas until a non-blank one opens the think block, instead of dropping them: a provider that streams whitespace before its first real reasoning token was silently losing those bytes from the persisted trace. Backfill routerMetadata.model with the retiring model's id on every assistant message lacking producer metadata when a conversation's model is switched (the "this model is no longer available" recovery flow). Without it, the cross-producer reasoning gate defaulted those messages to same-producer as the newly selected model and could attach the old model's reasoning_content to a turn it never produced. Harness: gate scenario regression on coherence too, not just HTTP success, when the baseline it's compared against was itself coherent - a request that succeeds but answers worse than the baseline it replaced is a real regression the exit code should catch. Also fail loudly on an empty compatibility cohort (all pinned models absent from the router) instead of vacuously printing SHIPPABLE with zero requests sent.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64425f07cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
| } | ||
| } | ||
| replayed.push(buildFinalMessage()); |
There was a problem hiding this comment.
When an MCP turn is aborted before producing final model text, the persisted updates can contain calls/results while remainingVisible and remainingParts are both empty. This unconditional append then inserts { role: "assistant", content: "" } between the tool result and the next user message, even though no such assistant response occurred; strict providers may reject the empty assistant message on the next request. Only append the final message when it contains visible content or attached reasoning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in d8a9700. buildFinalMessage now returns null when there's no visible content and no reasoning to attach, and both call sites (the no-tool-calls early return and the end-of-rounds push) skip appending it in that case, instead of a bare {role: assistant, content: ""} trailing an interrupted tool round. Added regression tests for both the tool-turn and plain-message cases.
| const nonceScenario = stats.find((s) => s.scenario === "N2-nonce-replay"); | ||
| const semanticFailure = nonceScenario?.nonceOk === false; |
There was a problem hiding this comment.
When every N2-nonce-replay repetition times out or returns an HTTP error, nonceOk is undefined, so this comparison reports no semantic failure. Because N2 is also absent from FAMILIES, the model is printed as green and the harness can exit SHIPPABLE while its sole semantic replay proof is logged as FAIL; partial failure with a successful nonce-bearing repetition has the same problem. Require the nonce scenario itself to be fully successful as well as nonce-correct.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real gap, fixed in d8a9700. nonceOk is undefined (not false) when every rep of N2-nonce-replay times out or errors, since nonceRuns is empty in that case - the gate now also requires the scenario to have succeeded overall (nonceScenario.ok), so a total failure of the harness's sole semantic-proof scenario can no longer read as a clean pass.
…equest itself fails
A turn interrupted before producing any final text or reasoning (e.g.
aborted mid-tool-call) replayed as a trailing {role: "assistant",
content: ""} with nothing else attached — an assistant turn that never
happened, which strict providers can reject outright. buildFinalMessage
now returns null in that case and both call sites skip appending it.
Harness: if every N2-nonce-replay repetition times out or errors,
nonceOk is undefined rather than false, which previously read as no
semantic failure — the run could print SHIPPABLE with its sole semantic
proof scenario reduced to noise. The scenario must now also have
succeeded overall to count as passed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8a97004da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (let i = remainingParts.length - 1; i >= 0; i -= 1) { | ||
| if (roundReasoning.includes(remainingParts[i])) { | ||
| remainingParts.splice(i, 1); |
There was a problem hiding this comment.
When persisted round reasoning is not an exact element of remainingParts—for example, several <think> fragments were joined or the round reasoning arrived alongside tool_calls and was never streamed into message.content—this fallback removes every remaining part that is merely a substring. Thus a final reasoning block such as weather is silently deleted when the earlier round trace was Need weather; only remove fragments that can be attributed positionally to that round.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real and a good catch - reproduced exactly your example shape in a test (round reasoning 'Need weather forecast', extracted parts 'Need weather' and 'weather', the latter belonging to the final answer). Fixed in 05aa616: the fallback no longer scans the whole array: parts are chronologically ordered and rounds are processed oldest-first, so only remainingParts[0] (the earliest still-unconsumed part) can be attributed to the current round when exact match fails. A later part being a substring is now correctly treated as coincidence, not evidence, and survives.
| if (!wantsReasoning || reasoning.length === 0) { | ||
| // Either nothing to attach, or attachment is disabled/gated: | ||
| // either way `visible` (think-stripped) is the correct shape, | ||
| // never the raw `message.content`. | ||
| return [{ role: "assistant", content: visible }]; |
There was a problem hiding this comment.
When a turn is interrupted before producing visible text and reasoning attachment is disabled or producer-gated, splitReasoning leaves visible empty and this branch still emits { role: "assistant", content: "" } before the next user message. Fresh evidence beyond the existing tool-replay finding is that the non-replay/plain flow remains unconditional here, so unflagged models and cross-producer histories can receive a phantom empty assistant turn that strict providers may reject; omit the message when neither visible content nor attached reasoning survives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, same phantom-empty-message shape as the tool-replay fix, just in the plain flow's unconditional {role: assistant, content: visible} return. Fixed in 05aa616: omitted entirely when visible is also empty, so a turn interrupted before any visible text with reasoning gated off or producer-mismatched no longer replays as a bare empty assistant message.
| coherent: okRuns.find((r) => r.coherent !== undefined)?.coherent, | ||
| nonceOk: nonceRuns.length > 0 ? nonceRuns.every((r) => r.nonceOk) : undefined, |
There was a problem hiding this comment.
When the first successful repetition is coherent but a later successful repetition is not, find records only the first true, so the regression gate can still print SHIPPABLE despite a sampled semantic failure. Fresh evidence after the earlier coherence-gate fix is this aggregation step: compute coherence across all successful repetitions, just as nonceOk does, rather than selecting one run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. Coherence is now aggregated across every judged successful repetition, mirroring nonceOk: any incoherent successful rep marks the scenario incoherent, so a sampled semantic failure can no longer be masked by an earlier good rep. Fixed in 7e0c7a2.
… turns in the plain flow too
The substring fallback for matching a round's persisted reasoning
against the extracted <think> parts scanned the entire remainingParts
array and deleted every part merely containing it as a substring. A
short, unrelated final reasoning block (e.g. "weather") could be
silently deleted just because an earlier round's mismatched reasoning
happened to contain it as a substring (e.g. "Need weather forecast").
Parts are chronologically ordered and rounds are processed oldest-first,
so only the earliest still-unconsumed part can be attributed to the
current round when exact match fails; the fallback no longer scans past
it.
The plain (non-tool-replay) reasoning-attachment branch had the same
phantom-empty-message issue already fixed on the tool-replay path: a
turn interrupted before any visible text, with reasoning gated off or
producer-mismatched, replayed as {role: assistant, content: ""} with
nothing else attached. Omitted entirely in that case, matching the
tool-replay fix.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05aa616202
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| exhausted = true; | ||
| resolved[i] = [entry.flat]; | ||
| } |
There was a problem hiding this comment.
When newer replay candidates exhaust the 100,000-character budget, an older interrupted tool turn or reasoning-only turn can have entry.flat.content === ""; this fallback still emits it as { role: "assistant", content: "" }. The normal replay and plain branches now omit these phantom turns, but this remaining path bypasses both guards, so long histories can still send an empty assistant message that strict providers reject. Skip the fallback entry when its stripped content is empty.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The budget-exhaustion fallback now applies the same phantom-turn guard as the replay and plain branches: when the flat candidate's stripped content is empty, the turn is omitted entirely instead of emitted as an empty assistant message. Fixed in 7e0c7a2 with a regression test (interrupted empty turn behind a budget-exhausting newer turn).
| if (remainingVisible.startsWith(roundContent)) { | ||
| remainingVisible = remainingVisible.slice(roundContent.length); | ||
| } else { | ||
| const idx = remainingVisible.indexOf(roundContent); | ||
| if (idx !== -1) { | ||
| remainingVisible = | ||
| remainingVisible.slice(0, idx) + remainingVisible.slice(idx + roundContent.length); |
There was a problem hiding this comment.
When a provider emits visible content in the same delta as its first tool_calls entry, runMcpFlow records that text in the Call update but does not stream it into message.content because sawToolCall is already true. If the final answer later contains the same text, this unconditional search removes it from remainingVisible and moves it before tool execution, corrupting the replayed conversational order; content arriving after the first tool delta can cause the same ambiguity. Use the persisted Stream/Call ordering to subtract only text known to have been merged into the stored final content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and thanks for the same-delta suppression evidence. Fixed in 7e0c7a2 by making the dedup prefix-only: rounds consume visible text chronologically, so a streamed preamble is always the next prefix, and a persisted preamble that is not a prefix was never merged into stored content, meaning a deeper indexOf hit could only be final-answer text. The mismatch failure mode is now mild duplication instead of reordering. Regression test added with your exact shape (preamble persisted on the Call update, identical text mid final answer).
A preamble persisted on a Call update but never merged into stored content (it arrived in the same delta as the first tool_calls entry) is not a prefix of the visible text; the indexOf fallback could match identical text belonging to the final answer and pull it before the tools, reordering the conversation. Dedup is now prefix-only, trading that corruption for mild duplication in the mismatch case. The budget-exhaustion fallback gets the same phantom-turn guard as the replay and plain branches: an interrupted turn whose stripped content is empty is omitted instead of sent as an empty assistant message. Harness coherence now aggregates across every judged successful rep like nonceOk, so one incoherent sample fails the gate even when another rep answered well.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e0c7a2490
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await collections.conversations.updateOne( | ||
| { _id: convId }, | ||
| { $set: { ...updateValues, messages: messagesForSave } } | ||
| ); |
There was a problem hiding this comment.
When a model switch overlaps an active generation or another request that updates the conversation, this writes the messages snapshot read before the switch back over the entire array. Any message or final generation update persisted between findOne and this updateOne is therefore lost; apply the metadata backfill atomically in MongoDB rather than replacing messages with a stale client-side copy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This still appears unresolved at 7e0c7a24. messagesForSave is derived from the earlier findOne snapshot and then replaces the complete messages array. Any generation-writer or concurrent request update between those operations can be overwritten. The producer-metadata backfill should be performed atomically in MongoDB, for example with an update pipeline that maps the current server-side array.
| const coherent = | ||
| content.length > 0 ? scenario.expect.every((re) => re.test(content)) : undefined; |
There was a problem hiding this comment.
When a repetition streams only reasoning or returns tool_calls without final text, runOne marks it successful but assigns coherent: undefined. The new aggregation treats that value as passing, so S2/S3 or P2 can still satisfy the regression gate despite never producing the expected answer; for scenarios with expect checks, an empty final content should be incoherent rather than unjudged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This still appears unresolved at 7e0c7a24. A reasoning-only or tool-calls-only response sets coherent to undefined, and the aggregation only treats false as failure. For any scenario with non-empty expect, an empty final content should be explicitly incoherent so the dependent scenario cannot pass without producing the expected answer.
pngwn
left a comment
pngwn
left a comment
There was a problem hiding this comment.
Two additional findings from the latest re-review.
| resolved[i] = entry; | ||
| continue; | ||
| } | ||
| const cost = JSON.stringify(entry.replay).length; |
There was a problem hiding this comment.
[P2] Budget the complete outgoing history
The budget is decremented only for ReplayCandidate entries. System messages, user messages, multimodal content, and ordinary assistant messages pass through without being counted, so a large existing history can still receive another 100k characters of replay and overflow a context window it previously fit. Please budget the complete resolved history—or explicitly budget only the incremental expansion against the model’s remaining context—rather than treating this as a cap on the whole request.
| { "id": "MiniMaxAI/MiniMax-M3", "description": "Natively multimodal 428B MoE with 1M context for frontier coding and agents." , "supportsReasoning": true, "supportsArtifacts": true, "parameters": { "max_tokens": 98304 } }, | ||
| { "id": "nvidia/NVIDIA-Nemotron-3-Ultra-550B-A55B-NVFP4", "description": "Hybrid Mamba-Transformer 550B MoE with 55B active params for efficient agentic reasoning.", "parameters": { "max_tokens": 98304 }, "supportsReasoning": true, "supportsArtifacts": true }, | ||
| { "id": "Qwen/Qwen3.6-27B", "description": "Dense 27B hybrid with DeltaNet attention and 1M context.", "supportsArtifacts": true, "parameters": { "max_tokens": 49152 } }, | ||
| { "id": "Qwen/Qwen3.6-27B", "description": "Dense 27B hybrid with DeltaNet attention and 1M context.", "supportsReasoning": true, "supportsArtifacts": true, "parameters": { "max_tokens": 49152 } }, |
There was a problem hiding this comment.
[P2] Separate preserved-thinking support from reasoning effort
supportsReasoning currently enables both historical reasoning_content replay and the low/medium/high effort control. Qwen3.6 documents preserved thinking through preserve_thinking, while the current Qwen/DashScope interface exposes only Boolean enable_thinking, not effort tiers. Marking these models supportsReasoning: true therefore exposes a misleading control and can send an unsupported or ineffective reasoning_effort value. Please split replay/preservation capability from effort-tier capability and wire Qwen’s provider-specific preservation option separately.
2 participants
Problem
In multi-turn tool conversations, past tool calls and results are never sent back to the model. History is flattened to plain
{role, content}, so on the next turn the model sees no evidence of the tools it just used. This produces conversations where the model runs a tool successfully, then insists one message later that it has no such capability ("no sandbox is exposed to me in this conversation"), because its only visible history is its own denial.There is also a model-specific issue stacked on top: preserved-thinking models (the Kimi K2/K3 family) require prior
reasoning_contentto be echoed back on assistant messages during multi-step tool calling. Moonshot documents this in their thinking-models guide and implements it in their official kimi-cli harness; several clients hit real bugs from dropping it (BerriAI/litellm#26156, open-webui/open-webui#23175). chat-ui stripped reasoning in two places: inside the tool loop (the<think>strip before each follow-up round) and across turns (history flattening).Changes
Inside the MCP loop (
runMcpFlow.ts): the<think>content of the previous round is now re-attached asreasoning_contenton the assistant tool-call message instead of being dropped. Thecontentfield is omitted entirely when nothing visible remains, since some OpenAI-compatible backends reject empty text next totool_callswith a 400 (kimi-cli guards against the same thing).Across turns (
prepareFiles.ts):prepareMessagesWithFilesgains an opt-inreplayToolHistoryused only by the MCP flow. Past assistant turns are rebuilt from their persisted tool updates into real assistant/tool message pairs, grouped back into per-round batches, with reasoning re-attached asreasoning_contentinstead of inline<think>text.Plain (tool-less) flow (
endpointOai.ts): the same reasoning preservation, without tool replay since this path never declares tools. Past assistant turns carryreasoning_content(merged from inline<think>blocks and the separately persistedmessage.reasoning) while staying flat otherwise. Historical<think>text is now always stripped from outgoing content regardless of whether reasoning_content is attached: an earlier version only stripped it inside the replay path, so a model withattachReasoningoff (Gemma, whose vendor requires stripping historical thoughts) could still receive raw inline<think>markup through this plain flow. Two guards from review: the cross-turn reasoning echo is gated on the model'ssupportsReasoningflag in both flows (matching howreasoning_effortis forwarded), so strict non-reasoning backends never see the nonstandard field, and reasoning payloads spend the same newest-first 100k budget as tool replay, falling back to the untouched flat shape when exhausted. The in-loop echo stays ungated because it only fires when the model emitted reasoning in that same turn.Cross-producer guard for the "omni" router alias: under
omni, each turn can be routed to a different model with no user action (per-message routing, recorded inmessage.routerMetadata.model). Reasoning is conditioned on the producing model's own prior thoughts, soreasoning_contentis now only replayed for a historical message whose recorded producer matches the model resolved for the current turn; messages with norouterMetadata(the common pinned-model case) are unaffected. Tool call/result replay is protocol-neutral and is never gated by this — only thereasoning_contentfield is.Robustness details:
tool_call_ids are derived from the persisted update uuids and normalized to nine alphanumeric characters, the shape Mistral-family chat templates require and every other provider accepts. This id is emitted unconditionally, even for calls whose original provider-issued id was also persisted (see below) — one code path, no branching on producer/provider.originalId,argumentsRaw) and used on replay: arguments prefer the raw string the model actually sent over reserializing the sanitized primitive parameters, which previously could under-represent nested objects, arrays, or file references down to{}. Messages persisted before these fields existed still fall back to the sanitized reconstruction.Which top router models need this
Vendor-documentation research across the current top 15 router models (API docs, model cards, and shipped chat templates):
reasoning_content, must be passed back (docs)thinking.keepis forced to"all"and cannot be disabled (docs)reasoning_contentis not passed back on tool-call turns (docs)preserve_thinking; cloud docs say omitting reasoning in tool flows degrades accuracyreasoning_content; no retention policy documentedreasoning_content; no retention policy documentedSix of the top 15 require this outright, seven counting gemma-4's in-loop mandate, and two of those (DeepSeek-V4) hard-error on their first-party API without it. gemma-4's split requirement maps onto this PR's design directly: the in-loop echo fires only when the model just emitted reasoning (satisfying "do not strip between function calls"), while the capability gate keeps
reasoning_contentaway from unflagged models across turns (satisfying "strip across completed turns"), and replay removes the inline think text the old flat shape used to leak into history.This PR also flags
supportsReasoningin the HuggingChat model config fordeepseek-ai/DeepSeek-V4-Flash(the one Required-verdict model that was missing it) and the two Qwen3.6 models (Recommended tier), in both prod and dev. gemma-4 stays unflagged deliberately, per Google's strip-across-turns requirement.Verification
<think>stripping when reasoning is disabled, and raw-argument/original-id replay with legacy fallback.scripts/reasoning-replay-harness.tsno longer samples "the first N models from/models" (an unstable population that says nothing about the models this PR actually targets). It's pinned to the models this PR's vendor research found a documented preservation policy for — Kimi-K3, Kimi-K2.7-Code, MiniMax-M3, both DeepSeek-V4 models, GLM-5.2, both Qwen3.6 models — plus two controls, gemma-4-31B-it (must NOT gain cross-turn reasoning, per Google's strip-across-turns requirement) and Llama-3.1-8B-Instruct (no reasoning mechanism at all).reasoning_contentexplicitly quoted the tool result's nonce, then answered correctly with it).Known limitations: per-round
reasoning/content/originalId/argumentsRaware only persisted for turns generated by this branch going forward, so a historical turn from before this PR still has its recovered reasoning attributed to the final message rather than its own tool round, and its tool-call arguments fall back to the sanitized primitive reconstruction rather than the exact JSON the model sent. Strict first-party APIs that validate reasoning shape on historical tool messages (DeepSeek's own endpoint) could in principle reject such a legacy turn, though every router-served provider accepts the shape per the harness. Separately, this PR replays reasoning as a plainreasoning_contentstring; it does not yet forward first-party preservation toggles some vendors also expect (Qwen'spreserve_thinking, Z.ai'sclear_thinking:false), nor structured/opaque reasoning representations some vendors use instead of a string (MiniMax'sreasoning_details) — the router is the only deployment surface this PR targets, and sending the field back is the portable half of each vendor's requirement under it.