feat: Enhance Folder.Clean() and CopyTo() methods with additional options

[thomhurst]

Summary

• Added Clean(bool removeReadOnlyAttribute) overload to handle read-only files
• Added CopyTo(string targetPath, bool preserveTimestamps) overload for timestamp control
• Both methods default to backward-compatible behavior:
  • Clean() defaults to removeReadOnlyAttribute: false (preserves original throwing behavior)
  • CopyTo() defaults to preserveTimestamps: false (preserves original OS-default behavior)

Changes

Folder.Clean()

• New overload accepts removeReadOnlyAttribute parameter
• When true, removes read-only attributes before deletion to prevent access errors
• Default Clean() preserves backward compatibility by NOT removing read-only attributes

Folder.CopyTo()

• New overload accepts preserveTimestamps parameter
• When true, preserves CreationTimeUtc, LastWriteTimeUtc, and LastAccessTimeUtc
• Default CopyTo() preserves backward compatibility by NOT preserving timestamps
• Log moved to start of method for consistency with other operations

RemoveReadOnlyAttributeRecursively

• Uses TopDirectoryOnly with recursion for efficient traversal
• Avoids redundant enumeration of nested directories

Test Plan

• Build succeeds
• Verify Clean() throws on read-only files by default
• Verify Clean(true) handles read-only files
• Verify CopyTo() uses OS-default timestamps
• Verify CopyTo(path, true) preserves timestamps

🤖 Generated with Claude Code

[thomhurst]

Summary

Enhances Folder.Clean() with read-only attribute handling and error resilience, and Folder.CopyTo() with attribute/timestamp preservation.

Critical Issues

1. Inefficient Read-Only Attribute Removal (Performance Bug)

Location: src/ModularPipelines/FileSystem/Folder.cs:413-435

The RemoveReadOnlyAttributeRecursively method uses SearchOption.AllDirectories which will recursively enumerate the entire tree for both files AND subdirectories:

foreach (var file in directory.EnumerateFiles("*", SearchOption.AllDirectories))
{
    // Removes read-only from all files in entire subtree
}

foreach (var subDirectory in directory.EnumerateDirectories("*", SearchOption.AllDirectories))
{
    // Removes read-only from all subdirectories in entire subtree
}

For a directory structure like:

root/
  a/
    b/
      c/

This will enumerate directories as: [a, a/b, a/b/c] and process each one, even though we're already at the root. The complexity is unnecessarily high.

Fix: Use SearchOption.TopDirectoryOnly and recursively call the method:

private static void RemoveReadOnlyAttributeRecursively(DirectoryInfo directory)
{
    if ((directory.Attributes & FileAttributes.ReadOnly) == FileAttributes.ReadOnly)
    {
        directory.Attributes &= ~FileAttributes.ReadOnly;
    }

    foreach (var file in directory.EnumerateFiles("*", SearchOption.TopDirectoryOnly))
    {
        if ((file.Attributes & FileAttributes.ReadOnly) == FileAttributes.ReadOnly)
        {
            file.Attributes &= ~FileAttributes.ReadOnly;
        }
    }

    foreach (var subDirectory in directory.EnumerateDirectories("*", SearchOption.TopDirectoryOnly))
    {
        RemoveReadOnlyAttributeRecursively(subDirectory);
    }
}

2. Logging Position Mismatch

Location: src/ModularPipelines/FileSystem/Folder.cs:198

The log statement appears at line 198 (after the copy completes) but uses present continuous tense "Copying Folder". This should either:

• Move to the beginning of the method (before line 110), OR
• Change to past tense: LogInformation("Copied Folder: {Source} > {Destination}", this, targetPath)

3. Behavioral Breaking Change in Clean()

Location: src/ModularPipelines/FileSystem/Folder.cs:93-154

The original Clean() method would throw exceptions on failure. The new version swallows exceptions and only logs warnings. This is a breaking change for any code that relies on exceptions to detect cleanup failures.

Recommendation: Consider adding a third overload to preserve backward compatibility:

public void Clean() => Clean(removeReadOnlyAttribute: true, throwOnError: true);

public void Clean(bool removeReadOnlyAttribute, bool throwOnError = false)
{
    // ... existing implementation with conditional throw
}

Or document this as a breaking change in release notes if the swallowing behavior is intentional.

Suggestions

Error Handling Consistency

The Clean() method now catches all exceptions (including OutOfMemoryException, StackOverflowException, etc.). Consider catching only specific exceptions like IOException, UnauthorizedAccessException, etc.

Attribute Preservation Edge Case

In CopyTo(), attributes are copied including potentially problematic ones (e.g., FileAttributes.Temporary, FileAttributes.Encrypted). Consider whether all attributes should be preserved or if some should be filtered.

Verdict

⚠️ REQUEST CHANGES - Critical performance issue and logging inconsistency must be addressed.

[thomhurst]

Summary

Adds overloads to Folder.Clean() and CopyTo() for handling read-only files and preserving timestamps.

Critical Issues

1. Contradictory CopyTo() default behavior

Location: src/ModularPipelines/FileSystem/Folder.cs:99

The PR description states: "preserves file/folder timestamps by default"

But the code shows:

public Folder CopyTo(string targetPath)
{
    return CopyTo(targetPath, preserveTimestamps: false);  // <- defaults to FALSE
}

Impact: Documentation doesn't match implementation. Based on the extensive timestamp preservation logic added, it appears the intent was preserveTimestamps: true as the default.

Fix: Change line 99 to return CopyTo(targetPath, preserveTimestamps: true);

---

2. Missing error handling in Clean()

Location: src/ModularPipelines/FileSystem/Folder.cs:108-134

The PR description states: "Uses try-catch blocks with warning logging for individual file/folder failures during clean"

But there are no try-catch blocks in the Clean() method. If a file/directory deletion fails after removing the read-only attribute (e.g., file in use, permission denied), the entire operation will throw an exception.

Impact: The method is not resilient to individual file failures as claimed.

Fix: Add try-catch blocks around the deletion operations with warning logging as described, or remove the claim from the PR description.

---

3. Breaking change: Clean() now removes read-only by default

Location: src/ModularPipelines/FileSystem/Folder.cs:93-95

The original Clean() would throw an exception when encountering read-only files. The new version silently removes the read-only attribute by default. While this might be desired behavior, it's a breaking change that could surprise users who relied on the exception to detect read-only files.

Impact: Existing code that expected exceptions on read-only files will now silently remove those attributes.

Suggestion: Consider:

• Making removeReadOnlyAttribute: false the default to preserve backward compatibility, OR
• Document this as a breaking change in release notes

Suggestions

4. CopyTo() log message placement inconsistency

The log now appears at the end after all work completes ("Copied"), but other methods like Create(), Delete(), and the original Clean() log at the start before performing work.

Consider moving the log to the start for consistency, or changing all methods to log at the end with past tense.

Verdict

⚠️ REQUEST CHANGES - Critical issues found (contradictory default, missing promised error handling, undocumented breaking change)

[thomhurst]

Summary

This PR enhances the Folder.Clean() and Folder.CopyTo() methods with new overloads to handle read-only file attributes and timestamp preservation.

Critical Issues

BLOCKING: Inconsistency between PR description and implementation ⚠️

The PR description states:

"Default Clean() preserves backward compatibility by NOT removing read-only attributes"

However, examining the current repository code at src/ModularPipelines/FileSystem/Folder.cs:100, the Clean() method calls:

Clean(removeReadOnlyAttribute: true);

But the PR diff shows this should be changed to:

Clean(removeReadOnlyAttribute: false);

The current code contradicts the stated backward compatibility goal. If the original Clean() threw on read-only files, then the default should be false (as the PR description states). Please verify which behavior is correct:

1. If original Clean() threw on read-only files → default should be removeReadOnlyAttribute: false (matches PR description)
2. If original Clean() handled read-only files → default should be removeReadOnlyAttribute: true (matches current code)

The XML documentation added in lines 98-101 also states this preserves backward compatibility by NOT removing read-only attributes, which supports option 1.

Suggestions

1. Logging position in CopyTo (Minor): The PR moves the log statement to the start of the method "for consistency with other operations." However, other operations in this file (Delete, MoveTo, Create) log at the beginning, but some log actual outcomes (e.g., "Deleted Folder", "Copied Folder"). Moving the log to the start changes it from logging completion to logging intent. Consider whether this aligns with the logging strategy - should it be "Copying Folder" (present continuous) or stay as "Copied Folder" (past tense) at the end?

2. Test coverage: The PR test plan shows checkboxes for verifying backward compatibility behavior, but none are checked. Ensuring tests validate the default behavior would prevent regressions.

Previous Review Status

No previous review comments found.

Verdict

⚠️ REQUEST CHANGES - Critical inconsistency between PR description/documentation and implementation must be resolved.

[thomhurst]

Response to Review

The PR diff correctly shows the change from true to false:

-        Clean(removeReadOnlyAttribute: true);
+        Clean(removeReadOnlyAttribute: false);

Clarification:

• main branch has Clean(removeReadOnlyAttribute: true) (the breaking change from merged PR feat: Add Phase 2 Activity-based logging to FileSystem operations #1694)
• This PR changes it to Clean(removeReadOnlyAttribute: false) (restoring backward compatibility)

The implementation now matches the PR description:

• ✅ Clean() defaults to removeReadOnlyAttribute: false - preserves original behavior (throws on read-only files)
• ✅ XML documentation added explaining backward compatibility
• ✅ CopyTo() log moved to start for consistency (using present tense "Copying")

The diff in the Files Changed tab should show this change clearly.

[Copilot]

Pull request overview

This PR aims to enhance the Folder.Clean() and Folder.CopyTo() methods by adding overloads with additional options for handling read-only files and preserving timestamps. However, there is a critical discrepancy between the PR description and the implementation regarding the default behavior of Clean().

Key Changes:

• Modified Clean() default behavior by changing the internal call from removeReadOnlyAttribute: true to false
• Added XML documentation explaining the new Clean(bool) overload behavior
• Repositioned logging in CopyTo() from after completion to before operation start

[Copilot]

The log message was moved from after the copy operation completes (line 214 in old code) to before it starts (line 160 in new code). This changes the semantics of the message from "Copied Folder" (past tense, indicating completion) to "Copying Folder" (present tense, indicating start). However, the existing logging at line 160 says "Copying" which is appropriate for the new position. Consider whether the completion logging should be restored, or if starting-only logging is sufficient for this operation. Other operations like MoveTo log at the start, so this change may be intentional for consistency.

[Copilot]

The change from Clean(removeReadOnlyAttribute: true) to Clean(removeReadOnlyAttribute: false) is a breaking change that contradicts both the PR description and the XML documentation. The PR states "removeReadOnlyAttribute=true" should be the default for "safe behavior," and the documentation claims to "preserve backward compatibility." However, the previous default was true, so changing it to false breaks existing behavior. This will cause Clean() to fail on read-only files where it previously succeeded. Either revert to true to maintain the stated goals, or acknowledge this is a breaking change and update the documentation accordingly.

[Copilot]

The documentation claims "This method preserves backward compatibility by not removing read-only attributes," but the implementation was changed from Clean(removeReadOnlyAttribute: true) to Clean(removeReadOnlyAttribute: false). This is actually introducing a breaking change, not preserving backward compatibility. The previous behavior was to remove read-only attributes by default, and this change inverts that behavior. Either revert the default to true to maintain backward compatibility, or update the documentation to acknowledge this is a breaking change.