fix: make write-existing-file-guard read-gated with overwrite bypass#1952
fix: make write-existing-file-guard read-gated with overwrite bypass#1952gustavosmendes wants to merge 3 commits intocode-yeongyu:devfrom
Conversation
|
All contributors have signed the CLA. Thank you! ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
![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: 84ead95491
ℹ️ 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".
There was a problem hiding this comment.
Lowercasing every path on darwin/win32 assumes the filesystem is case-insensitive, but both platforms can run case-sensitive volumes (e.g. APFS case-sensitive or Windows per-directory case sensitivity). In those environments, toLowerCase() collapses distinct files (Foo.txt vs foo.txt) into the same permission key, so a read of one file can incorrectly authorize an overwrite of another and bypass the write guard. This can cause unintended file modification when users work on case-sensitive repos from macOS/Windows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR refactors the write-existing-file-guard hook to address token waste issues (#1871) by implementing a read-gated overwrite policy. Instead of unconditionally blocking all writes to existing files, the hook now allows writes after a same-session read (single-use), with explicit overwrite: true bypass support, and implements cross-session invalidation to prevent race conditions.
Changes:
- Replaces unconditional write blocking with a read-permission-based gate that allows one write per read
- Adds support for explicit
overwrite: true/overwrite: "true"flag to bypass the guard - Implements path canonicalization using
realpathSync.native()to handle symlinks and case-insensitive platforms - Adds LRU-based session eviction (256 session limit) and cross-session read permission invalidation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 17 comments.
| File | Description |
|---|---|
| src/hooks/write-existing-file-guard/hook.ts | Core implementation of read-gated write logic with session tracking, path canonicalization, and overwrite flag handling |
| src/hooks/write-existing-file-guard/index.test.ts | Comprehensive test suite covering 14 scenarios including read gates, cross-session behavior, path variants, symlinks, case handling, and LRU eviction |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
In the symlink test, if symlinkSync fails (line 271-273), the test silently returns without any assertion. This means the test passes even if symlinks aren't supported on the platform, giving a false positive. Consider using test.skip() or at least adding a console message to indicate the test was skipped due to platform limitations, so that test results accurately reflect what was actually tested.
| } catch { | |
| } catch (error) { | |
| console.warn( | |
| "Skipping symlink test: symlinks are not supported or cannot be created in this environment.", | |
| error | |
| ) |
| }) | ||
| expect(output.args.overwrite).toBeUndefined() | ||
| }) | ||
|
|
There was a problem hiding this comment.
Missing test coverage for overwrite: false: The tests verify that overwrite: true and overwrite: "true" bypass the guard, but there's no test for overwrite: false or overwrite: "false" to ensure they don't bypass the guard. Add a test case that verifies setting overwrite: false (or other falsy values) still blocks writes to existing files without a read permission.
| test("#given overwrite falsy values #when write executes #then does not bypass guard", async () => { | |
| const existingFile = createFile("overwrite-falsy.txt") | |
| for (const overwrite of [false, "false"] as const) { | |
| await expect( | |
| invoke({ | |
| tool: "write", | |
| outputArgs: { | |
| filePath: existingFile, | |
| content: "new content", | |
| overwrite, | |
| }, | |
| }) | |
| ).rejects.toThrow(BLOCK_MESSAGE) | |
| } | |
| }) |
| if (!existsSync(resolvedPath) || !input.sessionID) { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential issue with read handling: The code only registers read permissions for existing files (line 158). This means if an agent reads a non-existent file (which would fail), then creates and tries to write it, the write will be blocked even though a read was attempted. While this might be the intended behavior (only successful reads grant write permission), it could be confusing. Consider documenting this behavior or logging when reads of non-existent files are skipped.
| if (!existsSync(resolvedPath) || !input.sessionID) { | |
| return | |
| } | |
| if (!existsSync(resolvedPath)) { | |
| log("[write-existing-file-guard] Skipping read permission registration for non-existent file", { | |
| sessionID: input.sessionID, | |
| filePath, | |
| resolvedPath, | |
| }) | |
| return | |
| } | |
| if (!input.sessionID) { | |
| return | |
| } |
| const consumeReadPermission = (sessionID: string, canonicalPath: string): boolean => { | ||
| const readSet = readPermissionsBySession.get(sessionID) | ||
| if (!readSet || !readSet.has(canonicalPath)) { | ||
| return false | ||
| } | ||
|
|
||
| readSet.delete(canonicalPath) | ||
| touchSession(sessionID) | ||
| return true | ||
| } |
There was a problem hiding this comment.
Race condition: The hook tracks read permissions per session but doesn't handle concurrent writes from the same session. If two write operations from the same session are in flight simultaneously after a read, both could consume the same read permission (the first check at line 196 might pass for both before either deletes the permission at line 123). While this is unlikely in typical LLM usage patterns, it violates the stated "single-use read gate" guarantee and could allow unintended overwrites.
Consider using atomic operations or ensuring the permission check and deletion happen atomically, or document that concurrent writes from the same session are not supported.
| }) | ||
|
|
||
| throw new Error("File already exists. Use edit tool instead.") | ||
| }, |
There was a problem hiding this comment.
Missing session cleanup: The hook does not implement an event handler for session.deleted events to clean up session-scoped state. This is a consistent pattern across other hooks in the codebase (e.g., context-window-monitor.ts:85-90, session-notification.ts:101). Without this cleanup, deleted sessions will remain in readPermissionsBySession and sessionLastAccess indefinitely, causing a memory leak. The LRU eviction only triggers when the session count exceeds MAX_TRACKED_SESSIONS, which may never happen if sessions are deleted before reaching that threshold.
Add an event handler that removes the session from both Maps when session.deleted events are received.
| }, | |
| }, | |
| "session.deleted": async (event) => { | |
| const sessionID = (event as { sessionID?: string }).sessionID | |
| if (!sessionID) { | |
| return | |
| } | |
| readPermissionsBySession.delete(sessionID) | |
| sessionLastAccess.delete(sessionID) | |
| }, |
| const isSisyphusPath = canonicalPath.startsWith(sisyphusRoot) | ||
| if (isSisyphusPath) { | ||
| log("[write-existing-file-guard] Allowing .sisyphus/** overwrite", { | ||
| sessionID: input.sessionID, | ||
| filePath, | ||
| }) | ||
| invalidateOtherSessions(canonicalPath, input.sessionID) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
The PR description states "allow one overwrite after same-session read", but the .sisyphus/** exception (lines 176-184) allows unlimited writes without consuming any read permission. This is inconsistent with the single-use read gate principle and could allow data loss scenarios where the agent repeatedly overwrites .sisyphus files without re-reading them. The original implementation only allowed .sisyphus/*.md files, but this broadens it to all files under .sisyphus/. Consider whether this broader exception is intentional or if it should still require read permissions for non-.md files, or at least document why .sisyphus files have unlimited write access.
| const isSisyphusPath = canonicalPath.startsWith(sisyphusRoot) | |
| if (isSisyphusPath) { | |
| log("[write-existing-file-guard] Allowing .sisyphus/** overwrite", { | |
| sessionID: input.sessionID, | |
| filePath, | |
| }) | |
| invalidateOtherSessions(canonicalPath, input.sessionID) | |
| return | |
| } |
|
|
||
| const overwriteEnabled = isOverwriteEnabled(args?.overwrite) | ||
|
|
||
| if (argsRecord && "overwrite" in argsRecord) { |
There was a problem hiding this comment.
The hook modifies the output.args object by deleting the overwrite property (line 169). This mutation happens on the object passed to the hook, which means the property is removed before it reaches the actual tool implementation. While this is the intended behavior according to the PR description ("strip overwrite before tool execution"), this side effect should be clearly documented in code comments, as it's not obvious that a hook would mutate the args object in this way.
| if (argsRecord && "overwrite" in argsRecord) { | |
| if (argsRecord && "overwrite" in argsRecord) { | |
| // Intentionally mutate the args object: strip the overwrite flag so that it | |
| // is not visible to the underlying tool implementation during execution. |
| } | ||
|
|
||
| function resolveInputPath(ctx: PluginInput, inputPath: string): string { | ||
| return normalize(isAbsolute(inputPath) ? inputPath : resolve(ctx.directory, inputPath)) |
There was a problem hiding this comment.
The resolveInputPath function calls normalize() on the result (line 30), but normalize() doesn't resolve symlinks or handle case-sensitivity. The canonical path resolution via toCanonicalPath() happens separately later (line 155). This means the .sisyphus root comparison (line 72) uses the canonical path, but if ctx.directory itself is provided as a non-canonical path during hook initialization and doesn't exist yet, the sisyphusRoot might not match files correctly. Consider canonicalizing ctx.directory more defensively in the initialization, with appropriate error handling if it doesn't exist.
| return normalize(isAbsolute(inputPath) ? inputPath : resolve(ctx.directory, inputPath)) | |
| const absolutePath = isAbsolute(inputPath) ? inputPath : resolve(ctx.directory, inputPath) | |
| return toCanonicalPath(absolutePath) |
| if (toolName !== "write" && toolName !== "read") { | ||
| return | ||
| } |
There was a problem hiding this comment.
Missing test coverage for non-write/read tools: The hook now processes both "write" and "read" tools (line 143), but there's no test verifying that other tools (e.g., "edit", "bash") are properly ignored and don't affect the read permission state. The original tests had coverage for this ("ignores non-write tools"), but it's missing in the new test suite. Add a test to verify tools other than read/write don't interfere with the guard's state.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 3/5
- Unconditionally lowercasing paths on darwin/win32 in
src/hooks/write-existing-file-guard/hook.tscan bypass case-sensitive filesystem protections, which is a concrete security risk on supported case-sensitive volumes. - The score reflects a medium-severity, security-relevant issue in a core path-handling hook; impact could be user-facing if running on case-sensitive setups.
- Pay close attention to
src/hooks/write-existing-file-guard/hook.ts- path normalization may break case-sensitive filesystem semantics.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/hooks/write-existing-file-guard/hook.ts">
<violation number="1" location="src/hooks/write-existing-file-guard/hook.ts:50">
P2: Unconditionally lowercasing paths on darwin/win32 can cause security issues on case-sensitive volumes. Both macOS (APFS case-sensitive) and Windows (per-directory case sensitivity) support case-sensitive filesystems where `Foo.txt` and `foo.txt` are distinct files. This code collapses them into the same permission key, allowing a read of one file to incorrectly authorize an overwrite of another, bypassing the write guard. Consider using the actual filesystem case behavior (e.g., via `realpathSync.native()` which already resolves to the canonical case) rather than forcing lowercase.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
gustavosmendes
force-pushed
the
codex/fix-write-existing-file-guard-1871
branch
from
84ead95 to
b6c433d
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Forward session.deleted events to write-existing-file-guard so per-session read permissions are actually cleared in runtime. Add plugin-level regression test to ensure event forwarding remains wired, alongside the expanded guard behavior and unit coverage.
1 participant
Summary
overwrite: true/overwrite: "true"bypass and stripoverwritebefore tool executionrealpathSync.native()to handle symlinks and case-insensitive platformsTests
bun test src/hooks/write-existing-file-guard/index.test.tsbun run buildCloses #1871
Summary by cubic
Replaced the unconditional existing-file write block with a read-gated overwrite policy. Addresses #1871 with one-time overwrite after same-session read, explicit bypass, robust path handling, and blocking writes outside the session directory.
Written for commit 73d9e1f. Summary will update on new commits.