v0.12.7

[yimsk]

Changes

• feat: support custom config file path (feat: support custom config file path #146)
  • -c, --config CLI flag
  • CLAWS_CONFIG env var
  • Tilde expansion support
• Add VHS integration tests (Add VHS integration tests with Require assertions #144)

[claude]

Pull Request Review: v0.12.7

This PR adds custom config file path support and VHS integration tests. Overall, the implementation is well-structured with excellent test coverage. Below is my detailed feedback:

---

✅ Strengths

Code Quality

• Excellent test coverage: Comprehensive unit tests for all new functionality (TestSetConfigPath, TestSetConfigPath_TildeExpansion, TestCustomConfigPath_Load)
• Thread-safe implementation: Proper use of sync.RWMutex to protect shared state
• Good separation of concerns: Config path logic cleanly separated from main application logic
• Clear precedence handling: CLI flag > env var > default is well-documented and implemented correctly

Documentation

• Well-documented feature: docs/configuration.md provides clear examples and use cases
• Good usage examples: Shows both CLI flag and env var approaches with practical scenarios

Testing Strategy

• Integration tests: VHS tapes serve dual purpose as both docs generation and integration tests
• Automated validation: GitHub Actions workflow ensures tapes run successfully on CI

---

🔍 Issues & Recommendations

1. Security: File Validation in SetConfigPath (Medium Priority)

Location: internal/config/file.go:51-62

Issue: The function only checks if the file exists using os.Stat(), but does not validate it is actually a file (not a directory) or check read permissions explicitly.

// Current implementation
if _, err := os.Stat(expanded); err != nil {
    return apperrors.Wrap(err, "config file", "path", expanded)
}

Recommendation: Add validation for file type and readability:

func SetConfigPath(path string) error {
    expanded, err := expandTilde(path)
    if err != nil {
        return apperrors.Wrap(err, "config file", "path", path)
    }
    
    // Validate path points to a regular file
    info, err := os.Stat(expanded)
    if err != nil {
        return apperrors.Wrap(err, "config file", "path", expanded)
    }
    if info.IsDir() {
        return apperrors.Wrap(fmt.Errorf("path is a directory, not a file"), "config file", "path", expanded)
    }
    
    configPathMu.Lock()
    customConfigPath = expanded
    configPathMu.Unlock()
    return nil
}

2. Bug: Tilde Expansion Edge Case (Low Priority)

Location: internal/config/file.go:38-47

Issue: expandTilde only handles "/" prefix but not standalone "". While rare for config paths, this could cause unexpected behavior.

Recommendation: Handle the standalone "~" case:

func expandTilde(path string) (string, error) {
    if path == "~" {
        return os.UserHomeDir()
    }
    if strings.HasPrefix(path, "~/") {
        home, err := os.UserHomeDir()
        if err != nil {
            return "", fmt.Errorf("expand ~: %w", err)
        }
        return filepath.Join(home, path[2:]), nil
    }
    return path, nil
}

3. Performance: Integration Test Timeout (Low Priority)

Location: .github/workflows/integration-test.yml:34-45

Issue: LocalStack health check has 30-second timeout with 1-second polling. If LocalStack is slow to start, this could cause flaky test failures.

Recommendation: Increase timeout to 60 seconds for more reliability and add attempt counter in output.

4. Code Quality: Global State Reset (Low Priority)

Location: internal/config/file_test.go:691-815

Issue: Tests manually reset global state with defer blocks. If tests panic, cleanup might not run, affecting subsequent tests.

Recommendation: Use t.Cleanup() for more reliable cleanup:

func resetCustomConfigPath(t *testing.T) {
    t.Helper()
    t.Cleanup(func() {
        configPathMu.Lock()
        customConfigPath = ""
        configPathMu.Unlock()
    })
}

5. Feature Gap: Directory Error Handling (Enhancement)

Issue: No test case for when a directory path is passed instead of a file. This should fail gracefully with a clear error message.

Recommendation: Add test case:

func TestSetConfigPath_Directory(t *testing.T) {
    tmpDir := t.TempDir()
    err := SetConfigPath(tmpDir)
    if err == nil {
        t.Error("SetConfigPath should fail when given a directory")
    }
}

6. VHS Integration Tests: Require Statements (Good Practice)

Positive Note: Excellent use of Require statements in VHS tapes (e.g., Require "Services", Require "Instance ID"). This ensures tests fail fast if expected UI elements are missing.

7. Documentation: Missing CLAWS_CONFIG Example in Docker/CI (Minor)

Location: docs/configuration.md

Recommendation: Add practical CI/CD example:

# Example: Using custom config in GitHub Actions
- name: Run claws with custom config
  env:
    CLAWS_CONFIG: ${{ github.workspace }}/configs/ci-config.yaml
  run: claws -p production

---

🔒 Security Review

✅ No critical security issues found

• Config file paths are validated before use
• No arbitrary code execution paths
• File operations use safe stdlib functions
• Environment variable handling is appropriate

Minor Recommendation:

• Consider documenting in configuration.md that config files should not contain credentials (only settings). Mention that AWS credentials should always be in ~/.aws/credentials or environment variables.

---

⚡ Performance Review

✅ No performance concerns

• Config path resolution happens once at startup
• Mutex usage is appropriate (RWMutex for read-heavy workload)
• File I/O is minimal and unavoidable
• Integration tests run in reasonable time

---

🧪 Test Coverage

Excellent coverage (estimated 95%+ for new code):

• ✅ Valid config path
• ✅ Non-existent file error
• ✅ Tilde expansion
• ✅ Config loading with custom path
• ✅ ConfigDir/ConfigPath return custom path
• ✅ GetConfigPath functionality
• ✅ Concurrent access safety
• ❌ Missing: Directory passed instead of file (should fail gracefully)
• ❌ Missing: Unreadable file (permission denied)

---

📝 Best Practices Observed

1. ✅ Atomic writes: Config saves use atomicWrite with temp file + rename
2. ✅ Thread safety: Proper mutex usage throughout
3. ✅ Error wrapping: Consistent use of apperrors.Wrap with context
4. ✅ Test isolation: Each test uses t.TempDir()
5. ✅ Documentation: Clear inline comments and external docs
6. ✅ Backwards compatibility: Old single-profile format still supported
7. ✅ Integration testing: VHS tapes validate real user workflows

---

🎯 Recommendations Summary

Must Fix (None)

No blocking issues found.

Should Fix (Recommended)

1. Add file type validation in SetConfigPath (reject directories)
2. Handle standalone "~" in expandTilde
3. Add test case for directory path error

Nice to Have

1. Add test cleanup helper using t.Cleanup()
2. Increase LocalStack timeout for CI reliability
3. Document CI/CD usage examples
4. Add security note about not storing credentials in config

---

✅ Approval

This is a high-quality PR with:

• ✅ Clean, well-tested implementation
• ✅ Comprehensive test coverage
• ✅ Good documentation
• ✅ No security vulnerabilities
• ✅ Follows project conventions
• ✅ Thread-safe implementation
• ✅ Excellent integration test strategy

The identified issues are minor improvements rather than blockers. Approved for merge with optional follow-up improvements.

Great work on this feature! The VHS integration testing approach is particularly clever. 🎉

---

Generated by Claude Code Review