From bee1411b8e638b9f074c417f5b09840acc0fe8bd Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Thu, 27 Aug 2026 05:35:01 -0500 Subject: [PATCH 1/9] fix(pr): exit status must agree with the failure the command just printed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit run_pr_create collected per-repo failures, printed "Failed to create N PR(s):" with every one of them, and 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, which is the same shape as the merge base in #917: the right value in hand, one line from where it was needed, dropped. Scope is deliberately the ruled half of grip#886 and no wider. A run that creates nothing and fails nothing still exits 0; that is the zero-match no-op question in #804/#836/#839, and folding it in would change the exit status of runs no witness here covers. Witness: tests/test_pr_create_exit_status.rs. The failure case asserts is_err and carries a control that the creation was actually ATTEMPTED, so a command that skipped the repo entirely cannot satisfy it for the wrong reason. The second test is the discriminating control -- a run where every PR is created must still return Ok -- without which "always Err" and "Err only on failure" are indistinguishable. Also carries F1 from the #917 gate, as r1 ruled. Patch 3 there left the whole squash-incident doc block attached to pr_base_or_stored_target, so verify_merge_commit_parents -- the function that block is about -- was undocumented. Moved back; each block now immediately precedes its own fn, asserted by ordering rather than by eye. Premium boundary: grip is OSS. This is CLI exit-status semantics, no identity, no org context. Ref #886 — closes at promotion --- src/cli/commands/pr/create.rs | 28 ++++++ src/cli/commands/pr/merge.rs | 32 +++---- tests/test_pr_create_exit_status.rs | 135 ++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 16 deletions(-) create mode 100644 tests/test_pr_create_exit_status.rs diff --git a/src/cli/commands/pr/create.rs b/src/cli/commands/pr/create.rs index 1a4c628..c2ef9f7 100644 --- a/src/cli/commands/pr/create.rs +++ b/src/cli/commands/pr/create.rs @@ -457,6 +457,34 @@ 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. + 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(()) } 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..fa72e45 --- /dev/null +++ b/tests/test_pr_create_exit_status.rs @@ -0,0 +1,135 @@ +//! `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:?}" + ); +} From 7093303c0494410b7d2609e0633b831b99fd43bc Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Thu, 27 Aug 2026 05:42:03 -0500 Subject: [PATCH 2/9] fix(pr): --json keeps exit 0 and carries pass/fail in the body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut bailed after both output branches, which changed the exit status of --json callers too. That is wrong here, and it is wrong against a SHIPPED convention rather than a stylistic preference: gr verify --json returns Ok before its own exit(1) (verify.rs:99), 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. The success field already carried the truth. So the bail now guards the human path, where the exit status is the only machine-readable signal the command emits. Third witness pins the --json case with a control that the creation was attempted, so it cannot pass against a build where nothing ever errors. Ref #886 — closes at promotion --- src/cli/commands/pr/create.rs | 12 +++++- tests/test_pr_create_exit_status.rs | 57 +++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/cli/commands/pr/create.rs b/src/cli/commands/pr/create.rs index c2ef9f7..31c725a 100644 --- a/src/cli/commands/pr/create.rs +++ b/src/cli/commands/pr/create.rs @@ -427,6 +427,14 @@ pub async fn run_pr_create( .collect(), }; 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:99), 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() { @@ -471,7 +479,9 @@ pub async fn run_pr_create( // 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. + // 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() diff --git a/tests/test_pr_create_exit_status.rs b/tests/test_pr_create_exit_status.rs index fa72e45..a405aa7 100644 --- a/tests/test_pr_create_exit_status.rs +++ b/tests/test_pr_create_exit_status.rs @@ -133,3 +133,60 @@ async fn a_run_with_no_failures_still_exits_zero() { "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:99), 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:?}" + ); +} From 08839ddb6435c1bba2b49cb615b6088f0b7d7431 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Thu, 27 Aug 2026 06:01:58 -0500 Subject: [PATCH 3/9] test(pr): witness the --json payload's success field (gate BLOCK from r1 and r2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviewers independently found the same real defect and proved it rather than arguing it: they mutated the --json payload's "success" field to unconditional true and all three of my tests still passed. That survivor breaks the exact guarantee that makes exit 0 safe in JSON mode. A scripted caller would have received process success AND payload success after the platform rejected the creation -- both instruments agreeing, both wrong. And my test was NAMED json_mode_reports_the_failure_in_the_body_and_still_exits _zero while never looking at the body, so the name, the commit message and the PR body all claimed a witness that did not exist. That is the overclaim half, and it is the worse half. The payload construction is now a single function, pr_create_json_payload, which run_pr_create has no way to route around, with three witnesses asserting the SERIALIZED text through the same to_string_pretty call the command makes -- 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. Three states pinned: failed -> success false and the repo and reason present; clean -> success true (the discriminating control, without which a hardcoded false would satisfy the first); no-op -> success false, so the deliberate choice to keep its EXIT status at 0 cannot be read as a claim that it succeeded. Verified under the reviewers' own mutation: with success forced to true, two of the three new witnesses go red and the clean-run control correctly survives, while the three integration tests still pass 3/3 -- reproducing exactly the gap r1 and r2 reported. Ref #886 — closes at promotion --- src/cli/commands/pr/create.rs | 181 +++++++++++++++++++++++++++------- 1 file changed, 143 insertions(+), 38 deletions(-) diff --git a/src/cli/commands/pr/create.rs b/src/cli/commands/pr/create.rs index 31c725a..12f526e 100644 --- a/src/cli/commands/pr/create.rs +++ b/src/cli/commands/pr/create.rs @@ -37,6 +37,67 @@ fn branch_to_title(branch: &str) -> String { /// Run the PR create command #[allow(clippy::too_many_arguments)] +#[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, and `run_pr_create` has no other way to build the payload, so the +/// witness cannot be routed around. +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(), + } +} + pub async fn run_pr_create( workspace_root: &Path, manifest: &Manifest, @@ -388,44 +449,7 @@ 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` @@ -863,3 +887,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}" + ); + } +} From 4bb112ae53d12071c8e35123cf52166280041f0f Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Thu, 27 Aug 2026 06:11:05 -0500 Subject: [PATCH 4/9] fix(pr): reattach the allow attribute the payload hoist detached MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hoisting the JSON payload construction above run_pr_create inserted sixty lines BETWEEN "#[allow(clippy::too_many_arguments)]" and the function it governs, so the attribute and the "Run the PR create command" doc comment both landed on the new struct instead. This is F1 committed again, one commit later. F1 was r1's carried finding on #917 -- a doc block left attached to the wrong function -- and fixing it is one of the stated reasons this PR exists. The same edit shape reproduced it, with an attribute this time rather than a doc comment, which is the version that has a behavioral consequence. Caught by MEASURING the clippy delta rather than by reading exit 0: total warnings went 2194 on dev to 2244 here, and only 48 of the extra 50 were attributable to the new test target. Chasing the remaining 2 found them -- "this function has too many arguments (10/7)", twice, because --all-targets builds the lib and its test harness and both report the same target name. The attribute had stopped suppressing anything. Exit 0 was true throughout and said nothing; a delta of "+50, and I could not tell you where 2 of them came from" is what actually carried the signal. Now 2242, delta +48, every one of them dead_code from the new integration test target -- the mod-common pattern, matching the sibling target added by #917. The hoisted block now sits ABOVE the doc comment, so doc and attribute are adjacent to the function again. Asserted by line ordering with a negative control proving the assertion can report WRONG -- my first version of that check said WRONG against correct code because I asserted a blank line that was not there, so the check needed fixing before it could clear anything. Ref #886 — closes at promotion --- src/cli/commands/pr/create.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cli/commands/pr/create.rs b/src/cli/commands/pr/create.rs index 12f526e..4e2f0f3 100644 --- a/src/cli/commands/pr/create.rs +++ b/src/cli/commands/pr/create.rs @@ -35,8 +35,6 @@ fn branch_to_title(branch: &str) -> String { } } -/// Run the PR create command -#[allow(clippy::too_many_arguments)] #[derive(serde::Serialize)] pub(crate) struct JsonPrCreateResult { success: bool, @@ -98,6 +96,8 @@ pub(crate) fn pr_create_json_payload( } } +/// Run the PR create command +#[allow(clippy::too_many_arguments)] pub async fn run_pr_create( workspace_root: &Path, manifest: &Manifest, From 614a2c12cad9709b3f5dace1c4a4f95045a319d4 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Thu, 27 Aug 2026 06:44:27 -0500 Subject: [PATCH 5/9] test: witness the bytes the shipped binary prints, not the helper's Sentinel's r1 BLOCK on v2, proved rather than argued: leave pr_create_json_payload correct, set result.success = true inside run_pr_create immediately before the production to_string_pretty call, and all six existing witnesses stay green. A scripted caller then receives process success AND payload success after the platform refused -- both instruments agreeing, both wrong. The helper tests call pr_create_json_payload and to_string_pretty themselves, so they pin CONSTRUCTION and say nothing about whether the production caller emits those bytes unchanged. That 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. This spawns the real gr binary in a real workspace against the mock platform and parses the JSON off its stdout, with a received-requests control so a binary that never reached the platform cannot satisfy it by printing nothing. Ref #886 --- tests/test_pr_create_exit_status.rs | 107 ++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/tests/test_pr_create_exit_status.rs b/tests/test_pr_create_exit_status.rs index a405aa7..b93fc45 100644 --- a/tests/test_pr_create_exit_status.rs +++ b/tests/test_pr_create_exit_status.rs @@ -190,3 +190,110 @@ async fn json_mode_reports_the_failure_in_the_body_and_still_exits_zero() { "--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(); + + // 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) + ); + + let parsed: serde_json::Value = serde_json::from_str(stdout.trim()).unwrap_or_else(|e| { + panic!("--json must print parseable JSON on stdout ({e}); got: {stdout}") + }); + + 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 exit-0 half of the convention, asserted on the same run rather than + // inferred from a different one. + assert_eq!( + out.status.code(), + Some(0), + "--json keeps exit 0 and carries pass/fail in the body: {stdout}" + ); +} From 7b1f6daef937068ecde9e0feed538958b0e050c7 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Thu, 27 Aug 2026 07:04:10 -0500 Subject: [PATCH 6/9] style: cargo fmt the wiring witness Ref #886 --- tests/test_pr_create_exit_status.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/tests/test_pr_create_exit_status.rs b/tests/test_pr_create_exit_status.rs index b93fc45..68b7b50 100644 --- a/tests/test_pr_create_exit_status.rs +++ b/tests/test_pr_create_exit_status.rs @@ -240,11 +240,8 @@ async fn the_shipped_binary_prints_success_false_after_a_platform_failure() { .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(); + 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(); @@ -252,7 +249,15 @@ async fn the_shipped_binary_prints_success_false_after_a_platform_failure() { .unwrap() .current_dir(&ws.workspace_root) .env("GITHUB_TOKEN", "mock-test-token") - .args(["pr", "create", "-t", "Add feature", "--repo", "frontend", "--json"]) + .args([ + "pr", + "create", + "-t", + "Add feature", + "--repo", + "frontend", + "--json", + ]) .output() .unwrap(); From f452d80b6fddee8869031f28a0f6dda090721bbf Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Thu, 27 Aug 2026 07:27:27 -0500 Subject: [PATCH 7/9] fix(pr): route the empty-branch_groups early return through the same payload helper Ref #886 --- src/cli/commands/pr/create.rs | 37 ++++++++--- tests/test_pr_create_exit_status.rs | 98 +++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 10 deletions(-) diff --git a/src/cli/commands/pr/create.rs b/src/cli/commands/pr/create.rs index 4e2f0f3..82d948f 100644 --- a/src/cli/commands/pr/create.rs +++ b/src/cli/commands/pr/create.rs @@ -69,8 +69,17 @@ struct JsonFailedRepo { /// 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, and `run_pr_create` has no other way to build the payload, so the -/// witness cannot be routed around. +/// 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)], @@ -186,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(()); } diff --git a/tests/test_pr_create_exit_status.rs b/tests/test_pr_create_exit_status.rs index 68b7b50..20bb63f 100644 --- a/tests/test_pr_create_exit_status.rs +++ b/tests/test_pr_create_exit_status.rs @@ -302,3 +302,101 @@ async fn the_shipped_binary_prints_success_false_after_a_platform_failure() { "--json keeps exit 0 and carries pass/fail in the body: {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}" + ); +} From 1b5043613fb113d23c1644e9ad50dd8accd299d3 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Thu, 27 Aug 2026 09:50:31 -0500 Subject: [PATCH 8/9] test(pr): guard witness (4) on exit status before parsing or content Atlas r2 BLOCKed v4 on a body sentence that was true of one witness and asserted over two: witness (5) guarded exit 0 and parseability before any content claim; witness (4) parsed stdout, asserted success:false and the repo name, and only then read out.status. Both now run the identical sequence -- exit status, parseable output, route control, content -- so "both" is checkable by reading them side by side. The trailing exit assertion is not duplicated: the one guard serves as both the harness check and the exit-0 half of the --json convention, asserted on the same run rather than inferred from another. Also corrects a citation repeated in the source comment and the PR body: verify.rs:99 is the struct literal's closing brace; the return Ok(()) is at verify.rs:101. Ref #886 -- closes at promotion --- src/cli/commands/pr/create.rs | 2 +- tests/test_pr_create_exit_status.rs | 41 ++++++++++++++++++++--------- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/src/cli/commands/pr/create.rs b/src/cli/commands/pr/create.rs index 82d948f..8b79d85 100644 --- a/src/cli/commands/pr/create.rs +++ b/src/cli/commands/pr/create.rs @@ -470,7 +470,7 @@ pub async fn run_pr_create( 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:99), and + // 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. diff --git a/tests/test_pr_create_exit_status.rs b/tests/test_pr_create_exit_status.rs index 20bb63f..524e644 100644 --- a/tests/test_pr_create_exit_status.rs +++ b/tests/test_pr_create_exit_status.rs @@ -263,6 +263,35 @@ async fn the_shipped_binary_prints_success_false_after_a_platform_failure() { 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 @@ -280,10 +309,6 @@ async fn the_shipped_binary_prints_success_false_after_a_platform_failure() { String::from_utf8_lossy(&out.stderr) ); - let parsed: serde_json::Value = serde_json::from_str(stdout.trim()).unwrap_or_else(|e| { - panic!("--json must print parseable JSON on stdout ({e}); got: {stdout}") - }); - assert_eq!( parsed["success"], serde_json::Value::Bool(false), @@ -293,14 +318,6 @@ async fn the_shipped_binary_prints_success_false_after_a_platform_failure() { stdout.contains("frontend"), "the payload must name the repo that failed: {stdout}" ); - - // The exit-0 half of the convention, asserted on the same run rather than - // inferred from a different one. - assert_eq!( - out.status.code(), - Some(0), - "--json keeps exit 0 and carries pass/fail in the body: {stdout}" - ); } /// THE SECOND WIRE: the empty-`branch_groups` early return. From 4ba0455576912a632f2babb35cca1d838cfa01cf Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Thu, 27 Aug 2026 10:04:01 -0500 Subject: [PATCH 9/9] docs(pr): finish the citation correction the last commit claimed 1b50436's message says it "corrects a citation repeated in the source comment and the PR body". It corrected one of three sites. A class sweep for `file.rs:NNN` across the touched surfaces found: create.rs:473 fixed, tests/test_pr_create_exit_status.rs:141 still verify.rs:99, and the PR body draft still verify.rs:99. The correct anchor is verify.rs:101 (the `return Ok(())`); :99 is inside the struct literal above it. This is the same defect the commit it follows was fixing -- a universal asserted after checking one instance -- committed a fourth time on this PR, this time in a commit message rather than a body or a witness. Filed as such rather than amended, so the miss stays legible. Body draft corrected out of tree; the v5 round entry is still owed at freeze time. Ref #886 -- closes at promotion --- tests/test_pr_create_exit_status.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_pr_create_exit_status.rs b/tests/test_pr_create_exit_status.rs index 524e644..3669cf6 100644 --- a/tests/test_pr_create_exit_status.rs +++ b/tests/test_pr_create_exit_status.rs @@ -138,7 +138,7 @@ async fn a_run_with_no_failures_still_exits_zero() { /// /// 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:99), and docs/PLAN-verify.md +/// 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.