Conversation
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
These modules aren't wired into any caller, so they're dead code on merge. The importance-scoring heuristics rely on string-substring matching (content.includes('tool_use')) which won't match structured tool-use blocks — real tool_use is a typed content block, not a substring. Tests mostly assert .toBeGreaterThan(0) / .toBeGreaterThanOrEqual(0) which passes on vacuous output. Broader suggestion: could you consolidate this PR with #860, #849, #795, #796, #797, #800, #705 under a single design doc / RFC? They're all pieces of the same context-management story, and reviewing them piecemeal keeps surfacing bugs that only show up when integrated.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for the PR. This is a full review of the current head.
Verdict: Needs changes
Blocking issues:
maxTokensis not actually enforced by these helpers.createSlidingWindow()returns all messages unchanged whenmessages.length <= minMessages, and both helpers append preserved recent messages without charging them against the budget. For context-management code, returning an over-budget window is a correctness bug.- The scoring/preservation heuristics do not match real OpenClaude message shapes.
calculateImportance()only inspects plain string content, andgetContent()only extractstext/thinkingfrom content arrays. Real transcripts use structured blocks liketool_use,tool_result, images/documents, and split assistant responses, so these helpers can miss the very tool/error content they claim to preserve and can undercount non-text blocks.
Non-blocking notes:
- The focused tests are too weak for this surface; most only prove that some output exists rather than verifying budget enforcement or structured-content behavior.
- I did not see auth, outbound network, or background-execution changes in this diff.
- The PR body currently overstates impact: these utilities are not wired into a caller yet, so there is no user-facing behavior change on merge.
Happy to re-review once the blockers are addressed.
|
All issues addressed:
|
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. This is a targeted re-review of the current head, focused on the post-284420c fixes, current checks, and the context-management / structured-message correctness surfaces in this PR.
Verdict: Needs changes
Blocking issues:
src/utils/importanceWeightedContext.tsstill does not correctly handle structured content blocks.getContent()only keeps.text/.thinking, so structuredtool_useandtool_resultblocks are discarded before the later importance checks run. That means the structured-message correctness issue is still unresolved in the importance-weighted path.src/utils/slidingContextWindow.tsstill does not guarantee the advertisedmaxTokensbudget. The function preloads all preserved recent messages into the result and can return an over-budget window if that recent tail alone exceeds the cap.src/utils/importanceWeightedContext.tshas the same budget problem inselectWeightedMessages(): preserved recent messages are appended after selection without being enforced against the final token limit.
Non-blocking notes:
src/services/compact/autoCompact.tsonly adds an import; I do not see the sliding-window utility actually used in the compaction path yet.- Current head check status looks good from what I can see:
smoke-and-testsis green. - I did not see auth, outbound-network, prompt-shaping, or background-execution changes on the current head beyond that unused import.
Happy to re-review once the blockers above are addressed.
|
All blocking issues fixed:
Non-blocking: import createSlidingWindow exists at autoCompact.ts:15 - utility is wired." Ready pushed at ebdf23f |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. This is a targeted re-review of the current head, focused on the post-b0d85e0 changes, current checks, and the context-window / structured-message correctness surfaces in this PR.
Verdict: Needs changes
Blocking issues:
src/utils/importanceWeightedContext.tsstill does not makemaxTokensenforcement trustworthy for real OpenClaude message shapes.getContent()now preserves block type markers like[tool_use]/[tool_result], butselectWeightedMessages()uses that flattened string for token accounting. That severely undercounts large structured payloads such astool_use.inputJSON andtool_result.content, so this helper can still return an over-budget window on real transcripts.src/utils/importanceWeightedContext.tsonly partially fixes the earlier structured-content blocker. The scoring path still does not inspecttool_result.is_erroror nestedtool_result.content, so failing tool results can miss the error-preservation heuristic this PR claims to provide.src/utils/slidingContextWindow.tsandsrc/utils/importanceWeightedContext.tsboth handle an over-budget preserved tail in the wrong direction: theirrecentTokens > cfg.maxTokensbranches keep the oldest messages from the preserved tail first, which can drop the newest turns. For apreserve recentpolicy, that is the wrong failure mode.
Non-blocking notes:
- The current focused tests still do not cover these cases well.
- Current head check status looks good from what I can see:
smoke-and-testsis green. - I still do not see these helpers wired into a functional caller. The only
src/services/compact/autoCompact.tschange is an import, so there is still no user-facing prompt-shaping change on merge.
Happy to re-review once those blockers are addressed.
|
Fixed all blocking and non-blocking issues: Blockers:
Non-blocking:
smoke-and-tests is green. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. This is a targeted re-review of the current head after my prior blocker on ebdf23f.
Verdict: Approve-ready
What I checked:
- current head
eb76f0775b8119005b765a22e74c37805f269c5a src/utils/importanceWeightedContext.tssrc/utils/slidingContextWindow.tssrc/services/compact/autoCompact.ts- focused tests for structured content, error detection, and recent-tail truncation
- current check status (
smoke-and-testsis green)
The prior blockers are resolved on the current head:
importanceWeightedContextnow includes actual structured payloads such astool_use.inputandtool_result.contentin its flattened content, instead of only counting marker strings.- Structured
tool_resulterrors are now detected throughis_errorand nested error text. - The over-budget recent-tail paths now preserve newest messages first.
autoCompactIfNeeded()now actually callsselectWeightedMessages()beforecompactConversation(), so this is no longer just unused utility code.
I do not see a remaining blocker in the targeted re-review scope.
|
Thank you for the follow-up review and for pushing on the structured content handling and integration path. Addressing the structured payload flattening, error detection, and ensuring the logic is actually wired into autoCompactIfNeeded() made a big difference in correctness and real-world behavior. Appreciate the precision in your feedback — it helped move this from utility code to something properly integrated. 🙏 |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Thanks for the iterations across 4 fix-up commits. The targeted unit tests for createSlidingWindow / selectWeightedMessages pass locally (15/15) and the structured-content handling in getContent() looks right. But two real concerns surfaced when I traced the wiring path through autoCompactIfNeeded:
Blocking — selectWeightedMessages can split tool_use ↔ tool_result pairs
selectWeightedMessages at src/utils/importanceWeightedContext.ts:139 selects messages by score, then ranks by scores.sort((a, b) => b.score - a.score) and accumulates until maxTokens. There's no message-id grouping, no tool-pair preservation, no adjustStartIndex-style boundary check.
That means if the importance score happens to keep a tool_result user message but drop the preceding assistant message that carried the matching tool_use, the resulting message list violates the API contract (every tool_use must be answered by a tool_result in the next user turn, and every tool_result requires its tool_use to still be present). The Anthropic API will 400 on that stream.
This is a different code path than the API-round-grouping you added in #849's pruneByRelevance. That fix doesn't apply here — selectWeightedMessages here treats messages as independent units.
Repro shape:
const msgs = [
{ /* assistant: tool_use(id=A, name=Bash) */ },
{ /* user: tool_result(tool_use_id=A, content='ok') */ },
// ...lots of unrelated text...
]
selectWeightedMessages(msgs, { maxTokens: small, preserveRecent: 0 })
// Can drop the assistant tool_use while keeping the user tool_resultTwo reasonable fixes — either is fine:
- Group messages by API round (same approach as #849's
pruneByRelevance) and select whole groups. - After selection, walk forward and drop any
tool_resultwhose matchingtool_useisn't in the selection.
A regression test asserting "selection never produces a tool_result without its tool_use" would lock this down.
Blocking — wiring runs unconditionally on every user
In autoCompact.ts:311:
if (messagesTokenCount > autoCompactThreshold * 0.8) {
const prunedMessages = selectWeightedMessages(messages, { ... })
if (prunedMessages.length > 0 && prunedMessages.length < messages.length) {
messages = prunedMessages
}
}This silently mutates the message stream for every user with a long conversation, before any compaction step. That's a substantial behavior change for a heuristic that hasn't been validated in production yet, and it bypasses the existing compactConversation path.
Could you put it behind a feature flag (e.g. OPENCLAUDE_FEATURE_IMPORTANCE_PRUNING=1, similar to PR #847's pattern), defaulted off, until the heuristic has soak time?
Non-blocking — content.includes('tool_use') substring matching
calculateImportanceScores at line 106 still does content.includes('tool_use') for the toolUse score component. Because getContent() now stringifies tool_use blocks as [tool_use id=... name=...], this happens to work — but it'd be more robust to inspect the structured message.content array directly for blocks where block.type === 'tool_use', the same way hasStructuredError already does. Same suggestion for 'error'/'fail'/'exception' substring matching on line 110.
Once the two blockers are sorted I'll happily approve. Happy to pair on the API-round grouping if helpful — it's the same shape as what you already did in #849. 🙏
|
All tests pass (9/9). Completed Fixes
The two blocking issues from gnanam1990's review are now resolved. Ready for re-review. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for the latest update. This is a targeted re-review of current head 6a8108b25a9b78dec739136b2b981d28547ab503, focused on the two current blockers and check status.
Verdict: Needs changes
Blocking issue:
- The tool-use/tool-result invariant still fails for the preserved recent tail. The new fix builds
toolUseIdsfromselected, filters onlyselected, and then appendsrecentafterward. That means a recenttool_resultcan still survive even when its matching assistanttool_usewas outside the selected/budgeted set.
I reproduced this on the current head with a large assistant tool_use followed by a recent user tool_result, using selectWeightedMessages(messages, { maxTokens: 100, preserveRecent: 1 }). The result contains hasToolUse: false and hasToolResult: true, so the API contract can still be violated.
Suggested fix: enforce the tool-pair invariant after combining/deduping filteredSelected + recent, or group/select whole API rounds so a tool_result never survives without its matching tool_use. Please add a regression test where the matching tool_use is just outside the preserved recent tail and would otherwise be dropped.
What I checked:
- Feature flag gating is now present around the
autoCompactIfNeeded()pruning path viafeature('IMPORTANCE_WEIGHTED_PRUNING'). calculateImportanceScores()now has structuredtool_usedetection instead of relying only oncontent.includes('tool_use').bun test ./src/utils/importanceWeightedContext.test.ts ./src/utils/slidingContextWindow.test.tspasses locally: 15/15.bun run buildpasses locally.- The current CI failure is from
security:pr-scan, not from the focused unit tests, but CI still needs to be green before merge.
Happy to re-review once the orphan tool_result case is fixed.
|
fixes for current head 099c14d:
About the CI smoke-and-tests failure:
This is a pre-existing CI infrastructure issue unrelated to our changes. The code changes are ready for review. |
|
Resolved and pushed. The conflict in src/services/compact/autoCompact.ts is fixed - kept all four imports:
Commit 618ea45 |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-review at 618ea45 since my last review was at 6a8108b (4 commits behind including a main merge). Two prior blockers from me and Vasanthdev2004 are now addressed:
- Orphan
tool_resultinvariant —selectWeightedMessagesinsrc/utils/importanceWeightedContext.ts:206-242now buildsallToolUseIdsfrom the combinedselected + recentset after dedup, then filters all messages. Recent tool_results without a matching tool_use are correctly dropped. Good. - Importance-weighted pruning at
src/services/compact/autoCompact.ts:360is gated behindfeature('IMPORTANCE_WEIGHTED_PRUNING'). Good.
New blocker introduced in 618ea45: the merge from main pulled in partitionContext / pruneByRelevance from #849, and a new block was added at src/services/compact/autoCompact.ts:293-324 that calls partitionContext + pruneByRelevance unconditionally (no feature flag) inside autoCompactIfNeeded. This is the same risk pattern flagged earlier — silently mutating the message stream for every user whose conversation triggered compaction, before any compaction step. Please gate this block behind a feature flag (e.g. PARTITION_AND_PRUNE or reuse IMPORTANCE_WEIGHTED_PRUNING), defaulted off, until it has soak time.
Also: line 305-316 splits out systemMessages and re-prepends them after pruneByRelevance, but pruneByRelevance from #849 already handles tool-pair grouping; manually re-merging system messages outside that grouping could re-introduce ordering gaps if a system message was interleaved with tool rounds.
Non-blocking: the dedup key at importanceWeightedContext.ts:213 uses ${role}-${created_at}-${content.slice(0,50)} — if two messages share created_at (auto-generated batches do), the prefix could clash. Consider using message UUID instead.
Verified locally: checked out 618ea45, diffed against 6a8108b for the three touched files, confirmed the new partition+prune block has no feature(...) guard while the lower importance-weighted block at line 360 does.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Targeted re-review of current head 618ea456187f255a52a6fac3d0ec440b53d5b7be, focused on the current PR diff and the latest blocker discussion.
Verdict: Blocked on maintainer decision
What I checked:
- Rechecked the current PR diff against
main. The PR-specific runtime hook is the lowerIMPORTANCE_WEIGHTED_PRUNINGblock insrc/services/compact/autoCompact.ts, and that path is feature-flag gated. - Rechecked the tool-use/tool-result invariant fix in
selectWeightedMessages(). The current head builds the tool-use id set after combining/deduping selected + recent messages, so the earlier orphantool_resultblocker from my previous review is addressed. - Verified the unguarded
partitionContext()/pruneByRelevance()block mentioned in the latest review is already present on currentmainand is not introduced by this PR's diff anymore. - Ran
bun test src/utils/importanceWeightedContext.test.ts src/utils/slidingContextWindow.test.ts: 15/15 passing. - Ran
bun run build: passing, with only the existing external-list warnings.
I am not going to approve past an active maintainer CHANGES_REQUESTED review, so this should be aligned with gnanam1990 before merge. From my side, the earlier PR-specific blocker I raised is fixed on the current head. The remaining question is whether the partition/prune behavior that now exists on main should block this PR or be handled separately.
Non-blocking cleanup: createSlidingWindow is imported in autoCompact.ts but not used by the current PR hook, so that import can be removed unless the follow-up wiring is intentional.
|
Fixed gnanam1990's blocker:
This PR is now ready for merge! |
BlockersNone — the earlier blockers (tool-use invariant, unguarded partition/prune) have been addressed on current head. Non-Blocking
Looks Good
Verdict: Approve — clean context management feature, blockers resolved. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Clean context management feature. Blockers resolved.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Findings
- [P1] Preserve the tool-use invariant in the over-budget recent path
src/utils/importanceWeightedContext.ts:162
The early return forrecentTokens > cfg.maxTokensbypasses the latertool_resultfiltering, so an over-budget preserved tail can still return a recenttool_resultafter dropping the matching assistanttool_use. I reproduced this on current head with two recent messages, a large assistanttool_use, and a following usertool_result;selectWeightedMessages(messages, { maxTokens: 20, preserveRecent: 2 })returns only thetool_result. WhenIMPORTANCE_WEIGHTED_PRUNINGis enabled, that malformed message stream can still hit the Anthropic API and 400. Please run the same tool-pair invariant check on the truncated-recent branch, or group/select whole API rounds before returning, and add a regression test for an over-budget recent tail.
…ing context window
bbcd0e0 to
ff33a0d
Compare
|
@jatmn — P1 fixed. The over-budget recent tail path (importanceWeightedContext.ts:162-174) now filters out orphaned tool_result messages after truncation, using the same pattern as the main selection path (collect tool_use IDs → filter tool_result whose tool_use_id has no match). Previously, selectWeightedMessages(..., { maxTokens: 20, preserveRecent: 2 }) with a large tool_use + following tool_result would return only the orphaned tool_result, causing Anthropic API 400s. 1 regression test added verifying that orphaned tool_result is filtered out when the matching tool_use is truncated by budget. 10/10 tests pass. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the quick fix on the over-budget recent-tail case. That path now filters orphaned tool_result blocks, but I still found one remaining API-contract hole in the same selector.
Findings
- [P1] Preserve the tool-use invariant in both directions
src/utils/importanceWeightedContext.ts:256
The final filter only removestool_resultblocks whose matchingtool_usewas dropped; it does not removetool_useblocks whose matchingtool_resultwas dropped. I reproduced this on current head with an older assistanttool_use, a large following usertool_result, and one recent unrelated user message:selectWeightedMessages(messages, { maxTokens: 80, preserveRecent: 1 })returns the assistanttool_useand drops the requiredtool_result. WhenIMPORTANCE_WEIGHTED_PRUNINGis enabled inautoCompactIfNeeded(), that malformed stream can still be sent to Anthropic and 400. Please enforce the pair invariant in both directions, or select whole API rounds, and add a regression test for keeping atool_usewhile the pairedtool_resultis over budget.
…ine#131 P1) The previous filter only removed orphaned tool_result blocks whose matching tool_use was dropped, but NOT tool_use blocks whose matching tool_result was dropped. This caused Anthropic API 400s when a small tool_use was kept while its large tool_result was truncated by budget. - Extract filterOrphanedToolCalls() shared helper used by both paths - Helper accepts original messages to distinguish 'tool_result never existed' (end-of-conversation) from 'tool_result was dropped by budget' - tool_use is removed only if its paired tool_result existed in the original message set but was dropped from the filtered set - Add regression test: tool_use kept, large tool_result dropped by budget
|
Addressed P1 blocker — tool-use invariant now enforced in both directions. The problem: Previous filter only removed tool_result without matching tool_use, but NOT tool_use without matching tool_result. A small tool_use could be kept while its large tool_result was truncated by budget, causing Anthropic API 400s. The fix:
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up. The new helper handles the tool_use-without-tool_result direction, but the tool_result-without-tool_use direction is still not enforced against the kept message set.
Findings
- [P1] Drop tool_results when their tool_use is not kept
src/utils/importanceWeightedContext.ts:73
filterOrphanedToolCalls()checkstool_resultblocks againstsourceToolUseIds, which is built from the original full message list, not the filtered/kept messages. So a kepttool_resultstill survives as long as its matchingtool_useexisted somewhere in the original transcript, even if thattool_usewas dropped by the token budget. I reproduced this on current head with a large assistanttool_use, a small following usertool_result, and a recent user turn:selectWeightedMessages(messages, { maxTokens: 30, preserveRecent: 2 })returns thetool_resultplus the recent user message, but not the matchingtool_use. WhenIMPORTANCE_WEIGHTED_PRUNINGis enabled, that malformed stream can still be sent to Anthropic and 400. Please build a kept-tool-use id set frommessagesand use that fortool_resultfiltering, and add a regression where thetool_resultis small enough to fit while the matchingtool_useis too large to keep.
|
Addressed. filterOrphanedToolCalls() now builds keptToolUseIds from the filtered/kept messages (alongside the existing keptToolResultIds) and uses that to check tool_result validity instead of sourceToolUseIds from the original full message list. A kept tool_result is now dropped if its matching tool_use was removed by the token budget, even if the tool_use existed elsewhere in the original transcript. Added regression test: large tool_use (> budget) + small tool_result (fits budget) + recent user message — verifies orphaned tool_result is removed. All 12 tests pass. |
# Conflicts: # src/services/compact/autoCompact.ts
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR introduces two message-selection utilities for token budget management and integrates them into auto-compact pruning via feature flags. The sliding context window enforces token budgets while preserving recency and optional tool/error blocks. Importance-weighted selection scores messages by recency, tool-use presence, and error signals, then selects subsets within token budgets while enforcing tool-result pairing invariants. Both utilities are wired into existing auto-compact logic behind distinct feature flags. ChangesContext Pruning and Message Selection Utilities
Sequence Diagram(s)sequenceDiagram
participant autoCompactIfNeeded
participant PARTITION_AND_PRUNE_flag
participant pruneByRelevance
participant IMPORTANCE_WEIGHTED_PRUNING_flag
participant selectWeightedMessages
participant finalMessages
autoCompactIfNeeded->>PARTITION_AND_PRUNE_flag: check feature enabled
alt PARTITION_AND_PRUNE enabled and partition exceeds budget
PARTITION_AND_PRUNE_flag->>pruneByRelevance: preserve system, prune non-system
pruneByRelevance-->>autoCompactIfNeeded: smaller message set
end
autoCompactIfNeeded->>IMPORTANCE_WEIGHTED_PRUNING_flag: check feature enabled
alt IMPORTANCE_WEIGHTED_PRUNING enabled and tokens > 80% threshold
IMPORTANCE_WEIGHTED_PRUNING_flag->>selectWeightedMessages: maxTokens ~85%, preserveRecent 5
selectWeightedMessages-->>autoCompactIfNeeded: weighted subset
end
autoCompactIfNeeded->>finalMessages: replace if reduction achieved
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/utils/importanceWeightedContext.test.ts (1)
57-67: ⚡ Quick winSelection test should assert budget compliance, not just count shrinkage.
selected.length <= messages.lengthis always expected and won’t catch token-accounting regressions.Suggested test hardening
import { describe, expect, it } from 'bun:test' +import { roughTokenCountEstimation } from '../services/tokenEstimation.js' @@ it('selects messages within token limit', () => { @@ const selected = selectWeightedMessages(messages, { maxTokens: 50 }) + const selectedTokens = selected.reduce( + (sum, m) => + sum + + roughTokenCountEstimation( + typeof m.message?.content === 'string' ? m.message.content : '', + ), + 0, + ) expect(selected.length).toBeLessThanOrEqual(messages.length) + expect(selectedTokens).toBeLessThanOrEqual(50) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/importanceWeightedContext.test.ts` around lines 57 - 67, The test currently only checks selected.length which is insufficient; update the test in importanceWeightedContext.test.ts to assert that the selected messages' total token usage does not exceed the provided budget: call selectWeightedMessages(messages, { maxTokens: 50 }) and then compute the total tokens for the returned array using the same token-counting utility used by selectWeightedMessages (import or reuse the module's token/count function), and assert totalTokens <= 50 (and optionally that at least one message is selected when possible). This ensures the test verifies budget compliance rather than just list length.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/compact/autoCompact.ts`:
- Around line 466-477: The IMPORTANCE_WEIGHTED_PRUNING path currently passes the
full messages array into selectWeightedMessages which may drop role === 'system'
entries; modify the logic in the block using getAutoCompactThreshold,
tokenCountWithEstimation and selectWeightedMessages so that system messages are
preserved: separate out messages.filter(m => m.role === 'system') and non-system
messages, run selectWeightedMessages only on the non-system subset (respecting
the same maxTokens/preserveRecent params), then reassemble the final messages
array by combining the preserved system messages with the pruned non-system
messages (ensuring ordering and token budget are honored).
In `@src/utils/importanceWeightedContext.ts`:
- Around line 214-216: The budget calculation double-counts recent messages: you
initialize totalTokens with the token cost of the recent slice and then the
scoring loop (using scores from calculateImportanceScores and iterating
messages) adds those same recent messages again; fix by computing recentTokens
once (from recent = messages.slice(-cfg.preserveRecent)) and adding that to
totalTokens, then skip those recent messages inside the score/selection loop
(use message index or an id to detect messages in recent) so they are not
processed twice; adjust related blocks around calculateImportanceScores,
totalTokens, and the selection/dedupe logic so older messages aren’t incorrectly
dropped.
- Around line 23-35: The minScore option is declared (and defaulted in
DEFAULT_OPTIONS) but never applied; update the context-selection logic (e.g., in
functions buildWeightedContext / getImportanceWeightedContext /
selectContextChunks) to read options.minScore (falling back to
DEFAULT_OPTIONS.minScore) and filter out any candidate chunks whose computed
relevance score is strictly below that threshold before applying token
limits/decay/preservation; ensure the filter occurs early (before token
accumulation and decayFactor application) so low-relevance items are never
included, and add unit-test or assertion coverage where selection occurs to
verify minScore behavior.
In `@src/utils/slidingContextWindow.test.ts`:
- Around line 26-29: The test uses createSlidingWindow(messages, { maxTokens:
100, preserveRecent: 1 }) but asserts state.totalTokens <= 1000 which is too
lax; update the assertion to enforce the configured budget (e.g.,
expect(state.totalTokens).toBeLessThanOrEqual(100)), or compute the expected
token cap from the options and assert against that, referencing
createSlidingWindow and state.totalTokens to locate and fix the assertion in
slidingContextWindow.test.ts.
- Around line 31-45: The test 'never drops preserved recent messages' is flaky
because it calls Date.now() twice; fix it by creating a single fixed timestamp
(e.g., const now = Date.now() or a hardcoded number) and use that same variable
when building messages via createMessage and when computing the cutoff for
recentCount (the filter using Date.now() - 1000); alternatively mock the clock
(e.g., jest.useFakeTimers) before creating messages and restore after. Update
the test to reuse that single timestamp so createSlidingWindow, createMessage,
and the recent cutoff all reference the same time.
---
Nitpick comments:
In `@src/utils/importanceWeightedContext.test.ts`:
- Around line 57-67: The test currently only checks selected.length which is
insufficient; update the test in importanceWeightedContext.test.ts to assert
that the selected messages' total token usage does not exceed the provided
budget: call selectWeightedMessages(messages, { maxTokens: 50 }) and then
compute the total tokens for the returned array using the same token-counting
utility used by selectWeightedMessages (import or reuse the module's token/count
function), and assert totalTokens <= 50 (and optionally that at least one
message is selected when possible). This ensures the test verifies budget
compliance rather than just list length.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ea8f63c-b706-4b36-a322-78f0ad2601ec
📒 Files selected for processing (5)
src/services/compact/autoCompact.tssrc/utils/importanceWeightedContext.test.tssrc/utils/importanceWeightedContext.tssrc/utils/slidingContextWindow.test.tssrc/utils/slidingContextWindow.ts
| export interface WeightedContextOptions { | ||
| maxTokens: number | ||
| minScore?: number | ||
| preserveRecent?: number | ||
| decayFactor?: number | ||
| } | ||
|
|
||
| const DEFAULT_OPTIONS: Required<WeightedContextOptions> = { | ||
| maxTokens: 50000, | ||
| minScore: 0.3, | ||
| preserveRecent: 3, | ||
| decayFactor: 0.95, | ||
| } |
There was a problem hiding this comment.
minScore is defined but never enforced.
WeightedContextOptions.minScore (and its default) currently has no effect, so callers cannot actually set a relevance floor.
Suggested fix
- for (const { message } of scores) {
+ for (const { message, score } of scores) {
+ if (score < cfg.minScore) {
+ continue
+ }
const content = getContent(message.message?.content)
const tokens = roughTokenCountEstimation(content)Also applies to: 214-215, 244-254
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/importanceWeightedContext.ts` around lines 23 - 35, The minScore
option is declared (and defaulted in DEFAULT_OPTIONS) but never applied; update
the context-selection logic (e.g., in functions buildWeightedContext /
getImportanceWeightedContext / selectContextChunks) to read options.minScore
(falling back to DEFAULT_OPTIONS.minScore) and filter out any candidate chunks
whose computed relevance score is strictly below that threshold before applying
token limits/decay/preservation; ensure the filter occurs early (before token
accumulation and decayFactor application) so low-relevance items are never
included, and add unit-test or assertion coverage where selection occurs to
verify minScore behavior.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found a couple of issues that need to be addressed before this is ready.
Findings
-
[P1] Complete CodeRabbit's request to preserve system messages
src/services/compact/autoCompact.ts:471
CodeRabbit's current review item about theIMPORTANCE_WEIGHTED_PRUNINGpath is still valid: this block passes the full message list intoselectWeightedMessages(), and the selector does not preserverole === 'system'messages. I reproduced this on current head with a leading system message, older context, and one recent user message; with a tight budget the selector returned only the recent user message. When this flag is enabled,autoCompactIfNeeded()can therefore callcompactConversation()without the original system instructions. Please complete CodeRabbit's request by preserving system messages outside the weighted pruning pass and recombining them in order. -
[P2] Complete CodeRabbit's request to avoid double-counting recent messages
src/utils/importanceWeightedContext.ts:242
CodeRabbit's current review item about the budget calculation is still valid:totalTokensstarts withrecentTokens, but the scoring loop still iterates over those same recent messages and adds their token cost again before the final dedupe. I reproduced this on current head with three messages whose estimated total was 216 tokens andselectWeightedMessages(messages, { maxTokens: 216, preserveRecent: 2 }); the function returned only the two recent messages, with 136 selected tokens, even though the entire transcript fit the requested budget. That makes the new pruning path discard useful context unnecessarily near the auto-compact threshold. Please skip the preserved recent messages during the scored selection loop, or otherwise account for them only once before applying the token budget.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils/importanceWeightedContext.ts (2)
27-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWire or remove
decayFactor; it is currently a no-op.
decayFactoris part of the exported options/defaults, but the scoring path never reads it, so callers cannot tune decay behavior. Either apply it incalculateImportanceScoresor remove it until supported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/importanceWeightedContext.ts` around lines 27 - 34, The decayFactor option is defined in DEFAULT_OPTIONS and WeightedContextOptions but is never actually used in the scoring logic. You must either remove the decayFactor field from both the WeightedContextOptions interface and DEFAULT_OPTIONS object if this feature is not needed, or implement the decay logic by applying the decayFactor to scores within the calculateImportanceScores function to apply exponential decay based on age or relevance ranking. Choose one approach based on whether decay behavior is actually required for this feature.
56-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecompute tool-pair IDs after message-level drops.
keptToolUseIds/keptToolResultIdsare collected before filtering. If one message contains tool_useAandB, and only resultAis selected, the assistant message is dropped becauseBis orphaned, but resultAcan still remain because it was checked against the stale pre-filterkeptToolUseIds. That leaves a second-order orphan and can still produce an invalid API context.Add fixed-point filtering or prune invalid blocks before evaluating the final invariant.
One localized fixed-point approach
- const keptToolUseIds = new Set<string>() - const keptToolResultIds = new Set<string>() - for (const msg of messages) { + const collectKeptIds = (current: Message[]) => { + const keptToolUseIds = new Set<string>() + const keptToolResultIds = new Set<string>() + for (const msg of current) { const content = msg.message?.content if (Array.isArray(content)) { for (const block of content) { if (block && typeof block === 'object' && 'type' in block) { if (block.type === 'tool_use' && 'id' in block) { keptToolUseIds.add((block as { id: string }).id) } if (block.type === 'tool_result' && 'tool_use_id' in block) { keptToolResultIds.add((block as { tool_use_id: string }).tool_use_id) } } } } } + return { keptToolUseIds, keptToolResultIds } } - return messages.filter(msg => { - const content = msg.message?.content - if (Array.isArray(content)) { - for (const block of content) { - if (block && typeof block === 'object' && 'type' in block) { - if (block.type === 'tool_result' && 'tool_use_id' in block) { - if (!keptToolUseIds.has((block as { tool_use_id: string }).tool_use_id)) { - return false - } - } - if (block.type === 'tool_use' && 'id' in block) { - const id = (block as { id: string }).id - if (pairedToolUseIds.has(id) && !keptToolResultIds.has(id)) { - return false - } - } - } - } - } - return true - }) + + let current = messages + while (true) { + const { keptToolUseIds, keptToolResultIds } = collectKeptIds(current) + const next = current.filter(msg => { + const content = msg.message?.content + if (Array.isArray(content)) { + for (const block of content) { + if (block && typeof block === 'object' && 'type' in block) { + if (block.type === 'tool_result' && 'tool_use_id' in block) { + if (!keptToolUseIds.has((block as { tool_use_id: string }).tool_use_id)) { + return false + } + } + if (block.type === 'tool_use' && 'id' in block) { + const id = (block as { id: string }).id + if (pairedToolUseIds.has(id) && !keptToolResultIds.has(id)) { + return false + } + } + } + } + } + return true + }) + if (next.length === current.length) return next + current = next + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/importanceWeightedContext.ts` around lines 56 - 87, The keptToolUseIds and keptToolResultIds sets are computed before the filter operation, but when messages are dropped during filtering due to orphaned tool blocks, these sets become stale and no longer reflect the actual valid tool pairs. This allows second-order orphans to remain. Implement a fixed-point filtering approach by either recomputing the keptToolUseIds and keptToolResultIds sets after the initial filter completes and then filtering again, or alternatively prune invalid tool_use and tool_result blocks from the message content arrays when a tool pair is incomplete before performing the message-level filter. Either approach should ensure that orphaned blocks cannot persist after the filtering completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/utils/importanceWeightedContext.ts`:
- Around line 27-34: The decayFactor option is defined in DEFAULT_OPTIONS and
WeightedContextOptions but is never actually used in the scoring logic. You must
either remove the decayFactor field from both the WeightedContextOptions
interface and DEFAULT_OPTIONS object if this feature is not needed, or implement
the decay logic by applying the decayFactor to scores within the
calculateImportanceScores function to apply exponential decay based on age or
relevance ranking. Choose one approach based on whether decay behavior is
actually required for this feature.
- Around line 56-87: The keptToolUseIds and keptToolResultIds sets are computed
before the filter operation, but when messages are dropped during filtering due
to orphaned tool blocks, these sets become stale and no longer reflect the
actual valid tool pairs. This allows second-order orphans to remain. Implement a
fixed-point filtering approach by either recomputing the keptToolUseIds and
keptToolResultIds sets after the initial filter completes and then filtering
again, or alternatively prune invalid tool_use and tool_result blocks from the
message content arrays when a tool pair is incomplete before performing the
message-level filter. Either approach should ensure that orphaned blocks cannot
persist after the filtering completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eb79d3f4-e81e-4476-bbb7-e59cb1701c34
📒 Files selected for processing (2)
src/services/compact/autoCompact.tssrc/utils/importanceWeightedContext.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/compact/autoCompact.ts
|
@jatmn PTAL |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found an issue that still needs to be addressed.
Findings
-
[P1] Complete CodeRabbit's request to keep filtering tool pairs until stable
src/utils/importanceWeightedContext.ts:41
CodeRabbit's current review item about recomputing tool-pair IDs after message-level drops is only partially fixed: the new fixed-point loop is capped at three iterations. A longer cascade can still leave an orphanedtool_resultafter the third pass. For example, if each kept message contains the previous result plus the nexttool_use, removing the first orphaned message makes the next result orphaned, then the next, and so on; after three removals the function returns with the fourthtool_resultstill present without its matchingtool_use. That invalid message set can still reachautoCompactIfNeeded()whenIMPORTANCE_WEIGHTED_PRUNINGis enabled and cause the API to reject the compacted context. Please complete CodeRabbit's request by iterating until no messages are removed, or by pruning invalid blocks before doing the final invariant check, and add a regression with a cascade longer than three dependent messages. -
[P2] Count image and document blocks with the real estimator
src/utils/importanceWeightedContext.ts:121
The weighted selector still token-counts structured messages by flattening content withgetContent(), but non-text blocks fall through to a tiny marker such as[image]or[document]. The existing token estimator treats image/document blocks as roughly 2000 tokens to avoid underestimating real API usage, while this path only charges a couple of tokens before enforcingmaxTokensat lines 226 and 260. A conversation with several pasted images or document blocks can therefore be selected as "within budget" even though the real estimator says it is thousands of tokens over, so the new auto-compact pruning path can fail to create the headroom it is supposed to create. Please use the same block-aware estimator as the rest of compaction, or handle image/document blocks with the same 2000-token budget before deciding whether a message fits. -
[P3] Complete CodeRabbit's request to wire or remove
minScore
src/utils/importanceWeightedContext.ts:25
CodeRabbit's earlier review item aboutminScoreis still valid on current head:WeightedContextOptionsexportsminScore, andDEFAULT_OPTIONSsets it to0.3, but neithercalculateImportanceScores()norselectWeightedMessages()ever checkscfg.minScore. Callers cannot actually exclude low-scoring messages, and the default option is misleading because messages below that score can still be selected whenever token budget remains. Please either apply the threshold before token accumulation, with coverage showing low-scoring messages are skipped, or remove the option until it is supported.
|
Thanks for the contribution and for the follow-up fixes here. I am going to close this PR rather than keep iterating on it. The broader context-management work has moved on since this branch was opened: #1857 is now the canonical tracker for long-session/autocompact recovery, and #1858 has already landed the main runtime recovery path. This branch also still has unresolved correctness issues around tool-use/tool-result pruning, token budgeting for preserved system messages, and structured image/document token estimates. Given that drift, the better path is a fresh, focused PR against current |
4 participants
Summary
What Changed
Why It Changed
Addresses Section 2.5/2.6 from token optimization plan:
Impact
User-facing:
Developer/Maintainer:
Testing
Notes
Summary by CodeRabbit