Conversation
…on provider switch (Fixes #1567) The context-window denominator was wrong for Codex models because tokenLimit() had no Codex entries, causing fallback to the 1M default when no profile context-limit override was present. Additionally, remoteTokenStats were only reset when the provider became null rather than on any provider identity change, carrying stale counts across provider switches. Changes: - tokenLimit(): add Codex model detection (256K default, 128K for gpt-5.3-codex-spark). Handles both ID-substring matching (gpt-5.x-codex) and provider-prefix matching (codex:gpt-5.5) for non-suffixed Codex models. Bare non-suffixed IDs remain ambiguous and fall through to the default. - OpenAIProviderContext: reset remoteTokenStats on provider identity change via a ref-tracked previous-value comparison, not just when the provider becomes null.
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
Summary by CodeRabbit
WalkthroughThe PR updates provider-change handling in ChangesOpenAI provider stats reset
tokenLimit Codex / gpt-5.x model resolution
Token accounting and history sync
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ 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}
Contributor
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 (1)
packages/cli/src/ui/contexts/OpenAIProviderContext.tsx (1)
84-95: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winProvider-switch reset can still be skipped due to upstream change detection gap.
Line 85 depends on
providerInfo.providerchanging, but upstreamuseOpenAIProviderInfoonly updates state when model/Responses-API/nullness changes. A non-null provider swap with unchanged model/flags can leaveproviderInfounchanged, so this reset never triggers.Suggested fix (upstream hook condition)
// packages/cli/src/ui/hooks/useOpenAIProviderInfo.ts if ( newInfo.currentModel !== providerInfo.currentModel || newInfo.isResponsesAPI !== providerInfo.isResponsesAPI || + newInfo.provider !== providerInfo.provider || (newInfo.provider !== null) !== (providerInfo.provider !== null) ) { setProviderInfo(newInfo); }🤖 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 `@packages/cli/src/ui/contexts/OpenAIProviderContext.tsx` around lines 84 - 95, The useEffect hook that resets remoteTokenStats depends on providerInfo.provider changing, but the upstream useOpenAIProviderInfo hook does not include provider in its change detection logic. This causes the reset to be skipped when a provider swap occurs with an unchanged model or other flags. Update the useOpenAIProviderInfo hook to include the provider in its dependency array or change detection condition so that any provider change triggers state updates and allows the downstream useEffect in this context to properly reset the token statistics when needed.
🤖 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 `@packages/cli/src/ui/contexts/OpenAIProviderContext.tsx`:
- Around line 84-95: The useEffect hook that resets remoteTokenStats depends on
providerInfo.provider changing, but the upstream useOpenAIProviderInfo hook does
not include provider in its change detection logic. This causes the reset to be
skipped when a provider swap occurs with an unchanged model or other flags.
Update the useOpenAIProviderInfo hook to include the provider in its dependency
array or change detection condition so that any provider change triggers state
updates and allows the downstream useEffect in this context to properly reset
the token statistics when needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 82018331-e52b-4550-bbc2-8640ceb9d119
📒 Files selected for processing (4)
packages/cli/src/ui/contexts/OpenAIProviderContext.test.tsxpackages/cli/src/ui/contexts/OpenAIProviderContext.tsxpackages/core/src/core/tokenLimits.test.tspackages/core/src/core/tokenLimits.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: CodeQL
- GitHub Check: Lint (Javascript)
- GitHub Check: Interactive UI (tmux)
- GitHub Check: Run LLxprt review
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/core/src/core/tokenLimits.test.tspackages/core/src/core/tokenLimits.ts
📚 Learning: 2026-02-15T21:44:56.598Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1407
File: packages/core/src/core/geminiChatHookTriggers.ts:56-65
Timestamp: 2026-02-15T21:44:56.598Z
Learning: Enforce the canonical speaker-to-role mapping used by GeminiChat hooks: in IContent.speaker, which is strictly typed as 'human | ai | tool' (no 'system'), map 'human' to the 'user' role, 'ai' to the 'model' role, and 'tool' to the 'user' role in all hook payloads. This pattern should be applied across related hook files within packages/core/src/core/ (not just the single file) to ensure consistent role assignment.
Applied to files:
packages/core/src/core/tokenLimits.test.tspackages/core/src/core/tokenLimits.ts
📚 Learning: 2026-02-16T19:18:56.265Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1436
File: packages/core/src/core/nonInteractiveToolExecutor.ts:0-0
Timestamp: 2026-02-16T19:18:56.265Z
Learning: Guideline: In the core scheduler architecture, the system runs in a single mode at a time—either interactive or non-interactive, never both on the same scheduler instance. Non-interactive (CLI one-shot) runs without any interactive session; interactive mode subagents run within the parent's interactive context and inherit its mode. When reviewing code, ensure non-interactive tool executions (e.g., in nonInteractiveToolExecutor.ts) create and use a fresh completionResolver per executeToolCall, and that there is no race with interactive sessions since they cannot coexist on the same scheduler instance. This pattern applies across files in packages/core/src/core/. Only apply to relevant files, not globally.
Applied to files:
packages/core/src/core/tokenLimits.test.tspackages/core/src/core/tokenLimits.ts
📚 Learning: 2026-03-22T04:06:53.600Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:391-402
Timestamp: 2026-03-22T04:06:53.600Z
Learning: When computing `lastPromptTokenCount` (e.g., in the streaming path like `_convertIContentStream`/equivalent), ensure it includes the full prompt token footprint: `lastPromptTokenCount = promptTokens + cache_read_input_tokens + cache_creation_input_tokens`. Do not use `promptTokens` alone, because cached context would otherwise cause `CompressionHandler.shouldCompress()` to underestimate context usage and may incorrectly suppress needed compression. Keep this combined computation consistent with the non-streaming path (e.g., `TurnProcessor._executeProviderCall`), and do not treat the presence of cache-token additions as redundant—both token types are required for correctness when cached context is active.
Applied to files:
packages/core/src/core/tokenLimits.test.tspackages/core/src/core/tokenLimits.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/core/src/core/tokenLimits.test.tspackages/core/src/core/tokenLimits.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
packages/core/src/core/tokenLimits.test.tspackages/cli/src/ui/contexts/OpenAIProviderContext.test.tsxpackages/core/src/core/tokenLimits.tspackages/cli/src/ui/contexts/OpenAIProviderContext.tsx
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
packages/core/src/core/tokenLimits.test.tspackages/cli/src/ui/contexts/OpenAIProviderContext.test.tsxpackages/core/src/core/tokenLimits.tspackages/cli/src/ui/contexts/OpenAIProviderContext.tsx
📚 Learning: 2026-06-19T17:16:56.523Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2108
File: packages/agents/src/api/agentImpl.ts:1047-1079
Timestamp: 2026-06-19T17:16:56.523Z
Learning: When the fake-provider test seam is active in vybestack/llxprt-code, `process.env.LLXPRT_FAKE_RESPONSES` is set to a fixture file path ending in a `.jsonl` (not to the string `'1'` or any other boolean-like value). In code, detect the seam by checking `process.env.LLXPRT_FAKE_RESPONSES !== undefined` (and/or that it is a non-empty string), rather than using `process.env.LLXPRT_FAKE_RESPONSES === '1'`. Update any callers of the env var accordingly (see `packages/providers/src/composition/providerManagerInstance.ts` and harness usages).
Applied to files:
packages/core/src/core/tokenLimits.test.tspackages/core/src/core/tokenLimits.ts
📚 Learning: 2026-04-23T23:33:11.797Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1907
File: packages/cli/src/ui/App.test.tsx:1583-1586
Timestamp: 2026-04-23T23:33:11.797Z
Learning: In UI test files under packages/cli/src/ui, prefer test titles/descriptions that state the feature being tested (what behavior the user cares about), even if the assertions specifically exercise a guard/edge path. For example, if the test covers the “empty queue ⇒ no auto-send” branch, a title like “auto-send queued messages” is acceptable; aligning the title perfectly to the exact assertion is optional and not required for lint/enforcement batches.
Applied to files:
packages/cli/src/ui/contexts/OpenAIProviderContext.test.tsx
🔇 Additional comments (3)
packages/cli/src/ui/contexts/OpenAIProviderContext.test.tsx (1)
83-163: LGTM!packages/core/src/core/tokenLimits.ts (1)
90-94: LGTM!Also applies to: 107-119
packages/core/src/core/tokenLimits.test.ts (1)
88-139: LGTM!
Contributor
LLxprt PR Review – PR #2145Title: fix(core): re-estimate tokens, fix cache double-counting on provider switch Issue AlignmentEvidence: Issue #1567 reported context token count exploding (96k → 369k) when switching providers. The PR correctly identifies and fixes three compounding bugs:
The Side Effects
Code QualityCorrectness: The generation-counter pattern for the race fix is sound. The lock closure captures the generation at call time and bails out if it changed. The Error Handling: No new error paths introduced; changes are additive. Data Validation: None needed — changes are arithmetic/accounting. Maintainability: The "input includes cache" invariant is well-commented throughout changed files, which will help future maintainers. Tests and CoverageCoverage impact: Increase Two new dedicated test files:
Both test files use behavioral assertions (e.g., "does NOT reset token accounting when creating a new HistoryService") rather than mock-theater. The Integration tests across 5 files were updated to reflect the new VerdictReady The PR correctly addresses all three root causes of issue #1567 with appropriate behavioral tests. The cache token accounting change is a deliberate, coherent design shift (total-including-cache) that is documented and tested. No blocking issues identified. |
…olling The polling change-detection only compared null/non-null transitions for the provider, so a swap between two non-null providers with unchanged model/API flags left providerInfo stale and the downstream remoteTokenStats reset in OpenAIProviderContext never fired. Replaced the nullness comparison with a direct identity comparison so any provider change triggers a state update.
Collaborator
Author
|
Addressed the CodeRabbit finding (provider-switch reset skipped due to upstream gap): updated the polling change-detection in useOpenAIProviderInfo.ts to use a direct identity comparison (newInfo.provider !== providerInfo.provider) instead of the nullness-only check. This ensures any provider swap — including between two non-null providers with unchanged model/API flags — propagates a state update so the downstream remoteTokenStats reset in OpenAIProviderContext fires correctly. Full CLI suite (4739 tests) passes. |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
…n provider switch (Fixes #1567) Three root causes of context token count blow-up on provider switch: 1. History tokens not re-estimated after provider switch: - When HistoryService is reused across a profile switch, totalTokens retained estimates from the old provider's tokenizer - Added resetTokenAccounting() + recalculateTotalTokens(model) call in ChatSessionFactory when HistoryService is reused 2. Stale sync state race condition: - syncTotalTokens() queued on async tokenizerLock while setBaseTokenOffset() runs synchronously, causing old provider drift to corrupt new count - Added syncGeneration counter to HistoryService; stale queued syncs are now skipped after resetTokenAccounting() increments the generation 3. Cache tokens double-counted for OpenAI/Gemini providers: - Sync formula (promptTokens + cache_read + cache_creation) was correct for Anthropic (non-cached input) but double-counted for OpenAI/Gemini (total input including cache) - Normalized Anthropic promptTokens to include cache, then simplified all sync formulas to use promptTokens directly - Updated telemetryEmitter, tokenUsageTracker, and StatsDisplay to treat input_token_count as total-including-cache consistently
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Combined main's sanitizeTokenCount helper with cache-inclusion fix (promptTokens = sanitizeTokenCount(input) + cacheRead + cacheCreation).
acoliver
changed the title
…ut invariant promptTokens now includes cache tokens for all providers (total-including-cache). The session total formula no longer adds cache separately (it's already in input). Updated test data so input reflects total-including-cache, keeping total assertions unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integration-tests/token-tracking-ui-behavioral.test.ts (1)
319-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression case for negative derived input.
These updated assertions cover the cache-inclusive happy path, but they still don't exercise the new clamp in
packages/cli/src/ui/components/StatsDisplay.tsxwheninputis missing andcached > prompt. Please add a case that would previously have rendered a negativeInput:value so this UI safeguard stays locked in.🤖 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 `@integration-tests/token-tracking-ui-behavioral.test.ts` around lines 319 - 328, Add a regression test that exercises the negative derived input clamp in formatSessionTokenUsage / StatsDisplay when input is missing and cached exceeds prompt. Extend the token-tracking UI behavioral test with a case where combined usage would previously format a negative Input value, and assert the displayed text is clamped to zero instead of showing a negative number. Use the existing formatSessionTokenUsage helper and StatsDisplay-related assertions so the safeguard in packages/cli/src/ui/components/StatsDisplay.tsx stays covered.
🤖 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 `@packages/core/src/services/history/HistoryService.ts`:
- Around line 261-269: The token-accounting reset logic in HistoryService only
invalidates pending syncs inside resetTokenAccounting(), so a queued
syncTotalTokens() can still apply after clear() or dispose() and restore stale
state. Extract the syncGeneration bump into a shared helper in HistoryService
and call it from every state-reset path that should cancel in-flight syncs,
including resetTokenAccounting(), clear(), and dispose(), so all resets
consistently invalidate pending work.
---
Outside diff comments:
In `@integration-tests/token-tracking-ui-behavioral.test.ts`:
- Around line 319-328: Add a regression test that exercises the negative derived
input clamp in formatSessionTokenUsage / StatsDisplay when input is missing and
cached exceeds prompt. Extend the token-tracking UI behavioral test with a case
where combined usage would previously format a negative Input value, and assert
the displayed text is clamped to zero instead of showing a negative number. Use
the existing formatSessionTokenUsage helper and StatsDisplay-related assertions
so the safeguard in packages/cli/src/ui/components/StatsDisplay.tsx stays
covered.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c6606aa-06a8-44f6-8160-16dd36d4086e
⛔ Files ignored due to path filters (1)
packages/cli/src/ui/components/__snapshots__/StatsDisplay.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (20)
integration-tests/token-tracking-behavioral.test.tsintegration-tests/token-tracking-integration.test.tsintegration-tests/token-tracking-provider-behavioral.test.tsintegration-tests/token-tracking-ui-behavioral.test.tsintegration-tests/token-tracking.test.tspackages/agents/src/core/ChatSessionFactory.test.tspackages/agents/src/core/ChatSessionFactory.tokenReestimate.test.tspackages/agents/src/core/ChatSessionFactory.tspackages/agents/src/core/TurnProcessor.tspackages/agents/src/core/chatSession.tokenSync.test.tspackages/agents/src/core/streamResponseHelpers.tspackages/cli/src/ui/components/StatsDisplay.tsxpackages/core/src/services/history/HistoryService.tokenReset.test.tspackages/core/src/services/history/HistoryService.tspackages/providers/src/ProviderManager.test.tspackages/providers/src/anthropic/AnthropicProvider.caching-metrics.test.tspackages/providers/src/anthropic/AnthropicResponseParser.tspackages/providers/src/anthropic/AnthropicStreamProcessor.tspackages/providers/src/logging/telemetryEmitter.tspackages/providers/src/tokenUsageTracker.ts
💤 Files with no reviewable changes (1)
- packages/providers/src/logging/telemetryEmitter.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (macOS)
- GitHub Check: CodeQL
- GitHub Check: Lint (Javascript)
- GitHub Check: Interactive UI (tmux)
- GitHub Check: Run LLxprt review
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/agents/src/core/chatSession.tokenSync.test.tspackages/providers/src/anthropic/AnthropicResponseParser.tspackages/providers/src/anthropic/AnthropicProvider.caching-metrics.test.tspackages/agents/src/core/ChatSessionFactory.tspackages/providers/src/anthropic/AnthropicStreamProcessor.tspackages/agents/src/core/ChatSessionFactory.tokenReestimate.test.tspackages/providers/src/tokenUsageTracker.tspackages/agents/src/core/ChatSessionFactory.test.tsintegration-tests/token-tracking-integration.test.tspackages/providers/src/ProviderManager.test.tsintegration-tests/token-tracking-provider-behavioral.test.tspackages/core/src/services/history/HistoryService.tokenReset.test.tspackages/agents/src/core/TurnProcessor.tsintegration-tests/token-tracking.test.tspackages/core/src/services/history/HistoryService.tspackages/agents/src/core/streamResponseHelpers.tsintegration-tests/token-tracking-ui-behavioral.test.tsintegration-tests/token-tracking-behavioral.test.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/agents/src/core/chatSession.tokenSync.test.tspackages/providers/src/anthropic/AnthropicResponseParser.tspackages/providers/src/anthropic/AnthropicProvider.caching-metrics.test.tspackages/agents/src/core/ChatSessionFactory.tspackages/providers/src/anthropic/AnthropicStreamProcessor.tspackages/agents/src/core/ChatSessionFactory.tokenReestimate.test.tspackages/providers/src/tokenUsageTracker.tspackages/agents/src/core/ChatSessionFactory.test.tspackages/providers/src/ProviderManager.test.tspackages/core/src/services/history/HistoryService.tokenReset.test.tspackages/agents/src/core/TurnProcessor.tspackages/core/src/services/history/HistoryService.tspackages/agents/src/core/streamResponseHelpers.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
packages/agents/src/core/chatSession.tokenSync.test.tspackages/providers/src/anthropic/AnthropicResponseParser.tspackages/providers/src/anthropic/AnthropicProvider.caching-metrics.test.tspackages/agents/src/core/ChatSessionFactory.tspackages/providers/src/anthropic/AnthropicStreamProcessor.tspackages/agents/src/core/ChatSessionFactory.tokenReestimate.test.tspackages/providers/src/tokenUsageTracker.tspackages/agents/src/core/ChatSessionFactory.test.tsintegration-tests/token-tracking-integration.test.tspackages/cli/src/ui/components/StatsDisplay.tsxpackages/providers/src/ProviderManager.test.tsintegration-tests/token-tracking-provider-behavioral.test.tspackages/core/src/services/history/HistoryService.tokenReset.test.tspackages/agents/src/core/TurnProcessor.tsintegration-tests/token-tracking.test.tspackages/core/src/services/history/HistoryService.tspackages/agents/src/core/streamResponseHelpers.tsintegration-tests/token-tracking-ui-behavioral.test.tsintegration-tests/token-tracking-behavioral.test.ts
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
packages/agents/src/core/chatSession.tokenSync.test.tspackages/providers/src/anthropic/AnthropicResponseParser.tspackages/providers/src/anthropic/AnthropicProvider.caching-metrics.test.tspackages/agents/src/core/ChatSessionFactory.tspackages/providers/src/anthropic/AnthropicStreamProcessor.tspackages/agents/src/core/ChatSessionFactory.tokenReestimate.test.tspackages/providers/src/tokenUsageTracker.tspackages/agents/src/core/ChatSessionFactory.test.tsintegration-tests/token-tracking-integration.test.tspackages/cli/src/ui/components/StatsDisplay.tsxpackages/providers/src/ProviderManager.test.tsintegration-tests/token-tracking-provider-behavioral.test.tspackages/core/src/services/history/HistoryService.tokenReset.test.tspackages/agents/src/core/TurnProcessor.tsintegration-tests/token-tracking.test.tspackages/core/src/services/history/HistoryService.tspackages/agents/src/core/streamResponseHelpers.tsintegration-tests/token-tracking-ui-behavioral.test.tsintegration-tests/token-tracking-behavioral.test.ts
📚 Learning: 2026-06-19T17:16:56.523Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2108
File: packages/agents/src/api/agentImpl.ts:1047-1079
Timestamp: 2026-06-19T17:16:56.523Z
Learning: When the fake-provider test seam is active in vybestack/llxprt-code, `process.env.LLXPRT_FAKE_RESPONSES` is set to a fixture file path ending in a `.jsonl` (not to the string `'1'` or any other boolean-like value). In code, detect the seam by checking `process.env.LLXPRT_FAKE_RESPONSES !== undefined` (and/or that it is a non-empty string), rather than using `process.env.LLXPRT_FAKE_RESPONSES === '1'`. Update any callers of the env var accordingly (see `packages/providers/src/composition/providerManagerInstance.ts` and harness usages).
Applied to files:
packages/agents/src/core/chatSession.tokenSync.test.tspackages/providers/src/anthropic/AnthropicResponseParser.tspackages/providers/src/anthropic/AnthropicProvider.caching-metrics.test.tspackages/agents/src/core/ChatSessionFactory.tspackages/providers/src/anthropic/AnthropicStreamProcessor.tspackages/agents/src/core/ChatSessionFactory.tokenReestimate.test.tspackages/providers/src/tokenUsageTracker.tspackages/agents/src/core/ChatSessionFactory.test.tspackages/providers/src/ProviderManager.test.tspackages/core/src/services/history/HistoryService.tokenReset.test.tspackages/agents/src/core/TurnProcessor.tspackages/core/src/services/history/HistoryService.tspackages/agents/src/core/streamResponseHelpers.ts
🔇 Additional comments (12)
packages/core/src/services/history/HistoryService.ts (1)
230-259: LGTM!Also applies to: 441-447
packages/core/src/services/history/HistoryService.tokenReset.test.ts (1)
1-178: LGTM!packages/agents/src/core/ChatSessionFactory.test.ts (1)
58-59: LGTM!Also applies to: 436-437
packages/agents/src/core/ChatSessionFactory.tokenReestimate.test.ts (1)
1-231: LGTM!packages/agents/src/core/ChatSessionFactory.ts (1)
348-352: LGTM!packages/providers/src/anthropic/AnthropicResponseParser.ts (1)
134-137: LGTM!packages/providers/src/anthropic/AnthropicStreamProcessor.ts (1)
242-248: LGTM!Also applies to: 365-379, 401-404, 541-543
packages/providers/src/anthropic/AnthropicProvider.caching-metrics.test.ts (1)
315-316: LGTM!Also applies to: 348-349
packages/providers/src/ProviderManager.test.ts (1)
171-173: LGTM!Also applies to: 182-191, 200-205, 218-218
integration-tests/token-tracking-integration.test.ts (1)
154-156: LGTM!Also applies to: 166-167
integration-tests/token-tracking-behavioral.test.ts (1)
108-109: LGTM!Also applies to: 131-132, 188-189, 209-210, 239-240, 249-250, 260-261, 533-535, 545-546
integration-tests/token-tracking-provider-behavioral.test.ts (1)
467-478: LGTM!Also applies to: 488-489, 556-558, 569-571, 581-582
Comment on lines
+261
to
+269
| resetTokenAccounting(): void { | ||
| this.syncGeneration++; | ||
| this.baseTokenOffset = 0; | ||
| this.emit('tokensUpdated', { | ||
| totalTokens: this.getTotalTokens(), | ||
| addedTokens: 0, | ||
| contentId: null, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Invalidate pending syncs on every token-accounting reset path.
Line 262 only bumps syncGeneration in resetTokenAccounting(). A queued syncTotalTokens() followed by clear() or dispose() can still replay later and restore a non-zero baseTokenOffset onto an emptied service, because those paths never advance the generation. Please extract this invalidation into a shared helper and call it from all state-reset paths, not just provider-switch reset.
🤖 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 `@packages/core/src/services/history/HistoryService.ts` around lines 261 - 269,
The token-accounting reset logic in HistoryService only invalidates pending
syncs inside resetTokenAccounting(), so a queued syncTotalTokens() can still
apply after clear() or dispose() and restore stale state. Extract the
syncGeneration bump into a shared helper in HistoryService and call it from
every state-reset path that should cancel in-flight syncs, including
resetTokenAccounting(), clear(), and dispose(), so all resets consistently
invalidate pending work.
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.
Summary
When switching providers (e.g., Codex → Anthropic via
/profile load), the displayed context token count would blow up dramatically (e.g., 96k → 369k), far beyond what tokenizer differences could explain. This caused the compression guard to throw negative numbers and made the UI unusable.Root cause: Three compounding bugs in the token accounting chain. The user confirmed the chat history SHOULD be reused across provider switches (with counts adjusting for tokenizer differences), but the count was doubling or worse.
Root Cause Analysis
Bug 1: History tokens not re-estimated after provider switch
When the
HistoryServicewas reused across a profile/provider switch,totalTokensretained estimates from the old provider's tokenizer. Switching from GPT (262K context) to Anthropic (200K context) meant the token count was still using GPT estimates — no re-estimation with the Anthropic tokenizer occurred.Fix: Added
resetTokenAccounting()+recalculateTotalTokens(model)calls inChatSessionFactorywhen theHistoryServiceis reused. This re-estimates all conversation history tokens using the new provider's tokenizer.Bug 2: Stale sync state race condition
syncTotalTokens()queues its operation on the asynctokenizerLockchain, whilesetBaseTokenOffset()runs synchronously. When theHistoryServicewas reused, a pendingsyncTotalTokensfrom the old provider ran AFTERsetBaseTokenOffsetfor the new provider, re-applying the old provider's drift correction to the new count — corrupting it.Fix: Added a
syncGenerationcounter toHistoryService. EachresetTokenAccounting()increments the generation; stale queued syncs are now skipped via a generation check inside the async callback.Bug 3: Cache tokens double-counted in sync formula for OpenAI/Gemini
The sync formula
promptTokens + cache_read + cache_creationwas correct for Anthropic (wherepromptTokens= non-cached input only) but double-counted for OpenAI/Gemini (wherepromptTokens= total input INCLUDING cached tokens).Fix: Normalized Anthropic's
promptTokensto include cache tokens (making it consistent across all providers = total input). Then simplified all sync formulas to usepromptTokensdirectly without adding cache tokens. UpdatedtelemetryEmitter,tokenUsageTracker, andStatsDisplayto treatinput_token_countas total-including-cache consistently.Changes
Core token accounting (
packages/core/src/services/history/HistoryService.ts)syncGenerationcounter andresetTokenAccounting()method (zeroes offset, increments generation, emits event)syncTotalTokens()now captures generation at call time and skips if stalerecalculateTotalTokens(modelName?)accepts the actual runtime model instead of hardcodinggpt-4.1Chat session factory (
packages/agents/src/core/ChatSessionFactory.ts)HistoryServiceis reused: callsresetTokenAccounting()thenawait recalculateTotalTokens(model)beforeapplySystemPromptTokenOffsetSync formulas (
packages/agents/src/core/TurnProcessor.ts,streamResponseHelpers.ts)cacheReads + cacheWritesadditions from all token sync paths —promptTokensis now used directlyAnthropic provider (
packages/providers/src/anthropic/AnthropicResponseParser.ts,AnthropicStreamProcessor.ts)promptTokensandtotalTokensnow includecacheRead + cacheCreationfor consistency with OpenAI/GeminiDownstream consumers (
tokenUsageTracker.ts,telemetryEmitter.ts,StatsDisplay.tsx)tokenUsageTracker: removedcacheTokensfrom session total accumulation; fixed hit rate formulatelemetryEmitter: removedcached_content_token_countfrom total token sumStatsDisplay: Input Tokens column now subtracts cached tokens (since input includes cache for all providers)Earlier commits in this PR (still included)
tokenLimits.ts: Codex model entries (256K for*-codex, 128K for*-codex-spark)OpenAIProviderContext.tsx: ResetremoteTokenStatson provider identity changeuseOpenAIProviderInfo.ts: Detect all provider identity changes (not just null transitions)Testing
HistoryServicetoken reset/generation/invalidationChatSessionFactoryre-estimation on reuseFixes #1567