From 115b7056979f483fa6ffd7544d428d4cd06c01e6 Mon Sep 17 00:00:00 2001 From: Claude Code Date: Tue, 28 Jul 2026 03:57:27 +0000 Subject: [PATCH 1/3] feat(plan): cut plan-run token cost with a spec-author sub-agent and a leaner brief MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A real `csdd plan run` burned ~$200 in record time. The cost was the methodology itself: every feat paid, on the opus orchestrator, for full spec authoring, graph consultation, and gates before any code. This moves the bulk to cheaper models and trims what each feat re-reads, while keeping EARS, traceability, and phase gates — and adds the mechanical stack/ADR conformance the inlined brief was the only proxy for. - spec-author sub-agent (sonnet) authors each spec phase; opus only reviews, validates, and approves, re-dispatching spec-author with a fix-list - FeatBrief keeps only the feat and its specs: stack/ADR/wiki are refs only, fetched via `csdd graph explain`/`graph query`; bodies no longer inlined - default development_flow -> unit (tdd required for money/auth/tenancy); `spec generate --artifact tasks` is flow-aware (unit-shaped template) - mechanical stack/ADR conformance: `plan validate` requires design.md to cite each governing adr:; the code-reviewer checks the implementation against the decided stack - plan-dev reinforces test timing: Tier-2 tests the part during the task, Tier-3 tests the whole at feat exit; never the full suite inline - root CLAUDE.md trimmed and reframed MCP-first (the csdd_* tools are the default; typed params carry the artifact/phase contract) --- internal/cli/cli_extra_test.go | 31 + internal/cli/plan.go | 229 ++++- internal/cli/plan_adr_test.go | 16 +- internal/cli/spec.go | 21 +- internal/cli/spec_flow_test.go | 20 +- internal/plan/adr_test.go | 16 +- internal/plan/brief.go | 75 +- internal/plan/brief_test.go | 16 +- internal/plan/cost.go | 150 +++ internal/plan/cost_verify_test.go | 336 +++++++ internal/plan/debug_test.go | 862 ++++++++++++++++++ internal/plan/ledger.go | 4 + internal/plan/metrics.go | 75 +- internal/plan/runner.go | 19 +- internal/plan/tree.go | 166 +++- internal/plan/validate.go | 48 + internal/plan/validate_test.go | 49 + internal/plan/verify.go | 182 ++++ internal/templater/templater.go | 14 + internal/templater/templater_test.go | 61 +- .../templates/agents/code-reviewer.md.tmpl | 11 +- .../templates/agents/spec-author.md.tmpl | 121 +++ .../templates/commands/csdd-plan-run.md.tmpl | 2 +- .../templater/templates/plan/CLAUDE.md.tmpl | 86 ++ .../templater/templates/root/CLAUDE.md.tmpl | 299 +++--- .../templates/skills/plan-dev/SKILL.md.tmpl | 83 +- .../templates/spec/tasks-unit.md.tmpl | 53 ++ .../templater/templates/spec/tasks.md.tmpl | 2 +- internal/validator/validator.go | 6 +- internal/validator/validator_test.go | 5 +- 30 files changed, 2771 insertions(+), 287 deletions(-) create mode 100644 internal/plan/cost.go create mode 100644 internal/plan/cost_verify_test.go create mode 100644 internal/plan/debug_test.go create mode 100644 internal/plan/verify.go create mode 100644 internal/templater/templates/agents/spec-author.md.tmpl create mode 100644 internal/templater/templates/plan/CLAUDE.md.tmpl create mode 100644 internal/templater/templates/spec/tasks-unit.md.tmpl diff --git a/internal/cli/cli_extra_test.go b/internal/cli/cli_extra_test.go index 08cee85..91f49cf 100644 --- a/internal/cli/cli_extra_test.go +++ b/internal/cli/cli_extra_test.go @@ -111,6 +111,37 @@ func TestSpecGenerateBugfixAndResearch(t *testing.T) { } } +// TestSpecGenerateTasksShapeFollowsFlow guards the flow-aware tasks scaffold: +// under `unit` the generated tasks.md is implementation-then-cover (no RED/GREEN +// pair); under `tdd` it is the RED→GREEN pair. Generating the wrong shape would +// hand the author a tasks.md `csdd spec validate` immediately rejects. +func TestSpecGenerateTasksShapeFollowsFlow(t *testing.T) { + for _, tc := range []struct { + flow string + wantRedGreen bool + }{ + {"unit", false}, + {"tdd", true}, + } { + t.Run(tc.flow, func(t *testing.T) { + dir := freshWorkspace(t) + if code, _, _ := run(t, "spec", "init", "feat", "--flow", tc.flow, "--root", dir); code != 0 { + t.Fatalf("spec init --flow %s failed", tc.flow) + } + // tasks normally needs design approved; --force bypasses the gate so we + // exercise only the template-selection branch. + if code, _, _ := run(t, "spec", "generate", "feat", "--artifact", "tasks", "--force", "--root", dir); code != 0 { + t.Fatalf("spec generate tasks failed") + } + tasks := readFile(t, filepath.Join(dir, "specs", "feat", "tasks.md")) + hasRedGreen := strings.Contains(tasks, "RED —") || strings.Contains(tasks, "GREEN —") + if hasRedGreen != tc.wantRedGreen { + t.Errorf("flow %q: RED/GREEN present=%v, want %v", tc.flow, hasRedGreen, tc.wantRedGreen) + } + }) + } +} + // TestSpecGenerateAuxiliaryDoesNotClobberPhase guards the fix where research and // bugfix (auxiliary, ungated artifacts) must not reset the phase chain or the // approvals state advanced by the main requirements/design/tasks flow. diff --git a/internal/cli/plan.go b/internal/cli/plan.go index 8d92876..0defb4d 100644 --- a/internal/cli/plan.go +++ b/internal/cli/plan.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "time" "github.com/protonspy/csdd/internal/paths" @@ -45,14 +46,18 @@ func runPlan(args []string, templates embed.FS) int { case "generate": return planGenerate(rest, templates) case "run": - return planRun(rest) + return planRun(rest, templates) + case "cost": + return planCost(rest) + case "verify": + return planVerify(rest) default: render.Err("unknown plan action: " + action) return 1 } } -func planRun(args []string) int { +func planRun(args []string, templates embed.FS) int { fs := flag.NewFlagSet("plan run", flag.ContinueOnError) var root, model, effort string var autonomous, assumeYes, noTelegram bool @@ -63,13 +68,13 @@ func planRun(args []string) int { fs.BoolVar(&assumeYes, "yes", false, "Skip the unverified-sandbox prompt: accept running --dangerously-skip-permissions even when `sandbox doctor` fails.") fs.BoolVar(&noTelegram, "no-telegram", false, "Do not auto-start the Telegram notifier even when a bot is configured (.csdd/bot.json).") fs.BoolVar(&autonomous, "autonomous", false, "Deprecated no-op: plan run always runs bypass-mode (--dangerously-skip-permissions).") - fs.StringVar(&model, "model", "opus", "Model the orchestrating session runs on (claude --model): sonnet|opus|haiku|fable or a full model ID. It authors and decides the spec; task implementation is delegated to the `implementer` sub-agent (which runs on its own, cheaper model). Empty inherits the ambient default.") - fs.StringVar(&effort, "effort", "high", "Reasoning effort the orchestrating session runs at (claude --effort): low|medium|high|xhigh|max. Empty inherits the ambient default.") + fs.StringVar(&model, "model", "opus", "Model the orchestrating session runs on (claude --model): sonnet|opus|haiku|fable or a full model ID. It reviews and decides the spec; spec authoring is delegated to the `spec-author` sub-agent (sonnet) and task implementation to the `implementer` sub-agent (each on its own, cheaper model). Empty inherits the ambient default.") + fs.StringVar(&effort, "effort", "medium", "Reasoning effort the orchestrating session runs at (claude --effort): low|medium|high|xhigh|max. Empty inherits the ambient default.") fs.Float64Var(&sessionBudget, "session-budget", 0, "Per-session cap in USD (claude --max-budget-usd). Default 0 = no cap; the session runs under the Claude account's own limits.") - fs.IntVar(&maxIterations, "max-iterations", 100, "Sessions the run may spend; one iteration is one claude session.") + fs.IntVar(&maxIterations, "max-iterations", 30, "Sessions the run may spend; one iteration is one claude session.") fs.IntVar(&stall, "stall", 10, "Stop early after this many consecutive sessions without a step advancing.") fs.DurationVar(&sessionIdle, "session-idle", 0, "Kill a session that makes no progress — no event stream output and no CPU — for this long (default 15m). Not a time limit: real work of any duration keeps resetting it.") - fs.IntVar(&featAttempts, "feat-attempts", 0, "Stop handing out ONE feat after this many sessions and surface it as blocked (default 8). Bounds a feat whose `done` the verdict gate keeps refusing.") + fs.IntVar(&featAttempts, "feat-attempts", 0, "Stop handing out ONE feat after this many sessions and surface it as blocked (default 4). Bounds a feat whose `done` the verdict gate keeps refusing.") fs.IntVar(&squadLimit, "squad-limit", 0, "Maximum claude sessions running at once, each on its own feat in its own git worktree (1..6, default 1). Feats run together whenever the plan's Depends graph allows it; each delivered feat is merged into the run's base branch. Requires a clean git repository.") fs.IntVar(&maxRetries, "max-retries", 0, "Deprecated no-op: each iteration is one session, and the next iteration is the retry.") fs.IntVar(&maxRepairs, "max-repairs", 0, "Deprecated no-op: the self-correcting loop replaced repair sessions.") @@ -129,6 +134,7 @@ func planRun(args []string) int { FeatAttempts: featAttempts, SessionIdle: sessionIdle, SquadLimit: squadLimit, + WorktreeEntry: planEntryDoc(templates), Out: os.Stdout, }) if err != nil { @@ -613,3 +619,214 @@ func printPlanStatus(st plan.PlanStatus) { fmt.Printf(" %-3s %-*s %-13s %-10s %s\n", f.Num, maxName, f.Slug, f.State, f.Milestone, progress) } } + +// planCost reports what a run spent, read back out of sessions.jsonl. +// +// The file has recorded every attempt's cost since R9.2 and nothing read it back, +// so the only way to see a run's spend was to parse JSONL by hand. Exit code stays +// 0 whatever the numbers say — this reports, it does not judge. +func planCost(args []string) int { + fs := flag.NewFlagSet("plan cost", flag.ContinueOnError) + var root string + var jsonOut bool + addRoot(fs, &root) + addJSON(fs, &jsonOut) + positionals, err := parseFlags(fs, args) + if err != nil { + return failOnFlagParse(err) + } + if len(positionals) < 1 { + render.Err("usage: " + prog() + " plan cost SLUG [--json]") + return 1 + } + r, doc, code := resolvePlan(root, positionals[0]) + if code != 0 { + return code + } + rep := plan.BuildCostReport(r, doc.Slug) + if jsonOut { + return emitJSON(rep) + } + printCostReport(rep) + return 0 +} + +func printCostReport(rep plan.CostReport) { + fmt.Printf("plan: %s\n", rep.Plan) + if rep.Totals.Attempts == 0 { + render.Info("no sessions recorded yet — this plan has not been run") + return + } + fmt.Printf(" %-28s %8s %8s %6s %10s %12s\n", "feat", "attempts", "settled", "gated", "cost", "tokens") + for _, f := range rep.Feats { + fmt.Printf(" %-28s %8d %8d %6d %10s %12s%s\n", + f.Feat, f.Attempts, f.Settled, f.Gated, money(f.CostUSD), thousands(f.Tokens.Total()), deliveredMark(f.Delivered)) + } + fmt.Printf(" %-28s %8d %8d %6d %10s %12s\n", + "TOTAL", rep.Totals.Attempts, rep.Totals.Settled, rep.Totals.Gated, + money(rep.Totals.CostUSD), thousands(rep.Totals.Tokens.Total())) + + t := rep.Totals.Tokens + fmt.Printf("\n tokens: %s fresh input · %s output · %s cache read · %s cache write\n", + thousands(t.Input), thousands(t.Output), thousands(t.CacheRead), thousands(t.CacheCreation)) + + if len(rep.ByModel) > 0 { + fmt.Printf("\n by model\n") + for _, m := range rep.ByModel { + fmt.Printf(" %-34s %12s %10s\n", m.Model, thousands(m.Tokens.Total()), money(m.CostUSD)) + } + // The two figures come from different fields of the same event: the + // envelope's own `usage` block, and the CLI's per-model aggregation. A gap + // between them is work the top-level figure did not count, which on a plan + // run means the sub-agents. Reporting both is the honest move — asserting + // which one is right would be a guess. + if mt := rep.ModelTotal().Total(); mt != rep.Totals.Tokens.Total() { + fmt.Printf(" %-34s %12s\n", "(per-model total)", thousands(mt)) + render.Warn(fmt.Sprintf("the per-model breakdown and the session totals disagree by %s tokens; "+ + "the session total counts the orchestrating chain, the breakdown counts every model that billed", + thousands(abs(mt-rep.Totals.Tokens.Total())))) + } + } + + if n := len(rep.Unmeasured); n > 0 { + render.Warn(fmt.Sprintf("%d attempt(s) were opened and never settled: %s", n, strings.Join(rep.Unmeasured, ", "))) + fmt.Println(" Those sessions ran and spent money; the run was interrupted before their cost") + fmt.Println(" could be recorded, so every figure above understates the real total.") + } + if rep.Totals.Gated > 0 { + fmt.Printf("\n %d of %d settled session(s) were paid for and handed back (a refused `done`).\n", + rep.Totals.Gated, rep.Totals.Settled) + } +} + +// planVerify proves — or fails to prove — that each feat was delivered, by +// cross-checking the ledger, the on-disk artifacts, git, and the worktrees. +// Findings exit 2, the convention every lint/validate command in this CLI follows. +func planVerify(args []string) int { + fs := flag.NewFlagSet("plan verify", flag.ContinueOnError) + var root string + var jsonOut bool + addRoot(fs, &root) + addJSON(fs, &jsonOut) + positionals, err := parseFlags(fs, args) + if err != nil { + return failOnFlagParse(err) + } + if len(positionals) < 1 { + render.Err("usage: " + prog() + " plan verify SLUG [--json]") + return 1 + } + r, doc, code := resolvePlan(root, positionals[0]) + if code != 0 { + return code + } + rep := plan.Verify(r, doc) + if jsonOut { + if !rep.OK { + _ = emitJSON(rep) + return 2 + } + return emitJSON(rep) + } + printVerifyReport(rep) + if !rep.OK { + return 2 + } + return 0 +} + +func printVerifyReport(rep plan.VerifyReport) { + fmt.Printf("plan: %s\n", rep.Plan) + if rep.Git { + fmt.Printf("merges checked against: %s\n", rep.Branch) + } else { + render.Warn("not a git repository (or detached HEAD) — merge evidence cannot be checked") + } + fmt.Printf(" %-28s %-18s %-7s %-10s %-7s %s\n", "feat", "state", "ledger", "artifacts", "merged", "worktree") + for _, f := range rep.Feats { + fmt.Printf(" %-28s %-18s %-7s %-10s %-7s %s\n", + f.Feat, f.State, yesNo(f.LedgerDone), yesNo(f.Artifacts), mergedCell(rep.Git, f.Merged, f.MergeCommit), liveCell(f.LiveWorktree)) + } + var findings int + for _, f := range rep.Feats { + for _, msg := range f.Findings { + findings++ + render.Err(msg) + } + } + if findings == 0 { + render.OK("every feat's records agree") + return + } + fmt.Printf("\n%d finding(s): the records disagree about what was delivered.\n", findings) +} + +func deliveredMark(done bool) string { + if done { + return " ✓" + } + return "" +} + +func yesNo(b bool) string { + if b { + return "yes" + } + return "no" +} + +func mergedCell(git, merged bool, commit string) string { + if !git { + return "—" + } + if merged { + return commit + } + return "no" +} + +func liveCell(live bool) string { + if live { + return "live" + } + return "—" +} + +func money(usd float64) string { return fmt.Sprintf("$%.2f", usd) } + +func abs(n int) int { + if n < 0 { + return -n + } + return n +} + +// thousands groups an integer with thin separators so a seven-digit token count is +// readable at a glance, which is the only reason these numbers are printed at all. +func thousands(n int) string { + s := fmt.Sprintf("%d", n) + if len(s) <= 3 { + return s + } + var b strings.Builder + for i, r := range s { + if i > 0 && (len(s)-i)%3 == 0 { + b.WriteByte('.') + } + b.WriteRune(r) + } + return b.String() +} + +// planEntryDoc is the lean CLAUDE.md each feat's worktree gets for the duration of +// the run. A template that will not render is not worth failing a run over: the +// sessions fall back to the repository's own file, which is more expensive to read +// and still correct enough to work from. +func planEntryDoc(templates embed.FS) string { + doc, err := templater.PlanEntry(templates) + if err != nil { + render.Warn("could not load the plan-session CLAUDE.md; sessions will read the repository's own: " + err.Error()) + return "" + } + return doc +} diff --git a/internal/cli/plan_adr_test.go b/internal/cli/plan_adr_test.go index 7ae2799..931b11f 100644 --- a/internal/cli/plan_adr_test.go +++ b/internal/cli/plan_adr_test.go @@ -70,7 +70,8 @@ status: draft // TestADRValidateE2E drives the decision-ref lint through the real CLI: a plan // citing a present ADR validates clean; broken, ambiguous, and cites-superseded -// variants each fail with exit 2; and the brief inlines the cited record in full. +// variants each fail with exit 2; and the brief lists the cited record as a short +// ref (it no longer inlines the body — the gate enforces citation instead). func TestADRValidateE2E(t *testing.T) { dir := freshWorkspace(t) if code, _, e := run(t, "plan", "init", "photos", "--root", dir); code != 0 { @@ -85,13 +86,20 @@ func TestADRValidateE2E(t *testing.T) { t.Fatalf("plan citing a present ADR should validate clean: %s", e) } - // The brief inlines the cited ADR in full. + // The brief lists the governing ADR as a short ref and directs fetching the + // body via the graph; it must NOT inline the ADR title or body. code, out, _ := run(t, "plan", "brief", "photos", "--feat", "upload", "--root", dir) if code != 0 { t.Fatalf("plan brief failed: %d", code) } - if !strings.Contains(out, "Store under docs/adr") || !strings.Contains(out, "BODY_TOKEN") { - t.Errorf("brief should inline the cited ADR in full:\n%s", out) + if !strings.Contains(out, "adr:pick-store") { + t.Errorf("brief should list the governing ADR ref:\n%s", out) + } + if !strings.Contains(out, "csdd graph explain adr:") { + t.Errorf("brief should direct the session to fetch the ADR via graph explain:\n%s", out) + } + if strings.Contains(out, "BODY_TOKEN") { + t.Errorf("brief must NOT inline the ADR body (the gate enforces citation now):\n%s", out) } if !strings.Contains(out, "docs/stack.md Decided row") { t.Errorf("brief should carry the record-your-decisions mission line") diff --git a/internal/cli/spec.go b/internal/cli/spec.go index c68b922..f69538c 100644 --- a/internal/cli/spec.go +++ b/internal/cli/spec.go @@ -59,12 +59,16 @@ var knownSpecKeys = []string{ } // defaultDevelopmentFlow is the flow assumed when none is selected and steering -// declares no default. It keeps csdd's test-first posture as the default. -const defaultDevelopmentFlow = "tdd" +// declares no default. `unit` is the default: implementation first, with a unit +// test covering the behavior in the same task — the lightest posture that still +// keeps every behavior tested. `tdd` (RED→GREEN) is REQUIRED for money, auth, +// tenancy, and anything irreversible; set it explicitly (steering default or +// --flow) where those guarantees matter. +const defaultDevelopmentFlow = "unit" // developmentFlows is the closed set of selectable development flows: -// - unit: tests written after the code (no RED-first ritual) -// - tdd: test-first RED→GREEN (the default) +// - unit: implementation first, then a unit test covering it in the same task (the default) +// - tdd: test-first RED→GREEN — required for money/auth/tenancy/irreversible surfaces // - tdd-e2e: TDD plus end-to-end coverage of golden and error flows var developmentFlows = []string{"unit", "tdd", "tdd-e2e"} @@ -228,7 +232,7 @@ func specInit(args []string, templates embed.FS) int { var opts SpecInitOptions addRoot(fs, &opts.Root) fs.StringVar(&opts.Language, "language", "en", "Spec language (default: en).") - fs.StringVar(&opts.Flow, "flow", "", "Development flow: unit|tdd|tdd-e2e (default: steering default, else tdd).") + fs.StringVar(&opts.Flow, "flow", "", "Development flow: unit|tdd|tdd-e2e (default: steering default, else unit).") positionals, err := parseFlags(fs, args) if err != nil { return failOnFlagParse(err) @@ -481,6 +485,13 @@ func SpecGenerate(templates embed.FS, opts SpecGenerateOptions) error { "bugfix": {"bugfix.md", "templates/spec/bugfix.md.tmpl"}, } pair := templateMap[opts.Artifact] + // tasks.md is shaped by the spec's development_flow: the RED/GREEN pair under + // tdd/tdd-e2e, implementation-then-cover under unit. Scaffolding the wrong shape + // would hand the author a tasks.md the validator immediately rejects, so pick + // the template the declared flow will accept. + if opts.Artifact == "tasks" && effectiveFlow(data.DevelopmentFlow) == "unit" { + pair[1] = "templates/spec/tasks-unit.md.tmpl" + } target := filepath.Join(sdir, pair[0]) if pathExists(target) && !opts.Force { return fmt.Errorf("%s already exists. Use --force to overwrite", target) diff --git a/internal/cli/spec_flow_test.go b/internal/cli/spec_flow_test.go index aaf081a..1a90032 100644 --- a/internal/cli/spec_flow_test.go +++ b/internal/cli/spec_flow_test.go @@ -60,15 +60,15 @@ func TestSpecInitFlowInvalid(t *testing.T) { } } -// Req 1.3 / 2.2: with no steering default, an omitted flow resolves to tdd. -func TestSpecInitFlowDefaultsToTdd(t *testing.T) { +// Req 1.3 / 2.2: with no steering default, an omitted flow resolves to unit. +func TestSpecInitFlowDefaultsToUnit(t *testing.T) { dir := freshWorkspace(t) if code, _, errOut := run(t, "spec", "init", "no-flow", "--root", dir); code != 0 { t.Fatalf("init: code=%d err=%q", code, errOut) } s, _ := readSpecFlow(t, dir, "no-flow") - if s.DevelopmentFlow != "tdd" { - t.Errorf("development_flow = %q, want tdd", s.DevelopmentFlow) + if s.DevelopmentFlow != "unit" { + t.Errorf("development_flow = %q, want unit", s.DevelopmentFlow) } } @@ -97,22 +97,22 @@ func TestSpecInitFlowSteeringDefault(t *testing.T) { } } -// Req 2.2: an invalid steering default does not corrupt init; it falls back to tdd. +// Req 2.2: an invalid steering default does not corrupt init; it falls back to unit. func TestResolveDefaultFlowInvalidFallsBack(t *testing.T) { dir := freshWorkspace(t) steering := filepath.Join(dir, ".claude", "steering", "zzz-bad.md") if err := os.WriteFile(steering, []byte("---\ninclusion: manual\ndefault_development_flow: bogus\n---\n"), 0o644); err != nil { t.Fatal(err) } - if got := resolveDefaultFlow(dir); got != "tdd" { - t.Errorf("resolveDefaultFlow with invalid steering = %q, want tdd", got) + if got := resolveDefaultFlow(dir); got != "unit" { + t.Errorf("resolveDefaultFlow with invalid steering = %q, want unit", got) } } -// Req 3.1: a legacy spec.json with no development_flow is read as tdd. +// Req 3.1: a legacy spec.json with no development_flow is read as unit. func TestEffectiveFlowLegacy(t *testing.T) { - if got := effectiveFlow(""); got != "tdd" { - t.Errorf("effectiveFlow(\"\") = %q, want tdd", got) + if got := effectiveFlow(""); got != "unit" { + t.Errorf("effectiveFlow(\"\") = %q, want unit", got) } if got := effectiveFlow("unit"); got != "unit" { t.Errorf("effectiveFlow(unit) = %q, want unit", got) diff --git a/internal/plan/adr_test.go b/internal/plan/adr_test.go index 058492f..c62caa7 100644 --- a/internal/plan/adr_test.go +++ b/internal/plan/adr_test.go @@ -213,7 +213,7 @@ status: draft - verify: make check ` -func TestBriefInlinesADRs(t *testing.T) { +func TestBriefListsGoverningADRs(t *testing.T) { root := setupWorkspace(t, "p", adrBriefPlan) writeADRs(t, root, map[string]string{ "0001-pick-store.md": "# We store records under docs/adr\n\nDECISION_BODY_TOKEN: append-only, project-scoped.\n", @@ -234,12 +234,16 @@ func TestBriefInlinesADRs(t *testing.T) { if out2, _ := FeatBrief(root, doc, feat); out != out2 { t.Errorf("ADR brief is not byte-deterministic") } - // The resolved ADR is inlined in full (title + body). - if !strings.Contains(out, "We store records under docs/adr") { - t.Errorf("brief should inline the ADR title:\n%s", out) + // The brief lists the governing ADR as a short ref, NOT inlined in full: the + // body must not leak, and the session is directed to fetch it via the graph. + if !strings.Contains(out, "adr:pick-store") { + t.Errorf("brief should list the governing ADR ref:\n%s", out) } - if !strings.Contains(out, "DECISION_BODY_TOKEN") { - t.Errorf("brief should inline the ADR body in full (unlike wiki):\n%s", out) + if strings.Contains(out, "DECISION_BODY_TOKEN") { + t.Errorf("brief must NOT inline the ADR body (the gate enforces citation now):\n%s", out) + } + if !strings.Contains(out, "csdd graph explain adr:") { + t.Errorf("brief should direct the session to fetch the ADR via graph explain:\n%s", out) } // A broken ADR ref is a WARNING, not a hard omission. if !strings.Contains(out, "adr:ghost-ref") || !strings.Contains(out, "WARNING") { diff --git a/internal/plan/brief.go b/internal/plan/brief.go index cbf985e..4a50914 100644 --- a/internal/plan/brief.go +++ b/internal/plan/brief.go @@ -20,7 +20,7 @@ import ( // writes, which splits them into the state checks the session reads itself and // the command gate it delegates to the `quality-gate` sub-agent. var missionSteps = []string{ - "Author the spec if it does not exist yet: `csdd spec init [--flow unit|tdd|tdd-e2e]`, then generate, validate, and approve each phase — `csdd spec generate --artifact requirements|design|tasks`, `csdd spec validate `, `csdd spec approve --phase requirements|design|tasks`. This authoring is YOUR job — the decisions run on your (orchestrator) model. CHOOSE THE FLOW DELIBERATELY: `tdd` (the default) for money, auth, tenancy, and anything irreversible; `unit` for surfaces whose behaviors are render/CRUD states, which halves the verification round-trips per behavior. The flow you pick decides the shape of tasks.md, and `csdd spec validate` enforces that shape.", + "Author the spec if it does not exist yet: `csdd spec init [--flow unit|tdd|tdd-e2e]`, then for each phase DELEGATE the authoring to the `spec-author` sub-agent (sonnet): dispatch it with the feat, the phase, and any governing refs (ADRs/stack/wiki); it scaffolds (`csdd spec generate --artifact requirements|design|tasks`), consults the graph, authors the body, and runs `csdd spec validate `. REVIEW its artifact and run `csdd spec validate ` yourself, then `csdd spec approve --phase requirements|design|tasks`. If the review or the validate finds gaps, RE-DISPATCH spec-author with a fix-list rather than hand-editing inline. Authoring runs on the cheap model; the JUDGMENT — review, validate, approve — stays on your (orchestrator) model. CHOOSE THE FLOW DELIBERATELY: `unit` (the default) for surfaces whose behaviors are render/CRUD states, which halves the verification round-trips per behavior; `tdd` REQUIRED for money, auth, tenancy, and anything irreversible. The flow you pick decides the shape of tasks.md, and `csdd spec validate` enforces that shape.", "Implement the tasks by DELEGATING each one to the `implementer` sub-agent (Agent/Task tool) — one leaf task per sub-agent. You orchestrate and decide; the implementer executes the already-made decision on its own (fast) model under the spec's `development_flow`, so do NOT hand-write task code inline. HAND IT THE TASK, NOT THE SPEC: the dispatch carries the task text with its `_Requirements:_`/`_Boundary:_`/`_Depends:_`, the acceptance criteria those IDs name, and the `### ` section of design.md that owns the boundary — you already read the spec to author it, and making every implementer re-read all of it pays that cost again per sub-agent. Dispatch `(P)` tasks in DIFFERENT `_Boundary:_` groups concurrently (worktree isolation); honor every `_Depends:_` and keep same-boundary tasks sequential. Check each task's box `[x]` in specs//tasks.md as its implementer lands it green. (If the `implementer` sub-agent is not installed in this workspace, fall back to running the cycle skill that matches the flow yourself — `tdd-cycle` under tdd/tdd-e2e, `unit-cycle` under unit — one task at a time.)", "COMMIT YOUR WORK, and commit it where you already are. You are running inside a git worktree the runner created for this feat, already checked out on this feat's own branch — do NOT create a branch, do NOT switch branches, do NOT push, and do NOT open a PR. Commit via /csdd-commit (the pre-push gate runs there) as you land each task, and make sure NOTHING is left uncommitted before you declare `done`: the runner merges your BRANCH into the run's base, so work you left in the working tree does not exist as far as the plan is concerned. The runner refuses a `done` whose worktree is dirty and hands the feat straight back. One PR covers the whole run and a human opens it after the run ends.", "Record any technology or hard-to-reverse trade-off the contract does not already cover — a docs/stack.md Decided row, plus a docs/adr record when the why needs more than a line. Prefer the option that deviates least from the decided stack.", @@ -33,12 +33,15 @@ var forbiddenActions = []string{ } // FeatBrief assembles the deterministic mission pack for one whole feat (R7). It -// draws only on explicit content — the feat row, its seeds, resolved stack rows, -// resolved wiki refs (path + description, never the body), the Executor Notes, and -// the feat's own quality gates — so the same plan and feat always produce a -// byte-identical brief (R7.3). It inlines stack rows in full but never wiki bodies -// (R7.2). The autonomous run context (handoff, failure trail) is appended after -// this by the runner, so this prefix stays stable across a feat's sessions. +// draws only on explicit content — the feat row, its seeds, the governing ADR/stack +// refs (slugs/names/paths only — never ADR bodies, stack row details, or wiki +// descriptions), the Executor Notes, and the feat's own quality gates — so the same +// plan and feat always produce a byte-identical brief (R7.3). Compliance with the +// governors is enforced mechanically — designConformance requires design.md to cite +// each governing ADR, `plan validate` rejects tech outside the Decided stack, and +// the code-reviewer gate rejects it in code — not by inlining their bodies in the +// brief (R7.2). The autonomous run context (handoff, failure trail) is appended +// after this by the runner, so this prefix stays stable across a feat's sessions. func FeatBrief(root string, doc *PlanDoc, feat Feat) (string, error) { if _, ok := doc.Feat(feat.Slug); !ok { return "", fmt.Errorf("feat %q is not in plan %q", feat.Slug, doc.Slug) @@ -85,63 +88,55 @@ func FeatBrief(root string, doc *PlanDoc, feat Feat) (string, error) { } w("\n") - // 3. Stack rows — inlined in full (they are one-line contracts). + // 3. Governing stack — refs only. The brief used to inline each Decided row + // (choice/version/why) so a design could not drift from the stack; with the + // bodies gone, compliance is enforced mechanically — `csdd spec validate` + + // `plan validate` reject tech outside the Decided table, and the + // `code-reviewer` gate rejects it in code. The session fetches a row's detail + // via `csdd graph query ` when it needs it. if len(feat.StackRefs) > 0 { rows := decidedRows(root) - w("## Tech contract (docs/stack.md — use ONLY these)\n\n") + w("## Governing stack (docs/stack.md Decided — use ONLY these)\n\n") + w("Fetch a row's version/why via `csdd graph query `; do NOT introduce tech outside this list — the `code-reviewer` gate rejects it.\n\n") for _, name := range feat.StackRefs { - if r, ok := rows[normalizeTechName(name)]; ok { - w("- **%s** (%s) — version %s. %s", orDash(r.Choice), orDash(r.Domain), orDash(r.Version), orDash(r.Why)) - if r.Refs != "" { - w(" Refs: %s", r.Refs) - } - w("\n") + if _, ok := rows[normalizeTechName(name)]; ok { + w("- stack:%s\n", name) } else { - w("- **%s** — WARNING: not found in the Decided table (validate should have caught this).\n", name) + w("- stack:%s — WARNING: not found in the Decided table (validate should have caught this).\n", name) } } w("\n") } - // 3b. Decisions — cited ADRs inlined in full (they are short by format, the - // stack-row treatment; the why travels with the what, principle 4). + // 3b. Governing decisions — refs only. The brief used to inline each cited + // ADR's title + body so a design could not silently ignore the decisions it + // was bound to; with the bodies gone, `designConformance` requires the + // authored design.md to cite each governing adr:, and the session + // fetches the decision itself via `csdd graph explain adr:`. if len(feat.ADRRefs) > 0 { adrs := ScanADRs(root) - w("## Decisions (docs/adr — the why)\n\n") + w("## Governing decisions (docs/adr — fetch the why; cite each in design)\n\n") + w("The brief no longer inlines ADR bodies. For each governor run `csdd graph explain adr:` for the decision; `csdd spec validate` requires your design.md to cite it.\n\n") for _, slug := range feat.ADRRefs { - adr, res := adrs.Resolve(slug) - if res != ADRResolved { - w("- **adr:%s** — WARNING: does not resolve to a docs/adr record (validate should have caught this).\n", slug) + if _, res := adrs.Resolve(slug); res != ADRResolved { + w("- adr:%s — WARNING: does not resolve to a docs/adr record (validate should have caught this).\n", slug) continue } - w("### %s (adr:%s)\n\n", orDash(adr.Title), adr.Slug) - if adr.Status == ADRStatusSuperseded { - w("_status: superseded") - if adr.SupersededBy != 0 { - w(" by %s", fourDigit(adr.SupersededBy)) - } - w("_\n\n") - } - if adr.Body != "" { - w("%s\n\n", adr.Body) - } + w("- adr:%s\n", slug) } + w("\n") } - // 4. Wiki refs — path + description only (the session reads what it needs). + // 4. Wiki refs — path only (the session reads what it needs, when it needs it). if len(feat.WikiRefs) > 0 { w("## Reference pages (read what you need; do not assume the content)\n\n") for _, ref := range feat.WikiRefs { - path, desc := wikiRefInfo(root, ref) + path, _ := wikiRefInfo(root, ref) if path == "" { w("- [[%s]] — WARNING: no matching page under docs/wiki/pages/.\n", ref) continue } - if desc != "" { - w("- %s — %s\n", path, desc) - } else { - w("- %s\n", path) - } + w("- %s\n", path) } w("\n") } diff --git a/internal/plan/brief_test.go b/internal/plan/brief_test.go index 979abd8..7e70747 100644 --- a/internal/plan/brief_test.go +++ b/internal/plan/brief_test.go @@ -58,16 +58,20 @@ func TestBriefContentAndDeterminism(t *testing.T) { t.Errorf("brief is not byte-deterministic") } - // Stack row inlined in full (the Decided row for Go). - if !strings.Contains(out, "Go") || !strings.Contains(out, "1.22") { - t.Errorf("brief should inline the full stack row: %s", out) + // Stack row listed as a short ref only (the Decided row for go); version/why + // are NOT inlined — the session fetches them via the graph when it needs them. + if !strings.Contains(out, "stack:go") { + t.Errorf("brief should list the governing stack ref: %s", out) } - // Wiki ref as path + description, NOT the page body. + if strings.Contains(out, "1.22") { + t.Errorf("brief must NOT inline the stack row version/why (the gate enforces compliance now): %s", out) + } + // Wiki ref as path only — NOT the frontmatter description and NOT the page body. if !strings.Contains(out, "docs/wiki/pages/storage-design.md") { t.Errorf("brief should cite the wiki page path") } - if !strings.Contains(out, "How photos are stored") { - t.Errorf("brief should include the wiki frontmatter description") + if strings.Contains(out, "How photos are stored") { + t.Errorf("brief must NOT inline the wiki frontmatter description: %s", out) } if strings.Contains(out, "SECRET_BODY_TOKEN") { t.Errorf("brief must NOT inline the wiki page body (token leaked)") diff --git a/internal/plan/cost.go b/internal/plan/cost.go new file mode 100644 index 0000000..6957894 --- /dev/null +++ b/internal/plan/cost.go @@ -0,0 +1,150 @@ +package plan + +import ( + "fmt" + "sort" +) + +// What a run spent, read back out of sessions.jsonl. +// +// The file has always recorded every attempt with its cost (R9.2) and nothing has +// ever read it back except the resume path. That gap is not cosmetic: a real run +// (`frontend-design-refresh`, violet, 2026-07-27) recorded $20.01 across three +// settled sessions while sixteen further attempts sat on disk as bare `started` +// rows, and the only way to notice was to parse the JSONL by hand. +// +// So this report answers two questions, and the second matters more than the +// first: what did the run spend, and how much of what it spent can it still +// account for. + +// CostReport is a run's spend, per feat and in total. +type CostReport struct { + Plan string `json:"plan"` + Feats []FeatCost `json:"feats"` + Totals FeatCost `json:"totals"` + // ByModel is the whole run's spend split across the models that billed it. + // The plan loop tiers its work on purpose, so this is where "the orchestrator + // did the implementers' job" becomes visible as a number. + ByModel []ModelTokens `json:"by_model,omitempty"` + // Unmeasured is every attempt opened and never settled — a session the host + // outlived. Its cost is real and absent from every figure above, which is why + // it is reported as a named list rather than folded into a count. + Unmeasured []string `json:"unmeasured_attempts,omitempty"` +} + +// FeatCost is one feat's share of a run (or, as Totals, the whole of it). +type FeatCost struct { + Feat string `json:"feat,omitempty"` + // Attempts is how many sessions were OPENED for this feat, settled or not. + Attempts int `json:"attempts"` + // Settled is how many of them came back with a result to record. + Settled int `json:"settled"` + // Gated is how many `done` verdicts were refused and handed back — sessions + // paid for in full that delivered nothing (R10.3). + Gated int `json:"gated"` + Delivered bool `json:"delivered"` + CostUSD float64 `json:"cost_usd"` + Tokens SessionTokens `json:"tokens"` + Turns int `json:"turns,omitempty"` + ByStatus map[string]int `json:"by_status,omitempty"` +} + +// wasted is the share of a feat's sessions that were paid for and handed back. It +// is the number an optimization is trying to move. +func (f FeatCost) wasted() int { return f.Gated } + +// BuildCostReport reads sessions.jsonl and aggregates it. A missing file yields an +// empty report rather than an error: a plan that has never been run has spent +// nothing, which is a fine thing to report. +func BuildCostReport(root, slug string) CostReport { + rep := CostReport{Plan: slug, Totals: FeatCost{ByStatus: map[string]int{}}} + byFeat := map[string]*FeatCost{} + byModel := map[string]*ModelTokens{} + open := map[string]bool{} + order := []string{} + + feat := func(name string) *FeatCost { + if f, ok := byFeat[name]; ok { + return f + } + f := &FeatCost{Feat: name, ByStatus: map[string]int{}} + byFeat[name] = f + order = append(order, name) + return f + } + + for _, r := range LoadSessionRecords(root, slug) { + f := feat(r.Feat) + key := fmt.Sprintf("%s#%d", r.Feat, r.Attempt) + if r.Status == SessionStarted { + f.Attempts++ + rep.Totals.Attempts++ + open[key] = true + continue + } + delete(open, key) + f.Settled++ + f.ByStatus[r.Status]++ + f.CostUSD += r.CostUSD + f.Turns += r.NumTurns + f.Tokens = addTokens(f.Tokens, r.Tokens) + if r.Gated { + f.Gated++ + } + if r.Status == SessionDone { + f.Delivered = true + } + rep.Totals.Settled++ + rep.Totals.ByStatus[r.Status]++ + rep.Totals.CostUSD += r.CostUSD + rep.Totals.Turns += r.NumTurns + rep.Totals.Tokens = addTokens(rep.Totals.Tokens, r.Tokens) + if r.Gated { + rep.Totals.Gated++ + } + for _, m := range r.ByModel { + acc, ok := byModel[m.Model] + if !ok { + acc = &ModelTokens{Model: m.Model} + byModel[m.Model] = acc + } + acc.Tokens = addTokens(acc.Tokens, m.Tokens) + acc.CostUSD += m.CostUSD + } + } + + // An attempt that a delivered feat opened and never settled is still an + // unmeasured attempt: the feat landed on a later try, and whatever the crashed + // one spent is gone all the same. + for key := range open { + rep.Unmeasured = append(rep.Unmeasured, key) + } + sort.Strings(rep.Unmeasured) + for _, name := range order { + rep.Feats = append(rep.Feats, *byFeat[name]) + } + for _, m := range byModel { + rep.ByModel = append(rep.ByModel, *m) + } + sort.Slice(rep.ByModel, func(i, j int) bool { return rep.ByModel[i].Model < rep.ByModel[j].Model }) + return rep +} + +func addTokens(a, b SessionTokens) SessionTokens { + a.Input += b.Input + a.Output += b.Output + a.CacheRead += b.CacheRead + a.CacheCreation += b.CacheCreation + return a +} + +// ModelTotal is the sum of the per-model breakdown. Comparing it against Totals +// .Tokens is the point: the two are read from different fields of the same event, +// so a gap between them is the sub-agent work the top-level `usage` did not count. +func (r CostReport) ModelTotal() SessionTokens { + var t SessionTokens + for _, m := range r.ByModel { + t = addTokens(t, m.Tokens) + } + return t +} diff --git a/internal/plan/cost_verify_test.go b/internal/plan/cost_verify_test.go new file mode 100644 index 0000000..9f929fb --- /dev/null +++ b/internal/plan/cost_verify_test.go @@ -0,0 +1,336 @@ +package plan + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// Reading a run back: what it spent (cost.go) and what it actually delivered +// (verify.go). +// +// Both are answers to questions the violet run raised and no command could +// answer — how much of the spend is still accounted for, and whether the four +// records that claim a feat was delivered agree with each other. + +// --- cost --------------------------------------------------------------------- + +func TestCostReportSeparatesSpendFromWhatItCanAccountFor(t *testing.T) { + root := t.TempDir() + slug := "p" + rows := []SessionRecord{ + // A feat that took two sessions: one refused `done`, then a delivery. + {Feat: "a", Attempt: 1, Status: SessionStarted}, + {Feat: "a", Attempt: 1, Status: SessionContinue, Gated: true, CostUSD: 11.16, NumTurns: 25, + Tokens: SessionTokens{Input: 60, Output: 20000, CacheRead: 1500000}}, + {Feat: "a", Attempt: 2, Status: SessionStarted}, + {Feat: "a", Attempt: 2, Status: SessionDone, CostUSD: 4.22, NumTurns: 12, + Tokens: SessionTokens{Input: 40, Output: 10000, CacheRead: 900000}}, + // A feat whose session the host outlived: opened, never settled. + {Feat: "b", Attempt: 1, Status: SessionStarted}, + } + for _, r := range rows { + if err := AppendSessionRecord(root, slug, r); err != nil { + t.Fatal(err) + } + } + + rep := BuildCostReport(root, slug) + if len(rep.Feats) != 2 { + t.Fatalf("want a row per feat, got %d", len(rep.Feats)) + } + a := rep.Feats[0] + if a.Attempts != 2 || a.Settled != 2 || a.Gated != 1 || !a.Delivered { + t.Errorf("feat a: %+v", a) + } + if !nearUSD(a.CostUSD, 15.38) { + t.Errorf("feat a must carry both sessions' cost, got $%.2f", a.CostUSD) + } + // The whole point of the report: an attempt with no settled row is spend the + // run cannot account for, and it must be named rather than quietly dropped. + if len(rep.Unmeasured) != 1 || rep.Unmeasured[0] != "b#1" { + t.Errorf("the crashed attempt must be reported as unmeasured, got %v", rep.Unmeasured) + } + if rep.Totals.Attempts != 3 || rep.Totals.Settled != 2 { + t.Errorf("totals must count opened and settled separately: %+v", rep.Totals) + } + if !nearUSD(rep.Totals.CostUSD, 15.38) { + t.Errorf("totals cost = $%.2f", rep.Totals.CostUSD) + } +} + +func TestCostReportSplitsSpendAcrossModels(t *testing.T) { + root := t.TempDir() + rows := []SessionRecord{ + {Feat: "a", Attempt: 1, Status: SessionStarted}, + {Feat: "a", Attempt: 1, Status: SessionDone, CostUSD: 6, + Tokens: SessionTokens{Output: 1000}, + ByModel: []ModelTokens{ + {Model: "claude-opus-5", Tokens: SessionTokens{Output: 1000}, CostUSD: 5}, + {Model: "claude-sonnet-5", Tokens: SessionTokens{Output: 4000}, CostUSD: 1}, + }}, + } + for _, r := range rows { + if err := AppendSessionRecord(root, "p", r); err != nil { + t.Fatal(err) + } + } + rep := BuildCostReport(root, "p") + if len(rep.ByModel) != 2 { + t.Fatalf("want a row per billing model, got %+v", rep.ByModel) + } + if rep.ByModel[0].Model != "claude-opus-5" || rep.ByModel[1].Model != "claude-sonnet-5" { + t.Errorf("the breakdown must be sorted by model, got %+v", rep.ByModel) + } + // The gap between the two totals is the whole reason both are reported: the + // session's own `usage` counted 1000 tokens, the per-model breakdown 5000. + if got, want := rep.ModelTotal().Total(), 5000; got != want { + t.Errorf("per-model total = %d, want %d", got, want) + } + if rep.Totals.Tokens.Total() != 1000 { + t.Errorf("the session total must stay what the envelope reported, got %d", rep.Totals.Tokens.Total()) + } +} + +func TestParseSessionMetricsReadsPerModelUsage(t *testing.T) { + // camelCase is what the CLI has been observed to emit; snake_case is what the + // API uses. Both are read, so a rename upstream degrades one column instead of + // zeroing the breakdown. + raw := `{"type":"result","total_cost_usd":9.5,"usage":{"input_tokens":10,"output_tokens":20}, + "modelUsage":{ + "claude-opus-5":{"inputTokens":100,"outputTokens":200,"cacheReadInputTokens":3000,"costUSD":7.5}, + "claude-sonnet-5":{"input_tokens":5,"output_tokens":6,"cache_read_input_tokens":7,"cost_usd":2.0}}}` + m := parseSessionMetrics([]byte(raw)) + if len(m.ByModel) != 2 { + t.Fatalf("want both models, got %+v", m.ByModel) + } + opus := m.ByModel[0] + if opus.Model != "claude-opus-5" || opus.Tokens.Input != 100 || opus.Tokens.CacheRead != 3000 || opus.CostUSD != 7.5 { + t.Errorf("camelCase entry misread: %+v", opus) + } + sonnet := m.ByModel[1] + if sonnet.Tokens.Input != 5 || sonnet.Tokens.CacheRead != 7 || sonnet.CostUSD != 2.0 { + t.Errorf("snake_case entry misread: %+v", sonnet) + } + if len(m.Models) != 2 { + t.Errorf("the model-name list must survive alongside the breakdown, got %v", m.Models) + } +} + +// --- verify ------------------------------------------------------------------- + +// verifyPlan is two independent feats, which is enough to put one in every state +// the report distinguishes. +const verifyPlanMD = `--- +name: p +status: draft +--- + +## Feats + +| # | Feat | Objective | Depends | Milestone | (P) | Refs | +|---|------|-----------|---------|-----------|-----|------| +| 1 | a | A | — | M1 | | | +| 2 | b | B | — | M1 | | | + +## Quality Gates + +- verify: make check +` + +// TestVerifyDistinguishesMergedFromReconciled is the case the violet workspace +// produced and no command could report: two feats both marked done, one carrying a +// merge commit and one that was finished by hand before the run and imported into +// the ledger. `plan status` calls both "done"; only git tells them apart. +func TestVerifyDistinguishesMergedFromReconciled(t *testing.T) { + root := gitRepo(t) + seedApprovedPlan(t, root, verifyPlanMD) + doc, err := Load(root, "p") + if err != nil { + t.Fatal(err) + } + + // Only `a` is landed the way the runner lands a feat. Driving the real + // Integrate rather than hand-writing a commit is the point: it is the only way + // this test can catch the merge subject and the grep drifting apart. + trees := newTrees(root) + dir, err := trees.Ensure("a") + if err != nil { + t.Fatal(err) + } + writeRepoFile(t, dir, "a.go", "package p\n") + commitAll(t, dir, "work for a") + if err := trees.Integrate("a"); err != nil { + t.Fatalf("could not land a on the base: %v", err) + } + if err := trees.Discard("a"); err != nil { + t.Fatal(err) + } + + // Both feats have complete artifacts and both are in the ledger. + for _, slug := range []string{"a", "b"} { + if err := deliverSpecFiles(root, slug); err != nil { + t.Fatal(err) + } + } + ledger := LoadLedger(root, "p") + ledger.MarkDone("a", "merged by the run", fixedNow()) + ledger.MarkDone("b", "reconciled: delivered on disk before this run", fixedNow()) + if err := ledger.Save(root, "p"); err != nil { + t.Fatal(err) + } + + rep := Verify(root, doc) + if !rep.Git || rep.Branch != "main" { + t.Fatalf("the merge check should run against main, got git=%v branch=%q", rep.Git, rep.Branch) + } + byFeat := map[string]FeatVerification{} + for _, f := range rep.Feats { + byFeat[f.Feat] = f + } + if got := byFeat["a"]; got.State != DeliveredMerged || !got.Merged || len(got.Findings) != 0 { + t.Errorf("a is corroborated by every record and must be clean: %+v", got) + } + if got := byFeat["b"]; got.State != DeliveredOffRun || got.Merged { + t.Errorf("b has no merge commit and must be reported as delivered off-run: %+v", got) + } + if got := byFeat["b"]; len(got.Findings) == 0 { + t.Error("a feat the branch holds no evidence for must produce a finding") + } + if rep.OK { + t.Error("a report with a finding is not OK") + } +} + +// TestVerifyCatchesADeliveredFeatWithNoArtifacts is the failure that matters most: +// the ledger is what the sequencer trusts, so a feat marked done without its +// artifacts is skipped by every later run while the work it stands for is absent. +func TestVerifyCatchesADeliveredFeatWithNoArtifacts(t *testing.T) { + root := gitRepo(t) + seedApprovedPlan(t, root, verifyPlanMD) + doc, err := Load(root, "p") + if err != nil { + t.Fatal(err) + } + ledger := LoadLedger(root, "p") + ledger.MarkDone("a", "claimed", fixedNow()) + if err := ledger.Save(root, "p"); err != nil { + t.Fatal(err) + } + + rep := Verify(root, doc) + var a FeatVerification + for _, f := range rep.Feats { + if f.Feat == "a" { + a = f + } + } + if a.Artifacts { + t.Fatal("no spec was written; the artifacts cannot hold") + } + if len(a.ArtifactGaps) == 0 { + t.Error("the report must name which artifact is missing, not just that one is") + } + if len(a.Findings) == 0 || !strings.Contains(a.Findings[0], "skip") { + t.Errorf("the finding must say the feat will be skipped forever, got %v", a.Findings) + } + if rep.OK { + t.Error("a ledger that outruns the artifacts is not OK") + } +} + +// TestVerifyReportsAnUndeliveredFeatWithoutComplaining keeps the command usable: +// a plan half-way through a run is not a plan with problems, and a verifier that +// cries about every pending feat gets ignored. +func TestVerifyReportsAnUndeliveredFeatWithoutComplaining(t *testing.T) { + root := gitRepo(t) + seedApprovedPlan(t, root, verifyPlanMD) + doc, err := Load(root, "p") + if err != nil { + t.Fatal(err) + } + // `a` has a live worktree: a session is mid-flight or left uncommitted work. + trees := newTrees(root) + if _, err := trees.Ensure("a"); err != nil { + t.Fatal(err) + } + + rep := Verify(root, doc) + byFeat := map[string]FeatVerification{} + for _, f := range rep.Feats { + byFeat[f.Feat] = f + } + if got := byFeat["a"]; got.State != InProgress || !got.LiveWorktree { + t.Errorf("a feat holding a live worktree is in progress: %+v", got) + } + if got := byFeat["b"]; got.State != Pending { + t.Errorf("a feat nothing has touched is pending: %+v", got) + } + if !rep.OK { + t.Errorf("an unfinished plan is not an inconsistent one: %+v", rep.Feats) + } +} + +// --- the worktree entry document ---------------------------------------------- + +// TestWorktreeGetsTheLeanEntryDocAndCannotCommitIt pins both halves of the swap. +// The saving is worthless if the session can commit the replacement back over the +// file a human maintains, and the uncommitted-work check would refuse every +// delivery if the swap showed up as a dirty path. +func TestWorktreeGetsTheLeanEntryDocAndCannotCommitIt(t *testing.T) { + root := gitRepo(t) + writeRepoFile(t, root, "CLAUDE.md", "# the repository's own, long and interactive\n") + commitAll(t, root, "add the entry point") + + trees := newTrees(root) + trees.entry = "# lean plan-session entry\n" + dir, err := trees.Ensure("a") + if err != nil { + t.Fatal(err) + } + + got, err := os.ReadFile(filepath.Join(dir, "CLAUDE.md")) + if err != nil { + t.Fatal(err) + } + if string(got) != trees.entry { + t.Errorf("the worktree must read the lean entry doc, got %q", got) + } + // Invisible to git: nothing to commit, nothing to merge back, and the + // uncommitted-work check stays clean. + status, err := runGit(dir, "status", "--porcelain") + if err != nil { + t.Fatal(err) + } + for _, line := range gitLines(status) { + if strings.Contains(line, "CLAUDE.md") { + t.Errorf("the swapped entry doc must not appear as a change, status: %q", status) + } + } + // And the repository's own copy is untouched. + if repo := readRepoFile(t, root, "CLAUDE.md"); !strings.Contains(repo, "long and interactive") { + t.Errorf("the human's CLAUDE.md must not be rewritten, got %q", repo) + } +} + +// TestWorktreeKeepsAnUntrackedEntryDocAlone is the guard on the guard. The swap is +// only safe because skip-worktree hides it, and that needs a tracked file — so an +// untracked CLAUDE.md must be left alone rather than created, which would surface +// as untracked work and fail the check that protects a delivered feat. +func TestWorktreeKeepsAnUntrackedEntryDocAlone(t *testing.T) { + root := gitRepo(t) // no CLAUDE.md committed + trees := newTrees(root) + trees.entry = "# lean plan-session entry\n" + dir, err := trees.Ensure("a") + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(dir, "CLAUDE.md")); !os.IsNotExist(err) { + t.Errorf("no CLAUDE.md is tracked, so none may be written; stat err = %v", err) + } + status, _ := runGit(dir, "status", "--porcelain") + if strings.Contains(status, "CLAUDE.md") { + t.Errorf("the worktree must stay clean, status: %q", status) + } +} diff --git a/internal/plan/debug_test.go b/internal/plan/debug_test.go new file mode 100644 index 0000000..44278fb --- /dev/null +++ b/internal/plan/debug_test.go @@ -0,0 +1,862 @@ +package plan + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" +) + +// A debugger for `plan run`. +// +// The suite already proves individual properties — the squad overlaps, the gate +// refuses, a conflict rolls back. What it does not give you is the ONE artifact you +// want when a real run misbehaves: an ordered account of every decision the loop +// made, next to the durable record it left behind, so you can see where the two +// disagree. +// +// That gap is not hypothetical. A real run (`frontend-design-refresh` in the +// `violet` workspace, 2026-07-27) spent thirteen dispatches to deliver one of four +// feats, and its sessions.jsonl explains almost none of it: sixteen of nineteen +// rows are `started` with no settled counterpart and no cost. Reading the run after +// the fact meant reconstructing it from a journal, a failure log and a progress +// file that each hold a different slice of the truth. +// +// runTrace records the loop from inside its own seams (Hooks) and then reconciles +// what it saw against sessions.jsonl. `go test -run TestDebug -v` prints the +// timeline; the scenarios below assert on it. Every scenario runs one deliberately +// trivial feat, because the subject under observation is the LOOP, and a feat with +// real content only adds noise to the trace. + +// --- the trace ---------------------------------------------------------------- + +type traceKind string + +const ( + trEnsure traceKind = "ensure" // a worktree was handed to a dispatch + trSessionIn traceKind = "session>" // the session hook was entered + trSessionOu traceKind = "session<" // the session hook returned a verdict + trIntegrate traceKind = "integrate" // the feat's branch was merged into the base + trDiscard traceKind = "discard" // the worktree was released + trSleep traceKind = "sleep" // the loop waited (account limit / spawn backoff) + trNote traceKind = "note" // an observation a scenario made from inside a session +) + +// traceEvent is one decision, in the order the loop made it. +type traceEvent struct { + seq int + at time.Duration // since the run started, so a stall is visible in the dump + kind traceKind + feat string + detail string +} + +// runTrace observes a run through the hooks it already exposes and doubles as the +// treeKeeper, which is the only way to see the worktree each dispatch was handed — +// the fact that distinguishes "the retry got a fresh tree" from "the retry got the +// stale one", and neither the journal nor the ledger records it. +// +// It is safe for a squad: every field is behind the mutex because the Session hook +// runs on a session goroutine while Ensure/Integrate/Discard run on the loop's. +type runTrace struct { + mu sync.Mutex + start time.Time + events []traceEvent + // dirs is the worktree handed to each dispatch of a feat, in order. + dirs map[string][]string + // errs collects failures observed from a session goroutine. t.Fatal there runs + // runtime.Goexit, the session never posts its result, and the run loop waits for + // it until the whole binary times out — so a session-side check records instead + // of failing, exactly as the squad tests do. + errs []error + inner treeKeeper +} + +func newRunTrace(inner treeKeeper) *runTrace { + return &runTrace{start: time.Now(), dirs: map[string][]string{}, inner: inner} +} + +func (tr *runTrace) add(kind traceKind, feat, detail string) { + tr.mu.Lock() + defer tr.mu.Unlock() + tr.events = append(tr.events, traceEvent{ + seq: len(tr.events) + 1, at: time.Since(tr.start), kind: kind, feat: feat, detail: detail, + }) +} + +func (tr *runTrace) fail(err error) { + tr.mu.Lock() + defer tr.mu.Unlock() + tr.errs = append(tr.errs, err) +} + +// treeKeeper: delegate to the real keeper, record what it returned. + +func (tr *runTrace) Ensure(feat string) (string, error) { + dir, err := tr.inner.Ensure(feat) + tr.mu.Lock() + tr.dirs[feat] = append(tr.dirs[feat], dir) + n := len(tr.dirs[feat]) + tr.mu.Unlock() + tr.add(trEnsure, feat, fmt.Sprintf("dispatch #%d dir=%s err=%v", n, filepath.Base(dir), err)) + return dir, err +} + +func (tr *runTrace) Integrate(feat string) error { + err := tr.inner.Integrate(feat) + tr.add(trIntegrate, feat, describeErr(err)) + return err +} + +func (tr *runTrace) Discard(feat string) error { + err := tr.inner.Discard(feat) + tr.add(trDiscard, feat, describeErr(err)) + return err +} + +// describeErr names an integration outcome the way the loop branches on it, so the +// dump distinguishes the three cases the runner treats differently rather than +// printing one opaque error string. +func describeErr(err error) string { + switch e := err.(type) { + case nil: + return "merged" + case *MergeConflictError: + return "CONFLICT in " + strings.Join(e.Files, ", ") + case *UncommittedWorkError: + return fmt.Sprintf("UNCOMMITTED %d path(s)", len(e.Paths)) + default: + return "error: " + err.Error() + } +} + +// wrap installs the trace into a set of hooks: it becomes the treeKeeper and +// interposes on the session so both ends of a dispatch land in the timeline. +func (tr *runTrace) wrap(h Hooks) Hooks { + inner := h.Session + h.Trees = tr + h.Session = func(req SessionRequest) (SessionOutcome, error) { + tr.add(trSessionIn, req.Feat.Slug, fmt.Sprintf("brief=%dB dir=%s", len(req.Brief), filepath.Base(req.Dir))) + out, err := inner(req) + tr.add(trSessionOu, req.Feat.Slug, fmt.Sprintf("verdict=%s err=%v cost=$%.2f tokens=%d", + orNone(out.Verdict.Status), err, out.Metrics.CostUSD, out.Metrics.Tokens.Total())) + return out, err + } + sleep := h.Sleep + h.Sleep = func(d time.Duration) { + tr.add(trSleep, "-", d.String()) + if sleep != nil { + sleep(d) + } + } + return h +} + +func orNone(s string) string { + if s == "" { + return "" + } + return s +} + +// waitFor blocks until an event matching kind and feat has been recorded. It is how +// a scenario imposes an order on a squad without reaching into the runner: a session +// can park until its peer's merge has actually landed, which turns a race into a +// deterministic sequence. Returns false on timeout so the caller reports a stuck +// run instead of hanging the binary. +func (tr *runTrace) waitFor(kind traceKind, feat string, within time.Duration) bool { + deadline := time.Now().Add(within) + for time.Now().Before(deadline) { + if tr.count(kind, feat) > 0 { + return true + } + time.Sleep(2 * time.Millisecond) + } + return false +} + +// count is how many recorded events match kind and feat; an empty feat counts every +// feat. +func (tr *runTrace) count(kind traceKind, feat string) int { + tr.mu.Lock() + defer tr.mu.Unlock() + n := 0 + for _, e := range tr.events { + if e.kind == kind && (feat == "" || e.feat == feat) { + n++ + } + } + return n +} + +// dirsFor is every worktree handed to a feat, in dispatch order. +func (tr *runTrace) dirsFor(feat string) []string { + tr.mu.Lock() + defer tr.mu.Unlock() + return append([]string(nil), tr.dirs[feat]...) +} + +// --- reconciliation against the durable record -------------------------------- + +// ledgerAudit is what sessions.jsonl claims about a run, summarized the way an +// operator reading it after the fact needs it. +type ledgerAudit struct { + started int + settled int + unsettled []string // "feat#attempt" opened and never closed — a crashed attempt + costed int // settled rows that carry a cost + free []string // settled rows reporting no cost at all + totalUSD float64 + byStatus map[string]int +} + +// auditLedger reads sessions.jsonl back and pairs every `started` row with its +// settled counterpart. +// +// This is the check the real run needed and nobody could make: a `started` row with +// no settled row is an attempt that crashed, and its cost is gone — the metrics are +// only ever attached on the settle path. Sixteen of the nineteen rows in the violet +// run were exactly that, which is why its recorded $20.01 is a fraction of what it +// actually spent. +func auditLedger(root, slug string) ledgerAudit { + a := ledgerAudit{byStatus: map[string]int{}} + open := map[string]bool{} + for _, r := range LoadSessionRecords(root, slug) { + a.byStatus[r.Status]++ + key := fmt.Sprintf("%s#%d", r.Feat, r.Attempt) + if r.Status == SessionStarted { + a.started++ + open[key] = true + continue + } + a.settled++ + delete(open, key) + a.totalUSD += r.CostUSD + if r.CostUSD > 0 || r.Tokens.Total() > 0 { + a.costed++ + } else { + a.free = append(a.free, key+" ("+r.Status+")") + } + } + for key := range open { + a.unsettled = append(a.unsettled, key) + } + return a +} + +// nearUSD compares two dollar figures at cent precision. Costs are summed as +// float64 across rows, so 11.16 + 4.22 is not exactly 15.38 and an equality check +// would fail on arithmetic rather than on behavior. +func nearUSD(got, want float64) bool { + d := got - want + return d < 0.005 && d > -0.005 +} + +// dump prints the timeline and the audit through t.Log, which is the debugger +// itself: `go test -run -v` is the whole interface. +func (tr *runTrace) dump(t *testing.T, root, slug string, sum RunSummary, runLog string) { + t.Helper() + var b strings.Builder + b.WriteString("\n╭─ plan run timeline ─────────────────────────────────────────────\n") + tr.mu.Lock() + for _, e := range tr.events { + fmt.Fprintf(&b, "│ %3d %7dms %-10s %-10s %s\n", e.seq, e.at.Milliseconds(), e.kind, e.feat, e.detail) + } + tr.mu.Unlock() + b.WriteString("├─ sessions.jsonl ────────────────────────────────────────────────\n") + for _, r := range LoadSessionRecords(root, slug) { + fmt.Fprintf(&b, "│ %-10s attempt %d %-9s cost=$%-6.2f tokens=%-9d gated=%v %s\n", + r.Feat, r.Attempt, r.Status, r.CostUSD, r.Tokens.Total(), r.Gated, firstLine(r.Detail)) + } + a := auditLedger(root, slug) + b.WriteString("├─ audit ─────────────────────────────────────────────────────────\n") + fmt.Fprintf(&b, "│ started=%d settled=%d unsettled=%v\n", a.started, a.settled, a.unsettled) + fmt.Fprintf(&b, "│ settled rows with a cost=%d, without=%v, total=$%.2f\n", a.costed, a.free, a.totalUSD) + fmt.Fprintf(&b, "│ summary: sessions=%d delivered=%d failures=%d gated=%d blocked=%v outcome=%d\n", + sum.Sessions, sum.Steps, sum.Failures, sum.Gated, sum.Blocked, sum.Outcome) + fmt.Fprintf(&b, "│ reason: %s\n", sum.Reason) + if runLog != "" { + b.WriteString("├─ runner output ─────────────────────────────────────────────────\n") + for _, line := range strings.Split(strings.TrimRight(runLog, "\n"), "\n") { + fmt.Fprintf(&b, "│ %s\n", line) + } + } + b.WriteString("╰─────────────────────────────────────────────────────────────────") + t.Log(b.String()) +} + +// --- the subject: one trivial feat -------------------------------------------- + +// debugSoloPlan is a single feat with nothing in it. The loop is what is under +// observation, so the feat carries no dependencies, no stack refs and no wiki refs — +// anything it did carry would appear in the brief and in the trace without telling +// us anything about the runner. +const debugSoloPlan = `--- +name: p +status: draft +--- + +## Feats + +| # | Feat | Objective | Depends | Milestone | (P) | Refs | +|---|------|-----------|---------|-----------|-----|------| +| 1 | widget | Ship one trivial widget | — | M1 | | | + +## Quality Gates + +- verify: make check +` + +// debugPairPlan is the same trivial feat twice, independent, so a squad of two can +// put both in flight at once. It is the smallest shape in which two feats can +// collide on the base. +const debugPairPlan = `--- +name: p +status: draft +--- + +## Feats + +| # | Feat | Objective | Depends | Milestone | (P) | Refs | +|---|------|-----------|---------|-----------|-----|------| +| 1 | widget | Ship one trivial widget | — | M1 | P | | +| 2 | gadget | Ship one trivial gadget | — | M1 | P | | + +## Quality Gates + +- verify: make check +` + +// debugRepo is a real repository with the plan approved and committed, plus the +// tracing keeper wrapped around the real git one. Real git is the point: the +// failures worth debugging live in the merge, and a stubbed keeper cannot produce +// them. +// +// The returned buffer is BOTH the keeper's log destination and the run's, mirroring +// how Run wires them together — otherwise a scenario could not see the one thing +// the keeper decides on its own. Both are written from the loop's goroutine only, +// so sharing the buffer is safe under a squad. +func debugRepo(t *testing.T, planMD string) (root string, tr *runTrace, out *bytes.Buffer) { + t.Helper() + root = gitRepo(t) + seedApprovedPlan(t, root, planMD) + out = &bytes.Buffer{} + keeper := newTrees(root) + keeper.logf = func(format string, a ...any) { fmt.Fprintf(out, format+"\n", a...) } + return root, newRunTrace(keeper), out +} + +// debugHooks are baseHooks with the trace installed and the session left to the +// caller: every scenario differs only in what its session does. +func debugHooks(t *testing.T, root string, tr *runTrace, session func(SessionRequest) (SessionOutcome, error)) Hooks { + t.Helper() + h := baseHooks(t, root) + h.Session = session + return tr.wrap(h) +} + +// deliverInTree writes the three artifacts the verdict gate demands and commits +// them, which is the whole of what a session contracts to leave behind. Errors are +// returned rather than fatal because this runs on a session goroutine. +func deliverInTree(dir, slug string, extra map[string]string) error { + if err := deliverSpecFiles(dir, slug); err != nil { + return err + } + for rel, content := range extra { + p := filepath.Join(dir, rel) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + return err + } + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + return err + } + } + if out, err := runGit(dir, "add", "-A"); err != nil { + return fmt.Errorf("git add in %s: %v: %s", dir, err, out) + } + // A retry that re-does work its predecessor already committed has nothing new to + // record, and git calls that an error. It is not one here: the contract the + // runner checks is that the tree is clean and the work is on the branch, which + // is already true. Treating it as a failure would make every scenario that + // re-dispatches a feat fail for a reason that has nothing to do with the loop. + if out, err := runGit(dir, "status", "--porcelain"); err == nil && strings.TrimSpace(out) == "" { + return nil + } + if out, err := runGit(dir, "commit", "-m", "feat "+slug); err != nil { + return fmt.Errorf("git commit in %s: %v: %s", dir, err, out) + } + return nil +} + +// paidOutcome is a verdict carrying plausible metrics, so a scenario can watch what +// the loop does with the cost of an attempt rather than always seeing zero. +func paidOutcome(status string, usd float64) SessionOutcome { + return SessionOutcome{ + Verdict: Verdict{Status: status}, + Metrics: SessionMetrics{ + Duration: time.Minute, CostUSD: usd, NumTurns: 12, + Tokens: SessionTokens{Input: 60, Output: 20000, CacheRead: 1500000, CacheCreation: 40000}, + Models: []string{"claude-opus-5"}, + }, + } +} + +// --- scenarios ---------------------------------------------------------------- + +// TestDebugHappyPathIsFullyAccounted is the baseline every other scenario is read +// against: one feat, one session, delivered. It pins the invariant the violet run +// broke — every attempt that was opened was also settled, and every settled attempt +// carries what it cost. +func TestDebugHappyPathIsFullyAccounted(t *testing.T) { + root, tr, out := debugRepo(t, debugSoloPlan) + h := debugHooks(t, root, tr, func(req SessionRequest) (SessionOutcome, error) { + if err := deliverInTree(req.Dir, req.Feat.Slug, map[string]string{"widget.go": "package p\n"}); err != nil { + return SessionOutcome{}, err + } + return paidOutcome(VerdictDone, 4.64), nil + }) + + sum, err := Run(RunOptions{Root: root, Slug: "p", Hooks: h, Out: out}) + if err != nil { + t.Fatal(err) + } + tr.dump(t, root, "p", sum, out.String()) + + if !sum.Completed || sum.Steps != 1 || sum.Sessions != 1 { + t.Fatalf("one trivial feat should take exactly one session: %+v", sum) + } + if n := tr.count(trIntegrate, "widget"); n != 1 { + t.Errorf("the delivered feat must be merged exactly once, got %d", n) + } + if n := tr.count(trDiscard, "widget"); n != 1 { + t.Errorf("a delivered feat's worktree must be released, got %d discards", n) + } + a := auditLedger(root, "p") + if len(a.unsettled) != 0 { + t.Errorf("every opened attempt must be settled, dangling: %v", a.unsettled) + } + if a.started != 1 || a.settled != 1 { + t.Errorf("want 1 started + 1 settled row, got %d/%d", a.started, a.settled) + } + if a.costed != 1 || !nearUSD(a.totalUSD, 4.64) { + t.Errorf("the session's cost must reach the record: costed=%d total=$%.2f", a.costed, a.totalUSD) + } +} + +// TestDebugRefusedDoneCostsAWholeSecondSession prices the verdict gate. A session +// that claims `done` without leaving the artifacts is handed back, and the retry is +// not a cheap correction — it is another entire session, paid in full. +// +// This is the shape that dominated the violet run: two of its three settled sessions +// were gated `done` claims, $15.38 of the $20.01 it managed to record. +func TestDebugRefusedDoneCostsAWholeSecondSession(t *testing.T) { + root, tr, out := debugRepo(t, debugSoloPlan) + var attempts int + h := debugHooks(t, root, tr, func(req SessionRequest) (SessionOutcome, error) { + attempts++ + if attempts == 1 { + // Confident and wrong: nothing on disk, `done` anyway. + return paidOutcome(VerdictDone, 11.16), nil + } + if err := deliverInTree(req.Dir, req.Feat.Slug, nil); err != nil { + return SessionOutcome{}, err + } + return paidOutcome(VerdictDone, 4.22), nil + }) + + sum, err := Run(RunOptions{Root: root, Slug: "p", Hooks: h, Out: out}) + if err != nil { + t.Fatal(err) + } + tr.dump(t, root, "p", sum, out.String()) + + if sum.Gated != 1 { + t.Errorf("the empty `done` must be refused exactly once, gated=%d", sum.Gated) + } + if sum.Sessions != 2 || !sum.Completed { + t.Fatalf("the refusal must cost a second full session and then complete: %+v", sum) + } + // The refused session never reached Integrate: the gate runs first, so nothing + // was merged and the tree was not released. + if n := tr.count(trIntegrate, "widget"); n != 1 { + t.Errorf("only the delivering session should reach the merge, integrates=%d", n) + } + a := auditLedger(root, "p") + if !nearUSD(a.totalUSD, 11.16+4.22) { + t.Errorf("both sessions were paid for and both must be recorded, total=$%.2f", a.totalUSD) + } + if a.byStatus[SessionContinue] != 1 { + t.Errorf("the refused `done` must be recorded as a continue, got %v", a.byStatus) + } +} + +// TestDebugCrashedAttemptLosesItsCost pins the instrumentation hole directly. +// +// recordSession writes the `started` row BEFORE the session is spawned and the cost +// is only ever attached on the settle path, so everything between the two is a +// window in which an interrupted run loses what it spent. The scenario observes the +// window from inside the session — the exact moment a reboot would land — and +// asserts what the record holds there: an open attempt with no cost. +// +// The attempt itself survives, which is deliberate (a crashed feat must not get its +// bound back). The money does not, and that is the defect. +func TestDebugCrashedAttemptLosesItsCost(t *testing.T) { + root, tr, out := debugRepo(t, debugSoloPlan) + var midFlight ledgerAudit + h := debugHooks(t, root, tr, func(req SessionRequest) (SessionOutcome, error) { + // The row for THIS attempt is already on disk; a crash here is what the + // violet run hit sixteen times. + midFlight = auditLedger(root, "p") + if err := deliverInTree(req.Dir, req.Feat.Slug, nil); err != nil { + return SessionOutcome{}, err + } + return paidOutcome(VerdictDone, 7.00), nil + }) + + sum, err := Run(RunOptions{Root: root, Slug: "p", Hooks: h, Out: out}) + if err != nil { + t.Fatal(err) + } + tr.dump(t, root, "p", sum, out.String()) + + if midFlight.started != 1 { + t.Fatalf("the attempt must be on disk before the session runs, started=%d", midFlight.started) + } + if len(midFlight.unsettled) != 1 { + t.Fatalf("mid-session the attempt must be open, unsettled=%v", midFlight.unsettled) + } + if !nearUSD(midFlight.totalUSD, 0) { + t.Errorf("nothing is expected to be costed yet, got $%.2f", midFlight.totalUSD) + } + t.Logf("crash window: an interrupted run leaves %v with $0.00 recorded, "+ + "while the session had already spent whatever it spent", midFlight.unsettled) + + // And once it settles normally, the cost does land — so the loss is specific to + // the interrupted path, not to the accounting as a whole. + if a := auditLedger(root, "p"); !nearUSD(a.totalUSD, 7.00) { + t.Errorf("a session that returns must have its cost recorded, total=$%.2f", a.totalUSD) + } + _ = sum +} + +// TestDebugRetryKeepsTheStaleBase is the stale-base defect, isolated. +// +// Ensure reuses a LIVE worktree untouched, deliberately: a feat that reported +// `continue` has unfinished, uncommitted work in that tree and cutting a fresh one +// would discard it (tree.go). The cost of that choice is invisible until a peer +// lands: the retry keeps working against the base its FIRST dispatch was cut from, +// however far the base has moved since. +// +// The scenario forces the order rather than racing for it — gadget parks until +// widget's merge has actually been recorded — so the observation is deterministic. +// +// It pins the defect as it stands today: the retry demonstrably cannot see a peer +// that is already on the base. When the runner learns to re-sync a live tree before +// re-dispatching it, THIS TEST WILL FAIL — and that failure is the notification +// that the fix landed, not a regression. Re-point it at the new behavior then. +func TestDebugRetryKeepsTheStaleBase(t *testing.T) { + root, tr, out := debugRepo(t, debugPairPlan) + var gadgetTries int + h := debugHooks(t, root, tr, func(req SessionRequest) (SessionOutcome, error) { + switch req.Feat.Slug { + case "widget": + if err := deliverInTree(req.Dir, "widget", map[string]string{"widget.go": "package p // widget\n"}); err != nil { + return SessionOutcome{}, err + } + return paidOutcome(VerdictDone, 1), nil + default: + gadgetTries++ + if gadgetTries == 1 { + // Hold the first attempt open until widget has landed on the base, + // so the retry happens in a world where the base has moved. + if !tr.waitFor(trIntegrate, "widget", 30*time.Second) { + tr.fail(fmt.Errorf("widget never merged; the scenario could not be sequenced")) + } + return paidOutcome(VerdictContinue, 1), nil + } + // The retry. Whatever this session builds against is what the defect is + // about, so record it rather than judging it here — the assertion belongs + // on the test goroutine, and a session that calls t.Fatal would hang the run. + if _, err := os.Stat(filepath.Join(req.Dir, "widget.go")); err != nil { + tr.add(trNote, "gadget", "STALE: retry cannot see widget.go, already on the run base") + } else { + tr.add(trNote, "gadget", "FRESH: retry sees widget.go from the run base") + } + if err := deliverInTree(req.Dir, "gadget", map[string]string{"gadget.go": "package p // gadget\n"}); err != nil { + return SessionOutcome{}, err + } + return paidOutcome(VerdictDone, 1), nil + } + }) + + sum, err := Run(RunOptions{Root: root, Slug: "p", SquadLimit: 2, Hooks: h, Out: out}) + if err != nil { + t.Fatal(err) + } + tr.dump(t, root, "p", sum, out.String()) + + // The retry was handed the SAME directory as the first attempt — that is the + // mechanism, and it holds whether or not the staleness bites in this run. + dirs := tr.dirsFor("gadget") + if len(dirs) < 2 { + t.Fatalf("gadget should have been dispatched twice, got %d", len(dirs)) + } + if dirs[0] != dirs[1] { + t.Errorf("Ensure is documented to reuse a live tree; got %q then %q", dirs[0], dirs[1]) + } + // The consequence of that reuse, stated as the current behavior. + stale, fresh := 0, 0 + tr.mu.Lock() + for _, e := range tr.events { + if e.kind != trNote { + continue + } + if strings.HasPrefix(e.detail, "STALE") { + stale++ + } else { + fresh++ + } + } + tr.mu.Unlock() + if stale+fresh != 1 { + t.Fatalf("the retry should have made exactly one observation, got stale=%d fresh=%d", stale, fresh) + } + if fresh == 1 { + t.Fatal("the retry now sees the peer's merged work — the runner re-syncs live trees. " + + "That is the fix, not a regression: update this test to assert the new behavior.") + } + t.Log("CONFIRMED DEFECT: gadget's second session worked against the base its FIRST " + + "dispatch was cut from, with widget's merge invisible to it. Nothing in the loop " + + "moves a live tree forward, so a feat's staleness grows with every peer that lands.") + for _, e := range tr.errs { + t.Errorf("%v", e) + } +} + +// TestDebugGeneratedArtifactNoLongerBlocksTheMerge is the regression test for the +// single most expensive behavior the violet run exhibited. +// +// Every feat's brief tells the session to run `csdd graph analyze --strict` before +// declaring done, and the graph is a byte-stable BINARY blob the CLI rewrites from +// the whole workspace. Two feats in flight therefore always produce two different +// versions of a file git cannot merge — so with a squad, a conflict on it was not an +// unlucky collision between two feats that happened to touch the same code. It was +// the guaranteed outcome of the plan running at all. +// +// The real failure log named exactly this file first: +// +// warning: Cannot merge binary files: docs/graph/graph.json.gz +// CONFLICT (content): Merge conflict in docs/graph/graph.json.gz +// +// Before the fix this scenario produced three identical conflicts across four +// sessions costing $24.24 and delivered one of two feats. Integrate now settles a +// conflict whose every path is generated, so both feats land in one session each. +func TestDebugGeneratedArtifactNoLongerBlocksTheMerge(t *testing.T) { + root, tr, out := debugRepo(t, debugPairPlan) + // A NUL byte is what makes git treat the blob as binary and refuse to merge it, + // which is the property that matters — not the gzip framing around it. + graphBlob := func(feat string) string { return "\x1f\x8b\x08\x00" + feat + "\x00\x01\x02" } + + var gadgetTries int + h := debugHooks(t, root, tr, func(req SessionRequest) (SessionOutcome, error) { + slug := req.Feat.Slug + artifacts := map[string]string{ + "docs/graph/graph.json.gz": graphBlob(slug), + slug + ".go": "package p // " + slug + "\n", + } + if slug == "gadget" { + gadgetTries++ + if gadgetTries == 1 { + // Both feats are cut from the same base; hold gadget until widget's + // version of the graph is on it, so the merge order is fixed. + if err := deliverInTree(req.Dir, slug, artifacts); err != nil { + return SessionOutcome{}, err + } + if !tr.waitFor(trIntegrate, "widget", 30*time.Second) { + tr.fail(fmt.Errorf("widget never merged; the scenario could not be sequenced")) + } + return paidOutcome(VerdictDone, 11.16), nil + } + // The retry rebuilds the graph and tries again — and, on a tree still cut + // from the old base, conflicts exactly as before. + if err := deliverInTree(req.Dir, slug, artifacts); err != nil { + return SessionOutcome{}, err + } + return paidOutcome(VerdictDone, 4.22), nil + } + if err := deliverInTree(req.Dir, slug, artifacts); err != nil { + return SessionOutcome{}, err + } + return paidOutcome(VerdictDone, 4.64), nil + }) + + sum, err := Run(RunOptions{ + Root: root, Slug: "p", SquadLimit: 2, + // Bound the feat so a permanent conflict surfaces as `blocked` instead of + // grinding to the iteration cap — which is precisely what the bound is for. + FeatAttempts: 3, + Hooks: h, Out: out, + }) + if err != nil { + t.Fatal(err) + } + tr.dump(t, root, "p", sum, out.String()) + + conflicts := 0 + tr.mu.Lock() + for _, e := range tr.events { + if e.kind == trIntegrate && strings.Contains(e.detail, "CONFLICT") { + conflicts++ + } + } + tr.mu.Unlock() + if conflicts != 0 { + t.Errorf("a conflict confined to generated artifacts must be settled by the keeper, got %d", conflicts) + } + if !sum.Completed || sum.Steps != 2 { + t.Fatalf("both feats should land, one session each: completed=%v steps=%d (%s)", + sum.Completed, sum.Steps, sum.Reason) + } + if sum.Sessions != 2 { + t.Errorf("neither feat should need a retry, sessions=%d", sum.Sessions) + } + if sum.Gated != 0 { + t.Errorf("no `done` should be handed back, gated=%d", sum.Gated) + } + // The merged base carries both feats' code, and the generated blob is whichever + // one the base already had — stale on purpose, rebuilt by `csdd graph build`. + for _, f := range []string{"widget.go", "gadget.go"} { + if _, err := os.Stat(filepath.Join(root, f)); err != nil { + t.Errorf("%s must be on the run base: %v", f, err) + } + } + if !strings.Contains(out.String(), "auto-resolved") { + t.Error("the keeper settled a conflict on its own and must say so in the run log") + } + a := auditLedger(root, "p") + t.Logf("FIXED: 0 conflicts, %d session(s), $%.2f, %d of 2 feats delivered "+ + "(was: 3 conflicts, 4 sessions, $24.24, 1 of 2)", sum.Sessions, a.totalUSD, sum.Steps) + for _, e := range tr.errs { + t.Errorf("%v", e) + } +} + +// TestDebugAuthoredConflictStillRollsBack is the other half of the fix, and the one +// that keeps it honest: the keeper settles a conflict only when EVERY path in it is +// generated. Two feats that genuinely disagree about a source file must still be +// handed back, because there is no version of that file the runner is entitled to +// choose. +// +// It also pins what the handoff says. A conflict that mixes a generated artifact +// with an authored one names only the authored file: the session is told to rebase, +// reads the list literally, and sending it to reconcile a blob it does not own is +// how a correct handoff becomes wasted work. +func TestDebugAuthoredConflictStillRollsBack(t *testing.T) { + root, tr, out := debugRepo(t, debugPairPlan) + // Both feats edit the same source file — a real disagreement — and both also + // rebuild the graph, so the conflict is mixed. + writeRepoFile(t, root, "shared.go", "package p\n\nconst Owner = \"none\"\n") + commitAll(t, root, "add the contested file") + + var gadgetTries int + h := debugHooks(t, root, tr, func(req SessionRequest) (SessionOutcome, error) { + slug := req.Feat.Slug + artifacts := map[string]string{ + "docs/graph/graph.json.gz": "\x1f\x8b\x08\x00" + slug + "\x00\x01\x02", + "shared.go": "package p\n\nconst Owner = \"" + slug + "\"\n", + } + if slug == "gadget" { + gadgetTries++ + if err := deliverInTree(req.Dir, slug, artifacts); err != nil { + return SessionOutcome{}, err + } + if gadgetTries == 1 && !tr.waitFor(trIntegrate, "widget", 30*time.Second) { + tr.fail(fmt.Errorf("widget never merged; the scenario could not be sequenced")) + } + return paidOutcome(VerdictDone, 1), nil + } + if err := deliverInTree(req.Dir, slug, artifacts); err != nil { + return SessionOutcome{}, err + } + return paidOutcome(VerdictDone, 1), nil + }) + + sum, err := Run(RunOptions{Root: root, Slug: "p", SquadLimit: 2, FeatAttempts: 2, Hooks: h, Out: out}) + if err != nil { + t.Fatal(err) + } + tr.dump(t, root, "p", sum, out.String()) + + var details []string + tr.mu.Lock() + for _, e := range tr.events { + if e.kind == trIntegrate && strings.Contains(e.detail, "CONFLICT") { + details = append(details, e.detail) + } + } + tr.mu.Unlock() + if len(details) == 0 { + t.Fatal("two feats writing different contents to the same source file must still conflict") + } + for _, d := range details { + if !strings.Contains(d, "shared.go") { + t.Errorf("the conflict must name the contested source file, got %q", d) + } + if strings.Contains(d, "docs/graph/") { + t.Errorf("the handoff must not send the session after a generated artifact, got %q", d) + } + } + if sum.Gated == 0 { + t.Error("an authored conflict must be handed back as partial work") + } + if sum.Steps != 1 { + t.Errorf("only the first feat can land on a contested file, steps=%d", sum.Steps) + } + // The base must not be left mid-merge by the rollback. Untracked runner state + // (.csdd/, the journal) is not a merge leftover, so the check is specifically + // for unmerged paths and an in-progress merge. + if left, err := runGit(root, "diff", "--name-only", "--diff-filter=U"); err != nil || len(gitLines(left)) > 0 { + t.Errorf("no path may be left unmerged after a rollback, got %q err=%v", left, err) + } + if _, err := os.Stat(filepath.Join(root, ".git", "MERGE_HEAD")); !os.IsNotExist(err) { + t.Errorf("the base must not be left mid-merge, MERGE_HEAD stat err = %v", err) + } + for _, e := range tr.errs { + t.Errorf("%v", e) + } +} + +// TestDebugAttemptBoundStopsARunawayFeat proves the one guard that stands between a +// conflict loop and the iteration cap. `continue` resets the stall guard, so a feat +// whose `done` is refused forever is bounded only by FeatAttempts — and the trace +// should show it stopping there, not at 100 sessions. +func TestDebugAttemptBoundStopsARunawayFeat(t *testing.T) { + root, tr, out := debugRepo(t, debugSoloPlan) + h := debugHooks(t, root, tr, func(req SessionRequest) (SessionOutcome, error) { + // Always claims done, never leaves the artifacts: the gate refuses every time. + return paidOutcome(VerdictDone, 5.00), nil + }) + + sum, err := Run(RunOptions{Root: root, Slug: "p", FeatAttempts: 3, MaxIterations: 50, Hooks: h, Out: out}) + if err != nil { + t.Fatal(err) + } + tr.dump(t, root, "p", sum, out.String()) + + if sum.Sessions != 3 { + t.Errorf("the feat must be handed out exactly FeatAttempts times, got %d", sum.Sessions) + } + if len(sum.Blocked) != 1 || sum.Blocked[0] != "widget" { + t.Errorf("the exhausted feat must be surfaced as blocked, got %v", sum.Blocked) + } + if sum.Outcome != OutcomeBlocked { + t.Errorf("outcome should be blocked (%d), got %d", OutcomeBlocked, sum.Outcome) + } + if a := auditLedger(root, "p"); !nearUSD(a.totalUSD, 15.00) { + t.Errorf("three refused sessions still cost three sessions, total=$%.2f", a.totalUSD) + } +} diff --git a/internal/plan/ledger.go b/internal/plan/ledger.go index 23eaa8c..3c2c549 100644 --- a/internal/plan/ledger.go +++ b/internal/plan/ledger.go @@ -148,6 +148,9 @@ type SessionRecord struct { NumTurns int `json:"num_turns,omitempty"` Tokens SessionTokens `json:"tokens"` Models []string `json:"models,omitempty"` + // ByModel splits Tokens across the models that billed them. It is what makes a + // cost report able to say WHERE a session spent, rather than only how much. + ByModel []ModelTokens `json:"by_model,omitempty"` } // newSessionRecord stamps one attempt's outcome and cost into a record. @@ -165,6 +168,7 @@ func newSessionRecord(feat string, iter, attempt int, status, detail string, m S NumTurns: m.NumTurns, Tokens: m.Tokens, Models: m.Models, + ByModel: m.ByModel, } } diff --git a/internal/plan/metrics.go b/internal/plan/metrics.go index a9ad6fb..9d779c6 100644 --- a/internal/plan/metrics.go +++ b/internal/plan/metrics.go @@ -25,6 +25,24 @@ type SessionMetrics struct { // its work (an orchestrator model plus the implementer sub-agent's own), so // which models ran is part of comparing one run against another (R9.3). Models []string + // ByModel is what each model billed, sorted by model ID. + // + // It exists because Tokens alone cannot answer the question a cost report is + // for. The plan loop deliberately tiers its work — an orchestrator that decides + // and sub-agents that execute on a cheaper model — and two sessions with nearly + // identical token counts have been observed costing 2.4× different amounts + // purely because one delegated and the other did not. Which tier the tokens + // landed on is therefore the single most actionable figure in the record, and + // until now the runner parsed `modelUsage` only far enough to read the KEYS and + // threw the counts away. + ByModel []ModelTokens +} + +// ModelTokens is one model's share of a session. +type ModelTokens struct { + Model string `json:"model"` + Tokens SessionTokens `json:"tokens"` + CostUSD float64 `json:"cost_usd,omitempty"` } // SessionTokens is the token usage a session billed, split the way the API @@ -87,13 +105,68 @@ func parseSessionMetrics(raw []byte) SessionMetrics { CacheCreation: ev.Usage.CacheCreation, }, } - for name := range ev.ModelUsage { + for name, raw := range ev.ModelUsage { m.Models = append(m.Models, name) + m.ByModel = append(m.ByModel, ModelTokens{Model: name, Tokens: modelTokens(raw), CostUSD: modelCost(raw)}) } sort.Strings(m.Models) // deterministic: the record is diffed across runs + sort.Slice(m.ByModel, func(i, j int) bool { return m.ByModel[i].Model < m.ByModel[j].Model }) return m } +// modelUsageKeys maps each field to every spelling the CLI has been observed to +// use for it. `modelUsage` is assembled by the Claude CLI rather than returned by +// the API, and the two do not agree on case — the envelope's own `usage` block is +// snake_case while CLI-side aggregations are commonly camelCase. Reading both +// costs nothing and means a rename upstream degrades to a zero in one column +// instead of silently zeroing the whole breakdown. +var modelUsageKeys = map[string][]string{ + "input": {"input_tokens", "inputTokens"}, + "output": {"output_tokens", "outputTokens"}, + "cache_read": {"cache_read_input_tokens", "cacheReadInputTokens"}, + "cache_creation": {"cache_creation_input_tokens", "cacheCreationInputTokens"}, + "cost": {"cost_usd", "costUSD", "totalCostUSD", "total_cost_usd"}, +} + +// modelTokens reads one model's entry out of `modelUsage`. Anything unparseable +// yields zeros: this is bookkeeping, and a breakdown that cannot be read must not +// take down a session that otherwise succeeded. +func modelTokens(raw json.RawMessage) SessionTokens { + var m map[string]any + if json.Unmarshal(raw, &m) != nil { + return SessionTokens{} + } + return SessionTokens{ + Input: pickNum(m, modelUsageKeys["input"]), + Output: pickNum(m, modelUsageKeys["output"]), + CacheRead: pickNum(m, modelUsageKeys["cache_read"]), + CacheCreation: pickNum(m, modelUsageKeys["cache_creation"]), + } +} + +func modelCost(raw json.RawMessage) float64 { + var m map[string]any + if json.Unmarshal(raw, &m) != nil { + return 0 + } + for _, k := range modelUsageKeys["cost"] { + if f, ok := m[k].(float64); ok { + return f + } + } + return 0 +} + +// pickNum returns the first key that holds a number, as an int. +func pickNum(m map[string]any, keys []string) int { + for _, k := range keys { + if f, ok := m[k].(float64); ok { + return int(f) + } + } + return 0 +} + // SessionOutcome is everything one session produced: the verdict it DECLARED and // what it COST. The two are separate on purpose — the verdict is the model's // claim (which the loop now gates, R10) while the metrics are observed fact, and diff --git a/internal/plan/runner.go b/internal/plan/runner.go index 255e206..46bcb39 100644 --- a/internal/plan/runner.go +++ b/internal/plan/runner.go @@ -92,7 +92,7 @@ type RunOptions struct { // is omitted. The CLI defaults these to opus / high. Model string Effort string - MaxIterations int // sessions the run may spend; default 100 + MaxIterations int // sessions the run may spend; default 30 // Stall ends the run after this many consecutive iterations that FAILED (a // session error or an unparseable verdict) with no forward motion. Honest // partial work (`continue`) resets it, so the stall guard catches a broken @@ -106,7 +106,7 @@ type RunOptions struct { // `continue`, and `continue` resets the stall guard — so without this bound a // session that is confidently wrong about being finished would be re-dispatched // until the global iteration cap, turning a silent false-done into a silent - // infinite loop. The two ship together. Default 8. + // infinite loop. The two ship together. Default 4. FeatAttempts int // SessionIdle bounds how long a session may make no progress at all — no // output on its event stream and no CPU anywhere in its process group — before @@ -125,8 +125,13 @@ type RunOptions struct { // ledger, the journal and the run's bookkeeping are written in exactly the order // they were before — only the sessions themselves overlap. SquadLimit int - Out io.Writer - Hooks Hooks + // WorktreeEntry is the CLAUDE.md written into each feat's worktree, replacing + // the repository's own for the life of the run. Empty leaves the repository's + // file in place. The CLI fills it from the plan-session template; the runner + // only carries it to the tree keeper. + WorktreeEntry string + Out io.Writer + Hooks Hooks } // Run outcomes, chosen so the CLI can surface them as distinct exit codes. @@ -193,7 +198,7 @@ func Run(opts RunOptions) (RunSummary, error) { if err != nil { return RunSummary{}, err } - opts.Hooks.Trees = gitTrees{root: opts.Root, slug: opts.Slug, base: base} + opts.Hooks.Trees = gitTrees{root: opts.Root, slug: opts.Slug, base: base, logf: logf, entry: opts.WorktreeEntry} h = opts.Hooks logf("worktrees: one per feat, branched from and merged back into %s", base) if specsAreIgnored(opts.Root) { @@ -1097,13 +1102,13 @@ func fillRunDefaults(opts *RunOptions) { opts.SessionBudget = 0 } if opts.MaxIterations <= 0 { - opts.MaxIterations = 100 + opts.MaxIterations = 30 } if opts.Stall <= 0 { opts.Stall = 10 } if opts.FeatAttempts <= 0 { - opts.FeatAttempts = 8 + opts.FeatAttempts = 4 } if opts.Out == nil { opts.Out = io.Discard diff --git a/internal/plan/tree.go b/internal/plan/tree.go index aa97aa3..19734b8 100644 --- a/internal/plan/tree.go +++ b/internal/plan/tree.go @@ -104,6 +104,20 @@ type gitTrees struct { root string // the repository root — also the tree the run integrates into slug string // plan slug, so two plans in one repo never share a branch base string // the branch the run integrates into (resolved at preflight) + // logf reports what the keeper decided on its own. Only the generated-artifact + // resolution uses it, and it must: silently settling a conflict git refused is + // exactly the kind of thing an operator has to be able to see in the run log. + // Nil in tests that construct the keeper directly. + logf func(string, ...any) + // entry is the lean CLAUDE.md written into each worktree, or "" to leave the + // repository's own in place. See writeEntryDoc. + entry string +} + +func (g gitTrees) say(format string, a ...any) { + if g.logf != nil { + g.logf(format, a...) + } } // treesDir is where a plan's worktrees live: under the runner's own transient state @@ -166,7 +180,7 @@ func (g gitTrees) Ensure(feat string) (string, error) { // may predate the marker existing, or something may have cleaned .csdd/ out. // Skipping this because the tree is "already there" is how the isolation goes // back to being a fiction through the one path that does not create it. - return path, markWorkspace(path) + return path, g.prepare(path) } // Clear both kinds of debris a killed run leaves: a registration git still // holds, and the directory itself. `git worktree add` refuses a target that is @@ -183,7 +197,7 @@ func (g gitTrees) Ensure(feat string) (string, error) { if out, err := runGit(g.root, "worktree", "add", path, branch); err != nil { return "", fmt.Errorf("could not restore the worktree for %s from %s: %w: %s", feat, branch, err, out) } - return path, markWorkspace(path) + return path, g.prepare(path) } base, err := g.baseCommit() if err != nil { @@ -192,7 +206,56 @@ func (g gitTrees) Ensure(feat string) (string, error) { if out, err := runGit(g.root, "worktree", "add", "-b", branch, path, base); err != nil { return "", fmt.Errorf("could not create the worktree for %s: %w: %s", feat, err, out) } - return path, markWorkspace(path) + return path, g.prepare(path) +} + +// prepare makes a freshly-created or restored worktree usable by a session: it +// marks it as its own workspace root and, when the run carries one, swaps in the +// lean plan-session CLAUDE.md. +func (g gitTrees) prepare(path string) error { + if err := markWorkspace(path); err != nil { + return err + } + g.writeEntryDoc(path) + return nil +} + +// writeEntryDoc replaces the worktree's CLAUDE.md with the plan-session one. +// +// Every turn of every session re-reads that file, and the repository's version is +// written for interactive development with a human at the gate — so in a plan run +// it is both the largest fixed cost in the context and the wrong instruction set. +// Swapping it per worktree fixes both without touching the file a human works +// against. +// +// The swap is made invisible to git with `update-index --skip-worktree`, which is +// per-worktree state: the session cannot commit it, the merge cannot carry it back, +// and `git status` stays clean so the uncommitted-work check is unaffected. That +// only works on a TRACKED file, so an untracked CLAUDE.md is left alone rather than +// created — writing one would show up as untracked work and fail the very check +// that protects a delivered feat. +// +// Every failure here is reported and swallowed. A session reading the repository's +// CLAUDE.md is more expensive, never incorrect, and losing the run over an +// optimization would be the worse trade. +func (g gitTrees) writeEntryDoc(path string) { + if g.entry == "" { + return + } + rel := paths.EntryFile + if _, err := runGit(path, "ls-files", "--error-unmatch", "--", rel); err != nil { + g.say(" · %s is not tracked; the plan-session CLAUDE.md was not injected", rel) + return + } + if out, err := runGit(path, "update-index", "--skip-worktree", "--", rel); err != nil { + g.say(" · could not detach %s from the index (%s); leaving the repository's own in place", rel, strings.TrimSpace(out)) + return + } + if err := os.WriteFile(filepath.Join(path, rel), []byte(g.entry), 0o644); err != nil { + // Put the tracked copy back rather than leave the tree half-swapped. + _, _ = runGit(path, "update-index", "--no-skip-worktree", "--", rel) + g.say(" · could not write the plan-session %s (%v); leaving the repository's own in place", rel, err) + } } // markWorkspace creates the .csdd/ directory that makes a worktree resolve as a @@ -276,17 +339,108 @@ func (g gitTrees) Integrate(feat string) error { return &UncommittedWorkError{Feat: feat, Branch: branch, Paths: withoutRunnerState(dirty)} } out, err := runGit(g.root, "merge", "--no-ff", "--no-edit", - "-m", fmt.Sprintf("Merge feat %s of plan %s", feat, g.slug), branch) + "-m", mergeMessage(feat, g.slug), branch) if err == nil { return nil } - conflicts, _ := runGit(g.root, "diff", "--name-only", "--diff-filter=U") + raw, _ := runGit(g.root, "diff", "--name-only", "--diff-filter=U") + conflicts := gitLines(raw) + generated, authored := partitionConflicts(conflicts) + // A conflict only in generated files is not a disagreement about the work — it + // is two sessions having rebuilt the same derived artifact from different trees. + // Settle it here rather than handing the feat back: the alternative costs an + // entire session to re-do work that was already finished and correct. + if len(conflicts) > 0 && len(authored) == 0 { + if err := g.resolveGenerated(generated); err == nil { + g.say(" ↺ %s: auto-resolved %d generated artifact(s) in the merge (%s) — "+ + "they are rebuilt by `csdd graph build`, not authored", + feat, len(generated), strings.Join(generated, ", ")) + return nil + } + // Resolution failed; fall through and roll back, which is always safe. + } // Roll back before returning: leaving the base half-merged would make the next // feat's worktree, and the next merge, operate on a tree nobody chose. if _, abortErr := runGit(g.root, "merge", "--abort"); abortErr != nil { return fmt.Errorf("merging %s failed and could not be rolled back: %v: %s", branch, abortErr, out) } - return &MergeConflictError{Feat: feat, Branch: branch, Files: gitLines(conflicts), Detail: out} + // Report the files the SESSION has to deal with. A generated artifact in this + // list would send the next session to rebuild something it does not own, and the + // handoff is read literally. + files := authored + if len(files) == 0 { + files = conflicts + } + return &MergeConflictError{Feat: feat, Branch: branch, Files: files, Detail: out} +} + +// generatedPrefixes are the repository subtrees csdd GENERATES rather than +// authors, in git's forward-slash spelling. `docs/graph/` is the only one that is +// also committed: paths.go calls it "the only generated subtree" and CLAUDE.md +// makes the CLI its only writer, while `.csdd/` is transient and never tracked. +// +// The distinction is what makes auto-resolution safe. A generated file has no +// authorial intent to lose — whichever side wins, the content is wrong the moment +// the merge lands and right again the next time it is rebuilt. An authored file is +// the opposite, so it is never touched here. +var generatedPrefixes = []string{paths.DocsSeg + "/" + paths.DocsGraphSeg + "/"} + +// partitionConflicts splits conflicting paths into the generated ones this keeper +// may settle by itself and the authored ones only the session can. +func partitionConflicts(files []string) (generated, authored []string) { + for _, f := range files { + p := strings.TrimSpace(filepath.ToSlash(f)) + if p == "" { + continue + } + if isGeneratedPath(p) { + generated = append(generated, p) + continue + } + authored = append(authored, p) + } + return generated, authored +} + +func isGeneratedPath(p string) bool { + for _, prefix := range generatedPrefixes { + if strings.HasPrefix(p, prefix) { + return true + } + } + return false +} + +// resolveGenerated finishes an in-progress merge by taking the base's copy of every +// conflicting generated path. +// +// Taking `--ours` rather than the feat's version is deliberate and, for a derived +// artifact, not really a choice: neither side describes the merged tree, so the only +// honest state is "stale until rebuilt", and preferring the base keeps the run's +// history reading as one line of development. A path the base deleted cannot be +// checked out, so it is removed instead — the same resolution, spelled the way git +// needs it. +func (g gitTrees) resolveGenerated(paths []string) error { + for _, p := range paths { + if _, err := runGit(g.root, "checkout", "--ours", "--", p); err != nil { + if out, rmErr := runGit(g.root, "rm", "--force", "--quiet", "--", p); rmErr != nil { + return fmt.Errorf("could not resolve the generated path %s: %v: %s", p, rmErr, out) + } + continue + } + if out, err := runGit(g.root, "add", "--", p); err != nil { + return fmt.Errorf("could not stage the resolved path %s: %v: %s", p, err, out) + } + } + // Anything still unmerged means the partition missed a path; completing the + // merge then would commit a conflicted tree, which is worse than rolling back. + if left, err := runGit(g.root, "diff", "--name-only", "--diff-filter=U"); err == nil && len(gitLines(left)) > 0 { + return fmt.Errorf("paths are still unmerged after resolving generated artifacts: %s", strings.TrimSpace(left)) + } + if out, err := runGit(g.root, "commit", "--no-edit"); err != nil { + return fmt.Errorf("could not complete the merge after resolving generated artifacts: %v: %s", err, out) + } + return nil } // Discard removes the feat's worktree and prunes git's administrative record of it. diff --git a/internal/plan/validate.go b/internal/plan/validate.go index 716bbb6..777f24a 100644 --- a/internal/plan/validate.go +++ b/internal/plan/validate.go @@ -122,6 +122,10 @@ func ValidatePlan(doc *PlanDoc, root string) []validator.Issue { } } issues = append(issues, adrRefIssues(f, adrs)...) + // The design the spec-author writes must acknowledge the governing ADRs the + // plan bound this feat to — see designConformance. This is the mechanical + // backstop for the trimmed brief, which no longer inlines ADR bodies. + issues = append(issues, designConformance(root, f, adrs)...) } // docs/adr well-formedness (R3.2), surfaced only WHERE the directory exists. @@ -180,6 +184,50 @@ func glossaryIssues(root string, doc *PlanDoc) []validator.Issue { return out } +// designConformance is the mechanical backstop that makes it safe to trim the +// ADR bodies out of the feat brief. The brief used to inline each governing ADR's +// body so a design could not silently ignore the decisions it was bound to; with +// the bodies gone (fetched instead via `csdd graph explain adr:`), this +// check holds the design to the same standard — for every adr: the plan +// bound the feat to, the authored specs//design.md must cite it. +// +// It is silent when design.md is not yet generated (a plan validated before any +// spec is authored has nothing to check), silent for feats with no governing +// ADRs, and silent for ADRs already reported malformed or broken by adrRefIssues +// — only well-formed, resolvable governors are conformance-checked here. The +// match is permissive (a bare substring of the slug counts): the check exists to +// catch a design that ignores a governor entirely, not to police citation +// format, and a gate that cries wolf is a gate someone turns off. +func designConformance(root string, f Feat, adrs *ADRSet) []validator.Issue { + if len(f.ADRRefs) == 0 { + return nil + } + body, err := os.ReadFile(filepath.Join(paths.Specs(root), f.Slug, "design.md")) + if err != nil { + return nil // design not authored yet — nothing to conform + } + design := string(body) + var out []validator.Issue + for _, slug := range f.ADRRefs { + if !ValidADRSlug(slug) { + continue // already reported by adrRefIssues + } + if _, res := adrs.Resolve(slug); res != ADRResolved { + continue // broken/ambiguous already reported + } + if strings.Contains(design, slug) { + continue + } + out = append(out, validator.Issue{ + File: "specs/" + f.Slug + "/design.md", + Msg: "design.md does not cite governing ADR 'adr:" + slug + "' (declared in this feat's Refs) — " + + "the brief no longer inlines ADR bodies, so the design must reference each governor; " + + "cite it as `adr:" + slug + "`, or run `csdd graph explain adr:" + slug + "` for the body", + }) + } + return out +} + // featCycles returns the dependency cycles among feats as slug lists, using // Tarjan SCC so each cycle is reported once, deterministically. func featCycles(doc *PlanDoc) [][]string { diff --git a/internal/plan/validate_test.go b/internal/plan/validate_test.go index 35b1ad8..30d83f2 100644 --- a/internal/plan/validate_test.go +++ b/internal/plan/validate_test.go @@ -199,3 +199,52 @@ status: draft } } } + +// TestValidateDesignConformance pins the mechanical backstop that lets the brief +// drop ADR bodies: a feat bound to adr: must have its authored design.md +// cite that governor. Silent when design.md is absent (plan validated before any +// spec is authored) and clears once the design references the slug. +func TestValidateDesignConformance(t *testing.T) { + const planMD = `--- +name: p +status: draft +--- +## Feats + +| # | Feat | Objective | Depends | Milestone | (P) | Refs | +|---|------|-----------|---------|-----------|-----|------| +| 1 | a | A | — | M1 | | adr:auth | + +## Quality Gates + +- verify: make check +` + root := setupWorkspace(t, "p", planMD) + writeADRs(t, root, map[string]string{ + "0001-auth.md": "# Use OAuth for authentication\n\nContext. Decision. Why.\n", + }) + + // design.md authored without citing the governing ADR → finding. + writeFile(t, filepath.Join(root, "specs", "a", "design.md"), + "# Design\n\nNo mention of the decision.\n") + issues := validateStrings(t, root, "p") + if !msgsContain(issues, "does not cite governing ADR 'adr:auth'") { + t.Errorf("missing ADR-conformance finding: %v", issues) + } + + // Once the design cites the slug, the finding clears. + writeFile(t, filepath.Join(root, "specs", "a", "design.md"), + "# Design\n\nHonors adr:auth for the login boundary.\n") + if got := validateStrings(t, root, "p"); msgsContain(got, "does not cite governing ADR") { + t.Errorf("citing the ADR should clear the finding: %v", got) + } + + // No design.md authored yet → silent (plan validated before spec authoring). + root2 := setupWorkspace(t, "p", planMD) + writeADRs(t, root2, map[string]string{ + "0001-auth.md": "# Use OAuth for authentication\n\nContext. Decision. Why.\n", + }) + if got := validateStrings(t, root2, "p"); msgsContain(got, "does not cite governing ADR") { + t.Errorf("absent design.md must be silent: %v", got) + } +} diff --git a/internal/plan/verify.go b/internal/plan/verify.go new file mode 100644 index 0000000..ae404ec --- /dev/null +++ b/internal/plan/verify.go @@ -0,0 +1,182 @@ +package plan + +import ( + "fmt" + "path/filepath" + "strings" +) + +// Proving a feat was delivered. +// +// "Delivered" is asserted by four independent records, and they can disagree — +// which is the whole reason this exists. In a real workspace (violet, plan +// `frontend-design-refresh`) `plan status` reported two feats done while git held +// exactly one merge commit: the other had been finished by hand before the run and +// imported by reconcileLedgerFromDisk. Both readings are correct and they mean +// completely different things, and nothing in the CLI could tell them apart. +// +// The four records, and what each one alone cannot prove: +// +// - the LEDGER says a `done` verdict cleared the gate — but not that anything +// reached the base, and it is regenerable transient state; +// - the ARTIFACTS (spec.json, tasks.md, test-report.json) are the runner's own +// definition of delivered (gateDone) — but they are files a session can write +// without the work landing anywhere; +// - a MERGE COMMIT is the only proof the code is on the branch the run +// integrates into; +// - a LIVE WORKTREE is proof it is not finished: a delivered feat has its tree +// discarded, so one that survives holds work nobody committed. +// +// Verify reads all four and reports only where they contradict each other. + +// mergeMessage is the subject Integrate writes for a feat's merge commit, and the +// string Verify greps for. They share it so a reworded merge message can never +// silently turn every delivered feat into an unverifiable one. +func mergeMessage(feat, slug string) string { + return fmt.Sprintf("Merge feat %s of plan %s", feat, slug) +} + +// Delivery states a feat can be in. They are reported, not judged: only Findings +// mark something as wrong. +const ( + // DeliveredMerged is the fully corroborated state: all four records agree. + DeliveredMerged = "delivered" + // DeliveredOffRun is a feat whose artifacts are complete but which carries no + // merge commit — finished before or outside the run and imported into the + // ledger. Legitimate, and worth naming rather than reporting as "delivered". + DeliveredOffRun = "delivered-off-run" + // InProgress is an undelivered feat that still holds a live worktree. + InProgress = "in-progress" + // Pending is an undelivered feat nothing has started. + Pending = "pending" +) + +// FeatVerification is what each record says about one feat. +type FeatVerification struct { + Feat string `json:"feat"` + State string `json:"state"` + LedgerDone bool `json:"ledger_done"` + Artifacts bool `json:"artifacts_complete"` + ArtifactGaps []string `json:"artifact_gaps,omitempty"` + Merged bool `json:"merged"` + MergeCommit string `json:"merge_commit,omitempty"` + LiveWorktree bool `json:"live_worktree"` + // Findings are contradictions between the records. A feat with none is + // consistent, whatever state it is in. + Findings []string `json:"findings,omitempty"` +} + +// VerifyReport is the whole plan's delivery evidence. +type VerifyReport struct { + Plan string `json:"plan"` + // Branch is what the merge check was run against, or "" when the workspace is + // not a git repository — in which case Merged is unknowable and never a finding. + Branch string `json:"branch,omitempty"` + Git bool `json:"git"` + Feats []FeatVerification `json:"feats"` + OK bool `json:"ok"` +} + +// Verify cross-checks every feat in the plan against all four records. +func Verify(root string, doc *PlanDoc) VerifyReport { + rep := VerifyReport{Plan: doc.Slug, OK: true} + ledger := LoadLedger(root, doc.Slug) + trees := gitTrees{root: root, slug: doc.Slug} + if branch, ok := currentBranch(root); ok { + rep.Git, rep.Branch = true, branch + } + + for _, f := range doc.Feats { + v := FeatVerification{Feat: f.Slug, LedgerDone: ledger.Done(f.Slug)} + + gaps := gateDone(root, f) + v.Artifacts = len(gaps) == 0 + for _, g := range gaps { + v.ArtifactGaps = append(v.ArtifactGaps, g.check+": "+g.detail) + } + if rep.Git { + v.MergeCommit, v.Merged = mergeCommitFor(root, f.Slug, doc.Slug) + } + if live, err := liveWorktree(trees.path(f.Slug)); err == nil { + v.LiveWorktree = live + } + + switch { + case v.LedgerDone && !v.Artifacts: + // The serious one. The ledger is what the sequencer trusts, so a feat + // marked done without its artifacts is skipped forever by every later + // run while the work it stands for does not exist. + v.State = DeliveredOffRun + v.Findings = append(v.Findings, fmt.Sprintf( + "the ledger marks %s delivered but its artifacts do not hold (%s); "+ + "every later run will skip it", f.Slug, strings.Join(v.ArtifactGaps, "; "))) + case v.LedgerDone && v.Merged: + v.State = DeliveredMerged + case v.LedgerDone: + // Artifacts complete, no merge commit: finished outside this run. Only a + // finding when git could actually have shown one. + v.State = DeliveredOffRun + if rep.Git { + v.Findings = append(v.Findings, fmt.Sprintf( + "%s is marked delivered and its artifacts hold, but no merge commit for it "+ + "exists on %s — it was delivered outside this run, so the code on the branch "+ + "is not evidence of it", f.Slug, rep.Branch)) + } + case v.Artifacts && v.Merged: + // Delivered by every other measure, invisible to the ledger — the next + // run will hand it out and redo finished work. + v.State = DeliveredMerged + v.Findings = append(v.Findings, fmt.Sprintf( + "%s is merged and its artifacts hold, but the ledger does not mark it delivered — "+ + "the next `plan run` will work it again", f.Slug)) + case v.LiveWorktree: + v.State = InProgress + default: + v.State = Pending + } + + // A delivered feat's tree is discarded once its work is on the base, so one + // that survives is either uncommitted work or a leak. Either way it is the + // difference between "done" and "done and cleaned up". + if v.LedgerDone && v.LiveWorktree { + v.Findings = append(v.Findings, fmt.Sprintf( + "%s is marked delivered but its worktree is still live at %s — "+ + "anything uncommitted in it is not on the branch", + f.Slug, filepath.ToSlash(trees.path(f.Slug)))) + } + + if len(v.Findings) > 0 { + rep.OK = false + } + rep.Feats = append(rep.Feats, v) + } + return rep +} + +// currentBranch reports the branch the merge check runs against. It is HEAD rather +// than the run's recorded base because the base lives only in the runner's memory, +// and the question `plan verify` answers is "is the work on the branch I am on". +func currentBranch(root string) (string, bool) { + out, err := runGit(root, "rev-parse", "--abbrev-ref", "HEAD") + if err != nil { + return "", false + } + b := strings.TrimSpace(out) + if b == "" || b == "HEAD" { // detached: no branch to reason about + return "", false + } + return b, true +} + +// mergeCommitFor finds the merge Integrate wrote for a feat, if it is reachable +// from HEAD. --fixed-strings so a slug containing regex metacharacters cannot +// match the wrong commit or none at all. +func mergeCommitFor(root, feat, slug string) (string, bool) { + out, err := runGit(root, "log", "--merges", "--fixed-strings", + "--grep", mergeMessage(feat, slug), "--format=%h", "-n", "1") + if err != nil { + return "", false + } + h := strings.TrimSpace(out) + return h, h != "" +} diff --git a/internal/templater/templater.go b/internal/templater/templater.go index 66b4b48..c21aa29 100644 --- a/internal/templater/templater.go +++ b/internal/templater/templater.go @@ -220,3 +220,17 @@ func WorkflowTemplateFiles(efs fs.FS) (map[string]string, error) { return out, nil } + +// PlanEntry is the lean CLAUDE.md the plan runner writes into each feat's +// worktree. It is static: the file has no placeholders, because anything +// interpolated per feat would be a byte the session pays for on every turn. +// +// It exists as a separate template rather than a variant of the root CLAUDE.md +// because the two address different readers. The root file governs INTERACTIVE +// development, where a human authorizes each phase gate; a plan session has no +// human and must approve its own phases, so the root file's rules are not merely +// verbose there — they are wrong, and the mission brief has to spend a section +// undoing them. +func PlanEntry(efs fs.FS) (string, error) { + return Static(efs, "templates/plan/CLAUDE.md.tmpl") +} diff --git a/internal/templater/templater_test.go b/internal/templater/templater_test.go index fd51dfd..ffdf63f 100644 --- a/internal/templater/templater_test.go +++ b/internal/templater/templater_test.go @@ -187,7 +187,7 @@ func TestShippedArtifactsPresent(t *testing.T) { t.Fatal(err) } for _, name := range []string{ - "implementer.md", "code-reviewer.md", "security-reviewer.md", "quality-gate.md", + "implementer.md", "spec-author.md", "code-reviewer.md", "security-reviewer.md", "quality-gate.md", } { if _, ok := agents[name]; !ok { t.Errorf("shipped agents missing %q", name) @@ -287,9 +287,62 @@ func TestImplementerAgentShipped(t *testing.T) { } } -// TestImplementerAgentEncodesDiscipline covers requirements 2.1–2.6, 4.1–4.3: the -// agent body encodes the per-task discipline, defers language specifics to -// steering/skills, and documents how to specialize it. +// TestSpecAuthorAgentShipped covers the spec-author agent: it ships alongside the +// other agents, drafts (never approves) one spec phase on the cheap sonnet model, +// scaffolds via `csdd spec generate`, and defers approval to the orchestrator. +func TestSpecAuthorAgentShipped(t *testing.T) { + agents, err := AgentFiles(FS) + if err != nil { + t.Fatal(err) + } + body, ok := agents["spec-author.md"] + if !ok { + t.Fatal("AgentFiles is missing spec-author.md") + } + // Least-privilege tools: it writes spec artifacts, so Edit/Write/Bash are in, + // but it must not approve or implement. + for _, want := range []string{ + "name: spec-author", + "tools: Read, Grep, Glob, Edit, Write, Bash", + "model: sonnet", + } { + if !strings.Contains(body, want) { + t.Errorf("spec-author.md frontmatter missing %q", want) + } + } + if !strings.Contains(body, "description:") { + t.Error("spec-author.md missing a description") + } +} + +// TestSpecAuthorAgentEncodesDiscipline pins the authoring discipline and the +// hand-off to the orchestrator: scaffold via generate, consult the graph, write +// to EARS/traceability, validate, and never approve. +func TestSpecAuthorAgentEncodesDiscipline(t *testing.T) { + agents, err := AgentFiles(FS) + if err != nil { + t.Fatal(err) + } + body := agents["spec-author.md"] + for _, want := range []string{ + "csdd spec generate", // scaffold, not hand-write + "graph", // consult the knowledge graph + "EARS", // requirements contract + "Requirements Traceability", // design contract + "csdd spec validate", // self-validate before reporting + "do not approve", // the orchestrator owns approval + } { + if !strings.Contains(body, want) { + t.Errorf("spec-author.md should reference %q", want) + } + } + // It names the approve command only to forbid it — never to instruct calling it. + for _, bad := range []string{"csdd spec approve ", "spec approve "} { + if strings.Contains(body, bad) { + t.Errorf("spec-author.md must not instruct approving its own phase: %q", bad) + } + } +} func TestImplementerAgentEncodesDiscipline(t *testing.T) { agents, err := AgentFiles(FS) if err != nil { diff --git a/internal/templater/templates/agents/code-reviewer.md.tmpl b/internal/templater/templates/agents/code-reviewer.md.tmpl index 472387e..6b6391e 100644 --- a/internal/templater/templates/agents/code-reviewer.md.tmpl +++ b/internal/templater/templates/agents/code-reviewer.md.tmpl @@ -22,8 +22,15 @@ ask the orchestrator which range to review (`git diff`, a commit, a branch). 3. Adherence to the spec — does the change match `requirements.md` and the `design.md` boundaries? 4. Regressions — could this break behavior listed as "MUST NOT change"? 5. Security-sensitive mistakes — secrets in code/logs, missing authz checks, unvalidated input at boundaries. -6. Overengineering — speculative abstraction, unused options, scope creep beyond the task. -7. Clarity — naming, dead code, comments that lie. +6. Stack & ADR conformance — every dependency, import, library, and framework the + change introduces must map to a `docs/stack.md` **Decided** row, and any + governing `adr:` the feat's Refs declare must be honored (not contradicted). + New tech outside the decided stack, or a decision that drifts from a cited ADR, + is a Blocker: surface it as a finding (and a `docs/stack.md`/ADR follow-up) rather + than merging it quietly. `csdd graph explain adr:` and + `csdd graph query ` fetch the governing context the brief no longer inlines. +7. Overengineering — speculative abstraction, unused options, scope creep beyond the task. +8. Clarity — naming, dead code, comments that lie. ## Output format diff --git a/internal/templater/templates/agents/spec-author.md.tmpl b/internal/templater/templates/agents/spec-author.md.tmpl new file mode 100644 index 0000000..30f6cfc --- /dev/null +++ b/internal/templater/templates/agents/spec-author.md.tmpl @@ -0,0 +1,121 @@ +--- +name: spec-author +description: Authors ONE spec phase artifact (requirements, design, or tasks) for a feat — scaffolds it with `csdd spec generate`, consults the knowledge graph for the governing code/ADRs/stack, writes the body to EARS and traceability, validates, and reports. Sonnet writes; the orchestrator (opus) reviews, validates, and approves. Does not implement, does not approve, does not touch the approved plan. +tools: Read, Grep, Glob, Edit, Write, Bash +model: sonnet +effort: medium +skills: + - graph +color: purple +--- + +You author **one spec phase** for one feat — `requirements`, `design`, or `tasks` +— and leave the spec honest: the artifact validates, follows EARS, and traces end +to end. You write the artifact; you do not approve it (the orchestrator that +dispatched you owns approval), and you do not implement tasks. You run on a +cheaper model on purpose — the orchestrator's job is to *review* what you wrote, +not to draft it. + +## Role + +Take a feat slug and a single phase name. Scaffold that phase's artifact, fill in +its body against the knowledge graph and the feat's already-approved prior +phases, validate it, and report. You do not make plan-level decisions, pick up +extra phases, or touch the approved plan (`plan.md`/`plan.json`) — surface a +genuinely wrong plan as a follow-up instead. + +## Operating mode + +Scaffold → consult the graph → author → validate → report. The orchestrator +reviews your artifact, runs `csdd spec validate`, and approves (or re-dispatches +you with a fix-list). You never call `csdd spec approve` — approving your own +work is the orchestrator's judgment call, not yours. + +## Collect inputs first + +**Read narrowly.** Your dispatch names the feat, the phase, and any governing +references (ADRs, stack rows, wiki, seeds). A spec's prior phases plus the graph +are your source material — not the whole repository. A spec is commonly 800+ +lines once complete and every line you pull in is re-read on each of your turns, +so reading past what the phase needs costs real time and buys nothing. + +- Which feat, and which **single** phase (`requirements` | `design` | `tasks`)? +- Which prior phases are already approved? (design needs requirements approved; + tasks needs design approved — the gate enforces this; if a prior phase is not + approved, stop and report rather than generating past it.) +- Which governing references apply? (ADR slugs, stack rows, wiki nodes, seeds.) +- What is the spec's `development_flow` in `spec.json` (absent ⇒ `unit`)? It + decides the shape of `tasks.md` — RED→GREEN pairs under `tdd`/`tdd-e2e`, + implementation-then-cover under `unit`. + +## Workflow + +1. **Scaffold the artifact.** + `csdd spec generate --artifact ` writes the empty template under + `specs//.md`. Start from that skeleton — do not hand-write the + file from scratch, and do not delete its annotation/rule comments. +2. **Consult the graph before reading code.** + Use the `graph` skill: `csdd graph query ` to find the nodes for the + concept, `csdd graph explain ` and `csdd graph path ` to traverse, + `csdd spec show ` for prior phases. Read the code the graph points at, + not the whole tree. For a brownfield/novel area, the orchestrator may ask you + to `--artifact research` first — fold its findings in. +3. **Author to the phase's contract.** + - **requirements:** EARS phrasing (the `WHEN/IF/… THEN … the system SHALL …` + form), one observable behavior per criterion, stable numeric IDs. No + implementation detail. + - **design:** an `## Architecture Pattern & Boundary Map`, a + `## File Structure Plan`, and a `## Requirements Traceability` row for + **every** requirement ID. Cite each governing ADR (by slug) and keep every + technology choice inside a `docs/stack.md` **Decided** row — introducing + tech outside the decided stack is a finding the validator catches. Prefer + the option that deviates least from the decided stack. + - **tasks:** every leaf task carries `_Requirements:_` (numeric IDs only); + every `(P)` task carries a `_Boundary:_` matching a design component; + `_Depends:_` IDs exist; no two `(P)` tasks share a boundary. Shape follows + `development_flow` — the scaffolded template already carries the right + shape; keep it. Sub-tasks are 1–3 hours of focused work. +4. **Validate — and fix what it finds.** + `csdd spec validate ` returns 2 on findings. Fix every finding in the + artifact body and re-validate until it returns 0. Do not `--force` past a + real finding. +5. **Report.** State the phase, the artifact path, a one-line summary of what it + covers, the validate result (0 / findings left), and any unresolved gap or + open question for the orchestrator. Your report **is** the handoff the + orchestrator reviews — it will not re-author the file, so name what you + decided and what you left open. + +## Scope discipline + +- One phase per run. Do not draft two phases "to save a round-trip". +- Stay inside the feat. Cross-feat surface as a follow-up. +- No `csdd spec approve` — the orchestrator owns the gate. +- Never hand-edit `spec.json` frontmatter, `plan.md`, `plan.json`, or `.csdd/`; + regenerate instead. +- Do not implement tasks or run the dev cycle — that is the `implementer`. + +## Gotchas + +- When the `csdd` MCP server is connected, prefer the `csdd_spec_*` / + `csdd_graph_*` tools over the `npx -y @protonspy/csdd` forms shown above — + identical gates and exit codes, typed arguments. Fall back to `npx` only when + the server is not connected. +- A `design.md` that does not cite a governing ADR, or pulls in tech outside a + Decided stack row, is a finding — fix it before reporting, do not leave it for + the reviewer. +- `tasks.md` shape is load-bearing: a `unit`-flow spec with RED/GREEN pairs (or a + `tdd`-flow spec with an implementation step and no test before it) fails + `csdd spec validate`. The scaffolded template matches the flow; keep its shape. +- If a prior phase is not approved, stop and report — generating past an + unapproved gate wastes the scaffold. + +## Completion criteria + +- [ ] The named phase artifact is scaffolded and authored under `specs//`. +- [ ] EARS (requirements) / traceability + boundary map + stack-ADR conformance + (design) / annotations + flow-correct shape (tasks) hold. +- [ ] `csdd spec validate ` returns 0 (or the remaining findings are named + and explicitly left for the orchestrator). +- [ ] No `spec approve` called; `spec.json` frontmatter, the plan, and `.csdd/` + untouched. +- [ ] Report states: phase, artifact path, summary, validate result, open gaps. \ No newline at end of file diff --git a/internal/templater/templates/commands/csdd-plan-run.md.tmpl b/internal/templater/templates/commands/csdd-plan-run.md.tmpl index 4cf6faa..a8f3988 100644 --- a/internal/templater/templates/commands/csdd-plan-run.md.tmpl +++ b/internal/templater/templates/commands/csdd-plan-run.md.tmpl @@ -12,7 +12,7 @@ Preflight first: `csdd plan status ` must show the plan **approved** and **drift-free**. Then: - `csdd plan run ` — the loop spawns one fresh session per iteration with - `--dangerously-skip-permissions` (default cap 100, `--max-iterations`). Each + `--dangerously-skip-permissions` (default cap 30, `--max-iterations`). Each session is handed ONE whole feat's mission brief and drives the entire flow itself. Run it inside the hardened sandbox: the runner checks `csdd sandbox doctor` first, and when isolation is NOT verified it alerts and diff --git a/internal/templater/templates/plan/CLAUDE.md.tmpl b/internal/templater/templates/plan/CLAUDE.md.tmpl new file mode 100644 index 0000000..a956471 --- /dev/null +++ b/internal/templater/templates/plan/CLAUDE.md.tmpl @@ -0,0 +1,86 @@ +# Autonomous plan session + +You are one session of `csdd plan run`, working **one feat** to completion in your +own git worktree. This file replaces the repository's interactive CLAUDE.md for the +duration of the run: those rules are written for a human-in-the-loop, and you have +no human in the loop. + +## Your authority + +A human already opened the gate by running `csdd plan approve` on this plan. That +is the authorization the phase gates require, and it covers every spec you author +here. **You are the approver.** Approve your own phases as they validate; never +return `continue` because something "needs approval". + +## Hard rules + +- `csdd` is the only author of managed files. Create and change specs, steering, + skills and agents through the CLI — never by writing those files directly. +- Never hand-edit `spec.json`, frontmatter, or task annotations. +- Never touch `plan.md`, `plan.json`, or `.csdd/`. The approved plan is the + contract; `.csdd/` is the loop's state. +- Do not create or switch branches, push, or open a PR. You are already on this + feat's branch. One PR covers the whole run and a human opens it. + +## The cycle + +``` +spec init → (generate → validate → approve) ×3 → implement → commit → verdict +``` + +1. **Spec.** `csdd spec init --flow `, then for each phase in order: + `csdd spec generate --artifact requirements|design|tasks`, + `csdd spec validate `, `csdd spec approve --phase `. +2. **Implement.** One leaf task at a time, each delegated to the `implementer` + sub-agent. Check its box `[x]` in `tasks.md` as it lands green. +3. **Commit** via `/csdd-commit` as you land each task. Nothing may be left + uncommitted: the runner merges your *branch*, so uncommitted work does not exist. +4. **Verdict.** `done` only when the three checks below hold; otherwise `continue` + with a handoff naming what remains. + +## Choosing the flow + +| flow | use for | cost | +|---|---|---| +| `unit` | render/CRUD behavior, presentational components | one verification round-trip per behavior | +| `tdd` | money, auth, tenancy, anything irreversible | two round-trips per behavior | +| `tdd-e2e` | the above plus a real cross-boundary path | most expensive; use sparingly | + +`unit` is the default choice unless the feat touches something you cannot take +back. The flow decides the shape of `tasks.md`, and `csdd spec validate` enforces it. + +## Token discipline + +You are metered. These are the four things that actually move the number: + +- **Delegate implementation.** Hand each task to the `implementer` sub-agent; it + runs on a cheaper model. Writing task code inline puts the whole implementation + on the orchestrator's context and re-reads it every turn after. +- **Hand it the task, not the spec.** A dispatch carries the task text with its + `_Requirements:_`/`_Boundary:_`/`_Depends:_`, the acceptance criteria those IDs + name, and the one `### ` section that owns the boundary. Making every + sub-agent re-read the whole spec pays that cost once per sub-agent. +- **Delegate the gate.** Run the suite, lint and typecheck through the + `quality-gate` sub-agent, once, at feat exit. Running them inline lands the full + output in your context and you re-read it on every remaining turn. +- **Traverse the graph, don't grep the tree.** `csdd graph query `, + `csdd graph explain `, `csdd graph path `. + +Dispatch `(P)` tasks in different `_Boundary:_` groups concurrently; honor every +`_Depends:_`; keep same-boundary tasks sequential. + +## Before you declare `done` + +Run each of these once, at feat exit — not after every task: + +- `csdd spec validate ` passes +- every task box in `specs//tasks.md` is `[x]` +- `specs//test-report.json` is green with no open attentions + +The loop checks these three on disk before accepting your verdict. It does not +review your code and does not re-run your suite — it reads your artifacts. A `done` +that fails any of them comes back as a `continue` and costs a whole extra session. + +An honest `continue` and a refused `done` cost the same. The `continue` spends it +carrying what you learned; the refused `done` spends it being told what you could +have checked yourself. diff --git a/internal/templater/templates/root/CLAUDE.md.tmpl b/internal/templater/templates/root/CLAUDE.md.tmpl index 68bc3bb..de21a0c 100644 --- a/internal/templater/templates/root/CLAUDE.md.tmpl +++ b/internal/templater/templates/root/CLAUDE.md.tmpl @@ -2,168 +2,155 @@ This repository follows a Spec-Driven + Test-Driven Development (SDD+TDD) workflow native to **Claude Code**. Steering, specs, skills, and sub-agents are -managed by the `csdd` CLI — or, preferably, its MCP tools (see *Driving csdd*). - -**This file is the contract and the operational reference** — the gates, command -surface, and phrasing rules all live here. Read it before you create, generate, -approve, or edit any steering file, spec, skill, or agent. Full CLI surface: -`npx @protonspy/csdd --help`. +managed by the `csdd` CLI — or, preferably, its MCP tools. **This file is the +contract and the operational reference** — the gates, command surface, and +phrasing rules all live here. Read it before you create, generate, approve, or +edit any steering file or spec. Full CLI surface: `npx @protonspy/csdd --help`. ## STOP — hard rules, enforced not advisory -Breaking one of these corrupts the contract layer that every human review and -every gate depends on: +Breaking one corrupts the contract layer every human review and every gate +depends on: 1. **`csdd` is the only sanctioned author of managed files.** Create and change - steering, specs, skills, and agents **only** through the `csdd_*` MCP tools or - the `csdd` CLI — never by writing the files directly. + steering, specs, skills, and agents **only** through the `csdd_*` MCP tools + or the `csdd` CLI — never by writing the files directly. 2. **Never hand-edit `spec.json`.** It records phase approvals and - `ready_for_implementation`. Writing to it directly — even to "unblock" - yourself — bypasses the human gate and is a process violation. Phases are - crossed *only* with `npx @protonspy/csdd spec approve --phase `, which a - human authorizes. + `ready_for_implementation`. Phases are crossed *only* with + `npx @protonspy/csdd spec approve --phase `, which a human + authorizes. 3. **Never hand-edit frontmatter, task annotations, or `.mcp.json`.** If a generated file is wrong, regenerate it from the template and edit the *body*, then re-validate — do not patch the machine-managed parts by hand. -4. **Never skip a phase gate.** `requirements → design → tasks → implementation` - is strictly ordered; you cannot generate a phase until the prior one is - human-approved. `--force` is allowed **only** when the user explicitly - authorizes a fast-track / Quick Plan. +4. **Never skip a phase gate.** `requirements → design → tasks → + implementation` is strictly ordered; you cannot generate a phase until the + prior one is human-approved. `--force` is allowed **only** when the user + explicitly authorizes a fast-track / Quick Plan. 5. **When a rule blocks you, stop and surface it** — report the blocked item to the human. Do not route around the gate. **One sanctioned exception — the autonomous `csdd plan run` loop.** The human authorized the whole plan up front (`csdd plan approve`), so a session there -self-approves its own *validated* spec phases and does not stop for a human at a -gate — rules 2, 4, and 5 govern *interactive* development. The session still -validates every phase before approving it and delegates each task to the -`implementer` sub-agent. See the `plan-dev` skill. - -## Driving csdd — prefer the MCP tools for the dev flow - -`npx @protonspy/csdd init` registers a **`csdd` MCP server** in `.mcp.json`. When it is -connected, drive the development flow with its **`csdd_*` tools** instead of -shelling out: - -| Resource | Tools | CLI fallback | -|---|---|---| -| steering | `csdd_steering_*` | `npx @protonspy/csdd steering …` | -| spec | `csdd_spec_*` | `npx @protonspy/csdd spec …` | -| skill | `csdd_skill_*` | `npx @protonspy/csdd skill …` | -| agent | `csdd_agent_*` | `npx @protonspy/csdd agent …` | - -Typed parameters (the `artifact`/`phase`/`inclusion` enums, required fields) stop -invalid values; the server builds the exact argv. **Phase gates, validators, and -exit codes are identical either way.** Use the **CLI** when the server isn't -connected, and always for **setup/management** — `init`, `copy`, `mcp`, `export` -are intentionally *not* tools. - -## Mental model — read this first - -Two distinctions matter more than anything else: - -- **Specification vs Design.** *Specification* is the contract: - `requirements.md`, the File Structure Plan inside `design.md`, and the - `_Boundary:_`/`_Depends:_` annotations on `tasks.md`. **Humans review and - approve this.** *Design* is the implementation space *inside* that contract — - components, internals, sequencing. **You are free here once the specification - is approved.** +self-approves its own *validated* spec phases (authoring delegated to the +`spec-author` sub-agent, each task to the `implementer`) and does not stop for a +human at a gate — rules 2, 4, and 5 govern *interactive* development. The +session still validates every phase before approving it. See the `plan-dev` +skill. + +## Driving csdd — the MCP tools are the default + +`csdd init` registers a **`csdd` MCP server** in `.mcp.json` — a stdio server +that runs the `csdd` binary under the hood — so it is configured by default. +Reach csdd through its **`csdd_*` tools** (`csdd_steering_*`, `csdd_spec_*`, …): +they take typed params (the `artifact`/`phase`/`inclusion` enums, required +fields) that stop invalid values, and build the exact argv. **Phase gates, +validators, and exit codes are identical to the CLI** — same binary either way. +Use the **CLI** (`npx @protonspy/csdd …`) when the server isn't connected, and +for setup/management — `init`, `copy`, `mcp`, `export` are intentionally *not* +tools. + +## Mental model + +- **Specification vs Design.** *Specification* is the contract — + `requirements.md`, the File Structure Plan in `design.md`, the `_Boundary:_`/ + `_Depends:_` annotations on `tasks.md` — **humans review and approve this.** + *Design* is the space *inside* that contract; **you are free there once the + specification is approved.** - **Phase gates.** Every spec advances `requirements → design → tasks → implementation`; the CLI blocks generating a phase until the prior one is - human-approved in `spec.json` (STOP rule 4). Those gates protect the - *Specification* half above. - -## Working discipline — how you write inside the gates - -The gates decide *what* to build and *when*; these decide *how*. Bias toward -caution over speed; use judgment on genuinely trivial changes. - -- **Think before you code.** State your assumptions; when a requirement reads two - ways, surface both instead of silently choosing; when a simpler path exists, say - so. If something is unclear, stop and name it — the STOP reflex, applied to design. -- **Simplicity first.** Write the least code that satisfies the task and passes its - tests — nothing speculative: no unrequested features, no abstraction for - single-use code, no config nobody asked for, no handling for impossible states. - If 200 lines could be 50, rewrite it. This is the *Design* freedom, spent frugally. -- **Stay surgical.** Touch only what the task needs. Don't reformat or refactor - adjacent code; match the surrounding style even when you'd write it differently. - Remove only the imports and symbols your change orphaned — flag pre-existing dead - code, don't delete it. Every changed line traces to a requirement the task carries. -- **Drive to a verifiable goal.** Turn the task into a checkable outcome first — - "add validation" becomes "a failing test for invalid input, then make it pass". - Sharp criteria let you loop to green on your own; vague ones ("make it work") - force round-trips. - -## Command cheat sheet - -Names are **kebab-case** (`api-conventions`, `photo-albums`, `code-reviewer`); -the CLI rejects anything else. Global flags on every command: `--root PATH` -(override workspace), `--force` (override safety checks), `-h`/`--help`, -`-v`/`--version`. Exit codes: `0` OK, `1` user error, `2` validation failure. + human-approved in `spec.json` (STOP rule 4). -```bash -# bootstrap (one time per repo) -npx @protonspy/csdd init [--with-baseline] [--exclude agents,skills,…] [--no-mcp] +## Working discipline -# copy one shipped artifact into the workspace (pairs with init --exclude) -npx @protonspy/csdd copy skills/tdd-cycle # or agents/…, rules/…, commands/…, hooks/…, templates/…, steering/… +Bias toward caution over speed; use judgment on genuinely trivial changes. -# steering (project memory) -npx @protonspy/csdd steering init -npx @protonspy/csdd steering create NAME --inclusion {always|fileMatch|manual|auto} \ - [--pattern GLOB]... [--description TEXT] -npx @protonspy/csdd steering {list,show,delete,validate} [NAME] +- **Think before you code** — state assumptions, surface ambiguity, pick the + simpler path and say so; unclear → stop and name it. +- **Simplicity first** — least code that satisfies the task and passes its tests; + no speculative features, no abstraction for single-use code. +- **Stay surgical** — touch only what the task needs, match surrounding style, + don't reformat adjacent code; flag pre-existing dead code, don't delete it. +- **Drive to a verifiable goal** — turn the task into a checkable outcome first. -# specs — generate → validate → human approves, one phase at a time -npx @protonspy/csdd spec init FEATURE -npx @protonspy/csdd spec generate FEATURE --artifact {requirements|design|tasks|research|bugfix} [--force] -npx @protonspy/csdd spec approve FEATURE --phase {requirements|design|tasks} [--force] -npx @protonspy/csdd spec status FEATURE # show + validate in one shot -npx @protonspy/csdd spec {list,show,validate,delete} FEATURE +## Command surface — CLI reference (the binary the MCP server runs) -# skills — executable workflow bundles -npx @protonspy/csdd skill create NAME --description TEXT -npx @protonspy/csdd skill {add-reference,add-script,add-asset} SKILL FILE -npx @protonspy/csdd skill {list,show,validate,delete} NAME +The MCP tools above are the default path for the agent; this is the canonical CLI +reference and the fallback when the server isn't connected. Names are +**kebab-case** (`api-conventions`, `photo-albums`); the CLI rejects anything else. +Global flags: `--root PATH`, `--force`, `-h`/`--help`, `-v`/`--version`. Exit +codes: `0` OK, `1` user error, `2` validation failure. -# custom sub-agents — least privilege -npx @protonspy/csdd agent create NAME --description TEXT [--tools TOOL]... [--model M] -npx @protonspy/csdd agent {list,show,delete} NAME - -# mcp servers (.mcp.json) -npx @protonspy/csdd mcp add NAME --command CMD [--arg A]... [--env K=V]... # stdio -npx @protonspy/csdd mcp add NAME --url URL [--type sse|http] [--env K=V]... # remote -npx @protonspy/csdd mcp {list,show,enable,disable,remove,validate} [NAME] +```bash +npx @protonspy/csdd init [--with-baseline] [--exclude …] [--no-mcp] # bootstrap once +npx @protonspy/csdd steering {init,create,list,show,delete,validate} … +npx @protonspy/csdd spec init FEATURE +npx @protonspy/csdd spec generate FEATURE --artifact {requirements|design|tasks|research|bugfix} [--force] +npx @protonspy/csdd spec approve FEATURE --phase {requirements|design|tasks} [--force] +npx @protonspy/csdd spec status FEATURE # show + validate +npx @protonspy/csdd spec {list,show,validate,delete} FEATURE +npx @protonspy/csdd graph {build,query,explain,path,analyze} … # knowledge graph +npx @protonspy/csdd plan {init,run,status,brief,validate,approve,cost,verify} … ``` +You can also create **skills**, **custom sub-agents**, and register **MCP +servers** with csdd — consult `csdd --help` (or `csdd skill --help`, `csdd +agent --help`, `csdd mcp --help`) for the exact commands rather than +hand-authoring those files. + ## The development cycle (follow it in order) +The steps below run through the **`csdd_*` MCP tools** by default — their typed +params carry the `artifact`/`phase` enums and required fields, so the contract +is enforced without you spelling flags. The CLI block above is the fallback. + ``` requirements ──approve──▶ design ──approve──▶ tasks ──approve──▶ implementation (human) (human) (human) ready_for_implementation: true ``` -1. **Start each feature on a clean context:** `/clear`, then `npx @protonspy/csdd spec init `. -2. **Generate → review → approve, one phase at a time:** `spec generate --artifact requirements` → human reviews → `spec approve --phase requirements`; then `design`, then `tasks`. The gate refuses the next `generate` until the current phase is approved. -3. `ready_for_implementation` flips to `true` only after all three phases are approved — and only csdd flips it. Do not set it yourself. -4. **Branch before the first task:** `git switch -c /` (kebab-case: `feat/ fix/ chore/ refactor/ docs/ test/ perf/`). With hooks enabled (`csdd init --hooks`), a PreToolUse hook blocks commits on the default branch. -5. Implement one task per iteration with fresh-context sub-agents — `implementer` drives the task through the cycle its spec's `development_flow` declares (`tdd-cycle` under tdd/tdd-e2e, `unit-cycle` under unit) → `code-reviewer` (plus `security-reviewer` for auth/secrets/input), with `quality-gate` running the command gate at feat exit — inside the approved design boundary. Independent `(P)` tasks in *different* `_Boundary:_` groups run as parallel `implementer` sub-agents in isolated worktrees; same-boundary tasks stay ordered and every `_Depends:_` is honored first. -6. Commit a slice only after review is clean, then push — with the pre-push gate installed (`csdd init --prepush`, then `git config core.hooksPath .githooks`), it runs the test gate and blocks a red push. -7. **Close each feature with a `/clear`** once its PR ships, so context does not accumulate across features. -8. Steering is updated only when a new pattern emerges that the agent could not derive from the code. +1. **Start each feature on a clean context:** `/clear`, then `csdd_spec_init`. + **Pick the flow deliberately:** `unit` (the default) for render/CRUD surfaces; + `tdd`/`tdd-e2e` **required** for money, auth, tenancy, and anything + irreversible — it decides the shape of `tasks.md` and `csdd_spec_validate` + enforces it. +2. **Generate → review → approve, one phase at a time:** `csdd_spec_generate` + (artifact) → human review → `csdd_spec_approve` (phase). The gate refuses the + next generate until the current phase is approved. +3. `ready_for_implementation` flips to `true` only after all three phases are + approved — and only csdd flips it. +4. **Branch before the first task:** `git switch -c /` (kebab-case). + With hooks (`csdd init --hooks`), a PreToolUse hook blocks commits on the + default branch. +5. Implement one task per iteration via the `implementer` sub-agent (the flow's + cycle: `tdd-cycle` under `tdd`/`tdd-e2e`, `unit-cycle` under `unit`) → + `code-reviewer` (+`security-reviewer` for auth/secrets/input), with + `quality-gate` at feat exit — inside the approved design boundary. + Independent `(P)` tasks in *different* `_Boundary:_` groups run as parallel + `implementer` sub-agents in isolated worktrees; same-boundary tasks stay + ordered; every `_Depends:_` honored first. +6. Commit a slice only after review is clean, then push — with the pre-push gate + (`csdd init --prepush`, `git config core.hooksPath .githooks`) it runs the + test gate and blocks a red push. +7. **Close each feature with a `/clear`** once its PR ships. ### Variants -- **Bugfix** — replace `requirements.md` with `bugfix.md` (`spec generate --artifact bugfix`); the phase gate is skipped. The validator requires `Current Behavior:`, `Expected Behavior:`, `Unchanged Behavior:`, and `Root Cause`. Document the root cause **before** writing any code. -- **Research** — for brownfield or novel domains add `spec generate --artifact research` before design. Every finding in `research.md` must connect to a concrete commitment in `design.md`. -- **No spec needed** — small reversible changes (typos, one-line fixes) skip the spec layer. SDD is a contract layer, not a tax. +- **Bugfix** — replace `requirements.md` with `bugfix.md` (`spec generate + --artifact bugfix`); the phase gate is skipped. The validator requires + `Current Behavior:`, `Expected Behavior:`, `Unchanged Behavior:`, and `Root + Cause`. Document the root cause **before** writing any code. +- **Research** — for brownfield or novel domains add `spec generate --artifact + research` before design. Every finding in `research.md` must connect to a + concrete commitment in `design.md`. +- **No spec needed** — small reversible changes (typos, one-line fixes) skip + the spec layer. SDD is a contract layer, not a tax. ## EARS — the only acceptable requirement phrasing Every criterion in `requirements.md` **must** use EARS. Trigger keywords -(`WHEN`/`WHILE`/`IF`/`WHERE`/`SHALL`/`THE SYSTEM`) are **fixed English** — localize -only the variable parts. Use **`SHALL`**, never `should`. One behavior per -criterion. Quantify ("fast"/"robust" are not testable). +(`WHEN`/`WHILE`/`IF`/`WHERE`/`SHALL`/`THE SYSTEM`) are **fixed English** — +localize only the variable parts. Use **`SHALL`**, never `should`. One +behavior per criterion. Quantify ("fast"/"robust" are not testable). | Pattern | Template | |---|---| @@ -191,12 +178,13 @@ outcomes**, never file edits. Every task carries machine-readable annotations: Sizing: major tasks 3–10 sub-tasks; leaves 1–3 hours. Phase order: `## Phase 1: Foundation → Phase 2: Core → Phase 3: Integration → Phase 4: Validation`. -## Steering, skills, agents, MCP — the essentials +## Steering — project memory -- **Steering** (`.claude/steering/*.md`) is always-on project memory, imported into the managed block below. `inclusion:` (`always|fileMatch|manual|auto`) documents intended scope. Put organizational patterns and the *why* here; **never** secrets, file dumps, or details derivable from code. Golden rule: *if new code follows existing patterns, steering should not need updating.* -- **Skills** (`.claude/skills//`) are executable capability bundles: `SKILL.md` (≤ 500 lines / ~5k tokens) + `references/` + `assets/` + `scripts/`. Required sections: `## Goal`, `## Execution Workflow`, `## Gotchas`, `## Verification Before Reporting`, `## Completion Criteria`. Every `references/` file must be cited by name with a load trigger. Build skills from real expertise, never generic LLM output. -- **Sub-agents** (`.claude/agents/.md`) default to `Read, Grep` (least privilege). The more powerful the agent, the smaller its scope and the larger the human review must be. -- **MCP servers** (`.mcp.json`) are managed only with `npx @protonspy/csdd mcp`. Each server sets exactly one transport — `command` (stdio) **or** `url` (remote), never both. Scope narrowly, avoid `--auto-approve`, pass secrets via `--env`, never inline them. +**Steering** (`.claude/steering/*.md`) is always-on project memory, imported +into the managed block below. `inclusion:` (`always|fileMatch|manual|auto`) +documents intended scope. Put organizational patterns and the *why* here; +**never** secrets, file dumps, or details derivable from code. Golden rule: *if +new code follows existing patterns, steering should not need updating.* ## Validation — what the gates catch @@ -205,41 +193,38 @@ continuing): - **steering** — `inclusion` is valid; `fileMatch` has a non-empty pattern; `auto` has `name` + `description`. - **spec (requirements)** — every criterion starts with an EARS keyword; no `should`; requirement headers unique. -- **spec (design)** — `## Architecture Pattern & Boundary Map` and `## File Structure Plan` present; every requirement ID appears in the `## Requirements Traceability` table; `design.md` ≤ 1000 lines. -- **spec (tasks)** — leaves have `_Requirements:_`; referenced IDs exist; every `(P)` has `_Boundary:_` matching a design component; `_Depends:_` IDs exist; no two `(P)` share a boundary. +- **spec (design)** — `## Architecture Pattern & Boundary Map` and `## File Structure Plan` present; every requirement ID appears in `## Requirements Traceability`; `design.md` ≤ 1000 lines; **each governing `adr:` the feat's Refs declare must be cited** (enforced by `plan validate`). +- **spec (tasks)** — leaves have `_Requirements:_`; referenced IDs exist; every `(P)` has `_Boundary:_` matching a design component; `_Depends:_` IDs exist; no two `(P)` share a boundary; the shape matches `development_flow` (RED→GREEN under `tdd`/`tdd-e2e`, implement→cover under `unit`). - **skill** — `name` matches the dir; `description` non-empty; body ≤ 500 lines / ~5k tokens; required headings present; every `references/` file cited. - **mcp** — well-formed JSON; exactly one transport per server; remote `type` is `sse`/`http`; no empty env keys. -Run `npx @protonspy/csdd spec status ` between every step. +Run `csdd spec status ` between every step (MCP `csdd_spec_status`, or the CLI above). ## Anti-patterns — refuse to do these -Skipping phase gates outside authorized fast-track work · hand-editing +Skipping phase gates outside authorized fast-track · hand-editing `spec.json`/frontmatter/`.mcp.json` · writing tasks as file edits · omitting -`_Boundary:_` on `(P)` tasks · letting `design.md` exceed 1000 lines · treating -`research.md` findings as decoration · putting secrets in steering/specs · -skipping the "why" when adding a steering rule · granting broad tool scope to a -sub-agent · adding MCP servers with broad scope or `--auto-approve` · adding AI or -assistant attribution (`Co-authored-by:`, `Claude-Session`, "Generated with…") to -commits or PRs. - -Escalate to a human when a feature touches auth, payments, PII, or regulated -data; when a change needs `--force` past a gate; or before anything destructive. +`_Boundary:_` on `(P)` tasks · `design.md` > 1000 lines · treating +`research.md` findings as decoration · secrets in steering/specs · skipping the +"why" on a steering rule · broad tool scope on a sub-agent · MCP servers with +broad scope or `--auto-approve` · AI/assistant attribution (`Co-authored-by:`, +"Generated with…") in commits or PRs. Escalate to a human for auth, payments, +PII, or regulated data; for any `--force` past a gate; or before anything +destructive. ## Knowledge base — graph, wiki, tech contract -The workflow moments for the knowledge base (consult the graph before searching; -rebuild after changes; ingest raw sources; gate with `analyze`/`wiki lint`) and -the tech-contract protocol live in the managed block below. `csdd init` keeps it -in sync — do not hand-edit between the markers. +The graph/wiki workflow moments and the tech-contract protocol live in the +managed block below — kept in sync by `csdd init`; do not hand-edit between the +markers. ## Project memory (steering) -Steering files are always-on project memory, loaded via the `@`-imports below. -`csdd` keeps this list in sync with `.claude/steering/` — do not hand-edit +Steering files are always-on project memory, loaded via the `@`-imports below +— `csdd` keeps this list in sync with `.claude/steering/`; do not hand-edit between the markers. @@ -248,13 +233,11 @@ between the markers. ## Layout - `.claude/steering/` — project memory imported above. -- `specs//` — per-feature contracts (`spec.json` + `requirements.md` + `design.md` + `tasks.md`). -- `.claude/rules/` — generation rules and review gates the agent runs mechanically. -- `.claude/templates/` — templates the agent uses when generating artifacts. -- `.claude/skills//` — executable workflow skills. -- `.claude/agents/.md` — custom sub-agents with least-privilege tool scope. -- `.claude/commands/.md` — slash commands (`/csdd-commit` generates the commit message from the diff + active spec). -- `.claude/settings.json` — permission guards; plus the hook automation wiring (format on edit, command safety, stop reminder) when `.claude/hooks/` is opted in (`csdd init --hooks`). -- `.githooks/pre-push` — the test gate at push time (opt-in: `csdd init --prepush`, then `git config core.hooksPath .githooks`). -- `.github/pull_request_template.md` — evidence-bearing PR body. -- `.mcp.json` — Model Context Protocol servers. +- `specs//` — per-feature contracts (`spec.json` + `requirements.md` + `design.md` + `tasks.md`). +- `.claude/{rules,templates}/` — generation rules, review gates, and artifact templates the agent runs mechanically. +- `.claude/skills//` — executable workflow skills. +- `.claude/agents/.md` — sub-agents (least-privilege tool scope). +- `.claude/commands/.md` — slash commands (`/csdd-commit` builds the message from the diff + active spec). +- `.claude/settings.json` — permission guards + hook wiring (with `csdd init --hooks`). +- `.githooks/pre-push` — the test gate at push (`csdd init --prepush` + `git config core.hooksPath .githooks`). +- `.github/pull_request_template.md` · `.mcp.json` — evidence-bearing PR body · MCP servers. \ No newline at end of file diff --git a/internal/templater/templates/skills/plan-dev/SKILL.md.tmpl b/internal/templater/templates/skills/plan-dev/SKILL.md.tmpl index 980ce41..17fb139 100644 --- a/internal/templater/templates/skills/plan-dev/SKILL.md.tmpl +++ b/internal/templater/templates/skills/plan-dev/SKILL.md.tmpl @@ -9,8 +9,10 @@ description: Use inside a `csdd plan run` session to deliver ONE approved-plan f Fully deliver **one** feat of an approved csdd plan in a single autonomous `plan run` session: drive its spec through `requirements → design → tasks` -(you author and decide), delegate each task to the `implementer` sub-agent -(test-first), own the git dev-cycle and the PR, then return the verdict schema. +(you delegate authoring to the `spec-author` sub-agent, then review and decide), +delegate each task to the `implementer` sub-agent (per the spec's +`development_flow`), own the git dev-cycle and the PR, then return the verdict +schema. The loop trusts your **judgment** — it never reviews your code or re-runs your suites — but it does check your **artifacts**. A `done` verdict passes through a @@ -21,7 +23,7 @@ missing, your `done` is converted to `continue` and handed back with a note nami what failed. So `done` means the whole feat is really delivered, not hoped — and claiming it early costs you an attempt rather than shipping a broken feat. -A feat that burns its attempt budget (default 8 sessions) is stopped and surfaced +A feat that burns its attempt budget (default 4 sessions) is stopped and surfaced for a human. Returning an honest `continue` is always cheaper than a `done` the gate refuses. @@ -57,26 +59,38 @@ self-approve**; the gate refuses the next phase until the current one is approve ### 1) Create the spec (if absent) - `csdd spec init `. If seeds were listed in your brief, fold them in. - -### 2) Requirements -- `csdd spec generate --artifact requirements`. -- Review the body: EARS phrasing, one behavior per criterion, numeric IDs. -- `csdd spec validate ` → fix every finding. +- **Pick the flow deliberately:** `unit` (the default) for render/CRUD surfaces; + `tdd`/`tdd-e2e` REQUIRED for money, auth, tenancy, anything irreversible. The + flow decides the shape of `tasks.md`, and `csdd spec validate` enforces it. + +### 2) Requirements — delegate to `spec-author`, then review and approve +- Dispatch the **`spec-author`** sub-agent with the feat + phase `requirements` + + any governing refs. It scaffolds, consults the graph, authors the body (EARS, + one behavior per criterion, numeric IDs), and runs `csdd spec validate`. +- **REVIEW** the artifact: EARS phrasing, one behavior per criterion, numeric IDs. +- Run `csdd spec validate ` yourself. Re-dispatch `spec-author` with a + fix-list if the review or validate finds gaps; do not hand-edit inline. - `csdd spec approve --phase requirements`. -### 3) Design -- `csdd spec generate --artifact design` (add `--artifact research` first - for a brownfield/novel area). -- Ensure `## Architecture Pattern & Boundary Map`, `## File Structure Plan`, and - a `## Requirements Traceability` row for every requirement ID. -- `csdd spec validate ` → fix → `csdd spec approve --phase design`. - -### 4) Tasks -- `csdd spec generate --artifact tasks`. -- Every leaf task has `_Requirements:_`; every `(P)` task has a `_Boundary:_` - matching a design component; `_Depends:_` IDs exist; no two `(P)` share a - boundary. -- `csdd spec validate ` → fix → `csdd spec approve --phase tasks`. +### 3) Design — delegate to `spec-author`, then review and approve +- Dispatch **`spec-author`** for phase `design` (add `--artifact research` first + for a brownfield/novel area, and pass the governing ADRs/stack it must comply + with). +- **REVIEW** the artifact: `## Architecture Pattern & Boundary Map`, + `## File Structure Plan`, a `## Requirements Traceability` row for every + requirement ID, and every technology choice inside a `docs/stack.md` Decided + row with governing ADRs cited. +- `csdd spec validate ` → re-dispatch `spec-author` to fix gaps → + `csdd spec approve --phase design`. + +### 4) Tasks — delegate to `spec-author`, then review and approve +- Dispatch **`spec-author`** for phase `tasks`. +- **REVIEW** the artifact: every leaf task has `_Requirements:_`; every `(P)` + task has a `_Boundary:_` matching a design component; `_Depends:_` IDs exist; + no two `(P)` share a boundary; the shape matches the `development_flow` + (`tdd`/`tdd-e2e`: RED→GREEN pairs; `unit`: implement → cover). +- `csdd spec validate ` → re-dispatch `spec-author` to fix gaps → + `csdd spec approve --phase tasks`. - `ready_for_implementation` flips to true only after all three approvals — and only csdd flips it. Never hand-edit `spec.json`. @@ -84,6 +98,23 @@ self-approve**; the gate refuses the next phase until the current one is approve You authored and approved the spec; those were the decisions and they ran on your (orchestrator) model. Implementation is the *execution* of a decision already made, so **delegate it** — do not hand-write task code inline. + +**Test timing — test the right thing at the right moment, not everything always.** +The verification work is split by scope and by who owns it, and each tier runs +**once** at its moment: +- **Tier 2 (task-exit, the `implementer`, `--fast`):** tests the **part** being + developed, during the task, on the implementer's own fast model. This is where + unit/conv tests for the task's behavior belong. +- **Tier 3 (batch-exit then feat-exit, the `quality-gate`, coverage on):** tests + the **whole** merged result — full suite, lint, typecheck, build, quality — + once per batch (5a) and once per feat (step 8), never per task. +- **Evidence:** each task records its own Tier-2 evidence + (`specs//test-report.json`) as it lands; the final feat-exit run records + the Tier-3 evidence. You read the result the sub-agent reports, not the suite + output. +- **Never run the full suite, lint, or typecheck inline in your orchestrator + context.** Their output lands in a context you re-read on every remaining turn + of the feat — that is real spend for no signal. Delegate; read the verdict. - Branch before the first task: `git switch -c feat/` (kebab-case). - For **each** leaf task, spawn the **`implementer`** sub-agent (Agent/Task tool), one task per sub-agent. It runs the cycle the spec's `development_flow` declares — @@ -204,10 +235,12 @@ Return exactly one, as the JSON schema the runner gave you: - You are the approver here — stalling at a phase "waiting for a human" is the single most common failure of this loop. Approve your own validated phases. -- You orchestrate; the `implementer` sub-agent implements. One task per implementer - (one cycle pass); do not batch tasks, hand-write task code inline, or drop the - flow's test obligation — the RED test under `tdd`/`tdd-e2e`, the same-task test - under `unit`. +- You orchestrate; you do not draft or implement. Delegate spec authoring to the + `spec-author` sub-agent (sonnet) and each task to the `implementer` sub-agent, + then review their work. One task per implementer (one cycle pass); do not + batch tasks, hand-write task code inline, hand-author a spec phase inline, or + drop the flow's test obligation — the RED test under `tdd`/`tdd-e2e`, the + same-task test under `unit`. - Never hand-edit `spec.json`, frontmatter, or `.csdd/`; regenerate instead. - Do not edit `plan.md`/`plan.json` — surface a genuinely wrong plan in your `continue` summary instead, and keep working what you can. diff --git a/internal/templater/templates/spec/tasks-unit.md.tmpl b/internal/templater/templates/spec/tasks-unit.md.tmpl new file mode 100644 index 0000000..34eb54c --- /dev/null +++ b/internal/templater/templates/spec/tasks-unit.md.tmpl @@ -0,0 +1,53 @@ +# Tasks + + + +## Phase 1: Foundation + +- [ ] 1. [Foundation: schema / scaffolding the behaviors build on] _Boundary: _ + - [ ] 1.1 [Minimal structure: migration, types, module skeleton] + - _Requirements: 1.1_ + +## Phase 2: Core + +- [ ] 2. [Core capability] _Boundary: _ + - [ ] 2.1 Implement [behavior] + - _Requirements: 1.1, 1.2_ + - _Depends: 1.1_ + - [ ] 2.2 Cover [behavior] with unit tests + - _Requirements: 1.1, 1.2_ + +## Phase 3: Integration + +- [ ] 3. [Integration across boundaries] _Boundary: _ + - [ ] 3.1 Wire [cross-boundary behavior] across components + - _Requirements: 1.2_ + - _Depends: 2.1_ + - [ ] 3.2 Cover the cross-boundary behavior with tests + - _Requirements: 1.2_ + +## Phase 4: Validation + +- [ ] 4. End-to-end tests for the golden path and the top error cases + - _Requirements: 1.1, 1.2_ + - _Depends: 3.1_ + +## Implementation Notes + \ No newline at end of file diff --git a/internal/templater/templates/spec/tasks.md.tmpl b/internal/templater/templates/spec/tasks.md.tmpl index ebe968d..97552ac 100644 --- a/internal/templater/templates/spec/tasks.md.tmpl +++ b/internal/templater/templates/spec/tasks.md.tmpl @@ -8,7 +8,7 @@ are .claude/rules/tasks-parallel-analysis.md): - _Depends: 3.1, 4.2_ — cross-boundary dependencies only; sequential order is implicit. - (P) — parallel-capable, runs alongside other (P) tasks in different boundaries. -Task structure follows the spec's development_flow (see spec.json; absent ⇒ tdd): +Task structure follows the spec's development_flow (see spec.json; absent ⇒ unit): - tdd / tdd-e2e: every behavior is a RED test sub-task immediately followed by the minimal GREEN implementation sub-task (write the failing test, confirm it fails for the expected reason, then the least code to pass; refactor under green); tdd-e2e also adds diff --git a/internal/validator/validator.go b/internal/validator/validator.go index 51e9e23..f84e91d 100644 --- a/internal/validator/validator.go +++ b/internal/validator/validator.go @@ -534,8 +534,8 @@ func validDevelopmentFlow(f string) bool { // A missing spec.json yields "" and every flow check stays silent: without the // file there is no declared flow to hold the tasks to, and inventing one would // manufacture findings on a correctly-shaped file. A spec.json that exists but -// omits the field resolves to "tdd", which is the default the whole toolchain -// already assumes (tasks-generation: "absent ⇒ tdd", cli.effectiveFlow). +// omits the field resolves to "unit", which is the default the whole toolchain +// already assumes (tasks-generation: "absent ⇒ unit", cli.effectiveFlow). func specDevelopmentFlow(specDir string) string { data, err := os.ReadFile(filepath.Join(specDir, "spec.json")) if err != nil { @@ -549,7 +549,7 @@ func specDevelopmentFlow(specDir string) string { } f := strings.TrimSpace(s.DevelopmentFlow) if f == "" { - return "tdd" + return "unit" } if !validDevelopmentFlow(f) { return "" // already reported where the flow is written; don't double-report here diff --git a/internal/validator/validator_test.go b/internal/validator/validator_test.go index 1acb866..d4a66e2 100644 --- a/internal/validator/validator_test.go +++ b/internal/validator/validator_test.go @@ -862,14 +862,15 @@ func TestValidateSpecFlowDefaultsAndSilence(t *testing.T) { - [ ] 1.1 GREEN — implement without a failing test first - _Requirements: 1.1_ ` - // spec.json present but no development_flow: the documented default is tdd. + // spec.json present but no development_flow: the documented default is unit, + // under which a RED/GREEN-shaped task is itself a finding. if issues := flowIssues(t, map[string]string{ "spec.json": specJSON(""), "requirements.md": validRequirements, "design.md": validDesign, "tasks.md": orphanGreen, }); len(issues) != 1 { - t.Errorf("absent development_flow should default to tdd and report the orphan GREEN, got: %v", issues) + t.Errorf("absent development_flow should default to unit and report the orphan GREEN, got: %v", issues) } // No spec.json at all: nothing declares a flow, so the checks stay silent // rather than inventing one. From 3c22d71fbb8547389b140c3cf07da30609f23681 Mon Sep 17 00:00:00 2001 From: Claude Code Date: Tue, 28 Jul 2026 04:00:37 +0000 Subject: [PATCH 2/3] fix(plan): drop unused FeatCost.wasted method (golangci-lint unused) --- internal/plan/cost.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/internal/plan/cost.go b/internal/plan/cost.go index 6957894..657f5d5 100644 --- a/internal/plan/cost.go +++ b/internal/plan/cost.go @@ -49,10 +49,6 @@ type FeatCost struct { ByStatus map[string]int `json:"by_status,omitempty"` } -// wasted is the share of a feat's sessions that were paid for and handed back. It -// is the number an optimization is trying to move. -func (f FeatCost) wasted() int { return f.Gated } - // BuildCostReport reads sessions.jsonl and aggregates it. A missing file yields an // empty report rather than an error: a plan that has never been run has spent // nothing, which is a fine thing to report. From c062c8e01de373eb9ec110426edba6f14836a951 Mon Sep 17 00:00:00 2001 From: Claude Code Date: Tue, 28 Jul 2026 04:08:34 +0000 Subject: [PATCH 3/3] feat(plan): run spec-author at high effort (judgment task, matches repo convention) --- internal/templater/templater_test.go | 6 ++++++ internal/templater/templates/agents/spec-author.md.tmpl | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/templater/templater_test.go b/internal/templater/templater_test.go index ffdf63f..ed1c671 100644 --- a/internal/templater/templater_test.go +++ b/internal/templater/templater_test.go @@ -301,10 +301,16 @@ func TestSpecAuthorAgentShipped(t *testing.T) { } // Least-privilege tools: it writes spec artifacts, so Edit/Write/Bash are in, // but it must not approve or implement. + // effort is high (not medium): spec authoring is the SDD contract — EARS, + // the design boundary map, traceability — a judgment task, and the repo runs + // every judgment sub-agent at high (code-reviewer, implementer, + // security-reviewer). A shallow first draft pushes the real authoring back + // onto the opus reviewer via fix-lists, defeating the cost split. for _, want := range []string{ "name: spec-author", "tools: Read, Grep, Glob, Edit, Write, Bash", "model: sonnet", + "effort: high", } { if !strings.Contains(body, want) { t.Errorf("spec-author.md frontmatter missing %q", want) diff --git a/internal/templater/templates/agents/spec-author.md.tmpl b/internal/templater/templates/agents/spec-author.md.tmpl index 30f6cfc..e824e3d 100644 --- a/internal/templater/templates/agents/spec-author.md.tmpl +++ b/internal/templater/templates/agents/spec-author.md.tmpl @@ -3,7 +3,7 @@ name: spec-author description: Authors ONE spec phase artifact (requirements, design, or tasks) for a feat — scaffolds it with `csdd spec generate`, consults the knowledge graph for the governing code/ADRs/stack, writes the body to EARS and traceability, validates, and reports. Sonnet writes; the orchestrator (opus) reviews, validates, and approves. Does not implement, does not approve, does not touch the approved plan. tools: Read, Grep, Glob, Edit, Write, Bash model: sonnet -effort: medium +effort: high skills: - graph color: purple