feat: integrate config file loading into CLI

[onsails]

Summary

Integrates config file loading from Task 5 into CLI commands. Config files are now automatically discovered, loaded, and merged with CLI flags.

Features

Config File Support

• Automatically loads config from discovered locations
• --config <path> to specify custom config file
• --no-config to skip all config files
• Graceful fallback to defaults if loading fails

Config File Locations (in precedence order):

1. --config <path> - Custom config via CLI flag
2. .ccsync.local - Project-local (gitignored)
3. .ccsync - Project config (committed)
4. ~/.config/ccsync/config.toml - Global XDG config

Precedence Rules

• CLI flags always override config file settings
• Config files merged with additive behavior for arrays
• Boolean values overridden by higher precedence sources

Supported Config Options

# Ignore patterns (gitignore-style)
ignore = ["**/test-*.md", "**/*.backup"]

# Include patterns
include = ["agents/**", "skills/**"]

# Default conflict strategy
conflict_strategy = "newer"  # fail, overwrite, skip, newer

Code Quality Improvements

Refactoring: SyncOptions Struct

• Created SyncOptions struct to avoid excessive boolean parameters
• Reduced execute() parameter count from 7 to 3
• Better extensibility for future options
• Proper clippy allowances for configuration structs

Before:

pub fn execute(
    types: &[ConfigType],
    conflict: &ConflictMode,
    verbose: bool,
    dry_run: bool,
    yes_all: bool,
    config_path: Option<&Path>,
    no_config: bool,
) -> Result<()>

After:

pub fn execute(
    types: &[ConfigType],
    conflict: &ConflictMode,
    options: &SyncOptions,
) -> Result<()>

Error Handling

• Graceful fallback when config files are missing or invalid
• Warnings shown only in verbose mode
• Users can still use tool with malformed configs

Documentation

• Updated README with config file examples
• Explained precedence order
• Documented all config file locations
• Examples for using and bypassing config files

Test Results

• ✅ 109/109 tests passing
• ✅ Zero clippy warnings with -D warnings
• ✅ All existing functionality preserved
• ✅ Backward compatible (works without config files)

Implementation

New Module: crates/ccsync-cli/src/commands/common.rs

• SyncOptions struct for execution parameters

Modified:

• to_local.rs, to_global.rs - Integrated ConfigManager
• main.rs - Creates SyncOptions from CLI flags
• README.md - Added configuration documentation

Closes #5 (Config file loading integration)

[claude]

Pull Request Review: Config File Integration

Summary

This PR successfully integrates config file loading into CLI commands with a solid foundation. The introduction of the SyncOptions struct is a significant improvement. However, there are several issues that need to be addressed before merging.

✅ Strengths

1. Excellent Refactoring

The SyncOptions struct is a great improvement:

• Reduces function parameters from 7 to 3
• Better extensibility for future options
• Proper clippy allowances (struct_excessive_bools, fn_params_excessive_bools)

Example (crates/ccsync-cli/src/commands/common.rs:4-16):

#[allow(clippy::struct_excessive_bools)]
pub struct SyncOptions<'a> {
    pub verbose: bool,
    pub dry_run: bool,
    pub yes_all: bool,
    pub config_path: Option<&'a std::path::Path>,
    pub no_config: bool,
}

2. Graceful Error Handling

Config loading failures fallback to defaults (to_local.rs:43-49, to_global.rs:43-49):

ConfigManager::load(options.config_path).unwrap_or_else(|e| {
    if options.verbose {
        eprintln!("Warning: Failed to load config files: {e}");
        eprintln!("Using default configuration");
    }
    Config::default()
})

3. Security Considerations

The underlying ConfigDiscovery uses symlink_metadata() to avoid following symlinks, which is good security practice.

4. Documentation

The README additions are clear and provide good examples of config file usage.

---

❌ Critical Issues

1. Missing Test Coverage

The PR claims "109/109 tests passing" but provides no new tests for the new functionality:

Missing tests:

• --config <path> flag behavior
• --no-config flag behavior
• Config file loading and merging
• CLI flag precedence over config files
• Invalid config path handling
• Interaction between config files and type filters

Recommendation: Add integration tests in crates/ccsync-cli/tests/cli_tests.rs:

#[test]
fn test_config_flag() {
    // Test --config <path> loads custom config
}

#[test]
fn test_no_config_flag() {
    // Test --no-config bypasses all config files
}

#[test]
fn test_cli_overrides_config() {
    // Test that CLI flags override config file settings
}

2. Significant Code Duplication (DRY Violation)

Both to_local.rs and to_global.rs have identical implementations of:

• merge_cli_flags (lines 106-126)
• convert_conflict_mode (lines 128-135)
• build_type_patterns (lines 137-153)
• Config loading logic (lines 36-53)

Recommendation: Extract to shared module:

// crates/ccsync-cli/src/commands/common.rs

impl SyncOptions<'_> {
    pub fn load_and_merge_config(
        &self,
        types: &[ConfigType],
        conflict: &ConflictMode,
    ) -> anyhow::Result<Config> {
        let mut config = if self.no_config {
            Config::default()
        } else {
            ConfigManager::load(self.config_path)
                .unwrap_or_else(|_| Config::default())
        };
        
        Self::merge_cli_flags(&mut config, types, conflict, self.dry_run);
        Ok(config)
    }
    
    // Move merge_cli_flags, convert_conflict_mode, build_type_patterns here
}

3. Incorrect Array Merge Behavior

The PR description claims "Config files merged with additive behavior for arrays" but the code replaces the entire include array:

Current behavior (to_local.rs:122-125):

// Handle type filters (CLI patterns override config patterns)
if !types.is_empty() {
    config.include = Self::build_type_patterns(types); // ❌ Replaces, not adds
}

Expected behavior:

if !types.is_empty() {
    let type_patterns = Self::build_type_patterns(types);
    // Add to existing patterns rather than replacing
    config.include.extend(type_patterns);
}

Impact: Users cannot combine config file include patterns with CLI type filters. This contradicts the documented behavior.

---

⚠️ Moderate Issues

4. Unused CLI Flags

The PR adds --global-path and --local-path flags (cli.rs:24-30) but they're never used in the execute methods:

pub global_path: Option<PathBuf>,  // ❌ Not used anywhere
pub local_path: Option<PathBuf>,   // ❌ Not used anywhere

Recommendation: Either implement these flags or remove them from this PR.

5. No Mutual Exclusivity Validation

The --config and --no-config flags should be mutually exclusive but aren't validated:

# This is allowed but shouldn't be:
ccsync to-local --config my.toml --no-config

Recommendation: Add clap conflict:

#[arg(long, global = true, conflicts_with = "no_config")]
pub config: Option<PathBuf>,

#[arg(long, global = true, conflicts_with = "config")]
pub no_config: bool,

6. Incomplete Flag Merging

The merge_cli_flags function only handles 3 settings:

• dry_run
• conflict_strategy
• include patterns

But ignores:

• verbose (though used directly from options)
• yes_all (though used directly from options)
• ignore patterns from config
• Other config options

Recommendation: Document this behavior clearly or expand the merging logic.

---

📝 Minor Issues

7. Documentation Inconsistency

The README mentions .ccsync.local is "gitignored" but there's no .gitignore entry shown.

Recommendation: Add to .gitignore:

.ccsync.local

8. Inconsistent Comments

Some comments say "CLI flags override config file settings" (to_local.rs:112) but the actual behavior is more nuanced (some fields are additive, others use Some() wrapping).

Recommendation: Update comments to reflect the actual precedence logic.

---

🎯 Performance Considerations

No performance issues identified. Config loading happens once per command execution.

---

🔒 Security Assessment

✅ Good security practices:

• Config paths validated (discovery.rs:44-50)
• Symlinks not followed (discovery.rs:78, 100)
• No unsafe code
• Proper error handling without panics

---

🧪 Recommendations Before Merge

High Priority:

1. ✅ Add integration tests for new flags and config loading
2. ✅ Extract duplicated code into shared implementation
3. ✅ Fix array merge behavior to match documentation

Medium Priority:

4. ✅ Implement or remove unused --global-path and --local-path flags
5. ✅ Add mutual exclusivity validation for --config and --no-config
6. ✅ Add .ccsync.local to .gitignore

Low Priority:

7. Update comments to accurately reflect merge behavior
8. Consider adding a warning when both --config and --no-config are used (if mutual exclusivity not enforced)

---

Conclusion

This is a solid foundation for config file integration with good error handling and security practices. However, the missing tests, code duplication, and incorrect array merge behavior need to be addressed before merging.

Overall Assessment: Request Changes

The refactoring work is excellent, but the implementation needs polish to match the documented behavior and reduce maintenance burden.

[claude]

Code Review: PR #9 - Config File Loading Integration

Thank you for this well-structured PR! The integration of config file loading is clean and follows good Rust practices. Here's my detailed review:

---

✅ Strengths

Excellent Code Quality

• SyncOptions Pattern: Great refactoring reducing function parameters from 7 to 3
• Error Handling: Smart distinction between explicit config failures (hard error) and optional config not found (graceful fallback)
• Lifetime Management: Proper use of lifetime parameter 'a in SyncOptions
• CLI Conflicts: Mutual exclusivity between --config and --no-config properly enforced
• Documentation: Good README updates with clear examples

Good Architecture

• Clean separation of concerns with new common.rs module
• Appropriate use of clippy allowances (struct_excessive_bools, fn_params_excessive_bools)
• #[must_use] on constructor prevents accidental discarding

---

🔴 Critical Issues

1. Unused CLI Flags (High Priority)

Three CLI flags are defined but never used:

// cli.rs:26-42
pub global_path: Option<PathBuf>,    // ❌ UNUSED
pub local_path: Option<PathBuf>,     // ❌ UNUSED  
pub preserve_symlinks: bool,         // ❌ UNUSED

Impact: Users see these in --help and may try to use them, but they have no effect.

Recommendation: Either implement them, remove them, or mark as "not yet implemented" in help text.

2. Significant Code Duplication (High Priority)

Identical methods duplicated in to_local.rs and to_global.rs:

• get_global_path()
• get_local_path()
• merge_cli_flags()
• convert_conflict_mode()
• build_type_patterns()

Recommendation: Move to common.rs as free functions or add to SyncOptions:

// In common.rs
impl<'a> SyncOptions<'a> {
    pub fn get_global_path() -> anyhow::Result<PathBuf> { ... }
    pub fn get_local_path() -> anyhow::Result<PathBuf> { ... }
    pub fn merge_cli_flags(&self, config: &mut Config, ...) { ... }
}

---

⚠️ Important Issues

3. Conflict Strategy Always Overrides Config

// merge_cli_flags() line 107
config.conflict_strategy = Some(Self::convert_conflict_mode(conflict));

Since clap provides default_value = "fail", this always overrides the config file, even when users don't explicitly pass --conflict.

Scenario: User sets conflict_strategy = "newer" in .ccsync → always overridden to "fail"

Recommendation: Only override if explicitly provided, or document this behavior clearly.

4. Type Pattern Merge is Additive

// Line 112
config.include.extend(cli_patterns);

Behavior: --type=skills adds to config patterns, doesn't replace them.

Example:

• Config: include = ["agents/**"]
• Command: ccsync to-local --type=skills
• Result: Syncs both agents AND skills

Is this intended? Users might expect --type to mean "sync ONLY this type."

Recommendation: Document this additive behavior clearly in README.

5. String-Based Error Detection is Fragile

// Line 64 in to_local.rs/to_global.rs
let err_msg = e.to_string();
if err_msg.contains("User aborted") {

Issue: Breaks if error message text changes.

Recommendation: Use typed errors:

pub enum SyncError {
    UserAborted,
    // ...
}

// Then match on error type instead of string

6. process::exit() Prevents Cleanup

std::process::exit(0);  // Line 66

Issues:

• Destructors don't run
• Can't test this code path
• Can't use as library

Recommendation: Return error from command, handle exit in main().

---

📋 Medium Priority

7. Missing Test Coverage

Current tests don't cover:

• Config file loading behavior
• CLI flag precedence over config
• --no-config functionality
• --config <path> with non-existent file
• Error handling scenarios

Recommendation: Add integration tests:

#[test]
fn test_config_flag_with_nonexistent_file() {
    Command::cargo_bin("ccsync").unwrap()
        .args(["--config", "/nonexistent.toml", "to-local"])
        .assert()
        .failure()
        .stderr(predicate::str::contains("Failed to load config file"));
}

8. API Ergonomics

Consider implementing From<&Cli> for cleaner usage:

impl<'a> From<&'a Cli> for SyncOptions<'a> {
    fn from(cli: &'a Cli) -> Self {
        Self::new(cli.verbose, cli.dry_run, cli.yes_all, 
                  cli.config.as_deref(), cli.no_config)
    }
}

// Usage in main.rs
let options = SyncOptions::from(&cli);

9. Documentation Gaps

merge_cli_flags() lacks doc comments explaining:

• In-place mutation
• Precedence rules
• Additive behavior

---

🔒 Security Notes

Home Directory Validation

let home = std::env::var("HOME")
    .or_else(|_| std::env::var("USERPROFILE"))

Concern: No validation of path. Malicious environment could set HOME=/etc.

Recommendation: Validate path exists and is a directory before use.

---

📊 Test Results (from PR description)

✅ 109/109 tests passing
✅ Zero clippy warnings
✅ Backward compatible

---

🎯 Summary

This PR delivers solid functionality with good architectural decisions. The main concerns are:

Must Fix:

1. Unused CLI flags (remove or implement)
2. Code duplication between to_local/to_global

Should Fix:
3. Conflict strategy override behavior
4. String-based error detection
5. process::exit() usage
6. Add integration tests for config loading

Nice to Have:
7. Improve API ergonomics
8. Better documentation
9. Home directory validation

Overall: Good work! The core functionality is sound, but addressing the code duplication and unused flags would significantly improve maintainability.

---

Review performed with cargo clippy, comprehensive code analysis, and Rust best practices evaluation.