feat(two-plane): phase 4 — machine listener, gui two-plane targets, usage slice - #2781
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: Team 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 |
github-actions
Bot
added
enhancement
|
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24841897f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| hubReachability: deps.machineApi?.hubReachability ?? (() => hubReachability), | ||
| setHubReachability: deps.machineApi?.setHubReachability ?? (value => { hubReachability = value; }), | ||
| }; | ||
| const relayEnabled = connection.managementTransport === "relay"; |
There was a problem hiding this comment.
When an operator selects the documented --management-transport relay option, connectClient still throws at src/client/connect.ts:186-188 before committing connection state. A repository-wide search shows that function is the only production writer of managementTransport, so this new relayEnabled branch is reachable only through hand-edited state or injected tests; remove the Phase 3 rejection and add an end-to-end relay connect case.
Useful? React with 👍 / 👎.
| const port = activePort; | ||
| try { activeServer?.stop(true); } catch { /* best effort */ } | ||
| cleanup(); | ||
| if (process.env.OCX_SERVICE !== "1" && port) { |
There was a problem hiding this comment.
When the client runtime was launched by Linux systemd or native Windows WinSW, OCX_SERVICE=1 makes this branch skip the detached standalone replacement and the timer later exits with status 0. The generated supervisors use Restart=on-failure in src/service.ts and <onfailure action="restart"> in src/lib/winsw.ts, so neither restarts a successful exit; disconnect therefore leaves the proxy stopped instead of transitioning to standalone. Exit with the supervisor's restart status or explicitly request a service restart, with focused coverage for these service modes.
Useful? React with 👍 / 👎.
| const outcome = await requestProxyStop(machineBase, { | ||
| formatFailure: status => t("dash.stopFailed", { status: String(status) }), | ||
| mode: targets.connected ? "client" : "standalone", |
There was a problem hiding this comment.
When connected disconnect returns its expected 202, handleStop handles only the rejection branch: targets.connected remains true and stopping remains set while the replacement standalone server starts. The still-open SPA consequently keeps sending shared calls to the old hub/relay target and offers another machine disconnect against a server that no longer has that route until the user manually reloads; on an accepted client-mode outcome, reload the page or rediscover and reconfigure both targets after the listener returns.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| {targets.connected && !sharedSessionReady && ( | ||
| <ConnectPairingForm target={targets.shared} onConnected={() => setSharedSessionReady(true)} /> | ||
| )} | ||
| {page === "dashboard" && <Dashboard apiBase={sharedBase} />} |
There was a problem hiding this comment.
On the normal connected-without-hub-session path, the pairing form and the selected shared-plane page mount together. The page immediately fetches a hub /api/* route, whose 401 enters resolveTokenAfter401 and opens the raw admin-token modal, so the user must dismiss an unrelated credential prompt before the pairing-code form is usable; render shared pages only after sharedSessionReady, while leaving any explicitly machine-only controls available.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
리뷰 · 우선순위 62 / 80이 PR은 remote-hub 스택 phase 4입니다. Draft이고 베이스는 새 파일은 선행 #2772·#2776·#2777이 먼저입니다. Draft라 CI 빨강도 예상됩니다. types/config 분할과는 직접 관련 적지만, GUI·클라 면적이 커서 리뷰 부담이 큽니다. preview deploy는 계획에 없습니다. 라인 src/client/hub-relay.ts - path traversal·redirect·origin 불일치 거부. body 상한 4MiB/16MiB. 라인 src/client/machine-listener.ts - loopback-only allowlist. /v1 차단이 핵심입니다. 라인 gui/src/api-targets.ts / api.ts - per-target 세션/CSRF. Startup/Integrations는 shared plane 유지. 라인 gui/src/pages/Usage.tsx - hub vs local usage 전환. 미러링 없음. 경로 draft - 머지 대상 아님. p5(#2786)가 이 위에 있습니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Holding this stack phase, not rejecting the two-plane direction. Exact head 44f9973a2d500ad28acfe6ec697e563b058744e7 is Draft and targets codex/remote-hub-p3, not an integration branch allowed by the repository policy. Phases 1-3 are still open, so the current 48-file diff and CI do not represent the final landing delta on dev. After the predecessor phases are resolved, retarget/rebase the actual Phase 4 delta onto current dev, complete readiness, and obtain exact-head Cross-platform CI plus React Doctor. Re-request security review then; I will review the loopback machine listener, fixed-target relay SSRF/redirect/header boundaries, per-target session/CSRF isolation, and usage-slice authorization on the final integrated head. Do not merge this intermediate stack head.
aa26159 to
ad1ab25
Compare
95787b9 to
edd0a81
Compare
github-actions
Bot
removed
the
intake: hygiene-blocked
660ddee to
38c3613
Compare
edd0a81 to
158424f
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 158424f0538eeb02f56630806e88556b070731a7.
The supervised disconnect recycle is fixed on this revision: service mode now exits with the failure status expected by systemd/WinSW and no longer leaves the client stopped. Three current product blockers remain:
-
The documented relay path is unreachable. Phase 3
connectClient()still throws whenevermanagementTransport === "relay", and that function is the production writer of the persisted transport. The Phase-4relayEnabledlistener branch can therefore be reached only by hand-edited state/tests. Remove the rejection at the stage that first supports relay and add a full connect-through-relay case. -
After a successful 202 disconnect, the SPA keeps the old connected targets and stopping state while the standalone replacement comes up. Rediscover/reconfigure both API targets or reload only after the accepted transition, so the page does not continue calling the dead hub/relay route.
-
The connected-without-session view mounts shared hub pages alongside the pairing form. Their immediate 401 opens the raw admin-token prompt before pairing can complete. Gate shared pages on
sharedSessionReady; leave only machine-local controls available before pairing.
The macOS job is red, this phase inherits the unresolved Phase-2/3 blockers, and it should not receive approval or sponsorship on the current stack head.
158424f to
4aad8ab
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 4aad8abbf21652ddd5b910d41209b10baa8dd93e.
The relay-connect blocker is fixed on this revision: connectClient() no longer rejects managementTransport: "relay", and the added relay path is now reachable. Two GUI state blockers from the previous review remain unchanged:
gui/src/App.tsx:220-234handles only!outcome.accepted. After the expected 202 disconnect,stoppingstays true andtargetscontinues to describe the old connected hub/relay while the standalone replacement starts. Rediscover and reconfigure both targets after the machine listener returns, or perform a deliberate reload after the accepted transition. Add a focused accepted-disconnect regression.gui/src/App.tsx:383-395mountsConnectPairingFormand every shared-plane page at the same time whilesharedSessionReadyis false. Those pages can issue a shared/api/*request, receive 401, and open the raw admin-token fallback before the pairing form is usable. Gate shared-plane pages onsharedSessionReady; keep only explicitly machine-local controls available before pairing, and assert that no shared request occurs before the pairing exchange succeeds.
Exact-head macos and aggregate ci are also red. This phase remains blocked by those two product paths and the predecessor stack.
…he D1/D2 client side Five defects on this phase, plus the client half of two contract changes the earlier phases made on the server side. /api/machine/* is declared. The seven routes this phase adds were absent from the headless parity table, so tests/cli-headless-parity.test.ts failed on undeclared endpoints. They are not undocumented: status/clients mirror `ocx connect status`, sync mirrors `ocx sync`, shim mirrors the client integration commands, and disconnect mirrors `ocx disconnect`. hub-relay is the transport those commands select with --management-transport relay rather than a verb of its own. Declared as one prefix with that mapping written down. Relay actually works now. connectClient() threw "relay management transport is not available before Remote Hub Phase 4" — but this IS phase 4, and the machine listener plus hub-relay both land here. A documented option that always threw was worse than an undocumented one. A supervised client comes back after disconnect. scheduleStandaloneRecycle() skipped its own respawn when OCX_SERVICE=1, correctly leaving the restart to the supervisor, then exited 0. The real supervisor configs are failure-only (systemd Restart=on-failure, WinSW onfailure, the Task Scheduler ERRORLEVEL loop), so a clean exit reads as "finished" and nothing restarts — the client stayed down until someone noticed. Now exits 1 under supervision, the same policy the dashboard recycle already uses. launchd KeepAlive was fine either way. D1 client side: --allow-insecure-http is removed from the CLI, the connect options, and the hub client. The hub refuses plaintext pairing outright now, so keeping the flag would only spend a single-use grant against a certain rejection. The client checks the same rule locally and refuses before sending. D2 client side: the catalog fetch is unconditional. /v1/catalog emits no validator, so If-None-Match had nothing to match and connect's "initial hub catalog did not include a fresh ETag" check would have failed every connection. The stored catalogEtag becomes catalogFingerprint — our own hash of the bytes we wrote. That value was never a cache concern: it answers "is the file on disk still ours" before disconnect removes it, which needs no server participation. usage/summary.ts keeps dev's per-attribution filtering and this phase's per-key entry slice; the comment now says which filter operates at which level, because they are deliberately different.
… its test The relayed pairing request went out unauthenticated. submitConnectPairing took `fetchImpl: typeof fetch = fetch`, and a default parameter binds the global as it was when the module was evaluated — the unwrapped original, not the wrapper installApiAuthFetch puts on window.fetch. The relay needs the machine-session headers that wrapper attaches, so the hub refused the exchange. Resolved at call time now. The transport also moves to its own module. One file exported both a transport function and a component, which react-refresh/only-export-components flags for good reason; the previous shape carried an eslint-disable instead. The two have no reason to share a file: the transport is testable without React and the form has no logic beyond calling it. tests/connect-pairing.test.ts passed alone and failed in the full GUI run. App calls installApiAuthFetch() at module scope, so it runs on first import only; a later test importing App gets the cached module and no install, leaving the wrapper bound to whichever window imported it first. The test now binds the wrapper to its own window before mounting, and claude-toggle-race.test.tsx clears the install latch in afterEach alongside the window it closes. Both are test isolation rather than product behavior.
… machine plane Three things a user who never enabled remote hub was paying for. Every dashboard load fired GET /api/machine/status. Discovery ran unconditionally and inferred standalone FROM the resulting 404, so the browser announced the feature's existence on every paint of a plain install. The server already injects session meta into the served document, so it now states the runtime role there too and the client reads it instead of asking. A missing tag reads as standalone, which covers an older server, a separately hosted GUI, and the Vite dev server — all of which should make no remote-hub request. The role meta is emitted independently of the session block. A standalone install never issues a GUI session, so tying the role to session issuance would have left exactly the case that needs it with nothing to read. The page body was gated behind targetsSettled, so a standalone user saw "Discovering local and shared targets…" before their own dashboard. Standalone now starts settled: there is nothing to discover, so there is nothing to wait for. A failed discovery replaced the entire body with a machine-plane error. A slow or restarting proxy cost a standalone user their dashboard over a plane they never turned on. It is a banner now; the requests that actually need the machine plane still report their own failures. Regressions are driven red against the previous behavior: standalone discovery makes zero fetches across null/standalone/hub roles, and a client role still discovers, so the tag narrows who asks rather than removing discovery.
c5420db to
20f3c11
Compare
4aad8ab to
95639f0
Compare
2 participants
Summary
devlog/_plan/260827_remote_hub/060_phase4_two_plane.md).src/client/{runtime,machine-listener,machine-api,machine-auth,hub-relay}.ts) serving/api/machine/{status,clients,sync,shim,disconnect}plus a fixed-targetPOST /api/machine/hub-relay/*relay (destination pinned to the connected hub, path allowlist, redirects rejected, hop-by-hop stripped, body caps, browserOriginforwarded verbatim on the pairing exchange, nothing logged). No/v1/*is reachable on this listener.gui/src/api-targets.tsdiscovers shared/machine call bases;gui/src/api.tskeeps per-target in-memory session/CSRF state; existing Startup/Integrations/ApiKeys/Grok calls stay on the shared plane per call site.gui/src/connect-pairing.ts), mounted fromApp.tsxin the connected-without-hub-session state, with a rendered-form test.apiKeyId(toggle to hub-wide), disconnected → localusage.jsonl. No mirroring.Screenshot (dashboard served from this branch's gui build):
Verification
Run on lidge-ai (remote CI host) at 2484189, per the no-local-suite policy:
bun run typecheck— clean.bun test tests/client-machine-listener.test.ts tests/client-hub-relay.test.ts tests/client-connect.test.ts tests/api-usage.test.ts tests/usage-summary.test.ts tests/cli-start-journal-order.test.ts tests/core-lab-boundary.test.ts— 118 pass, 0 fail.cd gui && bun run test— 1097 pass, 0 fail;bun run build— clean.Checklist