mirror of
https://github.com/alkimake/paperclip.git
synced 2026-06-14 01:50:39 +09:00
fix execution policy decision persistence
This commit is contained in:
parent
a0333f3e9d
commit
bce58d353d
9 changed files with 160 additions and 34 deletions
|
|
@ -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<unknown>) => 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<string, unknown>, 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<string, any>;
|
||||
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",
|
||||
}),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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 =
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
};
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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<typeof issues.$inferSelect>) => 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<typeof issues.$inferSelect>) => 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) =>
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue