[bborbe]

Summary

Goal and topic phase validation now accepts exactly the six canonical task phases (todo, planning, execution, ai_review, human_review, done), and an invalid phase exits non-zero with the validator's message listing all six values, rendered from the enum collection.

This reverses the four-value decision recorded in specs/completed/024-goal-phase-field.md and 052-topic-phase-field.md, both of which deferred the extension to a separate spec. Spec: specs/in-progress/062-goal-topic-phase-set.md.

Change set

• pkg/domain/goal_phase.go, pkg/domain/topic_phase.go — two new constants, the widened Available* collections, a String() on the plural type, and the valid-value list in Validate.
• pkg/domain/goal_frontmatter.go — setPhaseField now surfaces the validator's error instead of rebuilding it.
• pkg/domain/*_phase_test.go, pkg/domain/*_frontmatter_test.go, pkg/ops/frontmatter_entity_test.go — unit coverage.
• integration/cli_test.go — goal and topic set/get integration coverage.
• CHANGELOG.md — one ## Unreleased bullet.

Behavior

• vault-cli goal set <goal> phase ai_review|human_review and the topic equivalent succeed and write the value.
• goal set … phase bogus → exit 1, unknown goal phase 'bogus' (valid: todo, planning, execution, ai_review, human_review, done).
• topic set … phase bogus → exit 1, invalid topic phase: unknown topic phase 'bogus' (valid: …).
• in_progress stays a task-only alias and is still rejected.
• The valid-value list is rendered by iterating AvailableGoalPhases / AvailableTopicPhases; no second literal list exists in pkg/domain/.

Evidence

make precommit exits 0. Scratch-vault probe against a binary built from this branch:

### goal ai_review        → ai_review
### goal human_review     → human_review
### topic ai_review       → ai_review
### topic human_review    → human_review
### goal bogus            → exit 1, unknown goal phase 'bogus' (valid: todo, planning, execution, ai_review, human_review, done)
### topic bogus           → exit 1, unknown topic phase 'bogus' (valid: todo, planning, execution, ai_review, human_review, done)
### in_progress rejected  → exit 1, unknown goal phase 'in_progress' (valid: …)

Tests

• Unit: enum equality against AvailableTaskPhases, Contains, String(), derived message, in_progress rejection.
• Integration: goal/topic set success for both review phases; bogus and in_progress exit 1 with the six values and a byte-identical page; a hand-typed phase: in_progress reads back unchanged.

[ben-s-pull-request-reviewer]

The diff is fully reviewed. Let me now compile the verdict.

Mechanical funnel (ast-grep): NOT AVAILABLE — ast-grep/sg not in PATH. Per Step 4.0, reported as a Must Fix toolchain gap, but this is a Go-domain feature with no YAML rule surface, so it does not block.

Review summary:

• goal_phase.go / topic_phase.go: 2 new constants each (AIReview, HumanReview), added to the Available*Phases collections in declaration order, String() on the plural type renders the list from the enum, Validate error now includes (valid: %s) from the collection.
• goal_frontmatter.go: setPhaseField now returns the validator's error directly instead of rebuilding it — the validator's message (including the valid set) now surfaces to the CLI. Topic's setPhaseField already wrapped it, so no change needed.
• Error substring unknown goal phase '%s' is preserved (it appears in tests and probes).
• in_progress is still rejected by both validators.
• //nolint:dupl added with justification comment — goal and topic phases share the same enum shape but are distinct types, so duplication is intentional.
• Unit tests cover all 6 valid phases, rejection of in_progress and bogus, enum equality with AvailableTaskPhases, and derived-message rendering.
• Integration tests cover all four new phases (goal + topic × ai_review + human_review), rejection of bogus and in_progress with all six values in stderr, and byte-identical page after rejection.
• CHANGELOG.md has ## Unreleased below the preamble, above ## v0.174.1.

No issues found.

{
  "verdict": "approve",
  "summary": "Clean feature implementation: goal and topic phase enums widened from 4 to 6 canonical task phases, error messages now list accepted values rendered from the enum collection itself, and `in_progress` stays rejected. All acceptance criteria are covered by domain unit tests and integration tests.",
  "comments": [],
  "concerns_addressed": [
    {
      "concern": "correctness: goal_phase.go Validate now accepts 6 phases; in_progress alias still rejected",
      "disposition": "addressed",
      "detail": "Validated: GoalPhaseAIReview and GoalPhaseHumanReview added to constants and AvailableGoalPhases; Validate error uses AvailableGoalPhases; Contains and rejection tests cover all cases."
    },
    {
      "concern": "correctness: topic_phase.go Validate now accepts 6 phases; in_progress alias still rejected",
      "disposition": "addressed",
      "detail": "Validated: TopicPhaseAIReview and TopicPhaseHumanReview added to constants and AvailableTopicPhases; Validate error uses AvailableTopicPhases; Contains and rejection tests cover all cases."
    },
    {
      "concern": "correctness: goal_frontmatter.go setPhaseField surfaces validator error",
      "disposition": "addressed",
      "detail": "Validated: setPhaseField now returns err directly; topic setPhaseField already wrapped it. Error substring 'unknown goal phase' preserved in tests."
    },
    {
      "concern": "tests: goal_phase_test.go — all 6 phases, Contains, String, error message",
      "disposition": "addressed",
      "detail": "Validated: DescribeTable entries for all 6; Contains assertions for all 6; element-for-element equality vs AvailableTaskPhases; String() renders comma-separated list; derived-message test checks err contains the joined set; in_progress rejection test present."
    },
    {
      "concern": "tests: topic_phase_test.go — all 6 phases, Contains, String, error message",
      "disposition": "addressed",
      "detail": "Validated: same coverage as goal; the prior ai_review rejection case is removed and ai_review/human_review moved into the valid table."
    },
    {
      "concern": "tests: integration/cli_test.go — goal/topic set ai_review, human_review; rejection of bogus and in_progress",
      "disposition": "addressed",
      "detail": "Validated: 6 new integration specs (2 topic success + 2 topic rejection + 2 goal success + 2 goal rejection); bogus tests verify all 6 values in stderr; in_progress rejection tests verify all 6 values; byte-identical page after rejection."
    }
  ]
}

[ben-s-pull-request-reviewer]

Now I have all the information needed to complete the review. Let me summarize my findings.

Manual review findings:

1. pkg/domain/goal_phase.go — AvailableGoalPhases correctly widened from 4 to 6 phases. All 6 canonical phases present. Validate uses AvailableGoalPhases.Contains and the error message renders AvailableGoalPhases via its String() method, so the list cannot drift.

2. pkg/domain/topic_phase.go — identical structure, same 6 phases. String() method on TopicPhases renders the list by iterating the collection, matching AvailableTaskPhases exactly.

3. pkg/domain/goal_frontmatter.go — setPhaseField now surfaces phase.Validate(ctx) directly instead of rebuilding the error message. The spec's acceptance criterion "the local rebuild is gone, not merely supplemented" is met.

4. Unit tests (goal_phase_test.go, topic_phase_test.go) — both assert element-by-element equality with AvailableTaskPhases (6 elements each), reject in_progress, and verify the error message lists values derived from the collection.

5. Integration tests (cli_test.go) — test cases topic set refuses in_progress and goal set refuses in_progress verify the 6 accepted values are named in the error. Tests for ai_review and human_review write-and-read roundtrip.

6. CHANGELOG.md — ## Unreleased bullet correctly describes the widened enum and listed-valid-values error.

Concerns addressed:

• goal_phase.go / topic_phase.go widened to 6 phases, in_progress rejected → addressed
• goal_frontmatter.go surfaces validator error → addressed
• String() renders AvailableTopicPhases → addressed (verified TopicPhases.String() iterates t and joins)
• Version alignment → not an issue (## Unreleased means no release yet; auto-releaser will bump all four version strings post-merge)

Version note: Plugin manifests still at v0.174.1, which is correct — ## Unreleased means no version bump has occurred yet. The auto-releaser owns the post-merge bump.

{
  "verdict": "approve",
  "summary": "The PR widens goal and topic phase enums from 4 to 6 values, surfaces the validator error in goal setPhaseField, and renders the accepted-value list from the enum collection so it cannot drift. All acceptance criteria from spec 062 are covered by unit and integration tests. No correctness issues found.",
  "comments": [],
  "concerns_addressed": [
    {
      "concern": "correctness: goal_phase.go Validate widened from 4 to 6 phases",
      "disposition": "addressed",
      "detail": "AvailableGoalPhases has 6 entries matching AvailableTaskPhases; in_progress correctly rejected"
    },
    {
      "concern": "correctness: topic_phase.go Validate widened from 4 to 6 phases",
      "disposition": "addressed",
      "detail": "AvailableTopicPhases has 6 entries matching AvailableTaskPhases; in_progress correctly rejected"
    },
    {
      "concern": "correctness: goal_frontmatter.go setPhaseField surfaces validator error",
      "disposition": "addressed",
      "detail": "setPhaseField calls phase.Validate(ctx) and returns err directly; no local rebuild"
    },
    {
      "concern": "correctness: topic_phase.go String() renders valid-value list from AvailableTopicPhases",
      "disposition": "addressed",
      "detail": "TopicPhases.String() iterates the collection and strings.Join, matching AvailableTaskPhases exactly"
    },
    {
      "concern": "version alignment: plugin manifests at v0.174.1 with Unreleased in changelog",
      "disposition": "not-an-issue",
      "detail": "## Unreleased means no release has been cut yet; plugin manifests correctly stay at prior version until auto-releaser bumps post-merge"
    }
  ]
}