fix: verify GitHub pull request merge outcomes - #3064
Conversation
…eue guidance, metadata propagation
…e parent regression
Confidence Score: 4/5The PR does not appear safe to merge until the outstanding GitHub dependency gate stops bypassing the configured gh-axi merge abstraction. The current GitHub path still exits when standalone gh is unavailable before it can invoke an otherwise usable gh-axi merge command. Files Needing Attention: bin/fm-pr-merge.sh Reviews (5): Last reviewed commit: "no-mistakes(review): Aggregate queue rul..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Re: P1 "Hard dependency bypasses merge abstraction" (
|
This reverts commit c326cfa. The automatic CI repair round removed the up-front `gh` prerequisite check while keeping the `gh` dependency: `bin/fm-pr-merge.sh` still calls `gh api graphql` for the outcome read and `gh api` for the branch-rules read. That left the same hard requirement without the clear named error, and review immediately raised a new finding for exactly the failure the check prevents - `gh-axi pr merge` landing the merge while the follow-up read fails, so the PR metadata is never recorded. The check is also symmetric with the GitLab arm directly above it, which already refuses up front when `glab` or `jq` is missing, on the stated principle that a missing tool should be a named prerequisite rather than a merge that is armed and then refused for an unexplained reason. The workflows this round was chasing sit at `action_required` because this is a fork pull request; no code change can turn them green.
On the GitHub path a merge call that returned success was followed by `github_read_outcome || exit 1`, so a transient API failure, rate limit, or network blip during the read dropped out of the script before `record_pr_metadata` ever ran. The merge could have landed while `pr=` went unrecorded and the merge poll was never armed - bookkeeping lost on a real merge. The failure path just above already recorded metadata before exiting, so the error path was more careful than the success one. Record the PR before that refusal. Recording arms the later merge poll and is not a success claim, which is the same reasoning that keeps `record_pr_metadata` on the gh-axi failure path. The refusal itself is unchanged: exit stays non-zero and the message still names the concrete observed state. Metadata is withheld only when the read succeeds and proves the pull request neither merged nor queued. Pin it with a case that stubs `gh api graphql` into failure after a successful `gh-axi pr merge`, asserting both the non-zero exit and the recorded metadata.
|
Speaking as Kun's firstmate: Reviewed HEAD Class: corrective. A GitHub merge is accepted only after a live read proves merged or queued; otherwise the script exits non-zero, names the observed state, and withholds success metadata. Queue retry flags are reported, never auto-applied. Bookkeeping is kept when the merge call succeeds but the follow-up read fails. VISION (per rule):
The named- Fork CI was approved after this review. What is not cleared:
This is waiting on the author, not the captain: push a matching Merge-eligible: NO. |
wjkawecki-jt
changed the title
2 participants
Intent
GitHub can return success from a merge command without actually merging or queueing the pull request when branch rules require the merge queue. This change reads the pull request back after every successful GitHub merge call and reports success only when that read proves the pull request is merged or in the merge queue; every other readable outcome refuses with its concrete observed state.
When the base branch requires the merge queue, the refusal names the exact
-- --auto --<method>retry flags only when all applicable queue rules agree, and reports conflicting methods as ambiguous rather than guessing. The script never changes or auto-applies the operator's chosen merge semantics.PR metadata and the merge poll remain recorded whenever the pull request exists, including when the merge call succeeds but the outcome read fails. That read failure still exits non-zero and makes no success claim, preserving teardown bookkeeping without treating an unverifiable merge as landed.
The existing
ghprerequisite remains becausegh-axidepends on it and the verification reads GitHub's API; failing closed when that read is unavailable is intentional. The existing GitLab verification path is unchanged, and no harness or runtime-backend behavior is involved.What Changed
Risk Assessment
Testing
The focused merge and adjacent PR-check suites passed, with CLI evidence saved. The teardown suite failed only on the unchanged Herdr missing-adapter fixture. Lint and ShellCheck were not run because this assigned test phase forbids static-analysis commands; the outer executor owns those checks.
Evidence: Focused merge executable transcript
Source: Focused merge executable transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
bin/fm-pr-merge.sh:346- Required criterion: “when the base branch requires the merge queue ... name the exact operator flags that work.” The rules endpoint returns all active rules, and multiple rulesets may apply; if two applicable merge-queue rules use the same method, lines 346-348 reject count=2 and silently omit the compatible retry flags. Aggregate agreeing methods, or explicitly report an ambiguity when they conflict. See rules API and ruleset layering.🔧 Fix: Aggregate queue rules and report conflicts explicitly
✅ Re-checked - no issues remain.
tests/fm-teardown.test.sh:1547- The unchanged teardown suite fails at the Herdr missing-adapter fixture: removing the adapter causes an early source error instead of the expected refusal message. This is outside the target diff and unrelated to GitHub merge verification.rtk bash tests/fm-pr-merge.test.shrtk bash tests/fm-pr-check-security.test.shrtk bash tests/fm-teardown.test.shFocused Herdr missing-adapter reproduction with the adapter removed from the copied test fixtureEvidence capture viartk bash -o pipefail -c ...✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.