Stop telling a successor the tree is settled when every phase is stale
Run 8 rendered three phases as "verified at 14654d6cab63, stale because the tree is now at 56d9b9acdd59" and then printed "Every phase is verified" under them. The staleness was honest and the conclusion was not. 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 the successor needed was the names: which phases were verified against code that has since changed, and an instruction to reverify what its work touches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CVbaKucEYBjMqVeUgJUsc1
This commit is contained in:
@@ -416,6 +416,13 @@ func fallback(s string) string {
|
|||||||
// plan -> accepted research
|
// plan -> accepted research
|
||||||
// implement -> accepted research and accepted plan
|
// implement -> accepted research and accepted plan
|
||||||
// review -> 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
|
// renderPlanProgress states what Orchestra established about the accepted
|
||||||
// plan, which is the half a rotated successor cannot reconstruct. A verified
|
// 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
|
// 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 == "" {
|
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()
|
return b.String()
|
||||||
}
|
}
|
||||||
b.WriteString(planMismatchBrief)
|
b.WriteString(planMismatchBrief)
|
||||||
|
|||||||
@@ -660,3 +660,39 @@ func TestLegacyPlanSaysProgressIsUnavailable(t *testing.T) {
|
|||||||
t.Fatal("the legacy plan text was lost")
|
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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user