feat(agent): bind the model to the conversation, not the process

[arelchan]

Summary

The model was two attributes on a process-wide AgentLoop, so there was one answer for everyone: two sessions could not run different models, a switch in one moved all of them, and the "default" was whatever the last switch happened to write to agents.defaults. This makes the model a property of the conversation, and -- following the same thread down -- makes the credential that serves it something the user says rather than something Raven infers.

A ModelBinding is a model id, the credential that serves it, and how much that model can hold, as one value. ProviderPool is the single place deciding which credential a model id pairs with, cached per (vendor, model). run_turn resolves the binding for the turn's session and holds it in a ContextVar for the whole turn tree; the loop's provider / model / context_window_tokens, the context engine's LLM-backed segments, the skill gate and rewriter, and the consolidator all read that instead of a reference of their own.

Rule	How it lands
Different sessions, different models	A dict of overrides, read once at turn entry
A switch moves only the session that asked	Nothing else reads that session's entry
A new session starts on the configured default	A session-scoped switch does not write agents.defaults
A configured subsystem uses its own model+credentials; otherwise it follows the conversation	The factory resolves each pin through the pool, so a holder has either a complete pair or nothing
A switch mid-turn takes effect next turn	Free -- the turn holds the binding it entered on

Detached work inherits the context copy asyncio makes at task creation, so a subagent finishes on the model it was spawned under.

The window belongs to the binding

Sizing came to a head during the rebase. refresh_context_window and its cascade assumed the loop has one current model -- which this PR makes false, since two sessions can be on a 200k and a 1M model at once, and one int on the loop cannot answer for both. That is the same mis-sizing #341 removed, arriving through another door.

So the window moved onto ModelBinding, beside the credential, and every holder reads the binding of the turn it is running under. refresh_context_window is gone: a turn now enters on a binding that already knows its own size, so there is nothing to refresh.

It resolves on first read rather than at construction, because building a provider is what imports LiteLLM -- a lazily built one has not done it yet, so an eager window is the catalogue-miss default for every model, which is the defect on_built existed to paper over. A miss is deliberately not cached, so a cold read cannot make the process wrong for its lifetime. An explicit contextWindowTokens travels on every binding, so a pinned number survives whatever model a session switches to.

The provider is a word the user says

A model id does not name whose credential serves it. openrouter serving anthropic/claude-haiku-4-5 and anthropic serving claude-haiku-4-5 are both real, name different keys, bill different accounts, and look identical on the wire. Everything that used to fill in the blank was guessing which account pays.

agents.defaults.provider: "auto" was the same guess at the config layer, and it did not detect anything: a prefixed id was answered by the provider it names, but a bare id fell to keyword matching in PROVIDERS order and took the first configured claimant -- so with anthropic and openrouter both keyed, gpt-4.1 went to openrouter over openai because openrouter sits third in that list and openai eighth.

• /model <id> is refused, and the refusal names the two ways out: /model for the picker, or /model <provider> <id>. A prefixed id is refused too -- a prefix is LiteLLM routing syntax, not evidence about a credential.
• config.set model refuses without a provider, which is the boundary where the rule can be enforced rather than asked politely.
• raven provider use requires --provider, and names it alongside raven provider list in the error.
• --default is not an exception: changing what new sessions start on is the same choice about the same thing.
• agents.defaults.provider carries no default, and a one-shot migration resolves each pre-rule config through the old derivation and writes the answer into it. Behaviour is unchanged by construction -- the value written is what that config was already getting -- but it stops being inferred. It reuses the sidecar watermark from fix(*): stop an old default from capping every model's context window #341, with a floor per migration rather than one shared mark, so raising the generation for this one does not re-open the previous one. A config the derivation cannot answer for is left blank rather than filled with a vendor chosen to have something there.

The picker already asked in this order -- provider, then model, with a free-text row for an id no catalogue lists -- so it needed nothing. Both help strings now carry the usage, because a rule the user meets first as an error is a rule we chose not to tell them.

With nothing left deriving, providers/pin.py is deleted: its whole subject was "which provider to pin when the user changed the model without naming one", and its last caller was the onboarding wizard, which asks for the provider before the model and therefore always had one in hand.

Config defaults

No subsystem ships a vendor default. context.curator_model, token_wise.tool_result_lifecycle.summary_model and skill_forge.detect_model all hardcoded the same Gemini id, and token_wise.smart_routing.tiers shipped six models across three vendors -- for users who may hold no key for any of them. All are unset now, which is what "not configured" has to mean for the subsystem rule to be expressible.

context.curator_model and skill_forge.llm_gate_model took a model id and nothing else, so the pool had to guess which credential served it. Each pin now takes a provider alongside the model (curator_provider, llm_gate_provider). With the provider named, a vendor whose credentials are unusable is logged and dropped rather than silently borrowing the conversation's key. With it unset the pin still binds: a configured gateway takes it, because a gateway serves whatever id it is handed under its own credential, and only without one is the vendor derived from the id. That branch is unchanged from main and is documented as it behaves rather than as the rule would prefer -- an earlier revision of this description and of CONTEXT.md claimed the drop applied there too, which was wrong.

Release note. With context.curator_model unset, the Curator's slow path runs on the conversation's model instead of failing on a Gemini id nobody had a key for and dropping to the deterministic plan. That is the rule working as asked, but on a long conversation it is up to 12 tool-calling requests per turn of context housekeeping that previously cost nothing. Set context.curator_model (and curator_provider) to a small model to keep it cheap.

Review findings

Round 1 (pre-rebase), five findings. A pin naming a vendor with no spec crashed the agent at startup -- fixed on main, which now passes the provider name separately instead of reading it off a spec that may be None; verified with the reporter's own repro. /model before the first message no longer manufactures a zero-message session record (session.create is lazy, and session.title guards the identical case the same way). Every rpc call in the TUI's session commands has a .catch -- without it, /model typed before the first session.create resolves wrote a raw stderr line over the Ink render while the transcript showed nothing. The remaining two are the picker's provider row and the gateway pin; see below.

Round 2, three before-merge findings, all reproduced here before being fixed.

• The migration watermark was one high-water mark shared by every stamped migration, so raising it to 2 for the provider migration re-ran the context-window one on every config already stamped at 1 -- deleting a contextWindowTokens: 65536 a user had put back by hand after our own notice invited them to, and printing that invitation again. Each migration now has its own floor. The test that should have caught it built its precondition from CURRENT_CONFIG_VERSION, so it moved with the bump; it now pins the literal a shipped build wrote, and two tests cover the upgrade-from-an-older-generation path in both directions.
• Deleting _adopt_provider took its _image_tool_result_ok.clear() with it. Both capability caches key on a model id but are computed from the provider, so an apiBase repointed at a box with different capabilities, or a re-authenticated provider, kept the old endpoint's verdict for the life of the process -- images silently dropped from tool results with nothing in the log. Both binding setters now clear them.
• --default had no working path through the TUI. The picker footer taught /model <name> --default, which is the bare-id spelling the parser refuses; the refusal dropped the flag from the suggestion it told the user to follow; and /model --default opened the picker, whose selection sent a session-scoped switch that looked like it had changed the default. The flag now rides into the overlay state and back out through the picker's callback, the refusal keeps it, and the footer writes the spelling that works.

The two carried from round 1 are fixed here rather than dissolved. An earlier revision of this description said the work had dissolved them; it had not, and both were still reproducible.

• model.options re-derived the provider from the id. For a vendor with a spec that now reads the prefix stored_model_id writes, but find_by_model answers None for a passthrough vendor (mistral, xai), so the picker starred agents.defaults.provider for a session running on someone else's key -- and the starred row is exactly what a user reads to answer whose key is paying. It now reads the head of the stored id, and stars nothing when that names no configured provider.
• The gateway pin is documented as it behaves; see the Config defaults section above.

Round 3, after the rebase onto a main that gained #346. The conflict was additive and is
described in the thread; two follow-on commits closed the two nonblocking notes that survived it.
modelPicker.test.tsx was rendering <ModelPicker> without the scope prop this branch made
required, so the scope === 'default' footer had no coverage -- and tsconfig.json excludes
src/__tests__, which is why tsc --noEmit was clean and silent about it. And onModelSelect, the
callback that carries --default back out of the picker, had no test; the rule it applies is now its
own function, modelSelectCommand, because what a selection becomes on the command line has to
satisfy three separate constraints in parseModelArg at once.

Also taken, each a defect in what this PR itself added or removed: clear_session_binding was gated on SessionManager.delete returning True, which it does not for a session that switched model before its first save, so the entry leaked for the life of the process; _migrate_auto_provider validated its probe against a strict Config before the shims that relocate legacy top-level blocks, so a config still carrying skillRouter failed the probe, was skipped silently, and was stamped anyway -- never retried, on exactly the oldest configs most likely to still say auto; providers/lazy.py and AgentLoop.set_provider carried docstrings naming methods and callers this PR deletes; onboarding still wrote provider="auto" when clearing a removed provider's default; and ui-tui/package-lock.json is reverted to main, its only change being npm-version drift in libc metadata.

Type

• Fix
• Feature
• Docs
• CI / tooling
• Refactor
• Other

Verification

pytest tests/                     6700 passed, 33 skipped, 13 deselected   (TERM=dumb)
ruff check / format --check       clean, 832 files formatted
npx tsc --noEmit (ui-tui)         clean
npm test (ui-tui, vitest run)     998 passed (86 files)
npm run lint:rpc                  generated.ts in sync
commitlint + check_commit_messages.py   clean over github/main..HEAD

tests/test_cli_theme.py::test_bold_accent_renders_styled_not_bare is TERM-dependent, not
ordering-dependent: it fails under an interactive TERM and passes under TERM=dumb, which is what
the coverage target pins and what CI runs. That file is not touched by this PR.

Every fix in rounds 2 and 3 was mutation-checked rather than assumed: reverting the migration floor,
the cache clear, the flag pass-through, the passthrough-vendor fallback, the picker's scope prop
and the --default command-building each turns the corresponding new test red.

Driven by hand in an isolated home against the built TUI bundle, not only by tests: /model claude-opus-4-5 refused with the two ways out; /model openrouter/anthropic/claude-opus-4-5 refused
as well; /model openrouter claude-opus-4-5 applied; /model opening the picker at the provider
level. A real config carrying provider: auto and a bare claude-opus-4-5 migrated to provider: openrouter -- the vendor it was already resolving to -- with the notice printed and the watermark
written to the sidecar.

• Relevant tests pass locally
• Relevant lint / type checks pass locally
• User-facing docs or screenshots are updated when needed

Risk

User-visible behaviour changes, all deliberate:

/model <id>, config.set model without a provider, and raven provider use without --provider now fail where they used to guess. That is the point, but it is a workflow change for anyone who typed a bare id -- the errors name the fix, and the picker needs nothing new learned.

agents.defaults.provider is written into every config that did not have one, on the first launch after this lands. The value is what that config was already resolving to, so no request changes vendor; what changes is that it is now visible and editable.

With curator_model unset out of the box, the Curator's slow path starts actually running (see the release note above). Cost, not correctness.

One contract tightening is not backward compatible on its own: config.set key="model" now rejects a call with no provider, and the -32009 model_switch_in_turn code is removed from the error table. The in-repo TUI moves with it, but any client outside this repo that sends a bare model id starts failing on the first request after this lands.

Rollback: revert the commits. The config keeps an explicit provider, which every published build accepts and treats exactly as it treats a hand-written one. The config.migrations.json sidecar is left behind and every build since #341 does read it, so a reverted build sees {"version": 2} and treats the config as fully migrated -- which is correct, since it is. Delete the sidecar to force the migrations to be reconsidered.

• Security impact considered
• Backward compatibility considered
• Rollback path is clear for risky changes

Related Issues

N/A

[gloryfromca]

Review of the three commits stacked on #282 (3eaf3a0..03982ae)

Read every hunk plus the surrounding call graph -- provider pool, the binding
contextvar, the loop's holders and fan-out, the session and model RPC handlers,
the TUI slash command and the picker -- and reproduced the crash and the
mis-pairing below in a throwaway worktree at 03982ae rather than reasoning
about them from the source.

The shape is right

Making the model a ModelBinding -- a model id and the credential that serves
it as one value -- and capturing it once at run_turn entry into a ContextVar
is the correct answer to the bug this line of work started from. Specifically
checked and found sound:

• run_turn's session key (req.conversation or f"{channel}:{chat_id}")
  matches what the spine hands it, and the binding is captured before the
  turn's first read, so a mid-turn switch really is invisible to that turn
  without any parking. Deleting _turns_in_flight / _pending_provider is
  justified.
• ContextVar isolation across concurrent turns and detached work:
  asyncio.create_task and asyncio.to_thread both copy the context, and
  there is no run_in_executor on the LLM path.
• AgentLoop.provider / model becoming read-only properties breaks no
  production caller -- the remaining self.provider = ... assignments in the
  tree all belong to other classes.
• ContextBuilder is handed the provider too and is not in the fan-out, but
  its llm_provider reaches LocalSkillCatalog, where the parameter is
  accepted and unused. Not a missed holder.
• The curator's slow path calls through _curator_binding() (pair) rather
  than through the provider / model properties (which resolve to the
  conversation), so the pin is not half-applied there.
• The emptied config defaults (summary_model, detect_model,
  smart_routing.tiers) genuinely have no readers today.
• Fork metadata inheritance and the binding released on session.delete are
  correct.

uv run pytest tests/test_provider_pool.py tests/test_agent_loop_session_model.py tests/test_tui_rpc_model.py tests/test_tui_rpc_config.py tests/test_tui_rpc_session.py -q
-> 218 passed, 18 skipped on 03982ae.

Findings

Five, all inline. The first two are in the last commit (03982ae, the
subsystem-pin-as-a-pair change); both were reproduced, not inferred.

1. raven/providers/pool.py:183 -- _has_credentials feeds a possibly-None
   spec into schema._has_credentials, which dereferences spec.is_oauth. A
   curator_provider naming a vendor Raven has no spec for aborts
   AgentLoop.__init__. The call sits outside the try this commit
   deliberately broadened for exactly this reason.
2. raven/providers/pool.py:129 -- with a gateway forced and the pin's
   provider unset, the pin still binds (gateway, raw upstream id) and returns
   a binding. curator_model: gemini-2.5-flash under openrouter still 404s
   and is still swallowed by the curator's fallback, with nothing logged. That
   is the defect the description reports as fixed ("logged and dropped"),
   surviving for precisely the configs that carried the old shipped default,
   and no test covers the unset-provider branch.
3. raven/tui_rpc/methods/model.py:204 -- model.options re-derives the
   provider from the model id for a session that has its own binding, so the
   picker marks the wrong row for a gateway session. The right answer is on the
   binding and on the session record. A new test currently asserts the guess.
4. ui-tui/src/app/slash/commands/session.ts:92 -- /model sends
   scope: 'session' with a possibly-null sid, which the server now refuses,
   and the chain has no .catch(...). The user gets no transcript error and a
   raw unhandledRejection line over the Ink render. The missing .catch is
   pre-existing in this file; this commit is what makes it easy to reach.
5. raven/tui_rpc/methods/config.py:467 -- _remember_session_model saves
   unconditionally, so a /model before the first message materialises an empty
   session record and a phantom row in /sessions list. session.title
   handles the same case correctly one file over.

Lower confidence, worth a look

• _restore_session_model is wired only into session.resume and is the
  session record's only reader, while the record is written for any session key.
  Unreachable today because config.set is TUI-only, so this is latent rather
  than a defect: a non-TUI session (or a CLI path continuing a session) that
  ever acquired an override would run the default after a restart while its
  record said otherwise.
• QueryRewriter._call_provider passes no model and relies on "the pool builds
  each provider with the bound model as its default". True for pool-built
  bindings, and the construction sites look consistent, but it is an implicit
  contract with no test: AgentLoop(provider=p, model="X") where p's default
  is not X would silently rewrite on the wrong model.
• Not a defect, a product call worth confirming: with context.curator_model
  now unset by default, the curator's slow path moves from "fails on a Gemini id
  nobody had a key for and drops to the deterministic plan" to up to 12
  tool-calling requests per turn on the conversation's model. The description
  says this plainly; flagging it because it is the new default, not an opt-in.
• tests/test_tui_rpc_model.py has 18 parameterized skips. Pre-existing, not
  from this PR, but they narrow that file's green more than the count suggests.

[gloryfromca]

Blocking: context.curator_provider naming a passthrough vendor takes the agent down at construction -- @gloryfromca's pool.py:183 finding, which I reproduced independently. Nothing else I found rises above a suggestion.

I read git diff github/main...HEAD (47 files) plus the callers, and main's version of each thing that moved. Covered: the diff itself; the three production AgentLoop(...) sites; backward compatibility of the RPC surface; AGENTS.md (comment language, test-file naming, repo assets, CONTEXT.md domain terms); and whether tests were weakened to look green.

The blocker

Reproduced on 03982ae, independently of the thread:

Config.model_validate({'providers': {'mistral': {'apiKey': 'sk-test'}}})
ProviderPool(cfg).bind_pin('mistral-large-latest', 'mistral')
-> AttributeError: 'NoneType' object has no attribute 'is_oauth'

curator_provider is new in this PR, is str | None with no validation, and ProvidersConfig documents passthrough vendors as supported ("a provider LiteLLM supports but Raven has no spec for still works from config alone", schema.py:402-404). So this PR adds a field whose documented-valid values stop AgentLoop.__init__, from a call site the same commit deliberately widened a guard for. Detail and the fix I verified are in that thread.

What I tried to break and could not

• The binding itself. run_turn keys on req.conversation or f"{channel}:{chat_id}", and methods/turn.py:154 passes conversation=parsed.session_key -- the same string the TUI hands config.set as session_id. The write half and the read half agree, so a switch is not landing on a key no turn reads.
• Lost pins. All three production AgentLoop(...) sites (agent_commands.py:325, gateway_commands.py:227, tui_commands.py:443) pass a ProviderPool, so no entry point silently loses curator_model / llm_gate_model.
• A half-pair reaching the wire. The curator slow path calls binding.provider.chat_with_retry(..., model=binding.model) off one _curator_binding() (segments/curator.py:247-253). CuratorAssembler.provider / .model resolve the turn's binding rather than the pin, which is right: those feed the trimmer's window arithmetic, which is about the conversation.
• restore_session_model's narrow except (SystemExit, RuntimeError, ValueError) looked like the blocker's twin, given the same commit widened bind_pin to bare Exception for exactly that reason. It is not: check_provider_credentials raises typer.Exit, whose MRO is (click.exceptions.Exit, RuntimeError, ...), and both OAuth provider constructors are two assignments. I could not construct a failure the tuple drops. Withdrawn.
• Cost of the config supplier. ProviderPool.config re-reads through load_runtime_config(None, None) on every fingerprint, gateway lookup and credential check -- roughly 4 loads per bind_pin. Measured at 0.68 ms per load, and no per-turn path touches it (binding_for_session is a dict read). Not worth raising.
• Weakened tests. The four edits to existing tests (test_read_file_image.py, test_sandbox_unit.py, test_subagent_manager.py, test_rpc_schema_match.py) are mechanical signature and attribute updates that follow the production change; no assertion was loosened, and no skip or xfail was added. tests/test_agent_loop_session_model.py asserts the real things -- two concurrent turns on two models, a mid-turn switch landing on the next turn, a detached subagent keeping its spawn binding, and the restore path in both directions.
• Dropping error code -32009. Safe both ways: an older client meets a server that never raises model_switch_in_turn, a newer client meets an older server and falls through to a generic message. is_turn_active is still live for session.py:504,536 and turn.py:138, so nothing was orphaned.

Test state I measured, at 03982ae unmodified

uv run pytest tests/ -q          1 failed, 5362 passed, 58 skipped, 13 deselected
uv run ruff check raven/ tests/  All checks passed
npm test        (ui-tui)         974 passed (85 files)
npx tsc --noEmit                 clean
npm run lint:rpc                 generated.ts in sync
npx eslint src/                  0 errors, 22 warnings (pre-existing, untouched files)

The one failure is tests/test_cli_theme.py::test_bold_accent_renders_styled_not_bare, pre-existing and unrelated to this diff. One correction to how the description characterises it: it is not load-dependent. It fails deterministically whenever tests/test_cli_theme.py runs as a whole file (1 failed, 44 passed in 0.33s) and passes when run alone, so it is ordering inside that single file. Out of scope here, but it will not go away on a quieter machine.

[gloryfromca]

set_provider's docstring names two callers that do not exist.

AgentLoop.set_provider has no production caller -- not on this branch, and not on main either:

$ git grep -n "set_provider" HEAD -- raven/ | grep -v "def set_provider"
raven/agent/loop/main.py:716-718     # the three subsystem setters, from set_default_binding
raven/context_engine/...             # the subsystem setters themselves

Nothing calls loop.set_provider / agent.set_provider / agent_loop.set_provider. _set_model goes through set_default_binding now, and on main it assigned loop.provider / loop.model directly (methods/config.py, the two lines this PR replaces), so the method was already dead there. The only callers are tests/test_agent_loop_model_switch.py:92 and :117.

Not a runtime defect. The concrete cost is that the next reader takes "the gateway, a CLI one-shot" at face value and keeps both the method and the two-argument shape it implies, on the belief that an entry point depends on it. That is the same error the description says it corrected by deleting ProviderPool.default ("had no production caller at all: what a new session starts on is AgentLoop._default_binding") -- this one survived the same pass.

Either drop it and have those two tests call set_default_binding, or say what it is: a wrapper with no caller today, kept for shape.

[claude]

Claude finished @arelchan's task in 2m 7s —— View job

---

Review of 2148b1f..HEAD (commits 229a98f, 20e2c9d)

• Read prior review comments (0xKT, gloryfromca rounds 1-4)
• Identify what changed since the last-reviewed head (2148b1f)
• Review the new commits for correctness, conventions, tests
• Post findings / summary

No blockers; this can merge.

The delta since gloryfromca's last approval-leaning pass (2148b1f, "no blockers... nothing left in the code as far as I can see") is exactly the two nonblocking notes that review left open, and nothing else:

1. 229a98f — modelPicker.test.tsx now passes the (newly required) scope prop and adds a test asserting the two footer variants (--default sets vs scope: the default...) actually differ. Verified against modelPicker.tsx:1426-1428: the session-scope branch does contain --default sets and the default-scope branch does not, so the assertion is checking real, distinguishable output rather than a tautology. appOverlays.tsx:167-174, the one production caller, already passes scope correctly (from an earlier commit), so this was purely closing a test-only gap, not a production fix.
2. 20e2c9d — extracts onModelSelect's command-building into a standalone exported modelSelectCommand(model, providerSlug, pending) in useMainApp.ts:99-100, and adds modelSelectCommand.test.ts with 4 cases. Confirmed the extraction is behavior-preserving: pending === 'default' (renamed from overlay.modelPicker === 'default') is the same condition, same output string, same call site. Traced the four test cases against parseModelArg (session.ts:38-52) and the --default strip regex (session.ts:...) by hand — all four produce strings that parse the way the test asserts, including the round-trip in case 4.

Checked for regressions: overlay.modelPicker's type (interfaces.ts:93, boolean | 'default') matches all values used in the new tests; no other production caller of ModelPicker or modelSelectCommand needs updating; both commit messages are ASCII-only, correctly scoped (test(tui): ...), and well under the 100-char header limit.

Note: I could not run npm test/vitest in this sandbox (no node_modules, and installing requires network approval I don't have here) — the above is verified by static reading of the parser/regex/component logic rather than execution. Given the changes are small, additive, and test-only (plus one pure-function extraction with an unchanged call site), I'm confident in this assessment, but flagging the limitation per instructions.

[claude]

[nit] .catch(ctx.guardedErr) is chained twice here (lines 145 and 150), introduced together in eb83925 when the missing-catch bug was fixed. The second one is dead code — the first .catch already converts the rejection into a resolved promise, so the chain never reaches the second handler. Harmless, but worth collapsing to one .catch(ctx.guardedErr) (and the explanatory comment can stay on it).

Suggested change

	.catch(ctx.guardedErr)
	// Without this the rejection is unhandled, and setupGracefulExit writes
	// it raw to stderr over the Ink render: `/model` typed before the first
	// session.create resolves is refused by the server (session scope with
	// no session), and the transcript would show nothing at all.
	.catch(ctx.guardedErr)
	.catch(ctx.guardedErr)

[gloryfromca]

Blocking: bumping CURRENT_CONFIG_VERSION re-runs the context-window migration on every already-stamped config and deletes a contextWindowTokens: 65536 the user restored on purpose -- inline note on raven/config/loader.py:19. Everything else on this revision is a suggestion.

New head since my last pass (03982ae -> 6beb459, seven commits). I reviewed the delta rather than the whole diff again: the rebase onto a main that grew providers/auth, providers/wire, providers/pin and the lazy/per-model provider layer, plus the two features added after review -- retiring the auto provider, and requiring the provider as a spoken word on three surfaces. Covered: the delta and its callers; main's behaviour where this branch replaces it; the migration path for an existing config; AGENTS.md (comment language, test naming, CONTEXT.md domain terms); and whether tests were weakened.

Previously-raised findings, verified rather than taken on trust

• pool.py passthrough crash -- fixed, and the author's account of how is right: schema._has_credentials now takes the name separately (schema.py:501) instead of reading spec.is_oauth off a None. Re-ran the reporter's repro: bind_pin('mistral-large-latest', 'mistral') returns a binding, and empty / absent / apiBase-only sections each warn and return None.
• /model before the first session.create -- fixed, and the sweep went past the one call I said was in scope. Fine by me; the file was the outlier.
• The lazy session record -- fixed with the sessions.exists guard, and the metadata still rides the first real save.
• The picker's provider row -- fixed as a side effect of stored_model_id becoming unconditional, not by either fix discussed in that thread. Details, and the one vendor class where the symptom survives, in the thread.
• Still open, all nonblocking, none of them re-reported here: AgentLoop.set_provider's docstring still names two callers that do not exist (main.py:734); the gateway-pin path is still unlogged and CONTEXT.md:66 ("A pin that cannot be paired is reported and dropped") plus raven.py:96 ("Unset falls back to deriving the vendor from the id") still describe a rule the code does not follow on that branch.

The new features, tried and not broken

• Retiring auto. Every provider == "auto" test I could find also accepts "" (pool.py:112,151,159,175, schema.py:922), so a config the migration cannot write to keeps deriving instead of failing. The changed = X or changed shape in _persist_migrations does run both migrations rather than short-circuiting.
• provider as a required word. _set_model refuses a missing provider at the boundary, and MissingCredentialsError is caught explicitly (config.py:356) rather than falling into the RuntimeError branch where str(typer.Exit) used to surface as error: "1" -- the same class the rebase fixed in restore_session_model, handled here too.
• Where I expected stored_model_id to break the gateway case it did not: stored_model_id('openrouter', 'anthropic/claude-haiku-4-5') gives openrouter/anthropic/claude-haiku-4-5, which is the correct LiteLLM spelling, not a double prefix.
• provider: "auto" typed as the word writes auto/<model> through stored_model_id, but only reaches disk when agent_loop_factory yields None. With a real loop pool.bind raises MissingCredentialsError before anything is persisted, so the user gets an error and the config is untouched. Withdrawn.
• Tests: no skip or xfail added; the edits to existing tests in c3e300f1 replace loop.model = ... assignments with binding calls, which is what a real caller does, and the gate tests now assert the rule in both directions instead of the forwarding they used to assert. Nothing loosened.

Test state I measured, at 6beb459

uv run pytest tests/ -q          1 failed, 6443 passed, 43 skipped, 13 deselected
uv run ruff check raven/ tests/  All checks passed
npx vitest run    (ui-tui)       991 passed (86 files)
npx tsc --noEmit                 clean
npm run lint:rpc                 generated.ts in sync

The one failure is tests/test_cli_theme.py::test_bold_accent_renders_styled_not_bare, a file this PR does not touch; it fails identically when that file runs on its own (1 failed, 44 passed in 0.26s), so it is ordering inside that file and not attributable here.

One unrelated ask

ui-tui/package-lock.json is in the diff only to strip the libc arrays off a dozen optional dev dependencies -- an artifact of an npm older than the one that wrote main's lockfile. Nothing in this PR needs it. Worth reverting that file so it does not flip back and forth between contributors.

[0xKT]

Review at 6beb459: the shape is right, but three regressions and two false statements should not land

Two independent deep reviewers with no shared context read the diff plus the surrounding call graph, and every claim below was then re-verified by hand: reproduced in an isolated detached worktree at this head, with origin/main as the counterfactual, and with mutation experiments where the question was "does a test actually catch this". Nothing was written to this branch or to this PR.

What held up: the binding itself (run_turn captures once at entry, ContextVar isolation across concurrent turns and detached work, no run_in_executor on the LLM path, all five run_turn call sites funnel through AgentLoop.run_turn); all three production AgentLoop(...) sites pass a pool, so no entry point loses a pin; the auto ordering argument is exactly right (openrouter is the 3rd spec, openai the 8th, and gpt-4.1 really does land on openrouter); providers/pin.py is safe to delete (no caller but the wizard, which asks for the provider first); the emptied summary_model / detect_model / smart_routing.tiers defaults have no production readers; F1 is genuinely fixed by the rebase (bind_pin('mistral-large-latest', 'mistral') returns a binding rather than raising); and the core new tests are not idle -- mutating binding_for_session to always return the default turns 10 of 14 tests in tests/test_agent_loop_session_model.py red, and reverting the F5 guard turns test_a_model_switch_before_the_first_message_writes_no_session_file red.

Test state measured on this head: pytest tests/ -> 6681 passed, 33 skipped, 13 deselected, 0 failed. ruff check and ruff format --check clean. The CI unit (py3.12 / ubuntu-latest) job on this head agrees: 6681 passed, 0 failed. test_cli_theme.py::test_bold_accent_renders_styled_not_bare did not fail for either reviewer, whole-file or alone.

---

R1 - before-merge - correctness

Where: raven/config/loader.py:19, :346, :427-429

Problem: CURRENT_CONFIG_VERSION goes 1 -> 2, but the watermark is a single scalar shared by every stamped migration, so bumping it re-opens the gate for _migrate_legacy_context_window as well. That migration then runs a second time on configs it already visited, and deletes a contextWindowTokens: 65536 the user set by hand.

Failure scenario: a user who upgraded to 0.1.11, had the fossil cleared, read the notice ("Put the line back if you did want that number -- for an endpoint served with a smaller window, for instance") and did exactly that. Their ~/.raven/config.migrations.json says {"version": 1} and their config says contextWindowTokens: 65536. On the first load after this lands:

                     in-memory   on disk    stamp   notices
origin/main d4f0530   65536       kept       1       0
this head   6beb459   None        deleted    2       1 (the same "put the line back" notice)

So the value is removed a second time, and the same notice invites them to put it back a second time. This breaks the guarantee _write_migration_version states in its own docstring at loader.py:162-166: "from here on, a contextWindowTokens the user sets by hand is never second-guessed, whatever its value" -- and _migrate_legacy_context_window at :177-181 says the same thing ("Running it once means we clear what we planted; from then on the number is the user's").

The existing test cannot see this. tests/test_config_loader.py:298 test_context_window_pin_survives_once_stamped builds its precondition with {"version": CURRENT_CONFIG_VERSION} (:303) -- the very constant that changed -- so it moves with the bump and stays green. Replacing that with the literal a shipped build actually wrote:

_stamp_path(p).write_text(json.dumps({"version": 1}), encoding="utf-8")

fails immediately: assert None == 65536. No test in that file presets the stamp to an older literal version, so there is no coverage of the upgrade-from-an-older-generation path at all.

Suggested fix: either give each migration its own low-water mark (_migrate_legacy_context_window runs only when stamped < 1, _migrate_auto_provider only when stamped < 2), or record applied migrations by name in the sidecar ({"version": 2, "applied": ["legacy_context_window", "auto_provider"]}) and gate each on its own entry. While here: the Risk section says "the orphaned config.migrations.json sidecar is ignored by any build that does not look for it", but every build since #341 does look for it, and after a revert those builds read {"version": 2} and conclude the config is fully migrated.

Verify: add a test that writes {"version": 1} to the sidecar plus contextWindowTokens: 65536 to the config, then asserts the value is still in the file after load_config and that drain_migration_notices() is empty. It is red on this head.

---

R2 - before-merge - correctness

Where: raven/agent/loop/main.py:706 (set_session_binding), :721-734 (set_default_binding); deleted from origin/main:raven/agent/loop/main.py:685-687

Problem: deleting _adopt_provider also deleted its self._image_tool_result_ok.clear(), and neither of the two new binding setters replaces it. The verdict cached there is keyed by model id but computed from the provider (_supports_image_tool_result at :919-942 calls supports_image_tool_result(self.provider, key, spec)), which is exactly what the deleted comment said: "Cached per model id but computed from the provider, so a swap that keeps the model id would keep serving the old transport's verdict." That half of the why is also gone from the surviving comment at :359-362, which now only explains the per-model half.

Failure scenario: providers.custom.api_base = http://boxA/v1, model custom/my-model. boxA refuses an image in a tool result, so main.py:2160 learns _image_tool_result_ok["custom/my-model"] = False. The user points api_base at boxB, which supports images, and re-runs /model custom my-model --default. The credentials fingerprint changed, so the pool rebuilds the provider, but the cache key is still custom/my-model and nothing clears it -- images are dropped from tool results for the rest of the process, with no log line saying why. Re-authenticating an OAuth provider is the same shape. This is a regression: main clears it, this head does not. And the loop now holds several providers at once, so a model-id-keyed verdict answers for more callers than before.

Suggested fix: clear both caches in set_default_binding and set_session_binding:

self._image_tool_result_ok.clear()
self._vision_ok.clear()

and put the "computed from the provider" sentence back on the comment at :359. (_vision_ok at :364 has never had an invalidation point; it is pre-existing, but it is one line in the same place.)

Verify: in tests/test_agent_loop_model_switch.py, seed loop._image_tool_result_ok["x/y"] = False, call set_default_binding(ModelBinding(other_provider, "x/y")), assert the dict is empty. Red on this head.

---

R3 - before-merge - correctness

Where: raven/tui_rpc/methods/model.py:311-316, :468

Problem: model.options still re-derives the provider from the model id instead of reading the pair the session already recorded. _session_model returns only the model string, and metadata["provider"] -- written by _remember_session_model at config.py:437-441 -- is never read. This was raised, confirmed, and answered with a complete patch and a discriminating test in an earlier round; no code changed.

Failure scenario: the gateway case now happens to come out right, but only because _set_model runs stored_model_id(new_provider, raw_value) at config.py:327, which prefixes the id so find_by_model guesses correctly. It is a coincidence, not a fix, and it does not hold for a vendor Raven has no spec for:

mistral    + mistral-large-latest       -> stored 'mistral/mistral-large-latest'    find_by_model -> None
xai        + grok-4                     -> stored 'xai/grok-4'                      find_by_model -> None
openrouter + anthropic/claude-haiku-4-5 -> stored 'openrouter/anthropic/...'        find_by_model -> openrouter

With find_by_model returning None, current_provider falls through to the configured default. So a session running on Mistral's key gets the Anthropic row (or whatever agents.defaults.provider names) marked is_current, with the cursor parked on it. Passthrough vendors do appear in the picker, so this is a reachable row, and a user reading the marked row to answer "whose key is paying for this session" gets the wrong answer.

Suggested fix: the patch given in the earlier round -- have _session_model return tuple[str, str | None], reading the provider off sessions.peek(session_id).metadata["provider"], and fall back to find_by_model only when the session carries no provider of its own. The "auto" special case in that patch can be dropped now: _set_model no longer writes "auto".

Verify: test_options_reports_the_provider_the_session_was_switched_to with a passthrough vendor fixture (mistral): assert result["provider"] == "mistral" and that mistral is the only row with is_current. Red on this head.

---

R4 - before-merge - correctness

Where: ui-tui/src/components/modelPicker.tsx:1426, ui-tui/src/app/slash/commands/session.ts:108, ui-tui/src/app/useMainApp.ts:827-829

Problem: three pieces of user-facing copy added by this PR contradict each other, and --default has no working path through the TUI. The picker footer (changed from scope: global in this diff) teaches /model <name> --default sets the new-session default. A bare <name> is precisely what session.ts:105-108 now refuses, and the refusal's alternative spelling silently drops the flag.

Failure scenario:

1. The footer teaches /model <name> --default. The user types /model gpt-4.1 --default.
2. session.ts:96-97 strips the flag, parseModelArg sees one word, no provider, so :108 refuses with: /model needs a provider. Run /model to pick one, or: /model <provider> gpt-4.1. The --default is not in that suggestion.
3. The user follows it: /model openai gpt-4.1. asDefault is false, so this is a session-scoped switch, and the transcript says model -> ... rather than default model -> .... The default they wanted to change was not changed, and nothing tells them their intent was downgraded.
4. The other route dead-ends too: /model --default leaves rest empty, so :99 opens the picker; selecting a row calls slashRef.current("/model ${model} --provider ${providerSlug}") at useMainApp.ts:829, with no --default. Session scope again, and the picker looks like it worked.

The only spelling that actually changes the default is /model <provider> <id> --default, and no copy anywhere writes it that way.

Suggested fix: all three, since any one alone leaves the loop closed.

• session.ts:108: carry the flag into the suggestion when asDefault is set -- ... or: /model <provider> ${value}${asDefault ? ' --default' : ''}.
• modelPicker.tsx:1426: write the working spelling, /model <provider> <id> --default.
• useMainApp.ts:827-829: either pass the pending --default intent through to the picker's callback, or have /model --default say "the picker changes this conversation only" instead of opening it.

Verify: two vitest cases -- (a) the refusal text for /model gpt-4.1 --default contains --default; (b) a selection made after /model --default sends scope: 'default' (or the picker does not open). Both red on this head.

---

R5 - before-merge - docs

Where: CONTEXT.md:345-350

Problem: the pre-existing "Provider Pin" entry documents agents.defaults.provider as being rewritten by one rule at providers/pin.py::resolve. d44e8a9 deletes that file and removes derive-on-switch entirely, so the entry both dangles on a deleted path and describes a mechanism this PR's own last commit removed. It also now collides in name with the new Subsystem pin entry five lines above it at :57, which is a different concept. AGENTS.md section 6 requires a CONTEXT.md term to be verifiable against the code.

Failure scenario: the next reader looking for how the provider is decided finds an entry naming a file that is not in the tree, and two similarly named entries for two different things within six lines of each other.

Suggested fix: rewrite the entry to say what agents.defaults.provider is now (an explicit value the user says, written down once by the migration, derived by nothing), or delete it and let Subsystem pin plus the migration's own docs carry the subject. Either way resolve the name collision.

Verify: git grep -n "providers/pin" -- CONTEXT.md ui-tui/CONTEXT.md is empty, and grep -n "^\*\*.*[Pp]in\*\*" CONTEXT.md returns terms that do not read as the same thing. (.github/coverage-baseline.json:2851 also still carries an entry for raven/providers/pin.py; harmless, since the check only forbids a drop, but it is stale.)

---

R6 - before-merge - docs

Where: CONTEXT.md:66, raven/config/raven.py:97, raven/config/raven.py:1012 (and raven/providers/pool.py:107-108)

Problem: four places -- three of them text this PR adds -- state a rule the code does not have on the gateway path. CONTEXT.md:66: "A pin that cannot be paired is reported and dropped". raven.py:97 (curator_provider): "Unset falls back to deriving the vendor from the id". raven.py:1012 (llm_gate_provider): "leave it unset to derive the vendor from the id". pool.py:107-108 (bind_pin's own docstring): "Absent, the vendor is guessed". On the else branch at pool.py:126-134, a configured gateway wins over the id derivation, the pin binds, nothing is reported, and nothing is dropped.

Failure scenario: confirmed on this head with agents.defaults.provider: openrouter and only an openrouter key: bind_pin('gemini-2.5-flash', None) returns a binding whose provider.api_key is the openrouter key, carrying a bare Gemini id that gateway has no route for, and the log is silent. A user reading the curator_provider docstring to understand their gateway config builds the opposite model of what happens, and there is no log line that would let them notice.

To be clear about the split: the behaviour itself is not a regression -- main sends the same pair -- and the earlier round settled on keeping it. What is new here is the documentation, and it is documentation that teaches the wrong rule. That half is what should not merge.

Suggested fix: correct all four sentences to say what the code does -- with the provider unset, a configured gateway takes the pin, and only without one is the vendor derived from the id. The logger.warning agreed on in that round is a good idea and the patch for it is already written in the thread; that half is fine as a follow-up.

Verify: git grep -n "reported and dropped\|deriving the vendor from the id\|derive the vendor from the id" returns nothing that contradicts pool.bind_pin's branches.

---

R7 - before-merge - describe

Where: PR description, "Review findings" paragraph

Problem: the description says two of the five earlier findings "were dissolved by the work itself: the picker guessing a provider from the id, and the gateway winning over an unset pin, are both gone now that a pair is the only legal unit." Both are still present -- see R3 and R6, each reproduced on this head. The description also repeats "a pin that cannot be paired is logged and dropped rather than silently borrowing the conversation's key", which is not true on the gateway path. Separately, "a pair is the only legal unit" does not hold for a subsystem pin: curator_provider is str | None and unset is legal.

Failure scenario: this repository squash-merges with squash_merge_commit_message=PR_BODY, so this text becomes the commit body on main and records two live defects as resolved.

Suggested fix: move both from "dissolved" to "still open", or fix R3 and R6 and leave the sentence. While editing: the Verification block's "1 failed" no longer reproduces (CI on this head is 6681 passed / 0 failed, and neither reviewer reproduced the theme failure), and npx vitest run src/__tests__ is a narrower path than the repo's own npm test, so its 850 is not comparable to the 974 quoted earlier in the thread.

Verify: re-read the description against R3 and R6 before merging.

---

R8 - describe

Where: PR description, "Release note" paragraph -- and a suggestion from the earlier round that should not be taken

Problem: the thread suggested adding a sentence saying the release note applies to fresh installs only, on the grounds that save_config has no exclude_defaults and therefore every existing config literally carries "curatorModel": "gemini-2.5-flash". That premise is wrong. save_config at raven/config/loader.py:404 dumps schema.Config, whose top-level fields are agents, cli, channels, providers, gateway, tools, routing, cron, language -- there is no context block in it. Measured on this head:

top-level keys written : ['agents', 'channels', 'cli', 'cron', 'gateway', 'language', 'providers', 'routing', 'tools']
'context' in file      : False
'curatorModel' in file : False

The onboarding seeder does not write context either, and git grep curatorModel raven/ tests/ is empty.

Failure scenario: adopting that suggestion would put a false statement into the release note, and the up-to-12-requests-per-turn cost warning would be scoped away from the users it actually applies to.

Suggested fix: leave the release note as it is. It is correct for existing installs as well as new ones. (provider: "auto", by contrast, really was written into every config literally -- and that half the migration already handles.)

Verify: python -c "from raven.config.loader import load_config, save_config; ..." against a temp HOME, or just read schema.Config's field list at raven/config/schema.py:857-867.

---

follow-up

Worth tracking, none of them blocking:

• raven/tui_rpc/methods/session.py:439 -- clear_session_binding is still inside the if removed conjunct, and SessionManager.delete returns False when no file existed. Now that R-round F5's lazy guard has landed, a session that switched model but never saved leaves its _session_bindings entry behind for the life of the process. The earlier round agreed on moving the clear out of the conjunct; it is one line.

• raven/agent/loop/main.py:734 -- AgentLoop.set_provider has no production caller (only tests/test_agent_loop_model_switch.py:92,117), yet its docstring still names "the gateway, a CLI one-shot". Same class as the ProviderPool.default deletion this PR justifies by "had no production caller at all". It also builds ModelBinding(provider, model) without _configured_window, so a pinned window would be dropped the day it acquires a caller.

• raven/providers/lazy.py:52-61 -- a production docstring pointing at the deleted AgentLoop.refresh_context_window. Nothing in raven/ sets .on_built any more, and the set_context_window fan-out (context_engine/base.py, assembler.py, curator.py) has no production caller either, now that refresh_context_window is gone. Same stale narration in tests/test_agent_loop_lazy_provider.py:11-13, tests/test_provider_rates.py:741, tests/test_agent_loop_usage_sink.py:223, tests/test_context_engine_factory.py:322.

• deleting tests/test_provider_pin.py also deleted its last test, test_no_surface_writes_the_default_model_without_deciding_its_pin (origin/main:tests/test_provider_pin.py:178) -- a whole-repo AST guard that failed any set_default_model(...) without provider=, and any raw agents.defaults.model write not paired with an agents.defaults.provider write. That guard is the only machine enforcement of this PR's central thesis, and this head has no replacement. The rule still holds; only the "decide it with providers.pin.resolve" half of its message is obsolete. Worth moving to tests/test_provider_resolution_invariants.py with the message updated.

• _migrate_auto_provider runs before the normalizing shims in _migrate_config, and Config is extra="forbid", so a config carrying a legacy top-level skillRouter / skill_router block fails the probe, is skipped with only a DEBUG line -- and is still stamped at v2, so it is never retried. Measured:

  clean provider=auto          -> on-disk 'openrouter'  stamp 2  notices 1
  + top-level skillRouter      -> on-disk 'auto'        stamp 2  notices 0
  + top-level skill_router     -> on-disk 'auto'        stamp 2  notices 0
  

  One-line fix: pop those keys in the probe too, or move the migration after the shims. Narrow, but it lands on the oldest configs, which are the ones most likely to still say auto.

• raven/cli/provider_commands.py:583-604 -- --provider is now required but its value is never checked, and set_default_model writes to disk before credential_status is consulted. raven provider use claude-opus-4-5 --provider antropic exits 0 with the config already changed, and the warning it prints suggests raven provider set antropic --api-key <key>, which would create an orphan section. The same rule is enforced the other way round in the TUI: _set_model builds the provider first and raises before _save_config, so a failed rebuild leaves the file untouched. Worth making the two entry points agree.

• raven/cli/onboard_commands.py:1883 -- removing a provider's credentials still writes set_default_model("", provider="auto"). "" is the cleared spelling now; "auto" is the sentinel this PR retires, and since the watermark is already at v2 the migration will never come back to clean it up.

• raven/providers/pool.py:59 -- _credentials_fingerprint covers only config.providers, but the cached value now carries configured_window from agents.defaults.context_window_tokens (:93). Changing only that number leaves the fingerprint unchanged and the cached binding stale. Not a regression (the number was construction-time on main too), but the cache key no longer covers the cached value.

• raven/providers/pool.py:186 calls section_is_usable(section, find_by_name(provider_name)) without passing name, so a passthrough vendor goes through credential_status("", ..., spec=None). It answers correctly today, but only because that path falls back to the generic api-key method. Passing provider_name through, or a one-line comment, would make it deliberate rather than incidental.

• outside this diff, same bug class: raven/token_wise/pricing.py and raven/providers/rates.py look up rates by model id, so a call billed through a gateway and the same id billed direct resolve to the same number. This is the other face of the "an id does not name whose credential serves it" problem this PR fixes, and it is untouched -- worth its own issue rather than anything here.

nit

• ui-tui/src/app/slash/commands/session.ts:145,150 -- .catch(ctx.guardedErr) twice in a row; the second never runs. Note the explanatory comment sits between them, attached to the dead one, so deleting line 150 outright removes the only "why" on this chain: move the comment onto the surviving .catch first.
• ui-tui/src/app/slash/commands/session.ts:117 -- ...(provider ? { provider } : {}) cannot be false; :105-109 already returned when provider is falsy.
• ui-tui/package-lock.json -- the 18 deleted lines are libc platform metadata on optional dev dependencies, with no version or package change and no package.json edit. That is npm-version drift from one machine, not a dependency change; git checkout <base> -- ui-tui/package-lock.json takes the file out of the diff.

---

One process note: the branch is on the current main tip and all 11 checks are green, but mergeStateStatus is BLOCKED because the protect-branches ruleset wants one approving review and every review on this PR so far is COMMENTED.

[gloryfromca]

I checked your review against the tree rather than taking it, since two of the items are about things I said. Four notes; the branch has not moved, so this is not a re-review.

R8 is right and I was wrong. The save_config premise was mine, from the R6/gateway thread, and it does not hold: save_config dumps schema.Config, whose nine fields do not include context -- that is an extension block read by load_raven_config and never written back. Confirmed against a temp $HOME on this head (no context key, no curatorModel, git grep curatorModel -- raven/ tests/ empty). Retracted in the thread where I said it, with the measurement, so the author does not have to weigh two contradicting reviewers. The release note should be left alone.

R2 and R4 are real, and my summary on this head was wrong to say "everything else on this revision is a suggestion". Both verified here before I say so:

• R2: github/main:raven/agent/loop/main.py:687 clears _image_tool_result_ok inside _adopt_provider. On 6beb459, grep -n "_image_tool_result_ok\|_vision_ok" raven/agent/loop/main.py returns the two constructor lines at :363-364, the read/compute sites at :928-942, and the negative write at :2160 -- and no .clear() anywhere. :936 does compute the verdict from self.provider while keying on the model id, so the staleness is exactly the one the deleted comment named. Regression against main, caused by this diff.
• R4: confirmed at all four points -- the footer this PR changed from scope: global (modelPicker.tsx:1426), the strip-then-parse at session.ts:96-99, the refusal at :108 built from value alone so the flag is dropped, and useMainApp.ts:829 sending no --default. The only spelling that changes the default appears in none of the copy.

Credit is yours on both; I am not re-anchoring them. But the scope sentence in my summary should read "plus R2 and R4", and I would rather say that than leave it standing.

The theme test -- we can both stop wondering, and the author's framing was closer than mine. It is TERM-dependent, not ordering and not load:

uv run pytest tests/test_cli_theme.py -q              -> 1 failed, 44 passed
TERM=dumb uv run pytest tests/test_cli_theme.py -q    -> 45 passed

The repo's own coverage: target pins TERM=dumb (Makefile:63), which is why CI and your worktree never see it. So my "it will not go away on a quieter machine", offered as a correction to the description's "machine-dependent", was the wrong mechanism -- the description was right and I was over-precise. Either way it is a file this PR does not touch. (Your 6681 vs my 6443 is --all-extras: with the channel SDKs installed the seven channel test files collect instead of skipping.)

Where we differ, mildly: R3. Same facts -- I reproduced the passthrough case and posted it in that thread with a patch. I called it too narrow to hold the PR for; you call it before-merge. That is a judgement call about one picker row, not a disagreement about the code, and it is the author's to settle.

My position on this revision is unchanged and unchanged for R1's sake: it should not merge until the migration watermark is per-migration. R2 and R4 are now on that list too.

[arelchan]

Round 2 addressed at d7bd1c67 (six commits on 6beb459). Every claim was reproduced here before it was fixed, and each fix was mutation-checked -- reverting it turns the new tests red.

R1 -- each run-once migration now carries the generation that introduced it and is gated on that, so a bump cannot resurrect a past one. Your diagnosis of why the existing test could not see it was the important half: test_context_window_pin_survives_once_stamped built its precondition from CURRENT_CONFIG_VERSION, so it moved with the bump and could only ever test the generation it was run under. It now pins the literal {"version": 1}, plus two tests for the upgrade path in both directions -- a gen-1 config keeps a restored pin, and still picks up gen 2.

R2 -- both setters call one _forget_transport_verdicts(). _vision_ok is in it too, since it has never had an invalidation point and it was the same line in the same place. Your "computed from the provider" sentence is back on the constructor comment. set_provider also carried a second half of the same class: it built ModelBinding(provider, model) with no _configured_window, so a pinned window would be dropped the day it grew a caller.

R4 -- the flag rides into the overlay state and back out through the picker's callback; the refusal keeps it in both halves of its suggestion; the footer writes /model <provider> <id> --default; and the picker shows which scope its selection will use. Worth naming: one existing vitest case asserted the refusal text that dropped the flag, so the suite was holding the defect in place. It now asserts the spelling that works.

R3 -- taken, with @gloryfromca's patch. His narrowness call is fair, but the row a user reads to answer "whose key is paying for this" is not a row I want wrong, and the fix is small now that the id names its provider. A head naming no configured provider stars nothing.

R5, R6 -- Provider Pin is now Configured provider, which resolves the collision with Subsystem pin and stops pointing at a deleted file. All four gateway sentences say what the branches do, including the asymmetry: an explicitly named vendor that turns out unusable is dropped, while the gateway branch binds whatever it is handed, because a gateway having no route for an id is not visible from there.

R7 -- description rewritten. "Dissolved by the work itself" was wrong about both R3 and R6; it now says three rounds as they actually went. Also corrected: the Risk section's claim that the sidecar "is ignored by any build that does not look for it" -- every build since #341 does look, and after a revert reads {"version": 2}.

R8 -- taken as written, and thanks to both of you for converging on it rather than leaving me two contradicting reviews. The release note is unchanged.

Follow-ups taken, limited to defects in what this PR itself added or removed: clear_session_binding moved out of the if removed conjunct; _migrate_auto_provider's probe now keeps only what Config declares by field name or alias, so the legacy skillRouter case stops failing silently and being stamped anyway (a name filter rather than a pop list, so the next shim does not have to remember this one); the stale lazy.py and set_provider docstrings; onboarding's provider="auto"; and package-lock.json reverted to main.

Left for their own issues, as you both suggested: the deleted AST guard from test_provider_pin.py (it is the only machine enforcement of this PR's thesis and deserves a real home in test_provider_resolution_invariants.py, not a rushed one here); provider use writing to disk before checking the credential; _credentials_fingerprint not covering configured_window; pool.py:186's incidental spec=None path; and the rate-lookup-by-id problem in token_wise/pricing.py and providers/rates.py, which is the same bug class one layer over.

The theme test: confirmed TERM-dependent, and the run below is TERM=dumb -- what Makefile's coverage target pins and what CI runs.

pytest tests/                  6452 passed, 43 skipped, 13 deselected
ruff check / format --check    clean
npx tsc --noEmit               clean
npm test (ui-tui)              993 passed (86 files)
npm run lint:rpc               generated.ts in sync

[gloryfromca]

No blockers; this can merge as far as I am concerned. My R1 was the one thing I was holding it for, and it is properly fixed -- not papered over.

Six commits on 6beb459. I read the delta, re-ran every reproduction I had posted, and mutation-checked the fix to my own finding rather than taking the claim.

R1, the blocker I raised -- fixed, and the test now discriminates

Per-migration floors (_CONTEXT_WINDOW_MIGRATION = 1, _AUTO_PROVIDER_MIGRATION = 2), with _migrate_config's from_version defaulting to the current mark so a caller that cannot read the watermark runs none. Both directions measured on d7bd1c67:

gen-1 stamp + hand-restored 65536   in-memory 65536   on-disk 65536   stamp -> 2   notices 0
gen-1 stamp + provider: auto        on-disk provider -> openrouter    stamp -> 2
unstamped + top-level skillRouter   on-disk provider -> openrouter    (was silently skipped and stamped)

And the test genuinely holds it. Setting _CONTEXT_WINDOW_MIGRATION = 2 -- the shared-mark behaviour, one line -- turns test_context_window_pin_survives_once_stamped and test_a_later_generation_does_not_reopen_a_migration_already_run red. The precondition is now the literal {"version": 1} rather than CURRENT_CONFIG_VERSION, which was the half of my note that mattered.

The rest, verified rather than assumed

• R2 -- _forget_transport_verdicts() on both setters, _vision_ok included. I checked the objection I would have raised against clearing wholesale on every session switch: supports_image_tool_result is pure spec/type inspection with no I/O (capabilities.py:83-108), so a re-probe costs nothing. The "computed from the provider" sentence is back at :360-362.
• R3 -- reads the stored head. Measured: mistral/mistral-large-latest stars mistral, openrouter/anthropic/claude-haiku-4-5 stars openrouter, and an unknown head stars nothing rather than the configured default.
• R4 -- all four points closed, and the boolean -> boolean | 'default' widening is safe: every reader of overlay.modelPicker tests truthiness (overlayStore.ts:29, useInputHandlers.ts:168, appOverlays.tsx:137,165), none compares === true.
• F1 / R5 / R6 -- set_provider's docstring now says "no production caller today" and carries _configured_window (which ModelBinding does accept as its third field); Provider Pin is now Configured provider with the collision against Subsystem pin called out in _Avoid_, no dangling providers/pin.py, and the claim is verifiable against _migrate_auto_provider as I ran it; all four gateway sentences now describe the asymmetry. git grep "reported and dropped\|deriv\w* the vendor from the id" leaves only the one occurrence that is now true of the explicit branch.
• Follow-ups -- clear_session_binding out of the if removed conjunct, provider="" instead of "auto" in onboarding, the lazy.py docstring, and package-lock.json back to main (git diff github/main...HEAD -- ui-tui/package-lock.json is empty). The five items deferred to their own issues are the right five; none is a defect this PR introduced.
• Tests weakened? The opposite, and the author said so rather than leaving it: one existing vitest case had been pinning R4's dropped flag in place, and it now asserts the spelling that works. Nothing else was loosened; no skip or xfail added.

Test state, measured on d7bd1c67

TERM=dumb uv run pytest tests/   6689 passed, 33 skipped, 13 deselected, 0 failed
uv run ruff check / format       clean, 832 files formatted
npx vitest run    (ui-tui)       993 passed (86 files)
npx tsc --noEmit                 clean
npm run lint:rpc                 generated.ts in sync
npx eslint src/                  0 errors, 22 pre-existing warnings

Closing the loop on the disagreement from the last round: TERM=dumb is what makes the theme test pass, so nothing fails here any more. My earlier count (6443) versus yours was my own environment -- the channel SDKs got installed into this venv part way through, so those seven files collect instead of skipping. The branch never had anything to do with either number.

One nonblocking note inline, about a test that stopped compiling and a seam left untested. Neither misbehaves at runtime.

[arelchan]

Rebased onto main now that #346 has merged. New head 2148b1fb; the branch is 17 commits on 5a0c950c. Flagging it because the base moved, not just the tip -- comparing against the previous head alone would not show why.

One conflict, and it was additive. tests/test_config_loader.py: #346 added two save_config tests, this branch adds the auto-provider group. Both sides kept, nothing dropped. Everything else in #346 -- the stderr routing for migration notices, exclude_defaults in save_config -- applied without touching this branch's changes to the same file, which are about when the stamped migrations run rather than what save_config writes.

Two commits are mine cleaning up after that resolution, both trivial and both worth naming so they are not mistaken for content: 2148b1fb restores two blank lines the marker removal ate (ruff format --check caught it), and it was first committed as style(...) -- a type commitlint.config.cjs does not allow -- so I reworded it to test(...) and force-pushed again. That is the only history rewrite in this push beyond the rebase itself.

Re-verified on the new base, not carried over from the last run:

pytest tests/                  6463 passed, 43 skipped, 13 deselected   (TERM=dumb)
ruff check / format --check    clean
npx tsc --noEmit               clean
npm test (ui-tui)              993 passed (86 files)
npm run lint:rpc               generated.ts in sync
commitlint + check_commit_messages.py   both clean, run locally this time

No finding from either review round was re-opened by the rebase: the migration floors, the capability-cache invalidation, the --default path, the passthrough-vendor row and the four gateway sentences are all as reviewed at d7bd1c67.

@gloryfromca -- you asked that this not merge until the migration watermark was per-migration, and later added R2 and R4 to that list. All three are in, each reproduced before being fixed and each mutation-checked (reverting the fix turns the new tests red). A re-look when you have time would be welcome; the PR still needs one approval to become mergeable.

[gloryfromca]

No blockers; this can merge as far as I am concerned. Nothing new found -- this revision is the d7bd1c67 work replayed onto a main that gained #346, plus two blank lines, and I checked the seam rather than assuming it was clean.

One orientation note, because the PR still shows a red commit messages run: the head I was handed was 98115e15, whose style(test): header the repo's own linter rejects (style is not in ALLOWED_TYPES, and not in AGENTS.md's table). The pushed head 2148b1fb amends it to test(config): with a byte-identical tree, and scripts/check_commit_messages.py github/main..2148b1fb is silent. Already resolved; the red run is on the superseded sha.

What the rebase could have broken, and did not

#346 lands in exactly the code my R1 was about, so this was the whole job:

• R1 survives. Re-ran the reproduction on this tree: a gen-1 stamp with a hand-restored contextWindowTokens: 65536 keeps it in memory and on disk, gets stamped to 2, and prints no notice; a gen-1 config still saying provider: auto still picks up gen 2 (-> openrouter). The per-migration floors and both docstrings came through the replay intact.
• The test_config_loader.py conflict was resolved without losing a side. 40 tests, no duplicate names, both of #346's (test_save_config_writes_only_what_differs_from_the_defaults, test_save_config_keeps_every_value_the_user_chose) present, and all four of the branch's migration tests present with the literal {"version": 1} preconditions still literal -- which was the half of my note that mattered, so the replay did not quietly reintroduce the constant.
• #346's three changes do not collide with this branch. The new _migrate_auto_provider notice goes through the same _migration_notices / drain_migration_notices path #346 moved to stderr, so it inherits that fix rather than reopening it. save_config's new write-only-non-defaults cannot drop a key through config.set model: that path uses its own raw read-modify-write (methods/config.py:148-168), not loader.save_config. And main's new doctor Config section calls config.get_provider_name(), which on this branch answers 'anthropic' for an empty provider with a matching key, None when nothing serves the model, and the explicit name for a passthrough vendor -- None being what doctor already reports as "could not be routed to any configured provider", which is the check #346 deferred to this PR and which reads correctly here even unwritten.

Test state, measured on this tree

TERM=dumb uv run pytest tests/     6700 passed, 33 skipped, 13 deselected, 0 failed
uv run ruff check / format         clean, 832 files formatted
npx vitest run    (ui-tui)         993 passed (86 files)
npx tsc --noEmit                   clean
npm run lint:rpc                   generated.ts in sync
check_commit_messages.py           silent over github/main..2148b1fb

Still open, unchanged and not re-raised

The scope prop note from my last pass (modelPicker.test.tsx:179 no longer typechecks, and useMainApp.onModelSelect has no test) is untouched here. Still nonblocking, still the two lines it was.

mergeStateStatus is BLOCKED only because the ruleset wants one approving review and every review on this PR is COMMENTED -- nothing left in the code as far as I can see.

[gloryfromca]

No blockers; this can merge as far as I am concerned. One commit since 2148b1fb, test-only, and it closes the concrete half of the last note I had open.

git diff 2148b1fb..229a98f3 -- ':!*__tests__*' is empty, so production code is byte-identical to the revision I cleared last round. That leaves one thing to check, and I checked it rather than reading the commit message:

• The type error is gone. tsc over modelPicker.test.tsx directly reports zero TS2741; the mount helper now takes scope and defaults it to 'session', so every existing case keeps the behaviour it was written against.
• The new case is not idle. Replacing the footer's scope === 'default' with false -- the mutation that reintroduces exactly the missing coverage -- turns says which scope the selection will apply red. It discriminates.
• Everything still green on this head: TERM=dumb pytest tests/ 6700 passed / 0 failed, ruff check and format --check clean, npx vitest run 994 passed (86 files, one more than last round), npx tsc --noEmit clean, lint:rpc in sync, eslint src/ 0 errors. check_commit_messages.py github/main..HEAD is silent, and test(tui) is an established scope on main (11 commits, two of them test(tui)).

Two remarks on the shape of the test, neither worth acting on: expect(defaultFrame).not.toContain('--default sets') is a negative assertion, and a positive one on the default footer would fail closed if that branch ever rendered nothing -- the not.toBe(sessionFrame) line covers most of that, and the commit says why the long phrase cannot be matched intact. And the softer half of my note stands: useMainApp.onModelSelect, the line that appends --default to the shelled command, still has no test. Both are suggestions, neither is new, and I am not re-raising either.

[gloryfromca]

No blockers; this can merge as far as I am concerned, and with this commit I have nothing left open on it.

One commit since 229a98f3, closing the second half of my last note. It touches production code, so I checked the extraction rather than just the new test.

The refactor is behaviour-preserving. modelSelectCommand(model, slug, pending) computes pending === 'default' ? ' --default' : '', which is the same predicate the inline asDefault used, and the call still happens before patchOverlayState({ modelPicker: false }) -- the ordering R4's fix depends on. Identical output for all three overlay values.

The command it builds actually survives the parser, which is the coupling the test only asserts by proxy. Running session.ts's own two regexes over the produced string:

/model anthropic/claude-haiku-4-5 --provider openrouter --default
  asDefault = true
  parsed    = { provider: 'openrouter', value: 'anthropic/claude-haiku-4-5' }

So the flag form, the strip-before-parse, and the refusal of a bare id are all satisfied at once. And the two halves join up: the new unit test pins the string, while createSlashHandler.test.ts already pins that this spelling puts scope: 'default' on the wire.

The new test discriminates. Dropping the --default from modelSelectCommand -- the original defect exactly -- turns two of its four cases red.

One remaining check, since an extraction can leave a second copy to drift: modelSelectCommand is the only place in ui-tui/src that builds an executable /model command. The other /model strings are user-facing advice (the refusal at session.ts:115-116, the picker footer at modelPicker.tsx:1428), not commands the code runs.

Verification on this head

TERM=dumb uv run pytest tests/     6700 passed, 33 skipped, 13 deselected, 0 failed
uv run ruff check / format         clean, 832 files formatted
npx vitest run    (ui-tui)         998 passed
npx tsc --noEmit                   clean
npm run lint:rpc                   generated.ts in sync
npx eslint src/                    0 errors, 22 pre-existing warnings
check_commit_messages.py           silent over github/main..HEAD

Both halves of the note I had open are now closed, each mutation-checked rather than taken on the commit message. Nothing new found on this revision.

[arelchan]

Closing out the review threads, with where each was fixed on the current head (20e2c9d). Resolving rather than replying in seven places, but recording the mapping here so a later reader does not have to reconstruct it:

Thread	Closed by
pool.py:190 -- a pin naming a passthrough vendor took the agent down at startup	fixed on main; schema._has_credentials now takes the name separately instead of reading is_oauth off a None spec. Re-ran the reporter's repro.
pool.py:136 -- gateway pin unlogged, and four sentences describing a rule the branch does not have	faecc0fd. The behaviour is unchanged and deliberate; the documentation was what was wrong, including in this PR's own description.
model.py:316 -- the picker starred another vendor's row for a session running on this one's key	2f54f88f. Reads the head of the stored id; a head naming no configured provider stars nothing.
session.ts:124 -- /model before the first session.create wrote a raw stderr line over the Ink render	the .catch sweep in 1f83c0e7, and 20e2c9d9 now pins the picker's own path through the same command.
config.py:439 -- a switch before the first message manufactured an empty session record	the sessions.exists guard in 1f83c0e7.
loader.py:19 -- bumping the mark re-ran the context-window migration and deleted a restored pin	c53032d8. Per-migration floors, plus two tests covering the upgrade path in both directions, with the precondition pinned to the literal {"version": 1} rather than to the constant that moved.
modelPicker.tsx:1446 -- the new required prop left the component's own render test uncompilable	229a98f3. Worth noting why the pipeline could not see it: tsconfig.json excludes src/__tests__, so tsc --noEmit was clean and silent.

Every one was reproduced before being fixed, and each fix was mutation-checked -- reverting it turns the new test red.

Two things I did not do, both deliberate:

• The gateway pin's missing logger.warning (the actionable half of pool.py:136) is still not there. That thread settled on keeping the behaviour, and adding the log is a change to a branch nobody has complained about; it belongs to its own PR rather than to a round of review fixes.
• ui-tui/package-lock.json is reverted to main and src/app/chatStream.ts's prettier drift is left alone -- the latter is pre-existing on main and untouched by this branch, so formatting it here would only widen the diff.

Follow-ups I raised earlier and left for their own issues -- the provider use write-before-check ordering, _credentials_fingerprint not covering configured_window, the rate-lookup-by-id problem in token_wise/pricing.py, and restoring a session's model on surfaces other than the TUI -- are all now done on a branch stacked on this one. I will open that PR once this merges, so its diff reads against main rather than against this.