Skip to content

Repeated-draft-cycling counter has no delivery idempotency — a queue retry of one legitimate draft conversion inflates it to a false "2nd offense" and wrongly closes the PR #6414

Description

@JSONbored

Severity: HIGH
Location: src/db/repositories.ts:3909

Found via a full-system adversarial audit of the ORB review/scoring engine (2026-07-15) — 2 independent skeptic passes, both had to vote "confirmed" for this to survive.

Failure scenario

A non-maintainer contributor performs their FIRST-EVER, entirely legitimate ready→draft conversion on their own PR (reviewEvasionProtection is default-ON, #4011). GitHub delivers the converted_to_draft webhook. In processGitHubWebhook (src/queue/processors.ts:5845-5854), bumpPullRequestDraftConversionCount(env, repoFullName, pr.number) runs unconditionally and bumps the counter 0→1; maybeCloseRepeatedDraftCycling correctly no-ops since count<2. Processing continues for many more steps in the SAME webhook pass (public-surface publish, prReadyForReview, AI review, live GitHub CI/mergeable fetches at src/queue/processors.ts:5876-5957, which explicitly RE-THROWS on isGitHubRateLimitedError(error) || isRetryableJobError(error) at line 5945, proving mid-pass retries are an expected, designed-for occurrence). If ANY downstream step throws an uncaught/retryable error (a GitHub 5xx, a rate-limit edge, a transient D1 write failure), the top-level queue consumer's catch block in src/index.ts:88-105 calls message.retry(), redelivering the SAME message body (same deliveryId, same payload) for a full re-run of processJob. There is no dedup check anywhere before this point: getWebhookEvent/webhook_events is only ever consulted in the dead-letter-queue path (src/queue/dlq.ts), never as an idempotency gate in processGitHubWebhook itself. On retry, bumpPullRequestDraftConversionCount runs again for the SAME physical conversion, bumping 1→2. Now maybeCloseRepeatedDraftCycling(..., draftConversionCount=2) fires for real: it closes the PR, posts "LoopOver detected this pull request has been converted to draft 2 times — repeatedly cycling... is not allowed", applies the review-evasion label, and records a moderation-engine strike via applyModerationEscalationForRule — all against a contributor who converted to draft exactly once.

Impact

An innocent, first-time contributor gets their PR auto-closed and publicly accused of gaming the review process, plus a moderation strike toward the repo's ban threshold, purely from ordinary infrastructure retry behavior unrelated to their conduct — exactly the "contributor treated unfairly" failure mode this audit is meant to catch. bumpPullRequestDraftConversionCount (src/db/repositories.ts:3909-3921) is a bare +1 UPDATE with zero deliveryId/idempotency protection, and is explicitly documented as intentionally NOT head-scoped ("a fresh head must not reset the count back to zero"), removing the one accidental reset that might otherwise have limited the blast radius. This is a striking contrast with the rest of the same file: recordModerationViolation/hasModerationViolationForTarget (src/db/repositories.ts:3043-3078) is deliberately idempotent per (actor, eventType, targetKey) specifically "so a webhook redelivery or queue retry that re-executes an already-recorded close must not double-count", and bumpPullRequestMergeAttempt is headSha-scoped and only invoked inside a genuine merge-failure catch block (one bump per real GitHub API failure, not one bump per webhook-processing attempt). The downstream strike-dedup only protects against re-counting an already-successful enforcement action; it does nothing to stop the upstream input (draftConversionCount) from being poisoned by a retry before enforcement even runs. I confirmed via grep that every existing test in describe("repeated ready<->draft cycling"...) in test/unit/queue-lifecycle-guards.test.ts uses two DISTINCT deliveryIds to simulate two separate physical conversions; none exercises calling processJob twice with the SAME deliveryId, so this retry-double-count path has zero regression coverage.

Metadata

Metadata

Assignees

Labels

maintainer-onlyOwner-only work — yields no Gittensor points.

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions