feat(domain): add goal phase field (spec 024)

[bborbe]

Summary

Adds a validated phase frontmatter field to the Goal domain type (todo/planning/execution/done), mirroring the task-side Phase. Data-layer foundation for the Phase-Gated Goal Flow workflow — the plan-goal/execute-goal gate commands (separate, markdown) consume this field.

What

• New GoalPhase enum type + Validate (pkg/domain/goal_phase.go), shape-mirrored from TaskPhase, no aliases
• Phase() getter / SetPhase setter + phase case in GoalFrontmatter get/set (pkg/domain/goal_frontmatter.go) — rides the existing goal set/goal show commands, no new command
• Lazy migration: goals without phase parse and operate unchanged; missing reads as empty (no default-to-todo)
• Non-canonical values rejected on write; task-side Phase type left byte-for-byte unchanged

Testing

• pkg/domain Ginkgo suite — enum DescribeTable (4 canonical + bogus/empty/in_progress rejections) + frontmatter getter/setter round-trip
• make precommit green (golangci-lint 0 issues, security clean, CHANGELOG valid)
• verify-spec 024: all 9 acceptance criteria proven against a freshly-built binary — field writes to frontmatter, --output json surfaces it, bogus rejected with file unchanged, no-phase goal clean

Spec

dark-factory spec 024-goal-phase-field (see specs/completed/).

[ben-s-pull-request-reviewer]

Based on my review, the implementation is solid. Let me compile the findings:

Summary: The PR correctly implements spec 024 — adding a GoalPhase enum with 4 canonical values (todo/planning/execution/done) and a phase frontmatter field on goals. The lazy-migration strategy (tolerate legacy hand-typed values on read, reject them on re-write) is intentional and well-documented. The nil pointer deletion behavior is correct. Test coverage is thorough, including regression tests for unrelated mutations. No security concerns.

{
  "verdict": "approve",
  "summary": "The GoalPhase feature is correctly implemented per spec 024. The 4-value enum mirrors TaskPhase shape without aliases, the lazy-migration strategy correctly tolerates legacy values on read while rejecting them on write, and test coverage is comprehensive including regression tests. One minor issue: goal_phase.go has a 2025 copyright year despite being a new file committed in 2026.",
  "comments": [
    {
      "file": "pkg/domain/goal_phase.go",
      "line": 1,
      "severity": "minor",
      "message": "Copyright year is 2025 but file was committed in 2026 — should be (c) 2026"
    }
  ],
  "concerns_addressed": [
    "correctness: Phase() getter returns raw value without validation on read — confirmed intentional, clearly documented in method comment, legacy hand-typed values survive display and round-trip",
    "correctness: setPhaseField validates on write but not on read — confirmed intentional lazy-migration strategy, test at goal_frontmatter_test.go:350-355 verifies legacy alias rejection on re-set",
    "correctness: GoalPhase mirrors TaskPhase shape but has no aliases (4-of-7 subset) — confirmed intentional per spec 024, goal_phase_test.go:49-55 verifies in_progress is correctly rejected",
    "correctness: nil pointer to SetPhase deletes the key — confirmed correct behavior, goal_frontmatter_test.go:320-325 verifies deletion removes the key entirely",
    "tests: legacy goal round-trip regression test — confirmed sufficient, goal_frontmatter_test.go:375-383 verifies no phase injection on unrelated mutations",
    "security: Validate uses closed 4-value enum, no user-controlled string interpolation in error messages — confirmed, errors.Wrapf at goal_phase.go:53 safely formats the phase value without concatenation"
  ]
}