fix(#671): stop the store offering installs the server refuses, and say why a key is unverified - #672
Merged
Merged
Conversation
…ay why a key is unverified The two decisions left over from #605. Neither was a code bug on its own — both were surfaces that withheld what the operator needed to act. ## OM-06 — store "install" vs. admin "connected" The issue asked whether a provider is ONE concept or TWO. The code had already answered: a catalogue entry (registry + models) and a credential (vault, own verification lifecycle) are genuinely different things, and a provider can be known-but-unconfigured or configured-but-blocked. Merging them would erase real states. So the concepts stay; the surface is fixed. `install_available` was `install_state === 'available'`, which only says "not already installed". It never asked whether the CAPABILITY was taken — while `InstallService.create` refuses exactly that with 409 `install.capability_already_provided`. The store therefore advertised a button the server was guaranteed to reject, which is what the report saw: "Jetzt installieren" for a provider the admin area already listed as connected, with nothing linking the two. Same check, same helper, one turn earlier. `findActiveProviderCollision` starts from a catalog lookup, so it is blind to a hub-only entry — which is precisely the case that failed. Factored out `findProvidesCollision`, which takes the `provides` list the registry summary already carries; `findActiveProviderCollision` is now a thin wrapper so the two cannot drift. `blocked_by_active_provider` is structured rather than folded into `blocking_reasons` (server-authored English the client can only print), because the operator's next step is to CONFIGURE the provider they already have — and no client can build that link by parsing prose. The store detail page now renders that link. ## /setup 403 — ratified, and made legible Reviewed rather than rubber-stamped. #599 did NOT weaken the gate: a bare 403 becomes `unverified` with `reason: 'forbidden'`, never `verified`. Only an explicit `authentication_error` marker still earns `invalid`. That is a correction of a false accusation, not a relaxation — a region block is no evidence a key is bad, and the old behaviour locked out region-restricted operators with valid keys. Keeping it. What it left behind was a bare `UNVERIFIED` chip covering both "your key is fine, your region is blocked" and "the provider was down". The verdict already carried `reason`, and `ProviderVerificationReason`'s own comment says it exists so "a future UI can map it to a localized string without a second server change". This is that UI: `verifyReason` on the DTO, a closed code->key map in the panel, en+de copy for all six reasons. Unknown codes render nothing rather than leaking a raw code at the operator. ## Mutation checks Every behaviour is proven to fail, not assumed to: - collision helper: always-null, stop-ignoring-inactive, stop-ignoring-self - store wiring: ignore the collision, drop the structured field - UI: drop the line, render for any status, leak an unknown code One first attempt reported green because the perl edit never landed; each mutation now asserts its own anchor before running. ## Test-stub bug this exposed `fakeRegistry`/`fakeInstalled` in registryInstallMerge.test.ts omit `list()`, which `InstalledRegistry` requires — hidden by `as unknown as`. The new check calls it, so four remote-plugin tests 500'd. Stubs corrected rather than the production path made defensive: a stub that does not implement its interface is the defect.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #671
The two decisions left over from #605. Neither was a code bug on its own — both were surfaces that withheld what the operator needed in order to act.
OM-06 — store "Jetzt installieren" vs. admin "VERBUNDEN"
The question, answered from the code
The issue asked whether an LLM provider is one concept or two. The code has already answered: a catalogue entry (registry + models,
/api/v1/admin/providers) and a credential (vault key, its own verification lifecycle) are genuinely different things. A provider can be known-but-unconfigured, or configured-but-region-blocked. Merging them would erase real states.So the concepts stay as they are. What was broken is the surface.
What was actually wrong
install_availablewasinstall_state === 'available'— which only says "not already installed". It never asked whether the capability was taken. MeanwhileInstallService.createrefuses exactly that case with409 install.capability_already_provided.So the store advertised a button the server was guaranteed to reject. That is the reported shape: "Jetzt installieren" for a provider the admin area already listed as connected, with nothing linking the two surfaces.
Same check, same helper — one turn earlier.
The hub-only wrinkle
findActiveProviderCollisionbegins with a catalog lookup, so it returnsnullfor a plugin that has no local manifest — i.e. exactly the hub-published entry that failed. Verified locally:llm-adapter-anthropichas nomanifest.yaml(it is a library), andanthropic_base_urldoes not exist anywhere in this repo, so the entry in question is served from the remote registry.Factored out
findProvidesCollision, which takes theprovideslist the registry summary already carries (registryEntryToPluginpopulates it).findActiveProviderCollisionis now a thin wrapper over it, so the two cannot drift apart. A test asserts the wrapper and the new function agree.Why a structured field
blocked_by_active_provideris typed, not folded intoblocking_reasons.blocking_reasonsis a list of server-authored English strings a client can only print — and the operator's next step here is to configure the provider they already have. No client can build that link by parsing prose. The store detail page renders a localized line plus a deep-link to/admin/providers, which is the half the original report called out as missing./setup403 — ratified, and made legibleReviewed rather than rubber-stamped, and the review changed my reading of it.
#599 did not weaken the gate. A bare 403 becomes
unverifiedwithreason: 'forbidden'— neververified. Only an explicitauthentication_error/invalid_api_keymarker still earns the accusatoryinvalid:There are three states, not two. The change moved bare-403 from "your key is wrong" to "we could not tell" — a correction of a false accusation, not a relaxation. A region block is no evidence a key is bad, and the previous behaviour locked out region-restricted operators holding perfectly valid keys. Keeping it.
What it did leave behind
A bare
UNVERIFIEDchip, covering both "your key is fine, your region is blocked" and "the provider was down" with the same word.The verdict already carried
reason, andProviderVerificationReason's own comment says it exists so "a future UI can map it to a localized string without a second server change". This is that UI:verifyReasonon the provider DTO (a code, never a sentence — the web-ui owns all copy)en+decopy for all six reasonsMutation checks
Every behaviour is proven to fail, not assumed to.
install_availableunverifiedTwo of these needed a second attempt and are worth flagging: one reported green because the
perledit never landed, and one mutated a branch the outer guard made unreachable. Each mutation now asserts its own anchor before running, and prints proof it applied.A test-stub bug this exposed
fakeRegistry/fakeInstalledinregistryInstallMerge.test.tsomitlist(), whichInstalledRegistryrequires — hidden byas unknown as. The new check calls it, so four remote-plugin tests started returning 500.Fixed the stubs, not the production path: a stub that does not implement the interface it claims is the defect, and making the route defensive would have papered over it. (
providesitself is guarded with?? []— that one is a real runtime condition, since older hub payloads may omit it, and a 500 on the detail page would be strictly worse than the button being wrong.)Verification
capabilityResolversuitestoreProviderCollisionsuiteregistryInstallMerge(was 4× red)typecheck:testratcheti18n:checkOn the full middleware suite: it is intermittently red on this machine with rotating victims (
routinesTemplateRoutehit a 120 s file timeout, thenbuilderPreviewRoutes, thenstoreReadinessProjection). Every one of them passes in isolation, andstoreReadinessProjectionpasses 3/3 isolated both with and without this change — so this is the localhost-contention flake tracked in #605, not a regression here. CI's 4-vCPU runner is the arbiter.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.