fix(pr): exit status must agree with the failure the command just printed

src/cli/commands/pr/create.rs

@@ -35,6 +35,76 @@ fn branch_to_title(branch: &str) -> String {
     }
 }
 
+#[derive(serde::Serialize)]
+pub(crate) struct JsonPrCreateResult {
+    success: bool,
+    prs: Vec<JsonCreatedPr>,
+    failed: Vec<JsonFailedRepo>,
+}
+
+#[derive(serde::Serialize)]
+struct JsonCreatedPr {
+    repo: String,
+    branch: String,
+    number: u64,
+    url: String,
+}
+
+#[derive(serde::Serialize)]
+struct JsonFailedRepo {
+    repo: String,
+    reason: String,
+}
+
+/// The `--json` payload, built in ONE place so it can be witnessed.
+///
+/// `--json` deliberately keeps exit 0 and carries pass/fail in the body — the
+/// shipped convention, `gr verify --json` returns `Ok` before its own
+/// `exit(1)`. That trade is only safe while the payload is TRUE, so `success`
+/// is the load-bearing field of the whole design call, and it was previously
+/// computed inline inside `run_pr_create` where no test could reach it.
+///
+/// Both gate reviewers proved the consequence rather than arguing it: they
+/// mutated `success` to unconditional `true` and every test still passed. A
+/// scripted caller would then have received process success AND payload
+/// success after the platform rejected the creation — both instruments
+/// agreeing, both wrong. Extracting the construction gives that field a
+/// witness.
+///
+/// That witness is necessary and was never sufficient, which took two more
+/// gate rounds to establish. A unit calling this function pins CONSTRUCTION
+/// and says nothing about whether production emits the bytes it builds, so a
+/// call-site overwrite survived it. And "the only construction site" is a
+/// claim about EVERY production return: `run_pr_create` had a second,
+/// inline one at the empty-`branch_groups` early return that hardcoded
+/// `success: true` for the same zero/zero state this computes `false` from.
+/// Both routes now come through here, and both are witnessed against the
+/// shipped binary's stdout rather than against this function.
+pub(crate) fn pr_create_json_payload(
+    created: &[(String, String, u64, String)],
+    failed: &[(String, String)],
+) -> JsonPrCreateResult {
+    JsonPrCreateResult {
+        success: !created.is_empty() && failed.is_empty(),
+        prs: created
+            .iter()
+            .map(|(branch, repo, number, url)| JsonCreatedPr {
+                repo: repo.clone(),
+                branch: branch.clone(),
+                number: *number,
+                url: url.clone(),
+            })
+            .collect(),
+        failed: failed
+            .iter()
+            .map(|(repo, reason)| JsonFailedRepo {
+                repo: repo.clone(),
+                reason: reason.clone(),
+            })
+            .collect(),
+    }
+}
+
 /// Run the PR create command
 #[allow(clippy::too_many_arguments)]
 pub async fn run_pr_create(
@@ -125,14 +195,22 @@ pub async fn run_pr_create(
         if !json {
             println!("No repositories have changes to create PRs for.");
         } else {
-            println!(
-                "{}",
-                serde_json::to_string_pretty(&serde_json::json!({
-                    "success": true,
-                    "prs": [],
-                    "failed": []
-                }))?
-            );
+            // Routed through the same helper as the terminal branch, because
+            // production had TWO serialization sites and they disagreed about
+            // the same inputs: this one hardcoded `success: true` for
+            // zero-created/zero-failed while the terminal branch computed
+            // `success: false` from that identical state. Both were already
+            // shipping. A consumer could not rely on either answer, and which
+            // one it got depended on how far the command happened to get.
+            //
+            // Found by Sentinel and Atlas independently at the v3 gate, after
+            // two earlier rounds on this same PR fixed the same shape one
+            // layer in each time -- a test name, then a construction site, now
+            // an unrouted return. The recurring lesson is that "built in one
+            // function" is a claim about EVERY production return, and the only
+            // way to hold it is to enumerate them rather than to assert it.
+            let result = pr_create_json_payload(&[], &[]);
+            println!("{}", serde_json::to_string_pretty(&result)?);
         }
         return Ok(());
     }
@@ -388,45 +466,16 @@ pub async fn run_pr_create(
     }
 
     if json {
-        #[derive(serde::Serialize)]
-        struct JsonPrCreateResult {
-            success: bool,
-            prs: Vec<JsonCreatedPr>,
-            failed: Vec<JsonFailedRepo>,
-        }
-        #[derive(serde::Serialize)]
-        struct JsonCreatedPr {
-            repo: String,
-            branch: String,
-            number: u64,
-            url: String,
-        }
-        #[derive(serde::Serialize)]
-        struct JsonFailedRepo {
-            repo: String,
-            reason: String,
-        }
-
-        let result = JsonPrCreateResult {
-            success: !all_created_prs.is_empty() && all_failed_repos.is_empty(),
-            prs: all_created_prs
-                .iter()
-                .map(|(branch, repo, number, url)| JsonCreatedPr {
-                    repo: repo.clone(),
-                    branch: branch.clone(),
-                    number: *number,
-                    url: url.clone(),
-                })
-                .collect(),
-            failed: all_failed_repos
-                .iter()
-                .map(|(repo, reason)| JsonFailedRepo {
-                    repo: repo.clone(),
-                    reason: reason.clone(),
-                })
-                .collect(),
-        };
+        let result = pr_create_json_payload(&all_created_prs, &all_failed_repos);
         println!("{}", serde_json::to_string_pretty(&result)?);
+        // JSON mode keeps exit 0 and carries pass/fail in the body. That is
+        // this repo's shipped convention, not a guess: `gr verify --json`
+        // returns Ok before its own `exit(1)` (verify.rs:101), and
+        // docs/PLAN-verify.md states the reason -- a caller who asked for JSON
+        // is parsing the body by construction, and a non-zero exit makes a
+        // `set -e` script die before it can read the answer it asked for.
+        // `success` above already carries the truth.
+        return Ok(());
     } else {
         println!();
         if all_created_prs.is_empty() && all_failed_repos.is_empty() {
@@ -457,6 +506,36 @@ pub async fn run_pr_create(
         }
     }
 
+    // The exit status has to agree with what we just printed. Until now this
+    // returned Ok(()) unconditionally, so a run that reported "Failed to
+    // create N PR(s)" on stdout simultaneously told every caller gating on the
+    // exit status that it had succeeded -- a script, CI, or an agent deciding
+    // whether to continue reads the number, not the prose.
+    //
+    // The predicate was never missing: the --json branch a few lines above
+    // already computes `success: !created.is_empty() && failed.is_empty()`.
+    // The truth was computed and then discarded. This binds the return to the
+    // failure half of that same expression, so the two reports cannot disagree.
+    //
+    // Deliberately NOT changed here: a run that creates nothing and fails
+    // nothing still exits 0. That is the zero-match no-op question tracked in
+    // #804/#836/#839, and folding it in would change the status of runs no
+    // witness in this file covers. `--json` also keeps exit 0 -- see the
+    // return above -- so this guards the human path, where the exit status is
+    // the ONLY machine-readable signal the command emits.
+    if !all_failed_repos.is_empty() {
+        let names: Vec<&str> = all_failed_repos
+            .iter()
+            .map(|(repo, _)| repo.as_str())
+            .collect();
+        anyhow::bail!(
+            "failed to create {} of {} pull request(s): {}",
+            all_failed_repos.len(),
+            all_failed_repos.len() + all_created_prs.len(),
+            names.join(", ")
+        );
+    }
+
     Ok(())
 }
 
@@ -825,3 +904,84 @@ mod tests {
         assert_eq!(result, None);
     }
 }
+
+#[cfg(test)]
+mod json_payload_tests {
+    use super::pr_create_json_payload;
+
+    /// THE WITNESS BOTH GATE REVIEWERS REQUIRED.
+    ///
+    /// `--json` keeps exit 0 on failure, and that is only defensible because
+    /// the payload tells the truth. Before this, nothing pinned the payload:
+    /// r1 and r2 independently mutated `success` to unconditional `true` and
+    /// all three integration tests still passed. A scripted caller would have
+    /// received process success AND payload success after the platform
+    /// rejected the creation.
+    ///
+    /// Asserted on the SERIALIZED text, through the same
+    /// `serde_json::to_string_pretty` call the command makes, rather than on
+    /// the struct — the bytes on stdout are what a caller parses, and a field
+    /// renamed or skipped in serialization would pass a struct-level check
+    /// while breaking every consumer.
+    #[test]
+    fn a_failed_run_serializes_success_false_and_names_the_repo() {
+        let created: Vec<(String, String, u64, String)> = vec![];
+        let failed = vec![(
+            "frontend".to_string(),
+            "GitHub API 422: Validation Failed".to_string(),
+        )];
+
+        let out = serde_json::to_string_pretty(&pr_create_json_payload(&created, &failed)).unwrap();
+
+        assert!(
+            out.contains("\"success\": false"),
+            "the payload must say the run failed; exit 0 is only safe while it does: {out}"
+        );
+        assert!(
+            out.contains("\"repo\": \"frontend\""),
+            "the failing repo must be named in the payload: {out}"
+        );
+        assert!(
+            out.contains("422"),
+            "the reason must survive into the payload: {out}"
+        );
+    }
+
+    /// THE DISCRIMINATING CONTROL. Without it, `success: false` hardcoded
+    /// would satisfy the witness above, and a caller could never tell a
+    /// successful run from a failed one — the same defect sign-flipped.
+    #[test]
+    fn a_clean_run_serializes_success_true() {
+        let created = vec![(
+            "feat/thing".to_string(),
+            "frontend".to_string(),
+            11u64,
+            "https://example.invalid/pull/11".to_string(),
+        )];
+        let failed: Vec<(String, String)> = vec![];
+
+        let out = serde_json::to_string_pretty(&pr_create_json_payload(&created, &failed)).unwrap();
+
+        assert!(
+            out.contains("\"success\": true"),
+            "a run with no failures must report success: {out}"
+        );
+    }
+
+    /// The third state, and the one the scope note is about: nothing created
+    /// and nothing failed is NOT a success. Pinned so that the deliberate
+    /// choice to keep its EXIT status at 0 cannot be quietly read as a claim
+    /// that the run succeeded.
+    #[test]
+    fn a_no_op_run_is_not_reported_as_success() {
+        let created: Vec<(String, String, u64, String)> = vec![];
+        let failed: Vec<(String, String)> = vec![];
+
+        let out = serde_json::to_string_pretty(&pr_create_json_payload(&created, &failed)).unwrap();
+
+        assert!(
+            out.contains("\"success\": false"),
+            "a run that created nothing has not succeeded, whatever its exit status: {out}"
+        );
+    }
+}

src/cli/commands/pr/merge.rs

@@ -45,22 +45,6 @@ fn resolve_check_status(status: &StatusCheckResult) -> CheckStatus {
     }
 }
 
-/// After a `Merge`, assert the resulting commit actually has two parents.
-///
-/// Checked against the local repository, not the platform's response. The API
-/// reports that a merge happened; it does not report WHICH strategy produced
-/// the commit, and the incident behind this was a squash that reported success
-/// exactly like a merge. Fetching and counting parents asks the repository what
-/// is actually there.
-///
-/// Only meaningful for `Merge` -- squash and rebase produce single-parent
-/// commits by design, so asserting two parents for them would be wrong rather
-/// than strict.
-///
-/// A failure here is loud and it is NOT recoverable by this command: the merge
-/// already happened. The point is that the operator finds out now, from the
-/// tool, rather than days later from a broken ancestry -- which is how the
-/// original incident was discovered.
 /// The base a PR is actually open against.
 ///
 /// This is the hosting platform's answer, not the workspace's stored target.
@@ -81,6 +65,22 @@ fn pr_base_or_stored_target(platform_base: Option<&str>, stored_target: &str) ->
     }
 }
 
+/// After a `Merge`, assert the resulting commit actually has two parents.
+///
+/// Checked against the local repository, not the platform's response. The API
+/// reports that a merge happened; it does not report WHICH strategy produced
+/// the commit, and the incident behind this was a squash that reported success
+/// exactly like a merge. Fetching and counting parents asks the repository what
+/// is actually there.
+///
+/// Only meaningful for `Merge` -- squash and rebase produce single-parent
+/// commits by design, so asserting two parents for them would be wrong rather
+/// than strict.
+///
+/// A failure here is loud and it is NOT recoverable by this command: the merge
+/// already happened. The point is that the operator finds out now, from the
+/// tool, rather than days later from a broken ancestry -- which is how the
+/// original incident was discovered.
 fn verify_merge_commit_parents(
     local_path: &std::path::Path,
     base: &str,