fix: incremental context compression — correct token tracking, add ea…#78
Open
competitiveNN wants to merge 3 commits into
Open
fix: incremental context compression — correct token tracking, add ea…#78competitiveNN wants to merge 3 commits into
competitiveNN wants to merge 3 commits into
Conversation
…rly budget check, fix UI display - Forge: cache actual system prompt size in instructionsCache and expose via getCachedInstructionsSize() - ContextManager: use cached size in getContextBreakdown() instead of hardcoded 1800; add getContextWindow() accessor - useChat: use API inputTokens for post-compact token accounting instead of char-estimation; fix contextTokens reset - ContextBar: remove subagentChars double-counting in API mode - step-utils: add early context budget check at 65% in prepareStep to force text-only before overflow - subagent-tools: add estimateMessageTokens utility and maybeCompact callback interface
Fix indentation in step-utils.ts that caused TS1128 parse error. Reorder imports in manager.ts to match Biome's expected order.
competitiveNN
force-pushed
the
fix/context-tracking
branch
from
f40191d to
2fc0705
Compare
📝 Walkthrough⚠ Behavior-critical changes to agent core loop and token accounting: Core/Agent loop:
Tools:
UI/Chat:
⚠ Touches agent core (src/core/** and src/hooks/useChat.ts) — verify tests/docs updated Walkthroughsrc/core/agents/step-utils.ts:570-580 — early compaction threshold and token estimation added. Adds early context compaction at 65% context-window usage to prevent overflow. Introduces token estimation from message payloads, caches instruction sizes, exposes context-window visibility, wires an optional parent-dispatched compaction nudge, and refines token tracking and post-compaction accounting. ChangesEarly Context Compaction with Token Tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
🤖 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/core/agents/step-utils.ts`:
- Around line 717-735: The early compaction branch (guarded by
shouldEarlyCompact and !nudgeFired) currently forces text-only output but does
not mark the nudge as fired; update this block (the section that calls
emitSubagentStep with toolName "_nudge", pushes into hints, and sets
result.toolChoice = "none" / result.activeTools = []) to set nudgeFired = true
after emitting the subagent step so that subsequent checks (and the later
token-budget nudge logic) won’t emit a duplicate nudge.
- Around line 569-589: The block computing totalMsgChars and totalMsgTokens
duplicates logic; replace it by importing and calling the shared utility
estimateMessageTokens (from subagent-tools) instead of reimplementing the
chars/4 heuristic: remove the reducer that builds totalMsgChars and set
totalMsgTokens = estimateMessageTokens(messages). Ensure you add the import for
estimateMessageTokens at the top and keep the rest of the surrounding logic that
uses totalMsgTokens unchanged.
In `@src/core/agents/subagent-tools.ts`:
- Around line 63-64: Remove the dead maybeCompact callback: delete the
maybeCompact?: () => Promise<void> declaration from the SubagentTools type in
subagent-tools.ts, remove the maybeCompact parameter from buildSubagentTools and
any related function signatures, and remove the forwarding of that argument in
forge.ts (where the value is passed into buildSubagentTools). Also update any
call sites and types that reference maybeCompact (including the parameter name
in forge.ts at the place that constructs/forwards subagent tools) so the code
compiles without the unused property.
In `@src/hooks/useChat.ts`:
- Around line 1207-1208: The post-compaction usage reset is incorrectly falling
back to previous cache totals (prev.cacheRead / prev.cacheWrite), which leaks
old counters; update the fallback to the zeroed baseline instead of prev by
using compactUsage?.cacheReadTokens ?? ZERO_USAGE.cacheRead and
compactUsage?.cacheWriteTokens ?? ZERO_USAGE.cacheWrite (referencing
compactUsage, prev, ZERO_USAGE, and the cacheRead/cacheWrite fields in
useChat.ts) so that when compaction omits cache details the counters reset to
zero.
- Around line 1175-1185: The code adds compactUsage.inputTokens (the compaction
request prompt size) plus a recentTokenEstimate, which inflates contextTokens;
instead use the post-compaction context size instead of inputTokens. Replace the
branch that sets estimatedTokens to compactUsage.inputTokens +
recentTokenEstimate with logic that uses compactUsage.outputTokens (or whatever
field represents the compacted/context token count) as the base; if no
post-compaction field exists, fallback to using recentTokenEstimate or
Math.ceil(afterChars / charsPerToken). Update the assignment to estimatedTokens
and keep setContextTokens(estimatedTokens) unchanged, referencing compactUsage,
inputTokens, outputTokens (or equivalent), recentTokenEstimate, newCoreChars,
charsPerToken, afterChars, and contextWindow to locate the change.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7ced4137-42bc-4b2a-a0f3-9358bd6afc29
📒 Files selected for processing (6)
src/components/layout/ContextBar.tsxsrc/core/agents/forge.tssrc/core/agents/step-utils.tssrc/core/agents/subagent-tools.tssrc/core/context/manager.tssrc/hooks/useChat.ts
Comment on lines
+1207
to
+1208
| cacheRead: compactUsage?.cacheReadTokens ?? prev.cacheRead, | ||
| cacheWrite: compactUsage?.cacheWriteTokens ?? prev.cacheWrite, |
There was a problem hiding this comment.
src/hooks/useChat.ts:1207 carries stale cache counters after reset.
Line 1207 and Line 1208 fall back to prev.cacheRead/cacheWrite even though this block resets usage with ...ZERO_USAGE. If compaction usage omits cache details, old cache totals leak into the new post-compaction baseline.
Proposed fix
- cacheRead: compactUsage?.cacheReadTokens ?? prev.cacheRead,
- cacheWrite: compactUsage?.cacheWriteTokens ?? prev.cacheWrite,
+ cacheRead: compactUsage?.cacheReadTokens ?? 0,
+ cacheWrite: compactUsage?.cacheWriteTokens ?? 0,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cacheRead: compactUsage?.cacheReadTokens ?? prev.cacheRead, | |
| cacheWrite: compactUsage?.cacheWriteTokens ?? prev.cacheWrite, | |
| cacheRead: compactUsage?.cacheReadTokens ?? 0, | |
| cacheWrite: compactUsage?.cacheWriteTokens ?? 0, |
🤖 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/hooks/useChat.ts` around lines 1207 - 1208, The post-compaction usage
reset is incorrectly falling back to previous cache totals (prev.cacheRead /
prev.cacheWrite), which leaks old counters; update the fallback to the zeroed
baseline instead of prev by using compactUsage?.cacheReadTokens ??
ZERO_USAGE.cacheRead and compactUsage?.cacheWriteTokens ?? ZERO_USAGE.cacheWrite
(referencing compactUsage, prev, ZERO_USAGE, and the cacheRead/cacheWrite fields
in useChat.ts) so that when compaction omits cache details the counters reset to
zero.
…rly budget check, fix UI display
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/hooks/useChat.ts (1)
1172-1183:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
compactUsage.inputTokensis being treated as post-compaction context, but it is compaction-call usage.src/hooks/useChat.ts:1177-1179 uses compaction request
inputTokensas the newcontextTokensbaseline. That distorts post-compaction context and can mis-trigger threshold logic.Proposed fix
- let estimatedTokens: number; - if (compactUsage && compactUsage.inputTokens > 0) { - estimatedTokens = compactUsage.inputTokens; - } else { - estimatedTokens = Math.ceil(afterChars / charsPerToken); - } + const estimatedTokens = Math.ceil(afterChars / charsPerToken);In the Vercel AI SDK, what does `usage.inputTokens` from `generateText`/`streamText` represent exactly? Is it strictly the token count for that specific API call input, or can it be used as token count for a different subsequent prompt context?Also applies to: 1185-1205
🤖 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/hooks/useChat.ts` around lines 1172 - 1183, compactUsage.inputTokens is the compaction API call's input usage and should not be treated as the post-compaction context size; using it directly to set estimatedTokens and call setContextTokens (see compactUsage.inputTokens, estimatedTokens, setContextTokens, contextWindow, afterChars, charsPerToken) can misrepresent the actual prompt size and trigger thresholds incorrectly. Fix by computing post-compaction context tokens from the compaction response that explicitly reports post-compaction context (or if unavailable, continue to fall back to char-based estimation using afterChars/charsPerToken) — only assign setContextTokens with a value that is guaranteed to represent the new prompt size (e.g., a dedicated postCompactionContextTokens field from the compaction output), and do not reuse compactUsage.inputTokens as the context baseline unless the API docs confirm it is the post-compaction context size.
🤖 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/core/agents/forge.ts`:
- Around line 546-547: In buildInstructions ensure the cache entry always
contains the size so getCachedInstructionsSize never returns undefined: when
computing text/key inside buildInstructions, write instructionsCache.set(cm, {
text, key, size: text.length }) unconditionally (not only inside the snapshot
branch) and apply the same unconditional size inclusion for the other cache
writes referenced around the 550-553 block; update any code paths that currently
only call instructionsCache.set(...) when snapshot is truthy to include the size
property so getCachedInstructionsSize can rely on it.
---
Duplicate comments:
In `@src/hooks/useChat.ts`:
- Around line 1172-1183: compactUsage.inputTokens is the compaction API call's
input usage and should not be treated as the post-compaction context size; using
it directly to set estimatedTokens and call setContextTokens (see
compactUsage.inputTokens, estimatedTokens, setContextTokens, contextWindow,
afterChars, charsPerToken) can misrepresent the actual prompt size and trigger
thresholds incorrectly. Fix by computing post-compaction context tokens from the
compaction response that explicitly reports post-compaction context (or if
unavailable, continue to fall back to char-based estimation using
afterChars/charsPerToken) — only assign setContextTokens with a value that is
guaranteed to represent the new prompt size (e.g., a dedicated
postCompactionContextTokens field from the compaction output), and do not reuse
compactUsage.inputTokens as the context baseline unless the API docs confirm it
is the post-compaction context size.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7f154483-7397-49d6-9d5d-8a5d0a892b49
📒 Files selected for processing (4)
src/core/agents/forge.tssrc/core/agents/step-utils.tssrc/core/agents/subagent-tools.tssrc/hooks/useChat.ts
Comment on lines
+546
to
547
| if (snapshot) instructionsCache.set(cm, { text, key, size: text.length }); | ||
| return text; |
There was a problem hiding this comment.
Cache size unconditionally in buildInstructions.
src/core/agents/forge.ts:546 only writes cache when snapshot exists, so getCachedInstructionsSize can return undefined for valid instruction payloads and force fallback sizing.
Proposed fix
- if (snapshot) instructionsCache.set(cm, { text, key, size: text.length });
+ instructionsCache.set(cm, { text, key, size: text.length });Also applies to: 550-553
🤖 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/core/agents/forge.ts` around lines 546 - 547, In buildInstructions ensure
the cache entry always contains the size so getCachedInstructionsSize never
returns undefined: when computing text/key inside buildInstructions, write
instructionsCache.set(cm, { text, key, size: text.length }) unconditionally (not
only inside the snapshot branch) and apply the same unconditional size inclusion
for the other cache writes referenced around the 550-553 block; update any code
paths that currently only call instructionsCache.set(...) when snapshot is
truthy to include the size property so getCachedInstructionsSize can rely on it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…rly budget check, fix UI display