Skip to content

fix(pr): resolve the PR base from the request and the platform, not the stored target - #917

Merged
laynepenney merged 3 commits into
devfrom
fix/pr-base-resolution
Aug 27, 2026
Merged

fix(pr): resolve the PR base from the request and the platform, not the stored target#917
laynepenney merged 3 commits into
devfrom
fix/pr-base-resolution

Conversation

@laynepenney

Copy link
Copy Markdown
Member

Three commits on one seam: where gr gets the base branch of a pull request. Both defects were found during a release train.

What was wrong

gr pr create --base <branch> resolved the base correctly and then asked the platform about a different branch. The base is computed once, honoring --base; the pre-flight existence check re-derived it from the manifest's stored target. The two disagree only when --base was passed, which is exactly when the stored target is stale, because passing --base is what we do because it is stale. With a retired branch still stored, the command asked whether that retired branch existed, got a 404 for a branch nobody named, and skipped the repo reporting a base the operator never asked for. The create call one block below was already passing the resolved base, so the only thing between the operator and a correct PR was a guard checking a different branch.

gr pr merge bound its notion of the base to that same stored target. The platform's answer was already in hand and discarded: get_pull_request is called two blocks above for the mergeable flag and returns a PullRequest whose .base carries the branch the PR is genuinely open against. All four adapters populate it.

That wrong base then flowed into verify_merge_commit_parents, the post-merge assertion that a requested merge produced a two-parent commit. It read refs/remotes/origin/<base> through a bare ?, so a stored target naming a deleted branch produced None, and the caller prints nothing for None. "I looked and it was fine" and "I could not look" were the same observation.

What changed

  1. An unreadable base ref is reported instead of silently clearing the merge. The deliberate case is unchanged: an unreadable repository stays silent, so a missing local checkout cannot invent an outage, and its existing test still passes.
  2. The pre-flight check asks about the resolved base. Class swept, not just the cited instance: every remaining target_branch() in that file is either the resolution expression itself or sits inside the base_override.is_none() branch, where reading the stored target is correct.
  3. The merge's base comes from the platform, falling back to the stored target when that call failed, and on an empty answer rather than building refs/remotes/origin/.

Witnesses, and what each one pins

witness pins under mutation
an_absent_base_ref_in_a_readable_repository_is_reported the guard revert the guard: 4 passed; 1 failed
base_override_is_the_branch_that_gets_checked_and_the_pr_that_gets_opened run_pr_create end to end, asserting the mock server's received-request log revert the fix: 1 passed; 1 failed
without_an_override_the_stored_target_is_still_what_gets_checked negative control: without it, "prefer the override" and "ignore the manifest" are indistinguishable
base_binding_tests (3 cases) the resolution helper swap its arms: 2 passed; 1 failed

Each mutation kills only its own witness; the other module stays green. The first witness also carries a control requiring that some pre-flight check happened, so deleting the check could not pass it. run_pr_create had no test coverage before this.

Scope stated rather than implied. The base-binding witnesses pin the helper, not the call site. Measured, not assumed: reverting only the call-site bind while leaving the helper intact passes the entire 743-test lib suite. The call site's protection is structural, that repo.target_branch() no longer appears there as a standalone expression. An end-to-end witness would need the parent-assertion warning to be observable to a test; today it only reaches stdout.

Deliberately not folded in. run_pr_create returns Ok(()) after printing "Failed to create N PR(s)" — confirmed by the new test, which saw the command report failure and still return ok. That is the success-bound-to-control-flow class already tracked in #886 and #885; it changes exit semantics for scripts and deserves its own decision rather than riding along here.

Verification

  • cargo test --all: 1188 passed, 0 failed
  • cargo clippy --all-targets --all-features: exit 0, no errors. Warning count 153 to 155; the entire delta is one kind, function mock_branch_exists is never used, which is the pattern every other helper in that shared test module already produces
  • cargo fmt --all --check: clean

Premium boundary: grip is OSS because this is local workspace orchestration — resolving which branch a PR targets, and verifying the shape of the merge commit that results. No identity, no org state, no entitlement check.

Related: #886 and #885 for the exit-code class, which this deliberately leaves open.

…ring the merge

verify_merge_commit_parents collapsed two different states onto None: an
unreadable repository, which is deliberately silent so a missing local
checkout cannot invent an outage, and a READABLE repository whose base ref
cannot be read. The caller prints nothing for None, so the second state
reported 'I could not look' in the exact shape of 'I looked and it was fine'
-- clearance in the one direction the check cannot fail.

Only the second state changes. The unreadable-repository case keeps its
silence and its existing test still passes.

Witness: an_absent_base_ref_in_a_readable_repository_is_reported, which
carries a control asserting the same fixture DOES report a finding for a ref
that exists -- without it, a guard broken into always returning None would
pass by accident.
… target

run_pr_create resolves the base once at the top of the loop, honoring
--base. The pre-flight existence check then re-derived it from the
manifest's stored target, so the two disagreed on exactly the runs where
--base was passed -- and --base is what we pass BECAUSE the stored target
is stale.

Measured symptom: with a retired sprint branch still stored, 'gr pr create
--base dev' asked GitHub whether that retired branch existed, got 404 for a
branch nobody named, and skipped the repo reporting a base the operator
never asked for. The create call one block below was already passing the
resolved base, so the only thing between the operator and a correct PR was
a guard checking a different branch.

Class swept, not just the cited instance: every remaining target_branch()
in this file is either the resolution expression itself or sits inside the
base_override.is_none() branch, where reading the stored target is correct.

Witnesses (tests/test_pr_create_base.rs, run_pr_create end to end against
wiremock, asserting the server's received-request log):
- base_override_is_the_branch_that_gets_checked_and_the_pr_that_gets_opened
  fails on the old code with exactly the reported message, and carries a
  control requiring that SOME pre-flight check happened, so deleting the
  check could not pass it.
- without_an_override_the_stored_target_is_still_what_gets_checked is the
  negative control; without it, 'prefer the override' and 'ignore the
  manifest' would be indistinguishable.

run_pr_create had no test coverage before this.
… stored target

PRToMerge.base was set from repo.target_branch(), the workspace's stored
target. The platform's answer was already in hand and discarded: the
get_pull_request call two blocks above returns a PullRequest whose .base
carries the branch the PR is genuinely open against, and only .mergeable
was read off it. All four adapters populate base.

The two values differ exactly when the stored target is stale, which is the
ordinary state after a branch is retired. That wrong base then flowed into
verify_merge_commit_parents -- the post-merge assertion added for the squash
incident -- so it read a ref unrelated to the merge that just happened, and
read nothing at all once the stored target named a deleted branch. Paired
with the previous commit, which makes an unreadable base ref say so, the
assertion now both looks at the right ref and reports when it cannot.

The stored target stays as the fallback for a failed API call, and an empty
platform answer falls back too rather than building refs/remotes/origin/.

WITNESS SCOPE, stated rather than implied: base_binding_tests pins the
resolution helper, NOT the call site. Measured, not assumed -- reverting
only the call-site bind while leaving the helper intact fails no test in
this suite. The call site's protection is structural, that
repo.target_branch() no longer appears there as a standalone expression. An
end-to-end witness would need the parent-assertion warning to be observable
to a test; today it only reaches stdout.
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