feat(web-cli): git-sync subcommand

[PrzemyslawKlys]

Adds a first-class CLI subcommand for repository sync so you can pull public/private repos locally (Linux/macOS/Windows) without needing to wrap it in a pipeline.

New:

• powerforge-web git-sync (--repo --destination | --spec <file.json>) [--output json]
• Uses the same implementation as the pipeline git-sync step, including okenEnv auth, sparse checkout, lock files, and manifest output.

Tests:

• Adds a CLI test that clones a local git repo via the new subcommand.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[claude]

PR Review: feat(web-cli): git-sync subcommand

Summary

This PR adds a CLI subcommand git-sync that exposes the existing pipeline git-sync functionality as a first-class CLI command. The implementation is well-structured and follows existing patterns in the codebase.

Code Quality ✅

Strengths

1. Consistent with codebase patterns: The implementation follows the established pattern used by other CLI commands (e.g., nav-export, build)
2. Proper partial class usage: New command handler is in a separate partial class file (WebCliCommandHandlers.GitSyncCommands.cs), maintaining good separation of concerns
3. Comprehensive option aliases: Supports multiple option name variants (e.g., --repo/--repository/--url), improving usability
4. Good test coverage: Includes a basic integration test that validates core functionality with a local git repository
5. Reuses existing logic: Correctly delegates to ExecuteGitSync via the new RunGitSyncStepForCli method, avoiding code duplication
6. JSON output support: Properly implements --output json mode with consistent envelope structure

Code Structure

• Handler follows the standard pattern: parse args → validate → execute → format output
• Error handling is consistent with other commands using the Fail helper
• Test structure mirrors existing tests (e.g., WebPipelineRunnerGitSyncTests.cs)

Potential Issues 🔍

1. Security: Token Handling (Low Priority)

The code accepts --token as a CLI argument (line 55 in WebCliCommandHandlers.GitSyncCommands.cs).

Concern: Passing tokens via CLI arguments can expose them in:

• Process listings (ps aux)
• Shell history
• CI/CD logs

Recommendation: Consider adding a warning in documentation or even in the CLI help text that --token-env is preferred over --token for security. The existing HasInlineGitToken check in ExecuteGitSync already warns about inline tokens in config files, which is good.

2. Input Validation (Medium Priority)

The validation for required parameters is minimal.

Missing validations:

• No validation that destination path is safe (potential path traversal)
• No validation on integer options (e.g., negative --depth, excessive --timeout-seconds)
• The spec file path is resolved but not validated for existence before parsing

Recommendation:

• Add basic range checks for integer options
• Consider using the existing path validation/sanitization patterns used elsewhere in the codebase

3. Test Coverage (Medium Priority)

The test file WebCliGitSyncTests.cs has good basic coverage but could be enhanced:

Current coverage:

• ✅ Basic clone operation
• ✅ Git availability check
• ✅ Cleanup handling

Missing coverage:

• ❌ Error scenarios (missing git, invalid repo, permission errors)
• ❌ JSON output mode validation
• ❌ Option parsing (various flags and their combinations)
• ❌ Spec file mode (--spec)

Recommendation: Add tests for error cases, JSON output validation, and spec file mode.

4. Documentation (Low Priority)

The usage line added to PrintUsage() is good, but the many optional parameters aren't shown in the help text (unlike audit or doctor commands which list their options).

Recommendation: Consider adding a more detailed help section or at least mentioning key options like --ref, --clean, --token-env in a follow-up documentation PR.

Performance Considerations ✅

No performance concerns:

• Delegates to existing ExecuteGitSync which has been in production
• Doesn't introduce new blocking operations
• Proper timeout handling inherited from existing implementation

Security Concerns ⚠️

Authentication

1. Token exposure (mentioned above): CLI argument tokens are less secure than environment variables
2. Auth type validation: The code accepts arbitrary --auth-type values without validation, passing them through to the underlying implementation
   • Recommendation: Document valid auth types or add validation

Path Safety

The code resolves paths but doesn't validate against path traversal. Ensure destination paths are validated/sanitized, especially since this is a CLI tool that might be used in automated contexts.

Best Practices Assessment ✅

Follows Repo Conventions

From CONTRIBUTING.md:

• ✅ Core logic remains in the existing WebPipelineRunner.Tasks.GitSync.cs
• ✅ CLI handler is thin - just parameter binding and calling core service
• ✅ Uses typed models (JSON element deserialization)
• ✅ Has xUnit test coverage

Error Handling

• ✅ Uses consistent error patterns with Fail() helper
• ✅ Proper try-catch with JSON error output
• ✅ Returns appropriate exit codes (0, 1, 2)

Code Style

• ✅ Consistent naming conventions
• ✅ Proper use of string interpolation
• ✅ Good use of null-conditional operators
• ✅ Follows C# formatting standards visible in other files

Recommendations Summary

High Priority

None - the PR is solid for merge

Medium Priority

1. Add validation for integer option ranges
2. Add path traversal protection for destination paths
3. Expand test coverage for error cases and JSON output

Low Priority

1. Document security best practices for token handling in help/docs
2. Add more detailed usage examples in help text
3. Consider validating auth-type values

Conclusion

This is a well-implemented PR that follows the established patterns in the codebase. The code quality is good, and the feature addresses a legitimate need for exposing git-sync functionality directly via CLI.

Recommendation: ✅ Approve with minor suggestions

The medium-priority items can be addressed in follow-up PRs if desired. The current implementation is production-ready and maintains consistency with the existing codebase standards.

Great work on maintaining the architectural separation between CLI handlers and core logic! 🎉

[claude]

Pull Request Review: git-sync CLI Subcommand

Summary

This PR adds a first-class CLI subcommand for repository synchronization, allowing users to clone/sync public and private repositories locally. The implementation follows the existing codebase patterns well and reuses the proven pipeline git-sync implementation.

---

Code Quality

✅ Strengths

1. Excellent code reuse: The implementation properly delegates to the existing ExecuteGitSync method (WebPipelineRunner.Tasks.GitSync.cs:60-156), avoiding duplication and maintaining consistency with the pipeline implementation.

2. Comprehensive parameter mapping: The CLI handler (WebCliCommandHandlers.GitSyncCommands.cs:11-126) supports both kebab-case and camelCase parameter variants, providing a great user experience (e.g., --token-env and --tokenEnv).

3. Consistent error handling: The error handling pattern matches existing CLI commands, with proper JSON output support and user-friendly error messages.

4. Good test coverage: The test case covers the happy path with proper setup and cleanup, following established test patterns in the codebase.

---

Potential Issues & Recommendations

🔴 High Priority

1. Missing Namespace Declaration (WebCliGitSyncTests.cs:1-113)

The test file is missing a namespace declaration, unlike other test files in the project.

Current:

using System;
using System.Diagnostics;
using System.IO;
using PowerForge.Web.Cli;
using Xunit;

public class WebCliGitSyncTests

Recommended:

using System;
using System.Diagnostics;
using System.IO;
using PowerForge.Web.Cli;
using Xunit;

namespace PowerForge.Tests;

public class WebCliGitSyncTests

This maintains consistency with other test files like WebPipelineRunnerGitSyncTests.cs:8.

2. ParseIntOption Helper Missing Validation (WebCliCommandHandlers.GitSyncCommands.cs:50-53)

The ParseIntOption helper is called but not visible in the diff. Ensure it handles invalid input gracefully (non-numeric strings, overflow).

Verify it includes error handling:

private static int ParseIntOption(string? value, int defaultValue)
{
    if (string.IsNullOrWhiteSpace(value))
        return defaultValue;
    
    return int.TryParse(value, out var result) ? result : defaultValue;
    // Consider: Should invalid input throw an exception or use default?
}

🟡 Medium Priority

3. Inconsistent Boolean Option Handling (WebCliCommandHandlers.GitSyncCommands.cs:48-64)

The HasOption checks don't validate option values. Users might expect --clean=false to work, but it would still be treated as true.

Consider supporting:

• --clean (true)
• --clean=true (true)
• --clean=false (false)
• No flag (false)

This could be confusing if users try --writeManifest=false expecting it to disable the feature.

4. Test Could Verify More Edge Cases (WebCliGitSyncTests.cs:11-49)

The single test only covers the happy path. Consider adding tests for:

• Missing required parameters (--repo without --destination)
• Invalid repository paths
• JSON spec file input (--spec option)
• Various authentication scenarios
• Error conditions (git not available, permission denied)

Existing tests in WebPipelineRunnerGitSyncTests.cs show good patterns to follow.

5. Potential Information Disclosure (WebCliCommandHandlers.GitSyncCommands.cs:88)

When outputting errors in JSON mode, the full exception message is exposed:

Error = ex.Message

If the git-sync operation fails with authentication tokens in error messages, this could leak sensitive data. The existing implementation in WebPipelineRunner.Tasks.GitSync.cs uses FirstNonEmptyLine to preview errors - consider applying similar filtering here.

---

Security Considerations

✅ Security Strengths

1. Token handling: The code properly supports --token-env for environment variable-based authentication (line 54), avoiding inline tokens in command history.

2. Warning for inline tokens: The underlying implementation (WebPipelineRunner.Tasks.GitSync.cs:66-70) warns when inline tokens are detected.

🟡 Recommendations

Authentication Token in Command Line Arguments

While the code supports --token (line 55), this is inherently insecure as command-line arguments are visible in process lists and shell history.

Recommendation: Document that --token-env is preferred, or consider deprecating --token entirely for CLI usage. The pipeline usage (in JSON files) has different security implications than CLI arguments.

---

Performance Considerations

✅ Good Performance Practices

1. Efficient JSON handling: Uses JsonDocument with proper disposal patterns
2. Reuses existing implementation: No performance regression risk from duplication
3. Timeout configuration: Properly exposes timeout controls (--timeout-seconds)

---

Testing Assessment

Current Coverage

• ✅ Basic clone operation works
• ✅ Proper temp directory setup/cleanup
• ✅ Git availability check

Missing Coverage

• ❌ Error scenarios (missing git, invalid paths, auth failures)
• ❌ --spec file input mode
• ❌ Advanced options (sparse checkout, submodules, lock modes)
• ❌ JSON output validation
• ❌ Parameter validation (missing required args)

Recommendation: Add at least 2-3 more test cases covering error scenarios and the --spec input mode before merging.

---

Minor Improvements

🔵 Nice-to-Have

1. Help text ordering (WebCliHelpers.cs:23): The git-sync command appears in the middle of the list. Consider grouping related commands together logically.

2. Parameter naming consistency: The code accepts many aliases (--repo, --repository, --url). While user-friendly, this increases maintenance burden. Document which is the "canonical" name.

3. Missing XML documentation: The new HandleGitSync method lacks XML documentation comments. Other handlers in the codebase appear to also lack this, so this is consistent but worth noting.

---

Summary & Recommendation

Overall Assessment: Good quality implementation with minor issues

Before Merging:

1. ✅ Must fix: Add namespace declaration to WebCliGitSyncTests.cs
2. ⚠️ Should fix: Add error scenario test cases
3. ⚠️ Should verify: ParseIntOption handles invalid input gracefully
4. 💡 Consider: Document security implications of --token vs --token-env

Approval Status

The code is well-structured and follows existing patterns. The critical issue (missing namespace) is trivial to fix. I recommend approval with minor changes - specifically adding the namespace and at least one error scenario test.

Great work on maintaining consistency with the existing codebase and providing comprehensive parameter support! 🎉

[claude]

Pull Request Review: feat(web-cli): git-sync subcommand

I've reviewed PR #119 which adds a first-class CLI subcommand for repository synchronization. Overall, this is a well-implemented feature that follows the existing codebase patterns.

✅ Strengths

1. Code Reuse: Excellent reuse of existing ExecuteGitSync pipeline logic via the new RunGitSyncStepForCli method
2. Comprehensive Testing: Test suite covers multiple scenarios including basic cloning, spec-mode operation, and validation failures
3. Security-Conscious: RedactGitSyncSecrets function properly sanitizes tokens from error messages
4. Consistent API Design: Follows existing CLI patterns (dispatch routing, JSON output, option aliases)
5. Flexibility: Multiple option aliases improve usability

🔍 Code Quality Issues

1. Secret Redaction Limitation (WebCliCommandHandlers.GitSyncCommands.cs:128-143)

The RedactGitSyncSecrets function only collects tokens from the step JSON element, but tokens may also come from environment variables via tokenEnv. If a token is loaded from an environment variable, it won't be in the stepElement JSON and thus won't be redacted from error messages.

Recommendation: Also collect the token from the environment variable when redacting.

2. Dictionary Serialization Caveat (WebCliCommandHandlers.GitSyncCommands.cs:42-70)

The dictionary to JSON to JsonElement round-trip is fragile - integer values may be boxed incorrectly, null values may cause serialization issues, and the approach is inefficient.

Recommendation: Consider building a proper typed object for better type safety and performance.

3. Test Cleanup Robustness (WebCliGitSyncTests.cs:159-170)

The test cleanup swallows all exceptions. On Windows, git processes may hold locks on the .git directory causing cleanup failures.

Recommendation: Consider logging cleanup failures in CI environments for debugging.

4. Missing Git Availability Check

Tests have IsGitAvailable() to gracefully skip when git is missing, but main code will throw a generic exception.

Enhancement: Consider checking for git availability upfront with a clearer error message.

🔒 Security Considerations

• ✅ Token Sanitization: Good implementation
• ✅ No Command Injection: All git arguments properly added via ArgumentList
• ⚠️ Partial Redaction: Environment-sourced tokens may leak in errors

🧪 Test Coverage Assessment

Covered:

• ✅ Basic clone operation
• ✅ Spec file mode
• ✅ Missing destination validation
• ✅ Git availability check

Missing:

• ❌ Token redaction verification
• ❌ Option aliases testing
• ❌ Error handling when git clone fails
• ❌ JSON output format validation
• ❌ Multiple option combinations

🚀 Recommendations Summary

Critical: Fix token redaction to include environment-sourced tokens

High Priority: Add test coverage for secret redaction, consider refactoring dictionary serialization

Medium Priority: Improve test cleanup logging, add comprehensive option validation tests, consider upfront git availability check

Low Priority: Extract magic strings to constants, add integration test for common scenarios

✅ Approval Status

This PR is functionally sound and ready to merge with the understanding that the token redaction issue should be addressed either before merge or in a quick follow-up. The feature implementation is solid, follows project conventions, and includes reasonable test coverage.

Great work on maintaining consistency with the existing codebase architecture!

---

Reviewed by: Claude Code (Sonnet 4.5)
Review Date: 2026-02-15