Skip to content

fix(cli): check live proxy before journal recovery #1269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Ingwannu wants to merge 1 commit into from
+36 −1
Closed

fix(cli): check live proxy before journal recovery #1269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/cli/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,6 @@ async function handleStart(options: { block?: boolean } = {}) {
const serviceToken = loadServiceTokenFromFile(process.env);
if (serviceToken) process.env.OPENCODEX_API_AUTH_TOKEN = serviceToken;
const requestedPort = parsePortOption();
if (!currentExternalCodexModelProvider()) reconcileJournal();
const existingPid = readPid();
if (existingPid) {
const live = await findLiveProxy();
Expand All @@ -232,6 +231,10 @@ async function handleStart(options: { block?: boolean } = {}) {
}
removePid(existingPid);
}
// 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();
Comment on lines +234 to +237

@coderabbitai coderabbitai Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.ts

Repository: 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.


// Interactive-only update prompt. Must run BEFORE we bind a port / write a
// PID: choosing "Update now" installs globally and exits, so we never want a
Expand Down
32 changes: 32 additions & 0 deletions tests/cli-start-journal-order.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { describe, expect, test } from "bun:test";
import { readFileSync } from "node:fs";
import { join } from "node:path";

const source = readFileSync(join(import.meta.dir, "..", "src", "cli", "index.ts"), "utf8");

function handleStartSource(): string {
const start = source.indexOf("async function handleStart(");
const end = source.indexOf("async function handleEnsure(", start);
expect(start).toBeGreaterThanOrEqual(0);
expect(end).toBeGreaterThan(start);
return source.slice(start, end);
}

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);
Comment on lines +15 to +30

@coderabbitai coderabbitai Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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_HOME and CODEX_HOME, seed a dead journal PID, run a separate healthy proxy, invoke ocx 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

});
});
Loading