Repository navigation
fix(*): give the page the backend's model identity, aliases included - #677
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: The current-model lookup must use the backend's full identity, including supported provider aliases.
I reviewed the updated github/main...HEAD diff, the affected model source/store/callers, commit history, backward compatibility, tests for weakening, and the repository's AGENTS/CLAUDE/context architecture rules. The focused behavior is otherwise coherent, but the inline alias case leaves the stated identity fix incomplete.
Verification: npm test passed 193 files / 2,664 tests (with existing happy-dom aborted-fetch noise); the focused ModelChip/ModelPicker run passed 56 tests; npm run type-check, npm run lint -- --quiet, and git diff --check github/main...HEAD all passed.
e3bca47 to
bef5c7d
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: Hyphenated provider prefixes still need the backend's normalization.
The new route_names contract fixes the previously reported provider-renaming alias case, and I replied on and resolved that thread. I reviewed the full updated github/main...HEAD diff, the backend identity implementation and provider registry, web callers, generated RPC clients, history, backward compatibility, tests for weakening, and the repository's AGENTS/CLAUDE/context architecture rules. The remaining inline mismatch is the only blocker I found.
Verification: uv run pytest tests/test_rpc_model.py -x passed 214 tests; the focused web run passed 82 tests; full npm test passed 194 files / 2,694 tests (with existing happy-dom aborted-fetch noise); web type-check, lint, RPC generation check, TUI RPC generation check, and git diff --check passed.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The new TUI ModelOptionProvider.route_names declaration correctly brings the handwritten gateway type into line with the RPC contract; generated types and callers remain consistent. I rechecked the one-file revision delta against the full PR, affected contract consumers, backward compatibility, history, test coverage/weakening, and the repository's AGENTS/CLAUDE/context architecture rules.
Named follow-up: browser model identity still does not normalize hyphenated provider prefixes to underscores exactly as backend merge_key does. I resolved that thread because, at this fourth round, the pre-existing mixed-spelling passthrough corner does not meet the blocking bar.
Verification: TUI type-check, lint, and RPC generation check passed; 82 focused web tests and web type-check passed; uv run pytest tests/test_rpc_model.py -x passed 214 tests; git diff --check github/main...HEAD passed.
… string The column builder de-duplicates a provider's list against the conversation's model with `sameModel` (strip a leading `<provider>/`, lowercase, compare), because a list mixes ids added by hand with ids the vendor reports. Four readers of that same identity compared raw strings instead, so exactly the case the de-duplication exists for lost its marking: - the picker's row tick, and the pin that keeps the running model visible past the fold: with mixed spellings no row was ticked at all, leaving no sign of which model is in force; - the chip's account resolution, which found nobody and drew no provider mark; - the chip's label lookup, keyed by the account's spelling, which then fell through to the raw identifier instead of the registry label; - `currentProvider`, which decides the account a typed identifier joins, so a new one silently went to whichever account happened to be listed first. `serving()` now returns the account together with the identifier as that account spells it, which is what the label map is keyed by. Not fixed here: two connected accounts listing the same bare identifier still mark both rows. That is unchanged by this commit -- reverting the tick line reproduces it either way -- and cannot be resolved at this layer, because the store keeps the model without an account, `choose` drops the one it is handed, and the wire's `is_current` goes stale on a switch. Closing it means carrying the account beside the model through the three writers of `setCurrent`. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…sk its identity `merge_key` strips a leading prefix naming the provider, and `route_names` is the set it accepts: the provider's own name plus the ones it used to answer to. A page holding only the current name asks a narrower question, and nothing on the wire told it otherwise -- so a model configured before a rename (`zhipu/glm-4.6` on the account now called `zai`) read as a different model from the same one written the new way. `model.options` canonicalizes the slug but hands the configured list back as it stands, so the two spellings meet in the client. Measured against `merge_key` rather than read off it: under `zai`, `zhipu/glm-4.6` is the same model as `zai/glm-4.6` and as `glm-4.6`, while the page answered false to the first two. The set now travels on `model.options` and the client strips any of its members, which is what `ProviderSpec.route_names` asks for -- it says to compare against it rather than rebuild it, and a second copy of the alias table per surface is the drift that warns against. `bareModel` and `sameModel` take the provider rather than its name, so the type names every site that has to state what it knows: the picker's tick, fold pin and typed-id routing, the chip's account and label, the column de-duplication, the recent rows, and the settings popover's already-added test. A row that carries no set falls back to the provider's own name, which is the old rule rather than a wrong one, and is what every offline fixture gives. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
`gatewayTypes.ts` is a hand-written mirror of the wire, and its drift gate holds it to every property the generated contract carries. Adding `route_names` to `model.options` left that third copy behind: the generator and the strict Python model were both regenerated and in sync, and neither says anything about a type written by hand downstream. The TUI does not read the field. It is declared for the reason `gateway` beside it is: so the drift check stays honest about what the wire sends. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…unts marks one row Two connected accounts can list the same id -- a gateway reselling what a direct vendor also serves, or an id added by hand to both. The page kept only the model, so nothing could tell their rows apart: both were ticked, and the chip named whichever account came first in the list rather than the one the conversation runs on. That is the question the mark exists to answer. A pick names an account and so does `model.options`, so the store now keeps the two together and every writer states both: the gateway's answer, the optimistic switch, and the rollback behind a refused one. Empty means nothing has said, and the surfaces fall back to whichever account lists the model, which is what they did before. The pin follows. `column` put the running model at the head of the account the wire flagged unless another account listed it, because that flag went stale the moment a pick moved the conversation; the account is now known outright on that path, so the guess is only for when it is not. Without this the fix was inert in the shape that prompted it: the guess had already dropped the pin, so the account the conversation is on did not list the model at all and the mark went to the one that did. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
68f1f3a to
42e0946
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
No new findings in this revision. The provider-aware current-model state is carried consistently through gateway refresh, optimistic selection, rollback, chip resolution, picker marking, and typed-model destination; the prior nonblocking follow-up is unchanged and both threads I opened remain resolved.
I reviewed the new commit and full PR context, affected callers and history, backward compatibility, tests for weakening, and the repository's AGENTS/CLAUDE/context architecture rules. Verification passed: full web suite (198 files / 2,734 tests), web type-check/lint/RPC generation, TUI type-check/lint/RPC generation, uv run pytest tests/test_rpc_model.py -x (214 tests), and git diff --check.
…677) ## Summary The served page asked three narrower questions than the backend answers about which model a conversation is running, and which account is running it. The backend's identity for a stored model is `providers/wire.py`'s `merge_key`: strip a leading prefix that names the provider, lowercase, compare. Two ids that reduce to the same thing are the same model however either was written. **1. Raw string comparison.** The model column de-duplicates a provider's list against the conversation's model by that identity, because a list mixes identifiers added by hand with identifiers the vendor reports. Four readers of the same identity compared raw strings, so the case the de-duplication exists for was exactly the case that lost its marking: no row ticked, the chip with no account mark and the raw identifier in place of the registry label, and `currentProvider` -- which decides the account a typed identifier joins -- sending a newly typed one to whichever account happened to be listed first. **2. The prefix set.** `merge_key` strips any prefix the provider answers to, not only its current name: `ProviderSpec.route_names` is its own name plus the ones it used to answer to. Nothing on the wire carried that set, so a page could not ask the question the backend answers. Measured against `merge_key` rather than read off it, under the account now called `zai`: | ids compared | backend | page, before | | --- | --- | --- | | `zhipu/glm-4.6` and `zai/glm-4.6` | same model | different | | `zhipu/glm-4.6` and `glm-4.6` | same model | different | | `zai/glm-4.6` and `glm-4.6` | same model | same | Reachable rather than theoretical: the registry keeps `zhipu` as an alias precisely so identifiers written before the rename still resolve, and `model.options` canonicalizes the account's slug while handing its configured list back as it stands, so the two spellings meet in the client. `model.options` now carries `route_names` per account and the client strips any member of it. That is what `ProviderSpec.route_names` asks for: compare against the set rather than rebuild it, since a second copy of the alias table per surface is the drift it exists to prevent. **3. The account beside the model.** Two connected accounts can list one identifier -- a gateway reselling what a direct vendor also serves, or one added by hand to both. The page kept only the model, so nothing could tell their rows apart: both were ticked, and the chip named whichever account came first rather than the one the conversation runs on, which is the question the mark exists to answer. A pick names an account and so does `model.options`, so the store keeps the two together and every writer states both -- the gateway's answer, the optimistic switch, and the rollback behind a refused one. The pin follows from that. `column` put the running model at the head of the account the wire flagged unless another account listed it, because that flag went stale the moment a pick moved the conversation; the account is now known outright on that path, so the guess is only for when it is not. Without that half the third fix was inert in the shape that prompted it: the guess had already dropped the pin, so the account the conversation is on did not list the model at all and the mark went to the one that did. Empty means nothing has said, and every surface falls back to what it did before. Key decision: the wire does state a provider for the session's model, and the identity comparison deliberately does not use it as the *identity*. The store's own comment records that the per-row flag names the provider the session STARTED on and goes stale the moment a pick moves the conversation, which is why `carried()` exists. What travels now is the account the conversation is on, set together with the model by whoever knows both. `bareModel` and `sameModel` take the provider rather than its name, so the type names every site that has to say what it knows rather than letting one keep the narrower rule silently: the picker's tick, fold pin and typed-identifier routing, the chip's account and label, the column de-duplication, the recent rows, and the settings popover's already-added test, which had the same gap and no coverage at all. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification Every test here was watched to fail, for the expected reason, before the expression it covers was written; the ones written alongside a change were then proved load-bearing by narrowing the call site back and watching exactly that case redden, and no other. The identity table is measured against `merge_key` itself rather than read off its source, and is a literal copy on each side for the reason the kind table beside it already gives: a change to either side has to show up as a disagreement. Re-run after the rebase onto the current base: - `npx vitest run src/features/model/ src/chrome/ src/features/settings/ scripts/gates/fixture-shape.test.mjs` -> 30 files, 377 passed. - `npx tsc --noEmit -p tsconfig.json` -> no errors found, on both front ends. - `uv run --frozen --all-extras pytest tests/test_rpc_model.py tests/test_rpc_schema_match.py` -> 612 passed. - `node scripts/gen-rpc-client.mjs --check` -> matches the contract, 195 methods. - `node scripts/gen-rpc-types.mjs --check` (ui-tui) -> in sync. - `npm test -- src/__tests__/gatewayTypesDrift.test.ts` (ui-tui) -> 1 passed. - `python3 ui-web/build.py` -> both boot snapshots match their goldens. - `npx eslint .` -> 0 errors, 4 warnings, all `react-hooks/exhaustive-deps` in SubagentsPage, ConnectionsPage and CronPage, none of which this changes. - `node scripts/check-class-namespace.mjs` -> OK. - `scripts/check_commit_messages.py`, `scripts/check_source_language.py` and `scripts/check_large_files.py` -> all exit 0, no output. Browser checks over CDP, against temporarily patched offline fixtures, one per shape. Mixed spellings: the chip's mark went from absent to the owning account's and the tick from nowhere to one row under it. One identifier in two accounts: the mark went from the wrong account to the right one and the tick moved with it, leaving the other account's row listed and unmarked. No page errors in either. The first of those two is corroborated by the repo's own boot gate, which reported a node-count mismatch against that fixture on the base and matches the golden here, the missing node being the chip's provider icon. The full `ui-web` suite is not quoted as a base comparison, because on this machine two consecutive runs of the same tree differ by ten test ids. A reviewer's run of the same suite at an earlier head was 193 files / 2,664 tests green, which is the better evidence that the local instability is environmental. Two batch failures in `src/features/model/source.test.ts` are not from this change: removing the one line it adds to that file leaves both in place, and the file passes 32/32 on its own twice. The third box is left blank because nothing in the documentation or the screenshots describes the chip's marking, so there is nothing to update. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Risk User-visible changes, in the composer's model chip and picker and in the settings add-a-model popover, all of them about what is marked rather than what is offered: - where an account and a conversation spell one model differently, the running model is now ticked, the chip carries its account's mark and the registry label, and a model already configured under an older spelling is no longer offered as though it were missing; - where two accounts list one identifier, exactly one row is marked and it is the account the conversation is on; the other account still lists the model and can still be picked; - a typed identifier joins the account the conversation is on rather than the first listed one. That is a change to where data is written and the one worth a second look. The comparison is looser than the one it replaces. A prefix belonging to some other provider is still not stripped, which the identity table pins from that direction. Contract: `model.options` gains an optional `route_names` per provider row. It is additive, so an older client ignores it and a server that does not send it leaves the client on the provider's own name, which is today's rule. Both generated clients, the strict Python model and the hand-written TUI mirror were updated and re-checked against the schema. Security impact: none. No credential, permission or transport rule is read or written here. Backward compatibility: no state, storage key or existing wire field changes. The store gains a second value beside the model it already kept; empty is the old behaviour. Rollback: revert the commits. Each stands on its own except the last, which needs the one that adds the prefix set. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A --------- Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Summary
The served page asked three narrower questions than the backend answers about
which model a conversation is running, and which account is running it.
The backend's identity for a stored model is
providers/wire.py'smerge_key:strip a leading prefix that names the provider, lowercase, compare. Two ids that
reduce to the same thing are the same model however either was written.
1. Raw string comparison. The model column de-duplicates a provider's list
against the conversation's model by that identity, because a list mixes
identifiers added by hand with identifiers the vendor reports. Four readers of
the same identity compared raw strings, so the case the de-duplication exists
for was exactly the case that lost its marking: no row ticked, the chip with no
account mark and the raw identifier in place of the registry label, and
currentProvider-- which decides the account a typed identifier joins --sending a newly typed one to whichever account happened to be listed first.
2. The prefix set.
merge_keystrips any prefix the provider answers to,not only its current name:
ProviderSpec.route_namesis its own name plus theones it used to answer to. Nothing on the wire carried that set, so a page could
not ask the question the backend answers. Measured against
merge_keyratherthan read off it, under the account now called
zai:zhipu/glm-4.6andzai/glm-4.6zhipu/glm-4.6andglm-4.6zai/glm-4.6andglm-4.6Reachable rather than theoretical: the registry keeps
zhipuas an aliasprecisely so identifiers written before the rename still resolve, and
model.optionscanonicalizes the account's slug while handing its configuredlist back as it stands, so the two spellings meet in the client.
model.optionsnow carriesroute_namesper account and the client strips anymember of it. That is what
ProviderSpec.route_namesasks for: compare againstthe set rather than rebuild it, since a second copy of the alias table per
surface is the drift it exists to prevent.
3. The account beside the model. Two connected accounts can list one
identifier -- a gateway reselling what a direct vendor also serves, or one added
by hand to both. The page kept only the model, so nothing could tell their rows
apart: both were ticked, and the chip named whichever account came first rather
than the one the conversation runs on, which is the question the mark exists to
answer. A pick names an account and so does
model.options, so the store keepsthe two together and every writer states both -- the gateway's answer, the
optimistic switch, and the rollback behind a refused one.
The pin follows from that.
columnput the running model at the head of theaccount the wire flagged unless another account listed it, because that flag
went stale the moment a pick moved the conversation; the account is now known
outright on that path, so the guess is only for when it is not. Without that
half the third fix was inert in the shape that prompted it: the guess had
already dropped the pin, so the account the conversation is on did not list the
model at all and the mark went to the one that did. Empty means nothing has
said, and every surface falls back to what it did before.
Key decision: the wire does state a provider for the session's model, and the
identity comparison deliberately does not use it as the identity. The store's
own comment records that the per-row flag names the provider the session STARTED
on and goes stale the moment a pick moves the conversation, which is why
carried()exists. What travels now is the account the conversation is on, settogether with the model by whoever knows both.
bareModelandsameModeltake the provider rather than its name, so the typenames every site that has to say what it knows rather than letting one keep the
narrower rule silently: the picker's tick, fold pin and typed-identifier
routing, the chip's account and label, the column de-duplication, the recent
rows, and the settings popover's already-added test, which had the same gap and
no coverage at all.
Type
Verification
Every test here was watched to fail, for the expected reason, before the
expression it covers was written; the ones written alongside a change were then
proved load-bearing by narrowing the call site back and watching exactly that
case redden, and no other.
The identity table is measured against
merge_keyitself rather than read offits source, and is a literal copy on each side for the reason the kind table
beside it already gives: a change to either side has to show up as a
disagreement.
Re-run after the rebase onto the current base:
npx vitest run src/features/model/ src/chrome/ src/features/settings/ scripts/gates/fixture-shape.test.mjs-> 30 files, 377 passed.
npx tsc --noEmit -p tsconfig.json-> no errors found, on both front ends.uv run --frozen --all-extras pytest tests/test_rpc_model.py tests/test_rpc_schema_match.py-> 612 passed.
node scripts/gen-rpc-client.mjs --check-> matches the contract, 195 methods.node scripts/gen-rpc-types.mjs --check(ui-tui) -> in sync.npm test -- src/__tests__/gatewayTypesDrift.test.ts(ui-tui) -> 1 passed.python3 ui-web/build.py-> both boot snapshots match their goldens.npx eslint .-> 0 errors, 4 warnings, allreact-hooks/exhaustive-depsinSubagentsPage, ConnectionsPage and CronPage, none of which this changes.
node scripts/check-class-namespace.mjs-> OK.scripts/check_commit_messages.py,scripts/check_source_language.pyandscripts/check_large_files.py-> all exit 0, no output.Browser checks over CDP, against temporarily patched offline fixtures, one per
shape. Mixed spellings: the chip's mark went from absent to the owning account's
and the tick from nowhere to one row under it. One identifier in two accounts:
the mark went from the wrong account to the right one and the tick moved with
it, leaving the other account's row listed and unmarked. No page errors in
either. The first of those two is corroborated by the repo's own boot gate,
which reported a node-count mismatch against that fixture on the base and
matches the golden here, the missing node being the chip's provider icon.
The full
ui-websuite is not quoted as a base comparison, because on thismachine two consecutive runs of the same tree differ by ten test ids. A
reviewer's run of the same suite at an earlier head was 193 files / 2,664 tests
green, which is the better evidence that the local instability is environmental.
Two batch failures in
src/features/model/source.test.tsare not from thischange: removing the one line it adds to that file leaves both in place, and the
file passes 32/32 on its own twice.
The third box is left blank because nothing in the documentation or the
screenshots describes the chip's marking, so there is nothing to update.
Risk
User-visible changes, in the composer's model chip and picker and in the
settings add-a-model popover, all of them about what is marked rather than what
is offered:
model is now ticked, the chip carries its account's mark and the registry
label, and a model already configured under an older spelling is no longer
offered as though it were missing;
the account the conversation is on; the other account still lists the model
and can still be picked;
first listed one. That is a change to where data is written and the one worth
a second look.
The comparison is looser than the one it replaces. A prefix belonging to some
other provider is still not stripped, which the identity table pins from that
direction.
Contract:
model.optionsgains an optionalroute_namesper provider row. Itis additive, so an older client ignores it and a server that does not send it
leaves the client on the provider's own name, which is today's rule. Both
generated clients, the strict Python model and the hand-written TUI mirror were
updated and re-checked against the schema.
Security impact: none. No credential, permission or transport rule is read or
written here.
Backward compatibility: no state, storage key or existing wire field changes.
The store gains a second value beside the model it already kept; empty is the
old behaviour.
Rollback: revert the commits. Each stands on its own except the last, which
needs the one that adds the prefix set.
Related Issues
N/A