Skip to content

Fix/review remediation 018 026 - #21

Draft
skibkitty wants to merge 11 commits into
mainfrom
fix/review-remediation-018-026
Draft

skibkitty wants to merge 11 commits into
mainfrom
fix/review-remediation-018-026

Conversation

@skibkitty

Copy link
Copy Markdown
Owner

Summary

Implements TASK-018 through TASK-026 — the full-branch code-review remediation backlog recorded in docs/tasks.md. All nine tasks address review findings across domain immutability, application-layer deduplication, UI maintainability, persistence hardening, the benchmark-report contract, simulator deltas, and documentation accuracy. No behavior changes beyond the documented fixes.

Changes

TASK-018 — Fix recommendation deep-freeze gap

  • Deep-frozen the affectedTaskIds arrays on the tie-break-applied and blocked-status-eligible warnings, honoring the ADR-008 immutability contract
  • Regression test asserts Object.isFrozen on the nested arrays and elements

TASK-019 — Extract shared task-to-input mapper

  • Removed the copy-pasted Task → CreateTaskInput mapper from task-service.ts, goal-service.ts, and dependency-service.ts
  • Single shared implementation in src/application/task-input.ts (including the goalId: null → undefined bridge)

TASK-020 — Extract shared test helpers

  • Consolidated ~10 duplicated createStubRepository implementations (plus createInMemoryStorage) into src/test-support/index.ts
  • All call sites migrated; the shared stub seeds via initialProject and supports per-method overrides
  • Net −275 lines of duplicated helper code

TASK-021 — UI status options from domain constant

  • TaskForm/TaskList dropdowns now render from the domain's ALL_TASK_STATUSES
  • STATUS_COLORS in the dependency graph is keyed by TaskStatus (no typo-masking fallback)
  • as TaskStatus casts replaced with a type-safe find()

TASK-022 — Persistence validation and freeze hardening

  • Serialization copies and deep-freezes dependencies so serialized output does not alias mutable input
  • Deserialization rejects non-string dependency entries with a descriptive error instead of silently filtering
  • ADR-010 updated to document the chosen semantics

TASK-023 — Guard against project id collisions

  • ProjectService.createProject rejects an existing id (Project already exists: {id}), covering the sample-project seed path
  • Dashboard surfaces the error instead of silently overwriting

TASK-024 — Benchmark report emission as a hard contract

  • npm run benchmark now fails if it cannot write benchmark/results.txt, matching the CI if-no-files-found: error artifact contract
  • Covered by a write-failure simulation test

TASK-025 — Simulator blocked-task and risk deltas

  • SimulationResult now exposes blockedTaskCount per side, blockedTaskDelta, and newlyCriticalTaskIds (slack exhausted → becomes critical) as the deterministic risk indicator
  • Definitions recorded in ADR-009; the scenario comparison panel displays both
  • Backward compatible (new fields only)

TASK-026 — Documentation cleanup

  • Archived docs/task-006-review.md (superseded banner, no live references)
  • Corrected stale counts (TASK-015: 286→287; handoff engine tests 31→34)
  • Added isReachable(t, t) self-reachability note to the case study
  • Narrowed ADR-006's complexity claim; pointed TASK-014's determinism criterion at ADR-011
  • Removed unused .next/ entry from .gitignore

Additional (not in the task list)

  • Integration test locking the demo-story outcome: sample project's deterministic recommendation, factor breakdown, task count, and critical path

Verification

  • npm run verify passes (typecheck, 304 tests, lint, format:check)
  • npm run build succeeds
  • npm run benchmark passes (7 tests, benchmark/results.txt written)
  • npm run test:e2e passes (2 Playwright specs)

Docs

  • docs/tasks.md — TASK-018 through TASK-026 marked DONE
  • docs/decisions.md — ADR-009 (blocked-task/risk deltas), ADR-010 (dependency-entry validation)
  • docs/handoff.md — current task and verification updated
  • docs/progress.md — remediation-completed entry appended

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.
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<string,string> fallback) and node fill uses a
typed lookup. Added tests asserting every domain status appears in
the TaskForm and TaskList dropdowns.
…g 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.
#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant