Skip to content

fix(git): drop a terminal PR once its branch moves past it - #9443

Open
dakixr wants to merge 2 commits into
pingdotgg:mainfrom
dakixr:fix/terminal-pr-on-reused-branch
Open

fix(git): drop a terminal PR once its branch moves past it#9443
dakixr wants to merge 2 commits into
pingdotgg:mainfrom
dakixr:fix/terminal-pr-on-reused-branch

Conversation

@dakixr

@dakixr dakixr commented Sep 3, 2026

Copy link
Copy Markdown

Refs #4970.

What changed

lookupStatusPr suppresses merged/closed matches only when the branch is the repository's default branch:

if (details.isDefaultBranch && latest.state !== "open") {
  return { pr: null, headContext };
}

A long-lived integration branch never qualifies, so it keeps matching its merged release PR by name forever. Release develop into main and every thread created on develop afterwards is stamped with that same historical number — and, through branchPullRequest, settles against its merged state.

This compares the branch against the commit the change request was opened from instead. A branch still sitting on that commit is described by it; a branch that has moved on was reused for later work, and the change request is history.

  • ChangeRequest gains an optional headRefOid, populated from gh's existing pr list --json (one extra field, no extra call).
  • GitManager drops a terminal change request when every ref the branch is known by points somewhere other than that commit.

Why commits and not dates

Comparing the branch tip's date against the PR's updatedAt looked simpler, but it breaks squash and rebase merges — and it would have broken status finds a merged PR after its remote branch was deleted, which backdates the PR while committing "now". The recorded head commit is the head branch's own commit, so it survives any merge strategy.

Not knowable means "keep it"

The check returns false, preserving today's behaviour, when:

Losing the comparison can never drop a badge that is otherwise correct.

Cost

One git for-each-ref per lookup, inside prLookupCache (2 min TTL) rather than on the 1 s status path, and only for terminal candidates.

Tests

Two added to GitManager.test.ts, both against real git repositories with a fake gh:

  • status drops a merged PR once its long-lived branch moves past itdevelop merged into main, committed to again, PR gone.
  • status keeps a merged PR while its branch still sits on the merged commit — the squash-merge case, PR kept.

apps/server src/git/, src/sourceControl/ and ThreadSettlementReactor pass (246 tests). Full-repo typecheck and lint clean; four exact --json assertions updated for the new field.

No UI change — the badge simply stops appearing where it was wrong.

🤖 Generated with Claude Code


Note

Medium Risk
Changes PR association logic used by status and branch settlement; conservative fallbacks limit regressions, but wrong OID comparison could hide valid merged badges or leave stale ones.

Overview
Fixes long-lived branches (e.g. develop after a release merge) incorrectly keeping a merged PR badge and thread settlement context forever, because lookup only suppressed terminal PRs on the default branch.

ChangeRequest and GitHub gh pr list/view --json now include optional headRefOid. GitManager compares the branch tip (local ref first, then remote-tracking via git for-each-ref, with guards against nested ref name false positives) to that OID; for merged/closed PRs only, a mismatch clears the match so later work on the same branch name is not tied to the old release PR. Open PRs are unchanged. If OID or tip is unknown or git fails, behavior stays as before (badge kept).

Tests cover moved-past (pushed and unpushed), squash-merge in-place, deleted branch with nested ref pattern, and updated JSON field expectations across GitHub CLI/source control layers.

Reviewed by Cursor Bugbot for commit 2afe53b. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Drop merged/closed PR from branch status once branch tip moves past recorded head commit

  • Compares the current branch tip against the PR's recorded head commit ID and stops reporting a terminal (merged or closed) PR when they differ; open PRs are unaffected
  • Threads an optional headCommitId field through the full stack: ChangeRequest contract, GitHubPullRequestSchema decoder, GitHubCli.listOpenPullRequests and GitHubCli.getPullRequest JSON field selections, GitHubSourceControlProvider.toChangeRequest, and PullRequestInfo
  • Adds readBranchTipOid in GitManager.ts to resolve a branch's current commit from the exact local ref or matching remote-tracking ref, and tightens isRemoteTrackingRefFor so nested child refs (e.g. feature vs feature/sub) are not mistaken for the branch
  • branchMovedPastChangeRequest ignores missing/blank head IDs and converts git lookup failures to false, preserving the existing PR association when comparison data is unavailable
  • Behavioral Change: GitManager PR lookup now fetches branch tips via readBranchTipOid instead of relying solely on cached data; branches whose tips no longer match the recorded PR head commit will lose that PR from status output

Macroscope summarized 2afe53b.

A long-lived branch keeps matching a merged pull request by name forever.
Release `develop` into `main` and every later thread on `develop` is
stamped with that same historical number, and settles against its merged
state.

`lookupStatusPr` already suppresses terminal matches, but only on the
default branch, so an integration branch never qualifies. Compare the
branch against the commit the change request was opened from instead: a
branch still sitting on that commit is described by it, and a branch that
has moved on was reused for later work. Commits rather than dates, so
squash and rebase merges keep their badge — the recorded head commit is
the branch's own, not the base's.

`headRefOid` is optional, and the check is skipped whenever the answer is
not knowable: a forge that does not report the head commit, a deleted
branch with no ref left to compare, or a failed git call. Every one of
those keeps today's behaviour rather than dropping a badge.

Only GitHub populates it here; the other forges keep their current
behaviour until they carry the field too.

Refs pingdotgg#4970

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 3, 2026
Comment thread apps/server/src/git/GitManager.ts

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit f764477. Configure here.

Comment thread apps/server/src/git/GitManager.ts Outdated
@macroscopeapp

macroscopeapp Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This targeted fix changes production status and thread-settlement behavior by comparing terminal PR head OIDs with local or remote branch tips, alongside additive GitHub metadata plumbing. It also adds line-level static-analysis suppressions in the test file, so human review is warranted.

You can add or adjust custom eligibility rules. Learn more.

Two problems with the first pass, both from review.

`for-each-ref` matches a pattern literally *or* up to a slash, so
`refs/heads/feature/foo` also reports `refs/heads/feature/foo/child`. Git
forbids holding both at once, so the child only surfaces once the branch
itself is gone — exactly when the merged badge is meant to be kept — and
standing in for the deleted branch dropped it instead. Every row is now
matched back against the ref it has to be, the way `branchPullRequest`
already reads its own branch ref.

Collecting every ref also meant a branch committed to but not yet pushed
kept its badge, because the remote-tracking ref still sat on the old head
and any match was enough. Read one tip instead, preferring the local ref:
that is where a thread's work lands, so a branch with unpushed commits has
still moved on from a merged change request.

Head commits are compared case-insensitively.

Refs pingdotgg#4970

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dakixr

dakixr commented Sep 3, 2026

Copy link
Copy Markdown
Author

Both review findings were real. Fixed in 2afe53b.

Cursor Bugbot — tip check matches sibling branch refs. Correct, with one refinement. refs/heads/develop does not report refs/heads/development (the prefix match stops at a slash), and git forbids holding feature/foo and feature/foo/child at once:

fatal: cannot lock ref 'refs/heads/feature/foo': 'refs/heads/feature/foo/child' exists

So a sibling can never pin a badge on a live branch. The other half is the real one: the child only surfaces once the branch itself is gone — exactly when the merged badge is meant to be kept — and standing in for the deleted branch dropped it. Every row is now matched back against the ref it has to be, following the exact-refname read branchPullRequest already does.

Macroscope — stale badge when the local branch advances without pushing. The underlying observation is right, but dropping the headRefOid forwarding would have disabled the comparison entirely rather than tightened it. The actual cause was collecting every ref and keeping the badge if any of them matched, so a stale remote-tracking ref outvoted the local branch. It now reads one tip, preferring the local ref: that is where a thread's work lands, so a branch with unpushed commits has moved on from a merged change request.

Head commits are also compared case-insensitively now.

Two tests added, both verified to fail against the previous commit:

  • status drops a merged PR once its branch is committed to without pushing
  • branch PR lookup ignores a nested child ref once the branch itself is gone

248 tests pass across src/git/, src/sourceControl/ and ThreadSettlementReactor; full-repo typecheck and lint clean.

On the flagged suppressions — the only ones added are @effect-diagnostics-next-line preferSchemaOverJson:off on test fixtures, matching every other JSON.stringify fixture in GitManager.test.ts. Happy to drop them if you'd rather these fixtures went through a schema.

@github-actions github-actions Bot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Sep 3, 2026
@dakixr

dakixr commented Sep 3, 2026

Copy link
Copy Markdown
Author

Both blocking findings are clear on 2afe53b — Cursor Bugbot pass (was 1 issue), Macroscope Correctness pass, and the approvability verdict no longer lists a blocking correctness issue.

On the remaining note about line-level static-analysis suppressions in the test file: I checked whether they can be dropped, and they can't. Removing the four this PR adds fails typecheck with hard errors:

src/git/GitManager.test.ts(1484,13): error TS377026: This code uses `JSON.parse` or `JSON.stringify`.
  Use `Schema.UnknownFromJsonString` ... effect(preferSchemaOverJson)

The fake gh returns raw JSON stdout, so a fixture has to be a string at that boundary. GitManager.test.ts already carries 67 of these directives for exactly that reason; the four added here follow that convention rather than introducing it. Converting the fixtures to schemas would mean touching all 67 and is well outside this fix — happy to do it as a separate PR if that's wanted.

That leaves the verdict resting on "human review is warranted" for a change to status and settlement behavior, which seems right — flagging it for a maintainer rather than something further I can address in code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant