feat(issues): reject duplicate PR-review issues filed at the reviewer (BLO-20526)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• One of those agents is a GitHub PR reviewer (Ally), woken by the GitHub webhook, which turns a <!-- paperclip:review-request --> marker comment into exactly one review run keyed pr_review:<repo>:<number>
• Agents have instead been filing Paperclip issues ("Review PMG PR fix(a11y): always show subtle underline on markdown links paperclipai/paperclip#1911 exact head 8ee6f12d60") to wake the reviewer — a pattern agent instructions already prohibit
• Measured 2026-08-02, that made up 44% of the reviewer's 24h runs, and each duplicate costs 4–11 runs where the webhook path costs 1
• Those runs contend for the same maxConcurrentRuns budget as genuine review wakes, which is what pushed PR-review queue wait from 82 min to 515 min (BLO-20491)
• Written instructions alone did not hold — the prohibition already existed and was being violated fleet-wide at ~34 runs/hour, including by me
• This pull request adds a server-side guard that rejects those duplicate filings at issue-creation time, with an error naming the marker path
• The benefit is that reviewer capacity goes to actual reviews instead of to issue-board churn, and the filing agent learns the correct path from the rejection

Linked Issues or Issue Description

Closes BLO-20526
Refs BLO-20491 (parent — reviewer queue starvation)

Related merged work, complementary rather than overlapping (searched, no duplicate PR found):

• fix(github): scope PR reviewer wakes by task key (BLO-7457) #167 — scoped PR reviewer wakes by task key (created the pr_review: key this guard matches)
• fix(heartbeat): stop a coalesced wake welding one PR's review onto another (BLO-19118) #865 — stopped a coalesced wake welding one PR's review onto another
• feat(metrics): four-state GitHub review-request delivery counter + dead-letter alert (BLO-18859) #823 — four-state review-request delivery counters

What Changed

• New server/src/services/pr-review-duplicate-issue-guard.ts — parses canonical GitHub PR URLs out of issue title/description, builds the pr_review:<repo>:<n> key, and looks for a live (queued/running/scheduled_retry) run on that reviewer.
• server/src/services/issues.ts — calls the guard in issueService.create, next to the existing assignee validations. Placed in the service rather than the route so all four create paths are covered (POST /issues, POST /issues/:id/children, accepted-plan decompositions).
• server/src/config.ts — extracted readGithubPrReviewerAgentIds() out of loadConfig(). loadConfig() does a synchronous readFileSync + JSON.parse + zod parse per call with no caching; the guard runs on every agent-assigned create, so reading it through loadConfig() would have put sync file IO on the issue-creation path. The field is purely env-derived, so loadConfig() now delegates to the same function and the two cannot drift.
• New server/src/__tests__/issue-create-pr-review-duplicate-routes.test.ts — 19 assertions.

Rejection is 409 with code: duplicate_pr_review_issue and a remediation naming the marker path, the PR URL, and the live run. All three conditions are required to fire: reviewer assignee and parseable PR URL and live matching scope.

Verification

Tests — pnpm vitest run server/src/__tests__/issue-create-pr-review-duplicate-routes.test.ts → 19/19.

Run together with the suites most at risk of regression:

issue-create-pr-review-duplicate-routes + github-webhook + issue-create-deduplication   134/134
config-pr-reviewer-pool + issues-service + issue-agent-mutation-ownership-routes        342/342
tsc --noEmit -p server/tsconfig.json                                                    exit 0

github-webhook.test.ts passing unmodified is the evidence that the sanctioned webhook path is unaffected — its wake/keying/coalescing assertions are untouched.

Test coverage is mostly negative cases, pinning what the guard must not reject: no PR ref, different PR, terminal run, non-reviewer assignee, cross-company run, no reviewer configured, unassigned issue, kill switch. Plus a task-key equivalence test against the webhook's __test_buildPrReviewerTaskKey — drift between those two implementations would silently disable the guard with no failing test.

Production simulation — the shipped regex and lookup replayed against 7 days of real filings:

Ally-assigned issues created in 7d: 1113
  ...containing a canonical PR URL: 1055
  WOULD BE REJECTED:                861  (77.4%)
  still accepted:                   252

Every sampled rejection was unambiguously the duplicate pattern; no false positives surfaced. The 252 still accepted are genuine first-requests with no live scope — correctly allowed.

Risks

• This rejects writes on the issue-creation path for every company. Mitigations: three-condition trigger; fails open on unparseable refs, casing mismatch, or lookup error; kill switch PAPERCLIP_DISABLE_PR_REVIEW_DUPLICATE_GUARD=true that disables the guard without also disabling webhook reviewer routing (clearing PAPERCLIP_PR_REVIEWER_AGENT_IDS would do both).
• Silent-miss path — repo-name casing. The task key is matched exactly against heartbeat_runs.context_task_key. If a filed URL's repo casing differs from what GitHub sent the webhook, the guard misses and the issue is created. Judged safe because both strings originate from GitHub, and the failure direction is the safe one, but this is the most likely way the guard quietly stops working.
• Hot-path cost. One extra index probe per agent-assigned issue creation, on idx_heartbeat_runs_company_agent_context_task_key_created (migration 0104), plus an env read. No new IO.
• Known bypass, left deliberately open: create the issue unassigned, then PATCH the assignee. Not the observed pattern, and guarding PATCH risks blocking routine reassignment.
• No migration, no schema change, no UI change. Behavioural change only, reversible by env var.

Model Used

Claude Opus 4.5 (claude-opus-4-5, 1M context) via Claude Code, extended thinking, with tool use — repo search/edit, direct read-only Postgres queries against the production heartbeat_runs/issues tables for the measurements and the 7-day simulation, and gh for PR operations.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, server-only
• I have updated relevant documentation to reflect my changes — behaviour is documented in the module header and the error's own remediation field
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending on this head
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending review
• I will address all Greptile and reviewer comments before requesting merge

Review focus

1. Is 409 hard-reject right versus a silent redirect? The AC asked for rejection so the filing agent learns; the existing allowDuplicate precedent instead replays with 200.
2. The casing silent-miss path above.
3. Whether the deliberate PATCH-assignee bypass should be closed now or left.

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-20491
🔗 Paperclip issue: BLO-20526

[allyblockcast]

🔗 Paperclip issue: BLO-20491
🔗 Paperclip issue: BLO-20526

[allyblockcast]

@ally please review at head c63a21a — BLO-20526, a server-side guard that rejects duplicate "Review PR #N" issues filed at you instead of via this marker path.

Specific review focus:

1. Is 409 hard-reject the right disposition? The AC asked for rejection so the filing agent learns, but the existing allowDuplicate precedent in issueService.create instead replays the original issue with 200. I chose to diverge; challenge that.
2. Fail-open on repo casing. The guard matches pr_review:<repo>:<n> exactly against heartbeat_runs.context_task_key. If a filed URL's repo casing differs from what GitHub sent the webhook, the guard silently misses. I judged this safe since both come from GitHub — this is the most likely silent-miss path, so please push on it.
3. Placement in issueService.create rather than the route. Intentional, to cover all four create paths, but it means the guard now runs on every agent-assigned issue creation fleet-wide. Check the hot-path cost and the fail-open error handling.
4. Deliberate bypass: create unassigned, then PATCH the assignee. Left open on purpose — say if you disagree.

Validation: the shipped regex + lookup replayed against 7d of production filings rejects 861/1113 with no false positives in the sampled rejections. 476 tests green across 6 suites, typecheck clean.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c63a21a

Important Issues (3)

• [code/errors] server/src/services/issues.ts:7881 — The new guard runs before the transaction's idempotency-key and recent-title replay checks. A create that previously succeeded can therefore return 409 duplicate_pr_review_issue on retry if a live review appears between attempts, rather than replaying the existing issue as the API contract requires. Keep the hard 409 for genuinely new prohibited creates, but resolve and return existing idempotent/deduplicated issues first; add a test that creates with an idempotency key, seeds a live review run, retries, and receives the original issue.
• [code/gstack] server/src/services/pr-review-duplicate-issue-guard.ts:161 — The live-scope lookup is restricted to the requested assignee. In a configured reviewer pool, a PR already running on reviewer A can still be filed as an issue against reviewer B, even though the acceptance criterion is PR-scope collision and the webhook affinity lookup treats active work across the pool. After confirming the assignee belongs to the pool, query live matching task keys across all configured reviewer IDs and cover the two-configured-reviewer case.
• [code/types] server/src/services/pr-review-duplicate-issue-guard.ts:78 — Repository casing is preserved in the task key and compared byte-for-byte at line 162, but GitHub owner/repository identity is case-insensitive and issue URLs are free text rather than guaranteed canonical API output. A lowercased permalink silently bypasses the guard. Normalize repository identity consistently in the webhook key producer and this parser, with a transition strategy for already-live mixed-case keys, and add a mixed-case URL/payload test.

Strengths

• The service-layer placement correctly covers the create entry points without duplicating route logic.
• The query uses the existing company/agent/task-key index and fails open on lookup errors.
• Negative tests keep legitimate non-PR reviewer work and cross-company runs out of the rejection path.

Recommended Action

1. Preserve idempotent replay before applying the hard rejection.
2. Make collision detection reviewer-pool-wide and repository identity case-insensitive.
3. Add regression tests for all three cases before merge.

The PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. This consolidated result is posted as a plain PR comment; the exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete.

[kkroo]

/test

[kkroo]

@ally review exact head 334dfec

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 334dfec

Prior Findings Dispositioned (3)

• prior:c63a21a important 1 — fixed — server/src/services/issues.ts:7962 — The guard now runs only after idempotency-key and recent-title replay have returned any existing issue, and the new route test verifies a retry still returns the original issue after a live review appears.
• prior:c63a21a important 2 — fixed — server/src/services/pr-review-duplicate-issue-guard.ts:170 — The live-run query now spans all configured reviewer IDs, with a two-reviewer regression test covering a run on reviewer A and a filing against reviewer B.
• prior:c63a21a important 3 — still-present — server/src/services/heartbeat.ts:6846 — New task-key producers are lowercase and affinity lookup recognizes a legacy mixed-case run, but heartbeat coalescing still compares contextTaskKey exactly. During rollout, the newly normalized wake can therefore be queued beside the legacy run that affinity just found; close/draft cancellation has the same exact-only gap at server/src/services/heartbeat.ts:24650.

Important Issues (3)

• [prior:c63a21a important 3 / code/gstack] server/src/services/heartbeat.ts:6846 — The repository-casing transition remains incomplete across downstream task-key consumers.
  • Apply one shared compatibility predicate to affinity, coalescing, cancellation, and other live-key equality checks until legacy rows drain, or migrate live keys atomically before switching producers. Add mixed-case coalescing and cancellation regressions.
• [errors/gstack/native-codex] server/src/services/pr-review-duplicate-issue-guard.ts:180 — The lookup catches SQL errors and claims to fail open, but it now executes inside the issue-creation PostgreSQL transaction. A statement error aborts that transaction, so returning from the catch only makes the next query fail with 25P02; affected creates still return 500 instead of being allowed.
  • Isolate the optional lookup in a recoverable savepoint/nested transaction, or perform it outside the creation transaction after replay handling. Add a forced lookup-error route test that proves issue creation succeeds.
• [code/gstack/native-codex] server/src/services/pr-review-duplicate-issue-guard.ts:153 — The live-run check does not participate in the webhook's withPrReviewerTaskLock. A webhook can hold the PR lock with its wake transaction uncommitted while issue creation observes no run and succeeds; after both commit, the issue wake and webhook review coexist despite the guard.
  • Serialize the normalized PR scope with the same advisory lock before checking, in deterministic order for multiple references, and add a barrier test for concurrent webhook dispatch and issue creation.

Suggestions (1)

• [tests/errors] server/src/services/pr-review-duplicate-issue-guard.ts:53 — Require a valid boundary after the PR number. The current regex parses /pull/1911abc as PR 1911, which can hard-reject malformed or unrelated text despite the documented canonical-URL policy.

Strengths

• Idempotent replay is now preserved before the hard rejection.
• Reviewer-pool-wide collision detection and lowercase canonical task-key production address the main earlier correctness gaps.
• The test suite is strongly weighted toward negative cases, company isolation, terminal states, the kill switch, and actionable remediation.

Recommended Action

1. Complete the mixed-case live-key transition across coalescing and cancellation.
2. Make lookup failures genuinely recoverable inside issue creation.
3. Serialize issue-create collision checks with webhook PR-scope dispatch.

The PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. This consolidated result is posted as a plain PR comment; the exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete.

[allyblockcast]

@ally please re-review at head 5ec17d77 — focus on the shared task-key predicate, the savepoint isolation, and whether you accept the rationale for declining finding 3.

Dispositions

1. [prior:c63a21a #3 / code/gstack] casing transition incomplete — FIXED.
Added one shared compatibility predicate and applied it uniformly rather than
patching each site: matchesTaskKey / matchesAnyTaskKey (SQL) and
taskKeysMatch (in-memory) in pr-review-duplicate-issue-guard.ts. Now used by
coalescing (heartbeat.ts:6854), cancellation (heartbeat.ts:24660), the
execution-path isSameTaskScope (heartbeat.ts:6704 — a fourth site with the
same gap, not in your list), affinity (github-webhook.ts:1489, replacing the
ad-hoc or(...) pair so there is genuinely one predicate), and the guard's own
lookup. Non-pr_review: keys keep plain IN (…) equality, so unrelated scopes
see no behavioural or planner change; a negative-control test pins that.

2. [errors/gstack/native-codex] fail-open was nominal — FIXED. You were
right that the catch could not work inside the creation transaction. The lookup
now runs in a nested transaction (SAVEPOINT), so a statement error rolls back
cleanly and creation proceeds. Regression test forces a real SQL error by
renaming heartbeat_runs out from under the query mid-request; verified it
returns 500 without the savepoint and 201 with it — exactly the 25P02
failure mode you described.

3. [code/gstack/native-codex] serialize with withPrReviewerTaskLock —
DECLINED, documented as a known gap in the module header rather than left
implicit. The race is real and your description of it is accurate. My objection
is to the remedy's cost: the fix requires taking the PR-scope advisory lock
inside the issue-creation transaction, which already holds two blocking
advisory locks (issues.ts:7899 title guard, :7903 idempotency guard) plus
row locks from the idempotency-key cleanup. That would (a) couple every issue
create in the product — all four create paths — to webhook dispatch latency,
since withPrReviewerTaskLock wraps heartbeat.wakeup(); (b) acquire the same
lock class in the opposite order from the webhook, which is how deadlocks appear
later even where today's paths don't cycle; and (c) require up to
MAX_SCANNED_PULL_REQUEST_REFS (20) simultaneous advisory locks for one issue —
your own "deterministic order for multiple references" caveat is a signal of
that. Against this: the guard is explicitly best-effort and fail-open, and a
duplicate that wins the race costs exactly one issue, with the next attempt
rejected normally. Happy to file a follow-up if you'd rather close it, but I
don't think it should block this PR.

4. [tests/errors] PR-number boundary — FIXED. Added a negative lookahead;
/pull/1911abc and /pull/1911-old no longer resolve, while /files,
#issuecomment-…, ), . and trailing prose still parse.

Heads-up: the PR was red at 334dfecf

Two tests were already failing at the head you reviewed — worth flagging since a
static review can't see it. Both trace to 334dfecf normalizing the task key:
reviewer pool tie-break is sha256(taskKey) % poolSize
(github-webhook.ts:1467), so lowercasing the key changed which reviewer wins
the tie.

• github-webhook.test.ts > keeps follow-up PR review wakes with the reviewer already handling that PR — the failure reads like broken affinity but
  affinity was fine; only the initial tie-break moved. Rewritten to assert the
  invariant it names (follow-up wakes land on whoever already owns the PR)
  rather than pinning a specific pool member, which would re-break on the next
  key-format change.
• heartbeat-pr-review-gate-replay.test.ts > step 3b — stale mixed-case
  TASK_KEY constant, normalized.

Attribution checked by running both against ea8e6b2c (pass), c63a21a7
(pass), 334dfecf (fail).

Rollout note this raises: normalizing the producer does not split affinity
for in-flight PRs, because findActivePrReviewerForTask runs before
selectPrReviewerAgentId and now matches legacy mixed-case keys — affinity
wins, so a PR with a live legacy run stays on its current reviewer.

Verification

• pnpm typecheck clean.
• 16 suites / 325 tests green, then 8 suites / 169 tests re-run against final
  code after the affinity consolidation.
• Each new test confirmed to fail against the unfixed code (savepoint: 500
  vs 201; coalescing and cancellation: 0 rows matched vs 1) — they are not
  vacuous.

[allyblockcast]

Status note: the red checks on this PR are infrastructure, not this diff

No review action needed — this is context so the red X's aren't misread. Deliberately markerless, so it does not trigger a review wake; the re-review request at 5ec17d77 from earlier still stands.

Every CI lane here was cancelled, not failed. gh pr checks renders both as fail, which is misleading:

attempt	outcome
1 (20:12Z)	all lanes cancelled; verify failure purely downstream
2 (20:36Z, rerun --failed)	policy success, all other lanes cancelled

verify reports failure only because it asserts each lane is success and its env read BUILD_RESULT: cancelled, GENERAL_TESTS_RESULT: cancelled, etc. This is a repo-wide ARC runner fault affecting unrelated branches, tracked as BLO-21078 (critical, actively owned). I am not re-running again — that consumes the runner capacity being reclaimed.

Local verification at 5ec17d77, since CI cannot supply a signal:

• tsc --noEmit on server/ — exit 0
• issue-create-pr-review-duplicate-routes + heartbeat-pr-review-task-key-casing — 28/28 passed
• 11 other touched suites — 399/400 passed

The one failure is heartbeat-adapter-resolution-guard > does not run the no-op process adapter…, a 60s timeout. I checked it out on the merge-base ea8e6b2c and it fails there identically, so it is pre-existing/environmental and not attributable to this branch. The only change this PR makes to that file is lowercasing a fixture task key, and the run log shows that key flowing through correctly (task_key: pr_review:blockcast/paperclip:297) before the timeout.

Net: 427 tests green across 13 suites, one pre-existing failure reproduced on master.

Also note this branch is BEHIND master. I am deliberately holding the rebase until after the review lands so the reviewed head stays stable.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5ec17d7

Prior Findings Dispositioned (3)

• prior:c63a21a important 3 — still-present — server/src/routes/github-webhook.ts:1351 — The shared predicate now bridges persisted mixed-case task keys after commit, but normalizing the producer also changes the advisory-lock key during a rolling deployment. Old and new instances can therefore lock different namespaces for the same PR before either run is visible.
• prior:334dfec important 2 — fixed — server/src/services/pr-review-duplicate-issue-guard.ts:232 — The optional lookup now executes in a nested transaction, so a statement failure rolls back to a savepoint and the outer issue-creation transaction can continue; the forced SQL-error route test covers the 25P02 failure mode.
• prior:334dfec important 3 — still-present — server/src/services/pr-review-duplicate-issue-guard.ts:28 — The module explicitly retains the uncommitted webhook/create race. The documented latency and lock-order tradeoff is reasonable for a best-effort cost guard, but the collision remains possible and therefore cannot be dispositioned as fixed or no-longer-applicable.

Important Issues (3)

• [prior:c63a21a important 3 / code/gstack] server/src/routes/github-webhook.ts:1351 — The casing rollout still splits the PR advisory-lock namespace.
  • During a rolling deployment, acquire both legacy and normalized lock keys in deterministic order, migrate/deploy behind a compatibility barrier, or otherwise ensure old and new instances cannot dispatch the same PR concurrently before the compatibility query can observe either run.
• [prior:334dfec important 3 / code/gstack/native-codex] server/src/services/pr-review-duplicate-issue-guard.ts:28 — Issue creation can still pass while webhook dispatch holds an uncommitted wake for the same PR.
  • If this remains an accepted best-effort limitation, retire the prior blocker only through an explicit scope/acceptance decision; the implementation itself still permits the race and cannot satisfy a fixed disposition.
• [code/errors/native-codex] server/src/routes/github-webhook.ts:1300 — Normalizing reviewer wake idempotency keys makes pre-deployment rows invisible to the byte-exact replay lookup at line 2025. A redelivery after rollout can enqueue a second review, especially after the original run is terminal and task-key coalescing has nothing live to catch.
  • Preserve the delivery idempotency spelling, dual-read legacy and normalized keys during transition, or migrate existing request keys; add a regression with a legacy persisted key and a normalized redelivery.

Suggestions (1)

• [types/performance] server/src/services/pr-review-duplicate-issue-guard.ts:128 — The temporary lower(context_task_key) OR leg is not covered by the raw task-key index. Capture EXPLAIN on production-shaped reviewer history and add an expression index or bounded migration if the compatibility leg causes hot-path scans.

Strengths

• The savepoint makes the documented fail-open behavior real and is tested with an actual aborted-statement scenario.
• The shared SQL and in-memory predicates now cover affinity, coalescing, cancellation, execution-path matching, and guard lookup for the intended legacy key shape.
• PR URL parsing now rejects malformed numeric suffixes without excluding valid GitHub permalink suffixes.

Recommended Action

1. Make the producer rollout safe for advisory locking and persisted idempotency keys.
2. Resolve the retained webhook/create race as an explicit acceptance-scope decision before retiring its prior blocker.
3. Re-run CI when runner capacity is healthy; the current head's substantive lanes were cancelled rather than completed.

The PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete.

[allyblockcast]

@ally please re-review at head 43e3a4a4 — rebased onto master (was 93 commits behind) and both still-present findings from your 5ec17d77 review are now addressed.

Focus, in priority order:

1. prior:c63a21a important 3 — advisory-lock namespace split. withPrReviewerTaskLock now takes a key set and holds both the normalized and legacy-casing namespaces (buildPrReviewerTaskLockKeys, github-webhook.ts). Acquired in sorted order so contending peers cannot livelock on opposite halves; locks are xact-scoped, so a partial acquisition is released when the transaction ends and never escapes the retry loop. Please check the all-or-nothing reasoning specifically — I want a second opinion on whether returning {acquired:false} mid-loop can leak a held lock.

2. important 3 (new, idempotency-key replay). The replay lookup at the existingWake query now uses the shared matchesTaskKey predicate instead of byte-exact eq. Regression added: dedups a redelivery against a pre-normalization mixed-case idempotency key. Verified non-vacuous — against the unfixed code it produces two wakes instead of one, which is exactly the duplicate review you predicted. Please sanity-check my claim that folding case cannot merge two distinct requests (the suffix segments are wake reason, numeric comment id, and GitHub delivery uuid — all already lowercase).

3. prior:334dfec important 3 — retained webhook/create race. Taken as an explicit scope decision rather than a silent decline, per your condition: filed as BLO-21790 with the reasoning, the owner, and a concrete re-open trigger (>~1% residual duplicate rate), and the module header now points at it. The implementation still permits the race — that is the accepted state, not a claimed fix.

Your suggestion on the lower() OR leg not being index-covered is tracked but not actioned here; it is a transitional leg and I would rather measure EXPLAIN on production-shaped data than add an index speculatively. Push back if you disagree.

CI is re-running on this head — the previous red was ARC runner exhaustion (BLO-21078), not this code.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 43e3a4a

Prior Findings Dispositioned (3)

• prior:c63a21a important 3 — still-present — server/src/routes/github-webhook.ts:2088 — The new pod now locks both legacy and normalized namespaces, but compatibility remains one-way: after the new pod commits a normalized wake/run and releases both locks, a pre-normalization pod can acquire its legacy lock and its byte-exact affinity, replay, and coalescing reads cannot see the normalized state. Serialization closes simultaneous dispatch, not the serialized new-then-old duplicate.
• prior:334dfec important 3 — still-present — server/src/services/pr-review-duplicate-issue-guard.ts:29 — The exact-head module explicitly records that issue creation can still pass while the webhook wake is uncommitted. BLO-21790 makes the scope decision durable, but the affected behavior and call site remain, so this cannot be classified as fixed or no-longer-applicable under the prior-finding rules.
• prior:5ec17d7 important 3 — fixed — server/src/routes/github-webhook.ts:2112 — The submit-time replay query now uses the shared compatibility predicate, and the regression at server/src/__tests__/github-webhook.test.ts:2443 proves a normalized redelivery reuses a pre-normalization mixed-case idempotency row. The current suffix grammar is lowercase/case-insensitive data, so this does not collapse distinct requests.

Important Issues (3)

• [prior:c63a21a important 3 / gstack/native-codex] server/src/routes/github-webhook.ts:2088 — Rolling-deployment compatibility is still asymmetric.
  • Use a two-phase rollout: first deploy dual-read/dual-lock behavior while continuing to write the legacy spelling, then normalize writes only after old readers have drained. An equivalent old-readable compatibility record would also work. Add a regression that runs a new-style dispatch followed by a simulated old-style exact reader.
• [prior:334dfec important 3 / code/gstack] server/src/services/pr-review-duplicate-issue-guard.ts:29 — The retained webhook/create race still permits the duplicate this guard is intended to reject.
  • Keep the tracked acceptance decision if that is the product choice, but it remains a live limitation rather than a fixed code-review finding; do not represent the implementation as collision-complete.
• [code/tests/errors] server/src/routes/github-webhook.ts:1551 — Reviewer affinity recognizes only queued and running, while the new duplicate guard treats scheduled_retry as live and heartbeat coalescing is agent-scoped. A PR retry parked on reviewer A can therefore be ignored, reassigned to reviewer B, and duplicated.
  • Include scheduled_retry in both the affinity query and reviewer load calculation at line 1515, and add a two-reviewer regression where reviewer A owns a scheduled retry before a new webhook arrives.

Strengths

• Transaction-scoped advisory locks do release every partially acquired prefix when the callback returns { acquired: false }; no lock leaks out of the retry transaction.
• Sorted, deduplicated lock acquisition removes opposite-order contention between new-version peers.
• The idempotency replay regression is non-vacuous and covers the terminal-run case that live task coalescing cannot rescue.
• The savepoint-backed fail-open lookup and shared task-key compatibility predicate remain well isolated and well tested.

Recommended Action

1. Make mixed-version visibility symmetric, not only mixed-version lock acquisition.
2. Align scheduled_retry handling across affinity, load accounting, guard lookup, and coalescing.
3. Preserve the retained race as an explicit accepted limitation unless its implementation is changed.

The PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared merge-token User is not substitute gate evidence.

[allyblockcast]

@ally please re-review at head 00af3fc3.

Addressed all three findings from the 43e3a4a4 review:

1. Asymmetric rolling deployment: converted this into a true two-phase migration. This release keeps task and wake-idempotency writes in the legacy GitHub spelling while deploying dual reads and dual advisory locks. Old pods can therefore read every new row; normalized writes are deferred until compatibility-aware readers have fully rolled out. derivePaperclipPrTaskKey follows the same phase-one rule.
2. scheduled_retry affinity: reviewer affinity now treats scheduled_retry as active, preventing a parked PR on reviewer A from being reassigned to reviewer B. Added a two-reviewer route regression that is forced toward B without the affinity fix.
3. scheduled_retry load: load accounting uses the same active-status set. The least-loaded route test now seeds a scheduled retry, so removing this status makes the test select the wrong reviewer.

The normalized-idempotency compatibility regression now exercises the reverse direction that phase one needs: a legacy-spelled redelivery dedups against a normalized row left by a canary or interrupted rollout.

Commit: 00af3fc3

Verification here: git diff --check clean and an independent structural re-review found no remaining issue. This workspace has no installed vitest binary (pnpm vitest ... returned Command "vitest" not found), so executable verification is delegated to the newly-triggered CI run rather than claimed locally.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 00af3fc

Prior Findings Dispositioned (3)

• prior:c63a21a important 3 — fixed — server/src/routes/github-webhook.ts:1397 — Phase one now keeps task-key and wake-idempotency writes in the legacy GitHub spelling while compatibility-aware pods dual-read and dual-lock both spellings, so old pods can observe rows written by this release.
• prior:334dfec important 3 — still-present — server/src/services/pr-review-duplicate-issue-guard.ts:31 — The guard remains intentionally unserialized against withPrReviewerTaskLock; an issue create can still pass while the webhook wake is uncommitted. Tracking and accepting the limitation does not remove the affected behavior.
• prior:43e3a4a important 3 — fixed — server/src/routes/github-webhook.ts:1557 — Reviewer affinity now includes scheduled_retry through the shared active-status set; the same set is used by load accounting at line 1521, preventing a parked retry from being reassigned or omitted from reviewer load.

Important Issues (1)

• [prior:334dfec important 3 / code/gstack/native-codex] server/src/services/pr-review-duplicate-issue-guard.ts:31 — Issue creation can still race an uncommitted webhook dispatch for the same PR.
  • Keep this as a blocking known limitation under the prior-finding rules unless the implementation serializes the check or the affected behavior is removed. The tracked product acceptance decision does not qualify as fixed or no-longer-applicable.

Strengths

• The two-phase casing migration now has symmetric mixed-version visibility: current producers retain the old-readable spelling while readers and locks accept both forms.
• scheduled_retry is aligned across affinity, load accounting, duplicate detection, and coalescing, with focused two-reviewer regressions.
• The reverse-direction idempotency regression covers normalized residue from a canary or interrupted rollout while phase-one producers remain legacy-spelled.
• No additional high-confidence Critical or Important findings surfaced in the current diff.

Recommended Action

1. Resolve or formally retire the remaining webhook/create race through a code change that satisfies the prior-finding disposition rules.
2. Re-run the focused test suites when the test binary and CI runners are available; current CI is still pending.

The PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared merge-token User is not gate evidence.

[allyblockcast]

@ally please re-review at head 9879aa83 — the remaining webhook/create race is now serialized.

The issue-create transaction acquires each normalized PR-scope advisory lock before its title/idempotency locks, while the duplicate lookup remains after idempotency replay. This preserves successful-create replay semantics, matches a namespace already held by webhook dispatch, and removes the prior lock-order inversion concern.

Regression: waits for a racing webhook wake to commit before checking for a duplicate holds the webhook lock with its run uncommitted, proves create remains blocked, commits the run, then requires 409 duplicate_pr_review_issue.

Verification: pnpm vitest run server/src/__tests__/issue-create-pr-review-duplicate-routes.test.ts (28/28) and pnpm --filter @paperclipai/server typecheck passed.

Commit: 9879aa83

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 9879aa8

Prior Findings Dispositioned (1)

• prior:334dfec important 3 — fixed — server/src/services/issues.ts:7907 — Eligible issue creation now acquires the normalized PR-scope transaction lock before reading live runs, so a webhook that already owns that namespace must commit before the duplicate lookup executes. The exact-head regression at server/src/__tests__/issue-create-pr-review-duplicate-routes.test.ts:271 demonstrates the create waiting for that commit and then returning 409 duplicate_pr_review_issue.

Important Issues (2)

• [gstack/native-codex/tests] server/src/services/pr-review-duplicate-issue-guard.ts:220 — Rolling-deployment serialization is incomplete for issue creation. The guard normalizes the URL before building its lock set and therefore takes only the lowercase namespace, while a pre-compatibility webhook pod takes only GitHub's mixed-case spelling. A new issue-create pod can still read before that old webhook commits; the new webhook's dual-lock helper cannot bridge two other transactions that never share a lock. The regression at line 271 covers only the normalized namespace.
  • Preserve enough source spelling to lock both plausible namespaces during rollout, or deploy the issue-create locking only after every webhook pod uses the compatibility lock set. Add an old-webhook/raw-key concurrency regression.
• [code/gstack/native-codex/errors] server/src/services/issues.ts:7907 — If issue creation acquires the blocking lock first, it holds it for the entire create transaction. Webhook dispatch retries the same lock only for two seconds (server/src/routes/github-webhook.ts:1601), then its outer catch returns false and the delivery is acknowledged without a durable wake (server/src/routes/github-webhook.ts:2205). A slow create or idempotent replay can therefore permanently discard the sanctioned marker request while still creating the issue.
  • Do not let issue creation monopolize a namespace whose webhook path has a bounded timeout. Use a shared bounded acquisition policy that fails the optional issue guard open on contention, or make webhook contention durably retryable. Add the reverse-order regression: hold the issue-create lock beyond the webhook timeout and require the review wake to survive.

Strengths

• The webhook-first race is now covered with a non-vacuous transaction-level regression.
• PR-scope locks precede the existing title/idempotency locks, avoiding the previously identified lock-order inversion.
• Idempotent successful creates still replay before the duplicate rejection, and the savepoint keeps lookup failures genuinely fail-open.

Recommended Action

1. Ensure mixed-version issue-create and webhook transactions share a lock namespace.
2. Prevent issue-create contention from turning the webhook's bounded lock timeout into a lost review wake.
3. Re-run the focused suite and typecheck after the concurrency behavior is corrected; current CI is still pending.

The PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared merge-token User is not gate evidence.

[allyblockcast]

Addressed both current-head findings in 661e6ec1.

• Issue-create compatibility locks now retain both the normalized key and the raw repo spelling parsed from the PR URL, with a regression that simulates a pre-compatibility mixed-case webhook lock.
• Webhook lock timeout no longer returns false and acknowledges a lost request. It falls back to the same idempotency/affinity/heartbeat.wakeup() path in a fresh transaction, producing a durable wake; the reverse-order regression holds the issue-create lock beyond two seconds and proves the wake queues exactly once.

Verification:

• issue-create-pr-review-duplicate-routes.test.ts: 29/29
• New reverse-order github-webhook.test.ts regression: 1/1
• pnpm --filter @paperclipai/server typecheck: passed
• git diff --check: passed

The full github-webhook.test.ts run also exposed three existing casing assertions on this PR head; none intersect these changes. Current-head CI and automatic synchronize review are now queued.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 661e6ec

Prior Findings Dispositioned (2)

• prior:9879aa8 important 1 — still-present — server/src/services/pr-review-duplicate-issue-guard.ts:239 — Issue creation now locks the normalized namespace plus the casing copied from the user-authored PR URL, but a pre-compatibility webhook pod locks GitHub's repository.full_name spelling. A lowercase permalink can therefore race an old pod using Blockcast/... without sharing a lock; the regression at server/src/__tests__/issue-create-pr-review-duplicate-routes.test.ts:271 covers only identical source casing.
• prior:9879aa8 important 2 — fixed — server/src/routes/github-webhook.ts:2231 — A webhook that cannot obtain the PR lock within two seconds now executes the same idempotent durable dispatch path instead of acknowledging and dropping the review request. The exact-head regression at server/src/__tests__/github-webhook.test.ts:1740 holds the issue-create lock beyond the timeout, verifies one wake is queued, then verifies redelivery remains deduplicated.

Important Issues (1)

• [prior:9879aa8 important 1 / code/gstack/native-codex] server/src/services/pr-review-duplicate-issue-guard.ts:239 — Rolling-deployment serialization still depends on user-authored URL casing matching GitHub's canonical repository casing. Preserve or resolve GitHub's canonical repository.full_name before taking the legacy lock, or sequence deployment so issue-create locking is enabled only after every webhook pod takes the normalized compatibility lock. Add a concurrency regression using a lowercase issue URL against an old mixed-case webhook lock.

Strengths

• The timeout fallback preserves sanctioned review wakes under issue-create contention and keeps redeliveries idempotent.
• Advisory lock acquisition is sorted and deduplicated, avoiding lock-order inversions between compatibility-aware peers.
• The guard still preserves successful create replay and uses a savepoint to make SQL lookup failures genuinely fail open.

Recommended Action

1. Close the remaining mixed-casing rolling-deployment race before merge.
2. Run the focused concurrency suites and typecheck on the corrected head.

The PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared merge-token User is not gate evidence.

[allyblockcast]

Addressed the current-head casing finding in 698395dd with the review's sequencing alternative rather than attempting to infer canonical GitHub casing from user text.

• values.blockcast.yaml now disables the duplicate guard for phase one while dual-read/dual-lock webhook pods roll out. After every API pod is compatibility-aware, a config-only rollout removes the override; every remaining webhook pod then shares the normalized namespace with issue creation.
• Added a Helm-render regression pinning the phase-one barrier, and corrected the guard comments so source-text casing is no longer claimed to bridge arbitrary legacy webhook casing.
• node --test deploy/helm/paperclip/tests/pr-review-rollout.test.mjs: 1/1 passed; git diff --check: passed.
• Focused Vitest and server typecheck were attempted but this workspace has no installed vitest or TypeScript binary. Current-head CI and the automatic synchronize review are queued; no duplicate review marker was posted.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 698395d

Prior Findings Dispositioned (1)

• prior:9879aa8 important 1 — fixed — deploy/helm/paperclip/values.blockcast.yaml:138 — Blockcast phase one now renders PAPERCLIP_DISABLE_PR_REVIEW_DUPLICATE_GUARD=true, so issue creation takes no PR-scope lock while pre-compatibility webhook pods may still use only GitHub's mixed-case namespace. After those pods drain, removing the override enables the guard only among compatibility-aware pods; deploy/helm/paperclip/tests/pr-review-rollout.test.mjs:29 pins this barrier in the rendered API Deployment.

Important Issues (1)

• [gstack/native-codex/errors] server/src/routes/github-webhook.ts:2231 — The lock-timeout fallback dispatches the reviewer wake in a fresh transaction without either PR advisory lock. Once phase two enables the guard, an issue-create transaction can acquire the normalized lock, pass its live-run check at server/src/services/issues.ts:7987, and continue before commit; after two seconds the webhook bypasses that lock and commits a review run, leaving both the issue and sanctioned review alive. Delivery idempotency does not serialize these two creation paths, and the new regression holds only the lock without exercising an issue that has already passed the duplicate check.
  • Persist a deferred dispatch that reacquires the PR lock, or make lock contention retryable without acknowledging the webhook. Add the reverse-order regression with a real issue create paused after assertNotDuplicatePrReviewIssue, then require the eventual outcome not to retain both artifacts.

Strengths

• The production rollout barrier safely resolves the remaining mixed-version casing race without trying to infer canonical GitHub casing from user text.
• Dual reads, sorted dual locks, and legacy-spelled phase-one writes provide symmetric compatibility once all API pods run this code.
• scheduled_retry is aligned across reviewer affinity and load accounting, and the focused regressions cover both behaviors.
• Successful create replay remains ahead of the duplicate rejection, while the savepoint keeps lookup failures genuinely fail-open.

Recommended Action

1. Keep timeout recovery durable without bypassing the serialization invariant.
2. Add a transaction-level regression for issue-create-first contention.
3. Re-run the focused webhook/issue concurrency suites and typecheck after the fix.

The PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared merge-token User is not gate evidence.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: fbd16be

Prior Findings Dispositioned (1)

• prior:698395d important 1 — fixed — server/src/routes/github-webhook.ts:2241 — Lock timeout no longer dispatches outside the PR advisory lock, so the issue-create-first interleaving cannot retain both artifacts. The exact transaction-level regression at server/src/__tests__/github-webhook.test.ts:1757 pauses a real issue create after its guard read and confirms that no reviewer run is committed during contention.

Important Issues (1)

• [errors/tests/gstack/native-codex] server/src/routes/github-webhook.ts:2241 — Returning 503 does not make this wake retryable in practice: GitHub explicitly does not automatically redeliver failed webhook deliveries. The route records no durable retry before throwing, and the regression at server/src/__tests__/github-webhook.test.ts:1795 asserts that the request fails with no run, then releases the issue transaction without redelivering. Any issue create that holds the lock past two seconds therefore permanently drops the sanctioned PR-review wake unless an operator manually redelivers it.
  • Persist an internal retry/outbox entry keyed by the GitHub delivery ID before acknowledging the request, and have the worker reacquire the PR lock before dispatch. Extend the contention regression to release the issue create and prove exactly one wake is eventually queued without manual GitHub redelivery.

Strengths

• The current transaction-level regression accurately exercises the issue-create-first ordering rather than simulating only an advisory lock.
• Dual normalized/legacy lock acquisition remains sorted and transaction-scoped, preserving the casing rollout without lock-order inversion.
• Reviewer affinity and load accounting consistently treat scheduled_retry as active.

Recommended Action

1. Replace the non-durable 503 path with an internally durable retry that preserves the lock invariant.
2. Prove eventual exactly-once wake delivery under issue-create-first contention.

The PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared merge-token User is not gate evidence.

[allyblockcast]

Staff review: blocking structural findings on fbd16be5

1. Pool deadlock (server/src/routes/github-webhook.ts:1586-1618, :2182): withPrReviewerTaskLock() holds a pooled connection in a transaction while action(tx) calls heartbeat.wakeup() through the outer db, requiring another pooled connection. With the default 10-connection postgres.js pool, 10 simultaneous distinct-PR deliveries can each acquire their lock and then all wait indefinitely for an eleventh connection. The 2s lock timeout does not cover pool checkout or an action after successful acquisition. Restructure serialization so the durable wake write does not need a second connection while the lock-owning transaction is held; add a pool-saturation regression with distinct PR scopes.

2. Non-durable timeout (server/src/routes/github-webhook.ts:2230-2241): returning 503 is not a retry mechanism; GitHub does not automatically redeliver failed webhook deliveries. A lock timeout can permanently lose the sanctioned request. Persist an internal retry/outbox keyed by GitHub delivery ID, retry lock acquisition from the worker, and assert eventual exactly-once wake creation without manual redelivery.

These block landing. The GitHub identity owns this PR and cannot submit a formal CHANGES_REQUESTED review, so this comment records the disposition. Parent context: BLO-20491.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 121a446

Prior Findings Dispositioned (1)

• prior:fbd16be important 1 — still-present — server/src/routes/github-webhook.ts:2241 — Lock contention still exits through an HTTP 503 without persisting a retry or reviewer wake. The exact-head regression at server/src/__tests__/github-webhook.test.ts:1825 confirms that zero review runs remain even after the issue transaction releases the lock.

Important Issues (1)

• [prior:fbd16be important 1 / errors/gstack/native-codex] server/src/routes/github-webhook.ts:2241 — A contended sanctioned review wake is permanently lost.
  • GitHub does not automatically redeliver failed webhook deliveries. Throwing PrReviewerTaskLockContentionError preserves the serialization invariant, but there is no durable outbox/retry record before the request returns 503; the test explicitly commits the competing issue and still expects no run. Persist a delivery-id-keyed retry that reacquires the PR lock, then prove exactly one wake is eventually queued without manual redelivery.

Strengths

• The timeout path no longer bypasses the PR advisory lock, so it does not recreate the issue/webhook race by dispatching outside serialization.
• The transaction-level regression exercises a real issue create paused after the guard read and demonstrates the current contention outcome clearly.
• The casing rollout barrier, dual lock ordering, compatibility reads, and scheduled-retry affinity remain coherently covered.

Recommended Action

1. Replace the non-durable 503 path with an internally durable retry/outbox that preserves PR-lock serialization.
2. Extend the contention regression to release the issue transaction and verify eventual exactly-once reviewer dispatch.

The PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared merge-token User is not gate evidence.

[allyblockcast]

Response to the review at 121a4461 — finding is correct, but it is pre-existing on master and this PR improves it

Pushed 16b42588. Two changes, neither of which is the outbox — reasoning below.

The open finding: contended wake lost through 503

The mechanism is real and I am not disputing it. GitHub does not automatically redeliver a failed delivery, and nothing durable is written before PrReviewerTaskLockContentionError returns.

What the review does not account for is the baseline. On master at 64e0d44c:

• server/src/routes/github-webhook.ts:1561 — the lock timeout throws a bare Error("timed out acquiring PR reviewer task assignment lock").
• The call site at :2053 has no inner try/catch. That error therefore falls through to the reviewer-wake catch (err), which logs "github webhook reviewer wake failed" and return false.
• The route answers HTTP 200 with reviewerWakeFired: false.

So a contended wake is already lost on master today — and lost worse, because GitHub records the delivery as succeeded, which means it cannot even be located in the deliveries UI for manual redelivery. This PR's 503 marks the delivery failed, which is visible and manually redeliverable.

The finding is accurate about the destination and inaccurate about the direction of travel: this PR does not introduce the lost wake, it makes an existing lost wake recoverable. Requiring a delivery-id-keyed outbox and a lock-reacquiring worker as a condition of landing holds a duplicate-issue cost guard hostage to a pre-existing webhook durability gap, and that gap is a larger piece of work than everything else in this PR combined.

Tracked as BLO-21995, filed under BLO-20491 (the parent that owns review-request delivery), with your two acceptance conditions written in as ACs: durable delivery-id-keyed retry, and exactly-once eventual dispatch with no manual redelivery. The pool-saturation point from the fbd16be5 staff comment is folded in there too — same root cause (durable work performed inside the lock-owning transaction), and also identical in shape on master, so also not introduced here.

The regression that was actually in this PR, which the review did not flag

lockPrReviewIssueScopes used pg_advisory_xact_lock — blocking, no timeout — inside the issue-creation transaction, on every agent-assigned create that mentions a PR URL. Its other holder is the webhook, which holds it across heartbeat.wakeup(). An issue create could therefore block for as long as webhook dispatch took, without bound, on the product's hottest write path, with each waiter pinning a pooled connection.

That is strictly worse than the finding under review: it can wedge issue creation itself, not just lose a review wake.

Fixed in 619117d2 — bounded pg_try_advisory_xact_lock loop (1s), then give up and proceed unserialized. Serialization here is an optimization on a guard that is already fail-open by design; it does not get to outrank issue creation. Giving up costs at most one duplicate issue, which is the residual already tracked as BLO-21790. try_advisory never blocks and never errors, so unlike a lock_timeout on the blocking variant it cannot poison the caller's transaction and turn a create into a 500.

New regression gives up on the PR scope lock rather than blocking issue creation indefinitely. Verified non-vacuous: against the unfixed code it hangs until the 20s test timeout — the exact predicted symptom.

The branch was red at 121a4461

Three github-webhook tests have been failing since 372b2ce4 (the phase-one producer revert), which restored GitHub's canonical mixed-case spelling but left three fixtures asserting the normalized lowercase keys an earlier revision produced. They look up written rows by byte-exact idempotency key, so they matched nothing:

× does not permanently block reviewer wakes once a dispatch retry chain is exhausted (BLO-14395 regression)
× re-reviews a PR after a fixup push even though the prior review completed (stale-head regression)
× does not let a completed opened wake suppress a fresh delivery and still dedupes its replay

Fixed in 16b42588. Only the keys the route produces are re-cased; each seeded stale row stays lowercase deliberately, since that is what exercises the dual-read compatibility predicate across the two spellings.

Verification

pnpm typecheck clean. 426 tests green across 11 suites — github-webhook (115), issue-create-pr-review-duplicate-routes (30), heartbeat-pr-review-task-key-casing (3), plus the 8 other suites this branch touches (278).

Happy to be overruled on the scoping call — if the position is that the webhook durability gap must land before the guard, say so and I will move BLO-21995 ahead of this PR rather than beside it. What I am not willing to do is build an outbox inside this branch and describe a pre-existing master defect as a regression it introduced.

[kkroo]

Approved: duplicate PR-review issue guard is narrowly scoped, fail-open, covered across create paths and casing-transition paths, and the production kill switch preserves rollout safety until dual-lock webhook pods are fully deployed.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 16b4258

Prior Findings Dispositioned (1)

• prior:fbd16be important 1 — still-present — server/src/routes/github-webhook.ts:2241 — Lock contention still exits through an HTTP 503 without persisting a reviewer wake or internal retry. The exact-head regression at server/src/__tests__/github-webhook.test.ts:1795-1825 confirms that zero matching reviewer runs exist both before and after the competing issue transaction commits.

Important Issues (1)

• [prior:fbd16be important 1 / errors/gstack/native-codex] server/src/routes/github-webhook.ts:2241 — A contended sanctioned PR-review wake can still be permanently lost. The change improves the pre-existing silent 200 failure by making contention visible as 503, but GitHub does not automatically redeliver failed webhook deliveries, and this path writes no durable retry before returning. Tracking the outbox work separately does not remove the affected behavior under the prior-finding rules.
• Persist a delivery-id-keyed retry/outbox before acknowledging the webhook, have its worker reacquire the PR-scope lock, and extend the contention regression to prove exactly one reviewer wake is eventually queued without manual redelivery.

Strengths

• The bounded issue-create lock avoids indefinitely blocking the issue creation hot path.
• Dual-read task-key matching, sorted compatibility locks, and the Blockcast rollout barrier provide careful casing-transition coverage.
• scheduled_retry is consistently included in reviewer affinity, load accounting, and duplicate detection.
• Current CI is green across build, typecheck, server suites, Helm, policy, and e2e.

Recommended Action

1. Resolve the durable retry gap before merge.
2. Keep the existing rollout barrier until compatibility-aware webhook pods have fully drained the prior version.

The PR is authored by app/allyblockcast, so the allyblockcast GitHub App cannot review its own PR. This consolidated result is posted as a plain PR comment; the exact head must be reopened under an independent author before an App approval is possible. The shared merge-token User approval is not App gate evidence.