fix(nightly): test-merge bot PRs locally instead of filtering the lazy mergeable field - #898
fix(nightly): test-merge bot PRs locally instead of filtering the lazy mergeable field#898tend-agent wants to merge 4 commits into
Conversation
…KNOWN as clean Closes #897
tend-agent
left a comment
There was a problem hiding this comment.
The lazy-mergeable diagnosis is right and the retry loop settles it correctly — I ran the recipe verbatim from the repo root and it exits on the first pass with a warm cache, writes to sanitized paths, and reports #856 as CONFLICTING, matching the PR body.
One gap: the redirect makes a failed query indistinguishable from a clean one, which is the same shape as the bug being fixed. > "$out" truncates the file before gh writes, so any non-zero gh pr list (rate limit, transient 5xx, wrong working directory) leaves $out empty. jq on empty input emits nothing, so $(...) is the empty string and [ "" -eq 0 ] errors with integer expression expected instead of breaking — the loop burns all five iterations, then both reporting jq calls print nothing. The step then reads as "every PR settled, none conflicting" and gets skipped. I reproduced this by running the recipe with gh unable to resolve the repo: 2m30s per author, no output, exit 0.
[] (3 bytes) is non-empty while a failed query leaves 0 bytes, so [ -s "$out" ] separates "this author has no open PRs" from "the query never landed" without an extra call. Suggestion inline.
| gh pr list --author "$author" --json number,title,mergeable,headRefName,author \ | ||
| --jq '.[] | select(.mergeable == "CONFLICTING")' | ||
| out="/tmp/prs-${author//\//-}.json" # `app/dependabot` has a slash; strip it | ||
| for _ in 1 2 3 4 5; do |
There was a problem hiding this comment.
is there a way to do this with a function of "wait for the answer", rather than repeating for 5 steps?
There was a problem hiding this comment.
There isn't one on the GitHub side — mergeable has no blocking read, and the REST docs say so explicitly: "If the value is null, then GitHub has started a background job to compute the mergeability. After giving the job time to complete, resubmit the request." Polling is the documented interface.
But we don't have to ask GitHub. git merge-tree --write-tree performs the same merge locally and returns the answer synchronously, so the loop disappears rather than getting wrapped in a function. Pushed in 988b4c1.
Checked against GitHub's own answer on all 30 open bot PRs in this repo, with the cache warm so its values are authoritative: exact agreement, only #856 CONFLICTING. Whole step now takes ~2s across all three authors, no sleeps.
The one thing it does need is the PR heads locally — one batched git fetch of refs/pull/N/head (covers fork PRs too, --force since bot branches get force-pushed). fetch-depth: 0 is already set by the shared checkout macro, so every generated nightly job has the history for it.
Failure stays distinguishable from clean, which was the point of the previous round: git merge-tree exits non-zero both for a real conflict and for a ref it couldn't resolve, but a conflict prints the tree OID and conflicted paths on stdout while an error prints nothing — so the recipe tests the captured output, not the status alone. I exercised both branches directly; an unfetched ref reports "merge test never ran, conflicts unverified" rather than passing as clean.
git merge-tree answers the same question synchronously, so the retry loop goes away entirely.
tend-agent
left a comment
There was a problem hiding this comment.
Dropping mergeable for a local test-merge is the right call, and this run happens to demonstrate why: gh pr list --json mergeable returned UNKNOWN for 24 of the 26 open tend-agent PRs on a cold read, while the recipe answered synchronously and flagged #856 and #809 as conflicting. #809 is the case the old filter drops silently — cold mergeable says UNKNOWN, the local merge says CONFLICTING. The error/conflict discrimination holds up too: an unresolvable ref exits non-zero with empty stdout, so [ -n "$tree" ] is load-bearing exactly as the comment claims.
Two gaps, both in the same silent-miss class the PR is closing:
gh pr listhas no--limit, so it silently truncates at 30.review-reviewers's SKILL.md already carries this as a rule ("--limit 100is load-bearing —gh pr listdefaults to 30 and truncates silently"), and this repo is at 26 open bot PRs today; on a dependabot-heavy adopter the 31st PR onward is never test-merged and the step reads as clean for it.- The output line drops the author. The old snippet emitted the whole JSON object, so
.authorwas there; the two sub-steps that consume this list branch on exactly that field —app/dependabot/app/renovateroute to Upstream dependency bots: trigger the bot's own rebase (whose table is keyed on the--author (PR list)value),$BOT_LOGINroutes to the manual worktree resolution.CONFLICTING: #809 fix(report-failure): …doesn't say which, and title text isn't a reliable proxy.
Suggestions inline.
Problem
Step 3 of the bundled
nightlyskill found conflicted bot PRs with a singlegh pr listwhose result was filtered onselect(.mergeable == "CONFLICTING"), followed by "Skip the rest of this step if none of the queries return anything."mergeableis not stored — GitHub computes it lazily. The first query after the base branch moves returnsUNKNOWNand enqueues the computation; a later query returns the real value.select(.mergeable == "CONFLICTING")dropsUNKNOWNsilently, so a cold cache was indistinguishable from a clean one, and the skip line turned "I don't know" into "there are no conflicts" — skipping the whole step, including the Bot-authored PRs: resolve manually subagent dispatch that exists to rebase the bot's own conflicted PRs.Reported in #897 with session-log evidence from ten sampled nightly runs.
Reproduction
Confirmed live against
nodejs/node, two queries seconds apart with nothing touched in between:UNKNOWNCONFLICTINGMERGEABLEThe cold read reported zero conflicted PRs; the settled read revealed one that was genuinely
CONFLICTING.Solution
Don't read
mergeableat all — compute the merge locally withgit merge-tree --write-tree, which is synchronous and authoritative, so there is no cache to wait on and no retry loop. There is no blocking read on the GitHub side: the REST docs prescribe resubmitting the request untilmergeableis non-null.The recipe fetches every PR head in one batched
git fetch(refs/pull/N/head, so fork PRs are covered too,--forcebecause bot branches get force-pushed), then test-merges each againstorigin/main. Failure stays distinguishable from clean at both steps: an emptygh pr listoutput file (0 bytes, versus 3 for[]) reports as unverified, andgit merge-treeexits non-zero both for a real conflict and for a ref it couldn't resolve — the conflict prints the tree OID and conflicted paths on stdout, the error prints nothing, so the recipe tests the captured output rather than the exit status alone.The output path is
/tmp/prs-${author//\//-}.json—app/dependabotandapp/renovatecontain a slash, so the un-sanitized form suggested in the issue would redirect into a nonexistent/tmp/prs-app/directory and fail.Testing
Ran the recipe verbatim from the repo root against this repo's own PRs: it agrees exactly with GitHub's own (warm, therefore authoritative) answer across all 30 open bot PRs, flagging only #856 as
CONFLICTING, and finishes in ~2s for all three authors with no sleeps. Both failure branches were exercised directly — agh pr listthat can't resolve the repo reports "query never landed", and an unfetched head ref reports "merge test never ran" — neither reads as clean.select(.mergeableappears at no other site in the bundled skills; the only othermergeablementions are workflow comments about merge-ref materialization, which are unrelated.fetch-depth: 0is set by the shared checkout macro ingenerator/src/tend/templates/macros.yaml.j2, so every generated nightly job has the history the local merge needs.Closes #897 — automated triage