Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -286,31 +286,37 @@ export function validateManifestShape(manifest: OrchestrationManifest): string |
}

function validateTasks(tasks: OrchestrationManifest["tasks"]): string | null {
const seenIds = new Set<string>();
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`;
}
Comment thread
greptile-apps[bot] marked this conversation as resolved.
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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import type {
import type {
ModelRouting,
ModelSelection,
OrchestrationTask,
} from "../../../shared/types/orchestration";

async function makeTempLane(): Promise<string> {
Expand All @@ -31,6 +32,17 @@ async function rmTree(p: string): Promise<void> {
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 () => {
Expand Down Expand Up @@ -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`;
Expand Down
Loading