Seal the plan as a specification instead of four bullet lists

The plan artifact was Changes{Target,Intent} plus three string lists, every entry capped at 500 single-line characters. That bound makes a specification impossible: a phase cannot carry a code block, a paragraph of reasoning, or a verification command with its own argument list. renderSealed then flattened what little survived through collapse(), so an implement session received a summary of a summary. plan.md replaces it. Markdown, 128 KiB, no per-line cap, sealed through the existing path under the existing PlanRef. The parser enforces the structure the brief states: required sections, phases numbered from 1 with no gaps, Files, Changes and Verification per phase, and at least one automated or manual check, because a phase nobody can verify can never be established as done. Automated entries are JSON argv arrays, so a pipe is a literal argument rather than an operator. Headings inside fenced blocks are content, so a plan may show markdown without parsing its own example. Citations resolve at seal time against the accepted research, on the coordinator, which is the only party holding ResearchRef. A plan resting on a finding nobody recorded fails on the planner while its session is still alive to be told. The plan now renders byte for byte into the implement launch, and a rotated successor receives the same complete document. That is the property the whole change exists for. collapse() stays for research findings, which really are short claims. DecodeStoredPlan reads pre-markdown refs and renders them into the same type, labelled, so nothing downstream branches on which era a plan came from. A legacy plan carries no phases, which is honest: the old artifact never named an executable unit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CVbaKucEYBjMqVeUgJUsc1
2026-08-28 11:36:45 +04:00
parent 822f086451
commit 57c028f94f
22 changed files with 1070 additions and 145 deletions
@@ -0,0 +1,318 @@
+package workphase
+
+import (
+	"encoding/json"
+	"fmt"
+	"regexp"
+	"strconv"
+	"strings"
+)
+
+// PlanDoc is a sealed markdown specification. The document is retained
+// verbatim, because that is what an implement session receives: a plan
+// flattened to bullet lines is a summary, and a summary cannot be executed.
+//
+// The parsed fields exist so Orchestra can act on the plan without rereading
+// prose. Nothing outside this file interprets the markdown.
+type PlanDoc struct {
+	// Markdown is the exact bytes that were sealed.
+	Markdown string `json:"markdown"`
+	// Title is the level-one heading.
+	Title string `json:"title"`
+	// Phases are ordered and contiguous from 1.
+	Phases []PlanPhase `json:"phases"`
+	// References are the research ids the plan cites, deduplicated.
+	References []string `json:"references,omitempty"`
+}
+
+// PlanPhase is one executable unit of the plan. Files and Changes are prose
+// for the implementer; the verification entries are what Orchestra runs and
+// what the human signs off.
+type PlanPhase struct {
+	// ID is the stable name used in a progress request, "phase-1".
+	ID     string `json:"id"`
+	Number int    `json:"number"`
+	Name   string `json:"name"`
+	// Body is the phase's markdown, retained so a mismatch report can quote it.
+	Body string `json:"body"`
+	// Automated are argv arrays. Not shell strings: they execute through
+	// exec.Command with no shell, so a pipe or a redirection is a literal
+	// argument rather than an operator.
+	Automated [][]string `json:"automated,omitempty"`
+	// Manual are human-testable steps. A phase whose automated checks all pass
+	// while manual steps remain is awaiting a human, not verified.
+	Manual []string `json:"manual,omitempty"`
+}
+
+const (
+	// MaxPlanBytes bounds the whole document. A specification needs room that
+	// the 500-character single-line rule never allowed, but an unbounded
+	// artifact is a transcript that every later session pays to read.
+	MaxPlanBytes = 128 << 10
+	maxPhases    = 32
+	maxCommands  = 16
+	maxArgv      = 32
+)
+
+// requiredSections are the level-two headings every plan carries. They are the
+// questions a plan has to answer before anyone implements against it: what is
+// true now, what should be true after, and what is deliberately not being done.
+var requiredSections = []string{
+	"Overview", "Current state", "Desired end state", "Non-goals", "Approach",
+	"Testing strategy", "Risks and edge cases", "Migration", "References",
+}
+
+var (
+	phaseHeading = regexp.MustCompile(`^##\s+Phase\s+(\d+)\s*:\s*(.+?)\s*$`)
+	runLine      = regexp.MustCompile(`^-\s+run:\s*(.+?)\s*$`)
+	bulletLine   = regexp.MustCompile(`^[-*]\s+(.+?)\s*$`)
+	researchCite = regexp.MustCompile(`research:([a-z0-9][a-z0-9_-]{0,63})`)
+)
+
+// ParsePlan reads a markdown plan and reports the first structural problem it
+// finds. Errors name the section or phase, because "invalid plan" sends a
+// planner rereading its own document with nothing to go on.
+func ParsePlan(b []byte) (PlanDoc, error) {
+	if len(b) == 0 {
+		return PlanDoc{}, fmt.Errorf("plan: the document is empty")
+	}
+	if len(b) > MaxPlanBytes {
+		return PlanDoc{}, fmt.Errorf("plan: %d bytes exceeds the %d byte bound", len(b), MaxPlanBytes)
+	}
+	doc := PlanDoc{Markdown: string(b)}
+	lines := strings.Split(strings.ReplaceAll(doc.Markdown, "\r\n", "\n"), "\n")
+
+	sections := map[string]bool{}
+	// current tracks which phase's body the parser is inside, and which
+	// verification subsection, so a "- run:" line is attributed to the phase
+	// that actually contains it.
+	var current *PlanPhase
+	subsection := ""
+	inFence := false
+
+	flush := func() {
+		if current != nil {
+			current.Body = strings.TrimSpace(current.Body)
+			doc.Phases = append(doc.Phases, *current)
+			current = nil
+		}
+	}
+
+	for n, raw := range lines {
+		line := strings.TrimRight(raw, " \t")
+		// A heading inside a fenced block is content, not structure. Without
+		// this a plan that shows example markdown parses its own example.
+		if strings.HasPrefix(strings.TrimSpace(line), "```") {
+			inFence = !inFence
+		}
+		if !inFence {
+			switch {
+			case strings.HasPrefix(line, "# "):
+				if doc.Title == "" {
+					doc.Title = strings.TrimSpace(strings.TrimPrefix(line, "# "))
+				}
+				continue
+			case phaseHeading.MatchString(line):
+				flush()
+				m := phaseHeading.FindStringSubmatch(line)
+				number, _ := strconv.Atoi(m[1])
+				current = &PlanPhase{ID: "phase-" + m[1], Number: number, Name: m[2]}
+				subsection = ""
+				continue
+			case strings.HasPrefix(line, "## "):
+				flush()
+				sections[strings.TrimSpace(strings.TrimPrefix(line, "## "))] = true
+				subsection = strings.TrimSpace(strings.TrimPrefix(line, "## "))
+				continue
+			case strings.HasPrefix(line, "#### "):
+				subsection = strings.TrimSpace(strings.TrimPrefix(line, "#### "))
+			case strings.HasPrefix(line, "### "):
+				subsection = strings.TrimSpace(strings.TrimPrefix(line, "### "))
+			}
+		}
+		if current == nil {
+			continue
+		}
+		current.Body += line + "\n"
+		if inFence {
+			continue
+		}
+		switch subsection {
+		case "Automated":
+			m := runLine.FindStringSubmatch(line)
+			if m == nil {
+				continue
+			}
+			var argv []string
+			if err := json.Unmarshal([]byte(m[1]), &argv); err != nil {
+				return PlanDoc{}, fmt.Errorf("plan: %s line %d: run: must be a JSON argv array like [\"go\", \"test\", \"./...\"], got %s", current.ID, n+1, m[1])
+			}
+			if err := validArgv(current.ID, argv); err != nil {
+				return PlanDoc{}, err
+			}
+			current.Automated = append(current.Automated, argv)
+		case "Manual":
+			if m := bulletLine.FindStringSubmatch(line); m != nil {
+				current.Manual = append(current.Manual, m[1])
+			}
+		}
+	}
+	flush()
+
+	if doc.Title == "" {
+		return PlanDoc{}, fmt.Errorf("plan: a level-one heading naming the plan is required")
+	}
+	for _, name := range requiredSections {
+		if !sections[name] {
+			return PlanDoc{}, fmt.Errorf("plan: the %q section is required", name)
+		}
+	}
+	if err := doc.validatePhases(); err != nil {
+		return PlanDoc{}, err
+	}
+	doc.References = citations(doc.Markdown)
+	return doc, nil
+}
+
+func (d PlanDoc) validatePhases() error {
+	if len(d.Phases) == 0 {
+		return fmt.Errorf("plan: at least one \"## Phase 1: <name>\" section is required")
+	}
+	if len(d.Phases) > maxPhases {
+		return fmt.Errorf("plan: %d phases exceeds the %d phase bound", len(d.Phases), maxPhases)
+	}
+	for i, p := range d.Phases {
+		// Contiguous numbering from 1. A plan that jumps from phase 2 to
+		// phase 4 has either lost a phase or renumbered one that progress
+		// already refers to, and both are worse discovered later.
+		if p.Number != i+1 {
+			return fmt.Errorf("plan: phases must be numbered from 1 with no gaps; found Phase %d where Phase %d was expected", p.Number, i+1)
+		}
+		if strings.TrimSpace(p.Name) == "" {
+			return fmt.Errorf("plan: %s has no name", p.ID)
+		}
+		for _, required := range []string{"### Files", "### Changes", "### Verification"} {
+			if !strings.Contains(p.Body, required) {
+				return fmt.Errorf("plan: %s is missing its %q subsection", p.ID, strings.TrimPrefix(required, "### "))
+			}
+		}
+		// A phase nobody can check is a phase that can never leave
+		// in_progress, so it is refused at seal time rather than becoming a
+		// stuck task later.
+		if len(p.Automated) == 0 && len(p.Manual) == 0 {
+			return fmt.Errorf("plan: %s has no automated and no manual verification, so nothing could ever establish it is done", p.ID)
+		}
+		if len(p.Automated) > maxCommands {
+			return fmt.Errorf("plan: %s declares %d automated commands, at most %d", p.ID, len(p.Automated), maxCommands)
+		}
+	}
+	return nil
+}
+
+func validArgv(phase string, argv []string) error {
+	if len(argv) == 0 {
+		return fmt.Errorf("plan: %s declares an empty run: command", phase)
+	}
+	if len(argv) > maxArgv {
+		return fmt.Errorf("plan: %s declares a run: command of %d arguments, at most %d", phase, len(argv), maxArgv)
+	}
+	for i, a := range argv {
+		if strings.TrimSpace(a) == "" {
+			return fmt.Errorf("plan: %s run: argument %d is empty", phase, i)
+		}
+		if strings.ContainsAny(a, "\n\r\x00") {
+			return fmt.Errorf("plan: %s run: argument %d contains a newline or a null byte", phase, i)
+		}
+	}
+	return nil
+}
+
+// Phase finds a phase by its id.
+func (d PlanDoc) Phase(id string) (PlanPhase, bool) {
+	for _, p := range d.Phases {
+		if p.ID == id {
+			return p, true
+		}
+	}
+	return PlanPhase{}, false
+}
+
+// citations returns every research id the document cites, deduplicated, in
+// order of first appearance.
+func citations(markdown string) []string {
+	var out []string
+	seen := map[string]bool{}
+	for _, m := range researchCite.FindAllStringSubmatch(markdown, -1) {
+		if !seen[m[1]] {
+			seen[m[1]] = true
+			out = append(out, m[1])
+		}
+	}
+	return out
+}
+
+// ResolveReferences reports the cited research ids that the accepted research
+// does not contain. A plan resting on a finding nobody recorded is the failure
+// this prevents, and it lands on the planner rather than the implementer.
+func (d PlanDoc) ResolveReferences(r Research) []string {
+	known := map[string]bool{}
+	for _, f := range r.Findings {
+		known[f.ID] = true
+	}
+	var missing []string
+	for _, id := range d.References {
+		if !known[id] {
+			missing = append(missing, id)
+		}
+	}
+	return missing
+}
+
+// DecodeStoredPlan reads a plan already in the CAS. New seals are markdown;
+// refs sealed before plan.md existed are JSON, and refusing them would block
+// every task whose plan predates this change, at rotation most of all.
+//
+// A legacy plan is rendered into the same type so nothing downstream branches
+// on which era a plan came from. It carries no phases, which is honest: the
+// old artifact never named an executable unit, so plan progress genuinely
+// cannot apply to it.
+func DecodeStoredPlan(b []byte) (PlanDoc, error) {
+	if doc, err := ParsePlan(b); err == nil {
+		return doc, nil
+	}
+	p, err := DecodePlan(b)
+	if err != nil {
+		// Report the markdown failure, not the JSON one: every new seal is
+		// markdown, so that is the error a reader needs.
+		_, mdErr := ParsePlan(b)
+		return PlanDoc{}, mdErr
+	}
+	return p.legacyDoc(), nil
+}
+
+// legacyDoc renders a pre-markdown plan as the document type. The text is
+// labelled so nobody mistakes a flattened plan for a specification.
+func (p Plan) legacyDoc() PlanDoc {
+	var b strings.Builder
+	b.WriteString("# Accepted plan\n\nSealed before plan.md existed, so it names intent rather than phases.\n\n## Changes\n\n")
+	for _, c := range p.Changes {
+		fmt.Fprintf(&b, "- %s: %s\n", c.Target, c.Intent)
+	}
+	for _, section := range []struct {
+		title string
+		items []string
+	}{
+		{"Verification", p.Verification},
+		{"Risks", p.Risks},
+		{"Human decisions needed", p.DecisionsNeeded},
+	} {
+		if len(section.items) == 0 {
+			continue
+		}
+		fmt.Fprintf(&b, "\n## %s\n\n", section.title)
+		for _, v := range section.items {
+			fmt.Fprintf(&b, "- %s\n", v)
+		}
+	}
+	return PlanDoc{Markdown: b.String(), Title: "Accepted plan"}
+}
@@ -0,0 +1,192 @@
+package workphase
+
+import (
+	"strings"
+	"testing"
+)
+
+const goodPlan = "# Quiet flag implementation plan\n" + `
+## Overview
+Add a --quiet flag.
+
+## Current state
+The script prints unconditionally. See research:r1.
+
+## Desired end state
+--quiet suppresses the success line.
+
+## Non-goals
+No change to exit codes.
+
+## Approach
+Parse the flag in the existing loop.
+
+## Phase 1: Parse the flag
+
+### Files
+- scripts/orchestra_e2e_healthcheck.sh
+
+### Changes
+Add a quiet local and set it in the argument loop.
+
+### Verification
+
+#### Automated
+- run: ["bash", "-n", "scripts/orchestra_e2e_healthcheck.sh"]
+
+#### Manual
+- Run the script with --quiet and confirm it prints nothing.
+
+## Phase 2: Document the flag
+
+### Files
+- scripts/orchestra_e2e_healthcheck.sh
+
+### Changes
+State --quiet in the --help output, per research:r2.
+
+### Verification
+
+#### Automated
+- run: ["bash", "scripts/orchestra_e2e_healthcheck.sh", "--help"]
+
+## Testing strategy
+Shell syntax check plus a manual run.
+
+## Risks and edge cases
+An unknown flag is still ignored.
+
+## Migration
+None.
+
+## References
+- research:r1
+- research:r2
+`
+
+func TestParsePlanAcceptsACompleteSpecification(t *testing.T) {
+	doc, err := ParsePlan([]byte(goodPlan))
+	if err != nil {
+		t.Fatalf("parse: %v", err)
+	}
+	if doc.Title != "Quiet flag implementation plan" {
+		t.Fatalf("title %q", doc.Title)
+	}
+	if len(doc.Phases) != 2 {
+		t.Fatalf("got %d phases, want 2", len(doc.Phases))
+	}
+	one, ok := doc.Phase("phase-1")
+	if !ok {
+		t.Fatal("phase-1 is not addressable")
+	}
+	if len(one.Automated) != 1 || one.Automated[0][0] != "bash" || len(one.Automated[0]) != 3 {
+		t.Fatalf("phase-1 automated: %v", one.Automated)
+	}
+	if len(one.Manual) != 1 {
+		t.Fatalf("phase-1 manual: %v", one.Manual)
+	}
+	// A command is attributed to the phase that contains it, never to the
+	// previous one.
+	two, _ := doc.Phase("phase-2")
+	if len(two.Automated) != 1 || two.Automated[0][2] != "--help" {
+		t.Fatalf("phase-2 automated: %v", two.Automated)
+	}
+	if len(two.Manual) != 0 {
+		t.Fatalf("phase-2 borrowed manual steps: %v", two.Manual)
+	}
+	if strings.Join(doc.References, ",") != "r1,r2" {
+		t.Fatalf("references %v", doc.References)
+	}
+	// The document survives byte for byte. An implement session receives this,
+	// not a reconstruction of it.
+	if doc.Markdown != goodPlan {
+		t.Fatal("the sealed document was not retained verbatim")
+	}
+}
+
+func TestParsePlanRefusesStructuralGaps(t *testing.T) {
+	cases := map[string]struct {
+		mutate func(string) string
+		names  string
+	}{
+		"no title":            {func(s string) string { return strings.Replace(s, "# Quiet flag implementation plan", "", 1) }, "level-one"},
+		"missing section":     {func(s string) string { return strings.Replace(s, "## Non-goals", "## Notgoals", 1) }, "Non-goals"},
+		"no phases":           {func(s string) string { return strings.ReplaceAll(s, "## Phase ", "## Step ") }, "Phase 1"},
+		"phase without files": {func(s string) string { return strings.Replace(s, "### Files\n- scripts", "### Filez\n- scripts", 1) }, "Files"},
+		"unparsable run": {func(s string) string {
+			return strings.Replace(s, `- run: ["bash", "-n", "scripts/orchestra_e2e_healthcheck.sh"]`, `- run: bash -n scripts/x.sh`, 1)
+		}, "argv array"},
+	}
+	for name, c := range cases {
+		_, err := ParsePlan([]byte(c.mutate(goodPlan)))
+		if err == nil {
+			t.Errorf("%s: accepted, want a refusal", name)
+			continue
+		}
+		if !strings.Contains(err.Error(), c.names) {
+			t.Errorf("%s: error %q does not name %q", name, err, c.names)
+		}
+	}
+}
+
+// A phase nobody can check can never leave in_progress. Refusing it at seal
+// time puts the failure on the planner, while the implementer would otherwise
+// discover it as a task that never finishes.
+func TestParsePlanRefusesAPhaseWithNoVerification(t *testing.T) {
+	stripped := strings.Replace(goodPlan,
+		"#### Automated\n- run: [\"bash\", \"scripts/orchestra_e2e_healthcheck.sh\", \"--help\"]\n", "", 1)
+	_, err := ParsePlan([]byte(stripped))
+	if err == nil || !strings.Contains(err.Error(), "phase-2") {
+		t.Fatalf("expected phase-2 to be refused, got %v", err)
+	}
+}
+
+func TestParsePlanRefusesNoncontiguousPhaseNumbers(t *testing.T) {
+	renumbered := strings.Replace(goodPlan, "## Phase 2:", "## Phase 4:", 1)
+	_, err := ParsePlan([]byte(renumbered))
+	if err == nil || !strings.Contains(err.Error(), "Phase 4") {
+		t.Fatalf("expected a gap to be refused, got %v", err)
+	}
+}
+
+// A plan that shows example markdown must not parse its own example.
+func TestParsePlanIgnoresHeadingsInsideFences(t *testing.T) {
+	withFence := strings.Replace(goodPlan, "## Testing strategy\n",
+		"## Testing strategy\n\n```\n## Phase 9: not a real phase\n#### Automated\n- run: [\"rm\", \"-rf\", \"/\"]\n```\n\n", 1)
+	doc, err := ParsePlan([]byte(withFence))
+	if err != nil {
+		t.Fatalf("parse: %v", err)
+	}
+	if len(doc.Phases) != 2 {
+		t.Fatalf("a fenced example became %d phases", len(doc.Phases))
+	}
+}
+
+func TestParsePlanBoundsTheDocument(t *testing.T) {
+	if _, err := ParsePlan(nil); err == nil {
+		t.Fatal("an empty document was accepted")
+	}
+	huge := goodPlan + strings.Repeat("x", MaxPlanBytes)
+	if _, err := ParsePlan([]byte(huge)); err == nil {
+		t.Fatal("an oversized document was accepted")
+	}
+}
+
+func TestResolveReferencesNamesUnknownFindings(t *testing.T) {
+	doc, err := ParsePlan([]byte(goodPlan))
+	if err != nil {
+		t.Fatal(err)
+	}
+	full := Research{Findings: []Finding{
+		{ID: "r1", Confidence: Fact, Claim: "c", Evidence: "e"},
+		{ID: "r2", Confidence: Inference, Claim: "d", Evidence: "f"},
+	}}
+	if missing := doc.ResolveReferences(full); len(missing) != 0 {
+		t.Fatalf("resolved research reported missing: %v", missing)
+	}
+	partial := Research{Findings: []Finding{{ID: "r1", Confidence: Fact, Claim: "c", Evidence: "e"}}}
+	missing := doc.ResolveReferences(partial)
+	if len(missing) != 1 || missing[0] != "r2" {
+		t.Fatalf("want r2 reported missing, got %v", missing)
+	}
+}