Skip to content

fix(agent): honor max_len in build_tool_preview for values <= 3 (#9439) - #9464

Closed
LarHope wants to merge 1 commit into
NousResearch:mainfrom
LarHope:fix/build-tool-preview-max-len-9439
Closed

LarHope wants to merge 1 commit into
NousResearch:mainfrom
LarHope:fix/build-tool-preview-max-len-9439

Conversation

@LarHope

@LarHope LarHope commented Apr 14, 2026

Copy link
Copy Markdown

Closes #9439

What does this PR do?

agent.display.build_tool_preview() unconditionally truncated with preview[:max_len - 3] + \"...\". When max_len is 1, 2, or 3, max_len - 3 is ≤ 0, so the slice returns nearly the entire source string and the result ends up far longer than the caller asked for — violating the documented cap.

This PR special-cases max_len <= 3 to emit \".\" * max_len, so the return value is always bounded by max_len.

Related Issue

Fixes #9439

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • agent/display.py — add max_len <= 3 branch in build_tool_preview() (+4/-1 lines).
  • tests/agent/test_display.py — add three regression tests (max_len ∈ {1, 2, 3}, exactly-4 boundary, no-truncation path) and tighten the existing test_long_value_truncated assertion from <= 43 to <= 40 so the contract is actually enforced.

Total diff: 2 files, +27/-3.

How to Test

Reproduction from the issue:

```python
from agent.display import build_tool_preview
for ml in (1, 2, 3, 4, 10):
r = build_tool_preview('terminal', {'command': 'abcdefghijklmnopqrstuvwxyz'}, max_len=ml)
print(f'max_len={ml}: {r!r} len={len(r)}')
```

Before this PR:
```
max_len=1: 'abcdefghijklmnopqrstuvwx...' len=27
max_len=2: 'abcdefghijklmnopqrstuvwxy...' len=28
max_len=3: 'abcdefghijklmnopqrstuvwxyz...' len=29
```

After:
```
max_len=1: '.' len=1
max_len=2: '..' len=2
max_len=3: '...' len=3
max_len=4: 'a...' len=4
max_len=10: 'abcdefg...' len=10
```

Run the full display test module:

```bash
pytest tests/agent/test_display.py -v
```

All 27 tests pass (24 existing + 3 new regressions).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs — none touching build_tool_preview
  • My PR contains only changes related to this fix
  • I've run the display test module and all tests pass
  • I've added regression tests for the bug
  • Tested on Ubuntu 24.04 (Python 3.11)

Documentation & Housekeeping

  • No doc changes needed — N/A
  • No config keys changed — N/A
  • No architecture/workflow changes — N/A
  • Pure-Python string logic, no cross-platform concerns — N/A
  • No tool behavior changes — N/A

…Research#9439)

build_tool_preview used preview[:max_len - 3] + "..." unconditionally,
so max_len of 1, 2, or 3 produced a negative slice and returned nearly
the entire source string plus "...", violating the documented cap.

Special-case max_len <= 3 to emit "." * max_len instead, and extend
test_display with regressions for max_len in {1, 2, 3}, the exactly-4
boundary, and the no-truncation path.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels Apr 27, 2026
@alt-glitch

Copy link
Copy Markdown

Likely duplicate of #9497 — same fix for build_tool_preview() max_len<=3 edge case (#9439). Competing PRs.

@alt-glitch alt-glitch added the duplicate This issue or pull request already exists label Apr 27, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused regression fix. The issue remains on current main, but the implementation needs to be retargeted after later display refactoring.

Problems

  • agent/display.py:476-482 now sends terminal and execute_code previews through _truncate_preview, which already handles limits <= 3. Therefore the submitted terminal tests would pass while the remaining generic branch at agent/display.py:545-546 still performs the unsafe preview[:max_len - 3] + "..." truncation.

Suggested changes

  • Salvage the change at agent/display.py:545-546 by calling _truncate_preview(preview, max_len).
  • Add the tiny-limit regression against a generic path such as web_search, which exercises that remaining branch.

Automated hermes-sweeper review.

Comment thread agent/display.py
if max_len > 0 and len(preview) > max_len:
preview = preview[:max_len - 3] + "..."
if max_len <= 3:
preview = "." * max_len

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current main now routes terminal through _truncate_preview, so this branch no longer covers the PR's terminal repro. Please apply the shared helper to the current generic fallback branch (agent/display.py:545-546) and add a generic-tool regression such as web_search.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 12, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Salvaged onto current main as #109134 with your commit authorship preserved where the diff still applied (the file moved/was restructured since April, so some of it is a hand-port with credit in the commit message). Once #109134 merges this PR will be closed with a link to the landed SHA. Thanks for the fix.

teknium1 pushed a commit that referenced this pull request Sep 12, 2026
`build_tool_preview()`'s generic-key fallback and the cute-message helpers
still truncated with a bare `text[:max_len - 3] + "..."`; for max_len 1-3 the
slice goes negative and returns almost the whole string (27 chars for
max_len=1). `_truncate_preview` already had the guard, so the two code paths
disagreed.

One truncation helper (`_tail_trunc`) with the guard, used everywhere; the
head-truncating `_cute_path` gets the same clamp.

Salvage of PR #48483 by @HeLLGURD (current-code fix); the earliest reports and
patches were #9464 (@LarHope), #9477 (@kagura-agent) and #9497.

Co-authored-by: LarHope <12761142+LarHope@users.noreply.github.com>
Fixes #9439
@teknium1

Copy link
Copy Markdown
Collaborator

Landed via #109134 (merge 9d09598) with your commit cherry-picked so authorship is preserved — thank you @LarHope. Closing this PR as superseded by the merged salvage; the fix is on main now.

@teknium1 teknium1 closed this Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

build_tool_preview() breaks max_len contract for values under 4

3 participants