feat: implement bidirectional sync engine (Task 6)

[onsails]

Summary

Implements complete bidirectional synchronization engine with orchestration, action resolution, conflict handling, and reporting.

Changes

• Orchestrator (sync/orchestrator.rs): SyncEngine with direction support (ToLocal/ToGlobal)
• Actions (sync/actions.rs): SyncAction enum and resolver for determining operations
• Executor (sync/executor.rs): Atomic file operations with conflict strategy handling
• Reporting (sync/reporting.rs): Summary generation with colored output
• Integration tests: 9 comprehensive tests covering all sync scenarios
• Filter application: Pattern matching with ignore/include support
• Symlink management: Integrated with scanner's preserve_symlinks config

Test Coverage

• ✅ 105 tests passing (9 new integration tests)
• ✅ Zero clippy warnings
• ✅ All 7 subtasks complete (6.1-6.7)

Conflict Strategies

• Fail: Stop on conflicts (default)
• Overwrite: Always overwrite destination
• Skip: Skip conflicting files
• Newer: Use newer timestamp

Next Steps

Task 4 will add interactive prompts for conflict resolution.

Closes #6

[claude]

Code Review: PR #6 - Bidirectional Sync Engine Implementation

Executive Summary

This PR implements a complete bidirectional synchronization engine for the ccsync project. The code is well-structured, tested, and follows Rust best practices. However, there are several issues that should be addressed before merging.

Overall Assessment: ⚠️ NEEDS WORK

Code Quality Score: 7.5/10

---

🔴 Critical Issues (Must Fix)

1. Duplicate SyncDirection Enum

Files: crates/ccsync/src/sync/orchestrator.rs:16-22 and crates/ccsync/src/config/types.rs

Two separate SyncDirection enums exist in different modules, which will cause conflicts when both are used together.

Fix: Remove the duplicate from sync/orchestrator.rs and import from config::types:

use crate::config::SyncDirection;

2. Unused Parameter: _default_strategy

File: crates/ccsync/src/sync/actions.rs:41

The _default_strategy parameter is prefixed with _ but still passed from callers. The ComparisonResult::Conflict already contains the strategy.

Fix: Remove the unused parameter entirely from resolve() and update callers.

3. Config Ignored in get_conflict_strategy()

File: crates/ccsync/src/sync/orchestrator.rs:110-113

const fn get_conflict_strategy() -> ConflictStrategy {
    ConflictStrategy::Fail  // Always returns Fail, doesn't use config!
}

Fix: Use the config's conflict strategy:

fn get_conflict_strategy(&self) -> ConflictStrategy {
    self.config.conflict_strategy.unwrap_or(ConflictStrategy::Fail)
}

4. Pattern Matcher Recreated on Every Sync

File: crates/ccsync/src/sync/orchestrator.rs:51-59

The PatternMatcher is recreated for every sync operation instead of being compiled once.

Fix: Move pattern matcher construction to SyncEngine::new() and store as a field.

---

🟡 High Priority Issues (Should Fix)

5. Symlink Edge Case Not Tested

File: crates/ccsync/src/sync/orchestrator.rs:73-78

If symlinks resolve to absolute paths outside source_root, strip_prefix will fail.

Fix: Add test case and explicit handling:

#[test]
fn test_sync_with_external_symlink() {
    // Create symlink pointing outside source tree
    // Verify appropriate error or skip behavior
}

6. Dry-Run Uses stdout Instead of stderr

File: crates/ccsync/src/sync/executor.rs:34,42,49

Output to stdout makes it hard to parse programmatically.

Fix: Change to eprintln! or use a proper logging framework.

7. Missing Documentation on Public APIs

Files: All sync module files

Public types lack usage examples and error documentation.

Fix: Add module-level docs and examples:

/// # Examples
///
/// ```
/// let config = Config::default();
/// let engine = SyncEngine::new(config, SyncDirection::ToLocal);
/// let result = engine.sync("/source", "/dest")?;
/// ```

---

🟢 Low Priority Issues (Nice to Have)

8. Unnecessary Path Clone

File: crates/ccsync/src/sync/orchestrator.rs:89

let action = SyncActionResolver::resolve(
    file.path.clone(),  // ← Could be consumed instead
    dest_path,
    &comparison,
);

Optimization: Consider consuming the path or using references.

9. TOCTOU Race Condition

File: crates/ccsync/src/sync/executor.rs:119-137

Minor race condition between create_dir_all and fs::copy, unlikely in practice but worth noting.

---

✅ What's Great

• Architecture: Excellent separation of concerns (actions, executor, orchestrator, reporting)
• Testing: 9 comprehensive integration tests covering major scenarios
• Error Handling: Robust use of anyhow::Context throughout
• No unsafe code: All operations are safe
• Idiomatic Rust: Good use of #[must_use], Result<T>, pattern matching
• Zero clippy warnings

---

🧪 Missing Test Cases

1. Symlinks pointing outside source tree
2. Permission errors (read-only destinations)
3. Unicode/special characters in filenames
4. Conflict strategy with identical timestamps
5. Empty directories
6. Large files (>100MB)
7. Multiple errors in single sync operation

---

📋 Recommendations Before Merging

Must Fix (Blocking):

• Resolve duplicate SyncDirection enum
• Remove or use _default_strategy parameter
• Implement config-based conflict strategy
• Move pattern matcher to SyncEngine::new()

Should Fix:

• Add tests for symlinks outside source tree
• Add tests for permission errors and unicode filenames
• Document public APIs with examples
• Change dry-run output to stderr

Nice to Have:

• Optimize path cloning
• Add builder pattern for SyncEngine
• Make SyncReporter more flexible (JSON output)

---

Detailed Breakdown

Strengths:

• 9/10 Architecture
• 9/10 Error Handling
• 8/10 Testing
• 8/10 Security

Needs Improvement:

• 5/10 Documentation
• 7/10 Performance

Estimated effort to fix critical issues: 2-3 hours

---

Once the critical and high-priority issues are addressed, this PR will be in excellent shape for merging. Great work on the overall structure and test coverage! 🎉

[claude]

PR Review: Bidirectional Sync Engine (Task 6)

Overview

This PR successfully implements the core bidirectional synchronization engine with orchestration, action resolution, conflict handling, and reporting. The code is well-structured with clean separation of concerns across multiple modules.

Summary:

• ✅ 754 additions, 77 deletions across 12 files
• ✅ 9 new integration tests added (105 total tests)
• ✅ All 7 subtasks (6.1-6.7) marked complete
• ❌ CRITICAL ISSUE FOUND - Must be fixed before merge

---

🔴 CRITICAL ISSUES - MUST FIX

1. Error Swallowing Violates FAIL FAST Principle

Location: crates/ccsync/src/sync/orchestrator.rs:97-99

// Execute action
if let Err(e) = executor.execute(&action, &mut result) {
    result.errors.push(e.to_string());
}

Problem: This directly violates the FAIL FAST principle in CLAUDE.rust.md. The code catches errors from file operations, logs them, but continues processing remaining files. This is exactly the anti-pattern described in the project standards:

// WRONG: Logs but continues
if let Err(e) = operation() {
    log::error!("Failed: {}", e);  // Still swallowing!
}
// Continues execution...

Impact: If a critical error occurs (permission denied, disk full, corrupted filesystem), the sync engine continues instead of failing fast. This leads to partial syncs and potential data inconsistency.

Required Fix: Choose one of these approaches:

Option A - Fail immediately on any error (strictest):

// Execute action - propagate errors immediately
executor.execute(&action, &mut result)?;

Option B - Accumulate errors but fail at the end:

for file in &scan_result.files {
    // ... existing code ...
    if let Err(e) = executor.execute(&action, &mut result) {
        result.errors.push(e.to_string());
    }
}

// FAIL FAST if any errors occurred
if !result.errors.is_empty() {
    anyhow::bail!(
        "Sync failed with {} errors:\n  - {}", 
        result.errors.len(), 
        result.errors.join("\n  - ")
    );
}

Recommendation: Option B is more user-friendly (shows all errors at once) while still respecting FAIL FAST.

---

🟡 MEDIUM PRIORITY ISSUES

2. Use of unwrap_or Against Project Standards

Location: crates/ccsync/src/sync/orchestrator.rs:112

fn get_conflict_strategy(&self) -> ConflictStrategy {
    self.config.conflict_strategy.unwrap_or(ConflictStrategy::Fail)
}

Issue: While this specific usage is providing a default (not swallowing an error), CLAUDE.rust.md lines 40-42 discourage unwrap_or* methods for consistency.

Fix: Use explicit pattern matching:

fn get_conflict_strategy(&self) -> ConflictStrategy {
    match self.config.conflict_strategy {
        Some(strategy) => strategy,
        None => ConflictStrategy::Fail, // Explicit default for safety
    }
}

---

🔵 MINOR ISSUES (Consider for Future)

3. Intentional Result Suppression in Reporting

Location: crates/ccsync/src/sync/reporting.rs:17-30

Multiple uses of let _ = to ignore writeln! results. While technically safe (writing to String can't fail), consider making the assumption explicit:

writeln!(output, "Created:  {}", result.created)
    .expect("Writing to String should never fail");

4. Direct eprintln! Usage in Library Code

Locations: orchestrator.rs:104, executor.rs:34,42,77,85,92,99

Library crates should avoid direct stderr output. Consider:

• Accumulating warnings in SyncResult instead of eprintln!
• Using a logging framework or custom output handler for dry-run messages

This is acceptable for now but should be refactored when adding interactive prompts in Task 4.

---

✅ EXCELLENT PRACTICES OBSERVED

Code Quality

1. ✅ Proper error propagation with anyhow::Context providing helpful messages:

   .with_context(|| format!("Failed to strip prefix from {}", file.path.display()))?

2. ✅ No environment pollution - All tests use TempDir and pass config through parameters (compliant with CLAUDE.rust.md:68)

3. ✅ Appropriate #[must_use] attributes on all value-returning functions

4. ✅ Comprehensive documentation - All public items have doc comments with # Errors sections

5. ✅ Clean architecture - Excellent separation of concerns:

   • orchestrator.rs - workflow coordination
   • actions.rs - decision logic
   • executor.rs - file operations
   • reporting.rs - summary generation

6. ✅ Visibility control - Proper use of pub(crate) and #[cfg(test)] (compliant with CLAUDE.rust.md:84)

7. ✅ Dependency versioning - All dependencies use tilde requirements (~x.x.x) as required

8. ✅ Edition 2024 - Using the latest Rust edition as specified

---

🧪 TEST COVERAGE

Excellent coverage with 9 integration tests:

• ✅ Creating new files
• ✅ Skipping identical files
• ✅ Pattern filtering (ignore/include)
• ✅ Conflict detection (Fail strategy)
• ✅ Dry run mode
• ✅ Bidirectional sync workflow
• ✅ File updates with Overwrite strategy
• ✅ Reporter output formatting
• ✅ Error reporting

Missing (consider adding in follow-up):

• Skip and Newer conflict strategies
• Large file handling
• Permission error scenarios
• Symlink preservation edge cases

---

📋 REQUIRED ACTIONS BEFORE MERGE

1. [CRITICAL] Fix error swallowing in orchestrator.rs:97-99

   • Implement Option B (accumulate + fail at end) or Option A (immediate failure)

2. [MEDIUM] Replace unwrap_or on line 112 with explicit pattern matching

3. [REQUIRED] Run and verify:

   cargo clippy --workspace -- -D warnings
   cargo test --workspace

4. [OPTIONAL] Consider addressing minor issues (reporting.rs let _ =, eprintln! usage)

---

🎯 VERDICT

Status: ❌ NOT READY FOR MERGE

The error swallowing in the orchestrator violates the FAIL FAST principle, which is marked as CRITICAL in project standards. This must be fixed before merging.

Estimated effort: 15-30 minutes for critical fixes

After fixes: This will be an excellent addition to the codebase. The architecture is clean, tests are comprehensive, and the code follows Rust best practices.

---

💡 ADDITIONAL NOTES

• This PR correctly leaves interactive prompts for Task 4 (as noted in comments)
• The conflict strategy design is extensible and well-typed
• Good use of the similar crate for diff generation
• Pattern matching with the ignore crate is properly integrated
• No conflicts with pending Task Master tasks (Task 4, 7, 8)

Great work overall! Just need to address the critical error handling issue and this will be ready to merge. 🚀

[claude]

Code Review - PR #6: Bidirectional Sync Engine

Overall Assessment

Grade: A- (Excellent) ⭐⭐⭐⭐⭐

This is high-quality, production-ready code that demonstrates strong Rust development skills and thoughtful software engineering. The implementation is clean, well-documented, and follows Rust best practices.

Architecture

The sync module follows a clean, modular architecture:

• orchestrator.rs: Coordinates the entire sync workflow
• actions.rs: Determines what actions to take based on file comparisons
• executor.rs: Executes file operations with dry-run support
• reporting.rs: Generates summary reports
• sync.rs: Public API and comprehensive integration tests

---

✅ Strengths

Excellent Error Handling

• Comprehensive error contexts using anyhow::Context
• No unwraps or panics in production code (only in tests, which is correct)
• Proper fail-fast behavior with error collection
• All Result types properly propagated

Strong Type Safety

• Clear enums (SyncAction, ComparisonResult) effectively represent states
• Good use of const fn for simple constructors
• Proper #[must_use] attributes on pure functions

Outstanding Test Coverage (105 tests passing!)

Integration tests cover:

• ✅ Creating new files
• ✅ Skipping identical files
• ✅ Ignore pattern filtering
• ✅ All conflict strategies (Fail, Overwrite, Skip, Newer)
• ✅ Dry-run mode
• ✅ Bidirectional sync
• ✅ Update operations
• ✅ Error reporting

Proper Resource Management

• Streaming file hashing with 8KB buffer (prevents loading entire files into memory)
• No unnecessary temporary files
• Correct path handling with strip_prefix and join

Security

• ✅ No unsafe code - entirely memory-safe
• ✅ Proper path manipulation APIs (no path traversal vulnerabilities)
• ✅ No command injection risks
• ✅ Bounded resource usage

---

🔍 Minor Issues & Suggestions

Issue 1: Inconsistent Error Logging Pattern

File: crates/ccsync/src/sync/orchestrator.rs:91

Using eprintln! directly for error logging. Consider:

• Using a logging framework (log or tracing) for better control
• Providing a callback mechanism for error reporting
• Distinguishing between dry-run output and actual errors

Issue 2: Dead Code Attribute

File: crates/ccsync/src/sync/orchestrator.rs:18-20

The direction field is marked #[allow(dead_code)] but stored in struct. Consider:

• Using it now for logging/reporting (e.g., in error messages)
• Removing until actually needed
• Current approach is acceptable but not ideal

Issue 3: Pattern Matcher Repeated syscalls

File: crates/ccsync/src/sync/orchestrator.rs:67-73

Calls file.path.is_dir() for every file (syscall). Consider:

• Adding is_dir field to ScannedFile
• Caching during scanning phase

---

💡 Recommended Follow-ups (Post-Merge)

High Priority

1. Atomic File Operations: Use temp file + rename pattern to prevent corruption on interruption
2. Progress Reporting: Add callback/trait for progress indication on large syncs
3. Parallel Execution: Use rayon for parallel file copying to improve performance

Medium Priority

4. Metadata Preservation: Preserve file permissions and timestamps during copy
5. Better Error Aggregation: Keep structured error types instead of strings
6. Symlink Validation: Ensure symlink targets stay within expected boundaries

Low Priority

7. Logging Framework: Replace eprintln! with tracing
8. Metrics Collection: Add bytes copied, time taken to SyncResult
9. Documentation Examples: Add usage examples to module docs

---

📊 Code Quality Metrics

• Error Handling: ⭐⭐⭐⭐⭐ (5/5)
• Test Coverage: ⭐⭐⭐⭐⭐ (5/5)
• Documentation: ⭐⭐⭐⭐☆ (4/5)
• Performance: ⭐⭐⭐⭐☆ (4/5)
• Security: ⭐⭐⭐⭐⭐ (5/5)
• API Design: ⭐⭐⭐⭐⭐ (5/5)

Overall Score: 4.7/5.0

---

✅ Recommendation

APPROVED - Ready to merge! 🚀

No blocking issues found. All suggestions are optional enhancements that can be addressed in follow-up PRs. The code is maintainable, extensible, and production-ready.

Excellent work on Task 6! The bidirectional sync engine is solid and well-tested.

[claude]

Code Review - PR #6: Bidirectional Sync Engine

Executive Summary

This PR implements a high-quality, well-architected synchronization engine with excellent error handling, comprehensive testing, and security-conscious design. The code is production-ready with only one required fix and several optional improvements.

Overall Grade: A- (Excellent)

---

✅ Strengths

Architecture & Design

• Excellent separation of concerns: Clear module boundaries (orchestrator, actions, executor, reporting)
• Strategy pattern for conflict resolution provides flexibility
• Dependency injection makes components highly testable
• Single Responsibility Principle applied throughout

Code Quality

• No unsafe code - entirely safe Rust
• No unwrap() in production code - proper error handling everywhere
• Comprehensive error context using anyhow::Context
• Streaming file hashing with 8KB buffer (excellent for large files)
• Pattern compilation cached during SyncEngine construction
• Const functions where applicable

Security

• ✅ Proper symlink handling with symlink_metadata() to prevent TOCTOU attacks
• ✅ Path traversal prevention via canonical paths
• ✅ Config size limit (1MB) prevents DoS
• ✅ Symlink loop detection prevents infinite recursion
• ✅ No arbitrary code execution

Testing

• 105 tests passing with 9 new integration tests
• Comprehensive coverage: conflicts, dry-run, bidirectional sync, filters
• Edge cases tested: broken symlinks, Unicode paths, empty files
• Proper test isolation using tempfile

---

🔴 Required Changes

1. Missing conflict_strategy in Config Merge ⚠️

File: crates/ccsync/src/config/merge.rs:103-116

Issue: The config merger handles other Option fields (follow_symlinks, preserve_symlinks, dry_run, non_interactive) but does not merge conflict_strategy.

Impact: If multiple config files set different conflict strategies, the higher-precedence value won't be preserved.

Fix:

// In ConfigMerger::merge_into(), add after line 115:
if config.conflict_strategy.is_some() {
    base.conflict_strategy = config.conflict_strategy;
}

Test needed: Add test case in config/merge.rs to verify conflict_strategy merging behavior.

---

🟡 Major Findings (Non-Blocking)

2. Unused direction Field

File: crates/ccsync/src/sync/orchestrator.rs:18-20

#[allow(dead_code)]
direction: SyncDirection,

Observation: The comment indicates this is for future direction-specific rules and reporting. This is acceptable but consider:

• Implementing direction-specific logging in reporting (e.g., "Syncing to local..." vs "Syncing to global...")
• Or removing if Task 9 (CLI integration) won't use it yet

3. Unnecessary #[allow(dead_code)] on warnings

File: crates/ccsync/src/scanner.rs:50

Issue: The warnings field is actually used in orchestrator.rs:96-99:

for warning in &scan_result.warnings {
    eprintln!("Warning: {warning}");
}

Fix: Remove the #[allow(dead_code)] attribute - it's unnecessary.

4. Potential Race Condition in Timestamp Comparison

File: crates/ccsync/src/comparison/timestamp.rs:26-31

Issue: File modification times are read in two separate system calls. Files could theoretically be modified between reads.

Severity: Low in practice (sync typically runs on stable filesystems)

Recommendation: Document this limitation with a comment, or consider file locking for critical operations.

---

🟢 Minor Improvements (Optional)

5. Utility Structs Could Be Stateless

Files: comparison/diff.rs, comparison/hash.rs, comparison/timestamp.rs

These structs have no state:

pub struct FileHasher;
pub struct DiffGenerator;
pub struct TimestampComparator;

Suggestion: Make their methods static/associated functions:

impl FileHasher {
    pub fn hash(path: &Path) -> Result<String> { ... }
}
// Usage: FileHasher::hash(path)

This avoids unnecessary .new() calls and makes the API clearer.

6. Inconsistent Dry-Run Logging

File: sync/executor.rs

Dry-run messages appear in both Skip actions and action handlers, leading to inconsistent logging patterns.

Suggestion: Centralize all dry-run logging in one place for consistency.

7. Config Size Limit Could Be Higher

File: config/merge.rs:74

const MAX_CONFIG_SIZE: u64 = 1024 * 1024; // 1MB

Suggestion: Consider 10MB or make it configurable via environment variable. Current limit is fine for typical use cases.

8. Symlink Error Messages Could Include More Context

File: scanner/symlinks.rs:96-107

When handling broken symlinks, include both the original path and the target in error messages for better debugging.

---

📊 Test Coverage Assessment

Well-Tested ✅

• File comparison (hash, timestamp, diff)
• Scanner modes (flat, one-level, recursive)
• Conflict resolution strategies (Fail, Overwrite, Skip, Newer)
• Config merging and validation
• Symlink resolution and loop detection
• Pattern matching with ignore/include

Test Gaps (Future Work)

• Large file handling (>100MB) - current tests use small files
• Concurrent file modifications during sync
• Permission denied scenarios
• End-to-end CLI integration tests
• Performance benchmarks

---

🔒 Security Analysis

✅ Excellent security practices:

• No unsafe code
• Symlink attack prevention
• Path traversal protection
• DoS prevention via size limits
• Loop detection

⚠️ Minor consideration:

• No file permission validation before operations (consider checking permissions first to provide better error messages)

---

⚡ Performance Analysis

Efficient practices:

• Streaming hash computation (8KB buffer)
• Early exit on identical hashes
• Pattern compilation cached
• Minimal allocations

Future optimizations (not needed now):

• Parallel file scanning with rayon
• Batch file operations
• Progress reporting for large syncs

---

📝 Documentation

Excellent documentation:

• Module-level rustdoc on all modules
• Function documentation with Errors sections
• Inline comments for complex logic
• #[must_use] attributes on important functions

---

✅ Compliance with CLAUDE.md

The code follows all Rust project standards:

• ✅ Zero clippy warnings (per PR description)
• ✅ All tests passing (105 tests)
• ✅ Comprehensive error handling
• ✅ No panics in production code
• ✅ Proper documentation

Note: Per CLAUDE.md, the mandatory steps before marking complete are:

1. cargo clippy --workspace -- -D warnings ✅ (Zero warnings claimed)
2. cargo test --workspace ✅ (105 tests passing)
3. /review-rust-code ✅ (This review)
4. Apply fixes ⏳ (Required fix needed)

---

🎯 Recommendation

APPROVE WITH REQUIRED CHANGES

Before Merge (Required):

1. ✅ Add conflict_strategy merging in ConfigMerger::merge_into()
2. ✅ Add test case for conflict_strategy merging
3. ✅ Remove unnecessary #[allow(dead_code)] from warnings field

Nice to Have (Optional):

• Document timestamp comparison limitation
• Consider refactoring utility structs to static methods
• Enhance dry-run logging consistency

---

📦 Integration with Task 9

This PR completes Task 6 (Bidirectional Synchronization Logic) and properly sets up for Task 9 (CLI Command Integration). The SyncEngine API is clean and ready for CLI integration:

let engine = SyncEngine::new(config, direction)?;
let result = engine.sync(source_root, dest_root)?;
SyncReporter::generate_summary(&result);

Task 4 (Interactive Prompts) is correctly identified as a future dependency and not implemented here.

---

🏆 Final Verdict

This is excellent Rust code that demonstrates deep understanding of:

• Rust idioms and best practices
• Error handling patterns
• Testing strategies
• Security considerations
• Performance optimization

With the required conflict_strategy merge fix, this PR is ready to merge. Great work! 🎉

---

Reviewed by: Claude Code
Review Date: 2025-10-28
Files Reviewed: 27 files across sync, comparison, scanner, and config modules