fix(*): stop an old default from capping every model's context window - #341
Conversation
|
Claude finished @arelchan's task in 5m 29s —— View job Review of PR #341Blocking: the new Prior blockers/nits, now resolved by commit
New finding:
Also checked, no issues found: |
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the top-level configVersion stamp makes every earlier raven build refuse to boot on a config this migration touched -- including the git revert rollback the description promises. The diagnosis and the core fix are right, and the tests are honest; two smaller findings below the blocker.
What holds up, so it is on the record:
- The fix really does fix it.
AgentDefaults.context_window_tokensisint | None = None(schema.py:267), so popping the key hands the window back toproviders.rates; I confirmed end to end on a config shaped like an upgraded install. - The two design calls that look risky are the right ones. Re-reading the raw file in
_persist_migrated_configinstead of writing back the mappingload_configmigrated is necessary --pop_extension_keyshas already removedmemory/plugins/skillForge-- andtest_migration_write_back_keeps_extension_blockspins it. Avoidingsave_configis likewise necessary;save_configdoesmodel_dump(by_alias=True)(loader.py:300), which is how the fossil got planted in the first place, andtest_migration_write_back_does_not_materialise_defaultspins that too. - Tests are not weakened. 53 passed in
tests/test_config_loader.py tests/test_tui_rpc_session_init_bundle.py, nothing skipped or xfailed, and each new case asserts observable state (on-disk bytes, drained notices) rather than "did not raise". The read-only-home case does chmod the directory rather than mock the failure. - Repo rules: branch
fix/config_legacy_context_window_migrationand the header both match sections 2 and 3, theCo-authored-bytrailer is present, tests went into the existing files instead of new phase-suffixed ones (section 5.4), no assets (section 7).
Covered: the diff, AGENTS.md rules, callers of _migrate_config (including config/raven.py:1356, which is in-memory only and unaffected), the CLI entry points that load config, backward and forward compatibility, and whether the tests were weakened. I did not check the ui-tui side beyond reading the two changed files.
| # an absent key means "generation 0", so this default is free to name the | ||
| # current generation -- and must, so a config written fresh today is never | ||
| # taken for a pre-migration one. | ||
| config_version: int = Field(default=CURRENT_CONFIG_VERSION, alias="configVersion") |
There was a problem hiding this comment.
Blocking. Config is extra="forbid" (line 1015) and load_config turns a ValidationError into a raised ValueError rather than falling back to defaults (loader.py:270-276). So a config carrying configVersion is not merely unrecognised by an older build -- it is a hard boot failure. Confirmed against github/main's schema:
$ # main's Config.model_validate({'configVersion': 1, ...})
OLD SCHEMA REJECTED: ValidationError
configVersion
Extra inputs are not permitted [type=extra_forbidden, input_value=1, input_type=int]
Concrete failures:
- The rollback in the PR description does not work.
git revertremoves this field, and every install that had already launched once now fails at load withConfig at ~/.raven/config.json fails schema validation. The description says those configs "stay migrated ... which is the same state a fresh install has" -- they do not, they carry a key the reverted schema rejects. Since the squash body is this description, that claim lands onmain(section 3.7 asks Risk to state how to roll back). - It fires inside the repo today, not just on a hypothetical downgrade: worktrees share
~/.raven/config.json. Runravenonce on this branch, thenraven statusfrom a checkout ofmain, and it raises. Same for anyone who pins an older release.
Worth noting the escape hatch: Base (line 22) has no extra setting, so it defaults to ignore -- nested unknown keys are silently dropped by old and new builds alike. Stamping inside a Base block, or in a sidecar under ~/.raven/, would make old builds ignore the stamp instead of dying on it, and keeps a revert clean. If you would rather keep the top-level key, then the minimum is to say so in Risk: the revert must retain config_version in the schema, and downgrading to any published release will not boot.
|
|
||
| tmp = path.with_name(path.name + ".migrating") | ||
| try: | ||
| tmp.write_text(json.dumps(raw, indent=2, ensure_ascii=False), encoding="utf-8") |
There was a problem hiding this comment.
The write does not carry the file's mode across. write_text creates the temp file under the process umask and os.replace swaps the inode, so the mode of the original is gone -- which is the exact case config.paths.restrict_to_owner warns about ("a rewrite that replaces the file instead has to set the mode on the replacement itself"). config.json holds providers.*.apiKey (schema.py:364, 372).
Reproduced:
$ chmod 600 config.json # {"agents": {...65536}, "providers": {"anthropic": {"apiKey": "sk-secret"}}}
-rw------- config.json
$ python -c 'load_config(p)'
mode after migration: 0o644
A user who tightened their config to owner-only gets it widened to world-readable, silently, on the first launch after upgrading -- with the key still in it. os.chmod(tmp, path.stat().st_mode & 0o7777) before the os.replace fixes it (skip when the stat fails).
While in here, a smaller one on line 211: the temp name is fixed, so two processes migrating at once use the same path. The second one's write_text truncates the file the first is about to os.replace into place, which can leave config.json momentarily empty for a concurrent reader (that reader boots on defaults with the "not valid JSON" warning). The content does end up whole, so this is not the data loss the fixed name suggests, but path.name + f".migrating.{os.getpid()}" removes the window for free.
| ) | ||
|
|
||
|
|
||
| def print_config_migration_notices() -> None: |
There was a problem hiding this comment.
Only run and gateway call this, but load_config() migrates and rewrites the file from many more commands: status_commands.py:27, doctor_commands.py:172, cron_commands.py:80, provider_commands.py:375, channel_commands.py:439, skill_commands.py:41, sandbox_commands.py:48, import_commands.py:110.
Because the notice is keyed to the migration and the migration stamps the file, the telling is not deferred -- it is lost. If the first raven command a user happens to run after upgrading is raven status (or provider list, or cron list), the config is edited, configVersion: 1 is stamped, nothing is printed, and no later run will ever mention it: the next load sees the stamp and produces no notice.
That is the case the notice exists for. The user who genuinely needs a 64k ceiling (local model, endpoint with --max-model-len) loses the line and the explanation together. Calling this from load_runtime_config, or from the run() wrapper after the command returns, would cover every command instead of two. The TUI path is fine -- tui_commands.py:448 loads in the same process that serves RPC, so _default_session_info drains it.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| // Not a warning: the backend already migrated the config line it names. | ||
| for (const notice of info?.config_notices ?? []) { |
There was a problem hiding this comment.
[blocker] This drains and displays config_notices for session.create, but resumeById's .then(raw => ...) handler further down in this file (around where it reads r.info) never checks r.info?.config_notices (or config_warning/credential_warning). On the backend, session.resume also calls _default_session_info (raven/tui_rpc/methods/session.py:310), which drains the module-level _migration_notices list — a one-shot operation.
So a user whose first RPC call after an upgrade is session.resume (resuming from the picker, or raven tui --resume, before ever calling session.create) has the contextWindowTokens fossil silently stripped from ~/.raven/config.json with no notice shown at all — and it can never surface later in that process, since this drain already consumed and discarded it. That undercuts the PR's stated goal ("the user is told where the user is looking").
Also note SessionResumeResponse.info (ui-tui/src/gatewayTypes.ts:127) doesn't declare config_notices, so the resume path's type doesn't even model this field.
| ) | ||
|
|
||
|
|
||
| def print_config_migration_notices() -> None: |
There was a problem hiding this comment.
[nit] print_config_migration_notices() is only wired into agent_commands.py and gateway_commands.py. Other commands that call load_config()/load_runtime_config() directly — e.g. raven status (raven/cli/status_commands.py:27) and raven doctor (raven/cli/doctor_commands.py:172), both explicitly named in this file's own comments as reload-heavy callers — will silently migrate and persist the configVersion stamp (removing the fossil from the user's config.json) with no notice at all. This mirrors the pre-existing print_deprecated_memory_window_notice wiring (also limited to these two commands), so it may be accepted scope, but unlike that notice this one now has a real disk-write side effect attached to a previously pure-read load_config(). Worth a conscious call on whether status/doctor should also surface it, or whether that's left for the follow-up raven doctor --fix work mentioned in the PR description.
Configs written by 0.1.10 and earlier carry `contextWindowTokens: 65536` verbatim: back then that was the schema default, and the bootstrap dumped every default to disk. Since 0.1.11 the window is sized from the model's own catalogue, but an explicit pin still wins -- correctly, that is what a pin means -- so every upgraded install stayed capped at 64k no matter which model it ran, and `/model` could not shake it loose. The blast radius was never only cosmetic. The window sizes the per-turn history budget, the Curator's fast/slow path threshold and the memory consolidator's archive target, so an upgraded install trimmed history, paid for an extra LLM call per turn and consolidated far earlier than the model required. The single existing signal was a log line nobody reads. Clear the pin where it lives, once, under a new `configVersion` stamp: the value carries no provenance (65536 we planted and 65536 a user chose are the same three bytes), so the stamp is what lets a re-set value stand for good afterwards. It runs at config load rather than inside `raven upgrade` -- the upgrade helper is the outgoing version and cannot know a rule that ships with the new one, and install.sh / uv tool upgrade never call it at all, while every entry point loads the config. A file we did not edit is left byte for byte alone: the stamp lands only where something was actually removed, which keeps the promise commands like `provider use` make about not touching a config they decided against changing. The write itself re-reads the raw file (the mapping load_config migrated has the extension blocks popped, so writing it back would delete the user's memory / plugins / skillForge sections) and never goes through save_config (which dumps ~8 KB of defaults, re-planting this very kind of fossil). A read-only home degrades to an in-memory-only migration. The user is told in the terminal for CLI runs, and through a new `config_notices` field on the session init bundle for the TUI and the served page -- which also gives the client-side `config_warning` / `credential_warning` handlers their first server-side sibling that is actually populated. The notice names the way back, because a window genuinely smaller than 65536 (a local model, an endpoint served with --max-model-len) is a real configuration that this clears once. Drop the loop's 65536 warning: post-migration, a pin that reaches the constructor is a deliberate choice, and warning about those is noise. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Review found the stamp itself was the worse bug. `Config` is `extra='forbid'` and `load_config` raises instead of falling back, so `configVersion` in config.json is a hard boot failure for any build that does not know the key: a reverted release, a pinned older version, or -- already true in this repo -- a second checkout sharing one ~/.raven. Confirmed against a build without the field: `Config at ... fails schema validation`. The rollback the PR description promised did not work either, since a revert would leave every already-migrated config unloadable. Move the watermark to a sidecar (`config.migrations.json`, sibling of the config it describes so a --config path gets its own). No published build reads it, so old builds ignore it instead of dying on it, and reverting this change leaves nothing behind that breaks. The sidecar is ours rather than the user's, which also closes the hole the in-file stamp had to live with: stamp unconditionally, and a `contextWindowTokens` the user sets by hand afterwards is never second-guessed whatever its value -- while config.json is still only touched when the fossil is actually removed. Two more real defects from the same review: - The replace dropped the file mode. `os.replace` swaps the inode, so a config tightened to 0600 (it holds providers.*.apiKey) came back 0644. Reproduced, then fixed by chmod-ing the temp file to the original's mode before the swap; `config.paths.restrict_to_owner` documents this as the replacing writer's job. - The temp path was fixed, so two processes migrating at once shared it and the second's truncating write could be read as an empty config.json. Now per-PID. And the notice reached two commands out of thirteen that load the config. Because the watermark suppresses the next run's notice, an unsaid notice was lost, not deferred: `raven status` (or `provider list`, or `cron list`) would migrate the file silently and no later run could ever mention it -- exactly failing the user who needs a 64k ceiling. Drain from the `run()` wrapper so every command tells, keeping the eager telling in `agent` and `gateway` where it lands before a long-running command takes the screen. Same one-shot drain made `session.resume` swallow the notice for anyone resuming before creating a session (the picker, `--resume`); the resume handler now renders it like the create handler does. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
bcd8f78 to
3156fc3
Compare
|
All four findings reproduced and fixed in The stamp in config.json (blocking). Confirmed exactly as described: a build without the field raises Took the sidecar option: The suggestion turned out to be strictly better than what it replaced, not just safer. Because the sidecar is ours rather than the user's, it can be stamped unconditionally, which closes the hole the in-file stamp had to accept: previously a user hand-setting 65536 on a never-stamped config had it cleared once. Now config.json is touched only when the fossil is actually removed, and a value the user sets afterwards is never second-guessed. The PR description's Risk section is rewritten accordingly, since the squash body is that description. File mode (blocking in effect). Reproduced: 0600 -> 0644 with the apiKey still in the file. Fixed by chmod-ing the temp file to the original's mode before the swap, with the Took the per-PID temp name too. Agreed it is not data loss, but the window costs nothing to remove. Notice wired to two commands out of thirteen. The framing is what made this land: the telling is lost, not deferred, because the watermark leaves the next load with nothing to report. Drained from the session.resume swallowing the notice. Correct, and the picker-first flow is the common case, not the exotic one. The resume handler now renders Full gates after the rebase: 194 passed across the config / provider / RPC / smoke files, 31 in |
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned. Re-reviewed the new head (3156fc3c, rebased on 7bd02804 + the watermark commit). All three findings from my last pass are fixed, and fixed at the root rather than papered over.
Verified rather than taken on trust, on a config shaped like an upgraded install (fossil + providers.anthropic.apiKey + a memory block, chmod 600):
window -> None # pin gone, sized from the model
mode -> 0o600 # preserved across the os.replace
file -> {"agents": {"defaults": {"model": "x/y"}}, "providers": {...}, "memory": {...}}
sidecar -> ['config.json', 'config.migrations.json']
notice -> 1
OLD BUILD SCHEMA: accepts migrated config # main's Config.model_validate, the case that used to raise
second load pin -> None # idempotent
On each finding:
- The stamp moved out of the config entirely (
_stamp_pathsidecar) instead of just documenting the revert hazard. That is the stronger fix: nothing this build knows lands in a file an older build validates withextra="forbid", so a revert, a pinned release and a second checkout sharing~/.ravenall keep booting -- confirmed above againstmain's schema. It also closes the hole the old design accepted: the watermark is now written even when nothing needed removing, so a hand-set 65536 is never second-guessed even once.run_stampedkeeping the extension loader (config/raven.py:1356) out of the stamped path is right -- it only reads blocks, and the base config comes fromload_config. - Mode is carried over before the replace, with the
statfailure swallowed. Matches whatconfig.paths.restrict_to_ownerasks of a replacing writer. - The notice now covers every command via the
finallyincommands.run(), which wrapsapp(), sostatus/provider list/cron listall announce a migration they performed.agentandgatewaydrain first, so there is no double print. The PID-scoped temp name is in too, and_persist_migrationsreturns before stamping when the config write fails, so a read-only home retries instead of recording a migration that never happened. - The bot's
session.resumepoint is handled as well -- resuming first no longer swallows the notices.
Tests: tests/test_config_loader.py tests/test_config_raven_sections.py tests/test_cli_smoke.py tests/test_cli_helpers.py tests/test_tui_rpc_session_init_bundle.py -> 146 passed, 0 skipped; ruff check raven/ tests/ clean. The new cases assert observable state (on-disk bytes, sidecar contents, st_mode, the temp file's name) and the read-only case chmods a real directory rather than mocking the failure.
One nit, comments only, no behaviour: the test section header still says the pin is cleared "under a configVersion stamp", and test_config_without_the_fossil_is_left_byte_identical's docstring still argues "the stamp lands only on a file we actually edited" -- both describe the previous design, and the second is now contradicted by the test right below it. Worth a sweep so the next person bumping CURRENT_CONFIG_VERSION reads the mechanism that exists.
I checked the symlinked-config case (os.replace detaches a ~/.raven/config.json symlinked into dotfiles, leaving the tracked file behind with the fossil) and am not raising it: config/update.py::_write_atomic on main already writes that way, so it is the repo's existing convention rather than something this diff introduces -- and this writer is the one that improved on it by keeping the mode.
Covered this round: the delta since aa56f237, the new sidecar mechanism and its failure paths (unwritable stamp, malformed stamp, race, --config siblings), callers of _migrate_config, forward compatibility against main's schema, the CLI wiring for every command that loads a config, and whether the new tests actually exercise it.
| # So this is the catch-all for every other command. | ||
| from raven.cli._helpers import print_config_migration_notices | ||
|
|
||
| print_config_migration_notices() |
There was a problem hiding this comment.
[blocker] This finally block now runs print_config_migration_notices() after every command, including the two that promise machine-readable stdout: raven doctor --json (doctor_commands.py:510-511, console.print_json(...)) and raven import ... --json (import_commands.py:415-452).
Both are documented for automation (doctor's own help text: "Emit machine-readable JSON (CI-friendly)"). doctor calls load_config() at doctor_commands.py:176, so on the first raven doctor --json run after an upgrade that still carries the retired contextWindowTokens: 65536 pin, the migration fires, a notice gets queued, console.print_json(...) writes the JSON, and this finally then appends [yellow]Config updated: ...[/yellow] on the same stdout stream right after it — json.loads()/jq on the captured output now fails on trailing content. That's exactly the "upgrade, then run a CI health check" scenario this PR is built around.
Before this commit, print_config_migration_notices() was only wired into agent and gateway (neither has a --json mode), so this collision is newly introduced by moving the call into the generic run() wrapper.
Since the repo already routes non-interactive diagnostics to stderr elsewhere in this same file (the ConfigReadError handler at commands.py:171-173 uses Console(stderr=True)) and in loader.py's JSON-decode warning, printing config-migration notices to stderr instead of stdout would fix both --json paths without losing the notice for a human running the same command.
| # Pre-0.1.11 bootstraps dumped every schema default to disk, and back then | ||
| # ``contextWindowTokens`` defaulted to 65536. A pin outranks the model's real | ||
| # window by design, so on upgraded installs that fossil silently caps every | ||
| # model at 64k. It is cleared once, under a ``configVersion`` stamp -- the |
There was a problem hiding this comment.
[nit] This section comment still says "cleared once, under a configVersion stamp" — stale after this PR moved the watermark to the .migrations.json sidecar (_stamp_path). Doesn't affect behavior, just worth a wording pass.
|
Both second-round findings are accepted; merging this as-is and taking them in the immediate follow-up rather than re-opening an approved PR for them. The The fix is to print the notices on stderr, which is where the rest of this class of message already goes -- Scope note: it misfires only on the single first run after an upgrade, and only for a config that still carries the pin, so the exposure on main is one run per affected automation rather than an ongoing break. Weighed against holding an approved fix for a 64k cap that is degrading every upgraded install today, landing now and correcting the stderr target next is the better trade -- the follow-up PR ( Stale test section comment: correct, the wording predates the move to the sidecar. Same follow-up. Thanks for both rounds -- the watermark placement in particular was the difference between a fix and a footgun. |
|
Withdrawing my sign-off on Reproduced on this head, fake HOME with a fossil-carrying config: The config was migrated by that same run, so the corruption and the migration are the same event -- once per install, on the command whose help says "CI-friendly". One thing not yet said, and the reason a green suite told nobody: nothing in the suite drives On merge-now-versus-follow-up, my position is that it belongs here. It is one line ( Your call and the approver's, not mine to make -- but I would rather not be the sign-off that carried it in. |
…#346) ## Summary Three follow-ups from #341, which fixed a retired `contextWindowTokens: 65536` capping every upgraded install at 64k. Two are its loose ends; the third is the reason it happened at all. **The notice no longer prints on stdout.** #341 drains config-migration notices in the `run()` wrapper so every command tells the user, which caught the two whose stdout is a machine-readable document: `raven doctor --json` and `raven import --json`, both advertised for automation. Reproduced -- a config still carrying the retired pin makes `doctor --json` emit JSON with a sentence after it, and `jq` refuses the result. Moved to stderr, where the rest of this class of message already goes (the `ConfigReadError` branch a few lines up in the same wrapper, and the loader's malformed-config warning). That fixes any future `--json` command without it having to know this exists, which allow-listing the two would not. The window is narrow -- the migration fires once per install -- but the run it can spoil is a first run after upgrading, and a CI job is exactly the caller not watching. **`raven doctor` gains a Config section and a `--fix`.** The premise changed while building it: the migrations run at load, so by the time this command looks, nothing is pending -- they already happened, silently, one launch ago. What is left for a person to ask about is what Raven deliberately will not decide for them. So it reports a `contextWindowTokens` pinned below what the configured model holds, and names what rides on it: the per-turn history budget, when the Curator starts paying for a slow path, and when memory consolidation archives. `--fix` removes the pin. Reported without `--fix`, never written: that pin is a real configuration for a local model or an endpoint served smaller than the catalogue thinks, and the flag is the user saying which case theirs is. The write carries the file mode across (config.json holds `providers.*.apiKey`) and uses a per-PID temp name -- the same rules the loader's migration writer follows. One check rather than two. A config naming no provider is the other thing worth asking about, but it reads differently on either side of the explicit-provider rule in #284, so it belongs to that change; a model that routes to nothing is already reported above it. **The bootstrap stops writing every default into the user's config.** This is the cause of the bug #341 cured. `save_config` dumped the whole `Config()`, so a new install started life with eight kilobytes of settings its owner had never heard of and, worse, with that day's defaults frozen into their file -- after which a default we improve never reaches anyone who already has one. `contextWindowTokens: 65536` got onto disk exactly this way. It now writes only what differs from the defaults. Lossless on reload, since a value equal to its default reloads as that default. A fresh config drops from 7,985 bytes to 2, and to 634 once the extension blocks are seeded -- what is left is the handful of lines someone actually chose, which is also a config they can read. That third one is the only change here with a broad blast radius, and it has one visible consequence, which is an improvement: the onboarding picker used to lead with the schema's default model, because the bootstrap had written it into the config and the wizard read it back as "the current model" -- a value nobody had chosen, offered as though somebody had. With nothing written, the wizard leads with the provider's own recommendation instead. The test that pinned the old list now pins the new one and says why. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification ``` pytest tests/ 6650 passed, 1 failed, 33 skipped (tests/test_cli_theme.py::test_bold_accent_renders_styled_not_bare, which fails identically on github/main when its file runs together and passes alone on both) ruff check raven/ tests/ All checks passed ruff format clean ``` Driven by hand as well as by tests, each against a throwaway home: ``` doctor --json on a fossil config stdout parses as JSON again; the notice is on stderr doctor (pin 65536, model 1M) reports the pin and what is sized against it doctor --fix pin removed, file still 0600, a second run reports nothing save_config on a fresh Config() 2 bytes; 634 after the extension blocks are seeded ``` New cases cover the stderr split, both doctor paths including the mode preservation, and the save round-trip keeping every value the user chose. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Risk The bootstrap change alters what every new install's config file looks like: nearly empty instead of a full dump. Existing configs are untouched -- nothing rewrites them -- and both shapes load identically, so the risk is confined to anyone reading `config.json` as documentation of what can be set. `raven doctor` and the schema remain the answer to that question. The onboarding picker's default moves from the schema default to the provider's recommendation, as described above. `doctor --fix` writes only when asked, and only removes a key. Everything else here is output routing. Rollback: revert the commits. Nothing persists a new format, so a reverted build reads every config written under this one. ## Related Issues N/A --------- Co-authored-by: arelchan <204152633+arelchan@users.noreply.github.com> Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
## 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 #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 - [x] 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. - [x] Relevant tests pass locally - [x] 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. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A --------- Co-authored-by: arelchan <204152633+arelchan@users.noreply.github.com> Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
2 participants
Summary
Every config written by 0.1.10 or earlier carries
contextWindowTokens: 65536verbatim: that was the schema default back then, and the bootstrap dumped every default to disk. 0.1.11 (#287) started sizing the window from the model's own catalogue, but an explicit pin still wins over it -- correctly, that is what a pin means -- so the fix reached new installs only. Every upgraded install stayed capped at 64k on a 1M-token model, and/modelcould not shake it loose (refresh_context_windowreturns early for an explicit window, by design).The blast radius is not cosmetic. The window sizes the per-turn history budget (
_make_token_budget), the Curator's fast/slow path threshold (available_history * 0.60) and the memory consolidator's archive target (window // 2). An upgraded install therefore trimmed history early, paid for an extra Curator LLM call per turn, and consolidated at 32k instead of ~500k. The only existing signal was alogger.warningin a file nobody reads.Key decisions:
config.migrations.json). The value carries no provenance -- 65536 we planted and 65536 a user chose are the same three bytes -- so a watermark is the only thing that lets a re-set value stand for good. Without it, a user re-setting 65536 would have it stripped on every load, which is worse than the bug. The watermark deliberately does not live in config.json:Configisextra='forbid'andload_configraises rather than falling back, so a key this build knows and an older one does not is a hard boot failure for the older build -- a reverted release, a pinned version, or a second checkout sharing one~/.raven, which is already the case in this repo. A sidecar no published build reads is ignored instead of fatal. Because it is our file rather than the user's, it is also stamped unconditionally, which closes the one hole an in-file stamp had to accept: acontextWindowTokensthe user sets by hand afterwards is never second-guessed, whatever its value.raven upgrade. The upgrade helper is the outgoing version and cannot know a rule that ships with the incoming one; itexecves a bare Python bootstrap that never imports raven. Andinstall.sh/uv tool upgradenever callraven upgradeat all. Config load is the one chokepoint every entry point shares, so the user experience is "upgraded, and it was already fixed".provider useand friends promise not to touch a config they decided against changing, and that promise is theirs to keep, not ours to spend (test_use_leaves_the_config_alone_when_it_cannot_tellcaught an earlier draft that stamped unconditionally in the config).os.replaceswaps the inode, so a config tightened to 0600 -- it holdsproviders.*.apiKey-- came back 0644 in the first draft. The temp file is chmod-ed to the original's mode before the swap, and named per-PID so two processes migrating at once cannot have one truncate the file the other is about to move into place.load_configmigrated has the extension blocks popped (pop_extension_keys) and writing that back would delete the user'smemory/plugins/skillForgesections. It never goes throughsave_config, which dumps ~8 KB of defaults and would re-plant exactly this kind of fossil. A read-only home degrades to an in-memory-only migration rather than failing the boot.run()wrapper, so all thirteen config-loading commands tell it, not the two that print eagerly (agent,gateway, which keep the early print because they hold the screen for a long time). This matters because the watermark suppresses the next run's notice: an unsaid notice is lost, not deferred, andraven statuswould otherwise migrate the file in silence forever. The TUI and the served page get a newconfig_noticesfield on the session init bundle, rendered as a system line on bothsession.createandsession.resume-- resume drains the same one-shot list, so a picker-first user would otherwise see nothing. This also gives the client-sideconfig_warning/credential_warninghandlers their first server-side sibling that is actually populated -- both were dead wiring. The notice names the way back, because a window genuinely smaller than 65536 (a local model, an endpoint served with--max-model-len) is a real configuration this clears once.Follow-ups, deliberately not in this PR:
raven doctor --fixso the check can be run on demand rather than only at the next launch, and stopping the bootstrap from materialising defaults at all (save_config(load_config())in onboard) -- the root cause, and a change to every new user's config file shape, so it deserves its own review.Type
Verification
End-to-end on a config shaped like an upgraded install (
contextWindowTokens: 65536, plusmemoryandskillForgeblocks):Risk
User-visible behaviour changes: on the first launch after this lands, an upgraded install loses the
contextWindowTokens: 65536line from~/.raven/config.jsonand its context window jumps to whatever the model actually supports. Sessions get a longer history budget, the Curator drops back to its zero-LLM fast path far more often, and the status bar percentage falls accordingly. Users who genuinely need a 64k ceiling (a local model, a self-hosted endpoint with a smaller serving window) must re-add the line, which the notice tells them; those requests would fail loudly at the endpoint rather than silently, so the failure is self-announcing.Rollback: revert the commits. Nothing of ours is left inside config.json, so an already-migrated config is byte-for-byte a config that simply has no pin -- exactly the state a fresh install has, loadable by every published build. The orphaned
config.migrations.jsonsidecar is ignored by any build that does not look for it, and re-applying the change later just re-reads it. An earlier draft of this PR put the watermark in config.json and would have made a revert leave every migrated install unable to boot (Configisextra='forbid'); that is fixed, and the same reasoning covers pinning an older release or running a second checkout against the same~/.raven.Related Issues
N/A