From bce58d353d62547d03df06628f059f3daf5e001b Mon Sep 17 00:00:00 2001 From: dotta Date: Tue, 7 Apr 2026 17:07:10 -0500 Subject: [PATCH] fix execution policy decision persistence --- docs/guides/execution-policy.md | 2 +- packages/shared/src/types/issue.ts | 2 +- packages/shared/src/validators/issue.ts | 2 +- .../issue-comment-reopen-routes.test.ts | 84 ++++++++++++++++++- .../__tests__/issue-execution-policy.test.ts | 14 ++++ server/src/routes/issues.ts | 68 ++++++++++----- server/src/services/issue-execution-policy.ts | 3 +- server/src/services/issues.ts | 13 +-- ui/src/lib/issue-execution-policy.ts | 6 +- 9 files changed, 160 insertions(+), 34 deletions(-) diff --git a/docs/guides/execution-policy.md b/docs/guides/execution-policy.md index f281119e..f12af1cb 100644 --- a/docs/guides/execution-policy.md +++ b/docs/guides/execution-policy.md @@ -28,7 +28,7 @@ interface IssueExecutionPolicy { interface IssueExecutionStage { id: string; // auto-generated UUID type: "review" | "approval"; // stage kind - approvalsNeeded: number; // currently always 1 + approvalsNeeded: 1; // multi-approval is not supported yet participants: IssueExecutionStageParticipant[]; } diff --git a/packages/shared/src/types/issue.ts b/packages/shared/src/types/issue.ts index 9c408022..06608102 100644 --- a/packages/shared/src/types/issue.ts +++ b/packages/shared/src/types/issue.ts @@ -136,7 +136,7 @@ export interface IssueExecutionStageParticipant extends IssueExecutionStagePrinc export interface IssueExecutionStage { id: string; type: IssueExecutionStageType; - approvalsNeeded: number; + approvalsNeeded: 1; participants: IssueExecutionStageParticipant[]; } diff --git a/packages/shared/src/validators/issue.ts b/packages/shared/src/validators/issue.ts index 055591ff..63b070a6 100644 --- a/packages/shared/src/validators/issue.ts +++ b/packages/shared/src/validators/issue.ts @@ -91,7 +91,7 @@ export const issueExecutionStageParticipantSchema = issueExecutionStagePrincipal export const issueExecutionStageSchema = z.object({ id: z.string().uuid().optional(), type: z.enum(ISSUE_EXECUTION_STAGE_TYPES), - approvalsNeeded: z.number().int().positive().optional().default(1), + approvalsNeeded: z.literal(1).optional().default(1), participants: z.array(issueExecutionStageParticipantSchema).default([]), }); diff --git a/server/src/__tests__/issue-comment-reopen-routes.test.ts b/server/src/__tests__/issue-comment-reopen-routes.test.ts index 17e184ff..2594e36e 100644 --- a/server/src/__tests__/issue-comment-reopen-routes.test.ts +++ b/server/src/__tests__/issue-comment-reopen-routes.test.ts @@ -3,12 +3,15 @@ import request from "supertest"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { issueRoutes } from "../routes/issues.js"; import { errorHandler } from "../middleware/index.js"; +import { normalizeIssueExecutionPolicy } from "../services/issue-execution-policy.ts"; const mockIssueService = vi.hoisted(() => ({ getById: vi.fn(), update: vi.fn(), addComment: vi.fn(), findMentionedAgents: vi.fn(), + listWakeableBlockedDependents: vi.fn(), + getWakeableParentAfterChildCompletion: vi.fn(), })); const mockAccessService = vi.hoisted(() => ({ @@ -29,6 +32,14 @@ const mockAgentService = vi.hoisted(() => ({ })); const mockLogActivity = vi.hoisted(() => vi.fn(async () => undefined)); +const mockTxInsertValues = vi.hoisted(() => vi.fn(async () => undefined)); +const mockTxInsert = vi.hoisted(() => vi.fn(() => ({ values: mockTxInsertValues }))); +const mockTx = vi.hoisted(() => ({ + insert: mockTxInsert, +})); +const mockDb = vi.hoisted(() => ({ + transaction: vi.fn(async (fn: (tx: typeof mockTx) => Promise) => fn(mockTx)), +})); vi.mock("../services/index.js", () => ({ accessService: () => mockAccessService, @@ -74,7 +85,7 @@ function createApp() { }; next(); }); - app.use("/api", issueRoutes({} as any, {} as any)); + app.use("/api", issueRoutes(mockDb as any, {} as any)); app.use(errorHandler); return app; } @@ -106,6 +117,8 @@ describe("issue comment reopen routes", () => { authorUserId: "local-board", }); mockIssueService.findMentionedAgents.mockResolvedValue([]); + mockIssueService.listWakeableBlockedDependents.mockResolvedValue([]); + mockIssueService.getWakeableParentAfterChildCompletion.mockResolvedValue(null); }); it("treats reopen=true as a no-op when the issue is already open", async () => { @@ -212,4 +225,73 @@ describe("issue comment reopen routes", () => { }), ); }); + + it("writes decision ids into executionState and inserts the decision inside the transaction", async () => { + const policy = normalizeIssueExecutionPolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "approval", + participants: [{ type: "user", userId: "local-board" }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_review", + assigneeAgentId: null, + assigneeUserId: "local-board", + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "approval", + currentParticipant: { type: "user", userId: "local-board" }, + returnAssignee: { type: "agent", agentId: "22222222-2222-4222-8222-222222222222" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record, tx?: unknown) => ({ + ...issue, + ...patch, + executionState: patch.executionState, + status: "done", + completedAt: new Date(), + updatedAt: new Date(), + _tx: tx, + })); + + const res = await request(createApp()) + .patch("/api/issues/11111111-1111-4111-8111-111111111111") + .send({ status: "done", comment: "Approved for ship" }); + + expect(res.status).toBe(200); + expect(mockDb.transaction).toHaveBeenCalledTimes(1); + expect(mockIssueService.update).toHaveBeenCalledWith( + "11111111-1111-4111-8111-111111111111", + expect.objectContaining({ + executionState: expect.objectContaining({ + status: "completed", + lastDecisionId: expect.any(String), + lastDecisionOutcome: "approved", + }), + }), + mockTx, + ); + const updatePatch = mockIssueService.update.mock.calls[0]?.[1] as Record; + const decisionId = updatePatch.executionState.lastDecisionId; + expect(mockTxInsert).toHaveBeenCalledTimes(1); + expect(mockTxInsertValues).toHaveBeenCalledWith( + expect.objectContaining({ + id: decisionId, + issueId: "11111111-1111-4111-8111-111111111111", + outcome: "approved", + body: "Approved for ship", + }), + ); + }); }); diff --git a/server/src/__tests__/issue-execution-policy.test.ts b/server/src/__tests__/issue-execution-policy.test.ts index cbaf5798..aedc4305 100644 --- a/server/src/__tests__/issue-execution-policy.test.ts +++ b/server/src/__tests__/issue-execution-policy.test.ts @@ -95,6 +95,20 @@ describe("normalizeIssueExecutionPolicy", () => { expect(result!.mode).toBe("normal"); }); + it("rejects approvalsNeeded values above 1", () => { + expect(() => + normalizeIssueExecutionPolicy({ + stages: [ + { + type: "review", + approvalsNeeded: 2, + participants: [{ type: "agent", agentId: qaAgentId }], + }, + ], + }), + ).toThrow("Invalid execution policy"); + }); + it("throws for invalid input", () => { expect(() => normalizeIssueExecutionPolicy({ stages: [{ type: "invalid_type" }] })).toThrow(); }); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 482639d7..d133e5fe 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -1,3 +1,4 @@ +import { randomUUID } from "node:crypto"; import { Router, type Request, type Response } from "express"; import multer from "multer"; import { z } from "zod"; @@ -1210,15 +1211,57 @@ export function issueRoutes( }, commentBody, }); + const decisionId = transition.decision ? randomUUID() : null; + if (decisionId) { + const nextExecutionState = transition.patch.executionState; + if (!nextExecutionState || typeof nextExecutionState !== "object") { + throw new Error("Execution policy decision patch is missing executionState"); + } + transition.patch.executionState = { + ...nextExecutionState, + lastDecisionId: decisionId, + }; + } Object.assign(updateFields, transition.patch); let issue; try { - issue = await svc.update(id, { - ...updateFields, - actorAgentId: actor.agentId ?? null, - actorUserId: actor.actorType === "user" ? actor.actorId : null, - }); + if (transition.decision && decisionId) { + const decision = transition.decision; + issue = await db.transaction(async (tx) => { + const updated = await svc.update( + id, + { + ...updateFields, + actorAgentId: actor.agentId ?? null, + actorUserId: actor.actorType === "user" ? actor.actorId : null, + }, + tx, + ); + if (!updated) return null; + + await tx.insert(issueExecutionDecisions).values({ + id: decisionId, + companyId: updated.companyId, + issueId: updated.id, + stageId: decision.stageId, + stageType: decision.stageType, + actorAgentId: actor.agentId ?? null, + actorUserId: actor.actorType === "user" ? actor.actorId : null, + outcome: decision.outcome, + body: decision.body, + createdByRunId: actor.runId ?? null, + }); + + return updated; + }); + } else { + issue = await svc.update(id, { + ...updateFields, + actorAgentId: actor.agentId ?? null, + actorUserId: actor.actorType === "user" ? actor.actorId : null, + }); + } } catch (err) { if (err instanceof HttpError && err.status === 422) { logger.warn( @@ -1365,21 +1408,6 @@ export function issueRoutes( }); } - - if (transition.decision) { - await db.insert(issueExecutionDecisions).values({ - companyId: issue.companyId, - issueId: issue.id, - stageId: transition.decision.stageId, - stageType: transition.decision.stageType, - actorAgentId: actor.agentId ?? null, - actorUserId: actor.actorType === "user" ? actor.actorId : null, - outcome: transition.decision.outcome, - body: transition.decision.body, - createdByRunId: actor.runId ?? null, - }); - } - const assigneeChanged = issue.assigneeAgentId !== existing.assigneeAgentId || issue.assigneeUserId !== existing.assigneeUserId; const statusChangedFromBacklog = diff --git a/server/src/services/issue-execution-policy.ts b/server/src/services/issue-execution-policy.ts index de37df6f..86de20e4 100644 --- a/server/src/services/issue-execution-policy.ts +++ b/server/src/services/issue-execution-policy.ts @@ -39,7 +39,6 @@ type TransitionResult = { }; const COMPLETED_STATUS: IssueExecutionState["status"] = "completed"; -const IDLE_STATUS: IssueExecutionState["status"] = "idle"; const PENDING_STATUS: IssueExecutionState["status"] = "pending"; const CHANGES_REQUESTED_STATUS: IssueExecutionState["status"] = "changes_requested"; @@ -74,7 +73,7 @@ export function normalizeIssueExecutionPolicy(input: unknown): IssueExecutionPol return { id: stage.id ?? randomUUID(), type: stage.type, - approvalsNeeded: 1, + approvalsNeeded: 1 as const, participants: dedupedParticipants, }; }) diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 9dd53765..21c340f4 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -1562,12 +1562,13 @@ export function issueService(db: Db) { actorAgentId?: string | null; actorUserId?: string | null; }, + dbOrTx: any = db, ) => { - const existing = await db + const existing = await dbOrTx .select() .from(issues) .where(eq(issues.id, id)) - .then((rows) => rows[0] ?? null); + .then((rows: Array) => rows[0] ?? null); if (!existing) return null; const { @@ -1639,7 +1640,7 @@ export function issueService(db: Db) { patch.checkoutRunId = null; } - return db.transaction(async (tx) => { + const runUpdate = async (tx: any) => { const defaultCompanyGoal = await getDefaultCompanyGoal(tx, existing.companyId); const [currentProjectGoalId, nextProjectGoalId] = await Promise.all([ getProjectDefaultGoalId(tx, existing.companyId, existing.projectId), @@ -1663,7 +1664,7 @@ export function issueService(db: Db) { .set(patch) .where(eq(issues.id, id)) .returning() - .then((rows) => rows[0] ?? null); + .then((rows: Array) => rows[0] ?? null); if (!updated) return null; if (nextLabelIds !== undefined) { await syncIssueLabels(updated.id, existing.companyId, nextLabelIds, tx); @@ -1682,7 +1683,9 @@ export function issueService(db: Db) { } const [enriched] = await withIssueLabels(tx, [updated]); return enriched; - }); + }; + + return dbOrTx === db ? db.transaction(runUpdate) : runUpdate(dbOrTx); }, remove: (id: string) => diff --git a/ui/src/lib/issue-execution-policy.ts b/ui/src/lib/issue-execution-policy.ts index 8db0279a..4f3bf3b5 100644 --- a/ui/src/lib/issue-execution-policy.ts +++ b/ui/src/lib/issue-execution-policy.ts @@ -61,7 +61,7 @@ export function buildExecutionPolicy(input: { approverValues: string[]; }): IssueExecutionPolicy | null { const mode = input.existingPolicy?.mode ?? "normal"; - const stages = []; + const stages: IssueExecutionPolicy["stages"] = []; const existingReviewStage = input.existingPolicy?.stages.find((stage) => stage.type === "review"); const reviewParticipants = mergeParticipants(existingReviewStage?.participants, input.reviewerValues); @@ -69,7 +69,7 @@ export function buildExecutionPolicy(input: { stages.push({ id: existingReviewStage?.id ?? newId(), type: "review" as const, - approvalsNeeded: 1, + approvalsNeeded: 1 as const, participants: reviewParticipants, }); } @@ -80,7 +80,7 @@ export function buildExecutionPolicy(input: { stages.push({ id: existingApprovalStage?.id ?? newId(), type: "approval" as const, - approvalsNeeded: 1, + approvalsNeeded: 1 as const, participants: approvalParticipants, }); }