feat(codex): add encrypted native main profiles - #863
Conversation
📝 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
![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: 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".
| } | ||
|
|
||
| async function runOfficialCodexLogin(codexHome: string): Promise<number> { | ||
| const child = Bun.spawn(["codex", "login"], { |
There was a problem hiding this comment.
On Windows when Codex is installed through npm, the executable on PATH is a codex.cmd shim, and this shell-less bare-name spawn fails with ENOENT. The repository already documents this behavior and handles it in src/lib/win-exec.ts and src/codex/exec-invocation.ts; use codexExecInvocation/commandInvocation and forward its spawn options so ocx account main add can start the official login on Windows.
Useful? React with 👍 / 👎.
|
|
||
| async function withMainRequestDrain<T>(deps: NativeProfileApiDeps, operation: () => Promise<T>): Promise<T> { | ||
| if (isDraining()) throw new NativeProfileError("MAIN_REQUESTS_ACTIVE", "The proxy is already draining requests.", 503, true); | ||
| setDraining(true); |
There was a problem hiding this comment.
If a dashboard restart is requested while a profile switch or recovery owns this drain, acceptSystemRestart() in src/server/management/system-restart.ts sees isDraining() === true, reports accepted: true/alreadyDraining: true, and skips scheduling any restart because restartAccepted is still false. The unconditional reset in this function can also clear a shutdown drain that begins concurrently, so the profile operation needs separately owned or reference-counted admission state rather than sharing the shutdown boolean.
Useful? React with 👍 / 👎.
| createdAt: timestamp, | ||
| updatedAt: timestamp, | ||
| }; | ||
| vault.profiles.push(profile); |
There was a problem hiding this comment.
When the vault already contains 32 profiles, finishStage() appends another record and writes it successfully, even though parseVaultObject() rejects every vault with more than MAX_PROFILES entries. The command therefore reports success and deletes the staged credential, but every subsequent list, switch, doctor, or recovery operation fails with VAULT_INVALID; check the capacity before mutating or writing the vault.
Useful? React with 👍 / 👎.
| beforeVault: structuredClone(beforeVault), | ||
| afterVault, |
There was a problem hiding this comment.
With a valid vault approaching the 4 MiB metadata limit—for example, one staged auth envelope of roughly 1–2 MiB, which is accepted under MAX_AUTH_BYTES—embedding both complete vault snapshots here, in addition to duplicate source and target payloads, can produce a journal larger than MAX_METADATA_BYTES. writeJournal() persists it without validation and the switch proceeds, but after a crash both startup and explicit recovery reject that same journal as invalid, leaving the transaction unable to converge; validate the serialized journal before any auth mutation or use compatible storage bounds without duplicating payloads.
AGENTS.md reference: src/AGENTS.md:L17-L20
Useful? React with 👍 / 👎.
| if (!existsSync(manager.context.journalPath)) { | ||
| snapshot = ready(homeId); | ||
| settled = Promise.resolve(snapshot); | ||
| return settled; |
There was a problem hiding this comment.
If a pending journal exists but its parent directory cannot be searched or stat fails—for example because of a transient ACL or mount-permission problem—existsSync() returns false rather than distinguishing that error from ENOENT. Startup therefore marks native main traffic ready and can serve the physical credential while ownership recovery is unresolved; use an error-reporting stat/read and enter the blocked/manual-recovery state for every result except a confirmed missing file.
AGENTS.md reference: src/AGENTS.md:L17-L20
Useful? React with 👍 / 👎.
| authPath: join(codexHome, "auth.json"), | ||
| vaultPath: join(rootDir, `${homeId}.vault.json`), | ||
| journalPath: join(rootDir, `${homeId}.journal.json`), | ||
| lockPath: join(rootDir, `${homeId}.lock`), |
There was a problem hiding this comment.
When two OpenCodex instances use different OPENCODEX_HOME directories but the same effective CODEX_HOME, this path creates two independent SQLite locks even though both managers replace the same CODEX_HOME/auth.json. Their switches can therefore run concurrently with separate vaults and journals, defeating the advertised homeId serialization and allowing one transaction's read-back or rollback to overwrite the other's credential; the lock must resolve to state shared by every manager of the canonical Codex home.
AGENTS.md reference: src/AGENTS.md:L15-L20
Useful? React with 👍 / 👎.
| return jsonResponse( | ||
| await withMainRequestDrain(deps, () => manager.switch(input.target as string, input.confirmedStopped === true)), | ||
| 200, | ||
| req, | ||
| config, | ||
| ); |
There was a problem hiding this comment.
After startup recovery enters manual-recovery, a user can repair auth.json and call switch; NativeProfileManager.switch() automatically converges and removes the pending journal before performing the requested switch, but this branch never calls completeNativeMainRecovery(). The API can consequently report a successful switch while isNativeMainTrafficBlocked() remains true indefinitely, so either prohibit switch-driven recovery while the gate is blocked or reopen the matching home gate once the switch has cleared the journal.
AGENTS.md reference: src/AGENTS.md:L17-L20
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
| ### `ocx account main <subcommand>` | ||
|
|
||
| Manage named native Codex main-login profiles without changing OpenCodex account-pool routing: | ||
|
|
||
| ```text | ||
| ocx account main doctor | ||
| ocx account main list | ||
| ocx account main register <name> | ||
| ocx account main add <name> | ||
| ocx account main switch <name> --yes | ||
| ocx account main recover | ||
| ocx account main recover --rollback --yes | ||
| ``` | ||
|
|
||
| Version 1 supports file-based Codex authentication, encrypts stored profiles with AES-256-GCM, and | ||
| keeps the encryption key in the operating-system credential store. `add` stages the official Codex | ||
| login flow before importing the resulting credential. Close Codex before switching profiles; a | ||
| successful switch preserves local tasks and history, then requires Codex to be restarted. Use | ||
| `doctor` to inspect profile state and `recover` to finish or roll back an interrupted transition. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare documented `ocx account main` surface with CLI implementation and locale pages.
set -uo pipefail
echo "== Locale copies of the providers-accounts reference page =="
fd -t f 'providers-accounts.md' docs-site | while IFS= read -r f; do
printf '%s: account main mentions=%s\n' "$f" "$(rg -c 'account main' "$f" 2>/dev/null || echo 0)"
done
echo
echo "== CLI subcommand + flag parsing for `account main` =="
fd -t f 'account-main.ts' src | while IFS= read -r f; do
rg -n -C3 '"doctor"|"list"|"register"|"add"|"switch"|"recover"|--json|--yes|--rollback' "$f"
done
echo
echo "== Help text advertised for the subcommand =="
rg -n -C3 'account main' src/cliRepository: lidge-jun/opencodex
Length of output: 5827
🏁 Script executed:
#!/bin/bash
set -u
echo "== English reference section =="
sed -n '190,245p' docs-site/src/content/docs/reference/cli/providers-accounts.md
echo
echo "== Translated reference sections =="
for f in \
docs-site/src/content/docs/ja/reference/cli/providers-accounts.md \
docs-site/src/content/docs/ko/reference/cli/providers-accounts.md \
docs-site/src/content/docs/ru/reference/cli/providers-accounts.md \
docs-site/src/content/docs/zh-cn/reference/cli/providers-accounts.md
do
echo "--- $f"
rg -n -C3 'ocx account|account main|Native main|native Codex' "$f" | tail -80 || true
done
echo
echo "== Nearby account command documentation =="
rg -n -C4 -- '--json|ocx account (list|register|add|switch|recover|main)' \
docs-site/src/content/docs/reference/cli/providers-accounts.md
echo
echo "== Native main CLI implementation =="
sed -n '1,155p' src/cli/account-main.ts
echo
echo "== Design notes around the referenced surface =="
fd -t f . devlog/_plan/260801_native_main_profiles | while IFS= read -r f; do
rg -n -C4 'account main|profile-id-or-label|--json|--yes' "$f" || true
doneRepository: lidge-jun/opencodex
Length of output: 39794
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
from pathlib import Path
import re
cli = Path("src/cli/account-main.ts").read_text()
manager = Path("src/codex/native-profile-manager.ts").read_text()
usage = cli[cli.index("const USAGE"):cli.index("interface PublicProfile")]
expected = {
"doctor": "[--json]",
"list": "[--json]",
"register": "<label> [--json]",
"add": "<label>",
"switch": "<profile-id-or-label> --yes [--json]",
"recover": "[--rollback --yes] [--json]",
}
print("== CLI usage assertions ==")
for command, suffix in expected.items():
needle = f"ocx account main {command} {suffix}"
print(f"{command}: {'present' if needle in usage else 'MISSING'}")
print()
print("== Target resolution implementation ==")
match = re.search(
r"resolveTarget\s*\([^)]*\)\s*\{(?P<body>.*?)\n\s*\}",
manager,
re.S,
)
if not match:
print("resolveTarget: NOT FOUND")
else:
body = match.group("body")
print(body.strip())
print()
print("== Locale parity assertions ==")
for path in sorted(Path("docs-site/src/content/docs").glob("*/reference/cli/providers-accounts.md")):
text = path.read_text()
print(f"{path}: {'present' if 'ocx account main' in text else 'MISSING'}")
PY
echo
echo "== Exact target-resolution lines =="
sed -n '510,535p' src/codex/native-profile-manager.tsRepository: lidge-jun/opencodex
Length of output: 2103
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
from pathlib import Path
import re
cli = Path("src/cli/account-main.ts").read_text()
manager = Path("src/codex/native-profile-manager.ts").read_text()
usage = cli[cli.index("const USAGE"):cli.index("interface PublicProfile")]
expected = {
"doctor": "[--json]",
"list": "[--json]",
"register": "<label> [--json]",
"add": "<label>",
"switch": "<profile-id-or-label> --yes [--json]",
"recover": "[--rollback --yes] [--json]",
}
print("== CLI usage assertions ==")
for command, suffix in expected.items():
needle = f"ocx account main {command} {suffix}"
print(f"{command}: {'present' if needle in usage else 'MISSING'}")
print()
print("== Target resolution implementation ==")
match = re.search(
r"resolveTarget\s*\([^)]*\)\s*\{(?P<body>.*?)\n\s*\}",
manager,
re.S,
)
if not match:
print("resolveTarget: NOT FOUND")
else:
print(match.group("body").strip())
print()
print("== Locale parity assertions ==")
for path in sorted(Path("docs-site/src/content/docs").glob("*/reference/cli/providers-accounts.md")):
text = path.read_text()
print(f"{path}: {'present' if 'ocx account main' in text else 'MISSING'}")
PY
echo
echo "== Exact target-resolution lines =="
sed -n '510,535p' src/codex/native-profile-manager.tsRepository: lidge-jun/opencodex
Length of output: 2103
Synchronize ocx account main documentation across locales and flags.
- Add the missing section to
ja,ko,ru, andzh-cnproviders-accounts.mdpages. - Update the English command block at
docs-site/src/content/docs/reference/cli/providers-accounts.md:213-232to matchsrc/cli/account-main.ts:5-10: use<label>, document supported--jsonflags, and use<profile-id-or-label>forswitch. Keepswitch --yes; the CLI requires it.resolveTargetaccepts both IDs and labels.
🤖 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 `@docs-site/src/content/docs/reference/cli/providers-accounts.md` around lines
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.
Source: Path instructions
| try { | ||
| entry = await createEntry(service, account); | ||
| await entry.setSecret(secret, AbortSignal.timeout(timeoutMs)); | ||
| const readback = await entry.getSecret(AbortSignal.timeout(timeoutMs)); | ||
| stored = readback ? Buffer.from(readback) : null; | ||
| if (!stored || stored.byteLength !== secret.byteLength || !timingSafeEqual(stored, secret)) { | ||
| throw new Error("OS keyring smoke readback did not match the stored value."); | ||
| } | ||
| } finally { | ||
| try { | ||
| if (entry && !await entry.deleteCredential(AbortSignal.timeout(timeoutMs))) { | ||
| throw new Error("OS keyring smoke could not delete the temporary entry."); | ||
| } | ||
| } finally { | ||
| stored?.fill(0); | ||
| secret.fill(0); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the finally-block error masking in scripts/keyring-smoke.ts, then add a regression test for it. The nested finally in runKeyringSmoke can replace a pending readback-mismatch error with a cleanup-failure error, and no existing test exercises that compound failure.
scripts/keyring-smoke.ts#L34-L51: stop the innerfinally'sdeleteCredentialfailure path from throwing over a pending exception; log the cleanup failure instead of re-throwing it (see the proposed diff on this file).tests/keyring-smoke.test.ts#L44-L56: add a case wheregetSecretreturns a mismatched value anddeleteCredentialalso returnsfalse(or throws), then assert thatrunKeyringSmokestill rejects with the original "readback did not match" message.
🧰 Tools
🪛 Biome (2.5.5)
[error] 45-45: Unsafe usage of 'throw'.
(lint/correctness/noUnsafeFinally)
📍 Affects 2 files
scripts/keyring-smoke.ts#L34-L51(this comment)tests/keyring-smoke.test.ts#L44-L56
🤖 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 `@scripts/keyring-smoke.ts` around lines 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.
Source: Linters/SAST tools
| /** | ||
| * Apply a transaction-confirmed physical native-login change without waiting for | ||
| * a later auth.json observation. The caller owns credential commit/rollback. | ||
| */ | ||
| export function applyConfirmedMainCodexAccountTransition( | ||
| fromAccountId: string, | ||
| toAccountId: string, | ||
| ): boolean { | ||
| if (!fromAccountId || !toAccountId || fromAccountId === toAccountId) { | ||
| if (toAccountId) observedMainChatgptAccountId = toAccountId; | ||
| return false; | ||
| } | ||
| observedMainChatgptAccountId = toAccountId; | ||
| purgeCodexAccountRuntimeState(MAIN_CODEX_ACCOUNT_ID); | ||
| setMainAccountPlan(null); | ||
| invalidateCodexWebSocketsForAccount(MAIN_CODEX_ACCOUNT_ID); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the duplicated account-purge sequence into a shared helper.
Lines 56-58 repeat the exact sequence already used in reconcileMainCodexAccountRuntimeState (lines 37-39, unchanged in this diff): purgeCodexAccountRuntimeState(MAIN_CODEX_ACCOUNT_ID), setMainAccountPlan(null), invalidateCodexWebSocketsForAccount(MAIN_CODEX_ACCOUNT_ID). This sequence purges the main-account credential/state footprint on any identity transition. Keep it in one place so a future change to the purge sequence (for example, adding a new state store to clear) cannot be applied to one call site and forgotten on the other.
♻️ Proposed refactor
+function resetMainCodexAccountState(): void {
+ purgeCodexAccountRuntimeState(MAIN_CODEX_ACCOUNT_ID);
+ setMainAccountPlan(null);
+ invalidateCodexWebSocketsForAccount(MAIN_CODEX_ACCOUNT_ID);
+}
+
export function reconcileMainCodexAccountRuntimeState(): boolean {
const currentAccountId = getMainChatgptAccountId();
if (currentAccountId === null) return false;
const previousAccountId = observedMainChatgptAccountId;
observedMainChatgptAccountId = currentAccountId;
if (previousAccountId === undefined || previousAccountId === currentAccountId) return false;
- purgeCodexAccountRuntimeState(MAIN_CODEX_ACCOUNT_ID);
- setMainAccountPlan(null);
- invalidateCodexWebSocketsForAccount(MAIN_CODEX_ACCOUNT_ID);
+ resetMainCodexAccountState();
return true;
}
export function applyConfirmedMainCodexAccountTransition(
fromAccountId: string,
toAccountId: string,
): boolean {
if (!fromAccountId || !toAccountId || fromAccountId === toAccountId) {
if (toAccountId) observedMainChatgptAccountId = toAccountId;
return false;
}
observedMainChatgptAccountId = toAccountId;
- purgeCodexAccountRuntimeState(MAIN_CODEX_ACCOUNT_ID);
- setMainAccountPlan(null);
- invalidateCodexWebSocketsForAccount(MAIN_CODEX_ACCOUNT_ID);
+ resetMainCodexAccountState();
return true;
}🤖 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 `@src/codex/account-lifecycle.ts` around lines 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.
| } catch (error) { | ||
| const tooLarge = managementBodyTooLargeResponse(error, req, config); | ||
| if (tooLarge) return tooLarge; | ||
| if (error instanceof NativeProfileError) { | ||
| return jsonResponse({ error: error.message, code: error.code, retryable: error.retryable }, error.status, req, config); | ||
| } | ||
| return jsonResponse({ error: "Native-profile operation failed without changing or exposing credential data", code: "RECOVERY_REQUIRED" }, 500, req, config); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not reuse RECOVERY_REQUIRED for the generic catch-all error.
At line 107, every exception that is not a NativeProfileError maps to code: "RECOVERY_REQUIRED". This code is also the specific, actionable code the manager throws when a recovery journal is genuinely pending (see assertNoPendingRecovery in native-profile-manager.ts, which throws NativeProfileError("RECOVERY_REQUIRED", ...)). A caller that branches on code cannot distinguish "run ocx account main recover" from "an unrelated bug occurred (TypeError, I/O failure, etc.)". This can send a CLI/dashboard user down the wrong remediation path and hide the real defect.
Use a distinct, generic code (for example "INTERNAL_ERROR") for this fallback branch, and keep RECOVERY_REQUIRED reserved for the manager's actual pending-journal signal.
🐛 Proposed fix
- return jsonResponse({ error: "Native-profile operation failed without changing or exposing credential data", code: "RECOVERY_REQUIRED" }, 500, req, config);
+ return jsonResponse({ error: "Native-profile operation failed without changing or exposing credential data", code: "INTERNAL_ERROR" }, 500, req, config);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| const tooLarge = managementBodyTooLargeResponse(error, req, config); | |
| if (tooLarge) return tooLarge; | |
| if (error instanceof NativeProfileError) { | |
| return jsonResponse({ error: error.message, code: error.code, retryable: error.retryable }, error.status, req, config); | |
| } | |
| return jsonResponse({ error: "Native-profile operation failed without changing or exposing credential data", code: "RECOVERY_REQUIRED" }, 500, req, config); | |
| } | |
| } | |
| } catch (error) { | |
| const tooLarge = managementBodyTooLargeResponse(error, req, config); | |
| if (tooLarge) return tooLarge; | |
| if (error instanceof NativeProfileError) { | |
| return jsonResponse({ error: error.message, code: error.code, retryable: error.retryable }, error.status, req, config); | |
| } | |
| return jsonResponse({ error: "Native-profile operation failed without changing or exposing credential data", code: "INTERNAL_ERROR" }, 500, req, config); | |
| } | |
| } |
🤖 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 `@src/codex/native-profile-api.ts` around lines 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.
| test("stale HTTP/Responses-WebSocket work settles before switch and new turns stay fenced", async () => { | ||
| const oldTurn = tryAdmitTurn(); | ||
| expect(oldTurn).not.toBeNull(); | ||
| const order: string[] = []; | ||
| let slept = false; | ||
| const manager = { | ||
| switch: async () => { order.push("switch"); return { ok: true }; }, | ||
| } as unknown as NativeProfileManager; | ||
| const request = new Request("http://localhost/api/native-main-profiles/switch", { | ||
| method: "POST", | ||
| body: JSON.stringify({ target: "target", confirmedStopped: true }), | ||
| }); | ||
| const response = await handleNativeProfileAPI(request, new URL(request.url), {} as OcxConfig, { | ||
| manager, | ||
| drainTimeoutMs: 1_000, | ||
| sleep: async () => { | ||
| if (slept) return Bun.sleep(1); | ||
| slept = true; | ||
| expect(tryAdmitTurn()).toBeNull(); | ||
| order.push("old-http-or-ws-response-finished"); | ||
| oldTurn?.release(); | ||
| }, | ||
| }); | ||
| expect(response?.status).toBe(200); | ||
| expect(order).toEqual(["old-http-or-ws-response-finished", "switch"]); | ||
| const after = tryAdmitTurn(); | ||
| expect(after).not.toBeNull(); | ||
| after?.release(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Release the admission leases in a finally block.
This test acquires two process-global admission leases and releases both on the happy path only.
oldTurn (line 52) is released at line 71, inside the injected sleep callback. That callback runs only if handleNativeProfileAPI reaches the drain wait. If the drain observes no active turns and returns immediately, or if the assertion at line 69 throws, line 71 never runs. after (line 76) has the same exposure: an exception at line 74 or 75 skips line 78.
tryAdmitTurn uses shared module state in src/server/lifecycle.ts, and the afterEach hook at lines 12-16 resets only the draining flag. A leaked lease therefore persists into every later test in this file and into any other test file that shares the module instance. The failure mode is an unrelated later test seeing an unexpected active turn, which presents as a confusing cascade rather than a single failure.
Test 2 at lines 38-48 already uses the correct try/finally shape. Apply it here.
🛡️ Proposed fix
const oldTurn = tryAdmitTurn();
expect(oldTurn).not.toBeNull();
const order: string[] = [];
let slept = false;
- const manager = {
- switch: async () => { order.push("switch"); return { ok: true }; },
- } as unknown as NativeProfileManager;
- const request = new Request("http://localhost/api/native-main-profiles/switch", {
- method: "POST",
- body: JSON.stringify({ target: "target", confirmedStopped: true }),
- });
- const response = await handleNativeProfileAPI(request, new URL(request.url), {} as OcxConfig, {
- manager,
- drainTimeoutMs: 1_000,
- sleep: async () => {
- if (slept) return Bun.sleep(1);
- slept = true;
- expect(tryAdmitTurn()).toBeNull();
- order.push("old-http-or-ws-response-finished");
- oldTurn?.release();
- },
- });
- expect(response?.status).toBe(200);
- expect(order).toEqual(["old-http-or-ws-response-finished", "switch"]);
- const after = tryAdmitTurn();
- expect(after).not.toBeNull();
- after?.release();
+ let after: ReturnType<typeof tryAdmitTurn> = null;
+ try {
+ const manager = {
+ switch: async () => { order.push("switch"); return { ok: true }; },
+ } as unknown as NativeProfileManager;
+ const request = new Request("http://localhost/api/native-main-profiles/switch", {
+ method: "POST",
+ body: JSON.stringify({ target: "target", confirmedStopped: true }),
+ });
+ const response = await handleNativeProfileAPI(request, new URL(request.url), {} as OcxConfig, {
+ manager,
+ drainTimeoutMs: 1_000,
+ sleep: async () => {
+ if (slept) return Bun.sleep(1);
+ slept = true;
+ expect(tryAdmitTurn()).toBeNull();
+ order.push("old-http-or-ws-response-finished");
+ oldTurn?.release();
+ },
+ });
+ expect(response?.status).toBe(200);
+ expect(order).toEqual(["old-http-or-ws-response-finished", "switch"]);
+ after = tryAdmitTurn();
+ expect(after).not.toBeNull();
+ } finally {
+ if (!slept) oldTurn?.release();
+ after?.release();
+ }🤖 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 `@tests/native-profile-api.test.ts` around lines 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.
| expect(response!.status).toBeGreaterThanOrEqual(400); | ||
| const payload = JSON.stringify(await response!.json()); | ||
| expect(payload).not.toContain(missingHome); | ||
| expect(payload).not.toContain(process.env.USERNAME ?? "Administrator"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
The username-leak assertion is vacuous on Linux and macOS.
Line 114 reads process.env.USERNAME ?? "Administrator". USERNAME is a Windows convention. On Linux and macOS the account name lives in USER, and USERNAME is normally unset. The expression therefore collapses to expect(payload).not.toContain("Administrator") on the platforms that run CI, which passes for any payload that does not contain that literal string.
The assertion is meant to prove that the redacted management error boundary leaks no OS identity. On the primary CI platforms it currently proves nothing. Read the name from a source that resolves on every supported OS.
🛡️ Proposed fix
-import { tmpdir } from "node:os";
+import { tmpdir, userInfo } from "node:os"; const payload = JSON.stringify(await response!.json());
expect(payload).not.toContain(missingHome);
- expect(payload).not.toContain(process.env.USERNAME ?? "Administrator");
+ for (const name of [process.env.USER, process.env.USERNAME, userInfo().username]) {
+ if (name) expect(payload).not.toContain(name);
+ }Note that missingHome at line 100 is built from tmpdir(), which on macOS resolves under /var/folders/... and contains no username. The line 113 assertion therefore does not cover this gap either.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(payload).not.toContain(process.env.USERNAME ?? "Administrator"); | |
| for (const name of [process.env.USER, process.env.USERNAME, userInfo().username]) { | |
| if (name) expect(payload).not.toContain(name); | |
| } |
🤖 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 `@tests/native-profile-api.test.ts` at 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.
| afterEach(() => { | ||
| process.env.OPENCODEX_HOME = oldOcx; | ||
| process.env.CODEX_HOME = oldCodex; | ||
| for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restoring process.env from a possibly-undefined capture writes the string "undefined".
Both files capture environment variables that may be unset, then restore them with direct assignment. process.env setters coerce the assigned value to a string. When the captured value is undefined, the assignment does not delete the key; it sets the key to the literal string "undefined".
The concrete failure mode: if OPENCODEX_HOME or CODEX_HOME is unset in the CI environment, these hooks leave process.env.OPENCODEX_HOME === "undefined" after the test. Any later code that calls loadConfig() or resolves the Codex home then resolves a relative directory literally named undefined in the process working directory, instead of falling back to the real default under $HOME. Tests that assert on default-path behavior fail, or worse, silently read and write config in the wrong location. The restore inside fixture() makes this reachable mid-test, not only after the file finishes.
Two sibling files in this same PR already implement the correct pattern: tests/native-profile-route-security.test.ts lines 79-88 and tests/native-profile-api.test.ts lines 14-15 both branch on undefined and call delete. Apply that pattern at all four sites below.
tests/native-profile-crash-boundaries.test.ts#L17-L21: in theafterEachhook, replace the two direct assignments ofoldOcxandoldCodexwith a helper that deletes the key when the captured value isundefined.tests/native-profile-crash-boundaries.test.ts#L70-L71: apply the same helper to the mid-fixture restore ofOPENCODEX_HOMEandCODEX_HOMEthat follows thesaveConfigcall.tests/native-profile-startup.test.ts#L34-L38: in theafterEachhook, replace the direct assignments ofpreviousOpencodexHomeandpreviousCodexHomewith the same helper.tests/native-profile-startup.test.ts#L171-L172: apply the same helper to the mid-fixture restore that follows thesaveConfigandsaveCodexAccountCredentialcalls.
🛡️ Proposed fix, shown for tests/native-profile-crash-boundaries.test.ts
const roots: string[] = [];
const oldOcx = process.env.OPENCODEX_HOME;
const oldCodex = process.env.CODEX_HOME;
+const restoreEnv = (name: string, value: string | undefined): void => {
+ if (value === undefined) delete process.env[name];
+ else process.env[name] = value;
+};
+
afterEach(() => {
- process.env.OPENCODEX_HOME = oldOcx;
- process.env.CODEX_HOME = oldCodex;
+ restoreEnv("OPENCODEX_HOME", oldOcx);
+ restoreEnv("CODEX_HOME", oldCodex);
for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true });
});And at the end of fixture():
- process.env.OPENCODEX_HOME = oldOcx;
- process.env.CODEX_HOME = oldCodex;
+ restoreEnv("OPENCODEX_HOME", oldOcx);
+ restoreEnv("CODEX_HOME", oldCodex);
return { root, home, codexHome, configDir, source, target, key, manager, sourceProfile, targetProfile, initialRevision };Apply the equivalent change in tests/native-profile-startup.test.ts for previousOpencodexHome and previousCodexHome at lines 35-36 and 171-172.
📍 Affects 2 files
tests/native-profile-crash-boundaries.test.ts#L17-L21(this comment)tests/native-profile-crash-boundaries.test.ts#L70-L71tests/native-profile-startup.test.ts#L34-L38tests/native-profile-startup.test.ts#L171-L172
🤖 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 `@tests/native-profile-crash-boundaries.test.ts` around lines 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.
| test("two concurrent real switches serialize to one commit without credential overlap", async () => { | ||
| const f = await fixture(); | ||
| const firstReady = join(f.root, "first-ready"); | ||
| const firstRelease = join(f.root, "first-release"); | ||
| const firstResult = join(f.root, "first-result"); | ||
| const secondResult = join(f.root, "second-result"); | ||
| const first = spawnSwitch(f, { marker: firstReady, release: firstRelease, result: firstResult }); | ||
| await waitFor(firstReady); | ||
| const second = spawnSwitch(f, { result: secondResult }); | ||
| await Bun.sleep(100); | ||
| expect(existsSync(secondResult)).toBe(false); | ||
| writeFileSync(firstRelease, "release"); | ||
| expect(await first.exited).toBe(0); | ||
| expect(await second.exited).toBe(0); | ||
| const results = [JSON.parse(readFileSync(firstResult, "utf8")), JSON.parse(readFileSync(secondResult, "utf8"))]; | ||
| if (results.filter(item => item.ok).length !== 1) throw new Error(`unexpected concurrent results: ${JSON.stringify(results)}`); | ||
| expect(results.filter(item => !item.ok).map(item => item.code)).toEqual(["INVALID_REQUEST"]); | ||
| expect(readFileSync(f.manager.context.authPath, "utf8")).toBe(f.target); | ||
| expect(readNativeProfileVault(f.manager.context)!.activeProfileId).toBe(f.targetProfile.id); | ||
| expect(readNativeProfileJournal(f.manager.context)).toBeNull(); | ||
| }, 20_000); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a finally block so the lock-holding child cannot outlive a failed assertion.
This test spawns two switch children and has no cleanup path. first is spawned at line 202 with release: firstRelease. Inside tests/helpers/native-profile-switch-child.ts line 34, that child polls while (!existsSync(releasePath)) await Bun.sleep(10); with no deadline and no timeout.
firstRelease is written only at line 207. If waitFor(firstReady) at line 203 times out, or if the assertion at line 206 fails, execution leaves the test before line 207. Both children then survive. The afterEach hook at line 20 removes the fixture root, which deletes the very path first polls for, so the release marker can never appear and the child spins indefinitely. The children are spawned with stdout: "pipe" and stderr: "pipe" (helper spawn at line 98), so the runner holds handles on them.
Test 1 at lines 188-192 already uses the correct shape. Mirror it.
🛡️ Proposed fix
const first = spawnSwitch(f, { marker: firstReady, release: firstRelease, result: firstResult });
- await waitFor(firstReady);
- const second = spawnSwitch(f, { result: secondResult });
- await Bun.sleep(100);
- expect(existsSync(secondResult)).toBe(false);
- writeFileSync(firstRelease, "release");
- expect(await first.exited).toBe(0);
- expect(await second.exited).toBe(0);
- const results = [JSON.parse(readFileSync(firstResult, "utf8")), JSON.parse(readFileSync(secondResult, "utf8"))];
- if (results.filter(item => item.ok).length !== 1) throw new Error(`unexpected concurrent results: ${JSON.stringify(results)}`);
- expect(results.filter(item => !item.ok).map(item => item.code)).toEqual(["INVALID_REQUEST"]);
- expect(readFileSync(f.manager.context.authPath, "utf8")).toBe(f.target);
- expect(readNativeProfileVault(f.manager.context)!.activeProfileId).toBe(f.targetProfile.id);
- expect(readNativeProfileJournal(f.manager.context)).toBeNull();
+ let second: ReturnType<typeof spawnSwitch> | undefined;
+ try {
+ await waitFor(firstReady);
+ second = spawnSwitch(f, { result: secondResult });
+ await Bun.sleep(100);
+ expect(existsSync(secondResult)).toBe(false);
+ writeFileSync(firstRelease, "release");
+ expect(await first.exited).toBe(0);
+ expect(await second.exited).toBe(0);
+ const results = [JSON.parse(readFileSync(firstResult, "utf8")), JSON.parse(readFileSync(secondResult, "utf8"))];
+ if (results.filter(item => item.ok).length !== 1) throw new Error(`unexpected concurrent results: ${JSON.stringify(results)}`);
+ expect(results.filter(item => !item.ok).map(item => item.code)).toEqual(["INVALID_REQUEST"]);
+ expect(readFileSync(f.manager.context.authPath, "utf8")).toBe(f.target);
+ expect(readNativeProfileVault(f.manager.context)!.activeProfileId).toBe(f.targetProfile.id);
+ expect(readNativeProfileJournal(f.manager.context)).toBeNull();
+ } finally {
+ try { writeFileSync(firstRelease, "release"); } catch { /* fixture cleanup */ }
+ if (first.exitCode === null) first.kill();
+ if (second?.exitCode === null) second.kill();
+ await first.exited;
+ if (second) await second.exited;
+ }tests/native-profile-manager.test.ts lines 167-174 uses this exact cleanup pattern already.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("two concurrent real switches serialize to one commit without credential overlap", async () => { | |
| const f = await fixture(); | |
| const firstReady = join(f.root, "first-ready"); | |
| const firstRelease = join(f.root, "first-release"); | |
| const firstResult = join(f.root, "first-result"); | |
| const secondResult = join(f.root, "second-result"); | |
| const first = spawnSwitch(f, { marker: firstReady, release: firstRelease, result: firstResult }); | |
| await waitFor(firstReady); | |
| const second = spawnSwitch(f, { result: secondResult }); | |
| await Bun.sleep(100); | |
| expect(existsSync(secondResult)).toBe(false); | |
| writeFileSync(firstRelease, "release"); | |
| expect(await first.exited).toBe(0); | |
| expect(await second.exited).toBe(0); | |
| const results = [JSON.parse(readFileSync(firstResult, "utf8")), JSON.parse(readFileSync(secondResult, "utf8"))]; | |
| if (results.filter(item => item.ok).length !== 1) throw new Error(`unexpected concurrent results: ${JSON.stringify(results)}`); | |
| expect(results.filter(item => !item.ok).map(item => item.code)).toEqual(["INVALID_REQUEST"]); | |
| expect(readFileSync(f.manager.context.authPath, "utf8")).toBe(f.target); | |
| expect(readNativeProfileVault(f.manager.context)!.activeProfileId).toBe(f.targetProfile.id); | |
| expect(readNativeProfileJournal(f.manager.context)).toBeNull(); | |
| }, 20_000); | |
| const first = spawnSwitch(f, { marker: firstReady, release: firstRelease, result: firstResult }); | |
| let second: ReturnType<typeof spawnSwitch> | undefined; | |
| try { | |
| await waitFor(firstReady); | |
| second = spawnSwitch(f, { result: secondResult }); | |
| await Bun.sleep(100); | |
| expect(existsSync(secondResult)).toBe(false); | |
| writeFileSync(firstRelease, "release"); | |
| expect(await first.exited).toBe(0); | |
| expect(await second.exited).toBe(0); | |
| const results = [JSON.parse(readFileSync(firstResult, "utf8")), JSON.parse(readFileSync(secondResult, "utf8"))]; | |
| if (results.filter(item => item.ok).length !== 1) throw new Error(`unexpected concurrent results: ${JSON.stringify(results)}`); | |
| expect(results.filter(item => !item.ok).map(item => item.code)).toEqual(["INVALID_REQUEST"]); | |
| expect(readFileSync(f.manager.context.authPath, "utf8")).toBe(f.target); | |
| expect(readNativeProfileVault(f.manager.context)!.activeProfileId).toBe(f.targetProfile.id); | |
| expect(readNativeProfileJournal(f.manager.context)).toBeNull(); | |
| } finally { | |
| try { writeFileSync(firstRelease, "release"); } catch { /* fixture cleanup */ } | |
| if (first.exitCode === null) first.kill(); | |
| if (second?.exitCode === null) second.kill(); | |
| await first.exited; | |
| if (second) await second.exited; | |
| } |
🤖 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 `@tests/native-profile-crash-boundaries.test.ts` around lines 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.
| if (path === journalPath && content.includes('"phase": "auth-replaced"')) { | ||
| throw new Error("injected post-replacement failure"); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the journal serializer to confirm the pretty-print spacing the tests rely on.
set -euo pipefail
fd -t f 'native-profile-store.ts' src --exec ast-grep outline {} --items all
echo '--- serializeNativeProfileMetadata definition ---'
ast-grep run --lang typescript \
--pattern 'export function serializeNativeProfileMetadata($$$) { $$$ }' \
src
echo '--- any JSON.stringify indent arguments in native-profile modules ---'
rg -nP --type=ts -C3 'JSON\.stringify\s*\(' src/codex/native-profile-store.ts
echo '--- every consumer of the phase-string literal ---'
rg -nP --type=ts -C2 '"phase":\s*\\?"' tests srcRepository: lidge-jun/opencodex
Length of output: 3159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- serializer and journal reader ---'
sed -n '340,385p' src/codex/native-profile-store.ts
echo '--- test fault injection and leavePendingJournal ---'
sed -n '1,115p' tests/native-profile-manager.test.ts
sed -n '350,495p' tests/native-profile-manager.test.ts
echo '--- serializer call sites ---'
rg -n -C3 'serializeNativeProfileMetadata|auth-replaced|leavePendingJournal' src testsRepository: lidge-jun/opencodex
Length of output: 30141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const content = `${JSON.stringify({ phase: "auth-replaced" }, null, 2)}\n`;
console.log(JSON.stringify({
content,
exactMatch: content.includes('"phase": "auth-replaced"'),
parsedMatch: JSON.parse(content).phase === "auth-replaced",
}));
const compact = JSON.stringify({ phase: "auth-replaced" });
console.log(JSON.stringify({
compact,
exactMatch: compact.includes('"phase": "auth-replaced"'),
parsedMatch: JSON.parse(compact).phase === "auth-replaced",
}));
JSRepository: lidge-jun/opencodex
Length of output: 329
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const pretty = `${JSON.stringify({ phase: "auth-replaced" }, null, 2)}\n`;
const compact = JSON.stringify({ phase: "auth-replaced" });
console.log(JSON.stringify({
prettyExactMatch: pretty.includes('"phase": "auth-replaced"'),
prettyParsedMatch: JSON.parse(pretty).phase === "auth-replaced",
compactExactMatch: compact.includes('"phase": "auth-replaced"'),
compactParsedMatch: JSON.parse(compact).phase === "auth-replaced",
}));
JSRepository: lidge-jun/opencodex
Length of output: 260
Match the journal phase by parsing JSON in tests/native-profile-manager.test.ts:85-86 and :367-368.
serializeNativeProfileMetadata currently uses two-space JSON formatting, so the substring matches. A compact serializer would stop the injection. Use JSON.parse(content).phase === "auth-replaced" to test the journal value instead of its formatting.
🤖 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 `@tests/native-profile-manager.test.ts` around lines 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.
2 participants
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.