Repository navigation
Conversation
… UX across 11 locales
|
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
 |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
the
intake: hygiene-blocked
|
✅ 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: c8adfb2224
ℹ️ 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".
…ilize child.lock inode
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
github-actions
Bot
removed
the
intake: hygiene-blocked
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed at 75dc0635 against its stacked base feat/ocx-child-ephemeral-link @ c4d4d5d6. This review covers only this layer's diff. Focus was correctness of planting, rollback and purge, not a security scan. I did not review the GUI/i18n changes for UX, since that decision is the owner's.
Checked and looks right:
- POSIX plant:
umask 077, symlink refusal on every managed path component, a temp file in the 0700 dir, exact byte-count and (when a tool is available) SHA-256 checks beforemv -f, then--versionverification with removal on failure. Expected size and SHA travel as positional args, so the script text is never interpolated. - Windows plant:
CreateNewtemp, streaming SHA-256 with a size cap, reparse-point refusal on each component. - Apply rollback:
failed()now also runs for theremote_port/child_preflightearly returns. A fresh plant is purged throughdetach --purge, whose non-zero exit now preserves the binary and surfacesresidual(bot thread fixed in71352d39).child_port_busy/child_external_provider_conflictare raised before any Child file is written, soremoteMayBeConnected = falseis correct there. purge_managed_executable: only an exact-name file whose canonical parent equals the canonical managed dir qualifies, with symlink and reparse checks on the exe, its parent and each managed component. Foreign paths returnSkippedUnmanagedPath(covered by the new Rust test).probe/confirm-host/apply:allowMissingandplantare strictly typed.applyrefuses amissinghost withoutplant: true, andplantis ignored forfull/ephemeralhosts.- CI on this head: the required shards are green, and Windows/macOS lanes were not selected.
Inline notes below. None is marked blocking. The most user-visible one is the docs claim, because packaged installs have no artifact to plant.
Non-blocking:
childKindjoins the strictRECORD_KEYSset (src/link/store.ts). An older Home that readslinks.jsonafter a downgrade will reject the whole store. This is the same trade-offremotePlatformmade in #7832, so it only needs a release-note mention.resolveChildBinarySource: withOPENCODEX_HOME=""(set but empty),env.OPENCODEX_HOME?.trim() ?? …produces"", and the cache lookup becomes cwd-relative. The Rust side treats empty as unset, so|| join(...)would match it.
I did not approve because remote binary planting and execution need the explicit security review described in MAINTAINERS.md.
| ### Ephemeral Child machines (`ocx-child`) | ||
|
|
||
| For disposable SSH devboxes, containers and lightweight cloud VMs where installing the full Bun OpenCodex runtime is unnecessary, place the standalone Rust `ocx-child` binary (`cargo build --release --locked --manifest-path native/ocx-child/Cargo.toml`) in `~/.local/bin/ocx-child`, `~/.cache/opencodex/bin/ocx-child` (or `%LOCALAPPDATA%\opencodex\bin\ocx-child.exe` on Windows) or on `PATH`. Before enrollment, Home probes every full `ocx` candidate first and falls back to `ocx-child` when no runnable full `ocx` is present; once `$OPENCODEX_HOME/child/state.json` exists, remote commands keep targeting `ocx-child` until it detaches. | ||
| For disposable SSH devboxes, containers and lightweight cloud VMs where installing the full Bun OpenCodex runtime is unnecessary, Home can stream a SHA-256-verified standalone `ocx-child` binary (~0.9 MB, zero Node/Bun required) directly over SSH `stdin` into `~/.cache/opencodex/bin/ocx-child` (`%LOCALAPPDATA%\opencodex\bin\ocx-child.exe` on Windows) when you choose **Plant lightweight agent & connect** in `#remote` or run `ocx link add <alias> --plant --expect-fingerprint SHA256:…` (interactive TTY `ocx link add <alias>` prompts once before planting; air-gapped Children with zero outbound internet work out of the box). You can also pre-place `ocx-child` in `~/.local/bin/ocx-child`, `~/.cache/opencodex/bin/ocx-child`, or `PATH`. Before enrollment, Home probes every full `ocx` candidate first and falls back to `ocx-child` when no runnable full `ocx` is present; once `$OPENCODEX_HOME/child/state.json` exists, remote commands keep targeting `ocx-child` until it detaches. |
There was a problem hiding this comment.
As written, this describes behavior that a packaged (npm/desktop) Home cannot deliver yet. resolveChildBinarySource only finds an artifact in four places:
OPENCODEX_CHILD_BIN_DIRnative-bin/sidecars, which nothing in this stack produces or adds topackage.jsonfiles- a host-triple local
cargo build, which only exists in a dev checkout $OPENCODEX_HOME/cache/ocx-child/<version>/withSHA256SUMS, which nothing populates
The devlog says release-pipeline changes were deliberately left out. For a normal install, the probe will return plantable: false / artifact_not_found, and the bare-host flow dead-ends. That contradicts "air-gapped Children … work out of the box" here and the prerequisite bullet at line 12.
This paragraph also dropped the only instruction for obtaining the binary (cargo build --release …). Suggest stating the artifact requirement explicitly: where to place a binary plus SHA256SUMS, and that cross-OS/arch planting needs a binary built for the target. Keep that until release artifacts ship.
| "elif command -v shasum >/dev/null 2>&1; then", | ||
| " actual_sha=$(shasum -a 256 \"$tmp\" | awk '{print tolower($1)}')", | ||
| " if [ \"$actual_sha\" != \"$expected_sha\" ]; then printf 'Planted binary SHA-256 mismatch\\n' >&2; exit 1; fi", | ||
| "fi", |
There was a problem hiding this comment.
When neither sha256sum nor shasum exists on the Child (some minimal or distroless images), the SHA-256 check is skipped silently and only the byte count is verified before the binary is executed. SSH already protects the transport, so this is low severity. Still, the PR text and docs say "SHA-256-verified". Either fail closed when no hash tool exists, or print a stderr note so the result is not presented as hash-verified.
| const runner = runnerFor(ctx); | ||
| const platform = record.remotePlatform ?? await requireRemotePlatform(runner, record.alias, knownHostsFile(ctx)); | ||
| const result = await runner.run(buildExecArgv({ alias: record.alias, argv: remoteOcxArgv(["disconnect"], platform), knownHostsFile: knownHostsFile(ctx) }), { timeoutMs: 30_000 }); | ||
| const disconnectArgs = purge && record.childKind === "ephemeral" ? ["disconnect", "--purge"] : ["disconnect"]; |
There was a problem hiding this comment.
disconnect --purge goes through the generic candidate resolver. When child/state.json exists, ~/.local/bin/ocx-child is tried before the managed ~/.cache/opencodex/bin/ocx-child. If a user also has a copy in ~/.local/bin, that copy runs. is_managed_plant_executable returns false, the command exits 0 with purged:false, and Home reports success while the planted binary stays. The route does not pass --json, so it cannot detect this.
Purge is also silently dropped when record.childKind is absent, which is the case for any ephemeral link enrolled by the base layer before childKind existed.
Consider using remotePurgePlantedChildArgv for the purge case (it targets the managed path explicitly and already handles detach + removal). Alternatively, request --json and fail or report when purged is false.
| let child_dir = opencodex_home.join("child"); | ||
| Self { | ||
| lock_file: child_dir.join("lifecycle.lock"), | ||
| lock_file: opencodex_home.join("child.lock"), |
There was a problem hiding this comment.
Moving the lock to $OPENCODEX_HOME/child.lock (the correct fix for the inode race) has a side effect. FileLock::acquire_inner calls ensure_private_dir(parent), which now hardens $OPENCODEX_HOME itself, not just child/:
- on POSIX it runs
chmod 0700 - on Windows it calls
SetNamedSecurityInfoWwith a protected owner-only DACL plus inheritable ACEs, which also propagates to existing children
This runs on every attach/sync/detach/restart, and before assert_no_full_ocx_conflict. On a machine that also has a full ocx install under the same home, the child binary now rewrites the ACL of the full install's directory tree. Consider creating the lock file without hardening its parent ($OPENCODEX_HOME is not owned by ocx-child), or keeping it in a dedicated subdirectory that is never removed.
Summary
Stack (merge bottom-up):
feat/ocx-child-plant-ux) ← you are hereocx-childplanting,--purgeremoval, conflict diagnostics, CLI flags, & 11-locale#remoteUXstdinplant & rollback, managed-path--purge,#remoteUI & i18nfeat/ocx-child-ephemeral-link)native/ocx-childruntime & Home discovery/relay baseocx-childwire/journal conformance & base enrollmentDepends on #8237 (
feat/ocx-child-ephemeral-linkatc4d4d5d6311905bea62f065d7d04b9de7cdb5a86). Review this PR's diff only.Connecting a disposable SSH devbox, container, or cloud VM through
#remoteorocx link addpreviously required pre-installingocxor manually copyingocx-childonto the Child beforehand (502 remote_ocx_missing), offered no one-click or--purgeoption to remove a planted binary on disconnect, and surfaced generic preflight errors when port10100or~/.codex/config.tomlwas already owned externally.This layer completes end-to-end lightweight Ephemeral Child onboarding and lifecycle UX:
stdinplanting:POST /api/link/probeandPOST /api/link/confirm-host(allowMissing: true) inspect remotetargetTriple(darwin-arm64,darwin-x64,linux-x64,linux-arm64,windows-x64) and classifyruntimeKind(full,ephemeral, ormissing).src/link/child-binary-source.tsverifies non-symlinkocx-childartifacts (OPENCODEX_CHILD_BIN_DIR, sidecar directories, host-matching localnative/ocx-childrelease build, orSHA256SUMS-verified$OPENCODEX_HOME/cache/ocx-child/<version>/). Whenplant: trueis authorized onPOST /api/link/apply, Home streams the verified binary over SSHstdininto~/.cache/opencodex/bin/ocx-child(0700atomic write on POSIX) or%LOCALAPPDATA%\opencodex\bin\ocx-child.exe(insideOcxJob<= 8000encoded chars on Windows, refusing symlinks and reparse points) — working even on air-gapped Children — and automatically purges a newly planted binary if enrollment fails.fast detachvs--purge) & durablechildKind:native/ocx-child(detach/disconnect --purge),DELETE /api/link/:id({ purge: true }), andocx link revoke --link-id <id> --purgedelete the binary after restoring~/.codexand stoppingocx-child serveonly when the executable resides inside the managed plant path (refusing symlinks, ancestor symlinks/reparse points, and foreign/system paths; using rename-aside + detachedcmd.execleanup on Windows), while default disconnect keeps the cached binary for instant re-attach.LinkRecordpersistschildKind("full" | "ephemeral") and projects it onGET /api/link/status(alongsidegui-sessionhomeCodexSettingsDigest).childPortand externalmodel_provider/ useropenai_base_url/model_catalog_jsonownership in~/.codex/config.tomlmap deterministically to409 child_port_busyand409 child_external_provider_conflictwhile leaving Child files untouched.ocx link add <alias> [--plant]prompts once on interactive TTY for bare hosts and requires explicit--plantalongside--expect-fingerprintin non-TTY automation.#remoteDashboard UX across all 11 locales: Modularized#remote(RemoteLink.tsx,RemoteLinkSheet.tsx,RemoteLinkDialogs.tsx) and updated all 11 locale catalogs (en,ko,ja,zh,zh-TW,de,fr,pt,ru,tr,vi) with the Ready to plant lightweight agent confirmation card, Lightweight / Full install and Home settings synced / Sync needed badges on connected Child rows, specific conflict notices, and the Disconnect--purgecheckbox +~/.cache/opencodex/bin/ocx-child detachoffline recovery command.UI Screenshots (
origin/pr-assets@4d57c948b78bc66f2fb06b01cd9eb947766f1a60)1. Add Child — Ready to Plant Lightweight Agent (
runtimeKind: "missing",plantable: true)2. Connected Child Computers — Mode (
Lightweight/Full install) & Home Settings Sync Parity Badges3. Ephemeral Child Disconnect — Zero-Daemon Fast Detach & Optional
--purgeBinary DeletionVerification
cargo test --locked --manifest-path native/ocx-child/Cargo.toml— 9 passed (7unit/conformance +2full relay/lifecycle/--purge/symlink/conflict integration tests), 0 failed.cargo build --release --locked --manifest-path native/ocx-child/Cargo.toml— exit 0 (930 KBoptimizedocx-childbinary).bun run typecheck— exit 0 (2,321files checked, 0 errors).cd gui && bun run lint:i18n && bun run lint && bun test tests && bun run build+ rootbun run build:gui— exit 0 (3,738GUI tests passed across all 371 files including 11-locale parity &remote-link.test.tsx).bun test tests/clients/link-child-plant-foundations.test.ts tests/clients/link-ocx-child-e2e.test.ts tests/server/link-enrollment-ephemeral-child.test.ts tests/server/link-enrollment.test.ts tests/server/link-management-routes.test.ts tests/cli/cli-link-enrollment.test.ts tests/cli/cli-link-force.test.ts tests/cli/cli-link.test.ts— all passed (including real-binary bare-host SSHstdinplant +DELETE --purgeE2E).bun run test:changed—35,729passed,121skipped,0failed (306,106expect()calls across1,553import-connected files; scoped totest:changed+ explicit Rust/GUI/SSOT/hygiene/ratchet suites perAGENTS.mdto avoid redundant full-suite contention across concurrent worktrees while leaving unmodified unrelated domains to batch CI).bun run structure:check,bun run skill:surface:check,cd docs-site && bun run build(641pages,93,153internal links),bun run privacy:scan, andbun test tests/ci-workflows/repo-hygiene.test.ts tests/ci-workflows/file-size-ratchet.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts— all exit 0.Checklist