feat(server): one-port hub — same-port loopback companion listener, honest hub-gate messages - #4250
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
8440b9d to
dc01427
Compare
리뷰 · 우선순위 71 / 80이 PR는 지금 핵심은 라인 src/types/config.ts · src/config.ts - 라인 베이스 라인 src/cli/dispatch.ts 라인 src/config.ts 주석 라인 src/codex/loopback-target.ts 라인 src/server/index.ts companion 로그 - 포트형은 네 줄 라인 loopbackRouteAllowed 유지(의도) - companion으로 소켓은 살아도 라인 검증 - 타깃 테스트 숫자는 본문에 잘 적혀 있다. 호스티드 CI는 이 리뷰 시점에 아직 pending이 많다. 머지 전에 gates/test 샤드 green을 보고, 특히 companion 통합 테스트가 비루프백 IPv4 인터페이스 없으면 warn-and-skip 하는 경로가 CI 러너에서 실제로 돌았는지 확인하라. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
3e9aab9 to
28c2b79
Compare
…ailnet-bound proxy origin `ocx gui` derived the URL from the proxy bind, so a hub whose `hostname` is a Tailscale address opened a browser at that tailnet origin — a page the local browser cannot authenticate against the management plane, while the hub's own loopback management ingress was sitting there unused. Prefer the ingress when `runtimeRole: "hub"` has it enabled; every other topology keeps the previous URL derivation exactly. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`unauthenticatedLoopbackListener.port` becomes optional. `{ "enabled": true }` with no port is
the companion form: bind `127.0.0.1:<proxy port>` with the existing loopback policy view. That
is the one-port hub topology — remote clients dial `hostname:port` with a credential, local
processes dial `127.0.0.1:port` without one — and it is what lets the integrations that hardcode
`http://127.0.0.1:<proxy port>` keep working on a tailnet-bound hub whose public address they
cannot reach (#4236).
The form is legal only when `hostname` is a specific non-loopback, non-wildcard address; on
`127.0.0.1`/`localhost`/`0.0.0.0`/`::` the public listener already holds that loopback address.
`loopbackCompanionBindError` is the one sentence for that collision, naming the port and both
fixes (set a distinct `port`, or drop the listener because a loopback bind already admits local
callers). It runs at the write boundary over both `hostname` and the listener — so `ocx config
set hostname 127.0.0.1` on a companion host is refused there rather than breaking the next
start — and again in `startServer` before any bind, so a hand edit that skipped validation reads
the same diagnosis instead of EADDRINUSE from a rolled-back transaction.
`effectiveLoopbackListenerPort` joins `isLoopbackHostname` as the single answer to "where do
local callers dial", so no reader repeats `?? port`. The startup line distinguishes the two
forms: the companion states where local processes go, the ported form keeps today's
unauthenticated-surface warning verbatim.
The listener transaction, its rollback, the route allowlist and `startServer`'s synchronous
window are unchanged: this moves a bind address, not an admission decision.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e instead of the toggle Three consequences of the companion form, all in the readers (#4236): 1. `standaloneCodexRoutingTarget` resolves the listener through `effectiveLoopbackListenerPort`, so an omitted port means the public port. Every writer that already goes through it — the Codex provider block, `opencodeProxyBaseUrl`, `syncGrokConfig`, the integrations exporter — lands on `127.0.0.1:<proxy port>` with no admission header, which is the same origin the hardcoded local integrations write. `chooseListenPort` deliberately keeps reading `.port` directly: only an explicitly ported listener reserves a port, because reserving the shared one would refuse every start on a one-port hub. 2. The Grok fence drift check compares against a SET — `{listen.port} ∪ {effective loopback port}`. `ocx sync` writes the listener's port into the fence, so comparing against the public port alone told every hub operator that their freshly synced, working config pointed at a closed port, and to run the command that had just written it (issue defect 4). A third port still warns, still against the public listener. 3. The hub-role gate gets its own skip reason (`"hub-gated"`) and its own sentence: "This machine is a hub; it does not rewrite its own Codex/Grok/Claude configs unless unauthenticatedLoopbackListener is enabled." It travels the existing linearized channel (`CodexWriteLockSkipReason` → `codexInjectLockOutcome` → `CodexInjectResult` → `CodexSyncResult`) so the reason cannot disagree with the write. `ocx restore back` is the worst of the old reports: it committed the toggle ON, got a gated skip, and told the operator to "retry after the competing integration change finishes" — there is no competing writer. `ocx ensure` was the most destructive: it stripped the managed Grok block as if Grok had been switched off. Only an explicit `clientIntegrations.grok === false` authorizes that strip now; a gated hub is told why nothing was written and `~/.grok/config.toml` is left exactly as it is. `ocx sync`, `ocx sync-cache` and the three "startup left Codex native" lines say the same honest thing, and an explicit OFF keeps its existing wording byte for byte. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…hub-gate sentences Config boundary: the port-less form is accepted on a specific non-loopback bind and the absent port SURVIVES the parse (a schema that helpfully filled it in would make the pair look like the #1102 collision on the next write); it is refused on loopback, localhost, ::1 and the wildcards, with a message that names `127.0.0.1:<configured port>` and both fixes; and reverting `hostname` to loopback is refused by that same check, which is the `ocx config set hostname` path. Listener: a real companion start proves one port answers twice — 200 without a credential on 127.0.0.1, 401 on the tailnet address — and that the startup line says "companion" rather than the ported form's warning. A second case pins what did NOT change: `/api/*`, `/healthz`, `/` and `POST /v1/messages` still 404 on that socket, so sharing a port widens no surface. A third proves an impossible companion throws before the public listener opens, leaving the port bindable. Clients: `tests/server/loopback-companion-client-targets.test.ts` is the claim the whole PR rests on — `standaloneCodexRoutingTarget` and the untouched `buildClaudeEnv` resolve to the SAME origin on a companion hub, with no admission header — and keeps the ported form's split as a regression witness for the issue's table. Hub gate: `tests/cli/hub-gated-local-clients.test.ts` holds `restore back` to the gate's sentence instead of the phantom-conflict text (human and `--json`), holds `ocx ensure` to leaving a Grok block it did not own the decision to delete while still stripping on an explicit OFF, and pins the skip reason/message that every other caller prints. Both new files are registered in `scripts/test-layout/layout.json` and `tests/fixtures/test-layout-expected.json`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…p-line helper `already-running ensure leaves Raycast untouched…` transpiles the real `handleEnsure` body and evaluates it with every free identifier injected. Reporting a skipped sync now goes through `startupLeftCodexNativeLine` — the hub gate and the Codex toggle must not print the same sentence — so the harness has to supply that name too, or the case fails with a ReferenceError that says nothing about Raycast. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
What shipped, the decisions behind it — including the one this PR deliberately did NOT make, widening the loopback route allowlist — and the exact verification commands with their counts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e every wildcard spelling `localClientSkipReason` named the hub gate even when `clientIntegrations.codex` was false, pointing that operator at a listener that would not make the sync happen. It is now the conjunction the Grok path already used (toggle on AND gate closed), per client id. `isWildcardHostname` accepted `::0`, `[::0]`, `0::`, `0:0:0:0:0:0:0:0`, bare `0` and padded IPv4 zeros as specific addresses, so a port-less companion on those binds passed validation and then rolled back with EADDRINUSE. Every all-zero spelling is refused up front now. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The oracle pinned the pre-#4236 line; the hub-aware selector keeps the bind-host branch, so pin both the call and the retained branch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
28c2b79 to
216032d
Compare
1 participant
Summary
A hub that binds its tailnet address has no
127.0.0.1:<proxy port>, and most local integrationswrite exactly that. The issue's table lists eight of them —
ocx claude(buildClaudeEnv,fetchClaudeCodeState), Claude Desktop, the gateway cache,system-envand its shell variant,the Cursor route,
api-access, the vision helper — against four writers that do honorunauthenticatedLoopbackListener. That asymmetry is the "Codex works but nothing else does"report. It took 10100 for the tailnet bind, 10104 for the loopback listener, 10102 for the
management ingress, and a hand-made forwarder to paper over the rest.
unauthenticatedLoopbackListener.portbecomes optional.{ "enabled": true }is thecompanion form: bind
127.0.0.1:<proxy port>with the existing loopback policy view. One port,two sockets — remote clients dial
hostname:portwith a credential, local processes dial127.0.0.1:portwithout one. None of the eight sites is edited; they already write the rightURL, and that is the point of fixing the socket rather than the callers.
The form is legal only when
hostnameis a specific non-loopback, non-wildcard address. On127.0.0.1,localhost,::1,0.0.0.0or::the public listener already holds that loopbackaddress, so the pair is refused with one sentence that names the collision
(
127.0.0.1:<proxy port>) and both fixes: set a distinctport, or drop the listener — a loopbackbind already admits local callers. It is refused at the write boundary over both keys, so
ocx config set hostname 127.0.0.1on a companion host is caught there rather than breaking thenext start, and again in
startServerbefore any bind, so a hand edit that skipped validationreads the same diagnosis instead of EADDRINUSE from a rolled-back transaction.
startServerstayssynchronous and the listener transaction, its rollback and the route allowlist are untouched: this
moves a bind address, not an admission decision.
effectiveLoopbackListenerPort(config, publicPort)joinsisLoopbackHostnameinsrc/codex/loopback-target.tsas the one answer to "where do local callers dial", so no readerrepeats
?? port.chooseListenPortdeliberately keeps reading.port: only an explicitly portedlistener reserves a port, because reserving the shared one would refuse every start on a one-port
hub. The startup line distinguishes the two forms; the ported form keeps today's
unauthenticated-surface warning verbatim.
Defect 4. The Grok fence drift check now compares against the set
{listen.port} ∪ {effective loopback port}.ocx syncwrites the listener's port into the fence,so comparing against the public port alone told every hub operator that their freshly synced,
working config pointed at a closed port — and to run the command that had just written it.
Follow-up 2 — the hub gate stops lying.
localClientSyncAllowedrefusing to rewrite a hub'sown clients is the right decision; reporting it as the user's toggle was not. A distinct
"hub-gated"reason now carries one sentence — "This machine is a hub; it does not rewrite itsown Codex/Grok/Claude configs unless unauthenticatedLoopbackListener is enabled." — along the
existing linearized channel (
CodexWriteLockSkipReason→codexInjectLockOutcome→CodexInjectResult→CodexSyncResult), so the reason cannot disagree with the write.ocx restore backcommitted the toggle ON, got a gated skip, and told the operator to "retryafter the competing integration change finishes". There is no competing writer. It now names
the gate, in both the human and
--jsonpaths.ocx ensurestripped the managed Grok block as if Grok had been switched off. Only anexplicit
clientIntegrations.grok === falseauthorizes that strip now; a gated hub is told whynothing was written and
~/.grok/config.tomlis left exactly as it is.ocx sync,ocx sync-cacheand the three "startup left Codex native" lines say the same honestthing. An explicit OFF keeps its existing wording byte for byte.
The first commit is a separate hub defect found on the way:
ocx guiderived its URL from theproxy bind, so a tailnet-bound hub opened a browser at the tailnet origin while the hub's own
loopback management ingress sat unused.
Deliberately not done.
loopbackRouteAllowedis unchanged, so on a companion hubhttp://127.0.0.1:<port>/v1/messagesand/api/*still return 404 — a companion is a bind-addresschange, never an admission change (per the review note on #4236: do not add
/api/*to theunauthenticated listener). A test pins that. The consequence is recorded rather than hidden:
ocx claudeandfetchClaudeCodeStatereach a live socket on such a hub but not a wire thatserves them, and closing that needs the two destination contracts the reviewer described
(authenticated local management discovery vs. per-wire inference), which is its own change.
Only the English
reference/configuration/server.mdparagraph is corrected here — it claimed theport was required, which is now false. The remote-hub guide, the translated copies and
skills/ocxare PR4 of this stack.Stacked on the macOS launchd repair PR and targets its head branch; retarget to
devonce thatlands. Devlog:
devlog/_plan/260911_hub_single_port/020_loopback_companion.md.Verification
bun run typecheck— clean.bun run privacy:scan— passed (the startup log line changed).bun test tests/server/loopback-listener-admission.test.ts tests/server/loopback-companion-client-targets.test.ts tests/server/server-loopback-host-gate.test.ts— 44 pass.bun test tests/server/loopback-listener-integration.test.ts— 34 pass. A real companion startproves one port answers twice (200 with no credential on 127.0.0.1, 401 on the tailnet address),
that the startup line says "companion" rather than the ported warning, that
/api/*,/healthz,/andPOST /v1/messagesstill 404 on that socket, and that an impossible companion throwsbefore the public listener opens, leaving the port bindable. The same-port case needs a
non-loopback IPv4 interface to tell the two sockets apart and warns rather than passing silently
without one, matching the existing bind-scope case in that file.
bun test tests/cli/hub-gated-local-clients.test.ts tests/cli/cli-dispatch.test.ts— 50 pass.New file holds
restore backto the gate's sentence instead of the phantom-conflict text(human and
--json), holdsocx ensureto leaving a Grok block it did not own the decision todelete while still stripping on an explicit OFF, and pins the skip reason/message every other
caller prints.
bun test tests/providers/xai/grok-status.test.ts tests/providers/xai/grok-sync.test.ts tests/providers/xai/grok-lifecycle.test.ts tests/codex-integration/codex-desired-state.test.ts tests/codex-integration/codex-inject.test.ts tests/codex-integration/codex-sync-api.test.ts tests/cli/ensure-desired-integrations-race.test.ts— 149 pass.bun test tests/config/config-user-edits.test.ts tests/config/config-load-degrade.test.ts tests/cli/cli-json-contract.test.ts tests/cli/cli-restore-back.test.ts tests/clients/integrations-writer.test.ts tests/clients/sync-client-integrations.test.ts tests/server/startup-prompt.test.ts— 177 pass.bun test tests/cli/cli-transport-honesty.test.ts tests/cli/cli-status-json.test.ts tests/cli/cli-config-command.test.ts tests/cli/cli-start-journal-order.test.ts tests/cli/cli-capabilities.test.ts tests/cli/cli-help.test.ts— 108 pass.bun test tests/codex-integration/codex-write-lock.test.ts tests/codex-integration/codex-inject-write-lock.test.ts tests/codex-integration/codex-composed-acceptance.test.ts tests/codex-integration/codex-history-lock.test.ts tests/lab/lab-activation.test.ts tests/lab/core-lab-boundary.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts— 98 pass.core-lab-boundaryis the guard thatstartServerstays synchronous and that nothing awaits inits activation window.
bun test tests/update/update-stop-first.test.ts— 23 pass (the other source-oracle harness thatevaluates a CLI body).
scripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json. One pre-existing harness needed a line:tests/clients/sync-client-integrations.test.tstranspiles the realhandleEnsureand injectsevery free identifier, so the new startup-line helper had to join that map.
proof.
grep -rni loopback gui/srcfinds only unrelated copy, so this field is notrendered and
lint:guiwas not required.Checklist
Refs #4236 — this PR closes follow-ups 1 and 2 and defect 4. Defects 1 and 2 (the macOS launchd
repair outage) are the parent PR in this stack, so the issue stays open until that lands.