persist account footer state and remove redundant info toasts#80
Open
persist account footer state and remove redundant info toasts#80
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds persisted account indicator infrastructure, config/schema fields, runtime config synchronization, indicator lifecycle/formatting helpers, integration into account rotation/request flows, and two new plugin handlers for message and transform operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Runtime
participant PersistStore
participant UI
Client->>Runtime: Request / chat event (may include thread/session)
Runtime->>PersistStore: resolvePersistedAccountSessionID(candidateIDs)
PersistStore-->>Runtime: sessionID (or CODEX_THREAD_ID) + persisted indicators
Runtime->>Runtime: nextPersistedAccountIndicatorRevision -> formatPersistedAccountIndicator
Runtime->>PersistStore: setPersistedAccountIndicator / trimPersistedAccountIndicators
Runtime->>UI: refreshVisiblePersistedAccountIndicators or showTerminalToastResponse (503/429)
UI-->>Client: Render message with persistent footer indicators or show toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 3
🧹 Nitpick comments (1)
index.ts (1)
2857-2859: Avoid storage reads in the hot success path.Line 2857 introduces
loadAccounts()on every successful request just to compute account count. This can add unnecessary I/O and latency under sustained traffic. PreferaccountManager.getAccountCount()here and refresh from disk only on account mutation paths.Proposed fix
if (persistAccountFooter) { - const persistedStorage = await loadAccounts(); - const persistedAccountCount = persistedStorage?.accounts.length ?? - accountManager.getAccountCount(); + const persistedAccountCount = accountManager.getAccountCount(); setPersistedAccountIndicator( threadIdCandidate, account,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 2857 - 2859, The code is calling loadAccounts() in the hot success path to compute persistedAccountCount, causing unnecessary storage I/O; remove the loadAccounts() call and compute persistedAccountCount using accountManager.getAccountCount() instead (i.e., replace the persistedStorage/loadAccounts usage with accountManager.getAccountCount()), and ensure any disk refresh of persistedStorage is performed only on account mutation paths (create/update/delete) where you explicitly call loadAccounts() or otherwise reload from disk; update references to persistedStorage/persistedAccountCount accordingly in the surrounding logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.ts`:
- Around line 1157-1166: The helper resolvePersistedAccountSessionID currently
checks process.env.CODEX_THREAD_ID before the explicit ...candidates, causing
env-set session ID to override per-call candidates; change the iteration order
so explicit candidates are tested first and only fall back to
process.env.CODEX_THREAD_ID if none of the explicit candidates yield a non-empty
sessionID (i.e., iterate over [...candidates, process.env.CODEX_THREAD_ID] or
otherwise place process.env.CODEX_THREAD_ID after candidates), keeping the same
trimming and return-first-non-empty behavior in
resolvePersistedAccountSessionID.
- Around line 1919-1924: The code only reads lastUserMessage.info.sessionID
before calling getPersistedAccountIndicatorLabel, skipping the persisted
indicator when that exact field is missing; change this to use the shared
session-resolution logic used elsewhere (i.e., the same resolver that resolves a
fallback session ID across hooks) to compute the sessionID from lastUserMessage
(falling back to other available sources) and then pass that resolved sessionID
into getPersistedAccountIndicatorLabel so persisted footer decoration is applied
consistently.
In `@test/index.test.ts`:
- Around line 2119-2121: The test's expectedLabelOnlyIndicator is wrong; update
the test variable expectedLabelOnlyIndicator to match getPersistedAccountLabel's
behavior for accountId "acc-1" by changing its value to "Account 1 [id:acc-1]
[1/1]". Locate the test variable expectedLabelOnlyIndicator in
test/index.test.ts and replace the incorrect substring "ount-1" with "acc-1" so
it aligns with the getPersistedAccountLabel function's logic.
---
Nitpick comments:
In `@index.ts`:
- Around line 2857-2859: The code is calling loadAccounts() in the hot success
path to compute persistedAccountCount, causing unnecessary storage I/O; remove
the loadAccounts() call and compute persistedAccountCount using
accountManager.getAccountCount() instead (i.e., replace the
persistedStorage/loadAccounts usage with accountManager.getAccountCount()), and
ensure any disk refresh of persistedStorage is performed only on account
mutation paths (create/update/delete) where you explicitly call loadAccounts()
or otherwise reload from disk; update references to
persistedStorage/persistedAccountCount accordingly in the surrounding logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e65c410-c282-4d39-b9b5-9af190445605
📒 Files selected for processing (6)
index.tslib/config.tslib/schemas.tstest/index.test.tstest/plugin-config.test.tstest/schemas.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="index.ts">
<violation number="1" location="index.ts:2857">
P2: Avoid `loadAccounts()` on every successful response in this hot path; use `accountManager.getAccountCount()` directly to remove unnecessary disk I/O and reduce response-path blocking risk.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="index.ts">
<violation number="1" location="index.ts:2872">
P2: Use the live account count when setting the persisted footer indicator; the cached hint can become stale after runtime account removals and show an incorrect `[current/total]` value.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="index.ts">
<violation number="1" location="index.ts:2178">
P2: Avoid re-reading plugin config from disk on every fetch. A transient `loadPluginConfig()` failure can flip runtime footer flags back to defaults and cause inconsistent toast/footer behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the active account indicator persistent in the chat footer (instead of transient info toasts), while avoiding redundant config reads during account-switch refreshes and adding regression coverage for indicator caching/eviction and terminal toast behavior.
Changes:
- Add
persistAccountFooter+persistAccountFooterStyleconfig options (schema + getters). - Implement per-session persisted account indicators (LRU-capped, revision-protected) and suppress “Using … / Switched …” info toasts when enabled.
- Expand vitest coverage for styles, toast suppression, hot-path storage reads, revision race behavior, and eviction at the 200-session limit.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| index.ts | Adds persisted footer indicator cache + revisioning, hooks to decorate messages, and consolidates terminal toast responses. |
| lib/config.ts | Adds config getters + defaults for persistent footer behavior with env precedence. |
| lib/schemas.ts | Extends config schema with persisted footer fields and style enum validation. |
| test/index.test.ts | Adds extensive tests for footer decoration/toast suppression/race + eviction + terminal toast variants. |
| test/plugin-config.test.ts | Adds tests for new config getters and env precedence/validation behavior. |
| test/schemas.test.ts | Updates schema snapshot and adds rejection test for invalid footer style. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
index.ts
Outdated
Comment on lines
+228
to
+232
| type PersistAccountFooterStyle = | ||
| | "label-masked-email" | ||
| | "full-email" | ||
| | "label-only"; | ||
|
|
Comment on lines
+705
to
+710
| it('should prioritize env var over config', () => { | ||
| process.env.CODEX_AUTH_PERSIST_ACCOUNT_FOOTER = '0'; | ||
| expect(getPersistAccountFooter({ persistAccountFooter: true })).toBe(false); | ||
| process.env.CODEX_AUTH_PERSIST_ACCOUNT_FOOTER = '1'; | ||
| expect(getPersistAccountFooter({ persistAccountFooter: false })).toBe(true); | ||
| }); |
test/index.test.ts
Outdated
Comment on lines
+128
to
+137
| maskEmail: (email: string) => { | ||
| const atIndex = email.indexOf("@"); | ||
| if (atIndex < 0) return "***@***"; | ||
| const local = email.slice(0, atIndex); | ||
| const domain = email.slice(atIndex + 1); | ||
| const parts = domain.split("."); | ||
| const tld = parts.pop() || ""; | ||
| const prefix = local.slice(0, Math.min(2, local.length)); | ||
| return `${prefix}***@***.${tld}`; | ||
| }, |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/index.test.ts">
<violation number="1" location="test/index.test.ts:2278">
P2: This mock sequence can pass without actually testing the `liveAccountCount === 0` fallback branch. Use a persistent `0` mock after priming the first call so the indicator computation is forced through the hint path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/index.test.ts (1)
2009-2035:⚠️ Potential issue | 🟠 MajorReset
index.jsbetween fetch-handler cases.
setupPlugin()keeps re-importing../index.js, but this describe never clears the module cache. With the new footer/session caches in this PR, persisted indicators and the runtime footer snapshot can bleed across tests and make cases like the “first footer session” / overflow-cap assertions order-dependent.💡 Suggested fix
beforeEach(async () => { + vi.resetModules(); vi.clearAllMocks(); const configModule = await import("../lib/config.js");You can verify the leak surface by checking for module-scoped footer caches and the lack of
vi.resetModules()around the repeated../index.jsimports:#!/bin/bash set -euo pipefail printf '\n== Module-scoped footer caches ==\n' rg -n -C2 'persistedAccountIndicators|runtimePluginConfigSnapshot' --type ts printf '\n== index.js imports vs module resets in test/index.test.ts ==\n' rg -n -C1 'vi\.resetModules\(|import\("\.\./index\.js"\)' test/index.test.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` around lines 2009 - 2035, Tests are leaking module-scoped footer/session caches because setupPlugin() repeatedly imports ../index.js without resetting the module cache; add a vi.resetModules() call before each import of "../index.js" (or at the start of the beforeEach in this describe) so module-level state like persistedAccountIndicators and runtimePluginConfigSnapshot in index.js is cleared between cases, ensuring the first-footer/session behavior and overflow-cap assertions don't bleed across tests.
🧹 Nitpick comments (1)
test/index.test.ts (1)
2823-2849: Keepprompt_cache_keyalive in the delayed request.This regression case bypasses
sendPersistedAccountRequest()and falls back to the defaulttransformRequestForCodexstub, which rewritesbodyto{ model: "gpt-5.1" }. That makes the test ambiguous: if session lookup follows the transformed-body path used elsewhere in this suite, the late response is no longer tied to"session-stale", so the stale-write guard is never exercised.💡 Suggested fix
+ const fetchHelpers = await import("../lib/request/fetch-helpers.js"); + vi.mocked(fetchHelpers.transformRequestForCodex).mockImplementation( + async (init, _url, _config, _codexMode, parsedBody) => ({ + updatedInit: init, + body: parsedBody, + }), + ); + const pendingResponse = sdk.fetch!("https://api.openai.com/v1/chat", { method: "POST", body: JSON.stringify({ model: "gpt-5.1", prompt_cache_key: "session-stale" }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` around lines 2823 - 2849, The delayed fetch in the test loses prompt_cache_key because the request falls back to transformRequestForCodex which rewrites the body; update the test so the outgoing request keeps the original body with prompt_cache_key (e.g., ensure sdk.fetch's mocked implementation preserves the Request/RequestInit.body or injects the original JSON including "prompt_cache_key") so the late response remains tied to "session-stale" and the stale-write guard in sendPersistedAccountRequest is exercised; reference the mocked globalThis.fetch, the sdk.fetch call that creates pendingResponse, and the transformRequestForCodex/sendPersistedAccountRequest behavior when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.ts`:
- Around line 1918-1930: The current branch still calls showToast(...) when
refreshVisiblePersistedAccountIndicators(...) returns false even if
runtimePersistAccountFooter is true; change the logic so the info toast is
emitted only when runtimePersistAccountFooter is false and
refreshVisiblePersistedAccountIndicators(...) returned false—i.e., ensure you
check runtimePersistAccountFooter before calling showToast; update the condition
around the refreshVisiblePersistedAccountIndicators(...) call (references:
runtimePersistAccountFooter, refreshVisiblePersistedAccountIndicators,
showToast, runtimePersistAccountFooterStyle, previousManagedAccount, account,
index, storage.accounts.length) so persistent footers never trigger the
transient "Switched to account ..." toast.
---
Outside diff comments:
In `@test/index.test.ts`:
- Around line 2009-2035: Tests are leaking module-scoped footer/session caches
because setupPlugin() repeatedly imports ../index.js without resetting the
module cache; add a vi.resetModules() call before each import of "../index.js"
(or at the start of the beforeEach in this describe) so module-level state like
persistedAccountIndicators and runtimePluginConfigSnapshot in index.js is
cleared between cases, ensuring the first-footer/session behavior and
overflow-cap assertions don't bleed across tests.
---
Nitpick comments:
In `@test/index.test.ts`:
- Around line 2823-2849: The delayed fetch in the test loses prompt_cache_key
because the request falls back to transformRequestForCodex which rewrites the
body; update the test so the outgoing request keeps the original body with
prompt_cache_key (e.g., ensure sdk.fetch's mocked implementation preserves the
Request/RequestInit.body or injects the original JSON including
"prompt_cache_key") so the late response remains tied to "session-stale" and the
stale-write guard in sendPersistedAccountRequest is exercised; reference the
mocked globalThis.fetch, the sdk.fetch call that creates pendingResponse, and
the transformRequestForCodex/sendPersistedAccountRequest behavior when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2c45f70-c032-469a-9eaf-4a451e0d8292
📒 Files selected for processing (6)
index.tslib/config.tslib/persist-account-footer.tslib/schemas.tstest/index.test.tstest/logger.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/schemas.ts
- lib/config.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
index.ts (2)
2330-2336:⚠️ Potential issue | 🟠 MajorAvoid env-first session keying in fetch path to prevent key mismatch/bleed.
At Line 2335,
runtimeThreadId || resolvePersistedAccountSessionID(promptCacheKey)forces env precedence wheneverCODEX_THREAD_IDis set. That can diverge from explicit session IDs used by chat hooks and reintroduce cross-session indicator bleed/misses.Suggested fix
-const runtimeThreadId = process.env.CODEX_THREAD_ID?.toString().trim(); -const threadIdCandidate = - runtimeThreadId || resolvePersistedAccountSessionID(promptCacheKey); +const threadIdCandidate = resolvePersistedAccountSessionID(promptCacheKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 2330 - 2336, The fetch path currently prefers the environment-provided session key by using runtimeThreadId || resolvePersistedAccountSessionID(promptCacheKey), which can cause mismatches with chat hooks; change the selection logic so resolvePersistedAccountSessionID(promptCacheKey) is used when present and only fall back to process.env.CODEX_THREAD_ID (runtimeThreadId) if the persisted session id is falsy. Update the assignment of threadIdCandidate to use the persisted account session id first (resolvePersistedAccountSessionID) and only use runtimeThreadId as a fallback, leaving promptCacheKey and indicatorRevision logic unchanged.
1918-1930:⚠️ Potential issue | 🟡 MinorPersistent-footer mode still leaks a transient “Switched to …” info toast.
At Line 1928, info toast emission still depends on
refreshedVisibleIndicator. This means persistent-footer mode can still show a transient switch toast when refresh returns false, which conflicts with the suppression goal.Suggested fix
-const refreshedVisibleIndicator = runtimePersistAccountFooter - ? refreshVisiblePersistedAccountIndicators( - preReloadTargetAccount ?? account, - index, - storage.accounts.length, - runtimePersistAccountFooterStyle, - ) - : false; -if (!runtimePersistAccountFooter || !refreshedVisibleIndicator) { - await showToast(`Switched to account ${index + 1}`, "info"); -} +if (runtimePersistAccountFooter) { + refreshVisiblePersistedAccountIndicators( + preReloadTargetAccount ?? account, + index, + storage.accounts.length, + runtimePersistAccountFooterStyle, + ); +} else { + await showToast(`Switched to account ${index + 1}`, "info"); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 1918 - 1930, The persistent-footer mode still shows a transient "Switched to ..." toast when refreshVisiblePersistedAccountIndicators returns false; change the logic so toast emission only depends on runtimePersistAccountFooter and not on refreshedVisibleIndicator. In the block that calls refreshVisiblePersistedAccountIndicators (referencing runtimePersistAccountFooter, refreshedVisibleIndicator, refreshVisiblePersistedAccountIndicators, and showToast), remove refreshedVisibleIndicator from the toast condition and only call await showToast(`Switched to account ${index + 1}`, "info") when runtimePersistAccountFooter is false (i.e., wrap the showToast call in if (!runtimePersistAccountFooter) {...}).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@index.ts`:
- Around line 2330-2336: The fetch path currently prefers the
environment-provided session key by using runtimeThreadId ||
resolvePersistedAccountSessionID(promptCacheKey), which can cause mismatches
with chat hooks; change the selection logic so
resolvePersistedAccountSessionID(promptCacheKey) is used when present and only
fall back to process.env.CODEX_THREAD_ID (runtimeThreadId) if the persisted
session id is falsy. Update the assignment of threadIdCandidate to use the
persisted account session id first (resolvePersistedAccountSessionID) and only
use runtimeThreadId as a fallback, leaving promptCacheKey and indicatorRevision
logic unchanged.
- Around line 1918-1930: The persistent-footer mode still shows a transient
"Switched to ..." toast when refreshVisiblePersistedAccountIndicators returns
false; change the logic so toast emission only depends on
runtimePersistAccountFooter and not on refreshedVisibleIndicator. In the block
that calls refreshVisiblePersistedAccountIndicators (referencing
runtimePersistAccountFooter, refreshedVisibleIndicator,
refreshVisiblePersistedAccountIndicators, and showToast), remove
refreshedVisibleIndicator from the toast condition and only call await
showToast(`Switched to account ${index + 1}`, "info") when
runtimePersistAccountFooter is false (i.e., wrap the showToast call in if
(!runtimePersistAccountFooter) {...}).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
index.ts (1)
1949-1951:⚠️ Potential issue | 🟠 MajorPersistent-footer mode still leaks transient “Switched to …” info toasts.
At Line 1949, toast emission still occurs when footer mode is enabled but refresh returns
false. PR objective says these info toasts should be suppressed whenever persistent footer is enabled.Suggested change
-const refreshedVisibleIndicator = runtimePersistAccountFooter - ? refreshVisiblePersistedAccountIndicators( - preReloadTargetAccount ?? account, - index, - storage.accounts.length, - runtimePersistAccountFooterStyle, - ) - : false; -if (!runtimePersistAccountFooter || !refreshedVisibleIndicator) { - await showToast(`Switched to account ${index + 1}`, "info"); -} +if (runtimePersistAccountFooter) { + refreshVisiblePersistedAccountIndicators( + preReloadTargetAccount ?? account, + index, + storage.accounts.length, + runtimePersistAccountFooterStyle, + ); +} else { + await showToast(`Switched to account ${index + 1}`, "info"); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 1949 - 1951, The toast is still emitted when runtimePersistAccountFooter is true because the condition uses ||; change the logic so showToast is only called when persistent-footer is disabled. Locate the block using runtimePersistAccountFooter, refreshedVisibleIndicator and showToast (the if (!runtimePersistAccountFooter || !refreshedVisibleIndicator) { await showToast(...) }) and revise it so that showToast is guarded by a check for runtimePersistAccountFooter being falsy (e.g. only call showToast when !runtimePersistAccountFooter, and keep the refreshedVisibleIndicator check inside that guard as needed) to ensure no info toasts appear when persistent footer mode is enabled.
🧹 Nitpick comments (1)
index.ts (1)
1196-1202: Make cap enforcement invariant-based (while), not single-step.Current logic trims at most one entry per call. A
whilekeeps the map bounded even if future call sites grow it by more than one.Suggested change
const trimPersistedAccountIndicators = (): void => { - if (persistedAccountIndicators.size > MAX_PERSISTED_ACCOUNT_INDICATORS) { - const oldestKey = persistedAccountIndicators.keys().next().value; - if (oldestKey === undefined) return; - persistedAccountIndicators.delete(oldestKey); - } + while (persistedAccountIndicators.size > MAX_PERSISTED_ACCOUNT_INDICATORS) { + const oldestKey = persistedAccountIndicators.keys().next().value; + if (oldestKey === undefined) break; + persistedAccountIndicators.delete(oldestKey); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 1196 - 1202, The trimming function trimPersistedAccountIndicators currently deletes at most one entry per call; change the single-step if to a while loop so it repeatedly removes the oldest key until persistedAccountIndicators.size <= MAX_PERSISTED_ACCOUNT_INDICATORS. Keep the guard that oldestKey may be undefined and return if so, and use persistedAccountIndicators.keys().next().value to identify the oldest entry each iteration; this makes the cap enforcement invariant-based rather than single-step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.ts`:
- Around line 1455-1471: refreshAuthorizeStoragePath currently trusts
loadPluginConfig's return value even when it silently returns DEFAULT_CONFIG on
errors, which can flip perProjectAccounts; modify refreshAuthorizeStoragePath to
detect when loadPluginConfig returned the DEFAULT_CONFIG sentinel (or otherwise
indicates a fallback) and in that case keep using runtimePluginConfigSnapshot
(and emit a warning) instead of using the DEFAULT_CONFIG result. Concretely,
call loadPluginConfig(), assign to a local (e.g., newConfig), and if newConfig
=== DEFAULT_CONFIG && runtimePluginConfigSnapshot is defined then treat that as
a failure and retain runtimePluginConfigSnapshot; continue to use
getPerProjectAccounts(newOrFallbackConfig) and setStoragePath as before, and
reference the symbols refreshAuthorizeStoragePath, loadPluginConfig,
runtimePluginConfigSnapshot, DEFAULT_CONFIG, getPerProjectAccounts,
setStoragePath, and logWarn when making the change.
---
Duplicate comments:
In `@index.ts`:
- Around line 1949-1951: The toast is still emitted when
runtimePersistAccountFooter is true because the condition uses ||; change the
logic so showToast is only called when persistent-footer is disabled. Locate the
block using runtimePersistAccountFooter, refreshedVisibleIndicator and showToast
(the if (!runtimePersistAccountFooter || !refreshedVisibleIndicator) { await
showToast(...) }) and revise it so that showToast is guarded by a check for
runtimePersistAccountFooter being falsy (e.g. only call showToast when
!runtimePersistAccountFooter, and keep the refreshedVisibleIndicator check
inside that guard as needed) to ensure no info toasts appear when persistent
footer mode is enabled.
---
Nitpick comments:
In `@index.ts`:
- Around line 1196-1202: The trimming function trimPersistedAccountIndicators
currently deletes at most one entry per call; change the single-step if to a
while loop so it repeatedly removes the oldest key until
persistedAccountIndicators.size <= MAX_PERSISTED_ACCOUNT_INDICATORS. Keep the
guard that oldestKey may be undefined and return if so, and use
persistedAccountIndicators.keys().next().value to identify the oldest entry each
iteration; this makes the cap enforcement invariant-based rather than
single-step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dad1c363-b256-498e-86f4-32925022aaa7
📒 Files selected for processing (2)
index.tstest/index.test.ts
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="index.ts">
<violation number="1" location="index.ts:1463">
P2: Cold-start authorize still does two synchronous config reads; avoid the second `loadPluginConfig()` call to reduce Windows file-lock exposure and startup auth latency.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="index.ts">
<violation number="1" location="index.ts:1465">
P1: Cold-start authorize can silently use fallback defaults because refresh is skipped whenever `initialConfig` is present.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
index.ts (1)
1190-1203:⚠️ Potential issue | 🟠 MajorPrioritize explicit session IDs before
CODEX_THREAD_IDfallback.Line 1193 currently prefers
process.env.CODEX_THREAD_IDahead of explicit candidates. That can collapse different chats into one persisted indicator key and show the wrong footer across sessions.Suggested fix
const resolvePersistedIndicatorSessionID = ( ...candidates: Array<string | null | undefined> ): string | undefined => { - const runtimeThreadId = process.env.CODEX_THREAD_ID?.toString().trim(); - if (runtimeThreadId) { - return runtimeThreadId; - } - for (const candidate of candidates) { + for (const candidate of [...candidates, process.env.CODEX_THREAD_ID]) { const sessionID = candidate?.toString().trim(); if (sessionID) { return sessionID; } } return undefined; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 1190 - 1203, The function resolvePersistedIndicatorSessionID currently returns process.env.CODEX_THREAD_ID before checking explicit candidate IDs, which can merge distinct sessions; change the logic in resolvePersistedIndicatorSessionID to iterate over the supplied candidates first and return the first non-empty trimmed candidate, and only if no candidate is present then read and return process.env.CODEX_THREAD_ID (trimmed) as a fallback so explicit session IDs take priority over the CODEX_THREAD_ID fallback.
🧹 Nitpick comments (2)
test/index.test.ts (2)
2216-2231: Make loader read-count comparison deterministic across runs.
Line 2216compares baseline vs footer-enabled reads in a single test run. If module-level caches are warmed by the first call, the second call can be biased. Consider isolating each case (separate test or module reset + re-init per case) to avoid cache carryover affecting this assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` around lines 2216 - 2231, The comparison between baseline and footer-enabled storage reads is non-deterministic because module-level caches can be warmed by the first call; change the test so each case runs in isolation by resetting modules and re-importing the plugin before each measurement (e.g., call vi.resetModules() or an equivalent module-reset between calls), then recreate mockClient and re-import OpenAIOAuthPlugin and re-initialize plugin.auth.loader for each scenario; ensure you still clear vi.mocked(storageModule.loadAccounts) before each run and use disablePersistedFooter and enablePersistedFooter only in the isolated runs to get deterministic read counts.
2821-2826: Harden toast-suppression assertions to avoid false negatives.Using
not.toHaveBeenCalledWithon an exact object can still pass if the same info toast is emitted with additional fields. Prefer scanning calls for anyinfoswitch toast message. (Same pattern appears in other suppression checks.)🔧 Suggested assertion pattern
- expect(mockClient.tui.showToast).not.toHaveBeenCalledWith({ - body: { - message: "Switched to account 2", - variant: "info", - }, - }); + expect( + mockClient.tui.showToast.mock.calls.some(([payload]) => { + const body = (payload as { body?: { message?: string; variant?: string } })?.body; + return body?.variant === "info" && body?.message?.startsWith("Switched to account"); + }), + ).toBe(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` around lines 2821 - 2826, The current negative assertion uses not.toHaveBeenCalledWith with an exact object which can miss toasts that include extra fields; update the check for mockClient.tui.showToast to scan its mock calls and assert that no call has an arg with body.message === "Switched to account 2" and body.variant === "info" (e.g., use mockClient.tui.showToast.mock.calls.some(...) and expect(...).toBe(false)); apply the same pattern to other toast-suppression checks to ensure any info "Switched to account" toast — even with extra properties — is detected and fails the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@index.ts`:
- Around line 1190-1203: The function resolvePersistedIndicatorSessionID
currently returns process.env.CODEX_THREAD_ID before checking explicit candidate
IDs, which can merge distinct sessions; change the logic in
resolvePersistedIndicatorSessionID to iterate over the supplied candidates first
and return the first non-empty trimmed candidate, and only if no candidate is
present then read and return process.env.CODEX_THREAD_ID (trimmed) as a fallback
so explicit session IDs take priority over the CODEX_THREAD_ID fallback.
---
Nitpick comments:
In `@test/index.test.ts`:
- Around line 2216-2231: The comparison between baseline and footer-enabled
storage reads is non-deterministic because module-level caches can be warmed by
the first call; change the test so each case runs in isolation by resetting
modules and re-importing the plugin before each measurement (e.g., call
vi.resetModules() or an equivalent module-reset between calls), then recreate
mockClient and re-import OpenAIOAuthPlugin and re-initialize plugin.auth.loader
for each scenario; ensure you still clear vi.mocked(storageModule.loadAccounts)
before each run and use disablePersistedFooter and enablePersistedFooter only in
the isolated runs to get deterministic read counts.
- Around line 2821-2826: The current negative assertion uses
not.toHaveBeenCalledWith with an exact object which can miss toasts that include
extra fields; update the check for mockClient.tui.showToast to scan its mock
calls and assert that no call has an arg with body.message === "Switched to
account 2" and body.variant === "info" (e.g., use
mockClient.tui.showToast.mock.calls.some(...) and expect(...).toBe(false));
apply the same pattern to other toast-suppression checks to ensure any info
"Switched to account" toast — even with extra properties — is detected and fails
the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f25e9f19-90de-4292-af94-7eca76917288
📒 Files selected for processing (3)
index.tslib/config.tstest/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/config.ts
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
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.
What Changed
account.selectrefreshes, so switch events do not re-read config from disk or silently fall back when the config file is locked on Windows.Using ...andSwitched to ...info toasts when the persistent footer is enabled while preserving warning/error terminal toasts.Why
The footer should be the persistent source of truth for the active account instead of a temporary toast. The switch-event refresh path also needs to avoid new sync config reads so Windows file-lock stalls cannot silently downgrade footer updates back into toasts.
Validation
npm run typechecknpm run lintnpm test -- test/index.test.tsnpm run buildSummary by CodeRabbit
New Features
Behavior
Tests
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr ships the persisted account footer feature: after the first successful response, the active account is shown as a persistent footer indicator (masked email, full email, or label-only) in the chat ui instead of a transient toast. it introduces a 200-entry LRU session map, two new plugin hooks (
chat.messageandexperimental.chat.messages.transform), a runtime config snapshot cache (runtimePluginConfigSnapshot) to avoid repeated disk reads, a fallback-marker symbol onDEFAULT_CONFIGso transient config-loader failures can be distinguished from explicit user config, and suppressesUsing…/Switched to…info toasts when the persistent footer is active.key points:
syncRuntimePluginConfigcorrectly guards against stale closure state by preferring the in-memory snapshot over a marked-fallback disk read — this is the right approach for windows antivirus file-lock resiliencesetPersistedAccountIndicatordelete-then-reinsert promotes active sessions;refreshVisiblePersistedAccountIndicatorsintentionally skips re-promotion during bulk switchesaccount.selecthandler passespreReloadTargetAccount ?? accountto avoid label flicker onlabel-onlystyle when the manager hasn't reloaded yet — thoughtful detailsyncRuntimePluginConfig(authorizePluginConfig)beforerefreshAuthorizeStoragePath, then discards the return value. the standard OAuth path correctly passes the refreshed config tosyncRuntimePluginConfigafter the refresh call. on windows, if the config is locked during the initial read andrefreshAuthorizeStoragePathrecovers on retry, the manual path will leaveruntimePluginConfigSnapshotpointing at the fallback config — silently mis-routing storage paths and footer state for any subsequent event handlers or auth flows that callresolveRuntimePluginConfig()before the first fetch corrects it. no vitest regression covers this path.Confidence Score: 2/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant Fetch as innerFetch participant SelectEvent as account.select event participant Indicators as persistedAccountIndicators (LRU Map) participant ChatMsg as chat.message hook participant Transform as experimental.chat.messages.transform hook Fetch->>Fetch: syncRuntimePluginConfig(pluginConfig)<br/>→ request-local persistAccountFooter copy Fetch->>Fetch: indicatorRevision = nextRevision() Fetch->>Fetch: threadIdCandidate = resolvePersistedIndicatorSessionID(promptCacheKey) Fetch-->>Fetch: ...await network... Fetch->>Indicators: setPersistedAccountIndicator(sessionID, account, index, count, style, revision) Note over Indicators: delete + re-insert (LRU promotion)<br/>trimPersistedAccountIndicators() if size > 200 SelectEvent->>SelectEvent: await AccountManager.loadFromDisk() SelectEvent->>Indicators: refreshVisiblePersistedAccountIndicators(account, index, count, style) Note over Indicators: update-only, no LRU re-promotion<br/>revision check skips entries fresher than switch ChatMsg->>Indicators: getPersistedAccountIndicatorLabel(resolveSessionID(input.sessionID)) Indicators-->>ChatMsg: label (or undefined) ChatMsg->>ChatMsg: applyPersistedAccountIndicator(message, label, input.model)<br/>writes message.variant + message.model.variant Transform->>Transform: find last user message in output.messages Transform->>Indicators: getPersistedAccountIndicatorLabel(resolveSessionID(info.sessionID)) Indicators-->>Transform: label (or undefined) Transform->>Transform: applyPersistedAccountIndicator(lastUserMessage.info, label)<br/>no fallbackModel — model.variant written only if providerID/modelID already presentComments Outside Diff (25)
index.ts, line 1933-1993 (link)runtimePersistAccountFooterStylestill unguarded before firstinnerFetchruntimePersistAccountFooterandruntimePersistAccountFooterStyleare initialised tofalse/"label-masked-email"at closure level.syncRuntimePluginConfigis called insideinnerFetch, so the first time theaccount.selectevent fires it uses the initialised defaults rather than the loaded config.in a long-running session where the user enables the footer via config or env var and then manually switches accounts before the first successful fetch completes (e.g. switching accounts during startup),
runtimePersistAccountFooteris stillfalse. the handler will fire the "Switched to account N" toast instead of refreshing the footer indicator — silently defeating the feature for that switch.syncRuntimePluginConfig(pluginConfig)is already called in theloaderfunction; reading the returned values and storing them onruntimePersistAccountFooter/runtimePersistAccountFooterStylethere as well would close the window:no test currently covers
account.selecttriggered before the first successful fetch withpersistAccountFooter: true.Prompt To Fix With AI
index.ts, line 1446-1448 (link)resolveUiRuntimenow silently resetsruntimePersistAccountFooteron Windows config-file lockresolveUiRuntime()was changed from callingapplyUiRuntimeFromConfig(loadPluginConfig())tosyncRuntimePluginConfig(loadPluginConfig()).ui. the new call now also mutates the shared closure varsruntimePersistAccountFooterandruntimePersistAccountFooterStyleas a side effect.on windows, when antivirus holds the config file,
loadPluginConfig()returnsDEFAULT_CONFIG(which haspersistAccountFooter: false). callingsyncRuntimePluginConfig(DEFAULT_CONFIG)then silently resetsruntimePersistAccountFooter = false. the nextaccount.selectevent will fire the "Switched to account N" info toast instead of updating the footer indicator — defeating the feature this PR ships.before this PR,
resolveUiRuntime()only touched UI glyphs/colors. the side-effect on footer state is new and unguarded. consider makingsyncRuntimePluginConfigonly update the runtime flags when the caller explicitly opts in:there is also no vitest test covering the case where
resolveUiRuntime()triggers a stale-config reset ofruntimePersistAccountFooterwhile a user haspersistAccountFooter: true.index.ts, line 3003-3004 (link)authorize()fresh disk read can resetruntimePersistAccountFooteron WindowsloadPluginConfig()is a synchronousreadFileSynccall. its own try/catch silently returnsDEFAULT_CONFIG(which haspersistAccountFooter: false) on any i/o failure — including an antivirus lock on Windows.syncRuntimePluginConfig(authPluginConfig)then writesruntimePersistAccountFooter = falseto the shared closure, which theaccount.selecthandler reads at line 1911. the result: a user withpersistAccountFooter: truewho triggers an OAuth re-auth (e.g. token expiry) will silently lose the footer and start seeing "Switched to account N" toasts for all subsequent account switches — with no log or warning explaining why.the same risk exists in the manual OAuth
authorizeat line 3979.innerFetchalready solved this for the hot path by passing the loader's already-readpluginConfigtosyncRuntimePluginConfig(line 2210) instead of doing a fresh read. the same pattern should be applied here: pass the loader's cachedpluginConfigrather than callingloadPluginConfig()again. no regression test currently covers this failure mode on theauthorize()path.this also applies to:
index.ts:3978-3979(manual OAuth authorize)index.ts, line 1942 (link)chat.messageskipsCODEX_THREAD_IDfallback unlikeexperimental.chat.messages.transformgetPersistedAccountIndicatorLabel(input.sessionID)looks up the indicator directly. when a request has nopromptCacheKey,resolvePersistedAccountSessionID(promptCacheKey)at line 2313 returnsCODEX_THREAD_IDas the map key. if the chat system then callschat.messagewith aninput.sessionIDthat differs from the value ofCODEX_THREAD_ID(e.g., it assigns its own internal id), the lookup returnsundefinedand the indicator is silently dropped.the
experimental.chat.messages.transformhook correctly handles this case at line 1978 by callingresolvePersistedAccountSessionID(lastUserMessage.info.sessionID), which falls back toCODEX_THREAD_ID. there is no vitest coverage for: fetch with nopromptCacheKey+CODEX_THREAD_IDset →chat.messagecalled withsessionID = CODEX_THREAD_ID. the existing "falls back to CODEX_THREAD_ID in the transform hook" test coversexperimental.chat.messages.transformonly.consider wrapping the lookup with
resolvePersistedAccountSessionID:index.ts, line 2318-2319 (link)nextPersistedAccountIndicatorRevision()called unconditionally when footer is disabledindicatorRevisionis computed here on every request regardless of whetherpersistAccountFooteristrue. the revision counter is only consumed inside theif (persistAccountFooter)block further below. when the feature is off, the counter still advances on every request, so once the feature is enabled the first indicator revision starts much higher than the account-switch revision. theexisting.revision > revisionguard insetPersistedAccountIndicatoris intended to let a newer account switch win over a stale in-flight response, but the revision gap created by many disabled-state requests can make the first few switch events write with a lower number than outstanding indicator entries, silently failing to update footers right after the feature is turned on.guard the revision allocation with the flag:
index.ts, line 1917-1925 (link)silent no-feedback window on first switch with footer enabled
when
runtimePersistAccountFooteris true but no successful request has yet run (i.e.persistedAccountIndicatorsis empty),refreshVisiblePersistedAccountIndicatorsreturnsfalseimmediately and does nothing. theelsebranch (the "Switched to account N" toast) is also skipped. the user switches accounts and receives zero visual confirmation.the test
"uses the loader-synced footer setting before the first fetch completes"asserts the toast is absent, but asserts no alternative feedback. this is a regression for users who enable the footer and switch accounts before any request completes — common on startup.consider emitting a low-priority toast or a one-shot "no active sessions yet" indicator in this path, or only suppress the toast when the refresh actually updated at least one session:
index.ts, line 2321-2322 (link)session ID priority reversal breaks
chat.messagelookupthe old code was
process.env.CODEX_THREAD_ID ?? promptCacheKey, givingCODEX_THREAD_IDhigher priority. the newresolvePersistedAccountSessionID(promptCacheKey)triespromptCacheKeyfirst and only falls back toCODEX_THREAD_ID.the lookup paths (
chat.messagehook at line 1950 andexperimental.chat.messages.transformat line 1987) both resolve their session ID withinput.sessionID / lastUserMessage.info.sessionID → CODEX_THREAD_ID. whenpromptCacheKeyis present in the request body but differs from the host-assignedCODEX_THREAD_ID, the indicator is stored underpromptCacheKeybut looked up underCODEX_THREAD_ID— miss, no footer renders.the tests don't catch this because they
delete process.env.CODEX_THREAD_IDinbeforeEachand pass the same value as bothpromptCacheKeyandsessionID.restoring the original priority — env var first, then cache key — aligns storage and lookup:
alternatively, change the
resolvePersistedAccountSessionIDvariadic signature to accept an explicit env-var slot and document the intended priority order so it can't silently regress again.index.ts, line 1944-1963 (link)chat.messagehook missing role guardthe hook calls
applyPersistedAccountIndicatoronoutput.messagefor every message regardless of role. unlike theexperimental.chat.messages.transformhook — which explicitly scans backward for the last user message — this path writesmessage.variantandmessage.model.variantto assistant, tool, and system messages too. any non-user turn that fires this hook would get the account indicator label injected into its metadata, which downstream ui code readingvariantwould then render as the footer for that assistant turn.the fix is a role check before the
applyPersistedAccountIndicatorcall:there are no vitest tests exercising this hook on a non-user message; adding a case where
output.message.role === "assistant"and assertingvariantstays unset would catch any regression here.index.ts, line 1957-1959 (link)non-string role defaults to "user", misapplies indicator
when
message.roleis absent,null, or any non-string value,messageRolesilently defaults to"user"andapplyPersistedAccountIndicatorruns. a system-prompt message or tool-call message that arrives without a stringrolefield would receive the account indicator — writing the raw email into itsvariantandmodel.variantfields whenfull-emailstyle is configured. this is a pii write to a message type that should never carry account metadata.the
experimental.chat.messages.transformhook avoids this with a strict=== "user"check. the same guard should be used here:this brings the two hooks into parity and eliminates the accidental indicator injection on non-user roles.
index.ts, line 1939-2002 (link)stale indicators persist after footer is disabled
the
chat.messageandexperimental.chat.messages.transformhooks callgetPersistedAccountIndicatorLabeland unconditionally apply the result — they never checkruntimePersistAccountFooter. if a user toggles the persist-footer env var off mid-session, any indicator already stored inpersistedAccountIndicatorswill keep being applied to that session's messages indefinitely.concrete scenario:
setPersistedAccountIndicatorstores indicator forsession-XinnerFetch→syncRuntimePluginConfig→persistAccountFooter = false, no new indicator storedchat.messagefires forsession-X→ indicator found in map →applyPersistedAccountIndicatorapplies it anywaythe map is never cleared when footer transitions from enabled to disabled. adding a
runtimePersistAccountFooterguard in the hook dispatch, or clearing the map insidesyncRuntimePluginConfigwhen the flag transitionstrue → false, closes this gap. no vitest coverage exists for the toggle-off path.index.ts, line 287-291 (link)stale
persistedAccountCountHintsurvives footer disablewhen
syncRuntimePluginConfigdetects the footer was just disabled it clearspersistedAccountIndicators, but does not resetpersistedAccountCountHint. if the footer is later re-enabled, the stale hint acts as the denominator fallback wheneverliveAccountCounttransiently drops to 0 (e.g. the manager hasn't loaded yet). the first successful request will correct it, but the window between re-enable and that first request can produce a wrong[N/M]ratio in the indicator.test/index.test.ts, line 963-968 (link)type-unsafe
model?.variantaccess in test helperbuildMessageTransformOutputinfersinfo.modelas{ providerID: string; modelID: string }— novariantfield. the?.variantaccess on that type should be a compile-time error:Property 'variant' does not exist on type '{ providerID: string; modelID: string }'. at runtime the plugin mutates the object and addsvariant, so the test reads correctly, but the type system never sees that mutation and the access silently slips past thenpm run typecheckgate if the inferred type happens to beanyin a looser check. thePluginTypetest definition forexperimental.chat.messages.transformalso omitsvariantfrommodel, so any future refactor that renames the field won't be caught by the type checker.fix: extend the
PluginType.experimental.chat.messages.transforminfo.modeltype to includevariant?: string, and add the cast inreadPersistedAccountIndicator:test/index.test.ts, line 829-836 (link)PluginTypemodelmissingvariantfieldthe
modelfield in theexperimental.chat.messages.transformoutput type is declared as{ providerID: string; modelID: string }, which does not includevariant.applyPersistedAccountIndicatoralways writesmodel.variant, so every test assertion onmodel.variant(e.g. viareadPersistedAccountIndicator) reads a property that the TypeScript type does not know about. addvariant?: stringhere so the type system catches regressions:index.ts, line 3025-3030 (link)authorizepaths now use cached snapshot — staleperProjectAccountsriskresolveRuntimePluginConfig()returnsruntimePluginConfigSnapshot(set once by the loader) until the next loader run. foraccount.selectrefreshes this is the explicit intent, butauthorizeis a write path: if the user editsperProjectAccounts(e.g. flips it fromtruetofalse) after the loader ran, the nextauthorizecall passes the stale value tosetStoragePath, potentially writing new accounts to the wrong location (per-project store vs global store). the same problem exists for themanualOAuthauthorize path at the otherresolveRuntimePluginConfigcall below.authorizeis invoked infrequently and is not on the hot path, so the Windows file-lock protection is less critical here. consider keeping a directloadPluginConfig()call for the non-footer settings that matter (e.g.perProjectAccounts) and only sourcing footer vars from the cached snapshot:there is also no vitest case covering the scenario where
perProjectAccountschanges between the loader run and a subsequentauthorizecall.index.ts, line 1895 (link)account.selecthandler doesn't updatepersistedAccountCountHintrefreshVisiblePersistedAccountIndicatorsusesstorage.accounts.lengthasaccountCountfor the bulk label refresh, butupdatePersistedAccountCountHintis only called further down in the rotation sub-path. if the manual switch fires before the first successful request (sopersistedAccountCountHintis still 0), subsequent fetches that hit theliveAccountCount > 0branch are fine, but a transient zero fromgetAccountCount()immediately after a manual switch would fall back to a stale hint rather than the just-confirmedstorage.accounts.length. callingupdatePersistedAccountCountHint(storage.accounts.length)at the start of theaccount.selectcode path (alongside the existing call in the rotation path) would keep the hint consistent:this is also a windows safety concern: if
getAccountCount()returns 0 during a windows file-lock race, the stale hint is the only fallback.index.ts, line 3045-3051 (link)double disk read on first cold-start authorize
when
runtimePluginConfigSnapshotis stillundefined(no priorloaderorinnerFetchcall),resolveRuntimePluginConfig()falls through toloadPluginConfig()— that's read 1.refreshAuthorizeStoragePath()then unconditionally callsloadPluginConfig()again — read 2. the original code had exactly one read per authorize invocation.on windows, two synchronous
readFileSynccalls against the same config file double the antivirus-lock exposure window. the first read is only needed to populateruntimePluginConfigSnapshot; if it already populated the snapshot, the second read inrefreshAuthorizeStoragePathis redundant for the footer-config part.one fix: have
refreshAuthorizeStoragePathaccept an optional already-loaded config:and inside
refreshAuthorizeStoragePath, use the passed snapshot as the baseline before attempting a fresh read, mirroring the pattern already used in the function body.index.ts, line 1926-1960 (link)account.selectskipsupdatePersistedAccountCountHintthe rotation handler calls
updatePersistedAccountCountHint(storage.accounts.length)after saving, but theaccount.selectpath does not.refreshVisiblePersistedAccountIndicatorscorrectly passesstorage.accounts.lengthfor the immediate bulk refresh, so the visible label is right at switch time.the problem surfaces on the next request when
accountManager.getAccountCount()transiently returns 0 (e.g., during the async manager reload that can happen on windows with a locked file). the fallback insetPersistedAccountIndicatoris:if the switch added or removed an account,
persistedAccountCountHintstill holds the pre-switch value, so the denominator in the indicator ([1/N]) will be wrong for that one request.adding
updatePersistedAccountCountHint(storage.accounts.length)immediately aftersaveAccounts(storage)in theaccount.selectpath would keep the hint in sync, and no new test infrastructure is needed since the count-hint tests already cover the transient-zero scenario.index.ts, line 1421-1422 (link)resolveUiRuntimestill reads config from disk directlyresolveUiRuntime()callsloadPluginConfig()unconditionally — it wasn't migrated to useresolveRuntimePluginConfig(). on windows, every tool invocation (e.g.codex-list) that triggers a ui refresh hits a rawreadFileSync. if antivirus holds the lock at that moment, the call silently falls back toDEFAULT_CONFIGand the ui reverts to defaults.every other hot path in this PR was hardened to use the runtime snapshot, but this one was missed:
should be:
index.ts, line 1221-1240 (link)refreshVisiblePersistedAccountIndicatorsreturnstrueeven when no session was updatedthe function's return value drives toast suppression in the
account.selecthandler: if the map is non-empty the function returnstrueand the "Switched to account N" toast is skipped. but the innerforloop can skip every session via theexisting.revision > revisionguard without writing anything — in that case no footer was refreshed yet the toast is still silenced.in practice the guard can never trigger today (the sync
forloop has noawait, so no concurrent write can increment beyondrevisionbefore the loop finishes). however the semantic contract is wrong: the return should express "at least one session label was written", not "there were sessions to iterate".index.ts, line 1995-1996 (link)session key priority diverges from fetch path in both hooks
the fetch path explicitly prefers
CODEX_THREAD_IDoverprompt_cache_key:when
CODEX_THREAD_IDis set, the indicator is always stored under it regardless of anyprompt_cache_keyin the request body. however, both hooks use the caller-supplied session id as the primary key:chat.message(here):resolvePersistedAccountSessionID(input.sessionID)→ returnsinput.sessionIDimmediately if non-empty, never reachingCODEX_THREAD_IDexperimental.chat.messages.transform(lines 2038-2042): same pattern vialastUserMessage.info.sessionIDconcrete failure: host sets
CODEX_THREAD_ID="thread-123"but passesinput.sessionID="session-456"(non-empty, different value). the fetch stored the indicator under"thread-123". both hooks look up"session-456"first → miss → footer silently not applied.no vitest coverage for this mismatch path. the existing tests only verify that an empty
input.sessionIDfalls back toCODEX_THREAD_ID. to align with the fetch path:the same fix applies to the
experimental.chat.messages.transformhook at lines 2038-2042.index.ts, line 342-348 (link)=== DEFAULT_CONFIGsentinel relies on reference equalityrefreshedPluginConfig === DEFAULT_CONFIGonly fires the fallback warning whenloadPluginConfig()returns the exact same exportedDEFAULT_CONFIGobject reference. ifloadPluginConfig()ever returns a new object shaped like DEFAULT_CONFIG (e.g. after a future refactor that always allocates a fresh default, or if a future try/catch creates{ ...DEFAULT_CONFIG }), the Windows file-lock fallback silently stops working — the locked config is treated as a valid one andstoragePluginConfigis set to a fresh-default-shaped object. the per-project storage path could then be set incorrectly with no warning emitted.a safer sentinel that doesn't depend on object identity:
alternatively,
loadPluginConfigcould attach a__isDefault: truesymbol property when it falls back, making the sentinel explicit and identity-independent. at minimum add a comment noting the reference-equality requirement so future refactors don't silently break Windows file-lock protection.index.ts, line 2389-2395 (link)footer indicator silently missed when thread env var is absent
the fetch path stores the indicator under
resolvePersistedIndicatorSessionID(promptCacheKey). thechat.messageandexperimental.chat.messages.transformhooks look up byresolvePersistedIndicatorSessionID(input.sessionID)andlastUserMessage.info.sessionIDrespectively.when the thread env var is not set both paths fall back to their respective candidate values. if the host passes a
prompt_cache_keyvalue in the request body that differs from thesessionIDit supplies to the chat hooks, the stored indicator is never found and the footer is silently blank — no warning or log explains why.all tests feed the same string as both
prompt_cache_keyandsessionIDso the mismatch path has zero coverage. adding a vitest case where the two keys differ would lock in the observable behavior and make clear whether the thread env var is a hard requirement for this feature.index.ts, line 1988-2019 (link)"chat.message"hook role check may silently never applythe transform hook checks
message?.info.role === "user"(via theinfosub-object), but this hook accessesmessage?.roleat the top level ofoutput.message. if the host passes the full message envelope ({ info: { role, ... }, parts: [] }) rather than the flattened info object, thenmessage.roleis alwaysundefinedand the indicator is never applied through this hook path — silently.all indicator tests exercise
experimental.chat.messages.transformexclusively; there is zero vitest coverage for"chat.message". the missing tests mean neither the role-check level nor the indicator placement (message.variantvsmessage.info.variant) can be verified. adding at least one test that callsplugin["chat.message"]directly and asserts on the output shape would catch this regression.if
output.messageis already the info/content object (flat shape with a top-levelrole), add a comment making that explicit. if it is the full envelope, the role check needs to be:index.ts, line 1292-1305 (link)syncRuntimePluginConfigcan wipe footer state and poison the snapshot on a windows file-lock hitsyncRuntimePluginConfigis the single function that updates bothruntimePluginConfigSnapshotand the footer-reset gate. the function is called in theauthorizeflow via:resolveRuntimePluginConfig()isruntimePluginConfigSnapshot ?? loadPluginConfig(). on a cold-start authorize (no prior snapshot) that hits a windows file lock,loadPluginConfig()returns acreateFallbackPluginConfig()result wherepersistAccountFooter: false.syncRuntimePluginConfigthen:runtimePersistAccountFootercould betruefrom a previous hot-reload path while the fallback config showsfalse, triggeringresetPersistedAccountFooterState()and wiping all in-memory session indicators.runtimePluginConfigSnapshot = fallbackConfig. future calls toresolveRuntimePluginConfig()return this fallback, silently preventing the user'spersistAccountFooter: truefrom being re-read until the nextauth.loaderinvocation.by contrast,
refreshAuthorizeStoragePathalready guards against this exact scenario:applying the same guard in
syncRuntimePluginConfigbefore updatingruntimePluginConfigSnapshotand before callingresetPersistedAccountFooterStatewould close this inconsistency. no regression test exercises a cold-start authorize with a locked config file today.index.ts, line 4057-4063 (link)manual OAuth path discards
refreshAuthorizeStoragePathreturn value, stale snapshot on Windowsthe manual OAuth authorize path calls
syncRuntimePluginConfig(authorizePluginConfig)beforerefreshAuthorizeStoragePath, and then silently discardsrefreshAuthorizeStoragePath's return value. this is the opposite of what the standard OAuth path does (path 1 at ~line 3077, which callssyncRuntimePluginConfig(refreshedAuthorizePluginConfig)after the refresh).concrete failure path on windows:
resolveRuntimePluginConfig()falls through toloadPluginConfig()→ returns a fallback configsyncRuntimePluginConfig(fallbackConfig)caches the fallback inruntimePluginConfigSnapshotrefreshAuthorizeStoragePath(fallbackConfig)retries, succeeds, sets storage path correctly — but its return value (the real config) is thrown awayruntimePluginConfigSnapshotremains the fallback until the nextinnerFetchcorrects itresolveRuntimePluginConfig()in that window reads stale defaults (e.g.perProjectAccounts: truevs the user's configured value — routing auth writes to the wrong storage path)the fix is to mirror path 1: call
syncRuntimePluginConfigwith the return value ofrefreshAuthorizeStoragePath, not the pre-refreshauthorizePluginConfig.no vitest coverage for the manual oauth path verifying that a windows-fallback config is not persisted in
runtimePluginConfigSnapshotwhenrefreshAuthorizeStoragePathrecovers successfully.Prompt To Fix All With AI
Last reviewed commit: 9dd973a
Context used: