🧯 fix: Keep Faded Completed Tool Calls From Reading as Truncated - #564
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 38be268161
ℹ️ 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 if (hasExactKeys(keys, LEGACY_ENVELOPE_KEYS)) { | ||
| previewKey = '_truncated'; |
There was a problem hiding this comment.
When a persisted legacy envelope is already shorter than the current cap—which is especially likely after its v1 tier is discarded and the re-derived tier widens—this branch identifies _truncated but projectToolInputWithinLimit then returns the original value unchanged because serialization did not overflow. Consequently, affected histories continue showing the model the misleading _truncated marker and can still trigger the duplicate side effects this change is intended to prevent. Preserve whether the recognized envelope is legacy and force its conversion through createBoundedTruncationValue even when it fits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7851ab6: a recognized legacy envelope always converts to {_note, _originalChars, _inputPrefix}, even when it fits the cap. That covers structured args, serialized function.arguments, and custom-tool string inputs, whose [truncated: N chars] marker is rewritten (the custom-tool length shortcut was folded into the string projection so it no longer skips this). Tests cover all three.
| maxToolExchangeWidth = | ||
| message.getType() === 'human' | ||
| ? 1 |
There was a problem hiding this comment.
When callers provide a LangChain ChatMessage with role: 'user', its type is generic, so this condition does not recognize the new turn boundary. The library accepts and preserves such role-based messages elsewhere, and in this case a reloaded assistant message containing merged calls from an earlier turn remains part of maxToolExchangeWidth; the v2 tier can therefore still over-deepen and latch exactly as before this fix. Treat role === 'user' as a human boundary here as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7851ab6: startsUserTurn treats a generic ChatMessage with role: 'user' as a turn boundary. Covered by a test.
| maxToolExchangeWidth = | ||
| message.getType() === 'human' | ||
| ? 1 |
There was a problem hiding this comment.
When a tool or preemption hook appends SDK-generated context, ToolNode and Graph deliberately encode it as a stamped HumanMessage after the tool results so strict providers accept it. This condition consequently resets maxToolExchangeWidth after an actual parallel tool batch in the same user turn, forgetting the batch width before the next model call; under a tight effective budget, fading then leaves each call/result too large for the aggregate exchange and can force avoidable pruning or orphan repair. Only genuine user-turn messages should reset the width, excluding synthetic provider-context messages.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7851ab6: a HumanMessage stamped additional_kwargs.injected === true (hook context after a tool batch, and steers) does not reset the width; only genuine user turns do. The test checks that a live 8-wide batch followed by injected context keeps its tier.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Line 75 in 7851ab6
Migrate v1 tiers before validating subagent resumes
When resuming a subagent manifest created by the previous release with an informative fadingTier or fadingTiers, this version check now rejects the embedded v1 tier. isGraphResumeState uses isFadingTier for those fields (src/tools/subagent/SubagentReplay.ts:313-317), causing requireValidSubagentResumeManifest to throw Invalid subagent resume manifest and preventing an interrupted run from resuming after an upgrade. Accept v1 in this compatibility boundary and discard or migrate only its fading metadata rather than invalidating the entire replay manifest.
ℹ️ 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".
| typeof value === 'string' && | ||
| value.includes('"_truncated"') && | ||
| value.includes('"_originalChars"') | ||
| value.includes('"_originalChars"') && | ||
| (value.includes('"_inputPrefix"') || value.includes('"_truncated"')) |
There was a problem hiding this comment.
When persisted history uses the supported singular additional_kwargs.function_call representation and its serialized {_truncated, _originalChars} arguments fit the current cap, this new converter is never reached: projectLegacyFunctionCall returns the whole safe object unchanged at src/messages/prune.ts:2030-2042. The model therefore still sees _truncated on a completed call and can repeat the same side effect. Fresh evidence beyond the earlier fixed comment is this distinct singular-function-call fast path; the covered raw tool_calls[].function.arguments path does not exercise it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the next commit: the singular function_call fast path now excludes serialized legacy {_truncated, _originalChars} arguments (isLegacyEnvelopeString), so they fall through to the converter even when they fit. Covered by a test with a fitting legacy function_call.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38e42712a9
ℹ️ 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".
| * have latched on history whose steps storage had merged into one assistant | ||
| * message, so they are discarded and re-derived once. | ||
| */ | ||
| export const FADING_TIER_VERSION = 2; |
There was a problem hiding this comment.
After upgrading from the previous version, resuming a persisted subagent/HITL execution that captured a v1 fadingTier now fails completely: this version makes isFadingTier reject v1, while isGraphResumeState in src/tools/subagent/SubagentReplay.ts:313-317 treats any rejected tier as corruption and requireValidSubagentResumeManifest throws Invalid subagent resume manifest. Instead of invalidating the entire checkpoint, accept the legacy tier during resume parsing and discard or migrate it so the intended one-time re-derivation can occur.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: resume-manifest validation accepts a well-formed pre-v2 tier (isLegacyFadingTier) alongside current ones, so a checkpoint captured before the upgrade is not treated as corrupt. The run config still filters tiers through isFadingTier, so the legacy tier is dropped and re-derives once. Malformed tiers still fail closed. Covered in SubagentReplay.test.ts.
| if (type === 'generic') { | ||
| return (message as { role?: unknown }).role === 'user'; | ||
| } | ||
| return type === 'human' && message.additional_kwargs.injected !== true; |
There was a problem hiding this comment.
When a preemption steer follows a parallel tool batch in the same user turn, replay formatting creates a HumanMessage with additional_kwargs: { role: 'user', source: 'steer' } but no injected flag (src/messages/format.ts:2073-2078), so this condition resets the observed exchange width to 1. Under a tight budget, the fading rung is then calculated as though the preceding parallel batch were narrow. Fresh evidence beyond the earlier fixed comment is this persisted-steer formatting path, which is not exercised by the test using injected: true; exclude the SDK steer/meta markers as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: startsUserTurn also excludes HumanMessages with source: 'steer' (the replayed steer formatting) and isMeta: true, besides injected. Covered by a replayed-steer test that keeps the live batch's tier.
…ers from turn resets
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 637979275a
ℹ️ 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".
| return ( | ||
| kwargs.injected !== true && | ||
| kwargs.source !== 'steer' && | ||
| kwargs.isMeta !== true |
There was a problem hiding this comment.
When a tool hook returns additionalContexts after a parallel batch, src/tools/ToolNode.ts:4868-4871 appends a HumanMessage stamped { role: 'system', source: 'hook' }; none of these conditions excludes it, so the message resets maxToolExchangeWidth to 1 before the next model call. Under a tight budget this forgets the real batch width and can again produce caps too large for the aggregate exchange. Fresh evidence beyond the earlier comment is this distinct additionalContexts path, whose messages have neither injected: true, source: 'steer', nor isMeta: true; exclude synthetic hook messages as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: startsUserTurn now counts a HumanMessage as a user turn only if it has no source (the SDK stamps every message it synthesizes: hook, steer, routing, handoff, skill), is not injected or isMeta, and is not role: 'system'. The additionalContexts hook message is covered by a test.
| } else if (hasExactKeys(keys, LEGACY_ENVELOPE_KEYS)) { | ||
| previewKey = '_truncated'; |
There was a problem hiding this comment.
When a genuine tool input consists of the two fields _truncated and _originalChars, this branch classifies it as a legacy internal envelope solely from its keys and types, and the new forced-conversion path rewrites it even when it fits the cap. This shape is not merely hypothetical: the public truncateToolInput helper returns exactly these fields and does not necessarily prefix _truncated with … [truncated]\n. Require the known legacy marker before setting previewKey so ordinary or helper-produced arguments are not silently changed in replay history.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: a {_truncated, _originalChars} pair is treated as a legacy envelope only when _truncated starts with the … [truncated]\n marker, which every legacy writer prefixed. A genuine or truncateToolInput-produced input with those field names is left untouched (test added).
| _truncated: TOOL_INPUT_TRUNCATION_MARKER, | ||
| _originalChars: originalChars, | ||
| }; | ||
| const emptyEnvelope = toolInputElision(originalChars, ''); |
There was a problem hiding this comment.
At the deepest fading rung, FADING_MIN_BUDGET_TOKENS is 170, which makes calculateMaxToolCallInputChars return a 100-character cap. The new empty envelope is already 101 characters when _originalChars has five digits, so any shortened input of at least 10,000 characters takes the fallback at line 1606 and becomes bare {}. This removes the newly added completion note and original-size metadata exactly under maximum context pressure, leaving the model with a false empty invocation for large side-effecting calls; use a shorter bounded fallback or ensure the minimum cap can hold the envelope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: when the full envelope overflows, the fallback is {_note, _originalChars} (about 76 chars), recognized on re-projection so it stays stable. Only below that does it drop to {}. A 20,000-char input at the 100-char minimum cap now keeps the completion note and size (test added).
… and keep the note at the minimum cap
2 participants
Summary
Context fading presented the model's own completed tool calls as truncated, so models re-ran side-effecting tools. In LibreChat-AI/LibreChat#16328 a model re-sent the same email seven times in one turn, about 22 s apart, until the user stopped the run. Each earlier
send_emailcall in history carried{"_truncated": "… [truncated]\n{\"subject\":…", "_originalChars": 3810}next to a result saying the email was sent. The model read its own call as cut off, wrote "Let me now send the final, complete email properly", and called the tool again. Every new call was faded the same way on the next step, so the loop continued. Replaying the same history without fading stops after one send.Two defects combined to cause it:
_truncatedwith a preview cut mid-word. For a side-effecting tool, that reads as an invitation to call it again.maxToolExchangeWidthjumped from 3 to 8, the fit rung went from 3 to 5, and the tool-call input cap fell from 15,000 to 3,748 characters, at 6–8 % context pressure. The tier is latched and persisted by the host, so the conversation never recovered: any input over about 3.7K characters, such as an email body, was stubbed for the rest of the conversation's life.This PR fixes both and heals conversations that already latched.
{"_note": "Completed call; input shortened to save context.", "_originalChars": N, "_inputPrefix": "…"}, and inline string inputs end in… [shortened; call completed: N chars]. Nothing a model sees calls a completed call truncated. Legacy{_truncated, _originalChars}envelopes, including ones persisted in graph state or session files, are still recognized and re-capped into the new shape without nesting.toolExchangeWidthcounts only assistant messages after the last human message. Stored history doesn't record which calls came from one model response (even parallel calls get separate step ids), so an earlier turn's merged call count is an artifact of reconstruction, not fan-out. The tier stays latched, so a narrower width never loosens it; it only stops reconstruction from escalating it.FADING_TIER_VERSIONis now 2. Version 1 tiers may have latched on reconstructed widths, so they are discarded on seed and re-derived once. The cost is one prompt-cache miss per affected conversation.Details
_truncatedstarts with the… [truncated]marker line every legacy writer used, and it is rewritten even when it fits the cap. That covers structuredargs, serializedtool_calls[].function.arguments, singularfunction_call, and custom-tool string inputs. Inputs that merely share the field names are left alone.userchat message, opens a turn. SDK-synthesizedHumanMessages never do: they always carry asource(hook, steer, routing, handoff, skill) and ofteninjected,isMetaorrole: 'system'.{_note, _originalChars}, and only below that{}.isLegacyFadingTier), and the run drops the legacy tier so it re-derives instead of rejecting the checkpoint.Host compatibility
Hosts that validate the persisted tier version must accept 2. LibreChat pins
AGENT_FADING_TIER_VERSION = 1inpackages/data-schemas/src/utils/fading.ts. Its companion change (constant and type to 2, the Mongoose enum to[1, 2], and the tests) is prepared and should ship with the dependency bump. A host still on version 1 would only stop persisting tiers across runs, losing prompt-cache stability, not correctness.Testing
src/specs/prune.test.ts. A turn reloaded as one assistant message with 8 calls, followed by a new human message, no longer drives the tier below what the live turn's own width allows; this test fails without the width change. A legacy_truncatedenvelope re-caps into{_note, _originalChars, _inputPrefix}with the note, the original size and a clean prefix, and never contains "truncated".npx tsc --noEmitand ESLint are clean.llm.spec.tssuites, which need provider API keys not present locally.