fix(scheduling): Model milestones as CPM nodes to correctly determine critical path

[steilerDev]

Summary

Milestones were not appearing on the critical path, even when they sat on the longest path through the project. This fix models each milestone as a zero-duration CPM node so they are naturally included in critical path calculations.

Root cause: The previous implementation expanded milestones into synthetic work item→work item dependencies, causing milestones to disappear from the CPM graph entirely.

Solution: Model each milestone as a zero-duration CPM node with ID prefix milestone:<id>. The CPM engine naturally computes float for these nodes and includes them in the critical path when appropriate.

Changes

Shared Types (shared/src/types/timeline.ts)

• Add isCritical?: boolean to TimelineMilestone interface

Scheduling Engine (server/src/services/schedulingEngine.ts)

• Section 3-4: Replace milestone dependency expansion with milestone-as-CPM-node logic

  • Create one zero-duration SchedulingWorkItem per milestone with contributors or dependents
  • Construct milestone node ID as milestone:<id>
  • For completed milestones: use completedAt date; for incomplete: use targetDate
  • Create FS dependencies: each contributor→milestone and each milestone→dependent

• Section 7: Skip writing milestone nodes back to database

  • Filter out IDs starting with milestone: when writing scheduled dates
  • These are virtual CPM nodes only; they should never be persisted

Timeline Service (server/src/services/timelineService.ts)

• Section 5: Pass milestone CPM nodes to the scheduling engine

  • Build milestone nodes using same logic as autoReschedule()
  • Build milestone synthetic dependencies
  • Add both to the engine's work items and dependencies
  • Extract critical milestone IDs from CPM result
  • Filter milestone: entries from returned criticalPath (API should only contain work item IDs)

• Section 6: Propagate isCritical to milestones

  • Add isCritical field to each TimelineMilestone in the response based on critical milestone IDs
  • Skip milestone nodes when building work item end date map for projectedDate computation

Test Coverage

The QA agent will verify:

• CPM correctly identifies milestones on the critical path
• Non-critical milestones have isCritical: false
• Milestone nodes do not appear in the criticalPath array returned by the API
• autoReschedule() handles milestone nodes correctly during date updates

Fixes #484

🤖 Generated with Claude Code

[steilerDev]

[ux-designer]

Reviewed against the design system (tokens.css, GanttChart.tsx color resolution pattern, existing GanttBar critical path styling) and the accessibility requirements.

Token Adherence — PASS

criticalBorderColor is resolved via readCssVar('--color-gantt-bar-critical-border') in GanttChart.tsx, the same token already used for critical work item bar borders. This is correct and consistent — the SVG getComputedStyle resolution pattern is the right approach since SVG attributes cannot consume var() references directly.

--color-gantt-bar-critical-border maps to var(--color-orange-400) in light mode and var(--color-orange-300) in dark mode, both confirmed in tokens.css.

Visual Consistency — PASS

The strokeWidth step from 2 → 3 for critical milestones mirrors how GanttBar uses a distinct border treatment for critical work items. The approach is consistent across the chart.

Dark Mode — PASS

Critical color is read fresh on every MutationObserver theme change (the existing useEffect that calls resolveColors() when data-theme changes covers this). Dark mode is handled correctly — no hardcoded color values introduced.

Ghost Diamond Interaction — PASS with note

The ghost diamond early-returns before reaching the isCritical stroke logic (line 177 early return uses lateStroke + strokeWidth={1.5}), so ghost diamonds correctly never receive critical styling even though isCritical and criticalBorderColor are passed as props. The props are harmlessly unused on the ghost path. This is intentional and the PR description confirms it, but a brief inline comment on line 340 would make the intent explicit for future readers. Informational only — not blocking.

Accessibility — PASS

The aria-label addition , critical path for critical milestones is the right pattern; it matches how GanttBar announces critical bars via accessible labels. The label order (title, status, critical flag, target date) is logical for screen reader consumption.

Responsive / Touch — PASS

No layout or touch target changes. HIT_RADIUS_TOUCH = 22 is unchanged and sufficient.

Finding Summary

No blocking issues. One informational suggestion: add a comment on the ghost DiamondMarker call (line ~339) noting that isCritical/criticalBorderColor are passed but intentionally ignored by the ghost early-return, to prevent a future reader from removing them as "dead props".

[steilerDev]

[security-engineer]

PR #487 reviewed — no security findings.

Scope: Milestone CPM node refactor (, , , , 44 new tests).

Checklist

• No SQL/command/XSS injection vectors in new code
• Authentication/authorization enforced on all new endpoints (no new endpoints added)
• No sensitive data exposed in logs, errors, or client responses
• User input validated and sanitized at API boundaries
• No new dependencies introduced
• No hardcoded credentials or secrets
• CORS configuration unchanged
• Error responses do not leak internal details

Injection analysis — milestone: prefix handling

The parseInt(id.slice('milestone:'.length), 10) call in timelineService.ts:341 operates exclusively on values produced by the scheduling engine itself. The only strings that enter the criticalPath array with the milestone: prefix are constructed in the same service from DB-sourced integer milestone IDs (milestone:${milestoneId} where milestoneId is a number from milestoneMap.keys()). No user-controlled input reaches this code path. parseInt with radix 10 is the correct call and produces a safe integer.

DB-write guard

The scheduled.workItemId.startsWith('milestone:') guard in section 7 of autoReschedule correctly prevents virtual CPM nodes from being persisted. The filter is applied before any DB write loop, so there is no TOCTOU risk.

isCritical field exposure

The new isCritical: boolean field on TimelineMilestone is a computed boolean derived from the CPM result — it contains no PII, internal IDs, or implementation details beyond what the existing criticalPath work-item array already exposes. No new data-exposure risk.

Ghost diamond isolation

The ghost polygon always uses strokeWidth={1.5} and isGhost prop bypasses the isCritical && !isGhost ? 3 : 2 branch correctly — no styling bleed.

[steilerDev]

[product-architect]

Architecture Review — PR #487 (Bug #484: Milestones on Critical Path)

Verified

• Architecture compliance: Modeling milestones as zero-duration CPM nodes with milestone:<id> prefix is a clean, well-scoped approach. The scheduling engine (schedule()) remains a pure function — milestone nodes are injected as data, not as special-cased logic inside the CPM algorithm. This preserves the engine's generality (ADR-014).

• Shared type consistency: TimelineMilestone.isCritical is correctly added as a required boolean field (not optional), and all test fixtures across 6 files are updated to include it. The shared type is the single source of truth and both server and client consume it correctly.

• Critical path filtering: milestone: prefixed IDs are correctly stripped from the criticalPath array in the API response (API contract says criticalPath: string[] contains work item IDs only). The milestone criticality is conveyed separately via each milestone's isCritical field — good separation of concerns.

• DB write guard: The autoReschedule() function correctly skips milestone: prefixed IDs when writing scheduled dates back to the database. Test coverage confirms no phantom rows are created.

• Frontend rendering: Critical milestones get thicker stroke (3 vs 2) with criticalBorderColor, ghost diamonds are unaffected, aria-labels include "critical path" for accessibility. All props are optional with sensible defaults — backward compatible.

• Test coverage: 44 new tests across 3 test files (scheduling engine CPM, timeline service, GanttMilestones rendering) plus fixture updates in 6 more files. Coverage is thorough: critical/non-critical paths, completed milestones, multiple contributors, cycle detection, mixed rendering, and the DB write guard.

• CI: All quality gates, test shards, Docker, and E2E smoke tests pass.

Finding — Wiki API Contract Not Updated (Medium)

The TimelineMilestone interface in wiki/API-Contract.md (line ~4391) does not include the new isCritical field. It is also missing the pre-existing projectedDate field (a gap that predates this PR).

Action required: Update the wiki API-Contract.md to add:

interface TimelineMilestone {
  id: number;
  title: string;
  targetDate: string;
  isCompleted: boolean;
  completedAt: string | null;
  color: string | null;
  workItemIds: string[];
  projectedDate: string | null;  // pre-existing gap
  isCritical: boolean;           // new in this PR
}

This is a documentation-only gap — code and types are correct. Given this is a focused bug fix, I am not requesting changes for this; flagging it so it can be addressed in the current PR or as a follow-up.

Minor Note

The PR description says "Add isCritical?: boolean to TimelineMilestone" but the actual shared type uses required isCritical: boolean. The code is correct; the description is slightly inaccurate.

Code Duplication Note (Informational)

The milestone CPM node construction logic is duplicated between autoReschedule() (schedulingEngine.ts) and getTimeline() (timelineService.ts). Both build the same maps, iterate milestoneIdsWithLinks, and construct identical SchedulingWorkItem nodes and SchedulingDependency arrays. This is acceptable for a bug fix, but a future refactor could extract a shared buildMilestoneCpmNodes(db) helper to keep the two call sites in sync. No action needed now.

Verdict

Approve. The fix is architecturally sound, well-tested, and correctly scoped. The wiki gap should be addressed but does not block merge.

[steilerDev]

[product-architect]

Architecture Review — PR #487 (Bug #484: Milestones on Critical Path)

Verified

• Architecture compliance: Modeling milestones as zero-duration CPM nodes with milestone:<id> prefix is a clean, well-scoped approach. The scheduling engine (schedule()) remains a pure function — milestone nodes are injected as data, not as special-cased logic inside the CPM algorithm. This preserves the engine's generality (ADR-014).

• Shared type consistency: TimelineMilestone.isCritical is correctly added as a required boolean field (not optional), and all test fixtures across 6 files are updated to include it. The shared type is the single source of truth and both server and client consume it correctly.

• Critical path filtering: milestone: prefixed IDs are correctly stripped from the criticalPath array in the API response (API contract says criticalPath: string[] contains work item IDs only). The milestone criticality is conveyed separately via each milestone's isCritical field — good separation of concerns.

• DB write guard: The autoReschedule() function correctly skips milestone: prefixed IDs when writing scheduled dates back to the database. Test coverage confirms no phantom rows are created.

• Frontend rendering: Critical milestones get thicker stroke (3 vs 2) with criticalBorderColor, ghost diamonds are unaffected, aria-labels include "critical path" for accessibility. All props are optional with sensible defaults — backward compatible.

• Test coverage: 44 new tests across 3 test files (scheduling engine CPM, timeline service, GanttMilestones rendering) plus fixture updates in 6 more files. Coverage is thorough: critical/non-critical paths, completed milestones, multiple contributors, cycle detection, mixed rendering, and the DB write guard.

• CI: All quality gates, test shards, Docker, and E2E smoke tests pass.

Finding — Wiki API Contract Not Updated (Medium)

The TimelineMilestone interface in wiki/API-Contract.md (line ~4391) does not include the new isCritical field. It is also missing the pre-existing projectedDate field (a gap that predates this PR).

Action required: Update the wiki API-Contract.md to add:

interface TimelineMilestone {
  id: number;
  title: string;
  targetDate: string;
  isCompleted: boolean;
  completedAt: string | null;
  color: string | null;
  workItemIds: string[];
  projectedDate: string | null;  // pre-existing gap
  isCritical: boolean;           // new in this PR
}

This is a documentation-only gap — code and types are correct. Given this is a focused bug fix, I am not requesting changes for this; flagging it so it can be addressed in the current PR or as a follow-up.

Minor Note

The PR description says "Add isCritical?: boolean to TimelineMilestone" but the actual shared type uses required isCritical: boolean. The code is correct; the description is slightly inaccurate.

Code Duplication Note (Informational)

The milestone CPM node construction logic is duplicated between autoReschedule() (schedulingEngine.ts) and getTimeline() (timelineService.ts). Both build the same maps, iterate milestoneIdsWithLinks, and construct identical SchedulingWorkItem nodes and SchedulingDependency arrays. This is acceptable for a bug fix, but a future refactor could extract a shared buildMilestoneCpmNodes(db) helper to keep the two call sites in sync. No action needed now.

Verdict

Approve. The fix is architecturally sound, well-tested, and correctly scoped. The wiki gap should be addressed but does not block merge.

[steilerDev]

[security-engineer]

PR #487 reviewed — no security findings.

Scope: Milestone CPM node refactor (schedulingEngine.ts, timelineService.ts, shared/src/types/timeline.ts, GanttMilestones.tsx, 44 new tests).

Checklist

• No SQL/command/XSS injection vectors in new code
• Authentication/authorization enforced on all new endpoints (no new endpoints added)
• No sensitive data exposed in logs, errors, or client responses
• User input validated and sanitized at API boundaries
• No new dependencies introduced
• No hardcoded credentials or secrets
• CORS configuration unchanged
• Error responses do not leak internal details

Injection analysis — milestone: prefix handling

The parseInt(id.slice('milestone:'.length), 10) call in timelineService.ts:341 operates exclusively on values produced by the scheduling engine itself. The only strings that enter the criticalPath array with the milestone: prefix are constructed in the same service from DB-sourced integer milestone IDs (milestone:${milestoneId} where milestoneId is a number from milestoneMap.keys()). No user-controlled input reaches this code path. parseInt with radix 10 is the correct call and produces a safe integer.

DB-write guard

The scheduled.workItemId.startsWith('milestone:') guard in section 7 of autoReschedule correctly prevents virtual CPM nodes from being persisted. The filter is applied before any DB write loop, so there is no TOCTOU risk.

isCritical field exposure

The new isCritical: boolean field on TimelineMilestone is a computed boolean derived from the CPM result — it contains no PII, internal IDs, or implementation details beyond what the existing criticalPath work-item array already exposes. No new data-exposure risk.

Ghost diamond isolation

The ghost polygon always uses strokeWidth=1.5 and the isGhost prop correctly bypasses the isCritical && !isGhost conditional — no critical styling bleed onto the ghost diamond.

[github-actions]

🎉 This PR is included in version 1.12.0-beta.48 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀