76da8c40b7
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CVbaKucEYBjMqVeUgJUsc1
360 lines
12 KiB
Go
360 lines
12 KiB
Go
package operations
|
|
|
|
import (
|
|
"errors"
|
|
"strings"
|
|
"testing"
|
|
|
|
"orchestra/internal/authz"
|
|
"orchestra/internal/domain"
|
|
"orchestra/internal/registry"
|
|
"orchestra/internal/store"
|
|
)
|
|
|
|
const shaOne = "1111111111111111111111111111111111111111"
|
|
const shaTwo = "2222222222222222222222222222222222222222"
|
|
const shaThree = "3333333333333333333333333333333333333333"
|
|
|
|
func planProject() registry.Project {
|
|
p := registry.Project{
|
|
ID: "demo", MachineAffinity: []string{"m"},
|
|
WorkPhases: []domain.WorkPhase{domain.WorkPhaseFrame, domain.WorkPhaseResearch, domain.WorkPhasePlan, domain.WorkPhaseImplement, domain.WorkPhaseReview},
|
|
}
|
|
p.Verification.Allowed = [][]string{{"go", "test", "./internal/..."}, {"go", "build", "./..."}}
|
|
return p
|
|
}
|
|
|
|
// planWith seals research and a plan, leaving the task in implement.
|
|
func planWith(t *testing.T, markdown string) (*store.Store, registry.Project, string) {
|
|
t.Helper()
|
|
s, id := phaseStore(t)
|
|
// Leased, because everything these tests drive comes from a live implement
|
|
// session. Skipping it hid F65: two coordinator-side stops omitted the
|
|
// fencing fields Store.Append requires on a leased task, and every test
|
|
// passed because no test ever leased one.
|
|
lease(t, s, id)
|
|
project := planProject()
|
|
if _, err := AdvanceWorkPhase(s, project, id, nil); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if _, err := AdvanceWorkPhase(s, project, id, sealed(t, research)); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if _, err := AdvanceWorkPhase(s, project, id, []byte(markdown)); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
return s, project, id
|
|
}
|
|
|
|
const twoPhasePlan = "# Two phase plan\n" + `
|
|
## Overview
|
|
Two phases.
|
|
|
|
## Current state
|
|
Nothing, per research:r1.
|
|
|
|
## Desired end state
|
|
Both phases done.
|
|
|
|
## Non-goals
|
|
None.
|
|
|
|
## Approach
|
|
Straightforward.
|
|
|
|
## Phase 1: Build
|
|
|
|
### Files
|
|
- a.go
|
|
|
|
### Changes
|
|
Add a.
|
|
|
|
### Verification
|
|
|
|
#### Automated
|
|
- run: ["go", "build", "./..."]
|
|
|
|
## Phase 2: Test
|
|
|
|
### Files
|
|
- b.go
|
|
|
|
### Changes
|
|
Add b.
|
|
|
|
### Verification
|
|
|
|
#### Automated
|
|
- run: ["go", "test", "./internal/..."]
|
|
|
|
#### Manual
|
|
- Confirm the output by eye.
|
|
|
|
## Testing strategy
|
|
Per phase.
|
|
|
|
## Risks and edge cases
|
|
None.
|
|
|
|
## Migration
|
|
None.
|
|
|
|
## References
|
|
- research:r1
|
|
`
|
|
|
|
func TestPassingCommandsVerifyThePhase(t *testing.T) {
|
|
s, project, id := planWith(t, twoPhasePlan)
|
|
if _, err := RecordPlanPhaseVerification(s, project, id, "phase-1", shaOne,
|
|
[]VerificationRun{{Command: []string{"go", "build", "./..."}, ExitCode: 0}}); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
task, _ := s.Task(id)
|
|
rec, ok := task.PlanPhase("phase-1")
|
|
if !ok || rec.Status != domain.PlanPhaseVerified {
|
|
t.Fatalf("phase-1 = %+v", rec)
|
|
}
|
|
if rec.AtSHA != shaOne || rec.PlanRef != task.PlanRef {
|
|
t.Fatalf("record is not bound to the plan and the commit: %+v", rec)
|
|
}
|
|
}
|
|
|
|
// A failing command leaves the phase where it was. "Verified" is a conclusion
|
|
// about the commands, never about the request that asked for them.
|
|
func TestFailingCommandLeavesThePhaseInProgress(t *testing.T) {
|
|
s, project, id := planWith(t, twoPhasePlan)
|
|
if _, err := RecordPlanPhaseVerification(s, project, id, "phase-1", shaOne,
|
|
[]VerificationRun{{Command: []string{"go", "build", "./..."}, ExitCode: 2}}); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
task, _ := s.Task(id)
|
|
rec, _ := task.PlanPhase("phase-1")
|
|
if rec.Status != domain.PlanPhaseInProgress {
|
|
t.Fatalf("status = %q, want in_progress", rec.Status)
|
|
}
|
|
if len(rec.ExitCodes) != 1 || rec.ExitCodes[0] != 2 {
|
|
t.Fatalf("the exit code was not recorded: %+v", rec)
|
|
}
|
|
}
|
|
|
|
// Automated checks passing is not the whole phase when manual steps exist.
|
|
func TestManualStepsHoldThePhaseUntilAHumanSignsOff(t *testing.T) {
|
|
s, project, id := planWith(t, twoPhasePlan)
|
|
if _, err := RecordPlanPhaseVerification(s, project, id, "phase-2", shaOne,
|
|
[]VerificationRun{{Command: []string{"go", "test", "./internal/..."}, ExitCode: 0}}); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
task, _ := s.Task(id)
|
|
rec, _ := task.PlanPhase("phase-2")
|
|
if rec.Status != domain.PlanPhaseAwaitingManual {
|
|
t.Fatalf("status = %q, want awaiting_manual_verification", rec.Status)
|
|
}
|
|
|
|
// A generic later comment must not satisfy the gate.
|
|
humanReply(t, s, id, "d-generic", "looks good")
|
|
task, _ = s.Task(id)
|
|
rec, _ = task.PlanPhase("phase-2")
|
|
if rec.Status != domain.PlanPhaseAwaitingManual {
|
|
t.Fatal("an unrelated comment satisfied a manual verification gate")
|
|
}
|
|
|
|
// The sign-off names the plan and the phase it approves.
|
|
signOff(t, s, id, domain.PlanPhaseSubject(task.PlanRef, "phase-2"))
|
|
task, _ = s.Task(id)
|
|
rec, _ = task.PlanPhase("phase-2")
|
|
if rec.Status != domain.PlanPhaseVerified {
|
|
t.Fatalf("status = %q after a bound sign-off, want verified", rec.Status)
|
|
}
|
|
}
|
|
|
|
// A sign-off is bound to one plan. Replanning does not inherit it.
|
|
func TestSignOffForAnotherPlanDoesNotVerify(t *testing.T) {
|
|
s, project, id := planWith(t, twoPhasePlan)
|
|
if _, err := RecordPlanPhaseVerification(s, project, id, "phase-2", shaOne,
|
|
[]VerificationRun{{Command: []string{"go", "test", "./internal/..."}, ExitCode: 0}}); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
signOff(t, s, id, domain.PlanPhaseSubject("some-other-plan-ref", "phase-2"))
|
|
task, _ := s.Task(id)
|
|
rec, _ := task.PlanPhase("phase-2")
|
|
if rec.Status != domain.PlanPhaseAwaitingManual {
|
|
t.Fatalf("a sign-off naming another plan verified this one: %q", rec.Status)
|
|
}
|
|
}
|
|
|
|
// Verified at X, code moves to Y: the record stands as provenance and must
|
|
// read as stale, never as a claim about the current tree.
|
|
func TestVerificationGoesStaleWhenTheTreeMoves(t *testing.T) {
|
|
s, project, id := planWith(t, twoPhasePlan)
|
|
if _, err := RecordPlanPhaseVerification(s, project, id, "phase-1", shaOne,
|
|
[]VerificationRun{{Command: []string{"go", "build", "./..."}, ExitCode: 0}}); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
task, _ := s.Task(id)
|
|
rec, _ := task.PlanPhase("phase-1")
|
|
if rec.Stale(shaOne) {
|
|
t.Fatal("a verification at the current head reported stale")
|
|
}
|
|
if !rec.Stale(shaTwo) {
|
|
t.Fatal("a verification at an older commit did not report stale")
|
|
}
|
|
}
|
|
|
|
// The commands come from the accepted plan. A request cannot substitute one.
|
|
func TestReportedResultsMustMatchThePlansCommands(t *testing.T) {
|
|
s, project, id := planWith(t, twoPhasePlan)
|
|
_, err := RecordPlanPhaseVerification(s, project, id, "phase-1", shaOne,
|
|
[]VerificationRun{{Command: []string{"echo", "ok"}, ExitCode: 0}})
|
|
if !errors.Is(err, ErrPlanPhase) {
|
|
t.Fatalf("a substituted command was accepted: %v", err)
|
|
}
|
|
}
|
|
|
|
// A command outside project policy is refused before anything runs, and the
|
|
// refusal names the project so the planner learns its real reach.
|
|
func TestPolicyRefusalHappensBeforeExecution(t *testing.T) {
|
|
s, project, id := planWith(t, twoPhasePlan)
|
|
project.Verification.Allowed = [][]string{{"go", "build", "./..."}}
|
|
_, err := PlanPhaseCommands(s, project, id, "phase-2")
|
|
if !errors.Is(err, ErrPlanPhase) {
|
|
t.Fatalf("an unauthorised command was resolved: %v", err)
|
|
}
|
|
if !strings.Contains(err.Error(), "demo") || !strings.Contains(err.Error(), "go test") {
|
|
t.Fatalf("the refusal does not name the project and the command: %v", err)
|
|
}
|
|
task, _ := s.Task(id)
|
|
if _, ok := task.PlanPhase("phase-2"); ok {
|
|
t.Fatal("a refused phase produced a record")
|
|
}
|
|
}
|
|
|
|
// A plan sealed before plan.md declares no executable unit, and saying so
|
|
// beats inventing a phase it never had.
|
|
func TestLegacyPlanIsExplicitlyNonProgressable(t *testing.T) {
|
|
s, id := phaseStore(t)
|
|
project := planProject()
|
|
if _, err := AdvanceWorkPhase(s, project, id, nil); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if _, err := AdvanceWorkPhase(s, project, id, sealed(t, research)); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
// A pre-markdown plan, appended straight to the CAS the way an old ref
|
|
// would already be there.
|
|
ref, err := s.PutArtifact([]byte(`{"changes":[{"target":"a.go","intent":"do a thing"}]}`))
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
task, _ := s.Task(id)
|
|
task.PlanRef = ref
|
|
_, err = PlanPhaseCommands(s, project, id, "phase-1")
|
|
if err == nil {
|
|
t.Fatal("a legacy plan resolved a phase")
|
|
}
|
|
}
|
|
|
|
// signOff records a human decision bound to one phase of one plan, which is
|
|
// the only thing that satisfies a manual verification gate.
|
|
func signOff(t *testing.T, s *store.Store, taskID, subject string) {
|
|
t.Helper()
|
|
signOffFrom(t, s, taskID, subject, "signoff-"+subject)
|
|
}
|
|
|
|
// signOffFrom names the comment the sign-off came from. Two sign-offs on one
|
|
// subject are a real sequence once a rerun sends a phase back to the human,
|
|
// and provenance is unique per comment.
|
|
func signOffFrom(t *testing.T, s *store.Store, taskID, subject, externalID string) {
|
|
t.Helper()
|
|
task, _ := s.Task(taskID)
|
|
if err := s.Append(domain.Event{
|
|
ID: domain.NewID(), Type: domain.EventHumanDecisionRecorded, TaskID: taskID,
|
|
Version: task.Version + 1, Surface: string(authz.System),
|
|
Payload: mustJSONBytes(t, map[string]any{
|
|
"decision_id": domain.NewID(), "kind": "answer", "subject": subject,
|
|
"value": "manual steps confirmed",
|
|
"source": map[string]any{"provider": "gitea", "external_id": externalID},
|
|
}),
|
|
}); err != nil {
|
|
t.Fatalf("sign off: %v", err)
|
|
}
|
|
}
|
|
|
|
// F63, found live on run 19. A manual sign-off says a human read what this
|
|
// code prints. An edit afterwards can change exactly that, so rerunning the
|
|
// automated half at a new commit must not carry the human half with it.
|
|
func TestASignOffDoesNotSurviveTheTreeItWasGivenAgainst(t *testing.T) {
|
|
s, project, id := planWith(t, twoPhasePlan)
|
|
run := []VerificationRun{{Command: []string{"go", "test", "./internal/..."}, ExitCode: 0}}
|
|
if _, err := RecordPlanPhaseVerification(s, project, id, "phase-2", shaOne, run); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
task, _ := s.Task(id)
|
|
signOff(t, s, id, domain.PlanPhaseSubject(task.PlanRef, "phase-2"))
|
|
task, _ = s.Task(id)
|
|
rec, _ := task.PlanPhase("phase-2")
|
|
if rec.Status != domain.PlanPhaseVerified || rec.ManualAtSHA != shaOne {
|
|
t.Fatalf("sign-off did not bind to the tree it read: %+v", rec)
|
|
}
|
|
|
|
// The tree moves and the phase is re-verified. The commands pass again;
|
|
// the human has not seen the new output.
|
|
if _, err := RecordPlanPhaseVerification(s, project, id, "phase-2", shaTwo, run); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
task, _ = s.Task(id)
|
|
rec, _ = task.PlanPhase("phase-2")
|
|
if rec.Status != domain.PlanPhaseAwaitingManual {
|
|
t.Fatalf("status = %q at a tree the human never saw, want awaiting_manual_verification", rec.Status)
|
|
}
|
|
if rec.ManualAtSHA != shaOne {
|
|
t.Fatalf("the confirmed tree was lost: %+v", rec)
|
|
}
|
|
|
|
// A second rerun must not re-inherit it either, which is what carrying
|
|
// ManualAtSHA forward is for.
|
|
if _, err := RecordPlanPhaseVerification(s, project, id, "phase-2", shaThree, run); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
task, _ = s.Task(id)
|
|
rec, _ = task.PlanPhase("phase-2")
|
|
if rec.Status != domain.PlanPhaseAwaitingManual {
|
|
t.Fatalf("a second rerun re-inherited the sign-off: %q", rec.Status)
|
|
}
|
|
|
|
// Signing off again, on the tree that is now current, verifies it.
|
|
signOffFrom(t, s, id, domain.PlanPhaseSubject(task.PlanRef, "phase-2"), "signoff-second")
|
|
task, _ = s.Task(id)
|
|
rec, _ = task.PlanPhase("phase-2")
|
|
if rec.Status != domain.PlanPhaseVerified || rec.ManualAtSHA != shaThree {
|
|
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())
|
|
}
|
|
}
|