fix(review): recover lost panel-retrigger clicks instead of silently erasing them (#9000) - #9584
Conversation
…erasing them (#9000) 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.
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-28 19:37:17 UTC
Review summary Nits — 5 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. 🟩 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9584 +/- ##
==========================================
- Coverage 89.83% 89.03% -0.81%
==========================================
Files 875 878 +3
Lines 110958 111036 +78
Branches 26413 26434 +21
==========================================
- Hits 99680 98859 -821
- Misses 9992 11155 +1163
+ Partials 1286 1022 -264
Flags with carried forward coverage won't be shown. Click here to find out more.
|
![loopover-orb[bot]](https://avatars.githubusercontent.com/in/4139483?s=60&v=4){kind=small}
1 participant
Closes #9000
Summary
The lost-click half of #9000, completing the issue. Fix (2) — exhaustive skip auditing — was verified already-landed (my comment on the issue), and its CI enforcement merged as #9583. This PR is fix (1) and root-cause (a).
The defect
Three checkbox states looked identical to a maintainer:
Only the third is broken (three observed live on #8972), 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 panel renderer emits its checkboxes unticked unconditionally, so the next republish 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
createOrUpdateIssueCommentWithMarkeralready fetches the existing comment for its marker search. It now returns that pre-overwrite body, and every panel republish — webhook pass, re-gate sweep, backlog convergence — doubles as the detector. No new sweep, no new persistence, no extra GitHub calls; the issue's "within one sweep interval" acceptance is bounded by the republish cadence that already exists.On detection (ticked box + no recent processing + no pending marker), recovery:
github_app.pr_panel_retrigger_recovered— the named reason orb(observability): invariant — every decision or non-decision records a named reason; no silent gates on dispatch paths #9003's invariant demands, doubling as the loop guardforceAiReviewexactly like a deferred clickagent-regate-prjob immediately (with the queue: no 'current focus' concept exists, and oldest-first ordering is defeated by 5 of 8 regate producers omitting prCreatedAt #9499prCreatedAtsort key) — a PR with a lost click is precisely the PR that cannot count on a natural passFalse-positive proofing — the part that took the design care
forceAiReview) skips detection: overwriting its own tick is the receipt acknowledgement.countAuditEventsForTargetSince—hasAuditEventForDeliveryis deliberately actor+delivery-keyed, the wrong shape when the delivery never existed).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 on adapters without a real delete.
Acceptance, as the issue states it
Holds: processed → fresh review; deferred → marker honored on CI settle; lost → recovered, named, and re-reviewed on the enqueued pass. The end-to-end test pins the full loop, including that the recovery pass does not re-diagnose its own republish and that the lost click buys exactly one fresh review.
Testing
Eight tests in the new suite (regression, end-to-end, both race guards, ghost PR, two no-ops, empty-list guard) plus three
github-commentsassertions extended to pinpreviousBody— including that it is the posted body, not the re-render, on the byte-identical skip path.Changed lines across all three files: 0 uncovered statements, 0 uncovered branches. Full suite: 24,039 passed.