fix(codex): honor split SQLite state homes - #1425
Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe change adds Codex-compatible SQLite directory resolution, updates history and residue consumers to use it at call time, preserves ChangesCodex SQLite resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ServiceLauncher
participant CodexConfig
participant resolveCodexStateDbPath
participant HistoryProvider
participant SQLiteDatabase
ServiceLauncher->>ServiceLauncher: preserve CODEX_SQLITE_HOME
HistoryProvider->>resolveCodexStateDbPath: resolve state database at call time
resolveCodexStateDbPath->>CodexConfig: read sqlite_home
resolveCodexStateDbPath-->>HistoryProvider: return state_5.sqlite path
HistoryProvider->>SQLiteDatabase: read or mutate history
Possibly related PRs
Suggested labels: Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
Wibias
left a comment
There was a problem hiding this comment.
Request changes based on a full review of the current head.
Merge blocker:
resolveCodexSqliteHome() treats every config.toml read/parse failure as if no authoritative sqlite_home existed, then falls through to CODEX_SQLITE_HOME or CODEX_HOME. That is unsafe for the exact data-integrity problem this PR is fixing. ENOENT means the config is absent and fallback is valid; EACCES, EIO, ENOTDIR, or another read failure means OpenCodex does not know which database Codex selected. Falling back in that state can make history/admission/native-residue operations inspect or mutate a stale or unrelated state_5.sqlite.
Please fail closed for unreadable/indeterminate authoritative config while allowing the genuine-missing (ENOENT) case to fall through. Add regressions for at least:
- missing config -> environment fallback is allowed;
- unreadable config (for example EACCES) -> SQLite-home resolution fails closed and no fallback DB is selected.
The overall resolver centralization and service propagation look sound; I did not find a conventional credential or privilege-escalation issue. After the fix, refresh onto current dev and rerun exact-head CI.
151d27d to
3abca61
Compare
|
Addressed the fail-closed blocker on the current head
Verification on the rebased head:
@Wibias please re-review the exact head after CI completes. |
Wibias
left a comment
There was a problem hiding this comment.
Request changes after re-review of the current head 3abca61fc34059d83fb9d7861c2cee3a16bffd48.
The previous unreadable-config blocker is fixed: genuine ENOENT may fall through, while read failures such as EACCES now fail closed. There is still one remaining data-integrity edge in the same contract.
resolveCodexSqliteHome() uses readRootTomlString() to discover sqlite_home. That helper only recognises a quoted string assignment and returns null otherwise. As a result, an existing authoritative config such as:
sqlite_home = 123or a malformed sqlite_home assignment is indistinguishable from the key being absent. resolveCodexSqliteHome() then falls through to CODEX_SQLITE_HOME or CODEX_HOME, potentially selecting a different state_5.sqlite even though the config exists but its authoritative SQLite setting cannot be established.
For history mutation/admission/residue safety this should remain fail-closed. Please distinguish these states:
sqlite_homeabsent -> environment/home fallback allowed;- valid string -> use it;
sqlite_homepresent but malformed/wrong type, or otherwise indeterminate -> fail closed and do not select a fallback database.
Add a regression proving at minimum that sqlite_home = 123 with CODEX_SQLITE_HOME set throws instead of returning the environment database. Malformed-string coverage would also be useful.
After that fix, refresh onto current dev and rerun complete exact-head CI.
3abca61 to
5c15c37
Compare
|
Addressed the remaining SQLite-authority blocker and the full-suite residue regression on exact head
Verification on the final code:
The CPU-limited full-suite attempt reached 10,929 passed / 11 skipped. Its three resolver-related failures are the residue cases fixed and rerun green above; the one remaining @Wibias please re-review this exact head after CI completes. |
Wibias
left a comment
There was a problem hiding this comment.
Approving. Follow-ups on this head fail closed for unreadable/invalid sqlite_home, keep Codex-compatible SQLite home precedence, and preserve the read-only native-residue indeterminate boundary instead of selecting another database.
|
Thanks @Ingwannu — this was useful because split-home Codex installs (especially Windows Desktop + WSL) were letting history, admission, and residue checks talk to a different Merging now. |
Summary
sqlite_homein the effectiveconfig.toml, thenCODEX_SQLITE_HOME, then the effectiveCODEX_HOMEstate_5.sqlitesqlite_home; only an absent file or absent key may use the environment/home fallbackconfigindeterminate result instead of throwing or selecting another databaseRoot cause
OpenCodex rebuilt
CODEX_HOME/state_5.sqliteindependently in several paths. Codex can place SQLite thread state in another root, so a Windows Desktop + WSL installation could make the history writer, admission snapshot, and residue check reason about a database different from the one Codex actually opened.The initial resolver also used the tolerant root-string reader. That made a present invalid
sqlite_homeindistinguishable from an absent key and allowed an unsafe fallback. The strict resolver now distinguishes absent, valid, and invalid states without changing tolerant config readers used by unrelated injection/catalog paths.The fix centralizes one call-time resolver and passes the selected database identity through the existing history job/worker boundary. It deliberately does not give storage cleanup authority over an external SQLite root.
User impact
History migration and provider synchronization now follow the active Codex SQLite database in split-home installations. Background services retain the explicit install-time SQLite destination instead of silently falling back to
CODEX_HOME. Invalid or unreadable SQLite authority stops the operation rather than risking inspection or mutation of a stale database.Verification
bun run typecheck: passedbun run privacy:scan: passedbun install --frozen-lockfileand production build: 221 pagescodex-shimenvironment-token failure reproduces on cleandevgit diff --check: passedea2895046: pendingChecklist
Closes #1401
Summary by CodeRabbit
New Features
Bug Fixes
Documentation