fix(recovery): count an open pull request as an attendance path (PEN-2791)

[allyblockcast]

Thinking Path

• Started from the report, not the diagnosis. PEN-2791's filer inferred that the strandedness predicate omits external event wakes, and explicitly flagged that they had not read its source. So the first job was to confirm or refute that against the code rather than build on the inference.

• Read the predicate and enumerated what it actually counts. Five attendance paths — live run, deferred execution wake, pending wake interaction, active monitor, unresolved blocks blocker. recovery/service.ts contains exactly one occurrence of the string github, and it is inside a comment. The inference was correct.

• Found the sharper form: the two controls are not merely unreconciled, one is the other's trigger. hasActiveMonitorPath goes false the instant monitorNextCheckAt is nulled, and nulling it is precisely the convergence guard's job when a gate will not move. On a row with no blockers, that column is the only durable path. So the guard behaving correctly is what makes the row seizable — an assignee reasoning correctly about when not to poll is the assignee most likely to lose its issue.

• Then looked for what data already exists, before proposing a new one. issue_pull_requests is merged-only; pull_request_review_state covers open PRs but has no issue_id. Neither answers "is this issue waiting on an open PR". issue_work_products does — the webhook has persisted every PR it wakes on since BLO-19566, keyed to the issue and indexed on (company_id, issue_id, type).

• Checked the reported incident against the live row. PEN-2370 carried two ready_for_review PR work products (Blockcast/paperclip#1583, #1581) written by the same webhook that had already woken that owner from those PRs earlier the same day, with its monitor cleared at monitorAttemptCount: 8. The evidence of attendance was on the row, in an indexed table, and nothing read it. So this is not "the platform cannot see event-wake eligibility" — it is a read that was never performed.

• Argued myself into the bound rather than out of it. An open PR proves a wake arrives when the PR next moves, not on a schedule. Unbounded, an abandoned PR would hold its issue in_progress and unattended forever — the same silent-darkness failure PEN-2791 was filed about, entered from the other side. I raised this against my own proposal on the issue before writing it, and it is why there is a grace and a test pinning it.

• Rejected the one-line version of that bound. Reusing lapsedMonitorGraceMs (6h) was tempting and is wrong: it bounds belief in an anomaly, where 6h is generous. An open PR awaiting human merge is the normal resting state of correct work and routinely spans days here, so a 6h bound would re-seize exactly these rows on the second morning.

• Separated what I measured from what I quoted. The recovery-action record (fe131799, its evidence keys, the 2m27s gap) is the filer's; /api/issues/{id}/recovery-actions returns 404 from an agent seat, so I could not re-measure it. The code comments say so rather than presenting it as my own measurement.

• I searched for similar or duplicate prior PRs before opening this one.

Searched Blockcast/paperclip PRs for stranded, attendance, hasPersistedDurableWaitPath and work-product across all states. Closest prior art is #1330 (fix(recovery): bound how long a lapsed monitor counts as a live wake path, BLO-24782, merged), which bounded path 4; this adds a path 4 never had a sibling for. #1247 (BLO-19566) is the PR that introduced the PR work-product rows this reads. Neither duplicates this change, and no open PR touches hasPersistedDurableWaitPath.

Linked issue: PEN-2791 (https://paperclip.blockcast.net/PEN/issues/PEN-2791). Reported instance: PEN-2370.

What Changed

A sixth attendance path — an open, webhook-recorded GitHub PR — is now counted, and bounded.

file	change
server/src/services/pull-request-work-products.ts	OPEN_ / TERMINAL_PULL_REQUEST_WORK_PRODUCT_STATUSES, and the producer's return type narrowed from IssueWorkProduct["status"] to exactly what it can return, making the split a checked partition
server/src/config.ts	openPullRequestAttendanceGraceMs — 7d default, 1h–30d, OPEN_PULL_REQUEST_ATTENDANCE_GRACE_MS
server/src/services/recovery/service.ts	hasOpenPullRequestWakePath, consulted from hasPersistedDurableWaitPath; both graces now come off one loadConfig()
server/src/__tests__/issue-recovery-actions.test.ts	5 DB-backed sweep tests
server/src/__tests__/pull-request-work-products.test.ts	3 partition tests

No new table, writer, index or migration — this reads storage the webhook already maintains, on an index that already exists.

Three deliberate narrowings, each with a test that would catch its removal:

1. Only webhook-written rows count. Filtered on metadata source and system source-trust, mirroring the reverse lookup already in routes/github-webhook.ts. Only a row the webhook wrote is evidence the webhook will fire again; a hand-created PR work product means someone typed a URL, which predicts no wake at all.
2. Only the succeeded-run gate consults it. That gate is where the sweep reasons from absence of evidence, which is the defect. The failed/nonretryable arms escalate on positive evidence something broke, and an open PR does not refute that — widening this to them would suppress recovery from real faults.
3. The open/terminal split lives beside its producer. The consumer is a SQL inArray several files away; a second hand-written status list there would drift silently the first time the mapping gains a state. Instead the producer's return type is the partition, so adding a fifth PR state without classifying it fails the build.

Verification

Run on the fix commit, embedded Postgres, --pool=forks --isolate.

Typecheck — pnpm --filter @paperclipai/server typecheck: clean.

Full suite for the touched file — issue-recovery-actions.test.ts: 170 passed (170), i.e. the 165 pre-existing tests plus the 5 new ones. pull-request-work-products.test.ts: 12 passed (12).

Mutation-checked, because a passing test is not evidence of a working fix. Disabling only the new disjunct (if (false && await hasOpenPullRequestWakePath(...))) and re-running:

× does not escalate while an open pull request is recorded against the issue
AssertionError: expected 1 to be +0
Tests  1 failed | 4 passed | 165 skipped

escalated: 1 — the row is seized. That is the PEN-2370 outcome reproduced on demand, and it goes green with the disjunct restored. The other four PEN-2791 tests pass in both states by design: they are the negative controls (no PR / merged PR / hand-created PR / stale PR), and each asserts the row does escalate, so they must be insensitive to the fix. The control test in particular establishes that this row shape is genuinely at risk, without which the exemption test would prove nothing.

The five sweep tests, in the order they discriminate:

test	asserts
no PR recorded (control)	escalates — the shape is really seizable
open ready_for_review PR	does not escalate, stays in_progress, stays assigned, no recovery action
merged PR	escalates — terminal states emit no further webhook
hand-created open PR	escalates — provenance filter holds
open PR older than the grace	escalates — the bound holds

The seeding builds its rows through the real buildPullRequestWorkProductFields, not hand-written literals, so if the webhook's metadata source or source-trust constants move, the test moves with them and a stale filter fails loudly instead of silently matching nothing.

Risks

• An open PR that nobody ever touches again holds its issue out of the sweep for up to the grace. This is the honest limit of the evidence and the reason the grace exists; the last test pins it. Default 7d is a judgement call about how long "waiting on a human merge" stays a description of the PR rather than of a problem nobody is holding — it is the number most worth arguing with here, and it is a config knob precisely so that argument does not need a code change.
• Scope is narrower than PEN-2791's full ask. This fixes the PEN-2370 shape (succeeded run, cleared monitor, no blockers, open PR). I am not claiming it closes the PEN-2706 or PEN-2696 instances, which went dark by other routes, nor the filer's second done-when for rows with no PR at all — that is a larger argument about what attendance means for a row waiting on a purely human gate, and it should be scoped separately rather than smuggled in here.
• Behaviour change is one-directional: strictly fewer escalations. It can only cause the sweep to skip a candidate it would previously have moved to blocked, never the reverse. So the failure mode of a bug in this predicate is a row staying in_progress with its owner, not a row being taken from one.
• issue_work_products.updatedAt is the recency signal. That is sound because upsertByExternalId advances it only when it accepts a strictly-newer PR event — redeliveries and out-of-order webhooks are rejected without touching it, so this cannot be refreshed by webhook noise on a PR that is not really moving. If that upsert's ordering guard were ever relaxed, this bound would weaken with it.

Model Used

claude-opus-5 (Claude Code, 1m context)

[allyblockcast]

🔗 Paperclip issue: PEN-2696
🔗 Paperclip issue: BLO-19566
🔗 Paperclip issue: BLO-24782
🔗 Paperclip issue: PEN-2791
🔗 Paperclip issue: PEN-2370
🔗 Paperclip issue: PEN-2706

[allyblockcast]

🔗 Paperclip issue: PEN-2696
🔗 Paperclip issue: BLO-19566
🔗 Paperclip issue: BLO-24782
🔗 Paperclip issue: PEN-2791
🔗 Paperclip issue: PEN-2370
🔗 Paperclip issue: PEN-2706

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The change reuses the existing webhook-maintained work-product records instead of introducing a parallel persistence path.
• Provenance filtering and a bounded grace period prevent hand-created or indefinitely stale PR rows from becoming durable attendance claims.
• The status partition and recovery behavior have focused positive and negative-control tests.

Recommended Action

1. No Critical or Important changes are required from this review.
2. Consider the Suggestions section opportunistically.