fix(ledger): record WHY a pull request was held, not the gate's conclusion - #10072
Merged
Conversation
…usion The ledger filed 518 holds across 146 pull requests under `reason_code = "success"`. That is not a reason: deriveDecisionReasonCode ends with the gate conclusion when there is no blocker and no policy close, so a held PR whose gate passed records the conclusion as its cause. A reader of the public proof page's sample records sees the same thing. It also blocks #9729, which must loosen only what clears a backtest, PER PATH. `success` is not a path -- it is at least the seven inputs in MERGE_HOLD_INPUTS, and a clearance run against a bucket conflating them would mean nothing. reason_code is deliberately UNTOUCHED. replayDecision recomputes it and reports a divergence on mismatch, so changing the derivation would make all 518 existing records replay as unreproducible -- a false "this decision cannot be re-derived" about records that are perfectly fine -- and would change a value the proof page publishes. The cause goes in its own column, outside record_json, so digests and the replay contract are untouched. Historical rows stay NULL: we genuinely do not know which input held those 518, which is the whole finding. NO SIGNATURE CHANGE. The obvious route -- widening planAgentMaintenanceActions to return the cause -- churns five production call sites and roughly thirty test ones, and does not fit the data: a hold's defining shape is that NOTHING was planned, so there is no action to carry the reason. Instead the disposition-input construction is extracted into one builder that the planner and the new `resolveHoldCause` both call, so the cause recorded in the ledger is the same expression that suppressed the merge rather than a second opinion about it. Every value that builder needs is a function of the plan input, which is what makes recomputing it outside the planner sound rather than approximate. `derivePrDisposition` now returns `heldBy` -- which inputs held, in the table's declaration order so the recorded cause is deterministic. It was already a `.some(...)` over exactly that set, throwing the answer away; `heldForManualReview` is now derived from it, so the two cannot disagree. Released holds (#9808's cleared guardrail) are excluded, and the unstable mergeable state stays out because it is GitHub's computation rather than one of our declared inputs. Mutation-tested: ignoring released holds fails 1, forcing heldBy empty fails 3. The extraction is behaviour-neutral -- agent-actions' 347 tests pass unchanged. Closes #9991
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
Additional details and impacted files@@ Coverage Diff @@
## main #10072 +/- ##
==========================================
- Coverage 91.88% 91.88% -0.01%
==========================================
Files 930 930
Lines 113827 113837 +10
Branches 27466 27471 +5
==========================================
+ Hits 104588 104597 +9
Misses 7940 7940
- Partials 1299 1300 +1
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
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.
The ledger filed 518 holds across 146 pull requests under
reason_code = "success". That is not a reason —deriveDecisionReasonCodeends with the gate conclusion when there is no blocker and no policy close, so a held PR whose gate passed records the conclusion as its cause. A reader of the public proof page's sample records sees the same thing.It also blocks #9729, which must loosen only what clears a backtest per path.
successis not a path — it is at least the seven inputs inMERGE_HOLD_INPUTS, and a clearance run against a bucket conflating them would mean nothing.reason_codeis deliberately untouchedThis is the constraint that shaped everything else.
replayDecisionrecomputesreason_codeand reports a divergence on mismatch (decision-replay.ts:168). Changing the derivation would make all 518 existing records replay as unreproducible — a false "this decision cannot be re-derived" about records that are perfectly fine — and would change a value the proof page publishes.So the cause lands in its own column, outside
record_json, leaving every digest and the replay contract untouched. Historical rows stayNULL: we genuinely do not know which input held those 518, and backfilling a guess would be worse than the gap.No signature change
The obvious route — widening
planAgentMaintenanceActionsto return the cause — churns 5 production call sites and ~30 test ones, and does not fit the data anyway: a hold's defining shape is that nothing was planned, so there is noPlannedAgentActionto hang the reason on.Instead the disposition-input construction is extracted into one builder that both the planner and the new
resolveHoldCausecall. The cause recorded in the ledger is therefore the same expression that suppressed the merge, not a second opinion about it. Every value that builder needs (reviewGood,guardrailHit,guardrailEscalationCleared,closeActing) is a function of the plan input — I checked each, including thatconclusionis exactlyinput.conclusion— which is what makes recomputing it outside the planner sound rather than approximate.The extraction is behaviour-neutral:
agent-actions' 347 existing tests pass unchanged.heldByderivePrDispositionnow returns which inputs held, in the table's declaration order so the recorded cause is deterministic for identical inputs. It was already a.some(...)over exactly that set, throwing the answer away;heldForManualReviewis now derived fromheldBy, so the two cannot disagree — pinned by a test that walks every key inMERGE_HOLD_INPUT_KEYSrather than sampling one.Two exclusions are deliberate and tested: a released hold (#9808's guardrail cleared by a clean escalated review) is not a cause, and the unstable mergeable state stays out because it is GitHub's computation rather than one of our declared inputs — it is reported by
heldForUnstableMergeState.Mutation testing
heldByemptyVerification
test/unit+test/integration: 25,972 passed, 0 failedtypecheck,db:migrations:check,db:migrations:immutable:check,db:schema-drift:check,dead-exports:check,dead-source-files:check,import-specifiers:check,coverage-boltons:check,checkers-wired:check,ui:lintgreenrecord_jsonand thatreason_codestill readssuccessUnblocks #9729. Once this ships, the 518-hold bucket becomes splittable by cause for new verdicts, and per-path candidates can be chosen.
Closes #9991