Skip to content

fix(orb): backport the webhook-redelivery guard to two throttle functions - #8740

Merged
JSONbored merged 2 commits into
JSONbored:mainfrom
kai392:fix/critical-issue-throttle-redelivery-guard
Jul 26, 2026
Merged

fix(orb): backport the webhook-redelivery guard to two throttle functions#8740
JSONbored merged 2 commits into
JSONbored:mainfrom
kai392:fix/critical-issue-throttle-redelivery-guard

Conversation

@kai392

@kai392 kai392 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Fixes #8681

Root cause

maybeThrottleLoopOverCommand and maybeThrottleIntentRouting guard against a redelivered GitHub webhook double-counting toward a rate limit via hasAuditEventForDelivery (added by 066398eba2, mirrored by f8f281b87). Two older, structurally identical throttle functions — maybeThrottleReviewNagPing and maybeThrottleMonitoredMentions, the ones that guard was originally generalized from — never received the backport.

So a redelivered issue_comment (a known, already-solved problem elsewhere in this file) double-counted a review-nag ping or a monitored-mention toward its threshold, which could push a legitimate pinger over threshold early and post a duplicate cooldown/close comment.

Fix

Added the same hasAuditEventForDelivery guard to both functions, mirroring maybeThrottleIntentRouting's exact shape: right after the targetKey is computed, if a prior audit event already exists for this deliveryId within the redelivery window, short-circuit (return true) before counting. No other throttle logic changed.

Tests

  • New (reviewNag): fires the same issue_comment twice with one deliveryId; asserts github_app.review_nag_ping is recorded once, not twice.
  • Updated (monitoredMentions): an existing test at queue-5.test.ts explicitly documented the bug — it asserted 2 pings from 2 deliveries with a comment "does not dedup by deliveryId... documents CURRENT behavior." Updated it to assert 1 (the fixed behavior) and rewrote the comment. This is the monitored-mention half of the deliverable.

Both tests are genuinely non-vacuous: reverting the two guards makes each fail (count 2, expected 1). The whole queue-5 suite (199 tests) stays green — no other test assumed double-counting.

Validation

  • 199/199 in queue-5.test.ts, rebased onto current main
  • lcov scoped to processors.ts: both guard blocks' added lines and branches covered
  • npm run typecheck clean; oxlint clean on the changed lines; git diff --check clean

@kai392
kai392 requested a review from JSONbored as a code owner July 26, 2026 01:50
@superagent-security

superagent-security Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

…ions

maybeThrottleReviewNagPing and maybeThrottleMonitoredMentions never received
the hasAuditEventForDelivery redelivery guard that maybeThrottleLoopOverCommand
and maybeThrottleIntentRouting carry, so a redelivered issue_comment
double-counted a ping toward the throttle (and could re-post a cooldown/close).
Adds the same guard to both; a redelivery is now suppressed, counted once.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.19%. Comparing base (a93ae0a) to head (9fd940c).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8740      +/-   ##
==========================================
+ Coverage   90.56%   91.19%   +0.63%     
==========================================
  Files          96       97       +1     
  Lines       22490    25686    +3196     
  Branches     3884     5020    +1136     
==========================================
+ Hits        20367    23425    +3058     
- Misses       1945     1994      +49     
- Partials      178      267      +89     
Flag Coverage Δ
backend 95.68% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/queue/processors.ts 95.68% <100.00%> (ø)

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 26, 2026
@loopover-orb

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-26 05:25:27 UTC

2 files · 1 AI reviewer · no blockers · readiness 95/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR mirrors the existing `hasAuditEventForDelivery` redelivery guard from `maybeThrottleLoopOverCommand`/`maybeThrottleIntentRouting` into `maybeThrottleReviewNagPing` and `maybeThrottleMonitoredMentions`, placed identically right after `targetKey` computation before any counting occurs. The change is narrow, correctly closes issue #8681, and the accompanying test changes are non-fabricated: the new `reviewNag` test and the updated `monitoredMentions` assertion explicitly flip documented current (buggy) double-counting behavior to the fixed single-count behavior, which would fail if the guard were reverted. The `processors.ts` file is pre-existing and enormous (13.5k lines); this diff doesn't materially worsen that but is a real long-file convention violation being flagged generically.

Nits — 4 non-blocking
  • src/queue/processors.ts: the redelivery guard hunks duplicate the same 2-line pattern already used in two other throttle functions in this file — worth extracting a small shared helper (e.g. `guardWebhookRedelivery(env, actor, eventType, targetKey, deliveryId)`) to avoid a 4th near-identical block if a 5th throttle function needs it later.
  • test/unit/queue-5.test.ts: the new `reviewNag` regression test only asserts the audit-event count and doesn't also assert that the normal @​loopover reply/cooldown logic still fires correctly on the non-redelivered path, unlike some other tests in this file that check both.
  • Consider factoring the repeated `redeliverySinceIso`/`hasAuditEventForDelivery` pair into one small shared helper given it now appears in four throttle functions across this file.
  • If a fifth throttle-style guard is ever added, add a lint/test convention note so this backport pattern isn't missed again as it was for these two functions.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8681
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 153 registered-repo PR(s), 81 merged, 6 issue(s).
Contributor context ✅ Confirmed Gittensor contributor kai392; Gittensor profile; 153 PR(s), 6 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The diff adds the hasAuditEventForDelivery guard to both maybeThrottleReviewNagPing and maybeThrottleMonitoredMentions mirroring the existing pattern, and includes both a new redelivery test for the nag function and an updated assertion for the mention function proving the double-count is fixed.

Review context
  • Author: kai392
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, Cuda, JavaScript, Kotlin, Perl, TypeScript, Vue
  • Official Gittensor activity: 153 PR(s), 6 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Triage stale or unlinked PRs.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask 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.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

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.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@JSONbored
JSONbored merged commit 836c8d6 into JSONbored:main Jul 26, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(orb): review-nag and monitored-mention throttles never got the webhook-redelivery guard applied to the command rate limiter

3 participants