From a7aed3e53ec59e427fbbe5bc245ae3d7ac3fe7e1 Mon Sep 17 00:00:00 2001 From: Paperclip CTO Date: Sun, 2 Aug 2026 16:45:15 +0000 Subject: [PATCH] fix(authz): refuse delegate-recovery unpark when blockers are unresolved (BLO-20385) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `isCreatorOrManagerChainRecoveryPatch` gates the blocked -> todo delegate recovery PATCH purely on the request body shape, which mandates `blockedByIssueIds: []`. That empty array is then applied — so admitting the bypass on an issue whose blockers are still live did not merely unpark it, it silently deleted dependency edges the actor had no other way to remove, and returned 200 with no indication it had happened. Probed in production on BLO-18946 (unresolvedBlockerCount 1, live edge to BLO-17770, itself blocked): the PATCH returned 200 and left blockedBy empty. Edge restored via the #870 coordination path. Gate the admit on dependency readiness. Blockers that are all terminal still clear — that is the intended use — but any unresolved blocker now yields 409 `delegate_recovery_unresolved_blockers` naming the offending ids, and no write reaches the service. Deliberately not another opaque boundary 403: an unexplained deny on this exact path already cost a full diagnostic cycle. Scoped to the authorization admit in assertAgentIssueMutationAllowed. The shape check at the write-time concurrency guard is unchanged, as is the in_progress 409 guard and the #870 coordination-metadata allowlist. Co-Authored-By: Claude --- ...ue-agent-mutation-ownership-routes.test.ts | 70 +++++++++++++++++++ server/src/routes/issues.ts | 24 +++++++ 2 files changed, 94 insertions(+) diff --git a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts index 4e360db3ce7..b6244ae1d0d 100644 --- a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts +++ b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts @@ -2878,6 +2878,76 @@ describe("agent issue mutation checkout ownership", () => { expect(mockIssueService.update).not.toHaveBeenCalled(); }); + // BLO-20385: the patch shape mandates `blockedByIssueIds: []` and that empty + // array is applied, so admitting it on an issue with live blockers deletes + // dependency edges the actor could not otherwise remove. Probed in production + // on BLO-18946: a 200 silently dropped a live edge to a still-blocked issue. + it.each(commentGrantMutationDenialCases)( + "refuses the delegate recovery patch when blockers are unresolved, for a %s comment grant holder", + async (_kind, agentRows, issueOverrides) => { + useProductionIssueAuthorization(agentRows); + mockIssueService.getById.mockResolvedValue( + makeIssue({ status: "blocked", assigneeAgentId: ownerAgentId, ...issueOverrides }), + ); + const liveBlockerId = "11111111-1111-4111-8111-111111111111"; + mockIssueService.getDependencyReadiness.mockResolvedValue({ + issueId, + blockerIssueIds: [liveBlockerId], + unresolvedBlockerCount: 1, + unresolvedBlockerIssueIds: [liveBlockerId], + pendingFinalizeBlockerIssueIds: [], + allBlockersDone: false, + isDependencyReady: false, + }); + + const res = await request(await createApp(peerActor())) + .patch(`/api/issues/${issueId}`) + .send({ status: "todo", blockedByIssueIds: [] }); + + expect(res.status, JSON.stringify(res.body)).toBe(409); + expect(res.body.details).toMatchObject({ + reason: "delegate_recovery_unresolved_blockers", + unresolvedBlockerCount: 1, + unresolvedBlockerIssueIds: [liveBlockerId], + }); + // The edge must survive: no write may reach the service at all. + expect(mockIssueService.update).not.toHaveBeenCalled(); + }, + ); + + it.each(commentGrantMutationDenialCases)( + "still unparks past stale terminal blocker edges for a %s comment grant holder", + async (_kind, agentRows, issueOverrides) => { + useProductionIssueAuthorization(agentRows); + const stored = makeIssue({ status: "blocked", assigneeAgentId: ownerAgentId, ...issueOverrides }); + mockIssueService.getById.mockResolvedValue(stored); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...stored, + ...patch, + })); + // Edges exist but every one is terminal: clearing these is the whole point + // of the recovery patch and must keep working. + mockIssueService.getDependencyReadiness.mockResolvedValue({ + issueId, + blockerIssueIds: ["22222222-2222-4222-8222-222222222222"], + unresolvedBlockerCount: 0, + unresolvedBlockerIssueIds: [], + pendingFinalizeBlockerIssueIds: [], + allBlockersDone: true, + isDependencyReady: true, + }); + + const res = await request(await createApp(peerActor())) + .patch(`/api/issues/${issueId}`) + .send({ status: "todo", blockedByIssueIds: [] }); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + const [, patch] = mockIssueService.update.mock.calls.at(-1) as [string, Record]; + expect(patch).toMatchObject({ status: "todo", expectedCurrentStatus: "blocked" }); + expect(patch.blockedByIssueIds).toEqual([]); + }, + ); + it("surfaces 409 when the issue stops being blocked before the delegate recovery write lands", async () => { useProductionIssueAuthorization([ makeAgent(peerAgentId), diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index b5e144e45dd..c20c97eb289 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -4506,6 +4506,30 @@ export function issueRoutes( creatorOrManagerChainDecision && isCreatorOrManagerChainRecoveryPatch(issue, req.body as Record) ) { + // BLO-20385: the patch shape *mandates* `blockedByIssueIds: []`, and that + // empty array is then applied — so admitting the bypass on an issue that + // still has live blockers does not merely unpark it, it silently deletes + // dependency edges the actor could not otherwise remove. The shape check + // above only inspects the request body; it never looked at the issue's + // actual blockers. Unparking a row whose blockers are all terminal is the + // intended use and stays allowed (clearing those stale edges is the + // point). Refuse when any blocker is unresolved, and say so explicitly + // rather than reusing the opaque boundary 403 — an unexplained deny on + // this path already cost a full diagnostic cycle once. + const readiness = await svc.getDependencyReadiness(issue.id); + if (readiness.unresolvedBlockerCount > 0) { + res.status(409).json({ + error: + "Cannot unpark an issue that still has unresolved blockers: this patch shape clears blockedByIssueIds and would delete live dependency edges", + details: { + issueId: issue.id, + reason: "delegate_recovery_unresolved_blockers", + unresolvedBlockerCount: readiness.unresolvedBlockerCount, + unresolvedBlockerIssueIds: readiness.unresolvedBlockerIssueIds, + }, + }); + return false; + } return true; } if (creatorOrManagerChainDecision && !options.allowCreatorOrManagerChainOwnership) {