diff --git a/src/cli/commands/pr/create.rs b/src/cli/commands/pr/create.rs index 1a4c628..8b79d85 100644 --- a/src/cli/commands/pr/create.rs +++ b/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, + failed: Vec, +} + +#[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, - failed: Vec, - } - #[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}" + ); + } +} diff --git a/src/cli/commands/pr/merge.rs b/src/cli/commands/pr/merge.rs index 064116e..f7be09c 100644 --- a/src/cli/commands/pr/merge.rs +++ b/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, diff --git a/tests/test_pr_create_exit_status.rs b/tests/test_pr_create_exit_status.rs new file mode 100644 index 0000000..3669cf6 --- /dev/null +++ b/tests/test_pr_create_exit_status.rs @@ -0,0 +1,419 @@ +//! `gr pr create` must not report success after printing its own failure. +//! +//! `run_pr_create` collected per-repo failures into `all_failed_repos`, printed +//! `Failed to create N PR(s):` with every one of them, and then returned +//! `Ok(())`. Anything gating on the exit status -- a shell script, CI, an agent +//! deciding whether to continue -- read that as success while the command was +//! saying the opposite on stdout. +//! +//! The predicate was never missing. Four lines above the return, the `--json` +//! branch already computes `success: !created.is_empty() && failed.is_empty()` +//! and serializes it. The truth was computed and then discarded, exactly as the +//! merge base was in #917. +//! +//! Scope, deliberately narrow (grip#886 is an umbrella over nine verbs): this +//! covers ONLY "failures were reported and the exit status disagreed." The +//! zero-created/zero-failed no-op still exits 0 and is left alone -- that is +//! #804/#836/#839's question, and answering it here would change the exit +//! status of a successful `--dry-run`-shaped run without a witness for it. + +mod common; + +use common::fixtures::WorkspaceBuilder; +use common::git_helpers; +use common::mock_platform::{ + mock_branch_exists, mock_create_pr, mock_create_pr_validation_error, point_repo_at_mock, + setup_github_mock, +}; +use wiremock::http::Method; + +fn repo_ahead_of(ws: &common::fixtures::WorkspaceFixture, repo: &str, base: &str, feature: &str) { + let path = ws.repo_path(repo); + git_helpers::create_branch(&path, base); + git_helpers::commit_file(&path, "base.txt", "base", "Add base"); + git_helpers::push_branch(&path, "origin", base); + git_helpers::create_branch(&path, feature); + git_helpers::commit_file(&path, "feature.txt", "feature", "Add feature"); +} + +/// THE WITNESS. The platform refuses the PR; the command reports the failure +/// and must not simultaneously claim success through its exit status. +#[tokio::test] +async fn a_reported_failure_must_not_exit_zero() { + let (server, _adapter) = setup_github_mock().await; + + let ws = WorkspaceBuilder::new().add_repo("frontend").build(); + let mut manifest = ws.load_manifest(); + manifest.settings.target = Some("dev".to_string()); + + repo_ahead_of(&ws, "frontend", "dev", "feat/thing"); + point_repo_at_mock(&mut manifest, "frontend", &server); + + mock_branch_exists(&server, "owner", "repo", "dev").await; + mock_create_pr_validation_error(&server).await; + + let filter = vec!["frontend".to_string()]; + let result = gitgrip::cli::commands::pr::run_pr_create( + &ws.workspace_root, + &manifest, + Some("Add feature"), + None, + false, + false, + false, + Some(&filter), + None, + false, + ) + .await; + + // Control: the command must actually have ATTEMPTED the creation. Without + // this, a command that skipped the repo entirely -- never reaching the + // failure path at all -- would satisfy the assertion below for the wrong + // reason, and the test would be pinning a no-op. + let requests = server.received_requests().await.unwrap(); + let posts = requests + .iter() + .filter(|r| r.method == Method::POST && r.url.path().ends_with("/pulls")) + .count(); + assert!( + posts >= 1, + "control: the command must attempt the PR before it can report a failure; saw {posts} POSTs" + ); + + assert!( + result.is_err(), + "a run that printed 'Failed to create 1 PR(s)' returned Ok -- any caller \ + gating on the exit status reads the command's own reported failure as success" + ); +} + +/// THE DISCRIMINATING CONTROL. A run where every PR is created must still +/// return Ok. Without this case, "return Err whenever anything was attempted" +/// and "return Err only when something failed" are indistinguishable, and the +/// witness above would be satisfied by a command that always fails. +#[tokio::test] +async fn a_run_with_no_failures_still_exits_zero() { + let (server, _adapter) = setup_github_mock().await; + + let ws = WorkspaceBuilder::new().add_repo("frontend").build(); + let mut manifest = ws.load_manifest(); + manifest.settings.target = Some("dev".to_string()); + + repo_ahead_of(&ws, "frontend", "dev", "feat/thing"); + point_repo_at_mock(&mut manifest, "frontend", &server); + + mock_branch_exists(&server, "owner", "repo", "dev").await; + mock_create_pr(&server, 11, "https://github.com/owner/repo/pull/11").await; + + let filter = vec!["frontend".to_string()]; + let result = gitgrip::cli::commands::pr::run_pr_create( + &ws.workspace_root, + &manifest, + Some("Add feature"), + None, + false, + false, + false, + Some(&filter), + None, + false, + ) + .await; + + let requests = server.received_requests().await.unwrap(); + let posts = requests + .iter() + .filter(|r| r.method == Method::POST && r.url.path().ends_with("/pulls")) + .count(); + assert_eq!(posts, 1, "control: exactly one PR should have been created"); + + assert!( + result.is_ok(), + "a run in which every PR was created must not report failure: {result:?}" + ); +} + +/// `--json` keeps exit 0 and carries the failure in the body. +/// +/// Not an exception carved out for convenience -- it is this repo's shipped +/// convention, and it is shipped rather than merely planned: `gr verify --json` +/// returns Ok before its own `exit(1)` (verify.rs:101), and docs/PLAN-verify.md +/// gives 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. +/// +/// Without this case, the fix above would silently change the exit status of +/// every scripted `--json` caller, which is the sort of thing that gets found +/// in someone else's CI rather than here. +#[tokio::test] +async fn json_mode_reports_the_failure_in_the_body_and_still_exits_zero() { + let (server, _adapter) = setup_github_mock().await; + + let ws = WorkspaceBuilder::new().add_repo("frontend").build(); + let mut manifest = ws.load_manifest(); + manifest.settings.target = Some("dev".to_string()); + + repo_ahead_of(&ws, "frontend", "dev", "feat/thing"); + point_repo_at_mock(&mut manifest, "frontend", &server); + + mock_branch_exists(&server, "owner", "repo", "dev").await; + mock_create_pr_validation_error(&server).await; + + let filter = vec!["frontend".to_string()]; + let result = gitgrip::cli::commands::pr::run_pr_create( + &ws.workspace_root, + &manifest, + Some("Add feature"), + None, + false, + false, + false, + Some(&filter), + None, + true, // json + ) + .await; + + // Control: the same inputs in human mode DO error. Without this the test + // would pass equally well against a build where nothing ever errors, which + // is the state this whole PR exists to leave behind. + let requests = server.received_requests().await.unwrap(); + let posts = requests + .iter() + .filter(|r| r.method == Method::POST && r.url.path().ends_with("/pulls")) + .count(); + assert!(posts >= 1, "control: the creation must have been attempted"); + + assert!( + result.is_ok(), + "--json must keep exit 0 and carry pass/fail in the body: {result:?}" + ); +} + +/// THE WIRING WITNESS: the bytes the shipped binary actually prints. +/// +/// Sentinel blocked v2 on exactly this gap, and proved it rather than argued +/// it: he left `pr_create_json_payload` correct and set `result.success = true` +/// inside `run_pr_create` immediately before the production +/// `to_string_pretty` call. All six existing witnesses stayed green. A +/// scripted caller would then have received process success AND payload +/// success after the platform rejected the creation -- both instruments +/// agreeing, both wrong, which is the whole defect this PR exists to close. +/// +/// The reason the helper tests could not see it is that they call +/// `pr_create_json_payload` and `to_string_pretty` themselves. That pins +/// CONSTRUCTION; it says nothing about whether the production caller emits +/// those bytes unchanged. This is my own filed lesson -- a witness calling a +/// helper directly pins the helper, not its use -- committed again one layer +/// out from where I fixed it in v1. +/// +/// So this test spawns the REAL `gr` binary, in a real workspace, against the +/// mock platform, and parses the JSON off its stdout. Nothing between the +/// construction site and the process's output can hide from it: the assertion +/// is on the bytes a caller parses, because that is the only surface the +/// exit-0 convention is defensible on. +#[tokio::test] +async fn the_shipped_binary_prints_success_false_after_a_platform_failure() { + use assert_cmd::Command as AssertCommand; + + let (server, _adapter) = setup_github_mock().await; + + let ws = WorkspaceBuilder::new().add_repo("frontend").build(); + repo_ahead_of(&ws, "frontend", "dev", "feat/thing"); + + mock_branch_exists(&server, "owner", "repo", "dev").await; + mock_create_pr_validation_error(&server).await; + + // The subprocess reads the manifest from disk, so the mock repoint that + // `point_repo_at_mock` does in memory has to be persisted here instead. + let manifest_path = ws + .workspace_root + .join(".gitgrip") + .join("spaces") + .join("main") + .join("gripspace.yml"); + let mut manifest: serde_yaml::Value = + serde_yaml::from_str(&std::fs::read_to_string(&manifest_path).unwrap()).unwrap(); + let repo = manifest + .get_mut("repos") + .and_then(|r| r.get_mut("frontend")) + .expect("fixture must define the frontend repo"); + repo["url"] = serde_yaml::Value::String("https://github.com/owner/repo.git".into()); + repo["platform"] = + serde_yaml::from_str(&format!("type: github\nbase_url: {}\n", server.uri())).unwrap(); + manifest["settings"]["target"] = serde_yaml::Value::String("dev".into()); + std::fs::write(&manifest_path, serde_yaml::to_string(&manifest).unwrap()).unwrap(); + + let out = AssertCommand::cargo_bin("gr") + .unwrap() + .current_dir(&ws.workspace_root) + .env("GITHUB_TOKEN", "mock-test-token") + .args([ + "pr", + "create", + "-t", + "Add feature", + "--repo", + "frontend", + "--json", + ]) + .output() + .unwrap(); + + let stdout = String::from_utf8_lossy(&out.stdout).to_string(); + + // HARNESS GUARD BEFORE ANY CONTENT CLAIM, and it is also the exit-0 half of + // the `--json` convention -- one assertion serving both, asserted on this + // run rather than inferred from a different one. It has to come FIRST: + // `cargo test --test ` does not reliably rebuild the bin `cargo_bin` + // invokes, and a stale or half-written binary yields empty stdout whose + // failure reads exactly like a missing feature. Absent and could-not-look + // must not share a failure. + // + // It sat LAST until the v4 gate, where Atlas caught that this file's body + // claimed both subprocess witnesses guarded exit 0 first and only witness + // (5) did. Same shape as the three rounds above: a universal asserted over + // two things after checking one. + assert_eq!( + out.status.code(), + Some(0), + "harness/contract: `--json` keeps exit 0 and carries pass/fail in the \ + body ({:?}); stdout={stdout} stderr={}", + out.status.code(), + String::from_utf8_lossy(&out.stderr) + ); + + let parsed: serde_json::Value = serde_json::from_str(stdout.trim()).unwrap_or_else(|e| { + panic!( + "harness: --json must print parseable JSON on stdout ({e}); got: \ + {stdout} stderr={}", + String::from_utf8_lossy(&out.stderr) + ) + }); + + // Control: the run must have REACHED the platform. Without this, a binary + // that failed to load the workspace at all would print no JSON and the + // absence assertions below would pass for the wrong reason -- the same + // never-ran-but-looks-right shape the helper tests fell into. + let posts = server + .received_requests() + .await + .unwrap() + .iter() + .filter(|r| r.method == Method::POST && r.url.path().ends_with("/pulls")) + .count(); + assert!( + posts >= 1, + "control: the binary must have attempted the creation. stdout={stdout} stderr={}", + String::from_utf8_lossy(&out.stderr) + ); + + assert_eq!( + parsed["success"], + serde_json::Value::Bool(false), + "the shipped binary reported success after the platform refused: {stdout}" + ); + assert!( + stdout.contains("frontend"), + "the payload must name the repo that failed: {stdout}" + ); +} + +/// THE SECOND WIRE: the empty-`branch_groups` early return. +/// +/// Sentinel and Atlas found this independently at the v3 gate. `run_pr_create` +/// had TWO JSON serialization sites, and they disagreed about identical +/// inputs: the terminal branch computed `success: false` from +/// zero-created/zero-failed, while this early return hardcoded +/// `success: true`. Both were already shipping, so which answer a consumer got +/// depended on how far the command happened to get. +/// +/// The helper unit for the no-op case could not see it, because it calls +/// `pr_create_json_payload` directly — the same helper-not-use shape this file +/// names twice already. Only the shipped binary can tell these two routes +/// apart, which is why this witness is a subprocess one. +/// +/// Note what this pins and what it does not: it pins that BOTH routes now come +/// through one construction site, which is what makes the "built in one +/// function" claim checkable rather than asserted. It is deliberately not a +/// ruling on whether a no-op *ought* to read `success: false` — that is the +/// separate zero-match question, and the point here is that production must +/// not answer it two different ways at once. +#[tokio::test] +async fn the_early_no_op_return_serializes_through_the_same_helper() { + use assert_cmd::Command as AssertCommand; + + let ws = WorkspaceBuilder::new().add_repo("frontend").build(); + // No branch, no commits: nothing is ahead, so `branch_groups` is empty and + // production takes the early return rather than the terminal branch. + + let out = AssertCommand::cargo_bin("gr") + .unwrap() + .current_dir(&ws.workspace_root) + .env("GITHUB_TOKEN", "mock-test-token") + .args(["pr", "create", "-t", "Nothing to do", "--json"]) + .output() + .unwrap(); + + let stdout = String::from_utf8_lossy(&out.stdout).to_string(); + + // HARNESS GUARD BEFORE ANY CONTENT CLAIM. `cargo test --test ` does + // not reliably rebuild the bin `cargo_bin` invokes, and a stale or + // half-written binary yields empty stdout whose failure reads exactly like + // a missing feature. Absent and could-not-look must not share a failure. + assert!( + out.status.success(), + "harness: `gr pr create --json` did not exit 0 ({:?}); stderr={}", + out.status.code(), + String::from_utf8_lossy(&out.stderr) + ); + let parsed: serde_json::Value = serde_json::from_str(stdout.trim()).unwrap_or_else(|e| { + panic!( + "harness: --json must print parseable JSON on stdout ({e}); got: {stdout} stderr={}", + String::from_utf8_lossy(&out.stderr) + ) + }); + + // ROUTE CONTROL, and it has to be a discriminating one. Empty `prs` and + // `failed` do NOT identify the early return -- the terminal branch reports + // the same arrays for a run that built groups and created nothing. So the + // control is the one string only the early return can print: the same + // workspace in human mode must say it. Without this the witness could pass + // while silently covering the wrong wire, which is the failure this whole + // PR keeps rediscovering. + let human = AssertCommand::cargo_bin("gr") + .unwrap() + .current_dir(&ws.workspace_root) + .env("GITHUB_TOKEN", "mock-test-token") + .args(["pr", "create", "-t", "Nothing to do"]) + .output() + .unwrap(); + let human_stdout = String::from_utf8_lossy(&human.stdout).to_string(); + assert!( + human_stdout.contains("No repositories have changes to create PRs for"), + "control: this witness must exercise the empty-branch_groups early \ + return, and only that route prints this line. Got: {human_stdout} \ + stderr={}", + String::from_utf8_lossy(&human.stderr) + ); + + assert_eq!( + parsed["prs"].as_array().map(|a| a.len()), + Some(0), + "the zero-group route reports nothing created: {stdout}" + ); + assert_eq!( + parsed["failed"].as_array().map(|a| a.len()), + Some(0), + "the zero-group route reports nothing failed: {stdout}" + ); + + assert_eq!( + parsed["success"], + serde_json::Value::Bool(false), + "the early return must serialize through pr_create_json_payload, which \ + computes success from the inputs, rather than hardcoding a different \ + answer than the terminal branch gives for the same state: {stdout}" + ); +}