fix: address dependency mutation review comments

This commit is contained in:
Phil Larson
2026-05-30 13:53:34 -07:00
parent b1c1a33d34
commit a89e4edadb
2 changed files with 71 additions and 4 deletions

View File

@@ -25,6 +25,8 @@ describe("TaskStore dependency mutations", () => {
dependencies: [obsolete.id],
});
await store.updateTask(dependent.id, { status: "queued", blockedBy: obsolete.id });
const movedEvents: Array<{ from: string; to: string; task: { id: string } }> = [];
store.on("task:moved", (event) => movedEvents.push(event));
const updated = await store.updateTaskDependencies(dependent.id, {
operation: "replace",
@@ -38,6 +40,8 @@ describe("TaskStore dependency mutations", () => {
expect(updated.column).toBe("triage");
expect(updated.log.at(-2)?.action).toBe("Moved to triage for re-specification — new dependency added");
expect(updated.log.at(-1)?.action).toContain(`Replaced dependency ${obsolete.id} with ${canonical.id}`);
expect(movedEvents).toHaveLength(1);
expect(movedEvents[0]).toMatchObject({ from: "todo", to: "triage", task: { id: dependent.id } });
const reloaded = await store.getTask(dependent.id);
expect(reloaded.dependencies).toEqual([canonical.id]);
@@ -79,6 +83,63 @@ describe("TaskStore dependency mutations", () => {
expect(taskJson.column).toBe("triage");
});
it("removes dependencies and recomputes stale blockers", async () => {
const active = await store.createTask({ description: "active prerequisite" });
const resolved = await store.createTask({ description: "resolved prerequisite", column: "done" });
const dependent = await store.createTask({
description: "dependent task",
dependencies: [active.id, resolved.id],
});
await store.updateTask(dependent.id, { blockedBy: active.id });
await expect(
store.updateTaskDependencies(dependent.id, { operation: "remove", dependency: "FN-404" }),
).rejects.toThrow(/does not depend on/);
const updated = await store.updateTaskDependencies(dependent.id, {
operation: "remove",
dependency: active.id,
});
expect(updated.dependencies).toEqual([resolved.id]);
expect(updated.blockedBy).toBeUndefined();
const reloaded = await store.getTask(dependent.id);
expect(reloaded.dependencies).toEqual([resolved.id]);
expect(reloaded.blockedBy).toBeUndefined();
});
it("sets dependencies with validation and blocker recomputation", async () => {
const original = await store.createTask({ description: "original prerequisite" });
const replacement = await store.createTask({ description: "replacement prerequisite", column: "done" });
const dependent = await store.createTask({
description: "dependent task",
column: "todo",
dependencies: [original.id],
});
const cycle = await store.createTask({ description: "cycle prerequisite", dependencies: [dependent.id] });
await store.updateTask(dependent.id, { blockedBy: original.id });
await expect(
store.updateTaskDependencies(dependent.id, { operation: "set", dependencies: [replacement.id, replacement.id] }),
).rejects.toThrow(/already depends on/);
await expect(
store.updateTaskDependencies(dependent.id, { operation: "set", dependencies: [dependent.id] }),
).rejects.toThrow(/cannot depend on itself/);
await expect(
store.updateTaskDependencies(dependent.id, { operation: "set", dependencies: [cycle.id] }),
).rejects.toThrow(/Dependency cycle detected/);
const updated = await store.updateTaskDependencies(dependent.id, {
operation: "set",
dependencies: [replacement.id],
});
expect(updated.dependencies).toEqual([replacement.id]);
expect(updated.blockedBy).toBeUndefined();
expect(updated.column).toBe("triage");
});
it("rejects missing replacements, duplicates, self dependencies, and cycles", async () => {
const a = await store.createTask({ description: "a" });
const b = await store.createTask({ description: "b", dependencies: [a.id] });

View File

@@ -5672,7 +5672,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
new Map([[id, nextDependencies]]),
);
const previousDependencySet = new Set(previousDependencies);
const previousDependencySet = new Set(normalizedCurrent);
const hasNewDependencies = nextDependencies.some((dependencyId) => !previousDependencySet.has(dependencyId));
task.dependencies = nextDependencies;
@@ -5691,8 +5691,10 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
}
task.updatedAt = new Date().toISOString();
task.log ??= [];
let movedToTriage = false;
if (hasNewDependencies && task.column === "todo") {
task.column = "triage";
movedToTriage = true;
task.status = undefined;
task.columnMovedAt = task.updatedAt;
task.log.push({
@@ -5722,6 +5724,9 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
},
};
await this.atomicWriteTaskJsonWithAudit(dir, task, auditEvent);
if (movedToTriage) {
this.emit("task:moved", { task, from: "todo" as Column, to: "triage" as Column, source: "engine" });
}
this.emit("task:updated", task);
return task;
});
@@ -5805,9 +5810,10 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
// Detect new dependencies being added to a todo task → auto-move to triage
let movedToTriage = false;
if (updates.dependencies !== undefined) {
const oldDeps = new Set(task.dependencies);
const hasNewDeps = updates.dependencies.some((d) => !oldDeps.has(d));
task.dependencies = updates.dependencies;
const oldDeps = new Set((task.dependencies ?? []).map((dependency) => dependency.trim()).filter(Boolean));
const normalizedDependencies = updates.dependencies.map((dependency) => dependency.trim()).filter(Boolean);
const hasNewDeps = normalizedDependencies.some((d) => !oldDeps.has(d));
task.dependencies = normalizedDependencies;
if (hasNewDeps && task.column === "todo") {
task.column = "triage";