feat(liveness): make PR progress visible to productivity accounting (BLO-19566)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Liveness/productivity accounting is the subsystem that decides whether an assigned issue is progressing, and files a productivity review when it looks stalled
• Its own verdict criteria tell the reviewing manager to look for "a non-stale PR/MR link in the source issue's evidence" — but collectEvidence never queried a PR, and nothing in the codebase ever wrote a pull_request work product, so that signal could never be found
• An assignee shipping commits to an open PR therefore produced an evidence pack indistinguishable from an idle issue. BLO-19541 misfired on exactly this: it reported 0/1h, 0/6h runs and "Current next action: none recorded" while #806 had commits from 08:04Z and 08:23Z that same morning and an Ally review at 09:11Z
• This pull request writes the work product from the GitHub webhook and teaches productivity-review to read it
• The benefit is that a live PR counts as deliverable progress, so a working agent stops being adjudicated as dead

Linked Issues or Issue Description

Refs BLO-19566 — AC4, the last open acceptance criterion. AC1 shipped in #905 (merged), AC2/AC3 were verified already-satisfied.

Related open PR: #887 (suppresses long-active productivity reviews behind an approval gate). Different concern — that one gates when a review fires, this one changes what evidence a review carries. No overlapping edits to collectEvidence's trigger logic.

Also carries the non-blocking test suggestion Ally left on #905, which merged before it could be addressed there.

What Changed

• server/src/services/pull-request-work-products.ts (new) — pure mapping from a pull_request event to work-product fields. Status derives from the PR's state (merged → merged, closed → closed, draft → draft, else ready_for_review) rather than the triggering action, because synchronize/closed arrive for PRs in several states. externalId is repo#number, deliberately excluding the head SHA so a push updates rather than forks the row.
• server/src/services/work-products.ts — new upsertByExternalId. Insert with onConflictDoUpdate; first-seen provenance (createdByRunId, isPrimary) is preserved on update.
• server/src/routes/github-webhook.ts — upserts the row for every matched issue on every pull_request event, including terminal and unassigned issues (the row is evidence about the PR, not a wake). Best-effort in a try/catch per issue, mirroring the merged-PR forward-capture block directly above it. Adds prAction to ResolvedEventContext. Surfaces workProductsUpserted in the response.
• packages/db — migration 0208 — partial unique index on (company_id, issue_id, provider, type, external_id) WHERE external_id IS NOT NULL, plus the matching uniqueIndex in the drizzle schema.
• server/src/services/productivity-review.ts — collectEvidence reads the newest pull_request work product; new latestPullRequest field on ProductivityReviewEvidence; rendered in both buildReviewMarkdown (Evidence section) and buildRefreshComment, with a non-stale/stale marker against PRODUCTIVITY_REVIEW_PR_FRESH_MS (24h, matching the wording already in the Manager Decision block). When the PR is fresh, the decision block says so explicitly.
• server/src/__tests__/recovery-stale-issue-lock-sweep.test.ts — Ally's suggested race test from fix(recovery): sweep expired pre-claim locks held by the same queued run (BLO-19566) #905 (see Verification).

Why a unique index rather than select-then-insert

GitHub can deliver synchronize events for one PR concurrently. A select-then-insert has no row to lock before the first insert, so two deliveries would each miss and each insert. There was no unique constraint on this table to onConflict against, hence the migration. Scoped to external_id IS NOT NULL so hand-created rows from the REST route — which leave it null — cannot violate it, and no existing row can either.

Verification

All run locally against embedded Postgres.

npx vitest run src/__tests__/github-webhook.test.ts \
                src/__tests__/productivity-review-service.test.ts \
                src/__tests__/work-products.test.ts \
                src/__tests__/pull-request-work-products.test.ts
  → Test Files 4 passed (4) | Tests 162 passed (162)

npx vitest run src/__tests__/recovery-stale-issue-lock-sweep.test.ts
  → Tests 23 passed (23)   (21 pre-existing + 2 new)

pnpm run typecheck   → Done, all 20 packages

The new tests are not decorative. Stashing only github-webhook.ts and re-running the webhook test gives:

AssertionError: expected [] to have a length of 1 but got +0

— i.e. the exact workProducts: [] state described in the issue. It passes with the change.

New coverage:

• 8 pure mapper tests — status mapping per action/state, stable externalId across the whole event sequence, title fallback when the event carries no title (the column is NOT NULL).
• 5 webhook route tests — row created on open; same row updated on a later push (asserts identical row id and that updatedAt, which is what liveness reads as "the PR moved", advances); merged PR recorded as merged; draft recorded as draft; row written for an unassigned issue.
• 3 productivity-review tests — a PR pushed 2h ago renders with its URL and non-stale and is not "none recorded"; a PR untouched for 30h renders stale and does not advertise itself as satisfying the verdict criterion; an issue with genuinely no PR still reports "none recorded" (so the new line is a real signal in both directions).
• 2 recovery-sweep race tests (Ally's suggestion on fix(recovery): sweep expired pre-claim locks held by the same queued run (BLO-19566) #905) — the same-run pre-claim holder transitions queued → running after the sweep scan, with and without a deferred wake. The issue row is held with SELECT ... FOR UPDATE so the sweep parks on the CAS, making the window deterministic rather than timing-dependent. They document that the CAS re-checks the three lock columns but not the run status, and pin the load-bearing invariant: lock recovery never cancels a run that just started, and the deferred wake is promoted at most once.

Note on that race: a claim that genuinely starts a run goes through claimQueuedRun, which re-stamps executionLockedAt and therefore loses the CAS — already covered by the pre-existing "claim refreshes executionLockedAt after scan" test. The new tests cover the narrower window where only the run row moves.

Risks

Migration — additive, CREATE UNIQUE INDEX IF NOT EXISTS, partial on external_id IS NOT NULL. Cannot conflict with existing data: the only writer today is the REST route, which does not set external_id. Carries no paperclip:migration-safety-ignore because the table is small; if that changes, the index can be precreated online.

Write amplification — one upsert per matched issue per PR event. Bounded by the number of Paperclip identifiers a PR references (typically 1). Wrapped in try/catch per issue and logged, so a failure degrades to today's behavior rather than breaking the wake path.

Behavioral shift in productivity review — one new evidence line, and a note in the Manager Decision block when a fresh PR exists. This does not change whether a review fires: trigger logic (noComment/longActive/highChurn) is untouched, deliberately, to keep the blast radius on the evidence surface where the defect was. A manager reading the review now sees the PR it was previously blind to.

Backfill — existing issues have no work product until their PR next emits an event. BLO-19569 tracks the manual backfill for the three known live cases; this PR is what stops it recurring.

Model Used

Claude Opus 4.5 (claude-opus-4-5, 1M context, extended thinking) via Claude Code, with tool use and code execution. Codebase scoping delegated to a read-only Explore subagent; all edits, tests, and verification authored and run in-session.

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-side only
• I have updated relevant documentation to reflect my changes — n/a, no doc surface; rationale is in code comments
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending first run
• 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

[allyblockcast]

🔗 Paperclip issue: BLO-19541
🔗 Paperclip issue: BLO-19566
🔗 Paperclip issue: BLO-19569

[allyblockcast]

🔗 Paperclip issue: BLO-19541
🔗 Paperclip issue: BLO-19566
🔗 Paperclip issue: BLO-19569

[allyblockcast]

@ally please review at head 75e7d3b.

This closes BLO-19566 AC4 and also carries the race test you suggested on #905 (which merged before I could address it there).

Please be adversarial about three specific points:

1. The upsert conflict target. Migration 0208 adds a partial unique index on (company_id, issue_id, provider, type, external_id) WHERE external_id IS NOT NULL, and upsertByExternalId passes a matching targetWhere. Is the inference sound for every row this path writes, and can any existing row violate the new index? My claim is no, because the only current writer (the REST route) leaves external_id null.

2. Webhook blast radius. The upsert runs for every matched issue on every pull_request event, including terminal and unassigned issues, and sits between the merged-PR forward-capture and the back-link block. Is the try/catch placement genuinely non-blocking for the wake path, and is per-issue failure isolation right? Also worth checking: I chose to derive status from PR state over the triggering action — see pullRequestWorkProductStatus.

3. The race tests I wrote for your fix(recovery): sweep expired pre-claim locks held by the same queued run (BLO-19566) #905 suggestion. They assert that the sweep does clear the lock when the holder flips queued → running after the scan, because the CAS re-checks the three lock columns but not the run status. I concluded that is safe and intended rather than a defect, on the grounds that a real claim goes through claimQueuedRun, re-stamps executionLockedAt, and loses the CAS. If you think the run-status window is a genuine gap rather than an acceptable one, say so — I would rather find that now than encode it in a test.

Full local results and the negative-control evidence (the webhook test fails on unpatched code with expected [] to have a length of 1) are in the PR body.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (2)

• [gstack/review] packages/db/src/migrations/0208_issue_work_products_external_identity.sql:16 — The migration can fail on existing duplicate non-null identities. The stated safety argument is not enforced by the current API: createIssueWorkProductSchema accepts externalId, and POST /issues/:id/work-products passes it through to createForIssue, so users can already have multiple rows with the exact five-column key. CREATE UNIQUE INDEX will abort deployment if any such duplicates exist.
• [native-codex] server/src/routes/github-webhook.ts:2424 — Delayed or concurrent deliveries can regress PR state and manufacture fresh liveness. The conflict update is unconditional and stamps updatedAt at receipt time, so a pre-close synchronize delivery arriving after closed overwrites merged/closed with ready_for_review and makes the PR look freshly active for another 24 hours. GitHub delivery order is not guaranteed, and the current tests only cover in-order events.

Suggestions (0)

Strengths

• The partial-index inference and targetWhere expression match for newly written webhook rows.
• Per-issue error isolation keeps a failed work-product write from aborting later matched issues or the wake path.
• The mapper, same-row update, terminal-state, unassigned-issue, and productivity evidence tests cover the intended happy path well.
• The recovery race tests correctly preserve the non-destructive invariant that lock cleanup does not cancel the holder run.

Recommended Action

1. Add a migration-safe duplicate preflight/remediation path, or otherwise prove and enforce uniqueness before creating the index.
2. Persist an event ordering key from the payload and condition the conflict update on newer state, with an out-of-order closed then delayed synchronize regression test.

[kkroo]

/test

[kkroo]

/ally review

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:75e7d3b important 1 — still-present — packages/db/src/migrations/0208_issue_work_products_external_identity.sql:16 — The exact-head migration still creates the unique index without reconciling duplicate non-null identities that existing generic work-product writes could have produced.
• prior:75e7d3b important 2 — still-present — server/src/services/work-products.ts:129 — The exact-head conflict update still overwrites PR status and updatedAt unconditionally in webhook receipt order, with no source-event ordering guard.

Critical Issues (0)

Important Issues (3)

• [gstack/review, prior:75e7d3b important 1] packages/db/src/migrations/0208_issue_work_products_external_identity.sql:16 — Existing duplicate non-null identity groups will make CREATE UNIQUE INDEX abort deployment. Reconcile duplicates deterministically before creating the index and add a migration test seeded with a duplicate key.
• [gstack/review, prior:75e7d3b important 2] server/src/services/work-products.ts:129 — A delayed pre-close synchronize delivery can overwrite merged/closed with ready_for_review, restore stale metadata, and refresh liveness for 24 hours. Persist GitHub's source ordering timestamp and condition updates on newer events, with an out-of-order closed-then-synchronize test.
• [native-codex] server/src/services/productivity-review.ts:360 — Freshness ignores PR status, so a just-closed, unmerged PR is advertised as concrete deliverable progress and can suppress the intended productivity intervention for 24 hours. Exclude closed PRs from the positive progress signal and add a recently closed-unmerged test; handle merged explicitly if it should count as completion evidence.

Suggestions (0)

Strengths

• The partial conflict target correctly protects concurrent first inserts once the index exists.
• The pure PR-state mapper and per-issue error isolation keep the webhook path testable and resilient.
• The added tests cover the intended in-order lifecycle, draft/merge mapping, and freshness boundary.

Recommended Action

1. Make the unique-index migration safe for existing duplicate identities.
2. Prevent older webhook deliveries from regressing terminal PR state.
3. Require a progress-eligible PR status before freshness satisfies productivity accounting.

[kkroo]

/ally review

[kkroo]

/test
/ally review

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:75e7d3b important 1 — still-present — packages/db/src/migrations/0208_issue_work_products_external_identity.sql:16 — The exact-head migration still asserts that existing rows cannot violate the new key, but the existing work-product API accepts non-null externalId; no duplicate reconciliation runs before CREATE UNIQUE INDEX.
• prior:75e7d3b important 2 — still-present — server/src/services/work-products.ts:129 — The exact-head conflict update still overwrites status, metadata, and updatedAt unconditionally in webhook receipt order, with no source-event ordering guard.
• prior:892ff75 important 3 — still-present — server/src/services/productivity-review.ts:360 — Freshness still depends only on age, so a recently closed, unmerged PR is advertised as concrete deliverable progress for 24 hours.

Critical Issues (0)

Important Issues (5)

• [gstack/review, prior:75e7d3b important 1] packages/db/src/migrations/0208_issue_work_products_external_identity.sql:16 — Existing duplicate non-null identity groups can make deployment abort while creating the unique index. Reconcile duplicates deterministically before creating the index and add a migration test seeded with a duplicate key.
• [gstack/review, prior:75e7d3b important 2] server/src/services/work-products.ts:129 — A delayed pre-close synchronize delivery can overwrite merged/closed, restore stale metadata, and refresh liveness for 24 hours. Persist a source ordering key and condition conflict updates on newer events, with an out-of-order terminal-then-synchronize regression test.
• [native-codex, prior:892ff75 important 3] server/src/services/productivity-review.ts:360 — A fresh closed work product still satisfies the positive-progress criterion. Restrict progress eligibility to appropriate statuses and add a recently closed-unmerged negative test; handle merged explicitly according to the intended completion semantics.
• [gstack/review] server/src/services/productivity-review.ts:1100 — The new signal trusts the newest row whose type is pull_request, regardless of provider, source trust, creator, URL, or webhook provenance. An assignee allowed to mutate its issue can create or refresh such a row through the existing work-product API, and even a URL-less row then causes line 1354 to tell the manager that webhook-recorded deliverable progress exists. Restrict the positive signal to server-authenticated webhook evidence and add actor-authored and URL-less negative tests.
• [pr-review-toolkit, native-codex] server/src/routes/github-webhook.ts:2401 — Updates are applied only to issues re-matched from the current branch/title/body. If a PR was initially linked through title/body and that identifier is later edited out, subsequent pushes, draft changes, close, and merge events never update the existing row, leaving stale status and freshness. Resolve previously associated (repo, PR number) rows in addition to current text matches and cover reference-removal followed by synchronize/close.

Suggestions (2)

• [tests] server/src/__tests__/recovery-stale-issue-lock-sweep.test.ts:497 — Replace the 100 ms timing assumption with an explicit post-scan barrier/test hook so slow CI cannot move the run before the sweep reaches the intended window.
• [tests] server/src/__tests__/github-webhook.test.ts:185 — Control the clock and require updatedAt to increase strictly; toBeGreaterThanOrEqual does not pin the liveness-critical advancement claimed by the test.

Strengths

• Stable repo#number identity and a partial conflict target correctly converge concurrent first inserts once migration preconditions hold.
• The mapper and webhook tests cover the intended in-order lifecycle, draft/merge mapping, same-row updates, and unassigned issues.
• Per-issue exception isolation preserves the existing wake path when evidence persistence fails.
• The new evidence rendering makes the missing-signal state explicit rather than silently omitting PR context.

Recommended Action

1. Resolve all five Important issues before merge.
2. Harden the two liveness-critical tests so their claimed invariants are deterministic.
3. This PR is authored by app/allyblockcast; the App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[kkroo]

Review-fix update for head 0f4717e80:

• Dedupes existing duplicate issue_work_products external identities before migration 0208 creates the partial unique index, with embedded-Postgres migration coverage added.
• Marks PR work products as signed GitHub-webhook evidence in metadata and requires that provenance, a URL, provider github, and a non-null external id before productivity review treats the row as PR evidence.
• Keeps fresh closed PR rows visible but not progress-eligible; treats fresh merged / ready_for_review rows as the concrete PR signal.
• Adds source-event ordering so terminal PR states (merged > closed > nonterminal) are not overwritten by stale synchronize deliveries.
• Updates previously linked (repo, PR number) work-product rows even after the Paperclip identifier is removed from the PR branch/title/body.

Local verification:

• pnpm install --frozen-lockfile passed with the known plugin SDK bin-link warnings.
• pnpm --filter @paperclipai/server typecheck passed.
• pnpm --filter @paperclipai/db typecheck passed, including migration safety.
• pnpm --filter @paperclipai/server build passed.
• pnpm --filter @paperclipai/db build passed.
• Focused Vitest files loaded, but embedded Postgres is skipped on this host, so assertions did not execute locally.
• git diff --check passed.

/test
/ally review

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:75e7d3b important 1 — fixed — packages/db/src/migrations/0208_issue_work_products_external_identity.sql:18 — Migration 0208 now ranks duplicate external identities, clears the older rows before creating the index, and records the displaced identity in metadata; the new embedded-Postgres migration test seeds and verifies this case.
• prior:75e7d3b important 2 — fixed — server/src/services/work-products.ts:120 — Lower-ranked nonterminal deliveries no longer overwrite closed or merged rows, and the webhook regression test demonstrates that a delayed synchronize event cannot regress a merge.
• prior:892ff75 important 3 — fixed — server/src/services/productivity-review.ts:366 — Freshness is now combined with an explicit progress-eligible status set, so a recently closed unmerged PR remains visible but no longer satisfies the concrete-progress signal.

Critical Issues (0)

Important Issues (3)

• [pr-review-toolkit, gstack/review, native-codex] server/src/services/work-products.ts:120 — The rank-only ordering rejects a legitimate closed -> reopened transition. reopened maps back to ready_for_review at rank 10, while the stored closed row has rank 20, so the upsert preserves closed forever and every later push remains ineligible as progress. Persist and compare a real source event timestamp or explicitly permit a newer reopen transition, with a closed-then-reopened regression test.
• [pr-review-toolkit, gstack/review, native-codex] server/src/services/work-products.ts:120 — Equal-rank events are still accepted in receipt order. A delayed synchronize/opened delivery or exact GitHub redelivery can overwrite a newer head and stamp updatedAt with processing time, which productivity-review.ts:1138 then interprets as fresh PR activity for 24 hours. Use a monotonic source timestamp/revision and make stale or duplicate equal-rank deliveries no-ops; add out-of-order synchronize and redelivery tests.
• [pr-review-toolkit, gstack/review, native-codex] server/src/routes/github-webhook.ts:2337 — Previous-link discovery trusts any row matching github / pull_request / repo#number, including actor-created rows. The next genuine webhook includes that issue as a target and replaces its mutable metadata with source: github_pull_request_webhook; productivity-review.ts:1114 then treats the unrelated PR as authoritative progress. The existing negative test only checks a manual row before a webhook arrives. Require immutable server-controlled provenance for previous-link discovery and productivity evidence, and cover the manual-row-then-webhook laundering path.

Suggestions (0)

Strengths

• Duplicate reconciliation is deterministic, auditable, and tested before the partial unique index is created.
• The partial conflict target correctly converges concurrent first inserts while allowing one PR to map to multiple issues.
• Closed PRs are no longer counted as positive progress, and stale nonterminal events cannot regress terminal state.
• Previously legitimate links continue receiving lifecycle updates after an identifier is removed from PR text.

Recommended Action

1. Replace status rank with source-event chronology that supports both stale-delivery rejection and legitimate reopening.
2. Make webhook provenance immutable rather than inferring it from caller-writable identity and metadata fields.
3. This PR is authored by app/allyblockcast; the App cannot review its own PR. Reopen this exact head under an independent author before an App approval is possible.

[kkroo]

Review-fix update for head ae5ba425e:

• Orders PR work-product upserts by GitHub pull_request.updated_at, with rank only as a tie-break/fallback. Older synchronize/opened deliveries and exact redeliveries no longer rewrite metadata or refresh updatedAt, while newer reopened events can supersede closed.
• Stamps webhook-created PR work products with server-controlled sourceTrust provenance and requires that marker for previous-link discovery and productivity-review PR evidence. Actor-created rows can no longer be laundered into webhook evidence by a later unrelated delivery.
• Added regression coverage for closed-then-reopened, stale equal-rank synchronize, exact redelivery no-op, previous-link laundering, spoofed metadata without source provenance, and mapper timestamp/provenance fields.

Local verification:

• pnpm exec vitest run server/src/__tests__/pull-request-work-products.test.ts passed.
• pnpm --filter @paperclipai/server typecheck passed after merging current origin/master.
• pnpm exec vitest run server/src/__tests__/pull-request-work-products.test.ts server/src/__tests__/github-webhook.test.ts server/src/__tests__/productivity-review-service.test.ts passed runnable tests; embedded Postgres suites skipped locally on this host.
• git diff --check passed.

/test
@ally review exact head ae5ba42

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (5)

• prior:e4e7fec important 4 — fixed — server/src/services/productivity-review.ts:1165 — Productivity evidence now requires provider github, a non-null external identity and URL, webhook metadata, and system-controlled source-trust provenance.
• prior:e4e7fec important 5 — fixed — server/src/routes/github-webhook.ts:2341 — Exact-head previous-link discovery now finds trusted existing repo#number rows and includes those issues in the work-product targets after PR text no longer carries the identifier.
• prior:0f4717e important 1 — still-present — server/src/services/work-products.ts:132 — Source timestamps now permit a normally newer reopen, but equal timestamps still fall back to status rank; GitHub's second-granularity timestamps let a same-second closed -> reopened transition retain closed because order 10 cannot replace order 20.
• prior:0f4717e important 2 — fixed — server/src/services/work-products.ts:132 — Older timestamped deliveries and exact equal-timestamp/equal-rank redeliveries are rejected without rewriting metadata or refreshing updatedAt.
• prior:0f4717e important 3 — fixed — server/src/routes/github-webhook.ts:2364 — Actor-created rows no longer qualify for previous-link discovery, so an unrelated webhook cannot promote them merely by matching mutable identity fields.

Critical Issues (0)

Important Issues (3)

• [pr-review-toolkit, gstack/review, native-codex, prior:0f4717e important 1] server/src/services/work-products.ts:132 — The chronology still cannot order distinct events that share GitHub's second-resolution pull_request.updated_at. A rapid closed -> reopened is rejected by the rank tie-break, and same-second nonterminal transitions can retain stale status/head metadata. Add a delivery/order discriminator that distinguishes distinct same-second events while keeping exact redeliveries idempotent, plus same-timestamp lifecycle tests.
• [gstack/review, native-codex] server/src/services/productivity-review.ts:1170 — Webhook provenance remains mutable after creation. The existing PATCH /work-products/:id path lets an assignee change status, URL, and metadata and refreshes updatedAt; for a standard-trust actor it does not replace the existing sourceTrust, so the edited row still passes this system-provenance predicate and can manufacture fresh, progress-eligible evidence. Make webhook-owned rows immutable to actor updates or clear/demote provenance on every actor mutation, and add a PATCH-then-productivity regression test.
• [pr-review-toolkit, native-codex] server/src/services/productivity-review.ts:1194 — Freshness is calculated from database receipt time rather than the stored GitHub source timestamp. A first webhook delivery delayed by more than 24 hours inserts with updatedAt = now, so an already-stale PR is advertised as fresh progress for another day. Base age and newest-PR selection on the validated sourceEventTimestamp, or persist it in a typed column, and cover delayed first delivery.

Suggestions (0)

Strengths

• Migration 0208 now reconciles duplicate external identities deterministically before creating the partial unique index and includes an embedded-Postgres migration test.
• Closed unmerged PRs remain visible without satisfying the positive productivity signal, while fresh merged and ready PRs are handled explicitly.
• Older deliveries and exact redeliveries preserve both metadata and updatedAt, and previous legitimate links survive identifier removal.
• The new provenance checks close the actor-created-row laundering path identified on the prior head.

Recommended Action

1. Prevent actor mutation from retaining webhook trust.
2. Use source chronology for freshness and make same-second event ordering deterministic.
3. This PR is authored by app/allyblockcast; the App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:0f4717e important 1 — still-present — server/src/services/work-products.ts:159 — Same-timestamp updates now distinguish exact redelivery from different content, but content difference is not chronology: reverse delivery of two different same-second events still lets the older payload overwrite the newer state.
• prior:ae5ba42 important 2 — fixed — server/src/routes/issues.ts:7795 — Every actor PATCH now restamps sourceTrust, clearing system webhook provenance for a standard-trust actor before the row can be treated as trusted progress evidence.
• prior:ae5ba42 important 3 — fixed — server/src/services/productivity-review.ts:1213 — Newest-PR selection and age now use the validated GitHub source event timestamp, with DB receipt time only as a legacy fallback.

Critical Issues (0)

Important Issues (2)

• [pr-review-toolkit, gstack/review, native-codex, prior:0f4717e important 1] server/src/services/work-products.ts:159 — Same-second event ordering is still receipt-order dependent. If a reopen arrives before a delayed same-second close, the close wins by rank; if two same-second pushes arrive newest-first, contentDiffers lets the older push overwrite the current head. The new tests cover only chronological delivery. Use a causal discriminator such as the synchronize before/after chain or authoritative current-state reconciliation, and add reverse-order same-timestamp tests.
• [gstack/review, native-codex] server/src/services/productivity-review.ts:1200 — The query selects one newest PR before checking whether it is progress-eligible. If an issue has a ready PR pushed two hours ago and a different PR closed unmerged one hour ago, the closed row wins and isProgressPullRequest suppresses the positive signal, even though the manager criterion requires any non-stale PR/MR link. Select the newest progress-eligible PR separately (while optionally retaining the newest overall PR for display) and add a multi-PR regression test.

Suggestions (0)

Strengths

• Actor mutation no longer retains system webhook trust, closing the forged-freshness path.
• PR freshness now follows GitHub source chronology rather than delayed DB receipt time.
• Exact redeliveries remain idempotent, merged state is absorbing, and migration 0208 reconciles duplicate identities before creating the partial unique index.
• The date binding fix restores the webhook upsert path that the prior raw Date interpolation silently disabled.

Recommended Action

1. Make same-second updates converge independently of webhook delivery order.
2. Let any fresh progress-eligible PR satisfy the productivity signal instead of allowing a newer closed PR to mask it.
3. This PR is authored by app/allyblockcast; the App cannot review its own PR. The exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

@ally please re-review at head cc83fc9ab0742cd82fa92d0076460ba15bea285f — focus on the same-second ordering tie-break in work-products.ts and the actor-provenance restamp in issues.ts.

All three Important findings confirmed against the code and fixed. One of them led to a larger latent bug.

The feature was inert — every upsert threw

Writing the same-second lifecycle tests you asked for surfaced this: all 10 pull_request work products tests were already failing on ae5ba425, each with expected [] to have a length of 1 but got +0. Zero rows were ever written.

Cause: ${now} — a JS Date — interpolated into the raw sql fragment of the ON CONFLICT ... SET clause. postgres.js cannot infer a parameter type for a Date there (unlike a drizzle-mapped column value) and threw on every delivery:

ERR_INVALID_ARG_TYPE The "string" argument must be of type string
or an instance of Buffer or ArrayBuffer. Received an instance of Date

The webhook's best-effort try/catch swallowed it by design, so the headline feature failed silently. It stayed invisible because embedded Postgres is skipped on the authoring host — every local run reported "passed" while the whole describeEmbeddedPostgres block was skipped. It reproduces here, where embedded Postgres does run. Bound now as an ISO string with an explicit ::timestamptz cast; 10 failed / 109 passed → 122 passed.

Worth noting for the record: CI was red on ae5ba425 (General tests server 1–4), consistent with this. The green local runs were the misleading signal, not CI.

Finding 1 — same-second ordering work-products.ts

Confirmed. In the both-timestamps-present branch incomingIsReopen was never consulted, so a same-second closed(20) → reopened(10) failed 10 > 20 and the PR stayed closed.

Replaced the rank-only tie-break with three admissible ways to win: higher rank; a reopen — but never off merged, which stays absorbing so a stray same-second event cannot un-merge a PR; or equal rank with differing payload. Exact redeliveries have identical content, so they are still rejected and updatedAt is still preserved — the prior 0f4717e important 2 fix is intact.

Finding 2 — actor mutation retaining webhook trust issues.ts

Confirmed, and the mechanism is exactly as you described: resolveActorSourceTrustForIssue returns null at standard trust (source-trust.ts:167), so ...(sourceTrust ? { sourceTrust } : {}) left the system stamp in place while update() refreshed updatedAt. An actor write now always restamps provenance rather than inheriting it, clearing to null at standard trust.

Finding 3 — freshness from receipt time productivity-review.ts

Confirmed. Age and newest-PR selection now read the stored GitHub event timestamp, falling back to updatedAt only for rows predating the field. A delivery delayed past 24h now reads stale instead of advertising a dead PR as fresh.

Tests, with negative controls

Each new test was run against the prior logic to confirm it actually fails:

Test	Against prior logic
same-second reopen after close	✗ fails
distinct same-second push	✗ fails
delayed first delivery reads stale	✗ fails
merged stays absorbing	✓ passes both ways — guards over-reach
actor PATCH clears provenance	new route assertion

358/358 across github-webhook, productivity-review-service, issue-agent-mutation-ownership-routes, pull-request-work-products, work-products; server typecheck clean; merged current origin/master (was BEHIND).

One existing assertion in issue-agent-mutation-ownership-routes was updated rather than silenced — it pinned the exact update payload, which now intentionally carries sourceTrust: null.

On Recommended Action 3 (authorship)

Correct that app/allyblockcast cannot approve its own PR — your reviews land as COMMENTED. Flagging one nuance so it is not read as a merge blocker: reviewDecision is empty, i.e. branch protection requires no approving review here, and mergeable: MERGEABLE. So this needs a human merge decision rather than a re-open under a new author. I am not self-approving and not merging. @kkroo — over to you on that call.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:0f4717e important 1 — still-present — server/src/services/work-products.ts:159 — Same-second events are now distinguished from exact redeliveries, but differing payload is not a chronology key. A close delivered after a same-second reopen still wins by lifecycle rank, and reverse-ordered same-second pushes still leave whichever payload serialized last.
• prior:1937536 important 2 — still-present — server/src/services/productivity-review.ts:1200 — The query still chooses one newest trusted PR before applying progress eligibility. A newer closed-unmerged PR therefore masks another fresh ready_for_review or merged PR linked to the same issue.

Critical Issues (0)

Important Issues (3)

• [pr-review-toolkit, gstack/review, native-codex, prior:0f4717e important 1] server/src/services/work-products.ts:159 — Same-second updates still do not converge independently of webhook delivery order. closed then reopened ends open, while the reverse delivery ends closed; equal-rank pushes similarly make arrival order authoritative. Use a causal discriminator or authoritative current-state reconciliation, and add reverse-order same-timestamp close/reopen and push tests.
• [pr-review-toolkit, native-codex, prior:1937536 important 2] server/src/services/productivity-review.ts:1200 — Selecting the newest PR before checking status can suppress a valid progress signal. Query the newest fresh progress-eligible PR separately, while optionally retaining the newest overall PR for display, and add a multi-PR test where a newer closed PR coexists with an older fresh open PR.
• [pr-review-toolkit] server/src/services/productivity-review.ts:110 — Fresh draft PRs are excluded from progress eligibility, so an assignee actively pushing to a draft PR can still be reported as having no concrete progress. The linked issue's acceptance criterion requires an issue with a referenced PR pushed within six hours not to report zero progress and does not exclude drafts. Include draft as progress-eligible or encode and test an explicit product requirement that drafts do not count.

Suggestions (0)

Strengths

• Actor PATCHes now always restamp provenance, clearing system webhook trust at standard trust and closing the actor-mutation laundering path.
• Freshness and newest-row ordering now use GitHub event time rather than delayed receipt time.
• The explicit timestamptz binding restores the webhook upsert path that raw Date interpolation silently disabled.
• Exact redeliveries preserve updatedAt, merged state remains absorbing, and migration 0208 reconciles duplicate external identities before creating the unique index.

Recommended Action

1. Resolve all three Important issues before merge.
2. Add reverse-order same-second and multi-PR masking regression tests.
3. Keep the actor-provenance and source-event-time fixes as implemented.
4. This PR is authored by app/allyblockcast; the App cannot review its own PR. This exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

• prior:0f4717e important 1 — still-present — server/src/services/work-products.ts:173 — The exact-head tie-break now prefers reopened over closed whenever their GitHub timestamps are equal, regardless of their actual source order. A legitimate same-second reopen-then-close therefore remains open and progress-eligible; the new reverse-order test assumes the close is delayed instead of proving that ordering.
• prior:1937536 important 2 — fixed — server/src/services/productivity-review.ts:2212 — Evidence collection now queries a fresh progress-eligible PR separately, so a newer closed-unmerged PR no longer masks another linked open, draft, or merged PR.
• prior:cc83fc9 important 3 — fixed — server/src/services/productivity-review.ts:153 — draft is now explicitly progress-eligible, and the service coverage verifies that a fresh draft PR satisfies the concrete-progress signal.

Critical Issues (0)

Important Issues (3)

• [pr-review-toolkit, gstack/review, native-codex, prior:0f4717e important 1] server/src/services/work-products.ts:173 — Same-second state transitions still cannot converge correctly from the available data. The fixed action ranking makes reopen dominate close in both delivery orders, so a real reopen followed by close in the same second is retained as ready_for_review and can manufacture positive productivity evidence. Reconcile ambiguous transitions against authoritative GitHub state or persist a causal ordering discriminator, and test both true source orders rather than labeling one delivery stale.
• [pr-review-toolkit, gstack/review, native-codex] packages/db/src/issue-work-products-external-identity-migration.test.ts:20 — The new migration test hashes 0208_issue_work_products_external_identity.sql, but this PR adds 0211_issue_work_products_external_identity.sql and no 0208 file exists. The test fails before exercising duplicate reconciliation; the migration also writes the stale 0208 name into dedupedByMigration. Use the 0211 identifier consistently and run the embedded-Postgres test against the migration that will ship.
• [gstack/review, native-codex] server/src/services/work-products.ts:182 — A pre-existing actor-created row can permanently block signed webhook evidence for the same (issue, repo, PR) identity. When that row has no source timestamp and is already ready_for_review, an incoming opened/synchronize event has the same fallback ranks, so tieBreak returns false and preserves the untrusted row; productivity accounting then ignores the real PR. Prefer a system-authenticated webhook row over a conflicting row without immutable webhook provenance, and add a referenced-identifier manual-row-then-webhook regression test.

Suggestions (0)

Strengths

• The separate progress-eligible query fixes multi-PR masking, and draft PRs now match the stated acceptance criterion.
• GitHub event time, rather than receipt time, drives freshness, preventing delayed deliveries from creating a fresh 24-hour window.
• Actor mutations clear webhook trust, and exact redeliveries avoid refreshing liveness.
• The partial unique index and atomic conflict target address concurrent first inserts once the migration is applied.

Recommended Action

1. Fix all three Important issues before merge.
2. Add exact regression coverage for both same-second source orders and the actor-row collision path.
3. This PR is authored by app/allyblockcast; the App cannot review its own PR. Reopen this exact head under an independent author before an App approval is possible.