feat(deploy): phase 5 — hub management ingress, deployment recipes, oracle dogfood fixes - #2786
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 |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 64 / 80이 PR은 remote-hub phase 5입니다. 베이스는
Dogfood 수정 세 가지가 실사용 가치가 큽니다. (1) 지금 라인 src/server/index.ts managementIngress - 127.0.0.1 전용, allowlist 404, 바인드 실패 롤백, stop에 세 리스너. 라인 src/types/config.ts / src/config.ts - hub.managementIngress 스키마. enabled:false | {enabled:true,port}. 라인 src/cli/claude-agent-startup-sync.ts / desired-state - hub 역할은 호스트 클라이언트 설정을 다시 쓰지 않습니다. 라인 src/client/state.ts - hub에 client state가 없으면 mismatch가 아니라 disconnected. 라인 tests/loopback-listener-*.ts / server-management-auth.test.ts - ingress 경계·실소켓 종료를 잠급니다. 경로 base - #2781(draft) 위. p4 ready 전이면 이 PR도 대기입니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: a62c8eba20
ℹ️ 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".
| // A hub is a server role, not a broken client: without client state it simply is not | ||
| // connected, and refusing here blocked `ocx start` on every hub (found on the first | ||
| // clisu-oracle dogfood boot). Hub role WITH client state remains mismatched below. | ||
| if (!hasClient && role === "hub") return { kind: "disconnected" }; |
There was a problem hiding this comment.
When a hub has no client block, classifying it as disconnected lets connectClient() pass its state.kind === "disconnected" preflight and later makes commitClientConnection() replace runtimeRole: "hub" with "client". Thus, on a hub without a service-token file, running ocx connect silently converts the installation into a client and disables hub behavior on restart. Preserve the hub/client mismatch for connection operations and special-case hub startup in handleStart() instead, or add an explicit hub-role rejection to the connect preflight.
Useful? React with 👍 / 👎.
| const seeded = getDefaultConfig(); | ||
| seeded.runtimeRole = "client"; | ||
| seeded.client = structuredClone(state); | ||
| saveConfig(seeded); |
There was a problem hiding this comment.
When mutatePersistedConfig() reports missing, it returns before acquiring the mutation lock; another first-run command can therefore create a valid config.json before this unconditional whole-config saveConfig(seeded) runs. The seeded defaults then overwrite the newly created providers, credentials, and settings. Bootstrap creation needs to acquire the coordinator lock and confirm the file is still absent before writing, otherwise it should retry the field-scoped mutation or report a conflict.
Useful? React with 👍 / 👎.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head a62c8eba20ec1ecc7531f2ce8eb2310004052dde. It is 203 commits behind current dev@ae356a3cf, and exact-head CI is failing on product contracts, not only infrastructure noise: tests/cli-headless-parity.test.ts:287 exposes seven undeclared /api/machine/* routes; tests/update-stop-first.test.ts:225, tests/loopback-listener-admission.test.ts:196, and tests/release-version-line.test.ts:108 also fail on macOS, while the privacy gate fails. Please rebuild the deployment phase on current dev, explicitly declare or remove every machine management route from the CLI/headless surface, restore the stop-first and role-admission contracts, and keep the privacy/release gates intact. Re-request review only on a clean exact head with green CI.
44f9973 to
95787b9
Compare
a62c8eb to
8bcfcaa
Compare
95787b9 to
edd0a81
Compare
8bcfcaa to
9557186
Compare
edd0a81 to
158424f
Compare
9557186 to
ff3ce26
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head ff3ce26bd764d969169b7100334695ed9647e66d.
Two current-head state transitions are unsafe:
-
readClientConnectionState()mapsruntimeRole: "hub"with no client block to ordinarydisconnected. That was added to let a hub start, butconnectClient()also accepts every disconnected state andcommitClientConnection()then rewrites the installation asruntimeRole: "client". Runningocx connecton a hub can silently disable hub behavior on restart. Keep hub as a distinct non-client state for connection operations, and special-case hub startup at the startup call site, or explicitly reject hub role in connect preflight. -
The missing-config bootstrap performs
saveConfig(getDefaultConfig()+client)aftermutatePersistedConfig()reports missing. That missing result occurs before the mutation lock. Another first-run writer can create a real config in the gap, after which the unconditional whole-config save overwrites its providers/settings. Acquire the coordinator lock and recheck absence before creation; if the file appeared, retry the field-scoped mutation or return a conflict.
Add concurrent bootstrap and hub-role connect regressions. This phase also inherits the blocked parents and its exact-head macOS/aggregate CI is red, so it is not an approval candidate.
158424f to
4aad8ab
Compare
ff3ce26 to
072cc29
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 072cc29c3420a286c3be41fc923370ee2038a35d.
The restack is clean and exact-head CI is green, but both Phase-5 state-transition blockers remain in the production code:
src/client/state.ts:53-57still classifiesruntimeRole: "hub"with no client block as ordinarydisconnected.connectClient()accepts that state andcommitClientConnection()then replaces the role withclient, so runningocx connecton a hub can silently disable the hub role on restart. Preserve a distinct hub/non-client result for connect operations or reject hub role in connect preflight; special-case hub startup at its actual startup call site.src/client/state.ts:74-86still handles a missing config by calling whole-configsaveConfig(defaults + client)aftermutatePersistedConfig()returned before taking the mutation lock. A concurrent first-run writer can create a real config in that gap and then have its providers/settings overwritten by defaults. Acquire the coordinator lock and recheck absence before creation, or retry the field-scoped mutation when the file appeared.
Add deterministic hub-role connect and concurrent bootstrap regressions. Green CI cannot prove paths the tests do not cover, and the parent chain is still under changes requested.
…ed (first oracle dogfood boot)
…t (oracle dogfood)
…ing the commit (dogfood)
…lly reachable tests/loopback-listener-admission.test.ts asserted that every non-hub role is refused with "requires runtimeRole hub", looping over undefined, standalone, and client. The client case is refused earlier, by the separate rule that a client role needs a complete client connection block, so the assertion was testing an ordering that two independent validation rules never promised. Split rather than loosened. undefined and standalone still assert the exact ingress message. client asserts only that it is refused, because that is the guarantee the config actually makes for a role that is incomplete on its own. A second case closes the gap this would otherwise open: with a COMPLETE client connection, so the earlier rule no longer fires, the ingress rule is what refuses. Without it, the weaker client assertion could pass even if the ingress rule stopped applying to that role entirely. Also keeps both sides of two rebase conflicts. This phase inserts a management ingress pairing-exchange test ahead of the plaintext-pairing test that phase 2 rewrote, and structure/01_runtime.md gained a codex-cli-update sentence on dev and a hub-management-listener clause here; both rows carry both.
4aad8ab to
95639f0
Compare
072cc29 to
76257cc
Compare
2 participants
Summary
devlog/_plan/260827_remote_hub/070_phase5_deploy.md) plus the live clisu-oracle dogfood it prescribed.hub.managementIngress): a second 127.0.0.1-only listener serving GUI +/api+ the pairing exchange, default-404 allowlist, no/v1/*, startup rollback when the bind fails, three-listener shutdown covered by real-socket tests. Defaults byte-identical when unconfigured.OCX_API_TOKEN_FILEsecret, tailscale serve + ts.net cert walkthrough, headless OAuth viaoauthOpenBrowser:false+ manual code submit).runtimeRole=hubrefusedocx start; the hub role ran local Codex/Grok/Claude client syncs on startup (marking/readyzfailed and rewriting~/.grok/config.tomlon a server box);ocx connectrefused its final commit on a fresh machine with noconfig.json.devlog/_plan/260827_remote_hub/090_dogfood_record.md— pairing grant → per-client key auto-issue → injected client → real routedHUB_OKcompletion over the tailnet → per-apiKeyIdusage attribution on the hub → byte-identical disconnect restore.Verification
Run on lidge-ai (remote CI host) at f98081f, per the no-local-suite policy:
bun run typecheck— clean.bun test tests/server-live.test.ts tests/server-management-auth.test.ts tests/client-machine-listener.test.ts tests/client-connect.test.ts tests/codex-desired-state.test.ts tests/loopback-listener-integration.test.ts tests/core-lab-boundary.test.ts tests/service-install.test.ts— 156 pass, 0 fail./readyzwith protocol metadata over the tailnet, authenticated/v1/catalog, HTTPS-only admin refusal, pairing-code connect, routed completion, usage slice, offline-restore disconnect.Checklist