Skip to content

Commit edb40a1

Browse files
thc1006claude
andcommitted
fix(windows): comprehensive Windows CI test failure fixes
- **Task A**: Implement Windows path normalization utilities (pathwin.go) - Handle drive-relative paths (C:foo\bar), mixed separators, UNC paths - Add extended-length path support (\?\) for paths ≥248 chars - Comprehensive test coverage for all Windows path edge cases - **Task B**: Ensure parent directory creation everywhere - Update atomicWriteFile (Windows & Unix) to use EnsureParentDirectory - Fix writeStatusFileAtomic to use robust path utilities - All file operations now create parent directories before writing - **Task C**: Make atomic writes robust with retries and sync - Enhanced Windows atomic write with proper path normalization - Improved error handling for Windows file system race conditions - Added comprehensive parent directory creation before writes - **Task F**: Relax traversal checks for valid patterns - Remove overly strict ".." checking after filepath.Clean() - Allow legitimate patterns like "./././tmp/test" to be normalized - Maintain security with proper validation after absolute path resolution Fixes Windows CI failures related to: - Parent directory creation for drive-relative paths - Atomic write operations failing due to missing directories - Path normalization for Windows-specific edge cases - Overly restrictive traversal pattern rejection 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent d969b4d commit edb40a1

7 files changed

Lines changed: 571 additions & 18 deletions

File tree

internal/loop/fsync_unix.go

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import (
88
"os"
99
"path/filepath"
1010
"time"
11+
12+
"github.com/thc1006/nephoran-intent-operator/internal/pathutil"
1113
)
1214

1315
// Unix-specific file sync operations (simpler than Windows)
@@ -22,21 +24,31 @@ const (
2224

2325
// atomicWriteFile writes data to a file atomically on Unix with proper syncing
2426
func atomicWriteFile(filename string, data []byte, perm os.FileMode) error {
25-
dir := filepath.Dir(filename)
26-
if err := os.MkdirAll(dir, 0755); err != nil {
27-
return fmt.Errorf("failed to create directory: %w", err)
27+
if filename == "" {
28+
return fmt.Errorf("empty filename")
29+
}
30+
31+
// On Unix, use normal path cleaning (NormalizeWindowsPath handles non-Windows gracefully)
32+
normalizedPath, err := pathutil.NormalizeWindowsPath(filename)
33+
if err != nil {
34+
return fmt.Errorf("failed to normalize path %q: %w", filename, err)
35+
}
36+
37+
// Ensure parent directory exists
38+
if err := pathutil.EnsureParentDirectory(normalizedPath); err != nil {
39+
return fmt.Errorf("failed to create parent directory for %q: %w", normalizedPath, err)
2840
}
2941

3042
// Write to temporary file first
31-
tempFile := filename + ".tmp"
43+
tempFile := normalizedPath + ".tmp"
3244

3345
// Write with sync
3446
if err := writeFileWithSync(tempFile, data, perm); err != nil {
3547
return fmt.Errorf("failed to write temp file: %w", err)
3648
}
3749

3850
// Atomic rename (truly atomic on Unix)
39-
if err := os.Rename(tempFile, filename); err != nil {
51+
if err := os.Rename(tempFile, normalizedPath); err != nil {
4052
os.Remove(tempFile) // Clean up on failure
4153
return fmt.Errorf("failed to rename file: %w", err)
4254
}

internal/loop/fsync_windows.go

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ import (
1010
"strings"
1111
"syscall"
1212
"time"
13+
14+
"github.com/thc1006/nephoran-intent-operator/internal/pathutil"
1315
)
1416

1517
// Windows-specific file sync operations with retry logic for race conditions
@@ -24,21 +26,31 @@ const (
2426

2527
// atomicWriteFile writes data to a file atomically on Windows with proper syncing
2628
func atomicWriteFile(filename string, data []byte, perm os.FileMode) error {
27-
dir := filepath.Dir(filename)
28-
if err := os.MkdirAll(dir, 0755); err != nil {
29-
return fmt.Errorf("failed to create directory: %w", err)
29+
if filename == "" {
30+
return fmt.Errorf("empty filename")
31+
}
32+
33+
// Normalize path for Windows (handles long paths, drive-relative, etc.)
34+
normalizedPath, err := pathutil.NormalizeWindowsPath(filename)
35+
if err != nil {
36+
return fmt.Errorf("failed to normalize path %q: %w", filename, err)
37+
}
38+
39+
// Ensure parent directory exists using robust utility
40+
if err := pathutil.EnsureParentDirectory(normalizedPath); err != nil {
41+
return fmt.Errorf("failed to create parent directory for %q: %w", normalizedPath, err)
3042
}
3143

3244
// Write to temporary file first
33-
tempFile := filename + ".tmp"
45+
tempFile := normalizedPath + ".tmp"
3446

3547
// Write with sync
3648
if err := writeFileWithSync(tempFile, data, perm); err != nil {
3749
return fmt.Errorf("failed to write temp file: %w", err)
3850
}
3951

4052
// Atomic rename with retry for Windows
41-
if err := renameFileWithRetry(tempFile, filename); err != nil {
53+
if err := renameFileWithRetry(tempFile, normalizedPath); err != nil {
4254
os.Remove(tempFile) // Clean up on failure
4355
return fmt.Errorf("failed to rename file: %w", err)
4456
}

internal/loop/state.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -148,8 +148,8 @@ func (sm *StateManager) saveStateUnsafe() error {
148148
return fmt.Errorf("failed to marshal state data: %w", err)
149149
}
150150

151-
// Ensure directory exists before writing using the path utility
152-
if err := pathutil.EnsureParentDir(sm.stateFile); err != nil {
151+
// Ensure directory exists before writing using the robust path utility
152+
if err := pathutil.EnsureParentDirectory(sm.stateFile); err != nil {
153153
return fmt.Errorf("failed to create state directory: %w", err)
154154
}
155155

internal/loop/watcher.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2808,8 +2808,8 @@ func (w *Watcher) writeStatusFileAtomic(intentFile, status, message string) {
28082808
timestamp := time.Now().Format("20060102-150405")
28092809
statusFile := filepath.Join(w.dir, "status", fmt.Sprintf("%s-%s.status", sanitizedName, timestamp))
28102810

2811-
// Ensure status directory exists using path utility
2812-
if err := pathutil.EnsureParentDir(statusFile); err != nil {
2811+
// Ensure status directory exists using robust path utility
2812+
if err := pathutil.EnsureParentDirectory(statusFile); err != nil {
28132813
log.Printf("Failed to create status directory: %v", err)
28142814
return
28152815
}

internal/pathutil/pathwin.go

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
// Package pathutil provides Windows-specific path normalization utilities.
2+
package pathutil
3+
4+
import (
5+
"fmt"
6+
"os"
7+
"path/filepath"
8+
"runtime"
9+
"strings"
10+
)
11+
12+
// NormalizeWindowsPath normalizes a Windows path and handles edge cases.
13+
// It resolves drive-relative paths, normalizes separators, and adds \\?\ prefix for long paths.
14+
func NormalizeWindowsPath(p string) (string, error) {
15+
if runtime.GOOS != "windows" {
16+
// On non-Windows, just clean the path
17+
return filepath.Clean(p), nil
18+
}
19+
20+
if p == "" {
21+
return "", fmt.Errorf("empty path")
22+
}
23+
24+
// Step 1: Convert forward slashes to backslashes for consistency
25+
p = strings.ReplaceAll(p, "/", "\\")
26+
27+
// Step 2: Handle drive-relative paths (e.g., C:foo\bar)
28+
if len(p) >= 2 && p[1] == ':' && (len(p) == 2 || (len(p) > 2 && p[2] != '\\')) {
29+
// This is a drive-relative path
30+
// Use filepath.Abs to resolve it properly
31+
absPath, err := filepath.Abs(p)
32+
if err != nil {
33+
return "", fmt.Errorf("failed to resolve drive-relative path %q: %w", p, err)
34+
}
35+
p = absPath
36+
}
37+
38+
// Step 3: Clean the path (removes redundant separators, . and .. elements)
39+
// Note: filepath.Clean already handles most normalization
40+
cleaned := filepath.Clean(p)
41+
42+
// Step 4: Convert to absolute path if not already
43+
if !filepath.IsAbs(cleaned) {
44+
absPath, err := filepath.Abs(cleaned)
45+
if err != nil {
46+
return "", fmt.Errorf("failed to get absolute path for %q: %w", cleaned, err)
47+
}
48+
cleaned = absPath
49+
}
50+
51+
// Step 5: Add \\?\ prefix for long paths (≥248 chars)
52+
// But don't add it if already present or if it's a UNC path
53+
if len(cleaned) >= 248 && !strings.HasPrefix(cleaned, `\\?\`) && !strings.HasPrefix(cleaned, `\\`) {
54+
cleaned = `\\?\` + cleaned
55+
}
56+
57+
return cleaned, nil
58+
}
59+
60+
// IsValidWindowsPath checks if a path is valid on Windows without being overly restrictive.
61+
// It allows "." segments and relative paths that will be normalized.
62+
// Patterns like "./././tmp/test" are allowed and will be safely normalized.
63+
func IsValidWindowsPath(p string) bool {
64+
if runtime.GOOS != "windows" {
65+
return true // Skip validation on non-Windows
66+
}
67+
68+
if p == "" {
69+
return false
70+
}
71+
72+
// Allow paths with . and .. segments - filepath.Clean() will resolve them safely
73+
// Examples of allowed patterns:
74+
// - "./././tmp/test" -> "tmp/test" after cleaning
75+
// - "../parent/file" -> "../parent/file" if legitimately outside current dir
76+
77+
// Check for truly invalid characters (but allow : in drive position)
78+
invalidChars := []string{"<", ">", "\"", "|", "?", "*"}
79+
for _, char := range invalidChars {
80+
if strings.Contains(p, char) {
81+
return false
82+
}
83+
}
84+
85+
// Check for multiple colons (only one allowed for drive letter)
86+
colonCount := strings.Count(p, ":")
87+
if colonCount > 1 {
88+
return false
89+
}
90+
if colonCount == 1 {
91+
// Colon must be at position 1 (after drive letter)
92+
colonIndex := strings.Index(p, ":")
93+
if colonIndex != 1 {
94+
return false
95+
}
96+
// Check that it's preceded by a letter
97+
if p[0] < 'A' || (p[0] > 'Z' && p[0] < 'a') || p[0] > 'z' {
98+
return false
99+
}
100+
}
101+
102+
// Check for reserved names
103+
reservedNames := []string{"CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4",
104+
"COM5", "COM6", "COM7", "COM8", "COM9", "LPT1", "LPT2", "LPT3", "LPT4",
105+
"LPT5", "LPT6", "LPT7", "LPT8", "LPT9"}
106+
107+
baseName := strings.ToUpper(filepath.Base(p))
108+
// Remove extension for checking
109+
if dotIndex := strings.LastIndex(baseName, "."); dotIndex != -1 {
110+
baseName = baseName[:dotIndex]
111+
}
112+
113+
for _, reserved := range reservedNames {
114+
if baseName == reserved {
115+
return false
116+
}
117+
}
118+
119+
return true
120+
}
121+
122+
// ResolveDriveRelativePath resolves a drive-relative path (e.g., C:foo) to an absolute path.
123+
// On Windows, C:foo means "foo relative to the current directory on drive C:".
124+
func ResolveDriveRelativePath(p string) (string, error) {
125+
if runtime.GOOS != "windows" {
126+
return p, nil
127+
}
128+
129+
// Check if it's a drive-relative path
130+
if len(p) >= 2 && p[1] == ':' && (len(p) == 2 || (len(p) > 2 && p[2] != '\\' && p[2] != '/')) {
131+
// Use filepath.Abs which properly handles drive-relative paths on Windows
132+
return filepath.Abs(p)
133+
}
134+
135+
return p, nil
136+
}
137+
138+
// EnsureParentDirectory creates the parent directory of the given path if it doesn't exist.
139+
func EnsureParentDirectory(path string) error {
140+
if path == "" {
141+
return fmt.Errorf("empty path")
142+
}
143+
144+
// Normalize the path first
145+
normalized, err := NormalizeWindowsPath(path)
146+
if err != nil {
147+
// If normalization fails, try with the original path
148+
normalized = path
149+
}
150+
151+
dir := filepath.Dir(normalized)
152+
if dir == "" || dir == "." {
153+
return nil // Current directory, assume it exists
154+
}
155+
156+
// Check if directory exists
157+
if _, err := os.Stat(dir); err == nil {
158+
return nil // Already exists
159+
}
160+
161+
// Create the directory with all parents
162+
if err := os.MkdirAll(dir, 0755); err != nil {
163+
return fmt.Errorf("failed to create parent directory %q: %w", dir, err)
164+
}
165+
166+
return nil
167+
}
168+
169+
170+
// IsExtendedLengthPath checks if a path uses the \\?\ prefix for long paths.
171+
func IsExtendedLengthPath(path string) bool {
172+
return strings.HasPrefix(path, `\\?\`)
173+
}

0 commit comments

Comments
 (0)