Port: port/pr-33-fix/copyfile-efs-wsl2

[briansumma]

Ported from upstream cytostack/openwolf. Needs review before merge.

[briansumma]

Now I have everything I need. Let me analyze the diff thoroughly.

---

PR #9 Code Review: port/pr-33-fix/copyfile-efs-wsl2

State: DRAFT | Additions: 30 | Deletions: 14

---

Overview

This PR ports a fix from upstream cytostack/openwolf that replaces all calls to fs.copyFileSync with a new safeCopyFile wrapper in src/utils/fs-safe.ts. The motivation is a known libuv/9P bug where copy_file_range fails with EPERM on EFS-encrypted directories accessed through WSL2's 9P filesystem layer. The fix uses a plain readFileSync + writeFileSync approach to bypass the problematic syscall.

The change touches three files: src/utils/fs-safe.ts (new function), src/cli/init.ts (4 callsite swaps), src/cli/update.ts (8 callsite swaps).

---

🔴 Critical Issues

CR-01: Non-atomic write is inconsistent with the rest of fs-safe.ts and risks hook script corruption

File: src/utils/fs-safe.ts:74

Every other write function in this file (writeJSON, writeText) uses a temp-file + rename pattern to ensure atomicity. safeCopyFile writes directly to the destination:

// All other writes in this file:
const tmp = filePath + "." + crypto.randomBytes(4).toString("hex") + ".tmp";
fs.writeFileSync(tmp, content, "utf-8");
fs.renameSync(tmp, filePath);  // atomic on POSIX

// safeCopyFile (this PR):
fs.writeFileSync(dest, fs.readFileSync(src));  // NOT atomic

If the process is interrupted mid-copy (SIGTERM, disk full, power loss), the destination is left partially written. This is especially dangerous for hook scripts in .wolf/hooks/ — Claude Code executes these directly, so a half-written hook could cause silent failures or corrupted behavior. The original fs.copyFileSync also lacked this protection, but since this code is being added to fs-safe.ts — a module explicitly designed with safer write patterns — it should follow the same convention.

Fix:

export function safeCopyFile(src: string, dest: string): void {
  const dir = path.dirname(dest);
  if (!fs.existsSync(dir)) {
    fs.mkdirSync(dir, { recursive: true });
  }
  // Use temp+rename for atomicity (matches writeJSON/writeText pattern in this file).
  // readFileSync without encoding returns a Buffer — correct for binary files.
  const tmp = dest + "." + crypto.randomBytes(4).toString("hex") + ".tmp";
  try {
    fs.writeFileSync(tmp, fs.readFileSync(src));
    fs.renameSync(tmp, dest);
  } catch (err) {
    try { fs.unlinkSync(tmp); } catch {}
    throw err;
  }
  try {
    fs.chmodSync(dest, fs.statSync(src).mode);
    // chmod may fail on Windows or 9P mounts — non-fatal
  } catch {}
}

Note: crypto is already imported in fs-safe.ts.

---

🟡 Warnings

WR-01: Empty catch {} has no explanatory comment

File: src/utils/fs-safe.ts:75–77

try {
  fs.chmodSync(dest, fs.statSync(src).mode);
} catch {}  // ← no comment

Every other silent catch in this file has a comment explaining why the failure is acceptable (e.g., // On Windows, rename can fail if another process holds a handle.). This one is bare. Without context, a future reader can't tell if this is intentional or a lazy oversight.

Fix: Add a comment:

try {
  fs.chmodSync(dest, fs.statSync(src).mode);
  // chmod may fail on Windows (permissions model differs) or on WSL2 9P mounts — non-fatal
} catch {}

---

WR-02: safeCopyFile silently creates the destination directory, changing fs.copyFileSync semantics

File: src/utils/fs-safe.ts:70–73

const dir = path.dirname(dest);
if (!fs.existsSync(dir)) {
  fs.mkdirSync(dir, { recursive: true });
}

The docstring says "drop-in replacement for fs.copyFileSync", but fs.copyFileSync throws if the destination directory doesn't exist. This silent directory creation masks future call-site bugs where a caller forgets to call ensureDir() first. All current call sites already ensure dirs exist before calling, so this is dead logic today — and it changes the contract in a way the docstring doesn't document.

Fix: Either remove the ensureDir block (keep the same semantics as fs.copyFileSync) and rely on callers to prepare directories as they already do, or update the docstring to explicitly note the behavior difference:

// Drop-in replacement for fs.copyFileSync, with two differences:
// 1. Uses plain read+write to bypass copy_file_range (EFS/WSL2 EPERM workaround)
// 2. Silently creates the destination directory if it doesn't exist

---

✅ Things Done Well

• Binary file handling is correct. readFileSync(src) without an encoding argument returns a Buffer, and writeFileSync(dest, buffer) writes it faithfully — hooks are compiled .js files, so this matters.
• Permissions preservation via statSync(src).mode is a thoughtful touch — the original fs.copyFileSync preserves permissions by default, and this keeps that behavior.
• All 12 call sites are consistently updated — no fs.copyFileSync calls were missed in the two affected files.
• The root-cause comment in the function header is excellent and precise.
• Call sites are all guarded with existsSync/statSync.isFile() checks before calling, so the function won't receive a non-existent source path.

---

Summary

Severity	Count	Issue
🔴 Critical	1	Non-atomic write risks partial/corrupt destination files
🟡 Warning	2	Undocumented empty catch; silent dir creation changes stated contract

The core fix (using read+write instead of copy_file_range) is correct and necessary. The main concern before merging is aligning safeCopyFile with the atomic write pattern already established in fs-safe.ts, particularly for hook scripts that Claude Code runs directly.

[briansumma]

Autofixer skipped: merge conflicts with main detected.

[briansumma]

Autofixer skipped: merge conflicts with main detected.

[briansumma]

Autofixer skipped: merge conflicts with main detected.

[briansumma]

Autofixer skipped: merge conflicts with main detected.

[briansumma]

Autofixer skipped: merge conflicts with main detected.

[briansumma]

✅ Autofixer applied. Build and tests passed. Promoting to Ready for Review.