Skip to content

fix(utils): make keepRecent visibility-aware and exclude display-only messages from token counting - #1195

Open
Rudra2637 wants to merge 2 commits into
Nano-Collective:mainfrom
Rudra2637:fix/auto-compact-model-facing-filter
Open

fix(utils): make keepRecent visibility-aware and exclude display-only messages from token counting#1195
Rudra2637 wants to merge 2 commits into
Nano-Collective:mainfrom
Rudra2637:fix/auto-compact-model-facing-filter

Conversation

@Rudra2637

@Rudra2637 Rudra2637 commented Sep 5, 2026

Copy link
Copy Markdown

Description

Ensures mechanical message compression and token counting in source/utils/message-compression.ts handle display-only UI messages correctly:

  • Makes the keepRecent window calculation visibility-aware so trailing UI banners do not consume recent message slots or displace actual recent conversation turns.
  • Preserves display-only messages untouched in the output so terminal scrollback remains intact upon session resume with zero token cost.
  • Excludes display-only messages from originalTokenCount, compressedTokenCount, and preservedInfo.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:

  1. The keepRecent calculation 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.
  2. preservedInfo.recentMessages reported raw array length, counting display-only banners as "recent messages kept at full detail".
  3. countTotalTokens counted tokens for all messages, inflating token calculations with display-only chrome.

Changes

  • In source/utils/message-compression.ts:
    • Computed splitIndex by walking backward and counting only model-facing messages (isModelFacing(msg)), protecting the recent window from trailing UI banners.
    • In compressMessageSegment, passed display-only messages through untouched so they survive into compressedMessages without truncation, preserving transcript scrollback upon resume with zero token payload cost.
    • Updated preservedInfo.recentMessages to count recentMessages.filter(isModelFacing).length.
    • Updated countTotalTokens to iterate over filterModelFacing(messages) so display-only chrome is excluded from originalTokenCount and compressedTokenCount.
  • In source/utils/message-compression.spec.ts:
    • Added unit test asserting that display-only messages in both compressible and recent segments survive into the output while remaining strictly excluded from token counts and the recent window quota.
  • In .changeset/fix-message-compression-model-facing.md:
    • Updated changeset description to reflect scrollback preservation and visibility-aware recent message window handling.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All message-compression tests pass (npx ava source/utils/message-compression.spec.ts - 26/26 passed)
  • All auto-compact tests pass (npx ava source/utils/auto-compact.spec.ts - 41/41 passed)
  • TypeScript checks pass (tsc --noEmit)
  • Biome formatting/linter checks pass (biome check .)

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • No breaking changes (or clearly documented)

Copilot AI lite review requested due to automatic review settings September 5, 2026 05:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 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 isModelFacing so display-only messages aren’t included in the compressed summary segment.
  • Updated token counting to use filterModelFacing so display-only messages don’t contribute to originalTokenCount / 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 will-lamerton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@Rudra2637 Rudra2637 changed the title fix(utils): use model-facing visibility filters in message compression fix(utils): make keepRecent visibility-aware and exclude display-only messages from token counting Sep 7, 2026
@Rudra2637

Copy link
Copy Markdown
Author

Thanks for the thorough review and catch on the keepRecent boundary — that was indeed a significant gap.

I've pushed an update addressing all four points and updated the PR title/description

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants