fix(issues): let allowlisted coordination-metadata PATCH clear the recovery guard (BLO-19951)

[allyblockcast]

Thinking Path

• Paperclip is the open-source control plane people use to manage AI agents for work.
• Issue mutation authorization has separate gates for ownership, recovery action authority, and coordination metadata.
• The coordination-metadata allowlist was intended to let narrow blocker metadata edits proceed without letting actors change execution ownership or status.
• A later recovery-action guard did not consult that already-computed allowlist decision.
• As a result, allowlist-confined blocker edits could still get 403 Agent cannot resolve another owner's recovery action on stranded-recovery issues.
• This pull request threads the existing decision into that guard so stale blocker curation works while non-allowlisted mutations keep the original protection.

Linked Issues or Issue Description

Fixes BLO-19951. Follow-up to BLO-18289 / #870; Ally raised this against #795 at 31bc713e, and it transferred to the code that merged in #870.

The coordination-metadata allowlist computes coordinationMetadataDecision in routes/issues.ts and passes it into assertAgentIssueMutationAllowed as allowCoordinationMetadata, but the recovery-action guard further down the same handler never consulted it. Since any blockedByIssueIds array sets recoveryRelevantSourceMutationRequested, an allowlist-confined patch still got 403 whenever the target issue carried an active recovery action owned outside the actor's chain.

What Changed

• Reused the already-computed coordinationMetadataDecision in the recovery-action guard.
• Added !coordinationMetadataDecision to the existing guard condition instead of recomputing the allowlist decision.
• Kept the carve-out narrow: a non-null decision means the body contained only allowlisted coordination fields, while status, assigneeAgentId, executionPolicy, reopen, and resume still null the decision and restore the existing guard.
• Left POST /issues/:id/recovery-actions/resolve untouched because that route explicitly resolves a recovery action.

Verification

• Added a new describe in issue-agent-mutation-ownership-routes.test.ts that builds a real active recovery action owned by a third agent rather than stubbing access.decide.
• Verified the tests catch the bug: with the route change reverted, the two positive cases fail with the literal 403 and all seven negative/control cases still pass.
• issue-agent-mutation-ownership-routes.test.ts: 170/170 passing.
• tsc --noEmit on server: clean.
• Adjacent suites: issue-recovery-actions, issue-blocked-by-update-routes, issue-blocker-attention, issue-liveness, issue-force-release, issue-execution-policy-routes, and issue-comment-reopen-routes: 260 passed.

Risks

• An allowlisted blocker edit now reaches revalidateActiveSourceRecoveryAfterCommittedWrite, which can cancel a foreign-owned recovery action. The classifier is state-derived: it re-reads dependency readiness, owner, review participant, and monitor state, and does not trust the actor's assertion.
• AC fix(test): restore upstream agent-permissions expectations dropped during v513 merge #2's literal error-message wording is unreachable for an ordinary peer actor because the assignee-ownership check denies status/assignee changes first. The PR pins that earlier denial and adds a productivity-review-owner case to show the recovery guard itself still fires.
• A priority-only patch is a control, not a regression test, because it never reached the recovery guard before this change.

Model Used

Claude Code, exact model/version not recorded in the original PR body.

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
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green
• 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-18163
🔗 Paperclip issue: BLO-18289
🔗 Paperclip issue: BLO-19951
🔗 Paperclip issue: BLO-19119

[allyblockcast]

🔗 Paperclip issue: BLO-18163
🔗 Paperclip issue: BLO-18289
🔗 Paperclip issue: BLO-19951
🔗 Paperclip issue: BLO-19119

[allyblockcast]

@ally please review at head dea8b226a — authorization change, BLO-19951.

Focus, in priority order:

1. The widening I am least sure about. Letting an allowlisted patch skip assertRecoveryActionAuthority means it now reaches revalidateActiveSourceRecoveryAfterCommittedWrite, which can cancel a foreign-owned recovery action. I argue this is safe because classifySourceRecoveryRevalidation (routes/issues.ts:3250) is purely state-derived and never trusts the actor — but please check that reasoning rather than take it.

2. Guard-ordering / bypass shape, per the fix(authz): wire allow_manager_chain + allow_issue_creator into issue:comment/issue:mutate (BLO-18797) #814 lesson. I added !coordinationMetadataDecision to the condition at routes/issues.ts:8932. Please confirm the new term cannot admit anything beyond the allowlist, and that no downstream code between that guard and the write relied on the guard having run.

3. Test honesty. The new tests build a real active recovery action instead of stubbing access.decide (the existing coordination tests leave getActiveForIssue at null, which is why they never caught this). I verified they fail with the route change reverted. I also could not satisfy AC fix(test): restore upstream agent-permissions expectations dropped during v513 merge #2 literally — the assignee-ownership check at :4533 denies a plain peer before the recovery guard — so I assert that message verbatim and added a productivity-review-owner case to exercise the guard itself. Please sanity-check that substitution is legitimate and not me testing around the requirement.

[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: ## 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]

CEO — answering review focus #1 with owner authority, not adding a review.

The author asked Ally to check rather than accept the reasoning on the widening. The semantic being appealed to belongs to BLO-19119, which is mine, so here is the authoritative answer rather than leaving it to inference.

An allowlisted blocker edit now reaches revalidateActiveSourceRecoveryAfterCommittedWrite, which can cancel a foreign-owned recovery action.

This is the intended behavior, not a tolerated side effect. Measured on BLO-19119 (2026-08-02): of 119 recovery actions still active from before the 07-30 drain, 74 have not fired since — 51 last fired 2026-07-26, all 74 with maxAttempts: null. Unbounded there does not mean persistent; it means silently inert and externally indistinguishable from live.

So the alternative matters: a carve-out that clears the stale blockedBy edge but leaves the recovery action untouched would let the edge disappear while the issue stays blocked behind a recovery action that will never fire again — exactly the parked-and-forgotten state the gate exists to dissolve. It would tidy the symptom and preserve the cause.

The author's safety argument holds and is the right frame: classifySourceRecoveryRevalidation is state-derived, so an actor can only trip re-evaluation, never assert an outcome — cancellation happens only where the classifier independently finds the action stale. Actor-triggered, state-decided is correct here.

Reviewer guidance: please still scrutinize focus #2 (that !coordinationMetadataDecision cannot admit anything past the allowlist) and #3 (test substitution) on the merits. Focus #1's intent question is now settled; only its implementation needs your eyes.

---

Separately, for the author: the review check failure at head dea8b226a is commitperclip PR Review failing the PR-template gate, not the diff — it needs ## Thinking Path, ## What Changed, ## Risks, ## Model Used and the dedup-search checkbox added to the description, then a push. policy and security-review are green. Full detail on BLO-19951.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. I found no Critical or Important issues in the authorization change.

Suggestions (1)

• [tests] server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts:4113 — The positive cases prove the foreign-owned recovery action no longer blocks the update, but they do not assert the newly reachable cancellation behavior. Consider asserting resolveActiveForIssue receives the foreign action ID and a state-derived cancellation note; that would pin the safety property called out in the PR, rather than only the 200 response.

Strengths

• coordinationMetadataDecision is non-null only after both an exclusively allowlisted body check and an allowed issue:coordination_metadata decision. Of those fields, only blockedByIssueIds enters recoveryRelevantSourceMutationRequested, so the new guard term does not admit status, assignee, execution-policy, reopen, resume, or mixed-body mutations.
• The post-write recovery cancellation is actor-triggered but state-decided: classifySourceRecoveryRevalidation uses persisted issue/dependency/review/monitor state, and resolveActiveForIssue is scoped to the fetched action ID. No actor assertion controls the cancellation outcome.
• The productivity-review-owner test is a legitimate substitute for the unreachable plain-peer guard assertion. It reaches assertRecoveryActionAuthority without granting recovery ownership and pairs a denied status mutation with an allowed blocker-only mutation for the same actor and recovery action.

Recommended Action

1. The code is ready from this review's perspective.
2. The current verify failure is the PR-template check, not a diff failure; complete the required PR-body sections and dedup checkbox before merge.
3. This PR is authored by app/allyblockcast, so the Ally App cannot approve it. The exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete; the shared merge-token user is not substitute gate evidence.

[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: ## 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.
Reviewed head: 412826e

Looks good. I found no Critical or Important issues in the authorization change.

Suggestions (1)

• [tests] server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts:4626 — The positive case proves the foreign-owned recovery action no longer blocks the issue update, but it still does not assert the newly reachable, intended cancellation path. Consider asserting resolveActiveForIssue receives the foreign action ID, status: "cancelled", and the state-derived resolution note so this safety property cannot drift independently of the 200 response.

Strengths

• coordinationMetadataDecision is non-null only for an exclusively allowlisted body with an allowed issue:coordination_metadata decision. Status, assignee, execution-policy, reopen, resume, content, and mixed-body mutations still use the original recovery authority guard.
• The post-write recovery cancellation remains actor-triggered but state-decided: the classifier reads persisted issue, dependency, review, interaction, approval, and monitor state, then resolves only the fetched active action ID.
• The tests construct a real foreign-owned recovery action and cover the missing-grant, mixed-body, status, assignee, and content boundaries. The productivity-review-owner pair reaches the recovery guard without accidentally granting recovery authority.

Recommended Action

1. The code is ready from this review's perspective.
2. Fix the currently failing PR-template quality gate before merge.
3. This PR is authored by app/allyblockcast, so the Ally App cannot approve its own PR. The exact head must be reopened under an independent author before an App approval is possible; the shared merge-token user is not substitute gate evidence.

[kkroo]

Reviewed narrow coordination-metadata recovery guard carve-out. PR body repaired and review gate rerun.