Conversation
#1739) Fixes unbucketed profiles not checking keychain on 401 errors or new turns. Changes: Remove isEnabled() check blocking single-bucket failover, add invalidateAuthCache to BucketFailoverHandler interface, implement cache invalidation in BucketFailoverHandlerImpl, call invalidateAuthCache at turn boundaries in useGeminiStream.
github-actions
bot
added
the
maintainer:e2e:ok
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📜 Recent review details⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🧰 Additional context used🧠 Learnings (44)📓 Common learnings📚 Learning: 2026-03-26T00:29:42.510ZApplied to files:
📚 Learning: 2026-04-03T05:57:51.304ZApplied to files:
📚 Learning: 2026-04-03T05:57:42.326ZApplied to files:
📚 Learning: 2026-03-29T16:31:31.631ZApplied to files:
📚 Learning: 2026-03-26T02:12:39.396ZApplied to files:
📚 Learning: 2026-03-27T00:46:43.700ZApplied to files:
📚 Learning: 2026-03-29T20:44:28.357ZApplied to files:
📚 Learning: 2026-04-03T06:29:38.156ZApplied to files:
📚 Learning: 2026-03-20T01:26:21.401ZApplied to files:
📚 Learning: 2026-03-26T01:27:59.283ZApplied to files:
📚 Learning: 2026-03-26T03:34:18.861ZApplied to files:
📚 Learning: 2026-03-26T00:30:25.258ZApplied to files:
📚 Learning: 2026-03-26T00:29:57.154ZApplied to files:
📚 Learning: 2026-03-26T01:28:33.474ZApplied to files:
📚 Learning: 2026-03-27T01:00:29.058ZApplied to files:
📚 Learning: 2026-03-26T02:05:51.733ZApplied to files:
📚 Learning: 2026-03-27T02:12:12.434ZApplied to files:
📚 Learning: 2026-02-06T15:52:42.315ZApplied to files:
📚 Learning: 2026-03-26T00:49:43.150ZApplied to files:
📚 Learning: 2026-03-27T01:24:59.499ZApplied to files:
📚 Learning: 2026-03-31T02:12:43.093ZApplied to files:
📚 Learning: 2026-03-21T17:07:33.700ZApplied to files:
📚 Learning: 2026-02-15T19:33:03.569ZApplied to files:
📚 Learning: 2026-02-20T04:32:08.940ZApplied to files:
📚 Learning: 2026-02-15T21:44:56.598ZApplied to files:
📚 Learning: 2026-02-16T19:18:56.265ZApplied to files:
📚 Learning: 2026-03-22T04:06:53.600ZApplied to files:
📚 Learning: 2026-03-26T20:52:08.720ZApplied to files:
📚 Learning: 2026-03-26T21:40:39.044ZApplied to files:
📚 Learning: 2026-03-26T02:06:03.086ZApplied to files:
📚 Learning: 2026-03-26T20:01:50.052ZApplied to files:
📚 Learning: 2026-03-27T00:46:42.630ZApplied to files:
📚 Learning: 2026-03-19T23:27:49.587ZApplied to files:
📚 Learning: 2026-04-03T20:43:35.912ZApplied to files:
📚 Learning: 2026-03-25T19:21:43.266ZApplied to files:
📚 Learning: 2026-03-26T02:22:19.732ZApplied to files:
📚 Learning: 2026-03-24T21:07:40.805ZApplied to files:
📚 Learning: 2026-03-26T02:05:48.262ZApplied to files:
📚 Learning: 2026-03-03T17:18:48.615ZApplied to files:
📚 Learning: 2026-03-25T18:17:38.768ZApplied to files:
📚 Learning: 2026-04-03T20:43:54.493ZApplied to files:
📚 Learning: 2026-03-25T12:57:21.017ZApplied to files:
📚 Learning: 2026-03-26T22:12:58.549ZApplied to files:
🔇 Additional comments (5)
Summary by CodeRabbit
WalkthroughAdds an optional auth-cache invalidation hook to the bucket-failover interface, implements it in the CLI handler to call Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "UI Hook\nprepareTurnForQuery"
participant Handler as "BucketFailoverHandlerImpl\ninvalidateAuthCache"
participant AuthCore as "Auth Core\nflushRuntimeAuthScope"
participant Logger as "Logger"
UI->>Handler: invalidateAuthCache(runtimeId)
Handler->>AuthCore: flushRuntimeAuthScope(runtimeId)
AuthCore-->>Handler: ack
Handler->>Logger: debug("invalidated auth cache", runtimeId)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
ReadManyFiles Result (Target Dir:
|
![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/hooks/geminiStream/useGeminiStream.ts (1)
65-83: 🧹 Nitpick | 🔵 TrivialAdd regression coverage for turn-boundary invalidation ordering.
Please add a targeted test that locks the non-continuation sequence (
reset→invalidateAuthCache→ensureBucketsAuthenticated) so this auth-refresh behavior doesn’t regress.Based on learnings: issue
#1783identifiesuseGeminiStream.tsturn-boundary lifecycle triggers as a critical zero-coverage gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts` around lines 65 - 83, Add a targeted unit/integration test for useGeminiStream that asserts the non-continuation turn sequence calls happen in order: when isContinuation is false the handler.reset(), then handler.invalidateAuthCache(runtimeId) (with sessionId from config.getSessionId or 'default'), and finally handler.ensureBucketsAuthenticated(); also ensure the continuation branch calls resetSession() instead of reset(); mock config.getBucketFailoverHandler to expose spies for reset, invalidateAuthCache, resetSession, and ensureBucketsAuthenticated and lock/assert call order (e.g., using jest.fn() call order or a promise chain) to prevent regressions of the reset → invalidateAuthCache → ensureBucketsAuthenticated sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts`:
- Around line 65-83: Add a targeted unit/integration test for useGeminiStream
that asserts the non-continuation turn sequence calls happen in order: when
isContinuation is false the handler.reset(), then
handler.invalidateAuthCache(runtimeId) (with sessionId from config.getSessionId
or 'default'), and finally handler.ensureBucketsAuthenticated(); also ensure the
continuation branch calls resetSession() instead of reset(); mock
config.getBucketFailoverHandler to expose spies for reset, invalidateAuthCache,
resetSession, and ensureBucketsAuthenticated and lock/assert call order (e.g.,
using jest.fn() call order or a promise chain) to prevent regressions of the
reset → invalidateAuthCache → ensureBucketsAuthenticated sequence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3a14921-dfd2-4004-bc9b-d68821f3ecf4
📒 Files selected for processing (4)
packages/cli/src/auth/BucketFailoverHandlerImpl.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.tspackages/core/src/config/configTypes.tspackages/core/src/core/StreamProcessor.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (macOS)
- GitHub Check: Lint (Javascript)
🧰 Additional context used
🧠 Learnings (39)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/token-access-coordinator.ts:442-449
Timestamp: 2026-03-26T00:30:25.258Z
Learning: In `packages/cli/src/auth/token-access-coordinator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), `getToken()` calls `peekOtherProfileBuckets()` even when `explicitBucket` is `true`, and the associated profile-bucket count check can suppress auth when multiple profile buckets exist. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` getOAuthToken method (lines 1136-1412 on main). The bucket resolution logic intentionally supports profile-scoped session buckets with failover. Do not flag this as a bug in decomposition or future PRs — rearchitecting the bucket resolution chain would be a behavioral change beyond refactoring scope.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/auth-flow-orchestrator.ts:431-437
Timestamp: 2026-03-26T02:12:39.396Z
Learning: In `packages/cli/src/auth/auth-flow-orchestrator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the early return in `authenticateMultipleBuckets` when `unauthenticatedBuckets.length === 0` intentionally skips re-enabling OAuth in-memory/settings and skips installing a bucket failover handler. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` around line 2547 on main. Do not flag the missing success-side-effects (provider enablement, failover handler installation) on the all-valid fast path as a bug in decomposition or future PRs — adding these side-effects would be a behavioral change beyond the refactoring scope.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-03-29T16:31:31.631Z
Learning: In vybestack/llxprt-code issue `#1783` (OAuth bucket failover behavioral test spec), the formal scenario catalog was expanded from 39 to ~58 scenarios with four new categories: UE-01–UE-08 (User Entry Points/Lifecycle Triggers), SA-01–SA-04 (Subagent Isolation, tied to PR `#1720/`#1718/#1719), EC-01–EC-04 (Error & Edge Cases), and RO-01–RO-03 (Multi-bucket RetryOrchestrator Integration). Critical zero-coverage gaps are: SB-10 (auth flow mid-turn timeout), UE lifecycle triggers (useGeminiStream.ts turn boundary), SA subagent isolation regressions, and RO multi-bucket retry paths. Two mock-theater test files should be rewritten with MemoryTokenStore: `oauth-manager.failover-wiring.spec.ts` and `oauth-manager.getToken-bucket-peek.spec.ts`. Cross-process simulation uses shared SecureStore/lockDir (two KeyringTokenStore instances), not child_process.fork.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/auth/invalidateProviderCache.test.ts:107-163
Timestamp: 2026-04-03T05:57:42.326Z
Learning: In `packages/core/src/auth/invalidateProviderCache.test.ts` (vybestack/llxprt-code PR `#1874`), the profile-specific positive-match path for `invalidateProviderCache('anthropic', profileId)` is intentionally not directly tested because `resolveAuthentication` does not expose a `profileId` metadata parameter in its public API, making it impossible to construct a profile-keyed cache entry from outside. The wildcard invalidation test (`invalidateProviderCache('anthropic')` with no profileId) exercises the same matching predicate. Do not flag the missing profile-specific positive-match case as a test gap — adding it is a low-priority follow-up that requires internal API changes.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/providers/__tests__/RetryOrchestrator.onAuthError.test.ts:79-404
Timestamp: 2026-04-03T05:57:51.304Z
Learning: In vybestack/llxprt-code, `RetryOrchestrator.onAuthError` tests (packages/core/src/providers/__tests__/RetryOrchestrator.onAuthError.test.ts, PR `#1874`) are intentionally scoped to the default/single-bucket path covering issue1861's core fix. Multi-bucket / non-default session-bucket regression coverage belongs at the OAuthManager/TokenAccessCoordinator layer (forceRefreshToken.test.ts), not in RetryOrchestrator tests. Do not flag the absence of multi-bucket bucket-context tests in RetryOrchestrator.onAuthError.test.ts as a coverage gap.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/token-access-coordinator.ts:640-647
Timestamp: 2026-03-26T03:34:18.861Z
Learning: In `packages/cli/src/auth/token-access-coordinator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the locked disk-check path in `performDiskCheck()` calls `performDiskCheckUnderLock()` without a surrounding try-catch and does not call `scheduleProactiveRenewal()` on the result. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` line ~1308 on main. The proactive renewal call on the unlocked fallback (line ~653) is a targeted addition specific to that bypass path. The locked path feeds into the standard refresh cycle which handles renewal scheduling. Do not flag the missing error guard or missing renewal scheduling on the locked disk-check path as a decomposition regression in future reviews — adding them would be scope expansion beyond the refactoring goal.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/providers/RetryOrchestrator.ts:362-374
Timestamp: 2026-04-03T05:57:34.290Z
Learning: In vybestack/llxprt-code, `OnAuthErrorHandler.handleAuthError` declares `profileId` as optional because `profileId` is not available at the `RetryOrchestrator` level: `GenerateChatOptions` does not carry a `profileId` field and the runtime context does not expose it. The `forceRefreshToken` TOCTOU pattern in `TokenAccessCoordinator` works correctly without a profile qualifier. Do not flag the missing `profileId` in `RetryOrchestrator`'s `handleAuthError` call as a bug — propagating `profileId` through the entire provider call chain is a tracked follow-up, not a defect in the current fix (issue `#1861` / PR `#1874`).
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-02-16T19:05:47.580Z
Learning: Issue `#1442` resolution: Random Codex OAuth failures were caused by stale tokens from the old MultiProviderTokenStore after migration to KeyringTokenStore (commits 916605df, 8aeecbee). Old tokens failed CodexOAuthTokenSchema validation (missing account_id) causing random auth requests. Repeated logout/login manually cleaned up both storage locations. Codex provider lacks legacy cleanup methods that Gemini has (migrateFromLegacyTokens, clearLegacyTokens). Solution: add automatic legacy cleanup to Codex like Gemini.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-02-16T18:58:32.519Z
Learning: Issue `#1442`: Codex OAuth randomly fails with default bucket because `isOAuthEnabled()` checks are unreliable. After authentication, `setOAuthEnabledState()` updates in-memory settings but doesn't flush to disk, causing new processes to see OAuth as disabled. This triggers constant re-authentication requests. Subagents are affected 100% of the time because they create OAuthManager without LoadedSettings, so isOAuthEnabled() always returns false.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/cli/src/auth/__tests__/forceRefreshToken.test.ts:449-477
Timestamp: 2026-04-03T06:29:38.156Z
Learning: In `packages/cli/src/auth/token-access-coordinator.ts` (vybestack/llxprt-code PR `#1874`), `forceRefreshToken` uses a lock-first, single-read TOCTOU pattern: acquire refresh lock → read stored token once (under the lock) → compare stored token with failed access token → act. There is no pre-lock read. The test in `packages/cli/src/auth/__tests__/forceRefreshToken.test.ts` correctly simulates the "another process already refreshed" case by preloading an updated token in the store. Do not flag the absence of a second `getToken` call or suggest asserting `getToken` was called twice — the implementation intentionally reads only once under the lock.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-13T16:04:07.079Z
Learning: Support multi-bucket failover on a single profile: ordered buckets tried in sequence; on 429 or 402 advance to next bucket; on 401 attempt auto-renew then retry once before advancing.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1841
File: packages/cli/src/auth/__tests__/behavioral/user-entry-points.behavioral.spec.ts:131-159
Timestamp: 2026-03-29T20:44:28.357Z
Learning: In `packages/cli/src/auth/__tests__/behavioral/user-entry-points.behavioral.spec.ts` (vybestack/llxprt-code PR `#1841`), UE-03 intentionally calls `handler.resetSession()` directly rather than triggering it through the actual turn-boundary entry point in `useGeminiStream.ts` (L73). This is an explicit scope boundary: `useGeminiStream.ts` is a React hook that requires a full UI render context; testing through it would require mounting React components and simulating user input, which is a different category of integration test. The behavioral spec verifies the handler-level contract (resetSession clears triedBuckets and enables fresh failover). Do not flag the direct `resetSession()` call in this test as missing turn-boundary wiring coverage.
📚 Learning: 2026-03-26T02:12:39.396Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/auth-flow-orchestrator.ts:431-437
Timestamp: 2026-03-26T02:12:39.396Z
Learning: In `packages/cli/src/auth/auth-flow-orchestrator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the early return in `authenticateMultipleBuckets` when `unauthenticatedBuckets.length === 0` intentionally skips re-enabling OAuth in-memory/settings and skips installing a bucket failover handler. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` around line 2547 on main. Do not flag the missing success-side-effects (provider enablement, failover handler installation) on the all-valid fast path as a bug in decomposition or future PRs — adding these side-effects would be a behavioral change beyond the refactoring scope.
Applied to files:
packages/core/src/config/configTypes.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.tspackages/cli/src/auth/BucketFailoverHandlerImpl.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-22T03:20:15.367Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1740
File: packages/core/src/providers/anthropic/AnthropicRateLimitHandler.ts:175-181
Timestamp: 2026-03-22T03:20:15.367Z
Learning: In `packages/core/src/providers/anthropic/AnthropicRateLimitHandler.ts` (vybestack/llxprt-code PR `#1740`), `calculateWaitTime` was refactored in commit 6f62ce987 so that all three buckets (requests, tokens, input tokens) are evaluated before returning. A `shouldWait: true` decision from any bucket takes precedence over warning-only results (where `shouldWait: false` but no reset time is available). The old early-return-on-warning-only pattern was removed. Do not expect or re-introduce the early-exit behavior.
Applied to files:
packages/core/src/config/configTypes.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.tspackages/cli/src/auth/BucketFailoverHandlerImpl.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-26T00:29:57.154Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/OAuthBucketManager.ts:36-47
Timestamp: 2026-03-26T00:29:57.154Z
Learning: In `packages/cli/src/auth/OAuthBucketManager.ts` (vybestack/llxprt-code), `getSessionBucketScopeKey(provider, metadata?)` must remain public — it is called externally by `packages/cli/src/auth/token-bucket-failover-helper.ts` (lines 66 and 73) to compute session/bucket scope keys. Do not suggest making it private in code reviews.
Applied to files:
packages/core/src/config/configTypes.tspackages/cli/src/auth/BucketFailoverHandlerImpl.ts
📚 Learning: 2026-03-21T17:15:03.717Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:275-318
Timestamp: 2026-03-21T17:15:03.717Z
Learning: In `packages/core/src/core/TurnProcessor.ts` (vybestack/llxprt-code PR `#1743`), the non-streaming `sendMessage()` path in `_executeSendWithRetry` was improved to pass `params.config?.abortSignal` to `retryWithBackoff` (added in commit 870e85a91, retained). A matching config merge (`params.config` into the provider runtime context) was attempted but reverted in commit 88d894840 because spreading `GenerateContentConfig` into a `Config` class instance loses prototype methods (e.g., `getUserMemory()`), causing TS2339 errors in CI. Do not suggest re-adding the object-spread config merge — the correct approach would require a targeted method that selectively forwards GenerateContentConfig fields without destroying the Config class prototype. The abortSignal improvement is retained and should not be reverted.
Applied to files:
packages/core/src/config/configTypes.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-26T01:28:01.197Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/auth-flow-orchestrator.ts:692-705
Timestamp: 2026-03-26T01:28:01.197Z
Learning: In vybestack/llxprt-code, the production `KeyringTokenStore` (at `packages/core/src/auth/keyring-token-store.ts` line 94) normalizes an `undefined` bucket argument to `DEFAULT_BUCKET` (the string `"default"`) internally via `const resolvedBucket = bucket ?? DEFAULT_BUCKET`. As a result, `getToken(provider, undefined)` and `getToken(provider, "default")` are functionally identical in production. The original `oauth-manager.ts` also applied the same `bucket ?? "default"` normalization (line 2169). Do not flag the omission of explicit `"default"` bucket strings in `auth-flow-orchestrator.ts` or related decomposed modules as a behavioral divergence — passing `undefined` is equivalent and is a faithful extraction of pre-existing behavior. InMemoryTokenStore behavior in test helpers does not govern the production contract.
Applied to files:
packages/core/src/config/configTypes.ts
📚 Learning: 2026-03-26T00:29:42.510Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/BucketFailoverHandlerImpl.spec.ts:500-503
Timestamp: 2026-03-26T00:29:42.510Z
Learning: In `packages/cli/src/auth/BucketFailoverHandlerImpl.spec.ts` (vybestack/llxprt-code), the `if (provider)` guard pattern used after `oauthManager.getProvider(...)` to conditionally stub `provider.refreshToken` is pre-existing from the original test suite. Do not flag this as a silent-skip risk or suggest hardening (e.g., `expect(provider).toBeDefined()`) in decomposition or refactoring PRs — changing test structure is explicitly out of scope for those PRs.
Applied to files:
packages/core/src/config/configTypes.tspackages/cli/src/auth/BucketFailoverHandlerImpl.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-26T00:30:25.258Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/token-access-coordinator.ts:442-449
Timestamp: 2026-03-26T00:30:25.258Z
Learning: In `packages/cli/src/auth/token-access-coordinator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), `getToken()` calls `peekOtherProfileBuckets()` even when `explicitBucket` is `true`, and the associated profile-bucket count check can suppress auth when multiple profile buckets exist. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` getOAuthToken method (lines 1136-1412 on main). The bucket resolution logic intentionally supports profile-scoped session buckets with failover. Do not flag this as a bug in decomposition or future PRs — rearchitecting the bucket resolution chain would be a behavioral change beyond refactoring scope.
Applied to files:
packages/core/src/config/configTypes.tspackages/cli/src/auth/BucketFailoverHandlerImpl.ts
📚 Learning: 2026-02-20T04:32:08.940Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-02-20T04:32:08.940Z
Learning: In vybestack/llxprt-code, provider retries and OAuth bucket failover are centralized in packages/core/src/providers/RetryOrchestrator.ts. Failover triggers: 2×429, immediate 402, or 2nd 401/403. On success, counters and failover session reset; on switch, attempt-- so retries don’t burn on bucket change.
Applied to files:
packages/core/src/config/configTypes.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.tspackages/cli/src/auth/BucketFailoverHandlerImpl.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-04-03T05:57:51.304Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/providers/__tests__/RetryOrchestrator.onAuthError.test.ts:79-404
Timestamp: 2026-04-03T05:57:51.304Z
Learning: In vybestack/llxprt-code, `RetryOrchestrator.onAuthError` tests (packages/core/src/providers/__tests__/RetryOrchestrator.onAuthError.test.ts, PR `#1874`) are intentionally scoped to the default/single-bucket path covering issue1861's core fix. Multi-bucket / non-default session-bucket regression coverage belongs at the OAuthManager/TokenAccessCoordinator layer (forceRefreshToken.test.ts), not in RetryOrchestrator tests. Do not flag the absence of multi-bucket bucket-context tests in RetryOrchestrator.onAuthError.test.ts as a coverage gap.
Applied to files:
packages/core/src/config/configTypes.tspackages/cli/src/auth/BucketFailoverHandlerImpl.ts
📚 Learning: 2026-03-29T16:31:31.631Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-03-29T16:31:31.631Z
Learning: In vybestack/llxprt-code issue `#1783` (OAuth bucket failover behavioral test spec), the formal scenario catalog was expanded from 39 to ~58 scenarios with four new categories: UE-01–UE-08 (User Entry Points/Lifecycle Triggers), SA-01–SA-04 (Subagent Isolation, tied to PR `#1720/`#1718/#1719), EC-01–EC-04 (Error & Edge Cases), and RO-01–RO-03 (Multi-bucket RetryOrchestrator Integration). Critical zero-coverage gaps are: SB-10 (auth flow mid-turn timeout), UE lifecycle triggers (useGeminiStream.ts turn boundary), SA subagent isolation regressions, and RO multi-bucket retry paths. Two mock-theater test files should be rewritten with MemoryTokenStore: `oauth-manager.failover-wiring.spec.ts` and `oauth-manager.getToken-bucket-peek.spec.ts`. Cross-process simulation uses shared SecureStore/lockDir (two KeyringTokenStore instances), not child_process.fork.
Applied to files:
packages/core/src/config/configTypes.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.tspackages/cli/src/auth/BucketFailoverHandlerImpl.ts
📚 Learning: 2026-04-03T05:57:42.326Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/auth/invalidateProviderCache.test.ts:107-163
Timestamp: 2026-04-03T05:57:42.326Z
Learning: In `packages/core/src/auth/invalidateProviderCache.test.ts` (vybestack/llxprt-code PR `#1874`), the profile-specific positive-match path for `invalidateProviderCache('anthropic', profileId)` is intentionally not directly tested because `resolveAuthentication` does not expose a `profileId` metadata parameter in its public API, making it impossible to construct a profile-keyed cache entry from outside. The wildcard invalidation test (`invalidateProviderCache('anthropic')` with no profileId) exercises the same matching predicate. Do not flag the missing profile-specific positive-match case as a test gap — adding it is a low-priority follow-up that requires internal API changes.
Applied to files:
packages/core/src/config/configTypes.tspackages/cli/src/auth/BucketFailoverHandlerImpl.ts
📚 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/config/configTypes.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.tspackages/cli/src/auth/BucketFailoverHandlerImpl.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-21T00:08:34.456Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1741
File: packages/core/src/config/configTypes.ts:293-293
Timestamp: 2026-03-21T00:08:34.456Z
Learning: Do not flag or refactor the inline type pattern like `lsp?: import('../lsp/types.js').LspConfig | boolean` in `packages/core/src/config/configTypes.ts` and `packages/core/src/config/config.ts`. This deliberate inline import is used to represent optional LSP dependency types, and reviewers should avoid suggesting a replacement with a top-level `import type` statement since the optionality signal is intentional.
Applied to files:
packages/core/src/config/configTypes.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/config/configTypes.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.tspackages/cli/src/auth/BucketFailoverHandlerImpl.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-29T20:44:28.357Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1841
File: packages/cli/src/auth/__tests__/behavioral/user-entry-points.behavioral.spec.ts:131-159
Timestamp: 2026-03-29T20:44:28.357Z
Learning: In `packages/cli/src/auth/__tests__/behavioral/user-entry-points.behavioral.spec.ts` (vybestack/llxprt-code PR `#1841`), UE-03 intentionally calls `handler.resetSession()` directly rather than triggering it through the actual turn-boundary entry point in `useGeminiStream.ts` (L73). This is an explicit scope boundary: `useGeminiStream.ts` is a React hook that requires a full UI render context; testing through it would require mounting React components and simulating user input, which is a different category of integration test. The behavioral spec verifies the handler-level contract (resetSession clears triedBuckets and enables fresh failover). Do not flag the direct `resetSession()` call in this test as missing turn-boundary wiring coverage.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-26T20:01:50.052Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/checkpointPersistence.ts:0-0
Timestamp: 2026-03-26T20:01:50.052Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/checkpointPersistence.ts` (vybestack/llxprt-code PR `#1780`), `saveRestorableToolCalls` reserves the `callId` in `checkpointedCallIds` *before* calling `await createToolCheckpoint(...)` to prevent concurrent duplicate attempts. On failure (exception or false/no-write return), the `callId` is deleted from the set so the next effect run can retry. This reserve-before-await, delete-on-failure pattern was introduced in commit a41b97759.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-26T21:40:39.044Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:521-525
Timestamp: 2026-03-26T21:40:39.044Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts` (vybestack/llxprt-code PR `#1780`), `processGeminiStreamEvents` previously always returned `StreamProcessingStatus.Completed` even when a `UserCancelled` or `Error` event was received — this was pre-existing dead code from the original `useGeminiStream.ts` (line 1371 checked for UserCancelled but the function always returned Completed). Fixed in commit 4a28363bc by adding a `processingResult` variable that tracks the status through the event loop, setting it to `StreamProcessingStatus.UserCancelled` or `StreamProcessingStatus.Error` as appropriate, and returning it at the end instead of always returning `Completed`.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-04-03T20:43:35.912Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1876
File: packages/core/src/core/subagent.ts:441-464
Timestamp: 2026-04-03T20:43:35.912Z
Learning: In `packages/core/src/core/subagent.ts` (vybestack/llxprt-code), `runInteractiveTurn`'s `catch` block should only call `throw createAbortError()` when `abortController.signal.aborted` — it must NOT infer `SubagentTerminateMode.TIMEOUT` based on `!this.parentAbortSignal?.aborted`. `Turn.run()` absorbs idle-timeout events internally and emits `GeminiEventType.UserCancelled` before the generator exits, so the catch only fires for genuine errors or explicit cancel aborts. TIMEOUT is authoritatively set in the `onTimeout` callback of `nextStreamEventWithIdleTimeout` inside `runNonInteractiveTurn`. Do not conflate these two paths in future reviews.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-26T20:52:08.720Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:140-149
Timestamp: 2026-03-26T20:52:08.720Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts` (vybestack/llxprt-code PR `#1780`), the `applyThoughtToState` function's `setPendingHistoryItem` updater casts `item?.type as 'gemini' | 'gemini_content'` and uses `item?.text || ''` — this is intentional pre-existing behavior faithfully extracted verbatim from the original `useGeminiStream.ts` (lines 1110–1111 on main). The risk that a `tool_group` pending item type could be preserved is a known pre-existing pattern; fixing it requires careful testing of all thinking block rendering paths and is a behavioral change beyond decomposition scope. Do not flag this type cast as a bug in decomposition or refactoring PRs.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-26T02:22:12.153Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/checkpointPersistence.ts:145-153
Timestamp: 2026-03-26T02:22:12.153Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/checkpointPersistence.ts` (vybestack/llxprt-code PR `#1780`), `saveRestorableToolCalls` intentionally does NOT deduplicate checkpoint writes by `callId`. Each effect re-run may re-checkpoint the same `awaiting_approval` tool call (producing multiple snapshot files/git snapshots). This is pre-existing behavior faithfully extracted verbatim from the original `useGeminiStream.ts`. Adding per-callId deduplication would be a new feature beyond the decomposition scope. Do not flag the absence of a checkpointed-callId Set as a bug in decomposition or refactoring PRs.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-21T15:17:37.899Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:410-421
Timestamp: 2026-03-21T15:17:37.899Z
Learning: In `packages/core/src/core/TurnProcessor.ts` and `packages/core/src/core/compression/CompressionHandler.ts` (vybestack/llxprt-code PR `#1743`), both classes maintain a `lastPromptTokenCount` field. After the decomposition of `geminiChat.ts`, `TurnProcessor._executeProviderCall` must update both `this.lastPromptTokenCount` AND `this.compressionHandler.lastPromptTokenCount` (via `setLastPromptTokenCount`) whenever prompt token counts are received from the non-streaming provider response. The streaming path in `StreamProcessor` already updates `compressionHandler.lastPromptTokenCount` directly. Do not flag the dual-update pattern as redundant — `CompressionHandler.shouldCompress()` reads its own field and will use stale data if only `TurnProcessor.lastPromptTokenCount` is updated. Fixed in commit bac2e4153.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-21T17:07:33.700Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/StreamProcessor.ts:123-135
Timestamp: 2026-03-21T17:07:33.700Z
Learning: In `packages/core/src/core/StreamProcessor.ts` (vybestack/llxprt-code PR `#1743`), the retry boundary in `_executeStreamApiCall` only protects async generator *construction* (via `_buildAndSendStreamRequest`), not the actual network calls that occur during iteration inside `_aggregateStreamChunks`. This is a known pre-existing architectural limitation faithfully preserved from the original `geminiChat.ts`. The issue is tracked as `#1750` for the 0.10.0 improvement effort. Do not re-flag this as a regression or decomposition bug in future reviews — it is intentionally deferred.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-26T02:22:19.732Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/streamUtils.ts:100-116
Timestamp: 2026-03-26T02:22:19.732Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/streamUtils.ts` (vybestack/llxprt-code PR `#1780`), the `filteredPendingTools` predicate in `mergePendingToolGroupsForDisplay` only removes overlapping shell entries from the scheduler side, leaving overlapping non-shell tool callIds potentially duplicated. This is pre-existing behavior faithfully extracted verbatim from the original `useGeminiStream.ts` (lines 100-140). Do not flag this as a dedup bug in decomposition or refactoring PRs — any improvement to deduplicate non-shell overlapping callIds would be a separate enhancement, not a structural fix.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-25T12:57:21.017Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1768
File: packages/cli/src/ui/hooks/useCoreEventHandlers.ts:71-78
Timestamp: 2026-03-25T12:57:21.017Z
Learning: In files under `packages/cli/src/ui/hooks/`, it is OK for a `useEffect` to register process-level cleanup via `registerCleanup(consolePatcher.cleanup)` without returning a React cleanup function (e.g., without `return () => consolePatcher.cleanup()`). Treat this as an intentional pattern: `registerCleanup()` is the project’s mechanism for cleanup that is meant to persist beyond React unmount/re-render, so do not flag the missing `return` as a leak/bug.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-26T22:12:58.549Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:1-5
Timestamp: 2026-03-26T22:12:58.549Z
Learning: When reviewing this repo, do not flag these files for missing "upstream" Google LLC copyright. For `useGeminiStream.ts` and the extracted geminiStream-hook modules (under `packages/cli/src/ui/hooks/geminiStream/`), ensure the copyright header is `Copyright 2025 Vybestack LLC`—these files were already updated to Vybestack LLC before PR `#1780` and the decomposition outputs correctly use Vybestack LLC.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-24T21:33:43.130Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1766
File: packages/cli/src/runtime/providerMutations.ts:258-265
Timestamp: 2026-03-24T21:33:43.130Z
Learning: In `packages/cli/src/runtime/providerMutations.ts` (vybestack/llxprt-code PR `#1766`, decomposed from the original `runtimeSettings.ts`), `updateActiveProviderApiKey` intentionally does NOT clear `auth-key` in `settingsService` during the update (non-null key) branch. The update branch sets the ephemeral `auth-key` to the new value instead; only the remove (null/empty key) branch clears all aliases in both `settingsService` and config ephemeral settings. This asymmetry is pre-existing behavior from the original `runtimeSettings.ts` (lines 2182-2187 on main). Do not flag this as a credential-leak or stale-alias bug in decomposition reviews.
Applied to files:
packages/cli/src/auth/BucketFailoverHandlerImpl.ts
📚 Learning: 2026-02-18T01:44:54.715Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1459
File: packages/core/src/providers/LoadBalancingProvider.ts:631-648
Timestamp: 2026-02-18T01:44:54.715Z
Learning: Load balancer failover and bucket failover are two separate mechanisms: Load balancer failover (LoadBalancingProvider) distributes requests across different backends/providers and treats 401 as an immediate failover error. Bucket failover (BucketFailoverHandlerImpl) handles credential rotation within a single provider and may implement auto-renew semantics for 401. These systems have different 401 handling behaviors by design.
Applied to files:
packages/cli/src/auth/BucketFailoverHandlerImpl.tspackages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-01-13T23:04:39.246Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-13T23:04:39.246Z
Learning: Treat Anthropic 403 permission_error "OAuth token has been revoked" as an expired/auth error: trigger a one-time refresh retry, then bucket failover if refresh fails. Avoid auto-switching providers; only switch buckets within the same provider. This error likely means that we failed to lock when refreshing and multiple instances refreshed the same token.
Applied to files:
packages/cli/src/auth/BucketFailoverHandlerImpl.ts
📚 Learning: 2026-03-26T01:27:59.283Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/auth-flow-orchestrator.ts:692-705
Timestamp: 2026-03-26T01:27:59.283Z
Learning: When reviewing code under packages/cli/src/auth (including decomposed modules related to auth-flow orchestration), do not treat missing explicit bucket strings as a production behavioral change if the production token store normalizes `bucket` with `bucket ?? DEFAULT_BUCKET` (where `DEFAULT_BUCKET` is the string "default"). In particular, passing `undefined` for `bucket` should be considered equivalent to passing "default", so calls like `getToken(provider, undefined)` are functionally identical to `getToken(provider, "default")` in production. This normalization is part of the existing production contract (e.g., KeyringTokenStore / prior oauth-manager behavior); InMemoryTokenStore/test helpers do not govern that production behavior.
Applied to files:
packages/cli/src/auth/BucketFailoverHandlerImpl.ts
📚 Learning: 2026-03-26T01:28:33.474Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/provider-usage-info.ts:36-37
Timestamp: 2026-03-26T01:28:33.474Z
Learning: When doing token lookups via `tokenStore.getToken(provider, bucket)` in `packages/cli/src/auth`, treat `bucket ?? 'default'` as correct when the goal is to normalize missing buckets to the default bucket. The production `KeyringTokenStore` already normalizes `undefined` to `DEFAULT_BUCKET` (`"default"`), and this pattern was also used in the original `oauth-manager.ts`, so using `bucket ?? 'default'` here is faithful and should not be flagged as a contract/lookup violation nor used to recommend splitting `undefined` (lookup) vs `'default'` (display) behavior across this codebase.
Applied to files:
packages/cli/src/auth/BucketFailoverHandlerImpl.ts
📚 Learning: 2026-03-27T01:24:59.499Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1784
File: packages/cli/src/auth/codex-oauth-provider.ts:394-402
Timestamp: 2026-03-27T01:24:59.499Z
Learning: In `packages/cli/src/auth/**` (including `codex-oauth-provider.ts` and related OAuth bridge files), `-1` is an intentional sentinel value used by stub `addItem` callbacks where no real history ID is available (for example `p.setAddItem?.(() => -1)`). Reviewers should not flag this as a magic number, nor suggest replacing it with a named constant, since callers never consume the returned ID and the literal `-1` is an established convention in this codebase.
Applied to files:
packages/cli/src/auth/BucketFailoverHandlerImpl.ts
📚 Learning: 2026-03-21T15:50:32.992Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1740
File: packages/core/src/providers/anthropic/AnthropicStreamProcessor.ts:75-192
Timestamp: 2026-03-21T15:50:32.992Z
Learning: In `packages/core/src/providers/anthropic/AnthropicStreamProcessor.ts` (vybestack/llxprt-code PR `#1740`), stream retry after transient errors is guarded by a `hasYieldedContent` flag. If a transient network error occurs mid-stream after text, tool calls, or thinking blocks have been yielded, the generator throws immediately instead of retrying (to avoid duplicate/corrupted output), delegating recovery to the upper-layer RetryOrchestrator. Metadata-only yields (usage from `message_start`) do NOT set this flag, so retries before any real content are still allowed. Fixed in commit e5aa5043.
Applied to files:
packages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-21T15:18:51.729Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/StreamProcessor.ts:123-135
Timestamp: 2026-03-21T15:18:51.729Z
Learning: In `packages/core/src/core/StreamProcessor.ts` and original `packages/core/src/core/geminiChat.ts` (vybestack/llxprt-code PR `#1743`), the streaming path (`makeApiCallAndProcessStream` / `sendMessageStream`) intentionally does NOT call `_enrichSchemaDepthError` (formerly `maybeIncludeSchemaDepthContext`). In the original `geminiChat.ts`, `maybeIncludeSchemaDepthContext` was only called at approximately line 1106 inside the `sendMessage()` catch block. The `StreamProcessor._enrichSchemaDepthError` method exists for potential future use but was never wired in the streaming path. Do not flag this omission as a bug in future reviews — it faithfully preserves the original behavior.
Applied to files:
packages/core/src/core/StreamProcessor.ts
📚 Learning: 2026-03-21T17:57:02.742Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1740
File: packages/core/src/providers/anthropic/AnthropicStreamProcessor.ts:0-0
Timestamp: 2026-03-21T17:57:02.742Z
Learning: In `packages/core/src/providers/anthropic/AnthropicStreamProcessor.ts` (vybestack/llxprt-code PR `#1740`), the guard `chunk.type === 'message_delta' && chunk.usage` (around line 149) is intentional pre-existing behavior faithfully extracted verbatim from the original `AnthropicProvider.ts`. `message_delta` events without a `usage` field are intentionally skipped. Do not flag the `&& chunk.usage` condition as a bug or suggest removing it in decomposition reviews — any change to process usage-less `message_delta` events would be a behavioral modification beyond the decomposition scope.
Applied to files:
packages/core/src/core/StreamProcessor.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/StreamProcessor.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/StreamProcessor.ts
📚 Learning: 2026-03-21T17:07:10.889Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/ConversationManager.ts:94-101
Timestamp: 2026-03-21T17:07:10.889Z
Learning: In these core processor modules, ensure `makePositionMatcher()` is created once and reused across the entire batch (i.e., hoist it outside any per-`content` loops) rather than rebuilding it for each content item. Also ensure the `unmatched-tool` queue is consumed in-order across all batch items (not reset/handled independently per item), since that preserves correct batch-wide behavior.
Applied to files:
packages/core/src/core/StreamProcessor.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/StreamProcessor.ts
🔇 Additional comments (4)
packages/core/src/config/configTypes.ts (1)
330-334: Backward-compatible interface extension looks correct.Adding
invalidateAuthCacheas an optional method is the right shape for incremental adoption across handler implementations.packages/cli/src/auth/BucketFailoverHandlerImpl.ts (1)
667-670:invalidateAuthCacheimplementation is clean and focused.This correctly delegates to runtime-scope auth cache flushing and keeps observability with a debug log.
packages/core/src/core/StreamProcessor.ts (1)
463-463: Failover guard simplification is correct for single-bucket support.Proceeding when a handler exists (instead of checking
isEnabled()) aligns with the intended unbucketed-profile recovery behavior.packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts (1)
75-76: No action required.The call to
invalidateAuthCachecorrectly usesgetSessionId()as theruntimeIdparameter. This identifier aligns with how the main runtime is keyed when created—resolveRuntimeId(config)defaults toconfig.getSessionId()when no explicitruntimeIdoverride is provided. TheflushRuntimeAuthScope(runtimeId)function looks up the runtime's auth scope using this same key, so there is no mismatch or silent no-op risk.
Adds regression coverage for BucketFailoverHandlerImpl.invalidateAuthCache and single-bucket StreamProcessor failover handling for issue #1739.
Collaborator
Author
|
Addressed the missing automated coverage noted in review. What I added:
Targeted verification run:
Both targeted regression tests pass. |
Exports prepareTurnForQuery for direct unit coverage and adds regression tests for new-turn and continuation ordering in issue #1739.
Collaborator
Author
|
Addressed the remaining turn-boundary ordering review comment. What I changed:
Targeted verification run:
The new targeted regression tests pass. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts`:
- Around line 26-33: The oauthManager mock for the
BucketFailoverOAuthManagerLike interface is missing the required
forceRefreshToken method causing TS2741; update the oauthManager object to
include a forceRefreshToken property mocked with vi.fn() (matching how other
tests like auth-flow-orchestrator.spec.ts and token-access-coordinator.spec.ts
implement it) so the mock satisfies the interface contract and compiles.
In
`@packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts`:
- Around line 59-99: The test labeled as covering the default runtime-id
actually calls prepareTurnForQuery with isContinuation = true so the default
fallback path (and invalidateAuthCache behavior) is not exercised; add a new
test that calls prepareTurnForQuery with isContinuation = false (or duplicate
the existing test but set the first arg to false) and assert that
handler.invalidateAuthCache is called with the expected default runtime id and
that the call order reflects invalidate before ensure/reset as appropriate;
reference prepareTurnForQuery and handler.invalidateAuthCache to locate where to
change/add the test and mirror the existing test setup (startNewPrompt,
setThought, thinkingBlocksRef) while adjusting expectations for
invalidateAuthCache and any related handler.reset/startNewPrompt calls.
🪄 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
Run ID: c376d375-a28d-4a2e-a61e-3dac57cdd742
📒 Files selected for processing (4)
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.tspackages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (45)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/auth-flow-orchestrator.ts:431-437
Timestamp: 2026-03-26T02:12:39.396Z
Learning: In `packages/cli/src/auth/auth-flow-orchestrator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the early return in `authenticateMultipleBuckets` when `unauthenticatedBuckets.length === 0` intentionally skips re-enabling OAuth in-memory/settings and skips installing a bucket failover handler. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` around line 2547 on main. Do not flag the missing success-side-effects (provider enablement, failover handler installation) on the all-valid fast path as a bug in decomposition or future PRs — adding these side-effects would be a behavioral change beyond the refactoring scope.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-03-29T16:31:31.631Z
Learning: In vybestack/llxprt-code issue `#1783` (OAuth bucket failover behavioral test spec), the formal scenario catalog was expanded from 39 to ~58 scenarios with four new categories: UE-01–UE-08 (User Entry Points/Lifecycle Triggers), SA-01–SA-04 (Subagent Isolation, tied to PR `#1720/`#1718/#1719), EC-01–EC-04 (Error & Edge Cases), and RO-01–RO-03 (Multi-bucket RetryOrchestrator Integration). Critical zero-coverage gaps are: SB-10 (auth flow mid-turn timeout), UE lifecycle triggers (useGeminiStream.ts turn boundary), SA subagent isolation regressions, and RO multi-bucket retry paths. Two mock-theater test files should be rewritten with MemoryTokenStore: `oauth-manager.failover-wiring.spec.ts` and `oauth-manager.getToken-bucket-peek.spec.ts`. Cross-process simulation uses shared SecureStore/lockDir (two KeyringTokenStore instances), not child_process.fork.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/providers/__tests__/RetryOrchestrator.onAuthError.test.ts:79-404
Timestamp: 2026-04-03T05:57:51.304Z
Learning: In vybestack/llxprt-code, `RetryOrchestrator.onAuthError` tests (packages/core/src/providers/__tests__/RetryOrchestrator.onAuthError.test.ts, PR `#1874`) are intentionally scoped to the default/single-bucket path covering issue1861's core fix. Multi-bucket / non-default session-bucket regression coverage belongs at the OAuthManager/TokenAccessCoordinator layer (forceRefreshToken.test.ts), not in RetryOrchestrator tests. Do not flag the absence of multi-bucket bucket-context tests in RetryOrchestrator.onAuthError.test.ts as a coverage gap.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/token-access-coordinator.ts:442-449
Timestamp: 2026-03-26T00:30:25.258Z
Learning: In `packages/cli/src/auth/token-access-coordinator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), `getToken()` calls `peekOtherProfileBuckets()` even when `explicitBucket` is `true`, and the associated profile-bucket count check can suppress auth when multiple profile buckets exist. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` getOAuthToken method (lines 1136-1412 on main). The bucket resolution logic intentionally supports profile-scoped session buckets with failover. Do not flag this as a bug in decomposition or future PRs — rearchitecting the bucket resolution chain would be a behavioral change beyond refactoring scope.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/auth/invalidateProviderCache.test.ts:107-163
Timestamp: 2026-04-03T05:57:42.326Z
Learning: In `packages/core/src/auth/invalidateProviderCache.test.ts` (vybestack/llxprt-code PR `#1874`), the profile-specific positive-match path for `invalidateProviderCache('anthropic', profileId)` is intentionally not directly tested because `resolveAuthentication` does not expose a `profileId` metadata parameter in its public API, making it impossible to construct a profile-keyed cache entry from outside. The wildcard invalidation test (`invalidateProviderCache('anthropic')` with no profileId) exercises the same matching predicate. Do not flag the missing profile-specific positive-match case as a test gap — adding it is a low-priority follow-up that requires internal API changes.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1841
File: packages/cli/src/auth/__tests__/behavioral/user-entry-points.behavioral.spec.ts:131-159
Timestamp: 2026-03-29T20:44:28.357Z
Learning: In `packages/cli/src/auth/__tests__/behavioral/user-entry-points.behavioral.spec.ts` (vybestack/llxprt-code PR `#1841`), UE-03 intentionally calls `handler.resetSession()` directly rather than triggering it through the actual turn-boundary entry point in `useGeminiStream.ts` (L73). This is an explicit scope boundary: `useGeminiStream.ts` is a React hook that requires a full UI render context; testing through it would require mounting React components and simulating user input, which is a different category of integration test. The behavioral spec verifies the handler-level contract (resetSession clears triedBuckets and enables fresh failover). Do not flag the direct `resetSession()` call in this test as missing turn-boundary wiring coverage.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/token-access-coordinator.ts:640-647
Timestamp: 2026-03-26T03:34:18.861Z
Learning: In `packages/cli/src/auth/token-access-coordinator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the locked disk-check path in `performDiskCheck()` calls `performDiskCheckUnderLock()` without a surrounding try-catch and does not call `scheduleProactiveRenewal()` on the result. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` line ~1308 on main. The proactive renewal call on the unlocked fallback (line ~653) is a targeted addition specific to that bypass path. The locked path feeds into the standard refresh cycle which handles renewal scheduling. Do not flag the missing error guard or missing renewal scheduling on the locked disk-check path as a decomposition regression in future reviews — adding them would be scope expansion beyond the refactoring goal.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/cli/src/auth/__tests__/forceRefreshToken.test.ts:449-477
Timestamp: 2026-04-03T06:29:38.156Z
Learning: In `packages/cli/src/auth/token-access-coordinator.ts` (vybestack/llxprt-code PR `#1874`), `forceRefreshToken` uses a lock-first, single-read TOCTOU pattern: acquire refresh lock → read stored token once (under the lock) → compare stored token with failed access token → act. There is no pre-lock read. The test in `packages/cli/src/auth/__tests__/forceRefreshToken.test.ts` correctly simulates the "another process already refreshed" case by preloading an updated token in the store. Do not flag the absence of a second `getToken` call or suggest asserting `getToken` was called twice — the implementation intentionally reads only once under the lock.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:275-318
Timestamp: 2026-03-21T17:15:03.717Z
Learning: In `packages/core/src/core/TurnProcessor.ts` (vybestack/llxprt-code PR `#1743`), the non-streaming `sendMessage()` path in `_executeSendWithRetry` was improved to pass `params.config?.abortSignal` to `retryWithBackoff` (added in commit 870e85a91, retained). A matching config merge (`params.config` into the provider runtime context) was attempted but reverted in commit 88d894840 because spreading `GenerateContentConfig` into a `Config` class instance loses prototype methods (e.g., `getUserMemory()`), causing TS2339 errors in CI. Do not suggest re-adding the object-spread config merge — the correct approach would require a targeted method that selectively forwards GenerateContentConfig fields without destroying the Config class prototype. The abortSignal improvement is retained and should not be reverted.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/providers/RetryOrchestrator.ts:362-374
Timestamp: 2026-04-03T05:57:34.290Z
Learning: In vybestack/llxprt-code, `OnAuthErrorHandler.handleAuthError` declares `profileId` as optional because `profileId` is not available at the `RetryOrchestrator` level: `GenerateChatOptions` does not carry a `profileId` field and the runtime context does not expose it. The `forceRefreshToken` TOCTOU pattern in `TokenAccessCoordinator` works correctly without a profile qualifier. Do not flag the missing `profileId` in `RetryOrchestrator`'s `handleAuthError` call as a bug — propagating `profileId` through the entire provider call chain is a tracked follow-up, not a defect in the current fix (issue `#1861` / PR `#1874`).
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1459
File: packages/core/src/providers/LoadBalancingProvider.ts:631-648
Timestamp: 2026-02-18T01:44:54.715Z
Learning: Load balancer failover and bucket failover are two separate mechanisms: Load balancer failover (LoadBalancingProvider) distributes requests across different backends/providers and treats 401 as an immediate failover error. Bucket failover (BucketFailoverHandlerImpl) handles credential rotation within a single provider and may implement auto-renew semantics for 401. These systems have different 401 handling behaviors by design.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-13T16:04:07.079Z
Learning: Support multi-bucket failover on a single profile: ordered buckets tried in sequence; on 429 or 402 advance to next bucket; on 401 attempt auto-renew then retry once before advancing.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-02-20T04:32:08.940Z
Learning: In vybestack/llxprt-code, provider retries and OAuth bucket failover are centralized in packages/core/src/providers/RetryOrchestrator.ts. Failover triggers: 2×429, immediate 402, or 2nd 401/403. On success, counters and failover session reset; on switch, attempt-- so retries don’t burn on bucket change.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-13T23:04:39.246Z
Learning: Treat Anthropic 403 permission_error "OAuth token has been revoked" as an expired/auth error: trigger a one-time refresh retry, then bucket failover if refresh fails. Avoid auto-switching providers; only switch buckets within the same provider. This error likely means that we failed to lock when refreshing and multiple instances refreshed the same token.
📚 Learning: 2026-04-03T05:57:42.326Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/auth/invalidateProviderCache.test.ts:107-163
Timestamp: 2026-04-03T05:57:42.326Z
Learning: In `packages/core/src/auth/invalidateProviderCache.test.ts` (vybestack/llxprt-code PR `#1874`), the profile-specific positive-match path for `invalidateProviderCache('anthropic', profileId)` is intentionally not directly tested because `resolveAuthentication` does not expose a `profileId` metadata parameter in its public API, making it impossible to construct a profile-keyed cache entry from outside. The wildcard invalidation test (`invalidateProviderCache('anthropic')` with no profileId) exercises the same matching predicate. Do not flag the missing profile-specific positive-match case as a test gap — adding it is a low-priority follow-up that requires internal API changes.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.ts
📚 Learning: 2026-04-03T05:57:51.304Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/providers/__tests__/RetryOrchestrator.onAuthError.test.ts:79-404
Timestamp: 2026-04-03T05:57:51.304Z
Learning: In vybestack/llxprt-code, `RetryOrchestrator.onAuthError` tests (packages/core/src/providers/__tests__/RetryOrchestrator.onAuthError.test.ts, PR `#1874`) are intentionally scoped to the default/single-bucket path covering issue1861's core fix. Multi-bucket / non-default session-bucket regression coverage belongs at the OAuthManager/TokenAccessCoordinator layer (forceRefreshToken.test.ts), not in RetryOrchestrator tests. Do not flag the absence of multi-bucket bucket-context tests in RetryOrchestrator.onAuthError.test.ts as a coverage gap.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.ts
📚 Learning: 2026-03-26T00:29:42.510Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/BucketFailoverHandlerImpl.spec.ts:500-503
Timestamp: 2026-03-26T00:29:42.510Z
Learning: In `packages/cli/src/auth/BucketFailoverHandlerImpl.spec.ts` (vybestack/llxprt-code), the `if (provider)` guard pattern used after `oauthManager.getProvider(...)` to conditionally stub `provider.refreshToken` is pre-existing from the original test suite. Do not flag this as a silent-skip risk or suggest hardening (e.g., `expect(provider).toBeDefined()`) in decomposition or refactoring PRs — changing test structure is explicitly out of scope for those PRs.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.ts
📚 Learning: 2026-03-29T16:31:31.631Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-03-29T16:31:31.631Z
Learning: In vybestack/llxprt-code issue `#1783` (OAuth bucket failover behavioral test spec), the formal scenario catalog was expanded from 39 to ~58 scenarios with four new categories: UE-01–UE-08 (User Entry Points/Lifecycle Triggers), SA-01–SA-04 (Subagent Isolation, tied to PR `#1720/`#1718/#1719), EC-01–EC-04 (Error & Edge Cases), and RO-01–RO-03 (Multi-bucket RetryOrchestrator Integration). Critical zero-coverage gaps are: SB-10 (auth flow mid-turn timeout), UE lifecycle triggers (useGeminiStream.ts turn boundary), SA subagent isolation regressions, and RO multi-bucket retry paths. Two mock-theater test files should be rewritten with MemoryTokenStore: `oauth-manager.failover-wiring.spec.ts` and `oauth-manager.getToken-bucket-peek.spec.ts`. Cross-process simulation uses shared SecureStore/lockDir (two KeyringTokenStore instances), not child_process.fork.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-27T00:46:43.700Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/folderTrustOriginalSettingsParity.test.ts:347-383
Timestamp: 2026-03-27T00:46:43.700Z
Learning: In `packages/cli/src/config/__tests__/folderTrustOriginalSettingsParity.test.ts` (vybestack/llxprt-code PR `#1785`), the last test ("profile ephemeral folderTrust value does NOT change the trust check") intentionally omits a real profile load. Its sole purpose is to assert that `isWorkspaceTrusted` is called with the original settings object (not a profile-merged copy) in the untrusted-folder branch. The profile-merge path is covered by other parity test files. Do not suggest adding an inline profile or `LLXPRT_PROFILE` env var to this test — that would be scope expansion beyond its intended parity coverage.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.ts
📚 Learning: 2026-03-26T02:12:39.396Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/auth-flow-orchestrator.ts:431-437
Timestamp: 2026-03-26T02:12:39.396Z
Learning: In `packages/cli/src/auth/auth-flow-orchestrator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the early return in `authenticateMultipleBuckets` when `unauthenticatedBuckets.length === 0` intentionally skips re-enabling OAuth in-memory/settings and skips installing a bucket failover handler. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` around line 2547 on main. Do not flag the missing success-side-effects (provider enablement, failover handler installation) on the all-valid fast path as a bug in decomposition or future PRs — adding these side-effects would be a behavioral change beyond the refactoring scope.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-29T20:44:28.357Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1841
File: packages/cli/src/auth/__tests__/behavioral/user-entry-points.behavioral.spec.ts:131-159
Timestamp: 2026-03-29T20:44:28.357Z
Learning: In `packages/cli/src/auth/__tests__/behavioral/user-entry-points.behavioral.spec.ts` (vybestack/llxprt-code PR `#1841`), UE-03 intentionally calls `handler.resetSession()` directly rather than triggering it through the actual turn-boundary entry point in `useGeminiStream.ts` (L73). This is an explicit scope boundary: `useGeminiStream.ts` is a React hook that requires a full UI render context; testing through it would require mounting React components and simulating user input, which is a different category of integration test. The behavioral spec verifies the handler-level contract (resetSession clears triedBuckets and enables fresh failover). Do not flag the direct `resetSession()` call in this test as missing turn-boundary wiring coverage.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-04-03T06:29:38.156Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/cli/src/auth/__tests__/forceRefreshToken.test.ts:449-477
Timestamp: 2026-04-03T06:29:38.156Z
Learning: In `packages/cli/src/auth/token-access-coordinator.ts` (vybestack/llxprt-code PR `#1874`), `forceRefreshToken` uses a lock-first, single-read TOCTOU pattern: acquire refresh lock → read stored token once (under the lock) → compare stored token with failed access token → act. There is no pre-lock read. The test in `packages/cli/src/auth/__tests__/forceRefreshToken.test.ts` correctly simulates the "another process already refreshed" case by preloading an updated token in the store. Do not flag the absence of a second `getToken` call or suggest asserting `getToken` was called twice — the implementation intentionally reads only once under the lock.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.ts
📚 Learning: 2026-03-20T01:26:21.401Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/OpenAIClientFactory.test.ts:241-246
Timestamp: 2026-03-20T01:26:21.401Z
Learning: In `packages/core/src/providers/openai/OpenAIClientFactory.test.ts` (vybestack/llxprt-code PR `#1736`), the `instantiateClient` tests intentionally inspect the OpenAI SDK's internal `_options` field (e.g., `(client as unknown as Record<string, unknown>)._options`) to assert `defaultHeaders` and HTTP agent propagation. This is a deliberate pragmatic tradeoff over mocking the OpenAI constructor (which would require module-level `vi.mock`, constructor spy setup, and restore lifecycle). The `_options` field has been stable across many SDK versions, and the approach is considered acceptable. Do not flag `_options` inspection in this test file as relying on unstable internals.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.ts
📚 Learning: 2026-03-27T01:00:29.058Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/approvalModeParity.test.ts:351-393
Timestamp: 2026-03-27T01:00:29.058Z
Learning: In `packages/cli/src/config/__tests__/approvalModeParity.test.ts` (vybestack/llxprt-code PR `#1785`), the test suite is intentionally scoped to verifying that the extracted approval-mode resolution logic in `approvalModeResolver.ts` produces identical results to the original inline logic. Adding new combination scenarios (e.g., admin-disabled YOLO combined with an untrusted folder) is considered scope expansion beyond parity coverage. The ordering of admin checks before trust-fallback checks is preserved from the original code. Do not flag missing cross-branch combination tests in this file as a gap in refactoring PRs.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-27T02:12:12.434Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/profileOverridePrecedenceParity.test.ts:212-306
Timestamp: 2026-03-27T02:12:12.434Z
Learning: In `packages/cli/src/config/__tests__/profileOverridePrecedenceParity.test.ts` (vybestack/llxprt-code PR `#1785`), `applyProfileSnapshot` is intentionally mocked in both `../../runtime/profileSnapshot.js` (the primary call-tracking mock, capturing calls into `profileSnapshotCalls`) and `../../runtime/runtimeSettings.js` (a fallback mock). This dual mocking is required because `config.ts` imports `applyProfileSnapshot` from both paths. Do not flag this as confusing duplication or suggest collapsing into a single mock in future reviews.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.ts
📚 Learning: 2026-03-26T00:29:57.154Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/OAuthBucketManager.ts:36-47
Timestamp: 2026-03-26T00:29:57.154Z
Learning: In `packages/cli/src/auth/OAuthBucketManager.ts` (vybestack/llxprt-code), `getSessionBucketScopeKey(provider, metadata?)` must remain public — it is called externally by `packages/cli/src/auth/token-bucket-failover-helper.ts` (lines 66 and 73) to compute session/bucket scope keys. Do not suggest making it private in code reviews.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
📚 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/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-26T00:49:43.150Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/__tests__/auth-flow-orchestrator.spec.ts:309-324
Timestamp: 2026-03-26T00:49:43.150Z
Learning: In this repository’s Jest (or Jest-like) test files, it is acceptable to use `expect(promiseReturningFunction).resolves.not.toThrow()` when the function returns `Promise<void>`. Do not flag this as an incorrect or suboptimal matcher; for `Promise<void>` it is functionally equivalent to using `resolves.toBeUndefined()` to assert successful resolution.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-26T01:27:59.283Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/auth-flow-orchestrator.ts:692-705
Timestamp: 2026-03-26T01:27:59.283Z
Learning: When reviewing code under packages/cli/src/auth (including decomposed modules related to auth-flow orchestration), do not treat missing explicit bucket strings as a production behavioral change if the production token store normalizes `bucket` with `bucket ?? DEFAULT_BUCKET` (where `DEFAULT_BUCKET` is the string "default"). In particular, passing `undefined` for `bucket` should be considered equivalent to passing "default", so calls like `getToken(provider, undefined)` are functionally identical to `getToken(provider, "default")` in production. This normalization is part of the existing production contract (e.g., KeyringTokenStore / prior oauth-manager behavior); InMemoryTokenStore/test helpers do not govern that production behavior.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
📚 Learning: 2026-03-26T01:28:33.474Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/provider-usage-info.ts:36-37
Timestamp: 2026-03-26T01:28:33.474Z
Learning: When doing token lookups via `tokenStore.getToken(provider, bucket)` in `packages/cli/src/auth`, treat `bucket ?? 'default'` as correct when the goal is to normalize missing buckets to the default bucket. The production `KeyringTokenStore` already normalizes `undefined` to `DEFAULT_BUCKET` (`"default"`), and this pattern was also used in the original `oauth-manager.ts`, so using `bucket ?? 'default'` here is faithful and should not be flagged as a contract/lookup violation nor used to recommend splitting `undefined` (lookup) vs `'default'` (display) behavior across this codebase.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
📚 Learning: 2026-03-27T01:24:59.499Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1784
File: packages/cli/src/auth/codex-oauth-provider.ts:394-402
Timestamp: 2026-03-27T01:24:59.499Z
Learning: In `packages/cli/src/auth/**` (including `codex-oauth-provider.ts` and related OAuth bridge files), `-1` is an intentional sentinel value used by stub `addItem` callbacks where no real history ID is available (for example `p.setAddItem?.(() => -1)`). Reviewers should not flag this as a magic number, nor suggest replacing it with a named constant, since callers never consume the returned ID and the literal `-1` is an established convention in this codebase.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.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/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-26T20:52:08.720Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:140-149
Timestamp: 2026-03-26T20:52:08.720Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts` (vybestack/llxprt-code PR `#1780`), the `applyThoughtToState` function's `setPendingHistoryItem` updater casts `item?.type as 'gemini' | 'gemini_content'` and uses `item?.text || ''` — this is intentional pre-existing behavior faithfully extracted verbatim from the original `useGeminiStream.ts` (lines 1110–1111 on main). The risk that a `tool_group` pending item type could be preserved is a known pre-existing pattern; fixing it requires careful testing of all thinking block rendering paths and is a behavioral change beyond decomposition scope. Do not flag this type cast as a bug in decomposition or refactoring PRs.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-27T00:46:42.630Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/parseArgumentsParity.test.ts:7-16
Timestamp: 2026-03-27T00:46:42.630Z
Learning: In `packages/cli/src/config/__tests__/parseArgumentsParity.test.ts` (vybestack/llxprt-code PR `#1785`), the test suite is intentionally scoped to verifying that the extracted `parseArguments` function produces identical output to the original inline parsing. It deliberately does NOT cover subcommand exit behavior (mcp, hooks, extensions) via `handleSubcommandExit()` — that function is a direct mechanical extraction and adding integration tests for it would be new coverage beyond the refactoring scope. Do not flag the absence of subcommand-exit test cases as a gap in this file or in refactoring PRs that extract it.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-26T21:40:39.044Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:521-525
Timestamp: 2026-03-26T21:40:39.044Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts` (vybestack/llxprt-code PR `#1780`), `processGeminiStreamEvents` previously always returned `StreamProcessingStatus.Completed` even when a `UserCancelled` or `Error` event was received — this was pre-existing dead code from the original `useGeminiStream.ts` (line 1371 checked for UserCancelled but the function always returned Completed). Fixed in commit 4a28363bc by adding a `processingResult` variable that tracks the status through the event loop, setting it to `StreamProcessingStatus.UserCancelled` or `StreamProcessingStatus.Error` as appropriate, and returning it at the end instead of always returning `Completed`.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-02-15T19:33:03.569Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1405
File: packages/core/src/lsp/__tests__/lsp-service-client-integration.test.ts:93-102
Timestamp: 2026-02-15T19:33:03.569Z
Learning: In the llxprt-code codebase (`packages/core/src/lsp/__tests__/lsp-service-client-integration.test.ts`), the LspServiceClient integration tests use a synthetic/stub implementation with PassThrough streams for testing. The test config may include fields like `filetypes` that differ from production types (`LspServerConfig`), as these tests verify the integration contract rather than exact production type conformance. Future work may add more realistic subprocess tests.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-27T00:46:42.685Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/parseArgumentsParity.test.ts:7-16
Timestamp: 2026-03-27T00:46:42.685Z
Learning: In `packages/cli/src/config/__tests__/parseArgumentsParity.test.ts` (vybestack/llxprt-code PR `#1785`), the test suite intentionally does NOT cover subcommand exit behavior (`mcp`, `hooks`, `extensions` via `handleSubcommandExit()`). The suite's scope is limited to verifying that the extracted `parseArguments` function produces identical results to the original inline parsing. `handleSubcommandExit()` is a direct mechanical extraction and adding integration tests for it would be new coverage beyond the refactor's scope. Do not flag missing subcommand-exit test cases in this file as a gap in future reviews.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-26T02:06:03.086Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentChatSetup.ts:339-453
Timestamp: 2026-03-26T02:06:03.086Z
Learning: In `packages/core/src/core/subagentChatSetup.ts` (vybestack/llxprt-code PR `#1779`, decomposed from `subagent.ts`), the `assembleChatSession` function's `try/catch` block wraps only `new GeminiChat(...)` and not the preceding `await ctx.environmentContextLoader(...)` or `await getCoreSystemPromptAsync(...)` calls. This partial error boundary is pre-existing behavior faithfully preserved from the original `createChatObject` method in `subagent.ts`. Do not flag the missing async-await coverage as a decomposition regression in future reviews — it is an acknowledged pre-existing limitation, and any fix would be a behavioral change requiring a dedicated follow-up PR.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-25T19:21:43.266Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1771
File: packages/core/src/core/AgentHookManager.ts:78-96
Timestamp: 2026-03-25T19:21:43.266Z
Learning: In `packages/core/src/core/AgentHookManager.ts` (vybestack/llxprt-code PR `#1771`), `cleanupOldHookState(newPromptId, oldPromptId)` is always called at the start of a new prompt turn (when `prompt_id` changes), after the previous prompt's stream has been fully consumed by `MessageStreamOrchestrator`. Because JS is single-threaded, there is no concurrent execution path where an in-flight `fireAfterAgentHookSafe` for the old `prompt_id` could race with the cleanup. Do not flag this pattern as a race condition in future reviews — the sequential flow guarantees: previous prompt finishes → new prompt starts → cleanup runs → fresh hook state created.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-04-03T20:43:35.912Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1876
File: packages/core/src/core/subagent.ts:441-464
Timestamp: 2026-04-03T20:43:35.912Z
Learning: In `packages/core/src/core/subagent.ts` (vybestack/llxprt-code), `runInteractiveTurn`'s `catch` block should only call `throw createAbortError()` when `abortController.signal.aborted` — it must NOT infer `SubagentTerminateMode.TIMEOUT` based on `!this.parentAbortSignal?.aborted`. `Turn.run()` absorbs idle-timeout events internally and emits `GeminiEventType.UserCancelled` before the generator exits, so the catch only fires for genuine errors or explicit cancel aborts. TIMEOUT is authoritatively set in the `onTimeout` callback of `nextStreamEventWithIdleTimeout` inside `runNonInteractiveTurn`. Do not conflate these two paths in future reviews.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-03T17:18:48.615Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1656
File: packages/cli/src/auth/oauth-manager.auth-lock.spec.ts:47-70
Timestamp: 2026-03-03T17:18:48.615Z
Learning: In vybestack/llxprt-code TOCTOU defense tests (e.g., packages/cli/src/auth/oauth-manager.auth-lock.spec.ts), call-count assertions like `expect(getTokenCallCount).toBe(4)` are intentional structural guards that validate the double-check pattern (upfront checks + re-checks under lock). Behavioral assertions (e.g., authenticate() call counts per bucket) are primary, but call counts ensure the re-check path executes and catch regressions that might remove the defensive re-read while still passing behavioral tests.
```
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-25T18:17:38.768Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1768
File: packages/cli/src/ui/containers/AppContainer/hooks/useInputHandling.ts:127-135
Timestamp: 2026-03-25T18:17:38.768Z
Learning: In `packages/cli/src/ui/containers/AppContainer/hooks/useInputHandling.ts` (vybestack/llxprt-code PR `#1768`), the `hadToolCallsRef.current = false` reset inside `handleFinalSubmit` (before calling `submitQuery`) is intentional behavior preserved from the original `AppContainer.tsx` continuation flow contract. Do not flag this as a premature reset or race condition in decomposition or future PRs — any turn-boundary redesign (e.g., only clearing after `handleStreamCompleted`) is out of scope for the `#1576` refactoring and would constitute a behavioral change.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-04-03T20:43:54.493Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1876
File: packages/core/src/core/subagent.ts:675-710
Timestamp: 2026-04-03T20:43:54.493Z
Learning: In `packages/core/src/core/subagent.ts` (vybestack/llxprt-code PR `#1876`), `runNonInteractiveTurn` intentionally returns `{ functionCalls: [], textResponse: '' }` (instead of throwing) when `abortController.signal.aborted` is detected — both in the pre-loop early-abort check and inside the stream iteration loop. This is the correct design because: (1) `armTimeout` sets `output.terminate_reason = SubagentTerminateMode.TIMEOUT` *before* aborting, so abort-reason state is already recorded; (2) the caller in `runNonInteractive` checks `if (abortController.signal.aborted) return;` immediately after the call, resolving `runNonInteractive` as a normal `Promise<void>` (not a rejection); (3) existing TIMEOUT tests assert this resolution behavior and would break if `createAbortError()` were thrown instead. Do not flag the return-empty-on-abort pattern as dropping `finalizeOutput` or treating abort as success — it is intentional and test-validated.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-26T20:01:50.052Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/checkpointPersistence.ts:0-0
Timestamp: 2026-03-26T20:01:50.052Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/checkpointPersistence.ts` (vybestack/llxprt-code PR `#1780`), `saveRestorableToolCalls` reserves the `callId` in `checkpointedCallIds` *before* calling `await createToolCheckpoint(...)` to prevent concurrent duplicate attempts. On failure (exception or false/no-write return), the `callId` is deleted from the set so the next effect run can retry. This reserve-before-await, delete-on-failure pattern was introduced in commit a41b97759.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-21T17:15:03.717Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:275-318
Timestamp: 2026-03-21T17:15:03.717Z
Learning: In `packages/core/src/core/TurnProcessor.ts` (vybestack/llxprt-code PR `#1743`), the non-streaming `sendMessage()` path in `_executeSendWithRetry` was improved to pass `params.config?.abortSignal` to `retryWithBackoff` (added in commit 870e85a91, retained). A matching config merge (`params.config` into the provider runtime context) was attempted but reverted in commit 88d894840 because spreading `GenerateContentConfig` into a `Config` class instance loses prototype methods (e.g., `getUserMemory()`), causing TS2339 errors in CI. Do not suggest re-adding the object-spread config merge — the correct approach would require a targeted method that selectively forwards GenerateContentConfig fields without destroying the Config class prototype. The abortSignal improvement is retained and should not be reverted.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-21T17:07:15.661Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/ConversationManager.ts:94-101
Timestamp: 2026-03-21T17:07:15.661Z
Learning: In `packages/core/src/core/ConversationManager.ts`, `packages/core/src/core/TurnProcessor.ts`, `packages/core/src/core/StreamProcessor.ts`, and `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`, commit 870e85a91), `makePositionMatcher()` was hoisted outside all per-Content loops so that a single position matcher is shared across the entire batch. This supersedes the prior preserved-behavior pattern (original `geminiChat.ts` called it per-Content). The unmatched-tool queue is now consumed in order across all items in a batch, which is the correct behavior.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-25T12:57:21.017Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1768
File: packages/cli/src/ui/hooks/useCoreEventHandlers.ts:71-78
Timestamp: 2026-03-25T12:57:21.017Z
Learning: In files under `packages/cli/src/ui/hooks/`, it is OK for a `useEffect` to register process-level cleanup via `registerCleanup(consolePatcher.cleanup)` without returning a React cleanup function (e.g., without `return () => consolePatcher.cleanup()`). Treat this as an intentional pattern: `registerCleanup()` is the project’s mechanism for cleanup that is meant to persist beyond React unmount/re-render, so do not flag the missing `return` as a leak/bug.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-26T22:12:58.549Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:1-5
Timestamp: 2026-03-26T22:12:58.549Z
Learning: When reviewing this repo, do not flag these files for missing "upstream" Google LLC copyright. For `useGeminiStream.ts` and the extracted geminiStream-hook modules (under `packages/cli/src/ui/hooks/geminiStream/`), ensure the copyright header is `Copyright 2025 Vybestack LLC`—these files were already updated to Vybestack LLC before PR `#1780` and the decomposition outputs correctly use Vybestack LLC.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-21T17:07:33.700Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/StreamProcessor.ts:123-135
Timestamp: 2026-03-21T17:07:33.700Z
Learning: In `packages/core/src/core/StreamProcessor.ts` (vybestack/llxprt-code PR `#1743`), the retry boundary in `_executeStreamApiCall` only protects async generator *construction* (via `_buildAndSendStreamRequest`), not the actual network calls that occur during iteration inside `_aggregateStreamChunks`. This is a known pre-existing architectural limitation faithfully preserved from the original `geminiChat.ts`. The issue is tracked as `#1750` for the 0.10.0 improvement effort. Do not re-flag this as a regression or decomposition bug in future reviews — it is intentionally deferred.
Applied to files:
packages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.ts
📚 Learning: 2026-03-21T15:50:32.992Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1740
File: packages/core/src/providers/anthropic/AnthropicStreamProcessor.ts:75-192
Timestamp: 2026-03-21T15:50:32.992Z
Learning: In `packages/core/src/providers/anthropic/AnthropicStreamProcessor.ts` (vybestack/llxprt-code PR `#1740`), stream retry after transient errors is guarded by a `hasYieldedContent` flag. If a transient network error occurs mid-stream after text, tool calls, or thinking blocks have been yielded, the generator throws immediately instead of retrying (to avoid duplicate/corrupted output), delegating recovery to the upper-layer RetryOrchestrator. Metadata-only yields (usage from `message_start`) do NOT set this flag, so retries before any real content are still allowed. Fixed in commit e5aa5043.
Applied to files:
packages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.ts
📚 Learning: 2026-02-20T04:32:08.940Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-02-20T04:32:08.940Z
Learning: In vybestack/llxprt-code, provider retries and OAuth bucket failover are centralized in packages/core/src/providers/RetryOrchestrator.ts. Failover triggers: 2×429, immediate 402, or 2nd 401/403. On success, counters and failover session reset; on switch, attempt-- so retries don’t burn on bucket change.
Applied to files:
packages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStream.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/StreamProcessor.unbucketed-auth-failover.test.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/StreamProcessor.unbucketed-auth-failover.test.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/StreamProcessor.unbucketed-auth-failover.test.ts
📚 Learning: 2026-03-21T15:17:37.899Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/TurnProcessor.ts:410-421
Timestamp: 2026-03-21T15:17:37.899Z
Learning: In `packages/core/src/core/TurnProcessor.ts` and `packages/core/src/core/compression/CompressionHandler.ts` (vybestack/llxprt-code PR `#1743`), both classes maintain a `lastPromptTokenCount` field. After the decomposition of `geminiChat.ts`, `TurnProcessor._executeProviderCall` must update both `this.lastPromptTokenCount` AND `this.compressionHandler.lastPromptTokenCount` (via `setLastPromptTokenCount`) whenever prompt token counts are received from the non-streaming provider response. The streaming path in `StreamProcessor` already updates `compressionHandler.lastPromptTokenCount` directly. Do not flag the dual-update pattern as redundant — `CompressionHandler.shouldCompress()` reads its own field and will use stale data if only `TurnProcessor.lastPromptTokenCount` is updated. Fixed in commit bac2e4153.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-26T02:22:24.428Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/streamUtils.ts:264-267
Timestamp: 2026-03-26T02:22:24.428Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/streamUtils.ts` (vybestack/llxprt-code PR `#1780`, decomposed from the original `useGeminiStream.ts`), `buildFullSplitItem` uses `const profileName = liveProfileName ?? existingProfileName` — this is intentional pre-existing behavior faithfully extracted from the original `useGeminiStream.ts`. The precedence (liveProfileName first, existingProfileName as fallback) is identical to the source implementation. Changing to `existingProfileName ?? liveProfileName` would be a behavioral modification, not a decomposition fix. Do not flag this precedence order in decomposition or future PRs — any fix should be a dedicated follow-up.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-26T02:22:19.732Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/streamUtils.ts:100-116
Timestamp: 2026-03-26T02:22:19.732Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/streamUtils.ts` (vybestack/llxprt-code PR `#1780`), the `filteredPendingTools` predicate in `mergePendingToolGroupsForDisplay` only removes overlapping shell entries from the scheduler side, leaving overlapping non-shell tool callIds potentially duplicated. This is pre-existing behavior faithfully extracted verbatim from the original `useGeminiStream.ts` (lines 100-140). Do not flag this as a dedup bug in decomposition or refactoring PRs — any improvement to deduplicate non-shell overlapping callIds would be a separate enhancement, not a structural fix.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
📚 Learning: 2026-03-22T03:20:15.367Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1740
File: packages/core/src/providers/anthropic/AnthropicRateLimitHandler.ts:175-181
Timestamp: 2026-03-22T03:20:15.367Z
Learning: In `packages/core/src/providers/anthropic/AnthropicRateLimitHandler.ts` (vybestack/llxprt-code PR `#1740`), `calculateWaitTime` was refactored in commit 6f62ce987 so that all three buckets (requests, tokens, input tokens) are evaluated before returning. A `shouldWait: true` decision from any bucket takes precedence over warning-only results (where `shouldWait: false` but no reset time is available). The old early-return-on-warning-only pattern was removed. Do not expect or re-introduce the early-exit behavior.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts
🪛 GitHub Actions: LLxprt Code CI
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
[error] 26-26: TypeScript build failed (TS2741): Property 'forceRefreshToken' is missing in type '{ getSessionBucket: Mock; setSessionBucket: Mock; getTokenStore: Mock; getOAuthToken: Mock; authenticate: Mock<...>; authenticateMultipleBuckets: Mock<...>; }' but required in type 'BucketFailoverOAuthManagerLike'.
🔇 Additional comments (3)
packages/core/src/core/StreamProcessor.unbucketed-auth-failover.test.ts (1)
27-63: Good targeted regression coverage for the unbucketed failover pathThis test correctly guards the intended behavior change: single-bucket handlers still execute
tryFailover()and auth scope invalidation even whenisEnabled()is false.packages/cli/src/ui/hooks/geminiStream/useGeminiStream.ts (1)
58-83: Turn-boundary invalidation ordering looks correctThe new-turn path performs cache invalidation before
ensureBucketsAuthenticated, and the continuation path remains isolated toresetSessionas intended.packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts (1)
12-57: Strong order assertion for the non-continuation pathThis test gives good protection for the required sequencing (
reset→invalidateAuthCache(runtimeId)→ensureBucketsAuthenticated) and state resets.
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. |
Aligns the new auth regression tests with the real CLI/core types so build, lint, and E2E workflows can compile the branch for issue #1739.
Collaborator
Author
|
Addressed the current PR blockers from the failing lint and E2E jobs. Root cause:
Remediation:
Current local result:
|
Applies the formatter output required by the PR lint workflow to the new auth regression tests for issue #1739 without touching project memory files.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts`:
- Around line 1-53: The test file has Prettier/formatting issues; run the
project's formatter (e.g., npm run format or prettier --write) on the test file
containing the describe('BucketFailoverHandlerImpl.invalidateAuthCache'...) and
related imports (flushRuntimeAuthScope, BucketFailoverHandlerImpl,
OAuthTokenRequestMetadata) to apply Prettier rules, then commit the reformatted
file so CI's format check passes.
In
`@packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts`:
- Around line 1-141: The test file has formatting drift detected by Prettier;
run the project's formatter (e.g., npm run format or npx prettier --write) on
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
(the file containing the prepareTurnForQuery tests), review the changed
formatting, stage and commit the updated file so CI no longer reports
uncommitted Prettier changes.
🪄 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
Run ID: b3ebf2a7-92cd-4c62-aab7-66f8b00e54aa
📒 Files selected for processing (2)
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (macOS)
🧰 Additional context used
🧠 Learnings (35)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/auth/invalidateProviderCache.test.ts:107-163
Timestamp: 2026-04-03T05:57:42.326Z
Learning: In `packages/core/src/auth/invalidateProviderCache.test.ts` (vybestack/llxprt-code PR `#1874`), the profile-specific positive-match path for `invalidateProviderCache('anthropic', profileId)` is intentionally not directly tested because `resolveAuthentication` does not expose a `profileId` metadata parameter in its public API, making it impossible to construct a profile-keyed cache entry from outside. The wildcard invalidation test (`invalidateProviderCache('anthropic')` with no profileId) exercises the same matching predicate. Do not flag the missing profile-specific positive-match case as a test gap — adding it is a low-priority follow-up that requires internal API changes.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/auth-flow-orchestrator.ts:431-437
Timestamp: 2026-03-26T02:12:39.396Z
Learning: In `packages/cli/src/auth/auth-flow-orchestrator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the early return in `authenticateMultipleBuckets` when `unauthenticatedBuckets.length === 0` intentionally skips re-enabling OAuth in-memory/settings and skips installing a bucket failover handler. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` around line 2547 on main. Do not flag the missing success-side-effects (provider enablement, failover handler installation) on the all-valid fast path as a bug in decomposition or future PRs — adding these side-effects would be a behavioral change beyond the refactoring scope.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/providers/__tests__/RetryOrchestrator.onAuthError.test.ts:79-404
Timestamp: 2026-04-03T05:57:51.304Z
Learning: In vybestack/llxprt-code, `RetryOrchestrator.onAuthError` tests (packages/core/src/providers/__tests__/RetryOrchestrator.onAuthError.test.ts, PR `#1874`) are intentionally scoped to the default/single-bucket path covering issue1861's core fix. Multi-bucket / non-default session-bucket regression coverage belongs at the OAuthManager/TokenAccessCoordinator layer (forceRefreshToken.test.ts), not in RetryOrchestrator tests. Do not flag the absence of multi-bucket bucket-context tests in RetryOrchestrator.onAuthError.test.ts as a coverage gap.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-03-29T16:31:31.631Z
Learning: In vybestack/llxprt-code issue `#1783` (OAuth bucket failover behavioral test spec), the formal scenario catalog was expanded from 39 to ~58 scenarios with four new categories: UE-01–UE-08 (User Entry Points/Lifecycle Triggers), SA-01–SA-04 (Subagent Isolation, tied to PR `#1720/`#1718/#1719), EC-01–EC-04 (Error & Edge Cases), and RO-01–RO-03 (Multi-bucket RetryOrchestrator Integration). Critical zero-coverage gaps are: SB-10 (auth flow mid-turn timeout), UE lifecycle triggers (useGeminiStream.ts turn boundary), SA subagent isolation regressions, and RO multi-bucket retry paths. Two mock-theater test files should be rewritten with MemoryTokenStore: `oauth-manager.failover-wiring.spec.ts` and `oauth-manager.getToken-bucket-peek.spec.ts`. Cross-process simulation uses shared SecureStore/lockDir (two KeyringTokenStore instances), not child_process.fork.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/token-access-coordinator.ts:442-449
Timestamp: 2026-03-26T00:30:25.258Z
Learning: In `packages/cli/src/auth/token-access-coordinator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), `getToken()` calls `peekOtherProfileBuckets()` even when `explicitBucket` is `true`, and the associated profile-bucket count check can suppress auth when multiple profile buckets exist. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` getOAuthToken method (lines 1136-1412 on main). The bucket resolution logic intentionally supports profile-scoped session buckets with failover. Do not flag this as a bug in decomposition or future PRs — rearchitecting the bucket resolution chain would be a behavioral change beyond refactoring scope.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1841
File: packages/cli/src/auth/__tests__/behavioral/user-entry-points.behavioral.spec.ts:131-159
Timestamp: 2026-03-29T20:44:28.357Z
Learning: In `packages/cli/src/auth/__tests__/behavioral/user-entry-points.behavioral.spec.ts` (vybestack/llxprt-code PR `#1841`), UE-03 intentionally calls `handler.resetSession()` directly rather than triggering it through the actual turn-boundary entry point in `useGeminiStream.ts` (L73). This is an explicit scope boundary: `useGeminiStream.ts` is a React hook that requires a full UI render context; testing through it would require mounting React components and simulating user input, which is a different category of integration test. The behavioral spec verifies the handler-level contract (resetSession clears triedBuckets and enables fresh failover). Do not flag the direct `resetSession()` call in this test as missing turn-boundary wiring coverage.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/token-access-coordinator.ts:640-647
Timestamp: 2026-03-26T03:34:18.861Z
Learning: In `packages/cli/src/auth/token-access-coordinator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the locked disk-check path in `performDiskCheck()` calls `performDiskCheckUnderLock()` without a surrounding try-catch and does not call `scheduleProactiveRenewal()` on the result. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` line ~1308 on main. The proactive renewal call on the unlocked fallback (line ~653) is a targeted addition specific to that bypass path. The locked path feeds into the standard refresh cycle which handles renewal scheduling. Do not flag the missing error guard or missing renewal scheduling on the locked disk-check path as a decomposition regression in future reviews — adding them would be scope expansion beyond the refactoring goal.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/cli/src/auth/__tests__/forceRefreshToken.test.ts:449-477
Timestamp: 2026-04-03T06:29:38.156Z
Learning: In `packages/cli/src/auth/token-access-coordinator.ts` (vybestack/llxprt-code PR `#1874`), `forceRefreshToken` uses a lock-first, single-read TOCTOU pattern: acquire refresh lock → read stored token once (under the lock) → compare stored token with failed access token → act. There is no pre-lock read. The test in `packages/cli/src/auth/__tests__/forceRefreshToken.test.ts` correctly simulates the "another process already refreshed" case by preloading an updated token in the store. Do not flag the absence of a second `getToken` call or suggest asserting `getToken` was called twice — the implementation intentionally reads only once under the lock.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/BucketFailoverHandlerImpl.spec.ts:500-503
Timestamp: 2026-03-26T00:29:42.510Z
Learning: In `packages/cli/src/auth/BucketFailoverHandlerImpl.spec.ts` (vybestack/llxprt-code), the `if (provider)` guard pattern used after `oauthManager.getProvider(...)` to conditionally stub `provider.refreshToken` is pre-existing from the original test suite. Do not flag this as a silent-skip risk or suggest hardening (e.g., `expect(provider).toBeDefined()`) in decomposition or refactoring PRs — changing test structure is explicitly out of scope for those PRs.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/providers/RetryOrchestrator.ts:362-374
Timestamp: 2026-04-03T05:57:34.290Z
Learning: In vybestack/llxprt-code, `OnAuthErrorHandler.handleAuthError` declares `profileId` as optional because `profileId` is not available at the `RetryOrchestrator` level: `GenerateChatOptions` does not carry a `profileId` field and the runtime context does not expose it. The `forceRefreshToken` TOCTOU pattern in `TokenAccessCoordinator` works correctly without a profile qualifier. Do not flag the missing `profileId` in `RetryOrchestrator`'s `handleAuthError` call as a bug — propagating `profileId` through the entire provider call chain is a tracked follow-up, not a defect in the current fix (issue `#1861` / PR `#1874`).
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1459
File: packages/core/src/providers/LoadBalancingProvider.ts:631-648
Timestamp: 2026-02-18T01:44:54.715Z
Learning: Load balancer failover and bucket failover are two separate mechanisms: Load balancer failover (LoadBalancingProvider) distributes requests across different backends/providers and treats 401 as an immediate failover error. Bucket failover (BucketFailoverHandlerImpl) handles credential rotation within a single provider and may implement auto-renew semantics for 401. These systems have different 401 handling behaviors by design.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-13T16:04:07.079Z
Learning: Support multi-bucket failover on a single profile: ordered buckets tried in sequence; on 429 or 402 advance to next bucket; on 401 attempt auto-renew then retry once before advancing.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-02-20T04:32:08.940Z
Learning: In vybestack/llxprt-code, provider retries and OAuth bucket failover are centralized in packages/core/src/providers/RetryOrchestrator.ts. Failover triggers: 2×429, immediate 402, or 2nd 401/403. On success, counters and failover session reset; on switch, attempt-- so retries don’t burn on bucket change.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-13T23:04:39.246Z
Learning: Treat Anthropic 403 permission_error "OAuth token has been revoked" as an expired/auth error: trigger a one-time refresh retry, then bucket failover if refresh fails. Avoid auto-switching providers; only switch buckets within the same provider. This error likely means that we failed to lock when refreshing and multiple instances refreshed the same token.
📚 Learning: 2026-04-03T05:57:42.326Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/auth/invalidateProviderCache.test.ts:107-163
Timestamp: 2026-04-03T05:57:42.326Z
Learning: In `packages/core/src/auth/invalidateProviderCache.test.ts` (vybestack/llxprt-code PR `#1874`), the profile-specific positive-match path for `invalidateProviderCache('anthropic', profileId)` is intentionally not directly tested because `resolveAuthentication` does not expose a `profileId` metadata parameter in its public API, making it impossible to construct a profile-keyed cache entry from outside. The wildcard invalidation test (`invalidateProviderCache('anthropic')` with no profileId) exercises the same matching predicate. Do not flag the missing profile-specific positive-match case as a test gap — adding it is a low-priority follow-up that requires internal API changes.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-26T00:29:42.510Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/BucketFailoverHandlerImpl.spec.ts:500-503
Timestamp: 2026-03-26T00:29:42.510Z
Learning: In `packages/cli/src/auth/BucketFailoverHandlerImpl.spec.ts` (vybestack/llxprt-code), the `if (provider)` guard pattern used after `oauthManager.getProvider(...)` to conditionally stub `provider.refreshToken` is pre-existing from the original test suite. Do not flag this as a silent-skip risk or suggest hardening (e.g., `expect(provider).toBeDefined()`) in decomposition or refactoring PRs — changing test structure is explicitly out of scope for those PRs.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-04-03T05:57:51.304Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/core/src/providers/__tests__/RetryOrchestrator.onAuthError.test.ts:79-404
Timestamp: 2026-04-03T05:57:51.304Z
Learning: In vybestack/llxprt-code, `RetryOrchestrator.onAuthError` tests (packages/core/src/providers/__tests__/RetryOrchestrator.onAuthError.test.ts, PR `#1874`) are intentionally scoped to the default/single-bucket path covering issue1861's core fix. Multi-bucket / non-default session-bucket regression coverage belongs at the OAuthManager/TokenAccessCoordinator layer (forceRefreshToken.test.ts), not in RetryOrchestrator tests. Do not flag the absence of multi-bucket bucket-context tests in RetryOrchestrator.onAuthError.test.ts as a coverage gap.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-29T16:31:31.631Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-03-29T16:31:31.631Z
Learning: In vybestack/llxprt-code issue `#1783` (OAuth bucket failover behavioral test spec), the formal scenario catalog was expanded from 39 to ~58 scenarios with four new categories: UE-01–UE-08 (User Entry Points/Lifecycle Triggers), SA-01–SA-04 (Subagent Isolation, tied to PR `#1720/`#1718/#1719), EC-01–EC-04 (Error & Edge Cases), and RO-01–RO-03 (Multi-bucket RetryOrchestrator Integration). Critical zero-coverage gaps are: SB-10 (auth flow mid-turn timeout), UE lifecycle triggers (useGeminiStream.ts turn boundary), SA subagent isolation regressions, and RO multi-bucket retry paths. Two mock-theater test files should be rewritten with MemoryTokenStore: `oauth-manager.failover-wiring.spec.ts` and `oauth-manager.getToken-bucket-peek.spec.ts`. Cross-process simulation uses shared SecureStore/lockDir (two KeyringTokenStore instances), not child_process.fork.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-27T00:46:43.700Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/folderTrustOriginalSettingsParity.test.ts:347-383
Timestamp: 2026-03-27T00:46:43.700Z
Learning: In `packages/cli/src/config/__tests__/folderTrustOriginalSettingsParity.test.ts` (vybestack/llxprt-code PR `#1785`), the last test ("profile ephemeral folderTrust value does NOT change the trust check") intentionally omits a real profile load. Its sole purpose is to assert that `isWorkspaceTrusted` is called with the original settings object (not a profile-merged copy) in the untrusted-folder branch. The profile-merge path is covered by other parity test files. Do not suggest adding an inline profile or `LLXPRT_PROFILE` env var to this test — that would be scope expansion beyond its intended parity coverage.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
📚 Learning: 2026-03-26T02:12:39.396Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/auth-flow-orchestrator.ts:431-437
Timestamp: 2026-03-26T02:12:39.396Z
Learning: In `packages/cli/src/auth/auth-flow-orchestrator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the early return in `authenticateMultipleBuckets` when `unauthenticatedBuckets.length === 0` intentionally skips re-enabling OAuth in-memory/settings and skips installing a bucket failover handler. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` around line 2547 on main. Do not flag the missing success-side-effects (provider enablement, failover handler installation) on the all-valid fast path as a bug in decomposition or future PRs — adding these side-effects would be a behavioral change beyond the refactoring scope.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
📚 Learning: 2026-04-03T06:29:38.156Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1874
File: packages/cli/src/auth/__tests__/forceRefreshToken.test.ts:449-477
Timestamp: 2026-04-03T06:29:38.156Z
Learning: In `packages/cli/src/auth/token-access-coordinator.ts` (vybestack/llxprt-code PR `#1874`), `forceRefreshToken` uses a lock-first, single-read TOCTOU pattern: acquire refresh lock → read stored token once (under the lock) → compare stored token with failed access token → act. There is no pre-lock read. The test in `packages/cli/src/auth/__tests__/forceRefreshToken.test.ts` correctly simulates the "another process already refreshed" case by preloading an updated token in the store. Do not flag the absence of a second `getToken` call or suggest asserting `getToken` was called twice — the implementation intentionally reads only once under the lock.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
📚 Learning: 2026-03-29T20:44:28.357Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1841
File: packages/cli/src/auth/__tests__/behavioral/user-entry-points.behavioral.spec.ts:131-159
Timestamp: 2026-03-29T20:44:28.357Z
Learning: In `packages/cli/src/auth/__tests__/behavioral/user-entry-points.behavioral.spec.ts` (vybestack/llxprt-code PR `#1841`), UE-03 intentionally calls `handler.resetSession()` directly rather than triggering it through the actual turn-boundary entry point in `useGeminiStream.ts` (L73). This is an explicit scope boundary: `useGeminiStream.ts` is a React hook that requires a full UI render context; testing through it would require mounting React components and simulating user input, which is a different category of integration test. The behavioral spec verifies the handler-level contract (resetSession clears triedBuckets and enables fresh failover). Do not flag the direct `resetSession()` call in this test as missing turn-boundary wiring coverage.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-20T01:26:21.401Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/OpenAIClientFactory.test.ts:241-246
Timestamp: 2026-03-20T01:26:21.401Z
Learning: In `packages/core/src/providers/openai/OpenAIClientFactory.test.ts` (vybestack/llxprt-code PR `#1736`), the `instantiateClient` tests intentionally inspect the OpenAI SDK's internal `_options` field (e.g., `(client as unknown as Record<string, unknown>)._options`) to assert `defaultHeaders` and HTTP agent propagation. This is a deliberate pragmatic tradeoff over mocking the OpenAI constructor (which would require module-level `vi.mock`, constructor spy setup, and restore lifecycle). The `_options` field has been stable across many SDK versions, and the approach is considered acceptable. Do not flag `_options` inspection in this test file as relying on unstable internals.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
📚 Learning: 2026-03-27T02:12:12.434Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/profileOverridePrecedenceParity.test.ts:212-306
Timestamp: 2026-03-27T02:12:12.434Z
Learning: In `packages/cli/src/config/__tests__/profileOverridePrecedenceParity.test.ts` (vybestack/llxprt-code PR `#1785`), `applyProfileSnapshot` is intentionally mocked in both `../../runtime/profileSnapshot.js` (the primary call-tracking mock, capturing calls into `profileSnapshotCalls`) and `../../runtime/runtimeSettings.js` (a fallback mock). This dual mocking is required because `config.ts` imports `applyProfileSnapshot` from both paths. Do not flag this as confusing duplication or suggest collapsing into a single mock in future reviews.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
📚 Learning: 2026-03-26T03:34:18.861Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/token-access-coordinator.ts:640-647
Timestamp: 2026-03-26T03:34:18.861Z
Learning: In `packages/cli/src/auth/token-access-coordinator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the locked disk-check path in `performDiskCheck()` calls `performDiskCheckUnderLock()` without a surrounding try-catch and does not call `scheduleProactiveRenewal()` on the result. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` line ~1308 on main. The proactive renewal call on the unlocked fallback (line ~653) is a targeted addition specific to that bypass path. The locked path feeds into the standard refresh cycle which handles renewal scheduling. Do not flag the missing error guard or missing renewal scheduling on the locked disk-check path as a decomposition regression in future reviews — adding them would be scope expansion beyond the refactoring goal.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
📚 Learning: 2026-03-26T00:30:25.258Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/token-access-coordinator.ts:442-449
Timestamp: 2026-03-26T00:30:25.258Z
Learning: In `packages/cli/src/auth/token-access-coordinator.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), `getToken()` calls `peekOtherProfileBuckets()` even when `explicitBucket` is `true`, and the associated profile-bucket count check can suppress auth when multiple profile buckets exist. This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` getOAuthToken method (lines 1136-1412 on main). The bucket resolution logic intentionally supports profile-scoped session buckets with failover. Do not flag this as a bug in decomposition or future PRs — rearchitecting the bucket resolution chain would be a behavioral change beyond refactoring scope.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
📚 Learning: 2026-03-26T00:29:57.154Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/OAuthBucketManager.ts:36-47
Timestamp: 2026-03-26T00:29:57.154Z
Learning: In `packages/cli/src/auth/OAuthBucketManager.ts` (vybestack/llxprt-code), `getSessionBucketScopeKey(provider, metadata?)` must remain public — it is called externally by `packages/cli/src/auth/token-bucket-failover-helper.ts` (lines 66 and 73) to compute session/bucket scope keys. Do not suggest making it private in code reviews.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
📚 Learning: 2026-03-26T01:28:33.474Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/provider-usage-info.ts:36-37
Timestamp: 2026-03-26T01:28:33.474Z
Learning: When doing token lookups via `tokenStore.getToken(provider, bucket)` in `packages/cli/src/auth`, treat `bucket ?? 'default'` as correct when the goal is to normalize missing buckets to the default bucket. The production `KeyringTokenStore` already normalizes `undefined` to `DEFAULT_BUCKET` (`"default"`), and this pattern was also used in the original `oauth-manager.ts`, so using `bucket ?? 'default'` here is faithful and should not be flagged as a contract/lookup violation nor used to recommend splitting `undefined` (lookup) vs `'default'` (display) behavior across this codebase.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
📚 Learning: 2026-03-26T01:27:59.283Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/auth-flow-orchestrator.ts:692-705
Timestamp: 2026-03-26T01:27:59.283Z
Learning: When reviewing code under packages/cli/src/auth (including decomposed modules related to auth-flow orchestration), do not treat missing explicit bucket strings as a production behavioral change if the production token store normalizes `bucket` with `bucket ?? DEFAULT_BUCKET` (where `DEFAULT_BUCKET` is the string "default"). In particular, passing `undefined` for `bucket` should be considered equivalent to passing "default", so calls like `getToken(provider, undefined)` are functionally identical to `getToken(provider, "default")` in production. This normalization is part of the existing production contract (e.g., KeyringTokenStore / prior oauth-manager behavior); InMemoryTokenStore/test helpers do not govern that production behavior.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
📚 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/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-26T00:49:43.150Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/__tests__/auth-flow-orchestrator.spec.ts:309-324
Timestamp: 2026-03-26T00:49:43.150Z
Learning: In this repository’s Jest (or Jest-like) test files, it is acceptable to use `expect(promiseReturningFunction).resolves.not.toThrow()` when the function returns `Promise<void>`. Do not flag this as an incorrect or suboptimal matcher; for `Promise<void>` it is functionally equivalent to using `resolves.toBeUndefined()` to assert successful resolution.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-27T01:24:59.499Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1784
File: packages/cli/src/auth/codex-oauth-provider.ts:394-402
Timestamp: 2026-03-27T01:24:59.499Z
Learning: In `packages/cli/src/auth/**` (including `codex-oauth-provider.ts` and related OAuth bridge files), `-1` is an intentional sentinel value used by stub `addItem` callbacks where no real history ID is available (for example `p.setAddItem?.(() => -1)`). Reviewers should not flag this as a magic number, nor suggest replacing it with a named constant, since callers never consume the returned ID and the literal `-1` is an established convention in this codebase.
Applied to files:
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.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/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-26T20:52:08.720Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:140-149
Timestamp: 2026-03-26T20:52:08.720Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts` (vybestack/llxprt-code PR `#1780`), the `applyThoughtToState` function's `setPendingHistoryItem` updater casts `item?.type as 'gemini' | 'gemini_content'` and uses `item?.text || ''` — this is intentional pre-existing behavior faithfully extracted verbatim from the original `useGeminiStream.ts` (lines 1110–1111 on main). The risk that a `tool_group` pending item type could be preserved is a known pre-existing pattern; fixing it requires careful testing of all thinking block rendering paths and is a behavioral change beyond decomposition scope. Do not flag this type cast as a bug in decomposition or refactoring PRs.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-26T21:40:39.044Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:521-525
Timestamp: 2026-03-26T21:40:39.044Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts` (vybestack/llxprt-code PR `#1780`), `processGeminiStreamEvents` previously always returned `StreamProcessingStatus.Completed` even when a `UserCancelled` or `Error` event was received — this was pre-existing dead code from the original `useGeminiStream.ts` (line 1371 checked for UserCancelled but the function always returned Completed). Fixed in commit 4a28363bc by adding a `processingResult` variable that tracks the status through the event loop, setting it to `StreamProcessingStatus.UserCancelled` or `StreamProcessingStatus.Error` as appropriate, and returning it at the end instead of always returning `Completed`.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-26T02:06:03.086Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentChatSetup.ts:339-453
Timestamp: 2026-03-26T02:06:03.086Z
Learning: In `packages/core/src/core/subagentChatSetup.ts` (vybestack/llxprt-code PR `#1779`, decomposed from `subagent.ts`), the `assembleChatSession` function's `try/catch` block wraps only `new GeminiChat(...)` and not the preceding `await ctx.environmentContextLoader(...)` or `await getCoreSystemPromptAsync(...)` calls. This partial error boundary is pre-existing behavior faithfully preserved from the original `createChatObject` method in `subagent.ts`. Do not flag the missing async-await coverage as a decomposition regression in future reviews — it is an acknowledged pre-existing limitation, and any fix would be a behavioral change requiring a dedicated follow-up PR.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-02-15T19:33:03.569Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1405
File: packages/core/src/lsp/__tests__/lsp-service-client-integration.test.ts:93-102
Timestamp: 2026-02-15T19:33:03.569Z
Learning: In the llxprt-code codebase (`packages/core/src/lsp/__tests__/lsp-service-client-integration.test.ts`), the LspServiceClient integration tests use a synthetic/stub implementation with PassThrough streams for testing. The test config may include fields like `filetypes` that differ from production types (`LspServerConfig`), as these tests verify the integration contract rather than exact production type conformance. Future work may add more realistic subprocess tests.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-26T20:01:50.052Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/checkpointPersistence.ts:0-0
Timestamp: 2026-03-26T20:01:50.052Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/checkpointPersistence.ts` (vybestack/llxprt-code PR `#1780`), `saveRestorableToolCalls` reserves the `callId` in `checkpointedCallIds` *before* calling `await createToolCheckpoint(...)` to prevent concurrent duplicate attempts. On failure (exception or false/no-write return), the `callId` is deleted from the set so the next effect run can retry. This reserve-before-await, delete-on-failure pattern was introduced in commit a41b97759.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-27T00:46:42.630Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/parseArgumentsParity.test.ts:7-16
Timestamp: 2026-03-27T00:46:42.630Z
Learning: In `packages/cli/src/config/__tests__/parseArgumentsParity.test.ts` (vybestack/llxprt-code PR `#1785`), the test suite is intentionally scoped to verifying that the extracted `parseArguments` function produces identical output to the original inline parsing. It deliberately does NOT cover subcommand exit behavior (mcp, hooks, extensions) via `handleSubcommandExit()` — that function is a direct mechanical extraction and adding integration tests for it would be new coverage beyond the refactoring scope. Do not flag the absence of subcommand-exit test cases as a gap in this file or in refactoring PRs that extract it.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-27T01:00:29.058Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/approvalModeParity.test.ts:351-393
Timestamp: 2026-03-27T01:00:29.058Z
Learning: In `packages/cli/src/config/__tests__/approvalModeParity.test.ts` (vybestack/llxprt-code PR `#1785`), the test suite is intentionally scoped to verifying that the extracted approval-mode resolution logic in `approvalModeResolver.ts` produces identical results to the original inline logic. Adding new combination scenarios (e.g., admin-disabled YOLO combined with an untrusted folder) is considered scope expansion beyond parity coverage. The ordering of admin checks before trust-fallback checks is preserved from the original code. Do not flag missing cross-branch combination tests in this file as a gap in refactoring PRs.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-21T17:07:15.661Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/ConversationManager.ts:94-101
Timestamp: 2026-03-21T17:07:15.661Z
Learning: In `packages/core/src/core/ConversationManager.ts`, `packages/core/src/core/TurnProcessor.ts`, `packages/core/src/core/StreamProcessor.ts`, and `packages/core/src/core/DirectMessageProcessor.ts` (vybestack/llxprt-code PR `#1743`, commit 870e85a91), `makePositionMatcher()` was hoisted outside all per-Content loops so that a single position matcher is shared across the entire batch. This supersedes the prior preserved-behavior pattern (original `geminiChat.ts` called it per-Content). The unmatched-tool queue is now consumed in order across all items in a batch, which is the correct behavior.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-04-03T20:43:35.912Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1876
File: packages/core/src/core/subagent.ts:441-464
Timestamp: 2026-04-03T20:43:35.912Z
Learning: In `packages/core/src/core/subagent.ts` (vybestack/llxprt-code), `runInteractiveTurn`'s `catch` block should only call `throw createAbortError()` when `abortController.signal.aborted` — it must NOT infer `SubagentTerminateMode.TIMEOUT` based on `!this.parentAbortSignal?.aborted`. `Turn.run()` absorbs idle-timeout events internally and emits `GeminiEventType.UserCancelled` before the generator exits, so the catch only fires for genuine errors or explicit cancel aborts. TIMEOUT is authoritatively set in the `onTimeout` callback of `nextStreamEventWithIdleTimeout` inside `runNonInteractiveTurn`. Do not conflate these two paths in future reviews.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-25T19:21:43.266Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1771
File: packages/core/src/core/AgentHookManager.ts:78-96
Timestamp: 2026-03-25T19:21:43.266Z
Learning: In `packages/core/src/core/AgentHookManager.ts` (vybestack/llxprt-code PR `#1771`), `cleanupOldHookState(newPromptId, oldPromptId)` is always called at the start of a new prompt turn (when `prompt_id` changes), after the previous prompt's stream has been fully consumed by `MessageStreamOrchestrator`. Because JS is single-threaded, there is no concurrent execution path where an in-flight `fireAfterAgentHookSafe` for the old `prompt_id` could race with the cleanup. Do not flag this pattern as a race condition in future reviews — the sequential flow guarantees: previous prompt finishes → new prompt starts → cleanup runs → fresh hook state created.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-03T17:18:48.615Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1656
File: packages/cli/src/auth/oauth-manager.auth-lock.spec.ts:47-70
Timestamp: 2026-03-03T17:18:48.615Z
Learning: In vybestack/llxprt-code TOCTOU defense tests (e.g., packages/cli/src/auth/oauth-manager.auth-lock.spec.ts), call-count assertions like `expect(getTokenCallCount).toBe(4)` are intentional structural guards that validate the double-check pattern (upfront checks + re-checks under lock). Behavioral assertions (e.g., authenticate() call counts per bucket) are primary, but call counts ensure the re-check path executes and catch regressions that might remove the defensive re-read while still passing behavioral tests.
```
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-25T18:17:38.768Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1768
File: packages/cli/src/ui/containers/AppContainer/hooks/useInputHandling.ts:127-135
Timestamp: 2026-03-25T18:17:38.768Z
Learning: In `packages/cli/src/ui/containers/AppContainer/hooks/useInputHandling.ts` (vybestack/llxprt-code PR `#1768`), the `hadToolCallsRef.current = false` reset inside `handleFinalSubmit` (before calling `submitQuery`) is intentional behavior preserved from the original `AppContainer.tsx` continuation flow contract. Do not flag this as a premature reset or race condition in decomposition or future PRs — any turn-boundary redesign (e.g., only clearing after `handleStreamCompleted`) is out of scope for the `#1576` refactoring and would constitute a behavioral change.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-04-03T20:43:54.493Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1876
File: packages/core/src/core/subagent.ts:675-710
Timestamp: 2026-04-03T20:43:54.493Z
Learning: In `packages/core/src/core/subagent.ts` (vybestack/llxprt-code PR `#1876`), `runNonInteractiveTurn` intentionally returns `{ functionCalls: [], textResponse: '' }` (instead of throwing) when `abortController.signal.aborted` is detected — both in the pre-loop early-abort check and inside the stream iteration loop. This is the correct design because: (1) `armTimeout` sets `output.terminate_reason = SubagentTerminateMode.TIMEOUT` *before* aborting, so abort-reason state is already recorded; (2) the caller in `runNonInteractive` checks `if (abortController.signal.aborted) return;` immediately after the call, resolving `runNonInteractive` as a normal `Promise<void>` (not a rejection); (3) existing TIMEOUT tests assert this resolution behavior and would break if `createAbortError()` were thrown instead. Do not flag the return-empty-on-abort pattern as dropping `finalizeOutput` or treating abort as success — it is intentional and test-validated.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-25T12:57:21.017Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1768
File: packages/cli/src/ui/hooks/useCoreEventHandlers.ts:71-78
Timestamp: 2026-03-25T12:57:21.017Z
Learning: In files under `packages/cli/src/ui/hooks/`, it is OK for a `useEffect` to register process-level cleanup via `registerCleanup(consolePatcher.cleanup)` without returning a React cleanup function (e.g., without `return () => consolePatcher.cleanup()`). Treat this as an intentional pattern: `registerCleanup()` is the project’s mechanism for cleanup that is meant to persist beyond React unmount/re-render, so do not flag the missing `return` as a leak/bug.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
📚 Learning: 2026-03-26T22:12:58.549Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:1-5
Timestamp: 2026-03-26T22:12:58.549Z
Learning: When reviewing this repo, do not flag these files for missing "upstream" Google LLC copyright. For `useGeminiStream.ts` and the extracted geminiStream-hook modules (under `packages/cli/src/ui/hooks/geminiStream/`), ensure the copyright header is `Copyright 2025 Vybestack LLC`—these files were already updated to Vybestack LLC before PR `#1780` and the decomposition outputs correctly use Vybestack LLC.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
🪛 GitHub Actions: LLxprt Code CI
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts
[error] 1-1: Prettier --write changed formatting in this file; the format check failed due to uncommitted changes.
packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts
[error] 1-1: Prettier --write changed formatting in this file; the format check failed due to uncommitted changes.
🔇 Additional comments (2)
packages/cli/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts (1)
24-52: Good focused regression coverage for unbucketed cache invalidation.The test cleanly verifies that
handler.invalidateAuthCache()delegates toflushRuntimeAuthScopewith the runtime ID, which is the core contract for this fix.packages/cli/src/ui/hooks/geminiStream/__tests__/useGeminiStream.turnPreparation.test.ts (1)
18-140: Strong branch and ordering coverage for turn preparation.These tests validate the critical branch split and ordering guarantees (
new turnvscontinuation) and include the runtime ID fallback path, matching the intended auth-refresh behavior.
Collaborator
Author
|
Addressed the remaining PR lint failure. The latest Lint (Javascript) failure was from the formatter check, not a logic error. CI reformatted three regression-test files and then failed on the resulting diff. I applied the formatter output locally, rechecked the affected CLI surfaces, and pushed the update. Rechecked locally:
Both still pass after the formatting-only update. |
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
Fixes #1739 - unbucketed profiles should check keychain on 401/etc or new turn
Problem
Unbucketed profiles (single-bucket OAuth profiles) do not properly handle auth token refresh when they encounter 401/403 errors. When a key expires and the user re-authenticates, other agents can continue using cached invalid credentials instead of picking up the new token from the keychain.
Root Cause
Solution
Changes
Verification
Notes
This keeps multi-bucket behavior intact while allowing single-bucket profiles to use the existing Pass 3 foreground re-auth path and refresh keychain state on new turns.
Fixes #1739