fix: Improve path type detection heuristics in Downloader and PathHelpers

[thomhurst]

Summary

• Adds check for trailing directory separator before falling back to Path.HasExtension() heuristic
• Paths ending with / or \ are now reliably detected as directories
• Adds comprehensive XML documentation explaining detection logic and limitations
• Documents that directory names with dots (e.g., "my.folder") may still be misidentified

Fixes #1577, #1584

Test plan

• Build passes

🤖 Generated with Claude Code

[thomhurst]

Summary

Improves path type detection by checking for trailing directory separators before using the extension heuristic, preventing misidentification of directories with dots (e.g., "my.folder").

Critical Issues

1. Code Duplication - DRY Violation

The EndsWithDirectorySeparator() method is duplicated in both Downloader.cs and PathHelpers.cs:

// src/ModularPipelines/Context/Downloader.cs:122-131
private static bool EndsWithDirectorySeparator(string path)
{
    if (string.IsNullOrEmpty(path))
    {
        return false;
    }

    var lastChar = path[path.Length - 1];
    return lastChar == Path.DirectorySeparatorChar || lastChar == Path.AltDirectorySeparatorChar;
}

// src/ModularPipelines/Helpers/PathHelpers.cs:68-77 (identical)
private static bool EndsWithDirectorySeparator(string path) { ... }

Recommendation: Extract this method to PathHelpers as a public/internal helper and reuse it in Downloader.cs. This eliminates duplication and provides a single source of truth for this logic.

// In PathHelpers.cs
internal static bool EndsWithDirectorySeparator(string path)
{
    if (string.IsNullOrEmpty(path))
    {
        return false;
    }

    var lastChar = path[path.Length - 1];
    return lastChar == Path.DirectorySeparatorChar || lastChar == Path.AltDirectorySeparatorChar;
}

// In Downloader.cs - remove duplicate and use:
if (PathHelpers.EndsWithDirectorySeparator(options.SavePath))
{
    // ...
}

2. Missing Test Coverage

The PR adds new behavior (handling trailing directory separators) but doesn't add tests to verify it works correctly. Consider adding tests like:

[Test]
public async Task Directory_Path_Type_With_Trailing_Separator()
{
    var path = Path.Combine(TestContext.WorkingDirectory, "my.folder") + Path.DirectorySeparatorChar;
    await Assert.That(path.GetPathType()).IsEqualTo(PathType.Directory);
}

[Test]
public async Task Directory_Path_Type_With_Dots_But_No_Separator()
{
    // This should still work as documented - directories without trailing separator
    // and with dots will be detected based on extension heuristic
    var path = Path.Combine(TestContext.WorkingDirectory, "my.folder");
    // Depends on intended behavior
}

Similarly, add tests for the Downloader.GetSaveLocation() behavior with trailing separators.

Suggestions

The documentation is excellent - clear explanation of the heuristics and their limitations. The approach is sound, just needs to eliminate the duplication.

Verdict

⚠️ REQUEST CHANGES - Critical code duplication and missing test coverage

[thomhurst]

Addressed Feedback

Thanks for the detailed review! I've addressed all the issues:

1. DRY Violation Fixed: Made EndsWithDirectorySeparator() internal in PathHelpers.cs and removed the duplicate from Downloader.cs. Now Downloader calls PathHelpers.EndsWithDirectorySeparator().

2. Test Coverage Added: Added 7 new tests for the trailing separator behavior:

   • Directory_Path_Type_With_Trailing_Separator - verifies paths with trailing separator are detected as directories
   • Directory_Path_Type_With_Dots_And_Trailing_Separator - verifies "my.folder" is detected as directory
   • EndsWithDirectorySeparator_Returns_True_For_Trailing_Separator - tests DirectorySeparatorChar
   • EndsWithDirectorySeparator_Returns_True_For_Alt_Separator - tests AltDirectorySeparatorChar
   • EndsWithDirectorySeparator_Returns_False_For_No_Separator - tests paths without trailing separator
   • EndsWithDirectorySeparator_Returns_False_For_Empty_String - edge case
   • EndsWithDirectorySeparator_Returns_False_For_Null - edge case

All 12 PathHelpersTests now pass.

[thomhurst]

Summary

Improves path type detection by checking for trailing directory separators before falling back to the extension heuristic, fixing issues with directory paths ending in '/' or ''.

Critical Issues

None found ✅

Suggestions

1. Consider cross-platform edge case in EndsWithDirectorySeparator

Location: src/ModularPipelines/Helpers/PathHelpers.cs:72

The current implementation checks both Path.DirectorySeparatorChar and Path.AltDirectorySeparatorChar, which is good. However, on Windows, paths could theoretically contain a mix of separators (e.g., "C:/foo\bar/"). The current implementation correctly handles trailing separators, but it might be worth documenting that mixed separators in the middle of paths aren't normalized.

This is a minor point and doesn't affect the correctness of the fix.

2. Test coverage for null handling

Location: test/ModularPipelines.UnitTests/PathHelpersTests.cs:92

The test EndsWithDirectorySeparator_Returns_False_For_Null uses null-forgiving operator (null!) to test null handling. While this works, consider whether EndsWithDirectorySeparator should accept nullable strings (string?) in its signature for better API clarity, or add a guard clause with a proper exception if null is not expected.

Current behavior (returning false for null) is reasonable but implicit.

Verdict

✅ APPROVE - No critical issues

Review Notes:

• The fix correctly addresses the trailing separator issue by checking it before the extension heuristic
• Documentation is thorough and explains limitations clearly
• Test coverage is comprehensive (6 new tests covering the new functionality)
• The GetExtension method was appropriately made static (no instance state needed)
• Code is clean and follows the existing patterns in the codebase