fix(merge): the risk gate scored a commit it was not merging #5612
ccantynzAI Reviewcommented 45m ago
AI Triage
(no summary)
Priority: medium Risk area: mixed
Suggested labels: (no label suggestions) Suggested reviewers: (no reviewer suggestions)
Suggestions only — nothing has been applied. The PR author stays in control.
gluecron[bot]🤖 botAI Reviewcommented 45m ago
AI review unavailable
The platform's AI balance is exhausted, so AI generation is temporarily unavailable. Nothing was lost. You can queue this as a repair for the internal agent from the repository's Health page, or try again once the balance is restored. The PR is otherwise unchanged.
Cross-repo impact
See what breaks downstream if this PR merges.
⮌ Merged
This pull request was merged into main.
c comment · e edit title · m merge · a approve · r request changes · ? shortcuts
Found while merging this session's own PRs, which is the only reason it was visible: the merge response reported "Touches 11 file(s) with 1159 added" for a head that had eleven commits on it, and
riskScore.commitShanamed a commit eleven earlier. The score was real. It was about a different change.The bug
The gate's lookup chain was:
getCachedPrRisk— SHA-pinned. Correct.getLatestCachedPrRisk— documented as "the most recent cached score for a PR regardless of which SHA it was pinned to".computePrRiskForPullRequest— fresh.Step 2 is the whole defect. It sits between a correct answer and a computable one, and its only effect is to serve a score describing a different commit — step 3 was already there and would have been reached.
Why it isn't cosmetic
This gate soft-blocks the
criticalband. So:A score for a commit you are not merging is not a weaker signal than a fresh one. It is a statement about something else.
The fix
commitSha≠ that head is discarded rather than used.riskAppliesToHeadis exported and asserted directly — for the reason this shipped at all. It was invisible from the call site, where the fallback reads as an ordinary cache policy.auto-merge deliberately keeps the unpinned lookup
And now says why. It re-evaluates on every event for the branch, so a score lagging by one push is corrected next pass, and there the value can only withhold an auto-merge, never authorise one the other gates would refuse. The interactive gate is the opposite: it acts once, on a SHA the caller named. If auto-merge ever gains the ability to approve on risk rather than only hold back, it must be pinned too — that's in the comment so the next person doesn't have to rederive it.
Also:
manifestsScannedcounted attempts, not successesSelf-inflicted, from earlier in this same session, found by applying the GateTest session's point about asserting on honesty fields rather than trusting a reader.
The field was
toRead.length— manifests the walk decided to open. A manifest that failed to parse contributed nothing tototaland still counted toward the figure saying how much of the tree was read. That is exactly the "we looked" / "we succeeded" conflation the field was added to prevent. Now counts successful parses on all three manifest paths; therequirements.txtpath was missed on my first pass.Verification
riskAppliesToHead, including that a short-SHA prefix does not create a false pin.