feat(ci): enforce that dispatch gates can say why they suppressed work (#9003) - #9583
Conversation
#9003) The #9003 invariant: every decision -- and every NON-decision -- records a reason. "Nothing happened and nothing says why" must be structurally impossible. It came out of the 2026-07-26 restart investigation, where a forceAiReview pass completed with no fresh review and zero audit events explaining why (#9000), costing hours of elimination -- while the disposition lane, which names every hold, answered the same class of question in minutes. Much of the issue is already built: #9000's fix landed resolvePublicAiReviewGateSkipReason and the audit on the suppress branch, the vision gates return typed { run, reason } results, and the reputation skip pushes a named finding. What was missing is the piece that keeps it true: the issue asks that "a bare-boolean gate on a dispatch path should fail a lint/unit convention test, the same way no-direct-octokit enforces the write chokepoint." scripts/check-dispatch-gate-reasons.ts is that test. It scans the dispatch-path modules for exported gate-shaped functions (should*/evaluate*Gate*/resolve*Gate*) returning bare boolean, and requires each to either return its reason with the decision, or be allowlisted naming its paired resolver. Both existing conventions pass as-is; what fails is the shape that produced #9000. The checker found one real instance immediately: shouldStartAiReviewForAdvisory -- the exact gate from the #9000 incident. Verified end-to-end that BOTH of its false paths are now named by the caller (the hard gate via resolvePublicAiReviewGateSkipReason, the reputation skip via maybeAddReputationSkipHold's ai_review_inconclusive finding), so its allowlist entry states that mechanism rather than dodging it. Two of my own bugs were caught by the fixture tests while writing this, both worth recording in the file: - the return-type regex was anchored to a line start, so it only matched multi-line signatures -- the whole check was passing VACUOUSLY, including against the real tree - a fixed-width signature slice read past a gate's own body into its neighbour's `): boolean {`, wrongly flagging a reason-returning gate. Now bounded by the function's own opening brace, paren-depth-aware for inline object-type params. Also deliberately rejected: "a resolve*SkipReason anywhere in the module exempts the module" -- one resolver would exempt every future gate beside it, the same neighbour false negative check-regate-sort-key.ts had to kill with brace-bounding. Pairing must be stated per gate. 10 direct tests, including both formatting shapes, exact-name allowlisting, the neighbour non-exemption, and the real tree staying green. Wired into test:ci and the validate-code job.
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-28 18:59:36 UTC
Review summary Nits — 6 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionPartially addressed 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. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | ecb5060 | Commit Preview URL Branch Preview URL |
Jul 28 2026, 06:39 PM |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9583 +/- ##
=======================================
Coverage 89.84% 89.84%
=======================================
Files 875 875
Lines 110977 110977
Branches 26403 26403
=======================================
Hits 99702 99702
Misses 9993 9993
Partials 1282 1282
Flags with carried forward coverage won't be shown. Click here to find out more. |
Bundle ReportBundle size has no change ✅ |
…erasing them (#9000) (#9584) The lost-click half of #9000, completing the issue (the skip-auditing half landed earlier; its enforcement went in as #9583). THE DEFECT. Three checkbox states looked identical to a maintainer: processed (panel republished, box reset), deferred for CI (box stays ticked, honored later via the #7626 pending marker), and DELIVERY LOST (box stays ticked, nothing recorded, nothing will ever happen -- three such losses observed live on #8972, 2026-07-26). Only the third is broken, and it is unrecoverable from our own state alone: the tick exists only in the live comment body, because the webhook that would have told us about it never became a job. Worse, the next panel republish -- whose renderer emits its checkboxes unticked unconditionally -- would OVERWRITE the ticked box, erasing the only evidence of the click on the exact pass best placed to honor it. THE FIX, at zero additional API cost. createOrUpdateIssueCommentWithMarker already fetches the existing comment for its marker search; it now returns that pre-overwrite body. Every panel republish (webhook pass, re-gate sweep, backlog convergence) doubles as the detector: a ticked rerun box in the body being replaced, with no recent processing and no pending marker, IS a lost click. Recovery then: - records github_app.pr_panel_retrigger_recovered (the named reason #9003 demands, and the loop guard against re-detection), - persists the #7626 pending marker, so the next review pass consumes it as forceAiReview exactly like a deferred click, and - enqueues an agent-regate-pr job immediately (with the #9499 prCreatedAt sort key) -- a PR with a lost click is precisely the PR that cannot count on a natural pass. No new sweep, no new persistence: the issue's "within one sweep interval" acceptance is bounded by the republish cadence that already exists. FALSE-POSITIVE PROOFING, the part that took the design care: - A pass that IS the retrigger (forceAiReview) skips detection -- overwriting its own tick is the receipt acknowledgement, not a loss. - A recently-processed retrigger (actor-agnostic audit lookup, new countAuditEventsForTargetSince -- hasAuditEventForDelivery is deliberately actor+delivery-keyed, the wrong shape when the delivery never existed) means the delivery raced the pass rather than being lost. - A DEFERRED click is guarded by ordering, not luck: the retrigger handler now marks the pending marker BEFORE its readiness check (consuming it right back on the immediate path), so the CI-wait placeholder inside that very pass sees the marker and stands down. A crash between mark and consume degrades to one extra forced review -- the safe direction for an explicit user click. The #7626 sibling test's "never creates a pending marker" assertion is updated to the invariant it documents ("no pending INTENT survives the immediate path") rather than its old mechanism (key absence), since consumption can leave the one-shot consumed sentinel behind on adapters without a real delete. TESTS. Eight in the new suite: the #8972 regression (natural pass recovers the tick: event + marker + job with the sort key), full end-to-end (the recovery job's pass consumes the marker, spends exactly one fresh AI review, and does not re-diagnose its own republish), the raced-delivery guard, the deferred-click guard (CI genuinely pending, marker untouched afterward), the no-head ghost PR (marker guard skipped, job omits the optional sort key), unticked and first-publish no-ops, and the empty event-type-list guard. Plus the three github-comments assertions extended to pin previousBody, including that it is the POSTED body, not the re-render, on the byte-identical skip path. Changed lines: 0 uncovered statements, 0 uncovered branches.
Closes #9003
Summary
The #9003 invariant: every decision — and every non-decision — records a reason. "Nothing happened and nothing says why" must be structurally impossible. It came out of the 2026-07-26 restart investigation, where a
forceAiReview: truepass completed with no fresh review and zero audit events explaining why (#9000), costing hours of elimination — while the disposition lane, which names every hold viaagentHoldAuditDetail, answered the same class of question in minutes.Most of #9003 was already built — this PR adds the piece that keeps it true
Verified before writing anything:
resolvePublicAiReviewGateSkipReasonplus the audit on the suppress branch (deliverable 2's "one audit event per suppressed lane"){ run, reason }results (deliverable 1's shape)ai_review_inconclusivefindingWhat was missing is enforcement. The issue is explicit about the form: "a bare-boolean gate on a dispatch path should fail a lint/unit convention test, the same way
no-direct-octokitenforces the write chokepoint."scripts/check-dispatch-gate-reasons.tsis that test. It scans the dispatch-path modules for exported gate-shaped functions returning barebooleanand requires each to either return its reason with the decision (theevaluateVisualVisionGateshape) or be allowlisted naming its paired resolver (theshouldRequirePublicAiReviewForAdvisoryshape). Both existing conventions pass as-is. What fails is exactly the shape that produced #9000.It found the real one immediately
First honest run flagged
shouldStartAiReviewForAdvisory— the exact gate from the #9000 incident. Rather than silence it, I verified end-to-end that both of itsfalsepaths are now named by the caller (the hard gate viaresolvePublicAiReviewGateSkipReason; the reputation skip viamaybeAddReputationSkipHold), and its allowlist entry states that mechanism. An allowlist entry here is a checkable claim, not an escape hatch.Two of my own bugs, caught by the fixture tests while writing this
Recorded in the file because they're instructive:
): boolean {, wrongly flagging a reason-returning gate. Now bounded by the function's own opening brace, paren-depth-aware so an inline object-type parameter doesn't end the signature early.Also deliberately rejected: "a
resolve*SkipReasonanywhere in the module exempts it" — one resolver would exempt every future gate beside it, the same neighbour false negativecheck-regate-sort-key.tshad to kill with brace-bounding. Pairing must be stated per gate.Scope
Deliberately the named dispatch-path modules, not all of
src/**— the rule is about gates that suppress a review or an action, and applying it to every boolean helper would bury the few that matter (the same scoping argument asinert-config.ts). Deliverables 3 and 4 of #9003 (the.catch(() => undefined)completeness pass; per-gate forced-suppression audit tests) are follow-on work the issue lists separately; this PR lands the enforceable convention that was its acceptance criterion — "CI enforces the typed-reason convention on the dispatch paths."Testing
10 direct tests: both formatting shapes (the #9541 lesson — a family written two ways is a family a grep half-sees), exact-name allowlisting, the neighbour non-exemption, absent modules, stable output ordering, and the real tree staying green. Wired into
test:ciand thevalidate-codejob.