fix(utils): make keepRecent visibility-aware and exclude display-only messages from token counting - #1195
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The change is small, consistent with existing visibility helpers, and includes targeted test coverage for the reported regression.
Pull request overview
This PR fixes inconsistent handling of display-only (“chrome”) messages during mechanical message compression by reusing the shared visibility helpers (isModelFacing / filterModelFacing). This aligns the fallback compression path with the existing token gate and LLM summariser behavior so UI-only notices don’t affect compaction decisions or get folded into synthetic context.
Changes:
- Filtered the mechanically-compressible segment using
isModelFacingso display-only messages aren’t included in the compressed summary segment. - Updated token counting to use
filterModelFacingso display-only messages don’t contribute tooriginalTokenCount/compressedTokenCount. - Added a unit test covering exclusion of a large display-only message and added a patch changeset.
File summaries
| File | Description |
|---|---|
source/utils/message-compression.ts |
Applies model-facing filtering for the mechanical compressible segment and token counting. |
source/utils/message-compression.spec.ts |
Adds a regression test ensuring display-only messages are excluded from compression output and token totals. |
.changeset/fix-message-compression-model-facing.md |
Declares a patch release entry for the behavior fix. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
will-lamerton
left a comment
There was a problem hiding this comment.
Thanks for this. Verified on the branch: tsc --noEmit clean, biome check . clean, 26/26 tests pass. Right direction, but four things before merge.
1. Fixes #1144 doesn't match the issue. #1144 asks that auto-compact.ts and the LLM-summariser share a visibility helper. That's already true on main: both import from source/utils/message-visibility.ts (auto-compact.ts:13, used at :153/:208; llm-summariser.ts:4, used at :86). This PR changes a third file the issue never mentions, so merging as-is auto-closes an issue whose ask was already satisfied. Please re-scope the description and drop the Fixes link.
2. The fix is half-applied: the keepRecent window is still visibility-blind. message-compression.ts:92 is still raw-index-based, so display-only chrome counts toward keepRecent. Confirmed with a probe on this branch: given [u1, a1, u2, a2, '_Cancelled by user._'(displayOnly), 'Error: ...'(displayOnly)] with the default keepRecent: 2, the two banners consume the whole recent window and both genuinely-recent turns get truncated. That's the common case, since a cancellation or error notice is usually the last message. It also corrupts the user-facing stat: preservedInfo.recentMessages returns 2, which compact-handler.ts:242 renders as "2 recent messages kept at full detail" when zero model-facing messages were kept. The boundary is pre-existing, but this is the PR that should fix it, and leaving it makes the function internally inconsistent. llm-summariser.ts:74-77 already walks its split index backward for the analogous tool-pairing problem.
3. Dropping display-only messages deletes scrollback for zero token benefit. Every caller writes result.compressedMessages straight back into app state (compact-handler.ts:258, auto-compact.ts:258, conversation-loop.tsx:1132), and on resume applySession replays session.messages into scrollback (useAppHandlers.tsx:563), so the banners are permanently gone from a resumed transcript. The llm-summariser precedent doesn't carry over: that path structurally cannot keep them, since it replaces the whole segment with one synthetic message. The mechanical path keeps messages individually, so it can pass chrome through untouched, and since display-only is already stripped at convertToModelMessages (message-converter.ts:141) and now excluded from both token counts, keeping it costs the payload nothing. Suggestion: change only countTotalTokens and leave the partition alone. That fixes the real accounting bug without silently deleting user history. If you'd rather keep the filter, make it a documented decision in the code.
4. The second half of the new test is vacuous. By the time you compute expectedCompressedTokens, result.compressedMessages contains no display-only message (the only one was dropped in step 1), so the assertion sums the same set countTotalTokens sums and passes whether or not the filter exists. Put a display-only message inside the keepRecent window so it survives into the output, then assert it's excluded. t.true(result.compressedTokenCount < displayOnlyContent.length) is also weak enough to pass on almost any implementation.
Nits: message-compression.ts:95 reads better as else if (isModelFacing(msg)) since nothing follows the inner if; two trailing blank lines at EOF in the spec; preservedInfo.recentMessages counts display-only messages (follows from 2).
1 and 4 are cheap. 2 and 3 are the substantive calls: 2 is the missing half of the stated fix, 3 is worth a deliberate decision either way.
…pRecent visibility-aware
|
Thanks for the thorough review and catch on the I've pushed an update addressing all four points and updated the PR title/description |
Description
Ensures mechanical message compression and token counting in
source/utils/message-compression.tshandle display-only UI messages correctly:keepRecentwindow calculation visibility-aware so trailing UI banners do not consume recent message slots or displace actual recent conversation turns.originalTokenCount,compressedTokenCount, andpreservedInfo.recentMessages.Background & Context
Nanocoder separates messages into model-facing messages and display-only chrome (such as cancellation notices
_Cancelled by user._and UI error banners).Previously in
source/utils/message-compression.ts:keepRecentcalculation used a raw array slice index (i >= messages.length - keepRecent). If a session ended with cancellation or error notices, those banners consumed the entire recent quota and pushed genuinely recent conversation turns into the compression segment.preservedInfo.recentMessagesreported raw array length, counting display-only banners as "recent messages kept at full detail".countTotalTokenscounted tokens for all messages, inflating token calculations with display-only chrome.Changes
source/utils/message-compression.ts:splitIndexby walking backward and counting only model-facing messages (isModelFacing(msg)), protecting the recent window from trailing UI banners.compressMessageSegment, passed display-only messages through untouched so they survive intocompressedMessageswithout truncation, preserving transcript scrollback upon resume with zero token payload cost.preservedInfo.recentMessagesto countrecentMessages.filter(isModelFacing).length.countTotalTokensto iterate overfilterModelFacing(messages)so display-only chrome is excluded fromoriginalTokenCountandcompressedTokenCount.source/utils/message-compression.spec.ts:.changeset/fix-message-compression-model-facing.md:Type of Change
Changeset
pnpm changeset) describing this change for the changelogTesting
Automated Tests
.spec.ts/tsxfilesnpx ava source/utils/message-compression.spec.ts- 26/26 passed)npx ava source/utils/auto-compact.spec.ts- 41/41 passed)tsc --noEmit)biome check .)Checklist