diff --git a/internal/agentctx/agentctx.go b/internal/agentctx/agentctx.go index c109cd8..37a2506 100644 --- a/internal/agentctx/agentctx.go +++ b/internal/agentctx/agentctx.go @@ -416,6 +416,13 @@ func fallback(s string) string { // plan -> accepted research // implement -> accepted research and accepted plan // review -> accepted plan +func plural(n int, one, many string) string { + if n == 1 { + return one + } + return many +} + // renderPlanProgress states what Orchestra established about the accepted // plan, which is the half a rotated successor cannot reconstruct. A verified // phase is named with the commit it was verified at, and labelled stale when @@ -452,7 +459,23 @@ func renderPlanProgress(in Input) string { } } if current == "" { - b.WriteString("\nEvery phase is verified.\n") + // Stale still counts as done: the usual reason a phase goes stale is + // the next phase's own commit, and treating that as unverified would + // send the implementer back to phase 1 after every commit. What must + // not happen is telling a successor the tree is settled when the code + // under a verification has since changed, so name the stale phases + // rather than claiming the plan is finished. + var stale []string + for _, rec := range records { + if rec.Status == domain.PlanPhaseVerified && rec.Stale(in.Git.HeadSHA) { + stale = append(stale, rec.PhaseID) + } + } + if len(stale) == 0 { + b.WriteString("\nEvery phase is verified at the current tree.\n") + return b.String() + } + fmt.Fprintf(&b, "\nEvery phase is verified, but %s %s verified against code that has since changed. Reverify what your work touches by writing .orchestra/plan-progress.json for that phase.\n", strings.Join(stale, ", "), plural(len(stale), "was", "were")) return b.String() } b.WriteString(planMismatchBrief) diff --git a/internal/agentctx/agentctx_test.go b/internal/agentctx/agentctx_test.go index 4323bc0..3885a15 100644 --- a/internal/agentctx/agentctx_test.go +++ b/internal/agentctx/agentctx_test.go @@ -660,3 +660,39 @@ func TestLegacyPlanSaysProgressIsUnavailable(t *testing.T) { t.Fatal("the legacy plan text was lost") } } + +// A successor must never be told the tree is settled when every verification +// was earned against code that has since changed. Run 8 printed "Every phase +// is verified" under three phases all rendered stale in the same block. +func TestAllPhasesStaleIsNotReportedAsFinished(t *testing.T) { + plan := &workphase.PlanDoc{Phases: []workphase.PlanPhase{ + {ID: "phase-1", Name: "one"}, {ID: "phase-2", Name: "two"}, + }} + task := domain.Task{ + ID: "t1", PlanRef: "ref-a", + PlanProgress: &domain.PlanProgress{PlanRef: "ref-a", Phases: []domain.PlanPhaseRecord{ + {PlanRef: "ref-a", PhaseID: "phase-1", Status: domain.PlanPhaseVerified, AtSHA: strings.Repeat("a", 40)}, + {PlanRef: "ref-a", PhaseID: "phase-2", Status: domain.PlanPhaseVerified, AtSHA: strings.Repeat("a", 40)}, + }}, + } + stale := renderPlanProgress(Input{ + Task: task, Phase: domain.WorkPhaseImplement, Plan: plan, + Git: GitState{HeadSHA: strings.Repeat("b", 40)}, + }) + if strings.Contains(stale, "Every phase is verified at the current tree") { + t.Error("a fully stale plan was reported as verified at the current tree") + } + for _, want := range []string{"phase-1, phase-2", "since changed"} { + if !strings.Contains(stale, want) { + t.Errorf("stale render missing %q:\n%s", want, stale) + } + } + + current := renderPlanProgress(Input{ + Task: task, Phase: domain.WorkPhaseImplement, Plan: plan, + Git: GitState{HeadSHA: strings.Repeat("a", 40)}, + }) + if !strings.Contains(current, "Every phase is verified at the current tree") { + t.Errorf("a plan verified at HEAD was not reported as finished:\n%s", current) + } +}