diff --git a/cmd/orchestra-worker/main.go b/cmd/orchestra-worker/main.go index bd9389a..faf8d39 100644 --- a/cmd/orchestra-worker/main.go +++ b/cmd/orchestra-worker/main.go @@ -2089,6 +2089,15 @@ func (w *worker) rotateForPhase(ctx context.Context, id string, a herdr.Adapter, s.HandoffRequested, s.HandoffReason, s.HandoffRequestedAt = true, "phase_changed", time.Now().UTC() w.sessions[id] = s _ = w.save() + // A request belongs to the session that wrote it. Both files sit in the + // worktree, which outlives the session, so a successor in a different + // phase would find and execute them: that is how a reopened planning + // session verified a phase of the plan it was replacing. + for _, name := range []string{planProgressFile, phaseRequestFile} { + if err := os.Remove(filepath.Join(s.Worktree, ".orchestra", name)); err != nil && !os.IsNotExist(err) { + w.recordError(fmt.Errorf("phase rotation %s: drop %s: %w", id, name, err)) + } + } log.Printf("phase changed for %s: session rotating", id) } diff --git a/cmd/orchestra-worker/phase_test.go b/cmd/orchestra-worker/phase_test.go index 1cf3a5d..9a7a6ea 100644 --- a/cmd/orchestra-worker/phase_test.go +++ b/cmd/orchestra-worker/phase_test.go @@ -457,3 +457,32 @@ func TestPlanVerificationRunsThePlansCommandsAndReportsExitCodes(t *testing.T) { t.Fatalf("the outcome was not delivered: %v", backend.prompts) } } + +// A request belongs to the session that wrote it. The worktree outlives the +// session, so a rotation that leaves these files behind hands them to a +// successor running in a different phase (run 20). +func TestRotationDropsTheEndedSessionsRequests(t *testing.T) { + w, backend, wt, done := phaseWorker(t, func(rw http.ResponseWriter, r *http.Request) { + rw.Write([]byte(`{}`)) + }) + defer done() + progress := filepath.Join(wt, ".orchestra", planProgressFile) + request := filepath.Join(wt, ".orchestra", phaseRequestFile) + for _, p := range []string{progress, request} { + if err := os.WriteFile(p, []byte(`{"phase":"phase-1","status":"ready_for_verification"}`), 0o644); err != nil { + t.Fatal(err) + } + } + + a := herdr.CLIAdapter{Backend: backend, Harness: "claude"} + w.rotateForPhase(context.Background(), "task", a, w.sessions["task"]) + + for _, p := range []string{progress, request} { + if _, err := os.Stat(p); !os.IsNotExist(err) { + t.Fatalf("%s survived the rotation that ended the session that wrote it", filepath.Base(p)) + } + } + if s := w.sessions["task"]; !s.HandoffRequested || s.HandoffReason != "phase_changed" { + t.Fatalf("the session was not rotated: %+v", s) + } +} diff --git a/internal/operations/planprogress.go b/internal/operations/planprogress.go index f8c5e5e..143b494 100644 --- a/internal/operations/planprogress.go +++ b/internal/operations/planprogress.go @@ -43,6 +43,13 @@ func PlanPhaseCommands(s *store.Store, project registry.Project, taskID, phaseID if t.PlanRef == "" { return workphase.PlanPhase{}, fmt.Errorf("%w: this task has no accepted plan", ErrPlanPhase) } + // Verification is implementation work. A request that arrives in another + // phase belongs to a trajectory Orchestra has already ended: run 20's + // reopened planning session executed the implementer's leftover request + // and recorded a phase of a plan that was being replaced. + if current(t) != domain.WorkPhaseImplement { + return workphase.PlanPhase{}, fmt.Errorf("%w: phase verification belongs to the implement phase, and this task is in %s", ErrPlanPhase, current(t)) + } raw, err := s.Artifact(t.PlanRef) if err != nil { return workphase.PlanPhase{}, fmt.Errorf("read accepted plan: %w", err) diff --git a/internal/operations/planprogress_test.go b/internal/operations/planprogress_test.go index 3a41746..158d945 100644 --- a/internal/operations/planprogress_test.go +++ b/internal/operations/planprogress_test.go @@ -330,3 +330,30 @@ func TestASignOffDoesNotSurviveTheTreeItWasGivenAgainst(t *testing.T) { t.Fatalf("a fresh sign-off did not verify the current tree: %+v", rec) } } + +// Run 20: a replan reopened the plan phase, the implementer's leftover +// verification request outlived its session, and the planning session that +// replaced it executed the request. Orchestra recorded a verified phase of the +// plan it was in the middle of replacing. +func TestVerificationIsRefusedOutsideImplement(t *testing.T) { + s, project, id := planWith(t, twoPhasePlan) + task, _ := s.Task(id) + m := mismatch(task.PlanRef) + m.RequestedAction = domain.PlanMismatchReplan + if _, err := RecordPlanMismatch(s, project, id, m, shaOne); err != nil { + t.Fatal(err) + } + assertPhase(t, s, id, domain.WorkPhasePlan) + + _, err := RecordPlanPhaseVerification(s, project, id, "phase-1", shaOne, + []VerificationRun{{Command: []string{"go", "build", "./..."}, ExitCode: 0}}) + if !errors.Is(err, ErrPlanPhase) { + t.Fatalf("a reopened task verified a phase of the plan being replaced: %v", err) + } + if !strings.Contains(err.Error(), "implement") { + t.Fatalf("the refusal does not say which phase owns verification: %v", err) + } + if after, _ := s.Task(id); len(after.PlanPhases()) != 0 { + t.Fatalf("progress was recorded anyway: %+v", after.PlanPhases()) + } +}