Code Review — Request Changes

Approach is correct — build-tag file splitting is idiomatic Go for platform-specific code. Unix implementation is an exact copy of the original (good). However, the Windows implementation has 1 critical and 2 high issues that need fixing before merge.

🔴 Critical: checkHardlink is a no-op on Windows — security bypass

filesystem_windows.go:55-57

The Windows checkHardlink always returns nil. The comment says "os.FileInfo.Sys() does not expose nlink" — this is incorrect. Windows exposes hard link counts via GetFileInformationByHandle → BY_HANDLE_FILE_INFORMATION.NumberOfLinks.

Impact: All file tools (read_file, write_file, list_files) lose hardlink attack prevention on Windows. An attacker could hardlink to files outside the workspace boundary.

Fix: Use syscall.GetFileInformationByHandle (no extra dependency needed):

func checkHardlink(path string) error {
    info, err := os.Lstat(path)
    if err != nil || info.IsDir() {
        return nil
    }
    h, err := syscall.CreateFile(
        syscall.StringToUTF16Ptr(path),
        syscall.GENERIC_READ, syscall.FILE_SHARE_READ,
        nil, syscall.OPEN_EXISTING, syscall.FILE_FLAG_BACKUP_SEMANTICS, 0)
    if err != nil {
        return nil
    }
    defer syscall.CloseHandle(h)

    var fi syscall.ByHandleFileInformation
    if err := syscall.GetFileInformationByHandle(h, &fi); err != nil {
        return nil
    }
    if fi.NumberOfLinks > 1 {
        slog.Warn("security.hardlink_rejected", "path", path, "nlink", fi.NumberOfLinks)
        return fmt.Errorf("access denied: hardlinked file not allowed")
    }
    return nil
}

🟡 High: isDirWritable creates real temp files as side effect

filesystem_windows.go:47-53

isDirWritable creates a .wrchk* temp file to test writability. This is called on every resolvePath that encounters a symlink — it's a hot path.

Issues:

• Artifact leakage if os.Remove fails (antivirus lock, permissions change)
• Race condition between create and remove
• Unnecessary disk I/O for a read-path security check

Fix: Open the directory with write access instead:

func isDirWritable(dir string) bool {
    h, err := syscall.CreateFile(
        syscall.StringToUTF16Ptr(dir),
        syscall.GENERIC_WRITE, syscall.FILE_SHARE_READ|syscall.FILE_SHARE_WRITE,
        nil, syscall.OPEN_EXISTING, syscall.FILE_FLAG_BACKUP_SEMANTICS, 0)
    if err != nil {
        return false
    }
    syscall.CloseHandle(h)
    return true
}

🟡 High: Windows path reconstruction breaks on drive roots

filesystem_windows.go:17-27

For path C:\Users\foo, the split produces ["C:", "Users", "foo"]. First iteration sets current = "C:" — but in Windows path semantics, C: is a relative path (current directory on C: drive), not C:\.

Fix:

vol := filepath.VolumeName(clean)
if vol != "" {
    current = vol + string(filepath.Separator)  // "C:\"
}

Positive observations

• ✅ Correct Go build-tag pattern (//go:build !windows / //go:build windows)
• ✅ Unix code is an exact copy — no accidental changes
• ✅ PR correctly identifies the root cause (platform-specific syscall APIs)

Questions

1. Is Windows a real deployment target, or just "make it compile"? If the latter, a // TODO on the no-op checkHardlink might be acceptable, but the other two issues still apply.
2. Happy to help with the implementation if needed!