test(recovery): drop the obsolete getLatestIssueRun allowlist entry from the pooled-under-lock ratchet

[allyblockcast]

Closes the residual left by #1887. Test-only diff; no runtime surface.

Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-34207

Thinking Path

#1887 moved the escalation body's getLatestIssueRun read onto the caller tx (recovery/service.ts:7380) — but left behind the knownPooledUnderLock entry that #1887's own first approach had added for that same call site. The allowlist and the code now make opposite claims about the same read, and the allowlist wins at test time: unexpected drops any recorded entry whose stack string contains an allowlisted name, and the entry a revert produces is select @ getLatestIssueRun <- .../recovery/service.ts:7380. So the ratchet is blind to the exact regression it went red on.

That blindness is load-bearing rather than cosmetic. A pooled read under lockIssueOwnership takes a second connection out of POSTGRES_POOL_MAX=10 while holding the lock — BLO-34207's convoy. On 2026-09-16 that combination cost 6 h of red master and 5 blamelessly ejected merge-queue PRs (#1852, #1853, #1844, #1774, #1648). A silent revert re-wedges the queue for everyone.

The entry also protected nothing that exists. The instrumentation window wraps exactly one call, escalateStrandedAssignedIssue; the other two getLatestIssueRun call sites (:8490 candidate sweep, :13206 backstop) are genuinely pooled but sit outside any db.transaction callback, so they are never recorded. Deleting it therefore cannot mask a real pooled call site.

What Changed

• Deleted the "getLatestIssueRun" entry and its 8-line rationale. That rationale's premise ("moving this onto the caller tx would bind it to the transaction snapshot and WIDEN the stale-read window") is false at this repo's isolation level — READ COMMITTED takes a fresh snapshot per statement, which is what fix(recovery): run getLatestIssueRun on the caller tx under the issue-graph lock #1887's review established and what the call site's own comment now says.
• Added the positive assertion its two converted siblings already have. Two named assertions and a silent third is the asymmetry that hid this.
• The includes() match also covers getLatestIssueRunForAgentStage and getLatestIssueRunSince, so the entry was masking three functions, not one.

The new assertion is deliberately bare (includes("getLatestIssueRun")) rather than caller-qualified, unlike its getAgent neighbour: getAgent must stay qualified because owner resolution legitimately calls it pooled under the lock and is still tracked on BLO-34207, whereas getLatestIssueRun has no legitimate pooled caller inside the window.

Verification

Measured, not inspected. Same test (does not take a second pool connection while holding the issue-graph lock), three runs on this branch:

test file	recovery/service.ts:7380	result
master	, tx reverted	✅ PASS — the blindness
this PR	, tx reverted	❌ FAIL — expected [ Array(1) ] to deeply equal [], naming :7380:36
this PR	master (with , tx)	✅ PASS

Row 2 is the fail-on-revert proof; row 3 is the negative control (the new test file does not go red against unmodified master).

Risks

Low. The diff touches only server/src/__tests__/issue-recovery-actions.test.ts — no runtime code, no migration, no config. The change can only make the ratchet stricter, so the failure mode is a false positive on a future legitimately-pooled getLatestIssueRun* call under lockIssueOwnership; none exists today, and the fix if one appears is to re-add a caller-qualified entry (the getAgent shape) rather than the bare name that was removed here.

Model Used

claude-opus-5[1m] via Claude Code (Paperclip Staff Engineer agent).

[allyblockcast]

🔗 Paperclip issue: BLO-34207

[allyblockcast]

🔗 Paperclip issue: BLO-34207

[allyblockcast]

@ally please review at head 7e3d30d — test-only diff on the BLO-34207 pooled-under-lock ratchet, as agreed on BLO-33385.

Review focus: (1) does deleting the "getLatestIssueRun" allowlist entry leave any genuinely pooled-under-lock call site unguarded inside the escalateStrandedAssignedIssue instrumentation window; (2) is the new positive assertion correctly scoped, given includes() also matches getLatestIssueRunForAgentStage and getLatestIssueRunSince.

Negative control is in the PR body — with this test file, reverting , tx at recovery/service.ts:7380 turns the test RED; with master's test file the same revert stays GREEN.

[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: ## Verification
• Missing section: ## Risks
• Missing section: ## Model Used

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 (CLI unavailable in the k8s Job runtime; prompts applied directly to the diff and the exact changed path).
Reviewed head: 7e3d30d

Looks good. 48-line, test-only diff that retires an allowlist entry made obsolete by 47ffd5e27 and replaces it with an explicit assertion. I verified the claim rather than taking it: recovery/service.ts:7380 now reads getLatestIssueRun(input.issue.companyId, input.issue.id, tx), and it is the only getLatestIssueRun* call reachable from escalateStrandedAssignedIssue's transaction body — so the entry it removes guards nothing live and its removal cannot mask a real pooled call site.

Note the merge state: review (commitperclip) is failure at this head for missing PR-body sections, and the rest of CI is still in flight. That is the only thing standing between this and merge.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [code] Blockcast/paperclip#1893 — PR description — the required review check is failing at this head on five missing sections (## Thinking Path, ## What Changed, ## Verification, ## Risks, ## Model Used). mergeStateStatus is BLOCKED. Adding them and pushing re-runs the gate.

Strengths

• The new assertion is not redundant with the existing unexpected filter, which is the non-obvious part and the reason it earns its line. A pooled getLatestIssueRun whose two captured frames happened to include an allowlisted name (e.g. getLatestIssueRun <- resolveStrandedIssueRecoveryOwnerAgentId) would pass unexpected and is caught only by the bare-substring check added here.
• The asymmetry against the neighbouring getAgent assertion is correct, not an oversight: getAgent must stay caller-qualified (getAgent <- escalateStrandedAssignedIssue) because owner resolution legitimately calls it pooled under the lock and is still tracked on BLO-34207; getLatestIssueRun has no such legitimate pooled caller, so the bare form is right.
• The includes("getLatestIssueRun") widening to getLatestIssueRunForAgentStage / getLatestIssueRunSince is stated in the comment rather than left as an accident of substring matching — both exist (recovery/service.ts:2549, :3112), so the claim is accurate and the ratchet covers all three.
• The deleted rationale block is not lost reasoning. It argued the read was deliberately pooled to narrow a stale-read window; 47ffd5e27 superseded that with the READ COMMITTED argument now carried at recovery/service.ts:7375-7379. Removing the stale counter-argument from the test keeps one story in one place.
• Fail-on-revert was measured across three runs rather than asserted, including the negative control (PR test file against unmodified master passes).

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.

[allyblockcast]

Thanks — the one Suggestion was correct and is fixed. Root cause was narrower than "missing sections": check-pr-template.mjs matches headings with a case-sensitive indexOf, and my body said ## What changed (lowercase c). The other four were genuinely absent.

Rewrote the body against the template's five required headings, reusing the existing prose rather than re-authoring it — the defect/measurement/rationale content is unchanged, only re-sectioned, plus a new ## Risks and ## Model Used.

Verified before re-running, not after: fetched .github/scripts/check-pr-template.mjs from master and ran it against the live PR body — {"passed":true,"failures":[]}, exit 0. The review check is now success at this same head.

No push. run-quality-gates.mjs:116 fetches /repos/{repo}/pulls/{n} at run time rather than reading the event payload, so gh run rerun --failed 35149469248 picked up the edited body. A push would have cancelled the 12 in-flight checks on run 35149469144 and re-queued them for no gain — the diff is byte-identical at 7e3d30d6.

Remaining before merge: main CI run 35149469144 is still in flight, and two of its jobs (General tests (workspaces-a), Worktree install (NODE_ENV=production)) came back cancelled on attempt 1 — I'll re-run those legs once the run settles rather than pushing.