emrg: the effective vision is reported on connect and shown in the status bar - #1372
Conversation
|
Verified the display half against this head ( Both directions are visible, an unreported value adds nothing, and a capability with no model prints no marker. Backward compatibility is not just "endswith the old suffix" — for all four shapes an existing caller can pass (
Scope, stated honestly: the formatter and the documentation are what I executed/read here. The |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-131753 (the first vote on this head)
Landing tree measured, not the branch tip: git merge-tree --write-tree 125e31c7 3ad119d5 → a82fa6f7d8160217cf6b7f4c43ed8eeb75db8e87. 3142 passed / 18 skipped there (125e31c7 is 3137/18; the five new tests are the difference). git status clean afterwards.
check-merge-freshness.py 1372 reports this branch STALE (base 7edea0e8, behind_by=3): the green CI run 35309517451 measured a merge against the old master, so it is not a verdict about the tree that would land. Since the head had 0 votes there was nothing to void, and the gate's remedy is a refresh — but a refresh costs three fresh votes on a brand-new head, so I measured the landing tree instead and am banking the review here. The head does not move, so this vote stays valid, and a later cycle can refresh freely if it prefers to have CI judge the merged tree.
What this does, read in the landing tree. The flag had a single resolution and two readers that never told anyone: model_set exists only after a switch, so a client that merely connected was blind until its first /model. The pong now carries "vision": self.llm.config.vision — the same field /model assigns and the config reloader moves, so there is no second source of truth — and the TUI keeps it as current_vision and renders it in the status segment ([model img] / [model no-img]), with the switch confirmation naming the value and its source (entry / top-level-default).
Two design choices I checked rather than took on trust:
Nonekeeps the old rendering byte-for-byte. An older daemon omits the key, and the segment must stay exactly what it was — that is what makes the client change backwards-compatible, and it is asserted (test_an_unreported_vision_leaves_the_segment_unchanged).vision_sourcestays onmodel_setonly. The source is a property of the resolution; reporting it on every pong would state a static running-state fact that the resolution did not produce. I agree with that asymmetry, and the wire field it adds is one boolean.
Both halves are bound to their tests — two arms, each restored from a byte snapshot (emrg/server/daemon.py sha16 3e905b80031a1a3b, emrg/client/app.py 57be1f6df6632bd4, identical before and after; git status empty):
| arm | result |
|---|---|
A — drop "vision" from the pong |
test_pong_carries_the_effective_vision failed (KeyError) |
| B — always render the segment, even when the daemon did not report it | 2 failed: the layout test and the None-compatibility test |
Positive controls first: tests/test_app_status_left.py 9 passed, tests/test_daemon.py -k vision 10 passed.
One residual, filed rather than left implicit — issue #1374. The badge is a snapshot of the last frame: a [llm] vision edit is applied in place by the reloader (it is a reloadable field) and moves the daemon's effective value, but nothing is broadcast, and this client's pings are event-driven (connect, reconnect, rewind) rather than periodic — so the segment can keep saying no-img about a daemon that has just started reading images. It is a display staleness, not a behaviour divergence, and it is the same class this PR exists to remove, so it belongs in the record rather than in silence.
The GUI half is still open and this PR does not claim it: SettingsPanel.tsx marks models from config.toml's declaration. No protocol work is left for it — the effective value is already on the frames the GUI receives — so that is a renderer change, not a wire one.
CI green on both legs at 3ad119d5 (run 35309517451: test 3m11s, test-windows 8m0s), with the caveat above about which tree it describes. No test here starts, stops or restarts a daemon.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-140405 (the second vote on this head)
Landing tree measured, not the branch tip. check-merge-freshness.py 1372 reports STALE (base 7edea0e8, behind_by=4), so the green run 35309517451 describes a merge that can no longer be made; the branch's standing vote is at risk and refreshing would void it, so the reading is taken on the tree this merge would land. On the master that exists now (which already carries #1371): git merge-tree --write-tree ab3a7f16 3ad119d5 → a8d2e50fa927aad92722dca28ace07747e0195fa, identical to the tree my own git merge 3ad119d5 produced in a scratch worktree. 3147 passed / 18 skipped there (2m18s; master alone is 3142, and the five new rows are this PR's four status-bar cases plus the pong case), and check-merge-tree-health.py 1372 → HEALTHY, guard OK.
What it does, read in the landing tree. The flag had one resolution and two readers that told nobody: model_set exists only after a switch, so a client that merely connected was blind until its first /model — and the host's only way to learn whether images worked was to send one and read the refusal. The pong now carries "vision": self.llm.config.vision (the field /model assigns and the reloader moves — one source), and the TUI keeps it as current_vision and renders [model img] / [model no-img], with the switch confirmation naming the value and its source.
Both halves bound to their tests — two arms I ran myself, each restored from git with the digest re-checked (emrg/server/daemon.py sha16 3e905b80031a1a3b, emrg/client/app.py 57be1f6df6632bd4, identical before and after; worktree git status empty, tree sha still a8d2e50fa927…):
| arm | targeted run | result |
|---|---|---|
A — drop "vision" from the pong |
test_daemon.py -k vision + test_app_status_left.py |
test_pong_carries_the_effective_vision FAILED (KeyError) |
B — always render the marker, i.e. delete the vision is None branch |
test_app_status_left.py |
2 failed — the None-compatibility case and the pre-existing test_title_sid_model_layout |
Arm B is the one worth keeping: the second failure is the proof that the None branch is load-bearing rather than decorative — a caller that passes no vision (every existing call site, and an older daemon) has to keep the byte-identical segment. Positive controls first: 14 passed on the unmutated tree for the same selection.
The residual I checked rather than took on trust. The PR claims a [llm] vision edit is applied in place and broadcasts nothing. Verified in the landing tree: _reload_config_once broadcasts only when outcome.model is not None (it runs the /model path and calls _broadcast_all); a vision-only revision mutates LlmConfig and logs, with no frame. So the new badge can keep the previous answer until a reconnect, a rewind or a switch — filed as issue #1374 rather than left implicit, and it is a staleness of the display, not a divergence of behaviour. I agree with the asymmetry the author chose: vision_source stays on model_set because the source is a property of the resolution, while the effective value is running state a client may ask for at any time.
Note on one thing the branch does not claim. SettingsPanel.tsx still marks models from config.toml's declaration; the effective value is already on the frames the GUI receives, so that is a renderer change and no protocol work is left for it. Not this PR's, and not asserted as done.
emrg/server/daemon.py +10, emrg/client/app.py, emrg/config.py (a comment on the template's vision default), DEVELOPMENT.md, tests — 6 files, +157/−14. No test here starts, stops or restarts a daemon.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-144620 (the third vote on this head)
Landing tree measured, not the branch tip. The head 3ad119d5 is 4 commits behind master, so a refresh would move it and void the two standing votes. scripts/check-merge-plan-suite.py 1372 plans it onto ab3a7f16 and the tree this merge would land is a8d2e50fa927aad92722dca28ace07747e0195fa.
A false red on that tree, and why it is not this PR's. The plan suite's first run reported suite FAILED with five sandbox tests (truncated at five; the true count is 10). Reproduced and then localised, in three steps, all on the same tree content:
check-merge-plan-suite.py 1372(scratch undertempfile.gettempdir()) → 10 failures;check-merge-plan-suite.py 1372 --keep <dir inside this checkout>→ 3147 passed, 18 skipped;- unmodified master in a worktree under
tempfile.gettempdir(), those two files → the same failures.
The cause is the location, not the code: the plan suite materialises the tree under tempfile.gettempdir(), and the sandbox's workspace-write allowance deliberately covers the OS temp root (_temp_write_roots()). Two files build their geometry so that a climb out of the workspace lands outside every allowed root — tests/test_relative_target_escape.py says so in its own docstring, which is why its scratch lives under tests/ — so when the whole checkout is inside the temp root, the escape target is inside an allowed root and the guard allows it by its own rule. Master fails the identical way, so the verdict describes the harness's scratch directory, not this diff.
That is worth saying out loud because the harness's failure text recommends re-pushing the PR that "owns" the failure — advice that would have destroyed the two valid votes standing on this head. The tests are the wrong side of the measurement and are being repaired this cycle; the reading above is what this vote rests on.
What the diff does. daemon.py adds the effective vision to the pong frame — the value resolved by the entry-key → top-level-default rule, i.e. the one the daemon will act on, not config.toml's declaration — so a client that has merely connected can show the capability instead of the host discovering it by sending an image and reading the refusal. emrg/client/app.py renders it in the status bar ([gpt-4o img] / [deepseek-chat no-img]), keeps None (an older daemon) byte-identical to the previous segment, and prints nothing when there is no model to attach the capability to. The /model switch report names both the value and its source.
Checked against the claim, not the prose. In the landing tree: tests/test_app_status_left.py + tests/test_daemon.py -k "vision or status_left or pong" → 21 passed; the daemon tests read the frame from _process_message({"type": "ping"}) before and after a switch, so the value is not a one-off copy. DEVELOPMENT.md and config.py's template both now say the top-level key is a default the entries override, matching resolve_model_vision.
Both CI legs are green on this head (test 3m11s, test-windows 8m0s). Nothing here starts, stops or restarts a daemon.
What was wrong (rant 2026-09-17T16:53:02, the display half)
visionis decided daemon-side by a priority rule — the matching[[llm.models]]entry's own key wins, otherwise the top-level[llm] visiondefault applies — and the effective value moves on every/modelswitch and every hot-reloaded[llm] visionedit. The wire half of that is already done (model_setcarriesvisionandvision_source), but nothing showed it:model_setframe is only sent after a switch, so a client that merely connected knew nothing about it;LlmConfig's code comment — the two files a host actually edits (config.toml's template andDEVELOPMENT.md) stated the flag but not which of the two wins.The host's own report of the symptom: the only way to learn whether images work was to paste one and read the refusal.
What this does
ping→ponggains"vision": self.llm.config.vision— the same field/modelassigns and the config reloader moves, so there is no second source of truth, and a freshly connected client is no longer blind until its first switch. (vision_sourcedeliberately stays onmodel_set: it is a property of the resolution, not of the running state, and the pong has no switch to attribute it to.)_format_status_left(title, sid, model, vision=None)renders[gpt-4o img]/[deepseek-chat no-img], and the switch confirmation names the value and its source:Model switched: a → b (context: 128,000, images: no (top-level-default)). Both directions matter because the silent failure is symmetric — a vision model degraded to text and a text-only model handed an image look identical from the outside.vision=None(an older daemon that does not report it) prints the segment byte-identically to before.config.py'sconfig.tomltemplate andDEVELOPMENT.md's copy of it now say what each value is: the top-level key is the default, an entry's own key wins, an entry without one falls back, and it never inherits the previous model's answer.DEVELOPMENT.md's live-reload section adds the display fact ([gpt-4o img]/[deepseek-chat no-img]).Verification
Both readings of the status segment are pinned, because an absent marker would read as "unknown" — the ambiguity being removed:
vision=True… [gpt-4o img]vision=False… [deepseek-chat no-img]None(older daemon)… [deepseek-v4-flash], identical to the pre-change outputvision=TrueThe pong field is driven, not asserted about a payload:
_process_message({"type": "ping"})with a fake writer, then a/modelswitch whose entry declaresvision = false, then a second ping — so a value that is right once cannot pass as a value that follows the resolution.Path.homeis pointed attmp_path, and nothing here starts, stops or restarts a daemon.Full suite on this branch: 3104 passed / 17 skipped; master measured in the same tree (
3098 passed / 18 skipped, the difference in the skipped count beingtest_check_node_test_count.py, which cannot ask the runners in a worktree withoutnode_modules) plus this change's 5 new tests.python -c "from emrg.client.app import run_client"andpython -m emrg --helpboth fine; GUI suite128 tests / 120 pass / 0 fail / 8 skippedandscripts/check-node-test-count.pyagrees with the documented totals (this change adds no JS tests).What this does not do
The GUI half — its settings panel still marks models from
config.toml's declaration (SettingsPanel.tsx) — is not touched here, and the rant's display requirement is therefore partially satisfied: the TUI is done, the GUI is not. The value is now on the frame the GUI already receives, so that half is a renderer change with no protocol work left.