fix(orb): stop a conclusion-derived verdict from overwriting a recorded close - #8826
Merged
Conversation
…ed close Both gate_decision writers key the same deterministic row id (gate:<source>:<project>#<pr>@<sha>) with ON CONFLICT DO UPDATE, so the last writer wins. One caller records the ACTUAL disposition the bot acted on; the other derives the verdict from the gate-check conclusion alone, where success maps to merge. On a PR the bot closed for a downstream reason (CI failure, policy) the conclusion-only writer runs last and clobbers the real close with a merge. On #5861 the close landed at 20:23:31 and the contradicting merge verdict was written at 20:23:44 -- 13 seconds after the PR was already closed. Fleet calibration reads the latest gate_decision as the gate's prediction, so every such row is scored as a merge prediction that ended closed: a false positive that never happened. Measured on the live self-host, 59 rows carry a verdict timestamped after the close action it contradicts, out of 210 in that class -- biasing published accuracy DOWNWARD, opposite to the reversal under-counting in #8823. Advances #8825 (the recording half; scoring policy closes as their own class is the remaining part). The DO UPDATE now skips when the incoming write is conclusion-derived and the stored decision is already 'close'. A close is a terminal action that already happened and no later conclusion can un-close it. An explicit action still replaces it, and non-close rows keep latest-finalize-wins.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8826 +/- ##
==========================================
- Coverage 93.83% 92.28% -1.55%
==========================================
Files 800 800
Lines 79832 79833 +1
Branches 24197 24198 +1
==========================================
- Hits 74909 73677 -1232
- Misses 3558 5092 +1534
+ Partials 1365 1064 -301
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Stops a gate verdict derived from the check conclusion from overwriting the verdict the bot actually acted on. Advances #8825 — this is the recording half; deciding how policy closes should be scored is the remaining part of that issue.
Why
Two callers write
gate_decision, and both key the same deterministic row id (gate:<source>:<project>#<pr>@<sha>) withON CONFLICT DO UPDATE— so the last writer wins:processors.ts:3278records the actual disposition the bot acted on (action: disposition.actionClass).processors.ts:~10621records a verdict derived from the gate conclusion alone, wheresuccess → merge(nativeGateActionFromConclusion).On a PR the bot closed for a downstream reason (CI failure, contributor cap, policy), the conclusion-only writer runs last and clobbers the real
closewithmerge. Concretely, #5861:agent.action.closepr_outcomeclosedgate_decisionmerge/ success ← 13s AFTER the closeFleet calibration reads the latest
gate_decisionas the gate's prediction, so every such row is scored as "the gate said merge and it ended up closed" — a false positive that never happened. The gate decided close, and the close was correct.Measured on the live self-host across the
merge-verdict-but-closed-outcome class (n=210): 84 were bot-closed and 59 carry a verdict timestamped after the close action it contradicts. This biases published accuracy downward — the opposite direction from the reversal under-counting in #8823, which is why no accuracy figure is currently trustworthy in either direction.How
The
DO UPDATEis guarded: it skips when the incoming write is conclusion-derived (input.actionabsent) and the stored decision is alreadyclose. A close is a terminal action that already happened; no later conclusion can un-close it, so the older row wins.Deliberately narrow — the guard fires only for the exact contradiction observed:
close(a genuine later disposition change is still recorded);closerows keep latest-finalize-wins unchanged;Verification
Note on the historical rows
This stops new contradictions. The 59 already-recorded ones still misreport; quarantining or correcting them stays tracked on #8825 alongside the policy-close scoring decision.