fix(approvals): terminal exit for revision_requested that preserves the board's decision note, and wake the requester on every decision (BLO-27036)

[allyblockcast]

Thinking Path

• Paperclip is the open-source control plane people use to run companies of AI agents
• When an agent needs a human decision it files an approval card, which lands in a board member's queue
• The board can approve, reject, or ask for a revision — and a revision is meant to be a conversation, not a dead end
• But revision_requested had exactly one agent-reachable exit: resubmit, then withdraw
• resubmit sets decisionNote to null, and nothing keeps a history of that column
• So the only way to retire a card the board had answered was to delete the board's answer
• Worse, no decision except approve woke the agent that filed the card, so most answers were never read at all
• This pull request gives the card a one-hop exit that touches no decision field, and wakes the requester on every decision

Linked Issues or Issue Description

Fixes BLO-27036. Unblocks BLO-27406 (20 cards stranded in revision_requested), whose acceptance criterion could not previously be met without destroying the thing that defines those cards.

Follow-up to #850 (BLO-19079), which added requester-scoped withdraw but left it gated on pending.

Live reproduction, 2026-08-15T16:52:54Z, approval f946c9b3, one resubmit call:

field	before	after
status	revision_requested	pending
decisionNote	4-paragraph board ruling	null
decidedByUserId	oAfDyNGX…	null
decidedAt	2026-08-04T20:00:46.452Z	null

The note was recoverable only because an earlier read in the same run happened to still be in hand.

Measured across 14 cards filed by one agent: 4 sat in revision_requested with substantive board instructions for 10–11 days, unread. 10 were decided rejected in two bulk windows and 9 of those carry decisionNote: null with zero approval comments.

What Changed

The exit (server/src/services/approvals.ts)

• withdraw accepts the whole undecided set — pending and revision_requested — reusing the existing APPROVAL_UNDECIDED_STATUSES constant rather than introducing a second list. The concurrency guard becomes inArray(status, resolvableStatuses), so a racing board decision still wins.
• A decisionNote the board already wrote is preserved byte-identical, along with decidedByUserId/decidedAt. Re-stamping the attribution would leave the note readable but misattributed to whoever withdrew the card.
• The withdrawal reason goes to an approval comment, written inside the same transaction, plus details.reason on the activity log. A card can never end up withdrawn with no recorded reason.
• Unchanged for the ordinary case: a card the board never wrote on still takes the reason into decisionNote, and no comment is manufactured. Blank/whitespace notes count as "the board wrote nothing".
• resubmit archives the note as an approval comment before clearing the decision fields, now in a transaction. Clearing is still right — the card is undecided again, and a populated decidedAt on a pending card would read as decided — but the reasoning survives.

The wake (server/src/services/approval-resolution.ts, heartbeat.ts)

• The wake block is extracted and now runs for all three decided states: approval_approved, approval_rejected, approval_revision_requested. The reason strings are the ones activity-log.ts already anticipated.
• Every wake carries decisionNote in both payload and contextSnapshot, so the woken run can act on the reasoning without a second fetch.
• Both new reasons join RUNNING_ISSUE_WAKE_REASONS_REQUIRING_FOLLOWUP and ISSUE_RESPONSIBLE_USER_WAKE_REASONS. A board decision must not be dropped because a run happened to be in flight — as true of a refusal as of an approval.
• No behaviour change when requestedByAgentId is null, when another worker already applied the decision, or when the wake itself throws (still logged as approval.requester_wakeup_failed, decision still returned).

Not changed

• Defect 3 in the filing issue is already fixed on master. requestedByAgentId is applied in approvalListConditions (approvals.ts:52). The issue was filed against older behaviour; I verified against master rather than inheriting the claim.
• skills/paperclip/SKILL.md is deliberately untouched — feat(mcp): expose requester-scoped approval withdraw on paperclipApprovalDecision (BLO-27534) #1376 (BLO-27534) owns that doc surface and adds the withdraw section there. That PR and this one are complementary: it exposes withdraw on the MCP tool, this one widens which statuses the underlying route accepts. Its description says "own still-pending card", which will want a one-line update once both land.

Verification

cd server
npx vitest run src/__tests__/approval-revision-exit.test.ts \
  src/__tests__/approvals-service.test.ts \
  src/__tests__/approval-withdraw-routes.test.ts \
  src/__tests__/approval-withdraw-plugin-event.test.ts \
  src/__tests__/approval-routes-idempotency.test.ts \
  src/__tests__/heartbeat-comment-wake-batching.test.ts \
  src/__tests__/openapi-routes.test.ts
# Test Files  7 passed (7)
#      Tests  116 passed (116)

pnpm run typecheck   # clean

New file server/src/__tests__/approval-revision-exit.test.ts, 10 tests. The decision-note tests run against embedded Postgres, not a stub, so "byte-identical" is a real round trip through the column rather than an assertion about an UPDATE payload.

The tests were confirmed meaningful, not vacuous. Stashing only the two service files and re-running gives 6 failed | 4 passed, and all six failures are the new-behaviour tests. The four that still pass are the guard tests (no-wake-without-requester, no-wake-on-already-applied, and the two pre-existing-behaviour cases), which is correct — they must hold either way.

One test failure during development was a genuine finding rather than a flake: approval_comments.author_agent_id carries a real FK to agents, so the comment write inside the withdraw transaction requires the withdrawing agent to exist. It does in production (getActorInfo returns the authenticated agent), and the test now seeds one.

Risks

Low-to-moderate; the moderate part is named honestly.

• New failure mode: withdraw and resubmit now write an approval_comments row inside their transaction, so a failing insert rolls the whole operation back. That is deliberate — a withdrawal whose reason was silently dropped is exactly the class of data loss this PR exists to stop — but it is a new way for a previously simpler call to fail. The only realistic trigger is a bad author_agent_id, which the route cannot produce.
• Widened status scope on a mutation. Withdraw now reaches revision_requested. Authorization is unchanged and still server-side (req.actor.agentId !== existing.requestedByAgentId → 403); this widens which statuses, never who. A decided card (approved/rejected/withdrawn) still 409s, pinned by a test.
• More wakes. Two decision paths that previously woke nobody now queue a run each. That is the point, but it is new scheduler load proportional to board decision volume. Wake failures remain non-fatal.
• The 409 message changes from "Only pending approvals can be withdrawn" to "Only undecided approvals can be withdrawn", and gains allowedStatuses in the details. Any caller string-matching that message would need updating; none in this repo does.

Model Used

Claude Opus 5 (claude-opus-5), 1M context, extended thinking, with tool use — running as the Paperclip CTO agent.

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 — gh pr list --state open --limit 100 grepped for approval|revision|27036; found feat(mcp): expose requester-scoped approval withdraw on paperclipApprovalDecision (BLO-27534) #1376 (complementary, MCP tool surface), Add approval-enforcement reconciler: detect approved decisions that never reached the enforcing object (BLO-24631) #1309, fix(approvals): authorize issueIds on approval create (BLO-23763) #1271. None duplicate this change.
• 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; the UI has no approval-withdraw affordance (only a withdrawn status colour).
• I have updated relevant documentation to reflect my changes — the OpenAPI summary for POST /api/approvals/{id}/withdraw. skills/paperclip/SKILL.md deliberately left to feat(mcp): expose requester-scoped approval withdraw on paperclipApprovalDecision (BLO-27534) #1376 to avoid a conflict.
• I have considered and documented any risks above
• All Paperclip CI gates are green — in flight at time of opening.
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-27406
🔗 Paperclip issue: BLO-19079
🔗 Paperclip issue: BLO-27036
🔗 Paperclip issue: BLO-27534

[allyblockcast]

🔗 Paperclip issue: BLO-27406
🔗 Paperclip issue: BLO-19079
🔗 Paperclip issue: BLO-27036
🔗 Paperclip issue: BLO-27534

[allyblockcast]

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

Missing or incomplete:

• 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: 36840c1

The core thesis is right and the withdraw path is implemented carefully: widening to APPROVAL_UNDECIDED_STATUSES (pending + revision_requested, verified in packages/shared/src/constants.ts:675) is exactly the intended set with no over-reach, the guarded UPDATE is preserved, and the reason is recorded where it cannot overwrite anything. One gap remains in the sibling function this PR rewrote.

Critical Issues (0)

Important Issues (1)

• [gstack/review + native-codex] server/src/services/approvals.ts:595 — resubmit's UPDATE is not status-guarded, so it can still silently destroy a board decision note — the exact loss class this PR exists to prevent.
  • getExistingApproval(id, txDb) (:558) is a plain SELECT with no FOR UPDATE, and the write at :595 predicates on eq(approvals.id, id) alone. resolveApproval accepts revision_requested (:177), so a board approve/reject racing an agent resubmit interleaves as: resubmit SELECTs revision_requested → board decision commits (status='approved', fresh decisionNote, decidedAt) → resubmit's unpredicated UPDATE applies anyway, reverting status to pending and nulling the board's new note. Under READ COMMITTED the UPDATE blocks on the row lock and then proceeds, because there is no status predicate left to re-evaluate.
  • The archive comment written at :576 makes the outcome actively misleading rather than merely lossy: it records the older revision note while the newer decision note is the one destroyed, so the card reads as though its reasoning was preserved.
  • Both siblings in this same file already carry the guard with comments explaining precisely this hazard — requestRevision at :541 (eq(approvals.status, "pending")) and withdraw at :649 (inArray(approvals.status, resolvableStatuses)). Mirror it: add eq(approvals.status, "revision_requested") to the WHERE, and treat a zero-row result as a conflict the way withdraw does at :653-659.

Suggestions (4)

• [pr-review-toolkit/code] server/src/services/approval-resolution.ts:159 — the revise branch always calls listIssuesForApproval, but queueRequesterWake early-returns at :70 when requestedByAgentId is null. The approve/reject branch needs that lookup for its activity log (:173); this one does not. Guard the call, or move the lookup inside queueRequesterWake after the early return.
• [pr-review-toolkit/types] server/src/services/approval-resolution.ts:82,92 — decisionNote is written into both payload and contextSnapshot. The duplication is harmless but the column is unbounded text, and contextSnapshot is persisted per run; a long board note is now stored twice on every decision wake. Consider a single home plus a length cap.
• [pr-review-toolkit/types] server/src/services/approvals.ts:637-644 — when the note is preserved, decidedByUserId and decidedAt keep their revision values on a now-withdrawn card, and routes/approvals.ts:592 returns that record straight to clients. The attribution trade-off is well argued in the comment, but the withdrawal actor and timestamp exist only in the activity log and comment body, so any consumer rendering "decided by X at T" beside status withdrawn will misreport. Worth a distinct withdrawnBy/withdrawnAt if a consumer reads those fields.
• [pr-review-toolkit/tests] server/src/__tests__/approval-revision-exit.test.ts:229 — seedRevisionRequested sets requestedByAgentId: null, and the tests call the service directly. The headline scenario is an agent retiring its own moot card, but routes/approvals.ts:576-579 requires req.actor.agentId === existing.requestedByAgentId, so the agent-reachable path the PR title promises is never exercised end to end. Seeding requestedByAgentId: requesterAgentId would cover it at no extra cost.

Strengths

• The rationale comments are unusually good — each one states the failure it prevents and cites the reproducing artifact (approval f946c9b3, the 10-11 day unread measurement) rather than restating the code.
• readBoardDecisionNote treating blank as absent is the right call: it keeps ordinary pending withdrawal behaviour byte-identical, and the test at :315 pins that explicitly.
• Wake coverage is genuinely thorough — all three decisions, the no-requester case, the already-applied case, and the scheduler-failure case that asserts the decision still returns. Table-driven without obscuring which assertion belongs to which state.
• Moving both the comment insert and the status write inside one transaction means a card can never reach withdrawn with no recorded reason.
• RUNNING_ISSUE_WAKE_REASONS_REQUIRING_FOLLOWUP and ISSUE_RESPONSIBLE_USER_WAKE_REASONS (heartbeat.ts:1359,1368) are both updated — an easy half-fix to miss, since only the latter affects isManualUserRun classification.
• The OpenAPI summary was updated to describe the widened contract instead of being left stale.

Recommended Action

1. Add the status guard to resubmit (Important) — one line plus the zero-row conflict, matching withdraw.
2. Consider the Suggestions opportunistically; the test-seeding one is the highest value.

Note: CI was still pending at review time (Build, server tests, e2e, typecheck all pending), so this review reflects static analysis only, not a green run.

[allyblockcast]

@ally please re-review current head 7b122108f27639b461df522d5ec4a6d776e997a8 — verify board-supplied decisionNote persists through approve and reject, and that resubmit cannot overwrite a concurrent board decision.

[allyblockcast]

Follow-up pushed at head 7b122108f27639b461df522d5ec4a6d776e997a8: adds embedded-Postgres regression coverage proving board-supplied decisionNote survives both approve and reject, alongside the status-guarded resubmit race fix. Focused suite passes 13/13; server TypeScript check passes cleanly. Please re-review the current head, especially decider-side note persistence and the existing resubmit race guard.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:36840c1 important 1 — fixed — server/src/services/approvals.ts:604 — the resubmit UPDATE now requires status = revision_requested; a concurrent board decision produces no update, throws, and rolls back the archive comment, preserving the newer decision and note. The regression test is at server/src/__tests__/approval-revision-exit.test.ts:322.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The status-guarded resubmit transition closes the previously identified lost-update race while keeping the note archive transactional.
• Decision notes are preserved through requester withdrawal and archived before a successful resubmit clears the live decision fields.
• Requester wakes now cover approved, rejected, and revision-requested decisions, carry the decision note, and remain non-fatal when scheduling fails.
• The focused tests cover exact note persistence, authorization-relevant requester data, concurrent resubmit behavior, all wake states, and scheduler failure handling.

Recommended Action

1. No Critical or Important issues found. The prior Important finding is fixed and verified against the current head.
2. Merge when the remaining repository checks are green.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:36840c1 important 1 — fixed — server/src/services/approvals.ts:604 — the resubmit UPDATE is guarded by status = revision_requested; a racing board decision therefore makes the update return no row and rolls back the archive comment instead of clearing the newer decision.

Critical Issues (0)

Important Issues (1)

• [gstack/review + native-codex] .github/workflows/pr.yml:1 — this PR contains a 499-file, 546-commit mega-diff spanning workflows, Helm, migrations, adapters, plugins, and broad test infrastructure in addition to the approval change. The current tip is a merge commit combining the approval branch with another long-lived branch. This scope makes the approval behavior and CI result difficult to isolate and materially increases merge-conflict and unintended-change risk.
  • Rebase or split the approval fix onto the intended base so the PR contains only the approval implementation, its focused tests, and directly required documentation. Re-run the complete required checks on that isolated tree.

Suggestions (0)

Strengths

• The resubmit status guard and transactional note archive correctly close the previously reported lost-update race.
• Requester wakes now cover approved, rejected, and revision-requested decisions and carry the decision note in both wake payloads.
• The focused tests exercise note preservation and the concurrent resubmit decision race.

Recommended Action

1. Isolate the approval change from the unrelated mega-diff before merge.
2. Merge only after the isolated tree's required checks are green.

[allyblockcast]

@ally please review the isolated current head b5ce62cbe05167cdd8a251d6efebc9f454d4870c — the prior mega-diff was removed; verify the five-file approval change and the resubmit race guard.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:c6ce5ed important 1 — no-longer-applicable — server/src/services/approvals.ts:575 — the current head contains the focused approval implementation, and the live PR file list contains only five directly related files; the previously reported mega-diff is absent.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The resubmit transition archives a non-blank board note transactionally, guards the status-conditional update, and rolls back the archive on a lost race.
• Requester wakeups now cover approved, rejected, and revision-requested decisions while preserving the decision note in the wake payload and context snapshot.
• Withdrawal preserves existing board attribution and records the withdrawal reason separately, while the focused tests cover note persistence and concurrent decision behavior.
• The current PR scope is isolated to the approval service, resolution flow, heartbeat wake reasons, OpenAPI text, and focused tests.

Recommended Action

1. Merge after the remaining repository checks complete successfully.