fix: stop treating a model list as an allowlist - #3330
Conversation
770e89f to
7f41aaf
Compare
f795551 to
ea45a70
Compare
ea45a70 to
a9eb6c8
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Two merge-blocking authority inconsistencies remain on the reviewed revision. Both reproduce on the exact head despite green required CI and focused local verification.
Codex-assisted review performed under the maintainer-approved review workflow.
a9eb6c8 to
3e7a174
Compare
3e7a174 to
9d63444
Compare
|
Both findings landed. The conclusion I drew from them is that fixing either one in place would have left the next exit open, so the branch is rewritten and force-pushed rather than amended — the commits are new, and the old ones described a design that no longer exists. P1-1 is fixed at its root. P1-2 is fixed, though not quite where the comment points. Beyond the two findings: your suggested remedy — persist an explicit partial/non-authoritative classification before using absence as a veto — is what the reviewed revision already was, and it was the second revision to fail in that shape. So I took the question a step back. No list Maka holds is the set of models an account can run. A So the allowlist is removed rather than reclassified. Details in the updated description. CI is green on the new head. Claude Code (Opus 5) did the rewrite and drafted this reply under my direction; I reviewed the diff and own the result. |
… an allowlist A Volcengine Ark Agent Plan key could not select the models its own plan serves. The plan's key reaches only the inference data plane, so Maka had no way to enumerate the account and fell back to the array this build ships. Every gate then read that array as the set of models the account has, and refused everything else. The array is not that set, and no array Maka holds is. A `/models` response describes one call at one moment, scoped to whatever the provider chose to report; a shipped snapshot describes the provider, not the account; and four providers have no list endpoint at all. Only the user knows what their account can run, and only the provider can settle it — by answering the request. So the allowlist goes, rather than growing another exception: - `authorizeConnectionModel` in `llm-connections.ts` is the one seam that answers "can this connection run this model". The user's selection is the authorization. The observed row still wins wherever it exists, for its wire metadata and capabilities, but its absence authorizes nothing and vetoes nothing. - A fetch no longer prunes what the user chose. `reconcileConnectionAfterModelFetch` seeds a first default only for a connection that has never had a list to pick from, and otherwise returns the selection untouched. - `filterDiscoveredModels` no longer intersects a live response with the shipped snapshot, which was silently dropping models from six providers whose real catalogs outgrew it. - Readiness, the connection probe, the hosted execution target and the catalog coordinator all consult that one seam. `connection-model-admission.ts` and its hardcoded DeepSeek exception are deleted. What the picker SAYS is unchanged in spirit and now purely advisory: `classifyConnectionModelInventory` still separates a live response from a snapshot, and a model the live response omitted is annotated `not_in_live_list` — a warning the user can act on, not a refusal. Fixes #1584 Generated-by: Claude Code
…not learn Once a model no catalog lists is selectable, the user needs somewhere to put what they know about it, and a way to add it in the first place. Model declarations (`relayModelProfiles`) already existed but were refused on any non-relay connection. That gate split the wrong way. It turns on the FIELD, not the provider: - `contextWindow` and `vision` state facts about a model. A user has them whenever Maka does not — a model newer than the bundled snapshot, or any model on a provider with no model-list endpoint — so they are legal on every provider. - `thinkingLevels` and `serviceTier` name a wire feature instead (`reasoning_effort` tiers, priority processing) that only the OpenAI-compatible relays accept, and stay relay-only. Declaring them elsewhere would describe a request Maka never sends, or draw a 400 the user cannot explain. `assertProfileFieldsFitProvider` in the catalog codec is the single write seam for that rule, so the read paths do not re-derive it. On the settings side, a connection detail page now offers "Add model" alongside "Update models" rather than instead of it: the two answer different questions, and a provider that supports discovery can still serve a model its discovery did not report. The dialog takes an id plus the optional declarations above. The field name stays `relayModelProfiles` — renaming a persisted field is a migration this change does not need — and its doc comment now explains that the name is historical. Generated-by: Claude Code
9d63444 to
9fef223
Compare
|
Ready for another look. Since the review above:
@hqhq1025 re-requested since the finding was yours; anyone else on the list is welcome. Note that the commits are new — reviewing the previous ones would describe a design that no longer exists. |
hqhq1025
left a comment
There was a problem hiding this comment.
One actionable finding is attached inline.
Problem and mechanism: the PR correctly identifies the root problem: neither a bundled snapshot nor one /models response is a durable account allowlist. The rewritten design makes the user selection the authorization, keeps discovered rows as metadata and capability evidence, stops discovery and onboarding from pruning user choices, and adds a Desktop path for declaring a model plus the facts Maka cannot learn. The two findings from the previous revision are fixed at their common root.
First principles and Occam: removing the false allowlist is simpler and more accurate than adding provider-specific exceptions. authorizeConnectionModel is an appropriate single admission seam, while inventory classification remains advisory for UI provenance. The Storage refresh and onboarding paths now preserve undisplayed user choices and their declarations, and the runtime, readiness, and session paths consistently consult the same authority.
Optimality: the core architecture is sound, but the new Add model workflow does not preserve the user input across its own asynchronous persistence boundary. The dialog clears and closes before the write outcome exists, so a normal Runtime Host or persistence rejection turns a recoverable save error into lost form data. Make submission awaitable, retain the draft while saving or after failure, and close only after a confirmed write.
Deletion and tests: I did not find production code or low-value tests that must be deleted. A focused component or hook regression should cover rejected Add model writes and assert that the dialog remains open with both fields intact. No deeper authority refactor is required beyond the submit contract.
Merge verdict: not ready to merge until the Add model failure path preserves the draft. Residual non-blocking gaps are the documented CLI onboarding limitation and GitHub Copilot connection testing still treating its account model list as the diagnostic answer rather than issuing inference.
Verification: reviewed head 9fef223; required GitHub CI is green; npm run build:test passed. The focused suite passed 210 of 211 tests in one run, and the lone unrelated implementation-child patch test passed immediately when rerun in isolation.
The dialog closed on submit and cleared both fields while the write was still in flight, so a rejection left the user with a toast and nothing to retry from — and an exact model id is not something anyone reproduces from memory, which is the whole reason this dialog asks for one. `addDeclaredModel` now returns whether the write landed, matching `save` and `saveRelayProfiles` in the same hook, and the dialog closes only on success. While the write is in flight the confirm button shows a saving state and dismissal is refused, as in `goal-dialog`. Generated-by: Claude Code
|
The P2 is fixed in It was a local slip rather than a design gap: Verification on the new head: desktop 1017, Storybook 144 stories, |
Summary
A Volcengine Ark Agent Plan key cannot select the models its own plan serves (#1584). The plan's key reaches only the inference data plane, so Maka cannot enumerate the account and falls back to the array this build ships — and every gate read that array as the set of models the account has.
The array is not that set, and no array Maka holds is. A
/modelsresponse describes one call at one moment, scoped to whatever the provider chose to report; a shipped snapshot describes the provider, not the account; four providers have no list endpoint at all. Only the user knows what their account can run, and only the provider can settle it — by answering the request. If the user is wrong, the provider's own error says so, which is strictly better than Maka pre-refusing from a list that may be stale, filtered, or nonexistent.So this PR removes the allowlist rather than adding another exception to it.
authorizeConnectionModelis the one seam that answers "can this connection run this model", and the user's selection is the authorization. An observed row still wins wherever it exists — it carries wire metadata and capabilities a synthesized entry cannot — but its absence authorizes nothing and vetoes nothing. A fetch no longer prunes what the user chose;filterDiscoveredModelsno longer intersects a live response with the shipped snapshot (which was silently dropping models from six providers whose real catalogs outgrew it); readiness, the connection probe, the hosted execution target and the catalog coordinator all consult that one seam, andconnection-model-admission.tswith its hardcoded DeepSeek exception is deleted.What the picker says is unchanged in spirit and now purely advisory: a model the live response omitted is annotated
not_in_live_list, a warning the user can act on rather than a refusal.The second commit is the other half: nothing could put a model into such a connection. An "Add model" action now sits alongside "Update models" instead of replacing it, and the
relayModelProfilesgate splits by field rather than by provider —contextWindow/visionare facts about a model and legal everywhere,thinkingLevels/serviceTiername an OpenAI-compatible wire feature and stay relay-only.Fixes #1584
History
An earlier revision of this branch repaired the read path and kept the allowlist. Adversarial review found a P0 in it; the redo built a read-side authority, and review found two more P1s. Both rounds were the same shape — every gate that consulted a list Maka does not own was another exit — so the branch was rewritten against the conclusion above and the concept was removed. The commits are new; the previous ones described a design that no longer exists.
Verification
@maka/core584,@maka/storage848,@maka/runtime3002,@maka/runtime-host1042,@maka/desktop1017,maka-agent339,@maka/mcp115, Storybook smoke 144 stories,format:check, the Astryx surface inventory andtypecheckclean across every workspace. Rebased onto currentmain(past the Claude subscription retirement) and re-run there.Both commits were typechecked in isolation, so reverting the second one leaves a working tree.
The behavioural contracts that changed were rewritten rather than deleted: a discovery run that stops listing a selected model now asserts the selection survives, the connection probe asserts it tests the user's model even when the last inventory came back empty, and the workspace default target asserts it stays put. The end-to-end test drives a real
volcengine-agent-planconnection throughtestConnectionand asserts the wire body names the user's model.Known gap
The CLI onboarding wizard still offers only what discovery returned, so declaring a model by hand is reachable from desktop connection settings only. CLI
/setupno longer deletes models added there, but it cannot add one.AI use
Tool(s) and scope: Claude Code (Opus 5) wrote the implementation, tests and this description under my direction, and ran the suites above. I reviewed the final diff and commit messages and own the result.
Checklist
Does this PR entail a change in behavior?