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..657f5d5 --- /dev/null +++ b/internal/plan/cost.go @@ -0,0 +1,146 @@ +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"` +} + +// 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..ed1c671 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,68 @@ 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. + // 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) + } + } + 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..e824e3d --- /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: high +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.