Fix/review remediation 018 026

.gitignore

@@ -1,6 +1,5 @@
 .DS_Store
 node_modules/
-.next/
 dist/
 coverage/
 .env

benchmark/benchmark.test.ts

@@ -122,27 +122,49 @@ 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);
-});
 
-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,
+  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),
     );
-  }
+
+    // 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(() => writeReport([sample], RESULTS_FILE)).not.toThrow();
+    expect(readFileSync(RESULTS_FILE, "utf8")).toContain("Total measurements");
+  });
+});
+
+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);
 }
 
 describe("benchmark dependent operations", () => {

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.
 

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` — 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
@@ -243,6 +245,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.
 

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 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
 
-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, 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