feat(issues): count an open pull request as an in_review review path (PEN-2853)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Issue status is the signal that says who owns the next action; the strandedness sweep and the in_review write validator both read it
• An agent-authored move to in_review is refused unless one of five review paths holds — and none of them is an external event wake or an open pull request
• So for the commonest shape an agent finishes in, a green PR awaiting a human merge press, there was no honest path: the only unilaterally satisfiable remedy was to arm a monitor polling a gate no poll can move
• That pushed agents to either game the check or leave the row in_progress, which reads as "actively being worked" while it is really waiting on a human — and, being uncounted, left it seizable by the sweep (PEN-2370)
• This pull request adds a sixth path, open_pull_request_work_product, that delegates to the sweep's own predicate instead of restating it
• The benefit is that a PR-gated row can be called in_review truthfully, and the two predicates cannot drift apart again

Linked Issues or Issue Description

• Refs PEN-2853 — Finding 1. (Finding 2 shipped separately in fix(issues): bound the in_review validator's monitor to a future check (PEN-2853) #1588, merged.)
• Refs PEN-2791 / fix(recovery): count an open pull request as an attendance path (PEN-2791) #1584 (merged) — established the read-side predicate this reuses.

Related open PRs found in the dedup search — please read before merging:

• ⚠️ fix(recovery): count an open pull request as an in-review action path (PEN-3198) #1834 (PEN-3198) — extends the same PEN-2791 ruling to a third gate, the issue-graph liveness classifier. It conflicts with this PR textually and architecturally. See "Risks".
• fix(issues): say why a live park is not an in_review review path (BLO-33572) #1804 (BLO-33572) — also edits assertAgentInReviewReviewPath and issue-execution-policy-routes.test.ts. Textual conflict only; the two changes are independent in intent.
• test(recovery): pin the open-PR attendance path's failed-arm scoping (BLO-32679) #1717 (BLO-32679) — also edits issue-recovery-actions.test.ts and recovery/service.ts. Textual conflict only.

What Changed

• Moves hasOpenPullRequestWakePath out of recovery/service.ts into a new services/open-pull-request-attendance.ts, so it has one home and more than one caller. The sweep now imports it; its logic is unchanged.
• Adds a sixth validReviewPath, open_pull_request_work_product, to assertAgentInReviewReviewPath, delegating to that shared predicate.
• Updates both hand-maintained lists that must not drift from the checks above them: the validReviewPaths array and INVALID_AGENT_IN_REVIEW_DISPOSITION_MESSAGE, which now tells a PR-gated caller to open the PR rather than arm a monitor.
• Exports openPullRequestWakePathConditions under the same name fix(recovery): count an open pull request as an in-review action path (PEN-3198) #1834 chose, so that PR's rebase is an import rather than a second definition.
• Tests: a DB-backed agreement test, and a route-level admit test.

The validator inherits the sweep's grace and provenance filters rather than relaxing them. A point-in-time reading is tempting — a 10-day-old open PR is arguably "in review" at the instant of the write — but it would recreate Finding 2's defect in a new column: admitted by the write side, discounted by the read side, seizable the moment it lands. The invariant both callers preserve is everything the validator admits, the sweep counts. The asymmetry is safe in one direction only.

Deliberately not placed in pull-request-work-products.ts despite that module owning the constants it reads: it documents itself as DB- and network-free so the webhook mapping stays unit-testable without Postgres.

Verification

pnpm --filter @paperclipai/server exec tsc --noEmit                  # exit 0
npx vitest run src/__tests__/issue-execution-policy-routes.test.ts   # 47/47
npx vitest run src/__tests__/issue-recovery-actions.test.ts          # 191/191
node scripts/check-test-undefined-symbols.mjs                       # ok

Agreement test, on a real row. Driven through the real route against real Postgres, because the filters that matter — terminal status, webhook provenance, the updatedAt grace — live in SQL; a mock returning rows regardless of the WHERE clause would pass just as happily with every filter deleted. The expectation is not hand-written per fixture: it is hasOpenPullRequestWakePath itself, evaluated on the same row the route just saw, so it fails in either direction. Fixtures: no PR · open webhook-written PR · merged PR · hand-created row · open PR aged past the grace.

⚠️ Trap worth a reviewer's attention. The agent must hold the execution lock or the route answers 409 before the validator ever runs — and 409 !== 200 compares equal to "refused", so four of five fixtures passed while proving nothing. The test now asserts refusals are specifically 422 invalid_issue_disposition, which is what stops a non-validator status masquerading as a verdict.

Mutation check. Disabling only the new disjunct:

suite	under mutation	restored
issue-recovery-actions (open-PR describe)	1 failed / 5 passed	191/191
issue-execution-policy-routes	1 failed / 46 passed	47/47

Exactly the two new tests go red and nothing else moves, so they are pinned to this disjunct rather than riding on a neighbour.

Risks

The real one is #1834, and it needs a human decision, not a rebase-and-hope. That PR adds open-PR attendance to a third predicate and, to do it, defines its own openPullRequestWakePathConditions as a local function inside recovery/service.ts — which routes/ cannot import. This PR instead moves the predicate out to a shared module. Both edit the same lines.

Left alone the two would land two independent "single definitions" of one predicate — on the row filed because exactly that happened once already. To make converging cheap rather than a judgement call, this PR exports the conditions under the name #1834 already uses, so whichever merges second replaces its local definition with an import. Recommend merging the two deliberately in sequence, not concurrently.

Otherwise low risk, and the failure mode is conservative in the right direction:

• The new path is a widening — it only ever admits a transition that was previously refused, so no row that could reach in_review before is blocked now.
• It is the last disjunct, so the five existing paths short-circuit ahead of it and their behaviour is untouched.
• recovery/service.ts behaviour is unchanged; the predicate moved file, its logic did not (191/191).
• Honest limitation: the agreement test enforces delegation, not the shared definition's own semantics — change the definition and both callers move together and still agree. The grace and provenance semantics stay pinned by the pre-existing sweep tests, which still pass.

Model Used

Claude Opus 4.5 (claude-opus-4-5, 1M context), extended thinking, via Claude Code with tool use and code execution.

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, no UI surface
• I have updated relevant documentation to reflect my changes — the 422 message and the module docblocks are the user-facing documentation of this rule
• I have considered and documented any risks above
• All Paperclip CI gates are green — verify still running at time of writing
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: PEN-2791
🔗 Paperclip issue: PEN-2370
🔗 Paperclip issue: PEN-2853

[allyblockcast]

🔗 Paperclip issue: PEN-2791
🔗 Paperclip issue: PEN-2370
🔗 Paperclip issue: PEN-2853

[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
• 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]

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
• 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. The configured lens binaries were unavailable in this runtime; their review prompts were applied directly to the exact PR diff.
Reviewed head: f2ce407

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The shared attendance predicate keeps the write-side in_review validator and the recovery sweep aligned instead of duplicating SQL logic.
• The query preserves the existing status, webhook provenance, source-trust, and freshness constraints, avoiding a broad admission of arbitrary or abandoned PR links.
• The agreement test exercises both admission and refusal paths and explicitly guards against a route-level 409 masking a validator result.

Recommended Action

1. No Critical or Important changes requested. CI is still running; merge after the required checks pass.