fix(checkout): count repo failures and disjoin them into the exit code

docs/IMPLEMENTATION_PLAN.md

@@ -59,7 +59,7 @@ Pass `--hostname` to `gh` CLI in `src/platform/github.rs` `enable_auto_merge()`
 
 ### Phase 3: Repo iteration helper
 - [x] New `src/cli/repo_iter.rs`: `RepoVisitResult`, `RepoOpSummary`, `for_each_repo()`, `for_each_repo_path()`
-- [ ] Wire into commands — Deferred: most commands accumulate custom state that doesn't fit the simple Success/Skipped/Error enum
+- [ ] Wire into commands — Partially done: `checkout` adopted it (2026-08-21). The remaining commands are blocked on `?` propagation out of the loop, three disagreeing skip taxonomies, and per-arm payload types — not on the enum alone
 
 ### Phase 4-6: Migrate all commands to WorkspaceContext
 - [x] 28/30 commands in main.rs use `load_workspace_context()` (Init, Completions, Bench don't need workspace)

docs/PLAN-p2-maintainability.md

@@ -24,7 +24,9 @@ No conflicts expected — P0/P1 didn't touch `main.rs` or `cli/mod.rs`.
 ```
 cargo build && cargo test && cargo clippy && cargo fmt --check
 ```
-Watch for clippy warnings on unused `repo_iter` imports. If flagged, the `pub` visibility from `cli/mod.rs` → `lib.rs` should suppress it.
+~~Watch for clippy warnings on unused `repo_iter` imports. If flagged, the `pub` visibility from `cli/mod.rs` → `lib.rs` should suppress it.~~
+
+**Struck.** This prescribed silencing the one instrument that would have reported the problem. `pub` does not resolve a dead-code warning, it disables the analysis — an unused private item warns, an unused `pub` item does not — so following this step left `repo_iter` with zero callers and nothing anywhere going red for as long as it existed. A warning is a detector; suppressing it to reach a clean build is not a fix, and writing the suppression down as a step made it the default for whoever came next. If an item has no consumer, give it one or delete it.
 
 ### 3. Commit the refactor
 Stage all 4 files and commit:
@@ -59,7 +61,7 @@ gr pr create -t "refactor: P2 maintainability — WorkspaceContext and load_grip
 | Command signature migration to `&WorkspaceContext` | Would touch every command file + every test; current ctx field extraction in dispatch works fine |
 | Compact dispatch function (Phase 7) | 612-line match for 30 commands is standard; only one dispatch site |
 | sync.rs / release.rs decomposition (Phase 8) | Already have helpers (`sync_single_repo`, `execute_post_sync_hooks`, etc.) |
-| Wiring `for_each_repo()` into commands | Most commands accumulate custom state that doesn't fit the simple Success/Skipped/Error enum |
+| Wiring `for_each_repo()` into commands | Largely accurate, and now measured rather than assumed. Of the 9 command files that hand-roll these counters, `sync`/`pull` do not iterate repos at all, `pr/merge` iterates PRs and awaits, and `push`/`commit`/`forall` propagate `?` out of the loop — which the closure's return type cannot express. `checkout` was the one clean fit and has adopted it. |
 
 ## Files touched
 

src/cli/commands/checkout.rs

@@ -1,15 +1,13 @@
 //! Checkout command implementation
 
 use crate::cli::output::Output;
+use crate::cli::repo_iter::{for_each_repo, RepoVisitResult};
 use crate::core::manifest::Manifest;
 use crate::core::repo::{
     filter_repos, get_manifest_repo_info, validate_repo_filters_known, RepoInfo,
 };
 use crate::core::workspace_checkout;
-use crate::git::{
-    branch::{branch_exists, checkout_branch, create_and_checkout_branch},
-    open_repo,
-};
+use crate::git::branch::{branch_exists, checkout_branch, create_and_checkout_branch};
 use std::path::Path;
 
 /// Run the checkout command
@@ -51,71 +49,62 @@ pub fn run_checkout(
     ));
     println!();
 
-    let mut success_count = 0;
-    let mut _skip_count = 0;
-
-    for repo in &repos {
-        if !repo.exists() {
-            Output::warning(&format!("{}: not cloned", repo.name));
-            _skip_count += 1;
-            continue;
-        }
-
-        match open_repo(&repo.absolute_path) {
-            Ok(git_repo) => {
-                let exists = branch_exists(&git_repo, branch_name);
-
-                if create {
-                    // -b flag: create if doesn't exist, checkout if it does
-                    if exists {
-                        match checkout_branch(&git_repo, branch_name) {
-                            Ok(()) => {
-                                Output::success(&format!(
-                                    "{}: checked out (already exists)",
-                                    repo.name
-                                ));
-                                success_count += 1;
-                            }
-                            Err(e) => Output::error(&format!("{}: {}", repo.name, e)),
-                        }
-                    } else {
-                        match create_and_checkout_branch(&git_repo, branch_name) {
-                            Ok(()) => {
-                                Output::success(&format!("{}: created and checked out", repo.name));
-                                success_count += 1;
-                            }
-                            Err(e) => Output::error(&format!("{}: {}", repo.name, e)),
-                        }
-                    }
-                } else {
-                    // Normal checkout: skip if branch doesn't exist
-                    if !exists {
-                        Output::info(&format!("{}: branch doesn't exist, skipping", repo.name));
-                        _skip_count += 1;
-                        continue;
-                    }
-
-                    match checkout_branch(&git_repo, branch_name) {
-                        Ok(()) => {
-                            Output::success(&repo.name);
-                            success_count += 1;
-                        }
-                        Err(e) => Output::error(&format!("{}: {}", repo.name, e)),
+    // Iterating through for_each_repo rather than by hand is what gives this
+    // command an error count at all. The previous loop reported every failure
+    // to the terminal and incremented nothing, so the summary below counted
+    // successes against a total and stayed silent about the difference.
+    let summary = for_each_repo(&repos, false, |repo, git_repo| {
+        let exists = branch_exists(git_repo, branch_name);
+
+        if create {
+            // -b flag: create if doesn't exist, checkout if it does
+            if exists {
+                match checkout_branch(git_repo, branch_name) {
+                    Ok(()) => RepoVisitResult::Success(format!(
+                        "{}: checked out (already exists)",
+                        repo.name
+                    )),
+                    Err(e) => RepoVisitResult::Error(format!("{}: {}", repo.name, e)),
+                }
+            } else {
+                match create_and_checkout_branch(git_repo, branch_name) {
+                    Ok(()) => {
+                        RepoVisitResult::Success(format!("{}: created and checked out", repo.name))
                     }
+                    Err(e) => RepoVisitResult::Error(format!("{}: {}", repo.name, e)),
                 }
             }
-            Err(e) => Output::error(&format!("{}: {}", repo.name, e)),
+        } else if !exists {
+            // Normal checkout: a missing branch is a skip, not a failure.
+            RepoVisitResult::Skipped(format!("{}: branch doesn't exist, skipping", repo.name))
+        } else {
+            match checkout_branch(git_repo, branch_name) {
+                Ok(()) => RepoVisitResult::Success(repo.name.clone()),
+                Err(e) => RepoVisitResult::Error(format!("{}: {}", repo.name, e)),
+            }
         }
-    }
+    });
 
     println!();
     println!(
         "Switched {}/{} repos to {}",
-        success_count,
+        summary.success_count,
         repos.len(),
         Output::branch_name(branch_name)
     );
 
+    // The count above is a ratio a caller has to read and interpret. The exit
+    // code is the only failure signal a script sees, so it has to disjoin the
+    // per-repo failures rather than report the batch as done.
+    if summary.error_count > 0 {
+        anyhow::bail!(
+            "{} of {} repos failed to switch to {}",
+            summary.error_count,
+            repos.len(),
+            branch_name
+        );
+    }
+
     Ok(())
 }
 

src/cli/repo_iter.rs

@@ -5,7 +5,7 @@
 
 use crate::cli::output::Output;
 use crate::core::repo::RepoInfo;
-use crate::git::{open_repo, path_exists};
+use crate::git::open_repo;
 use git2::Repository;
 
 /// Result of visiting a single repo
@@ -45,7 +45,12 @@ where
     };
 
     for repo in repos {
-        if !path_exists(&repo.absolute_path) {
+        // `RepoInfo::exists` tests for `.git`, which is what "cloned" means.
+        // A bare `path_exists` on the directory answers a different question:
+        // a checkout whose `.git` is gone still has its files, so it would
+        // pass that check and then fail to open, turning a not-cloned skip
+        // into an error. This is what the docstring above has always claimed.
+        if !repo.exists() {
             if !quiet {
                 Output::warning(&format!("{}: not cloned", repo.name));
             }
@@ -81,49 +86,3 @@ where
 
     summary
 }
-
-/// Iterate over repos by path (without opening git2::Repository).
-///
-/// Useful for operations that shell out to `git` directly rather than
-/// using libgit2 (e.g., cherry-pick, gc).
-pub fn for_each_repo_path<F>(repos: &[RepoInfo], quiet: bool, mut op: F) -> RepoOpSummary
-where
-    F: FnMut(&RepoInfo) -> RepoVisitResult,
-{
-    let mut summary = RepoOpSummary {
-        success_count: 0,
-        skip_count: 0,
-        error_count: 0,
-    };
-
-    for repo in repos {
-        if !path_exists(&repo.absolute_path) {
-            if !quiet {
-                Output::warning(&format!("{}: not cloned", repo.name));
-            }
-            summary.skip_count += 1;
-            continue;
-        }
-
-        match op(repo) {
-            RepoVisitResult::Success(msg) => {
-                if !quiet {
-                    Output::success(&msg);
-                }
-                summary.success_count += 1;
-            }
-            RepoVisitResult::Skipped(msg) => {
-                if !quiet {
-                    Output::info(&msg);
-                }
-                summary.skip_count += 1;
-            }
-            RepoVisitResult::Error(msg) => {
-                Output::error(&msg);
-                summary.error_count += 1;
-            }
-        }
-    }
-
-    summary
-}

tests/test_checkout.rs

@@ -635,3 +635,67 @@ fn test_absolute_repo_path_in_metadata_cannot_escape_the_checkout() {
         "a rejected reconstruction should explain why, not fail silently"
     );
 }
+
+// ── Every repo fails to open ────────────────────────────────────
+// Witness for the error arms, which previously reported each failure to the
+// terminal and incremented no counter, so the command printed "Switched 0/N"
+// and exited 0. Each .git is replaced by an empty directory: the repo still
+// counts as cloned under either cloned-check, so this exercises the error arm
+// rather than the not-cloned skip arm.
+
+#[test]
+fn test_checkout_all_repos_failing_is_not_success() {
+    let ws = WorkspaceBuilder::new()
+        .add_repo("frontend")
+        .add_repo("backend")
+        .build();
+
+    let manifest = ws.load_manifest();
+
+    for name in ["frontend", "backend"] {
+        let git_dir = ws.repo_path(name).join(".git");
+        std::fs::remove_dir_all(&git_dir).unwrap();
+        std::fs::create_dir(&git_dir).unwrap();
+        assert!(
+            git_dir.exists(),
+            "{name}: .git must still exist for this witness"
+        );
+    }
+
+    let result = gitgrip::cli::commands::checkout::run_checkout(
+        &ws.workspace_root,
+        &manifest,
+        "main",
+        false,
+        None,
+        None,
+    );
+
+    assert!(
+        result.is_err(),
+        "every repo failed to open; checkout must not report success"
+    );
+}
+
+// The companion that keeps the fix from over-correcting: a branch that is
+// simply absent is a skip, not a failure, and must still exit zero.
+#[test]
+fn test_checkout_absent_branch_is_still_success() {
+    let ws = WorkspaceBuilder::new().add_repo("frontend").build();
+    let manifest = ws.load_manifest();
+
+    let result = gitgrip::cli::commands::checkout::run_checkout(
+        &ws.workspace_root,
+        &manifest,
+        "no-such-branch",
+        false,
+        None,
+        None,
+    );
+
+    assert!(
+        result.is_ok(),
+        "an absent branch is a skip, not a failure: {:?}",
+        result.err()
+    );
+}