Skip to content

fix(#5320): record planned-phase plan count on Plan:/Current Plan: documents - #5424

Open
davdittrich wants to merge 1 commit into
open-gsd:nextfrom
davdittrich:fix/5320-planned-phase-plan-label
Open

davdittrich wants to merge 1 commit into
open-gsd:nextfrom
davdittrich:fix/5320-planned-phase-plan-label

Conversation

@davdittrich

Copy link
Copy Markdown
Contributor

Fix PR

Using the wrong template?
— Enhancement: use enhancement.md
— Feature: use feature.md


Linked Issue

Required. This PR will be auto-closed if no valid issue link is found.

Fixes #5320

The linked issue must have the confirmed-bug label. If it doesn't, ask a maintainer to confirm the bug before continuing.


What was broken

state.planned-phase --plans N exited 0 without recording the plan count when STATE.md carried a Plan: / Current Plan: Current Position line instead of Total Plans in Phase:. Plan: and a bare Status: Ready stayed stale and frontmatter progress.total_plans was not updated.

What this fix does

plannedPhaseCore now writes Plan: A of N in current phase (and the legacy Current Plan: spelling) through the existing Current Position writer, moves a bare Ready status to Ready to execute, and reports Plan / Current Plan in updated only when that line changed. A same-phase replan keeps the executed-plan counter (2 of 5 becomes 2 of 6); a different phase resets it, and a total below the executed count fails before any write. When --plans has no line to land on, it exits non-zero before any write; reruns on an already-planned document still exit 0. --plans accepts only plain non-negative safe integers.

Root cause

The count was written only through the Total Plans in Phase label, and the Current Position call never received plan/currentPlan. Ready is not a known template default, so Status was treated as executor-authored and left alone.

Testing

How I verified the fix

npm run build:lib && node --test tests/state-transition.test.cjs tests/state.test.cjs tests/state-document.test.cjs. The new #5320 tests in tests/state.test.cjs run the public CLI on the issue fixture and check the JSON result, the on-disk body and the derived frontmatter. They also cover the legacy Current Plan:-only document and advance-plan afterwards, replan progress, rerun idempotency, no place to record, and --plans abc / --plans -1. The red tests failed on unmodified next.

Regression test added?

  • Yes — added a test that would have caught this bug
  • No — explain why:

Platforms tested

  • macOS
  • Windows (including backslash path handling)
  • Linux
  • N/A (not platform-specific)

Runtimes tested

  • Claude Code
  • Antigravity
  • OpenCode
  • Other: ___
  • N/A (not runtime-specific)

Checklist

  • Issue linked above with Fixes #NNN — PR will be auto-closed if missing
  • Linked issue has the confirmed-bug label
  • Fix is scoped to the reported bug — no unrelated changes included
  • Regression test added (or explained why not)
  • All existing tests pass (npm test)
  • .changeset/ fragment added if this is a user-facing fix (npm run changeset -- --type Fixed --pr <NNN> --body "...") — or no-changelog label applied
  • No unnecessary dependencies added

Breaking changes

state planned-phase --plans N now exits non-zero, writing nothing, when STATE.md ## Current Position has none of Plan:, Current Plan: or Total Plans in Phase:. It previously exited 0 with nothing recorded. The error names the line to add. --plans values such as -1, 3.5 and 3abc are now rejected.

@davdittrich
davdittrich force-pushed the fix/5320-planned-phase-plan-label branch 3 times, most recently from c68c73c to ae26865 Compare October 15, 2026 08:38

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge conflict with next — blocks review and merge.

git merge-tree --write-tree origin/next <PR head> against next @ f7efbf110a reports conflicts in:

  • tests/fixtures/compact-content-benchmark-baseline.json

Please bring the branch current with next and resolve the conflict(s). The substantive review resumes once the PR is mergeable. This notice is about the conflict only; it does not retire any open finding from earlier reviews.

@trek-e trek-e added the needs-review: merge-conflict Has a merge conflict that must be resolved label Oct 17, 2026
…Plan: documents

begin-phase --plans on a planned phase left the Plan:/Current Plan: line without the plan count. Record it, validate --plans strictly, keep replan progress only within the same phase, and fail a planned phase with no plan line instead of silently no-opping.

Emitted-Drift-Ack-Growth: plan-phase.md — 13b now documents the new non-zero exit of planned-phase when no plan line exists; callers need it to handle the abort
@davdittrich
davdittrich force-pushed the fix/5320-planned-phase-plan-label branch from 35db850 to 3005ba1 Compare October 17, 2026 18:13
@davdittrich
davdittrich requested a review from trek-e October 17, 2026 18:18

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review: merge-conflict Has a merge conflict that must be resolved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(state): planned-phase exits 0 with updated: [] when STATE.md uses the Plan: Current Position label (no Total Plans in Phase)

2 participants