Skip to content

fix(desktop): align the connection base URL gate with the Runtime Host - #3673

Open
shaokeyibb wants to merge 1 commit into
apache:mainfrom
shaokeyibb:fix/connection-base-url-client-gate
Open

fix(desktop): align the connection base URL gate with the Runtime Host#3673
shaokeyibb wants to merge 1 commit into
apache:mainfrom
shaokeyibb:fix/connection-base-url-client-gate

Conversation

@shaokeyibb

@shaokeyibb shaokeyibb commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

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:

模型连接服务暂时不可用,请稍后重试。
The model connection service is temporarily unavailable. Try again later.

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. validateConnectionBaseUrl checked length, URL parseability, and an http/https allowlist, and documented "trim is the only canonicalization". normalizeCatalogConnectionBaseUrl additionally rejected credentials and ?/#, canonicalized through URL.toString(), and capped the canonical form at 2048 bytes.

The rule now has one implementation. canonicalizeConnectionBaseUrl holds the shape check, the scheme allowlist, the credential and query/fragment bans, both caps, and the canonical form. validateConnectionBaseUrl, normalizeConnectionBaseUrl, and the Host's normalizeCatalogConnectionBaseUrl all 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 baseUrl field, and the detail page's endpoint row reports the rule that was broken instead of the generic service message.

Two notes on scope:

  • Canonicalization on the client is new and deliberate. The previous comment argued that rewriting https://Example.com:443/V1 could 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.
  • The Host contract is untouched. Relaxing it to allow query strings, which Azure-style ?api-version= gateways want, is a product and security decision for dev@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.

Suite Result
@maka/core 651 pass / 0 fail
@maka/desktop 1356 pass / 0 fail
@maka/storage 906 pass / 0 fail (14 pre-existing skips)
@maka/runtime-host 1119 pass / 0 fail

The reported scenario, before and after, run against the built tree:

                          before                  after
1. add-form field gate    ACCEPTED                REJECTED {field: 'baseUrl', reason: 'invalid',
                                                            rejection: 'query_or_fragment'}
2. main-process IPC gate  ACCEPTED                REJECTED baseUrl must not contain a query or fragment
3. Runtime Host codec     REJECTED                REJECTED connection base URL must not contain a
                                                           query or fragment

The message the user reads at step 1 is now the rule, on the endpoint field:

en -> The service URL must not contain a query (?) or fragment (#). Use the advanced request
      settings below to add parameters.
zh -> 服务地址不能包含查询参数(?)或锚点(#)。如需附加参数,请使用下方的高级请求设置。

New tests, each pinning one contract:

  • core: the four rules the client did not have (credentials, query, fragment, byte cap), plus a percent-encoded ? staying accepted — the Host reads the typed text rather than the parsed URL, and so does the client now.
  • core: the anti-drift test — a table of fifteen endpoints asserting the client gate and the Host codec agree on both the accept/reject verdict and the canonical value. The defect was drift between the two, not either one's rules, so this is the test that fails if they separate again.
  • desktop: the field gate's new invalid issues, 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-models e2e fixture whose seeded no-models connection is an openai-compatible relay with an editable endpoint row. Same steps in both: edit the service URL row, enter the URL above, save.

Toast
Before Failed to save model connection — The model connection service is temporarily unavailable. Try again later.
After Failed to save model connection — The service URL must not contain a query (?) or fragment (#). Use the advanced request settings below to add parameters.

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 canonicalizeConnectionBaseUrl and reverting that one assertion leaves the rest of the fix intact.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

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

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Copilot AI lite review requested due to automatic review settings August 24, 2026 04:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@shaokeyibb
shaokeyibb force-pushed the fix/connection-base-url-client-gate branch 2 times, most recently from ca6e445 to 26b3d03 Compare August 24, 2026 04:46

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 test run 32691266671: terminal success.
  • Local @maka/core tests: 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.

@shaokeyibb

shaokeyibb commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author
before after

FRI, here's before-after screenshots

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>
@shaokeyibb
shaokeyibb force-pushed the fix/connection-base-url-client-gate branch from 26b3d03 to 3a83833 Compare August 24, 2026 08:43
@shaokeyibb

shaokeyibb commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

rebased and resolve the conflict to match latest changes in main branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(desktop): connection base URL gate is looser than the Runtime Host contract, so a rejected endpoint reports a misleading error

3 participants