feat(codex): add encrypted native main profiles - #863
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (39)
📝 WalkthroughWalkthroughChangesNative Codex profile management
Audit and keyring safeguards
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CLI
participant ManagementAPI
participant NativeProfileManager
participant CodexAuth
participant StartupGate
CLI->>ManagementAPI: register, switch, or recover profile
ManagementAPI->>NativeProfileManager: validate and execute operation
NativeProfileManager->>CodexAuth: replace and verify native auth envelope
NativeProfileManager->>StartupGate: publish transition or complete recovery
StartupGate->>CodexAuth: allow native main traffic after recovery
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Maintainer security review on head Must change before merge (priority order):
Items 1-3 are the true blockers. If you are short on cycles, say so — a maintainer will take over on top of your branch. This is genuinely close to the capability we want to ship. |
9a2c1fa to
fc1f6ab
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc1f6ab7a7
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 218-236: The non-Linux branch of the “OS keyring
create/read/delete smoke” workflow runs directly against persistent self-hosted
keyring state. Update this step to isolate the self-hosted Windows leg from the
real Credential Manager, or add reliable pre- and post-job cleanup that removes
all lingering opencodex.keyring-smoke.* entries, including when the smoke test
is interrupted.
In `@docs-site/src/content/docs/reference/cli/providers-accounts.md`:
- Around line 213-232: Synchronize the `ocx account main` documentation across
the English, Japanese, Korean, Russian, and Simplified Chinese
`providers-accounts.md` pages. Update the English command block to use `<label>`
for profile creation, `<profile-id-or-label>` for `switch`, retain `switch
--yes`, and document every supported `--json` flag according to
`src/cli/account-main.ts` and its `resolveTarget` behavior; add the
corresponding section to the three non-English locale pages.
In `@scripts/keyring-smoke.ts`:
- Around line 34-51: The nested cleanup in runKeyringSmoke must not mask a
pending readback-mismatch error: in scripts/keyring-smoke.ts lines 34-51, log
deleteCredential failure instead of re-throwing it. Add a regression case in
tests/keyring-smoke.test.ts lines 44-56 where getSecret mismatches and
deleteCredential fails, asserting runKeyringSmoke rejects with the original
“readback did not match” message.
In `@src/codex/account-lifecycle.ts`:
- Around line 43-60: Extract the repeated main-account cleanup sequence from
reconcileMainCodexAccountRuntimeState and
applyConfirmedMainCodexAccountTransition into a shared helper. Have that helper
call purgeCodexAccountRuntimeState, setMainAccountPlan(null), and
invalidateCodexWebSocketsForAccount for MAIN_CODEX_ACCOUNT_ID, then replace both
inline sequences with the helper while preserving transition behavior.
In `@src/codex/native-profile-api.ts`:
- Around line 101-109: Update the generic fallback branch in the native-profile
error handler to return a distinct internal-failure code such as
"INTERNAL_ERROR" instead of "RECOVERY_REQUIRED". Preserve "RECOVERY_REQUIRED"
exclusively for the NativeProfileError path emitted by assertNoPendingRecovery.
In `@src/codex/native-profile-manager.ts`:
- Around line 494-507: Refactor the operation around the existing native profile
import body and cleanup so deleteStageById never throws from finally. Always
perform buffer zeroization and staging cleanup, capture cleanup failure,
preserve and rethrow any original operation error, and only raise
STAGING_CLEANUP_REQUIRED after a successful writeVault result. Update that error
message to state the profile was imported before cleanup failed, then return the
committed profile normally when cleanup succeeds.
- Around line 310-344: Update doctor() to catch failures from sweepStaleStages()
and readNativeProfileVault(), recording degraded classifications in new
vaultStatus and stagingSweep fields while preserving the existing diagnostic
report. Ensure corrupt vaults still return profileCount and activeProfileId
safely, and staging cleanup failures do not abort the report; keep
credential-store, auth, and recoveryPending diagnostics available.
In `@src/codex/native-profile-processes.ts`:
- Line 10: Validate the SystemRoot-derived executable path before the PowerShell
invocation in the native profile process flow, using the existing C:\Windows
fallback whenever the environment value is absent or implausible. Ensure
execFileSync uses only the validated Windows system path and does not execute a
location selected from an untrusted writable directory.
- Around line 17-43: Set an explicit sufficiently large maxBuffer on both
execFileSync calls in powershellProcessCount and unixProcessCount, including the
ps output path with full arguments. Preserve the existing counting and
validation behavior while preventing expected process-list output from raising
ENOBUFS and being downgraded by probeNativeCodexProcesses to unknown.
In `@src/codex/native-profile-recovery.ts`:
- Around line 78-91: Update decideNativeProfileRecovery to consume
observation.digest and set an externallyRefreshed signal when the target has a
changed digest during the commit-recovery path, while preserving the existing
commit-target action. Add that signal to NativeProfileRecoveryDecision and
propagate it through recoverLocked’s returned object so CLI and management API
callers can report the external refresh; otherwise remove the unused digest
field if warning behavior is intentionally deferred.
In `@src/codex/native-profile-store.ts`:
- Around line 358-375: Separate malformed or unreadable journals from valid
pending journals in readNativeProfileJournal, while preserving RECOVERY_REQUIRED
for normal fail-closed credential operations. Update recover/recoverLocked so an
explicit confirmed rollback catches the malformed-journal case, renames
context.journalPath to a unique quarantine filename using the transaction ID or
timestamp, leaves auth.json and the vault unchanged, and returns the quarantine
path. Keep register, switching, and non-confirmed recovery blocked.
- Around line 381-387: Update validateNativeProfileLabel and parseVaultObject to
share a predicate that rejects bidi override/isolate characters and zero-width
characters in addition to the existing control-character checks. Define the
predicate once near the profile-label validation logic, reuse it in both
validation paths, and preserve trimming, length limits, and the existing
INVALID_REQUEST error behavior.
In `@src/codex/native-profile-types.ts`:
- Line 68: Remove the duplicate NativeProfileJournalPhase declaration from
native-profile-recovery.ts and import/re-export the canonical type from
native-profile-types.ts. Keep consumers such as decideNativeProfileRecovery
using the shared type so native-profile-store.ts and recovery logic remain
synchronized.
In `@tests/helpers/native-profile-lock-child.ts`:
- Around line 19-23: Change the simulated crash branch in onLockAcquired to exit
with the distinct non-zero code used by native-profile-switch-child.ts, then
update the corresponding assertion in native-profile-manager.test.ts to expect
that code so the test verifies the crash path executed.
In `@tests/helpers/native-profile-startup-child.ts`:
- Around line 71-77: Update the promise chain around
waitForNativeMainStartupGate() to attach a rejection handler that writes the
failure details into settledPath, including errors thrown by
nativeMainStartupGateSnapshot(), isMainAccountTokenLive(), or loadConfig() in
the success callback. Preserve the existing success payload and ensure every
failure path writes a JSON record so parent tests surface the cause instead of
timing out.
In `@tests/native-profile-api.test.ts`:
- Around line 51-79: Update the test “stale HTTP/Responses-WebSocket work
settles before switch and new turns stay fenced” to release both admission
leases with try/finally cleanup: always release oldTurn after the request flow,
and always release after when it is acquired. Preserve the existing assertions
and ordering checks while ensuring cleanup runs if the drain returns early or
any assertion throws.
- Line 114: Update the username-leak assertion in the native-profile API test to
read the current OS account name from a cross-platform source that resolves on
Windows, Linux, and macOS, rather than relying only on process.env.USERNAME.
Keep the assertion focused on ensuring the redacted management error payload
does not contain that resolved identity.
In `@tests/native-profile-crash-boundaries.test.ts`:
- Around line 17-21: Restore captured environment variables through a shared
helper that deletes the key when the captured value is undefined, otherwise
assigns the saved value. Apply this in
tests/native-profile-crash-boundaries.test.ts at lines 17-21 and 70-71, and in
tests/native-profile-startup.test.ts at lines 34-38 and 171-172, covering the
afterEach hooks and mid-fixture restores.
- Around line 196-216: Wrap the concurrent-switch test body in a finally block
that always writes firstRelease and awaits both first.exited and second.exited,
including when waitFor or an assertion fails. Mirror the cleanup pattern used by
the nearby test and the existing native-profile-manager test, while preserving
the current success assertions.
In `@tests/native-profile-manager.test.ts`:
- Around line 85-87: Update the journal-phase checks in the test’s injected
failure logic at both matching locations to parse content with JSON.parse and
compare its phase property to "auth-replaced". Replace formatting-dependent
substring matching while preserving the existing path condition and failure
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7593e26e-ba06-4588-acb8-b66bf30f340a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
.github/workflows/ci.yml.github/workflows/release.ymldevlog/_plan/260801_native_main_profiles/000_design.mddevlog/_plan/260801_native_main_profiles/001_validation.mddevlog/_plan/260801_native_main_profiles/002_implementation.mddocs-site/src/content/docs/reference/cli/providers-accounts.mdpackage.jsonscripts/keyring-smoke.tsscripts/release.tssrc/cli/account-api.tssrc/cli/account-main.tssrc/cli/account.tssrc/cli/help.tssrc/codex/account-lifecycle.tssrc/codex/account-usability.tssrc/codex/auth-api.tssrc/codex/auth-context.tssrc/codex/native-profile-api.tssrc/codex/native-profile-manager.tssrc/codex/native-profile-processes.tssrc/codex/native-profile-recovery.tssrc/codex/native-profile-startup.tssrc/codex/native-profile-store.tssrc/codex/native-profile-types.tssrc/server/index.tssrc/server/lifecycle.tssrc/server/management-api.tssrc/server/management/context.tstests/cli-native-profile.test.tstests/codex-websocket-registry.test.tstests/helpers/native-profile-lock-child.tstests/helpers/native-profile-startup-child.tstests/helpers/native-profile-switch-child.tstests/keyring-smoke.test.tstests/native-profile-api.test.tstests/native-profile-crash-boundaries.test.tstests/native-profile-manager.test.tstests/native-profile-recovery.test.tstests/native-profile-route-security.test.tstests/native-profile-startup.test.ts
| - name: OS keyring create/read/delete smoke | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| if [ "$RUNNER_OS" != "Linux" ]; then | ||
| bun run scripts/keyring-smoke.ts | ||
| exit 0 | ||
| fi | ||
|
|
||
| keyring_home="$(mktemp -d)" | ||
| runtime_dir="$(mktemp -d)" | ||
| cleanup() { rm -rf "$keyring_home" "$runtime_dir"; } | ||
| trap cleanup EXIT | ||
| chmod 700 "$keyring_home" "$runtime_dir" | ||
| HOME="$keyring_home" XDG_RUNTIME_DIR="$runtime_dir" dbus-run-session -- bash -euo pipefail -c ' | ||
| eval "$(gnome-keyring-daemon --start --components=secrets)" | ||
| bun run scripts/keyring-smoke.ts | ||
| ' | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial
Self-hosted Windows runner does not get the same keyring isolation as Linux.
For $RUNNER_OS != "Linux", this step runs bun run scripts/keyring-smoke.ts directly against whatever OS keyring session the runner already has. That is safe on ephemeral windows-latest/macos-latest hosts, because the whole VM is discarded after the job.
The windows matrix leg can instead resolve to the self-hosted ocx-home runner (see the select-windows-runner job in this same file). Per the comments on this file's "Clean workspace (self-hosted only)" step, that runner keeps state between jobs. That means the smoke test on ocx-home writes to, reads from, and deletes an entry in the real, persistent Windows Credential Manager of that host, not a throwaway store. If the job is killed mid-run (timeout, cancellation) before the finally block in scripts/keyring-smoke.ts deletes the entry, a stray credential entry with a random UUID service name accumulates on that persistent machine.
Consider giving the self-hosted leg the same explicit isolation treatment as Linux, or at minimum a pre/post-job sweep that deletes any lingering opencodex.keyring-smoke.* entries on ocx-home.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 218 - 236, The non-Linux branch of the
“OS keyring create/read/delete smoke” workflow runs directly against persistent
self-hosted keyring state. Update this step to isolate the self-hosted Windows
leg from the real Credential Manager, or add reliable pre- and post-job cleanup
that removes all lingering opencodex.keyring-smoke.* entries, including when the
smoke test is interrupted.
fc1f6ab to
8790758
Compare
Draft checkpoint updatePushed This checkpoint includes the implementation for all 27 existing review threads plus the follow-up hardening completed during integration: canonical Local validation on Bun
The PR intentionally remains Draft. I will wait for hosted CI, then perform thread-by-thread readback before resolving comments or marking it Ready for review. |
|
Review-thread readback against the public head 8790758 is complete. I resolved 26 of 27 threads whose fixes and regression coverage are present in this revision. I intentionally left only the hosted keyring runner-isolation thread open until the Ubuntu, Windows, and macOS keyring jobs actually run on this head. The PR remains Draft while that hosted evidence and the latest dev refresh are pending. |
8790758 to
9273337
Compare
|
Draft checkpoint updated onto the latest
The PR remains Draft. The hosted keyring-smoke review thread stays open until the disposable Ubuntu, Windows, and macOS jobs have actually run. |
|
@Wibias The latest Draft head is ready for a Ready-state decision:
Would you recommend marking this PR Ready now so review and hosted workflow approval can proceed, or keeping it Draft until workflow sponsorship is available? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 927333765c
ℹ️ 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".
| const firstArg = basename(match[3]!.trim().split(/\s+/, 1)[0] ?? "").toLowerCase(); | ||
| if (command === "codex" || command === "codex.exe" || firstArg === "codex" || firstArg === "codex.exe") count += 1; |
There was a problem hiding this comment.
On Unix installations where the npm codex shim runs under Node or Bun, ps reports arguments such as node /path/to/codex, while comm may be node, bun, or MainThread. This code checks only comm and the first argv token, so it returns clear and permits a profile switch while Codex can still refresh the source credential; inspect the interpreter's script argument as well and add a realistic ps fixture.
AGENTS.md reference: src/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| // The pinned child has its own bounded port reclaim, so it can launch before this | ||
| // process exits without binding the old listener or extending the drain deadline. | ||
| await (io.spawnStart ?? spawnDetachedStart)(port); |
There was a problem hiding this comment.
When an unsupervised drain is still pending at the deadline, the old listener and PID file can still be live when this detached child is spawned. The child then reaches handleStart() in src/cli/index.ts, sees the existing PID and healthy proxy, and exits with “Proxy already running”; meanwhile the parent treats the child’s spawn event as a successful replacement and exits, leaving no server. Stop the listener before spawning, or launch the child with a handoff mode that waits for the parent rather than performing the ordinary already-running check.
Useful? React with 👍 / 👎.
| acquiredLockIdentity = candidateIdentity; | ||
| } catch (error) { | ||
| try { candidate?.close(); } catch { /* acquisition already failed */ } | ||
| try { if (candidateHandle !== undefined) closeSync(candidateHandle); } catch { /* acquisition already failed */ } |
There was a problem hiding this comment.
When one profile operation holds BEGIN IMMEDIATE and a second same-process operation reaches this busy path, closing candidateHandle closes another descriptor for the same SQLite inode. On POSIX, closing any descriptor for that inode releases all record locks owned by the process, so another process can acquire the transaction database while the first operation is still changing the vault or credential. Reuse the ref-counted stable-descriptor mechanism from native-main-lock-file.ts, or otherwise defer raw descriptor closure until every connection holding the lock has finished.
AGENTS.md reference: src/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
Summary
Add an opt-in, CLI/backend-only native-main profile workflow under
ocx account main:doctor,list,register,add,switch, andrecover. It changes the physical Codex login in the effectiveCODEX_HOME, is independent from Pool routing, and preserves task/history files.Each native Codex
auth.jsonis handled as an exact opaque envelope. Inactive profiles and the recovery journal use AES-256-GCM with AAD binding and a random master key held by the native OS credential store through@napi-rs/keyring. The feature supports Codex credential-store modefileonly and fails closed for unavailable keyring, unsupportedkeyring/auto/ephemeralmodes, invalid layouts, and home/path mismatches. There is no plaintext key-file or shell fallback.Safety and recovery
__main__work, requires native Codex to be stopped, atomically publishes the exact target bytes, verifies read-back, updates encrypted vault/runtime state, and restores the exact source on failure.__main__traffic while an unresolved journal exists; health, management, Direct, and ordinary Pool traffic remain available.RECOVERY_REQUIRED; list, doctor, cancel, recover, and switch recovery remain available.codex loginruns in an isolated restricted home. Plaintext staging credentials are removed on success, failure, cancellation, and expiry.__main__runtime-derived state is reconciled; tasks and history remain untouched.Management API boundary
The native-profile management routes are behind the existing local management authentication, CSRF, and origin gates. They carry labels, profile IDs, and staging IDs only; they do not accept or return auth envelopes, access tokens, refresh tokens, raw account IDs, or decrypted vault payloads. Real-server route-admission tests cover missing/wrong admin auth, hostile origins, GUI session/CSRF rejection, and valid admin or GUI admission.
Maintainer follow-up addressed
bun audit --audit-level=highrelease/CI gates and supported-OS keyring create/read/delete smoke jobs.devrebase, corrected CLI documentation, pending-recovery mutation guard, and refreshed-target rollback preservation.Validation
bun audit --audit-level=high: passed.git diff --check upstream/dev...HEAD: passed.A broad local root-suite run completed 6,918 passing and 6 skipped tests, with five fixed 5-second timeout overruns in unchanged account-store/auth-context/routing tests. The run took about 1,555 seconds versus the suite's roughly 210-second normal-duration warning, and isolated failures exposed the existing Windows ACL/timing environment rather than native-profile assertions. Hosted CI on this exact head is the authoritative cross-platform result.
The docs-site frozen install currently reports that the existing upstream
bun.lockwould change before a docs build can start. This PR does not rewrite that unrelated lockfile state.Scope
No dashboard UI, Pool-to-native credential conversion, native process termination, non-file credential-store support, or plaintext fallback is included.
Fixes #656
Related context: #821, #823
Summary by CodeRabbit
ocx account main, including listing, registration, switching, diagnostics, and recovery.