Skip to content

fix(ui): remember the engine pick on the GitHub and Inbox hand-offs (#906) - #907

Open
sapersky wants to merge 3 commits into
open-mercato:mainfrom
sapersky:fix/issue-906-remember-engine-pick
Open

sapersky wants to merge 3 commits into
open-mercato:mainfrom
sapersky:fix/issue-906-remember-engine-pick

Conversation

@sapersky

Copy link
Copy Markdown
Contributor

Closes #906

🎯 Goal

An explicit engine pick made on the GitHub tab's hand-off panel or an Inbox card is remembered, exactly like the composer's, so choosing auto stays auto across reloads and item hops. The symptom this removes: a user who picked auto on every single hand-off still had every run pinned to the model named in their coding agent's own settings file — silently the most expensive one available, at real quota and money cost.

🔍 Problem

defaultModels is seeded server-side from the coding agent's native settings (~/.claude/settings.json and its siblings), so "the user has not chosen a model" is not a neutral state — it hands the choice to that file. Both start surfaces outside /new held their engine pick in unpersisted useState, so every mount put them back into exactly that state. The pick was not merely forgotten in the UI: engineBody sent the resolved native model as an explicit model field, so the pin was real. Separately, Settings → Agents could not express an explicit auto either, so the same override could not be neutralized there — the select snapped straight back to the native model.

🔍 Root Cause

Two defects, one cause: null ("never touched") was the only state these surfaces could be in after a mount, and null is the state that lets the native default win.

  1. github.tsx held useState<EnginePick>({runner: null, model: null, account: null}) while its sibling workflow/skills state hydrated from and wrote back to hand-to-agent-draft.ts — whose FollowupSelection carried no engine fields. inbox.tsx had the identical unpersisted state. useResolvedEngine then called resolveModel(null, …, defaultModels, …), which by its own documented contract ("only null means never touched") returns defaults[runner], and engineBody put that on the wire. The composer escaped this only because its pick lives in a persisted draft (new-task-draft.ts).
  2. agents-section.tsx sent event.target.value || null, PUT /api/config treats null/'' as delete the key, and configAnswer then let the native value show through again. There was no way to say "ignore the native default" short of hand-editing the vendor's file.

What Changed

  • packages/web/src/routes/github/hand-to-agent-draft.tsFollowupSelection gained runner and model, normalized per-key in the store's existing tolerant stance (an unknown runner or a non-string model degrades to null, and the pre-Fix: GitHub/Inbox hand-off forgets the model pick and silently pins the native default model #906 two-field shape still reads as "engine never touched"). '' round-trips as '': keeping an explicit auto distinct from never-touched is the fix, so collapsing them would leave the bug in place. writeFollowupSelection became a read-modify-write merge — the same stance PUT /api/config takes on the raw config.json — because the workflow pick and the engine pick now share one key under separate owners, and a full overwrite would let whichever effect ran last erase the other's choice.
  • packages/web/src/components/engine-pills.tsx — the new useRememberedEnginePick / useSeededEnginePick hooks, plus the validity sweep. They live here because this file already exists so these two surfaces "cannot drift from the composer", and "is the pick remembered at all" was the one axis the shared component did not own — which is exactly where they drifted. The sweep drops a remembered runner that is not connected on this host, or a model modelConflictsWithRunner rejects for the resolved runner, rather than letting it ride a POST; it waits for provider status to load first, since an in-flight fetch is not evidence of absence. This is not theoretical — cockpits for different repos share one localhost:<port> origin and therefore this localStorage key. An explicit auto ('') never conflicts and always survives.
  • packages/web/src/routes/github/github.tsx — the hand-off panel's pick is now remembered on the same footing as its workflow pick.
  • packages/web/src/routes/inbox.tsx — the card seeds from the remembered pick without writing back. This is a deliberate asymmetry rather than an oversight, and the one place this PR departs from the issue's written plan: writing back would break #401's recorded rule that "run this one on codex must not silently re-aim the card below it", and persisting would make that leak global and survive reloads — strictly worse than the route state #401 already rejected. Seeding alone still removes the reported symptom here, because an untouched card no longer falls through to the native default. Both halves are pinned by tests so the asymmetry stays a decision.
  • packages/cezar/src/config.ts, packages/contract/src/workspace.ts, packages/cezar/src/server/server.ts — a new optional defaultModelsAuto (per-runner boolean). configAnswer answers '' for an overridden runner, layered over the native seed and under a repo preset, so naming a preset later simply wins. The fixed-model policy (modelsLocked) refuses the override on exactly the same terms as a preset, and only true is ever stored — false/null deletes the key rather than persisting something that means nothing.
  • packages/web/src/routes/settings/agents-section.tsx — picking auto now clears the preset and claims the override; picking a real model releases it.
  • BACKWARD_COMPATIBILITY.md — documents the new key and why it is a separate key rather than a sentinel.

🧪 Tests

  • npm run typecheck — clean. npm test — 6112 passed / 324 files. npm run test:unit — 35 passed. npm run build — ok (check:pack ok). npm run test:package — 15 passed.
  • 12 failures in npm test are pre-existing and environmental on the contributor's machine, verified identical on a clean checkout of this branch's base: TMPDIR resolves inside the repository here, so the "outside a git repository" / "non-repo dir" assertions cannot hold, plus one number formatted by a non-en-US locale and two load-related timeouts. None involve the files in this PR.
  • New regression coverage, each case verified to fail with its half of the fix removed:
    • hand-to-agent-draft.test.ts — the engine round-trip; '' vs null kept distinct (including in the stored JSON); the pre-Fix: GitHub/Inbox hand-off forgets the model pick and silently pins the native default model #906 two-field shape reading as never-touched; an unknown runner and a non-string model degrading to null; the merge write, so the engine and workflow picks cannot clobber each other.
    • github.test.tsx — an explicitly picked auto surviving a full cold remount on a host whose config reports a native default, with no model on the POST; both sweep branches (a dead runner omitted from the request and cleared from the store; a foreign-backend model cleared from the store).
    • inbox.test.tsx — a remembered explicit auto beating the configured default and leaving no body on the wire (gated on a post-config signal, so it cannot pass on the pre-load state); and the #401 guard, which fails if the Inbox is made to persist.
    • config-api.test.ts — the auto override beating a native default and answering ''; the per-runner merge storing only a real opinion; the modelsLocked refusal writing nothing at all; a config written before this change round-tripping untouched.
    • agents-section.test.tsx — picking auto claims the override, naming a model releases it.

💥 Breaking Changes

  • None. GET/PUT /api/v1/config is a protected surface (BACKWARD_COMPATIBILITY.md §2) and this change is strictly additive: one new optional key, absent by default, ignored by older readers, with no change to the meaning of defaultModels or of null/'' on it. Answering '' needed no response-schema change because runnerModelsSchema already accepts it, and '' already is auto in every model picker — so an older cockpit reading a newer server shows auto rather than meeting an unknown sentinel. A newer cockpit against an older server simply fails to persist the override, with no error.
  • The cez-followup-selection localStorage key gains fields; normalizeSelection still accepts the old two-field shape, so an existing install upgrades silently.
  • Confirmed no run-launch path reads defaultModels, so what auto means at run time (no --model flag; the agent CLI decides) is untouched.

…pen-mercato#906)

The GitHub tab's hand-off panel and the Inbox card held their engine pick in
plain `useState`, so every mount reset it to "never touched" — and that is
precisely the state `resolveModel` resolves to `defaultModels[runner]`, which
`configAnswer` seeds from the coding agent's OWN settings file. A user who
picked `auto` on every hand-off still got every run pinned to whatever
`~/.claude/settings.json` names, and `engineBody` sent it as an explicit
`model`, so the pin was real rather than cosmetic.

The pick now lives in the `cez-followup-selection` store that already remembers
the workflow and skills pick, behind two hooks in `engine-pills.tsx` — the file
that exists so these surfaces cannot drift from the composer. `null` still
means "never touched" and an explicit auto is `''`; collapsing the two is the
bug, so the store round-trips `''` verbatim and `writeFollowupSelection` became
a merge write, letting the engine and the workflow pick share one key without
clobbering each other. The GitHub tab persists; the Inbox seeds from the store
without writing back, which fixes the same symptom there while preserving
open-mercato#401's rule that a pick on one card must not re-aim the card below it. A
validity sweep drops a remembered runner this host cannot honour, or a model
belonging to another backend, rather than letting it ride a POST — the pick is
remembered in a localStorage key that cockpits for different repos share.

Settings -> Agents could not express an explicit auto at all: clearing the
preset was indistinguishable from never setting one, so the native default
showed straight back through and the select snapped back to it. A new optional
`defaultModelsAuto` key fixes that additively — `configAnswer` answers `''` for
an overridden runner, which every model picker already reads as auto, so no
response shape changes and older readers are unaffected. It is deliberately its
own key rather than an "auto" sentinel inside `defaultModels`, which an older
cezar would pass straight to --model.
@sapersky

sapersky commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

🤖 om-open-pr — 🏷️ label rationale

Updated in place by om-auto-review-pr after the review verdict.

I do not have label permissions on this repository, so none of these could be applied — they are recorded here for a maintainer, and this single comment is the current intended set.

  • 🚀 merge-queue — the review is done and the verdict is approve: no blockers, no remaining majors. It replaces review, which described the state before the verdict. (The merge is still gated: see needs-qa below, and GitHub forbids self-approval so a maintainer's approving review is required regardless.)
  • 🐛 bug — it removes broken behavior in shipped code: an explicit, visible user selection was discarded on every mount and the run was then pinned to a model the user did not choose. It adds one small optional config key, but only in service of that fix, so the category stays bug rather than feature.
  • 📸 needs-qa — the change is user-facing on three surfaces (the GitHub hand-off panel, the Inbox card, and Settings → Agents) and its whole point is memory across mounts, which unit tests can only approximate: the interesting failures appear when you leave a surface and come back. Manual routes are in the QA-instructions comment below. skip-qa would be wrong here, and I am deliberately not adding qa-approved — that is earned by a QA pass, not by reading the diff.
  • 🔹 priority-medium — per this repo's scale priority-high is for release-blocking defects and this is not one: cezar runs the task correctly either way. Its real cost is silent quota and money burn on the most expensive available model, plus a preference the user must re-set on every hand-off.
  • 🟡 risk-medium — the client half is contained (two route components, one localStorage store, one shared component), but the fix also touches the GET/PUT /api/v1/config answer, which BACKWARD_COMPATIBILITY.md lists as a protected surface. That change is strictly additive and I verified against the live Exact contract-parity assertion that the response type did not widen — but it is not low blast radius. The shared localStorage key is the other reason: it is shared across repos on one localhost origin, which is why the validity sweep exists.

Not applied and why: no review (superseded by merge-queue), no qa (that means "manual QA in progress" and belongs to the QA reviewer), no changes-requested / blocked / do-not-merge (nothing blocks), no in-progress (the chain's lock on this PR is comment-based only, for the same permission reason).

@sapersky

Copy link
Copy Markdown
Contributor Author

🤖 om-open-pr — chain lock moved from #906 to this PR at 2026-08-19T07:52:35Z; om-auto-review-pr continues here. Other auto-skills should skip this PR until the lock is released.

The assignee and the in-progress label could not be set (no write permission on this repository), so this comment is the only available lock signal.

@sapersky

Copy link
Copy Markdown
Contributor Author

🤖 om-auto-review-pr taking over the chain lock on this PR at 2026-08-19T07:53:08Z (inherited from om-open-pr). Reviewing now; the lock is retained while the chain continues.

open-mercato#906)

Review follow-up. "Nothing is connected" and "provider status has not answered
yet" are indistinguishable inside the sweep, so a version that treats an empty
runner list as evidence clears the remembered pick on every mount — and, because
the GitHub surface persists, writes the `null` back to the store. That destroys
the choice permanently and quietly reintroduces open-mercato#906. Removing the `loaded`
guard did fail a test already, but only obliquely: it tripped the
foreign-backend-model case, whose name tells a future maintainer nothing about
why the guard exists. AGENTS.md asks for exactly this pairing — a fail-open
pass-through needs the empty/absent-input case pinned, or it lies.

Also type `autoModelOverrides` explicitly. `Object.fromEntries` answers with an
index signature, and `configAnswer`'s return type is asserted `Exact` against
`configResponseSchema`; the current spread order happens not to leak it, but
reordering would widen `defaultModels` past the contract.

@sapersky sapersky left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Code Review

🎯 Summary

PR #907 fixes #906 on two fronts. The primary defect: the GitHub tab's hand-off panel and the Inbox card held their engine pick in unpersisted useState, so every mount reset it to null — and null is precisely the state resolveModel resolves to defaultModels[runner], which configAnswer seeds from the coding agent's own settings file. The pick was not merely forgotten; engineBody sent the resolved native model as an explicit model, so a user who chose auto every time still had every run pinned. The secondary defect: Settings → Agents could not express an explicit auto at all, because clearing the preset was indistinguishable from never setting one.

The fix is well-placed. The pick moves into the cez-followup-selection store that already remembers the workflow and skills pick, behind hooks in engine-pills.tsx — the file whose own doc block says it exists so these surfaces "cannot drift from the composer", and "is the pick remembered at all" was exactly the axis it did not own. The null vs '' distinction is preserved deliberately and tested explicitly, which matters because collapsing the two would leave the bug in place. writeFollowupSelection becoming a read-modify-write merge is the right call now that two owners share one key.

Two things I verified rather than assumed, because they are this repository's load-bearing invariants. First, the contract-parity guard: configAnswer's return type is asserted Exact against configResponseSchema, and I confirmed the guard is live by adding a stray response field and watching contract-parity.workspace.test.ts fail with "schema-is-wider" — so the new '' answer genuinely does not widen the response. Second, defaultModelsAuto needed to reach the contract (not just server.ts) because SetConfigInput is the type the cockpit's putConfig and agents-section.tsx compile against; it does.

Verdict: approve — no blockers and no remaining majors. Two findings surfaced during review and both were fixed in-review in 58fe2ef1 (this PR is the automation's own, run with --autofix), so neither blocks. Note that GitHub does not permit approving one's own pull request, so this is submitted as a review comment: a maintainer's approval is still required, and the merge-queue / needs-qa labels could not be applied for lack of write permission.

🧪 Validation Gate

Command Result
npm run typecheck ✅ pass — 0 errors across contract, api-client, server and web
npm test ✅ pass — 6122 passed / 321 files (see the environment note below)
npm run test:unit ✅ pass — 35 passed, 0 failed
npm run build ✅ pass — check:pack ok — 475 files, 85 under web/dist
npm run test:package ✅ pass — 15 passed, 0 failed

Environment note, stated plainly because the gate rule allows no excuses and I therefore did not offer one. A first npm test run on the contributor's machine showed 12 failures. Rather than wave them off as pre-existing, I identified and demonstrated the cause: TMPDIR was …/cezar/.ai/cezar/tmp/<taskId>, which git rev-parse --show-toplevel resolves to inside the repository — so every assertion of the form "outside a git repository" / "not a git worktree" / "non-repo dir" cannot hold — and LANG was pl_PL.UTF-8, which formats 20000 as 20 000 and breaks a toContain('20,000') assertion. Re-running the four affected files with TMPDIR=/tmp LANG=en_US.UTF-8 turned all 137 of their tests green, and the full suite dropped from 12 failures to 3. Those last 3 are load-related timeouts (~4–5s each) whose membership changes between runs; all pass in isolation. None of the 12 touches a file in this diff, and all reproduce identically on the unmodified base commit. CI remains the authority — only license/cla (pass) has reported so far.

🟠 Major

1. packages/web/src/components/engine-pills.tsx:198 — the sweep's fail-open guard had no test naming it. Fixed in-review (58fe2ef1).

useEnginePickValidity cannot distinguish "no backend is connected" from "provider status has not answered yet" — both are an empty runners array. The loaded guard is what stops the second case being treated as evidence, and it is load-bearing: without it a remembered pick is cleared on every mount and, because the GitHub surface persists, the null is written straight back to the store. That destroys the user's choice permanently and quietly reintroduces #906, and it also turns an unreachable agent CLI — a documented degradation path, priority 2 in CODE_REVIEW.md — into silent state loss.

Removing the guard did already fail one test, but only incidentally: it tripped the foreign-backend-model case, whose name would tell a future maintainer nothing about why the guard exists. AGENTS.md is explicit here — "a fail-open helper needs a populated-input guarantee, or it lies… pair every silent pass-through with a test that pins the empty/absent input case."

Fix applied: a test asserting that with PROVIDERS_NONE the remembered runner and model both survive, gated on the Run button becoming disabled so it cannot pass on a pre-query render. Verified to fail when the loaded guard is removed.

🔵 Minor

2. packages/cezar/src/server/server.ts:5032autoModelOverrides relied on inference where the contract is asserted Exact. Fixed in-review (58fe2ef1).

The helper returned Object.fromEntries(...), whose type carries an index signature. It is spread into configAnswer's defaultModels, and that object's type is asserted Exact against configResponseSchema. The current spread order happens not to leak the index signature — I confirmed the assertion passes — but that is a property of the ordering, not of the code: moving autoModelOverrides after the preset spread could widen defaultModels past the contract, and the failure would surface as an opaque type error in a parity test far from the change. Now annotated : AgentModelDefaults, which is the shape it actually produces.

⚪ Nits

3. packages/web/src/components/engine-pills.tsx:15 — a shared component imports from a route directory. @/routes/github/hand-to-agent-draft is route-local by path, and engine-pills.tsx is a shared component. I checked before flagging: the file already imports @/routes/new-task-form for the whole resolution quartet, so the direction is established precedent rather than something new, and hand-to-agent-draft.ts is a React-free pure store, so nothing route-shaped is pulled into the shared chunk. The alternative — duplicating the store or hoisting it to @/lib — would either fork the cez-followup-selection key or churn a file with its own passing test suite for no behavioral gain. Recording it so the next reader knows it was a decision.

4. packages/web/src/components/engine-pills.tsx:141 — the persistence effect writes the seed back on first mount, a no-op localStorage write of values just read from it. Harmless, and guarding it would cost more code than it saves.

5. packages/web/src/components/engine-pills.tsx:198 — the sweep still evaluates while modelsLocked is on, so it can clear a remembered model that the locked host was ignoring anyway. The user's stored preference would otherwise return when the lock is lifted. Genuinely trivial; worth a thought only if modelsLocked ever becomes common.

💥 Breaking Changes

  • Exported APIs — no export removed or renamed. FollowupSelection gains two fields and writeFollowupSelection widens its parameter to Partial<…>, which is accepting-more, not breaking. Both are internal to the cockpit bundle, not a published entry point.
  • HTTP routes and response shapesGET/PUT /api/v1/config is listed as a protected surface in BACKWARD_COMPATIBILITY.md §2, and the change is strictly additive: one new optional request key. The response shape is unchanged — runnerModelsSchema already accepted '', so answering it needed no schema edit, verified by the live Exact parity assertion. No route added, so no bc-route-inventory entry is due.
  • Semantics of existing keysdefaultModels and the meaning of null/'' on it are untouched. The override is deliberately a separate key rather than an "auto" sentinel inside defaultModels, because an older cezar would pass such a value straight to --model. An older cockpit reading a newer server sees '', which already means auto in every picker; a newer cockpit against an older server simply fails to persist the override, with no error.
  • Config formatsconfig.json gains one optional key with .optional().catch(undefined), matching its siblings, so a malformed value degrades per-key and old files parse unchanged. BACKWARD_COMPATIBILITY.md is updated in the same commit, including the rationale for a separate key.
  • Persisted client state — the cez-followup-selection localStorage key gains fields; normalizeSelection still accepts the old two-field shape, pinned by a test, so an existing install upgrades silently.
  • Run-time behavior of auto — confirmed by search that no run-launch path reads defaultModels; it is purely a cockpit preselect. What auto means when a run starts (no --model flag, the agent CLI decides) is unchanged, as #906 required.
  • Graceful degradationlocalStorage unavailable still degrades to "nothing remembered" on both read and write, including the new merge-read inside writeFollowupSelection.

🧪 Test Coverage

Coverage is strong, and unusually well-targeted for this kind of fix. Nine cases across five files, and — following AGENTS.md's "prove the regression test fails without the fix" rule — I verified each one bites in the right direction rather than trusting that it does:

  • hand-to-agent-draft.test.ts — the engine round-trip; '' kept distinct from null including in the stored JSON; the pre-#906 two-field shape reading as never-touched; an unknown runner and a non-string model degrading to null; and the merge write, so the engine and workflow picks cannot clobber each other.
  • github.test.tsx — an explicit auto surviving a full cold remount on a host whose config reports a native default, with no model on the POST; both sweep branches; and the new unanswered-provider guard.
  • inbox.test.tsx — a remembered auto beating the configured default with no body on the wire, and the #401 guard that a card's pick never reaches the store.
  • config-api.test.ts — the override beating a native default and answering ''; the per-runner merge storing only a real opinion; the modelsLocked refusal writing nothing at all; and an old-shaped config round-tripping untouched.
  • agents-section.test.tsx — picking auto claims the override, naming a model releases it.

Two details worth crediting, because both are the failure mode this repo warns about. The Inbox assertion is deliberately gated on a post-config signal (the default runner arriving) — an earlier draft asserted "the pill shows auto" and passed on the pre-load render, green either way, which is exactly the green-either-way test AGENTS.md calls out. And the #401 no-write-back guard was verified to fail if the Inbox is switched to the persisting hook, so the asymmetry between the two surfaces is pinned as a decision rather than left to a comment.

The one deliberate departure from #906's written plan — the Inbox seeds from the store but never writes back — is argued in the PR body and in the code, and is the right call: writing back would break #401's recorded rule that a pick on one card must not re-aim the card below it, and persisting would make that leak global and survive reloads, which is strictly worse than the route state #401 already rejected. Seeding alone still removes the reported symptom there.

@sapersky

Copy link
Copy Markdown
Contributor Author

🧪 Manual QA instructions (needs-qa)

This PR is approved on review (as a comment — GitHub forbids self-approval; a maintainer's approving review is still required) and it wants manual QA. I could not apply any labels on this repository, so treat this as the intended state: merge-queue + needs-qa, with the QA-approval gate (qaGate: true) holding the merge until qa-approved is added.

The whole point of this fix is memory across mounts, which no unit test can fully stand in for — the interesting failures only appear when you leave a surface and come back. Every route below needs a host whose coding-agent settings name a model, otherwise the bug being fixed is invisible: put "model": "opus" in ~/.claude/settings.json (or set a per-runner default in Settings → Agents) before you start, and confirm Settings → Agents shows it.

P1 — GitHub tab hand-off: the model pill remembers auto

Where to click

  • /p/<project>/github → pick any issue or PR → the hand-off panel at the bottom of the detail pane

What to verify

  • The Model pill initially shows the model from your agent settings (opus) — that is the pre-fix starting state, and it is still correct for an untouched pick.
  • Set the Model pill to auto. Now hop to a different issue in the list → the pill must still read auto (it used to snap back to opus).
  • Hard-reload the page and reselect an issue → still auto.
  • Hit Run agent on this issue while auto is selected, then open the created task and check its model: it must not be pinned to opus. In .ai/cezar/runs.json the record's "model" should be null, which is the observable proof the pin is gone.
  • Pick a real model (e.g. sonnet) instead, hop away and back → sonnet is remembered too, and a run started from it is pinned to sonnet.

What can go wrong

  • The pill reads auto but the run is still pinned — that would mean the display and the request body disagree, the exact split this fix removes.
  • The workflow or skills pickers forgetting their selection after you touch the engine pill (or vice versa): these now share one storage key, so a clobbering bug would show up here first. Set a workflow and a model, reload, and confirm both survived.

P1 — Inbox card: seeded, but deliberately not sticky

Where to click

  • /p/<project>/inbox (needs at least two runnable cards; CEZ_FOLLOWUPS=1 if the Inbox is gated off)

What to verify

  • With auto remembered from the GitHub tab, a freshly loaded Inbox card's Model pill reads auto, not opus, and ▶ Run sends no model.
  • Now change one card's Runner or Model pill. The card below it must not move — this asymmetry is intentional (feat: follow-up flow — skill picker, editable suggested prompt, model and runner selection #401) and is the one place this PR knowingly departs from the issue's written plan.
  • Reload. The card you re-aimed is back to the remembered pick, not the one-off choice you made on it.

What can go wrong

  • A pick made on one card leaking to its neighbours, or surviving a reload — that is the regression this design specifically avoids.

P1 — Settings → Agents: auto (default) stops snapping back

Where to click

  • /p/<project>/settings/agents → the per-runner Default model selects

What to verify

  • Choose auto (default) for claude. It must stay on auto after the save and after a full page reload — previously it jumped straight back to the model in your settings file.
  • Choose a real model, then auto again, then a real model: it should track your choice each time.
  • Confirm the other runners' selects are untouched by a claude change (the write merges per runner).
  • Check .ai/cezar/config.json: picking auto should leave "defaultModelsAuto": {"claude": true} and no defaultModels.claude. Picking a model should do the reverse — the two must never both be set for one runner.

What can go wrong

  • A stored "auto" string inside defaultModels — that shape is deliberately avoided, because an older cezar would pass it to --model verbatim. Seeing it means the wrong mechanism shipped.
  • With CEZ_AGENT_MODELS_LOCKED=1 (or modelsLocked: true), the selects must be read-only and any attempt to save a model or an auto override must be refused with a 409 — not silently accepted.

P2 — Degradation and upgrade paths

What to verify

  • Unreachable agent CLI: with no provider connected (quit the agent CLI / disconnect in Settings), load the GitHub tab. Your remembered pick must still be there when providers come back — an unanswered provider status must never wipe it. This is the case the review added a test for.
  • Private browsing / storage blocked: the pickers must still work for the session, just without memory — never an error or a blank panel.
  • Upgrade: with an existing install that has a cez-followup-selection entry from before this change (two fields only), the tab must load normally and simply treat the engine pick as untouched.
  • Cold-load perceived performance on /github and /inbox: the shell and loading state should appear as before, with no new flash of the wrong model in the pill before it settles.

Pass/fail

  • All routes pass → remove the qa label and add merge-queue plus qa-approved (this clears the QA-approval gate)
  • Any route fails → remove the qa label, add qa-failed, and leave a comment describing the failure.

@sapersky

Copy link
Copy Markdown
Contributor Author

🤖 om-auto-review-pr completed at 2026-08-19T08:06:14Z: APPROVED (submitted as a review comment — GitHub forbids self-approval, so a maintainer's approving review is still required). No blockers and no remaining majors; the two findings raised during review were fixed in-review in 58fe2ef1.

Lock retained — chain continues (om-auto-qa-pr next, for the UI-touching surfaces). The label set could not be applied for lack of write permission; the intended set is recorded in the 🏷️ label rationale comment above.

@sapersky

Copy link
Copy Markdown
Contributor Author

🤖 om-auto-review-pr — CI result

No CI has run, and none can until a maintainer releases it. As of 2026-08-19T08:10:39Z there are zero workflow runs for the head SHA 58fe2ef1 (GET /actions/runs?head_sha=… is empty, and /check-runs likewise). ci.yml does trigger on pull_request targeting main, so this is the fork-PR approval gate, not a misconfiguration: the run needs "Approve and run workflows" from someone with write access. The only reported status is license/cla (pass — the commits are authored with the CLA-linked address).

I waited a bounded interval and re-checked rather than assume; the result was unchanged. No further CI follow-up will come from this agent.

Standing in for CI, here is the full validation.commands gate run locally on 58fe2ef1this is evidence, not a substitute for branch protection; the required checks still gate the merge:

Command Result
npm run typecheck ✅ 0 errors (contract, api-client, server, web)
npm test ✅ 6122 passed / 324 files
npm run test:unit ✅ 35 passed, 0 failed
npm run build check:pack ok — 475 files, 85 under web/dist
npm run test:package ✅ 15 passed, 0 failed

One caveat stated openly, because it affected the numbers. On this machine npm test first reported 12 failures. They are environmental and I demonstrated it rather than asserting it: TMPDIR was set to …/cezar/.ai/cezar/tmp/<taskId>, which git rev-parse --show-toplevel resolves to a path inside this repository, so every assertion of the form "outside a git repository" / "not a git worktree" / "non-repo dir" is unsatisfiable; and LANG=pl_PL.UTF-8 renders 20000 as 20 000, failing a toContain('20,000') check. Re-running the four affected files with TMPDIR=/tmp LANG=en_US.UTF-8 turned all 137 of their tests green and took the full suite to the numbers above. The 3 that still fail in a full parallel run are ~4–5s timeouts whose membership changes between runs and which pass in isolation; none touches a file in this diff, and all reproduce identically on the unmodified base commit. A CI runner has a normal TMPDIR and a C/en_US locale, so none of this should appear there — but that is a prediction, and the real run is the one that settles it.

@sapersky

Copy link
Copy Markdown
Contributor Author

🤖 om-auto-qa-pr taking over the chain lock on this PR at 2026-08-19T08:11:21Z (inherited from om-auto-review-pr). Running UI verification on the changed cockpit surfaces; the lock is retained while the chain continues.

@sapersky

Copy link
Copy Markdown
Contributor Author

📸 UI verification — PASS

om-auto-qa-pr drove the changed cockpit surfaces in a real Chrome via agent-browser 0.33.2 against a production build of this PR's head (58fe2ef1), booted by .ai/scripts/test-env-up.sh with CEZ_DRY_RUN=1. 13 checkpoints, all PASS, with two checks honestly not exercised (listed at the end). Evidence-only run: no labels were changed, and qa-approved was not applied.

The precondition matters, so I verified it rather than assumed it. This host's ~/.claude/settings.json really does contain "model": "opus[1m]", and with no .ai/cezar/config.json at all the running server answered GET /api/v1/configdefaultModels: {"claude":"opus[1m]","codex":"gpt-5.6-sol"}. That is #906's exact reported condition — the native file, and nothing else, choosing the model — reproduced live before any assertion below.

P1 — GitHub hand-off: the pick is remembered

# Action Observed
01 Open the hand-off with an empty remembered selection Pill = opus[1m]; store {runner:null, model:null} ✅ correct for an untouched pick, and the state the bug fed on
02 Pick auto Pill = auto; store model: "" ✅ an explicit auto, stored distinctly from never-touched
03 Hop to issue #139 (panel remounts) Pill = auto ✅ pre-fix this reverted to opus[1m]
04 Full page reload Pill = auto; store model: "" ✅ the reported symptom, gone
05 Run agent on this issue GET /api/v1/runs{'model': None, 'runner': 'claude'} ✅ the decisive one — see below

Checkpoint 05 is the one that proves the fix is real rather than cosmetic. #906's evidence was that .ai/cezar/runs.json recorded "model": "opus[1m]" for GitHub-tab runs while /new recorded "model": null. A run started here from the GitHub tab with auto selected recorded "model": null — the split the issue described is closed, measured on the same surface with the same data shape.

P1 — Settings → Agents: auto (default) stops snapping back

# Action Observed
06 Open Settings → Agents claude = opus[1m], codex = gpt-5.6-sol, opencode/pi = auto; no config.json ✅ defect 2's starting state
07 Choose auto (default) for claude Select stays on auto; config.json = {"defaultModelsAuto":{"claude":true}}; API → defaultModels: {"claude":"","codex":"gpt-5.6-sol"}
08 Reload the page Still auto
09 Choose sonnet instead config.json = {"defaultModels":{"claude":"sonnet"}}, defaultModelsAuto removed ✅ the two are never both set

Two details worth naming. The stored shape is the additive defaultModelsAuto key — there is no "auto" sentinel inside defaultModels, which is the shape an older cezar would have passed straight to --model. And codex kept its own native default (gpt-5.6-sol) throughout, so the per-runner merge really is per-runner.

P1 — Inbox: seeded from the remembered pick, still per-card

# Action Observed
10 Open the Inbox with the native default at opus[1m] and config.json = {} Both cards show auto ✅ the remembered pick beats the native default here too
11 Re-aim card 1 to codex card 1 = codex / GPT-5.6-Sol; card 2 unchanged (claude / auto); store untouched (runner:null, model:"") #401 holds

Checkpoint 11 is the deliberate asymmetry this PR argues for, confirmed in a browser: the Inbox reads the remembered pick but never writes to it, so re-aiming one card cannot re-aim its neighbour — and nothing that happened on a card leaked into storage for the next reload to inherit.

P2 — Perceived performance and the untouched-behavior guard

# Action Observed
12 Cold-load the changed route domContentLoaded 29ms, loadEvent 29ms; pill reads auto on first paint with no flash of the native model
13 Open in a brand-new browser profile (empty localStorage) Pill = opus[1m] ✅ a new user with no remembered pick still gets the configured default

Checkpoint 13 is a guard rather than a fix: it pins the behavior this PR deliberately did not change. Remembering a pick must not stop an untouched surface honouring the configured default.

⚠️ Not exercised — stated plainly, not folded into the pass

  • Mobile viewport smoke. Not attempted-and-passed; not attempted. agent-browser 0.33.2 has no viewport or device command — it answers Unknown command: viewport, and an earlier attempt that appeared to resize in fact left innerWidth at 1280, so the screenshot it produced was desktop-width and I deleted it rather than present it as mobile evidence. A mobile pass on these surfaces still needs a human or a provider that can set a viewport.
  • The remembered pick surviving an unanswered provider status. This needs /api/v1/providers/status stalled, which the live env cannot do. It is covered by the unit test added in 58fe2ef1 during review, verified to fail when the loaded guard is removed — but it is test coverage, not browser evidence, and I am not claiming otherwise.

Environment

Booted via .ai/scripts/test-env-up.sh (production build, CEZ_DRY_RUN=1, agent CLIs mocked) at http://127.0.0.1:54811. One deviation to disclose: the Inbox is capability-gated off by default (/api/v1/health reported followups: false, and /api/v1/todos returns [] regardless of file contents while it is off), so for checkpoints 10–11 I restarted the app with the descriptor's own start command plus CEZ_FOLLOWUPS=1 and seeded two runnable todos.json fixtures. Everything else ran on the stock environment. Both the fixtures and the environment are torn down with this run; source files were not touched.

Evidence hosting

The screenshots are hosted on a dedicated, slash-free qa-evidence-pr907 branch of the fork (sapersky/cezar) rather than of this repository, because I have no write access here. The fork is public, so the raw.githubusercontent.com URLs render inline exactly as they would otherwise — a better outcome than the links-only fallback, and it keeps evidence off this change's own branch, which is the rule that matters. The full artifact set (13 PNGs plus report.json and report.md) is also on disk at worktrees/09c4c396-f394-451d-ae3e-a3170287b2c9/.ai/qa/artifacts_pr907.

Screenshots

step-01-untouched-shows-native-opus

step-01-untouched-shows-native-opus.png

step-02-picked-auto

step-02-picked-auto.png

step-03-auto-survives-item-hop

step-03-auto-survives-item-hop.png

step-04-auto-survives-full-reload

step-04-auto-survives-full-reload.png

step-05-run-started-with-auto

step-05-run-started-with-auto.png

step-06-settings-shows-native-opus

step-06-settings-shows-native-opus.png

step-07-settings-auto-sticks

step-07-settings-auto-sticks.png

step-08-settings-auto-survives-reload

step-08-settings-auto-survives-reload.png

step-09-real-model-releases-override

step-09-real-model-releases-override.png

step-10-inbox-cards-seeded-with-auto

step-10-inbox-cards-seeded-with-auto.png

step-11-inbox-per-card-pick-not-shared

step-11-inbox-per-card-pick-not-shared.png

step-12-cold-load-desktop

step-12-cold-load-desktop.png

step-13-fresh-profile-still-honors-native-default

step-13-fresh-profile-still-honors-native-default.png

@sapersky

Copy link
Copy Markdown
Contributor Author

🧪 Follow-up: a browser-level test this change does not yet ship

HAS_UI_TEST=false — the PR adds thorough component/unit coverage (vitest, jsdom) but no test under packages/web/e2e/, and the behavior it fixes is memory across a real page reload, which jsdom can only approximate. settings-agents.e2e.ts and new-task.e2e.ts already cover neighbouring ground, so there is an obvious home for it. Recording the scenario so a follow-up run can implement it via om-integration-tests; not filing an issue unless a maintainer wants one.

Suggested file: packages/web/e2e/engine-pick-memory.e2e.ts (runner: packages/web/e2e/vitest.config.ts, driven by the existing packages/web/e2e/agent-browser.ts harness).

Fixture requirement — the part that makes or breaks the test. The whole defect only exists when defaultModels is non-empty, and on a real host that value is seeded from ~/.claude/settings.json. A test that runs against an empty config would pass no matter which way the code went — the exact green-either-way trap AGENTS.md warns about. So the test must first establish a per-runner default it can then override, either by pointing HOME at a fixture directory containing a .claude/settings.json with {"model":"opus"}, or by PUT /api/v1/config with {"defaultModels":{"claude":"opus"}} during setup. Assert the precondition (GET /api/v1/config reports it) before asserting anything else.

Scenario

  1. Establish the default per above; open /p/<boot>/github and select an issue. Assert the Model pill reads opus — the untouched-pick state.
  2. Open the Model pill, choose auto. Assert the pill reads auto and localStorage['cez-followup-selection'] parses to model: "" — the empty string specifically, since null is what the bug produced.
  3. Reload the page, reselect an issue, assert the pill still reads auto. This is the assertion that would have caught Fix: GitHub/Inbox hand-off forgets the model pick and silently pins the native default model #906.
  4. Click Run agent on this issue; assert the created run's model is null via GET /api/v1/runs — proving the pick reached the request body, not just the pill.
  5. Re-aim the pill to a real model, reload, assert it is remembered too (so the test pins memory, not a hardcoded auto).
  6. /p/<boot>/settings/agents: choose auto (default) for claude, reload, assert it stayed; assert config.json gained defaultModelsAuto.claude === true and carries no defaultModels.claude.
  7. Inbox (needs CEZ_FOLLOWUPS=1 and seeded runnable todos.json — see the environment note in the QA comment above): assert a fresh card seeds to auto, then re-aim one card and assert both that its neighbour did not move and that localStorage was not written. That second half guards #401 and is the invariant most likely to be broken by a well-meaning "make the two surfaces identical" refactor.

Also worth covering, and not exercisable in this QA pass: stall or fail /api/v1/providers/status and assert the remembered pick is still intact afterwards. A unit test for this was added in 58fe2ef1, but an E2E version would cover the persisted round-trip that unit test cannot reach — and this is the failure mode that would silently destroy user state on every load.

@sapersky

Copy link
Copy Markdown
Contributor Author

🤖 om-auto-qa-pr completed: PASS (13/13 checkpoints, evidence-only — no labels touched, qa-approved deliberately not applied). Environment torn down; the QA fixtures (todos.json, a QA-created config.json) were removed and git status is clean, so nothing was left behind in the worktree.

🤖 om-auto-fix-issue run complete at 2026-08-19T08:21:38Z: #906 verified, root-caused, fixed, reviewed and UI-verified on this PR. Lock released.

Where this stands, and what needs a human. The work itself is done and self-consistent: two commits, the full local validation gate green, an approve verdict on review with both findings raised there fixed in-review, and browser evidence that the reported symptom is gone (a run started from the GitHub tab with auto selected now records "model": null where #906 measured "opus[1m]"). Three things I could not do from here, all permission-related rather than technical:

  • No approving review exists. GitHub forbids approving your own PR, so the review is a comment and reviewDecision still reads REVIEW_REQUIRED. A maintainer's approval is required.
  • No label could be applied. The intended set is merge-queue, bug, needs-qa, priority-medium, risk-medium, with reasoning in the 🏷️ label rationale comment. needs-qa should stay: my pass is agent-driven evidence, not the QA sign-off the gate wants, and two checks were genuinely not exercised (mobile viewport, and the stalled-provider case) — both named openly in the evidence comment rather than folded into the pass.
  • CI has never run. ci.yml triggers on pull_request, but no workflow run exists for 58fe2ef1 — the fork "Approve and run workflows" gate. The local gate stands in as evidence only; required checks still gate the merge.

Not merging and not adding qa-approved — both belong to the pipeline's gates, not to this chain.

One conflict, in `packages/web/src/components/engine-pills.tsx`, and only in its
import block. open-mercato#924 moved the agent-account lookup out of `useAgentProfiles`/`useRepo`
into the shared `@/api/agent-accounts` hook, while open-mercato#906 added the `useEffect`/`useState`
the remembered-pick hooks need. Both sides kept: main's account imports, plus the react
import this branch introduced. The bodies merged without overlap — the account logic and
the remembered-pick hooks touch different parts of the file.

typecheck, build and check:pack clean. The 13 `npm test` failures are the environmental
ones already documented in the PR body (TMPDIR resolves inside the repo here, so the
"outside a git repository" assertions cannot hold; two numbers formatted under pl-PL),
plus one from main that fails for the same locale reason.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix: GitHub/Inbox hand-off forgets the model pick and silently pins the native default model

1 participant