fix(agent-mode): preserve selected model across backend startup - #2709
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 766b1cd6b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8f7928c79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c07cc6f330
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9b8a2ad2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73ede36306
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!manager) return; | ||
| if (installState.kind !== "ready") return; | ||
| if (manager.getCachedBackendState(descriptor.id)?.model) return; | ||
| if (manager.getCachedModelCatalog(descriptor.id)) return; |
There was a problem hiding this comment.
Retry probes whose cached catalog is incomplete
When the load-time probe settles with mode state but no model catalog, the preloader deliberately caches the truthy object { availableModels: null }, so this check suppresses the settings panel's recovery probe. On a clean install where the user opens this settings panel without first starting a live chat, discovery never enrolls any models and the enable list remains empty; only skip the probe when the cached availableModels value is actually present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1dcf678. BackendPanel now skips recovery only when cached availableModels is present; triggers preloadModels just like a missing catalog. Added a direct mount regression covering the incomplete cached-catalog case. Verification: focused 2 suites/46 tests, full Jest 345 suites/4,902 tests, format, lint, build, and diff-check all pass.
There was a problem hiding this comment.
Correction to the previous reply: the incomplete { availableModels: null } catalog now triggers preloadModels just like a missing catalog. The fix and verification details remain as stated.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dcf67817a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Catalog rows may be visible before session/new supplies the active value. | ||
| // Keep them non-interactive so opening and dismissing the merged picker | ||
| // cannot commit an unrelated row into a still-starting tab. | ||
| disabled: ctx.activeSession !== null && ctx.activeSession.getState() === null, |
There was a problem hiding this comment.
Keep model-less session picker dismissals noncommitting
Fresh evidence beyond the prior startup-null fix is that BackendState explicitly permits a non-null mode-only state with model: null, and the new preloader tests exercise exactly that response. In this case this condition enables the picker even though valueKey is still empty; ModelEffortPicker then initializes its draft to the first enabled catalog row and commits it when the popover is merely dismissed. When that row belongs to another backend, dismissing the picker replaces and closes the active model-less tab. Disable while an active session has no model selection, or make an empty initial value noncommitting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 32485cc. buildAgentModelPicker now remains disabled whenever an active session lacks a model selection, including non-null mode-only backend state. Regression tests cover startup-null, mode-only model-null, no-session, and loaded-model boundaries. Verification: focused 3 suites/169 tests, full Jest 345 suites/4,907 tests, format, lint, build, and diff-check all pass.
| const target = this.getDefaultSelection(backendId); | ||
| if (!target) return; |
There was a problem hiding this comment.
Stop promising a live reset when clearing the default
When the user changes an explicit default to “Agent default,” getDefaultSelection() returns null and this branch leaves every open session on the old explicit model indefinitely. The settings control still tells users that open chats switch on their next turn, so the next prompt can run on the model they just cleared. Since the backend contract has no reset operation, update the setting copy to distinguish clearing from choosing a concrete default, or restart/recreate the affected session to honor the promise.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 32485cc. The settings description now distinguishes explicit model changes from clearing to Agent default: choosing a model switches open chats on the next turn, while Agent default applies to future sessions because the backend contract has no native-reset operation. A rendered copy assertion covers the distinction; the existing manager test continues to prove clearing does not infer or apply a catalog model. Verification: focused 3 suites/169 tests, full Jest 345 suites/4,907 tests, format, lint, build, and diff-check all pass.
| const models = manager.getCachedModelCatalog(backendId)?.availableModels ?? null; | ||
| const found = models?.find((m) => m.baseModelId === baseModelId); | ||
| const reported = found?.effortOptions ?? []; | ||
| if (reported.length > 0) return reported; | ||
| return manager.getEffortCatalog(backendId)?.[baseModelId] ?? EMPTY_EFFORT_OPTIONS; |
There was a problem hiding this comment.
Feed live effort options into the settings picker
Fresh evidence beyond the prior merged-picker fix is that AgentDefaultModelSetting still calls this shared-only resolver. When OpenCode's best-effort effort prefetch fails or is skipped and the user activates a different model, the live session can report that model's thought_level options, but this lookup sees neither them nor a prefetched entry; consequently the chat picker offers the effort while Settings renders no “Default effort” control for the same model. Let the settings consumer prefer the active session entry when its backend/model matches before falling back to probe discovery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 32485cc. resolveEffortOptions now centralizes precedence as active current-model state, shared discovery, then prefetch. AgentDefaultModelSetting passes matching active-session state, so Settings exposes live thought-level options when shared discovery is empty. Direct tests cover live-current precedence, non-current rejection, and rendered Settings fallback. Verification: focused 3 suites/169 tests, full Jest 345 suites/4,907 tests, format, lint, build, and diff-check all pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32485cc7ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Catalog rows may be visible before session/new supplies the active value. | ||
| // Keep them non-interactive so opening and dismissing the merged picker | ||
| // cannot commit an unrelated row into a still-starting tab. | ||
| disabled: ctx.activeSession !== null && ctx.activeModelState === null, |
There was a problem hiding this comment.
Keep the picker disabled until startup settles
When session/new has returned state but confirmSeededSelection() is still awaiting the startup model write, activeModelState is non-null even though the session remains starting, so this enables the merged picker. ModelEffortPicker does not honor the sibling effort.disabled flag and calls effort.onChange when dismissed after an effort edit; that second config RPC can race the startup model/effort RPC and either target the old model's effort option or be overwritten by the persisted startup selection. Include the active session's startup status in this disabled condition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 03de561. buildAgentModelPicker now keeps the merged picker disabled for the entire starting status even after session/new has supplied a non-null model state, and it still disables mode-only model-null state. The new regression test covers a seeded non-null model while startup confirmation remains pending. Verification: focused 4 suites/191 tests, full Jest 345 suites/4,911 tests, format, lint, build, and diff-check all pass.
| if (manager.getCachedModelCatalog(descriptor.id)?.availableModels) return; | ||
| manager | ||
| .preloadModels(descriptor.id) |
There was a problem hiding this comment.
Replace the warm probe before retrying discovery
When the load-time probe returns mode state but availableModels: null, it still leaves a live process in the preloader's warm map, and this recovery path invokes raw preloadModels() without clearing that entry. The retry eventually overwrites the map entry without shutting down the original process or removing its exit listener, so opening Settings after an incomplete probe leaks at least one backend process; repeated incomplete retries accumulate more processes that even preloader shutdown can no longer reach. Route this through a refresh/clear-before-preload path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 03de561. AgentSessionManager.preloadModels now detects an incomplete cached catalog and routes the retry through the preloader refresh path, which clears and shuts down the old warm process before probing again. AgentSettings continues using the manager boundary instead of reaching into the preloader. A direct manager regression test proves refresh is used and raw preload is not; the existing preloader tests cover warm-process shutdown. Verification: focused 4 suites/191 tests, full Jest 345 suites/4,911 tests, format, lint, build, and diff-check all pass.
| const activeSession = manager.getActiveSession(); | ||
| const activeModelState = | ||
| activeSession?.backendId === descriptor.id ? (activeSession.getState()?.model ?? null) : null; |
There was a problem hiding this comment.
Include live state in the settings snapshot
Fresh evidence after the live-effort fallback fix is that this newly read active-session state is absent from the component's useSyncExternalStore snapshot, which still consists only of getModelCacheSignature(). If Settings is already mounted when the active session later reports model-specific effort options, the active UI subscription fires but React observes an unchanged snapshot and skips rendering, so activeModelState is never reread and the Default effort control remains absent until an unrelated cache or settings change. Add the matching active-session model signature to the snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 03de561. AgentDefaultModelSetting now combines the shared model-cache signature with the matching active session model-state signature. The existing active UI subscription therefore produces a changed external-store snapshot when live model-specific effort options arrive, so Settings rerenders immediately. A deterministic subscription test emits the active-state notification and waits for the effort control to appear. Verification: focused 4 suites/191 tests, full Jest 345 suites/4,911 tests, format, lint, build, and diff-check all pass.
| async applySelection(session: AgentSession, selection: ModelSelection, context): Promise<void> { | ||
| const apply = session.getState()?.model?.apply; | ||
| if (apply?.kind === "setConfigOption" && apply.effortConfigId) { |
There was a problem hiding this comment.
Switch config-option models before looking up effort
When OpenCode's native startup model has no thought_level option but the persisted target model does, apply.kind is setConfigOption while apply.effortConfigId is absent, so this bypasses the two-step branch and sends the encoded <target>/<effort> string to the model config option. Newer OpenCode exposes exact bare model IDs through that option and only reports the target's effort option after the model switch, so startup can reject the malformed value and silently remain on the native model. This is newly exposed now that spawn config no longer starts OpenCode on the persisted target; handle every setConfigOption model by switching the bare model first, then use the refreshed effort ID when present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 03de561. OpenCode applySelection now takes the two-step config-option path whenever the model apply mechanism is setConfigOption, even when the native startup model reports no effort option. It switches the bare target model first, rereads the refreshed state, and applies effort only through the newly reported effort config id. The regression test starts from Minimax with no effortConfigId and proves the bare model write precedes the effort write. Verification: focused 4 suites/191 tests, full Jest 345 suites/4,911 tests, format, lint, build, and diff-check all pass.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44410f5bcc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| expect(mgr.isPreloadReady("opencode")).toBe(true); | ||
| }); | ||
|
|
||
| describe("getCachedModelCatalog()", () => { |
There was a problem hiding this comment.
Consolidate the manager tests under a class suite
The newly added callable groups (getCachedModelCatalog(), getModelCacheSignature(), refreshEffortCatalog(), and preloadModels()) are nested inside the top-level describe("AgentSessionManager preload status", ...), while this file remains split across numerous peer top-level suites. Consolidate the file under one module suite with an AgentSessionManager class suite, then place each method in its single callable group so ownership remains visible.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6afae77. AgentSessionManager.test.ts now has one module suite containing one AgentSessionManager class suite. The added getCachedModelCatalog(), getModelCacheSignature(), refreshEffortCatalog(), and preloadModels() groups are direct class-suite siblings, and the legacy peer suites were consolidated under that ownership boundary. Format, lint, full Jest (346 suites / 4,931 tests), build, and diff-check pass.
| if (this.effortCatalogOwners.get(backendId) === warmEffortToken) { | ||
| this.effortCatalog.delete(backendId); | ||
| this.effortCatalogOwners.delete(backendId); |
There was a problem hiding this comment.
Cancel the effort sweep when the warm process exits
If the warm subprocess exits while prefetchEffortCatalog() is still running, no effort owner has been assigned yet, so this cleanup removes nothing and leaves the prefetch token valid. OpenCode's sweep catches subsequent per-model RPC failures and can return the entries collected before the exit; runEffortPrefetch() then publishes that partial catalog after the process is dead, and later live recovery skips retrying because effortCatalog.has(backendId) is true. Invalidate the in-flight effort token in the exit handler so a dead probe cannot publish partial results.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6afae77. The warm-process exit handler now invalidates the matching in-flight effort token before any late prefetch result can publish. The regression test holds the first effort result, exits the warm process, releases a partial result, waits deterministically for the queued recovery probe, and confirms the partial catalog was discarded. Format, lint, full Jest (346 suites / 4,931 tests), build, and diff-check pass.
| const incomingModels = catalog.availableModels; | ||
| const cachedModels = this.modelCatalogCache.get(backendId)?.availableModels; | ||
| if (this.disposed || !incomingModels || incomingModels.length === 0) return; | ||
| if (cachedModels && cachedModels.length > 0) return; |
There was a problem hiding this comment.
Replace discovery after an adopted backend crashes
When a warm process is adopted, takeWarm() removes the preloader's exit listener; if that manager-owned process later exits unexpectedly, the manager removes its sessions but leaves this non-empty catalog cached. A retry can then start a new process with a different catalog, but seedModelCatalogIfMissing() rejects the fresh result here, unlike the removed live-state mirroring which used to replace it. Clear the backend's cached discovery from the manager exit handler, or otherwise allow the first post-crash session to replace the orphaned catalog, so the picker and enrollment do not remain stale until reload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6afae77. AgentSessionManager now clears the preloader discovery owned by the current backend process when that process exits, before the no-session early return. A warm-adoption regression test starts a session on the adopted process, emits its later exit, and verifies the backend catalog is cleared for fresh discovery. Format, lint, full Jest (346 suites / 4,931 tests), build, and diff-check pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6afae77240
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type="select" | ||
| title="Default model" | ||
| description="Used for new chats and multi-agent answers on this agent. Open chats switch on their next turn." | ||
| description="Used for new chats and multi-agent answers. Choosing a model switches open chats on their next turn; Agent default applies to future sessions." |
There was a problem hiding this comment.
Document the new agent-default semantics
This changes user-facing behavior so selecting an explicit default updates open chats, while clearing it to “Agent default” affects only future sessions, but the PR makes no corresponding update to docs/agent-mode-and-tools.md or another user guide. Users consulting the documentation therefore cannot discover this distinction and may expect clearing the setting to reset their current chat; add the setting and its live-session behavior to the Agent Mode documentation.
AGENTS.md reference: AGENTS.md:L36-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0a3e29d. docs/agent-mode-and-tools.md now explains where to configure each agent’s Default model, that choosing an explicit model updates new sessions, multi-agent answers, and open chats on their next turn, and that Agent default affects future sessions while existing chats keep their current model. It also tells users to start a new chat or use the chat picker when they want to change an existing chat. Prettier and diff-check pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a3e29d053
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await this.startEffortPrefetch( | ||
| backendId, | ||
| descriptor, | ||
| proc, | ||
| probe.sessionId, | ||
| probe.state, |
There was a problem hiding this comment.
Cancel transient effort sweeps when their process exits
Fresh evidence beyond the warm-process exit fix: a transient retainWarm=false recovery probe launches this effort sweep without registering any proc.onExit handler. If OpenCode exits after a few model switches, prefetchEffortCatalog() catches the remaining per-model RPC failures and returns the partial result, which is still published because its token remains valid; recoverEffortCatalog() then treats the nonempty effort cache as complete and skips another attempt, leaving later models without effort choices until another explicit refresh or reload. Invalidate the transient sweep token on unexpected exit, while unregistering that handler before the intentional shutdown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d776564. Transient effort recovery now registers a process-exit listener with its own sweep token; an unexpected exit invalidates that token so a late partial result cannot publish. The listener is explicitly removed before the normal on-settled shutdown, so intentional cleanup is not treated as a crash. A deterministic regression test holds the effort result, emits transient exit, releases a partial catalog, waits for shutdown, and confirms the cache stays empty. Format, lint, full Jest (346 suites / 4,932 tests), build, and diff-check pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d776564d64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .then(() => { | ||
| if (session.getStatus() === "closed") return; | ||
| const target = | ||
| this.getDefaultSelection(backendId) ?? this.nativeDefaultSelection(backendId); | ||
| const target = this.getDefaultSelection(backendId); | ||
| if (!target) return; | ||
| return descriptor.applySelection(session, target); |
There was a problem hiding this comment.
Defer default reapplication until the session is idle
When a user changes the default while a Claude chat is running, this immediately invokes applySelection; ClaudeSdkBackendProcess.setSessionModel() then forwards the change to the active query through session.active.setModel(), so the in-flight turn can switch models even though the documented contract says open chats switch on their next turn. Queue the reapplication until the session leaves running/awaiting_permission, while continuing to coalesce rapid settings changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4120fb7. Default reapplication now waits for both session.ready and session.waitForIdle() before calling the descriptor, so changing the configured default cannot switch Claude during an in-flight turn. The chain still reads the latest configured target after the idle wait. Added a deterministic running-to-idle manager test and verified the full 346-suite / 4,937-test run.
| if (Object.keys(catalog).length > 0) { | ||
| this.effortCatalog.set(backendId, catalog); | ||
| this.effortCatalogOwners.set(backendId, token); | ||
| this.notify(); |
There was a problem hiding this comment.
Retry incomplete effort-catalog sweeps
During the new transient recovery or agent-model enablement refresh, OpenCode catches failures for individual model switches and returns the entries collected for the other models. Any nonempty partial result is cached here as if discovery completed, and recoverEffortCatalog() subsequently stops whenever effortCatalog.has(backendId) is true; therefore a single transient RPC failure permanently withholds effort choices for that model until another explicit refresh or plugin reload. Track sweep completeness or leave incomplete results eligible for retry rather than treating any nonempty object as recovered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4120fb7. Per-model OpenCode probe failures now call markIncomplete(), and AgentModelPreloader refuses to publish or cache that partial effort catalog as complete. A regression test proves the partial first result stays uncached and a later explicit refresh can populate the full catalog. Full suite: 346 suites / 4,937 tests.
| preloadModels(backendId: BackendId): Promise<void> { | ||
| const catalog = this.preloader.getCachedModelCatalog(backendId); | ||
| if (catalog && (catalog.availableModels?.length ?? 0) === 0) { | ||
| return this.preloader.refresh(backendId) ?? this.preloader.preload(backendId); | ||
| } | ||
| return this.preloader.preload(backendId); |
There was a problem hiding this comment.
Avoid retaining a recovery process beside a live backend
When the load-time warm process has already been adopted and its visible session still reports a null or empty model catalog, opening Settings reaches this refresh path while the manager already owns a running process. refresh() launches a normal retaining probe and stores it in warm, but ensureBackend() always returns the existing manager-owned process before calling takeWarm(), so the recovery process remains running and unconsumed for the rest of the plugin lifetime. Use a transient catalog-recovery probe when this.backends already contains this backend, rather than retaining a second subprocess.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4120fb7. When the manager already owns a live backend and its catalog is absent or empty, preloadModels() now requests refreshModelCatalog(), which uses a transient probe and shuts it down instead of creating a second retained warm backend. Added manager and preloader tests that assert the transient path and no warm retention. Full suite: 346 suites / 4,937 tests.
| const added = change.enabledModels.filter((id) => !previous.has(id)); | ||
| const removed = change.previousEnabledModels.filter((id) => !next.has(id)); | ||
| const changed = [...added, ...removed]; | ||
| if (changed.some((id) => resolveOrigin(id) !== "agent")) return "restart"; | ||
| if (added.length > 0) return "refreshEffort"; |
There was a problem hiding this comment.
Coalesce initial enrollment into one effort sweep
On first OpenCode enrollment, AgentSetupApi enables every discovered model with a separate backend-config mutation, so the first addition immediately starts a transient effort probe while every later addition marks pendingEffortRecovery. When that probe finishes, the pending flag unconditionally launches a second sweep; because process startup is slower than the enrollment writes, the first sweep generally already sees the complete enabled set, making the second subprocess and all per-model configuration RPCs redundant. Coalesce the enrollment burst before returning refreshEffort, or delay the first recovery until the enabled-model writes have settled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4120fb7. Recovery requests are now coalesced through one zero-delay scheduled transient probe, so synchronous enrollment writes before the first sweep start only one process. Requests arriving while a probe is already in flight still produce exactly one trailing recovery. Added deterministic fake-timer coverage for the enrollment burst; full suite: 346 suites / 4,937 tests.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4120fb7e83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
| if (this.disposed) return; | ||
| if (Object.keys(catalog).length > 0) this.effortCatalog.set(backendId, catalog); | ||
| if (isAborted() || !complete) return; |
There was a problem hiding this comment.
Retry failed refreshes despite an older effort cache
Fresh evidence beyond the prior partial-sweep fix is the single-model enablement case after an earlier successful sweep: effortCatalog already contains the old models. If probing the newly enabled model calls markIncomplete(), this return preserves that old map and schedules no retry; recoverEffortCatalog() would also stop because effortCatalog.has(backendId) remains true. The new model consequently has no effort choices until another explicit refresh or reload, so mark the cached catalog incomplete or queue a retry instead of retaining it as proof that discovery is complete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0d435ad. An incomplete incremental effort sweep now removes the formerly complete cached catalog. If that stale cache existed, the preloader queues exactly one trailing recovery; a repeated failure with no complete cache does not create an infinite retry loop. The regression test starts with a successful old catalog, fails the newly enabled-model sweep, and proves the bounded retry publishes the expanded catalog. Full suite: 346 suites / 4,941 tests.
| } catch (error) { | ||
| if (!(error instanceof MethodUnsupportedError)) throw error; | ||
| } |
There was a problem hiding this comment.
Fall back when the guessed model option is rejected
Fresh evidence beyond the prior mode-only startup fix is that this fallback runs only when the entire session/set_config_option method is unsupported. An older OpenCode version can support that method for mode changes while exposing models only through legacy session/set_model; because startup has no model state, the hardcoded model option is attempted and rejected as an invalid config ID, which AcpBackendProcess.dispatchCapability() does not convert to MethodUnsupportedError. This rethrow is then swallowed by startup without trying setSessionModel, so explicit defaults still fail for visible sessions (and the equivalent catch in FanoutOrchestrator has the same problem); fall back when the guessed model option itself is unavailable, not only on method-not-found.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0d435ad. When startup has no model apply contract, the guessed OpenCode model-option write is now treated as an untrusted compatibility attempt: any rejection falls through to the legacy model RPC. The catch is scoped to that guessed model write, so a later advertised effort-option failure is not misclassified. The same fallback now covers fan-out sessions, with regression tests for both paths. Full suite: 346 suites / 4,941 tests.
| // Reaching here means the session is fully ready — it captured its context | ||
| // and opened its backend session (a failed startup rejects into `.catch` and | ||
| // never runs this, so the project stays dirty and a re-entry re-spawns). | ||
| this.seedModelCatalogFromState(resolvedId, session.getState()); |
There was a problem hiding this comment.
Seed discovery when live model state arrives later
When the load-time probe failed and a live session/new returns the supported mode-only state, this one-shot seed sees no model and returns. If the backend subsequently emits a state_changed event containing its model catalog, AgentSession updates its local state, but the manager no longer subscribes to model changes after attachModelCacheSync was removed, so the preloader and enrollment wiring never receive that recovered catalog. A clean installation therefore remains unenrolled and exposes at most the active model until Settings launches another probe or the plugin reloads; retain a subscription until a nonempty live catalog has been seeded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0d435ad. A session whose shared probe/startup state has no catalog now gets a temporary onModelChanged subscription. Mode-only updates are ignored; the first nonempty live model catalog seeds shared discovery and immediately unsubscribes, and session teardown also removes the listener. The regression test drives mode-only state followed by a late catalog and proves subsequent model changes no longer reseed. Full suite: 346 suites / 4,941 tests.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
obsidian-copilot/src/agentMode/backends/opencode/descriptor.ts
Lines 276 to 279 in 0d435ad
When an enabled BYOK model still has a stored key (credentialState: "ok") but OpenCode omits it from modelState.availableModels—the state rendered elsewhere as “Not offered by agent”—this loop nevertheless tries to activate it. The expected rejection calls markIncomplete(), after which AgentModelPreloader discards the entire effort catalog, so one stale model removes effort choices for every usable model and every refresh repeats the failure. Skip enabled IDs absent from the reported catalog before switching them.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
|
|
||
| it("seeds enabledModels to the agent's current model on first enrollment", async () => { | ||
| it("keeps every registered model enabled regardless of catalog order", async () => { |
There was a problem hiding this comment.
Wrap discovery tests in one module suite
The newly changed discovery cases remain split between the peer top-level describe("wireAgentModelDiscovery", ...) and describe("buildManagedOpencodeProviderIds", ...) suites. Wrap both callable groups in exactly one module-level suite so this modified test module follows the required ownership structure.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0927e81. agentModelDiscovery.test.ts now has exactly one module-level suite, with wireAgentModelDiscovery() and buildManagedOpencodeProviderIds() as its two callable groups. Assertions are unchanged; full suite: 346 suites / 4,942 tests.
| }); | ||
|
|
||
| it("sets top-level model from the persisted defaultModel.baseModelId", async () => { | ||
| it("leaves the process on OpenCode's native model even when a sticky model exists", async () => { |
There was a problem hiding this comment.
Group OpenCode backend tests by module and callable
This new buildOpencodeConfig case is still placed in one of several peer top-level suites; the file currently has three separate top-level groups for that function plus separate backend and constant groups. Consolidate them under one module suite, with one callable group for buildOpencodeConfig() and the class methods nested under an OpencodeBackend suite.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0927e81. OpencodeBackend.test.ts now has one module suite, one consolidated buildOpencodeConfig() group, one OpencodeBackend class suite containing buildSpawnDescriptor(), and the provider-map assertions directly under the module. Full suite: 346 suites / 4,942 tests.
| ); | ||
| }); | ||
|
|
||
| it("uses backend-confirmed startup state instead of the optimistic session state", async () => { |
There was a problem hiding this comment.
Nest descriptor callables under one module suite
The new startup-selection cases remain inside one of four peer top-level descriptor callable suites (wire.decode, wire.encode, applySelection, and prefetchEffortCatalog). Add one top-level module suite and place each callable group beneath it instead of continuing the split top-level structure.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0927e81. descriptor.test.ts now has one descriptor module suite containing the four callable groups: wire.decode(), wire.encode(), applySelection(), and prefetchEffortCatalog(). Full suite: 346 suites / 4,942 tests.
| }); | ||
|
|
||
| it("attempts setModel when defaultModelSelection is set", async () => { | ||
| it("confirms a Claude-style seed against the backend-reported model", async () => { |
There was a problem hiding this comment.
Consolidate AgentSession cases under its class suite
The modified startup-selection case remains a direct child of the top-level describe("AgentSession.create (via start)", ...), while the file still splits AgentSession across numerous peer top-level suites. Introduce the required single module suite and single AgentSession class suite, then keep each method's cases in exactly one nested callable group.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0927e81. AgentSession.test.ts now has one module suite, module-level function groups directly beneath it, and one AgentSession class suite. Split startup cases are consolidated into start(), and the formerly split fan-out cases are consolidated into sendPrompt(). Full suite: 346 suites / 4,942 tests.
| }); | ||
|
|
||
| describe("FanoutOrchestrator.run", () => { | ||
| it("applies an OpenCode default through the model config option when startup has no model state", async () => { |
There was a problem hiding this comment.
Consolidate fan-out tests under module and class suites
The added FanoutOrchestrator.run() cases remain in a peer top-level suite alongside createFanoutTurn. Wrap the file in one module suite, keep createFanoutTurn() directly beneath it, and place run() beneath a single FanoutOrchestrator class suite so callable ownership is explicit.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0927e81. FanoutOrchestrator.test.ts now has one module suite, createFanoutTurn() directly beneath it, and one FanoutOrchestrator class suite containing run(). Full suite: 346 suites / 4,942 tests.
| } catch (e) { | ||
| logWarn(`[AgentMode] could not apply seeded selection ${encoded}; reverting seed`, e); | ||
| this.currentState = originalState; |
There was a problem hiding this comment.
Preserve backend-confirmed state after an effort failure
When startup applies a model and effort in two steps, the model write may succeed before the effort write rejects—for example, after an agent upgrade removes a persisted effort value. setModel/the model config option has already updated currentState and the backend to the selected model, but this catch replaces the local snapshot with originalState without rolling the backend back; the session then becomes ready and prompts run on the selected model while the picker shows the original one. Distinguish a first-step failure from a later effort failure and retain the last backend-confirmed state, or explicitly roll back the successful model change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0927e81. confirmSeededSelection now captures the optimistic snapshot and only restores originalState if no backend write advanced currentState. If the model write succeeds and the later effort write fails, the last backend-confirmed model state remains authoritative. Added a regression test for successful model switch plus rejected effort; full suite: 346 suites / 4,942 tests.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0927e81e55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const refreshedApply = session.getState()?.model?.apply; | ||
| const effortConfigId = | ||
| refreshedApply?.kind === "setConfigOption" ? refreshedApply.effortConfigId : undefined; | ||
| if (effortConfigId) { | ||
| await session.setConfigOption(effortConfigId, selection.effort); |
There was a problem hiding this comment.
Retry the effort after model state arrives
When OpenCode's startup state is model-less and the guessed model option succeeds but its immediate response is still mode-only, refreshedApply remains absent and this branch returns without applying an explicit saved effort. If the model and thought_level state then arrives through the supported later state_changed path, nothing retries the effort, so visible chats run with OpenCode's native effort despite the user's default; FanoutOrchestrator.applyDefaultModel() has the equivalent early return for raw fan-out sessions. Keep watching until the post-switch effort option appears, as the catalog-recovery path already does for delayed model state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3e56f84. Visible OpenCode startup now keeps a temporary model-state subscription after a successful guessed model-option write; when delayed state_changed supplies the model-specific effort config, startup applies the saved effort before ready resolves. Fan-out tracks the same delayed state through its session handler, with abort/timeout cleanup, before dispatching the prompt. Added deterministic delayed-state regression tests for both paths. Full suite: 346 suites / 4,944 tests.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e56f8406b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (selection.effort !== null) { | ||
| const effortConfigId = await waitForEffortConfigId(session); | ||
| await session.setConfigOption(effortConfigId, selection.effort); |
There was a problem hiding this comment.
Bound the delayed effort-state wait
When a model-less OpenCode startup accepts the guessed model config write but never emits a follow-up model state_changed event—for example, a compatibility version continues returning mode-only state—an explicit default with a non-null effort waits here forever. AgentSession.initialize() awaits this call while startupSettled remains false, so the chat stays in starting and cannot send prompts; add a timeout/cancellation fallback like the equivalent fan-out wait, or let startup settle while applying the effort asynchronously.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a7d6509. The visible-session wait for OpenCode’s model-specific effort option is now capped at 10 seconds. If no model state arrives, startup settles with the already-applied base model, skips the unavailable effort write, clears the temporary listener, and allows the chat to proceed; closed/error sessions still reject immediately. Added a deterministic fake-timer regression covering the no-state path. Verification: format, lint, build, diff check, and 346 suites / 4,945 tests.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7d6509c6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| describe("AgentSetupApi.registerAgentProvider", () => { | ||
| it("creates exactly one agent-origin provider, N models, all enrolled into the agent backend only", async () => { | ||
| it("enrolls every initially reported model into the agent backend only", async () => { |
There was a problem hiding this comment.
Consolidate AgentSetupApi tests under module and class suites
The modified registerAgentProvider() case remains in one of two peer top-level suites, with syncAgentModels() starting another at line 509. Wrap the file in one module suite, add one AgentSetupApi class suite, and place each method under its single callable group so the required ownership structure is preserved.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| expect(listener).toHaveBeenLastCalledWith({ | ||
| backend: CHAT, | ||
| previousEnabledModels: [], | ||
| enabledModels: ["m1"], | ||
| }); |
There was a problem hiding this comment.
Add the BackendConfigRegistry module and class layers
Although the changed assertion is inside subscribe(), that group is still nested directly under the sole top-level BackendConfigRegistry suite, while the other class methods remain direct it children. Add a module suite containing one class suite, then consolidate every method under its own callable group.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| origin: { kind: "byok" }, | ||
| }); | ||
| expect(listener).toHaveBeenCalledTimes(1); | ||
| expect(listener).toHaveBeenLastCalledWith({ provider: registry.get(id) }); |
There was a problem hiding this comment.
Add the ProviderRegistry module and class layers
The modified subscribe() test remains under a top-level ProviderRegistry suite that also contains direct cases for the class's other methods. Introduce the required module wrapper and single ProviderRegistry class suite, with exactly one nested callable group per method.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| ).toBeNull(); | ||
| }); | ||
|
|
||
| it("returns null while the active session is starting", () => { |
There was a problem hiding this comment.
Nest buildAgentModePicker under a module suite
This newly added case is a direct child of the top-level describe("buildAgentModePicker", ...), so the file has a callable suite but no enclosing module suite. Add one module-level wrapper and keep all cases in a single nested describe("buildAgentModePicker()", ...) group.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| expect(persist).toHaveBeenCalledWith("opencode", null); | ||
| }); | ||
|
|
||
| it("explains that clearing to Agent default affects future sessions", () => { |
There was a problem hiding this comment.
Group AgentDefaultModelSetting cases by callable
The added component cases remain direct children of the sole top-level AgentDefaultModelSetting suite, rather than living in a callable group beneath a module suite. Add the module wrapper and consolidate the component's cases under exactly one nested describe("AgentDefaultModelSetting()", ...).
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| mockPreloadModels.mockReset().mockResolvedValue(undefined); | ||
| }); | ||
|
|
||
| it("skips model preload when the shared catalog is already available", async () => { |
There was a problem hiding this comment.
Group AgentSettings cases by callable
The newly added preload cases are direct children of the top-level AgentSettings suite, leaving no distinct module and callable layers. Wrap the component tests in one nested describe("AgentSettings()", ...) beneath a single module suite so all cases for the callable remain consolidated.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| disabled: | ||
| ctx.activeSession !== null && | ||
| (ctx.activeSession.getStatus() === "starting" || ctx.activeModelState === null), |
There was a problem hiding this comment.
Keep model-less idle sessions selectable
When OpenCode completes a supported mode-only startup and never emits later model state, a previously discovered shared catalog can still populate this picker, but activeModelState === null keeps the trigger disabled even after the session reaches idle. The user therefore cannot select any of those known models from the chat, and the new state-missing model-option fallback is unreachable from this UI; disable only during startup, or guard the empty-value dismissal without blocking explicit row selection.
Useful? React with 👍 / 👎.
| if ( | ||
| this.disposed || | ||
| this.effortCatalog.has(backendId) || | ||
| this.effortPrefetchTokens.has(backendId) | ||
| ) { | ||
| return; |
There was a problem hiding this comment.
Queue recovery behind an active effort sweep
If a live session supplies the first nonempty catalog while the load-time or warm effort sweep is still active, this branch simply returns instead of marking a trailing recovery pending. When that older sweep was based on an empty/incomplete catalog and then fails or calls markIncomplete() without an earlier effort cache, it publishes nothing, and its finally sees no pending request to retry; the recovered models consequently lack prefetched effort options until another explicit refresh or reload. Route this case through scheduleTransientProbe() so the existing pending mechanism queues recovery behind the active token.
Useful? React with 👍 / 👎.
a7d6509 to
8515bb4
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8515bb46d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.modelCatalogCache.set(backendId, { | ||
| availableModels: probe.state.model?.availableModels ?? null, | ||
| }); |
There was a problem hiding this comment.
Preserve the backend-reported native default
When OpenCode's session/new default is not the first catalog entry—for example, a user supplies a model through OPENCODE_CONFIG_CONTENT—this projection discards probe.state.model.current, while getDefaultBaseModelId() later treats availableModels[0] as the native default. Because this PR also removes the generated spawn-time model, creating a chat with “Agent default” or clearing an explicit default now switches to the first catalog model rather than retaining the model OpenCode actually chose. Preserve the probe-reported default separately or stop synthesizing a fallback selection.
AGENTS.md reference: AGENTS.md:L72-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7dcbb07. Agent default no longer synthesizes a selection from catalog ordering: normal and fan-out sessions keep the model reported by their own session/new, and clearing an explicit default leaves an existing session unchanged. The obsolete catalog-default accessor and tests were deleted. Verification: focused manager/fan-out suites (127 tests), format, lint, build, diff check, and the full suite (345 suites / 4,895 tests).
| } | ||
|
|
||
| describe("AgentModelPreloader.takeWarm", () => { | ||
| describe("AgentModelPreloader", () => { |
There was a problem hiding this comment.
Organize preloader tests by module and callable
The changed suite uses AgentModelPreloader as its top-level suite and leaves the takeWarm(), shutdown(), refresh(), and clearCached() cases as direct it children, so this modified test module still lacks the required module wrapper and one callable group per method. Add a module suite, nest one AgentModelPreloader class suite beneath it, and consolidate each method's cases into its own nested describe.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this in PR #2709. This requests restructuring the entire pre-existing test module and does not identify a behavioral failure introduced by the model-ownership fix. The changed preloader behavior has direct regression coverage, and broad test-layout churn was intentionally removed to keep this PR scoped to issue #227.
| it("returns null when both session and cache are empty", () => { | ||
| const got = resolveActiveDisplayState(null, "codex", () => null); | ||
| expect(got).toBeNull(); | ||
| describe("collectModelActiveContext", () => { |
There was a problem hiding this comment.
Consolidate model-picker helper tests by module and callable
The modified helper tests remain split across peer top-level suites such as collectModelActiveContext, buildPickerEntries, and buildEffortOptionsByModelKey, and buildPickerEntries is additionally split into a second top-level capability-propagation suite. Wrap the file in one module suite and merge every callable's cases into exactly one nested describe("callableName()", ...) group.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this in PR #2709. Consolidating every pre-existing helper suite is a file-wide structural rewrite, not a correctness fix for the session-owned selection/catalog boundary. The changed picker behavior is directly covered; reintroducing broad test-only churn would work against the narrowed scope.
| ); | ||
| expect(backendStateSignature(before)).not.toBe(backendStateSignature(after)); | ||
| }); | ||
| describe("modelCatalogSignature", () => { |
There was a problem hiding this comment.
Wrap backend-state translator tests in one module suite
The newly changed modelCatalogSignature cases are still one of many peer top-level callable suites in this file, alongside the split translateBackendState suites and the other signature helpers. Add a single module-level suite and keep each module function under exactly one nested callable group, merging the multiple translator groups.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this in PR #2709. Wrapping and merging all pre-existing translator suites is unrelated to the catalog-signature behavior changed here. The new signature contract has direct unit coverage, while a whole-file test reorganization would be scope-only churn.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Fixes logancyang/obsidian-copilot-preview#227
GitHub does not register this cross-repo closing reference, so issue #227 must be closed manually on merge.
Problem
At session startup, the picker could show the persisted model before the backend had actually switched to it. When
session/newreported a different current model, the descriptor compared the desired selection with optimistically seeded UI state and could skip the required backend write.The preloader also cached a full backend state per backend. That incorrectly treated the current model, effort, mode, and apply mechanism as shared across tabs even though those values belong to one session.
Fix
Treat
session/newas the startup source of truth. Compare the desired model with that backend-reported selection, apply a real model change when they differ, and keep the session instartinguntil the write settles.Restrict shared preload data to model choices and prefetched effort options. The active session is now the only owner of current model, effort, mode, and apply state.
Treat “Agent default” as the absence of an explicit selection. New sessions keep the model reported by their own
session/new; catalog ordering never substitutes for a backend default, and clearing a preference does not retarget an existing chat.Scope
Budget gate: PASS — 32 files, +874/−931; production code is net −187 lines, and the shared catalog, startup comparison, and explicit-selection paths are the smallest shape that removes backend-wide session state without recovery machinery.
Changes
session/new; prompts remain gated until that startup selection settles.Verification
npm run format— passed.npm run lint— passed.npm test -- --runInBand— 345 suites and 4,895 tests passed.npm run build— passed.git diff --check FETCH_HEAD...HEAD— passed.copilot-test-vaultrepro on21869ae5— Codex → existing OpenCode/Qwen → Big Pickle; the picker stayedOpenCode Zen/Big Pickle, the exact smoke response returned, and Obsidian reported no errors.acp-frames/5ccab3db/acp-frames.ndjsonlines 1798–1858 —session/new, explicit confirmedsession/set_config_option model=opencode/big-pickle, thensession/promptandend_turn.7dcbb073— focused manager/fan-out suites passed (127 tests); the full verification ladder above passed afterward.🤖 Generated with Codex