ledger(policy): require a declared reason for any repeat verdict at one head SHA - #9865
Conversation
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-29 17:32:56 UTC
Review summary Nits — 6 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
…ne head SHA (#9742) decision_records already kept every evaluation of a (repo, pull, head_sha) -- a repeat lands as <id>:revN, so nothing was ever silently replaced. What the record could not say is WHY a second evaluation happened, which verdict it supersedes, or who asked for it: "evaluated once" and "evaluated three times and one result was kept" were indistinguishable in the public record. The write layer now refuses a repeat verdict that does not declare a reason, so no caller can bypass it by writing the row itself. A new head SHA is untouched -- that is a fresh verdict by definition and needs no reason. The reason is DERIVED, not judged per call site. The delivery-id prefixes that already partitioned job producers were scattered string literals with matching startsWith reads elsewhere; they now live in one module, and the reason map is typed Record<DeliveryIdOrigin, ReevaluationReason> -- adding a producer without assigning a reason fails the build rather than writing an unexplained verdict. scheduled_recheck names the routine sweep. It is by far the highest-volume cause, and without it every sweep tick past the first would have had to borrow one of the incident-shaped codes -- drowning them, and (since both call sites swallow a throw) doing it silently while aborting the rest of the maintenance pass. DecisionRecordContext.reevaluation is required rather than defaulted: a repeat verdict whose cause nobody declared is exactly what this refuses, so a future call site must answer it at the type level instead of inheriting a plausible-looking guess.
ccb76e7 to
dd7448a
Compare
…IGIN key
The allowlist matches a producer by a marker string inside its object literal. The
manual re-gate route no longer repeats the `manual-regate:` prefix literal -- it calls
deliveryIdFor("manualRegate", ...) -- so the marker moved with it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9865 +/- ##
=======================================
Coverage 91.74% 91.75%
=======================================
Files 918 919 +1
Lines 112965 112987 +22
Branches 27184 27193 +9
=======================================
+ Hits 103642 103667 +25
+ Misses 8037 8034 -3
Partials 1286 1286
Flags with carried forward coverage won't be shown. Click here to find out more.
|
… not longest-match The longest-prefix tie-break could only run if one prefix were a prefix of another, which none is -- untestable defensive code standing in for a guarantee nobody stated. The guarantee is now an invariant test, so adding an ambiguous prefix fails with the real problem named rather than being silently absorbed by a scan.
![loopover-orb[bot]](https://avatars.githubusercontent.com/in/4139483?s=60&v=4){kind=small}
1 participant
Closes #9742.
Problem
A verdict's integrity was provable, but its uniqueness was not.
decision_recordsalready keeps every evaluation of a(repo, pull, head_sha)— a repeat lands as<id>:revN, so nothing was ever silently replaced — but the record could not say why a second evaluation happened, which verdict it supersedes, or who asked for it. "This PR was evaluated once" and "this PR was evaluated three times and one result was kept" were indistinguishable in the public record.What changed
Enforcement at the ledger-write layer.
persistDecisionRecordrefuses a repeat verdict for a head SHA that already carries one unless the write declares a reason, so no caller can bypass it by writing the row itself. A new head SHA (force-push, new commits) is untouched — that is a fresh verdict by definition and needs no reason.migrations/0204adds three nullable columns (reason, superseded record id, actor), all NULL for a first evaluation, which is the overwhelming majority of rows.The reason is derived, not judged per call site. Every
agent-regate-prjob already carried a delivery id, and synthetic ones already tagged themselves with a prefix naming their producer — but those prefixes were string literals repeated across 8 producers with matchingstartsWithreads scattered elsewhere. They now live insrc/queue/delivery-id.ts, andREEVALUATION_REASON_BY_ORIGINis typedRecord<DeliveryIdOrigin, ReevaluationReason>: adding a producer without assigning it a reason fails the build rather than writing an unexplained verdict.isScheduledRegateSweepJoband the queue-fairness lane classifier now read from the same table, so they cannot drift from it.scheduled_recheckis a new reason code. The periodic re-gate sweep re-evaluates every open PR at an unchanged head, which is by far the highest-volume cause of a repeat verdict. Without its own code, every sweep tick past the first would have had to borrow one of the incident-shaped codes — drowning them, and (since both call sites swallow a throw) doing so silently while aborting the rest of the maintenance pass.DecisionRecordContext.reevaluationis required, not defaulted. A repeat verdict whose cause nobody declared is precisely what this invariant refuses, so a future call site has to answer at the type level instead of inheriting a plausible-looking guess. Making it required is what surfaced all six executor call sites rather than leaving me to find them by hand; five derive from their owndeliveryId, and the approval-queue accept path — the one case whose cause is a named human — attributes the verdict to whoever accepted it.Acceptance
For any PR, the public record now shows exactly how many evaluations each head SHA received and why any beyond the first occurred.
Validation
test/unit/decision-record.test.ts— 58 tests: refusal without a reason, the linked supersession chain, new-SHA unaffected, actor recorded/trimmed/absent, and the full origin→reason mapping including longest-prefix resolution soregate-repair:is never mis-read as routineregate-sweep:maintenance.npm run test:coverageunsharded: 25,422 passing.tsc --noEmit,db:migrations:check(contiguous 0001..0204),db:schema-drift:check, anddocs:drift-checkall clean.