fix(agent-core-v2): spill oversized tool outputs and surface dropped content - #3227
fix(agent-core-v2): spill oversized tool outputs and surface dropped content#32277Sageer wants to merge 8 commits into
Conversation
🦋 Changeset detectedLatest commit: bc1f6f6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…zation normalizeToolResult rebuilds every tool result with a field whitelist, which stripped untruncatedOutput / untruncatedOutputTotalChars / spillExempt before ToolResultTruncationService could see them: the spill-on-truncation path never fired for Bash/Grep/FetchURL/WebSearch, and reads of spill files were not exempted from re-spilling. Pass the three engine-internal fields through, and add executor-level integration tests that run a retainFullOutput tool and a spill-exempt result through the real ToolResultTruncationService.
|
@codex review |
![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
Persist spill files in the bound runtime
When an agent is switched to a non-local runtime, this writes the retained output through the app-host IFileSystemStorageService and returns an absolute path under the host's bootstrap.homeDir; however, the suggested Read operation acquires the agent's bound runtime filesystem, where that host path generally does not exist. In that context the model receives a recovery pointer it cannot read, so persist the spill in the bound runtime or expose a reader for the host-side store.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| finishReason: meta.finishReason ?? null, | ||
| rawFinishReason: meta.rawFinishReason ?? null, | ||
| traceId: meta.traceId ?? null, | ||
| ...(meta.traceId !== undefined ? { traceId: meta.traceId } : {}), |
There was a problem hiding this comment.
This new conditional spread violates the repository's required construction pattern for optional properties; assign traceId: meta.traceId directly so optional fields are handled consistently.
AGENTS.md reference: AGENTS.md:L52-L54
Useful? React with 👍 / 👎.
| * Engine-internal: opts the result out of spill-to-disk (e.g. reading a file | ||
| * that already lives in the spill directory). | ||
| */ | ||
| readonly spillExempt?: true | undefined; |
There was a problem hiding this comment.
The newly added optional spillExempt property explicitly unions undefined, which is the prohibited optional-property shape in this repository; declare it as spillExempt?: true instead, including the duplicate declaration on the error result.
AGENTS.md reference: AGENTS.md:L55-L57
Useful? React with 👍 / 👎.
main banned JSDoc in comment-free packages (#3226); remove the doc blocks on the new contract fields and on isSpillFilePath. Type the harness scripted stream error as Error to satisfy only-throw-error under oxlint --type-aware.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28ae3c3252
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| output: renderPersistedToolResult( | ||
| input.toolName, | ||
| input.toolCallId, | ||
| untruncatedOutput, |
There was a problem hiding this comment.
When a retained builder result is modified after write()—for example, ToolDedupeService.appendReminder adds the repeated-call guard, or BashTool adds an exit/timeout message and its existing full-log reference—untruncatedOutput still contains only the raw writes. This branch saves that stale value and replaces the final output, so those additions disappear; in particular, a noisy repeated tool call never delivers the dedupe reminder and can continue repeating. Preserve final-only suffixes when constructing the spill result instead of replacing them with the retained buffer.
Useful? React with 👍 / 👎.
| const tail = text.slice(tailStart); | ||
| const lines = [ | ||
| `Tool output exceeded ${String(TOOL_RESULT_MAX_CHARS)} characters; showing a preview only.`, | ||
| `Tool output exceeded ${String(TOOL_RESULT_MAX_CHARS)} characters; the full output was saved to a file.`, |
There was a problem hiding this comment.
For builder output above DEFAULT_MAX_RETAINED_CHARS (10,000,000 characters), untruncatedOutput contains only the retained prefix, yet this message says the full output was saved and the subsequent next_step again calls it the full output. The size detail later admits that only a prefix survived, but the model is still directed to page through a supposedly complete file and cannot retrieve the omitted tail. Render the file and instructions as a partial preservation whenever totalChars > text.length.
Useful? React with 👍 / 👎.
appendReminder additions lived only in the final output, so they were dropped when the spill pointer replaced it; mirror the reminder into untruncatedOutputSuffix for retained results, widening ToolDedupeResult to ExecutableToolResult plus its message field. Also stop claiming the full output was saved when retention capped out, narrow spillExempt to 'true' per the optional-property convention, and pass traceId directly in the harness.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ff87b96ed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| : foregroundTimeoutMs; | ||
|
|
||
| const builder = new ToolResultBuilder(); | ||
| const builder = new ToolResultBuilder({ retainFullOutput: true }); |
There was a problem hiding this comment.
When Bash emits more than 50,000 characters, the retained fields created here cause the truncation service to replace the final output, but Bash still adds recovery data afterward without updating those fields: addForegroundOutputReference appends the complete task-log path only to output, and backgroundStartedResult reconstructs a detached result without copying untruncatedOutput at all. Consequently, a foreground result over the 10,000,000-character retention cap loses the only pointer to its complete task log, while a noisy command detached after its foreground window is returned pre-truncated and never spilled. Fresh evidence in the current tree is that the completion and dedupe suffixes were mirrored, but these two Bash transformations remain unchanged.
Useful? React with 👍 / 👎.
commit: |
…peline Route every tool result through ToolResultTruncationService.truncateForModel as the single model-context decision point: spillExempt pass-through, the 50k char budget, per-line shaping, spill persistence with a 10MB retention cap, and append-or-replace pointer rendering. Tools no longer declare truncation options; sources keep only memory-safety caps. - rename ToolResultBuilder to ToolOutputAccumulator and drop its options - mcp keeps only its media pipeline and shares the unified 50k budget - bash persists foreground logs at the spill threshold and reuses them as spill.outputPath only within the retention budget - carry completion/error messages in spill.suffix so retention capping cannot drop them - render a bounded preview when spill persistence fails - suppress suffix lines already present inline in append mode - call out text-only persistence when media parts stay attached
2 participants
Related Issue
N/A — internal task.
Problem
The engine silently destroyed content the model had no way to recover:
truncated: true; the loop-level spill service skipped already-truncated results, so the cut portion had no disk copy and the model only saw[...truncated]with no retrieval hint.What changed
ToolResultBuildergainsretainFullOutput(with a 10 MBmaxRetainedCharscap to stay safe on unbounded streams); on char-cap truncation the result carries the full text via the engine-internaluntruncatedOutputcontract field. The centralToolResultTruncationServicepersists it and replaces the output with a pointer: path-first render, true size (with "only the first N characters were preserved" when retention capped out), head 4,096 + tail 1,024 previews, and the exact elided[start, end)range so the model can compute Read/Grep slices. Fail-open on disk errors, and the internal fields are stripped before results continue down the pipeline. Bash, Grep, FetchURL, and WebSearch opt in; per-linemaxLineLengthcuts stay cosmetic and unchanged.return nullpaths inconvertMCPContentBlocknow return[MCP content dropped: ...]text parts carrying mime type, size, and uri — forresource_linkthe uri lets the model fetch the content itself.loopServiceno longer gatesappendInterruptedStreamContenton user abort, so partial output is appended to the wire on request errors too, keeping UI, wire, and context consistent.isSpillFilePath; the Read tool marks results for paths inside the agent's spill directoryspillExempt, so reading a spill file never triggers another spill.Tests: new cases for builder retention, the spill pointer format (head/tail/elided offsets, partial preservation, exempt passthrough,
isSpillFilePath), MCP drop notices, and a mid-stream error loop test. The test harness gained scripted mid-stream failures:generateBackedResponsenow yields already-streamed parts before propagating an error (it previously swallowed them, and an error stream no longer clears the inner traceId). Fullagent-core-v2suite passes except 11 pre-existingtest/app/plugin/*failures caused by the missingzipbinary in this environment, unrelated to this change.Checklist
/approve).gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.