feat: guard write tool from overwriting existing files#1476
Conversation
|
Related to #1474 - Thank you @kaizen403 for the inspiration! Your work on protecting plan files from overwrites motivated this more general hook that guards all existing files when using the Write tool. Once both PRs are merged, we'll have comprehensive protection:
|
|
This PR provides a more fundamental solution to the problem addressed in #1474. While #1474 adds plan-specific protection in the prometheus-md-only hook, this PR implements a general-purpose hook that guards all existing files from being overwritten by the Write tool - addressing the root cause at the tool level. Once this is merged, #1474 can be closed as this provides broader coverage. @kaizen403 Thank you so much for identifying this issue and your contribution! Your work helped highlight the need for this protection. 🙏 |
There was a problem hiding this comment.
2 issues found across 7 files
Confidence score: 3/5
- There is a concrete bypass risk:
src/hooks/write-existing-file-guard/index.tsonly checksfilePath, while other tools usepath/file_path, so the guard may not trigger and existing files could be overwritten. - Relative path handling in
src/hooks/write-existing-file-guard/index.test.tsrelies onfs.existsSync(filePath)withprocess.cwd(), which can diverge fromctx.directoryand allow incorrect existence checks. - Score reflects medium severity, user-impacting guard failures but no evidence of broader breakage.
- Pay close attention to
src/hooks/write-existing-file-guard/index.ts,src/hooks/write-existing-file-guard/index.test.ts- guard may be bypassed due to argument naming and CWD mismatch.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/hooks/write-existing-file-guard/index.test.ts">
<violation number="1" location="src/hooks/write-existing-file-guard/index.test.ts:8">
P1: The hook implementation relies on `fs.existsSync(filePath)` which resolves relative paths against `process.cwd()`. This is unsafe as the agent's working directory (`ctx.directory`) often differs from the process CWD (e.g. in VSCode workspaces or monorepos).
The hook factory should be updated to accept `ctx` and use `path.resolve(ctx.directory, filePath)`.
This test instantiation should then be updated to pass a mock context with `tempDir`.</violation>
</file>
<file name="src/hooks/write-existing-file-guard/index.ts">
<violation number="1" location="src/hooks/write-existing-file-guard/index.ts:13">
P2: The hook only checks for `filePath`, but the `write` tool (and others) often use `path` or `file_path` as the argument name (see `src/hooks/comment-checker/index.ts`). This could cause the guard to be bypassed if the tool uses one of those alternatives.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| import * as os from "os" | ||
|
|
||
| describe("createWriteExistingFileGuardHook", () => { | ||
| const hook = createWriteExistingFileGuardHook() |
There was a problem hiding this comment.
P1: The hook implementation relies on fs.existsSync(filePath) which resolves relative paths against process.cwd(). This is unsafe as the agent's working directory (ctx.directory) often differs from the process CWD (e.g. in VSCode workspaces or monorepos).
The hook factory should be updated to accept ctx and use path.resolve(ctx.directory, filePath).
This test instantiation should then be updated to pass a mock context with tempDir.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/write-existing-file-guard/index.test.ts, line 8:
<comment>The hook implementation relies on `fs.existsSync(filePath)` which resolves relative paths against `process.cwd()`. This is unsafe as the agent's working directory (`ctx.directory`) often differs from the process CWD (e.g. in VSCode workspaces or monorepos).
The hook factory should be updated to accept `ctx` and use `path.resolve(ctx.directory, filePath)`.
This test instantiation should then be updated to pass a mock context with `tempDir`.</comment>
<file context>
@@ -0,0 +1,87 @@
+import * as os from "os"
+
+describe("createWriteExistingFileGuardHook", () => {
+ const hook = createWriteExistingFileGuardHook()
+ let tempDir: string
+
</file context>
| return | ||
| } | ||
|
|
||
| const args = output.args as { filePath?: string } | undefined |
There was a problem hiding this comment.
P2: The hook only checks for filePath, but the write tool (and others) often use path or file_path as the argument name (see src/hooks/comment-checker/index.ts). This could cause the guard to be bypassed if the tool uses one of those alternatives.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/write-existing-file-guard/index.ts, line 13:
<comment>The hook only checks for `filePath`, but the `write` tool (and others) often use `path` or `file_path` as the argument name (see `src/hooks/comment-checker/index.ts`). This could cause the guard to be bypassed if the tool uses one of those alternatives.</comment>
<file context>
@@ -0,0 +1,29 @@
+ return
+ }
+
+ const args = output.args as { filePath?: string } | undefined
+ const filePath = args?.filePath
+ if (!filePath) {
</file context>
|
@code-yeongyu glad I could contribute :) this general approach is much better! |
28ef939 to
601ee9d
Compare
…overwriting existing files Adds a PreToolUse hook that intercepts write operations and throws an error if the target file already exists, guiding users to use the edit tool instead. - Throws error: 'File already exists. Use edit tool instead.' - Hook is enabled by default, can be disabled via disabled_hooks - Includes comprehensive test suite with BDD-style comments
601ee9d to
ddf878e
Compare
|
@cubic-dev-ai please re-review - I've addressed both issues:
|
@code-yeongyu I have started the AI code review. It will take a few minutes to complete. |
Summary
disabled_hooks: ["write-existing-file-guard"]Changes
src/hooks/write-existing-file-guard/index.ts- hook implementationsrc/hooks/write-existing-file-guard/index.test.ts- comprehensive test suite with BDD-style comments"write-existing-file-guard"toHookNameSchemainsrc/config/schema.tssrc/hooks/index.tssrc/index.tstool.execute.beforechainTest Plan
Summary by cubic
Adds a pre-execution hook that blocks the Write tool from overwriting existing files. Enabled by default to prevent accidental data loss; prompts using the Edit tool instead.
Written for commit ddf878e. Summary will update on new commits.