[#203] Let the user pick the Claude model (default stays Haiku) - #208
Conversation
The CLI tier was hard-pinned to Haiku: session.ts built engineConfig with no `model` field, so ClaudeCliEngine's `config.model` was always undefined and buildClaudeArgs fell through to DEFAULT_MODEL on every install. Adds a curated allow-list (the CLI's tier aliases haiku/sonnet/opus, not dated snapshot ids that age out) in the engine, mirrored by a Rust clamp and a webview mirror, and threads the pick through the start message into engineConfig so it reaches `--model`. Both CLI lanes share one config, so translate and extras always run the same model. The default stays Haiku, and needs no migration split unlike #202: before this change every install was already running Haiku, so "field absent" and "fresh install" describe the same running state. Nothing downloads either way — a heavier tier costs plan budget, not disk, which is what the picker copy says. Cost accounting is untouched and stays model-independent: the adapter reports whatever the CLI put in `total_cost_usd`, with no per-model rate table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deleting the one line that puts the model pick into engineConfig was caught by nothing — all 182 tests and typecheck passed, which is exactly the silent shape of the bug #203 fixes. Extracting that object into a pure buildCliEngineConfig lets the wiring be asserted without spawning a session; re-seeding the deletion now fails three tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Epic Alignment: PASS
PR #208 implements #203's curated Claude-model choice without changing the Haiku default, and preserves the Rust/web clamps, argv contract, and CLI-reported cost path required by the epic.
Checked (evidence)
- Structural gate: PR body includes EPIC Alignment, Self-Verification, Design Fidelity, and Deviations sections; the fidelity table matches the cited settings/onboarding implementation.
- Verified final chain: settings.rs/app-settings.ts clamps agree; session.rs → start-config.ts → buildCliEngineConfig carries the selected model; args.ts emits exactly one --model.
- Verified real-spawn e2e in packages/engine/test/claude-cli-engine.e2e.test.ts: default and opus runs differ only in --model, with identical usage accounting on the same reported stream.
- Verified settings-sheet.ts and onboarding.ts render the same claudeModelLabel and handle unknown values via the default.
- Riskiest part: the extracted buildCliEngineConfig seam; it isolates the load-bearing session wiring and both CLI lanes consume the same model.
- Kill-list: scanned the full diff — clean.
- CI: gh pr checks 208 → all five checks pass.
Findings
None.
Decision
The model selection is user-visible, clamped on both sides, reaches the spawned argv, and leaves Haiku as the default. Cost accounting remains CLI-authoritative with no rate table, and all live CI checks pass.
RE2 — APPROVEPR #208 (Fixes #203) @ Checked (evidence)1. The AC that matters — the pick reaches real argv — is proven the right way. 2. Your item 1 — the 3. The chain is complete and clamped at every hop. 4. Curated aliases, not snapshot ids — 5. No migration split, and the reasoning is right. Before #203 every install ran Haiku because 6. Cost: verified, not assumed, with an honest boundary. 7. The two surfaces cannot disagree by construction. Both onboarding and the sheet render through the same 8. Gates. app 185, engine 275, archive 110, both typechecks exit 0, Your item 2 — the alignment answer you asked for: complete in practice, with one real exceptionYou asked whether the clamps agree "genuinely, rather than just for the cases I thought of". The two TS clamps agree on every input — both use JS
So Why this is a note and not a blocker, stated explicitly since I blocked #199 over a single sentence: that one was a factual claim about published data that a conclusion rested on. This one has no reachable consequence and fails safe. Rust sanitizes on the way in from disk and is the stricter side, so a value-level BOM resolves to If you want it tightened, either soften the two comments to name the exception, or make the Rust side Notes
(The shared bot token cannot file a formal GitHub approval, so this comment plus my chat message is the RE2 verdict of record.) |
RE2 — correction to my own table (approval unchanged)@dev is right and my table was wrong. One row of the trim comparison in comment What I got wrong, and whyI reported U+0085 (NEL) as trimmed by both sides. It is not trimmed by JS. Re-measured by string length instead of by how the value prints: Cause: my first script printed The corrected picture — two divergences, opposite directions
The cause is two different definitions, as @dev states: ECMAScript This strengthens the non-blocking call rather than weakening itNeither divergence is reachable — Rust sanitizes on load and the start message carries an already-sanitized value, and the picker only emits curated values — and both fail safe in the same place: whichever side is stricter clamps to Withdrawing my suggested fixI proposed adding On the (a)/(b) ruling — @Head's call, and my position is (a)Merge as-is and fold the wording into #205. The code is correct and identical under either option; a comment-only push invalidates two evidence-bound approvals at (The shared bot token cannot file a formal GitHub approval, so this comment plus my chat message is the RE2 verdict of record.) |
Fixes #203
Lets the user choose which Claude model the CLI tier runs. The default stays Haiku, and no install changes behaviour until someone touches the picker.
EPIC Alignment
settings.rs↔app-settings.ts↔packages/engine/src/args.ts); the chosen model reaches Claude argv; the default remains Haiku; the CLI-reported cost stays authoritative — no per-model rate table added.616c919), which this branches from. Enables: [refactor] Make the budget/fallback seam engine-agnostic — hours-remaining as the primitive, USD as only one derivation #205.What was actually broken
src/host/session.tsbuiltengineConfigwith nomodelfield, soClaudeCliEngine'sconfig.modelwas alwaysundefinedandbuildClaudeArgsfell through toDEFAULT_MODELon every install. The Settings sheet advertised "Claude CLI · Haiku" as if it were a statement of fact; it was the only possible outcome.The chain
Settings sheet → AppSettings.claudeModel → settings.json → start message → resolveStartConfig (clamp) → buildCliEngineConfig → ClaudeCliEngine → buildClaudeArgs → --modelCurated picks are the CLI's tier aliases —
haiku/sonnet/opus— not dated snapshot ids. An alias keeps resolving to the current build of its tier; a pinned id (claude-3-5-haiku-20241022) eventually retires and 404s on an install nobody touched. Verified live: the three aliases resolved toclaude-haiku-4-5-20251001,claude-sonnet-5, andclaude-opus-5respectively.No migration split, unlike #202 — and that difference is the point. Before this PR the model was hard-pinned to Haiku with no way to change it, so "settings file predates #203" and "fresh install" describe installs that were both already running Haiku. One default covers both honestly. Nothing downloads either way: the model runs on Anthropic's side, so a heavier pick costs plan budget, not disk. (#202 needed the split precisely because its default moved and cost a 547 MB download.)
The cost interaction — verified, not assumed
The ticket asked me to verify that the reported cost field behaves identically on a non-default model. I ran the real CLI (
claude2.1.221) on all three, same trivial prompt,--output-format stream-json:--modelmodelUsage)total_cost_usdis_errorhaikuclaude-haiku-4-5-20251001sonnetclaude-sonnet-5opusclaude-opus-5Same field, same shape, same success semantics on every tier — only the value differs. That is exactly what the ledger needs:
stream-parser.ts:121readstotal_cost_usd,recordUsagederives a monotonic per-turn delta from it, andCreditAccountantaccumulates that. The CLI already prices per model, so a heavier model is tracked correctly because we have no rate table of our own to drift.The spread is also why the picker copy warns about budget rather than staying silent: ~3.6× Haiku→Sonnet and ~7× Haiku→Opus on identical input. Those numbers are evidence for this PR, deliberately not put in the UI or the code — that would be the rate table #203 forbids, and it would drift the moment pricing moves.
Control run, which also confirms why the clamp exists:
--model claude-3-5-haiku-20241022returnedapi_error_status: 404,is_error: true,total_cost_usd: 0,terminal_reason: "api_error". An unknown model does not fail loudly at startup — it produces a zero-cost error turn every turn, i.e. a dead translation lane. That is the failure the clamps prevent.Self-Verification
Tests:
pnpm test:app185 passed (182 + 3 new), engine 275, archive 110.pnpm lint,pnpm typecheck(both configs),./scripts/no-stub-gate.sh,./scripts/color-guard.sh→ all clean.The chosen model reaches the built argv, asserted on a real spawned process (AC: "not on a mock").
claude-cli-engine.e2e.test.tsruns the engine twice againstfake-cli.mjs, which records its ownprocess.argv:--model haiku;model: "opus"→ spawned with--model opus--modelreaches the CLIRust proven on Linux, not deferred to CI.
livecap-appcan't build here, so the settings surface was extracted verbatim into an offline cargo harness running realserde/serde_json— 17/17 checks: fresh install, absent field, all three curated picks, 7 junk/blank inputs, whitespace, corrupt file, camelCase wire key, and #202's STT migration still intact. Then both new#[test]bodies were copied verbatim into that harness and compiled —2 passed— so a type or syntax error surfaces here rather than inapp-macos.Seeded-violation proofs — every new guard confirmed load-bearing by reintroducing the bug:
claude-3-5-haiku-20241022survives into settingsstart-config.ts)start-config.test.ts— unclamped' sonnet 'reaches the configDEFAULT_MODELmoved off haikuclaude-model.test.tsmirror + clamp-agreement assertions (2 failures)buildClaudeArgsignoresoptions.modelargs.test.ts×2 and the spawned-argv e2emodel:line dropped from the engine configclaude-model.test.ts×3"haiku"claude-model.test.ts×3rustfmt --checkparity:settings.rs5 diffs andsession.rs13 — identical counts toorigin/main, so none are introduced here.Two defects this pass caught in my own work, fixed rather than documented as caveats:
" sonnet "would have clamped to Haiku on one side and passed assonneton the other. All three clamps now trim identically, with a test asserting the TS mirror and the engine clamp agree on every input including whitespace.engineConfig— the exact regression this ticket exists to fix — was caught by nothing: all 182 tests andtscpassed. That line was untestable becausesession.tsbuilds a live session (CLI detect, mkdir, two engines, accountant). I extracted the config object into a purebuildCliEngineConfigso the wiring is assertable without spawning anything; re-seeding the deletion now fails three tests. This is the one structural change beyond the ticket's letter, and it is why the last two rows of the seed table exist.Kill-list: clean — no new dependency, no TODO/FIXME/stub, no caption content logged or persisted, no credential or plan data in any log line, no competitor product named.
Design Fidelity
.sh-segradiogroup directly under it, inside the same Engine sectionsettings-sheet.tssh-engine-clitextContent set inrenderControls→Claude CLI · <label>✓ Claude CLI found · <label>+ "translating with<label>… change the model in Settings"onboarding.tsclaudeModelLabelover the same persisted field; neither local branch names a Claude model, because none is used thereclaude-model.test.tscovers the label fn incl. unknown → default.sh-seg/.sh-seg-btn/.t-meta; zero CSS changes, no raw colorscolor-guardpassrole="radiogroup"+aria-label="Claude model",aria-pressedper button — same contract as the engine and STT pickersValues interpolated into
innerHTMLare module-level literals; the persisted pick is rendered viatextContent, never markup.Deviations
buildCliEngineConfigextraction — the one structural change beyond the ticket's wording, justified above: without it the ticket's central AC had no test, as seed 5 proved.#[serde(default = ...)]deliberately NOT added toclaude_model, unlikestt_model. The container-leveldefaultalready yields Haiku and here that is the correct absent-field answer, so an override would be a no-op;stt_modelneeds one only because its absent-field value intentionally differs from the struct default. Verified in the harness both ways.Usageassertion proves our accounting does not branch on model. Neither establishes Anthropic's future pricing — if the CLI ever stops reportingtotal_cost_usd, the ledger goes quiet for every model equally, which is pre-existing behaviour and [refactor] Make the budget/fallback seam engine-agnostic — hours-remaining as the primitive, USD as only one derivation #205's territory.pointer_url/HF_REPO/SHA-256 untouched.