fix(cli): check live proxy before journal recovery - #1269
Conversation
📝 WalkthroughWalkthrough
ChangesStartup lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
|
Adopting this — the One gap: Could you extend the fix to On the test: the |
⏳ DRAFT
What to do
This pull request was already a draft. Its draft status will be preserved after every issue above is resolved. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/cli/index.ts`:
- Around line 234-237: Move the ownership gate currently guarding
reconcileJournal near the startup path into a shared helper, including PID
existence, live-proxy detection, and stale-PID cleanup. Invoke this helper
before reconcileJournal in both the startup flow and handleEnsure, so a healthy
active proxy prevents journal merging while stale ownership is cleaned up first;
add an ensure regression case covering this behavior.
In `@tests/cli-start-journal-order.test.ts`:
- Around line 15-30: Replace the source-offset assertions in the journal
ownership ordering test with Bun behavioral regression tests that invoke the
actual CLI lifecycle. In the healthy-owner case, isolate OPENCODEX_HOME and
CODEX_HOME, seed a dead journal PID, run a healthy proxy, execute ocx start, and
verify managed Codex configuration, catalog, journal, profile, and history
remain unchanged; in the dead-owner/no-listener case, use the isolated state
without a proxy and verify reconciliation restores the expected state. Add
equivalent ocx ensure coverage when it uses the shared startup gate, using
existing test helpers and symbols rather than asserting source layout.
🪄 Autofix
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: 28e246d3-22ea-4c78-8117-76a364e9b03c
📒 Files selected for processing (2)
src/cli/index.tstests/cli-start-journal-order.test.ts
| // A losing concurrent start must not restore the active proxy's Codex config. | ||
| // Establish that the PID-file owner is stale before reconciling a dead journal; | ||
| // a healthy owner exits above without changing integration state (#1230). | ||
| if (!currentExternalCodexModelProvider()) reconcileJournal(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/cli/index.ts --items all --type function
rg -n -C 30 'async function handleEnsure|reconcileJournal|findLiveProxy|readPid|removePid' src/cli/index.tsRepository: lidge-jun/opencodex
Length of output: 41991
Apply the ownership gate to handleEnsure before merging.
src/cli/index.ts:444 calls reconcileJournal() before handleEnsure checks for a live proxy at line 450. When another healthy proxy already owns the lifecycle, ocx ensure can restore the stale journal and overwrite the active proxy's Codex configuration/profile. Apply the same PID existence/live-proxy/stale-PID cleanup sequence used in src/cli/index.ts:225-237, share it for both startup paths, and add an ensure regression case.
🤖 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/cli/index.ts` around lines 234 - 237, Move the ownership gate currently
guarding reconcileJournal near the startup path into a shared helper, including
PID existence, live-proxy detection, and stale-PID cleanup. Invoke this helper
before reconcileJournal in both the startup flow and handleEnsure, so a healthy
active proxy prevents journal merging while stale ownership is cleaned up first;
add an ensure regression case covering this behavior.
| describe("handleStart journal ownership ordering (#1230)", () => { | ||
| test("a healthy PID-file proxy is detected before journal reconciliation", () => { | ||
| const handleStart = handleStartSource(); | ||
| const readPid = handleStart.indexOf("const existingPid = readPid();"); | ||
| const findLive = handleStart.indexOf("const live = await findLiveProxy();", readPid); | ||
| const healthyExit = handleStart.indexOf("process.exit(1);", findLive); | ||
| const removeStalePid = handleStart.indexOf("removePid(existingPid);", healthyExit); | ||
| const reconcile = handleStart.indexOf("reconcileJournal();", removeStalePid); | ||
| const updatePrompt = handleStart.indexOf("await maybeShowUpdatePrompt();", reconcile); | ||
|
|
||
| expect(readPid).toBeGreaterThanOrEqual(0); | ||
| expect(findLive).toBeGreaterThan(readPid); | ||
| expect(healthyExit).toBeGreaterThan(findLive); | ||
| expect(removeStalePid).toBeGreaterThan(healthyExit); | ||
| expect(reconcile).toBeGreaterThan(removeStalePid); | ||
| expect(updatePrompt).toBeGreaterThan(reconcile); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Replace the source-order assertion with behavioral coverage.
This test compares string offsets only. It never runs handleStart, writes a PID or journal, starts a healthy proxy, or verifies Codex state. A broken implementation can still satisfy Lines [18-30]. Moving reconcileJournal() into a helper also makes the test enforce source layout instead of behavior. The test cannot detect the handleEnsure gap.
Add two Bun regression tests:
- For a healthy owner, use isolated
OPENCODEX_HOMEandCODEX_HOME, seed a dead journal PID, run a separate healthy proxy, invokeocx start, and assert that managed Codex configuration, catalog, journal, profile, and history remain unchanged. - For a dead owner with no listener, use the same isolated state without a running proxy and assert that journal reconciliation restores the expected state.
Also cover ocx ensure if it uses the shared startup lifecycle gate.
As per path instructions, runtime behavior changes require focused regression coverage in tests/**. The PR objective also requires healthy-owner and dead-owner/no-listener cases.
🤖 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/cli-start-journal-order.test.ts` around lines 15 - 30, Replace the
source-offset assertions in the journal ownership ordering test with Bun
behavioral regression tests that invoke the actual CLI lifecycle. In the
healthy-owner case, isolate OPENCODEX_HOME and CODEX_HOME, seed a dead journal
PID, run a healthy proxy, execute ocx start, and verify managed Codex
configuration, catalog, journal, profile, and history remain unchanged; in the
dead-owner/no-listener case, use the isolated state without a proxy and verify
reconciliation restores the expected state. Add equivalent ocx ensure coverage
when it uses the shared startup gate, using existing test helpers and symbols
rather than asserting source layout.
Source: Path instructions
2 participants
Summary
ocx start.Closes #1230
Verification
taskset -c 0-1 bun test tests/cli-start-journal-order.test.ts tests/codex-journal.test.ts tests/proxy-liveness.test.ts tests/stale-state-purge.test.ts— 105 passed.taskset -c 0-1 bun run typecheck— passed.taskset -c 0-1 bun run privacy:scan— passed.taskset -c 0-1 bun run test— 9,765 passed and 10 skipped; the local all-suite run also retained unrelated baseline/environment failures in the Codex shim token fixture, Live sideband close timing, and GUI dependency setup. The focused lifecycle suites above are green; exact-head CI remains authoritative.Checklist
Summary by CodeRabbit
Bug Fixes
Tests