fix(desktop): align the connection base URL gate with the Runtime Host - #3673
fix(desktop): align the connection base URL gate with the Runtime Host#3673shaokeyibb wants to merge 1 commit into
Conversation
ca6e445 to
26b3d03
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Scope
First independent full review of exact head 26b3d031a39e3e75d84ef5eb28b64c4a7daed5f2.
I covered the shared base-URL validation/normalization contract, the Runtime Host catalog codec and canonical persisted-entry boundary, create/edit renderer-to-IPC flows, localized rejection copy, tests, and the exact-head CI result.
Findings
No P0–P3 findings in the covered scope.
The renderer and Runtime Host now use the same canonicalization authority. The codec continues to reject non-canonical or invalid persisted entries at the existing canonical decode boundary; I found no new silent-drop path or behavior that deletes legacy connections. Clear intent remains distinguishable from an omitted update, and OAuth/default endpoint handling remains explicit.
Verification
- Exact-head CI
testrun32691266671: terminal success. - Local
@maka/coretests: 651 passed, 0 failed. - Changed-file Biome check: passed.
git diff --check: passed.
The desktop main build was not used as a PR signal because this shared workspace has pre-existing cross-package dist/type drift; its errors were outside the changed endpoint path.
Evidence gap
The PR changes user-visible endpoint error copy, but no screenshot or visual artifact is attached. The new focused tests validate behavior and copy selection, not rendered visual output. I record this as an acceptance-evidence gap, not a code finding.
The Desktop forms accepted endpoints the Host refuses. A service URL carrying a query string, a fragment, or embedded credentials passed the add form, passed the IPC gate, and failed at the write — where the Host's English domain error reached the renderer unclassified and fell through to `actionFallback`: "the model connection service is temporarily unavailable". That named neither the field nor the rule, and asked for a retry that could never succeed. The rule now has one implementation. `canonicalizeConnectionBaseUrl` holds the shape check, the scheme allowlist, the credential and query/fragment bans, the byte cap, and the canonical form; `validateConnectionBaseUrl`, `normalizeConnectionBaseUrl`, and the Host's `normalizeCatalogConnectionBaseUrl` all delegate to it and keep only their own wording. A rejection travels as a code, so the Desktop form can mark its endpoint field with localized copy while the Host keeps its wire-facing sentences unchanged. Both write surfaces check before they write: the add form reports on the `baseUrl` field, and the detail page's endpoint row reports the rule instead of the generic service message. Canonicalization is new on the client and deliberate. The previous note argued that rewriting `https://Example.com:443/V1` could surprise whoever typed it — but the Host canonicalized on write regardless, so that form was never what got stored. Doing it here only moves the surprise to where the user can still correct it. The Host contract is untouched. Allowing query strings, for Azure-style `?api-version=` gateways, is a product and security decision for dev@maka.apache.org, not an implementation detail. Fixes apache#3672 Generated-by: Claude Code Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
26b3d03 to
3a83833
Compare
|
rebased and resolve the conflict to match latest changes in main branch |


Summary
The Desktop connection forms accepted endpoints the Runtime Host refuses. A service URL carrying a query string, a fragment, or embedded credentials passed the add form's field gate, passed the main-process IPC gate, and then failed at the write — where the Host's English domain error reached the renderer unclassified and fell through to
actionFallback:That names neither the field nor the rule, and asks for a retry that can never succeed. Both write paths were affected: adding a custom relay and editing an existing endpoint.
Two validators had drifted.
validateConnectionBaseUrlchecked length,URLparseability, and an http/https allowlist, and documented "trim is the only canonicalization".normalizeCatalogConnectionBaseUrladditionally rejected credentials and?/#, canonicalized throughURL.toString(), and capped the canonical form at 2048 bytes.The rule now has one implementation.
canonicalizeConnectionBaseUrlholds the shape check, the scheme allowlist, the credential and query/fragment bans, both caps, and the canonical form.validateConnectionBaseUrl,normalizeConnectionBaseUrl, and the Host'snormalizeCatalogConnectionBaseUrlall delegate to it and keep only their own wording — the Host's wire-facing sentences are byte-identical to before, since they are pinned by protocol tests. A rejection travels as a code (credentials,query_or_fragment, …), which is what lets the Desktop form render localized field copy from the same decision the Host makes.Both write surfaces now check before they write: the add form marks its
baseUrlfield, and the detail page's endpoint row reports the rule that was broken instead of the generic service message.Two notes on scope:
https://Example.com:443/V1could surprise whoever typed it that way. But the Host canonicalized on write regardless, so that form was never what got stored — the client was preserving a value the system discarded one hop later. Doing it here only moves the surprise to where the user can still see and correct it. The one test that pinned the old behavior is updated with that reasoning.?api-version=gateways want, is a product and security decision fordev@maka.apache.org, not an implementation detail. This PR only makes the client agree with the contract that already exists.Fixes #3672
Verification
npm run lint,npm run format:check,npm run build,npm run typecheck,npx knip --workspace apps/desktop,npx knip --workspace packages/ui— all clean.@maka/core@maka/desktop@maka/storage@maka/runtime-hostThe reported scenario, before and after, run against the built tree:
The message the user reads at step 1 is now the rule, on the endpoint field:
New tests, each pinning one contract:
?staying accepted — the Host reads the typed text rather than the parsed URL, and so does the client now.invalidissues, that the rule is not relay-only (a local runtime's editable port is checked too), that Cloudflare stays exempt because its field holds an account id rather than a URL, and that a bare host, a local port, and a percent-encoded?remain accepted.Driven through the real app, using the
settings-modelse2e fixture whose seededno-modelsconnection is anopenai-compatiblerelay with an editable endpoint row. Same steps in both: edit the service URL row, enter the URL above, save.The row stays open with the draft intact either way; what changes is whether the message describes the thing that went wrong. Screenshots follow in a comment.
Review focus
The canonicalization change is the one behavior change a reviewer should weigh rather than read past — it is what required editing an existing test's expectation rather than only adding to it. If the project would rather keep the client's as-typed value and fix only the misleading error, dropping the canonical form from
canonicalizeConnectionBaseUrland reverting that one assertion leaves the rest of the fix intact.AI use
Select exactly one:
Tool(s) and scope: Claude Code diagnosed the drift between the two validators, implemented the shared rule across core / the Host codec / the Desktop forms and copy, and wrote the tests and verification above. I reviewed and verified the result.
Checklist
Does this PR entail a change in behavior?