diff --git a/src/cli/commands/add.rs b/src/cli/commands/add.rs index b05be19..d7649df 100644 --- a/src/cli/commands/add.rs +++ b/src/cli/commands/add.rs @@ -1,9 +1,10 @@ //! Add command implementation +use crate::cli::outcome::CliOutcomeError; use crate::cli::output::Output; use crate::core::manifest::Manifest; use crate::core::manifest_paths; -use crate::core::repo::{filter_repos, RepoInfo}; +use crate::core::repo::{filter_repos, validate_repo_filters_known, RepoInfo}; use crate::git::cache::invalidate_status_cache; use crate::git::{get_workdir, open_repo, path_exists}; use crate::util::log_cmd; @@ -19,6 +20,9 @@ pub fn run_add( repos_filter: Option<&[String]>, group_filter: Option<&[String]>, ) -> anyhow::Result<()> { + validate_repo_filters_known(manifest, repos_filter) + .map_err(|error| CliOutcomeError::refusal(error.to_string()))?; + Output::header("Checking repositories for changes to stage..."); println!(); diff --git a/src/cli/commands/commit.rs b/src/cli/commands/commit.rs index 20ad923..88f86b7 100644 --- a/src/cli/commands/commit.rs +++ b/src/cli/commands/commit.rs @@ -1,9 +1,10 @@ //! Commit command implementation +use crate::cli::outcome::CliOutcomeError; use crate::cli::output::Output; use crate::core::manifest::Manifest; use crate::core::manifest_paths; -use crate::core::repo::{filter_repos, RepoInfo}; +use crate::core::repo::{filter_repos, validate_repo_filters_known, RepoInfo}; use crate::git::cache::invalidate_status_cache; use crate::git::{get_workdir, open_repo, path_exists}; use crate::util::log_cmd; @@ -22,6 +23,9 @@ pub fn run_commit( repos_filter: Option<&[String]>, group_filter: Option<&[String]>, ) -> anyhow::Result<()> { + validate_repo_filters_known(manifest, repos_filter) + .map_err(|error| CliOutcomeError::refusal(error.to_string()))?; + if !json { Output::header("Committing changes..."); println!(); diff --git a/src/cli/commands/push.rs b/src/cli/commands/push.rs index adf0e45..84ec0ce 100644 --- a/src/cli/commands/push.rs +++ b/src/cli/commands/push.rs @@ -1,8 +1,11 @@ //! Push command implementation +use crate::cli::outcome::CliOutcomeError; use crate::cli::output::Output; use crate::core::manifest::Manifest; -use crate::core::repo::{filter_repos, get_manifest_repo_info, RepoInfo}; +use crate::core::repo::{ + filter_repos, get_manifest_repo_info, validate_repo_filters_known, RepoInfo, +}; use crate::git::remote::{force_push_branch, push_branch}; use crate::git::{get_current_branch, open_repo, path_exists}; use git2::Repository; @@ -27,6 +30,9 @@ pub fn run_push( repos_filter: Option<&[String]>, group_filter: Option<&[String]>, ) -> anyhow::Result<()> { + validate_repo_filters_known(manifest, repos_filter) + .map_err(|error| CliOutcomeError::refusal(error.to_string()))?; + if !json { if force { Output::header("Force pushing changes..."); diff --git a/tests/cli_tests.rs b/tests/cli_tests.rs index 5be90a7..a08253d 100644 --- a/tests/cli_tests.rs +++ b/tests/cli_tests.rs @@ -475,3 +475,104 @@ fn test_checkout_remove_rejects_extra_positional_args() { "unexpected extra arguments after checkout name", )); } + +// --- #196: repo-filter validation on the staging/commit/push verbs ------------- +// +// `validate_repo_filters_known` already existed and already produced the right +// message, including a basename suggestion for the common mistake that surfaced +// this: a manifest entry named `-` checked out at `./`, where +// the operator naturally types `--repo `. These three verbs did not call +// the validator, so an unknown `--repo` name matched zero repos and the command +// reported success. +// +// The failure direction is what makes it worth a test: another agent reads +// "pushed" or "staged" and acts on it. + +#[test] +fn test_add_unknown_repo_filter_is_refused_not_silently_empty() { + let ws = WorkspaceBuilder::new().add_repo("app").build(); + + let mut cmd = Command::cargo_bin("gr").unwrap(); + cmd.current_dir(&ws.workspace_root) + .arg("add") + .arg(".") + .arg("--repo") + .arg("missing") + .assert() + .code(2) + .stderr(predicate::str::contains( + "repo filter 'missing' not found in local manifest", + )); +} + +#[test] +fn test_commit_unknown_repo_filter_is_refused_not_silently_empty() { + let ws = WorkspaceBuilder::new().add_repo("app").build(); + + let mut cmd = Command::cargo_bin("gr").unwrap(); + cmd.current_dir(&ws.workspace_root) + .arg("commit") + .arg("-m") + .arg("msg") + .arg("--repo") + .arg("missing") + .assert() + .code(2) + .stderr(predicate::str::contains( + "repo filter 'missing' not found in local manifest", + )); +} + +#[test] +fn test_push_unknown_repo_filter_is_refused_not_silently_empty() { + let ws = WorkspaceBuilder::new().add_repo("app").build(); + + let mut cmd = Command::cargo_bin("gr").unwrap(); + cmd.current_dir(&ws.workspace_root) + .arg("push") + .arg("--repo") + .arg("missing") + .assert() + .code(2) + .stderr(predicate::str::contains( + "repo filter 'missing' not found in local manifest", + )); +} + +/// Control for the three tests above: a KNOWN repo name must still REACH THE +/// WORK, so the rejections cannot be passing merely because `gr add` fails for +/// some unrelated reason in this fixture. +/// +/// The control asserts the DESTINATION, not the absence of a message. An +/// earlier version of this test wrote no file and checked only that one +/// substring was missing from stderr — which passes identically whether `add` +/// stages the file or does nothing at all, and those are exactly the two +/// outcomes a control has to separate. The fixture commits its files before +/// cloning, so the worktree starts clean and the test must dirty it itself. +#[test] +fn test_add_known_repo_filter_reaches_the_work_and_stages() { + let ws = WorkspaceBuilder::new().add_repo("app").build(); + let repo = ws.repo_path("app"); + std::fs::write(repo.join("control.txt"), "dirty\n").unwrap(); + + let mut cmd = Command::cargo_bin("gr").unwrap(); + cmd.current_dir(&ws.workspace_root) + .arg("add") + .arg(".") + .arg("--repo") + .arg("app") + .assert() + .success() + .stderr(predicate::str::contains("not found in local manifest").not()); + + let staged = std::process::Command::new("git") + .args(["diff", "--cached", "--name-only"]) + .current_dir(&repo) + .output() + .unwrap(); + let staged = String::from_utf8_lossy(&staged.stdout); + assert!( + staged.contains("control.txt"), + "known --repo name must reach the work: expected control.txt in the index, got {staged:?}" + ); +}