fix(queue): close four webhook-redelivery holes and guard the family in CI (#9561, #9562, #9563) - #9567
Merged
Conversation
…in CI (#9561, #9562, #9563) GitHub redelivers the same issue_comment, and the queue's max_retries:3 plus the DLQ re-drive reuse the identical deliveryId. There is no global delivery-level dedupe -- src/github/webhook.ts suppresses a re-POST whose row is already `processed`, but a QUEUE retry re-enters processGitHubWebhook directly and recordWebhookEvent is an upsert written after the handler returns. #9312 added a guard to the handlers someone greped for at the time; four were missed. Eleven functions share the signature (env, deliveryId, payload), written in TWO formattings -- one-line and five-line. A grep for either shape sees only half the family. That is the mechanical cause, not carelessness. WHAT A REPLAY COST, per handler: - maybeProcessResolveCommand: a SECOND permanent review-memory suppression row per finding (recordReviewSuppression), plus a re-posted confirmation. - maybeProcessGateOverrideCommand: the writes are individually idempotent, which is why this was easy to miss -- but resolveOverrideHeadSha re-fetches the LIVE head on purpose, so if a commit landed in between, the replay neutralizes the Gate on a NEWER commit the maintainer never overrode, breaking the handler's own "no permanent bypass, this commit only" invariant. - maybeProcessPrPanelRetrigger: the publish passes forceAiReview, which by design bypasses the AI-review cache AND the cross-head fingerprint cache AND steals the review lock ("a duplicate LLM call is the explicitly accepted cost"). Right for a maintainer re-ticking the box; wrong for a queue retry. A guaranteed second paid review, and non-deterministic output can flip the published verdict. - maybeProcessPrPanelGenerateTests: a second real AI generation plus a brand-new public comment (createIssueComment, not the in-place marker kind). Its text twin already guarded the identical event type and targetKey. - maybeProcessLoopOverMentionCommand: the ~20 commands answered INLINE. Checks BOTH agent_command_replied and agent_command_reply_skipped, because a dry-run original still spent the agent run and the AI summary while recording only the skipped event -- keying on the completed event alone would leave exactly the replays that already cost money unguarded. Verified: with only the completed arm, the dry-run regression test fails. maybeThrottleLoopOverCommand is NOT a substitute: it returns early on `policy === "off"`, and commandRateLimitPolicy defaults to 'off' (migration 0097). VERIFIED SAFE, left unguarded with the mechanism stated in the allowlist: - maybeProcessConfigurationCommand: idempotent createOrUpdateAgentCommandComment with a deterministic body, so the PATCH is skipped. - maybeProcessPlanCommand: isPlanCommandCoolingDown short-circuits first; its key is strictly broader and ISSUE_PLAN_COOLDOWN_MS equals the redelivery window. - maybeProcessAgentCommandFeedbackReaction: recordAgentCommandFeedback is an onConflictDoUpdate on (answerId, actorHash) behind a unique index, so a replay rewrites the same vote rather than double-counting. scripts/check-command-redelivery-guards.ts fails CI on a twelfth handler. It normalizes whitespace so both formattings match, and bounds each body by brace depth rather than a line window -- a fixed window would let a neighbour's guard satisfy the scan for the handler actually missing one, the exact false negative hit while check-regate-sort-key.ts was written. Every guard has a regression test proven against its own removal: each fails with the guard taken out (2 suppression rows, a second overridden SHA, 2 AI runs, 2 forced refreshes, a duplicate public answer) and passes with it in.
Contributor
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-28 10:15:19 UTC
Review summary Nits — 7 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.
|
Contributor
|
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 | 1236db6 | Commit Preview URL Branch Preview URL |
Jul 28 2026, 09:55 AM |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9567 +/- ##
=======================================
Coverage 89.62% 89.62%
=======================================
Files 867 867
Lines 110801 110813 +12
Branches 26346 26351 +5
=======================================
+ Hits 99307 99319 +12
Misses 10229 10229
Partials 1265 1265
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Bundle ReportChanges will decrease total bundle size by 210 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
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 #9561
Closes #9562
Closes #9563
Summary
GitHub redelivers the same
issue_comment, and the queue'smax_retries: 3plus the DLQ re-drive reuse the identicaldeliveryId. There is no global delivery-level dedupe —src/github/webhook.tssuppresses a re-POST whose row is alreadyprocessed, but a queue retry re-entersprocessGitHubWebhookdirectly, andrecordWebhookEventis an upsert written after the handler returns.#9312 added a redelivery guard to the handlers someone greped for at the time. Four were missed.
Eleven functions share the signature
(env: Env, deliveryId: string, payload: GitHubWebhookPayload), written in two formattings — one line and five. A grep for either shape sees only half the family. That is the mechanical cause, not carelessness, and it is what the new CI check removes.What a replay actually cost
maybeProcessResolveCommandmaybeProcessGateOverrideCommandresolveOverrideHeadShare-fetches the live head on purpose, so a commit landing in between means the replay neutralizes the Gate on a newer commit the maintainer never overrodemaybeProcessPrPanelRetriggerforceAiReviewbypasses the review cache and the fingerprint cache and steals the lock, by designmaybeProcessPrPanelGenerateTestscreateIssueComment, not the in-place marker kind)maybeProcessLoopOverMentionCommandThe gate-override writes are each individually idempotent (the check-run is PATCHed in place, the confirmation is a marker comment) — which is exactly why the missing guard was easy to miss. The damage is target drift, not duplication.
The dispatcher checks two event types, and both are load-bearing
A dry-run/paused original still spent the agent run and the AI summary, but recorded
agent_command_reply_skippedrather thanagent_command_replied. Keying on the completed event alone would leave exactly the replays that already cost money unguarded. Verified by removing one arm at a time: with only the completed arm, the dry-run regression test fails.maybeThrottleLoopOverCommandcontains its own redelivery short-circuit but is not a substitute — it returns early onpolicy === "off", andcommandRateLimitPolicydefaults to'off'(migration 0097), so on a default repo it never runs.Verified safe, left unguarded — with the mechanism stated
The allowlist in the new checker carries these, so "this one is fine" is a claim a reviewer can check rather than an absence someone has to re-derive:
maybeProcessConfigurationCommand— idempotentcreateOrUpdateAgentCommandCommentwith a deterministic body, so a replay's PATCH is skipped.maybeProcessPlanCommand—isPlanCommandCoolingDownshort-circuits first; its key (actor + repo) is strictly broader than the guard's, andISSUE_PLAN_COOLDOWN_MSis identical toCOMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS.maybeProcessAgentCommandFeedbackReaction—recordAgentCommandFeedbackis anonConflictDoUpdateon(answerId, actorHash)behind a unique index, so a replay rewrites the same vote rather than double-counting.The CI check
scripts/check-command-redelivery-guards.ts, in the established idiom ofcheck-dead-source-files.tsandcheck-regate-sort-key.ts:check-regate-sort-key.tswas being written)Testing
Every guard has a regression test proven against its own removal — each fails with the guard taken out and passes with it in:
Plus 10 direct tests for the checker itself, including both signature formattings, the brace-depth bounding in both orders, nested blocks, exact-name allowlisting, and the real tree staying clean.
src/queue/processors.tschanged lines: 0 uncovered statements, 0 uncovered branches.