fix(plan-phase): update STATE.md after planning completes

[Tibsfox]

Summary

Closes #1626

After /gsd:plan-phase completes, STATE.md was never updated — it continued showing the previous phase state. The execute-phase workflow calls state begin-phase but plan-phase had no state update at all.

• Added STATE.md update to step 14 ("Present Final Status") in plan-phase.md
• Uses state update to set Status ("Phase N planned — ready to execute"), Last Activity (today's date), and Last Activity Description
• Commits the state change before routing to auto-advance or Next Up block
• Chose state update over state patch because STATE.md uses space-separated field names (Last Activity) that don't match state patch's hyphenated key parsing

Test plan

• New test tests/plan-phase-state-update.test.cjs (6 tests) verifies:
  • plan-phase.md contains state update calls
  • Status message includes "planned" and "ready to execute"
  • State update is positioned before auto-advance (step 15)
  • Last Activity field is updated
  • State change is committed
• All 2022 existing tests pass

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

[trek-e]

Blocking: insertion point creates ambiguity with step 15's auto-advance logic

The STATE.md update block is inserted at the top of step 14, before the <offer_next> routing. This is fine for the non-auto-advance path. But plan-phase step 15 already has its own tracking logic (reading AUTO_CHAIN, AUTO_CFG, and conditionally advancing). If a user is in auto-advance mode, the commit added in step 14 runs, then step 15 may also produce a commit for the auto-advance state transition. The two commits are not harmful but the step 14 commit message docs(state): record phase ${PHASE} planning complete fires unconditionally even when auto-advance immediately chains into execute-phase — at that point STATE.md will be overwritten again by execute-phase's own state update, making the step 14 commit a no-op commit with stale data.

Specific issues:

1. PLAN_COUNT uses ls -1 ${PHASE_DIR}/*-PLAN.md — PHASE_DIR is not defined at this point in the workflow. The workflow uses PHASE_NUMBER, PADDED_PHASE, and constructs paths inline. This will silently produce PLAN_COUNT=0 or error.

2. The commit command uses --files .planning/STATE.md but the gsd-tools commit subcommand does not accept a --files flag based on the existing usage patterns throughout the codebase. All other STATE.md commits are done via git add + git commit or via the gsd-tools commit without a files argument. Verify this flag exists in gsd-tools.cjs.

3. The test for "state update happens in step 14" uses string position arithmetic across the entire file content, which is brittle. It passes today but will break silently if section headers change. The core behavioral test (does the update actually run?) is not covered.

Please fix the PHASE_DIR reference, verify the --files flag exists, and add a guard so the step 14 commit is skipped when auto-advance will immediately overwrite STATE.md.

[trek-e]

Adversarial Re-Review — REQUEST CHANGES (Round 2)

Author force-pushed (new commit timestamp) but the diff is unchanged from the first review. None of the three requested fixes were addressed.

Prior Issue Tracking

1. PHASE_DIR undefined: RETRACTED
On closer inspection, PHASE_DIR is a contextual pseudo-variable used throughout plan-phase.md (steps 5, 6, 7, 8, 10, 12) in the same unassigned form. It is resolved by the LLM agent at runtime from the planning context. This is consistent with the existing workflow pattern. My original concern was incorrect — this is not a bug.

2. --files flag on gsd-tools commit: VERIFIED
gsd-tools.cjs lines 421-440 handle --files for the commit subcommand. This is correct.

3. Auto-advance guard to skip redundant commit: NOT ADDRESSED
The step 14 commit fires unconditionally. When auto-advance is active, execute-phase immediately overwrites STATE.md with its own state update, making the step 14 commit a no-op with stale data in the git history. This was the primary blocking request.

Fix required: Wrap the STATE.md update and commit in step 14 with a guard:

If auto-advance is NOT active (no --auto flag, AUTO_CHAIN is false, AUTO_CFG is false):
  [existing state update + commit block]

When auto-advance IS active, execute-phase's own state begin-phase call handles the state transition, so the step 14 commit is redundant.

Test brittleness (non-blocking, reiterated)

The step 14 position test uses string index arithmetic (indexOf comparisons). It works but will break silently if section headers change. Acceptable for now since the behavioral assertion (state update exists between step 14 and 15 headers) is sound.

[trek-e]

Code Review

Verdict: APPROVE

Security

No issues found. The added bash snippet uses wc -l | tr -d ' ' for safe whitespace stripping and only passes the result into a node CLI call with a fixed command name. No user-controlled input is interpolated into shell-unsafe positions.

Logic / Correctness

The PLAN_COUNT variable is computed with ls -1 ${PHASE_DIR}/*-PLAN.md 2>/dev/null | wc -l and then used only in the description string, so a zero-plan edge case produces "0 plans" rather than causing an error — acceptable. The three state update calls are sequenced correctly (Status → Last Activity → Last Activity Description), and the commit follows all three, which is the right order. Placement between step 14 and the Route to <offer_next> line is correct: state is recorded before the workflow exits to the next block.

The PR description's rationale for choosing state update over state patch (space-separated field names) is sound and consistent with how ship.md handles the same fields.

Test Coverage

Six tests covering the key assertions: file existence, presence of a state update call, correct status text, step-14 placement (bounded by both step 14 and step 15 header positions), Last Activity field, and commit of STATE.md. The positional test (stateUpdatePos > step14Pos && < step15Pos) is precise and will catch accidental regressions if the block moves. The content variable is initialised lazily but without resetting between tests — tests 2-6 rely on the read done in test 1 implicitly passing; if test 1 throws, the later tests will also throw with a sensible message (Cannot read properties of undefined). This is acceptable for a workflow-text test suite, though declaring content inside a before hook would be cleaner. Not a blocking issue.

All three CI platforms (macOS, Ubuntu, Windows) on Node 22 plus Ubuntu Node 24 are green.

Style

Consistent with surrounding workflow prose and existing test files in the repo. Commit message follows the fix(scope): description (#issue) convention. No issues.

Summary

Straightforward gap-fill: plan-phase was the only major workflow that never wrote back to STATE.md; this PR adds the missing update in the correct location with adequate tests and all CI green. Ready to merge.

[Tibsfox]

Closing — this was addressed upstream in #1647 (feat(state): add programmatic gates for STATE.md consistency), which added a dedicated state planned-phase command at step 13b in plan-phase.md. A cleaner approach than the individual state update calls here. Thanks for the fast turnaround! 🎉