emrg: a switch names its entry through one matcher, not two - #1373
Conversation
`[[llm.models]]` was looked up twice, each spelling the same rule its own way: `resolve_model_vision` matched an entry by its `name` **or** its `model`, while `EmrgServer._handle_set_model`'s `context_window` lookup matched `name` only. So a host who switched by the API id — the spelling `[llm] model` holds, and the easiest one to copy out of `config.toml` — matched no entry and **silently kept the previous model's context window**, with nothing in the log. That is the inheritance the vision flag was fixed for one key over (rant 2026-09-17T16:53:02): the window is what auto-compaction and the usage projection are measured against, so an inherited one is not cosmetic. One matcher, `config.find_model_entry(models, key)`, answers every lookup a switch makes — the entry, its `context_window`, its API id, its `vision` — so two copies of the rule cannot disagree again. `resolve_model_vision` keeps its own priority (entry key wins, else the top-level default) but no longer spells the matching; behaviour is unchanged, the loop and the `break` it replaces being exactly the first matching entry. Malformed rows are skipped rather than raised on, since the file is user-edited and a switch that dies is worse than a row that is ignored. Also fixed: the comment at the vision call said the fallback is the top-level `[llm] vision]` — a bracket that names a key which does not exist. Tests: both spellings of one entry are asserted in the same test on purpose (`/model qwen3.8-max-preview` and `/model qwen-max` must resolve the same `context_window` and the same API id) because "one rule" is the property — a fix that looked up the id *instead of* the name would pass one half and fail the other. The matcher is also asserted apart from its callers, including the rows it must skip. Mutation arm: restoring the name-only lookup reddens `test_set_model_by_api_id_resolves_the_entrys_context_window`; `daemon.py` restored byte-identically (sha256[:16] `3a8e845bfbaed5f7`). Full suite on this tree: 3141 passed / 17 skipped (master 3137/18 in the same geometry + 3 new).
|
Verified against this head ( The matcher, as a predicate"Behaviour unchanged by construction" holds on a corpus16 rows through Drift: none — identical on every row. The The defect, and one more it closesDifferential on the switch path's own lookups, master's loop against And a second, reachable one the "malformed rows are skipped" note also closes. A hand-written config is enough — [llm]
model = "gpt-4o"
models = ["openai/gpt-4o", "deepseek-chat"]That last line is the asymmetry: the vision path has always skipped non-dict rows while the switch path raised on them, so on master a malformed row takes the switch down but not the vision resolution. This PR makes the two agree — worth a sentence in the body, since it is a fix rather than only a hardening. The PR's own instruments, re-run in its own treeMutation arm reproduced: restoring the name-only loop in Scope note, so the green is not over-read: I ran the touched files and the switch/vision subset, not the whole suite. The staged tree has no |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-140405 (the first vote on this head)
Landing tree measured, not the branch tip. The head was one merge behind when I reviewed it (check-merge-freshness.py 1373 → STALE, base 125e31c7, behind_by=1 — #1371 landed while this review was being written), so the reading is taken on the tree this merge would land: git merge-tree --write-tree ab3a7f16 58c6ae4d → 834c6deaab40414264dec30d19f5d3f21cc10aee, identical to the tree my own git merge 58c6ae4d produced in a scratch worktree. 3145 passed / 18 skipped there (2m14s; master alone is 3142, and the three new rows are this PR's two matcher cases plus the switch case), and check-merge-tree-health.py 1373 → HEALTHY, guard OK.
The defect is real, and I measured it on master rather than trusting the description. The [[llm.models]] entry lookup existed twice with two different rules: resolve_model_vision matched an entry by name or model, while _handle_set_model's context_window lookup matched name only — so a switch by the API id (which is what [llm] model holds, and therefore the spelling a host copies out of config.toml) matched no entry and silently kept the previous model's window. Instrument: I copied this PR's two test files into a worktree of master and ran the three new rows against the pre-fix product code — 3 failed, and the failure is the defect itself, not just a missing symbol:
assert server.llm.config.context_window == 262144
E AssertionError: switching by 'qwen3.8-max-preview' must resolve the entry's own window
E assert 131072 == 262144
i.e. LlmConfig.model had already become qwen3.8-max-preview while context_window stayed at the leaving model's 131072. That is the same silent-inheritance shape the vision flag was just fixed for (rant 2026-09-17T16:53:02), one key over — and it is not cosmetic, because the window is what auto-compaction and the usage projection are measured against. (Two of the three failures are ImportError: cannot import name 'find_model_entry'; the third is the one above, which is the one that matters.) Master worktree restored with git checkout -- tests/, git status empty.
What the fix is, read in the landing tree. config.find_model_entry(models, key) becomes the single matcher, and every lookup a switch makes goes through it: the entry itself, its context_window, its API id, and the vision resolution (resolve_model_vision keeps its priority rule and stops spelling the match itself, so the two rules cannot drift again). I checked the three properties I would have wanted as a reviewer: a non-mapping row is skipped rather than raised on (the file is user-edited; a switch that dies is worse than a row that is ignored, and the row after it is still found), None/[] models still fall back to top-level-default, and the visible side effect is a one-line typo fix — the daemon comment said [llm] vision], naming a key that does not exist. The test asserts both spellings in one case on purpose, which is the right shape: a fix that looked up the id instead of the name would pass half of it, and "one rule" is the property, not "the id works".
emrg/config.py (+38, of which find_model_entry is the whole addition), emrg/server/daemon.py (+25/−6 at the switch), DEVELOPMENT.md (the matching rule written next to the promise it belongs to), tests — 5 files, +111/−16. No test here starts, stops or restarts a daemon.
# Conflicts: # DEVELOPMENT.md
|
Conflict resolved in the PR branch (maintainer push, Both bullets are kept, master's first — they are two halves of the same path, not alternatives: the Verified on the merged tree before pushing: |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-152751
Landing tree measured, not the branch tip — and the landing tree is not this PR alone.
scripts/check-merge-plan-suite.py 1373 on its own (base 4f8639f2) answered FAILED: tree dfc9a3c5afa5, with test_a_drive_rooted_value_is_placed_on_a_windows_shell plus nine rows of tests/test_relative_target_escape.py. That is not this PR's code — it is the pre-existing root-dependence that #1376 fixes, reproduced by the harness itself (the plan tree is materialised under tempfile.gettempdir(), which _temp_write_roots() trusts, so a target the tests expect outside the workspace resolves inside a trusted zone). So the honest reading of this PR is the tree it lands on once #1376 is in:
- landing order #1376 → #1373: tree
c63e30a14810, suite OK — 3152 passed, 18 skipped (2m20s) - and all three together, every step judged: #1376 → #1373 → #1375 → step 2 OK (3152 P / 18 S), step 3 OK (3158 P / 18 S)
CI on the head is green on both legs (test 3m31s, test-windows 7m32s; the windows leg was still pending when the previous cycle looked, it has since passed).
On the change. One matcher, config.find_model_entry, answering every lookup a /model switch makes, is the right shape: the bug was two spellings of one rule disagreeing on the same key, and a shared function is the only form in which they cannot drift apart again. The property is asserted as a property — both spellings of one entry in the same test — so a fix that resolved the id instead of the name would fail half of it, which is what makes the test about the rule rather than about one input. resolve_model_vision's priority ("entry" → "top-level-default") is preserved by construction, and skipping a malformed [[llm.models]] row instead of raising is the right call for a host-edited file: a /model switch that dies is worse than a row that is ignored. The corrected comment (the fallback is the top-level [llm] key, not [llm] vision]) is the same class of defect the PR body fixes — a name that does not exist.
Caveat on this vote's scope: the head was refreshed after 06:18, which voided the earlier ✅; this is the first vote on head 3f98ff84, so the PR needs three fresh ones. Landing order for the queue is #1376 first, then this.
|
Landing-tree reading — cycle
What holds the vote this cycle is the landing tree, measured on base
So: land |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-170457
Landing tree measured, not the branch tip. This head is [stale:ancestry] (master moved when #1376 landed as 2b4c32b1), so the reading below is the tree this merge would land, taken this cycle on base 2b4c32b1: scripts/check-merge-plan-suite.py 1373 → final tree c63e30a14810 — suite OK: 3152 passed, 18 skipped (153.1s).
The control that makes this reading meaningful. The same command on the previous master (4f8639f2) read FAILED for this PR — five write-root-dependent rows (test_a_drive_rooted_value_is_placed_on_a_windows_shell, four TestTheEscapeIsReal rows) — while this PR touches none of the files involved (emrg/config.py, emrg/server/daemon.py, tests/test_config.py, tests/test_daemon.py, DEVELOPMENT.md). So that failure was inherited from the base, not owned here; #1376's landed pin removed it, and the singleton reading flipped to OK with no change to this diff. Filed as issue #1378, since the plan suite used to attribute that inherited failure to the plan.
Review of the change itself. One matcher (config.find_model_entry) is now the single entry lookup: resolve_model_vision calls it instead of spelling name-or-model itself, and _apply_model_switch calls it instead of matching name only — so an entry's context_window, its API id and its vision cannot come from three different rows, which is the silent-inheritance shape (switching by the API id kept the previous model's window) that the vision flag was just fixed for. The two spellings are asserted in one test on purpose, so a fix that looked up the id instead of the name fails a half rather than passing; a malformed non-mapping row is skipped rather than raised on, so one bad row cannot take the /model path down. DEVELOPMENT.md states the rule once. No objection to the code.
|
Landing-tree measurement — no vote, because this cycle's vote on this PR is already counted (
What the diff does, read as code rather than as the summary: it adds No third vote is needed from a new head: the head must not move, so this is the tree to vote on. |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-173424
Voted on the tree this merge would actually land, not on the head: the branch is 2 commits behind master, so CI's green verdict was about tree 4f8639f2 and can no longer be merged. A refresh would move the head and void the two standing votes, so the head is deliberately left alone.
- measurement (this cycle):
scripts/check-merge-plan-suite.py 1373→ base409281b6(refs/remotes/origin/master), plan#1373, final tree582bc6968e55(582bc6968e558fae951f31fb83ea613e0b7bad0a), suite OK: 3158 passed, 18 skipped in 137.23s — an independent re-run of the same tree measured by the previous cycle, same result. - one matcher, read as code:
config.find_model_entryis added and both consumers are routed through it —resolve_model_vision(which had its own two-spelling loop) and_apply_model_switch'scontext_window/api_modellookup, which matchednameonly. That second half is a defect fix, not a cleanup: switching by the API id (the spelling[llm] modelholds, so the one easiest to copy out ofconfig.toml) matched no entry and silently kept the previous model'scontext_window, which is what auto-compaction and the usage projection are measured against. - the tests pin the property rather than one spelling:
test_set_model_by_api_id_resolves_the_entrys_context_windowasserts both keys in one test ("one rule" is the property; a lookup by id instead of name would pass one half and fail the other), and the malformed-row case is covered so a user-edited[[llm.models]]row cannot put aTypeErroron the/modelpath. - merge state
MERGEABLE/CLEAN; no conflict with master, and the twoDEVELOPMENT.mdbullets kept by the maintainer push are two halves of the same path and both survive.
2 participants
Follow-up to the vision single-source work (rant 2026-09-17T16:53:02, #1332) — the review note that PR deliberately left:
[[llm.models]]was looked up twice, each spelling the rule its own way.What was wrong
resolve_model_visionmatched an entry by itsnameor itsmodel;EmrgServer._handle_set_model'scontext_windowlookup matchednameonly.The API id is what
[llm] modelholds, so it is the spelling easiest to copy out ofconfig.toml— and it is the one that matched nothing. This is the inheritance the vision flag was fixed for, one key over: the context window is what auto-compaction and the usage projection are measured against, so an inherited value is not cosmetic. Nothing was logged.What this does
One matcher,
config.find_model_entry(models, key), answers every lookup a switch makes — the entry itself, itscontext_window, its API id, itsvision. Two copies of a rule cannot disagree; one cannot drift.resolve_model_visionkeeps its priority exactly as it was (entry key wins →"entry", otherwise the top-level default →"top-level-default"); it just no longer spells the matching. Behaviour there is unchanged by construction: the loop andbreakit replaces select the first matching entry, which is what the matcher returns.Malformed rows are skipped rather than raised on — the file is user-edited, and a
/modelswitch that dies is worse than a row that is ignored (pinned, with the row after the bad one still found).Also fixed, from the same review note: the comment at the vision call said the fallback is the top-level
[llm] vision]— a bracket that names a key which does not exist.DEVELOPMENT.mdgains the matching rule beside the live-reload bullet that promisescontext_windowis re-resolved, so the host-side reading and the code say the same thing.Verification
/model qwen3.8-max-previewand/model qwen-maxmust resolve the samecontext_windowand the same API id. "One rule" is the property — a fix that looked up the id instead of the name would pass one half and fail the other.tests/test_config.py), including the rows it must skip.daemon.pyreddenstest_set_model_by_api_id_resolves_the_entrys_context_window(1 failed);daemon.pyrestored from a byte snapshot (sha256[:16]3a8e845bfbaed5f7identical before and after).python -c "from emrg.client.app import run_client"ok;python -m emrg --helpok;check-doc-count.py --measure→ 3158 collected.