fix(agent-core): prepend repeat reminders so they survive oversized-result truncation#2176
fix(agent-core): prepend repeat reminders so they survive oversized-result truncation#2176vincentllm wants to merge 3 commits into
Conversation
…esult truncation The dedupe repeat reminders (streak 3/5/8) were appended at the tail of the tool output, but the oversized-result offload path keeps only a 2K head preview, so the reminder was silently cut off exactly in the large-output scenarios where repeat loops are most likely. Prepend the reminder ahead of the output in both engines (agent-core, agent-core-v2) and add regression tests covering the truncation window.
🦋 Changeset detectedLatest commit: 36a656f 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 |
…ation in regression tests
c64dd94 to
1b3c785
Compare
![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: 1b3c785e0c
ℹ️ 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".
| /** | ||
| * Prepends the reminder to the tool result output. | ||
| * | ||
| * The reminder MUST be at the head, not the tail: oversized text results are | ||
| * later replaced by a head-only preview (`toolResultTruncation` keeps only the | ||
| * first 2000 chars after `onDidExecuteTool` hooks run). A tail-appended | ||
| * reminder is silently cut off there, so the model never sees it exactly in | ||
| * the large-output scenarios where repeat loops are most likely. | ||
| */ | ||
| function appendReminder(result: ToolDedupeResult, reminderText: string): ToolDedupeResult { |
There was a problem hiding this comment.
appendReminder now prepends content, so its name contradicts its behavior and the new paragraph docblock merely compensates by narrating an internal helper. Rename it to prependReminder and keep the ordering invariant in the existing regression test or a single concise WHY comment; this package explicitly forbids block/paragraph docstrings on internal helpers.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L170-L172
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — agreed. I will rename appendReminder to prependReminder (both engines) and drop the paragraph docblock, keeping a single concise WHY line for the head-placement invariant, since the ordering constraint against the head-only truncation preview is non-obvious. Will fold this into the next push.
| // Oversized text results are later replaced by a head-only 2K preview | ||
| // (agent/toolResultTruncation). Reminders used to be appended at the tail | ||
| // and were silently cut off there — the model looped 12 times with no | ||
| // reminder ever visible. They must now sit at the head. |
There was a problem hiding this comment.
This newly added comment narrates the prior implementation and bug history rather than stating the test contract; the test name and assertions already explain the required head placement. Remove the preamble, as the scoped test-comment convention explicitly prohibits regression-history comments.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L182-L184
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — I will drop the regression-history preamble from the new describe block; the test names and assertions already carry the contract. Same cleanup applies to the v1 test file.
| const truncation = new ToolResultTruncationService( | ||
| { homeDir: '/home/user' } as never, | ||
| { scope: (s: string) => s } as never, | ||
| { write: async () => undefined } as never, | ||
| ); |
There was a problem hiding this comment.
When ToolResultTruncationService gains or changes a DI dependency, this composition test will fail even if its public contract remains unchanged because it directly constructs the concrete implementation with three as never substitutes. Register the real implementation and a storage-boundary stub in the existing harness, then resolve IAgentToolResultTruncationService; that exercises production wiring and keeps the test coupled to the contract rather than constructor details.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair point — the three as never constructor stubs couple the composition test to constructor details. I will register the real ToolResultTruncationService with a storage-boundary stub in the existing harness and resolve IAgentToolResultTruncationService instead, so the test exercises the production wiring and stays coupled to the public contract.
| function appendReminder(result: ExecutableToolResult, reminderText: string): ExecutableToolResult { | ||
| const output = result.output; | ||
| let newOutput: string | ContentPart[]; | ||
| if (typeof output === 'string') { | ||
| newOutput = output + reminderText; | ||
| newOutput = reminderText + output; |
There was a problem hiding this comment.
The legacy implementation now prepends the reminder but retains the name appendReminder, while the ToolCallDeduplicator contract below still says results are “suffixed” at lines 111–115. Any maintainer following that contract can reintroduce tail placement or misunderstand output ordering; rename the helper and update the stale contract to consistently describe prepending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — I will rename the legacy helper as well and update the stale ToolCallDeduplicator contract at L111–115 so it describes prepending instead of "suffixed"; leaving it as-is could indeed nudge a maintainer back to tail placement.
…lean up per review
1 participant
Related Issue
Resolve #2177
Problem
See linked issue. In short: the dedupe repeat reminders (streak 3/5/8) are appended at the tail of the tool output, and the oversized-result offload path keeps only a 2K head preview — so the reminders are silently discarded for any tool result >50K chars, in both v1 (
agent-core) and v2 (agent-core-v2). The turn's only remaining guardrail is the streak-12 force stop.What changed
agent/turn/tool-dedup.ts(v1) andagent/toolDedupe/toolDedupeService.ts(v2): the reminder is now prepended ahead of the result output instead of appended at the tail, so it survives the head-only truncation preview. A comment documents the ordering constraint.ContentPart[]assertions that assumed tail placement were updated.@moonshot-ai/kimi-codepatch.Note: I could not run
pnpm testlocally (no pnpm available in this environment); the new and updated tests follow the existing harness patterns and should be exercised in CI. Happy to iterate if maintainers prefer an alternative fix (e.g. budgeting before dedupe finalize, or a head+tail preview).Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.