feat: Add Phase 2 Activity-based logging to FileSystem operations

[thomhurst]

Summary

• Added Activity context information to all FileSystem logging operations
• Enables better distributed tracing by including module name and activity ID in logs
• Phase 2 of the Activity-based tracing refactor (follows Phase 1 in refactor: Add Activity-based tracing for module execution (Phase 1) #1466)

Changes

ModuleActivityTracing.cs

• Added GetCurrentModuleName() helper to retrieve current module from Activity.Current
• Added GetCurrentActivityId() helper to retrieve current Activity ID

File.cs

• Added LogFileOperation() and LogFileOperationWithDestination() helper methods
• All file operations now log with format: "{Operation}: {Path} [Module: {ModuleName}, Activity: {ActivityId}]"

Folder.cs

• Added logging helper methods for folder operations
• All folder operations now include Activity context in logs

Dependencies

• Depends on feat: Enhance Folder.Clean() and CopyTo() methods with additional options #1693 (Folder.Clean/CopyTo enhancements)

Test Plan

• Build succeeds
• Verify logs include module name and activity ID
• Verify Activity.Current integration works correctly

🤖 Generated with Claude Code

[thomhurst]

Summary

Adds Activity-based distributed tracing context (module name and activity ID) to all FileSystem logging operations.

Critical Issues

1. Breaking Behavioral Change in Folder.Clean()

Location: Folder.cs:109-147

The new Clean() implementation catches and logs exceptions but continues execution, whereas the previous implementation would throw on any failure. This is a breaking behavioral change.

Previous behavior:

• directory.Delete(true) throws on failure
• file.Delete() throws on failure

New behavior:

• Catches exceptions and logs warnings
• Continues execution after failures

Impact:

• Callers expecting exceptions on failure will now receive silent warnings
• Partial cleanup failures will go unnoticed unless logs are monitored
• Could mask permission issues or file locks that should be surfaced

Recommendation:

• Keep the exception-throwing behavior by default for backward compatibility
• If resilient cleanup is desired, add it as an opt-in parameter: Clean(bool throwOnError = true)
• Or create a separate method like CleanSafe() for the non-throwing variant

2. Breaking Behavioral Change in Folder.CopyTo()

Location: Folder.cs:126-130

The new overload defaults preserveTimestamps: true, but this changes the behavior of the existing CopyTo(string) method.

Previous behavior: Timestamps were NOT explicitly preserved (relied on OS defaults)

New behavior: Timestamps ARE explicitly preserved by default

Impact:

• Changes observable behavior for existing callers
• File modification times will now match source instead of being set to copy time
• Could affect downstream logic that depends on modification times

Recommendation:

• Keep the original CopyTo(string) behavior unchanged
• Default preserveTimestamps: false to match historical behavior
• Or make it explicit: require callers to specify the parameter

Suggestions

1. Consider PR Dependencies

The PR description states: "Depends on #1693 (Folder.Clean/CopyTo enhancements)". However, the changes to Clean() and CopyTo() appear in this PR (#1694), not in #1693.

Either:

• The dependency is backwards (1693 should depend on 1694), or
• These enhancements should be split into feat: Enhance Folder.Clean() and CopyTo() methods with additional options #1693, keeping only the logging changes in feat: Add Phase 2 Activity-based logging to FileSystem operations #1694

2. Activity Context Documentation

The helper methods have good XML docs explaining the Activity integration. Consider adding a brief code comment explaining why both ModuleLogger.Current (AsyncLocal) and Activity.Current are used together during this transition phase.

Previous Review Status

No previous comments found.

Verdict

⚠️ REQUEST CHANGES - Breaking behavioral changes in Folder.Clean() and Folder.CopyTo() must be addressed to maintain backward compatibility.

[thomhurst]

Updated Review

After re-examining the diff more carefully, I need to retract my previous critical issues. The implementation is actually correct and maintains backward compatibility.

Previous Issues - RESOLVED ✅

Issue 1: Folder.Clean() Breaking Change - FALSE ALARM

Status: NOT AN ISSUE

The new implementation:

• Adds a new overload Clean(bool removeReadOnlyAttribute)
• The parameterless Clean() calls Clean(removeReadOnlyAttribute: true)
• Still throws exceptions (no try-catch in the code)
• Only adds read-only attribute removal, which enables deletion - doesn't suppress errors

My previous review incorrectly stated there was exception catching. This was an error in my analysis.

Issue 2: Folder.CopyTo() Breaking Change - FALSE ALARM

Status: NOT AN ISSUE

The new implementation:

• Adds a new overload CopyTo(string targetPath, bool preserveTimestamps)
• The original CopyTo(string) calls CopyTo(targetPath, preserveTimestamps: false)
• Defaults to false, maintaining backward compatibility
• Timestamps are NOT preserved by default (matches original behavior)

My previous review incorrectly stated it defaulted to true. This was an error in my analysis.

Actual Review

Summary

Adds Activity-based distributed tracing context (module name and activity ID) to FileSystem logging operations. Also enhances Folder.Clean() and CopyTo() with optional behaviors while maintaining backward compatibility.

Critical Issues

None found ✅

Suggestions

1. Clarify PR Dependency

The PR description states "Depends on #1693 (Folder.Clean/CopyTo enhancements)" but the Clean() and CopyTo() enhancements are in this PR (#1694), not #1693. The dependency description appears to be backwards or outdated.

2. Missing GetCurrentActivityId() Implementation

Location: ModuleActivityTracing.cs:126-130

The new method GetCurrentActivityId() returns Activity.Current?.Id, which is of type string?. However, the logging helpers in File.cs and Folder.cs don't appear to handle null values explicitly. The code works because structured logging handles nulls, but it might be clearer to return Activity.Current?.Id ?? "none" for consistency.

3. Unused LogFolderWarning Method

Location: Folder.cs:463-470

The LogFolderWarning() helper is defined but never used in the codebase. Consider removing it or documenting why it's included for future use.

Verdict

✅ APPROVE - No critical issues. The implementation correctly maintains backward compatibility while adding useful functionality.

---

Apology for confusion: My previous review contained significant errors in analyzing the code changes. I apologize for the incorrect blocking concerns.