Skip to content

feat(desktop): fetch custom relay models before connect - #3299

Open
localhost-copilot wants to merge 2 commits into
apache:mainfrom
localhost-copilot:Connect-Custom-relay-fetch-models
Open

feat(desktop): fetch custom relay models before connect#3299
localhost-copilot wants to merge 2 commits into
apache:mainfrom
localhost-copilot:Connect-Custom-relay-fetch-models

Conversation

@localhost-copilot

Copy link
Copy Markdown
Contributor

Summary

Add model discovery for unsaved custom relay providers.

Users can fetch models using the configured endpoint, API key, and request headers, then
select a default model from the returned catalog. If discovery fails or /models is
unavailable, manual model entry remains available and required.

The preview uses transient Runtime Host verification and does not persist credentials or
connection data. The Runtime Host compatibility epoch is bumped so older hosts reject the
new preview inputs safely.

Verification

  • npm --workspace @maka/runtime-host test — 1022 passed
  • npm --workspace @maka/desktop test — 972 passed
  • npm --workspace @maka/desktop run typecheck
  • npm run lint
  • npm run format:check
  • npx knip --workspace apps/desktop
  • git diff --check

UI evidence:
mac_1787214476615

Add transient model discovery for unsaved custom relay configurations, expose it through the Desktop bridge, and let users select a discovered model while preserving manual entry as fallback.

@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.

Reviewed at d20fd8c8. The feature is well-shaped and the two things most likely to go wrong in a change like this are both handled correctly — I checked them specifically rather than assuming:

  • A user-supplied endpoint never receives a stored credential. #discoverOnboarding sets candidate = undefined whenever input.baseUrl is present, so exportCredentialMaterial is never consulted and secret collapses to the supplied key alone. Without that guard, { baseUrl: <attacker>, apiKey: null } would have shipped the saved provider key to an arbitrary host. It reads as deliberate, and it is the right guard.
  • Preview-only fields cannot reach persistence. Changing ConnectionOnboardingSaveInput from extends ConnectionOnboardingVerifyInput to a standalone interface means baseUrl and requestHeaders are structurally absent on the save path, so a transient endpoint cannot be smuggled into a stored connection. That is the correct way to express "preview only", better than a runtime check.

The blocking problem is not in this PR's own logic — it is a collision with a PR that is open right now. #3236 also bumps RUNTIME_HOST_COMPATIBILITY_EPOCH from 27 to 28, for an unrelated reason (staged access.credential.prepare/finalize). Both branches write the literal 28, so a textual merge is clean and silent, and the second one to land ships an epoch that no longer distinguishes two independent, mutually-incompatible protocol changes. Details inline; this needs coordinating before either merges.

The remaining architectural question is one of contract rather than correctness. validateConnectionBaseUrl allows any http:/https: URL with no restriction on the host, so this operation lets a Client make the Runtime Host issue an outbound request to an arbitrary address with arbitrary headers. That capability is not new — a user could already create a connection with any baseUrl, call connection.models.fetch, and delete it. What changes is that it now requires no catalog mutation and leaves no trace, and it arrives at the same time as #3236 makes remote Runtime Hosts a first-class deployment. Whether a remote Host should accept arbitrary outbound targets from its Client is a decision worth making explicitly rather than inheriting.

Reviewed with Claude Opus as an analysis assistant. Every claim here was verified by reading source at this head — including assertExactKeys, validateConnectionBaseUrl, the epoch comparison in client/connection.ts, and #3236's own diff. Nothing was executed; the epoch collision is a reading of both branches, not an observed merge.

export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 27 as const;
// 27: Runtime Policy carries the Host-owned shell preference used by tool,
// PTY, and prompt composition. Older peers cannot safely preserve that field.
export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 28 as const;

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.

[P1] Coordinate this epoch bump with #3236, which raises the same constant to the same value for a different reason. #3236 changes 27 → 28 for staged access.credential.prepare/finalize; this PR changes 27 → 28 for the transient endpoint and request-header fields. Both branches write the literal 28, so git merges them without a conflict and the second to land silently ships one epoch covering two independent incompatibilities. The concrete failure: a Client built from this branch and a Host built from #3236 both advertise 28 and are admitted by compatibilityEpoch !== RUNTIME_HOST_COMPATIBILITY_EPOCH, then the Host's requireExactRecord rejects baseUrl as an unknown field and aborts the transport — which is precisely the outcome the epoch exists to replace with a structured incompatible frame. Confirmed by reading both branches at their current heads; not reproduced by merging. Whichever PR lands second must take 29 and append its own comment line rather than accepting the textual merge. A test that pins the epoch to a literal would turn this silent collision into a failing check; there is currently none.

const slug = deriveConnectionSlug(input.providerType);
const catalog = await this.#stores.connectionCatalog.getSnapshot();
const candidate = catalog.connections.find((connection) => connection.slug === slug);
const candidate = input.baseUrl

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.

[P2] Decide explicitly whether a Client may direct the Host's outbound requests at an arbitrary address. validateConnectionBaseUrl constrains only scheme and length, so any http:/https: URL is accepted here — including link-local and private-range addresses such as a cloud metadata endpoint — and createRequestCustomizationFetch attaches caller-supplied headers to that request. This is a contract decision, not a defect: the capability already exists via create-connection plus connection.models.fetch, and the credential guard immediately below this line correctly prevents a stored key from reaching a supplied endpoint. What changes is that the request now requires no catalog mutation and leaves no persisted trace, arriving as #3236 makes remote Runtime Hosts first-class — so the actor and the Host are increasingly on different machines and networks. Confirmed by reading code at this head; not exercised against a live Host. Either state in docs/runtime-host-remote-access.md that a Client may originate arbitrary outbound HTTP from the Host, or constrain preview targets. Regression test: whichever rule you choose, assert it here — a preview against a link-local address should have a defined, tested outcome.

const input = requireExactRecord(value, 'connection onboarding verification input', [
'providerType',
'apiKey',
...('baseUrl' in fields ? ['baseUrl'] : []),

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.

[P3] Pass the full key list to requireExactRecord instead of deriving it from the value being validated. assertExactKeys only rejects keys that are not in the allowlist — it never requires a listed key to be present — so ...('baseUrl' in fields ? ['baseUrl'] : []) admits exactly the same inputs as listing 'baseUrl' unconditionally. The conditional and the extra requireRecord call above it are therefore dead machinery, and worse, they read as though the allowlist adapts to the payload, which is the one thing an exact-record check must never do. The next reader auditing this decoder for injectable fields has to work out that it is a no-op before they can trust it. Confirmed by reading codec.ts at this head. List all four keys directly and drop the fields binding.

- Reserve epoch 28 for staged access credential pairing
- Keep onboarding override compatibility isolated to epoch 29

@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.

Re-reviewed at f96af70e. The P1 is resolved. The epoch is now 29, and the comment block reserves 28 for #3236's staged access-credential pairing — which is better than just moving your own number, because it makes the next person's collision impossible to create silently. Nothing else moved: the delta is protocol/index.ts and protocol.test.ts only.

My other two findings still stand at this head and I am not re-filing them inline:

  • P2, the outbound-target contract: validateConnectionBaseUrl still constrains only scheme and length, so this operation lets a Client point the Host's /models fetch at any http:/https: address with caller-supplied headers. The credential guard remains correct — a supplied baseUrl still forces candidate = undefined, so no stored key can reach a supplied endpoint — this is about whether the Host should originate arbitrary outbound requests at all, which matters more as #3236 makes remote Hosts first-class.
  • P3, the requireExactRecord allowlist computed from the value's own keys, which is a no-op because assertExactKeys only rejects unknown keys.

Reviewed with Claude Opus as an analysis assistant; verified by diffing against the head I previously reviewed and re-reading both files at this one.

@Astro-Han

Copy link
Copy Markdown
Contributor

Heads-up on a cross-PR collision — not a review comment on your change.

RUNTIME_HOST_COMPATIBILITY_EPOCH is 27 on main, and three open PRs based on main each take it to 28 with different wire changes: #3236 (access credential prepare/finalize), #3199 (goal.arm), #3133 (session trace cursor pages). #3299 sits at 29 on the assumption that exactly one 28 lands.

The trap is that this does not conflict. All three branches write the same text to that line, so git's three-way merge takes it silently; only the adjacent comment block conflicts, and keeping both comments is the natural resolution. Each PR's own assert epoch > 27 still passes. The result is two incompatible protocols sharing epoch 28 — and since client/connection.ts compares with strict inequality, a matching epoch admits the peer, and the unknown operation then fails decode and tears down the transport, bypassing the structured incompatibility path the epoch exists to provide.

Please re-check against main immediately before merge rather than at rebase time; whoever lands second needs to re-bump. Filed #3313 to stop doing this by hand.

(Posted with Claude Code (Opus 5) assistance; the epoch values were read from each branch head.)

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.

2 participants