Skip to content

Fix: fix(hooks): GateGuard permanently blocks all operations on Windows — fs.renameSync EEXIST not handled #1444

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shahyashish wants to merge 1 commit into
base: main
Choose a base branch
from
+5 −2
Open

Fix: fix(hooks): GateGuard permanently blocks all operations on Windows — fs.renameSync EEXIST not handled #1444

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions scripts/hooks/gateguard-fact-force.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ const fs = require('fs');
const path = require('path');

// Session state — scoped per session to avoid cross-session races.
// Uses CLAUDE_SESSION_ID (set by Claude Code) or falls back to PID-based isolation.
// Uses CLAUDE_SESSION_ID (set by Claude Code) or falls back to stable CWD-based isolation.
const STATE_DIR = process.env.GATEGUARD_STATE_DIR || path.join(process.env.HOME || process.env.USERPROFILE || '/tmp', '.gateguard');
const SESSION_ID = process.env.CLAUDE_SESSION_ID || process.env.ECC_SESSION_ID || `pid-${process.ppid || process.pid}`;
const SESSION_ID = process.env.CLAUDE_SESSION_ID || process.env.ECC_SESSION_ID || `cwd-${crypto.createHash('sha256').update(process.cwd()).digest('hex').slice(0, 16)}`;
const STATE_FILE = path.join(STATE_DIR, `state-${SESSION_ID.replace(/[^a-zA-Z0-9_-]/g, '_')}.json`);

// State expires after 30 minutes of inactivity
Expand Down Expand Up @@ -82,6 +82,9 @@ function saveState(state) {
// Atomic write: temp file + rename prevents partial reads
const tmpFile = STATE_FILE + '.tmp.' + process.pid;
fs.writeFileSync(tmpFile, JSON.stringify(state, null, 2), 'utf8');
// On Windows, fs.renameSync throws EEXIST if the destination exists.
// Unlink first to ensure cross-platform replacement.
try { fs.unlinkSync(STATE_FILE); } catch (_) { /* ignore if not exists */ }
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Apr 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Unconditional pre-delete before rename creates a non-atomic write window and can silently lose persisted state if rename fails.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/hooks/gateguard-fact-force.js, line 87:

<comment>Unconditional pre-delete before rename creates a non-atomic write window and can silently lose persisted state if rename fails.</comment>

<file context>
@@ -82,6 +82,9 @@ function saveState(state) {
     fs.writeFileSync(tmpFile, JSON.stringify(state, null, 2), 'utf8');
+    // On Windows, fs.renameSync throws EEXIST if the destination exists.
+    // Unlink first to ensure cross-platform replacement.
+    try { fs.unlinkSync(STATE_FILE); } catch (_) { /* ignore if not exists */ }
     fs.renameSync(tmpFile, STATE_FILE);
   } catch (_) { /* ignore */ }
</file context>
Fix with Cubic

fs.renameSync(tmpFile, STATE_FILE);
Comment on lines +87 to 88
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unlink-before-rename breaks atomic write semantics

Always unlinking first creates a window between line 87 and 88 where STATE_FILE doesn't exist. Any concurrent loadState() call in that window returns empty state (checked: []), causing isChecked to return false and the gate to fire spuriously. The comment above still says "Atomic write" but the write is no longer atomic.

A safer pattern catches EEXIST specifically, so the unlink only happens when the destination actually exists — preserving the atomic case on the happy path:

Suggested change
try { fs.unlinkSync(STATE_FILE); } catch (_) { /* ignore if not exists */ }
fs.renameSync(tmpFile, STATE_FILE);
try {
fs.renameSync(tmpFile, STATE_FILE);
} catch (err) {
if (err.code === 'EEXIST') {
try { fs.unlinkSync(STATE_FILE); } catch (_) { /* ignore */ }
fs.renameSync(tmpFile, STATE_FILE);
}
}

The worst-case failure mode here is still a spurious re-gate (safe direction), but reducing the frequency matters for stability on async hook runs.

} catch (_) { /* ignore */ }
Comment on lines 83 to 89
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Orphaned temp files not pruned

If renameSync throws after unlinkSync succeeds (e.g., an antivirus tool holds a lock on tmpFile), the outer catch (_) { /* ignore */ } silently swallows the error and tmpFile (state-...json.tmp.<PID>) is left on disk permanently.

pruneStaleFiles skips it because the filter requires f.endsWith('.json') — files ending in .json.tmp.<PID> don't match. Consider extending the prune loop to also remove stale .tmp.* files:

// in pruneStaleFiles, add alongside the existing state-*.json cleanup:
if (f.includes('.tmp.') && now - stat.mtimeMs > SESSION_TIMEOUT_MS) {
  try { fs.unlinkSync(path.join(STATE_DIR, f)); } catch (_) {}
}

Comment on lines +85 to 89
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In current Node.js LTS on Windows, what are the documented fs.renameSync behaviors when the destination already exists (including EEXIST/EPERM), and what cross-platform pattern is recommended to preserve atomic file replacement semantics?

💡 Result:

In current Node.js LTS versions (v24 Active LTS "Krypton" and v22/v20 Maintenance LTS as of 2026-04-15), fs.renameSync(oldPath, newPath) behavior when the destination already exists is platform-dependent, emulating POSIX rename(2) semantics via libuv. On POSIX (Linux/macOS): Overwrites the destination if it's a file (atomically replaces it). If destination is an existing non-empty directory, throws ENOTEMPTY. If empty directory, may succeed (replaces it). On Windows: Intended to overwrite existing files using MoveFileEx with MOVEFILE_REPLACE_EXISTING flag in libuv. However, frequently throws EPERM ("operation not permitted") or EBUSY, especially if files are locked by antivirus/indexer/Windows Search, or under concurrent access/high load. EEXIST may occur if replacement fails. Retries (as in graceful-fs) are a common workaround, but not built into core fs. Official Node.js docs (fs.html) do not explicitly document per-platform differences or error codes for existing destinations, referencing POSIX rename(2) generally. Issues like nodejs/node#29481 and libuv discussions confirm Windows unreliability for atomic replacement via rename. Cross-platform pattern for preserving atomic file replacement: Write new content to a temporary file (e.g., destination + unique suffix like '.' + hash/process.pid), then fs.renameSync(tempPath, destination). This leverages rename's atomicity within the same filesystem. If destination exists, rename will attempt replacement (POSIX: succeeds; Windows: may fail with EPERM but often succeeds if temp is new). Popular libraries like write-file-atomic implement this with retries, fsync, chown/mode preservation, handling Windows quirks for reliability. Example: const fs = require('node:fs'); const path = require('node:path'); const crypto = require('node:crypto'); function atomicWriteSync(dest, data) { const tmp = dest + '.' + crypto.createHash('md5').update(dest + process.pid + Date.now).digest('hex'); fs.writeFileSync(tmp, data); fs.renameSync(tmp, dest); // Atomic replace } Use libraries like 'write-file-atomic' for production (handles edge cases, retries EPERM on Windows).

Citations:

Remove unconditional unlink and handle state-write errors explicitly.

Line 87 unconditionally deletes STATE_FILE before rename, creating a no-file gap where concurrent reads may find no file during the replacement. Additionally, line 89 silently swallows all errors from the rename, so if the operation fails, state may never persist and the gate can continue re-blocking incorrectly.

On POSIX systems (Linux, macOS), fs.renameSync() atomically replaces an existing file. On Windows, it may fail with EPERM due to file locks; conditional handling is needed only in that case.

Suggested approach

Attempt fs.renameSync() first on all platforms. Only on Windows, if it fails with EEXIST or EPERM, unlink the destination and retry. Wrap the entire operation in explicit error handling to fail fast and prevent silent state corruption.

Per the coding guidelines: "Always handle errors explicitly at every level and never silently swallow errors."

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@scripts/hooks/gateguard-fact-force.js` around lines 85 - 89, Remove the
unconditional fs.unlinkSync(STATE_FILE) and change the tmpFile -> STATE_FILE
replace logic to try fs.renameSync(tmpFile, STATE_FILE) first; on failure only
handle Windows-specific errors (check error.code === 'EEXIST' || error.code ===
'EPERM') by calling fs.unlinkSync(STATE_FILE) and retrying the rename, and for
any other error or a second failure, surface the error (throw or log and exit)
instead of swallowing it; update the code paths that reference STATE_FILE,
tmpFile, fs.renameSync and fs.unlinkSync so failures are handled explicitly and
fail fast.

}
Expand Down