fix(issues): stop a stale run lock from authorizing irreversible deletes (BLO-29150)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Issue mutation on the REST surface is gated by one shared helper, assertAgentIssueMutationAllowed, which ~25 routes call — including the irreversible ones
• That helper short-circuits on isCurrentIssueExecutionRun, which compares only checkoutRunId/executionRunId against the actor's run and never looks at the assignee, so it answers "is this run executing this row" and is read as "may this run do anything to this row"
• The single-assignee checkout invariant means "A holds the lock, B is the assignee" is produced by ordinary operation, not abuse: the heartbeat's reassignment lock-release deliberately leaves a running holder's lock in place, and two recovery paths strand it outright
• Combine the two and a lock left stale by a reassignment authorized a hard delete of an issue now owned by another agent, plus deletion of its attachment objects from storage — before any change was made, on master
• This pull request denies a bare run lock as authority on the three routes whose effect is irreversible, route-locally, without touching the short-circuit the other ~24 routes depend on
• The benefit is that authorization computed at checkout time can no longer outlive the assignment it was computed against, for the operations that cannot be undone

Linked Issues or Issue Description

Refs BLO-29150 (Paperclip board — https://paperclip.blockcast.net/BLO/issues/BLO-29150)

Found while asserting BLO-27356's AC-2 ("DELETE /issues/:id and the other routes sharing assertAgentIssueMutationAllowed show no behaviour change — asserted, not assumed"). BLO-27356 was filed on the premise that fixing release would "grant lock-holders delete authority as a side effect — a strictly worse bug than the one being fixed." That side effect already existed on master and required no widening to reach.

Measured matrix before this change (DELETE /api/issues/:id, agent A, embedded-Postgres route harness):

A holds lock	A is assignee	result	row survives
yes	no	200 — deleted	no
no	no	403 (grant boundary)	yes
yes	yes	200 — deleted (expected)	yes→no
no	yes	409 run ownership conflict	yes

The only difference between row 1 and the row-2 control is the checkout lock. The lock was the grant. Note the inversion: the assignee without the lock is refused, while a non-assignee with a stale lock succeeded.

This is a TOCTOU staleness, not escalation from nothing — since #1353 acquiring the lock requires clearing issue:mutate at checkout time, so the holder was authorized when it checked out. The defect is that this authorization was still honoured for an irreversible delete after the row had been reassigned away.

Prior art: #1353 (merged) made acquiring the lock require clearing issue:mutate at checkout time. No open or merged PR addresses the post-reassignment case; nearest neighbours reviewed were #1054, #911 and #1124 (stale-lock lifecycle, not delete authority).

It also contradicted the helper's own documented intent, which names DELETE /issues/:id twice as the route that must fail closed (issues.ts comments at the allowCreatorOrManagerChainOwnership option and the creator/manager-chain deny). The short-circuit sat above both guards and defeated them.

What Changed

• Added requireAssignmentForRunLockAuthority to assertAgentIssueMutationAllowed. When set, the isCurrentIssueExecutionRun short-circuit additionally requires that the lock holder still be the issue's assignee (or the row be unassigned).
• The guard falls through to the ordinary checks rather than denying, so an actor with real authority over the row — a tasks:manage_active_checkouts override, an unassigned row — is still allowed by the paths below. Returning false there would have denied a manager that legitimately clears the boundary.
• Opted in the three routes whose effect is irreversible:
  • DELETE /issues/:id — svc.remove is a hard delete, and the route then deletes every attachment object from storage.
  • DELETE /attachments/:attachmentId — calls storage.deleteObject before removing the row.
  • DELETE /work-products/:id — workProductsSvc.remove is a hard db.delete(issueWorkProducts).
• Left the shared short-circuit itself untouched for the other ~24 callers, so a lock holder can still conclude its own in-flight work (release, document upsert, PATCH of its own row).
• Added a BLO-29150 describe block asserting the full lock×assignee matrix plus the sweep cases.

Audit of the other destructive routes on this helper

The issue asked for this explicitly rather than leaving it implicit. All 24 call sites reviewed; the destructive ones:

route	reachable with a stale lock	effect	opted in?
DELETE /issues/:id	yes	hard delete + storage.deleteObject per attachment	yes — the reported defect
DELETE /attachments/:attachmentId	yes	storage.deleteObject before the row, then removeAttachment hard-deletes both issueAttachments and the shared assets row (services/issues.ts:11825, deletes at :11851-11852). Storage failures are only logger.warn-ed, so the blob can be destroyed even when the DB step 404s	yes
DELETE /work-products/:id	yes	bare db.delete(issueWorkProducts) (services/work-products.ts:282) — no tombstone, no revision history	yes
DELETE /issues/:id/comments/:commentId	yes	?mode=cancel / queued branch is a hard tx.delete(issueComments) (services/issues.ts:11511); the default branch tombstones. Both branches are gated route-locally by actorOwnsComment, so a stale lock holder can only destroy a comment it authored	no — author-gated
DELETE /issues/:id/approvals/:approvalId	yes	hard db.delete(issueApprovals) (services/issue-approvals.ts:128) of the join row — the approval itself survives and the link is re-creatable; also gated by assertCanManageIssueApprovalLinks	no — re-creatable
DELETE /issues/:id/watchdog	yes	disable, re-upsertable	no — reversible
PUT /issues/:id/documents/:key	yes	new revision, history retained	no — recoverable

Corrections to my own first pass, from the deeper audit on BLO-29356 (which I found after opening this PR and then verified line-by-line at source):

• I had described the comment route as simply "soft (tombstoneComment)". That is true only of the default branch — the queued/?mode=cancel branch is a hard tx.delete. It stays out of scope because of actorOwnsComment, not because it is soft, and my original reason was wrong.
• I had described the attachment route as deleting "the storage object before the row". It also hard-deletes the shared assets row, not just the join. Same conclusion (opt in), understated severity.
• I had called approval-unlink "reversible" without noting it is a physical db.delete of the join row.

BLO-29356 also records two scope corrections worth carrying: the document delete/lock family does not route through this helper at all (DELETE /issues/:id/documents/:key, POST …/lock, POST …/unlock are all req.actor.type !== "board" → 403), and consequently the helper's own doc comment at the allowCreatorOrManagerChainOwnership option is stale where it claims the helper backs "the document delete/lock paths". I have not touched that comment in this PR — flagging it rather than silently widening the diff.

One finding worth flagging for review: assertDeliverableMutationAllowedByRunContext, which sits next to the helper call on both the attachment and work-product routes, reads like a second gate but is not an ownership check — it filters cheap status-only / planning-only recovery runs by their context snapshot and returns true for an ordinary run. It does not close this hole on its own.

Verification

npx vitest run server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts -t "BLO-29150"
  Tests  9 passed | 238 skipped (247)

The test was confirmed to actually catch the bug. With the fix reverted on the route only (helper option left in place), the two non-assignee cells fail exactly as the production matrix measured — expected 200 to be 403 and expected 200 to be 409, i.e. the row was deleted:

 × refuses the lock holder once the row is assigned to another agent
 × refuses a boundary-clearing lock holder that is not the assignee
 Tests  2 failed | 4 passed

Regression scope:

npx vitest run server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts \
                server/src/__tests__/issue-stale-execution-lock-routes.test.ts
  Test Files  2 passed (2)
       Tests  297 passed (297)

npx vitest run server/src/__tests__/issue-attachment-routes.test.ts \
  server/src/__tests__/issue-watchdogs-routes.test.ts \
  server/src/__tests__/inbox-archive-routes.test.ts \
  server/src/__tests__/issue-comment-cancel-routes.test.ts \
  server/src/__tests__/issue-execution-policy-routes.test.ts \
  server/src/__tests__/low-trust-red-team-routes.test.ts \
  server/src/__tests__/done-gate-durable-artifact.test.ts
  Test Files  7 passed (7)

npx tsc -p server/tsconfig.json --noEmit    # exit 0

Two notes on how the tests are built, both deliberate:

1. They go through the express harness, not the service. The defect lives in the route-layer authorization helper; a service-level test would pass while the hole stayed open. That is the "tests that pass while missing the real failure mode" trap for this specific bug.
2. They assert the matrix, not the fixed cell. The short-circuit is shared by ~25 routes, so a later change there can flip a different cell. A test pinning only the non-assignee case would stay green while the legitimate owner silently lost the ability to delete its own issue. Both the holds lock + is assignee → 200 and unassigned row → 200 cells are pinned for that reason, as is watchdog delete → 200 so that a future blanket application of the new option trips a test.

There is also a deliberately sharper case than the reported one: an actor that does clear the issue:mutate boundary (company-wide grant) still must not delete a row assigned elsewhere on the strength of the lock. Without that cell, "fix it by leaning on the boundary" would have looked sufficient.

Risks

• Behavioural change, narrow by construction. The new option defaults false; only the three named routes pass it. Every other caller is byte-identical.
• A previously-succeeding call now fails on the three delete routes when the caller holds only a stale lock and is not the assignee. That is the intended fix, and the response is a normal 403/409 the caller already handles — but if some automation relied on deleting a reassigned row via a lock, it will now be refused. No such caller was found in-tree.
• Unassigned rows are unchanged on purpose. assigneeAgentId === null still short-circuits, so behaviour there is exactly as before. This is the agents.remove strand case; it is not a live hole because the surviving lock names a run belonging to a deleted agent, which cannot authenticate. Tightening it would be a separate, wider change.
• Not fixed here: the three upstream producers of the stale pair — the heartbeat's reassignment lock-release leaving a running holder alone (by design), escalateStaleRunRefire writing assignment through a direct db.update that bypasses svc.update's lock-clearing, and agents.remove leaving both run columns pointed at a deleted agent's runs. This PR makes the stale pair harmless for irreversible deletes rather than preventing it.
• Migration safety: no schema change, no contract change, no UI change.

Model Used

Claude Opus 5 (claude-opus-5, 1M-context variant), extended thinking, agentic tool use (Claude Code via the Paperclip claude_k8s adapter). Fix and tests authored in-session; the pre-fix matrix was reproduced against the route harness and the post-fix behaviour verified by reverting the route opt-in and observing the two cells fail.

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-only
• I have updated relevant documentation to reflect my changes — behaviour is documented in-code at the new option and each opt-in site; no doc/ or command surface changed
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending first run on this branch
• 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-29150
🔗 Paperclip issue: BLO-27356

[allyblockcast]

🔗 Paperclip issue: BLO-29150
🔗 Paperclip issue: BLO-27356

[allyblockcast]

@ally please review at head 8722e21 — this is an authorization change on the shared issue-mutation helper, so please weight these four:

1. Fall-through vs deny. The new requireAssignmentForRunLockAuthority guard falls through to the ordinary checks instead of returning false, so a lock-holding non-assignee can still be authorized further down by hasActiveCheckoutManagementOverride or the unassigned early-return. Is that the right call, or does falling through re-open anything the short-circuit was shielding? I chose it so a legitimate manager is not denied, but it means the deny is decided by code I did not touch.

2. Scope of the sweep. I opted in DELETE /attachments/:id and DELETE /work-products/:id alongside DELETE /issues/:id because all three are irreversible; I deliberately left watchdog-disable, approval-unlink and document-upsert alone as reversible. Disagreement on that line is worth raising — the issue originally asked for the sweep to be a separate change.

3. The unassigned carve-out. assigneeAgentId === null still short-circuits, preserving today's behaviour. I argued it is not a live hole (the surviving lock names a run of a deleted agent, which cannot authenticate). Please check that reasoning rather than the code.

4. Test shape. The matrix is asserted through the express harness, not the service, because a service-level test passes while the route-layer hole stays open. If you see a fifth cell that a future change to the shared helper could flip without failing anything, name it.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. The fix is correct, minimal, and lands the guard at the right altitude — route-local opt-in rather than a change to the shared short-circuit that ~25 callers depend on. I verified the deny actually materialises by tracing the fall-through, and I independently re-ran the destructive-route audit rather than taking the PR table on trust.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

• [comments] server/src/routes/issues.ts:5695 — The new option's JSDoc opens BLO-29150: DELETE /issues/:id only., but three routes pass it (issues.ts:9435, 12055, 14413), and the same docstring's closing sentence ("Opt in from routes whose effect outlives the run holding the lock") contradicts it. This matters more than usual here because the sibling options in this exact block use X only as a load-bearing scope contract — allowExecutionStageParticipantDecision is PATCH /issues/:id only, and allowCoordinationMetadata's comment leans on being single-caller. A later auditor reading only as an invariant finds it already violated at merge.

  • Suggest opening with the general rule and naming the current opt-ins, e.g. Opt-in, currently DELETE /issues/:id, DELETE /attachments/:attachmentId and DELETE /work-products/:id.

• [code] server/src/routes/issues.ts:5751 — The issue.assigneeAgentId === null carve-out is described as "an unassigned row has no assignee to diverge from", but assigneeAgentId === null is also how a row assigned to a human is represented (assigneeUserId set). So for an issue reassigned from agent A to a person, A's stale lock still authorizes the irreversible delete — which is the case the PR's stated invariant ("authorization computed at checkout time can no longer outlive the assignment it was computed against") would most want covered.

  • Pre-existing and not worsened by this PR: tightening this condition alone would change nothing, because the fall-through hits the older unassigned early-return at issues.ts:5875 and returns true regardless. Closing it needs both sites plus a boundary check on user-assigned rows. Worth either a follow-up or one sentence in the carve-out comment saying user-assigned rows are knowingly out of scope, so the next reader doesn't conclude delete authority is now assignment-gated in general.

• [tests] server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts:4361 — The matrix is the right call and four of the nine cells genuinely fail on master, but the two irreversible-route cells assert only the status code (409) where the DELETE /issues/:id cells also pin res.body.details and the error string. Since 409 is reachable from both deny_active_checkout here and assertCheckoutOwner elsewhere in this suite, adding expect(res.body.error).toBe("Issue is checked out by another agent") to the attachment and work-product cells would pin why they were refused, matching the precision of the cells above them.

Strengths

• Falling through instead of return false is the subtle, correct choice, and the comment explains why. return false would have denied a tasks:manage_active_checkouts manager that clears the boundary on its own authority — I confirmed the fall-through reaches hasActiveCheckoutManagementOverride (issues.ts:5885) and that path still works.
• The destructive-route audit holds up. I re-derived it from the router table rather than the PR body: of the ten router.delete handlers, the three opted in are exactly the ones that are irreversible and agent-reachable. DELETE /issues/:id/documents/:key (issues.ts:9092) does hard-delete via documentsSvc.deleteIssueDocument, but it is board-only (403 at issues.ts:9096), so it is correctly excluded — and the two storage.deleteObject call sites in the file (12070, 14421) are both behind the new guard.
• The observation that assertDeliverableMutationAllowedByRunContext is not an ownership gate is accurate and worth having written down at the call site — it sits immediately below the helper on both deliverable routes and reads like a second lock.
• Test-mock reasoning is sound: the permissive default mockAccessService.decide allows issue:mutate but not tasks:manage_active_checkouts, so the "boundary-clearing lock holder" cell really does isolate the lock as the only variable rather than passing for the wrong reason.

Recommended Action

1. No Critical or Important issues — nothing blocking merge from this review.
2. Consider the Suggestions opportunistically; the issues.ts:5695 docstring is a one-line fix and the cheapest of the three.
3. Note that CI was still in flight when this review was written — General tests (server 1–4/4), Typecheck + Release Registry and Build were all pending. This review is a read of the code, not a substitute for those going green; the new tests are mock-based, so the assertCheckoutOwner/decideIssueAccess interactions they stand in for are only proven by the suite actually running.