feat(chat): bind mobile tasks to Vertex account profiles - #449
Conversation
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 6 issue(s) (1 critical) (3 warning).
frontend/src/pages/ChatView.tsx
The main concern is a critical regression: servers without Vertex configuration have a completely non-functional mobile chat because the send path requires a non-null account selection that can never be populated from an empty catalog. The account binding persistence and SDK env construction look solid otherwise, with a secondary concern around unguarded JSON.parse in the event store.
- 🔴 regressions (L168): When no accounts are configured (no Vertex env vars, no profile file),
loadAccountProfiles()returns an empty catalog,AccountModelPickersets an error and never callsonChangewith a non-null value, soaccountSelectionstays permanently null. The guardif (!activeSessionId && !accountSelection) return falsethen blocks every send — the mobile chat is completely unusable. The same guard at line 139 also blocks the pending-session auto-send path. Before this PR, the chat worked without any account configuration; this is a regression for any server not running Vertex AI.[fixable] - 🟡 regressions (L228): The hardcoded model
<select>dropdown (Opus 4.8, Sonnet 5, etc.) was removed from the header and replaced entirely by AccountModelPicker. When accounts are configured this works, but when they are not (see critical finding above), there is no model selector at all — users lose the ability to switch models even if the send-blocking issue were fixed. The old dropdown should remain as a fallback when no accounts are available.[fixable]
packages/protocol/src/event-store.ts
The main concern is a critical regression: servers without Vertex configuration have a completely non-functional mobile chat because the send path requires a non-null account selection that can never be populated from an empty catalog. The account binding persistence and SDK env construction look solid otherwise, with a secondary concern around unguarded JSON.parse in the event store.
- 🟡 bugs (L794):
JSON.parse(row.account_binding)has no try/catch. If the column contains malformed JSON (manual DB edit, partial write, future schema change), this throws an unhandled exception that propagates up throughgetSession,listSessions,getAttentionSessions, andsearchSessions, crashing any operation that touches the corrupt row. Other parse sites in this file (e.g.extractSnippetat line 745) do have try/catch protection. Wrap in try/catch and fall back to null.[fixable] - 🔵 style (L336): The
account_bindingcolumn migration does not log, unlike every other migration in the file (e.g.agent_nameat line 333 andboot_contextat line 340 both log). Addthis.log.info('migrated sessions table: added account_binding')for consistency and operational visibility.[fixable]
server/ws-handler-v2.ts
The main concern is a critical regression: servers without Vertex configuration have a completely non-functional mobile chat because the send path requires a non-null account selection that can never be populated from an empty catalog. The account binding persistence and SDK env construction look solid otherwise, with a secondary concern around unguarded JSON.parse in the event store.
- 🟡 unsafe_assumptions (L461):
resolveAccountSelectionis called here for validation (return value discarded), then called again inside_startChatInner(chat.ts:780) for actual use. Both calls invokeloadAccountProfiles()which does a synchronousreadFileSyncof the profile file. This is a TOCTOU window (profile file could change between the two reads) and doubles the disk I/O per message send. Consider resolving once and passing the result through, or caching the loaded profiles.[fixable]
server/account-profiles.ts
The main concern is a critical regression: servers without Vertex configuration have a completely non-functional mobile chat because the send path requires a non-null account selection that can never be populated from an empty catalog. The account binding persistence and SDK env construction look solid otherwise, with a secondary concern around unguarded JSON.parse in the event store.
- 🔵 unsafe_assumptions (L107):
loadAccountProfiles()callsreadFileSyncon every invocation with no caching. It is called from/api/accounts(every page load), twice per WS send (ws-handler + chat.ts), and fromsdkEnv. For a small local file this is unlikely to cause issues, but a module-level lazy cache with a short TTL or process-lifetime cache would eliminate redundant reads.[fixable]
| // ── Actions ────────────────────────────────────────────────────────────── | ||
|
|
||
| function handleSend(text: string, images?: ImageAttachment[], ctxBlocks?: string[]): boolean { | ||
| if (!activeSessionId && !accountSelection) return false; |
There was a problem hiding this comment.
🔴 regressions: When no accounts are configured (no Vertex env vars, no profile file), loadAccountProfiles() returns an empty catalog, AccountModelPicker sets an error and never calls onChange with a non-null value, so accountSelection stays permanently null. The guard if (!activeSessionId && !accountSelection) return false then blocks every send — the mobile chat is completely unusable. The same guard at line 139 also blocks the pending-session auto-send path. Before this PR, the chat worked without any account configuration; this is a regression for any server not running Vertex AI. [fixable]
| @@ -217,20 +228,7 @@ export function ChatView() { | |||
| ! | |||
There was a problem hiding this comment.
🟡 regressions: The hardcoded model <select> dropdown (Opus 4.8, Sonnet 5, etc.) was removed from the header and replaced entirely by AccountModelPicker. When accounts are configured this works, but when they are not (see critical finding above), there is no model selector at all — users lose the ability to switch models even if the send-blocking issue were fixed. The old dropdown should remain as a fallback when no accounts are available. [fixable]
| lastStateChange: row.last_state_change ?? null, | ||
| agentName: row.agent_name ?? null, | ||
| bootContext: row.boot_context ?? null, | ||
| accountBinding: row.account_binding ? JSON.parse(row.account_binding) : null, |
There was a problem hiding this comment.
🟡 bugs: JSON.parse(row.account_binding) has no try/catch. If the column contains malformed JSON (manual DB edit, partial write, future schema change), this throws an unhandled exception that propagates up through getSession, listSessions, getAttentionSessions, and searchSessions, crashing any operation that touches the corrupt row. Other parse sites in this file (e.g. extractSnippet at line 745) do have try/catch protection. Wrap in try/catch and fall back to null. [fixable]
| this.log.info('migrated sessions table: added agent_name'); | ||
| } | ||
| if (!columnNames.has('account_binding')) { | ||
| db.exec('ALTER TABLE sessions ADD COLUMN account_binding TEXT'); |
There was a problem hiding this comment.
🔵 style: The account_binding column migration does not log, unlike every other migration in the file (e.g. agent_name at line 333 and boot_context at line 340 both log). Add this.log.info('migrated sessions table: added account_binding') for consistency and operational visibility. [fixable]
| { 'ws.connectionId': connectionId, 'ws.sessionId': msg.sessionId ?? 'new' }, | ||
| (span) => { | ||
| try { | ||
| resolveAccountSelection( |
There was a problem hiding this comment.
🟡 unsafe_assumptions: resolveAccountSelection is called here for validation (return value discarded), then called again inside _startChatInner (chat.ts:780) for actual use. Both calls invoke loadAccountProfiles() which does a synchronous readFileSync of the profile file. This is a TOCTOU window (profile file could change between the two reads) and doubles the disk I/O per message send. Consider resolving once and passing the result through, or caching the loaded profiles. [fixable]
|
|
||
| /** A file contains profiles and credential references, never credential values. */ | ||
| export function loadAccountProfiles(): AccountProfiles { | ||
| if (process.env.MITZO_ACCOUNT_PROFILES_FILE) { |
There was a problem hiding this comment.
🔵 unsafe_assumptions: loadAccountProfiles() calls readFileSync on every invocation with no caching. It is called from /api/accounts (every page load), twice per WS send (ws-handler + chat.ts), and from sdkEnv. For a small local file this is unlikely to cause issues, but a module-level lazy cache with a short TTL or process-lifetime cache would eliminate redundant reads. [fixable]
* fix(ci): propagate test failures and repair stale test fixtures * fix(ci): validate stacked pull requests * test(ci): isolate credential discovery and share store fixtures
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 7 issue(s) (1 critical) (4 warning).
frontend/src/pages/ChatView.tsx
The account binding plumbing is solid, but removing the fallback model selector creates a critical regression: any /api/accounts failure permanently blocks chat with no in-app recovery. The server side has a TOCTOU race from loading the profile file multiple times per request.
- 🔴 regressions (L168): The old hardcoded model
<select>was removed entirely and replaced byAccountModelPicker. If/api/accountsfails or returns an empty catalog,accountSelectionstaysnullpermanently, causinghandleSendto returnfalseon every call. The user cannot send any message and has no in-app recovery path (no retry button, no fallback selector). This is a single-point-of-failure regression: an account API outage now blocks all chat, whereas previously chat always worked with the hardcoded model list.[fixable] - 🟡 bugs (L139): The
pendingSessionauto-send effect now gates onaccountSelection. If the account catalog fails to load, pending sessions (set by 'Start Session' from inbox/todo) silently never fire.clearPendingSession()is never called on the failure path, so the stale prompt sits in the store indefinitely—and could fire unexpectedly if the catalog loads later due to asessionIdchange re-triggering theAccountModelPickereffect.[fixable]
server/chat.ts
The account binding plumbing is solid, but removing the fallback model selector creates a critical regression: any /api/accounts failure permanently blocks chat with no in-app recovery. The server side has a TOCTOU race from loading the profile file multiple times per request.
- 🟡 bugs (L787): TOCTOU race:
resolveAccountSelection()at line 780 callsloadAccountProfiles()(reads the profile file from disk), then line 787 callsloadAccountProfiles()again for.sdkEnv().sdkEnvinternally callsthis.resume(binding)which re-derivesprofileRevisionand compares it to the binding. If the profile file is modified between the two synchronous reads,resume()throws 'Account configuration changed' even though the binding was just resolved. Fix by capturing theAccountProfilesinstance once and reusing it.[fixable]
server/ws-handler-v2.ts
The account binding plumbing is solid, but removing the fallback model selector creates a critical regression: any /api/accounts failure permanently blocks chat with no in-app recovery. The server side has a TOCTOU race from loading the profile file multiple times per request.
- 🟡 unsafe_assumptions (L461):
resolveAccountSelection()is called here as an early validation gate, but its return value is discarded. It is then called again inside_startChatInner. Each call invokesloadAccountProfiles()which does a synchronousreadFileSyncfrom disk. Combined with the third read inchat.ts:787, a single send triggers up to 3 synchronous file reads on the event loop. Consider loading profiles once at the WS handler level and passing the instance through.[fixable]
frontend/src/components/AccountModelPicker.tsx
The account binding plumbing is solid, but removing the fallback model selector creates a critical regression: any /api/accounts failure permanently blocks chat with no in-app recovery. The server side has a TOCTOU race from loading the profile file multiple times per request.
- 🟡 bugs (L56): If an account's
modelsarray is empty,first.models[0].idthrows a TypeError. The same crash exists in the account-switch handler at line 86 (nextAccount.models[0].id). The server's Zod schema enforces.min(1)on models, but the client has no defensive check—a malformed API response or a future schema relaxation would crash the component.[fixable] - 🔵 style (L34):
onChange(null)is called unconditionally at the top of the effect before the async fetch begins. This resets the parent'saccountSelectionto null on every effect run. The chain is currently stable becauseselectAccountisuseCallback-wrapped, but if that wrapper is ever removed,onChangeinstability would cause an infinite render loop. Consider moving the reset inside the fetch's finally/error path, or documenting the stability requirement.[fixable]
packages/protocol/src/event-store.ts
The account binding plumbing is solid, but removing the fallback model selector creates a critical regression: any /api/accounts failure permanently blocks chat with no in-app recovery. The server side has a TOCTOU race from loading the profile file multiple times per request.
- 🔵 style (L336): The
account_bindingmigration is missing thethis.log.info('migrated sessions table: added account_binding')log line that every other column migration in this method includes. Inconsistent with the established pattern and makes migration issues harder to debug.[fixable]
| // ── Actions ────────────────────────────────────────────────────────────── | ||
|
|
||
| function handleSend(text: string, images?: ImageAttachment[], ctxBlocks?: string[]): boolean { | ||
| if (!activeSessionId && !accountSelection) return false; |
There was a problem hiding this comment.
🔴 regressions: The old hardcoded model <select> was removed entirely and replaced by AccountModelPicker. If /api/accounts fails or returns an empty catalog, accountSelection stays null permanently, causing handleSend to return false on every call. The user cannot send any message and has no in-app recovery path (no retry button, no fallback selector). This is a single-point-of-failure regression: an account API outage now blocks all chat, whereas previously chat always worked with the hardcoded model list. [fixable]
| const pendingConsumed = useRef<string | null>(null); | ||
| useEffect(() => { | ||
| if (!pendingSession) return; | ||
| if (!pendingSession || !accountSelection) return; |
There was a problem hiding this comment.
🟡 bugs: The pendingSession auto-send effect now gates on accountSelection. If the account catalog fails to load, pending sessions (set by 'Start Session' from inbox/todo) silently never fire. clearPendingSession() is never called on the failure path, so the stale prompt sits in the store indefinitely—and could fire unexpectedly if the catalog loads later due to a sessionId change re-triggering the AccountModelPicker effect. [fixable]
| ); | ||
| if (accountBinding) { | ||
| options = { ...options, model: accountBinding.model }; | ||
| accountEnv = loadAccountProfiles().sdkEnv(accountBinding, sdkEnv()); |
There was a problem hiding this comment.
🟡 bugs: TOCTOU race: resolveAccountSelection() at line 780 calls loadAccountProfiles() (reads the profile file from disk), then line 787 calls loadAccountProfiles() again for .sdkEnv(). sdkEnv internally calls this.resume(binding) which re-derives profileRevision and compares it to the binding. If the profile file is modified between the two synchronous reads, resume() throws 'Account configuration changed' even though the binding was just resolved. Fix by capturing the AccountProfiles instance once and reusing it. [fixable]
| { 'ws.connectionId': connectionId, 'ws.sessionId': msg.sessionId ?? 'new' }, | ||
| (span) => { | ||
| try { | ||
| resolveAccountSelection( |
There was a problem hiding this comment.
🟡 unsafe_assumptions: resolveAccountSelection() is called here as an early validation gate, but its return value is discarded. It is then called again inside _startChatInner. Each call invokes loadAccountProfiles() which does a synchronous readFileSync from disk. Combined with the third read in chat.ts:787, a single send triggers up to 3 synchronous file reads on the event loop. Consider loading profiles once at the WS handler level and passing the instance through. [fixable]
| accountId: first.id, | ||
| model: first.models.some((m) => m.id === preferredModel) | ||
| ? preferredModel | ||
| : first.models[0].id, |
There was a problem hiding this comment.
🟡 bugs: If an account's models array is empty, first.models[0].id throws a TypeError. The same crash exists in the account-switch handler at line 86 (nextAccount.models[0].id). The server's Zod schema enforces .min(1) on models, but the client has no defensive check—a malformed API response or a future schema relaxation would crash the component. [fixable]
| setError(''); | ||
| setBindingLabel(''); | ||
| setSelection(null); | ||
| onChange(null); |
There was a problem hiding this comment.
🔵 style: onChange(null) is called unconditionally at the top of the effect before the async fetch begins. This resets the parent's accountSelection to null on every effect run. The chain is currently stable because selectAccount is useCallback-wrapped, but if that wrapper is ever removed, onChange instability would cause an infinite render loop. Consider moving the reset inside the fetch's finally/error path, or documenting the stability requirement. [fixable]
| this.log.info('migrated sessions table: added agent_name'); | ||
| } | ||
| if (!columnNames.has('account_binding')) { | ||
| db.exec('ALTER TABLE sessions ADD COLUMN account_binding TEXT'); |
There was a problem hiding this comment.
🔵 style: The account_binding migration is missing the this.log.info('migrated sessions table: added account_binding') log line that every other column migration in this method includes. Inconsistent with the established pattern and makes migration issues harder to debug. [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 7 issue(s) (3 warning).
server/ws-handler-v2.ts
Solid account profiles implementation with good test coverage and security boundaries. Main concerns: redundant double-validation with disk I/O on every WS send, non-atomic binding persistence in onSessionResolved, and the mobile model selector removal leaves no override path for follow-up messages.
- 🟡 bugs (L465): Double validation:
resolveAccountSelection()is called here (return value discarded) and again inside_startChatInner(chat.ts:793). Both paths also calleventStore.getSession()independently. The ws-handler call is a fail-fast guard, but it introduces a TOCTOU window: between the two reads of the stored binding, a concurrent request could persist a binding, causing divergent validation results. More practically,loadAccountProfiles()does a synchronousreadFileSyncon every invocation — for bound sessions this means disk I/O on every WS send message, twice. Consider passing the resolved binding from ws-handler into startChat instead of discarding it.[fixable]
frontend/src/pages/ChatView.tsx
Solid account profiles implementation with good test coverage and security boundaries. Main concerns: redundant double-validation with disk I/O on every WS send, non-atomic binding persistence in onSessionResolved, and the mobile model selector removal leaves no override path for follow-up messages.
- 🟡 regressions (L186): For follow-up messages on existing sessions (
activeSessionIdis truthy), the spread...(!activeSessionId && accountSelection ? accountSelection : {})produces...{}, so neithermodelnoraccountIdis sent in the WS payload. Previously the code explicitly sentmodel: modelState. The store'ssendMessagefalls back toconfig.modelId, which is kept in sync viasetModel— so this is functionally equivalent today. However, after a session switch (user navigates to session A, then back to session B),config.modelIdmay reflect session A's model. The old code used local statemodelStatewhich had the same issue, so this is not a new regression, but the removal of the explicit model selector from the mobile header (the hardcoded<select>dropdown was deleted) means the user now has no way to override the model for follow-up messages on an active session.[fixable]
server/chat.ts
Solid account profiles implementation with good test coverage and security boundaries. Main concerns: redundant double-validation with disk I/O on every WS send, non-atomic binding persistence in onSessionResolved, and the mobile model selector removal leaves no override path for follow-up messages.
- 🟡 unsafe_assumptions (L1088): Account binding is persisted in
onSessionResolved(after the SDK assigns a session ID), and boot context is persisted in a separateupsertSessioncall at line 1092. These two writes are not atomic: a crash between them leaves the session with a binding but no boot context, or vice versa. More importantly, if the server crashes beforeonSessionResolvedfires, the binding is lost entirely — on resume the session would be treated as legacy (unbound), potentially routing to different billing credentials.[fixable]
server/account-profiles.ts
Solid account profiles implementation with good test coverage and security boundaries. Main concerns: redundant double-validation with disk I/O on every WS send, non-atomic binding persistence in onSessionResolved, and the mobile model selector removal leaves no override path for follow-up messages.
- 🔵 bugs (L152): When a client resumes a bound session and sends
{ model: 'new-model' }withoutaccountId, the guard at line 153 is skipped (it only checks whenselection.accountIdis truthy). The function returns the stored binding with the original model, and chat.ts:795 silently overwrites the client's requested model. The client gets no error or warning that its model change was ignored. Consider either rejecting the model change explicitly or documenting this as intentional.[fixable] - 🔵 style (L88): The env cleanup regex strips
ANTHROPIC_*,OPENAI_*,CLAUDE_CODE_USE_*etc., and the explicit list stripsGOOGLE_API_KEY, butGOOGLE_APPLICATION_CREDENTIALSis not explicitly stripped — it's only overwritten by the spread at line 100. This is functionally correct today but inconsistent with the cleanup pattern for other credential variables. If theGOOGLE_APPLICATION_CREDENTIALSassignment were ever made conditional, the old value would leak through.[fixable]
frontend/src/components/AccountModelPicker.tsx
Solid account profiles implementation with good test coverage and security boundaries. Main concerns: redundant double-validation with disk I/O on every WS send, non-atomic binding persistence in onSessionResolved, and the mobile model selector removal leaves no override path for follow-up messages.
- 🔵 unsafe_assumptions (L123): Non-null assertion
accounts.find(...)!is safe under current code paths (accounts and selection are always set together), but provides no runtime protection. If a future change clearsaccountsindependently ofselection, this would throw a TypeError when accessingaccount.models. A defensive null check with fallback would be safer.[fixable]
packages/protocol/src/event-store.ts
Solid account profiles implementation with good test coverage and security boundaries. Main concerns: redundant double-validation with disk I/O on every WS send, non-atomic binding persistence in onSessionResolved, and the mobile model selector removal leaves no override path for follow-up messages.
- 🔵 style (L813): The
parseAccountBindingsentinel ({ accountId: 'unavailable', provider: 'unavailable', ... }) correctly prevents silent downgrade to the legacy route, but produces misleading error messages downstream. When this sentinel flows intoresolveAccountSelection, the user sees 'This task is bound to its original account' or 'Account is unavailable' — neither mentions the root cause is corrupt binding data. Consider a distinct error path for corrupt bindings.[fixable]
| ? ctx.eventStore.getSession(msg.sessionId)?.accountBinding | ||
| : null; | ||
| const accountProfiles = msg.accountId || storedBinding ? loadAccountProfiles() : undefined; | ||
| resolveAccountSelection(msg, storedBinding, !!msg.sessionId, accountProfiles); |
There was a problem hiding this comment.
🟡 bugs: Double validation: resolveAccountSelection() is called here (return value discarded) and again inside _startChatInner (chat.ts:793). Both paths also call eventStore.getSession() independently. The ws-handler call is a fail-fast guard, but it introduces a TOCTOU window: between the two reads of the stored binding, a concurrent request could persist a binding, causing divergent validation results. More practically, loadAccountProfiles() does a synchronous readFileSync on every invocation — for bound sessions this means disk I/O on every WS send message, twice. Consider passing the resolved binding from ws-handler into startChat instead of discarding it. [fixable]
| @@ -167,7 +186,7 @@ export function ChatView() { | |||
| storeSendMessage(text, { | |||
There was a problem hiding this comment.
🟡 regressions: For follow-up messages on existing sessions (activeSessionId is truthy), the spread ...(!activeSessionId && accountSelection ? accountSelection : {}) produces ...{}, so neither model nor accountId is sent in the WS payload. Previously the code explicitly sent model: modelState. The store's sendMessage falls back to config.modelId, which is kept in sync via setModel — so this is functionally equivalent today. However, after a session switch (user navigates to session A, then back to session B), config.modelId may reflect session A's model. The old code used local state modelState which had the same issue, so this is not a new regression, but the removal of the explicit model selector from the mobile header (the hardcoded <select> dropdown was deleted) means the user now has no way to override the model for follow-up messages on an active session. [fixable]
| options.resume ? undefined : fullPrompt, | ||
| { | ||
| connRegistry: _connRegistry ?? undefined, | ||
| onSessionResolved: (sessionId: string) => { |
There was a problem hiding this comment.
🟡 unsafe_assumptions: Account binding is persisted in onSessionResolved (after the SDK assigns a session ID), and boot context is persisted in a separate upsertSession call at line 1092. These two writes are not atomic: a crash between them leaves the session with a binding but no boot context, or vice versa. More importantly, if the server crashes before onSessionResolved fires, the binding is lost entirely — on resume the session would be treated as legacy (unbound), potentially routing to different billing credentials. [fixable]
| resuming = false, | ||
| profiles?: AccountProfiles, | ||
| ): AccountBinding | undefined { | ||
| if (stored) { |
There was a problem hiding this comment.
🔵 bugs: When a client resumes a bound session and sends { model: 'new-model' } without accountId, the guard at line 153 is skipped (it only checks when selection.accountId is truthy). The function returns the stored binding with the original model, and chat.ts:795 silently overwrites the client's requested model. The client gets no error or warning that its model change was ignored. Consider either rejecting the model change explicitly or documenting this as intentional. [fixable]
| this.resume(binding); | ||
| const profile = this.profiles.find((p) => p.id === binding.accountId)!; | ||
| const env = { ...base }; | ||
| for (const key of Object.keys(env)) { |
There was a problem hiding this comment.
🔵 style: The env cleanup regex strips ANTHROPIC_*, OPENAI_*, CLAUDE_CODE_USE_* etc., and the explicit list strips GOOGLE_API_KEY, but GOOGLE_APPLICATION_CREDENTIALS is not explicitly stripped — it's only overwritten by the spread at line 100. This is functionally correct today but inconsistent with the cleanup pattern for other credential variables. If the GOOGLE_APPLICATION_CREDENTIALS assignment were ever made conditional, the old value would leak through. [fixable]
| </> | ||
| ); | ||
| if (!selection) return <span>Loading accounts…</span>; | ||
| const account = accounts.find((a) => a.id === (selection.accountId ?? ''))!; |
There was a problem hiding this comment.
🔵 unsafe_assumptions: Non-null assertion accounts.find(...)! is safe under current code paths (accounts and selection are always set together), but provides no runtime protection. If a future change clears accounts independently of selection, this would throw a TypeError when accessing account.models. A defensive null check with fallback would be safer. [fixable]
| ) | ||
| ) | ||
| return binding; | ||
| } catch { |
There was a problem hiding this comment.
🔵 style: The parseAccountBinding sentinel ({ accountId: 'unavailable', provider: 'unavailable', ... }) correctly prevents silent downgrade to the legacy route, but produces misleading error messages downstream. When this sentinel flows into resolveAccountSelection, the user sees 'This task is bound to its original account' or 'Account is unavailable' — neither mentions the root cause is corrupt binding data. Consider a distinct error path for corrupt bindings. [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 6 issue(s) (3 warning).
frontend/src/components/AccountModelPicker.tsx
Well-structured feature with good fail-closed design and test coverage; the main concerns are a sync file read in the WS hot path, a silent send rejection before accounts load, and a minor redundant Zod re-parse.
- 🟡 bugs (L65): After
safeParsevalidates data (checked on the!parsed.successguard),parseis called a second time on the samedata. This double-parse is redundant. Useparsed.datainstead of re-callingmodelsSchema.parse(data)/catalogSchema.parse(data)to avoid unnecessary re-validation and a second allocation.[fixable]
frontend/src/pages/ChatView.tsx
Well-structured feature with good fail-closed design and test coverage; the main concerns are a sync file read in the WS hot path, a silent send rejection before accounts load, and a minor redundant Zod re-parse.
- 🟡 regressions (L175):
handleSendreturnsfalsewith no user-visible feedback whenaccountSelectionis null (i.e. accounts are still loading or the picker errored and the user hasn't retried). The AccountModelPicker shows 'Loading accounts…' but ChatInput's send button appears active and silently rejects taps. Consider disabling the input or showing a toast so the user knows why the send was swallowed.[fixable]
server/ws-handler-v2.ts
Well-structured feature with good fail-closed design and test coverage; the main concerns are a sync file read in the WS hot path, a silent send rejection before accounts load, and a minor redundant Zod re-parse.
- 🟡 unsafe_assumptions (L462): For every follow-up message to a bound session,
loadAccountProfiles()callsreadFileSync(MITZO_ACCOUNT_PROFILES_FILE)andJSON.parsesynchronously in the hot path of the WS handler. The profiles are re-read and the binding is re-validated on every single message even though the session's binding is immutable after creation. Consider caching theAccountProfilesinstance per request or globally with file-change invalidation.[fixable] - 🔵 style (L463):
resolveAccountSelectionis called here for early validation (result discarded), then called again inside_startChatInnerwhere the result is captured. Both calls share the sameaccountProfilesinstance so correctness is fine, but the double validation reads the event store twice for the same session and runsprofiles.resume()twice for resume cases. Consider lettingstartChatown validation entirely, or capturing the ws-handler result and passing it through.[fixable]
server/account-profiles.ts
Well-structured feature with good fail-closed design and test coverage; the main concerns are a sync file read in the WS hot path, a silent send rejection before accounts load, and a minor redundant Zod re-parse.
- 🔵 unsafe_assumptions (L119):
loadAccountProfiles()creates avertex-defaultprofile whenANTHROPIC_VERTEX_PROJECT_IDis set, even withoutCLAUDE_CODE_USE_VERTEX=1. Operators who set that env var for unrelated tools would silently get a Vertex billing profile in the account catalog. The only way to suppress it isCLAUDE_CODE_USE_VERTEX=0. The design doc acknowledges this, but it's worth a comment or log line since it could lead to unexpected billing.[fixable]
frontend/src/pages/__tests__/ChatViewAccounts.test.tsx
Well-structured feature with good fail-closed design and test coverage; the main concerns are a sync file read in the WS hot path, a silent send rejection before accounts load, and a minor redundant Zod re-parse.
- 🔵 missing_tests: The test covers the paused-launch recovery flow well, but there's no test for the case where
pendingSessionarrives after the AccountModelPicker has already entered its error state. In that scenario,accountUnavailablewould fire with the latestpendingSession, but only if the component re-renders and the picker re-callsonUnavailable— which it won't because it's already showing the error. The pending session could be silently stuck.
| : 'Existing task · legacy account', | ||
| ); | ||
| } else { | ||
| const parsed = legacy ? modelsSchema.safeParse(data) : catalogSchema.safeParse(data); |
There was a problem hiding this comment.
🟡 bugs: After safeParse validates data (checked on the !parsed.success guard), parse is called a second time on the same data. This double-parse is redundant. Use parsed.data instead of re-calling modelsSchema.parse(data) / catalogSchema.parse(data) to avoid unnecessary re-validation and a second allocation. [fixable]
| @@ -155,6 +173,7 @@ export function ChatView() { | |||
| // ── Actions ────────────────────────────────────────────────────────────── | |||
|
|
|||
| function handleSend(text: string, images?: ImageAttachment[], ctxBlocks?: string[]): boolean { | |||
There was a problem hiding this comment.
🟡 regressions: handleSend returns false with no user-visible feedback when accountSelection is null (i.e. accounts are still loading or the picker errored and the user hasn't retried). The AccountModelPicker shows 'Loading accounts…' but ChatInput's send button appears active and silently rejects taps. Consider disabling the input or showing a toast so the user knows why the send was swallowed. [fixable]
| (span) => { | ||
| try { | ||
| const storedBinding = msg.sessionId | ||
| ? ctx.eventStore.getSession(msg.sessionId)?.accountBinding |
There was a problem hiding this comment.
🟡 unsafe_assumptions: For every follow-up message to a bound session, loadAccountProfiles() calls readFileSync(MITZO_ACCOUNT_PROFILES_FILE) and JSON.parse synchronously in the hot path of the WS handler. The profiles are re-read and the binding is re-validated on every single message even though the session's binding is immutable after creation. Consider caching the AccountProfiles instance per request or globally with file-change invalidation. [fixable]
| try { | ||
| const storedBinding = msg.sessionId | ||
| ? ctx.eventStore.getSession(msg.sessionId)?.accountBinding | ||
| : null; |
There was a problem hiding this comment.
🔵 style: resolveAccountSelection is called here for early validation (result discarded), then called again inside _startChatInner where the result is captured. Both calls share the same accountProfiles instance so correctness is fine, but the double validation reads the event store twice for the same session and runs profiles.resume() twice for resume cases. Consider letting startChat own validation entirely, or capturing the ws-handler result and passing it through. [fixable]
| ); | ||
| } catch { | ||
| throw new Error( | ||
| 'Cannot load account profiles. Check MITZO_ACCOUNT_PROFILES_FILE on the Mac.', |
There was a problem hiding this comment.
🔵 unsafe_assumptions: loadAccountProfiles() creates a vertex-default profile when ANTHROPIC_VERTEX_PROJECT_ID is set, even without CLAUDE_CODE_USE_VERTEX=1. Operators who set that env var for unrelated tools would silently get a Vertex billing profile in the account catalog. The only way to suppress it is CLAUDE_CODE_USE_VERTEX=0. The design doc acknowledges this, but it's worth a comment or log line since it could lead to unexpected billing. [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 7 issue(s) (3 warning).
server/chat.ts
Well-structured feature with solid test coverage and correct fail-closed semantics. The main concern is orphaned session metadata in the event store when SDK startup fails after pre-persisting the account binding — needs cleanup in the catch block.
- 🟡 bugs (L1031): When
query()throws before emitting any events, the pre-persisted session (newSdkSessionIdwithaccountBinding) is never cleaned up from the event store. Thecatchblock at line 1100 cleans up worktrees and aborts the registry but does not remove the orphaned session metadata. This leaves a dangling session in the store that appears in session listings, has no events, and never transitions to ENDED. Consider addingeventStore.deleteSession(newSdkSessionId)or marking it ENDED in the catch block.[fixable]
server/ws-handler-v2.ts
Well-structured feature with solid test coverage and correct fail-closed semantics. The main concern is orphaned session metadata in the event store when SDK startup fails after pre-persisting the account binding — needs cleanup in the catch block.
- 🟡 regressions (L465): The return value of
resolveAccountSelection()at line 465 is discarded — only its throw behavior is used for early validation. However, whenstartChatis called later (lines 607-621 and 630-644), it callsresolveAccountSelectionagain inside_startChatInner(chat.ts line 793). This meansloadAccountProfiles()reads the profile file twice ifoptions.accountProfilesis not passed — but it is passed, so the same instance is reused. The double-validation is intentional (fail-fast guard), but the discarded return value at line 465 is confusing. Consider assigning it to a variable or adding a comment clarifying this is a validation-only call.[fixable]
frontend/src/pages/ChatView.tsx
Well-structured feature with solid test coverage and correct fail-closed semantics. The main concern is orphaned session metadata in the event store when SDK startup fails after pre-persisting the account binding — needs cleanup in the catch block.
- 🟡 regressions (L140): The
accountUnavailablecallback capturespendingSessionin its closure viauseCallback(..., [pendingSession, clearPendingSession]). However, whenonUnavailablefires fromAccountModelPicker, it's called during render or in a useEffect triggered by error/empty state. IfpendingSessionarrives after the error has already been set (the 'after' timing in the test),accountUnavailablewas created withpendingSession: null. The test works because the component re-renders whenpendingSessionchanges, creating a newaccountUnavailableclosure, and theuseEffectat line 41-43 re-fires whenerroris already set. But there's a subtle timing dependency: theonUnavailableeffect depends on botherrorandonUnavailable(which changes whenpendingSessionchanges). IfonUnavailablechanges buterrorstays the same, the effect re-runs and correctly calls the new closure. This works but is fragile. - 🔵 bugs (L150): The
pendingConsumedref guards duplicate sends by keying onpendingSession.prompt. If two successive pending sessions arrive with identical prompt text (e.g., two task launches with the same template), the second will be silently dropped. Consider using a unique key like a combination of prompt + telosTaskId or a counter instead.[fixable]
server/account-profiles.ts
Well-structured feature with solid test coverage and correct fail-closed semantics. The main concern is orphaned session metadata in the event store when SDK startup fails after pre-persisting the account binding — needs cleanup in the catch block.
- 🔵 style (L90): The regex
/^(ANTHROPIC_|OPENAI_|CLAUDE_CODE_USE_|CLAUDE_CODE_SKIP_|VERTEX_REGION_)/strips env vars that are then re-set on lines 101-106. While correct (delete-then-reassign), the intent is not obvious to a reader. TheCLAUDE_CODE_SKIP_*prefix is forward-looking — no such vars exist in the codebase today. Consider documenting the scrubbing intent or using an explicit allowlist of keys to preserve instead of a deletion regex.[fixable] - 🔵 style (L117): The
catchblock inloadAccountProfiles()at line 117 catches all errors (including JSON parse errors, file-not-found, and Zod validation failures) and replaces them with a generic message. For operator debugging, preserving the original error message or logging it server-side would help diagnose misconfigured profile files.[fixable]
server/__tests__/chat-account-startup.test.ts
Well-structured feature with solid test coverage and correct fail-closed semantics. The main concern is orphaned session metadata in the event store when SDK startup fails after pre-persisting the account binding — needs cleanup in the catch block.
- 🔵 missing_tests: The test verifies pre-persistence of the binding before
query()starts, and covers the failure case wherequery()throws. However, there is no test verifying that the orphaned session is cleaned up afterquery()throws (because it isn't cleaned up — see the orphan session finding). Adding a test that asserts the session is removed or marked ENDED after a startup failure would codify the expected cleanup behavior.[fixable]
| // Bound sessions have durable routing before the SDK can create history or side effects. | ||
| const newSdkSessionId = accountBinding && !resolvedResume ? randomUUID() : undefined; | ||
| try { | ||
| if (newSdkSessionId) { |
There was a problem hiding this comment.
🟡 bugs: When query() throws before emitting any events, the pre-persisted session (newSdkSessionId with accountBinding) is never cleaned up from the event store. The catch block at line 1100 cleans up worktrees and aborts the registry but does not remove the orphaned session metadata. This leaves a dangling session in the store that appears in session listings, has no events, and never transitions to ENDED. Consider adding eventStore.deleteSession(newSdkSessionId) or marking it ENDED in the catch block. [fixable]
| ? ctx.eventStore.getSession(msg.sessionId)?.accountBinding | ||
| : null; | ||
| const accountProfiles = msg.accountId || storedBinding ? loadAccountProfiles() : undefined; | ||
| resolveAccountSelection(msg, storedBinding, !!msg.sessionId, accountProfiles); |
There was a problem hiding this comment.
🟡 regressions: The return value of resolveAccountSelection() at line 465 is discarded — only its throw behavior is used for early validation. However, when startChat is called later (lines 607-621 and 630-644), it calls resolveAccountSelection again inside _startChatInner (chat.ts line 793). This means loadAccountProfiles() reads the profile file twice if options.accountProfiles is not passed — but it is passed, so the same instance is reused. The double-validation is intentional (fail-fast guard), but the discarded return value at line 465 is confusing. Consider assigning it to a variable or adding a comment clarifying this is a validation-only call. [fixable]
| // Auto-send pending session (from "Start Session" on inbox/todo items) | ||
| const pendingConsumed = useRef<string | null>(null); | ||
| const [pausedLaunch, setPausedLaunch] = useState<typeof pendingSession>(null); | ||
| const accountUnavailable = useCallback(() => { |
There was a problem hiding this comment.
🟡 regressions: The accountUnavailable callback captures pendingSession in its closure via useCallback(..., [pendingSession, clearPendingSession]). However, when onUnavailable fires from AccountModelPicker, it's called during render or in a useEffect triggered by error/empty state. If pendingSession arrives after the error has already been set (the 'after' timing in the test), accountUnavailable was created with pendingSession: null. The test works because the component re-renders when pendingSession changes, creating a new accountUnavailable closure, and the useEffect at line 41-43 re-fires when error is already set. But there's a subtle timing dependency: the onUnavailable effect depends on both error and onUnavailable (which changes when pendingSession changes). If onUnavailable changes but error stays the same, the effect re-runs and correctly calls the new closure. This works but is fragile.
| if (!pendingSession || !accountSelection) return; | ||
| // Guard against double-consumption of the same pending session | ||
| const key = pendingSession.prompt; | ||
| if (pendingConsumed.current === key) return; |
There was a problem hiding this comment.
🔵 bugs: The pendingConsumed ref guards duplicate sends by keying on pendingSession.prompt. If two successive pending sessions arrive with identical prompt text (e.g., two task launches with the same template), the second will be silently dropped. Consider using a unique key like a combination of prompt + telosTaskId or a counter instead. [fixable]
| const env = { ...base }; | ||
| for (const key of Object.keys(env)) { | ||
| if ( | ||
| /^(ANTHROPIC_|OPENAI_|CLAUDE_CODE_USE_|CLAUDE_CODE_SKIP_|VERTEX_REGION_)/.test(key) || |
There was a problem hiding this comment.
🔵 style: The regex /^(ANTHROPIC_|OPENAI_|CLAUDE_CODE_USE_|CLAUDE_CODE_SKIP_|VERTEX_REGION_)/ strips env vars that are then re-set on lines 101-106. While correct (delete-then-reassign), the intent is not obvious to a reader. The CLAUDE_CODE_SKIP_* prefix is forward-looking — no such vars exist in the codebase today. Consider documenting the scrubbing intent or using an explicit allowlist of keys to preserve instead of a deletion regex. [fixable]
| return new AccountProfiles( | ||
| JSON.parse(readFileSync(process.env.MITZO_ACCOUNT_PROFILES_FILE, 'utf8')), | ||
| ); | ||
| } catch { |
There was a problem hiding this comment.
🔵 style: The catch block in loadAccountProfiles() at line 117 catches all errors (including JSON parse errors, file-not-found, and Zod validation failures) and replaces them with a generic message. For operator debugging, preserving the original error message or logging it server-side would help diagnose misconfigured profile files. [fixable]
1 participant
Mobile chat now offers a server-backed account/model picker. Starting a task validates the selected Vertex profile and pins the SDK environment to its project, region and ADC reference. Bound conversations retain their original account/model across follow-ups, reloads and restarts. Unknown selections and changed configuration fail explicitly.
Before starting a bound SDK session, the server assigns a valid UUID and persists account binding plus boot context together. A startup failure therefore cannot turn newly created SDK history into an unbound legacy task. Profiles are loaded once per request and revalidated before startup. Credential contents stay on the server.
The picker validates catalog data, offers retry on errors, and offers an explicit legacy-server-account choice only for an empty catalog. Send stays disabled until an account is ready, preserving typed drafts. Failed quick launches—including those arriving after an error—pause with prompt and task metadata preserved until explicit send. Distinct launches with identical prompt text remain independent. Failed SDK startups are marked ended/inactive while retaining the account binding. Existing task bindings are visible; changing account/model requires a new task. Legacy clients' global model hints intentionally do not override a saved binding.
This is the Vertex mobile profile slice, with #450's dependency fixes included. OpenAI execution and full physical-phone acceptance remain separate work. No deployment is included.
Validation at 90c0796: 207 files / 3,216 tests passed; server/frontend types, lint, formatting and builds passed. Current-head CI run 34099544817 passed. Earlier candidate validation exercised ADC refresh, SDK Read, follow-up, reload and restart in a 390 × 844 browser viewport; that is not physical-phone acceptance of this final head. Existing lint/bundle warnings remain. See docs/features/account-profiles.md for configuration, persistence and remaining acceptance.
Final review resolution evidence:
Round 3 addressed in 8912a39, following the catalog/request-snapshot fixes in f7913b0.
Validation: 207 files / 3,213 tests; lint, formatting, server/frontend types and builds passed. Physical phone acceptance and deployment are not claimed.
Round 5 fixes at 90c0796:
Full suite: 207 files / 3,216 tests passed, with all required checks.