Skip to content

feat(pr-management-code-review): always report the branch's conflict state - #1125

Merged
potiuk merged 1 commit into
apache:mainfrom
potiuk:feat/code-review-conflict-state
Aug 29, 2026
Merged

feat(pr-management-code-review): always report the branch's conflict state#1125
potiuk merged 1 commit into
apache:mainfrom
potiuk:feat/code-review-conflict-state

Conversation

@potiuk

@potiuk potiuk commented Aug 29, 2026

Copy link
Copy Markdown
Member

Summary

  • Fetch mergeStateStatus alongside mergeable, and treat UNKNOWN as "not yet computed" rather than "clean" — re-read once, and say so if it stays unknown.
  • Add a Merge: field to the per-PR headline.
  • New rule in posting.md: a conflicting branch is always stated in the review body, whatever the disposition.

Motivation

The skill already asked for mergeable, but GitHub computes it lazily — a PR that has not been tested against its base since the last push answers UNKNOWN, and keeps answering UNKNOWN unless something asks again. Across a full review session every PR came back UNKNOWN, so the headline's "mergeable state" carried no information and a conflicting branch never reached the author.

The failure that produces is an approving review on a branch that cannot merge. The author reads the approval as "done"; the conflict surfaces weeks later at the merge button, when the reviewer has moved on.

Found while reviewing apache/airflow#68359, which was DIRTY against main throughout a review that would otherwise have approved it without mentioning the conflict.

Design note

Conflicts deliberately do not feed the disposition rules. A PR can be entirely correct and merely trail its base; gating a finished review behind a mechanical rebase wastes a round trip. Stating it is enough. Rebasing stays a triage action per Golden rule 9.

Migration path for existing adopters

No config knob and no opt-out — a doc-contract change to an existing step. Adopters get the extra headline field and the body sentence on their next /magpie-setup upgrade.

Test plan

  • prek run --files over all three changed files — all hooks pass (markdownlint, lychee, check-placeholders, skill-and-tool-validate, vendor-neutrality).

Drafted-by: Claude Code (Claude Opus 5); reviewed by @potiuk before posting

…state

The skill read `mergeable` but never `mergeStateStatus`, and GitHub
computes `mergeable` lazily — a PR not tested against its base since
the last push answers `UNKNOWN`, and keeps answering `UNKNOWN` unless
something asks again. In practice every PR in a review session came
back `UNKNOWN`, so the headline's "mergeable state" carried no
information and conflicts never reached the author.

The failure that produces is an approving review on a branch that
cannot merge. The author reads the approval as "done" and finds out
weeks later, at the merge button, when the reviewer has long moved on.

Conflicts stay out of the disposition rules deliberately: a correct PR
that merely trails its base should not be gated behind a mechanical
rebase. Stating it is enough.

Generated-by: Claude Code (Claude Opus 5)
@potiuk potiuk added family:pr-management pr-management-* skills capability:review Deep per-item code review or contributor mentoring labels Aug 29, 2026
@potiuk
potiuk merged commit 2caca58 into apache:main Aug 29, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

capability:review Deep per-item code review or contributor mentoring family:pr-management pr-management-* skills

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant