From 021db8349fb78481071a4daf20b52b42e6cedbec Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Sat, 22 Aug 2026 05:09:54 -0500 Subject: [PATCH] fix(cli): refuse an unknown --repo filter on add, commit, and push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unknown --repo name matched zero repositories and the command reported success having done nothing. With a modified file present in the worktree, `gr add . --repo missing` printed "No changes to stage." and exited 0. There was a change to stage; gr said there was not. That is a false claim about the working tree, which is what justifies a fix rather than a documentation note. validate_repo_filters_known already existed in src/core/repo.rs, already produced the right diagnostic, and already carried a basename-matching branch that names the intended repository. It was not called from add, commit, or push. This calls it from all three. Wired to CliOutcomeError::refusal. outcome.rs defines EXIT_REFUSED = 2 and carries a test named refusal_is_distinct_from_operational_failure, so the codebase already separates a refusal from an operational failure. pr/merge uses the same mapping. Three regression tests assert that an unknown --repo name exits 2 with the diagnostic, one per verb. A control asserts that a KNOWN name still reaches the work: it writes a file, runs the command, and asserts that file is present in the repository's index. It asserts the destination rather than the absence of a message. Mutation-verified in both directions. Removing add.rs's validation call turns exactly the add rejection test red while commit and push stay green. Making run_add return early after validation turns exactly the control red while the three rejection tests stay green. Other commands accept a repo filter without validating it. Identifying which requires reading each command's behavior rather than grepping an identifier, and is deliberately left to a follow-on so each batch is reviewed on its own. Ref #886 — closes at promotion. Premium boundary: grip is OSS — local workspace orchestration, no identity, org, or entitlement semantics. --- src/cli/commands/add.rs | 6 ++- src/cli/commands/commit.rs | 6 ++- src/cli/commands/push.rs | 8 ++- tests/cli_tests.rs | 101 +++++++++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+), 3 deletions(-) diff --git a/src/cli/commands/add.rs b/src/cli/commands/add.rs index b05be190..d7649dfd 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 20ad923a..88f86b7e 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 adf0e45f..84ec0ce8 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 5be90a70..a08253d5 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:?}" + ); +}