Fix Windows tests failing in CI

[dubadub]

On Windows, recipe paths passed with forward slashes (e.g., "Breakfast/pancakes.cook") need to be converted to backslashes before passing to cooklang-find library.

This fixes tests that use forward slashes in path arguments, which work on Unix but fail on Windows where the native path separator is backslash.

Changes:

• src/util/mod.rs: get_recipe() normalizes path separators
• src/recipe/read.rs: normalize CLI recipe path argument
• src/server/handlers/recipes.rs: normalize URL path to filesystem path
• src/server/ui.rs: normalize URL path to filesystem path

[claude]

Code Review: Fix Windows tests failing in CI

Thank you for addressing the Windows path separator issue! This is an important cross-platform fix.

Strengths

1. Correct Problem Identification - The fix correctly identifies that Windows uses backslashes while URLs and Unix systems use forward slashes
2. Consistent Application - The normalization is applied in all the right places (CLI input, API handlers, UI handlers)
3. Platform-Specific - Using cfg(target_os = windows) ensures no performance impact on Unix systems
4. Good Comments - Clear explanations of why the conversion is needed

Code Quality Concerns

1. Code Duplication (Medium Priority)

The same normalization logic is duplicated in 4 different files. Recommend creating a helper function in src/util/mod.rs to reduce duplication and make the logic easier to test.

2. Variable Shadowing

In src/util/mod.rs, the code shadows clean_name which could be confusing. A helper function would avoid this pattern.

3. Missing Test Coverage

Good news: The test at tests/cli_integration_test.rs:246 already tests forward slashes in paths. Verify it now passes on Windows.

Potential Bugs

1. Incomplete Solution for Mixed Separators

If a user on Windows types native backslashes, the URL might not work correctly through the web server routing. Does the web server need reverse conversion?

2. Consider Using std::path::MAIN_SEPARATOR_STR

The codebase already uses MAIN_SEPARATOR elsewhere (e.g., src/util/cooklang_to_human.rs:283). Using it here would be more idiomatic and consistent.

Summary

Aspect	Rating
Correctness	Good - Fixes the Windows issue
Code Quality	Fair - Code duplication needs addressing
Testing	Fair - Existing test should verify
Security	Good - No concerns
Performance	Good - Negligible impact

Recommended Actions

1. HIGH PRIORITY: Extract duplicated code into a helper function
2. MEDIUM PRIORITY: Use std::path::MAIN_SEPARATOR_STR instead of hardcoded backslash
3. BEFORE MERGE: Verify the existing test at tests/cli_integration_test.rs:246 now passes on Windows CI

Overall, this is a solid fix for an important issue. The main improvement would be reducing code duplication. Nice work!

[claude]

Code Review: Fix Windows tests failing in CI

Thank you for tackling the Windows path separator issue! This is an important cross-platform compatibility fix. I've reviewed the changes and have several observations and suggestions.

✅ Strengths

1. Clear Problem Identification: The PR correctly identifies that Windows uses backslashes while URLs and Unix systems use forward slashes.
2. Consistent Pattern: The fix is applied consistently across multiple entry points (CLI, web handlers, utilities).
3. Good Documentation: Comments clearly explain why the normalization is needed.

⚠️ Issues and Concerns

1. Potential Over-normalization Issue

The current approach has a fundamental problem: it normalizes paths after they're already strings, but Rust's path handling libraries are designed to handle this correctly. The camino crate (which provides Utf8PathBuf) already handles cross-platform path separators correctly.

Problem locations:

• src/util/mod.rs:341: Replacing / with \\ in a string
• src/shopping_list.rs:253: Same issue
• src/recipe/read.rs:122: Same issue

Why this is problematic:

• String replacement doesn't understand path semantics (e.g., URLs vs filesystem paths)
• The cooklang-find library likely uses Utf8PathBuf internally, which would normalize paths correctly if we pass them as proper path types
• This creates a dependency on target_os rather than letting the path libraries handle it

2. MAIN_SEPARATOR Logic Issue in shopping_list.rs

In src/shopping_list.rs:263:

if entry_without_scaling.contains(std::path::MAIN_SEPARATOR)

This check happens after the Windows normalization at line 253, which means:

• On Windows, you've already converted Breakfast/pancakes.cook → Breakfast\\pancakes.cook
• Then you check if it contains \\ (which it now does)
• But the original input had /, so this logic may not work as intended for the original input

The order should be: check first, then normalize.

3. Incomplete Path Handling

The strip_prefix logic handles both ./ and .\\ (lines 270-272), but the earlier normalization (line 253) means by the time we reach this code on Windows, forward slashes are already gone. This redundancy suggests the approach may need rethinking.

4. Missing cfg for Unused Variables

In src/util/mod.rs and other files, when you do:

let clean_name = name.strip_prefix...
#[cfg(target_os = "windows")]
let clean_name = clean_name.replace('/', "\\");

On non-Windows platforms, if the replace is the only use of the first clean_name, you might get unused variable warnings. Consider using #[cfg_attr] or restructuring.

🔧 Suggested Improvements

Option 1: Let Rust Handle It (Recommended)

Instead of string manipulation, use proper path types:

use camino::Utf8PathBuf;

pub fn get_recipe(base_path: &Utf8PathBuf, name: &str) -> Result<RecipeEntry> {
    // Remove ./ or .\\ prefix if present
    let clean_name = name
        .strip_prefix("./")
        .or_else(|| name.strip_prefix(".\\\\"))
        .unwrap_or(name);
    
    // Convert to Utf8PathBuf - this handles separators cross-platform
    let recipe_path = Utf8PathBuf::from(clean_name);
    
    Ok(cooklang_find::get_recipe(
        vec\![base_path.clone()],
        recipe_path,
    )?)
}

This way, Utf8PathBuf handles the separator conversion internally based on the platform.

Option 2: If String Replacement is Required

If cooklang-find specifically requires platform-native separators as strings (which would be unusual), then:

1. Move normalization earlier in shopping_list.rs before the MAIN_SEPARATOR check
2. Use a helper function to avoid duplication:

#[cfg(target_os = "windows")]
fn normalize_path_separators(path: &str) -> String {
    path.replace('/', "\\\\")
}

#[cfg(not(target_os = "windows"))]
fn normalize_path_separators(path: &str) -> String {
    path.to_string()
}

Then use normalize_path_separators(&entry) consistently.

Option 3: Check cooklang-find's API

Since this is an external crate dependency, check its documentation:

• Does it accept Utf8PathBuf or &str?
• Does it handle path normalization internally?
• Are the tests written with Unix-style paths intentionally?

The tests use Breakfast/pancakes.cook which suggests the library should accept forward slashes. Maybe the issue is elsewhere?

🧪 Testing Suggestions

1. Add unit tests for the path normalization logic
2. Test edge cases:
   • Paths with only recipe names (no separators)
   • Absolute paths on Windows (C:\\recipes\\....)
   • UNC paths on Windows (\\\\\\server\\share\\....)
   • Mixed separators (Breakfast\\\\lunch/dinner.cook)
3. Verify the actual error: What specific error occurred on Windows? Was it in cooklang-find or in file system access?

📋 Minor Style Notes

1. Comments are good, but could be more concise:

   // On Windows, convert URL-style forward slashes to filesystem backslashes

2. Consider extracting magic values:

   const FORWARD_SLASH: char = '/';
   const BACKWARD_SLASH: char = '\\\\';

🎯 Action Items

Before merging, I recommend:

1. Verify the root cause: Run the failing Windows tests and capture the exact error message
2. Check cooklang-find source: See how it handles paths internally
3. Consider using Utf8PathBuf throughout instead of string manipulation
4. Add tests that specifically verify Windows path handling
5. Fix the MAIN_SEPARATOR logic in shopping_list.rs (check before normalize)

📚 References

• Rust Path Documentation
• Camino Crate - already in your dependencies

---

Overall, this is a good start at fixing a real issue, but the implementation could be more robust by leveraging Rust's built-in path handling rather than string manipulation. The cross-platform path handling is a solved problem in Rust - we should use those solutions rather than reinventing them.

Let me know if you'd like help investigating the cooklang-find API or testing on Windows!