From 76da8c40b7349b140a4e57fce9a5cfb1197ea49e Mon Sep 17 00:00:00 2001 From: kami Date: Sun, 30 Aug 2026 05:46:11 +0400 Subject: [PATCH] End the implementer's trajectory when Orchestra reopens the phase Run 20 showed the leak precisely: the reopen rotated the session, and 31 seconds later the planning session that replaced it recorded phase-2 of the plan being replaced. The implementer's .orchestra/plan-progress.json had outlived it in the worktree, and the successor executed it. Two changes. Phase verification now belongs to the implement phase and is refused anywhere else, which covers every worker and every path rather than the one that produced it. The rotation also drops the ended session's request files, so a successor never inherits work from a trajectory Orchestra has already invalidated. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CVbaKucEYBjMqVeUgJUsc1 --- cmd/orchestra-worker/main.go | 9 ++++++++ cmd/orchestra-worker/phase_test.go | 29 ++++++++++++++++++++++++ internal/operations/planprogress.go | 7 ++++++ internal/operations/planprogress_test.go | 27 ++++++++++++++++++++++ 4 files changed, 72 insertions(+) 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()) + } +}