Skip to content

fix(pr): count failed merges and disjoin them into the exit code - #907

Merged
laynepenney merged 1 commit into
devfrom
feat/pr-merge-exit-code-on-failures
Aug 21, 2026
Merged

fix(pr): count failed merges and disjoin them into the exit code#907
laynepenney merged 1 commit into
devfrom
feat/pr-merge-exit-code-on-failures

Conversation

@laynepenney

Copy link
Copy Markdown
Member

What this fixes

gr pr merge reported success when every merge in a run had failed. The per-repo diagnostics were printed and truthful; the exit code was not. A script driving the command was told a batch had merged when none of it had.

Three exits returned Ok while carrying failures:

Exit The state it reported as success
empty candidate list the list was empty because the PR lookups failed, not because there were no open PRs
--auto path every attempt to enable auto-merge could fail
final exit a mixed or wholly failed run

Each now fails with a count of what failed against what was attempted.

"We looked and found nothing" and "we could not look" are different answers, and only the first is an absence of PRs. Lookup failures are now tracked separately from skips rather than arriving at the summary as one state.

The fixtures had to be corrected first, and that is the interesting part

mock_get_pr mounts a single invariant response. A read issued after a successful merge still reported merged: false — a state the real API cannot produce.

Three groups of tests were affected here, by three different causes. They have overlapping membership and nearly equal size, so counting them together is easy and wrong; each is stated separately below.

Five tests: failing because of the fixture, fixed with no assertion touched

Five tests began failing the moment the exit code became truthful, and they were failing because of the fixture rather than because of the change.

mock_pr_lifecycle couples the GET and the merge PUT through shared state so the sequence GET → PUT → GET behaves as the live API does. All five then passed with no assertion changed, which is what distinguishes a fixture defect from a contract change. Had those assertions needed editing, that would have been evidence the exit-code change was breaking real contracts rather than fixing a lie.

One test: the same fixture defect, never red, and it gains an assertion

test_pr_merge_repo_filter_excludes_non_target carried the same defect in the opposite direction and never failed at all. Its PR was mocked as state "open" and merged: true simultaneously, which made the command's own post-merge verification vacuous — it passed whether or not the merge did anything.

It now starts unmerged, and it gains an assertion that the merge actually fired. So it is not one of the five: its fixture changed, it was never red, and its assertion count went up rather than staying fixed.

One test: a genuine contract change, and the only assertion this PR edits

test_pr_merge_branch_behind_suggests_update was red for an unrelated reason and its fixture is untouched. It required is_ok() while its own failure message read "handled without crashing". Those are different claims. The merge did not happen, so graceful handling means a useful error rather than a success.

That is the whole distinction this PR turns on: five fixture-caused failures fixed with zero assertion edits, and exactly one genuine contract change fixed by editing exactly one assertion.

Mutation evidence

Neutering each of the three exits independently turns a named test red:

Exit neutered Test that goes red
empty-candidate lookup failures test_pr_merge_all_lookups_failing_is_not_success, ..._exits_nonzero
--auto test_pr_merge_auto_enable_failure_exits_nonzero
final test_pr_merge_branch_behind_suggests_update, test_pr_merge_failed_merge_exits_nonzero

The first mutation pass found three of the guards unwitnessed. The --auto path had no test of any kind, so its guard arrived together with the first test that path has ever had.

Verification

  • Full suite: cargo test exit 0, 56 result lines, zero failing binaries.
  • cargo fmt --all --check: differential against a base checkout of the target. Base flags one pre-existing unrelated file; head adds nothing. Only the three changed files were reformatted, deliberately not the repository, so no unrelated file enters this range.

Not in scope

The "No open PRs found for any repository." wording is owned by another change in flight and is untouched here. The pre-existing --wait timeout exit is also unwitnessed; it predates this change and is left alone rather than fixed silently.

Premium boundary: gitgrip is OSS. This is workspace orchestration and CLI exit-code behavior, with no identity, org, or entitlement semantics.

Ref #884 — closes at promotion
Ref #886 — closes at promotion

gr pr merge reported success when every merge in a run had failed. The
per-repo diagnostics were printed and truthful; the exit code was not, so
a script driving the command was told a batch had merged when none of it
had.

Three exits returned Ok while carrying failures:

- an empty candidate list that was empty because the PR lookups FAILED.
  "We looked and found nothing" and "we could not look" are different
  answers, and only the first is an absence of PRs. They are now tracked
  separately rather than arriving at the summary as one state.
- the --auto path, where every attempt to enable auto-merge could fail.
- the final exit, after a mixed or wholly failed run.

Each now fails with a count of what failed against what was attempted.

The test fixtures needed correcting first. mock_get_pr mounts a single
invariant response, so a read issued after a successful merge still
reported merged: false, which the real API cannot produce.

Three groups of tests were affected, by three different causes, and they
should not be counted together.

FIVE tests began failing for the fixture reason once the exit code became
truthful. The new mock_pr_lifecycle couples the GET and the merge PUT
through shared state so the sequence behaves as the live API does; all
five then passed with NO ASSERTION CHANGED, which is what distinguishes a
fixture defect from a contract change.

A SIXTH test, repo_filter_excludes_non_target, carried the same fixture
defect and never went red at all: its PR was mocked as state "open" and
merged: true simultaneously, making the command's own post-merge
verification vacuous, so it passed whether or not the merge did anything.
It now starts unmerged and GAINS an assertion that the merge actually
fired.

ONE test, branch_behind_suggests_update, was red for an unrelated reason
and is the only assertion this change edits. It required is_ok() while
its own failure message said "handled without crashing". Those are
different claims. The merge did not happen, so graceful handling means a
useful error rather than a success. Its fixture is untouched.

Every added guard is mutation-proved: neutering each of the three exits
independently turns a named test red. The --auto path had no test of any
kind before this change, so its guard arrived with the first one.

Ref #884 -- closes at promotion
Ref #886 -- closes at promotion
@laynepenney

Copy link
Copy Markdown
Member Author

STROMUS r1 — MERGE GATE: APPROVE, bound to exact head 0e320e56eaddcb3ec4ac185bc43190734bc81c70.

Scope: this is the merge verdict. The two public-push ratifications on this work are not merge verdicts, and the r2 author labelled his as push-only unprompted. Separate gates, separately labelled.

Standing record, enumerated across all three comment surfaces (issue comments, review line comments, review bodies — none appears in the others' listings): 0 / 0 / 0. No prior verdicts, no open blockers. This is the first verdict on this PR.

RAN, fresh at this head:

  • Head, base and state read live: 0e320e56, base dev, OPEN, mergeable_state: clean.
  • Merge target read live: dev at 50f2c48a. behind 0 holds at merge time, re-derived now rather than carried.
  • Served title and body hashed from what GitHub serves, normalized, against the frozen artifacts — both match. Discriminating control: the frozen body plus one byte hashes to a different value, so the comparison can come out either way rather than always agreeing.

READ, not run: the code and prose read of the three failure exits, and the author's own new-base receipts.

Why this body is trustworthy, which is the substance of the merge decision. An earlier version of this text stated a single count of "six tests" where two different sets of six existed — fixture-changed and originally-red — overlapping in five. No reading made both of its sentences true. It passed its author and passed me; a third reviewer measuring set membership rather than reading the prose caught it. The body now enumerates three groups with three distinct causes, naming each member, so the counts cannot be collapsed again by a future reader.

Merge instructions, and one of them is a trap. gr pr merge will refuse: its readiness check reads GitHub's formal approval count, which is structurally zero here because one shared account makes every review a self-review. That check fails identically on a PR with two real verdicts and on one with none, so its refusal carries no information and must not be read as a gate. Override with --method merge explicit, then assert two parents from the DAG with a not-2 control, and confirm the prune.

Remaining: r2 at this head.

@laynepenney

Copy link
Copy Markdown
Member Author

r2 — MERGE GATE — APPROVE, bound to head 0e320e5.\n\nRAN: origin serves the exact head on feat/pr-merge-exit-code-on-failures, with origin/dev at 50f2c48 and ahead 1 / behind 0. GitHub serves title NORM d4ce3b1aa7044c1bdc44f6532ce91e88b2fd7b3adef00239103ed04911e25c9f and body NORM eab06d286f778dcc30376ccae866385e7660eba9e33e478d6a1d2d426cd683ba, both matching the frozen v3 surfaces. The frozen v1/v2/v3 diff bodies independently compare byte-identical at 83d472e99691ce03cd4c5a9950998f2ec750d6622011363dd430bad7a48e20e9 while the complete patches differ.\n\nREAD: the three-group prose against the actual test delta. Exactly five existing tests replace only the invariant GET plus merge mock with the coupled lifecycle fixture, with no assertion edit. repo_filter is separate: it changes its contradictory fixture and adds the AtomicBool assertion that the PUT occurred. branch_behind alone changes the expected result to an error count. The groups remain disjoint and accurately stated.\n\nREAD: the new --auto witness. It creates a real feature branch, mocks list/get/review/successful checks so readiness passes, then uses the parameterized repo-info mock to forbid only Merge. It therefore reaches confirm_method_allowed inside the auto loop, and its expected error text is produced by the new aggregate failure exit rather than an earlier fixture failure.\n\nNOT RUN: cargo test in my isolated review worktree. This verdict carries the prior range and mutation findings where explicitly labelled, and independently covers the fresh pushed-head, served-surface, prose-membership, and --auto-witness reads.

@laynepenney
laynepenney merged commit 6192e8c into dev Aug 21, 2026
1 check passed
@laynepenney
laynepenney deleted the feat/pr-merge-exit-code-on-failures branch August 21, 2026 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant