Conversation
github-actions
bot
added
the
maintainer:e2e:ok
Contributor
Summary by CodeRabbit
WalkthroughReplaced a hardcoded Codex device-auth callback URI with Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SecureStore
participant Keyring as Keyring Adapter
participant Fallback as Fallback File
Client->>SecureStore: set(key, value, {fallbackPolicy})
SecureStore->>Keyring: setPassword(key, encryptedValue)
alt Keyring succeeds
Keyring-->>SecureStore: success
alt fallbackPolicy == 'deny' and platform != linux
SecureStore-->>Client: return success (no fallback)
else
SecureStore->>Fallback: try writeFallbackFile(...) (backup)
alt write succeeds
Fallback-->>SecureStore: written
SecureStore-->>Client: return success
else write fails
SecureStore-->>SecureStore: debug log error
SecureStore-->>Client: return success
end
end
else Keyring fails
Keyring-->>SecureStore: error
alt fallbackPolicy == 'allow' or platform == linux
SecureStore->>Fallback: writeFallbackFile(...) (await)
Fallback-->>SecureStore: written
SecureStore-->>Client: return success (via fallback)
else fallbackPolicy == 'deny'
SecureStore-->>Client: throw SecureStoreError (UNAVAILABLE or classified)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
LLxprt PR Review – PR #1897Issue AlignmentThe PR correctly addresses Issue #1895 with two targeted fixes:
Side Effects
Code QualityOverall sound. Key observations:
Tests and CoverageCoverage impact: Increase
Flag: The test "should persist fallback file even when keyring write succeeds on Linux" only asserts VerdictReady — The PR cleanly fixes both root causes of Issue #1895 with targeted, well-tested changes. The fallback dual-write on Linux, canonical URI wiring, and |
Collaborator
Author
|
Updated the PR description to explicitly include Fixes #1895 so the automated review can evaluate the linked issue context. |
Collaborator
Author
|
Addressed the formatter failure by updating packages/core/src/storage/secure-store.spec.ts to the exact layout expected by the CI Prettier check, then pushed commit 66a7a33. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/storage/secure-store.ts (1)
425-449:⚠️ Potential issue | 🟠 MajorThis changes
fallbackPolicy: 'allow'from fallback-permitted to always-write-a-disk-copy.
packages/core/src/storage/provider-key-storage.ts:71-75usesfallbackPolicy: 'allow'in production, so this branch now writes a fallback file on every successful provider-credential save, not just the Linux recovery case from issue#1895. That's a material storage-model change for healthy keyring installs. Please gate the extra backup write behind a narrower opt-in or Linux-specific path instead of the existing broadallowpolicy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/storage/secure-store.ts` around lines 425 - 449, The current logic writes a fallback file whenever fallbackPolicy === 'allow', causing every successful keyring write to also create a disk backup; change the condition around writeFallbackFile so it only writes the fallback when either the keyring write failed (keyringWriteSucceeded === false) or when running on Linux for reliability (process.platform === 'linux'), or behind a new explicit opt-in flag (e.g., writeFallbackOnSuccess) if you prefer configuration; update the branch in the set flow that calls writeFallbackFile (referencing fallbackPolicy, keyringWriteSucceeded, and writeFallbackFile) to check these narrower conditions so production use of fallbackPolicy: 'allow' no longer forces an extra backup write.
🤖 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/core/src/storage/secure-store.spec.ts`:
- Around line 1-233: The new test file "secure-store.spec.ts" (the "SecureStore
- Linux Keyring Fallback Reliability" suite) is failing Prettier; run the
project formatter (npm run format or the repo's Prettier config) on that spec to
apply the style fixes, ensure the formatted output for the tests (describe
blocks, it cases, imports, and async callbacks around SecureStore,
SecureStoreError, KeyringAdapter usage) is committed, and push the updated
formatted file so CI no longer reports Prettier rewrites.
In `@packages/core/src/storage/secure-store.ts`:
- Around line 442-449: The set() flow currently treats writeFallbackFile
failures as fatal even after keyringWriteSucceeded is true; change it so that
when keyringWriteSucceeded is true and fallbackPolicy === 'allow' you attempt
writeFallbackFile(key, value) but catch and log any errors (use
this.logger.error/debug with fallbackDir and key) and do not rethrow, making the
fallback best-effort; only treat a fallback write as required (and propagate
errors) when the keyring path did not persist the secret (i.e.,
keyringWriteSucceeded is false), preserving the existing behavior for the
non-keyring code paths.
---
Outside diff comments:
In `@packages/core/src/storage/secure-store.ts`:
- Around line 425-449: The current logic writes a fallback file whenever
fallbackPolicy === 'allow', causing every successful keyring write to also
create a disk backup; change the condition around writeFallbackFile so it only
writes the fallback when either the keyring write failed (keyringWriteSucceeded
=== false) or when running on Linux for reliability (process.platform ===
'linux'), or behind a new explicit opt-in flag (e.g., writeFallbackOnSuccess) if
you prefer configuration; update the branch in the set flow that calls
writeFallbackFile (referencing fallbackPolicy, keyringWriteSucceeded, and
writeFallbackFile) to check these narrower conditions so production use of
fallbackPolicy: 'allow' no longer forces an extra backup write.
🪄 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: fa73840f-9527-45c9-883c-0453beba7186
📒 Files selected for processing (5)
packages/cli/src/auth/codex-oauth-provider.spec.tspackages/cli/src/auth/codex-oauth-provider.tspackages/core/src/auth/codex-device-flow.tspackages/core/src/storage/secure-store.spec.tspackages/core/src/storage/secure-store.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: Run LLxprt review
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:docker
🧰 Additional context used
🧠 Learnings (34)
📓 Common learnings
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: 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/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: 1778
File: packages/cli/src/auth/qwen-oauth-provider.ts:234-242
Timestamp: 2026-03-26T00:30:20.796Z
Learning: In `packages/cli/src/auth/qwen-oauth-provider.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the broad `catch` block in `openQwenBrowserIfInteractive` that silently swallows all errors from the dynamic import of `../runtime/runtimeSettings.js` (setting `noBrowser = false` as the default) is pre-existing behavior faithfully extracted from the original `oauth-manager.ts`. Do not flag the absence of debug logging or error discrimination in this catch block as a gap in decomposition or future PRs — adding error-type discrimination would be a behavioral change beyond the refactoring scope.
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/provider-usage-info.ts:228-233
Timestamp: 2026-03-26T00:30:00.337Z
Learning: In `packages/cli/src/auth/provider-usage-info.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), `getHigherPriorityAuth` calls `getSettingsService().get('authOnly')` globally (without a try/catch guard) alongside the passed `LoadedSettings`. This is pre-existing behavior faithfully extracted from the original `getHigherPriorityAuth` method at line 1836 of `oauth-manager.ts` on main. Do not flag the unconditional `getSettingsService()` call or the `authOnly` handling pattern as a new issue or scope expansion in decomposition or future PRs — rearchitecting the auth-priority dependency chain is explicitly out of scope.
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.
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: 1778
File: packages/cli/src/auth/auth-status-service.ts:241-263
Timestamp: 2026-03-26T02:12:35.416Z
Learning: In `packages/cli/src/auth/auth-status-service.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), `logoutAll()` only iterates `tokenStore.listProviders()` and does not include providers whose auth state is managed exclusively via `provider.isAuthenticated()` (i.e., providers with no persisted token store entry). This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` `logoutAll()`. Do not flag this as a regression or gap in decomposition PRs — improving coverage to include registry-only providers is a follow-up enhancement, not a refactoring concern.
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.
📚 Learning: 2026-03-26T00:30:20.796Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/qwen-oauth-provider.ts:234-242
Timestamp: 2026-03-26T00:30:20.796Z
Learning: In `packages/cli/src/auth/qwen-oauth-provider.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the broad `catch` block in `openQwenBrowserIfInteractive` that silently swallows all errors from the dynamic import of `../runtime/runtimeSettings.js` (setting `noBrowser = false` as the default) is pre-existing behavior faithfully extracted from the original `oauth-manager.ts`. Do not flag the absence of debug logging or error discrimination in this catch block as a gap in decomposition or future PRs — adding error-type discrimination would be a behavioral change beyond the refactoring scope.
Applied to files:
packages/cli/src/auth/codex-oauth-provider.tspackages/core/src/auth/codex-device-flow.ts
📚 Learning: 2026-02-16T19:05:47.580Z
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.
Applied to files:
packages/cli/src/auth/codex-oauth-provider.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/codex-oauth-provider.tspackages/cli/src/auth/codex-oauth-provider.spec.ts
📚 Learning: 2026-03-26T00:30:00.337Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/provider-usage-info.ts:228-233
Timestamp: 2026-03-26T00:30:00.337Z
Learning: In `packages/cli/src/auth/provider-usage-info.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), `getHigherPriorityAuth` calls `getSettingsService().get('authOnly')` globally (without a try/catch guard) alongside the passed `LoadedSettings`. This is pre-existing behavior faithfully extracted from the original `getHigherPriorityAuth` method at line 1836 of `oauth-manager.ts` on main. Do not flag the unconditional `getSettingsService()` call or the `authOnly` handling pattern as a new issue or scope expansion in decomposition or future PRs — rearchitecting the auth-priority dependency chain is explicitly out of scope.
Applied to files:
packages/cli/src/auth/codex-oauth-provider.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/codex-oauth-provider.tspackages/cli/src/auth/codex-oauth-provider.spec.ts
📚 Learning: 2026-03-26T02:12:35.416Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/auth-status-service.ts:241-263
Timestamp: 2026-03-26T02:12:35.416Z
Learning: In `packages/cli/src/auth/auth-status-service.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), `logoutAll()` only iterates `tokenStore.listProviders()` and does not include providers whose auth state is managed exclusively via `provider.isAuthenticated()` (i.e., providers with no persisted token store entry). This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` `logoutAll()`. Do not flag this as a regression or gap in decomposition PRs — improving coverage to include registry-only providers is a follow-up enhancement, not a refactoring concern.
Applied to files:
packages/cli/src/auth/codex-oauth-provider.tspackages/cli/src/auth/codex-oauth-provider.spec.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/codex-oauth-provider.tspackages/core/src/auth/codex-device-flow.tspackages/core/src/storage/secure-store.tspackages/cli/src/auth/codex-oauth-provider.spec.tspackages/core/src/storage/secure-store.spec.ts
📚 Learning: 2026-03-03T15:00:27.688Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1656
File: packages/cli/src/auth/gemini-oauth-provider.ts:349-361
Timestamp: 2026-03-03T15:00:27.688Z
Learning: Centralize token expiry handling in OAuthManager and keep OAuth provider implementations as read-only pass-throughs. In vybestack/llxprt-code, after the issue `#1652` refactor, providers (GeminiOAuthProvider, CodexOAuthProvider, AnthropicOAuthProvider, QwenOAuthProvider) should not perform expiry checks or refresh logic; OAuthManager.getToken() and OAuthManager.getOAuthToken() must validate expiry and refresh when needed before returning tokens. Provider refreshIfNeeded() should be deprecated no-ops. This pattern should apply to all OAuth provider implementations, not just the Gemini provider.
Applied to files:
packages/cli/src/auth/codex-oauth-provider.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/codex-oauth-provider.tspackages/cli/src/auth/codex-oauth-provider.spec.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/codex-oauth-provider.tspackages/cli/src/auth/codex-oauth-provider.spec.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/codex-oauth-provider.tspackages/core/src/auth/codex-device-flow.tspackages/core/src/storage/secure-store.tspackages/cli/src/auth/codex-oauth-provider.spec.tspackages/core/src/storage/secure-store.spec.ts
📚 Learning: 2026-03-25T00:25:22.892Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1741
File: packages/core/src/config/configConstructor.ts:77-203
Timestamp: 2026-03-25T00:25:22.892Z
Learning: In `packages/core/src/config/configConstructor.ts` (vybestack/llxprt-code PR `#1741`), `ConfigConstructorTarget` is an intentionally exported shadow interface that exists to break the circular dependency between `configConstructor.ts` and `config.ts` — `configConstructor.ts` cannot import `Config` directly without creating a cycle. The six `as unknown as Config` casts inside `applyConfigParams` are intentional and limited to operations that genuinely require the full concrete `Config` type: `initializeTelemetry`, `FileExclusions`, `createAgentRuntimeStateFromConfig`, and `logCliConfiguration`. Both `ConfigConstructorTarget` and `applyConfigParams` must remain exported so `config.ts` can import and use them. Do not flag this pattern as a maintenance smell or suggest moving these symbols to be non-exported or file-local in decomposition/refactoring PRs.
Applied to files:
packages/core/src/auth/codex-device-flow.ts
📚 Learning: 2026-03-24T23:55:40.614Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1741
File: packages/core/src/config/config.ts:52-82
Timestamp: 2026-03-24T23:55:40.614Z
Learning: In vybestack/llxprt-code, `TelemetryTarget` was never exported from `packages/core/src/config/config.ts` on main. It is always imported directly from `../telemetry/index.js` (or re-exported via `configTypes.ts`). Do not flag its absence from the backward-compat re-export block in `config.ts` as a missing export — no consumer imports it from any config path.
Applied to files:
packages/core/src/auth/codex-device-flow.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/auth/codex-device-flow.ts
📚 Learning: 2026-03-27T01:25:02.388Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1784
File: packages/cli/src/auth/codex-oauth-provider.ts:394-402
Timestamp: 2026-03-27T01:25:02.388Z
Learning: In `packages/cli/src/auth/codex-oauth-provider.ts` and related files (vybestack/llxprt-code), `-1` is an established sentinel value for stub `addItem` callbacks where no real history ID is available (e.g., `p.setAddItem?.(() => -1)` in `useUpdateAndOAuthBridges.ts`). Callers never consume the returned ID. Do not suggest replacing this literal with a named constant — it is intentional codebase-wide convention.
Applied to files:
packages/core/src/auth/codex-device-flow.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/core/src/storage/secure-store.ts
📚 Learning: 2026-03-24T21:35:42.622Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1766
File: packages/cli/src/runtime/settingsResolver.ts:96-103
Timestamp: 2026-03-24T21:35:42.622Z
Learning: In `packages/cli/src/runtime/settingsResolver.ts` (vybestack/llxprt-code PR `#1766`), the `--key` and `--keyfile` branches both call `updateActiveProviderApiKey`, which internally calls `config.setEphemeralSetting('auth-key-name', undefined)` at line 289 of `providerMutations.ts` (update branch) and line 265 (remove branch). Do not flag missing `auth-key-name` clearing at the `settingsResolver.ts` call sites — it is already handled inside the mutation function. Trace the full call chain into `updateActiveProviderApiKey` before raising such a comment.
Applied to files:
packages/core/src/storage/secure-store.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/core/src/storage/secure-store.tspackages/core/src/storage/secure-store.spec.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/storage/secure-store.tspackages/core/src/storage/secure-store.spec.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/codex-oauth-provider.spec.tspackages/core/src/storage/secure-store.spec.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/codex-oauth-provider.spec.tspackages/core/src/storage/secure-store.spec.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/cli/src/auth/codex-oauth-provider.spec.tspackages/core/src/storage/secure-store.spec.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/codex-oauth-provider.spec.tspackages/core/src/storage/secure-store.spec.ts
📚 Learning: 2026-03-19T23:27:49.587Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/__tests__/OpenAIProvider.e2e.test.ts:199-203
Timestamp: 2026-03-19T23:27:49.587Z
Learning: In `packages/core/src/providers/openai/__tests__/OpenAIProvider.e2e.test.ts` (vybestack/llxprt-code), Scenarios 3, 7a, 7b, and 7c use `buildMessagesWithReasoning` (imported directly from `OpenAIRequestBuilder`) rather than calling `provider.generateChatCompletion`. This is intentional and pre-existing behavior: the original tests accessed the same helper via a hacky `buildMessagesWithReasoning.call(provider, ...)` private-method pattern. The PR's direct import is an improvement, not a regression. Do not flag these scenarios as insufficiently integrated — they are helper-level tests by design, and adding full provider-path coverage is out of scope for refactoring PRs.
Applied to files:
packages/cli/src/auth/codex-oauth-provider.spec.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/codex-oauth-provider.spec.tspackages/core/src/storage/secure-store.spec.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/codex-oauth-provider.spec.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/codex-oauth-provider.spec.tspackages/core/src/storage/secure-store.spec.ts
📚 Learning: 2026-03-31T02:14:05.575Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/providers/openai/schemaConverter.issue1844.test.ts:14-20
Timestamp: 2026-03-31T02:14:05.575Z
Learning: In `packages/core/src/providers/openai/schemaConverter.issue1844.test.ts` and neighboring issue regression test files (e.g., `openai-vercel/schemaConverter.issue1844.test.ts`, `subagentRuntimeSetup.issue1844.test.ts`, `AnthropicResponseParser.issue1844.test.ts`), the pattern of using a `beforeAll` async dynamic import to load the module under test is intentional. This style is used consistently across all `*.issue1844.test.ts` regression test files for loading consistency. Do not suggest replacing dynamic imports with static imports in these files.
Applied to files:
packages/cli/src/auth/codex-oauth-provider.spec.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/core/src/storage/secure-store.spec.ts
📚 Learning: 2026-03-27T01:00:28.649Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/toolGovernanceParity.test.ts:458-472
Timestamp: 2026-03-27T01:00:28.649Z
Learning: In `packages/cli/src/config/__tests__/toolGovernanceParity.test.ts` (vybestack/llxprt-code PR `#1785`), the "non-interactive DEFAULT with explicit --allowed-tools" and corresponding YOLO test cases intentionally use `read_file` (a tool already in `READ_ONLY_TOOL_NAMES`) as the explicit `--allowed-tools` value. The tests are scoped to verifying that existing allowlist behavior is preserved through the refactor, not to proving union/exclusion semantics with non-read-only tools. Do not flag the choice of `read_file` as insufficient for proving merging behavior in parity or refactoring PRs — improving assertion specificity is new test design work beyond the refactoring scope.
Applied to files:
packages/core/src/storage/secure-store.spec.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/core/src/storage/secure-store.spec.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/core/src/storage/secure-store.spec.ts
📚 Learning: 2026-03-26T01:28:37.959Z
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:37.959Z
Learning: In `packages/cli/src/auth/provider-usage-info.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the `bucket ?? 'default'` normalization before calling `tokenStore.getToken(provider, bucket)` is correct and functionally identical to passing `undefined`. The production `KeyringTokenStore` (packages/core/src/auth/keyring-token-store.ts, line 94) already normalizes `undefined` to `DEFAULT_BUCKET` (the string `"default"`) internally via `const resolvedBucket = bucket ?? DEFAULT_BUCKET`. The `InMemoryTokenStore` used in test helpers does not define the production contract. Do not flag `bucket ?? 'default'` lookups as a contract violation or suggest splitting into `undefined`-for-lookup vs `'default'`-for-display in this codebase — the original `oauth-manager.ts` used the same pattern (line 2169) and the behavior is faithfully preserved.
Applied to files:
packages/core/src/storage/secure-store.spec.ts
🪛 GitHub Actions: LLxprt Code CI
packages/core/src/storage/secure-store.spec.ts
[error] 133-133: Prettier failed during formatting step (npm run format / prettier --experimental-cli --write .). Prettier reported a diff indicating formatting changes were applied, and the job exited with code 1.
🔇 Additional comments (3)
packages/core/src/auth/codex-device-flow.ts (1)
18-20: Good centralization of Codex OAuth constants.Exporting
CODEX_CONFIGhere gives CLI/core a single source of truth and directly prevents callback URI drift.packages/cli/src/auth/codex-oauth-provider.ts (1)
11-12: Canonical callback wiring is correctly applied.Nice fix: provider now consumes
CODEX_CONFIG.deviceAuthCallbackUriinstead of a literal, which addresses the redirect mismatch at the root cause.Also applies to: 336-341
packages/cli/src/auth/codex-oauth-provider.spec.ts (1)
294-363: Targeted regression test is solid.This test cleanly locks in the canonical device callback URI behavior and explicitly guards against reintroducing the old incorrect URI.
Collaborator
Author
|
Addressed the formatter failure by updating packages/core/src/storage/secure-store.spec.ts to the layout expected by the CI Prettier check, then pushed commit 66a7a33. |
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/core/src/storage/secure-store.spec.ts`:
- Around line 116-121: The test currently calls store.set('denied-key',
'secret') twice; instead call it once, capture the thrown rejection (e.g., const
p = store.set('denied-key', 'secret')) and assert both the error class
(SecureStoreError) and the message ('Keyring is unavailable and fallback is
denied') from that single invocation so the failure path is verified without
re-executing store.set.
- Around line 43-60: The test's keyring-failure simulation is flaky due to
timer-based toggling; modify the mockKeyring used by SecureStore to expose a
controllable state (e.g., a boolean flag or a promise you can resolve) instead
of calling setTimeout and sleeping—have keyring.setPassword check that
flag/promise and behave as successful first call then fail deterministically
when you flip the flag (or resolve the promise) in the test before continuing;
update the test to flip the flag or resolve the promise instead of relying on
setTimeout + sleep so SecureStore.set('test-key', ...) and the subsequent
failure behavior are reproducible.
🪄 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: 6b3ea789-3535-4f1a-ac12-9e8bc7a43eab
📒 Files selected for processing (1)
packages/core/src/storage/secure-store.spec.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: Lint (Javascript)
🧰 Additional context used
🧠 Learnings (19)
📓 Common learnings
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: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: 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: 1778
File: packages/cli/src/auth/qwen-oauth-provider.ts:234-242
Timestamp: 2026-03-26T00:30:20.796Z
Learning: In `packages/cli/src/auth/qwen-oauth-provider.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the broad `catch` block in `openQwenBrowserIfInteractive` that silently swallows all errors from the dynamic import of `../runtime/runtimeSettings.js` (setting `noBrowser = false` as the default) is pre-existing behavior faithfully extracted from the original `oauth-manager.ts`. Do not flag the absence of debug logging or error discrimination in this catch block as a gap in decomposition or future PRs — adding error-type discrimination would be a behavioral change beyond the refactoring scope.
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/provider-usage-info.ts:36-37
Timestamp: 2026-03-26T01:28:37.959Z
Learning: In `packages/cli/src/auth/provider-usage-info.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the `bucket ?? 'default'` normalization before calling `tokenStore.getToken(provider, bucket)` is correct and functionally identical to passing `undefined`. The production `KeyringTokenStore` (packages/core/src/auth/keyring-token-store.ts, line 94) already normalizes `undefined` to `DEFAULT_BUCKET` (the string `"default"`) internally via `const resolvedBucket = bucket ?? DEFAULT_BUCKET`. The `InMemoryTokenStore` used in test helpers does not define the production contract. Do not flag `bucket ?? 'default'` lookups as a contract violation or suggest splitting into `undefined`-for-lookup vs `'default'`-for-display in this codebase — the original `oauth-manager.ts` used the same pattern (line 2169) and the behavior is faithfully preserved.
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: 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/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.
📚 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/core/src/storage/secure-store.spec.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/core/src/storage/secure-store.spec.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/storage/secure-store.spec.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/storage/secure-store.spec.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/storage/secure-store.spec.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/core/src/storage/secure-store.spec.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/core/src/storage/secure-store.spec.ts
📚 Learning: 2026-03-27T01:00:28.649Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/toolGovernanceParity.test.ts:458-472
Timestamp: 2026-03-27T01:00:28.649Z
Learning: In `packages/cli/src/config/__tests__/toolGovernanceParity.test.ts` (vybestack/llxprt-code PR `#1785`), the "non-interactive DEFAULT with explicit --allowed-tools" and corresponding YOLO test cases intentionally use `read_file` (a tool already in `READ_ONLY_TOOL_NAMES`) as the explicit `--allowed-tools` value. The tests are scoped to verifying that existing allowlist behavior is preserved through the refactor, not to proving union/exclusion semantics with non-read-only tools. Do not flag the choice of `read_file` as insufficient for proving merging behavior in parity or refactoring PRs — improving assertion specificity is new test design work beyond the refactoring scope.
Applied to files:
packages/core/src/storage/secure-store.spec.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/core/src/storage/secure-store.spec.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/core/src/storage/secure-store.spec.ts
📚 Learning: 2025-11-25T16:56:18.980Z
Learnt from: CR
Repo: vybestack/llxprt-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T16:56:18.980Z
Learning: Before reporting a task as finished, run `npm run format` from the repository root and ensure it succeeds (exit code 0)
Applied to files:
packages/core/src/storage/secure-store.spec.ts
📚 Learning: 2026-02-26T19:06:23.993Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1627
File: integration-tests/run_shell_command.test.ts:550-557
Timestamp: 2026-02-26T19:06:23.993Z
Learning: In `integration-tests/run_shell_command.test.ts`, the "rejects invalid shell expressions" test intentionally does not assert `toolRequest.success === false` for shell syntax errors because issue `#1625` tracks that `run_shell_command` currently reports `success: true` for commands that fail with non-zero exit codes (it only sets `result.error` on spawn failures, not non-zero exits). The test uses FakeProvider to script model responses but executes the real tool, so asserting `success === false` would fail until `#1625` is resolved. The test correctly verifies the tool was invoked with the invalid syntax and the model responded with FAIL.
<!--
Applied to files:
packages/core/src/storage/secure-store.spec.ts
📚 Learning: 2026-03-24T21:07:40.805Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1767
File: packages/cli/src/ui/components/shared/golden-snapshot.test.ts:64-65
Timestamp: 2026-03-24T21:07:40.805Z
Learning: In `packages/cli/src/ui/components/shared/golden-snapshot.test.ts` (vybestack/llxprt-code PR `#1767`), `parseAction` uses `actionStr.split(':')` to parse action corpus entries. The action corpus (`project-plans/issue1577/action-corpus.json`) has been confirmed to contain zero multi-colon action strings, so the current two-element destructuring is correct for all existing entries. Do not flag this as a truncation bug in future reviews — the corpus format is validated and the parsing matches it.
Applied to files:
packages/core/src/storage/secure-store.spec.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/storage/secure-store.spec.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/storage/secure-store.spec.ts
📚 Learning: 2026-03-26T01:28:37.959Z
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:37.959Z
Learning: In `packages/cli/src/auth/provider-usage-info.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the `bucket ?? 'default'` normalization before calling `tokenStore.getToken(provider, bucket)` is correct and functionally identical to passing `undefined`. The production `KeyringTokenStore` (packages/core/src/auth/keyring-token-store.ts, line 94) already normalizes `undefined` to `DEFAULT_BUCKET` (the string `"default"`) internally via `const resolvedBucket = bucket ?? DEFAULT_BUCKET`. The `InMemoryTokenStore` used in test helpers does not define the production contract. Do not flag `bucket ?? 'default'` lookups as a contract violation or suggest splitting into `undefined`-for-lookup vs `'default'`-for-display in this codebase — the original `oauth-manager.ts` used the same pattern (line 2169) and the behavior is faithfully preserved.
Applied to files:
packages/core/src/storage/secure-store.spec.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/storage/secure-store.spec.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/storage/secure-store.spec.ts
🔇 Additional comments (2)
packages/core/src/storage/secure-store.spec.ts (2)
132-155: Good precedence test for keyring-over-fallback reads.This case clearly locks down the intended source-of-truth ordering and protects against regressions in mixed-storage states.
190-231: Nice policy-preservation coverage forallowvsdeny.These tests map directly to behavior contract boundaries and complement the Linux fallback regression cases well.
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. |
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/core/src/storage/secure-store.ts (1)
416-439:⚠️ Potential issue | 🟠 MajorPreserve the real keyring error on the deny path.
If
adapter.setPassword()fails because the keyring is locked, denied, or times out, Lines 431-439 rewrite that intoUNAVAILABLE. Downstream callers likepackages/core/src/tools/tool-key-storage.tsalready treatUNAVAILABLEas “fall back to file storage”, so this can silently bypass a locked/denied keychain and show the wrong remediation. ReserveUNAVAILABLEfor the no-adapter case and rethrow the classified keyring error when the write itself failed.Suggested fix
const adapter = await this.getKeyring(); let keyringWriteSucceeded = false; + let keyringWriteError: unknown = null; if (adapter !== null) { try { await adapter.setPassword(this.serviceName, key, value); this.recordKeyringSuccess(); this.logger.debug(() => `[set] key='${key}' → keyring (OS keychain)`); keyringWriteSucceeded = true; } catch (error) { + keyringWriteError = error; this.recordKeyringFailure(); const msg = error instanceof Error ? error.message : String(error); this.logger.debug( () => `[set] key='${key}' keyring write failed: ${msg}`, ); @@ // If keyring unavailable or write failed, check fallback policy if (!keyringWriteSucceeded && this.fallbackPolicy === 'deny') { + if (adapter !== null && keyringWriteError !== null) { + const code = classifyError(keyringWriteError); + throw new SecureStoreError( + keyringWriteError instanceof Error + ? keyringWriteError.message + : String(keyringWriteError), + code, + getRemediation(code), + ); + } this.logger.debug( () => `[set] key='${key}' fallback denied — throwing UNAVAILABLE`, ); throw new SecureStoreError( 'Keyring is unavailable and fallback is denied',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/storage/secure-store.ts` around lines 416 - 439, When adapter.setPassword() fails, capture and preserve the real error (instead of losing it and always throwing a generic UNAVAILABLE) by storing the caught error in a local variable (e.g., keyringWriteError) inside the catch that also calls recordKeyringFailure(), and then in the deny-path check that currently inspects keyringWriteSucceeded and this.fallbackPolicy, rethrow the preserved keyringWriteError if present (or rethrow it as-is/convert to SecureStoreError if needed); only throw the generic UNAVAILABLE when there was no adapter error at all (the true “no-adapter” case). Reference: the catch around adapter.setPassword(), recordKeyringFailure(), keyringWriteSucceeded, and the fallbackPolicy === 'deny' branch.
🤖 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/core/src/storage/secure-store.ts`:
- Around line 416-439: When adapter.setPassword() fails, capture and preserve
the real error (instead of losing it and always throwing a generic UNAVAILABLE)
by storing the caught error in a local variable (e.g., keyringWriteError) inside
the catch that also calls recordKeyringFailure(), and then in the deny-path
check that currently inspects keyringWriteSucceeded and this.fallbackPolicy,
rethrow the preserved keyringWriteError if present (or rethrow it as-is/convert
to SecureStoreError if needed); only throw the generic UNAVAILABLE when there
was no adapter error at all (the true “no-adapter” case). Reference: the catch
around adapter.setPassword(), recordKeyringFailure(), keyringWriteSucceeded, and
the fallbackPolicy === 'deny' branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1f13356-99ca-4b63-8978-1c4d190ad0ec
📒 Files selected for processing (2)
packages/core/src/storage/secure-store.spec.tspackages/core/src/storage/secure-store.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 (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: Lint (Javascript)
🧰 Additional context used
🧠 Learnings (29)
📓 Common learnings
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: 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/qwen-oauth-provider.ts:234-242
Timestamp: 2026-03-26T00:30:20.796Z
Learning: In `packages/cli/src/auth/qwen-oauth-provider.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the broad `catch` block in `openQwenBrowserIfInteractive` that silently swallows all errors from the dynamic import of `../runtime/runtimeSettings.js` (setting `noBrowser = false` as the default) is pre-existing behavior faithfully extracted from the original `oauth-manager.ts`. Do not flag the absence of debug logging or error discrimination in this catch block as a gap in decomposition or future PRs — adding error-type discrimination would be a behavioral change beyond the refactoring scope.
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/provider-usage-info.ts:36-37
Timestamp: 2026-03-26T01:28:37.959Z
Learning: In `packages/cli/src/auth/provider-usage-info.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the `bucket ?? 'default'` normalization before calling `tokenStore.getToken(provider, bucket)` is correct and functionally identical to passing `undefined`. The production `KeyringTokenStore` (packages/core/src/auth/keyring-token-store.ts, line 94) already normalizes `undefined` to `DEFAULT_BUCKET` (the string `"default"`) internally via `const resolvedBucket = bucket ?? DEFAULT_BUCKET`. The `InMemoryTokenStore` used in test helpers does not define the production contract. Do not flag `bucket ?? 'default'` lookups as a contract violation or suggest splitting into `undefined`-for-lookup vs `'default'`-for-display in this codebase — the original `oauth-manager.ts` used the same pattern (line 2169) and the behavior is faithfully preserved.
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/provider-usage-info.ts:228-233
Timestamp: 2026-03-26T00:30:00.337Z
Learning: In `packages/cli/src/auth/provider-usage-info.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), `getHigherPriorityAuth` calls `getSettingsService().get('authOnly')` globally (without a try/catch guard) alongside the passed `LoadedSettings`. This is pre-existing behavior faithfully extracted from the original `getHigherPriorityAuth` method at line 1836 of `oauth-manager.ts` on main. Do not flag the unconditional `getSettingsService()` call or the `authOnly` handling pattern as a new issue or scope expansion in decomposition or future PRs — rearchitecting the auth-priority dependency chain is explicitly out of scope.
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.
📚 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/core/src/storage/secure-store.spec.tspackages/core/src/storage/secure-store.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/core/src/storage/secure-store.spec.tspackages/core/src/storage/secure-store.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/storage/secure-store.spec.tspackages/core/src/storage/secure-store.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/storage/secure-store.spec.tspackages/core/src/storage/secure-store.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/core/src/storage/secure-store.spec.tspackages/core/src/storage/secure-store.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/storage/secure-store.spec.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/core/src/storage/secure-store.spec.tspackages/core/src/storage/secure-store.ts
📚 Learning: 2026-03-27T01:00:28.649Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/toolGovernanceParity.test.ts:458-472
Timestamp: 2026-03-27T01:00:28.649Z
Learning: In `packages/cli/src/config/__tests__/toolGovernanceParity.test.ts` (vybestack/llxprt-code PR `#1785`), the "non-interactive DEFAULT with explicit --allowed-tools" and corresponding YOLO test cases intentionally use `read_file` (a tool already in `READ_ONLY_TOOL_NAMES`) as the explicit `--allowed-tools` value. The tests are scoped to verifying that existing allowlist behavior is preserved through the refactor, not to proving union/exclusion semantics with non-read-only tools. Do not flag the choice of `read_file` as insufficient for proving merging behavior in parity or refactoring PRs — improving assertion specificity is new test design work beyond the refactoring scope.
Applied to files:
packages/core/src/storage/secure-store.spec.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/core/src/storage/secure-store.spec.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/core/src/storage/secure-store.spec.ts
📚 Learning: 2025-11-25T16:56:18.980Z
Learnt from: CR
Repo: vybestack/llxprt-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T16:56:18.980Z
Learning: Before reporting a task as finished, run `npm run format` from the repository root and ensure it succeeds (exit code 0)
Applied to files:
packages/core/src/storage/secure-store.spec.ts
📚 Learning: 2026-02-26T19:06:23.993Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1627
File: integration-tests/run_shell_command.test.ts:550-557
Timestamp: 2026-02-26T19:06:23.993Z
Learning: In `integration-tests/run_shell_command.test.ts`, the "rejects invalid shell expressions" test intentionally does not assert `toolRequest.success === false` for shell syntax errors because issue `#1625` tracks that `run_shell_command` currently reports `success: true` for commands that fail with non-zero exit codes (it only sets `result.error` on spawn failures, not non-zero exits). The test uses FakeProvider to script model responses but executes the real tool, so asserting `success === false` would fail until `#1625` is resolved. The test correctly verifies the tool was invoked with the invalid syntax and the model responded with FAIL.
<!--
Applied to files:
packages/core/src/storage/secure-store.spec.ts
📚 Learning: 2026-03-24T21:07:40.805Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1767
File: packages/cli/src/ui/components/shared/golden-snapshot.test.ts:64-65
Timestamp: 2026-03-24T21:07:40.805Z
Learning: In `packages/cli/src/ui/components/shared/golden-snapshot.test.ts` (vybestack/llxprt-code PR `#1767`), `parseAction` uses `actionStr.split(':')` to parse action corpus entries. The action corpus (`project-plans/issue1577/action-corpus.json`) has been confirmed to contain zero multi-colon action strings, so the current two-element destructuring is correct for all existing entries. Do not flag this as a truncation bug in future reviews — the corpus format is validated and the parsing matches it.
Applied to files:
packages/core/src/storage/secure-store.spec.ts
📚 Learning: 2026-03-24T21:35:42.622Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1766
File: packages/cli/src/runtime/settingsResolver.ts:96-103
Timestamp: 2026-03-24T21:35:42.622Z
Learning: In `packages/cli/src/runtime/settingsResolver.ts` (vybestack/llxprt-code PR `#1766`), the `--key` and `--keyfile` branches both call `updateActiveProviderApiKey`, which internally calls `config.setEphemeralSetting('auth-key-name', undefined)` at line 289 of `providerMutations.ts` (update branch) and line 265 (remove branch). Do not flag missing `auth-key-name` clearing at the `settingsResolver.ts` call sites — it is already handled inside the mutation function. Trace the full call chain into `updateActiveProviderApiKey` before raising such a comment.
Applied to files:
packages/core/src/storage/secure-store.spec.tspackages/core/src/storage/secure-store.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/core/src/storage/secure-store.spec.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/storage/secure-store.spec.tspackages/core/src/storage/secure-store.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/core/src/storage/secure-store.spec.ts
📚 Learning: 2026-03-25T18:17:59.248Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1768
File: packages/cli/src/ui/__tests__/AppContainer.render-budget.test.tsx:555-597
Timestamp: 2026-03-25T18:17:59.248Z
Learning: In `packages/cli/src/ui/__tests__/AppContainer.render-budget.test.tsx` (vybestack/llxprt-code PR `#1768`), the `performance.now()` assertions with 1s (mount) and 2s (10 re-renders) time limits are intentionally conservative and stable by design. Do not flag these absolute time thresholds as flaky or suggest replacing them with deterministic render-count signals in decomposition or future PRs — any test strategy redesign is out of scope for the `#1576` refactoring.
Applied to files:
packages/core/src/storage/secure-store.spec.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/storage/secure-store.spec.tspackages/core/src/storage/secure-store.ts
📚 Learning: 2026-03-26T01:28:37.959Z
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:37.959Z
Learning: In `packages/cli/src/auth/provider-usage-info.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the `bucket ?? 'default'` normalization before calling `tokenStore.getToken(provider, bucket)` is correct and functionally identical to passing `undefined`. The production `KeyringTokenStore` (packages/core/src/auth/keyring-token-store.ts, line 94) already normalizes `undefined` to `DEFAULT_BUCKET` (the string `"default"`) internally via `const resolvedBucket = bucket ?? DEFAULT_BUCKET`. The `InMemoryTokenStore` used in test helpers does not define the production contract. Do not flag `bucket ?? 'default'` lookups as a contract violation or suggest splitting into `undefined`-for-lookup vs `'default'`-for-display in this codebase — the original `oauth-manager.ts` used the same pattern (line 2169) and the behavior is faithfully preserved.
Applied to files:
packages/core/src/storage/secure-store.spec.tspackages/core/src/storage/secure-store.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/storage/secure-store.spec.tspackages/core/src/storage/secure-store.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/storage/secure-store.spec.tspackages/core/src/storage/secure-store.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/core/src/storage/secure-store.ts
📚 Learning: 2026-03-27T19:31:04.895Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1797
File: packages/cli/src/config/settings.ts:903-904
Timestamp: 2026-03-27T19:31:04.895Z
Learning: In `packages/cli/src/config/settings.ts` (vybestack/llxprt-code PR `#1797`), calling `migrateDeprecatedSettings(loadedSettings)` inside `loadSettings()` at the end (before returning) is intentional. Persisting one-time deprecated-settings migrations (e.g., `disableAutoUpdate` → `enableAutoUpdate`, `disableLoadingPhrases` → `enableLoadingPhrases`) during startup via `setValue()` is by design in this code path. The default `removeDeprecated = false` means deprecated keys are only supplemented with new equivalents, not deleted. Do not flag this as an unintended side-effect or mutating admin/system settings in future reviews.
Applied to files:
packages/core/src/storage/secure-store.ts
📚 Learning: 2026-03-26T00:30:00.337Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/provider-usage-info.ts:228-233
Timestamp: 2026-03-26T00:30:00.337Z
Learning: In `packages/cli/src/auth/provider-usage-info.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), `getHigherPriorityAuth` calls `getSettingsService().get('authOnly')` globally (without a try/catch guard) alongside the passed `LoadedSettings`. This is pre-existing behavior faithfully extracted from the original `getHigherPriorityAuth` method at line 1836 of `oauth-manager.ts` on main. Do not flag the unconditional `getSettingsService()` call or the `authOnly` handling pattern as a new issue or scope expansion in decomposition or future PRs — rearchitecting the auth-priority dependency chain is explicitly out of scope.
Applied to files:
packages/core/src/storage/secure-store.ts
📚 Learning: 2026-03-27T00:46:47.069Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/configBuilder.ts:104-111
Timestamp: 2026-03-27T00:46:47.069Z
Learning: In `packages/cli/src/config/configBuilder.ts` (vybestack/llxprt-code PR `#1785`, decomposed from `config.ts`), the `onReload` async callback in `buildHooksConfig` calls `loadSettings(cwd)` without a try/catch guard. This is pre-existing behavior faithfully extracted from the original `loadCliConfig` in `config.ts`. Do not flag the missing error handling around `loadSettings` in the `onReload` callback as a decomposition regression in future reviews — adding error handling would be a behavioral change beyond the refactoring scope, and a follow-up improvement has been filed.
Applied to files:
packages/core/src/storage/secure-store.ts
📚 Learning: 2026-03-27T19:31:15.215Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1797
File: packages/cli/src/ui/utils/commandUtils.ts:194-205
Timestamp: 2026-03-27T19:31:15.215Z
Learning: In `packages/cli/src/ui/utils/commandUtils.ts` (vybestack/llxprt-code), the Windows OSC-52 clipboard path calls `fs.writeSync(fd, data)` without a short-write loop. The partial-write concern is acknowledged as theoretically valid but intentionally deferred because OSC-52 payloads are small terminal escape sequences in practice. Do not re-flag the missing loop as a blocking issue in future reviews — it is an accepted known limitation.
Applied to files:
packages/core/src/storage/secure-store.ts
📚 Learning: 2026-03-29T05:46:51.084Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1827
File: packages/core/src/core/compression/CompressionHandler.ts:714-722
Timestamp: 2026-03-29T05:46:51.084Z
Learning: In `packages/core/src/core/compression/CompressionHandler.ts` (vybestack/llxprt-code PR `#1827`), `lastSuccessfulCompressionTime` is intentionally set in two places when fallback compression succeeds via `runCompressionWithRetryAndFallback`: once inside `performFallbackCompression` (needed for the `forceTruncationIfIneffective`-only call path) and once in `performCompression` after `runCompressionWithRetryAndFallback` returns. The duplicate assignment is harmless (both use `Date.now()`) and is deliberately left without extra branching or flags to keep the code simple. Do not flag this dual assignment as a redundancy in future reviews.
Applied to files:
packages/core/src/storage/secure-store.ts
Collaborator
Author
|
Addressed the remaining SecureStore deny-path review concern. Action taken:
Verification run for this update:
Additional repo verification notes:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/src/auth/precedence.ts`:
- Around line 695-696: The sentinel string 'no-profile' used for profileScopeId
can collide with a real profile named the same; replace it with a
guaranteed-unique sentinel constant (e.g. const NO_PROFILE_SENTINEL =
'\0NO_PROFILE' or similar non-printable/prefixed value) and use it wherever
profileScopeId is computed (variable profileScopeId and the other occurrences
noted), i.e. change lines that do const profileScopeId = profileId ??
'no-profile' to const profileScopeId = profileId ?? NO_PROFILE_SENTINEL and
ensure the same NO_PROFILE_SENTINEL is used for all token-scoping/cache-key
generation to avoid aliasing.
🪄 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: 816aa25b-811f-4a98-bfab-4b8c43734c13
📒 Files selected for processing (2)
packages/core/src/auth/precedence.test.tspackages/core/src/auth/precedence.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). (6)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
- GitHub Check: Lint (Javascript)
- GitHub Check: CodeQL
- GitHub Check: Run LLxprt review
🧰 Additional context used
🧠 Learnings (24)
📓 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: 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/__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: 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: 1778
File: packages/cli/src/auth/qwen-oauth-provider.ts:234-242
Timestamp: 2026-03-26T00:30:20.796Z
Learning: In `packages/cli/src/auth/qwen-oauth-provider.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the broad `catch` block in `openQwenBrowserIfInteractive` that silently swallows all errors from the dynamic import of `../runtime/runtimeSettings.js` (setting `noBrowser = false` as the default) is pre-existing behavior faithfully extracted from the original `oauth-manager.ts`. Do not flag the absence of debug logging or error discrimination in this catch block as a gap in decomposition or future PRs — adding error-type discrimination would be a behavioral change beyond the refactoring scope.
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: 1778
File: packages/cli/src/auth/provider-usage-info.ts:36-37
Timestamp: 2026-03-26T01:28:37.959Z
Learning: In `packages/cli/src/auth/provider-usage-info.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), the `bucket ?? 'default'` normalization before calling `tokenStore.getToken(provider, bucket)` is correct and functionally identical to passing `undefined`. The production `KeyringTokenStore` (packages/core/src/auth/keyring-token-store.ts, line 94) already normalizes `undefined` to `DEFAULT_BUCKET` (the string `"default"`) internally via `const resolvedBucket = bucket ?? DEFAULT_BUCKET`. The `InMemoryTokenStore` used in test helpers does not define the production contract. Do not flag `bucket ?? 'default'` lookups as a contract violation or suggest splitting into `undefined`-for-lookup vs `'default'`-for-display in this codebase — the original `oauth-manager.ts` used the same pattern (line 2169) and the behavior is faithfully preserved.
📚 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/auth/precedence.test.tspackages/core/src/auth/precedence.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/auth/precedence.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/core/src/auth/precedence.test.tspackages/core/src/auth/precedence.ts
📚 Learning: 2026-03-26T00:30:00.337Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/provider-usage-info.ts:228-233
Timestamp: 2026-03-26T00:30:00.337Z
Learning: In `packages/cli/src/auth/provider-usage-info.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), `getHigherPriorityAuth` calls `getSettingsService().get('authOnly')` globally (without a try/catch guard) alongside the passed `LoadedSettings`. This is pre-existing behavior faithfully extracted from the original `getHigherPriorityAuth` method at line 1836 of `oauth-manager.ts` on main. Do not flag the unconditional `getSettingsService()` call or the `authOnly` handling pattern as a new issue or scope expansion in decomposition or future PRs — rearchitecting the auth-priority dependency chain is explicitly out of scope.
Applied to files:
packages/core/src/auth/precedence.test.tspackages/core/src/auth/precedence.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/core/src/auth/precedence.test.ts
📚 Learning: 2026-03-26T02:12:35.416Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/auth-status-service.ts:241-263
Timestamp: 2026-03-26T02:12:35.416Z
Learning: In `packages/cli/src/auth/auth-status-service.ts` (vybestack/llxprt-code PR `#1778`, decomposed from `oauth-manager.ts`), `logoutAll()` only iterates `tokenStore.listProviders()` and does not include providers whose auth state is managed exclusively via `provider.isAuthenticated()` (i.e., providers with no persisted token store entry). This is pre-existing behavior faithfully extracted from the original `oauth-manager.ts` `logoutAll()`. Do not flag this as a regression or gap in decomposition PRs — improving coverage to include registry-only providers is a follow-up enhancement, not a refactoring concern.
Applied to files:
packages/core/src/auth/precedence.test.tspackages/core/src/auth/precedence.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/core/src/auth/precedence.test.tspackages/core/src/auth/precedence.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/auth/precedence.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/core/src/auth/precedence.test.tspackages/core/src/auth/precedence.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/core/src/auth/precedence.test.ts
📚 Learning: 2026-03-21T16:36:12.168Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1740
File: packages/core/src/providers/anthropic/AnthropicRequestBuilder.ts:386-408
Timestamp: 2026-03-21T16:36:12.168Z
Learning: In `packages/core/src/providers/anthropic/AnthropicRequestBuilder.ts` (vybestack/llxprt-code PR `#1740`), `buildAnthropicRequestBody` intentionally spreads `options.modelParams` after the validated base fields (`model`, `messages`, `max_tokens`, `stream`). This allows user-specified model parameters to override defaults and is pre-existing behavior faithfully preserved from the original `AnthropicProvider.ts`. Do not flag this spread order as a security or correctness issue in decomposition PRs — restricting which keys can be spread would be a behavioral change beyond refactoring scope.
Applied to files:
packages/core/src/auth/precedence.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/core/src/auth/precedence.test.tspackages/core/src/auth/precedence.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/auth/precedence.test.tspackages/core/src/auth/precedence.ts
📚 Learning: 2026-04-03T05:57:34.290Z
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`).
Applied to files:
packages/core/src/auth/precedence.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/core/src/auth/precedence.ts
📚 Learning: 2026-03-27T01:00:36.000Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/profileResolution.ts:287-305
Timestamp: 2026-03-27T01:00:36.000Z
Learning: In `packages/cli/src/config/profileResolution.ts` (vybestack/llxprt-code PR `#1785`), `loadAndPrepareProfile` contains two sequential branches: one for inline profile (`bootstrapArgs.profileJson != null`) and one for file-based profile (`profileToLoad`). There is intentionally no guard between them preventing both from running, because the caller (`resolveProfileToLoad`) already returns `profileToLoad: undefined` when `bootstrapArgs.profileJson` is non-null, ensuring the file branch is never reached with a real inline profile. Adding `&& bootstrapArgs.profileJson == null` to the file-branch guard would be a redundant behavioral change beyond the refactoring scope. Do not flag this pattern as a missing guard in future reviews.
Applied to files:
packages/core/src/auth/precedence.ts
📚 Learning: 2026-04-02T11:09:51.504Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1870
File: packages/cli/src/runtime/profileApplication.ts:460-462
Timestamp: 2026-04-02T11:09:51.504Z
Learning: In vybestack/llxprt-code, core type Profile requires ephemeralSettings (non-optional). In packages/cli/src/runtime/profileApplication.ts, directly indexing subProfile.ephemeralSettings[...] for load-balancer sub-profile resolution is correct; do not flag missing-defaults for ephemeralSettings on sub-profiles in future reviews.
Applied to files:
packages/core/src/auth/precedence.ts
📚 Learning: 2026-04-02T11:09:42.031Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1870
File: packages/cli/src/gemini.tsx:561-565
Timestamp: 2026-04-02T11:09:42.031Z
Learning: In packages/cli/src/gemini.tsx, the bootstrapProfileName calculation intentionally uses `argv.profileLoad?.trim() ?? <env>` so that a trimmed empty string ('') does not fall back to LLXPRT_BOOTSTRAP_PROFILE; only null/undefined should trigger the env fallback.
Applied to files:
packages/core/src/auth/precedence.ts
📚 Learning: 2026-03-24T21:35:22.678Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1766
File: packages/cli/src/runtime/runtimeRegistry.ts:68-72
Timestamp: 2026-03-24T21:35:22.678Z
Learning: In `packages/cli/src/runtime/runtimeRegistry.ts` (vybestack/llxprt-code PR `#1766`, decomposed from `runtimeSettings.ts`), `resolveActiveRuntimeIdentity` falls back to the first registered `runtimeId` (`runtimeRegistry.keys().next().value`) when the active provider context carries an unregistered per-call `runtimeId` (e.g. a per-call UUID from BaseProvider). This is intentional pre-existing behavior identical to the original `resolveActiveRuntimeIdentity` on main (lines 269-305). The fallback is a deliberate heuristic for the legacy singleton pattern. Do not flag this as a cross-runtime state bug in decomposition or future PRs.
Applied to files:
packages/core/src/auth/precedence.ts
📚 Learning: 2026-03-24T21:34:55.312Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1766
File: packages/cli/src/runtime/providerSwitch.ts:736-745
Timestamp: 2026-03-24T21:34:55.312Z
Learning: In `packages/cli/src/runtime/providerSwitch.ts` (vybestack/llxprt-code PR `#1766`, decomposed from the original `runtimeSettings.ts`), `switchActiveProvider` uses a linear, non-atomic sequence: `clearPreviousProviderSettings` → `activateProviderContext` → `switchSettingsProvider` → base-URL/model/defaults initialization, with no rollback on failure. This is intentional pre-existing behavior faithfully preserved from the original `switchActiveProvider` on main. Do not flag the absence of transactional/rollback semantics as a regression or decomposition bug — it is tracked as a known limitation for a future follow-up.
Applied to files:
packages/core/src/auth/precedence.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/core/src/auth/precedence.ts
📚 Learning: 2026-03-24T21:35:42.622Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1766
File: packages/cli/src/runtime/settingsResolver.ts:96-103
Timestamp: 2026-03-24T21:35:42.622Z
Learning: In `packages/cli/src/runtime/settingsResolver.ts` (vybestack/llxprt-code PR `#1766`), the `--key` and `--keyfile` branches both call `updateActiveProviderApiKey`, which internally calls `config.setEphemeralSetting('auth-key-name', undefined)` at line 289 of `providerMutations.ts` (update branch) and line 265 (remove branch). Do not flag missing `auth-key-name` clearing at the `settingsResolver.ts` call sites — it is already handled inside the mutation function. Trace the full call chain into `updateActiveProviderApiKey` before raising such a comment.
Applied to files:
packages/core/src/auth/precedence.ts
📚 Learning: 2026-01-09T07:46:06.289Z
Learnt from: sogoiii
Repo: vybestack/llxprt-code PR: 1052
File: packages/cli/src/ui/components/ProfileCreateWizard/utils.ts:35-40
Timestamp: 2026-01-09T07:46:06.289Z
Learning: In the Profile Create Wizard (packages/cli/src/ui/components/ProfileCreateWizard), provider values are always passed as `provider.value` from provider objects that originate from PROVIDER_OPTIONS, ensuring consistent provider ID lookups without needing normalization.
Applied to files:
packages/core/src/auth/precedence.ts
🔇 Additional comments (2)
packages/core/src/auth/precedence.test.ts (1)
367-367: Updated metadata assertion is correct.Expecting
profileId: undefinedhere correctly validates the no-profile OAuth metadata behavior.packages/core/src/auth/precedence.ts (1)
159-178: No-profile metadata handling is improved.Returning
nullfrom profile resolution and passingprofileIdasundefinedin request metadata correctly avoids emitting a synthetic default profile identifier.Also applies to: 774-775
Comment on lines
+695
to
696
| const profileScopeId = profileId ?? 'no-profile'; | ||
|
|
Contributor
There was a problem hiding this comment.
Prevent cache-key aliasing with real 'no-profile' profile names.
At Line 695, profileScopeId uses the literal 'no-profile' as a sentinel. If a user creates a profile named 'no-profile', runtime cache keys collide and token scoping/invalidation can cross-contaminate between “no profile loaded” and that real profile.
Suggested fix
- const profileId = resolveProfileId(settingsService);
- const profileScopeId = profileId ?? 'no-profile';
+ const profileId = resolveProfileId(settingsService);
+ const profileScopeId =
+ profileId === null ? '__scope:no-profile__' : `profile:${profileId}`;Also applies to: 746-747, 760-761, 813-814
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/auth/precedence.ts` around lines 695 - 696, The sentinel
string 'no-profile' used for profileScopeId can collide with a real profile
named the same; replace it with a guaranteed-unique sentinel constant (e.g.
const NO_PROFILE_SENTINEL = '\0NO_PROFILE' or similar non-printable/prefixed
value) and use it wherever profileScopeId is computed (variable profileScopeId
and the other occurrences noted), i.e. change lines that do const profileScopeId
= profileId ?? 'no-profile' to const profileScopeId = profileId ??
NO_PROFILE_SENTINEL and ensure the same NO_PROFILE_SENTINEL is used for all
token-scoping/cache-key generation to avoid aliasing.
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.
Fixes #1895
Summary
Verification