Keep trimmed tool outputs inside max_output_chars - #4691
Conversation
`_trim_function_call_output` and `_trim_legacy_tool_search_results` sliced `preview_chars` characters and only checked the result against the original length, so `max_output_chars` acted as a trigger threshold rather than a budget. With `max_output_chars=100, preview_chars=5000` a 6000-char output was "trimmed" to 5062 chars. The structured path already sized its preview to fit, so the three paths disagreed. Clamp both free-text paths through a shared helper that shrinks the preview to whatever the header leaves inside the budget, and report the real preview length in the header instead of always claiming `preview_chars`. When not even the header fits, the output is left untouched.
![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: 3f73909e66
ℹ️ 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".
| ellipsis = "..." if preview_len < len(text) else "" | ||
| room = self.max_output_chars - len(header) - len(ellipsis) | ||
| if preview_len <= room: | ||
| return f"{header}{text[:preview_len]}{ellipsis}" | ||
| if room < 0: | ||
| return None |
There was a problem hiding this comment.
With an allowed tight budget such as max_output_chars=60 and a normal my_tool output, room becomes negative because the trailing ... does not fit, even though a zero-preview header (or another compact fallback) does fit within the budget. Returning None therefore leaves the entire large old output untrimmed, defeating the configured cap for this supported configuration; emit a bounded fallback when the header fits instead of treating lack of ellipsis room as inability to summarize.
AGENTS.md reference: AGENTS.md:L147-L147
Useful? React with 👍 / 👎.
`_fit_preview_summary` gave up as soon as the header plus the trailing "..." exceeded the budget, so a tight but supported `max_output_chars` left the whole oversized output in place instead of replacing it. The early return also fired before the preview had shrunk, so a budget that a shorter preview length would have fit was rejected too. Treat the newline and the ellipsis as preview overhead rather than as part of the header: a zero-length preview now yields the bare header, and the loop shrinks the preview to zero before concluding that nothing fits. `None` now means only that the header itself exceeds the budget.
…into fix/tool-output-trimmer-budget
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 618b1be536
ℹ️ 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".
| if preview_len == 0: | ||
| return None |
There was a problem hiding this comment.
When tool_namespace() is given a long, locally accepted name, the returned qualified tool name can make even the zero-preview header exceed a valid tight budget (for example, max_output_chars=60). This branch then returns None, so _trim_function_call_output retains the entire multi-kilobyte output rather than enforcing the requested limit; this remains reachable after the header-only fallback, unlike structured outputs that can emit [Trimmed]. Fall back to a compact identifier-free marker when the verbose header cannot fit.
AGENTS.md reference: AGENTS.md:L147-L147
Useful? React with 👍 / 👎.
… fit A long qualified tool name can push even a preview-less header past a tight but valid max_output_chars, and the free-text paths then declined to trim at all - leaving the whole multi-kilobyte output in place, overshooting the budget by far more than any header would. Try progressively more compact headers, the way the structured path already does: the full header, then an identifier-free one that keeps the sizes, then "[Trimmed]", and finally as much of that as the budget allows. Trimming a free-text output can no longer fail, so both call sites lose their None branch.
|
Closing this PR because the referred issue was closed: #4686 (comment) |
2 participants
Fixes #4686.
Problem
ToolOutputTrimmertreatedmax_output_charsas a budget in one path and as a mere trigger threshold in the other two._trim_function_call_output(plain-string outputs) and_trim_legacy_tool_search_resultsslicedpreview_charscharacters and then only checked the result against the original length, never against the configured budget:_trim_structured_function_call_outputalready did the right thing, sizing the preview so the replacement fits insidemax_output_chars.The same f-string also always claimed
{preview_chars} char previewin the header, even when the real preview was shorter.Fix
This takes option 2 from the issue: clamp rather than reject, so existing configs keep working.
Both free-text paths now go through one
_fit_preview_summary()helper that shrinks the preview to whatever the header leaves insidemax_output_charsand reports the actual preview length in the header. If not even the header fits, it returnsNoneand the output is left untouched — no replacement would be an improvement at that point.Option 1 (rejecting
preview_chars > max_output_charsin__post_init__) would not have been sufficient on its own: the header is ~50 characters, somax_output_chars=100, preview_chars=100would still overflow the budget.The header length depends on the preview length it reports, so the helper re-fits after shrinking; the loop terminates because the preview length strictly decreases on every retry.
Behavior
max_output_chars=500, preview_chars=200) produces byte-identical summaries to before.len(summary) >= output_lenguard is gone; it is unreachable once the summary is bounded bymax_output_chars, which is itself belowoutput_lenwhenever trimming runs.test_skips_trim_when_summary_would_exceed_original) asserted the old overflow-then-skip behavior formax_output_chars=500, preview_chars=490. That input now trims to a 500-character summary instead of being skipped, so the test is rewritten astest_summary_never_exceeds_max_output_chars.Tests
test_trimmed_output_fits_budget_for_any_config— parametrized overmax_output_chars×preview_chars, asserts no configuration can produce a replacement larger than the budget.test_summary_reports_the_actual_preview_length— covers the header defect.test_legacy_tool_search_results_fit_budget— covers the second free-text path.All fail on
mainand pass here; the rest of the trimmer suite is unchanged.