From b34447ecdc21251c4cd9bdfab22f904e5b8664ed Mon Sep 17 00:00:00 2001 From: Saffron <263493777+itsmiso-ai@users.noreply.github.com> Date: Thu, 3 Sep 2026 06:15:15 +0000 Subject: [PATCH] fix(issue-claim): stop unclaim from overwriting GitHub labels from the stale local cache releaseIssueClaim computed the new label set from the cached row and wrote the whole set to GitHub via updateIssueLabels, which replaces the label set. When the cache was behind GitHub, unclaiming silently reverted every label change made on GitHub since the last sync (e.g. restoring status/backlog and a blocked/* label that had been cleared). Prefer the targeted primitive: remove the agent label with removeIssueLabel and let status transitions go through transitionIssueStatus, which already uses add/remove primitives. Every other label is left untouched, so a stale cache cannot revert it. stale-work.ts inherits the fix since it calls releaseIssueClaim. Tests updated to assert targeted writes and that updateIssueLabels is never called, plus a regression test for the stale-cache scenario. Fixes #921 Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com> --- src/app/api/issues/unclaim/route.test.ts | 68 ++++++++++++++++-------- src/lib/issue-claim.ts | 10 ++-- 2 files changed, 51 insertions(+), 27 deletions(-) diff --git a/src/app/api/issues/unclaim/route.test.ts b/src/app/api/issues/unclaim/route.test.ts index 45853efb..dbe0a15b 100644 --- a/src/app/api/issues/unclaim/route.test.ts +++ b/src/app/api/issues/unclaim/route.test.ts @@ -244,16 +244,10 @@ describe("POST /api/issues/unclaim — operator path", () => { const res = await basicAuthRequest(); expect(res.status).toBe(200); - expect(mocks.updateIssueLabels).toHaveBeenCalledWith( - "org/repo", - 42, - expect.arrayContaining(["status/ready"]), - ); - expect(mocks.updateIssueLabels).toHaveBeenCalledWith( - "org/repo", - 42, - expect.not.arrayContaining(["status/in-progress"]), - ); + // Status transition uses targeted add/remove, not a full-set write. + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, "status/in-progress"); + expect(mocks.addIssueLabel).toHaveBeenCalledWith("org/repo", 42, "status/ready"); + expect(mocks.updateIssueLabels).not.toHaveBeenCalled(); expect(mocks.updateIssue).toHaveBeenCalledWith({ where: { id: "issue-1" }, data: expect.objectContaining({ @@ -308,9 +302,12 @@ describe("POST /api/issues/unclaim — status handling", () => { expect(body.labels).toEqual(["status/ready"]); expect(body.status).toBe("status/ready"); expect(body.statusNote).toBeNull(); - expect(mocks.updateIssueLabels).toHaveBeenCalledWith("org/repo", 42, [ - "status/ready", - ]); + // Targeted label writes: remove agent label, remove old status, add new status. + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, "agent/test-agent"); + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, "status/in-progress"); + expect(mocks.addIssueLabel).toHaveBeenCalledWith("org/repo", 42, "status/ready"); + // Full-set write must never be used — it would revert labels changed on GitHub. + expect(mocks.updateIssueLabels).not.toHaveBeenCalled(); expect(mocks.updateIssue).toHaveBeenCalledWith({ where: { id: "issue-1" }, data: expect.objectContaining({ labels: ["status/ready"] }), @@ -332,9 +329,10 @@ describe("POST /api/issues/unclaim — status handling", () => { expect(body.labels).toEqual(["status/ready"]); expect(body.status).toBe("status/ready"); expect(body.statusNote).toBeNull(); - expect(mocks.updateIssueLabels).toHaveBeenCalledWith("org/repo", 42, [ - "status/ready", - ]); + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, "agent/test-agent"); + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, "status/blocked"); + expect(mocks.addIssueLabel).toHaveBeenCalledWith("org/repo", 42, "status/ready"); + expect(mocks.updateIssueLabels).not.toHaveBeenCalled(); expect(mocks.updateIssue).toHaveBeenCalledWith({ where: { id: "issue-1" }, data: expect.objectContaining({ labels: ["status/ready"] }), @@ -356,9 +354,10 @@ describe("POST /api/issues/unclaim — status handling", () => { expect(body.labels).toEqual(["status/blocked"]); expect(body.status).toBe("status/blocked"); expect(body.statusNote).toContain("blockedReason is set"); - expect(mocks.updateIssueLabels).toHaveBeenCalledWith("org/repo", 42, [ - "status/blocked", - ]); + // No status transition — only the agent label is removed. + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, "agent/test-agent"); + expect(mocks.addIssueLabel).not.toHaveBeenCalled(); + expect(mocks.updateIssueLabels).not.toHaveBeenCalled(); expect(mocks.updateIssue).toHaveBeenCalledWith({ where: { id: "issue-1" }, data: expect.objectContaining({ labels: ["status/blocked"] }), @@ -385,9 +384,10 @@ describe("POST /api/issues/unclaim — status handling", () => { expect(body.status).toBe("status/in-review"); expect(body.statusNote).toContain("PR #7 is still open"); expect(mocks.fetchPullRequestState).toHaveBeenCalledWith("org/repo", 7); - expect(mocks.updateIssueLabels).toHaveBeenCalledWith("org/repo", 42, [ - "status/in-review", - ]); + // No status transition — only the agent label is removed. + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, "agent/test-agent"); + expect(mocks.addIssueLabel).not.toHaveBeenCalled(); + expect(mocks.updateIssueLabels).not.toHaveBeenCalled(); expect(mocks.updateIssue).toHaveBeenCalledWith({ where: { id: "issue-1" }, data: expect.objectContaining({ labels: ["status/in-review"] }), @@ -474,7 +474,7 @@ describe("POST /api/issues/unclaim — guards", () => { }); it("writes failure audit log when GitHub API fails", async () => { - mocks.updateIssueLabels.mockRejectedValueOnce(new Error("github 500")); + mocks.removeIssueLabel.mockRejectedValueOnce(new Error("github 500")); const res = await postRequest(); expect(res.status).toBe(500); @@ -500,6 +500,8 @@ describe("POST /api/issues/unclaim — guards", () => { const res = await postRequest(); expect(res.status).toBe(200); + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, "agent/test-agent"); + expect(mocks.updateIssueLabels).not.toHaveBeenCalled(); expect(mocks.updateIssue).toHaveBeenCalledWith({ where: { id: "issue-1" }, data: expect.objectContaining({ @@ -507,4 +509,24 @@ describe("POST /api/issues/unclaim — guards", () => { }), }); }); + + it("does not revert labels changed on GitHub since the last sync (stale cache)", async () => { + // Cache is behind GitHub: status/ready was already cleared on GitHub and + // a blocked/* label was added there. A full-set write from this stale row + // would restore status/ready and delete the blocked label. + mocks.findUnique.mockResolvedValueOnce({ + id: "issue-1", + state: "open", + labels: ["agent/test-agent", "status/ready"], + } as never); + + const res = await postRequest(); + expect(res.status).toBe(200); + + // Only the agent label is removed; no full-set write touches any other label. + expect(mocks.removeIssueLabel).toHaveBeenCalledTimes(1); + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, "agent/test-agent"); + expect(mocks.addIssueLabel).not.toHaveBeenCalled(); + expect(mocks.updateIssueLabels).not.toHaveBeenCalled(); + }); }); \ No newline at end of file diff --git a/src/lib/issue-claim.ts b/src/lib/issue-claim.ts index b4bdcb00..41d06f82 100644 --- a/src/lib/issue-claim.ts +++ b/src/lib/issue-claim.ts @@ -1,5 +1,5 @@ import { AGENT_PREFIX, getAgentFromLabels } from "@/types"; -import { removeIssueLabel, updateIssueLabels } from "@/lib/github"; +import { removeIssueLabel } from "@/lib/github"; import { transitionIssueStatus } from "@/lib/issue-status"; import { fetchPullRequestState } from "@/lib/github-prs"; @@ -151,9 +151,11 @@ export async function releaseIssueClaim(params: { } } - // Preserve the established full-label update for operator unclaims, then - // defensively DELETE the agent label in case the cache was stale. - await updateIssueLabels(repoFullName, issueNumber, updatedLabels); + // Only targeted label writes reach GitHub here: the agent label is removed + // directly and any status transition goes through transitionIssueStatus, + // which itself uses add/remove primitives. Never replace the whole label + // set — the cached row can be behind GitHub, and a full-set write would + // silently revert every label change made on GitHub since the last sync. await removeIssueLabel(repoFullName, issueNumber, agentLabel); await prisma.issue.update({