From 29391cb36b20173dc3104c929561ddc4f818c766 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Sat, 30 May 2026 02:11:19 -0400 Subject: [PATCH] Fix duplicate orchestration task id rejection --- .../orchestration/manifestNormalization.ts | 20 ++-- .../orchestrationService.test.ts | 97 +++++++++++++++++++ 2 files changed, 110 insertions(+), 7 deletions(-) diff --git a/apps/desktop/src/main/services/orchestration/manifestNormalization.ts b/apps/desktop/src/main/services/orchestration/manifestNormalization.ts index 1860e1340..8be1da963 100644 --- a/apps/desktop/src/main/services/orchestration/manifestNormalization.ts +++ b/apps/desktop/src/main/services/orchestration/manifestNormalization.ts @@ -286,31 +286,37 @@ export function validateManifestShape(manifest: OrchestrationManifest): string | } function validateTasks(tasks: OrchestrationManifest["tasks"]): string | null { + const seenIds = new Set(); for (const task of tasks ?? []) { if (!task || typeof task !== "object") return "manifest.tasks entries must be objects"; if (typeof task.id !== "string" || !task.id.trim()) { return "manifest.tasks entries must include a non-empty id"; } + const taskId = task.id.trim(); + if (seenIds.has(taskId)) { + return `manifest.tasks contains duplicate id ${taskId}`; + } + seenIds.add(taskId); if (!isPhaseId((task as { phaseId?: unknown }).phaseId)) { - return `manifest task ${task.id} must include phaseId; use phaseId, not phase`; + return `manifest task ${taskId} must include phaseId; use phaseId, not phase`; } if (typeof task.title !== "string" || !task.title.trim()) { - return `manifest task ${task.id} must include a non-empty title`; + return `manifest task ${taskId} must include a non-empty title`; } if (typeof task.description !== "string") { - return `manifest task ${task.id} must include description`; + return `manifest task ${taskId} must include description`; } if (!isTaskStatus(task.status)) { - return `manifest task ${task.id} has invalid status`; + return `manifest task ${taskId} has invalid status`; } if (!task.validationGate || typeof task.validationGate !== "object") { - return `manifest task ${task.id} must include validationGate`; + return `manifest task ${taskId} must include validationGate`; } if (typeof task.validationGate.required !== "boolean") { - return `manifest task ${task.id} validationGate.required must be boolean`; + return `manifest task ${taskId} validationGate.required must be boolean`; } if (!Array.isArray(task.validationGate.stepIds)) { - return `manifest task ${task.id} validationGate.stepIds must be an array`; + return `manifest task ${taskId} validationGate.stepIds must be an array`; } } return null; diff --git a/apps/desktop/src/main/services/orchestration/orchestrationService.test.ts b/apps/desktop/src/main/services/orchestration/orchestrationService.test.ts index 644bc185a..7bbf95f57 100644 --- a/apps/desktop/src/main/services/orchestration/orchestrationService.test.ts +++ b/apps/desktop/src/main/services/orchestration/orchestrationService.test.ts @@ -20,6 +20,7 @@ import type { import type { ModelRouting, ModelSelection, + OrchestrationTask, } from "../../../shared/types/orchestration"; async function makeTempLane(): Promise { @@ -31,6 +32,17 @@ async function rmTree(p: string): Promise { await fsp.rm(p, { recursive: true, force: true }); } +function makeTask(id: string): OrchestrationTask { + return { + id, + phaseId: "developing", + title: `Task ${id}`, + description: "", + status: "pending", + validationGate: { required: false, stepIds: [] }, + }; +} + describe("orchestrationService", () => { let lane: string; beforeEach(async () => { @@ -720,6 +732,91 @@ describe("orchestrationService", () => { ).toThrow(/numeric/i); }); + it("rejects duplicate task ids appended in one manifest patch", async () => { + const svc = createOrchestrationService({ + resolveLaneWorktree: () => lane, + }); + const { manifest } = await svc.runCreate({ + laneId: "L-1", + leadSessionId: "S-lead", + bundleRoot: lane, + }); + + const result = await svc.manifestPatch( + { + runId: manifest.runId, + ifMatchEtag: manifest.etag, + actorRole: "lead", + actorSessionId: "S-lead", + patches: [ + { op: "add", path: "/tasks/-", value: makeTask("T-1") }, + { op: "add", path: "/tasks/-", value: makeTask("T-1") }, + ], + }, + manifest.bundlePath, + ); + + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error).toBe("validation_failed"); + if (!("message" in result)) throw new Error("expected validation failure message"); + expect(result.message).toMatch(/duplicate id T-1/); + + const onDisk = JSON.parse( + await fsp.readFile(path.join(manifest.bundlePath, "manifest.json"), "utf-8"), + ) as { tasks: unknown[] }; + expect(onDisk.tasks).toHaveLength(0); + await svc.dispose(); + }); + + it("rejects re-adding an existing task id through manifestPatch", async () => { + const svc = createOrchestrationService({ + resolveLaneWorktree: () => lane, + }); + const { manifest } = await svc.runCreate({ + laneId: "L-1", + leadSessionId: "S-lead", + bundleRoot: lane, + }); + + const first = await svc.manifestPatch( + { + runId: manifest.runId, + ifMatchEtag: manifest.etag, + actorRole: "lead", + actorSessionId: "S-lead", + patches: [{ op: "add", path: "/tasks/-", value: makeTask("T-1") }], + }, + manifest.bundlePath, + ); + + expect(first.ok).toBe(true); + if (!first.ok) return; + + const duplicate = await svc.manifestPatch( + { + runId: manifest.runId, + ifMatchEtag: first.etag, + actorRole: "lead", + actorSessionId: "S-lead", + patches: [{ op: "add", path: "/tasks/-", value: makeTask("T-1") }], + }, + manifest.bundlePath, + ); + + expect(duplicate.ok).toBe(false); + if (duplicate.ok) return; + expect(duplicate.error).toBe("validation_failed"); + if (!("message" in duplicate)) throw new Error("expected validation failure message"); + expect(duplicate.message).toMatch(/duplicate id T-1/); + + const onDisk = JSON.parse( + await fsp.readFile(path.join(manifest.bundlePath, "manifest.json"), "utf-8"), + ) as { tasks: unknown[] }; + expect(onDisk.tasks).toHaveLength(1); + await svc.dispose(); + }); + it("validates spawn-brief required sections", () => { expect(validateSpawnBrief("nothing here").ok).toBe(false); const good = `## TASK\nBuild login\n## FILES\nsrc/x.ts\n## DEPENDENCIES\nnone\n## GATES\nreverify_changes\n## PEERS\nnone\n## SUCCESS\ntests pass`;