fix(cli): let the TUI wizard create custom relay connections - #3467
Conversation
Adversarial review record (pre-submission)An independent review pass probed the change with running experiments (driven wizard sessions against the built TUI, storage-level probes through the real document stack, protocol decode drills, dist fault injections). It found five defects, all fixed in the submitted head, plus confirmations: Defects found and fixed
It also flagged the storage half as untested — two dist fault injections (removing the URL-change commit condition; making the intent field required) survived every existing suite. The submitted head adds a storage intent-journal test (round-trip + legacy replay) and the endpoint-swap coordinator assertions to close the reachable half of that gap. Confirmed safe (with evidence)
Two observations left as-is, for the record: the protocol accepts an endpoint override for any api-key provider (Desktop's settings already allow the same, and the TUI only offers the step for relays); |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for extending the existing Runtime Host onboarding seam instead of adding a CLI-only connection path. The protocol, validation, journal replay, endpoint-change cleanup, and TUI lifecycle all line up well on this head. One existing-connection identity gap becomes user-visible for custom relays, noted inline.
AI-assisted review disclosure: OpenAI Codex coordinated two independent exact-head review passes. I verified the retained connection-identity path, current CI state, reviews, and mergeability, and I made the final review decision.
Adversarial review record — identity-targeting increment (de62675)The connection-identity change was probed by an independent review pass with experiments against the real storage stack before pushing. Two defects found and fixed in the pushed head, one pre-existing issue documented: Fixed:
Verified safe (with running experiments):
Pre-existing, documented not fixed: crash-recovery treats a non-convergent onboarding intent as fatal ( |
|
Hi — this PR conflicts with current I tested a rebase onto current
These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current Thanks for the contribution — happy to help if any conflict is unclear. AI-assisted maintenance note, not a review. It does not count as the required human review under |
de62675 to
d1fe0c8
Compare
|
Rebased onto current Both conflicts were the compatibility epoch: Also picked up in the rebase: the new storage test file gained the ASF license header main's audit gate now requires ( Local verification on the rebased head (Node 24.18): storage 900 pass / 0 fail, CLI 393/393, desktop 1243/1243 + typecheck clean, biome clean on the full diff vs main. runtime-host's onboarding/protocol suites pass; its |
d1fe0c8 to
327ed2e
Compare
|
Rebased again onto current Two things changed underneath:
Local verification on the new head: storage 901 pass / 0 fail (includes #3571's new recovery test alongside this PR's journal tests), CLI 405/405, runtime-host onboarding + protocol suites 65/65, biome clean on the full diff, ASF header audit clean. Ready for the CI approval on this head. |
|
Rebased a third time — Verification on this head: clean rebuild, storage 901/0, CLI 406/406, runtime-host onboarding + protocol suites 65/65, biome clean on the full diff, ASF header audit clean. @Astro-Han this PR only ever conflicts on the epoch line, and each cycle costs a maintainer CI approval — if it goes stale again before review, I'm happy to keep rebasing, but if you have a review window it may be cheaper to look at this head while it's fresh. |
327ed2e to
def0ab4
Compare
There was a problem hiding this comment.
English
Reviewed exact head def0ab4ab9c690f42d080ed17d1bce03f8810ee2: NO-GO — 1×P1 + 1×P2.
- [P1] This head's epoch 43 is below current
mainepoch 44 even though the PR adds required fields and new values to closed onboarding wire shapes. The merge-result guard rejects base=44/head=43 aswent backward; this change is not compatible-declaration eligible because old peers reject the new shapes. - [P2] Remote onboarding discovery is not bound to the connection revision, effective endpoint, and credential basis used by the final commit. A supported concurrent policy update can persist relay B/key B with the model inventory discovered from relay A/key A; the production-coordinator reproduction is documented inline.
The protocol/storage/TUI propagation is otherwise feature-proportional and keeps the Host as the persistent authority. The previous custom-slug identity issue is closed on this head; no additional decision-changing issue was found.
Verification: 205/205 focused affected tests passed; Core, Storage, Runtime, Runtime Host, MCP, Eval, and CLI builds passed; Biome and git diff --check passed.
Gate state: the exact-head hosted test is terminal red in an unrelated MCP timing test. The PR also conflicts with current main, so GitHub cannot synthesize a current merge ref and no fresh merge-result gate evidence can be obtained until rebase. Current main is independently red in a Storage compile failure; that baseline failure is not attributed to this PR.
中文
已审查 exact head def0ab4ab9c690f42d080ed17d1bce03f8810ee2:NO-GO——1 条 P1 + 1 条 P2。
- [P1] 当前 head 的 epoch 43 低于当前
main的 epoch 44,但本 PR 又给闭合 onboarding wire shape 新增了必填字段和新值。merge-result guard 会把 base=44/head=43 判定为went backward;旧 peer 会拒绝这些新形状,因此本变更不能使用 compatible-change 声明。 - [P2] 远程 onboarding discovery 没有绑定最终 commit 所依赖的连接 revision、实际 endpoint 和凭证依据。受支持的并发 policy 更新可以把 relay B/密钥 B 与 relay A/密钥 A 发现的模型清单一起持久化;生产 coordinator 复现已记录在行内 finding。
除此之外,协议、存储与 TUI 的传播复杂度与功能相称,并继续以 Host 作为持久化权威。上一轮 custom-slug 身份问题已在当前 head 关闭;没有发现其他足以改变合并判断的问题。
验证结果:受影响的 focused tests 205/205 通过;Core、Storage、Runtime、Runtime Host、MCP、Eval、CLI 构建通过;Biome 与 git diff --check 通过。
门禁状态:exact-head hosted test 因无关的 MCP 时序测试终态失败。PR 还与当前 main 冲突,因此 GitHub 无法生成当前 merge ref,在 rebase 前无法取得新的 merge-result 门禁证据。当前 main 自身也因 Storage 编译失败而红;这个基线失败不归因于本 PR。
The setup wizard filtered out every provider without a built-in base URL — an explicit phase-1 scope cut (apache#1254) whose phase-2 base-URL prompt never landed — and the onboarding protocol had no field to carry an endpoint anyway, so the three custom relays were creatable from Desktop but unreachable from the TUI. List the category:'custom' relays (cloudflare-workers-ai stays out: its endpoint is an account-id template, not a user-supplied URL), add a base-URL step to the wizard between provider pick and API key with host-mirroring local validation, and thread an always-present 'baseUrl: string | null' through connection.onboarding.verify/save — exact-record wire style like apiKey, normalized by the shared catalog rules, rejected as base_url_not_configured when a relay has no endpoint from input, existing connection, or registry. Blank input on an existing relay reuses its persisted endpoint, mirroring the blank-key contract. Discovery runs against the supplied endpoint and commit persists it: the intent journal gains the field (legacy journals still replay), the upsert resolves input ?? previous ?? registry default, a URL-only change still commits, and a swapped endpoint drops relayModelProfiles and lastTest — the endpoint-keyed contract the update path already enforces. The onboarding wire shapes are closed schemas, so the compatibility epoch moves to 37. Fixes apache#3405 Generated-by: Claude Code
… relays edit in place The wizard recognized an existing connection only at the derived canonical slug, so a relay created in Desktop under a custom slug read as unconfigured in /setup and saving created a second canonical-slug connection, leaving the old credential and default target behind. Onboarding inputs now carry 'connectionId: string | null': the catalog projection resolves the existing connection (canonical slug first, else the provider type's sole connection), the wizard threads its identity through verify/save, the coordinator targets it directly (rejecting a stale id as connection_not_found instead of duplicating), and the storage upsert finds the row by identity first, preserving its slug. A stale-snapshot rejection renders without the retype-the-key framing. Epoch-37 builds from this PR's own review cycle require baseUrl but not connectionId, so the identity shape gets epoch 38 rather than reusing 37 for a second mutually-undecodable frame. Generated-by: Claude Code
…revalidates Model discovery ran outside the mutation lane and the final commit re-read latest state, so a concurrent supported policy update could persist relay B/key B with the inventory discovered from relay A/key A. Adopt the model-fetch ticket shape: beginConnectionOnboarding locates the target under the write lane and issues a one-shot WeakMap ticket whose basis pins the connection revision (covering the endpoint and every other catalog-visible property), the api-key credential status plus stored secret from one vault read, and the effective proxy with its credential — returning the pinned proxy so discovery runs through the egress the basis certifies rather than re-resolving it. complete revalidates that basis atomically before the durable intent is written: drift returns 'superseded' (a new save rejection, riding this PR's unpublished epoch), a vanished target keeps reporting connection_not_found, and no journal is written on either. Verify abandons its ticket (WeakMap-held); save begins its own. Regression test drives the reviewed race end to end: discovery paused on relay A/key A, endpoint moved and key rotated concurrently, the save supersedes with relay B/key B intact and relay A's inventory never persisted, and a retry commits cleanly. Generated-by: Claude Code
def0ab4 to
bbe7af7
Compare
|
Both findings addressed on head
Local verification: storage 908/0, runtime-host 1123/0, CLI 424/424, desktop typecheck clean, biome + ASF header audit clean. The branch is conflict-free against that main; ready for CI approval on this head. |
Astro-Han
left a comment
There was a problem hiding this comment.
Re-review at bbe7af77. One new [P2], filed inline on connection-effect-coordinator.ts. Not blocking beyond that finding — everything previously raised is resolved at this head.
Closed since the last pass (verified at this head, threads resolved):
- Connection identity —
projectProviders()now resolves a Desktop-created relay under a custom slug to the existing connection and plumbsconnectionIdthrough verify/save, with tests covering the sole-connection, multi-connection and canonical-wins cases. - Discovery/commit basis — the begin/complete ticket pins revision, credential and proxy, so a concurrent policy change supersedes the save instead of pairing a new endpoint with an inventory it never produced.
- Compatibility epoch — head advertises 45 against current
main's 44, monotonic.
CI: test is terminal green on this exact head. Withholding approval only for the open [P2].
中文
在 bbe7af77 上复审。新增一条 [P2],已作为行内评论提在 connection-effect-coordinator.ts。之前提过的问题在这个 head 上都已解决:连接身份(projectProviders() 现在能认出自定义 slug 下的 relay 并透传 connectionId,且有测试覆盖)、discovery/commit 基准(begin/complete ticket 绑定 revision、凭证与代理)、兼容 epoch(head 45 > 当前 main 44,单调)。对应三条旧 thread 已关闭。exact head 的 test 已终态绿;仅因这条未解决的 [P2] 暂不 approve。
… request customization The onboarding probe went out on the bare transport fetch, while the models path wraps it with the connection's custom request headers and body overlay — so a connection that authenticates through a custom header verified and fetched models fine but failed re-onboarding. beginConnectionOnboarding now pins the request-headers secret for the probe and adds its credential status to the ticket's basis, so a header rotation between discovery and commit supersedes the save the same way an endpoint or key change does. Generated-by: Claude Code
Astro-Han
left a comment
There was a problem hiding this comment.
APPROVE at exact head bc3d1e1d3603040ab7993e395a90cc6e30512094. The one remaining [P2] is closed; no P0–P3 open.
Scope of this pass: confirmation that the request-customization [P2] from review 5004343688 is genuinely fixed, plus the gate state at this head. This is not a fresh full-surface re-review of all 19 files — the earlier passes covered that, and their findings are closed.
The [P2] is closed, and verified at the source rather than taken on report. That finding was that onboarding discovery passed the raw transport.fetch, bypassing the createRequestCustomizationFetch(headers + bodyOverlay) wrapper that the ordinary models path applies. At this head both paths construct the same wrapper:
- onboarding —
connection-effect-coordinator.ts:263-268:createRequestCustomizationFetch(transport.fetch, { headers: begun.requestHeadersSecret ? parseRequestHeaders(begun.requestHeadersSecret) : {}, bodyOverlay: base.requestBodyOverlay }) - models —
:407-410: the same call shape withprepared.secretMaterial.requestHeadersandprepared.connection.requestBodyOverlay
The onboarding side is the stricter of the two, deliberately so. It reads the header secret pinned on the ticket (begun.requestHeadersSecret) rather than re-resolving it, and the comment above it states why: a flip-and-restore between the two reads would otherwise pass the basis check. That reasoning also covers the proxy (:251-254). Header create/delete/rotation are checked for superseded before the commit, so a rotation landing mid-discovery is rejected rather than silently committed.
What that means for the connections this finding was about: a connection that authenticates through custom request headers now gets those headers on the onboarding discovery request, so re-onboarding no longer fails for it.
Gates at this exact head
test— terminalsuccess(run32696317110)MERGEABLE, non-draft, no stale approvals- unresolved review threads — 0 (the last one,
PRRT_kwDOSpfFGs6bkiwe, is resolved with this review; it wasisOutdatedbut that only means the anchor line moved, so it was held open until the fix itself was checked) - against current
main04836d3b: the synthetic merge tree is clean and the changed files do not overlapmain's recent commits
No merge from us. Merging is a maintainer's call.
中文
在 exact head bc3d1e1d 上 APPROVE,唯一剩下的 [P2] 已闭合,无 P0–P3。
本次范围:只确认 review 5004343688 那条 request-customization [P2] 是否真的修好,以及该 head 的门禁状态;不是对 19 个文件的重新全量审查——那部分由此前几轮覆盖,结论均已闭合。
[P2] 已闭合,而且是回到源码核的,不是照报告采信。 原问题是 onboarding discovery 传了裸的 transport.fetch,绕过了普通 models 路径套的 createRequestCustomizationFetch(headers + bodyOverlay)。当前 head 上两条路构造的是同一个包装:onboarding 在 connection-effect-coordinator.ts:263-268,models 在 :407-410。
onboarding 那一侧比 models 更严,而且是刻意的:它用 ticket 上钉住的 begun.requestHeadersSecret,不重新解析;上方注释写明了理由——两次读取之间的"翻转再还原"否则能绕过 basis 校验。同一理由也覆盖了 proxy(:251-254)。header 的新建/删除/轮换在 commit 前会判 superseded,所以 discovery 期间发生的轮换会被拒绝而不是静默提交。
对这条 finding 关心的那类连接意味着什么:靠自定义请求头鉴权的连接,现在 onboarding discovery 请求也会带上这些头,重新 onboarding 不再因此失败。
门禁:test 终态 success(run 32696317110);MERGEABLE、非 draft、无陈旧 approval;未决线程 0(最后一条 PRRT_kwDOSpfFGs6bkiwe 随本 review 关闭——它显示 outdated,但 outdated 只说明锚点行移动,所以一直挂着直到修复本身被核实);对当前 main 04836d3b 的 synthetic merge tree 干净、改动文件与 main 近期提交无重叠。
我们不合并,合并是 maintainer 的决定。
|
LGTM — merging. The wizard being able to create a custom relay connection closes a real gap; thanks for working through the review rounds on it. 中文看过了,合并。向导能建自定义中转连接补上了一个实际缺口,感谢配合几轮修改。 |
Summary
The TUI setup wizard (
/setupand first-run) could not create any of the three custom relay providers — Desktop could. Two gaps, as diagnosed in the issue: the catalog filtered out every provider without a built-in base URL, and the onboarding protocol had no field to carry an endpoint even if they were listed. Per the archaeology in the issue thread, the filter was an explicit phase-1 scope cut in #1254 whose phase-2 base-URL step never landed.This implements the issue's proposed boundary:
listApiKeyOnboardableProviders()now lists thecategory: 'custom'relays;requiresBaseUrltells the wizard to collect an endpoint. Providers whose endpoint is derived rather than user-supplied stay excluded —cloudflare-workers-aialso has an empty registrybaseUrl, but it interpolates an account id into a URL template, and Desktop deliberately keeps it out of the base-URL field too; a naive!baseUrlunfilter would have offered it a relay prompt it cannot use.requiresBaseUrlproviders (relays show a 4-step flow, everything else keeps its 3 steps). Input is validated in place with the same rules the Host enforces (http/https, no credentials/query/fragment, 2048-byte cap), so mistakes fail with a readable message instead of a protocol decode error. Blank input is allowed only when a connection already exists — it reuses the persisted endpoint, mirroring the blank-key-reuses-stored-secret pattern.connection.onboarding.verify/savecarry an always-presentbaseUrl: string | null(exact-record wire style, likeapiKey). A non-null value goes through the shared catalog normalizer (normalizeCatalogConnectionBaseUrl; a provider-default value collapses to null). A relay with no endpoint from input, existing connection, or registry is rejected with the newbase_url_not_configuredreason before any network probe. Since the input shapes and result unions are closed wire schemas,RUNTIME_HOST_COMPATIBILITY_EPOCHmoves to 37 so a mixed pair fails the handshake instead of tearing down the session on the first/setup.commitConnectionOnboardingpersists it: the journaled onboarding intent gains the field (allowed-but-not-required on read, so an intent journaled by an older build still replays as "no override"), andprepareOnboardingUpsertresolvesinput ?? previous ?? registry default. Two consistency rules ride along: the no-change early-return also compares the base URL so a URL-only change still commits, and a swapped endpoint dropsrelayModelProfilesand the last test result — the same endpoint-keyed contractapplyConnectionUpdatealready enforces, so a new relay does not inherit capability declarations or a "verified" badge from the relay it replaced.Saving a relay updates the same derived-slug connection Desktop manages, so both surfaces stay in sync.
Fixes #3405
Verification
npm --workspace @maka/runtime-host run test— 1064 pass / 0 failnpm --workspace @maka/storage run test— 864 pass / 0 fail (14 pre-existing skips)npm --workspace maka-agent run test(CLI) — 358 pass / 0 failnpm --workspace @maka/desktop run typecheck+ tests — clean, 1038 pass (desktop consumes the changed protocol types; it has no onboarding-operation callers)biome checkon all changed files — clean;knipoutput byte-identical tomainNew tests (kept deliberately light — five additions, each pinning one contract):
base_url_not_configuredbefore any probe; a supplied endpoint reaches discovery and persists; a blank re-verify reuses the persisted endpoint; a re-onboarding that swaps the URL persists it and drops the old relay's profile table2/4), a malformed endpoint is rejected in place, and the endpoint threads through bothverifyandsaveAn independent adversarial review pass probed the change with running experiments before submission; it surfaced five defects — the missing compatibility-epoch bump, the
cloudflare-workers-aiscope leak, stale endpoint-keyed state surviving a URL swap, a missing local byte cap, and two formatter misses — all fixed and re-verified above, with its blank-reuse/crash-replay/normalizer/state-machine probes coming back clean. Details in the review-record comment on this PR.AI use
Select exactly one:
Tool(s) and scope: Claude Code implemented the fix across the protocol/coordinator/storage/TUI layers, wrote the tests, and ran the verification; an independent adversarial review pass (also Claude) probed the change with fault-injection experiments and its findings were fixed before submission. I reviewed and verified the result.
Checklist
Does this PR entail a change in behavior?