fix(*): close the follow-ups the model-binding review left open

[arelchan]

Summary

Six follow-ups from #284's review rounds. Five are defects in what that PR itself added or removed and were deliberately deferred rather than rushed into a diff already under review; the sixth is a check #346 wrote down as belonging here, because it only becomes a real state once the provider is explicit.

A session's model now survives a restart on every surface, not only in the TUI. The read half of the persistence hung off session.resume, so only the surface that calls that handler got it: a conversation arriving from a channel came back on the configured default with its own choice sitting unread in its own session record. The read moves onto the loop, where every entry point already goes, and the handler's duplicate copy is gone -- one reader instead of two. has_session_binding restores first as well, or a session that switched before a restart reports as having inherited the default and the picker stars the wrong row. clear_session_binding counts as having consulted the record, so returning a session to the default is not undone by the next read.

raven provider use refuses an unroutable provider before it writes. A typo exited 0 with the config already changed, then printed a credential warning suggesting raven provider set <typo> --api-key ... -- advice that would have created a section for a vendor nothing routes to. provider set already applied the right test before writing a section; it is exposed as ensure_routable_provider and called first, which is the order the TUI's /model already used. Only an unroutable name is refused: a known provider with no credentials yet is still written and reported, because picking a model before configuring its key is a normal order to do things in. "No spec of ours" cannot be the typo test either -- Raven carries none for mistral or xai, and refusing on it would block every passthrough vendor the config documents as supported.

The guard that keeps a model and its provider paired has a home again. A whole-repo AST scan failing any set_default_model without provider=, and any raw agents.defaults.model write not paired with a provider write, lived in tests/test_provider_pin.py and was deleted with that file. Only half its message was obsolete; the rule it enforces became more central, and it is the only place a machine checks it. test_provider_resolution_invariants.py is where the other source guards of this kind already live.

The binding cache is keyed on everything it caches. The fingerprint covered config.providers, but bind also copies contextWindowTokens onto the binding, so editing that number left a stale binding answering with the old one until a restart -- the "I changed it and nothing happened" shape the window ladder exists to prevent, arriving through the cache instead.

A gateway call is priced from the gateway's own table. Who routes a request is not who bills for it. LiteLLM answered first for every id, including one naming a gateway, and the two disagree: measured on the pinned LiteLLM, openrouter/z-ai/glm-4.6 is filed at 0.40/1.75 per million where OpenRouter's own current table says 0.50/2.00, so every such call was reported a fifth under its real cost. OpenRouter's catalogue now answers ahead of LiteLLM for ids that name it, fresh-only and never fetching: an expired copy is not better evidence than the router's table and would suppress the refetch the expiry exists to trigger, while a blocking fetch would be paid by the turn this runs after. Unchanged for a direct id, which is what once priced a self-hosted deployment at a hosted model's rate.

raven doctor can ask about a provider nothing resolved. #346 deferred this with its reason: before the explicit-provider rule, the field defaulted to auto and blank never happened. Now blank means the load-time migration tried the derivation and had no answer. Two findings -- blank, and a name nothing routes to -- both report-only. Neither is offered to --fix, and that is not an omission: only the user knows which vendor they meant to pay, and the migration has already tried. Guessing again under the name --fix would be the same guess wearing more confidence.

Type

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

Verification

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

Every fix is mutation-checked rather than assumed -- reverting it turns the new test red. That is worth naming for two of them, where writing the test taught something the fix alone did not:

• The doctor check's scope is narrower than it looks. With a configured vendor that serves the model, the migration fills the blank during load_config and there is nothing left to report, so the fixture has to be genuinely unresolvable -- which is exactly when the finding is real.
• The first attempt at the pricing tier read the any-age cache and made a stale disk copy outrank a refetch. test_expired_disk_triggers_refetch caught it, which is why the tier is fresh-only.

Three review rounds landed on this PR after it opened, each fixed and each verified rather than
taken from the report:

• Blocking: the doctor provider check read a leftover auto as an unroutable vendor name. That
  is the legacy state the check exists for -- _migrate_auto_provider leaves the literal in place
  when it cannot resolve one -- so the user was told to fix a typo they did not make. It now matches
  Config._match_provider's own forced != "auto" test, and says the word is retired rather than
  merely ignoring it. Neither neighbouring test used a literal auto, which is how the branch
  between them went unentered.

• _restore_attempted.add in clear_session_binding was inert, and its stated reason did not hold
  for the only caller: session.delete unlinks the record before the clear runs. Dropped rather than
  relocated -- marking in set_session_binding would claim the record had been consulted when it had
  not. The test that was supposed to guard it is renamed and its docstring now scopes what it does
  and does not pin.

• _routes_anywhere caught Exception where the sibling check catches KeyError, and the new
  refusal printed a different failure marker from the eleven others in its file.

• Relevant tests pass locally

• Relevant lint / type checks pass locally

• User-facing docs or screenshots are updated when needed

Risk

Two user-visible changes, both deliberate:

raven provider use now exits 1 on a provider name nothing routes to, where it used to exit 0 with the config already written. A script passing a misspelled --provider starts failing; a script passing a real one is unaffected, including for a passthrough vendor.

Cost figures change for calls made through OpenRouter on ids where its catalogue and LiteLLM's copy disagree. The new number is the one the gateway bills, so this corrects a systematic under-report rather than introducing a new figure -- but a user comparing today's total to yesterday's will see the step.

Rollback: revert the commits. Nothing here writes a new field or changes a stored shape, so a revert needs no migration.

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

Related Issues

N/A -- follow-ups from the review of #284, which is merged.

[claude]

Claude finished @arelchan's task in 48s —— View job

---

Reviewing PR #349

• Read prior review rounds (mine and gloryfromca's) to avoid repeats
• Diff the latest delta (e170bb04..f1c52ba) against what's already been reviewed
• Check correctness / edge cases in the delta
• Check repo conventions (commit message, test naming)
• Post final findings

No blockers; this can merge.

Reviewing only the new delta, e170bb04..f1c52ba (everything before that was covered in earlier rounds by me and @gloryfromca, and nothing in this delta touches those areas).

The delta

One commit, one file, docstring-only: tests/test_agent_loop_session_model.py:588-593. It resolves gloryfromca's last open nit on test_a_restored_session_stays_on_the_default_once_cleared — the docstring previously said test_the_stored_model_is_read_once_per_session "pins the marking itself," which overstated it (the test only pins the write in _restore_once at raven/agent/loop/main.py:695; the redundant write in restore_session_model at main.py:739 is unpinned).

I checked the factual claim against the code rather than taking it on faith: _restore_attempted has exactly two writers, main.py:695 (_restore_once) and main.py:739 (restore_session_model), matching what the new wording says. The new sentence — "pins the marking in _restore_once; the redundant one in restore_session_model is unpinned, and deleting it passes the suite" — is accurate and doesn't overclaim.

No code behavior changes in this commit, so no new mutation check is needed beyond what's already on record from the prior rounds.

Conventions

Commit header docs(test): name which of the two markings this suite actually pins — conventional, correct scope, ASCII, under 100 chars, Co-authored-by trailer present. No new test file, so AGENTS.md 5.1/5.4 still hold.

This closes every item raised across the review thread (the auto sentinel blocker, the except KeyError narrowing, the inert clear_session_binding marking, the [red]x[/red] marker mismatch, and now this docstring precision nit). Nothing new to raise.

[gloryfromca]

No blockers; this can merge as far as I am concerned. One nonblocking note inline about the sixth change being unreachable and unguarded, and a one-character nit below.

What I checked

• The diff vs github/main (6 commits, 14 files), plus the callers and the code around each change: AgentLoop.run_turn / binding_for_session, tui_rpc/methods/config.py::_remember_session_model and _set_model, session.delete / session.branch, model_options, ProviderPool.bind, cli/_helpers.make_provider, config/update_providers._provider_schema_cls / _litellm_knows, model_catalog_cache.load, token_wise/pricing.estimate_cost_usd.
• Repo conventions: commit grammar, scopes, ASCII, Co-authored-by trailers (all six clean); test file naming under AGENTS.md 5.1/5.4 (existing files updated, no new files, no phase/ticket suffixes); no new domain term coined, so no CONTEXT-MAP change owed; no assets.
• Backward compatibility of the two behaviour changes the description names.
• Whether tests were weakened, by mutation rather than by reading.

Tests, actually run

TERM=dumb uv run pytest tests/ -q -p no:randomly
6476 passed, 43 skipped, 13 deselected, 1 warning in 152.26s

That matches the description exactly. I looked at every skip: 8 are optional-dependency imports (dingtalk_stream, lark_oapi, nio, botpy, slack_sdk, telegram, wecom_aibot_sdk), and 33 are data-driven parametrize exclusions in test_provider_catalog.py / test_provider_resolution_invariants.py that pre-date this branch. Nothing here is skipped to look green.

Mutation checks I ran myself

I reverted three of the six fixes in place and re-ran the tests named as their guards:

reverted	result
_cache_fingerprint back to credentials-only	test_an_edited_window_drops_the_binding_that_carried_the_old_one FAILS -- real guard
tier 0 removed from the token_rates ladder	test_the_gateway_price_wins_over_the_routers_copy FAILS on (2e-06, 8e-06) != (4e-06, 1.6e-05) -- real guard
_restore_attempted.add removed from clear_session_binding	all 94 tests in test_agent_loop_session_model.py + test_tui_rpc_session.py still PASS

The third is the inline note. Working tree restored to HEAD afterwards.

Things I considered and dropped

• _cache_fingerprint overclaims. Its docstring says "everything a cached binding was built from", but bind -> make_provider also reads agents.defaults.model_overrides, temperature, reasoning_effort and llm_call_timeout, none of which are in the key. I chased this and could not make it fail: config.set's agent.temperature writes an agent.temperature key, not agents.defaults.temperature, so it never reaches make_provider; and nothing rebuilds the default binding on a raw file edit of those fields either, so dropping the pool entry would not help. contextWindowTokens really is the only one of the group with a live-update path, so the fix is complete for every reachable case -- the docstring is just wider than the key.
• _routes_anywhere pays the ~2s authoritative LiteLLM import in a report-only command, which is the cost _litellm_knows documents as one "a command that only reports configuration should not" pay. But authoritative=False would answer False for a vendor newer than the bundled snapshot, i.e. it would tell a user with a working provider that nothing routes to it -- worse than 2 seconds in a diagnostic, and the opposite of what the helper's own docstring argues for. True is the right call here.
• Tier 0 runs _fresh_openrouter_models() unconditionally, before the cheap openrouter/ prefix test, on a path that runs after every completion. On a miss model_catalog_cache.load() is one path.exists(); on a hit it fills the module cache once and every later call short-circuits. Not worth a branch.
• Tier 0 seeds _OPENROUTER_CACHE from an expired disk copy. I checked this cannot suppress the refetch: _fetch_openrouter_models TTL-checks the in-process cache at line 265 before the disk copy at 270, so tier 2 still goes to the network. test_expired_disk_triggers_refetch covers it.
• provider use with no --provider cannot reach ensure_routable_provider("") -- the required-option guard above it already exits 1.

Nit

provider_commands.py:606 prints [red]x[/red] where the sibling failure three lines up, and the other 16 error prints in the file, use [red]✗[/red]. There is precedent for the ASCII form in onboard_commands.py, so this is taste rather than a rule -- but the two error paths of one command reading differently is easy to even up while you are here.

The substance

The five real fixes hold up. Moving the read onto binding_for_session is the right place for it -- every entry point goes through run_turn, and test_a_channel_turn_restores_the_model_that_session_chose exercises the loop with no resume call anywhere, which is exactly the gap. has_session_binding restoring first matters more than the description makes it sound: session.branch gates the child's inherited binding on it, so before this a fork of a restored session silently dropped to the default. And _restore_attempted remembering the miss is what keeps this off the per-turn path for the sessions that never switched -- test_the_stored_model_is_read_once_per_session pins the negative, which is the half that is easy to forget.

[arelchan]

Blocker fixed in 01b25ed7, reproduced first rather than taken from the description:

config: {"agents": {"defaults": {"model": "some/unclaimed-model", "provider": "auto"}}}

after load_config, provider = 'auto'
finding: agents.defaults.provider is 'auto', which nothing routes to. ...
         raven provider list  # the names this accepts

Exactly as reported, and the diagnosis is right about why it matters: this is the legacy state the check exists for, and reading it as a name tells the user to fix a typo they did not make while the advice that would help goes unsaid. Config._match_provider already treats it as unset (forced != "auto", schema.py:921); the check now does the same.

One thing added beyond the fix: when the literal is present, doctor says so -- (your config says auto, which is the retired spelling of unset -- it never detected anything). Silently treating it as unset would be correct and still leave the user staring at a word in their own file with no idea it stopped meaning anything.

On how the gap survived: neither of the two checks around it used a literal auto -- one used "", the other a misspelled vendor -- so the branch between them was never entered. That is the same shape as the scope prop note on #284: a test that exercises the neighbours of a case is not a test of the case. The new test uses the literal, and mutation-checking it (reverting to if not provider:) turns it red.

The nit is taken too: _routes_anywhere now catches KeyError only, matching provider use. Your reasoning is the part worth keeping -- a broad catch files a genuine fault in the lookup as an ordinary "unroutable" finding, which reads as the user's problem rather than ours.

pytest tests/                  6477 passed, 43 skipped, 13 deselected   (TERM=dumb)
ruff check / format --check    clean, 832 files
commitlint + ASCII check       clean over github/main..HEAD

[gloryfromca]

No blockers; this can merge as far as I am concerned.

Re-reviewing only the delta 699d54de..01b25ed7 -- the substance I went through last round is unchanged.

The auto blocker is genuinely fixed

I checked the reachability claim rather than taking it from the commit message. _migrate_auto_provider (config/loader.py:277-278) returns False on if not resolved: before the defaults["provider"] = resolved write two lines down, so the literal survives load_config -- the bot's diagnosis was right and so is the reproduction. And schema.py:922 really is if forced and forced != "auto":, so provider == "auto" mirrors the runtime's own test exactly, including its case-sensitivity. Matching the runtime is the right bar here; a case-insensitive doctor would report a state the runtime does not treat that way.

Mutation-checked myself: reverting or provider == "auto" back to if not provider: turns test_doctor_reads_a_leftover_auto_as_unset_not_as_a_typo red. The test also pins the half that is easy to leave out -- assert not any("nothing routes to" ...) -- so it fails a fix that reports both findings at once rather than replacing the wrong one. Working tree restored afterwards.

The extra (your config says \auto` ...)line is two-space indented, which is the form_render_human_output` already dims, so it renders as a continuation rather than as a third bullet.

The except KeyError narrowing is safe

I went looking for a currently-reachable non-KeyError that the old broad catch was absorbing, because _inspect_config_health is called unguarded at doctor_commands.py:669 -- if one existed, narrowing would turn a report into a traceback. There is none: _litellm_knows already funnels every failure of the LiteLLM import into None, and the two module-level imports left outside its try (litellm_provider_names, litellm_setup) are first-party and stdlib-only at module scope. So nothing in that call path raises anything but the two explicit KeyErrors. The nit was right and the fix does not cost the report-only guarantee.

Tests, actually run

TERM=dumb uv run pytest tests/ -q -p no:randomly
6477 passed, 43 skipped, 13 deselected, 1 warning in 121.55s

One more than the previous revision, which is the one new test. Identical skip set to last round -- 8 optional-dependency imports, 33 data-driven parametrize exclusions, none of them new and none of them hiding this change.

Commit is conventional, ASCII, 68-char header, Co-authored-by present. No new test file, so AGENTS.md 5.1/5.4 still hold.

Considered and dropped

Doctor strips before comparing ((... or "").strip() == "auto") while _match_provider compares the raw value, so provider: " auto " diverges. It is not worth raising: the .strip() pre-dates this commit, and in the divergent case doctor's message is the more useful of the two anyway -- it points at raven provider use --provider <name>, which is the advice that helps.

Still open from last round, both nonblocking

• raven/agent/loop/main.py:773 -- my note on clear_session_binding stands as written; no reply yet, and it does not block.
• raven/cli/provider_commands.py:606 -- the [red]x[/red] / [red]✗[/red] mismatch is still there.

[gloryfromca]

No blockers; this can merge as far as I am concerned.

Reviewing only the delta 01b25ed7..8221ad85. The change is the right resolution of my note and the docstrings are now honest about what the code does and does not promise. One inline nit on a sentence in the new test docstring that overstates its guard by half -- verified by mutation, and nonblocking.

The drop is correct and safe

_restore_attempted now has exactly two writers (main.py:695, main.py:739) and none in clear_session_binding. I re-checked the only caller: session.delete runs mgr.delete(session_key) before clear(session_key), and SessionManager.delete calls self.invalidate(key) then unlinks -- so peek afterwards misses in both the cache and on disk regardless of delete's return value. Nothing can read the choice back in, which is what makes dropping the marking a no-op rather than a behaviour change.

I agree with dropping rather than moving it to set_session_binding, and for the reason given: marking there would assert the record had been consulted when it had not. The old line was inert; that one would have been wrong. Trading an idle invariant for a false one is not an improvement.

The new docstring is also right that clearing an unread key would leave it readable afterwards, and right that no caller creates that state. A documented non-guarantee of an unreachable state owes no test.

Mutation checks I ran

reverted	result
mark removed from _restore_once (main.py:695)	test_the_stored_model_is_read_once_per_session FAILS, test_a_restored_session_stays_on_the_default_once_cleared stays green
mark removed from restore_session_model (main.py:739)	all 94 green

The first row is exactly what the commit message claims. The second is the inline note. Working tree restored to HEAD after each.

Tests, actually run

TERM=dumb uv run pytest tests/ -q -p no:randomly
6477 passed, 43 skipped, 13 deselected, 1 warning in 125.14s

Same count and the same skip set as the previous revision -- 8 optional-dependency imports, 33 data-driven parametrize exclusions. The test was renamed, not removed, and its assertions are unchanged, so nothing was traded away for the green.

Commit is conventional: refactor(agent) is a real subpackage scope, 71-char header, ASCII throughout, Co-authored-by present. Test file naming untouched, so AGENTS.md 5.1/5.4 still hold.

Still open, nonblocking

raven/cli/provider_commands.py:606 -- the [red]x[/red] / [red]✗[/red] mismatch.

[gloryfromca]

No blockers; this can merge as far as I am concerned.

Delta 8221ad85..e170bb04 is the one-character marker swap I raised, and it is complete: no [red]x[/red] left in provider_commands.py, and the cross count goes 16 -> 17, so the new refusal now matches the missing---provider refusal three lines above it. Nothing in the tests was coupled to the old literal -- test_use_refuses_a_misspelled_provider_before_it_writes asserts on "antropic" in r.output, not on the marker -- so this could not have been green by accident.

TERM=dumb uv run pytest tests/ -q -p no:randomly
6477 passed, 43 skipped, 13 deselected, 1 warning in 123.66s

ruff check / format --check raven/cli/provider_commands.py    clean

Identical counts and skip set to the last three revisions. Commit is conventional: 60-char header, Co-authored-by present, and the message describes the glyph in words rather than embedding it, so the message stays ASCII under AGENTS.md 3.1.1 while the source keeps the character it needs.

That closes every item I raised except one, which does not block: tests/test_agent_loop_session_model.py:592 still names test_the_stored_model_is_read_once_per_session as pinning "the marking" when it pins only the one in _restore_once. It is a sentence in a docstring, the fix is to name which writer, and it is fine to land as is or to fold in later.

[gloryfromca]

No blockers; this can merge as far as I am concerned. Every item from all four rounds is now closed, and I have nothing left open.

Delta e170bb04..f1c52ba3 is docstring-only -- no code, no assertion changed -- and it says the accurate thing.

One detail worth recording, because the new wording claims more than my note did. I said the redundant marking "can be deleted with the suite green" on the evidence of 94 tests in two files; this docstring says "deleting it passes the suite." So I re-ran mutation B against everything rather than let the stronger claim stand on my narrower check:

# _restore_attempted.add removed from restore_session_model
TERM=dumb uv run pytest tests/ -q -p no:randomly
6477 passed, 43 skipped, 13 deselected

The whole-suite claim holds -- nothing anywhere else pins that marking. Working tree restored to HEAD afterwards, and tests/test_agent_loop_session_model.py is 28 passed at HEAD.

Keeping the redundant marking is the right call for the reason given: a caller who supplies the pair has genuinely consulted the record, which is exactly the property clear_session_binding lacked. Redundant and honest is a different thing from inert and false, and the docstring now distinguishes them by name instead of gesturing at "the marking".

Commit is conventional -- 67-char header, ASCII, Co-authored-by present.

I checked docs(test) against AGENTS.md 3.1's "a top-level subpackage of raven/" and am not raising it: neither commitlint.config.cjs nor scripts/commit_lint.py enforces a scope enum, and the last 400 commits already carry deps, install, build, readme, release, tools, commitlint and one prior test. The rule is applied loosely here by long practice, so this is precedent rather than a miss.