diff --git a/cmd/orchestra-worker/main.go b/cmd/orchestra-worker/main.go index 94f01f3..6549cda 100644 --- a/cmd/orchestra-worker/main.go +++ b/cmd/orchestra-worker/main.go @@ -1776,6 +1776,13 @@ func (w *worker) federatedTurn(ctx context.Context, id string, a herdr.Adapter, w.rotateForPhase(ctx, id, a, s) return } + // A plan phase asks to be verified before the work phase asks to move. + // Checked first because verifying the last phase is usually what makes a + // session ready to leave implement at all, and this session keeps running + // either way: verification is progress within a phase, not a change of one. + if w.requestPlanVerification(ctx, id, s) { + return + } // The agent asks for a phase change here, at a boundary it has reached // (F21). Orchestra decides, and an accepted change ends this session. if w.requestPhase(ctx, id, s) { @@ -2048,3 +2055,134 @@ func (w *worker) paneProgress(ctx context.Context, adapter herdr.CLIAdapter, s h } return adapter.PaneCapture(ctx, s, "recent") } + +// planProgressFile is the implementer's bounded verification request. It +// carries one phase and one status, and the only status it may carry is a +// request: an agent that could write "verified" would be marking its own work +// done, which is the whole thing this machinery exists to prevent. +const planProgressFile = "plan-progress.json" + +type planProgressRequest struct { + Phase string `json:"phase"` + Status string `json:"status"` +} + +// requestPlanVerification runs one phase's automated checks and posts the +// results. It reports whether anything was done, so the caller does not treat +// an absent request as a failure. +// +// The commands come from the accepted plan, which the coordinator resolves and +// authorises. This worker never reads a command out of the request: the +// request names a phase, and nothing else about it is trusted. +func (w *worker) requestPlanVerification(ctx context.Context, id string, s herdr.Session) bool { + path := filepath.Join(s.Worktree, ".orchestra", planProgressFile) + b, err := os.ReadFile(path) + if err != nil { + return false + } + var req planProgressRequest + if err := json.Unmarshal(b, &req); err != nil { + w.answerRefusedProgress(ctx, id, s, path, fmt.Sprintf(".orchestra/%s is not valid JSON: %v", planProgressFile, err)) + return false + } + if req.Status != domain.PlanPhaseRequestStatus { + // Naming this refusal precisely matters: an agent that wrote + // "verified" is trying to establish a fact, and it has to learn that + // establishing facts is not its job rather than retry the same file. + w.answerRefusedProgress(ctx, id, s, path, fmt.Sprintf("status %q is not writable by you. The only status you may request is %q; Orchestra decides the rest by running the plan's own commands.", req.Status, domain.PlanPhaseRequestStatus)) + return false + } + l := w.leases[id] + commands, err := w.api.PlanPhaseCommands(ctx, id, req.Phase, l.Epoch) + if err != nil { + // A refusal is an answer and reaches the agent. A transport failure is + // not, and the request survives for the next boundary. + var status *federation.StatusError + if errors.As(err, &status) && status.Code == http.StatusConflict { + w.answerRefusedProgress(ctx, id, s, path, status.Body) + return false + } + w.recordError(fmt.Errorf("plan verification %s: %w", id, err)) + return false + } + head, err := git(ctx, s.Worktree, "rev-parse", "HEAD") + if err != nil { + w.recordError(fmt.Errorf("plan verification %s: head: %s: %w", id, head, err)) + return false + } + runs := make([]federation.VerificationRun, 0, len(commands)) + for _, argv := range commands { + // No shell. The quality gate's envelope is exec in the worktree, and a + // plan command is agent-authored, so it gets that envelope and never a + // weaker one. A pipe here is a literal argument. + cmd := exec.CommandContext(ctx, argv[0], argv[1:]...) + cmd.Dir = s.Worktree + out, runErr := cmd.CombinedOutput() + code := 0 + if runErr != nil { + code = 1 + var exit *exec.ExitError + if errors.As(runErr, &exit) { + code = exit.ExitCode() + } + } + runs = append(runs, federation.VerificationRun{Command: argv, ExitCode: code, Output: tail(string(out), review.MaxGateOutputBytes)}) + } + status, err := w.api.RecordPlanPhase(ctx, id, req.Phase, strings.TrimSpace(string(head)), l.Epoch, runs) + if err != nil { + w.recordError(fmt.Errorf("plan verification %s: %w", id, err)) + return false + } + // Durable before the request is removed. Removing first would lose the + // request if the response were lost, and the agent would wait on an answer + // that already happened. + if err := os.Remove(path); err != nil { + w.recordError(fmt.Errorf("plan verification %s: %w", id, err)) + } + w.tellProgressOutcome(ctx, id, s, req.Phase, status, runs) + return true +} + +// tellProgressOutcome delivers what Orchestra established. A phase that did +// not verify is the case that matters: without this the agent sees a request +// disappear and has to guess whether it worked. +func (w *worker) tellProgressOutcome(ctx context.Context, id string, s herdr.Session, phase, status string, runs []federation.VerificationRun) { + var b strings.Builder + switch status { + case string(domain.PlanPhaseVerified): + fmt.Fprintf(&b, "Orchestra verified %s. Every automated check passed. Move to the next phase of the plan.", phase) + case string(domain.PlanPhaseAwaitingManual): + fmt.Fprintf(&b, "Orchestra ran %s's automated checks and they passed. The phase is waiting for the human to confirm its manual steps, which you cannot do. Continue with the next phase; the sign-off arrives on its own.", phase) + default: + fmt.Fprintf(&b, "Orchestra ran %s's checks and it is not verified. Fix what failed, then write .orchestra/%s again.\n", phase, planProgressFile) + for _, r := range runs { + if r.ExitCode == 0 { + continue + } + fmt.Fprintf(&b, "\n%s exited %d:\n%s\n", strings.Join(r.Command, " "), r.ExitCode, r.Output) + } + } + if err := w.sendPrompt(ctx, s, b.String()); err != nil { + w.recordError(fmt.Errorf("deliver plan verification %s: %w", id, err)) + } + log.Printf("plan phase %s of %s: %s", phase, id, status) +} + +// answerRefusedProgress tells the implementer why its request was refused and +// drops the file so a corrected one can be written. Recording a refusal only +// in worker health leaves a live session rewriting the same rejected file at +// every boundary, which is the silent-loop shape this codebase keeps producing +// (F39, F42, and the completion tail). +func (w *worker) answerRefusedProgress(ctx context.Context, id string, s herdr.Session, path, reason string) { + w.recordError(fmt.Errorf("plan verification %s refused: %s", id, reason)) + text := "Orchestra refused your verification request: " + reason + + "\n\nWrite a corrected .orchestra/" + planProgressFile + ", or keep working. Do not repeat the refused request." + if err := w.sendPrompt(ctx, s, text); err != nil { + w.recordError(fmt.Errorf("deliver plan verification refusal %s: %w", id, err)) + return + } + if err := os.Remove(path); err != nil { + w.recordError(fmt.Errorf("plan verification %s: %w", id, err)) + } + log.Printf("plan verification %s refused: %s", id, reason) +} diff --git a/cmd/orchestra-worker/phase_test.go b/cmd/orchestra-worker/phase_test.go index a756e24..1cf3a5d 100644 --- a/cmd/orchestra-worker/phase_test.go +++ b/cmd/orchestra-worker/phase_test.go @@ -359,3 +359,101 @@ func TestClaudeHarnessReachesTheTurnBoundary(t *testing.T) { } _ = backend } + +// An agent may request verification. It may never assert one: writing +// "verified" is claiming its own work is done, which is what the whole +// machinery exists to prevent. The refusal has to reach the pane, or the +// session rewrites the same rejected file at every boundary. +func TestPlanVerificationRefusesAnyStatusButARequest(t *testing.T) { + for _, status := range []string{"verified", "awaiting_manual_verification", "failed", "skipped", ""} { + w, backend, wt, done := phaseWorker(t, func(rw http.ResponseWriter, r *http.Request) { + t.Errorf("a refused request reached the coordinator at %s", r.URL.Path) + rw.WriteHeader(http.StatusNotFound) + }) + path := filepath.Join(wt, ".orchestra", planProgressFile) + if err := os.WriteFile(path, []byte(`{"phase":"phase-1","status":"`+status+`"}`), 0o644); err != nil { + t.Fatal(err) + } + if w.requestPlanVerification(context.Background(), "task", w.sessions["task"]) { + t.Fatalf("status %q was accepted", status) + } + if len(backend.prompts) == 0 { + t.Fatalf("status %q was refused with nothing delivered to the pane", status) + } + if !strings.Contains(backend.prompts[0], "ready_for_verification") { + t.Fatalf("the refusal does not name the only writable status: %s", backend.prompts[0]) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("status %q left the refused request in place", status) + } + done() + } +} + +// An absent request is not a failure and must not reach the pane. +func TestNoPlanVerificationRequestIsSilent(t *testing.T) { + w, backend, _, done := phaseWorker(t, func(rw http.ResponseWriter, r *http.Request) { + t.Errorf("an absent request reached the coordinator at %s", r.URL.Path) + }) + defer done() + if w.requestPlanVerification(context.Background(), "task", w.sessions["task"]) { + t.Fatal("an absent request reported work done") + } + if len(backend.prompts) != 0 { + t.Fatalf("an absent request spoke to the pane: %v", backend.prompts) + } +} + +// The commands come from the accepted plan, resolved and authorised by the +// coordinator. The worker must never take one out of the agent's request. +func TestPlanVerificationRunsThePlansCommandsAndReportsExitCodes(t *testing.T) { + var reported map[string]any + w, backend, wt, done := phaseWorker(t, func(rw http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/plan-phase"): + json.NewEncoder(rw).Encode(map[string]any{"commands": [][]string{{"true"}, {"false"}}}) + case strings.HasSuffix(r.URL.Path, "/plan-phase-result"): + json.NewDecoder(r.Body).Decode(&reported) + json.NewEncoder(rw).Encode(map[string]any{"status": "in_progress"}) + default: + t.Errorf("unexpected %s", r.URL.Path) + rw.WriteHeader(http.StatusNotFound) + } + }) + defer done() + // A git worktree, so the verification can anchor to a real commit. + for _, args := range [][]string{{"init"}, {"config", "user.email", "t@example.com"}, {"config", "user.name", "t"}, {"commit", "--allow-empty", "-m", "base"}} { + if out, err := git(context.Background(), wt, args...); err != nil { + t.Fatalf("git %v: %s: %v", args, out, err) + } + } + path := filepath.Join(wt, ".orchestra", planProgressFile) + if err := os.WriteFile(path, []byte(`{"phase":"phase-1","status":"ready_for_verification","commands":[["rm","-rf","/"]]}`), 0o644); err != nil { + t.Fatal(err) + } + if !w.requestPlanVerification(context.Background(), "task", w.sessions["task"]) { + t.Fatal("a valid request did nothing") + } + runs, _ := reported["runs"].([]any) + if len(runs) != 2 { + t.Fatalf("reported %d runs, want the plan's 2: %v", len(runs), reported) + } + first, _ := runs[0].(map[string]any) + second, _ := runs[1].(map[string]any) + if first["exit_code"].(float64) != 0 || second["exit_code"].(float64) == 0 { + t.Fatalf("exit codes were not reported faithfully: %v", runs) + } + // The command list in the request is ignored entirely. + cmd, _ := first["command"].([]any) + if len(cmd) != 1 || cmd[0].(string) != "true" { + t.Fatalf("the worker ran something other than the plan's command: %v", cmd) + } + if sha, _ := reported["at_sha"].(string); len(sha) != 40 { + t.Fatalf("the verification was not anchored to a commit: %q", sha) + } + // A phase that did not verify has to say why, or the agent sees its + // request vanish and guesses. + if len(backend.prompts) == 0 || !strings.Contains(backend.prompts[0], "not verified") { + t.Fatalf("the outcome was not delivered: %v", backend.prompts) + } +} diff --git a/cmd/orchestra/main.go b/cmd/orchestra/main.go index 38e5dc1..e8e086a 100644 --- a/cmd/orchestra/main.go +++ b/cmd/orchestra/main.go @@ -1339,7 +1339,7 @@ func main() { w.WriteHeader(http.StatusNoContent) }) mux.HandleFunc("/v1/federation/workers/", func(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodPost || (!strings.HasSuffix(r.URL.Path, "/heartbeat") && !strings.HasSuffix(r.URL.Path, "/renew") && !strings.HasSuffix(r.URL.Path, "/start") && !strings.HasSuffix(r.URL.Path, "/nack") && !strings.HasSuffix(r.URL.Path, "/handoff") && !strings.HasSuffix(r.URL.Path, "/pickup") && !strings.HasSuffix(r.URL.Path, "/complete") && !strings.HasSuffix(r.URL.Path, "/submit") && !strings.HasSuffix(r.URL.Path, "/captures")) { + if r.Method != http.MethodPost || (!strings.HasSuffix(r.URL.Path, "/heartbeat") && !strings.HasSuffix(r.URL.Path, "/renew") && !strings.HasSuffix(r.URL.Path, "/start") && !strings.HasSuffix(r.URL.Path, "/nack") && !strings.HasSuffix(r.URL.Path, "/handoff") && !strings.HasSuffix(r.URL.Path, "/pickup") && !strings.HasSuffix(r.URL.Path, "/complete") && !strings.HasSuffix(r.URL.Path, "/submit") && !strings.HasSuffix(r.URL.Path, "/plan-phase") && !strings.HasSuffix(r.URL.Path, "/plan-phase-result") && !strings.HasSuffix(r.URL.Path, "/captures")) { http.Error(w, "not found", 404) return } @@ -1389,23 +1389,26 @@ func main() { return } var b struct { - TaskID string `json:"task_id"` - TTLSeconds int `json:"ttl_seconds"` - ExpectedVersion int `json:"expected_version"` - HandoffRef string `json:"handoff_ref"` - AnchorSHA string `json:"anchor_sha"` - TransactionID string `json:"transaction_id"` - LeaseVersion int `json:"lease_version"` - LeaseEpoch string `json:"lease_epoch"` - ResultSHA string `json:"result_sha"` - Branch string `json:"branch"` - Remote string `json:"remote"` - Receipt map[string]any `json:"receipt"` - FailureClass string `json:"failure_class"` - LastError string `json:"last_error"` - SessionEvidence domain.SessionEvidence `json:"session_evidence"` - Review review.Result `json:"review"` - Gate domain.GateResult `json:"gate"` + TaskID string `json:"task_id"` + TTLSeconds int `json:"ttl_seconds"` + ExpectedVersion int `json:"expected_version"` + HandoffRef string `json:"handoff_ref"` + AnchorSHA string `json:"anchor_sha"` + TransactionID string `json:"transaction_id"` + LeaseVersion int `json:"lease_version"` + LeaseEpoch string `json:"lease_epoch"` + ResultSHA string `json:"result_sha"` + Branch string `json:"branch"` + Remote string `json:"remote"` + Receipt map[string]any `json:"receipt"` + FailureClass string `json:"failure_class"` + LastError string `json:"last_error"` + SessionEvidence domain.SessionEvidence `json:"session_evidence"` + Review review.Result `json:"review"` + Gate domain.GateResult `json:"gate"` + PhaseID string `json:"phase_id"` + AtSHA string `json:"at_sha"` + Runs []operations.VerificationRun `json:"runs"` } if json.NewDecoder(r.Body).Decode(&b) != nil || b.TaskID == "" { http.Error(w, "invalid lease body", 400) @@ -1432,6 +1435,34 @@ func main() { http.Error(w, "lease version conflict", http.StatusConflict) return } + if strings.HasSuffix(r.URL.Path, "/plan-phase") || strings.HasSuffix(r.URL.Path, "/plan-phase-result") { + project, ok := rr.Project(t.Project) + if !ok { + http.Error(w, "unknown project "+t.Project, 409) + return + } + if strings.HasSuffix(r.URL.Path, "/plan-phase") { + // Resolve and authorise before anything runs. A command the + // project does not permit is refused here, so a partial + // execution can never leave side effects behind. + phase, err := operations.PlanPhaseCommands(s, project, b.TaskID, b.PhaseID) + if err != nil { + http.Error(w, err.Error(), http.StatusConflict) + return + } + json.NewEncoder(w).Encode(map[string]any{"commands": phase.Automated}) + return + } + e, err := operations.RecordPlanPhaseVerification(s, project, b.TaskID, b.PhaseID, b.AtSHA, b.Runs) + if err != nil { + http.Error(w, err.Error(), http.StatusConflict) + return + } + var rec domain.PlanPhaseRecord + _ = json.Unmarshal(e.Payload, &rec) + json.NewEncoder(w).Encode(map[string]any{"status": string(rec.Status), "event": e}) + return + } if strings.HasSuffix(r.URL.Path, "/submit") { // Seal the review, then submit the commit it examined. Both events // are Orchestra's; the worker supplies verified evidence and the diff --git a/deploy/config.example.jsonc b/deploy/config.example.jsonc index 9ae63e0..01aa85f 100644 --- a/deploy/config.example.jsonc +++ b/deploy/config.example.jsonc @@ -17,6 +17,23 @@ // Overrides global ORCHESTRA_WORKTREE_ROOT. "quality_gate": "go test ./... && go vet ./..." // Worker runs this before deterministic delivery. , + // What a plan's automated verification may execute. A plan command is + // agent-authored, so it does not inherit the operator-authored quality + // gate's envelope: it runs as argv with no shell, and only if some + // pattern here matches it positionally. An absent policy refuses every + // plan command, and the planner is told so when it seals. + // "*" matches exactly one element, any value + // "x/..." matches a path argument under that prefix + // a trailing "*" matches the remaining elements, and is the only way + // a pattern authorises a longer command + "verification": { + "allowed": [ + ["go", "test", "./..."], + ["go", "test", "./internal/..."], + ["go", "vet", "./..."], + ["go", "build", "./..."] + ] + }, // Cognitive phase path. Omit for the default // frame -> research -> plan -> implement -> review. A phase left out is // skipped, which is how a trivial project runs frame/implement/review. diff --git a/internal/agentctx/agentctx.go b/internal/agentctx/agentctx.go index 9e3da94..5efe333 100644 --- a/internal/agentctx/agentctx.go +++ b/internal/agentctx/agentctx.go @@ -305,6 +305,12 @@ func renderTask(in Input) string { fmt.Fprintf(&b, "- uncommitted changes: %t\n", in.Git.Dirty) b.WriteString(renderSealed(in)) + // Below the plan, above continuity. Progress is a fact about the plan, so + // it follows the plan; continuity is one predecessor's account, so it + // ranks under both. + if in.Phase == domain.WorkPhaseImplement { + b.WriteString(renderPlanProgress(in)) + } b.WriteString(renderFindings(in)) b.WriteString(renderEvidence(in)) @@ -410,6 +416,49 @@ func fallback(s string) string { // plan -> accepted research // implement -> accepted research and accepted plan // review -> accepted plan +// renderPlanProgress states what Orchestra established about the accepted +// 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 +// the tree has moved, so "verified" never reads as a claim about code that has +// since changed. +func renderPlanProgress(in Input) string { + if in.Plan == nil || len(in.Plan.Phases) == 0 { + return "" + } + records := in.Task.PlanPhases() + byPhase := map[string]domain.PlanPhaseRecord{} + for _, r := range records { + byPhase[r.PhaseID] = r + } + var b strings.Builder + b.WriteString("\n## Plan progress\n\nOrchestra established this by running the plan's own verification. You cannot write it.\n\n") + current := "" + for _, phase := range in.Plan.Phases { + rec, ok := byPhase[phase.ID] + switch { + case !ok: + fmt.Fprintf(&b, "- %s (%s): not started\n", phase.ID, collapse(phase.Name)) + case rec.Status == domain.PlanPhaseVerified && rec.Stale(in.Git.HeadSHA): + fmt.Fprintf(&b, "- %s (%s): verified at %s, stale because the tree is now at %s\n", phase.ID, collapse(phase.Name), short(rec.AtSHA), short(in.Git.HeadSHA)) + case rec.Status == domain.PlanPhaseVerified: + fmt.Fprintf(&b, "- %s (%s): verified at %s\n", phase.ID, collapse(phase.Name), short(rec.AtSHA)) + case rec.Status == domain.PlanPhaseAwaitingManual: + fmt.Fprintf(&b, "- %s (%s): automated checks passed at %s, waiting for the human to confirm the manual steps\n", phase.ID, collapse(phase.Name), short(rec.AtSHA)) + default: + fmt.Fprintf(&b, "- %s (%s): in progress, last verification exited %v\n", phase.ID, collapse(phase.Name), rec.ExitCodes) + } + if current == "" && (!ok || rec.Status != domain.PlanPhaseVerified) { + current = phase.ID + } + } + if current == "" { + b.WriteString("\nEvery phase is verified.\n") + return b.String() + } + fmt.Fprintf(&b, "\nYour current phase is %s. When you believe it is done, write .orchestra/plan-progress.json:\n\n {\"phase\": %q, \"status\": \"ready_for_verification\"}\n\nThat is a request, not a result. Orchestra runs that phase's own automated commands and records what they exit. No other status is writable: you cannot mark a phase verified, and claiming one would be refused.\n", current, current) + return b.String() +} + func renderSealed(in Input) string { var b strings.Builder research := in.Research @@ -446,6 +495,12 @@ func renderSealed(in Input) string { } } } + if plan != nil && len(plan.Phases) == 0 { + // A plan sealed before plan.md names no executable unit, so phase + // progress cannot apply to it. Saying so beats a silently absent + // progress section, which reads as "no phase is done yet". + b.WriteString("\nThis is a legacy accepted plan, sealed before plan.md. Phase progress is unavailable for it: work from the plan text and finish the phase the usual way.\n") + } if plan != nil { // Verbatim, never collapsed. The plan is the execution map an // implement session works from, and a rotated successor has to receive diff --git a/internal/domain/domain.go b/internal/domain/domain.go index a43cfff..2bbe7f7 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -203,7 +203,11 @@ type Task struct { // finished. The next phase reads these, never the session that wrote them. ResearchRef string `json:"research_ref,omitempty"` PlanRef string `json:"plan_ref,omitempty"` - LastError string `json:"last_error,omitempty"` + // PlanProgress is what Orchestra established about the accepted plan's + // phases. Read it through PlanPhases, which discards records belonging to + // a superseded plan. + PlanProgress *PlanProgress `json:"plan_progress,omitempty"` + LastError string `json:"last_error,omitempty"` } // ReviewRef binds a sealed review artifact to one commit. @@ -262,7 +266,7 @@ func ValidateEvent(e Event) error { if e.SchemaVersion >= 2 && strings.TrimSpace(e.Surface) == "" { return fmt.Errorf("%w: surface required", ErrInvalid) } - allowed := map[string]bool{"TaskCreated": true, "TaskLeased": true, "TaskLeaseRenewed": true, "TaskReleased": true, "TaskLaunchAcknowledged": true, "TaskPickupValidated": true, "TaskCompleted": true, "TaskFailed": true, "TaskBlocked": true, "TaskNeedsAttention": true, "ApprovalRequested": true, "ApprovalGranted": true, "ApprovalDenied": true, "TaskAmended": true, "TaskCorrected": true, "QuotaReported": true, "StandupAdvisory": true, EventHumanDecisionRecorded: true, EventHumanDecisionSuperseded: true, EventWorkPhaseChanged: true, EventDeferredFindingRecorded: true, EventReviewRecorded: true, EventTaskSubmitted: true, EventTaskChangesRequested: true} + allowed := map[string]bool{"TaskCreated": true, "TaskLeased": true, "TaskLeaseRenewed": true, "TaskReleased": true, "TaskLaunchAcknowledged": true, "TaskPickupValidated": true, "TaskCompleted": true, "TaskFailed": true, "TaskBlocked": true, "TaskNeedsAttention": true, "ApprovalRequested": true, "ApprovalGranted": true, "ApprovalDenied": true, "TaskAmended": true, "TaskCorrected": true, "QuotaReported": true, "StandupAdvisory": true, EventHumanDecisionRecorded: true, EventHumanDecisionSuperseded: true, EventWorkPhaseChanged: true, EventDeferredFindingRecorded: true, EventReviewRecorded: true, EventTaskSubmitted: true, EventTaskChangesRequested: true, EventPlanPhaseVerified: true} if !allowed[e.Type] { return fmt.Errorf("%w: unknown type %q", ErrInvalid, e.Type) } @@ -540,6 +544,8 @@ func ValidatePayload(typ string, p map[string]any) error { return fmt.Errorf("%w: decision_ids entries must be ids", ErrInvalid) } } + case EventPlanPhaseVerified: + return ValidatePlanPhaseVerified(p) case EventReviewRecorded: if err := requiredHash(p, "artifact_ref"); err != nil { return err diff --git a/internal/domain/planprogress.go b/internal/domain/planprogress.go new file mode 100644 index 0000000..ba5de79 --- /dev/null +++ b/internal/domain/planprogress.go @@ -0,0 +1,135 @@ +package domain + +import ( + "fmt" + "strings" + "time" +) + +// EventPlanPhaseVerified records that Orchestra ran a plan phase's automated +// verification and what happened. The implementer never emits it: an agent may +// request verification, and only the plane can establish it. +const EventPlanPhaseVerified = "PlanPhaseVerified" + +// PlanPhaseStatus is what Orchestra established about one phase. +type PlanPhaseStatus string + +const ( + // PlanPhaseInProgress is the default and the outcome of a failed run. It + // is never written by a request, only left in place by one. + PlanPhaseInProgress PlanPhaseStatus = "in_progress" + // PlanPhaseAwaitingManual means every automated check passed and manual + // steps remain. A human signs those off; the agent cannot. + PlanPhaseAwaitingManual PlanPhaseStatus = "awaiting_manual_verification" + // PlanPhaseVerified means nothing further is required for this phase. + PlanPhaseVerified PlanPhaseStatus = "verified" +) + +func (s PlanPhaseStatus) Valid() bool { + switch s { + case PlanPhaseInProgress, PlanPhaseAwaitingManual, PlanPhaseVerified: + return true + } + return false +} + +// PlanPhaseRequestStatus is the single value an implementer may write. Every +// other status is a conclusion Orchestra reaches, so allowing an agent to +// assert one would let it declare its own work verified. +const PlanPhaseRequestStatus = "ready_for_verification" + +// PlanPhaseRecord is one verification run, bound to the plan it belongs to and +// the commit it ran against. +// +// Both bindings are load-bearing. Without PlanRef, a phase verified under plan +// A survives into plan B, which is the same class of bug as a review that +// outlives the commit it examined. Without AtSHA, "verified" outlives the code +// that made it true. +type PlanPhaseRecord struct { + PlanRef string `json:"plan_ref"` + PhaseID string `json:"phase_id"` + Status PlanPhaseStatus `json:"status"` + Commands [][]string `json:"commands,omitempty"` + ExitCodes []int `json:"exit_codes,omitempty"` + AtSHA string `json:"at_sha"` + // EvidenceRef is the CAS ref of the captured command output. + EvidenceRef string `json:"evidence_ref,omitempty"` + At time.Time `json:"at"` +} + +// Stale reports whether the tree has moved since this phase was verified. A +// stale record is retained and labelled rather than discarded: it is still +// true that the phase passed at that commit, and hiding it would lose the +// provenance. What it must never do is read as current. +func (r PlanPhaseRecord) Stale(headSHA string) bool { + return headSHA != "" && r.AtSHA != "" && r.AtSHA != headSHA +} + +// PlanProgress is the durable verification state for one accepted plan. +type PlanProgress struct { + // PlanRef is the plan these records belong to. A record from a superseded + // plan is never counted, so a replan cannot inherit progress it did not + // earn. + PlanRef string `json:"plan_ref"` + Phases []PlanPhaseRecord `json:"phases,omitempty"` +} + +// PlanPhases returns the records that belong to the currently accepted plan. +// A task whose plan was superseded reports none, whatever the log still holds. +func (t Task) PlanPhases() []PlanPhaseRecord { + if t.PlanProgress == nil || t.PlanRef == "" || t.PlanProgress.PlanRef != t.PlanRef { + return nil + } + return t.PlanProgress.Phases +} + +// PlanPhase returns the record for one phase of the accepted plan. +func (t Task) PlanPhase(id string) (PlanPhaseRecord, bool) { + for _, r := range t.PlanPhases() { + if r.PhaseID == id { + return r, true + } + } + return PlanPhaseRecord{}, false +} + +// PlanPhaseSubject is the decision subject a manual sign-off carries. The key +// binds the approval to one phase of one plan, so a later "looks good" on an +// unrelated thread cannot satisfy a gate nobody was talking about. +func PlanPhaseSubject(planRef, phaseID string) string { + return "plan_phase_verification:" + planRef + ":" + phaseID +} + +const maxVerificationCommands = 16 + +func ValidatePlanPhaseVerified(p map[string]any) error { + planRef, _ := p["plan_ref"].(string) + if strings.TrimSpace(planRef) == "" { + return fmt.Errorf("%w: plan_ref required", ErrInvalid) + } + phaseID, _ := p["phase_id"].(string) + if strings.TrimSpace(phaseID) == "" { + return fmt.Errorf("%w: phase_id required", ErrInvalid) + } + status, _ := p["status"].(string) + if !PlanPhaseStatus(status).Valid() { + return fmt.Errorf("%w: status %q is not a plan phase status", ErrInvalid, status) + } + if sha, _ := p["at_sha"].(string); len(sha) != 40 { + return fmt.Errorf("%w: at_sha must be a full commit sha", ErrInvalid) + } + codes, _ := p["exit_codes"].([]any) + if len(codes) > maxVerificationCommands { + return fmt.Errorf("%w: %d exit codes exceeds the %d command bound", ErrInvalid, len(codes), maxVerificationCommands) + } + // A verified phase whose commands failed would be a contradiction the + // reducer could not detect later. + if PlanPhaseStatus(status) != PlanPhaseInProgress { + for _, c := range codes { + if code, ok := c.(float64); !ok || code != 0 { + return fmt.Errorf("%w: status %q cannot carry a non-zero exit code", ErrInvalid, status) + } + } + } + return nil +} diff --git a/internal/domain/planprogress_test.go b/internal/domain/planprogress_test.go new file mode 100644 index 0000000..b478eb7 --- /dev/null +++ b/internal/domain/planprogress_test.go @@ -0,0 +1,78 @@ +package domain + +import "testing" + +// Progress earned under plan A must not survive into plan B. Without this, a +// replan inherits verification it did not earn, which is the same shape as a +// review outliving the commit it examined. +// +// The reducer also clears the records on a re-seal. This guards the read side, +// so a record that reaches a reader by any other route is still not counted. +func TestPlanPhasesIgnoresRecordsFromASupersededPlan(t *testing.T) { + task := Task{ + PlanRef: "plan-b", + PlanProgress: &PlanProgress{PlanRef: "plan-a", Phases: []PlanPhaseRecord{ + {PhaseID: "phase-1", PlanRef: "plan-a", Status: PlanPhaseVerified, AtSHA: "abc"}, + }}, + } + if got := task.PlanPhases(); len(got) != 0 { + t.Fatalf("progress from plan-a counted under plan-b: %+v", got) + } + if _, ok := task.PlanPhase("phase-1"); ok { + t.Fatal("a superseded phase was addressable") + } + task.PlanProgress.PlanRef = "plan-b" + if got := task.PlanPhases(); len(got) != 1 { + t.Fatalf("progress for the accepted plan was discarded: %+v", got) + } +} + +// A task with no accepted plan counts nothing, whatever the projection holds. +func TestPlanPhasesRequiresAnAcceptedPlan(t *testing.T) { + task := Task{PlanProgress: &PlanProgress{PlanRef: "plan-a", Phases: []PlanPhaseRecord{{PhaseID: "phase-1"}}}} + if got := task.PlanPhases(); len(got) != 0 { + t.Fatalf("progress counted with no PlanRef: %+v", got) + } +} + +func TestPlanPhaseSubjectBindsPlanAndPhase(t *testing.T) { + if PlanPhaseSubject("ref-a", "phase-1") == PlanPhaseSubject("ref-b", "phase-1") { + t.Fatal("two plans share a manual sign-off subject") + } + if PlanPhaseSubject("ref-a", "phase-1") == PlanPhaseSubject("ref-a", "phase-2") { + t.Fatal("two phases share a manual sign-off subject") + } +} + +// An agent may request verification. Every other status is a conclusion +// Orchestra reaches, so a payload claiming one with a failing command is a +// contradiction the reducer could not detect later. +func TestVerifiedStatusCannotCarryAFailingCommand(t *testing.T) { + sha := "1111111111111111111111111111111111111111" + base := func() map[string]any { + return map[string]any{"plan_ref": "r", "phase_id": "phase-1", "at_sha": sha} + } + ok := base() + ok["status"] = string(PlanPhaseVerified) + ok["exit_codes"] = []any{float64(0)} + if err := ValidatePlanPhaseVerified(ok); err != nil { + t.Fatalf("a passing verification was refused: %v", err) + } + bad := base() + bad["status"] = string(PlanPhaseVerified) + bad["exit_codes"] = []any{float64(1)} + if err := ValidatePlanPhaseVerified(bad); err == nil { + t.Fatal("verified with a non-zero exit code was accepted") + } + agent := base() + agent["status"] = PlanPhaseRequestStatus + if err := ValidatePlanPhaseVerified(agent); err == nil { + t.Fatal("ready_for_verification was accepted as a durable status") + } + noSHA := base() + noSHA["status"] = string(PlanPhaseVerified) + noSHA["at_sha"] = "abc" + if err := ValidatePlanPhaseVerified(noSHA); err == nil { + t.Fatal("a verification with no anchored commit was accepted") + } +} diff --git a/internal/federation/client.go b/internal/federation/client.go index b2b8caa..83bd0e5 100644 --- a/internal/federation/client.go +++ b/internal/federation/client.go @@ -326,6 +326,52 @@ func (c Client) Submit(ctx context.Context, taskID, epoch string, expectedVersio return out.Status, nil } +// VerificationRun is one plan command the worker executed. +type VerificationRun struct { + Command []string `json:"command"` + ExitCode int `json:"exit_code"` + Output string `json:"output,omitempty"` +} + +// PlanPhaseCommands asks the coordinator which commands a phase's verification +// runs. The worker never takes a command from the agent's request: the plan +// settled that at seal time, and the project's policy authorises it here. +func (c Client) PlanPhaseCommands(ctx context.Context, taskID, phaseID, epoch string) ([][]string, error) { + resp, err := c.request(ctx, http.MethodPost, "/v1/federation/workers/"+url.PathEscape(c.WorkerID)+"/plan-phase", map[string]any{ + "task_id": taskID, "phase_id": phaseID, "lease_epoch": epoch, + }) + if err != nil { + return nil, err + } + defer resp.Body.Close() + var out struct { + Commands [][]string `json:"commands"` + } + if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { + return nil, err + } + return out.Commands, nil +} + +// RecordPlanPhase reports what the commands exited and returns the status +// Orchestra established. The worker supplies evidence; it does not conclude. +func (c Client) RecordPlanPhase(ctx context.Context, taskID, phaseID, atSHA, epoch string, runs []VerificationRun) (string, error) { + resp, err := c.request(ctx, http.MethodPost, "/v1/federation/workers/"+url.PathEscape(c.WorkerID)+"/plan-phase-result", map[string]any{ + "task_id": taskID, "phase_id": phaseID, "at_sha": atSHA, "lease_epoch": epoch, "runs": runs, + }) + if err != nil { + return "", err + } + defer resp.Body.Close() + var out struct { + Status string `json:"status"` + } + if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { + return "", err + } + return out.Status, nil +} + func (c Client) PublishCapture(ctx context.Context, capture Capture) (Capture, error) { resp, err := c.request(ctx, http.MethodPost, "/v1/federation/workers/"+url.PathEscape(c.WorkerID)+"/captures", capture) if err != nil { diff --git a/internal/operations/planprogress.go b/internal/operations/planprogress.go new file mode 100644 index 0000000..4d7a54e --- /dev/null +++ b/internal/operations/planprogress.go @@ -0,0 +1,171 @@ +package operations + +import ( + "encoding/json" + "fmt" + "strings" + "time" + + "orchestra/internal/authz" + "orchestra/internal/domain" + "orchestra/internal/registry" + "orchestra/internal/store" + "orchestra/internal/workphase" +) + +// ErrPlanPhase reports that a verification request cannot proceed. The reason +// is always specific, because it is delivered to a live implementer that has +// to act on it. +var ErrPlanPhase = fmt.Errorf("plan phase verification refused") + +// VerificationRun is the outcome of one command the worker executed. +type VerificationRun struct { + Command []string `json:"command"` + ExitCode int `json:"exit_code"` + Output string `json:"output,omitempty"` +} + +// PlanPhaseCommands resolves the commands a phase's verification will run. +// +// The commands come from the accepted plan, never from the request. An +// implementer asks to verify a phase; what that phase is checked with was +// settled when the plan was sealed, so a request cannot smuggle in a command +// the planner did not write. +// +// Every command is checked against the project's policy before any of them +// runs. A partial execution followed by a refusal would leave side effects +// behind with nothing recording them. +func PlanPhaseCommands(s *store.Store, project registry.Project, taskID, phaseID string) (workphase.PlanPhase, error) { + t, ok := s.Task(taskID) + if !ok { + return workphase.PlanPhase{}, domain.ErrNotFound + } + if t.PlanRef == "" { + return workphase.PlanPhase{}, fmt.Errorf("%w: this task has no accepted plan", ErrPlanPhase) + } + raw, err := s.Artifact(t.PlanRef) + if err != nil { + return workphase.PlanPhase{}, fmt.Errorf("read accepted plan: %w", err) + } + doc, err := workphase.DecodeStoredPlan(raw) + if err != nil { + return workphase.PlanPhase{}, fmt.Errorf("read accepted plan: %w", err) + } + if len(doc.Phases) == 0 { + // A plan sealed before plan.md names no executable unit. Saying so is + // the honest answer; inventing a phase would make progress against a + // plan that never had any. + return workphase.PlanPhase{}, fmt.Errorf("%w: the accepted plan predates plan.md and declares no phases, so phase progress does not apply to it", ErrPlanPhase) + } + phase, ok := doc.Phase(phaseID) + if !ok { + return workphase.PlanPhase{}, fmt.Errorf("%w: the accepted plan has no %s; it has %s", ErrPlanPhase, phaseID, phaseNames(doc)) + } + for _, argv := range phase.Automated { + if allowed, why := project.Verification.Allows(argv); !allowed { + // Refused before anything ran. The planner wrote a command the + // project does not permit, and the implementer is told rather + // than left retrying a phase that can never verify. + return workphase.PlanPhase{}, fmt.Errorf("%w: %s declares a command %s cannot run: %s", ErrPlanPhase, phaseID, project.ID, why) + } + } + return phase, nil +} + +func phaseNames(doc workphase.PlanDoc) string { + out := make([]string, 0, len(doc.Phases)) + for _, p := range doc.Phases { + out = append(out, p.ID) + } + return strings.Join(out, ", ") +} + +// RecordPlanPhaseVerification establishes what the runs prove, and appends the +// durable record. +// +// The status is derived here and never taken from the caller. An implementer +// may request verification; only this function decides whether a phase is +// verified, awaiting a human, or still in progress. +func RecordPlanPhaseVerification(s *store.Store, project registry.Project, taskID, phaseID, atSHA string, runs []VerificationRun) (domain.Event, error) { + t, ok := s.Task(taskID) + if !ok { + return domain.Event{}, domain.ErrNotFound + } + if len(atSHA) != 40 { + return domain.Event{}, fmt.Errorf("%w: verification must name the commit it ran against", ErrPlanPhase) + } + phase, err := PlanPhaseCommands(s, project, taskID, phaseID) + if err != nil { + return domain.Event{}, err + } + if len(runs) != len(phase.Automated) { + return domain.Event{}, fmt.Errorf("%w: %s declares %d automated commands but %d results were reported", ErrPlanPhase, phaseID, len(phase.Automated), len(runs)) + } + record := domain.PlanPhaseRecord{ + PlanRef: t.PlanRef, PhaseID: phaseID, AtSHA: atSHA, + Status: domain.PlanPhaseVerified, At: time.Now().UTC(), + } + for i, r := range runs { + // The result must describe the command the plan named. A reordered or + // substituted result would attribute one command's exit code to + // another. + if strings.Join(r.Command, "\x00") != strings.Join(phase.Automated[i], "\x00") { + return domain.Event{}, fmt.Errorf("%w: result %d reports %q but the plan declares %q", ErrPlanPhase, i, strings.Join(r.Command, " "), strings.Join(phase.Automated[i], " ")) + } + record.Commands = append(record.Commands, r.Command) + record.ExitCodes = append(record.ExitCodes, r.ExitCode) + if r.ExitCode != 0 { + record.Status = domain.PlanPhaseInProgress + } + } + if record.Status == domain.PlanPhaseVerified && len(phase.Manual) > 0 { + // Automated checks passing is not the whole phase. A human owns the + // manual steps, and the phase waits rather than claiming more than + // was established. + record.Status = domain.PlanPhaseAwaitingManual + } + if record.Status != domain.PlanPhaseInProgress && manuallySignedOff(s, t, phaseID) { + record.Status = domain.PlanPhaseVerified + } + if ref, err := s.PutArtifact(verificationEvidence(runs)); err == nil { + record.EvidenceRef = ref + } else { + return domain.Event{}, fmt.Errorf("store verification evidence: %w", err) + } + payload := map[string]any{ + "plan_ref": record.PlanRef, "phase_id": record.PhaseID, "status": string(record.Status), + "commands": record.Commands, "exit_codes": record.ExitCodes, "at_sha": record.AtSHA, + "evidence_ref": record.EvidenceRef, "at": record.At, + } + if t.Lease != nil { + payload["harness_id"], payload["lease_epoch"] = t.Lease.HarnessID, t.Lease.Epoch + } + b, err := json.Marshal(payload) + if err != nil { + return domain.Event{}, err + } + e := domain.Event{ID: domain.NewID(), Type: domain.EventPlanPhaseVerified, TaskID: taskID, Version: t.Version + 1, Payload: b, Surface: string(authz.System)} + return e, s.Append(e) +} + +// manuallySignedOff reports whether a human has already approved this exact +// phase of this exact plan. The subject carries both, so a later "looks good" +// on an unrelated thread cannot satisfy a gate nobody was discussing. +func manuallySignedOff(s *store.Store, t domain.Task, phaseID string) bool { + intent, err := s.EffectiveIntent(t.ID) + if err != nil { + return false + } + subject := domain.PlanPhaseSubject(t.PlanRef, phaseID) + for _, d := range intent.Decisions { + if d.Subject == subject { + return true + } + } + return false +} + +func verificationEvidence(runs []VerificationRun) []byte { + b, _ := json.Marshal(runs) + return b +} diff --git a/internal/operations/planprogress_test.go b/internal/operations/planprogress_test.go new file mode 100644 index 0000000..7346836 --- /dev/null +++ b/internal/operations/planprogress_test.go @@ -0,0 +1,267 @@ +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) + } +} diff --git a/internal/registry/registry.go b/internal/registry/registry.go index cd88032..a3c4726 100644 --- a/internal/registry/registry.go +++ b/internal/registry/registry.go @@ -50,6 +50,9 @@ type Project struct { HumanDecisions struct { MaxRequestsPerTask int `json:"max_requests_per_task,omitempty"` } `json:"human_decisions,omitempty"` + // Verification bounds what a plan's automated checks may execute. Absent + // policy refuses every plan command; see VerificationPolicy. + Verification VerificationPolicy `json:"verification,omitempty"` } // MaxDecisionRequests is the per-task question budget. diff --git a/internal/registry/verification.go b/internal/registry/verification.go new file mode 100644 index 0000000..321c454 --- /dev/null +++ b/internal/registry/verification.go @@ -0,0 +1,84 @@ +package registry + +import ( + "fmt" + "strings" +) + +// VerificationPolicy is what a plan's automated checks may execute in this +// project. It exists because a plan command is agent-authored, while the +// quality gate is operator-authored: the two must not share an execution +// envelope just because they run in the same worktree. +// +// Absence is a refusal, never a default-allow. A project that declares no +// policy runs no plan command, and the planner is told so at seal time. +type VerificationPolicy struct { + // Allowed is a list of argv patterns. A command runs only if some pattern + // matches it positionally. + Allowed [][]string `json:"allowed,omitempty"` +} + +// Allows reports whether argv matches any pattern, and names the reason when +// it does not. The reason reaches the planner, so "not allowed" alone would +// send it guessing at the project's real reach. +func (p VerificationPolicy) Allows(argv []string) (bool, string) { + if len(argv) == 0 { + return false, "the command is empty" + } + if len(p.Allowed) == 0 { + return false, "this project declares no verification policy, so no plan command may run" + } + for _, pattern := range p.Allowed { + if matchArgv(pattern, argv) { + return true, "" + } + } + return false, fmt.Sprintf("%q is not in this project's verification policy, which allows: %s", strings.Join(argv, " "), p.describe()) +} + +func (p VerificationPolicy) describe() string { + out := make([]string, 0, len(p.Allowed)) + for _, pattern := range p.Allowed { + out = append(out, strings.Join(pattern, " ")) + } + return strings.Join(out, "; ") +} + +// matchArgv matches positionally, never by prefix. +// +// - a literal element matches that element exactly +// - "*" matches exactly one element, any value +// - an element ending in "/..." matches a path argument under that prefix, +// so ./internal/... covers ./internal/store/... but never ./internalsecrets +// - "*" as the last pattern element matches every remaining element, and is +// the only way a pattern covers a longer command. An operator writes that +// deliberately for a runner that takes free-form arguments. +// +// A shorter pattern otherwise fails, so ["go", "test"] never authorises +// ["go", "test", "-exec", "curl"]. +func matchArgv(pattern, argv []string) bool { + if len(pattern) == 0 { + return false + } + for i, want := range pattern { + if want == "*" && i == len(pattern)-1 && len(argv) >= len(pattern) { + // Trailing wildcard: everything from here on is covered. + return true + } + if i >= len(argv) { + return false + } + got := argv[i] + switch { + case want == "*": + // One element, any value. An empty argument is still an argument. + case strings.HasSuffix(want, "/..."): + if !strings.HasPrefix(got, strings.TrimSuffix(want, "...")) && got != strings.TrimSuffix(want, "/...") { + return false + } + case want != got: + return false + } + } + return len(pattern) == len(argv) +} diff --git a/internal/registry/verification_test.go b/internal/registry/verification_test.go new file mode 100644 index 0000000..c1bcc3c --- /dev/null +++ b/internal/registry/verification_test.go @@ -0,0 +1,85 @@ +package registry + +import "testing" + +func TestVerificationPolicyMatchesPositionally(t *testing.T) { + p := VerificationPolicy{Allowed: [][]string{ + {"go", "test", "./..."}, + {"go", "test", "./internal/..."}, + {"go", "vet", "./..."}, + {"npm", "test", "--", "*"}, + {"make", "*"}, + }} + allowed := [][]string{ + {"go", "test", "./..."}, + {"go", "test", "./internal/store/..."}, + {"go", "vet", "./..."}, + {"npm", "test", "--", "unit"}, + // A trailing "*" covers the remaining elements, which is the only way + // a pattern authorises a longer command. The operator writes that + // deliberately when a runner takes free-form arguments. + {"npm", "test", "--", "unit", "extra"}, + {"make", "check"}, + {"make", "check", "verbose"}, + // Narrower than an allowed pattern: ./other/... is a subset of ./... + {"go", "test", "./other/..."}, + } + for _, argv := range allowed { + if ok, why := p.Allows(argv); !ok { + t.Errorf("%v refused: %s", argv, why) + } + } + refused := [][]string{ + // A shorter pattern must not authorise a longer command, or + // ["go","test"] would cover ["go","test","-exec","curl"]. + {"go", "test", "./...", "-exec", "curl"}, + {"go", "test"}, + {"go", "build", "./..."}, + {"curl", "https://example.com"}, + {}, + } + for _, argv := range refused { + if ok, _ := p.Allows(argv); ok { + t.Errorf("%v allowed, want a refusal", argv) + } + } +} + +// Absence is a refusal. A project that declares no policy must not inherit the +// quality gate's operator-authored envelope, because a plan command is written +// by an agent. +func TestAbsentVerificationPolicyRefusesEverything(t *testing.T) { + var p VerificationPolicy + ok, why := p.Allows([]string{"go", "test", "./..."}) + if ok { + t.Fatal("an empty policy allowed a command") + } + if why == "" { + t.Fatal("a refusal with no reason sends the planner guessing") + } +} + +// The path prefix is a prefix of the path, not of the argument text. Tested +// against a policy that does not also allow ./..., which would cover +// everything and hide the distinction. +func TestPathPrefixDoesNotMatchASiblingWithTheSameLetters(t *testing.T) { + p := VerificationPolicy{Allowed: [][]string{{"go", "test", "./internal/..."}}} + for _, argv := range [][]string{ + {"go", "test", "./internalsecrets"}, + {"go", "test", "./internal-tools/..."}, + {"go", "test", "./..."}, + } { + if ok, _ := p.Allows(argv); ok { + t.Errorf("%v allowed, want a refusal", argv) + } + } + for _, argv := range [][]string{ + {"go", "test", "./internal/..."}, + {"go", "test", "./internal/store/..."}, + {"go", "test", "./internal"}, + } { + if ok, why := p.Allows(argv); !ok { + t.Errorf("%v refused: %s", argv, why) + } + } +} diff --git a/internal/store/store.go b/internal/store/store.go index 79b3d56..2d2e5ff 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -253,6 +253,7 @@ func (s *Store) apply(e domain.Event) error { // folded into the contract projection. var p struct { DecisionID string `json:"decision_id"` + Subject string `json:"subject"` Source domain.HumanDecisionSource `json:"source"` } if err := json.Unmarshal(e.Payload, &p); err != nil { @@ -261,6 +262,21 @@ func (s *Store) apply(e domain.Event) error { if p.Source.ExternalID != "" { s.decisionSource[p.Source.Provider+"\x00"+p.Source.ExternalID] = p.DecisionID } + // A manual sign-off is the one decision that moves plan progress. The + // subject carries the plan ref and the phase id, so an approval + // applies to exactly the gate it named and to no other. + if t.PlanProgress != nil { + for i, rec := range t.PlanProgress.Phases { + if rec.Status != domain.PlanPhaseAwaitingManual { + continue + } + if p.Subject == domain.PlanPhaseSubject(rec.PlanRef, rec.PhaseID) { + t.PlanProgress.Phases[i].Status = domain.PlanPhaseVerified + t.Version = e.Version + s.replaceTask(e.TaskID, t) + } + } + } } if e.Type == "StandupAdvisory" || e.Type == "ApprovalGranted" || e.Type == "ApprovalDenied" { return nil @@ -282,8 +298,13 @@ func (s *Store) apply(e domain.Event) error { t.ResearchRef = p.ArtifactRef } case domain.WorkPhasePlan: - if p.ArtifactRef != "" { + if p.ArtifactRef != "" && p.ArtifactRef != t.PlanRef { + // A new plan supersedes the old one, and the progress earned + // against the old one with it. PlanPhases would filter these + // out anyway; clearing here means a superseded record is not + // carried around waiting for a reader that forgets to. t.PlanRef = p.ArtifactRef + t.PlanProgress = nil } } t.WorkPhase = p.Phase @@ -449,6 +470,31 @@ func (s *Store) apply(e domain.Event) error { t.Version = e.Version s.replaceTask(e.TaskID, t) return nil + case domain.EventPlanPhaseVerified: + var pp domain.PlanPhaseRecord + if err := json.Unmarshal(e.Payload, &pp); err != nil { + return err + } + // Records are kept per plan. A record naming a plan the task no longer + // accepts is projected onto nothing: it stays in the log as + // provenance, and PlanPhases refuses to count it. + if t.PlanProgress == nil || t.PlanProgress.PlanRef != pp.PlanRef { + t.PlanProgress = &domain.PlanProgress{PlanRef: pp.PlanRef} + } + replaced := false + for i, existing := range t.PlanProgress.Phases { + if existing.PhaseID == pp.PhaseID { + t.PlanProgress.Phases[i] = pp + replaced = true + break + } + } + if !replaced { + t.PlanProgress.Phases = append(t.PlanProgress.Phases, pp) + } + t.Version = e.Version + s.replaceTask(e.TaskID, t) + return nil case domain.EventReviewRecorded: var rp struct { ArtifactRef string `json:"artifact_ref"` @@ -827,7 +873,7 @@ func (s *Store) validateTransition(e domain.Event, t domain.Task, exists bool, p return nil } switch e.Type { - case "TaskLeaseRenewed", "TaskLaunchAcknowledged", "TaskReleased", "TaskPickupValidated", "TaskCompleted", "TaskBlocked", "TaskNeedsAttention", "TaskFailed", domain.EventTaskSubmitted: + case "TaskLeaseRenewed", "TaskLaunchAcknowledged", "TaskReleased", "TaskPickupValidated", "TaskCompleted", "TaskBlocked", "TaskNeedsAttention", "TaskFailed", domain.EventTaskSubmitted, domain.EventPlanPhaseVerified: owner, _ := p["harness_id"].(string) epoch, _ := p["lease_epoch"].(string) // Expiry is the one coordinator-owned relinquish path. It still binds