Conversation
Add temp-clone support to run fyai on remote Git repos
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis pull request introduces remote repository support to the application, adding new CLI options ( Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI Handler
participant Main as main()
participant Repo as repository::run_on_repository()
participant Git as Git Command
participant TempDir as Temp Directory
participant Config as run_with_config()
User->>CLI: Provide --repo URL, --repo-branch, output path
CLI->>Main: Pass parsed arguments
Main->>Repo: Call run_on_repository(url, branch, config)
Repo->>Git: Execute git clone [--branch] --depth 1
Git->>TempDir: Create repository copy
TempDir->>Repo: Return TempDir handle & path
Repo->>Config: Update config.directory to cloned path
Repo->>Config: Call run_with_config(updated_config)
Config->>Config: Process files in cloned repository
Config->>User: Write results to output file
Repo->>TempDir: Drop TempDir (cleanup)
TempDir->>User: Cleanup complete, return success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds support for processing a remote git repository by cloning it into a temporary directory, running the existing file-combine flow on the clone, and cleaning up afterward. This also introduces a typed AppError error model and updates tests/docs to match.
Changes:
- Add
--repo(+ optional--repo-branch/--repo-commit) and implement clone+run workflow via newrepositorymodule. - Introduce
AppError/AppResult(viathiserror) and migrate clipboard/config/init flows + tests to structured errors. - Add README + changelog updates and repository integration tests.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/repository.rs | Implements git clone + optional checkout into TempDir, then runs processing on the clone |
| src/cli.rs | Adds --repo, --repo-branch, --repo-commit CLI flags |
| src/main.rs | Routes execution through repository flow when --repo is provided; adds typed error handling + clipboard fallback behavior |
| src/error.rs | Defines AppError / AppResult |
| src/config.rs | Switches config parsing + YAML parse errors to AppError |
| src/clipboard.rs | Converts clipboard errors into AppError |
| src/tests/repository_tests.rs | Adds tests for remote-repo processing, commit checkout, and temp cleanup |
| src/tests/*.rs, src/tests/mod.rs | Updates tests to match typed errors and registers new test module |
| README.md | Documents remote repo usage + subcommands |
| CHANGELOG.md | Adds release notes for the feature |
| Cargo.toml / Cargo.lock | Adds thiserror and promotes tempfile to non-dev dependency |
Comments suppressed due to low confidence (1)
src/config.rs:140
ExplicitFlagsis intended to track which CLI values were explicitly provided vs clap defaults, but*_setis computed viatry_get_one, which will beSomeeven when values come fromdefault_value(e.g., directory/output). This causes config-file values fordirectory/outputto never take effect. UseArgMatches::value_source(or similar) to detectValueSource::CommandLineinstead of presence.
pub fn config_from_matches(matches: clap::ArgMatches) -> AppResult<(Config, ExplicitFlags)> {
let directory_set = match matches.try_get_one::<String>("directory") {
Ok(Some(_)) => true,
Ok(None) => false,
Err(_) => false,
};
let output_set = match matches.try_get_one::<String>("output") {
Ok(Some(_)) => true,
Ok(None) => false,
Err(_) => false,
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let output = Command::new("git") | ||
| .arg("-C") | ||
| .arg(&clone_path) | ||
| .args(["checkout", commit]) |
There was a problem hiding this comment.
git checkout is invoked with the user-provided commit as a raw argument. If the commit string starts with -, git can parse it as an option (option injection). Validate commit (and potentially branch) to reject leading -, or use a safer invocation pattern that prevents option parsing.
| .args(["checkout", commit]) | |
| .arg("checkout") | |
| .arg("--") | |
| .arg(commit) |
| fn init_sample_repo() -> TempDir { | ||
| let repo_dir = TempDir::new().expect("create temp repo"); | ||
|
|
||
| let status = Command::new("git") | ||
| .args(["init", "."]) | ||
| .current_dir(repo_dir.path()) | ||
| .status() | ||
| .expect("init git repo"); |
There was a problem hiding this comment.
These tests assume the git executable is available on the test runner; if it isn't, they will fail with expect("init git repo"). Consider adding a quick git --version probe at the start and early-return/skip when git isn't installed to keep the suite portable.
| serde_yaml = "0.9.34" | ||
| walkdir = "2.5.0" | ||
| tempfile = "3.19.1" | ||
| thiserror = "1.0" |
There was a problem hiding this comment.
Adding a direct dependency on thiserror = "1.0" introduces a second thiserror major version alongside an existing transitive thiserror 2.x (see Cargo.lock), increasing compile time and binary size. Prefer aligning to the already-used major version (if MSRV allows) or otherwise pin/justify the dual-version situation.
| thiserror = "1.0" | |
| thiserror = "2" |
| Arg::new("repo") | ||
| .long("repo") | ||
| .value_name("URL") | ||
| .help("Clone a git repository (GitHub/GitLab) into a temporary directory before processing").conflicts_with("directory"), |
There was a problem hiding this comment.
--repo is declared as conflicts_with("directory"), but directory has a default_value("."). With clap defaults treated as present, this makes --repo conflict even when the user did not pass --dir, effectively breaking the remote-repo feature. Consider removing the default on directory (set it in code when --repo is absent) or using clap value-source/conditional defaults so the conflict only triggers when --dir is explicitly provided.
| .help("Clone a git repository (GitHub/GitLab) into a temporary directory before processing").conflicts_with("directory"), | |
| .help("Clone a git repository (GitHub/GitLab) into a temporary directory before processing"), |
| if let Some(branch) = branch { | ||
| cmd.args(["--branch", branch]); | ||
| } | ||
| cmd.arg(repo_url).arg(&clone_path); |
There was a problem hiding this comment.
repo_url is passed to git clone without an end-of-options delimiter. A malicious or accidental value starting with - can be interpreted as extra git clone flags (option injection). Add -- before the URL (if supported) and/or validate repo_url to reject values starting with -.
| cmd.arg(repo_url).arg(&clone_path); | |
| cmd.arg("--").arg(repo_url).arg(&clone_path); |
Summary by CodeRabbit
New Features
--repo,--repo-branch, and--repo-commitCLI optionsChanged
--repo-commitFixed
--repoand--dirTests