Files
orchestra/internal/operations/planprogress_test.go
T
kami a221502356 Let Orchestra establish plan progress instead of the implementer asserting it
A detailed plan that nothing enforces is a document. This makes the phases
executable: the implementer may write exactly one status, and every other
status is a conclusion Orchestra reaches by running the plan's own commands.

    agent may request:  ready_for_verification
    agent may not assert: verified, awaiting_manual_verification, failed, skipped

The worker resolves commands from the coordinator, never from the request, so a
request cannot smuggle in a command the planner did not write. They run as argv
through exec with Dir set to the worktree, which is the quality gate's existing
envelope and not a weaker one. There is no shell, so a pipe is a literal
argument.

Project policy decides executable reach. registry.Project.Verification matches
argv positionally, and an absent policy refuses everything: a plan command is
agent-authored, so inheriting the operator-authored gate's reach by default
would be the wrong direction to fail in. A refused command is refused before
anything runs, and the refusal names the project and the command so the planner
learns its real reach.

Two bindings make the record mean something later. PlanRef, so progress earned
under plan A cannot survive into plan B. AtSHA, so "verified" does not outlive
the code that made it true: a record whose commit has moved is retained as
provenance and rendered as stale, never as a claim about the current tree.
Both are the same failure this codebase already fixed for reviews, which bind
to the commit they examined.

Manual steps hold a phase at awaiting_manual_verification. The sign-off is an
ordinary human decision whose subject carries the plan ref and the phase id, so
a later "looks good" on an unrelated thread cannot satisfy a gate nobody was
discussing.

A plan sealed before plan.md declares no executable unit, and says so: the
implement context states that phase progress is unavailable and the work
continues under the old semantics. Inventing phases it never had would be worse
than admitting it has none.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CVbaKucEYBjMqVeUgJUsc1
2026-08-28 11:59:39 +04:00

268 lines
8.1 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"
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)
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()
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": "signoff-" + subject},
}),
}); err != nil {
t.Fatalf("sign off: %v", err)
}
}