fix(codex): stop reporting every history failure as a Codex DB lock - #1202
Conversation
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughHistory jobs now preserve specific failure reasons and report them in apply, restore, and recovery flows. Path validation compares platform-aware path identities. The doctor command probes the history coordinator namespace. Proxy shutdown now detects foreign ownership and avoids shared-state teardown. ChangesHistory reliability
Proxy lifecycle safety
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![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: 89b8503409
ℹ️ 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".
634dba4 to
d18aa52
Compare
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/codex/inject.ts`:
- Around line 1625-1631: Update formatApplyHistoryFailure so the deferred
headline is selected for both blocked outcomes with reason "busy" and failed
outcomes whose historyFailureReason is "busy"; retain the existing skipped and
not-changed headlines for other cases. Add a regression test covering the failed
busy apply outcome and asserting the deferred wording.
🪄 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: ef9ffa40-10ce-47ee-b3ca-71371a8c05e7
📒 Files selected for processing (4)
src/cli/index.tssrc/codex/history-job.tssrc/codex/inject.tstests/codex-history-job.test.ts
304e5b8 to
58746f6
Compare
|
|
|
|
|
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d4d533ed2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/doctor.ts`:
- Around line 911-917: The coordinator probe in the doctor flow must remain
read-only: update resolveCodexHistorySerializationDatabasePath or introduce a
separate pure path resolver so it computes and validates the namespace without
creating directories, while retaining directory initialization in the
history-operation setup path. Update the probe near resolveEffectiveUserIdentity
and getCodexHome to use the non-mutating behavior, and add a focused Bun
regression test under tests/ asserting the probe leaves the filesystem
unchanged.
In `@src/codex/history-job.ts`:
- Around line 206-209: Update redactWorkerMessage to use the same platform-aware
path normalization and case-sensitivity rules as samePathIdentity when replacing
the home directory, while preserving the existing "~" redaction and non-home
message behavior.
In `@src/codex/inject.ts`:
- Around line 1243-1249: Update the final fallback message in the reason/detail
conditional that constructs the Codex resume-history restoration error so it
names the outcome as unknown rather than blaming the Codex app. Preserve the
existing permission, busy, and detail-specific messages, changing only the
no-reason default branch.
- Around line 1261-1308: Update failedHistoryRestoreFromOutcome to delegate
non-busy and non-permission failure messages to the imported
describeHistoryJobFailure formatter, passing the outcome in each applicable
blocked and failed branch. Retain the existing failedHistoryRestore("busy") and
failedHistoryRestore("permission") handling, and preserve the desired-state
messages that are specific to this mapper.
- Around line 1545-1549: In the outcome-selection ternary, combine the `blocked`
and `failed` cases into one condition that calls
`failedHistoryRestoreFromOutcome(outcome)`. Retain `failedHistoryRestore()` as
the fallback for all other outcomes.
- Around line 1618-1639: Add focused regression tests for
formatApplyHistoryFailure and failedHistoryRestoreFromOutcome. Assert busy
blocked and failed apply outcomes use “Codex resume history migration deferred,”
while non-busy failures use “Codex resume history NOT changed”; also assert
blocked and failed restore outcomes mention “holding the history database” only
for genuine busy reasons and explicitly name other failure reasons.
🪄 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: 3714ed64-8603-4dca-a84e-c427739836a0
📒 Files selected for processing (5)
src/cli/doctor.tssrc/codex/history-job.tssrc/codex/inject.tssrc/codex/user-identity.tstests/codex-history-job.test.ts
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/doctor.ts`:
- Around line 917-918: Update the status === "missing" output in the doctor
namespace reporting flow to state only that the history coordinator namespace is
not present, removing the unsupported lifecycle claim. Add a focused regression
test under tests/ that verifies an absent namespace after prior use is reported
with the neutral wording.
🪄 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: 699dd516-eecc-4f7e-b75e-904e4ecc3645
📒 Files selected for processing (6)
src/cli/doctor.tssrc/codex/history-job.tssrc/codex/inject.tssrc/codex/user-identity.tstests/codex-inject-history-wording.test.tstests/codex-user-identity.test.ts
5bc5a3c to
44b0a04
Compare
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/index.ts (1)
712-716: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBlock shared teardown after a proxy ownership refusal.
The
stopProxycatch at Line 676 through Line 681 explicitly handles ownership refusal errors, but it does not setownershipBlocked. The new guard at Line 715 through Line 716 checks service ownership only.If a foreign proxy rejects the stop,
restoreSharedClientStateAfterStop()still restores Codex configuration and removes the Grok fence while that proxy is running. Record proxy ownership refusal in the same block flag, or skip shared teardown until ownership is verified.As per path instructions, changes under
src/**must not bypass the shared routing/config layers.🤖 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 712 - 716, Update the stopProxy ownership-refusal catch block to set the existing ownershipBlocked flag when a foreign proxy rejects shutdown, so restoreSharedClientStateAfterStop() is skipped in that case. Preserve environment rollback behavior and the existing shared routing/configuration layers.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/cli/index.ts`:
- Around line 712-716: Update the stopProxy ownership-refusal catch block to set
the existing ownershipBlocked flag when a foreign proxy rejects shutdown, so
restoreSharedClientStateAfterStop() is skipped in that case. Preserve
environment rollback behavior and the existing shared routing/configuration
layers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d87f3fc2-3eae-40ea-b5a5-1751f2ef0081
📒 Files selected for processing (3)
src/cli/index.tssrc/codex/inject.tssrc/codex/transition-state.ts
|
✅ Action performedReview finished.
|
The history Worker collapsed every non-converged outcome - a genuine SQLite busy, a permission denial, an unsafe coordinator path, an unavailable coordinator database, or a dead/timed-out Worker - into one message blaming the Codex app/IDE for holding state_5.sqlite. On Windows that reads as a false lock when nothing holds the file (issue lidge-jun#1191), and it made every other failure undiagnosable. - The H/K/N lock modules now compare the requested lock path against its realpath case-insensitively on Windows (samePathIdentity), matching the case-insensitive path identity Windows actually has, instead of refusing legitimate spellings as unsafe-path. - The Windows runtime root is canonicalized via realpath after creation, so differently-cased or junctioned LocalAppData spellings land on one lock namespace. - restoreLegacyOpenaiHistory now reports its failure reason (busy or permission) instead of a bare failed flag, so recover-history gets the same honest classification. - Apply, restore, and recover-history messages now distinguish genuine lock/busy from unsafe-path, unavailable coordinator DB, permission denial, worker error (with the real message), worker death, and worker timeout, each pointing at 'ocx doctor' when the app is not the cause.
58e72c8 to
7883173
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/codex/inject.ts (1)
640-650: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the loopback host with the loopback listener.
Lines 640-650 replace only
port. The later calls at Lines 677, 761, 767, and 803 still useconfig?.hostname. Ifhostnameis0.0.0.0, a LAN address, or another non-loopback host, Codex receives that host with the loopback-listener port.The public listener does not own that port. It can also require
x-opencodex-api-key, whileshouldInjectApiAuthHeadercorrectly suppresses that header for the unauthenticated loopback listener. Codex then cannot reach the intended listener.Set one effective injection hostname to
127.0.0.1whenunauthenticatedLoopbackListener.enabledis true. Pass it to every generated provider URL and profile URL.Proposed fix
const loopback = config?.unauthenticatedLoopbackListener; +const injectionHostname = loopback?.enabled ? "127.0.0.1" : config?.hostname; if (loopback?.enabled) port = loopback.port; - `... http://${providerBaseHost(config?.hostname)}:${port}/v1` + `... http://${providerBaseHost(injectionHostname)}:${port}/v1` - buildProviderTableBlock(port, websocketsEnabled(config ?? {}), true, config?.hostname) + buildProviderTableBlock(port, websocketsEnabled(config ?? {}), true, injectionHostname) - setRootOpenaiBaseUrl(content, port, config?.hostname) + setRootOpenaiBaseUrl(content, port, injectionHostname) - buildProfileFile(port, catalogPath, websocketsEnabled(config ?? {}), legacyMode, config?.hostname, config?.fastMode) + buildProfileFile(port, catalogPath, websocketsEnabled(config ?? {}), legacyMode, injectionHostname, config?.fastMode)🤖 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/inject.ts` around lines 640 - 650, Update the injection setup around the loopback listener to derive an effective hostname of 127.0.0.1 whenever unauthenticatedLoopbackListener.enabled is true, while preserving config?.hostname otherwise. Use this effective hostname for every generated provider and profile URL at the later injection call sites, including the flows around lines 677, 761, 767, and 803, alongside the existing effective port.Source: Path instructions
src/codex/transition-state.ts (1)
356-369: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject initialized coordinator state before applying first-use initialization.
initialize()does not accept adatabaseWasAbsentflag, butopenCodexCoordinatorTransaction()atsrc/codex/transition-state.ts:426passestruefor zero-byte files fromdatabaseWasEmpty. Ininitialize(), unsupported schema versions throw, while existing rows are left unchanged anduser_versionis only set when it is zero. A competing process can initialize a populated coordinator database while this process'slstatsaw zero bytes;databaseWasEmptythen overrides that existing state inside the transaction. Move the coordinator-state check into the transaction afterBEGIN IMMEDIATE, or prove the first-use path cannot be applied to a populated database before allowing it.🤖 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/transition-state.ts` around lines 356 - 369, Update openCodexCoordinatorTransaction and initialize so first-use initialization is decided after BEGIN IMMEDIATE using the transaction’s current coordinator state, not the earlier databaseWasEmpty lstat result. Reject unsupported or already-initialized state before applying first-use initialization, while preserving normal initialization for genuinely empty databases and avoiding overwriting existing rows or schema versions.tests/grok-lifecycle.test.ts (1)
46-56: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd runtime regression coverage for proxy ownership refusal.
These assertions inspect
CLI_SOURCEandPROCESS_CONTROL_SOURCE. They do not executestopProxyorhandleStop. They can pass even whenProxyOwnershipRefusedErrorfails theinstanceofcheck or whenrestoreSharedClientStateAfterStop()runs after the HTTP 409 path.Add a Bun test that drives the real lifecycle with a mocked 409 response. Cover both proxy-stop call sites. Assert that the command fails, the typed refusal is recognized, and shared teardown is not called. Keep the source checks as supplemental contract checks.
As per path instructions, a behavior change in
src/should come with a focused regression test near the existing tests for that subsystem.Also applies to: 58-63, 79-85, 88-101, 103-110, 112-117, 187-187
🤖 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/grok-lifecycle.test.ts` around lines 46 - 56, Add focused Bun runtime regression tests near the existing lifecycle tests that execute the real stopProxy and handleStop flows with the proxy stop request mocked to return HTTP 409. Cover both proxy-stop call sites, asserting the command fails, ProxyOwnershipRefusedError is recognized via instanceof, and restoreSharedClientStateAfterStop() is not called; retain the existing CLI_SOURCE and PROCESS_CONTROL_SOURCE assertions as supplemental checks.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/codex/inject.ts`:
- Around line 640-650: Update the injection setup around the loopback listener
to derive an effective hostname of 127.0.0.1 whenever
unauthenticatedLoopbackListener.enabled is true, while preserving
config?.hostname otherwise. Use this effective hostname for every generated
provider and profile URL at the later injection call sites, including the flows
around lines 677, 761, 767, and 803, alongside the existing effective port.
In `@src/codex/transition-state.ts`:
- Around line 356-369: Update openCodexCoordinatorTransaction and initialize so
first-use initialization is decided after BEGIN IMMEDIATE using the
transaction’s current coordinator state, not the earlier databaseWasEmpty lstat
result. Reject unsupported or already-initialized state before applying
first-use initialization, while preserving normal initialization for genuinely
empty databases and avoiding overwriting existing rows or schema versions.
In `@tests/grok-lifecycle.test.ts`:
- Around line 46-56: Add focused Bun runtime regression tests near the existing
lifecycle tests that execute the real stopProxy and handleStop flows with the
proxy stop request mocked to return HTTP 409. Cover both proxy-stop call sites,
asserting the command fails, ProxyOwnershipRefusedError is recognized via
instanceof, and restoreSharedClientStateAfterStop() is not called; retain the
existing CLI_SOURCE and PROCESS_CONTROL_SOURCE assertions as supplemental
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 436c0114-c82d-4b97-97bd-449e52d10188
📒 Files selected for processing (5)
src/cli/index.tssrc/codex/inject.tssrc/codex/transition-state.tssrc/lib/process-control.tstests/grok-lifecycle.test.ts
|
|
Records a campaign that gave a terminal disposition to every open bug issue and bug PR at the 2026-08-08 cutoff, and what it got wrong along the way. The unit carries the inventory and disposition matrix with file:line evidence per verdict, the rebase-and-co-author republish protocol, and per-work-phase implementation docs. Execution records cover the CI approval unblock, the first CI results, and the merges and closes. What the audits overturned is the more useful half. Two issues were queued for closure as resolved and were not - lidge-jun#1176 carried a maintainer comment from the same morning asking for a v2.11.0 retest, and lidge-jun#1024 rested on an upstream attribution the plan itself proposed to test and had not tested. lidge-jun#1155 was queued as an unreachable path and is reachable. lidge-jun#1263 was diagnosed as having no defect and has a real TOCTOU race, shown by contrast experiment. lidge-jun#1119 was described as fully absorbed and was not, so its coverage was recreated on dev before it was closed. Two execution faults are recorded as faults: lidge-jun#1202 was merged without checking its exact-head CI had concluded success, violating a reading rule written earlier in this same unit; and a public comment to a contributor made a false claim about stream defaulting, corrected on the PR. A transient six-failure test run is recorded as unexplained rather than dismissed as flaky, since its log was overwritten before the names could be preserved. Nothing in the build, typecheck, or test path reads from devlog/. privacy:scan passes; repo-hygiene 11 pass / 0 fail.
2 participants
Summary
Closes #1191.
ocx start,ocx sync,ocx stop, andocx restorecollapsed every non-converged history outcome into "the history DB is locked (Codex app/IDE open?)". A Windows machine with nothing holdingstate_5.sqlitestill saw the warning on every command, and any other failure (permission, unsafe path, dead worker) was equally hidden.H/K/N) now compares the requested lock path against its realpath case-insensitively on Windows viasamePathIdentity, so legitimate case differences are not refused asunsafe-path; the Windows runtime root is canonicalized through realpath before anything is keyed on it, so differently-cased or junctionedLocalAppDataspellings share one lock namespace.restoreLegacyOpenaiHistorynow reports its failure reason (busy/permission) like the other history paths, soocx recover-historyclassifies honestly too.ocx doctorinstead of blaming the Codex app.Verification
bun run typecheck— clean on the PR head.bun testfocused suites (history provider/worker/job, user identity, lock, transition, guardian, residue, convergence, update-stop, etc.) — 207 tests green.bun test tests/crash-guard.test.ts— green in isolation.bun run privacy:scan— passed.git diff --check— clean.tests/management-provider-validation.test.ts(reproduced on cleandevwithout this change), 1 flaky crash-guard 5s timeout (green in isolation), 1 pre-existing management-seam race.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.