Skip to content

fix(display): guard tool-preview truncation against tiny tool_preview_length - #48483

Closed
HeLLGURD wants to merge 1 commit into
NousResearch:mainfrom
HeLLGURD:fix/display-preview-truncation-guard
Closed

HeLLGURD wants to merge 1 commit into
NousResearch:mainfrom
HeLLGURD:fix/display-preview-truncation-guard

Conversation

@HeLLGURD

Copy link
Copy Markdown

Bug

agent/display.py truncates tool previews with the idiom text[:max_len - 3] + "..."
in four places, but only one of them guards against a small max_len.

_truncate_preview (the canonical helper) does it correctly:

def _truncate_preview(text: str, max_len: int | None) -> str:
    if max_len and max_len > 0 and len(text) > max_len:
        if max_len <= 3:                 # ? guard: avoids a negative slice
            return "." * max_len
        return text[:max_len - 3] + "..."
    return text

The three siblings omit that guard:

# build_tool_preview()
preview = preview[:max_len - 3] + "..."

# _trunc()
return (s[:limit-3] + "...") if len(s) > limit else s

# _path()
return ("..." + p[-(limit-3):]) if len(p) > limit else p

max_len / limit come from _tool_preview_max_len, which is set straight from
the user''s display.tool_preview_length config via set_tool_preview_max_len()
(only clamped to >= 0). So a user can set tool_preview_length: 1 (or 2/3),
and then for any preview longer than the limit:

  • max_len = 3 ? text[:0] + "..." ? "..." (3 chars for a limit of 3 - over budget)
  • max_len = 2 ? text[:-1] + "..." ? drops the last character and appends ...,
    producing output longer than the configured limit (e.g. "hello" ? "hell...")
  • _path with limit = 2 ? p[-(-1):] = p[1:] ? drops the first character and
    prepends ... - same corruption from the other end.

So a small tool_preview_length silently mangles previews instead of shortening
them - the opposite of what the setting is for. The author already fixed this in
_truncate_preview; the siblings were missed.

Fix

  • build_tool_preview() and _trunc() now route through _truncate_preview,
    reusing its guard (and removing the duplicated slice logic - DRY).
  • _path() truncates from the front (keeps the tail), so it can''t reuse
    _truncate_preview; it gets the same limit <= 3 guard inline.

Behavior is unchanged for normal tool_preview_length values (0 = unlimited, or
any value > 3); only the previously-corrupting 1-3 range is corrected.

Verification

  • Confirmed all four sites and the _truncate_preview guard on current main.
  • Confirmed _tool_preview_max_len is user-settable to 1-3 via
    display.tool_preview_length (set_tool_preview_max_len clamps only to >= 0).
  • No open PR touches tool_preview (checked before opening).

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have labels Jun 18, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for identifying a real boundary-condition bug. Current main still has the three unsafe CLI display slices in agent/display.py:553-555 and agent/display.py:1285-1297; routing them through the guarded helper and guarding _path is correct.

Problems

  • The same configured-cap bug remains in gateway progress rendering: gateway/run.py:17379-17380, gateway/run.py:17399-17400, gateway/run.py:17426-17427, plus gateway/platforms/base.py:2641-2642 and gateway/platforms/base.py:2653-2654. These consume the same display.tool_preview_length flow (gateway/run.py:17060-17062) and still corrupt or exceed caps of 1–3.
  • Please add regression coverage. Current tests exercise normal limits only (tests/agent/test_display.py:230-256; tests/gateway/test_stream_events.py:97-106), not the 1–3 boundary.

Suggested changes

  • Apply the guarded truncation contract to each gateway renderer above.
  • Add parameterized 1/2/3-cap tests that assert the preview payload never exceeds its cap, including the tail-preserving path case.

Automated hermes-sweeper review.

@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 14, 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 @HeLLGURD. Closing this PR as superseded by the merged salvage; the fix is on main now.

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 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.

3 participants