fix(gate): never plan an add and a remove of the manual-review label together - #10167
Merged
Conversation
Contributor
|
Important 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏳ LoopOver is waiting…LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting |
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (92.85%) is below the target coverage (99.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #10167 +/- ##
==========================================
- Coverage 92.21% 91.34% -0.88%
==========================================
Files 931 931
Lines 114071 114069 -2
Branches 27548 27547 -1
==========================================
- Hits 105188 104193 -995
- Misses 7584 8773 +1189
+ Partials 1299 1103 -196
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…together #10155 is flapping in production on beta.7: the label is removed and re-added roughly every 90 seconds -- four cycles in eight minutes, each pair a GitHub write and a subscriber notification. The audit trail shows both operations in the SAME pass, two seconds apart. Three post-plan transforms surface the manual-review hold -- the merge circuit-breaker, the close circuit-breaker, and the close-audit holdout (#8831). All three carried the identical idempotency check, which looks for an existing ADD (`labelOp !== "remove"`) and therefore cannot see a planned REMOVE. When the planner has already scheduled a release -- section 1b does, whenever nothing IT knows about still wants a hold -- the transform appended an add next to that remove, and the executor performed both. Latent until #10116. `noManualReviewHoldWanted` used to include `!mergeableStateUnstable`, which suppressed the release on exactly these PRs and accidentally masked the missing case. Removing that term (correctly -- it was the latch keeping #10098 stuck) let the release fire and exposed the contradiction. #10116 did not create this; the label was previously a sticky latch, quieter but strictly more broken. The planner cannot fix it from its side. Section 1b's doc calls noManualReviewHoldWanted "every reason that would ADD this label, in one place"; it is not, and cannot be, because these three run AFTER planning -- #8831 landed long after that comment was written. Extending the planner's list would just be one more thing to remember on the next transform. So one shared withManualReviewHoldLabel() drops a planned release of the same label before adding, and all three call sites use it. The contradiction becomes unrepresentable where the add happens. The remove is dropped rather than the add skipped: a transform only gets there by having just diverted a merge or a close, so its hold is strictly newer than the release decided before that diversion. Scoped to this label only -- dropping every label remove would silently defeat the stale-disposition-label cleanup, which is its own mutation test. gate.closeAuditHoldoutPct is 20 (the maximum) in the global Orb config, so about one in five would-close PRs is eligible for the path that triggered this. Closes #10164
JSONbored
force-pushed
the
fix/label-add-remove-flap
branch
from
July 31, 2026 10:53
69a6e37 to
64f7dee
Compare
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
1 participant
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.
Closes #10164
P1 — live in production. #10155 on
orb-v3.7.0-beta.7:Four add/remove cycles in eight minutes on one PR — a GitHub write and a subscriber notification each. The audit trail shows both operations in the same pass, two seconds apart:
Root cause
Three post-plan transforms surface this hold —
downgradeMergeToHold,downgradeCloseToHold, andapplyCloseAuditHoldout(#8831). All three carried the identical idempotency check:It looks for an existing add, so by construction it cannot see a planned remove. When the planner has already scheduled a release, the transform appends an add beside it. One plan, both operations, same label — forever.
Why now, and why #10116 is not to blame
Latent until #10116.
noManualReviewHoldWantedused to include!mergeableStateUnstable, which suppressed the release on exactly these PRs and accidentally masked the missing case. Removing that term — correctly; it was the latch keeping #10098 stuck — let the release fire and exposed the contradiction. The label was previously a sticky latch: quieter, but strictly more broken.Why the planner can't fix it
Section 1b's doc calls
noManualReviewHoldWanted"the complete condition: every reason that would ADD this label, in one place." It is not, and cannot be — these three run after planning, so their reasons are not knowable there, and #8831 landed long after that comment was written. Extending the planner's list is one more thing to remember on the next transform.Fix
One shared
withManualReviewHoldLabel(planned, labelSettings, action)that drops a planned release of the same label before adding, used by all three sites. The contradiction becomes unrepresentable where the add happens.The remove is dropped rather than the add skipped: a transform only reaches there by having just diverted a merge or a close, so its hold is strictly newer information than the release the planner decided before that diversion.
Scoped to this one label — dropping every label remove would silently defeat the stale-disposition-label cleanup.
Verification
agent-actions+close-audit-holdoutsuites: 357 pass.That last one is why the filter is label-scoped: the naive fix breaks the stale-label cleanup.
Note
This also removes three copies of the same 8-line block, which is what let one of them (#8831, added later) diverge from the other two without anyone noticing.
gate.closeAuditHoldoutPctis 20 — the maximum — in the global Orb config, so roughly one in five would-close PRs is eligible for the path that triggered this. Per maintainer decision the holdout stays enabled and we ship the fix rather than zeroing it, so the close-precision measurement series keeps no gap.