feat(connect): phase 3 — ocx connect client core, per-client keys, mode-aware sync - #2777
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. |
![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: aa26159532
ℹ️ 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".
| if (!options.noSync && options.selectedClients.includes("codex")) { | ||
| const injected = await injectCodexConfig(config.port, injectConfig, { | ||
| routingTarget: target, | ||
| catalogPath: DEFAULT_CATALOG_PATH, | ||
| journalOwner: { kind: "client", apiKeyId: issued.id }, |
There was a problem hiding this comment.
When Codex is already routed through the local OpenCodex proxy, as it normally is after ocx start, injectCodexConfig() reaches writeJournal(), which returns early for already-injected content and leaves the existing process-owned journal unchanged. The connection is nevertheless committed, so disconnectClient() later rejects the process-owned journal as conflicting; if final commit fails, rollback can also be partial and leave Codex pointing at the hub after the new token is removed. Before injection, either safely transfer the existing journal/preimage to the new client owner or refuse/reconcile this state before issuing a key.
Useful? React with 👍 / 👎.
| const bytes = new Uint8Array(await response.arrayBuffer()); | ||
| if (bytes.byteLength > maxBytes) { | ||
| throw new HubClientError("body_too_large", "Hub response exceeded the allowed size", response.status); |
There was a problem hiding this comment.
For a hub response without a trustworthy Content-Length—for example, a chunked or deliberately misreported /readyz, /api/keys, or /v1/catalog response—response.arrayBuffer() buffers the entire body before the size check runs. A misconfigured or compromised remote hub can therefore consume unbounded client memory despite these APIs advertising bounded bodies. Read the response stream incrementally, cancel it as soon as maxBytes is exceeded, and only then decode the accumulated bytes.
Useful? React with 👍 / 👎.
| ocx connect <url> Connect this machine to a remote OpenCodex hub (credential via stdin) | ||
| ocx disconnect Restore local state and clear the hub connection |
There was a problem hiding this comment.
The CLI now exposes a substantial user-facing connect/disconnect workflow, including stdin-only credentials, client selection, HTTP opt-ins, synchronization behavior, persisted client state, and the manual post-disconnect key-revocation requirement, but this commit adds no docs-site/ documentation. Add an English source page and keep translated locales consistent so users can safely configure and recover this feature rather than relying on the terse command help.
AGENTS.md reference: src/AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
리뷰 · 우선순위 70 / 80이 PR은 원격 허브 스택 3단계입니다. 베이스는 하는 일은 한 문장으로 말하면 Codex 쪽은 보안 설계는 대체로 단단합니다. 자격은 stdin만, 라인 src/cli/connect.ts --management-transport - CLI는 라인 src/lib/service-secrets.ts writeServiceApiTokenFile - 이미 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head aa26159532d4da2002f1d0aed897620788a1c6e0. It is 203 commits behind current dev@ae356a3cf, and exact-head CI has three concrete assertion failures: the GUI gate fails in gui/tests/api-auth-memory.test.ts:23, and tests/release-version-line.test.ts:108 fails on Linux shard 2 and macOS. This is a 34-file client/key/sync phase, so those failures plus the large integration drift make the current head unsafe to approve. Please rebuild it on current dev, preserve the current dashboard auth-memory boundary, fix the release-line contract rather than weakening it, and return with a fully green exact-head matrix.
7099760 to
b728285
Compare
aa26159 to
ad1ab25
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head ad1ab25d8f2e1e44ca8433152e26247dc7d0d20d.
This phase is not merely waiting on the parent stack: its client contract is still the pre-correction contract and is incompatible with the exact Phase-1/Phase-2 bases it targets.
- Phase 1 now serves
/v1/catalogunconditionally withCache-Control: no-store, no ETag, and no 304. Phase 3 still requires a fresh ETag during connect (src/client/connect.ts:207-210), persistscatalogEtag, sendsIf-None-Match, accepts 304 asnot-modified, and uses the ETag as local catalog ownership (src/client/hub-client.ts:280-300,src/client/connect.ts:303-319,343-350). Against the exact07d7f1006parent, every real connect reaches a valid 200 catalog with no ETag and then throwsinitial hub catalog did not include a fresh ETag. Replace this with unconditional bounded fetches and a locally computed catalog fingerprint for ownership/rollback. Treat any unsolicited 304 as a protocol error that preserves the LKG. Update the stale200/304tests accordingly. --allow-insecure-httpis still public CLI/config behavior (src/cli/connect.ts:25-27,122-145,src/client/connect.ts:50-58,src/client/hub-client.ts:169-184). The corrected Phase-2 server rejects non-loopback HTTP regardless of opt-in, so this option is both unsafe guidance and nonfunctional against the exact parent. Remove it completely; reject legacy argv rather than silently suggesting the operator can make reusable pairing safe over plaintext.- Exact-head CI is red on two concrete assertions. The GUI gate inherits the unresolved Phase-2
gui/tests/api-auth-memory.test.ts:23failure, and Linux shard 3 failstests/cli-transport-honesty.test.ts:107because the newsyncdispatch body matches the forbidden handler-result-discard shape. Keep the connected no-local-fallback behavior, but make the runner satisfy the existing exit-code honesty invariant instead of weakening the guard.
The parent chain #2771 → #2772 → #2776 is itself under CHANGES_REQUESTED, and #2776 has a credential-transport proof bypass. This PR targets the stacked codex/remote-hub-p2 branch, not an integration branch. Fix the contract against corrected parents, rebase the actual Phase-3 delta onto the resulting current dev, and require a fully green exact-head matrix before another review. The transactional rollback, owner-only token file, offline disconnect, and client-mode no-local-provider-fallback direction should be preserved.
660ddee to
38c3613
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 38c361362142f49f5cd1b8f800b076e188a034f7.
The old catalog-contract blocker remains unchanged. Phase 1 serves /v1/catalog with Cache-Control: no-store, no ETag, and no 304. This client still requires a fresh ETag during initial connect, persists catalogEtag, sends If-None-Match, accepts 304, and uses the ETag as local catalog ownership. Against the exact parent 07d7f1006, a valid 200 catalog has no ETag and connect throws initial hub catalog did not include a fresh ETag.
Remove the validator/304 contract from the client and bind local ownership to the already-computed catalog body fingerprint. Sync should accept every bounded 200 body, and disconnect should compare the stored fingerprint rather than an HTTP validator.
Two current-head boundaries also remain valid:
boundedText()callsresponse.arrayBuffer()before checking the size, so a chunked/misreported hub response can consume unbounded client memory. Read and cancel incrementally at the limit.- Treating every process-owned Codex journal as safe to unwind does not establish ownership transfer. A live process can still own and mutate that journal. Re-own/fence the exact preimage before issuing the remote key, or refuse the transition until the process journal is reconciled; cover final-commit rollback and disconnect.
Add the missing connect/operator documentation in this phase or before any phase exposing the CLI can land. Exact-head CI being green does not make the client compatible with its exact parent while these contracts remain.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Re-reviewing exact head c5420db.
The new prior-catalog restoration and profile-unlink completion checks are useful, but they do not resolve any of the three current blockers from the previous exact-head review.
- connectClient still rejects every valid Phase-1 catalog without an ETag, persists catalogEtag, and syncConnectedClient still sends the validator and accepts not-modified. The exact parent serves unconditional no-store 200 bodies without ETag or 304. Replace this protocol dependency with a local body fingerprint before extending the rollback payload.
- The bounded catalog reader still materializes response.arrayBuffer before enforcing its byte ceiling. Incrementally read and cancel at the limit so a chunked or dishonest response cannot allocate without bound.
- disconnectClient still explicitly treats every process-owned journal as safe to unwind. The new restore completeness check does not establish ownership transfer or fence a live process that can still mutate the same Codex state. Re-own/fence the exact journal preimage during connect, or refuse while a live process owns it, and cover connect final-commit rollback plus disconnect.
Keep the prior-catalog restoration direction, but bind its ownership to the local catalog fingerprint that replaces ETag. The parent stack remains blocked, so this phase is not a merge candidate on this head.
Connecting after `ocx start` is the ordinary path: routing is already injected and the Codex journal is owned by the proxy process. Ownership never transfers during connect, because writeJournal() refuses to overwrite a journal whose config is already injected — so the process owner survives into the connected state. disconnectClient() read any non-matching owner as a conflict and refused. That stranded the connection: the operator could not disconnect, and no action available to them would make the check pass. The artifacts were preserved, so nothing was lost, but the connected state had no exit. A process-owned journal records the pre-injection baseline this same tool wrote, so restoring it is exactly the right unwind. The genuine conflict is a journal owned by a DIFFERENT client key, where restoring would tear down another key's routing; that case still refuses, and its existing test still passes. Injected routing with no journal at all now gets its own message. Previously it fell into the ownership error, which named the wrong cause: there is no recorded baseline to restore, so unwinding would be guessing at the original config rather than reading it. The regression test drives the real shape — injected config plus a process-owned journal — and fails against the previous refusal.
…anch tests/cli-transport-honesty.test.ts flags any runner that awaits a handler and then returns a literal 0, because that erases a failure the handler recorded in process.exitCode. The exemption list requires a verified reason rather than a name, and the connected sync branch has none: handleConnectedSyncCatalogWrite drives app-server restarts, so a failure there must survive. Returns process.exitCode like every other runner. Node types it as number | string; only a numeric code is meaningful to the dispatcher.
…uck profile a clean restore Two rollback defects. Both let disconnect report that native Codex state was restored while leaving the user worse off than before they connected. Connect overwrites whatever catalog is already at DEFAULT_CATALOG_PATH. The pre-connect bytes were snapshotted only into an in-memory `priorCatalog`, which covers a connect that fails and rolls back in the same run — not a disconnect, which is a different process on a different day. Durable state recorded only the remote catalog's fingerprint, so disconnect deleted the remote catalog and left the user with none. That is the one artifact a rollback cannot reconstruct from anywhere else: the token can be reissued and the config is journaled, but a catalog the user brought with them is simply gone. The snapshot is now persisted on the connection as `priorCatalog` (base64, or "" for "there genuinely was none") and disconnect writes it back. An older connection with the field absent keeps the previous removal behavior, since nothing recorded what to restore. Ownership is still checked first — a catalog edited since connect belongs to the user and `changed` refuses rather than overwriting it. The result gains `catalogRestored` so the two outcomes are distinguishable instead of both reading as `catalogRemoved`. restoreJournalState set profileRestored = true after a swallowed unlink. When the original profile was absent, "delete the one we generated" failing meant the function still reported complete, which deletes the journal — the only record that the leftover profile is ours. The user is told native state was restored while our profile stays on disk with nothing left pointing at it. Now only a verified removal counts, with ENOENT treated as success because the file being already gone is the outcome the removal wanted. The catalog fix carries a runtime regression driven red against the previous behavior. The profile fix is asserted source-level, and the test says why: making unlink fail requires denying writes on the Codex home, which denies the atomic config write earlier in the same function, so the branch is unreachable from a test process. Asserting a fabricated runtime failure would prove less than asserting the shape.
2b36ad4 to
dad1712
Compare
c5420db to
20f3c11
Compare
2 participants
Summary
devlog/_plan/260827_remote_hub/050_phase3_connect.md).ocx connect <url>/ocx disconnect/ocx connect status/ocx connect revokeand the client runtime undersrc/client/(state,hub-client,connect)./readyzprotocol compatibility (p2/min1 accepted; explicit too-new/too-old errors) → one-time authority via--pairing-code-stdinor--admin-token-stdin(held as bytes, retained until commit/rollback, then released) → per-client key auto-issued through the hub key API → key written only through the service-secrets owner to the owner-only token file the shim already reads (neverconfig.toml) → catalog downloaded atomically → injector preflight → inject → client state persisted (includingapiKeyIdfor connected-only revoke). Any pre-commit failure leaves the machine untouched.ocx syncbecomes mode-aware with no local-provider fallback in client mode; the Claude launcher gains a hub target (launcher-scope env only); disconnect restores from the injector journal fully offline.Verification
Run on lidge-ai (remote CI host) at aa26159, per the no-local-suite policy:
bun run typecheck— clean.bun test tests/client-connect.test.ts tests/codex-inject.test.ts tests/codex-inject-integration.test.ts tests/service-secrets.test.ts tests/cli-headless-parity.test.ts tests/cli-start-journal-order.test.ts tests/core-lab-boundary.test.ts— 140 pass, 0 fail (P3-A1..A15: transaction rollback at every step, key-file permissions, zeroization, revoke-when-disconnected refusal, sync fallback refusal, standalone byte compatibility, offline disconnect).Checklist