fix(provider): record the picked model so model-less sessions inherit it - #1347
Conversation
defaultModel() picks the default from state/model.json's `recent` list, but nothing in this fork writes that list — upstream wrote it from the TUI, and replacing the TUI with the desktop UI kept only the reader. With `recent` permanently empty, any session that sends no explicit model (a Telegram /new, a bare HTTP caller) falls through to the first configured provider instead of the user's actual last choice. Restore the writer on the server side: after a user's top-level prompt settles on a model, record it into `recent` via a locked read-modify-write that preserves the file's other fields (favorite/variant). Only the user's own top-level prompt seeds the default — automation runs and subagent/agent-tool child sessions are filtered out, so an inner agent model can never leak into the default a fresh session inherits. Verified against upstream/dev (tui writes recent; this fork dropped it). Tests cover the pollution filter and recent dedupe/cap/field preservation.
|
Warning Review limit reached
More reviews will be available in 28 minutes and 17 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a ChangesRecent Model History
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
harness
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ModelState namespace to persist and manage the user's last-used model in state/model.json. It includes pure functions to compute the updated recent models list, filters to prevent automation or subagent sessions from polluting the global default, and a locked write-modify-write mechanism to safely persist the state. Additionally, corresponding unit tests are added, and the state-recording logic is integrated into the session prompt workflow. The feedback highlights an incorrect relative import path for Flock that should be updated to its proper package entrypoint.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
A slash command resolves its own (often pinned) model and reuses the prompt path, so the recent-model writer recorded that command-scoped model into state/model.json. A later model-less session (e.g. a Telegram /new) then inherited the command's utility model instead of the user's chat model. Treat command invocations as a pollution source alongside automation and subagent child sessions: thread an internal fromCommand flag through PromptRuntimeOptions and fold it into shouldRecordRecent so a command never seeds the global default. Reported by Codex review (P2).
Review round 1Codex (xhigh) — 1× P2, fixed in 3db6a46: Gemini — 1× high ( CodeRabbit — rate-limited, no review delivered. |
The recent-model writer recorded the resolved message.info.model, which is input.model ?? agent.model ?? lastModel — so a session whose agent pins its own model (ag.model, used when no input.model is passed) leaked that model into the global default a model-less session (Telegram /new, HTTP) inherits. Gate on input.model instead: only the user's own explicit pick from the chat model picker seeds recent. A model merely derived from the agent, a command, automation, or a subagent does not. The desktop always sends input.model (the picker refuses to send without one), so this is zero-change for desktop chat and only drops the headless ag.model pollution. Adds the integration coverage the pure-function tests lacked: a prompt with an explicit model writes model.json.recent; an agent-only or automation prompt does not; and Provider.defaultModel reads a recordRecent-seeded model back. Semantic: only explicit user selections become the default. Reported by code review (P2).
model-state.ts defined its own isRecord; util/record already exports an identical guard. Drop the local copy. Reported by code review (P3).
Review round 2P2 — recorded the resolved model, not the user's explicit choice (fixed 6505f42): Chosen semantic (per the review's product-decision callout): only explicit user model selections become the new-session default. A model merely derived from the agent, a slash command, automation, or a subagent does not seed it. This matches upstream (the TUI wrote P2 — only pure functions were tested (fixed 6505f42):
P3 — reused shared isRecord (fixed 1731f4b): dropped model-state.ts's local copy in favour of model-state 9 · provider 109 · prompt 24 pass · typecheck clean. |
…ate tests The desktop UI always sends a resolved model with every prompt, and that model can be the selected agent's own configured pin rather than a model-picker choice (the renderer's model falls back to the agent's model). Gating the recent-model writer on input.model alone therefore still let an agent's utility model become the default a model-less session (Telegram /new) inherits — the pollution the guard is meant to prevent. createUserMessage now surfaces the agent's configured model so prompt() can tell an explicit pick apart from a fall-through to the agent pin; shouldRecordRecent gains a modelFromAgent guard alongside the existing automation/subagent/command exclusions. Tests: the new prompt→model.json→defaultModel tests share the process-wide state/model.json, so clear it around each so assertions on recent[0] stay independent of run order.
Round 3 — review fixes (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/opencode/test/session/prompt.test.ts (1)
1399-1595: ⚡ Quick winUse the Effect test harness (
testEffect+it.live) for this new suite.This block exercises Effect services and real filesystem/time behavior, so it should use the test harness pattern required for
packages/opencode/test/**/*.test.{ts,tsx}instead of rawtest(...).As per coding guidelines: “Use
testEffect(...)fromtest/lib/effect.tsfor tests that exercise Effect services… Useit.live(...)when the test depends on real time [and] filesystem…”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/session/prompt.test.ts` around lines 1399 - 1595, The test suite in the describe block "session.prompt seeds the recent default model" is currently using raw `test(...)` functions but should use the Effect test harness since the tests exercise Effect services (SessionPrompt.Service, Session.Service) and depend on real filesystem operations (fs.readFile, fs.rm) and real time behavior (setTimeout polling). Import `it.live` from `test/lib/effect.ts` at the top of the file and replace each `test(...)` call with `it.live(...)` for the four test cases: "an explicit-model user prompt seeds recent[0]", "a prompt that only inherits the agent's model does NOT seed recent", "an automation prompt does NOT seed recent", and "a prompt whose model equals the agent's configured model does NOT seed recent".Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/provider/model-state.ts`:
- Around line 81-83: The issue is in the withLock block where
Filesystem.readJson catches all read errors indiscriminately and treats them as
undefined, causing applyRecent to create a fresh object that overwrites the file
and loses existing favorite/variant data. Instead, differentiate between ENOENT
(file not found) errors and other read errors (permission denied, malformed
JSON, transient failures) by checking the error code. Only call applyRecent and
write via Filesystem.writeJson when the file doesn't exist; skip the write
operation if the file exists but is temporarily unreadable or malformed to
preserve existing data.
In `@packages/opencode/test/session/prompt.test.ts`:
- Around line 1487-1490: The test at lines 1487, 1533, and 1582 uses a fixed
setTimeout(150) before asserting that recent data is not polluted by a detached
recordRecent operation, which is unreliable on slow CI environments. Replace
each single setTimeout(150) with a bounded polling mechanism that repeatedly
checks the condition (that recent data does not contain the unwanted model with
providerID "openai" and modelID "gpt-5.2") within a reasonable timeout window,
ensuring the test waits for the actual state rather than relying on a fixed time
delay. This pattern should be applied consistently to all three test blocks
mentioned in the comment.
---
Nitpick comments:
In `@packages/opencode/test/session/prompt.test.ts`:
- Around line 1399-1595: The test suite in the describe block "session.prompt
seeds the recent default model" is currently using raw `test(...)` functions but
should use the Effect test harness since the tests exercise Effect services
(SessionPrompt.Service, Session.Service) and depend on real filesystem
operations (fs.readFile, fs.rm) and real time behavior (setTimeout polling).
Import `it.live` from `test/lib/effect.ts` at the top of the file and replace
each `test(...)` call with `it.live(...)` for the four test cases: "an
explicit-model user prompt seeds recent[0]", "a prompt that only inherits the
agent's model does NOT seed recent", "an automation prompt does NOT seed
recent", and "a prompt whose model equals the agent's configured model does NOT
seed recent".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d56bba84-27dd-4d98-9302-3b49e0793f6a
📒 Files selected for processing (5)
packages/opencode/src/provider/model-state.test.tspackages/opencode/src/provider/model-state.tspackages/opencode/src/session/prompt.tspackages/opencode/test/provider/provider.test.tspackages/opencode/test/session/prompt.test.ts
…mpt path The prompt path cannot distinguish an explicit model pick from an agent-pin fallback: the renderer always sends input.model (current() falls back to agent.current().model), so any prompt-path heuristic mislabels one or the other. The earlier attempts (shouldRecordRecent provenance flags, then a modelFromAgent equality guess) only traded false positives for false negatives. Record at the one place that truly knows the user chose a model: the desktop model picker. On an explicit pick the renderer calls a new POST /provider/recent, which persists the choice into state/model.json's `recent` list — the source Provider.defaultModel() reads when a session carries no model (e.g. a Telegram /new). - model-state: drop the prompt-path provenance helpers; recordRecent does a locked read-modify-write that preserves sibling fields and treats only a missing file (ENOENT) as empty — a parse/permission failure skips the write rather than clobber sibling state. - prompt.ts: fully reverted, no provenance plumbing. - server: POST /provider/recent (provider.recordRecent). - sdk: generated recordRecent client method + types. - renderer: mirror an explicit pick to the server, best-effort.
github-actions
Bot
added
app
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/src/provider/model-state.test.ts (1)
51-51: 💤 Low valueAwait the cleanup promise for consistency.
The
beforeEachhook properly awaits its async operations, butafterEachreturns the promise without awaiting. While Bun handles returned promises, addingasync/awaithere aligns withbeforeEachand makes the async behavior explicit.- afterEach(() => fs.rm(modelFile(), { force: true })) + afterEach(async () => await fs.rm(modelFile(), { force: true }))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/provider/model-state.test.ts` at line 51, The afterEach hook returns the promise from fs.rm without awaiting it, while the beforeEach hook properly awaits its async operations. To fix this for consistency, make the afterEach callback function async and add the await keyword before the fs.rm call to ensure the promise is properly awaited, matching the explicit async pattern used in beforeEach.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/opencode/src/provider/model-state.test.ts`:
- Line 51: The afterEach hook returns the promise from fs.rm without awaiting
it, while the beforeEach hook properly awaits its async operations. To fix this
for consistency, make the afterEach callback function async and add the await
keyword before the fs.rm call to ensure the promise is properly awaited,
matching the explicit async pattern used in beforeEach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a1c0f59a-c2f3-4a44-88e5-97544cd0f537
⛔ Files ignored due to path filters (2)
packages/sdk/js/src/v2/gen/sdk.gen.tsis excluded by!**/gen/**packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (4)
packages/app/src/context/local.tsxpackages/opencode/src/provider/model-state.test.tspackages/opencode/src/provider/model-state.tspackages/opencode/src/server/instance/provider.ts
Astro-Han
changed the title
The unit tests exercised ModelState.recordRecent and the recordRecent -> defaultModel round-trip directly; nothing hit the actual /provider/recent route. Add a route-level test (Server.Default().app) that POSTs a model and asserts it lands at model.json recent[0], plus a 400 case for a body missing modelID — so the route path, request body schema, and handler -> model.json wiring all fail loudly if broken.
Match the async/await form of the file's beforeEach and the sibling route test's hook. No behavior change (Bun already awaits a returned promise).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/test/server/provider-recent-route.test.ts (1)
46-57: ⚡ Quick winAssert no state write on 400 responses.
This test verifies status code, but not the no-side-effect guarantee. Add an assertion that
model.jsonis still absent/unchanged when schema validation fails.Proposed test hardening
test("rejects a body missing modelID (request schema is enforced)", async () => { await using tmp = await tmpdir({ git: true }) const app = Server.Default().app const response = await app.request("/provider/recent", { method: "POST", headers: { "x-opencode-directory": tmp.path, "content-type": "application/json" }, body: JSON.stringify({ providerID: "deepseek" }), }) expect(response.status).toBe(400) + await expect(fs.access(modelFile())).rejects.toThrow() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/server/provider-recent-route.test.ts` around lines 46 - 57, The test "rejects a body missing modelID (request schema is enforced)" currently only verifies the HTTP status code is 400, but does not assert that no state was written to the filesystem. Add an assertion after the status code check that verifies model.json is still absent or unchanged in the tmp directory to ensure the no-side-effect guarantee when schema validation fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/opencode/test/server/provider-recent-route.test.ts`:
- Around line 46-57: The test "rejects a body missing modelID (request schema is
enforced)" currently only verifies the HTTP status code is 400, but does not
assert that no state was written to the filesystem. Add an assertion after the
status code check that verifies model.json is still absent or unchanged in the
tmp directory to ensure the no-side-effect guarantee when schema validation
fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6996b8e5-8f45-4fce-80ba-7efd9b55d12a
📒 Files selected for processing (2)
packages/opencode/src/provider/model-state.test.tspackages/opencode/test/server/provider-recent-route.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/provider/model-state.test.ts
…see a partial file recordRecent held a lock, but Provider.defaultModel() reads model.json unlocked and treats a parse failure as empty recent (falling back to the first provider's model), and writeFile is not atomic — so a read landing inside the write window could see a half-written file and pick the wrong default. Write to a temp file and rename (atomic on the same filesystem): a concurrent reader now sees either the old or the new complete file, never a partial one. Scoped to recordRecent (no change to shared Filesystem behavior). Add a concurrency test asserting a valid file and no temp residue.
applyRecent only deduped; a structurally-invalid old entry (non-object, or missing string providerID/modelID) survived, occupied a cap slot, and could push a still-valid older model out of `recent` — which defaultModel() would otherwise fall through to. Filter the previous list to well-formed refs first; sibling fields (favorite/variant) are untouched.
…elper The pick-to-server call lived inline in local.tsx's model.set, untestable without standing up the whole Local context. Extract recordPickedModel(client, item, options) — which owns the "only an explicit pick (recent flag) records" gate — and unit-test it: an explicit pick records exactly once with the picked ref; a non-explicit set (model.cycle / plain set) and a cleared selection record nothing; a rejected recordRecent is swallowed so the pick is never disrupted.
Harden the schema-rejection case to prove the no-side-effect guarantee: a body missing modelID is rejected before the handler runs, so model.json stays absent.
The extracted recordPickedModel helper was a production module created only for testability, and its unit test exercised the helper in isolation — not that local.tsx actually calls it — so it added indirection without proving the real wiring. Inline the one best-effort call back into the picker's recent branch. The real HTTP boundary stays covered by the /provider/recent route test; the gate is the existing `if (!options?.recent) return` shared with models.recent.
The test only read the file after all writes settled, so a non-atomic plain write would have passed it too — false confidence. A deterministic atomicity test would need a slow-writer injection (production DI just for the test) or timing-based partial reads (flaky); neither is worth it. The temp+rename write is atomic by construction, and its read-modify-write path is already exercised end-to-end by the sibling-preservation / ENOENT / corrupt-file tests.
… (#1354) Follow-up to the #1347 review (recent-model default): guard the picker's recordRecent wiring with a unit test, plus a model-state comment trim. Root cause of the gap: `model.set(item, { recent: true })` is the only path that calls `provider.recordRecent`. The existing `ModelState`, route, and `defaultModel` tests all exercise the server side, so a refactor that drops `{ recent: true }` at the picker call site — or deletes the client call — would leave every test green while silently breaking the model-less-session default (a Telegram /new inherits the recent model). Change boundary: - Extract the picker's single write path into a dependency-free `selectModel()` (`model-picker-select.ts`); `model-picker.tsx`'s `ModelList.onSelect` now calls it. - `model-picker-select.test.ts` asserts the load-bearing invariant: an explicit pick AND clearing the selection both pass `{ recent: true }`. - `model-state.ts`: trim the upstream-TUI backstory header and PR-style doc to the load-bearing constraints (recent list in `state/model.json`; ENOENT starts empty; other read failures skip to protect sibling state; atomic rename for the unlocked reader). Why extract instead of testing the component: importing `model-picker.tsx` pulls `@kobalte/core`, which throws `Client-only API called on the server side` under the bun server-condition test runtime, so the real picker interaction is reachable only via Playwright e2e. The pure-function seam mirrors the existing `sidebar-item-navigation.ts` precedent. Review follow-ups: - An earlier attempt extracted `set`/`cycle` into a `createModelActions` factory; that added production surface for test-only reasons (YAGNI) and was reverted — `local.tsx`/`local.test.ts` are back at the merge base. - The negative cases (plain programmatic `set` / `cycle` must not record) are intentionally not asserted: there is no light real-path seam for them, and exercising them would require mounting the full `LocalProvider` — the heavy e2e the #1347 review vetoed. Verification: - `bun test model-picker-select.test.ts local.test.ts` — 8 pass (2 new + 6 unchanged local) - red/green gate confirmed: removing `{ recent: true }` from `selectModel` turns the test red - `bun run typecheck` app + opencode clean, eslint clean, full CI green Residual risk: the real picker click interaction (DOM → onSelect) is covered by e2e, not this unit test; this guards the wiring invariant only.
1 participant
Summary
Restore the writer for
state/model.json'srecentlist, so a session that sends no explicit model inherits the user's actually-picked model instead of falling back to the first configured provider. The list is written at the one place that truly knows the user chose a model — the desktop model picker — via a new server endpoint.provider/model-state.ts:applyRecent(pure: front-promote, dedupe, cap, preserve sibling fields) andrecordRecent(locked, best-effort read-modify-write). Reuses the sharedutil/recordisRecord.POST /provider/recent(provider.recordRecent) persists{providerID, modelID}intorecent.model.set(item, { recent: true }), the model picker'sonSelect),local.tsxcallsprovider.recordRecentbest-effort, alongside its ownmodels.recent.push.recordRecentclient method + types for the new endpoint.Why record on pick, not on the prompt path. The prompt path cannot tell an explicit pick from an agent-pin fallback: the renderer always sends
input.model(current()falls back toagent.current().model), so a prompt-path heuristic mislabels one or the other (an earliershouldRecordRecentprovenance filter, then amodelFromAgentequality guess, each traded false positives for false negatives). The model picker is the only place with unambiguous intent, so recording there is structurally correct — an agent's pinned model, a slash command, automation, or a subagent can never leak into the global default, with no provenance guessing.prompt.tsis fully reverted.Why
Provider.defaultModel()picks the default fromrecent, but nothing in this fork wrote that list — upstream wrote it from the TUI (packages/tui/src/context/local.tsx), and replacing the TUI with the desktop UI kept only the reader (provider.ts). Withrecentpermanently empty, every model-less entry point gets the wrong default:/new(remote-bridgecreateSession/sendPromptsend no model)The desktop chat hides this because the renderer sends its per-workspace selection as an explicit
input.model.Related Issue
No issue. Surfaced while debugging the mobile-companion work (#1339) — a Telegram
/newused the wrong model. This is an independent opencode-core gap, so it ships as its own PR.Human Review Status
Pending
Review Focus
recent: true) calls the endpoint — mirroring the renderer's ownrecentsemantics.model.cycle(norecent) and every prompt path do not. Is the picker the right and complete single source of truth?recordRecentusesFlock.withLock+ read-modify-write becausewriteJsonis not atomic; it preservesfavorite/variantand treats only a missing file (ENOENT) as empty — a parse/permission failure skips the write rather than clobber sibling state.Risk Notes
defaultModel()'srecentbranch was designed for. Intentional; matches upstream semantics.sdk.gen.ts/types.gen.tsgain only the newrecordRecentmethod + its types (+77 lines, additive). Running the full generator also surfaces ~640 lines of pre-existing drift between dev's committed SDK and its server (e.g.roots?: boolean→"true" | "false"); that drift is unrelated and would change other endpoints' caller-facing types, so it is deliberately excluded — the additions here match what the generator emits for this endpoint.state/model.json(locked, best-effort, sibling fields preserved)..catch(() => {})); a failure never disrupts the pick.Global.Path.state.How To Verify
Screenshots or Recordings
N/A — no visible UI change.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit