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
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user