From 3a801b9001396053322b4aa1677613741c989dbc Mon Sep 17 00:00:00 2001 From: 67Midas Date: Sat, 5 Sep 2026 12:54:44 -0500 Subject: [PATCH 01/11] fix(domain): deep-freeze recommendation warning affectedTaskIds --- src/domain/decision/recommendation.test.ts | 23 ++++++++++++++++++++++ src/domain/decision/recommendation.ts | 6 ++++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/domain/decision/recommendation.test.ts b/src/domain/decision/recommendation.test.ts index 51fa7c5..ac022ad 100644 --- a/src/domain/decision/recommendation.test.ts +++ b/src/domain/decision/recommendation.test.ts @@ -332,6 +332,29 @@ describe("recommendNextTask — determinism and immutability", () => { } for (const warning of recommendation.warnings) { expect(Object.isFrozen(warning)).toBe(true); + if (warning.affectedTaskIds) { + // The affectedTaskIds array itself is deep-frozen and owned by the + // warning, so it can never be mutated by a caller. + expect(Object.isFrozen(warning.affectedTaskIds)).toBe(true); + } } }); + + it("does not share affectedTaskIds arrays across warnings", () => { + // Two BLOCKED candidates in separate subgraphs produce a tie-break + // warning AND a blocked-status warning, each carrying affectedTaskIds. + const recommendation = recommend([ + task("a", { status: "BLOCKED", value: 5 }), + task("b", { status: "BLOCKED", value: 5 }), + ]); + const warned = recommendation.warnings.filter( + (w) => w.affectedTaskIds !== undefined, + ); + expect(warned.length).toBeGreaterThanOrEqual(2); + const first = warned[0].affectedTaskIds!; + const second = warned[1].affectedTaskIds!; + expect(first).not.toBe(second); + expect(Object.isFrozen(first)).toBe(true); + expect(Object.isFrozen(second)).toBe(true); + }); }); diff --git a/src/domain/decision/recommendation.ts b/src/domain/decision/recommendation.ts index c99000c..2a72b21 100644 --- a/src/domain/decision/recommendation.ts +++ b/src/domain/decision/recommendation.ts @@ -110,7 +110,9 @@ function buildWarnings( Object.freeze({ id: "tie-break-applied", message: `${tied.length} candidates tie at score ${top.score}; the recommendation was chosen by the documented tie-breaking policy (ascending task id).`, - affectedTaskIds: tied, + // Build a fresh frozen copy so each warning owns its own immutable + // array; never share or freeze a caller-supplied array in place. + affectedTaskIds: Object.freeze([...tied]), }), ); } @@ -137,7 +139,7 @@ function buildWarnings( id: "blocked-status-eligible", message: "Tasks marked BLOCKED satisfy the eligibility rule (all prerequisites DONE) and therefore remain under consideration; the BLOCKED flag does not exclude tasks.", - affectedTaskIds: blockedEligible, + affectedTaskIds: Object.freeze([...blockedEligible]), }), ); } From 29aaabd3c1c6f6d271ec0d593dc32904b53ef9a3 Mon Sep 17 00:00:00 2001 From: 67Midas Date: Sat, 5 Sep 2026 12:57:24 -0500 Subject: [PATCH 02/11] refactor(application): extract shared toCreateTaskInput mapper --- src/application/dependency-service.ts | 18 ++-------- src/application/goal-service.ts | 18 ++-------- src/application/task-input.test.ts | 51 +++++++++++++++++++++++++++ src/application/task-input.ts | 24 +++++++++++++ src/application/task-service.ts | 16 +-------- 5 files changed, 80 insertions(+), 47 deletions(-) create mode 100644 src/application/task-input.test.ts create mode 100644 src/application/task-input.ts diff --git a/src/application/dependency-service.ts b/src/application/dependency-service.ts index 45893b4..5c62587 100644 --- a/src/application/dependency-service.ts +++ b/src/application/dependency-service.ts @@ -1,25 +1,11 @@ -import type { Task, CreateTaskInput } from "../domain/index.js"; +import type { Task } from "../domain/index.js"; import { createTask, createProject, createDependencyGraph, } from "../domain/index.js"; import type { ProjectRepository } from "./repository.js"; - -function toCreateTaskInput(task: Task): CreateTaskInput { - return { - id: task.id, - title: task.title, - description: task.description, - status: task.status, - value: task.value, - urgency: task.urgency, - estimatedEffort: task.estimatedEffort, - confidence: task.confidence, - goalId: task.goalId ?? undefined, - dependencies: task.dependencies, - }; -} +import { toCreateTaskInput } from "./task-input.js"; export class DependencyService { private readonly repository: ProjectRepository; diff --git a/src/application/goal-service.ts b/src/application/goal-service.ts index 077204d..038393f 100644 --- a/src/application/goal-service.ts +++ b/src/application/goal-service.ts @@ -1,21 +1,7 @@ -import type { Goal, Task, CreateGoalInput } from "../domain/index.js"; +import type { Goal, CreateGoalInput } from "../domain/index.js"; import { createGoal, createTask, createProject } from "../domain/index.js"; import type { ProjectRepository } from "./repository.js"; - -function toCreateTaskInput(task: Task) { - return { - id: task.id, - title: task.title, - description: task.description, - status: task.status, - value: task.value, - urgency: task.urgency, - estimatedEffort: task.estimatedEffort, - confidence: task.confidence, - goalId: task.goalId ?? undefined, - dependencies: task.dependencies, - }; -} +import { toCreateTaskInput } from "./task-input.js"; export class GoalService { private readonly repository: ProjectRepository; diff --git a/src/application/task-input.test.ts b/src/application/task-input.test.ts new file mode 100644 index 0000000..ab9258d --- /dev/null +++ b/src/application/task-input.test.ts @@ -0,0 +1,51 @@ +import { describe, it, expect } from "vitest"; +import { createTask } from "../domain/index.js"; +import { toCreateTaskInput } from "./task-input.js"; + +describe("toCreateTaskInput", () => { + it("round-trips every task field without loss", () => { + const task = createTask({ + id: "t1", + title: "Build feature", + description: "Core feature", + status: "IN_PROGRESS", + value: 8, + urgency: 6, + estimatedEffort: 4, + confidence: 0.7, + goalId: "g1", + dependencies: ["t0"], + }); + + const input = toCreateTaskInput(task); + expect(input).toEqual({ + id: "t1", + title: "Build feature", + description: "Core feature", + status: "IN_PROGRESS", + value: 8, + urgency: 6, + estimatedEffort: 4, + confidence: 0.7, + goalId: "g1", + dependencies: ["t0"], + }); + + // Rebuilding through the domain factory preserves the original task. + expect(createTask(input)).toEqual(task); + }); + + it("converts goalId null to undefined so createTask yields the same null", () => { + const task = createTask({ id: "t1", title: "No goal", goalId: "g1" }); + const withoutGoal = createTask({ + ...toCreateTaskInput(task), + goalId: undefined, + }); + // Removing the goal reference leaves the domain's "no goal" representation. + expect(withoutGoal.goalId).toBeNull(); + + const noGoalTask = createTask({ id: "t2", title: "Plain" }); + expect(toCreateTaskInput(noGoalTask).goalId).toBeUndefined(); + expect(createTask(toCreateTaskInput(noGoalTask)).goalId).toBeNull(); + }); +}); diff --git a/src/application/task-input.ts b/src/application/task-input.ts new file mode 100644 index 0000000..bac426c --- /dev/null +++ b/src/application/task-input.ts @@ -0,0 +1,24 @@ +import type { Task, CreateTaskInput } from "../domain/index.js"; + +/** + * Map a persisted Task back to a CreateTaskInput so services can rebuild a + * task through the domain factory without losing any field. + * + * `goalId: null` (the domain's "no goal" representation) is converted to + * `goalId: undefined` because `CreateTaskInput.goalId` is optional — omitting + * it produces the same `goalId: null` result in `createTask`. + */ +export function toCreateTaskInput(task: Task): CreateTaskInput { + return { + id: task.id, + title: task.title, + description: task.description, + status: task.status, + value: task.value, + urgency: task.urgency, + estimatedEffort: task.estimatedEffort, + confidence: task.confidence, + goalId: task.goalId ?? undefined, + dependencies: task.dependencies, + }; +} diff --git a/src/application/task-service.ts b/src/application/task-service.ts index 182b933..37c3fbf 100644 --- a/src/application/task-service.ts +++ b/src/application/task-service.ts @@ -1,21 +1,7 @@ import type { Task, CreateTaskInput, TaskStatus } from "../domain/index.js"; import { createTask, createProject } from "../domain/index.js"; import type { ProjectRepository } from "./repository.js"; - -function toCreateTaskInput(task: Task): CreateTaskInput { - return { - id: task.id, - title: task.title, - description: task.description, - status: task.status, - value: task.value, - urgency: task.urgency, - estimatedEffort: task.estimatedEffort, - confidence: task.confidence, - goalId: task.goalId ?? undefined, - dependencies: task.dependencies, - }; -} +import { toCreateTaskInput } from "./task-input.js"; export class TaskService { private readonly repository: ProjectRepository; From ed2a0e040488724fd1c147a5ea76591d3f869977 Mon Sep 17 00:00:00 2001 From: 67Midas Date: Sat, 5 Sep 2026 13:09:20 -0500 Subject: [PATCH 03/11] test: consolidate createStubRepository into shared test-support Replace ten duplicated per-file repository stubs with one behavioral stub in src/test-support. The shared stub seeds an in-memory Map via the initialProject option and accepts per-method overrides; base methods are vi.fn()-wrapped so tests can still reconfigure them with vi.mocked(repository.x).mockResolvedValue(...). No production code touched; -275 lines of test helper duplication removed. --- docs/progress.md | 12 +++ src/application/dependency-service.test.ts | 42 ++--------- src/application/goal-service.test.ts | 45 +++-------- src/application/project-service.test.ts | 32 +------- .../recommendation-service.test.ts | 37 +-------- src/application/scenario-service.test.ts | 34 +-------- src/application/task-service.test.ts | 53 ++++--------- src/test-support/index.ts | 75 +++++++++++++++++++ src/ui/Dashboard.test.tsx | 13 +--- src/ui/ScenarioPanel.test.tsx | 13 +--- src/ui/TaskList.test.tsx | 13 +--- src/ui/accessibility.test.tsx | 43 +++-------- 12 files changed, 137 insertions(+), 275 deletions(-) create mode 100644 src/test-support/index.ts diff --git a/docs/progress.md b/docs/progress.md index da8c294..c999996 100644 --- a/docs/progress.md +++ b/docs/progress.md @@ -435,3 +435,15 @@ Completed: - Updated `docs/handoff.md` to reflect TASK-017 DONE and all phases complete - Updated `docs/tasks.md` to mark TASK-017 DONE - Verified: `npm run verify` (typecheck, 287 tests, lint, format:check), `npm run build`, all pass + +## 2026-09-05 — Consolidated test repository stubs (review remediation, on fix/review-remediation-018-026) + +Completed: +- Extracted the duplicated `createStubRepository` helper into a single shared module `src/test-support/index.ts` (with the existing `createInMemoryStorage`) +- The shared stub seeds an in-memory Map via the `initialProject` option and accepts per-method `overrides`; each base method is a `vi.fn()` around its behavioral implementation, so tests can both rely on real save/load/list/delete behavior and reconfigure methods in place via `vi.mocked(repository.x).mockResolvedValue(...)` — no behavioral difference from the old per-file stubs +- Migrated all ten test files that previously defined their own stub to import the shared one: + - Application service tests (`project`, `task`, `goal`, `dependency`, `recommendation`, `scenario`): positional `createStubRepository(project)` calls rewritten to the `{ initialProject }` options bag + - UI tests (`Dashboard`, `ScenarioPanel`, `TaskList`, `accessibility`): local vi.fn stubs replaced with the shared stub; `accessibility.test.tsx` passes its vi.fn list/load spies through `overrides`, and the focus-management test now relies on the stub's real save/load store instead of hand-rolled `currentProject` mutation +- Net diff: −275 lines of duplicated test helper code; no production code touched +- Test count verified unchanged at HEAD and after the change: 290 `it`/`test` blocks (older progress entries citing 287 were already stale relative to HEAD) +- Verified: `npm run verify` (typecheck, 290 tests, lint, format:check) and `npm run build` both pass diff --git a/src/application/dependency-service.test.ts b/src/application/dependency-service.test.ts index 9704011..605ae2d 100644 --- a/src/application/dependency-service.test.ts +++ b/src/application/dependency-service.test.ts @@ -1,37 +1,7 @@ import { describe, it, expect } from "vitest"; import { DependencyService } from "./dependency-service.js"; -import type { ProjectRepository, ProjectSummary } from "./repository.js"; -import type { Project } from "../domain/index.js"; import { createTask } from "../domain/index.js"; - -function createStubRepository(initialProject?: Project): ProjectRepository { - const store = new Map(); - if (initialProject) { - store.set(initialProject.id, initialProject); - } - return { - save: async (project: Project) => { - store.set(project.id, project); - }, - load: async (id: string) => store.get(id) ?? null, - list: async () => { - const summaries: ProjectSummary[] = []; - for (const [id, project] of store) { - summaries.push({ - id, - name: project.name, - description: project.description, - taskCount: project.tasks.length, - goalCount: project.goals.length, - }); - } - return Object.freeze(summaries.sort((a, b) => a.id.localeCompare(b.id))); - }, - delete: async (id: string) => { - return store.delete(id); - }, - }; -} +import { createStubRepository } from "../test-support/index.js"; function makeProject(tasks: ReturnType[]) { return { @@ -50,7 +20,7 @@ describe("DependencyService", () => { createTask({ id: "a", title: "A" }), createTask({ id: "b", title: "B" }), ]); - const repo = createStubRepository(project); + const repo = createStubRepository({ initialProject: project }); const service = new DependencyService(repo); const updated = await service.addDependency("p1", "b", "a"); @@ -66,7 +36,7 @@ describe("DependencyService", () => { createTask({ id: "a", title: "A" }), createTask({ id: "b", title: "B", dependencies: ["a"] }), ]); - const repo = createStubRepository(project); + const repo = createStubRepository({ initialProject: project }); const service = new DependencyService(repo); const result = await service.addDependency("p1", "b", "a"); @@ -78,7 +48,7 @@ describe("DependencyService", () => { it("throws for non-existent task", async () => { const project = makeProject([createTask({ id: "a", title: "A" })]); - const repo = createStubRepository(project); + const repo = createStubRepository({ initialProject: project }); const service = new DependencyService(repo); await expect(service.addDependency("p1", "missing", "a")).rejects.toThrow( @@ -102,7 +72,7 @@ describe("DependencyService", () => { createTask({ id: "a", title: "A" }), createTask({ id: "b", title: "B", dependencies: ["a"] }), ]); - const repo = createStubRepository(project); + const repo = createStubRepository({ initialProject: project }); const service = new DependencyService(repo); const updated = await service.removeDependency("p1", "b", "a"); @@ -114,7 +84,7 @@ describe("DependencyService", () => { createTask({ id: "a", title: "A" }), createTask({ id: "b", title: "B" }), ]); - const repo = createStubRepository(project); + const repo = createStubRepository({ initialProject: project }); const service = new DependencyService(repo); const result = await service.removeDependency("p1", "b", "a"); diff --git a/src/application/goal-service.test.ts b/src/application/goal-service.test.ts index 65214a0..eec8030 100644 --- a/src/application/goal-service.test.ts +++ b/src/application/goal-service.test.ts @@ -1,37 +1,8 @@ import { describe, it, expect } from "vitest"; import { GoalService } from "./goal-service.js"; -import type { ProjectRepository, ProjectSummary } from "./repository.js"; import type { Project, Goal } from "../domain/index.js"; import { createGoal } from "../domain/index.js"; - -function createStubRepository(initialProject?: Project): ProjectRepository { - const store = new Map(); - if (initialProject) { - store.set(initialProject.id, initialProject); - } - return { - save: async (project: Project) => { - store.set(project.id, project); - }, - load: async (id: string) => store.get(id) ?? null, - list: async () => { - const summaries: ProjectSummary[] = []; - for (const [id, project] of store) { - summaries.push({ - id, - name: project.name, - description: project.description, - taskCount: project.tasks.length, - goalCount: project.goals.length, - }); - } - return Object.freeze(summaries.sort((a, b) => a.id.localeCompare(b.id))); - }, - delete: async (id: string) => { - return store.delete(id); - }, - }; -} +import { createStubRepository } from "../test-support/index.js"; function makeProject(goals: Goal[] = []): Project { return { @@ -46,7 +17,7 @@ function makeProject(goals: Goal[] = []): Project { describe("GoalService", () => { describe("addGoal", () => { it("adds a goal to the project", async () => { - const repo = createStubRepository(makeProject()); + const repo = createStubRepository({ initialProject: makeProject() }); const service = new GoalService(repo); const goal = await service.addGoal("p1", { @@ -62,9 +33,11 @@ describe("GoalService", () => { }); it("rejects duplicate goal ids", async () => { - const repo = createStubRepository( - makeProject([createGoal({ id: "g1", name: "Existing" })]), - ); + const repo = createStubRepository({ + initialProject: makeProject([ + createGoal({ id: "g1", name: "Existing" }), + ]), + }); const service = new GoalService(repo); await expect( @@ -88,7 +61,7 @@ describe("GoalService", () => { createGoal({ id: "g1", name: "Launch" }), createGoal({ id: "g2", name: "Ship" }), ]); - const repo = createStubRepository(project); + const repo = createStubRepository({ initialProject: project }); const service = new GoalService(repo); await service.removeGoal("p1", "g1"); @@ -99,7 +72,7 @@ describe("GoalService", () => { }); it("throws for non-existent goal", async () => { - const repo = createStubRepository(makeProject()); + const repo = createStubRepository({ initialProject: makeProject() }); const service = new GoalService(repo); await expect(service.removeGoal("p1", "missing")).rejects.toThrow( diff --git a/src/application/project-service.test.ts b/src/application/project-service.test.ts index ed6c291..8ca8d86 100644 --- a/src/application/project-service.test.ts +++ b/src/application/project-service.test.ts @@ -1,36 +1,6 @@ import { describe, it, expect } from "vitest"; import { ProjectService } from "./project-service.js"; -import type { ProjectRepository, ProjectSummary } from "./repository.js"; -import type { Project } from "../domain/index.js"; - -function createStubRepository( - overrides: Partial = {}, -): ProjectRepository { - const store = new Map(); - return { - save: async (project: Project) => { - store.set(project.id, project); - }, - load: async (id: string) => store.get(id) ?? null, - list: async () => { - const summaries: ProjectSummary[] = []; - for (const [id, project] of store) { - summaries.push({ - id, - name: project.name, - description: project.description, - taskCount: project.tasks.length, - goalCount: project.goals.length, - }); - } - return Object.freeze(summaries.sort((a, b) => a.id.localeCompare(b.id))); - }, - delete: async (id: string) => { - return store.delete(id); - }, - ...overrides, - }; -} +import { createStubRepository } from "../test-support/index.js"; describe("ProjectService", () => { describe("createProject", () => { diff --git a/src/application/recommendation-service.test.ts b/src/application/recommendation-service.test.ts index 0f7b07d..7a6d5c3 100644 --- a/src/application/recommendation-service.test.ts +++ b/src/application/recommendation-service.test.ts @@ -1,37 +1,8 @@ import { describe, it, expect } from "vitest"; import { RecommendationService } from "./recommendation-service.js"; -import type { ProjectRepository, ProjectSummary } from "./repository.js"; -import type { Project, Task } from "../domain/index.js"; +import type { Task } from "../domain/index.js"; import { createTask } from "../domain/index.js"; - -function createStubRepository(initialProject?: Project): ProjectRepository { - const store = new Map(); - if (initialProject) { - store.set(initialProject.id, initialProject); - } - return { - save: async (project: Project) => { - store.set(project.id, project); - }, - load: async (id: string) => store.get(id) ?? null, - list: async () => { - const summaries: ProjectSummary[] = []; - for (const [id, project] of store) { - summaries.push({ - id, - name: project.name, - description: project.description, - taskCount: project.tasks.length, - goalCount: project.goals.length, - }); - } - return Object.freeze(summaries.sort((a, b) => a.id.localeCompare(b.id))); - }, - delete: async (id: string) => { - return store.delete(id); - }, - }; -} +import { createStubRepository } from "../test-support/index.js"; function makeProject(tasks: Task[]) { return { @@ -50,7 +21,7 @@ describe("RecommendationService", () => { createTask({ id: "a", title: "A", value: 10 }), createTask({ id: "b", title: "B", value: 5 }), ]); - const repo = createStubRepository(project); + const repo = createStubRepository({ initialProject: project }); const service = new RecommendationService(repo); const rec = await service.getRecommendation("p1"); @@ -81,7 +52,7 @@ describe("RecommendationService", () => { dependencies: ["a"], }), ]); - const repo = createStubRepository(project); + const repo = createStubRepository({ initialProject: project }); const service = new RecommendationService(repo); const result = await service.getGraph("p1"); diff --git a/src/application/scenario-service.test.ts b/src/application/scenario-service.test.ts index 0e36eae..9d607e6 100644 --- a/src/application/scenario-service.test.ts +++ b/src/application/scenario-service.test.ts @@ -1,37 +1,7 @@ import { describe, it, expect } from "vitest"; import { ScenarioService } from "./scenario-service.js"; -import type { ProjectRepository, ProjectSummary } from "./repository.js"; -import type { Project } from "../domain/index.js"; import { createTask } from "../domain/index.js"; - -function createStubRepository(initialProject?: Project): ProjectRepository { - const store = new Map(); - if (initialProject) { - store.set(initialProject.id, initialProject); - } - return { - save: async (project: Project) => { - store.set(project.id, project); - }, - load: async (id: string) => store.get(id) ?? null, - list: async () => { - const summaries: ProjectSummary[] = []; - for (const [id, project] of store) { - summaries.push({ - id, - name: project.name, - description: project.description, - taskCount: project.tasks.length, - goalCount: project.goals.length, - }); - } - return Object.freeze(summaries.sort((a, b) => a.id.localeCompare(b.id))); - }, - delete: async (id: string) => { - return store.delete(id); - }, - }; -} +import { createStubRepository } from "../test-support/index.js"; describe("ScenarioService", () => { describe("runScenario", () => { @@ -51,7 +21,7 @@ describe("ScenarioService", () => { ], goals: [], }; - const repo = createStubRepository(project); + const repo = createStubRepository({ initialProject: project }); const service = new ScenarioService(repo); const result = await service.runScenario("p1", { diff --git a/src/application/task-service.test.ts b/src/application/task-service.test.ts index cc6d976..1be7bf6 100644 --- a/src/application/task-service.test.ts +++ b/src/application/task-service.test.ts @@ -1,37 +1,8 @@ import { describe, it, expect } from "vitest"; import { TaskService } from "./task-service.js"; -import type { ProjectRepository, ProjectSummary } from "./repository.js"; import type { Project, Task } from "../domain/index.js"; import { createTask, TaskStatus } from "../domain/index.js"; - -function createStubRepository(initialProject?: Project): ProjectRepository { - const store = new Map(); - if (initialProject) { - store.set(initialProject.id, initialProject); - } - return { - save: async (project: Project) => { - store.set(project.id, project); - }, - load: async (id: string) => store.get(id) ?? null, - list: async () => { - const summaries: ProjectSummary[] = []; - for (const [id, project] of store) { - summaries.push({ - id, - name: project.name, - description: project.description, - taskCount: project.tasks.length, - goalCount: project.goals.length, - }); - } - return Object.freeze(summaries.sort((a, b) => a.id.localeCompare(b.id))); - }, - delete: async (id: string) => { - return store.delete(id); - }, - }; -} +import { createStubRepository } from "../test-support/index.js"; function makeProject(tasks: Task[] = []): Project { return { @@ -46,7 +17,7 @@ function makeProject(tasks: Task[] = []): Project { describe("TaskService", () => { describe("addTask", () => { it("adds a task to the project", async () => { - const repo = createStubRepository(makeProject()); + const repo = createStubRepository({ initialProject: makeProject() }); const service = new TaskService(repo); const task = await service.addTask("p1", { @@ -63,9 +34,11 @@ describe("TaskService", () => { }); it("rejects duplicate task ids", async () => { - const repo = createStubRepository( - makeProject([createTask({ id: "t1", title: "Existing" })]), - ); + const repo = createStubRepository({ + initialProject: makeProject([ + createTask({ id: "t1", title: "Existing" }), + ]), + }); const service = new TaskService(repo); await expect( @@ -88,7 +61,7 @@ describe("TaskService", () => { const project = makeProject([ createTask({ id: "t1", title: "Task", status: TaskStatus.BACKLOG }), ]); - const repo = createStubRepository(project); + const repo = createStubRepository({ initialProject: project }); const service = new TaskService(repo); const updated = await service.updateTaskStatus( @@ -104,7 +77,7 @@ describe("TaskService", () => { }); it("throws for non-existent task", async () => { - const repo = createStubRepository(makeProject()); + const repo = createStubRepository({ initialProject: makeProject() }); const service = new TaskService(repo); await expect( @@ -120,7 +93,7 @@ describe("TaskService", () => { createTask({ id: "t2", title: "B", dependencies: ["t1"] }), createTask({ id: "t3", title: "C", dependencies: ["t1", "t2"] }), ]); - const repo = createStubRepository(project); + const repo = createStubRepository({ initialProject: project }); const service = new TaskService(repo); await service.removeTask("p1", "t1"); @@ -133,7 +106,7 @@ describe("TaskService", () => { }); it("throws for non-existent task", async () => { - const repo = createStubRepository(makeProject()); + const repo = createStubRepository({ initialProject: makeProject() }); const service = new TaskService(repo); await expect(service.removeTask("p1", "missing")).rejects.toThrow( @@ -145,7 +118,7 @@ describe("TaskService", () => { describe("getTask", () => { it("returns the task when it exists", async () => { const project = makeProject([createTask({ id: "t1", title: "Task" })]); - const repo = createStubRepository(project); + const repo = createStubRepository({ initialProject: project }); const service = new TaskService(repo); const task = await service.getTask("p1", "t1"); @@ -154,7 +127,7 @@ describe("TaskService", () => { }); it("returns null for non-existent task", async () => { - const repo = createStubRepository(makeProject()); + const repo = createStubRepository({ initialProject: makeProject() }); const service = new TaskService(repo); expect(await service.getTask("p1", "missing")).toBeNull(); diff --git a/src/test-support/index.ts b/src/test-support/index.ts new file mode 100644 index 0000000..abe86c6 --- /dev/null +++ b/src/test-support/index.ts @@ -0,0 +1,75 @@ +import { vi } from "vitest"; +import type { Project } from "../domain/index.js"; +import type { + ProjectRepository, + ProjectSummary, +} from "../application/repository.js"; +import type { StorageProvider } from "../infrastructure/storage.js"; + +export interface StubRepositoryOptions { + /** Seed the in-memory store with an existing project. */ + readonly initialProject?: Project; + /** Replace individual repository methods (e.g. with vi.fn() spies). */ + readonly overrides?: Partial; +} + +/** + * In-memory ProjectRepository stub for tests. Persists projects in a Map so + * save/load/list/delete behave like a real repository. Use `overrides` to + * substitute a method (for example a vi.fn() with controlled resolution). + * + * Each base method is a vi.fn() around its behavioral implementation, so + * tests may also reconfigure them in place via `vi.mocked(repository.x)` + * (e.g. `vi.mocked(repository.load).mockResolvedValue(project)`). + */ +export function createStubRepository( + options: StubRepositoryOptions = {}, +): ProjectRepository { + const store = new Map(); + if (options.initialProject) { + store.set(options.initialProject.id, options.initialProject); + } + const repository: ProjectRepository = { + save: vi.fn(async (project: Project) => { + store.set(project.id, project); + }), + load: vi.fn(async (id: string) => store.get(id) ?? null), + list: vi.fn(async () => { + const summaries: ProjectSummary[] = []; + for (const [id, project] of store) { + summaries.push({ + id, + name: project.name, + description: project.description, + taskCount: project.tasks.length, + goalCount: project.goals.length, + }); + } + return Object.freeze(summaries.sort((a, b) => a.id.localeCompare(b.id))); + }), + delete: vi.fn(async (id: string) => store.delete(id)), + ...options.overrides, + }; + return repository; +} + +/** + * In-memory StorageProvider for tests. `getInternal` exposes the raw stored + * JSON so a test can assert on the serialized format itself. + */ +export function createInMemoryStorage(): StorageProvider & { + getInternal(key: string): string | null; +} { + const store = new Map(); + return { + getItem: (key: string) => store.get(key) ?? null, + setItem: (key: string, value: string) => { + store.set(key, value); + }, + removeItem: (key: string) => { + store.delete(key); + }, + keys: () => Object.freeze([...store.keys()]), + getInternal: (key: string) => store.get(key) ?? null, + }; +} diff --git a/src/ui/Dashboard.test.tsx b/src/ui/Dashboard.test.tsx index 343872a..687bca8 100644 --- a/src/ui/Dashboard.test.tsx +++ b/src/ui/Dashboard.test.tsx @@ -10,18 +10,7 @@ import type { ProjectRepository } from "../application/repository.js"; import type { ProjectSummary } from "../application/repository.js"; import type { Project } from "../domain/index.js"; import { createProject, createTask, createGoal } from "../domain/index.js"; - -function createStubRepository( - overrides: Partial = {}, -): ProjectRepository { - return { - save: vi.fn().mockResolvedValue(undefined), - load: vi.fn().mockResolvedValue(null), - list: vi.fn().mockResolvedValue([]), - delete: vi.fn().mockResolvedValue(false), - ...overrides, - }; -} +import { createStubRepository } from "../test-support/index.js"; function makeTestProject(): Project { const goal = createGoal({ id: "g1", name: "Ship MVP" }); diff --git a/src/ui/ScenarioPanel.test.tsx b/src/ui/ScenarioPanel.test.tsx index eb2f389..c7af444 100644 --- a/src/ui/ScenarioPanel.test.tsx +++ b/src/ui/ScenarioPanel.test.tsx @@ -5,18 +5,7 @@ import { ScenarioService } from "../application/scenario-service.js"; import type { ProjectRepository } from "../application/repository.js"; import type { Project } from "../domain/index.js"; import { createProject, createTask } from "../domain/index.js"; - -function createStubRepository( - overrides: Partial = {}, -): ProjectRepository { - return { - save: vi.fn().mockResolvedValue(undefined), - load: vi.fn().mockResolvedValue(null), - list: vi.fn().mockResolvedValue([]), - delete: vi.fn().mockResolvedValue(false), - ...overrides, - }; -} +import { createStubRepository } from "../test-support/index.js"; function makeTestProject(): Project { const task1 = createTask({ diff --git a/src/ui/TaskList.test.tsx b/src/ui/TaskList.test.tsx index 51abb10..9f29930 100644 --- a/src/ui/TaskList.test.tsx +++ b/src/ui/TaskList.test.tsx @@ -9,18 +9,7 @@ import { createDependencyGraph, calculateSchedule, } from "../domain/index.js"; - -function createStubRepository( - overrides: Partial = {}, -): ProjectRepository { - return { - save: vi.fn().mockResolvedValue(undefined), - load: vi.fn().mockResolvedValue(null), - list: vi.fn().mockResolvedValue([]), - delete: vi.fn().mockResolvedValue(false), - ...overrides, - }; -} +import { createStubRepository } from "../test-support/index.js"; describe("TaskList", () => { let repository: ProjectRepository; diff --git a/src/ui/accessibility.test.tsx b/src/ui/accessibility.test.tsx index 0d7de1f..ccf4177 100644 --- a/src/ui/accessibility.test.tsx +++ b/src/ui/accessibility.test.tsx @@ -13,18 +13,7 @@ import { createProject, createTask, createGoal } from "../domain/index.js"; import { DependencyEditor } from "./DependencyEditor.js"; import { DependencyGraphVisualization } from "./DependencyGraph.js"; import { createDependencyGraph, calculateSchedule } from "../domain/index.js"; - -function createStubRepository( - overrides: Partial = {}, -): ProjectRepository { - return { - save: vi.fn().mockResolvedValue(undefined), - load: vi.fn().mockResolvedValue(null), - list: vi.fn().mockResolvedValue([]), - delete: vi.fn().mockResolvedValue(false), - ...overrides, - }; -} +import { createStubRepository } from "../test-support/index.js"; function makeTestProject(): Project { const goal = createGoal({ id: "g1", name: "Ship MVP" }); @@ -106,8 +95,10 @@ function makeSingleProjectRepo(): { goalCount: project.goals.length, }; const repository = createStubRepository({ - list: vi.fn().mockResolvedValue([summary]), - load: vi.fn().mockResolvedValue(project), + overrides: { + list: vi.fn().mockResolvedValue([summary]), + load: vi.fn().mockResolvedValue(project), + }, }); return { repository, summary, project }; } @@ -150,8 +141,12 @@ describe("Accessibility and UX", () => { }; let resolveList: (v: readonly ProjectSummary[]) => void = () => {}; const repository = createStubRepository({ - list: vi.fn().mockReturnValue(new Promise((res) => (resolveList = res))), - load: vi.fn().mockResolvedValue(project), + overrides: { + list: vi + .fn() + .mockReturnValue(new Promise((res) => (resolveList = res))), + load: vi.fn().mockResolvedValue(project), + }, }); renderDashboard(repository); expect(screen.getByRole("status")).toBeInTheDocument(); @@ -200,21 +195,7 @@ describe("Accessibility and UX", () => { it("moves focus to the workspace on project selection but not on a subsequent refresh", async () => { const project = makeTestProject(); - const summary: ProjectSummary = { - id: project.id, - name: project.name, - description: project.description, - taskCount: project.tasks.length, - goalCount: project.goals.length, - }; - let currentProject = project; - const repository = createStubRepository({ - list: vi.fn().mockResolvedValue([summary]), - load: vi.fn().mockResolvedValue(currentProject), - save: vi - .fn() - .mockImplementation(async (p: Project) => (currentProject = p)), - }); + const repository = createStubRepository({ initialProject: project }); renderDashboard(repository); await waitFor(() => { From 3705773f7ae5de416e4461517562b43eb301b42e Mon Sep 17 00:00:00 2001 From: 67Midas Date: Sat, 5 Sep 2026 13:13:40 -0500 Subject: [PATCH 04/11] fix(ui): drive task status dropdowns from domain constant TaskForm and TaskList hard-coded STATUS_OPTIONS, duplicating the domain's ALL_TASK_STATUSES. Both now render options from the domain constant and resolve select changes with a typed find() instead of 'as TaskStatus' casts. DependencyGraph's STATUS_COLORS is now keyed by TaskStatus (no Record fallback) and node fill uses a typed lookup. Added tests asserting every domain status appears in the TaskForm and TaskList dropdowns. --- src/ui/DependencyGraph.tsx | 7 ++++--- src/ui/TaskForm.test.tsx | 8 ++++++++ src/ui/TaskForm.tsx | 16 ++++++---------- src/ui/TaskList.test.tsx | 35 ++++++++++++++++++++++++++++++++++- src/ui/TaskList.tsx | 20 ++++++++------------ 5 files changed, 60 insertions(+), 26 deletions(-) diff --git a/src/ui/DependencyGraph.tsx b/src/ui/DependencyGraph.tsx index e49edc4..0abd901 100644 --- a/src/ui/DependencyGraph.tsx +++ b/src/ui/DependencyGraph.tsx @@ -1,10 +1,11 @@ import { useMemo } from "react"; +import { TaskStatus } from "../domain/index.js"; import type { Task } from "../domain/index.js"; import type { DependencyGraph } from "../domain/index.js"; import type { ScheduleResult } from "../domain/index.js"; import { computeLayout, NODE_WIDTH, NODE_HEIGHT } from "./graph-layout.js"; -const STATUS_COLORS: Record = { +const STATUS_COLORS: Record = { DONE: "#22c55e", IN_PROGRESS: "#3b82f6", TODO: "#f59e0b", @@ -91,8 +92,8 @@ export function DependencyGraphVisualization({ {layout.nodes.map((node) => { const isCritical = criticalPathSet.has(node.taskId); const task = tasks.find((t) => t.id === node.taskId); - const status = task?.status ?? "BACKLOG"; - const fillColor = STATUS_COLORS[status] ?? "#94a3b8"; + const status = task?.status ?? TaskStatus.BACKLOG; + const fillColor = STATUS_COLORS[status]; return ( { it("renders the form fields", () => { @@ -15,6 +16,13 @@ describe("TaskForm", () => { expect(screen.getByTestId("add-task-button")).toBeInTheDocument(); }); + it("offers every domain task status in the status dropdown", () => { + render(); + const select = screen.getByTestId("task-status-input") as HTMLSelectElement; + const optionValues = [...select.options].map((option) => option.value); + expect(optionValues).toEqual(ALL_TASK_STATUSES); + }); + it("calls onSubmit with form data", () => { const onSubmit = vi.fn(); render(); diff --git a/src/ui/TaskForm.tsx b/src/ui/TaskForm.tsx index 657c79c..1527d68 100644 --- a/src/ui/TaskForm.tsx +++ b/src/ui/TaskForm.tsx @@ -1,14 +1,7 @@ import { useState } from "react"; +import { ALL_TASK_STATUSES } from "../domain/index.js"; import type { TaskStatus } from "../domain/index.js"; -const STATUS_OPTIONS: readonly TaskStatus[] = [ - "BACKLOG", - "TODO", - "IN_PROGRESS", - "BLOCKED", - "DONE", -]; - export interface TaskFormProps { existingTaskIds: readonly string[]; onSubmit: (input: { @@ -164,10 +157,13 @@ export function TaskForm({ existingTaskIds, onSubmit }: TaskFormProps) { - onUpdateTaskStatus(task.id, e.target.value as TaskStatus) - } + onChange={(e) => { + const next = ALL_TASK_STATUSES.find( + (s) => s === e.target.value, + ); + if (next !== undefined) onUpdateTaskStatus(task.id, next); + }} aria-label={`Status for ${task.id}`} data-testid={`task-status-${task.id}`} > - {STATUS_OPTIONS.map((s) => ( + {ALL_TASK_STATUSES.map((s) => ( From f336eda1995e826b2ad56e8c431814ced55ad887 Mon Sep 17 00:00:00 2001 From: 67Midas Date: Sat, 5 Sep 2026 13:16:34 -0500 Subject: [PATCH 05/11] fix(persistence): freeze serialized dependencies and reject non-string entries serializeTask now copies and freezes the task dependencies array so a caller mutating their own array cannot alias into an earlier serialized output. deserializeTask no longer silently filters non-string dependency entries: an array containing one is rejected with a descriptive error, matching the deterministic-rejection stance of the schema validator. ADR-010 documents the chosen semantics. Adds tests for alias-freedom, rejection of non-string entries, and acceptance of valid string entries. --- docs/decisions.md | 1 + src/infrastructure/local-repository.test.ts | 37 +++++++++++++++++++++ src/infrastructure/serialization.ts | 20 +++++++++-- 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/docs/decisions.md b/docs/decisions.md index c36e1fe..1e6148c 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -243,6 +243,7 @@ TASK-008 requires persisting projects locally behind a repository abstraction. T - **Serialization format**: `ProjectData` in `src/infrastructure/serialization.ts` mirrors the domain model with an added `schemaVersion` field. Version 1 is the initial format. Serialized output is deeply frozen. - **Schema validation**: deserialization validates `schemaVersion` strictly — older versions are rejected (no implicit migration), newer versions are rejected (data may be incomparable), and the current version proceeds with field validation. - **Field validation**: every required field is type-checked; missing optional fields fall back to domain defaults via `createTask`, `createGoal`, and `createProject` factories — the same invariant validation used elsewhere. +- **Dependency entry validation** (TASK-022): a task's `dependencies` field, when present, must be an array of strings. A missing or non-array `dependencies` value falls back to the domain default (empty list), consistent with the optional-field convention above. An array containing a non-string entry is **rejected** with a descriptive error rather than silently filtered — dropping entries on load would quietly change the persisted project state. Serialization copies and freezes `dependencies`, so mutating the caller's task array after `serialize` cannot alias into the stored output (the serialized output is deeply frozen per the format contract). - **Corrupted data handling**: `list()` skips entries that fail JSON parsing or deserialization rather than failing the entire list. `load()` propagates deserialization errors to the caller. - **Project summaries**: `list()` returns lightweight `ProjectSummary` objects (id, name, description, task count, goal count) sorted by id, avoiding deserialization of full project graphs when only metadata is needed. diff --git a/src/infrastructure/local-repository.test.ts b/src/infrastructure/local-repository.test.ts index dc16fee..37ebab4 100644 --- a/src/infrastructure/local-repository.test.ts +++ b/src/infrastructure/local-repository.test.ts @@ -188,6 +188,43 @@ describe("Serialization", () => { expect(task!.dependencies).toEqual([]); }); + it("copies and freezes dependencies into serialized output", () => { + const dependencies = ["task-1"]; + const project = createProject({ + id: "proj-1", + name: "Test", + tasks: [createTask({ id: "t1", title: "T1", dependencies })], + }); + const data = serialize(project); + expect(Object.isFrozen(data.tasks[0].dependencies)).toBe(true); + // Mutating the caller's array must not affect earlier serialized output. + dependencies.push("task-2"); + expect(data.tasks[0].dependencies).toEqual(["task-1"]); + }); + + it("rejects tasks with non-string dependency entries", () => { + const project = sampleProject(); + const data = { + ...serialize(project), + tasks: [{ id: "t1", title: "T1", dependencies: ["a", 42, "b"] }], + }; + expect(() => deserialize(data)).toThrow( + "Invalid task data: dependency entries must be strings", + ); + }); + + it("accepts tasks with valid string dependency entries", () => { + const project = sampleProject(); + const data = { + ...serialize(project), + tasks: [{ id: "t1", title: "T1", dependencies: ["a", "b"] }], + }; + const restored = deserialize(data); + const task = restored.tasks.find((t) => t.id === "t1"); + expect(task).toBeDefined(); + expect(task!.dependencies).toEqual(["a", "b"]); + }); + it("defaults missing goal description to empty string", () => { const project = sampleProject(); const data = { diff --git a/src/infrastructure/serialization.ts b/src/infrastructure/serialization.ts index fdd71c7..07050ca 100644 --- a/src/infrastructure/serialization.ts +++ b/src/infrastructure/serialization.ts @@ -58,7 +58,9 @@ function serializeTask(task: Task): TaskData { estimatedEffort: task.estimatedEffort, confidence: task.confidence, goalId: task.goalId, - dependencies: task.dependencies, + // Copy so later mutation of the caller's array cannot alias into the + // serialized output, then freeze to match the deep-freeze contract. + dependencies: Object.freeze([...task.dependencies]), }); } @@ -172,11 +174,25 @@ function deserializeTask(data: unknown): Task { confidence: typeof obj.confidence === "number" ? obj.confidence : undefined, goalId: typeof obj.goalId === "string" ? obj.goalId : undefined, dependencies: Array.isArray(obj.dependencies) - ? obj.dependencies.filter((d): d is string => typeof d === "string") + ? deserializeDependencies(obj.dependencies) : undefined, }); } +function deserializeDependencies(value: unknown[]): readonly string[] { + const dependencies: string[] = []; + for (const entry of value) { + if (typeof entry !== "string") { + throw new Error( + "Invalid task data: dependency entries must be strings, got " + + `${entry === null ? "null" : typeof entry}`, + ); + } + dependencies.push(entry); + } + return Object.freeze(dependencies); +} + const VALID_STATUSES = new Set(ALL_TASK_STATUSES); function isValidTaskStatus(value: unknown): value is TaskStatus { From 0cba7caa89596880c032e4a321218b9a998172d1 Mon Sep 17 00:00:00 2001 From: 67Midas Date: Sat, 5 Sep 2026 13:20:23 -0500 Subject: [PATCH 06/11] fix(application): reject duplicate project ids in createProject --- src/application/project-service.test.ts | 38 +++++++++++++++++++++++-- src/application/project-service.ts | 4 +++ src/ui/Dashboard.test.tsx | 27 ++++++++++++++++++ 3 files changed, 66 insertions(+), 3 deletions(-) diff --git a/src/application/project-service.test.ts b/src/application/project-service.test.ts index 8ca8d86..5972569 100644 --- a/src/application/project-service.test.ts +++ b/src/application/project-service.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from "vitest"; import { ProjectService } from "./project-service.js"; +import { createSampleProject } from "../ui/sample-data.js"; import { createStubRepository } from "../test-support/index.js"; describe("ProjectService", () => { @@ -18,14 +19,45 @@ describe("ProjectService", () => { expect(project.goals).toEqual([]); }); - it("overwrites existing project with same id", async () => { + it("rejects a duplicate project id with a descriptive error", async () => { const repo = createStubRepository(); const service = new ProjectService(repo); await service.createProject({ id: "p1", name: "First" }); - await service.createProject({ id: "p1", name: "Second" }); + + await expect( + service.createProject({ id: "p1", name: "Second" }), + ).rejects.toThrow("Project already exists: p1"); + }); + + it("keeps the original project when a duplicate id is rejected", async () => { + const repo = createStubRepository(); + const service = new ProjectService(repo); + await service.createProject({ + id: "p1", + name: "First", + description: "original", + }); + + await expect( + service.createProject({ id: "p1", name: "Second" }), + ).rejects.toThrow("Project already exists: p1"); const loaded = await service.getProject("p1"); - expect(loaded!.name).toBe("Second"); + expect(loaded!.name).toBe("First"); + expect(loaded!.description).toBe("original"); + }); + + it("rejects re-seeding the sample project", async () => { + const repo = createStubRepository(); + const service = new ProjectService(repo); + await service.createProject(createSampleProject()); + + await expect( + service.createProject(createSampleProject()), + ).rejects.toThrow("Project already exists: sample-project"); + + const loaded = await service.getProject("sample-project"); + expect(loaded!.name).toBe("Trajectory Demo"); }); }); diff --git a/src/application/project-service.ts b/src/application/project-service.ts index 6b9bd53..db30fa1 100644 --- a/src/application/project-service.ts +++ b/src/application/project-service.ts @@ -10,6 +10,10 @@ export class ProjectService { } async createProject(input: CreateProjectInput): Promise { + const existing = await this.repository.load(input.id); + if (existing !== null) { + throw new Error(`Project already exists: ${input.id}`); + } const project = createProject(input); await this.repository.save(project); return project; diff --git a/src/ui/Dashboard.test.tsx b/src/ui/Dashboard.test.tsx index 687bca8..39bcc14 100644 --- a/src/ui/Dashboard.test.tsx +++ b/src/ui/Dashboard.test.tsx @@ -319,4 +319,31 @@ describe("Dashboard", () => { }); expect(screen.getByTestId("recommendation-card")).toHaveTextContent("t2"); }); + + it("surfaces an error when re-seeding the sample project instead of overwriting", async () => { + render( + , + ); + + const seedButton = screen.getByTestId("seed-sample-button"); + fireEvent.click(seedButton); + + await waitFor(() => { + expect(screen.getByText(/Trajectory Demo/)).toBeInTheDocument(); + }); + + fireEvent.click(seedButton); + + await waitFor(() => { + expect(screen.getByRole("alert")).toHaveTextContent( + /Project already exists: sample-project/, + ); + }); + }); }); From a9b4ab824965a955234ad345d1da3840d7f2fd3f Mon Sep 17 00:00:00 2001 From: 67Midas Date: Sat, 5 Sep 2026 13:23:38 -0500 Subject: [PATCH 07/11] test(integration): lock demo-story outcome to the seeded sample project --- .../recommendation-scenario.test.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/integration/recommendation-scenario.test.ts b/src/integration/recommendation-scenario.test.ts index ac8618e..c0c7a65 100644 --- a/src/integration/recommendation-scenario.test.ts +++ b/src/integration/recommendation-scenario.test.ts @@ -114,6 +114,24 @@ describe("sample project through the real stack (integration)", () => { expect(second.taskId).not.toBeNull(); expect(second.taskId).not.toBe(first.taskId); }); + + it("locks the demo-story outcome to the seeded sample project", async () => { + const services = createServices(); + const input = createSampleProject(); + await services.projectService.createProject(input); + + // The demo story (recommendation, graph shape, critical path) must be + // derived from the single seeded sample, not duplicated test fixtures. + const rec = await services.recommendationService.getRecommendation( + input.id, + ); + expect(rec.taskId).toBe("t4"); + expect(rec.factors).toHaveLength(6); + + const graph = await services.recommendationService.getGraph(input.id); + expect(graph.tasks).toHaveLength(8); + expect(graph.schedule.criticalPath).toEqual(["t1", "t2", "t4", "t5", "t8"]); + }); }); describe("cross-service state sharing through one repository (integration)", () => { From 4c10d3bc970a2565b496ad817f54d5fac494171f Mon Sep 17 00:00:00 2001 From: 67Midas Date: Sat, 5 Sep 2026 13:30:15 -0500 Subject: [PATCH 08/11] test(benchmark): make report emission a hard contract (TASK-024) --- benchmark/benchmark.test.ts | 45 ++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/benchmark/benchmark.test.ts b/benchmark/benchmark.test.ts index 6222c4f..0f1bb24 100644 --- a/benchmark/benchmark.test.ts +++ b/benchmark/benchmark.test.ts @@ -122,27 +122,42 @@ describe("benchmark harness output", () => { // Emit the promised report as an explicit artifact so `npm run benchmark` // always produces a results table even when console output is captured. + // A failure to emit the report must FAIL the suite: CI treats the artifact + // as mandatory (`if-no-files-found: error`), so a local pass that silently + // skipped the write would only surface late and confusingly in the upload. expect(results.length).toBeGreaterThan(0); writeReport(results); }, 180_000); + + it("fails, rather than passing silently, when the report cannot be written", () => { + const sample: OperationResult = { + operation: "graph-construction", + taskCount: 100, + meanMs: 1, + minMs: 1, + iterations: 1, + }; + + // A path whose parent is an existing file makes mkdirSync throw, simulating + // an unwritable report location. This must surface as a failure. + const badPath = fileURLToPath( + new URL("./results.txt/unwritable.txt", import.meta.url), + ); + expect(() => writeResultsFile([sample], badPath)).toThrow(); + + // The same result writes cleanly to the canonical report location. + expect(() => writeResultsFile([sample], RESULTS_FILE)).not.toThrow(); + expect(readFileSync(RESULTS_FILE, "utf8")).toContain("Total measurements"); + }); }); function writeReport(results: readonly OperationResult[]): void { - try { - if (existsSync(RESULTS_FILE)) unlinkSync(RESULTS_FILE); - writeResultsFile(results, RESULTS_FILE); - const written = readFileSync(RESULTS_FILE, "utf8"); - expect(written).toContain("Total measurements"); - expect(written).toContain("mean (ms)"); - console.log("\n" + written); - } catch (err) { - // Writing the report is a best-effort artifact; a failure to persist should - // not hide correctness failures in the benchmark itself. - console.warn( - "Benchmark report could not be written to disk:", - err instanceof Error ? err.message : err, - ); - } + if (existsSync(RESULTS_FILE)) unlinkSync(RESULTS_FILE); + writeResultsFile(results, RESULTS_FILE); + const written = readFileSync(RESULTS_FILE, "utf8"); + expect(written).toContain("Total measurements"); + expect(written).toContain("mean (ms)"); + console.log("\n" + written); } describe("benchmark dependent operations", () => { From f6a03e8640e2de0b04b54e9e21f264a64d248776 Mon Sep 17 00:00:00 2001 From: 67Midas Date: Sat, 5 Sep 2026 13:30:15 -0500 Subject: [PATCH 09/11] feat(simulation): report blocked-task and risk deltas (TASK-025) --- docs/case-study.md | 4 +- docs/decisions.md | 6 ++- src/domain/simulation/simulation.test.ts | 62 ++++++++++++++++++++++++ src/domain/simulation/simulation.ts | 28 +++++++++++ src/ui/ScenarioPanel.test.tsx | 29 +++++++++++ src/ui/ScenarioPanel.tsx | 20 ++++++++ 6 files changed, 145 insertions(+), 4 deletions(-) diff --git a/docs/case-study.md b/docs/case-study.md index a28c4d6..8f69a15 100644 --- a/docs/case-study.md +++ b/docs/case-study.md @@ -61,7 +61,7 @@ The dependency graph (`src/domain/graph/dependency-graph.ts`) is constructed fro - **Direct lookup**: `getPrerequisites(id)` and `getDependents(id)` — O(1) via pre-built adjacency maps - **Transitive traversal**: `getAllPrerequisites(id)` and `getAllDependents(id)` — BFS, returning sorted results -- **Reachability**: `isReachable(from, to)` — BFS with short-circuit +- **Reachability**: `isReachable(from, to)` — BFS with short-circuit. `isReachable(t, t)` follows the self-reachability convention: it is `false` for acyclic graphs (no task reaches itself through an edge) and `true` only for tasks that are part of a cycle. - **Cycle detection**: `hasCycle()` and `getCyclicTaskIds()` — checks self-reachability for each node (BFS from a node back to itself), which yields precisely the tasks on cycles, excluding tasks merely downstream - **Topological ordering**: Kahn's algorithm with a lexicographically sorted ready queue — deterministic, independent of input ordering @@ -213,7 +213,7 @@ A fourth type (deadline change) is deferred until a date model exists. ### 7.4 Comparison Output -Each side (baseline, projected) exposes: `projectDuration`, `criticalPath`, `recommendedTaskId`, `recommendedScore`. Deltas: `durationDelta`, `criticalPathChanged`, `recommendationChanged`, and `valueRemoved` (for de-scope only). +Each side (baseline, projected) exposes: `projectDuration`, `criticalPath`, `recommendedTaskId`, `recommendedScore`, `blockedTaskCount`. Deltas: `durationDelta`, `blockedTaskDelta`, `newlyCriticalTaskIds` (the project's risk indicator — tasks whose slack fell to zero), `criticalPathChanged`, `recommendationChanged`, and `valueRemoved` (for de-scope only). **Affected downstream**: the target plus its transitive dependents, filtered to tasks whose `[earliestStart, earliestFinish]` window actually changed. Merely being downstream of the change is not enough — slack absorbs some changes. diff --git a/docs/decisions.md b/docs/decisions.md index 1e6148c..54564a0 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -128,7 +128,7 @@ TASK-004 requires calculating deterministic scheduling information from task dur ### Consequences - Scheduling is deterministic and reproducible for identical inputs. -- The algorithm runs in O(V + E) time per pass (two passes total). +- Each pass (forward and backward) is O(V + E) as a linear sweep over the topological order. The function as a whole is dominated by the deterministic topological sort it invokes (Kahn's algorithm with a lexicographically sorted ready queue), which carries queue-sorting overhead per insertion. - No external scheduling library is used. - The domain remains framework-independent. - Fractional effort values are supported (e.g., 1.5 days). @@ -212,7 +212,9 @@ TASK-007 requires comparing a baseline project state with deterministic what-if - **Derivation over mutation**: `applyScenario(tasks, scenario)` returns a new array. Only the targeted task is rebuilt via `createTask`; untouched task objects keep their identity (`===`). For `remove-task`, surviving tasks keep identity unless they referenced the removed task, in which case that dependency entry is stripped — de-scoping must leave a constructible graph, and silently dropping the edge is preferable to rejecting the scenario or leaving dangling references. - **Reuse of domain layers**: `simulateScenario` builds baseline and projected state through the existing `createDependencyGraph`, `calculateSchedule`, and `recommendNextTask` functions. No scheduling or scoring logic is duplicated, so scenarios automatically inherit CPM semantics (ADR-006) and the deterministic engine (ADR-007/008), including custom factor-set pass-through. - **Affected downstream**: the target plus its transitive dependents, filtered to tasks whose `[earliestStart, earliestFinish]` window actually changed between baseline and projected schedules, sorted lexicographically. "Affected" means a measurable schedule change — merely being downstream of the change is not enough. A removed task never appears (it does not survive into the projection). -- **Comparison shape**: each side (`baseline`, `projected`) exposes `projectDuration`, `criticalPath`, `recommendedTaskId`, `recommendedScore`. Deltas: `durationDelta` (rounded to three decimals, matching engine precision), `criticalPathChanged` (ordered id-sequence equality), `recommendationChanged` (selected id equality), and `valueRemoved` (target value, present only for `remove-task`). +- **Comparison shape**: each side (`baseline`, `projected`) exposes `projectDuration`, `criticalPath`, `recommendedTaskId`, `recommendedScore`, and `blockedTaskCount`. Deltas: `durationDelta` (rounded to three decimals, matching engine precision), `blockedTaskDelta`, `newlyCriticalTaskIds`, `criticalPathChanged` (ordered id-sequence equality), `recommendationChanged` (selected id equality), and `valueRemoved` (target value, present only for `remove-task`). +- **Blocked-task count** (PROJECT_PLAN §10): per side, the number of non-DONE tasks with at least one non-DONE prerequisite, counted per side and reported as `blockedTaskDelta` (projected − baseline). Blocking is derived from the graph, never from the informational `BLOCKED` status flag, matching eligibility semantics (ADR-007). De-scoping a prerequisite therefore lowers the count (its dependency edges are stripped), while delay/effort scenarios never change it — all without date or status mutations. +- **Risk indicator** (PROJECT_PLAN §10): `newlyCriticalTaskIds` — the tasks on the projected critical path that were not on the baseline critical path, sorted lexicographically. A task that becomes newly critical had its slack exhausted (fell to zero), which is the single deterministic signal that best captures schedule risk for a given scenario. No probabilistic or date-based analysis is introduced. - **Determinism and immutability**: the result and its nested arrays are frozen; `scenarioTasks` is sorted by id so serialization is independent of input order; output is verified by JSON-equality across repeated and reordered runs. Cyclic inputs throw through the normal scheduling path — no special handling. ### Alternatives considered diff --git a/src/domain/simulation/simulation.test.ts b/src/domain/simulation/simulation.test.ts index f82abf0..3e03a63 100644 --- a/src/domain/simulation/simulation.test.ts +++ b/src/domain/simulation/simulation.test.ts @@ -249,6 +249,67 @@ describe("simulateScenario — recommendation comparison", () => { }); }); +describe("simulateScenario — blocked-task delta", () => { + function blockedBaseline(): Task[] { + return [ + task("root", { status: "DONE" }), + task("mid", { status: "TODO", dependencies: ["root"] }), + task("leaf", { status: "TODO", dependencies: ["mid"] }), + task("solo", { status: "BACKLOG" }), + ]; + } + + it("counts blocked tasks per side (non-DONE with a non-DONE prerequisite)", () => { + const result = simulateScenario(blockedBaseline(), { + kind: "delay-task", + taskId: "root", + additionalEffort: 1, + }); + expect(result.baseline.blockedTaskCount).toBe(1); + expect(result.projected.blockedTaskCount).toBe(1); + expect(result.blockedTaskDelta).toBe(0); + }); + + it("reports a lower blocked count when de-scoping removes a blocking prerequisite", () => { + const result = simulateScenario(blockedBaseline(), { + kind: "remove-task", + taskId: "mid", + }); + expect(result.baseline.blockedTaskCount).toBe(1); + expect(result.projected.blockedTaskCount).toBe(0); + expect(result.blockedTaskDelta).toBe(-1); + }); +}); + +describe("simulateScenario — risk indicators", () => { + const schedule = (): Task[] => [ + task("critical", { estimatedEffort: 5 }), + task("side", { estimatedEffort: 1 }), + task("join", { estimatedEffort: 1, dependencies: ["critical", "side"] }), + ]; + + it("reports tasks that became newly critical when slack is exhausted", () => { + const result = simulateScenario(schedule(), { + kind: "delay-task", + taskId: "side", + additionalEffort: 5, + }); + expect(result.baseline.criticalPath).toEqual(["critical", "join"]); + expect(result.projected.criticalPath).toEqual(["side", "join"]); + expect(result.newlyCriticalTaskIds).toEqual(["side"]); + }); + + it("reports no newly critical tasks when a delay is absorbed by slack", () => { + const result = simulateScenario(schedule(), { + kind: "delay-task", + taskId: "side", + additionalEffort: 2, + }); + expect(result.durationDelta).toBe(0); + expect(result.newlyCriticalTaskIds).toEqual([]); + }); +}); + describe("simulateScenario — determinism and immutability", () => { it("produces identical results across repeated runs and input order", () => { const build = (): Task[] => [ @@ -290,6 +351,7 @@ describe("simulateScenario — determinism and immutability", () => { expect(Object.isFrozen(result.projected)).toBe(true); expect(Object.isFrozen(result.affectedDownstreamTaskIds)).toBe(true); expect(Object.isFrozen(result.scenarioTasks)).toBe(true); + expect(Object.isFrozen(result.newlyCriticalTaskIds)).toBe(true); expect(Object.isFrozen(result.baseline.criticalPath)).toBe(true); }); }); diff --git a/src/domain/simulation/simulation.ts b/src/domain/simulation/simulation.ts index 6277a35..c08fe5f 100644 --- a/src/domain/simulation/simulation.ts +++ b/src/domain/simulation/simulation.ts @@ -33,6 +33,7 @@ export interface SimulationSide { readonly criticalPath: readonly string[]; readonly recommendedTaskId: string | null; readonly recommendedScore: number | null; + readonly blockedTaskCount: number; } export interface SimulationResult { @@ -41,6 +42,8 @@ export interface SimulationResult { readonly baseline: SimulationSide; readonly projected: SimulationSide; readonly durationDelta: number; + readonly blockedTaskDelta: number; + readonly newlyCriticalTaskIds: readonly string[]; readonly criticalPathChanged: boolean; readonly recommendationChanged: boolean; readonly affectedDownstreamTaskIds: readonly string[]; @@ -129,9 +132,27 @@ function buildSide( criticalPath: Object.freeze([...schedule.criticalPath]), recommendedTaskId: recommendation.taskId, recommendedScore: recommendation.score, + blockedTaskCount: countBlockedTasks(tasks, graph), }); } +function countBlockedTasks( + tasks: readonly Task[], + graph: DependencyGraph, +): number { + // A task is blocked when it is not DONE and at least one prerequisite is not + // DONE. Blocking is derived from the graph (as in eligibility), never from + // the informational BLOCKED status flag. + const statusByTaskId = new Map(tasks.map((t) => [t.id, t.status])); + return tasks.filter( + (task) => + task.status !== "DONE" && + graph + .getPrerequisites(task.id) + .some((prereq) => statusByTaskId.get(prereq) !== "DONE"), + ).length; +} + type ScheduleWindow = Pick< ScheduleResult["taskSchedules"][number], "earliestStart" | "earliestFinish" @@ -206,12 +227,19 @@ export function simulateScenario( a.id.localeCompare(b.id), ); + const baselineCritical = new Set(baselineSchedule.criticalPath); + const newlyCriticalTaskIds = scenarioSchedule.criticalPath + .filter((taskId) => !baselineCritical.has(taskId)) + .sort((a, b) => a.localeCompare(b)); + return Object.freeze({ scenario, scenarioTasks: Object.freeze(orderedScenarioTasks), baseline, projected, durationDelta: round(projected.projectDuration - baseline.projectDuration), + blockedTaskDelta: projected.blockedTaskCount - baseline.blockedTaskCount, + newlyCriticalTaskIds: Object.freeze(newlyCriticalTaskIds), criticalPathChanged: baseline.criticalPath.join("\u0000") !== projected.criticalPath.join("\u0000"), diff --git a/src/ui/ScenarioPanel.test.tsx b/src/ui/ScenarioPanel.test.tsx index c7af444..d22a31e 100644 --- a/src/ui/ScenarioPanel.test.tsx +++ b/src/ui/ScenarioPanel.test.tsx @@ -173,6 +173,35 @@ describe("ScenarioPanel", () => { expect(screen.getByTestId("value-removed")).toHaveTextContent("8"); }); + it("reports blocked-task and newly-critical deltas in the comparison", async () => { + const project = makeTestProject(); + vi.mocked(repository.load).mockResolvedValue(project); + + render( + , + ); + + // Delaying nothing on the critical path keeps blocking unchanged. + fireEvent.change(screen.getByTestId("scenario-task-select"), { + target: { value: "t2" }, + }); + fireEvent.change(screen.getByTestId("scenario-amount-input"), { + target: { value: "10" }, + }); + fireEvent.click(screen.getByTestId("run-scenario-button")); + + await waitFor(() => { + expect(screen.getByTestId("scenario-comparison")).toBeInTheDocument(); + }); + + expect(screen.getByTestId("blocked-task-delta")).toHaveTextContent("0"); + expect(screen.getByTestId("newly-critical")).toHaveTextContent("none"); + }); + it("rejects a non-positive delay amount", async () => { const project = makeTestProject(); vi.mocked(repository.load).mockResolvedValue(project); diff --git a/src/ui/ScenarioPanel.tsx b/src/ui/ScenarioPanel.tsx index b42e958..5a10d81 100644 --- a/src/ui/ScenarioPanel.tsx +++ b/src/ui/ScenarioPanel.tsx @@ -196,6 +196,26 @@ function ScenarioComparison({ {formatDelta(result.durationDelta)} + + Blocked tasks + {baseline.blockedTaskCount} + {projected.blockedTaskCount} + + {formatDelta(result.blockedTaskDelta)} + + + + Newly critical tasks + — + + {result.newlyCriticalTaskIds.length > 0 + ? result.newlyCriticalTaskIds.join(", ") + : "none"} + + + {result.newlyCriticalTaskIds.length > 0 ? "at risk" : "—"} + + Critical path {baseline.criticalPath.join(", ")} From 4a9a4e5f1dd5ced1f1f42b043d8325ece8b492fa Mon Sep 17 00:00:00 2001 From: 67Midas Date: Sat, 5 Sep 2026 13:46:16 -0500 Subject: [PATCH 10/11] docs: complete review remediation records and cleanup (TASK-026) --- .gitignore | 1 - docs/handoff.md | 15 ++- docs/progress.md | 16 +++ docs/task-006-review.md | 11 ++ docs/tasks.md | 222 ++++++++++++++++++++++++++++++++++++++- src/ui/ScenarioPanel.tsx | 4 +- 6 files changed, 258 insertions(+), 11 deletions(-) diff --git a/.gitignore b/.gitignore index d6ca18a..7ae59a7 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,5 @@ .DS_Store node_modules/ -.next/ dist/ coverage/ .env diff --git a/docs/handoff.md b/docs/handoff.md index cfff275..bb28f02 100644 --- a/docs/handoff.md +++ b/docs/handoff.md @@ -6,11 +6,11 @@ All planned phases complete. Domain foundation, dependency graph, scheduling and ## Current Task -TASK-017 — Architecture case study and final documentation (DONE). All acceptance criteria satisfied: case study written, ADR-013 added for project naming/availability, resume story updated with actual measurements, architecture.md and README.md updated to reference the case study. +TASK-018 through TASK-026 (the full-branch code-review remediation backlog) are implemented, verified, marked DONE, and pushed on `fix/review-remediation-018-026`. The branch is pending human review and merge. See "Next Recommended Action". ## State -TASK-017 adds `docs/case-study.md` — a comprehensive architecture case study covering the core problem, domain model, dependency graph, scheduling/CPM, decision engine, explainability, scenario simulation, architecture/layering, persistence, testing strategy, measured performance (with full benchmark table from `benchmark/results.txt`), CI/CD, design tradeoffs, and repository structure. ADR-013 resolves the project naming/availability decision: product name is Trajectory, repository is `skibkitty/trajectory-project` on GitHub, availability is source code on GitHub (no deployment target for MVP). PROJECT_PLAN.md §24 resume story updated with concrete claims based on actual benchmark results and test counts. README.md and docs/architecture.md updated to reference the case study. TASK-016 is merged to main (PR #18) and marked DONE; 287 correctness/component/integration tests and 2 E2E specs pass locally, `npm run build` succeeds, and `npm run benchmark` passes (6 tests). +The review-remediation branch `fix/review-remediation-018-026` contains all nine remediation tasks: deep-frozen warning `affectedTaskIds` arrays (TASK-018), a shared `toCreateTaskInput` mapper (TASK-019), shared `createStubRepository`/`createInMemoryStorage` test helpers (TASK-020), UI status options driven from `ALL_TASK_STATUSES` with a typed `STATUS_COLORS` and no `as TaskStatus` casts (TASK-021), serialization that copies+freezes `dependencies` and rejects non-string dependency entries (TASK-022), a duplicate-project-id guard in `createProject` including the sample seed and dashboard error surfacing (TASK-023), a hard benchmark-report emission contract (TASK-024), simulator blocked-task and newly-critical risk deltas documented in ADR-009 and surfaced in the scenario panel (TASK-025), and documentation cleanup (archived `task-006-review.md`, corrected stale counts, `isReachable`/complexity/determinism notes, `.next/` removed from `.gitignore`). Earlier state: TASK-017 DONE, TASK-016 merged to main (PR #18). ## Completed @@ -37,7 +37,7 @@ TASK-017 adds `docs/case-study.md` — a comprehensive architecture case study c - Scheduling: forward/backward pass CPM, critical path identification, slack calculation - Scheduling tests (13 tests = 71 total passing) - Decision engine: eligibility rules, composable additive scoring (six default factors), deterministic lexicographic tie-breaking, structured factor breakdowns, frozen results -- Decision engine tests (31 tests = 102 total passing across 8 files) +- Decision engine tests (34 tests = 105 total passing across 8 files) - ADR-007 documenting eligibility, scoring model, normalization, tie-breaking, and selection policy; ADR-004 marked Accepted - Recommendation explainability: `recommendNextTask` with machine-readable factor ids, fixed-order assumptions, ordered conditional warnings, explainable empty state, frozen deterministic output - Engine additions: stable factor ids on `EvaluationFactor`, normalization `maxValues` exposed on `EvaluationResult` @@ -90,9 +90,11 @@ TASK-017 adds `docs/case-study.md` — a comprehensive architecture case study c - Architecture case study (TASK-017): `docs/case-study.md` covering core problem, domain model, dependency graph, scheduling/CPM, decision engine, explainability, scenario simulation, architecture/layering, persistence, testing strategy, measured performance (full benchmark table), CI/CD, design tradeoffs, and repository structure - ADR-013 resolving project naming and availability (product name: Trajectory, repository: `skibkitty/trajectory-project`, availability: source code on GitHub) - Resume story (PROJECT_PLAN §24) updated with concrete claims based on actual benchmark results and test counts +- Review remediation (TASK-018–TASK-026) on `fix/review-remediation-018-026`: deep-freeze warning `affectedTaskIds`, shared `toCreateTaskInput` mapper, shared `createStubRepository`/`createInMemoryStorage` test helpers, `ALL_TASK_STATUSES`-driven UI dropdowns, versioned-serialization dependency freeze/rejection, `createProject` duplicate-id guard, hard benchmark-report emission contract, simulator blocked-task/newly-critical deltas (ADR-009 + scenario panel), and documentation cleanup ## Not Yet Started +- TASK-018 through TASK-026 (the full-branch review remediation backlog) are implemented, verified, and marked DONE on the `fix/review-remediation-018-026` branch, which is pending human review and merge. See "Next Recommended Action". - Branch protection / required status checks on GitHub (human repository-settings action, not a repo-file change) - Phase 18 — Logging and observability (structured logging for domain, application, and infrastructure layers) @@ -109,7 +111,10 @@ Project naming and availability are resolved per ADR-013: product name Trajector ## Next Recommended Action -TASK-017 is the final task in the current backlog. All planned implementation phases are complete. Possible next steps (not yet defined as tasks): +The full-branch review remediation backlog (TASK-018 through TASK-026) is implemented, verified, and marked DONE on the pushed branch `fix/review-remediation-018-026`. That branch should be reviewed and merged to `main` via pull request (per the standard workflow — wait for human review before merging). It cannot be stacked on any other pending branch; it is independent from main. + +Beyond the remediation backlog, possible future tracks (not yet defined as tasks): +- Property-based testing (PROJECT_PLAN §15; requires a new dev dependency, human approval) - Phase 18 — Logging and observability (structured logging via injected interfaces) - Calendar-based scheduling (date model, deadline scenarios) - Weighted-random selection policy (requires `RandomSource` abstraction) @@ -118,7 +123,7 @@ TASK-017 is the final task in the current backlog. All planned implementation ph ## Verification -TASK-017 verification is complete. Locally: `npm run verify` (typecheck, 287 tests, lint, format:check), `npm run build`, and `npm run benchmark` (6 tests) all pass. `npm run test:e2e` (2 Playwright specs) also passes with browsers installed. All documentation is accurate against the implemented codebase. +The review remediation is verified complete. Locally on `fix/review-remediation-018-026`: `npm run verify` (typecheck, 304 tests, lint, format:check), `npm run build`, and `npm run benchmark` (7 tests) all pass. `npm run test:e2e` (2 Playwright specs) passes with browsers installed. ## Important Constraint diff --git a/docs/progress.md b/docs/progress.md index c999996..7a9210c 100644 --- a/docs/progress.md +++ b/docs/progress.md @@ -447,3 +447,19 @@ Completed: - Net diff: −275 lines of duplicated test helper code; no production code touched - Test count verified unchanged at HEAD and after the change: 290 `it`/`test` blocks (older progress entries citing 287 were already stale relative to HEAD) - Verified: `npm run verify` (typecheck, 290 tests, lint, format:check) and `npm run build` both pass + +## 2026-09-05 — Review remediation backlog completed (TASK-018 through TASK-026, on fix/review-remediation-018-026) + +Completed: +- TASK-018 — deep-frozen the `affectedTaskIds` arrays on the `tie-break-applied` and `blocked-status-eligible` warnings (fresh frozen copies per warning), with a regression test asserting `Object.isFrozen` on the nested arrays and elements +- TASK-019 — extracted the copy-pasted `Task → CreateTaskInput` mapper (`goalId: null → undefined` bridge included) into `src/application/task-input.ts`; task/goal/dependency services now import it instead of redefining it +- TASK-020 — consolidated the ten duplicated `createStubRepository` helpers (plus `createInMemoryStorage`) into `src/test-support/index.ts`; all call sites migrated (see the earlier entry) +- TASK-021 — `TaskForm` and `TaskList` dropdowns now render from the domain `ALL_TASK_STATUSES`; `STATUS_COLORS` in `DependencyGraph` is keyed by `TaskStatus`; `as TaskStatus` casts replaced with a typed `find()`; dropdown coverage tests added +- TASK-022 — `serializeTask` copies and freezes `dependencies` (no caller aliasing); `deserializeTask` rejects non-string dependency entries with a descriptive error instead of silently filtering; ADR-010 documents the chosen semantics; alias-freedom and rejection tests added +- TASK-023 — `ProjectService.createProject` rejects an existing id (`Project already exists: {id}`) including the `sample-project` seed; Dashboard test asserts the error surfaces instead of overwriting +- TASK-024 — the benchmark report write is now a hard contract: `writeReport` propagates write failures (no warn-and-pass), and a new test simulates an unwritable location and asserts failure +- TASK-025 — `SimulationSide` now exposes `blockedTaskCount` and `SimulationResult` adds `blockedTaskDelta` plus `newlyCriticalTaskIds` as the deterministic risk indicator (slack exhausted = becomes critical); definitions recorded in ADR-009; the scenario comparison panel displays both; backward-compatible (new fields only) +- TASK-026 — archived `docs/task-006-review.md` (superseded banner, no live references), corrected stale counts in `docs/tasks.md` (TASK-015: 286→287) and `docs/handoff.md` (engine tests 31→34, ADR total now 13), added the `isReachable(t, t)` self-reachability note to the case study, narrowed ADR-006's per-pass complexity claim (dominated by the topological sort), pointed TASK-014's determinism criterion at ADR-011, and removed the unused `.next/` entry from `.gitignore` +- Also: the demo-story outcome is locked by an integration test (`a9b4ab8`) asserting the sample project's deterministic recommendation, factor breakdown, task count, and critical path +- Verified: `npm run verify` (typecheck, 304 tests, lint, format:check), `npm run build`, and `npm run benchmark` (7 tests, with `benchmark/results.txt` written) all pass +- Branch pushed as `fix/review-remediation-018-026`; pending human review and merge (no PR created from automation) diff --git a/docs/task-006-review.md b/docs/task-006-review.md index 5ed7e1f..a7c4cb2 100644 --- a/docs/task-006-review.md +++ b/docs/task-006-review.md @@ -1,5 +1,16 @@ # TASK-006 Implementation Review +> **ARCHIVED — historical record.** +> +> This document was written during the TASK-006 review cycle. Every substantive +> issue it raises has since been resolved: element-level freezing (issue #1) was +> applied in the TASK-006 review follow-up and the `affectedTaskIds` arrays in +> warnings were deep-frozen in TASK-018; the structural type, naming, warning, +> and test-coverage items were addressed in the same follow-up, ADR-008, and +> later remediation tasks (TASK-018–TASK-026). It is retained for history and +> must not be read as a description of the current codebase. See `docs/decisions.md` +> (ADR-008) and `docs/progress.md` for the current contract and state. + ## Summary This document records issues found during a thorough review of the TASK-006 diff --git a/docs/tasks.md b/docs/tasks.md index c23e94b..18a168f 100644 --- a/docs/tasks.md +++ b/docs/tasks.md @@ -318,7 +318,7 @@ Acceptance criteria: - benchmark tests exercise the key domain algorithms (graph construction, cycle detection, topological ordering, dependency traversal, critical-path analysis, decision-engine scoring, scenario simulation) - benchmark datasets include at least 100, 1000, and 5000 tasks - benchmarks report wall-clock time for each operation at each dataset size -- benchmark results are deterministic (same input produces same time-ordered results) +- benchmark results are deterministic (same input produces identical results; wall-clock timings are machine-dependent, so determinism is interpreted as identical domain results — see ADR-011) - benchmarks run as a separate npm script, not as part of the default test suite - domain code has no benchmark-specific dependencies - all verification commands continue to pass @@ -346,7 +346,7 @@ Acceptance criteria: - focus management is correct on route/state changes Verification: -- npm run verify passes (typecheck, 286 tests, lint, format) +- npm run verify passes (typecheck, 287 tests, lint, format) - npm run build succeeds ## TASK-016 — CI/CD @@ -394,3 +394,221 @@ Acceptance criteria: Verification: - review the case study and supporting docs for accuracy against the code - confirm every performance claim traces to a benchmark result in the repo + +## TASK-018 — Fix recommendation deep-freeze gap + +Status: DONE + +Goal: +Honor the ADR-008 immutability contract for recommendation warnings. + +Prerequisites: +None. + +Context: +ADR-008 promises "the recommendation, all of its arrays, and every contained factor/assumption/warning object are frozen." `Object.freeze` on the warning objects is shallow: the `affectedTaskIds` arrays embedded in the `tie-break-applied` and `blocked-status-eligible` warnings are built unfrozen and never frozen, so they remain runtime-mutable. This is the review finding: + +- `tie-break-applied` warning embeds an unfrozen `tied` array. +- `blocked-status-eligible` warning embeds an unfrozen `blockedEligible` array. +- The existing freeze test asserts the warning objects and top-level arrays but never `Object.isFrozen` on the nested `affectedTaskIds` arrays. + +Acceptance criteria: +- every `affectedTaskIds` array on every emitted warning is deeply frozen +- a regression test asserts `Object.isFrozen` on the nested arrays (and elements) for both warnings that carry them +- the commit records which warnings carry `affectedTaskIds` so a future warning type knows what to freeze + +Verification: +- npm run verify passes (typecheck, tests, lint, format) +- the new freeze regression test exercises a tie and a blocked-but-eligible candidate set + +## TASK-019 — Extract shared task-to-input mapper + +Status: DONE + +Goal: +Remove the copy-pasted `toCreateTaskInput` bridge from application services. + +Prerequisites: +None. + +Context: +The identical 12-line `Task → CreateTaskInput` mapper (including the `goalId: null → undefined` bridge) is defined separately in `task-service.ts`, `goal-service.ts`, and `dependency-service.ts`. A change to task fields currently forces edits in three files. Review finding: duplicated code across services. + +Acceptance criteria: +- the mapper lives in exactly one shared location in the application layer +- all three services import it instead of redefining it +- behavior is unchanged (all existing service tests pass without modification of expectations) +- add a small focused test for the `goalId` bridging behavior if one does not already exist + +Verification: +- npm run verify passes (typecheck, tests, lint, format) + +## TASK-020 — Extract shared test helpers + +Status: DONE + +Goal: +Collapse the duplicated `createStubRepository` and `createInMemoryStorage` helpers into one test-support module. + +Prerequisites: +None. + +Context: +`createStubRepository` is re-implemented in roughly ten test files (project/task/goal/dependency/recommendation/scenario service suites plus several UI suites) and `createInMemoryStorage` in at least three more (local-repository, both integration suites). Any future addition to the `ProjectRepository` interface forces edits across those files. Review finding: shotgun surgery on the repository contract. This is additive: new helper module first, then migrate call sites, keeping CI green per batch. + +Acceptance criteria: +- a single test-support module exports both helpers +- every call site imports from that module; no file defines its own copy +- all existing tests pass unchanged in expectation after migration +- the support module lives outside `src/domain` so domain dependency rules are unaffected + +Verification: +- npm run verify passes (typecheck, tests, lint, format) + +## TASK-021 — UI status options from domain constant + +Status: DONE + +Goal: +Drive the UI task-status dropdowns from the domain's `ALL_TASK_STATUSES` instead of a UI-local copy. + +Prerequisites: +None. + +Context: +`TaskForm.tsx` and `TaskList.tsx` each hard-code a local `STATUS_OPTIONS` list duplicating `ALL_TASK_STATUSES` in the domain. Infrastructure was already refactored to consume the domain constant; the UI is the divergent leftover, so adding a status would silently not appear in the dropdowns. Review findings: duplicated constant, plus related nits — `STATUS_COLORS` in the graph component is `Record` masking status typos, and `as TaskStatus` casts trust DOM option values. + +Acceptance criteria: +- both dropdowns render from the domain constant (or an exported UI alias derived from it), so a domain status change flows through automatically +- `STATUS_COLORS` is keyed by `TaskStatus` with no fallback that hides typos +- any `as TaskStatus` casts driven by select values are removed or made type-safe +- existing UI tests still pass; add/adjust a test asserting all domain statuses appear in the dropdown if not already covered + +Verification: +- npm run verify passes (typecheck, tests, lint, format) + +## TASK-022 — Persistence validation and freeze hardening + +Status: DONE + +Goal: +Make serialization honor the ADR-010 "reject or fall back" model and its frozen-output claim. + +Prerequisites: +None. + +Context: +Two review findings in `serialization.ts`: + +- `dependencies` is passed to `TaskData` by reference — neither copied nor frozen — so the "serialized output is deeply frozen" claim holds only at depth 1. It also aliases the caller's mutable domain task. +- invalid (non-string) dependency entries are silently filtered out during deserialization instead of being rejected like other malformed fields or falling back to a default like `dependencies: undefined`. This is an undocumented third behavior in the ADR-010 model, and a corrupted persisted graph silently loses edges. + +Acceptance criteria: +- serialized `dependencies` is copied (and frozen) so serialized output is deeply frozen and does not alias mutable input +- non-string dependency entries cause deterministic rejection (matching the field-validation behavior of other malformed fields) rather than silent filtering; the committed choice is documented in ADR-010 +- tests cover: mutated caller array does not affect earlier serialized output, and rejected vs. valid dependency payloads +- ADR-010 is updated to state the chosen validation semantics for dependency entries + +Verification: +- npm run verify passes (typecheck, tests, lint, format) + +## TASK-023 — Guard against project id collisions + +Status: DONE + +Goal: +Reject duplicate project ids on creation instead of silently overwriting an existing project. + +Prerequisites: +None. + +Context: +The dashboard generates `proj-${Date.now()}` ids and `ProjectService.createProject` never checks whether the id already exists. Two creations in the same millisecond (or a clock collision with an existing saved project) silently overwrite an earlier project. The task/goal/dependency services all guard duplicates; project creation does not. Review finding. The fixed `sample-project` id has the same exposure, so the guard should cover the sample seed path too. + +Acceptance criteria: +- `createProject` rejects an id that already exists with a descriptive error, matching the duplicate guards in the other services +- the guard is covered by a service test (including the sample-project seeded case) +- the dashboard-facing create path surfaces the error rather than overwriting + +Verification: +- npm run verify passes (typecheck, tests, lint, format) + +## TASK-024 — Benchmark report emission as a hard contract + +Status: DONE + +Goal: +Make `npm run benchmark` fail when it cannot emit `benchmark/results.txt`, matching the CI artifact contract. + +Prerequisites: +None. + +Context: +The benchmark suite wraps the report write in a try/catch that only warns, so `npm run benchmark` can pass locally without producing `results.txt`. CI treats the artifact as mandatory (`if-no-files-found: error` on the `benchmark-results` upload), so the failure surfaces late and confusingly. Review finding: local-pass/CI-fail mismatch. The warn-only catch was meant to avoid hiding correctness failures; the fix is to fail the suite on write failure instead. + +Acceptance criteria: +- a `writeResultsFile` failure fails the benchmark suite rather than passing with a warning +- the hard contract is covered by a test (simulate a write failure and assert failure/no-silent-pass) +- `npm run benchmark` still prints the table and writes the report on success + +Verification: +- npm run benchmark passes and produces `benchmark/results.txt` +- npm run verify passes (typecheck, tests, lint, format) + +## TASK-025 — Simulator blocked-task and risk deltas + +Status: DONE + +Goal: +Return the two PROJECT_PLAN-section-10 simulator deltas that are currently missing: blocked-task count and risk indicators. + +Prerequisites: +None. + +Context: +PROJECT_PLAN §10 lists "blocked-task count" and "risk indicators" among the deltas the simulator should report. `SimulationResult` exposes duration/critical-path/recommendation/value-removed deltas and affected-downstream ids but neither blocked-task count nor any risk indicator, and ADR-009 records no decision to drop them. Review finding. Define the two metrics deterministically before implementing; do not introduce dates or probabilistic analysis. + +Suggested deterministic definitions (candidate — confirm in ADR-009 before implementation): +- blocked-task count: number of non-DONE tasks whose prerequisites are not all DONE (in baseline and projected), reported per side and as a delta +- risk indicators: at least one deterministic signal, e.g. number of tasks whose slack fell to zero, or counting newly-critical tasks between sides; document whichever is chosen + +Acceptance criteria: +- baseline and projected sides expose blocked-task count; a delta (or enumerated change) is reported +- at least one deterministic risk indicator is defined in ADR-009 and returned; the definition is documented before/with implementation +- both are covered by simulation tests (construction of a blocked case and a changed-slack case) +- existing simulation behavior and output shape remain backward compatible (new fields only) + +Verification: +- npm run verify passes (typecheck, tests, lint, format) +- ADR-009 records the chosen definitions + +## TASK-026 — Documentation cleanup + +Status: DONE + +Goal: +Remove residual contradictions between repo docs and the implemented code. + +Prerequisites: +None. + +Context: +Review findings on documentation accuracy: + +- `docs/task-006-review.md` reads as a live defect report but documents issues fixed in the same PR that added it (it asserts element-level freezing is not applied and "all 125 tests pass"; the suite is now 287). Mark it historical/archived or delete it. +- `docs/tasks.md` TASK-015 verification says "286 tests"; actual is 287. +- `docs/handoff.md` freezes "31 engine tests" (actual 34) and "ADR-001 through ADR-010" (the file holds 12 ADRs). +- `isReachable(t, t)` returns false for acyclic graphs (self-reachability convention per ADR-005); this deserves one documenting line in the graph section. +- ADR-006's "O(V+E) per pass" claim is true only per pass; the function is dominated by the topological sort (ready queue is `.sort()`ed at each insertion). Narrow the wording. +- TASK-014's literal "same input produces same time-ordered results" determinism criterion is unsatisfiable for wall-clock timings; ADR-011's domain-result reinterpretation should be pointed to from the task text. +- `.gitignore` contains `.next/` for a framework this repo never uses. + +Acceptance criteria: +- `docs/task-006-review.md` is marked historical/superseded (or removed), and nothing in the repo reads it as current +- the stale counts in `docs/tasks.md` and `docs/handoff.md` are corrected +- one-line notes added for the `isReachable` convention, the schedule complexity claim, and the TASK-014 determinism reading +- the `.next/` ignore entry is removed + +Verification: +- grep confirms no live references treat `task-006-review.md` as current +- npm run verify still passes (docs-only task, but run the gate) diff --git a/src/ui/ScenarioPanel.tsx b/src/ui/ScenarioPanel.tsx index 5a10d81..1925348 100644 --- a/src/ui/ScenarioPanel.tsx +++ b/src/ui/ScenarioPanel.tsx @@ -212,9 +212,7 @@ function ScenarioComparison({ ? result.newlyCriticalTaskIds.join(", ") : "none"} - - {result.newlyCriticalTaskIds.length > 0 ? "at risk" : "—"} - + {result.newlyCriticalTaskIds.length > 0 ? "at risk" : "—"} Critical path From 3b81044de0b4a4b50b8b19fe0814036f857faf67 Mon Sep 17 00:00:00 2001 From: 67Midas Date: Sun, 6 Sep 2026 12:11:39 -0500 Subject: [PATCH 11/11] fix(simulation): derive newlyCriticalTaskIds from slack transitions (PR #21 review) - newlyCriticalTaskIds now computed from per-task slack transitions (baseline slack > 0 to projected slack 0) instead of projected-critical-path membership difference, matching ADR-009's documented definition and decoupling the metric from how the scheduler's criticalPath set is represented - add regression test for two parallel paths where both become zero-slack, asserting every newly-critical task is reported across both paths - benchmark failure test now exercises the harness's writeReport emission path (bad file path parameter) instead of calling writeResultsFile directly - document on ProjectRepository.save that id uniqueness is enforced by the application layer (check-then-save in createProject), not atomically by the repository - ADR-009 wording updated to the slack-transition definition; handoff/progress updated --- benchmark/benchmark.test.ts | 19 +++++++++++++------ docs/decisions.md | 2 +- docs/handoff.md | 4 ++-- docs/progress.md | 8 ++++++++ src/application/repository.ts | 8 ++++++++ src/domain/simulation/simulation.test.ts | 22 ++++++++++++++++++++++ src/domain/simulation/simulation.ts | 16 +++++++++++++--- 7 files changed, 67 insertions(+), 12 deletions(-) diff --git a/benchmark/benchmark.test.ts b/benchmark/benchmark.test.ts index 0f1bb24..b074719 100644 --- a/benchmark/benchmark.test.ts +++ b/benchmark/benchmark.test.ts @@ -143,18 +143,25 @@ describe("benchmark harness output", () => { const badPath = fileURLToPath( new URL("./results.txt/unwritable.txt", import.meta.url), ); - expect(() => writeResultsFile([sample], badPath)).toThrow(); + + // Exercise the harness's actual emission path (writeReport), not just the + // lower-level helper, so a failure here is what `npm run benchmark` would + // actually hit. + expect(() => writeReport([sample], badPath)).toThrow(); // The same result writes cleanly to the canonical report location. - expect(() => writeResultsFile([sample], RESULTS_FILE)).not.toThrow(); + expect(() => writeReport([sample], RESULTS_FILE)).not.toThrow(); expect(readFileSync(RESULTS_FILE, "utf8")).toContain("Total measurements"); }); }); -function writeReport(results: readonly OperationResult[]): void { - if (existsSync(RESULTS_FILE)) unlinkSync(RESULTS_FILE); - writeResultsFile(results, RESULTS_FILE); - const written = readFileSync(RESULTS_FILE, "utf8"); +function writeReport( + results: readonly OperationResult[], + filePath: string = RESULTS_FILE, +): void { + if (existsSync(filePath)) unlinkSync(filePath); + writeResultsFile(results, filePath); + const written = readFileSync(filePath, "utf8"); expect(written).toContain("Total measurements"); expect(written).toContain("mean (ms)"); console.log("\n" + written); diff --git a/docs/decisions.md b/docs/decisions.md index 54564a0..6550fc4 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -214,7 +214,7 @@ TASK-007 requires comparing a baseline project state with deterministic what-if - **Affected downstream**: the target plus its transitive dependents, filtered to tasks whose `[earliestStart, earliestFinish]` window actually changed between baseline and projected schedules, sorted lexicographically. "Affected" means a measurable schedule change — merely being downstream of the change is not enough. A removed task never appears (it does not survive into the projection). - **Comparison shape**: each side (`baseline`, `projected`) exposes `projectDuration`, `criticalPath`, `recommendedTaskId`, `recommendedScore`, and `blockedTaskCount`. Deltas: `durationDelta` (rounded to three decimals, matching engine precision), `blockedTaskDelta`, `newlyCriticalTaskIds`, `criticalPathChanged` (ordered id-sequence equality), `recommendationChanged` (selected id equality), and `valueRemoved` (target value, present only for `remove-task`). - **Blocked-task count** (PROJECT_PLAN §10): per side, the number of non-DONE tasks with at least one non-DONE prerequisite, counted per side and reported as `blockedTaskDelta` (projected − baseline). Blocking is derived from the graph, never from the informational `BLOCKED` status flag, matching eligibility semantics (ADR-007). De-scoping a prerequisite therefore lowers the count (its dependency edges are stripped), while delay/effort scenarios never change it — all without date or status mutations. -- **Risk indicator** (PROJECT_PLAN §10): `newlyCriticalTaskIds` — the tasks on the projected critical path that were not on the baseline critical path, sorted lexicographically. A task that becomes newly critical had its slack exhausted (fell to zero), which is the single deterministic signal that best captures schedule risk for a given scenario. No probabilistic or date-based analysis is introduced. +- **Risk indicator** (PROJECT_PLAN §10): `newlyCriticalTaskIds` — tasks whose slack was positive in the baseline and fell to zero in the projection, sorted lexicographically. A task that becomes newly critical had its slack exhausted (fell to zero), which is the single deterministic signal that best captures schedule risk for a given scenario. This definition is deliberately based on per-task slack transitions rather than membership in the scheduler's `criticalPath` representation, so the metric stays correct regardless of how that set is represented. No probabilistic or date-based analysis is introduced. - **Determinism and immutability**: the result and its nested arrays are frozen; `scenarioTasks` is sorted by id so serialization is independent of input order; output is verified by JSON-equality across repeated and reordered runs. Cyclic inputs throw through the normal scheduling path — no special handling. ### Alternatives considered diff --git a/docs/handoff.md b/docs/handoff.md index bb28f02..698749f 100644 --- a/docs/handoff.md +++ b/docs/handoff.md @@ -6,7 +6,7 @@ All planned phases complete. Domain foundation, dependency graph, scheduling and ## Current Task -TASK-018 through TASK-026 (the full-branch code-review remediation backlog) are implemented, verified, marked DONE, and pushed on `fix/review-remediation-018-026`. The branch is pending human review and merge. See "Next Recommended Action". +TASK-018 through TASK-026 (the full-branch code-review remediation backlog) are implemented, verified, marked DONE, and pushed on `fix/review-remediation-018-026`. The PR #21 review response is applied and pushed on the same branch (slack-transition `newlyCriticalTaskIds`, benchmark failure test through `writeReport`, and repository uniqueness-contract doc). The branch is pending human review and merge. See "Next Recommended Action". ## State @@ -123,7 +123,7 @@ Beyond the remediation backlog, possible future tracks (not yet defined as tasks ## Verification -The review remediation is verified complete. Locally on `fix/review-remediation-018-026`: `npm run verify` (typecheck, 304 tests, lint, format:check), `npm run build`, and `npm run benchmark` (7 tests) all pass. `npm run test:e2e` (2 Playwright specs) passes with browsers installed. +The review remediation is verified complete, including the PR #21 review response. Locally on `fix/review-remediation-018-026`: `npm run verify` (typecheck, 305 tests, lint, format:check), `npm run build`, and `npm run benchmark` (7 tests) all pass. `npm run test:e2e` (2 Playwright specs) passes with browsers installed. ## Important Constraint diff --git a/docs/progress.md b/docs/progress.md index 7a9210c..4ad4782 100644 --- a/docs/progress.md +++ b/docs/progress.md @@ -463,3 +463,11 @@ Completed: - Also: the demo-story outcome is locked by an integration test (`a9b4ab8`) asserting the sample project's deterministic recommendation, factor breakdown, task count, and critical path - Verified: `npm run verify` (typecheck, 304 tests, lint, format:check), `npm run build`, and `npm run benchmark` (7 tests, with `benchmark/results.txt` written) all pass - Branch pushed as `fix/review-remediation-018-026`; pending human review and merge (no PR created from automation) + +## 2026-09-05 — PR #21 review response (on fix/review-remediation-018-026) + +Completed: +- `newlyCriticalTaskIds` re-derived from per-task slack transitions (baseline slack > 0 → projected slack 0) instead of projected-critical-path membership difference; ADR-009 wording updated to match. Semantics unchanged for the current scheduler (its `criticalPath` is all zero-slack tasks, ADR-006), but the metric is now insensitive to how the critical-path set is represented. Regression test added for the review's two-parallel-paths scenario: baseline critical `["a","b"]`, projected critical `["c","d"]`, `newlyCriticalTaskIds` = `["c","d"]`. +- Benchmark failure test now exercises the harness's actual `writeReport()` emission path (parameterized with a bad file path) instead of calling `writeResultsFile()` directly, matching the reviewer's request. +- `ProjectRepository.save` gains a doc comment stating the repository is not atomic on id uniqueness — the application layer enforces check-then-save in `createProject`; a concurrent/distributed backend must enforce uniqueness at the storage level. +- Verified: `npm run verify` (typecheck, 305 tests, lint, format:check), `npm run build`, and `npm run benchmark` (7 tests) all pass. diff --git a/src/application/repository.ts b/src/application/repository.ts index d195291..acf6841 100644 --- a/src/application/repository.ts +++ b/src/application/repository.ts @@ -9,6 +9,14 @@ export interface ProjectSummary { } export interface ProjectRepository { + /** + * Persist a project, replacing anything stored under `project.id`. + * + * Id uniqueness is enforced by the application layer (check-then-save in + * ProjectService.createProject), not atomically by the repository. A + * concurrent or distributed backend that requires atomic uniqueness must + * enforce it at the storage level (e.g., a unique-id constraint). + */ save(project: Project): Promise; load(id: string): Promise; list(): Promise; diff --git a/src/domain/simulation/simulation.test.ts b/src/domain/simulation/simulation.test.ts index 3e03a63..44c43fc 100644 --- a/src/domain/simulation/simulation.test.ts +++ b/src/domain/simulation/simulation.test.ts @@ -308,6 +308,28 @@ describe("simulateScenario — risk indicators", () => { expect(result.durationDelta).toBe(0); expect(result.newlyCriticalTaskIds).toEqual([]); }); + + it("reports newly critical tasks on every parallel path that loses slack", () => { + // Two parallel paths in the baseline: A→B is critical (duration 4); C→D + // has slack 2. Delaying C pushes C→D to the same duration as A→B, so the + // projected state has TWO parallel zero-slack paths. The metric must + // report every task whose slack was exhausted, not just the tasks on a + // single path pulled from criticalPath membership. + const baseline: Task[] = [ + task("a", { estimatedEffort: 2 }), + task("b", { estimatedEffort: 2, dependencies: ["a"] }), + task("c", { estimatedEffort: 1 }), + task("d", { estimatedEffort: 1, dependencies: ["c"] }), + ]; + const result = simulateScenario(baseline, { + kind: "delay-task", + taskId: "c", + additionalEffort: 2, + }); + expect(result.baseline.criticalPath).toEqual(["a", "b"]); + expect(result.projected.criticalPath).toEqual(["a", "b", "c", "d"]); + expect(result.newlyCriticalTaskIds).toEqual(["c", "d"]); + }); }); describe("simulateScenario — determinism and immutability", () => { diff --git a/src/domain/simulation/simulation.ts b/src/domain/simulation/simulation.ts index c08fe5f..0223404 100644 --- a/src/domain/simulation/simulation.ts +++ b/src/domain/simulation/simulation.ts @@ -227,9 +227,19 @@ export function simulateScenario( a.id.localeCompare(b.id), ); - const baselineCritical = new Set(baselineSchedule.criticalPath); - const newlyCriticalTaskIds = scenarioSchedule.criticalPath - .filter((taskId) => !baselineCritical.has(taskId)) + // "Newly critical" means a task whose slack was positive in the baseline and + // fell to zero in the projection. Deriving it from per-task slack transitions + // rather than criticalPath membership keeps the metric correct regardless of + // how the scheduler's criticalPath set is represented. + const baselineSlack = new Map( + baselineSchedule.taskSchedules.map((s) => [s.taskId, s.slack] as const), + ); + const newlyCriticalTaskIds = scenarioSchedule.taskSchedules + .filter((s) => { + const before = baselineSlack.get(s.taskId); + return before !== undefined && before > 0 && s.slack === 0; + }) + .map((s) => s.taskId) .sort((a, b) => a.localeCompare(b)); return Object.freeze({