← back to Cli Printing Press

fix(pipeline): backfill PlanStatus in migration and persist state

3a9e977bf9f9278ce35c7bfe606fb5c7f3eb3f86 · 2026-03-27 19:42:40 -0700 · Trevin Chow

Fixes from code review:
- Migration else-branch now backfills PlanStatus=completed for v1
  phases that have Status=completed but empty PlanStatus. Without this,
  NextPhase() treats them as pending and re-executes completed phases.
- Migration now calls Save() to persist to disk, preventing repeated
  re-migration on every load.
- Test updated: v1 fixture includes phases with empty PlanStatus to
  cover the real-world case, plus NextPhase() assertion on migrated state.

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

Files touched

• M internal/pipeline/state.go
• M internal/pipeline/state_test.go

Diff

commit 3a9e977bf9f9278ce35c7bfe606fb5c7f3eb3f86
Author: Trevin Chow <trevin@trevinchow.com>
Date:   Fri Mar 27 19:42:40 2026 -0700

    fix(pipeline): backfill PlanStatus in migration and persist state
    
    Fixes from code review:
    - Migration else-branch now backfills PlanStatus=completed for v1
      phases that have Status=completed but empty PlanStatus. Without this,
      NextPhase() treats them as pending and re-executes completed phases.
    - Migration now calls Save() to persist to disk, preventing repeated
      re-migration on every load.
    - Test updated: v1 fixture includes phases with empty PlanStatus to
      cover the real-world case, plus NextPhase() assertion on migrated state.
    
    Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 internal/pipeline/state.go      | 14 +++++++++++++-
 internal/pipeline/state_test.go | 14 +++++++++++---
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/internal/pipeline/state.go b/internal/pipeline/state.go
index 7c5c5b0a..07904feb 100644
--- a/internal/pipeline/state.go
+++ b/internal/pipeline/state.go
@@ -136,13 +136,25 @@ func LoadState(apiName string) (*PipelineState, error) {
 					PlanPath:   filepath.Join(PipelineDir(apiName), fmt.Sprintf("%s-plan.md", name)),
 				}
 			} else {
-				// Migrate existing phases from index-based to name-based PlanPath.
+				// Migrate existing phases: update PlanPath from index-based to
+				// name-based format, and backfill PlanStatus for completed phases
+				// that predate the PlanStatus field.
 				p := s.Phases[name]
 				p.PlanPath = filepath.Join(PipelineDir(apiName), fmt.Sprintf("%s-plan.md", name))
+				if p.Status == StatusCompleted && p.PlanStatus == "" {
+					p.PlanStatus = PlanStatusCompleted
+				}
 				s.Phases[name] = p
 			}
 		}
 		s.Version = currentStateVersion
+		// Persist the migration so it doesn't re-run on every load.
+		// Note: plan files on disk keep their old index-based names.
+		// Completed phases' plans are not re-read; pending phases get
+		// new seeds written at the new paths by Init.
+		if err := s.Save(); err != nil {
+			return nil, fmt.Errorf("saving migrated state: %w", err)
+		}
 	}
 	return &s, nil
 }
diff --git a/internal/pipeline/state_test.go b/internal/pipeline/state_test.go
index b4ee6abf..31651e75 100644
--- a/internal/pipeline/state_test.go
+++ b/internal/pipeline/state_test.go
@@ -161,15 +161,16 @@ func TestLoadStateMigratesV1ToV2(t *testing.T) {
 	require.NoError(t, os.MkdirAll(dir, 0o755))
 	defer os.RemoveAll(dir)
 
-	// Simulate a v1 state file without agent-readiness phase and with index-based paths.
+	// Simulate a v1 state file without agent-readiness phase, with index-based
+	// paths, and some completed phases missing PlanStatus (pre-PlanStatus code).
 	v1State := PipelineState{
 		Version:   1,
 		APIName:   apiName,
 		OutputDir: "/tmp/migrate-cli",
 		Phases: map[string]PhaseState{
 			PhasePreflight:   {Status: StatusCompleted, PlanStatus: PlanStatusCompleted, PlanPath: dir + "/00-preflight-plan.md"},
-			PhaseResearch:    {Status: StatusCompleted, PlanStatus: PlanStatusCompleted, PlanPath: dir + "/01-research-plan.md"},
-			PhaseScaffold:    {Status: StatusCompleted, PlanStatus: PlanStatusCompleted, PlanPath: dir + "/02-scaffold-plan.md"},
+			PhaseResearch:    {Status: StatusCompleted, PlanPath: dir + "/01-research-plan.md"},  // no PlanStatus (pre-PlanStatus v1)
+			PhaseScaffold:    {Status: StatusCompleted, PlanPath: dir + "/02-scaffold-plan.md"},  // no PlanStatus
 			PhaseEnrich:      {Status: StatusCompleted, PlanStatus: PlanStatusCompleted, PlanPath: dir + "/03-enrich-plan.md"},
 			PhaseRegenerate:  {Status: StatusCompleted, PlanStatus: PlanStatusCompleted, PlanPath: dir + "/04-regenerate-plan.md"},
 			PhaseReview:      {Status: StatusCompleted, PlanStatus: PlanStatusCompleted, PlanPath: dir + "/05-review-plan.md"},
@@ -198,8 +199,15 @@ func TestLoadStateMigratesV1ToV2(t *testing.T) {
 		assert.Equal(t, expected, loaded.Phases[name].PlanPath, "migrated PlanPath for %s", name)
 	}
 
+	// Completed phases with empty PlanStatus get backfilled.
+	assert.Equal(t, PlanStatusCompleted, loaded.Phases[PhaseResearch].PlanStatus, "PlanStatus backfilled for research")
+	assert.Equal(t, PlanStatusCompleted, loaded.Phases[PhaseScaffold].PlanStatus, "PlanStatus backfilled for scaffold")
+
 	// Existing phase statuses preserved (comparative was pending).
 	assert.Equal(t, StatusPending, loaded.Phases[PhaseComparative].Status)
+
+	// NextPhase() skips the backfilled agent-readiness and returns comparative.
+	assert.Equal(t, PhaseComparative, loaded.NextPhase())
 }
 
 func TestPhaseStateJSONIncludesPlanStatus(t *testing.T) {

← da9500e4 refactor(cli): use ClaimOutputDir for atomic output director  ·  back to Cli Printing Press  ·  fix(cli): capture explicitOutput before default assignment ca5c11fd →