diff --git a/README.md b/README.md index b83433a..c13daf2 100644 --- a/README.md +++ b/README.md @@ -302,9 +302,9 @@ csdd plan approve ──▶ csdd plan run ──▶ Pull Request The session's verdict declares one of three intents: `done` (the whole feat is delivered and the session's own checks — `csdd spec validate`, `csdd graph analyze --strict`, the plan's Quality Gates — pass), `continue` (honest partial work; the summary is the handoff to the next session on the same feat), or `blocked` (the feat turned out to need another feat of this plan that has not landed — it parks without spending an attempt, and the discovered edge is recorded in `.csdd/plan//discovered-deps.json`, never written back into `plan.md`). -What a session is handed is **the feat, not the method**. The brief carries the feat row, its governing refs, the seeds, the Executor Notes and the plan's Quality Gates; the development process — the session's authority to approve its own phases, the cycle, what it delegates, the git it owns and what makes a `done` acceptable — lives in the plan-session `CLAUDE.md` written into each worktree and in the `plan-dev` skill, both of which the session reads every turn anyway. +What a session is handed is **the feat, not the method**. The brief opens by naming the role it is written for — the whole thing is fed to `claude -p` as the prompt, so its first lines are where the session's role is set — then carries the feat row, its governing refs, the seeds, the Executor Notes and the plan's Quality Gates; the development process — the session's authority to approve its own phases, the cycle, what it delegates, the git it owns and what makes a `done` acceptable — lives in the plan-session `CLAUDE.md` written into each worktree and in the `plan-dev` skill, both of which the session reads every turn anyway. -Before a feat is dispatched, a **context pass** (`--enrich-model`, default `sonnet`) reads its worktree once and records what the feat touches, which decisions and stack rows govern it, what is already there and what is not, into `.csdd/plan//briefs/.json`. Every path and every `adr:`/`stack:` citation is checked against the workspace before it is stored — an unverifiable claim is dropped, not rendered — and the pack is keyed to the feat row, so a feat's later attempts reuse it instead of rediscovering the same tree at the orchestrator's price. `csdd plan brief --feat F --refresh` re-runs the pass and prints the brief, which is how you review (and, by editing the JSON, correct) what a session will be handed. `--enrich-model none` turns the pass off and briefs from the plan alone. +Before a feat is dispatched, a **context pass** (`--enrich-model`, default `sonnet`) reads its worktree once and records what the feat touches, which decisions and stack rows govern it, what is already there and what is not, into `.csdd/plan//briefs/.json`. Every path and every `adr:`/`stack:` citation is checked against the workspace before it is stored — an unverifiable claim is dropped, not rendered — and the pack is keyed to the feat row, so a feat's later attempts reuse it instead of rediscovering the same tree at the orchestrator's price. `csdd plan brief --feat F` prints the brief and runs the pass itself when the feat has no pack yet, which is how you review (and, by editing the JSON, correct) what a session will be handed. A pack already on disk is reused as-is — even when the feat's row has since changed, which is only reported, since briefing is something you do repeatedly while editing a plan and regenerating costs a model call; `--refresh` is how you replace it. `--enrich-model none` turns the pass off and briefs from the plan alone. Feats are handed out in **dependency order**, not table order: the `Depends` column the plan already declares is what schedules the run, so a feat is only dispatched once everything it builds on is delivered. If a feat is blocked, everything downstream of it is reported as unreachable with the root cause named, instead of being dispatched into a tree where its foundation does not exist. @@ -328,7 +328,8 @@ csdd plan run --yes # pre-accept the unverified-sandbox --session-budget 5 # per-session USD cap · --max-iterations (100) · --stall (10) --squad-limit 3 # run up to 3 sessions at once (1..6, default 1), one git worktree each --enrich-model none # skip the per-feat context pass (default: sonnet) -csdd plan brief --feat F --refresh # re-run the context pass and print what the session gets +csdd plan brief --feat F # print what the session gets (enriches on first use) +csdd plan brief --feat F --refresh # discard the stored context pack and run the pass again ``` #### When a run ends without completing diff --git a/internal/cli/plan.go b/internal/cli/plan.go index 7eb7649..6509ecc 100644 --- a/internal/cli/plan.go +++ b/internal/cli/plan.go @@ -101,11 +101,7 @@ func planRun(args []string, templates embed.FS) int { render.Err(fmt.Sprintf("--squad-limit must be between 1 and %d (got %d)", planSquadLimitMax, squadLimit)) return 1 } - // `none` is how a human spells "no enrichment pass"; the runner reads an empty - // model as disabled. - if strings.EqualFold(strings.TrimSpace(enrichModel), "none") { - enrichModel = "" - } + enrichModel = normalizeEnrichModel(enrichModel) if autonomous { render.Warn("--autonomous is deprecated and now a no-op: plan run always runs bypass-mode") } @@ -521,14 +517,26 @@ func planNext(args []string) int { } } +// normalizeEnrichModel maps `none` — how a human spells "no context pass" — onto +// the empty model every enrichment caller already reads as disabled. It is shared +// so `plan run` and `plan brief` cannot disagree about what `none` means: for a +// while only the runner honored it, and `plan brief --enrich-model none` spawned a +// session for a model literally named "none". +func normalizeEnrichModel(model string) string { + if strings.EqualFold(strings.TrimSpace(model), "none") { + return "" + } + return model +} + func planBrief(args []string) int { fs := flag.NewFlagSet("plan brief", flag.ContinueOnError) var root, feat, enrichModel string var refresh bool addRoot(fs, &root) fs.StringVar(&feat, "feat", "", "Feat to brief (default: the sequencer's next feat).") - fs.BoolVar(&refresh, "refresh", false, "Re-run the context pass for this feat before printing, replacing its stored pack. This is the same pass `plan run` makes before dispatching a feat, so it is how you review — and, by editing .csdd/plan//briefs/.json, correct — what a session will be handed.") - fs.StringVar(&enrichModel, "enrich-model", "sonnet", "Model --refresh runs the context pass on (claude --model).") + fs.BoolVar(&refresh, "refresh", false, "Discard this feat's stored context pack and run the pass again, even when the stored one is still current. Without it the pass runs only when there is nothing usable on disk — a stored pack whose plan row has not changed is reused as-is.") + fs.StringVar(&enrichModel, "enrich-model", "sonnet", "Model the context pass runs on (claude --model). `none` turns the pass off and briefs from the plan alone.") positionals, err := parseFlags(fs, args) if err != nil { return failOnFlagParse(err) @@ -561,25 +569,56 @@ func planBrief(args []string) int { } target = f } - // --refresh runs the pass against the workspace itself rather than a worktree: - // there is no run in flight to have cut one, and the tree a human is looking at - // is the tree they are asking about. Diagnostics go to stderr so the brief on - // stdout stays a clean artifact to pipe or diff. + // The discovered half is not an extra. Without it the brief is the plan restated + // — the feat row a human already wrote — and the session rediscovers the tree at + // orchestrator prices, which is the whole cost the pass exists to avoid. So the + // pass runs BY DEFAULT here, exactly as it does before `plan run` dispatches a + // feat. `--enrich-model none` is how a caller that must not spawn a model — CI, a + // scripted diff — opts out. + // + // A pack already on disk is kept, and that is a stronger rule here than in the + // runner: it is reused even when the feat's row has moved on since it was + // written. Regenerating costs a model call, briefing is something a human does + // repeatedly while editing a plan, and re-spending on every one of those edits is + // not a default anybody would choose. A stale pack costs a line on stderr and + // `--refresh` is the way to replace it. (`plan run` keeps invalidating on a + // changed row: what it dispatches a session to build has to match what it briefed + // that session with.) + // + // The pass runs against the workspace itself rather than a worktree: there is no + // run in flight to have cut one, and the tree a human is looking at is the tree + // they are asking about. Diagnostics go to stderr so the brief on stdout stays a + // clean artifact to pipe or diff. + hook := plan.EnrichHook(normalizeEnrichModel(enrichModel)) if refresh { + if hook == nil { + render.Err("--refresh needs a model: pass --enrich-model (or drop --refresh)") + return 1 + } if err := plan.RemovePack(r, doc.Slug, target.Slug); err != nil { render.Err(err.Error()) return 1 } - hook := plan.EnrichHook(enrichModel) - if hook == nil { - render.Err("--refresh needs a model: pass --enrich-model") - return 1 + } + pack, err := plan.LoadPack(r, doc.Slug, target.Slug) + if err != nil { + render.Warn(err.Error()) + } + switch { + case pack != nil: + if pack.Key != plan.PackKey(doc, target) { + render.Warn("the stored context pack for " + target.Slug + " predates the current plan row; `--refresh` regenerates it") } + case hook != nil: if plan.EnsurePack(r, doc, target, r, hook, func(format string, a ...any) { render.Warn(strings.TrimSpace(fmt.Sprintf(format, a...))) }) == nil { render.Warn("the context pass produced nothing for " + target.Slug + "; briefing from the plan alone") } + default: + // Nothing on disk and the pass switched off: say so, rather than printing a + // brief that is silently missing half of itself. + render.Warn("no stored context pack for " + target.Slug + " and the context pass is off (--enrich-model none); briefing from the plan alone") } out, err := plan.FeatBrief(r, doc, target) if err != nil { diff --git a/internal/cli/plan_adr_test.go b/internal/cli/plan_adr_test.go index 9dd60ce..48639ba 100644 --- a/internal/cli/plan_adr_test.go +++ b/internal/cli/plan_adr_test.go @@ -87,8 +87,9 @@ func TestADRValidateE2E(t *testing.T) { } // 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) + // body via the graph; it must NOT inline the ADR title or body. The context pass + // is off so the test never spawns a `claude` — it now runs by default. + code, out, _ := run(t, "plan", "brief", "photos", "--feat", "upload", "--root", dir, "--enrich-model", "none") if code != 0 { t.Fatalf("plan brief failed: %d", code) } diff --git a/internal/cli/plan_bridge_cli_test.go b/internal/cli/plan_bridge_cli_test.go index f87564b..417679e 100644 --- a/internal/cli/plan_bridge_cli_test.go +++ b/internal/cli/plan_bridge_cli_test.go @@ -93,7 +93,9 @@ func TestPlanNextAndBriefCLI(t *testing.T) { } // brief prints the feat's mission pack: the feat itself, what governs it and the // plan's gates — the development process lives in the worktree's CLAUDE.md. - code, out, errOut := run(t, "plan", "brief", "photos", "--root", dir) + // `--enrich-model none` because the context pass now runs by DEFAULT: without it + // this test would spawn a real `claude` on any machine that has one installed. + code, out, errOut := run(t, "plan", "brief", "photos", "--root", dir, "--enrich-model", "none") if code != 0 { t.Fatalf("plan brief failed (code=%d): %s", code, errOut) } @@ -152,3 +154,70 @@ func TestPlanNextDriftExitCode(t *testing.T) { t.Errorf("drifted plan next should exit 5, got %d", code) } } + +// TestPlanBriefNamesAMissingContextPack pins the notice that replaced a silent +// omission. +// +// The discovered half of a brief — what the feat touches, what governs it, what is +// already there — is simply absent when no context pack has been stored, and the +// brief used to render without it and without a word. That is indistinguishable +// from a pass that ran and found nothing, and it is exactly how a reader concludes +// the enrichment is broken. The pass runs by default now, so the only way to reach +// that state is to turn it off — and then it has to say so. +func TestPlanBriefNamesAMissingContextPack(t *testing.T) { + dir := scaffoldApprovedPlan(t) + code, _, errOut := run(t, "plan", "brief", "photos", "--root", dir, "--enrich-model", "none") + if code != 0 { + t.Fatalf("plan brief exit=%d: %s", code, errOut) + } + if !strings.Contains(errOut, "no stored context pack") { + t.Errorf("a brief with no pack and the pass switched off must say so on stderr:\n%s", errOut) + } +} + +// TestPlanBriefRefreshRefusesWithNoModel keeps --refresh honest: it forces the pass +// to run, so asking for it with the pass turned off is a contradiction the CLI must +// reject rather than silently print a stale brief for. +func TestPlanBriefRefreshRefusesWithNoModel(t *testing.T) { + dir := scaffoldApprovedPlan(t) + code, _, errOut := run(t, "plan", "brief", "photos", "--root", dir, "--refresh", "--enrich-model", "none") + if code != 1 { + t.Fatalf("--refresh with no model should exit 1, got %d: %s", code, errOut) + } + if !strings.Contains(errOut, "--refresh needs a model") { + t.Errorf("the refusal should name the flag conflict:\n%s", errOut) + } +} + +// TestPlanBriefKeepsAStoredPackEvenWhenStale pins the reuse rule that separates +// `plan brief` from the runner: a pack on disk is what the human gets, whether or +// not the feat's row has changed since it was written. +// +// Regenerating is a model call, and briefing is what a human does over and over +// while editing a plan — invalidating on every edit turns reading a brief into a +// recurring charge. So the staleness is REPORTED and the pack is still used; +// `--refresh` is the way to replace it. The pass is switched off here so the test +// spawns nothing: with a pack present it would not run either way, which is the +// property under test. +func TestPlanBriefKeepsAStoredPackEvenWhenStale(t *testing.T) { + dir := scaffoldApprovedPlan(t) + packs := filepath.Join(dir, ".csdd", "plan", "photos", "briefs") + if err := os.MkdirAll(packs, 0o755); err != nil { + t.Fatal(err) + } + // `key` is deliberately not the current plan row's key: this pack is stale. + pack := `{"touches":[{"path":"app/upload.go","why":"the upload handler lives here"}],"key":"stale"}` + if err := os.WriteFile(filepath.Join(packs, "upload.json"), []byte(pack), 0o644); err != nil { + t.Fatal(err) + } + code, out, errOut := run(t, "plan", "brief", "photos", "--root", dir, "--enrich-model", "none") + if code != 0 { + t.Fatalf("plan brief exit=%d: %s", code, errOut) + } + if !strings.Contains(out, "the upload handler lives here") { + t.Errorf("the stored pack must be rendered into the brief:\n%s", out) + } + if !strings.Contains(errOut, "predates the current plan row") { + t.Errorf("a stale pack must be reported, not silently reused:\n%s", errOut) + } +} diff --git a/internal/plan/brief.go b/internal/plan/brief.go index a91c31d..ca76a81 100644 --- a/internal/plan/brief.go +++ b/internal/plan/brief.go @@ -11,9 +11,9 @@ import ( "github.com/protonspy/csdd/internal/paths" ) -// FeatBrief assembles the mission pack for one whole feat (R7): what this feat is, -// what governs it, what it is likely to touch, and what verifies it. It does NOT -// carry the development process — how a session authors and approves its spec +// FeatBrief assembles the mission pack for one whole feat (R7): who is reading it, +// what this feat is, what governs it, what it is likely to touch, and what verifies +// it. It does NOT carry the development process — how a session authors and approves its spec // phases, what it may delegate, what git it owns and what makes a `done` acceptable // live in the plan-session CLAUDE.md the runner writes into every worktree and in // the `plan-dev` skill, both of which the session already reads every turn. Stating @@ -40,6 +40,17 @@ func FeatBrief(root string, doc *PlanDoc, feat Feat) (string, error) { var b strings.Builder w := func(format string, a ...any) { fmt.Fprintf(&b, format, a...) } + // 0. Who is reading this. The brief is fed to `claude -p` on stdin as the entire + // prompt, so its opening lines are the only place the session's role is set — + // without them the first thing the model sees is a feat row, and it has to infer + // from context what it is supposed to be. It stays four lines on purpose: the + // process this engineer follows is NOT here (see the pointer at the end), and a + // preamble that grows is re-read on every turn of every session. + w("You are a senior software engineer. You deliver ONE feat of an approved plan to\n") + w("completion on your own, in an autonomous session with no human at the gate.\n") + w("Everything below is the mission: what this feat is, what governs it, where it\n") + w("lives in this repository, and what verifies it.\n\n") + // 1. The feat itself. w("# Feat: %s — plan %s\n\n", feat.Slug, doc.Slug) w("- Objective: %s\n", orDash(feat.Objective)) diff --git a/internal/plan/brief_test.go b/internal/plan/brief_test.go index b97f823..f0d225a 100644 --- a/internal/plan/brief_test.go +++ b/internal/plan/brief_test.go @@ -185,3 +185,39 @@ func TestBriefWithoutPackIsPlanOnly(t *testing.T) { t.Errorf("brief should still carry the feat's objective:\n%s", out) } } + +// TestBriefOpensByNamingTheRole pins the first four lines. +// +// The brief IS the prompt — it is fed to `claude -p` on stdin with nothing in front +// of it — so whatever it opens with is where the session's role is established. It +// used to open on a feat row, leaving the model to infer what it was supposed to be +// from a table of objectives. The preamble is deliberately short: it names the role +// and the shape of what follows, and stops. Process belongs to the plan-session +// CLAUDE.md and the `plan-dev` skill (see TestBriefCarriesNoProcess), so a preamble +// that starts explaining the cycle is this boundary eroding again. +func TestBriefOpensByNamingTheRole(t *testing.T) { + out := briefFor(t, setupWorkspace(t, "p", briefPlan), "p", "upload") + if !strings.HasPrefix(out, "You are a senior software engineer.") { + t.Errorf("the brief must open by naming the role it is written for:\n%s", firstLines(out, 6)) + } + // The role sits BEFORE the feat header — everything after it reads as material + // for that role rather than as a document that happened to arrive. + role, feat := strings.Index(out, "You are a senior software engineer"), strings.Index(out, "# Feat:") + if feat < 0 || role > feat { + t.Errorf("the role must come before the feat header:\n%s", firstLines(out, 8)) + } + // Four lines, then the mission. A preamble long enough to need a heading has + // stopped being a preamble. + if head, _, _ := strings.Cut(out, "# Feat:"); strings.Count(head, "\n") > 6 { + t.Errorf("the role preamble should stay short, got:\n%s", head) + } +} + +// firstLines returns at most n lines of s, for readable failure output. +func firstLines(s string, n int) string { + lines := strings.Split(s, "\n") + if len(lines) > n { + lines = lines[:n] + } + return strings.Join(lines, "\n") +}