fix: write Codex session cache atomically#295
Open
zerone0x wants to merge 1 commit intoexospherehost:mainfrom
Open
fix: write Codex session cache atomically#295zerone0x wants to merge 1 commit intoexospherehost:mainfrom
zerone0x wants to merge 1 commit intoexospherehost:mainfrom
Conversation
📝 WalkthroughWalkthrough
ChangesAtomic Cache Writes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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 `@lib/codex-sessions.ts`:
- Around line 63-66: The read-modify-write around readCache() -> modify
cache[sessionId] -> writeFileSync(tmpPath) -> renameSync(CACHE_PATH) is racy and
can clobber concurrent updates; wrap that whole sequence in a cross-process
critical section (e.g., obtain an advisory lock on CACHE_PATH or a dedicated
lockfile with retry/backoff) so only one process reads+updates+renames at a
time, or implement an optimistic merge-retry: after writing the tmp file but
before rename, re-read CACHE_PATH (or stat+contents), merge any new keys into
your in-memory cache, and retry the write/rename loop until no concurrent
changes remain. Use the existing symbols (readCache, CACHE_PATH, tmpPath,
sessionId, writeFileSync, renameSync) to locate and protect the critical region.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dcf9bced-ad41-4db2-a05c-3725406257cf
📒 Files selected for processing (2)
__tests__/lib/codex-sessions.test.tslib/codex-sessions.ts
Comment on lines
63
to
+66
| const cache = readCache(); | ||
| cache[sessionId] = path; | ||
| writeFileSync(CACHE_PATH, JSON.stringify(cache), "utf-8"); | ||
| writeFileSync(tmpPath, JSON.stringify(cache), "utf-8"); | ||
| renameSync(tmpPath, CACHE_PATH); |
There was a problem hiding this comment.
Atomic rename here still allows concurrent lost updates
This is still a read-modify-write race: two writers can both read the same old cache, each add a different sessionId, and the last renameSync wins, dropping the other entry. Atomic replacement prevents partial JSON, but not clobbering.
Use a cross-process critical section for read+write (e.g., lock file with retry/backoff) or a merge-on-conflict retry loop after failed/changed writes so concurrent writers preserve both entries.
🤖 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 `@lib/codex-sessions.ts` around lines 63 - 66, The read-modify-write around
readCache() -> modify cache[sessionId] -> writeFileSync(tmpPath) ->
renameSync(CACHE_PATH) is racy and can clobber concurrent updates; wrap that
whole sequence in a cross-process critical section (e.g., obtain an advisory
lock on CACHE_PATH or a dedicated lockfile with retry/backoff) so only one
process reads+updates+renames at a time, or implement an optimistic merge-retry:
after writing the tmp file but before rename, re-read CACHE_PATH (or
stat+contents), merge any new keys into your in-memory cache, and retry the
write/rename loop until no concurrent changes remain. Use the existing symbols
(readCache, CACHE_PATH, tmpPath, sessionId, writeFileSync, renameSync) to locate
and protect the critical region.
8 tasks
NiveditJain
added a commit
that referenced
this pull request
May 5, 2026
…atch + complete the PR #293 audit (#297) PR #293 wired per-CLI tool-name canonicalization so builtin policies that match Claude PascalCase names (`Bash`, `Read`, `Write`, `Edit`, `Glob`, `Grep`) fire under non-Claude CLIs. The map for Copilot was incomplete: Copilot's `view` tool — used by the model both to read files and to list directory contents — was not mapped, so `block-read-outside-cwd` never fired on `view` calls. User-reported regression: with `block-read-outside-cwd` enabled under Copilot CLI, the model could still list `$HOME` via a `view` call (a single tool invocation with `tool_input: {path: "/home/user"}`) where PR #293 had only fixed the `bash ls -la` flow. Empirical confirmation in this user's local session log at `~/.copilot/session-state/.../events.jsonl`: `{"type":"tool.execution_start","data":{"toolName":"view","arguments":{"path":"/home/nivedit"}}}` against Copilot CLI 1.0.39. Auditing all seven supported CLIs against their public tool registries plus on-disk session evidence revealed three more gaps in the same class: - Copilot was missing `view`, `create`, `apply_patch`, `web_fetch`, `powershell`, `*_bash`/`*_powershell` (the eight session-management tools), `rg`, `show_file` — directory/file reads, file creation, patches, PowerShell, web fetches all bypassed policies. - Cursor (PR #293 left it as passthrough) — Cursor uses `Shell` for what Claude calls `Bash`, so every Bash builtin (`block-sudo`, `block-rm-rf`, `block-read-outside-cwd` Bash branch, …) silently no-op'd on Cursor sessions. - Codex (PR #293 left it as passthrough) — Codex hooks report `tool_name: "apply_patch"` even when matchers say `Edit`/`Write`; live sessions also expose `write_stdin` which sends input to a running shell. - OpenCode was missing `apply_patch` and `websearch`. Fix: - Extend `COPILOT_TOOL_MAP` in `src/hooks/types.ts` with the full Copilot CLI tool surface — `view`/`show_file` → `Read`, `create` → `Write`, `apply_patch` → `Edit`, `web_fetch` → `WebFetch`, `powershell` and the `*_bash`/`*_powershell` session-management tools → `Bash`, `rg` → `Grep`. - Extend `OPENCODE_TOOL_MAP` with `apply_patch` → `Edit`, `websearch` → `WebSearch`. Mirror the same entries in the OpenCode shim template at `src/hooks/integrations.ts:734` (the shim must stay self-contained — it's loaded in-process by opencode). - Add `CURSOR_TOOL_MAP` (`Shell` → `Bash`) and `CODEX_TOOL_MAP` (`apply_patch` → `Edit`, `write_stdin` → `Bash`) in `src/hooks/types.ts`, plus matching cursor/codex branches in `handler.ts:canonicalizeToolName`. `apply_patch` maps to `Edit` (not `Write`) for consistency with the existing `str_replace_editor` → `Edit` entry; the choice was confirmed via AskUserQuestion. The trade-off is documented: `Edit` preserves Claude semantics (Claude's own `Edit` tool doesn't trigger `block-write-outside-cwd` either), while `Write` would have been stricter but inconsistent with Claude. Tests: - `__tests__/hooks/handler.test.ts` — extend the existing per-Copilot canonicalization loop to cover every new entry (with `[view, Read]` listed first as the regression anchor); add new test blocks for Cursor (`Shell` → `Bash`, plus passthrough for `Read`/`Write`/`Grep`/`Delete`/`Task`/`MCP:*`) and Codex (`apply_patch` → `Edit`, `write_stdin` → `Bash`, plus passthrough for `Bash`/`mcp__*`). - `__tests__/e2e/hooks/copilot-integration.e2e.test.ts` — pinned regression test "blocks `view` of a path outside cwd under Copilot (regression for #295)" mirroring the PR #293 `ls -la` test, plus a new `CopilotPayloads.preToolUse.view` factory in `__tests__/e2e/helpers/payloads.ts`. - `__tests__/hooks/opencode-plugin-shim.test.ts` — extend the OPENCODE_TOOL_MAP coverage loop with `apply_patch` and `websearch`. Verified: `bun run test:run` → 1461 passed, `bun run test:e2e` → 291 passed (Copilot e2e went 11 → 12), `bunx tsc --noEmit` → clean. Manual repro of all three: Copilot `view /etc` denies, Cursor `Shell sudo …` denies, Codex `write_stdin` denies (canonicalized to `Bash`). Pre-existing item not in this PR: the dogfood `.opencode/plugins/failproofai.mjs` was never updated with `TOOL_NAME_MAP` after PR #293 (the template was updated but the dogfood file is hand-maintained). Production users get the correct map via the template; only contributors running OpenCode against this repo are affected. Reading or rewriting that file requires temporarily disabling the `block-read-outside-cwd` agent-settings guard — deferred to a follow-up PR. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #276
Test Plan
bun run test:run __tests__/lib/codex-sessions.test.tsbun run lintbun run test:runSummary by CodeRabbit
Bug Fixes
Tests