diff --git a/CLAUDE.md b/CLAUDE.md index 71ad1da..03d1712 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -12,7 +12,7 @@ This file covers working *on* scc. The product's own rules and methodology are n Note: this repo is not itself an scc workspace (no harness directory, `specs/`, `plans/`, or `docs/` are committed) — those trees only exist in workspaces the binary scaffolds, and in test temp dirs. -**Status: v0.4.0-shaped.** Everything through `design/plan.md` phase 10 is built and green: scaffolding (`init`), artifact creation (`spec`, `plan`), and all eight validators behind `scc validate`. `init` also scaffolds the six knowledge-base skills named in `design/orchestration.md` §6 — one per `docs/` artifact a validator checks, plus `prd` — each with a `scc-`-prefixed slash command derived from the same list (`assets.KnowledgeSkills`), wherever the harness has a command surface. +**Status: v0.4.0-shaped.** Everything through `design/plan.md` phase 10 is built and green: scaffolding (`init`), artifact creation (`spec`, `plan`), and all eight validators behind `scc validate`. `init` also scaffolds the seven skills named in `design/orchestration.md` §6 — the six knowledge-base authors (one per `docs/` artifact a validator checks, plus `prd`) and the `plan-run` workflow skill — each with a `scc-`-prefixed slash command derived from the same list (`assets.Skills()`, which is `KnowledgeSkills` + `WorkflowSkills`), wherever the harness has a command surface. Three things landed after phase 10 and all are documented in `design/orchestration.md` §6 and §12: diff --git a/README.md b/README.md index 65c0ce0..3c074fc 100644 --- a/README.md +++ b/README.md @@ -36,8 +36,9 @@ Installed globally (`npm i -g @protonspy/scc`) the same commands are just `scc i ### Three harnesses, one methodology -The same rules, review agents, and knowledge-base skills are scaffolded into -whichever tool you work in — only the paths and the frontmatter dialect change. +The same rules, review agents, and skills — the knowledge base's authors, plus +`plan-run`, which drives a whole plan group by group — are scaffolded into whichever +tool you work in. Only the paths and the frontmatter dialect change. | | Claude Code | Codex | opencode | |---|---|---|---| diff --git a/design/orchestration.md b/design/orchestration.md index 9e82462..741e533 100644 --- a/design/orchestration.md +++ b/design/orchestration.md @@ -415,7 +415,7 @@ merge base stays exact. **Codex ships no slash commands, deliberately.** Its custom prompts are user-global and now deprecated in favor of skills, so there is nothing project-scoped to write -and scc does not write into anyone's home directory. The six skills are the whole +and scc does not write into anyone's home directory. The skills are the whole surface there, which is what Codex itself recommends. **Codex and opencode share `AGENTS.md`, and the first one initialized wins it.** A @@ -431,10 +431,10 @@ rewriting a file the user owns. `scc update` keeps both trees current regardless, because it works from each harness's own manifest rather than from the entry file. -### Which skills ship — the authors of `docs/` +### Which skills ship — the authors of `docs/`, plus one runner -Six, and the rule that picks them is mechanical: **a skill ships for each `docs/` -artifact a validator checks, and for nothing else.** +Seven, in two lists, and each list has its own rule. The first is mechanical: **a +skill ships for each `docs/` artifact a validator checks, and for nothing else.** | Skill | Authors | Checked by | |---|---|---| @@ -455,6 +455,61 @@ delivery are rules under `.claude/rules/` (§3, §7, §9) — read when the conc live. csdd shipped `tdd-cycle`, `unit-cycle`, and `verify-change` as skills; here that would be a second copy of a rule, and the copy is the one that goes stale. +**The one exception, and the line it draws — `plan-run`.** The second list holds +skills that *run* the methodology rather than describe it, and it holds exactly one. + +| Skill | Runs | Owned by no rule | +|---|---|---| +| `plan-run` | a whole `plans/.md`, group by group | the loop **across** units of work | + +The rule above is what makes it admissible rather than a leak. §9's delivery sequence +ends at one merged pull request, because that is where one unit of work ends — a spec, +or a plan's leaf. A plan is many of those in sequence, and running one to the end means +picking the next group, branching it from the merge the previous group produced, +holding CI green because every later group inherits that base, and recovering the +position from `main` when a session dies mid-plan. None of that is in any rule, and +none of it belongs in one: a rule is read when a concern is live, and this is a +procedure a person starts on purpose and lets run. + +So the line is not "methodology may now be a skill." It is: **a skill may hold a +procedure no rule holds; it may never restate one that a rule does.** `plan-run` +therefore cites `delivery.md` for the single-group mechanics instead of copying +them — if it ever grows its own account of how to open a PR, it has crossed back over. + +**It has its own kickoff, and that is not a violation of §4 — it is §4 applied.** The +rule there is *ask once, before doing anything, and record the answer*; the mistake it +forbids is asking repeatedly, not asking at all. A plan's recorded `autonomy` and `ci` +were given when the plan was **written**. Starting a loop that will open and merge pull +requests for hours is a different and larger thing to agree to, and it raises a +question authoring never had — worktree per group, or the checkout you are in. + +So `plan-run` asks three questions, once, **after reporting the groups**, because the +answers are only meaningful to someone who can see what they are agreeing to. Anything +already in the frontmatter is shown as a proposed answer to confirm, never as a +decision already taken. Four keys go back into the plan, and a resumed session reads +them instead of asking again: + +| Key | Values | Decides | +|---|---|---| +| `autonomy` | `auto` · `gated` | straight through, or stop at each group boundary | +| `worktree` | `per-group` · `in-place` | a worktree per group, or a branch in the current checkout | +| `ci` | `wait` · `no-wait` | whether the checks have to settle before the merge | +| `merge` | `auto` · `manual` | whether the loop merges, or stops at the PR for the developer | + +`worktree` and `merge` are new, plan-only, and validated by `plan` on exactly the terms +`autonomy` and `ci` are — **only when present**, since a plan no loop has run over is +not a plan with a defect. They are validated at all because the skill writes them and +every later session reads them, so `worktree: yes` would silently decide how the next +ten groups get built. + +The tempting alternative was for the loop to **override** `ci: no-wait`, on the +argument that the next group branches from the merge and an unverified base poisons +everything after it. Rejected: the argument is sound and the developer is still the one +who gets to weigh it. Stating the cost at the moment of the question is how a tool +respects a decision it disagrees with; overriding is how it stops being trusted. +`no-wait` therefore stays available inside a loop, and the skill says out loud what it +buys and what it costs before accepting it. + **One slash command per skill, namespaced `scc-`.** A skill is model-invoked through its description; a command is the human saying *now*. Both are cheap, they are derived from one list so they cannot drift, and the prefix is not decoration — slash commands @@ -465,6 +520,30 @@ The skills carry the *procedure and the judgment*; the format they produce stays `.claude/rules/knowledge-base.md`, stated once. A skill that restated the format would be the same duplication the methodology rules are being kept out of skills to avoid. +**How a skill names a rule — the path from the root, and never a link.** Every +template writes a rule's path the same way, `/rules/.md`, expanded at +render time so it is correct in each of the three trees. A `SKILL.md` is no exception, +but it must write that path as **inline code rather than as a Markdown link**, and the +two constraints that force it point in opposite directions: + +- An agent reads files from the workspace root. `/rules/delivery.md` is the + path it can act on directly; a relative walk like `../../rules/delivery.md` is one + the agent has to resolve against a directory it was never told it was in. +- `skill` resolves a Markdown link **against the skill's own directory** and holds it + to the Agent Skills spec's one-level-deep rule. The same path written as a link + would resolve to `/skills///rules/…`, and scc would report two + findings against its own scaffold. + +Inline code satisfies both: correct to read, and not a link, so the check that would +misjudge it never sees it. This was got wrong once in the other direction — skills +linking relatively, which conformed but handed the agent a path it could not use. + +Guarded from both sides, because neither guard alone is enough. +`TestFreshArtifactsPassTheirOwnValidators` catches the link form, since a validator +firing on scc's own output is the one defect that teaches users to disbelieve the +other seven. `TestSkillsNameRulesByTheirHarnessPath` catches what no validator can +see — a `../..` sitting inside a code span is silently useless to whoever reads it. + ### The four seeded anchors — `docs/` is not an empty tree `init` writes `docs/glossary.md`, `docs/stack.md`, `docs/wiki/index.md`, and diff --git a/internal/assets/assets.go b/internal/assets/assets.go index 7e77000..63b8c7f 100644 --- a/internal/assets/assets.go +++ b/internal/assets/assets.go @@ -62,7 +62,9 @@ import ( // report in a fixed shape the orchestrator can act on. // 5: one template set for three harnesses — Claude Code, Codex, opencode. // 6: the knowledge base states the stack check without naming an ecosystem. -const Version = "6" +// 7: the plan-run workflow skill and its command — running a whole plan, group by +// group, is a procedure with an entry point rather than a rule read in passing. +const Version = "7" // The embedded tree. "all:" so nothing is silently dropped for having a name the // default embed pattern skips. @@ -148,11 +150,10 @@ func Workspace(h paths.Harness) []File { Kind: Agent, }) } - // The knowledge base's authors. Every one of these produces an artifact - // `scc validate` already checks — a workspace that ships the eight validators - // and no skill teaching the formats would demand conformance to documents - // nobody was told how to write. - for _, skill := range KnowledgeSkills { + // The knowledge base's authors, and the workflow skills that run the + // methodology rather than describe it. Why each list holds what it holds is on + // the lists themselves; here they are one set of files on identical terms. + for _, skill := range Skills() { set = append(set, File{ Name: "skills/" + skill + "/SKILL.md", Rel: under(h.SkillsSeg, skill, "SKILL.md"), @@ -224,9 +225,9 @@ func Seeds() []Seed { // review is where a cold context is worth paying for, and authorship is not. var ReviewAgents = []string{"code-review", "security-review"} -// KnowledgeSkills names the skills scc ships, each one the author of a `docs/` -// artifact a validator checks. Both the skill directory and its slash command are -// derived from this list, so the two cannot drift apart. +// KnowledgeSkills names the skills that author a `docs/` artifact a validator +// checks — one per artifact, so a workspace shipping the eight validators never +// demands conformance to a document nobody was told how to write. // // The methodology is deliberately absent from it: the cycles, verification, and // delivery are rules under the harness's rules directory, read when the concern @@ -234,6 +235,26 @@ var ReviewAgents = []string{"code-review", "security-review"} // goes stale. var KnowledgeSkills = []string{"adr", "codewiki", "glossary", "prd", "stack", "wiki"} +// WorkflowSkills names the skills that drive the methodology instead of authoring a +// document. There is one, and the bar for a second is the rule above: if a skill +// would restate what a rule already says, it should be the rule. +// +// plan-run clears that bar because it holds what no rule does — the loop *across* +// units of work. delivery.md ends at one merged pull request, which is exactly where +// one unit of work ends; running a whole plan means picking the next group, branching +// it from the merge the last group produced, and recovering the position from `main` +// when a session dies mid-plan. That is a procedure, and a procedure a person invokes +// needs an entry point they can type. +var WorkflowSkills = []string{"plan-run"} + +// Skills is every skill scc ships, knowledge first. Both the skill directory and its +// slash command are derived from this one list, so the two cannot drift apart, and a +// skill added to either half above reaches workspaces that already exist through +// `scc update` on the same terms as any other managed file. +func Skills() []string { + return append(append([]string{}, KnowledgeSkills...), WorkflowSkills...) +} + // commandPrefix namespaces the scaffolded slash commands. const commandPrefix = "scc-" diff --git a/internal/assets/assets_test.go b/internal/assets/assets_test.go index e6f789d..67fbf4f 100644 --- a/internal/assets/assets_test.go +++ b/internal/assets/assets_test.go @@ -1,6 +1,7 @@ package assets import ( + "path" "strings" "testing" @@ -376,11 +377,11 @@ func TestAgentProseIsSafeToEmbedInTOML(t *testing.T) { } } -// Every skill in KnowledgeSkills ships a SKILL.md in every harness, and its slash -// command wherever the harness has a command surface. The two are derived from one -// list so they cannot drift, and this is the assertion that keeps the derivation -// honest — including for Codex, where the answer is deliberately no commands at -// all. +// Every skill in Skills() — knowledge and workflow alike — ships a SKILL.md in every +// harness, and its slash command wherever the harness has a command surface. The two +// are derived from one list so they cannot drift, and this is the assertion that +// keeps the derivation honest — including for Codex, where the answer is +// deliberately no commands at all. func TestEverySkillShipsWithItsCommand(t *testing.T) { for _, h := range paths.Harnesses() { skills, commands := map[string]bool{}, map[string]bool{} @@ -394,19 +395,20 @@ func TestEverySkillShipsWithItsCommand(t *testing.T) { commands[name] = true } } - if len(skills) != len(KnowledgeSkills) { - t.Fatalf("%s: %d skills, want %d", h.ID, len(skills), len(KnowledgeSkills)) + all := Skills() + if len(skills) != len(all) { + t.Fatalf("%s: %d skills, want %d", h.ID, len(skills), len(all)) } - wantCommands := len(KnowledgeSkills) + wantCommands := len(all) if h.CommandsSeg == "" { wantCommands = 0 } if len(commands) != wantCommands { t.Fatalf("%s: %d commands, want %d", h.ID, len(commands), wantCommands) } - for _, name := range KnowledgeSkills { + for _, name := range all { if !skills[name] { - t.Errorf("%s: %s is in KnowledgeSkills and ships no SKILL.md", h.ID, name) + t.Errorf("%s: %s is in Skills() and ships no SKILL.md", h.ID, name) } if wantCommands > 0 && !commands[commandPrefix+name] { t.Errorf("%s: %s ships no %s%s command", h.ID, name, commandPrefix, name) @@ -415,6 +417,37 @@ func TestEverySkillShipsWithItsCommand(t *testing.T) { } } +// A skill names a rule by its path from the workspace root — `.claude/rules/x.md`, +// expanded per harness — and never by a relative walk out of its own directory. +// +// Two things have to hold at once, and only one form satisfies both. An agent reads +// files from the root, so the harness-relative path is the one it can act on. But +// `skill` resolves a Markdown link against the skill's own directory and holds it to +// the Agent Skills spec's one-level-deep rule, so writing that same path as a *link* +// would make scc fail its own validator. Inline code is the form that is both correct +// to read and invisible to the link check. +// +// TestFreshArtifactsPassTheirOwnValidators in internal/cli catches the link half of +// this; this test catches the other half, which no validator can see — a `../..` that +// stays inside a code span is silently useless to the agent reading it. +func TestSkillsNameRulesByTheirHarnessPath(t *testing.T) { + for _, h := range paths.Harnesses() { + for _, f := range Workspace(h) { + if !strings.HasSuffix(f.Rel, "/SKILL.md") { + continue + } + out, err := Render(h, f) + if err != nil { + t.Fatalf("%s %s: %v", h.ID, f.Rel, err) + } + if strings.Contains(out, "../") { + t.Errorf("%s %s: reaches out of the skill directory with \"../\"; name the rule as %s/.md", + h.ID, f.Rel, path.Join(h.Dir, h.RulesSeg)) + } + } + } +} + // The Agent Skills contract is what `scc skill validate` enforces on everyone else, // and the two fields it can break loading on are checked here at the source. The // full conformance run happens against a scaffolded workspace in internal/cli — a diff --git a/internal/assets/templates/commands/scc-plan-run.md b/internal/assets/templates/commands/scc-plan-run.md new file mode 100644 index 0000000..3aab60d --- /dev/null +++ b/internal/assets/templates/commands/scc-plan-run.md @@ -0,0 +1,22 @@ +--- +description: Run a plan under plans/ to completion group by group — worktree, implement, review, PR, CI, merge, then the next group +argument-hint: [the plan, plus any standing instruction for every group] +--- + +Use the `plan-run` skill. + +Plan, and how to run it: $ARGUMENTS + +Read the plan and name the groups back, numbered and in order, before writing any +code. The order is the one thing the user can correct cheaply now and expensively +after three merges. + +Then ask how this loop should run — automatic or gated, a worktree per group or the +current checkout, and what happens once each PR is open. Three questions, one +exchange, after they can see the groups they are agreeing to. **These are the +developer's calls.** Anything the plan's frontmatter already records is a proposed +answer to confirm, not a decision already made. + +Anything said above about *how* to implement is a standing instruction: it applies +to every group, and you carry it into each one explicitly rather than trusting it to +survive from the first group to the last. diff --git a/internal/assets/templates/entry.md b/internal/assets/templates/entry.md index d731326..d8899fd 100644 --- a/internal/assets/templates/entry.md +++ b/internal/assets/templates/entry.md @@ -30,10 +30,10 @@ plans/.md one file: structure, plus a checklist and/or spec references docs/ the knowledge base — wiki, adr, codewiki, glossary, stack {{.Rules}}/ — the methodology above -{{.Skills}}/ — how to author each part of docs/, invoked when it applies +{{.Skills}}/ — how to author each part of docs/, and how to run a plan to the end {{.Agents}}/ — code-review and security-review, run before the PR {{- if .HasCommands}} -{{.Commands}}/ — the same skills on demand, as /scc-wiki, /scc-adr, /scc-prd, … +{{.Commands}}/ — the same skills on demand, as /scc-plan-run, /scc-wiki, /scc-adr, … {{- end}} ``` diff --git a/internal/assets/templates/skills/adr/SKILL.md b/internal/assets/templates/skills/adr/SKILL.md index ae737f9..fd9adb5 100644 --- a/internal/assets/templates/skills/adr/SKILL.md +++ b/internal/assets/templates/skills/adr/SKILL.md @@ -5,7 +5,7 @@ description: Record a decision that is hard to reverse as a numbered ADR under d You write the record of a decision, so that the reasoning survives the people who were in the room. The filename shape, the frontmatter, and the citation form are in -[knowledge-base.md](../../rules/knowledge-base.md). This skill is when to write one +`{{.Rules}}/knowledge-base.md`. This skill is when to write one and how to write one worth reading. ## Is this an ADR? diff --git a/internal/assets/templates/skills/codewiki/SKILL.md b/internal/assets/templates/skills/codewiki/SKILL.md index 08bb802..3e2616f 100644 --- a/internal/assets/templates/skills/codewiki/SKILL.md +++ b/internal/assets/templates/skills/codewiki/SKILL.md @@ -8,7 +8,7 @@ is about. The citation is what makes this different from a wiki page: it goes st loudly. When the code moves, `scc validate` fails instead of the page quietly becoming a lie. -The format is in [knowledge-base.md](../../rules/knowledge-base.md). This skill is +The format is in `{{.Rules}}/knowledge-base.md`. This skill is the procedure and the judgment. ## The citation form diff --git a/internal/assets/templates/skills/glossary/SKILL.md b/internal/assets/templates/skills/glossary/SKILL.md index 74dbba0..f127bc9 100644 --- a/internal/assets/templates/skills/glossary/SKILL.md +++ b/internal/assets/templates/skills/glossary/SKILL.md @@ -8,7 +8,7 @@ working in parallel produce three names for one thing inside a week, and every o of them ends up in code, requirements, and the wiki, where nobody can tell whether two documents are describing the same concept or two different ones. -The entry format is in [knowledge-base.md](../../rules/knowledge-base.md). This +The entry format is in `{{.Rules}}/knowledge-base.md`. This skill is when to reach for it and how to decide. ## The entry diff --git a/internal/assets/templates/skills/plan-run/SKILL.md b/internal/assets/templates/skills/plan-run/SKILL.md new file mode 100644 index 0000000..04bdad4 --- /dev/null +++ b/internal/assets/templates/skills/plan-run/SKILL.md @@ -0,0 +1,163 @@ +--- +name: plan-run +description: Drive a whole plan under plans/ to completion group by group — read the plan, report the groups, ask the developer how the loop should run, then for each group branch from a green main, implement, open the PR, settle CI, merge, pull, and take the next. Resumes from what is on main rather than from memory. Use it when someone asks to implement an entire plan, to keep going until the plan is finished, or runs /scc-plan-run. Not for a single spec or a one-off change, which delivery.md already carries end to end on its own. +--- + +You run a plan to the end, one merged pull request per group. + +The mechanics of delivering *one* unit of work are not here — they are in +`{{.Rules}}/delivery.md`, and repeating them would give this project two +copies of one procedure. This skill owns only what a loop adds on top: choosing the +next group, branching every group from the merge the last one produced, knowing when +to stop, and finding your place again after a session dies. + +And it owns one thing that is nobody's default to assume — **how the loop runs is the +developer's decision, taken once, after they have seen the groups.** + +## What a group is + +A **group** is the smallest part of the plan that can merge on its own. + +| In the plan | One group is | What implementing it means | +|---|---|---| +| `## Decomposition` | one leaf, `specs//` | an ordinary spec — the whole cycle, by the ordinary rules | +| `## Tasks` | one family of task numbers sharing a major number (`1.1`, `1.2` → group 1) | those tasks, in order | + +The order is the order they are written in, unless `## Notes` says otherwise. Notes +wins — that heading exists precisely to say what must not be merged out of sequence. + +A plan with a flat, unnumbered checklist has exactly one group. Say so and run it +once, rather than inventing a decomposition the author did not write. + +## Before the first group — read, report, then ask + +1. **Read the plan and work out the groups.** Ask nothing yet. The questions below + are only answerable by someone who can see what they are agreeing to. +2. **Name the groups back, numbered, in order.** Order is the one thing a person can + correct cheaply now and expensively after three merges. +3. **Then ask, once, in one exchange — all three questions together.** How this loop + runs is the developer's call, not yours, and not a default you inferred from a + file. + +| Ask | Answers | Recorded as | +|---|---|---| +| Run every group straight through, or stop at each group boundary for review? | automatic · gated | `autonomy: auto` · `gated` | +| A git worktree per group, or a branch in the checkout you are already in? | worktree · in place | `worktree: per-group` · `in-place` | +| Once a group's PR is open — wait for CI and merge, merge without waiting, or stop and let me merge? | wait and merge · merge now · stop at the PR | `ci` + `merge`, below | + +The plan's frontmatter may already carry `autonomy` and `ci` from when it was written. +**Show those as the proposed answers and confirm them; do not ask blind, and do not +treat them as decided.** They were given for authoring the plan — this is a loop that +will open and merge pull requests for hours, which is a larger thing to agree to. + +Say what each answer costs, in one line each, because two of them have a consequence +that only shows up later: + +- **wait and merge** (`ci: wait`, `merge: auto`) — the loop runs to the end unattended. +- **merge now** (`ci: no-wait`, `merge: auto`) — fastest, and every later group + branches from a base CI never checked. Say this out loud before accepting it. +- **stop at the PR** (`merge: manual`) — the loop pauses after every group until the + merge lands. It is no longer unattended, and that is a legitimate thing to want. +- **in place** (`worktree: in-place`) — one directory, and this session cannot run + alongside another on the same repo. Right when the project's setup is expensive to + duplicate; wrong when the user is running several features at once. + +**Write all four answers into the plan's frontmatter before starting**, then never ask +again for this plan: + +```yaml +--- +autonomy: auto +ci: wait +worktree: per-group +merge: auto +--- +``` + +That is what makes a resumed session pick up where this one stopped instead of +interrogating the developer a second time. `scc validate` checks the values. + +4. If the invocation carried a standing instruction, restate it and apply it to + every group — carry it into each group explicitly, rather than trusting it to + survive the context between the first group and the fifth. + +## The loop + +For each group, in order: + +1. **Start green.** In the primary checkout, `git switch main && git pull --ff-only`. + Every group branches from the previous group's merge, which is the whole reason + this is a loop and not a fan-out. +2. **Branch**, in a worktree or in place, as `worktree:` was answered. The worktree + mechanics are `{{.Rules}}/delivery.md`'s; `in-place` means the same + branch without the worktree, and it means you must leave the checkout on `main` + and clean when the group ends. +3. **Implement the group.** A leaf is a spec and gets the spec cycle. A task family + is its tasks, sequential, each verified before the next. +4. **Deliver**, following delivery.md's sequence in full — suite and lint, `scc + validate`, both review subagents, commit, push, open the PR. +5. **Record the group's state in that same PR.** A task group's checkboxes are ticked + in the plan file, in the branch that does the work, so `main` and the plan agree + the moment the merge lands. A leaf is never ticked — its state lives in the spec + and is read from there. +6. **CI and merge, exactly as answered.** `ci: wait` means watch the checks until they + settle and fix what is red before merging. `merge: auto` means you merge once that + answer is satisfied; `merge: manual` means you open the PR, say where it is, and + stop — the loop resumes when the developer's merge is on `main`. +7. **Back to a green main** — `git switch main && git pull --ff-only`. Every group + branches from the previous group's merge, which is what makes this a loop. +8. **Remove the worktree** now that its branch has landed, if there was one. +9. **Report the group in one line**, then start the next. + +## Where the loop stops + +Stopping part-way is a legitimate outcome, and continuing past any of these is not: + +- A review subagent returns `blocked`, or any `blocker`/`critical` finding survives a + round of fixes. +- CI is red twice on the same cause. A second identical failure is a person's + problem, not a third push. +- The branch conflicts with `main` in a way that is not mechanical — something landed + that the plan did not account for. +- The group turns out to need a decision the plan never made. Bring the decision back; + do not make it silently in the middle of a loop nobody is watching. + +Under `autonomy: gated`, stop at every group boundary and wait, having reported what +merged. + +Whenever you stop, say which groups merged, which one is open and where, and which +are untouched. A half-run plan the user cannot locate is worse than one that never +started. + +## Resuming + +Derive your position from `main`, never from what you remember. A compaction, a +crash, or a fresh session must land in the same place: + +- Pull `main` and re-read the plan **there** — the copy in an old worktree is stale by + construction. +- **The four answers are in that frontmatter. Read them and carry on — do not ask + again.** They were the developer's call once; asking a second time because your + context died makes them pay for your problem. +- A task group whose boxes are ticked on `main` is done. +- A leaf whose spec's `tasks.md` is fully ticked on `main` is done. +- An open PR for this plan means that group is mid-flight. Under `merge: manual` that + is the expected resting state and the merge may simply not have happened yet. + Finish it before starting another; two open groups is the fan-out this loop exists + to avoid. +- A leftover worktree whose branch is already merged is debris. Remove it. + +A plan whose frontmatter carries no `worktree` or `merge` is a plan no loop has run +over. Ask the three questions. + +## Degrading + +- **No remote, or no `gh`.** delivery.md stops at the branch, and a loop cannot — + the next group needs the last one on `main`. Say so *when you ask the three + questions*, not after the first group is written: "there is no remote here, so this + runs local-only — each group merges into your `main` with `git merge --no-ff`, no PR + is opened and no CI runs." That turns a degraded run into something the developer + chose. Never let it look like every group was reviewed. +- **The plan grows while the loop runs.** Re-read it at each group boundary. The + group list from step 2 is a report, not a contract, and a group appended after you + started is still part of the plan. diff --git a/internal/assets/templates/skills/prd/SKILL.md b/internal/assets/templates/skills/prd/SKILL.md index c4e5d84..9546dae 100644 --- a/internal/assets/templates/skills/prd/SKILL.md +++ b/internal/assets/templates/skills/prd/SKILL.md @@ -9,12 +9,12 @@ file under `plans/`, holding the decomposition and nothing else. **A note on the word.** "PRD" is what you arrive with — a product document, a roadmap line, a paragraph in a chat. It is not an artifact this project keeps. The artifact is a **plan**, and the vocabulary in -[routing.md](../../rules/routing.md) is what the rest of the workspace uses. Do not +`{{.Rules}}/routing.md` is what the rest of the workspace uses. Do not create a `docs/prd/` directory. ## First, check this is a plan at all -[routing.md](../../rules/routing.md) asks one question, and it is worth asking +`{{.Rules}}/routing.md` asks one question, and it is worth asking before you start: does this need requirements and a design settled before any code? - **Too large for one spec** — decomposes into several. That is a plan. Continue. diff --git a/internal/assets/templates/skills/stack/SKILL.md b/internal/assets/templates/skills/stack/SKILL.md index 720902a..efc4de1 100644 --- a/internal/assets/templates/skills/stack/SKILL.md +++ b/internal/assets/templates/skills/stack/SKILL.md @@ -4,7 +4,7 @@ description: Own docs/stack.md — every adopted technology with one line on why --- You own `docs/stack.md`. The rule it enforces is in -[knowledge-base.md](../../rules/knowledge-base.md), and it is short: **technology +`{{.Rules}}/knowledge-base.md`, and it is short: **technology not listed here is an open decision, never something adopted silently.** This is checkable because dependency manifests are structured data. `scc validate` diff --git a/internal/assets/templates/skills/wiki/SKILL.md b/internal/assets/templates/skills/wiki/SKILL.md index e9be4b4..b73c875 100644 --- a/internal/assets/templates/skills/wiki/SKILL.md +++ b/internal/assets/templates/skills/wiki/SKILL.md @@ -7,7 +7,7 @@ You own `docs/wiki/`: the durable half of what this project knows. A spec says w one feature does now; the wiki says what is true across features and outlasts them. The format is not yours to invent — it is in -[knowledge-base.md](../../rules/knowledge-base.md), and `scc validate` enforces it. +`{{.Rules}}/knowledge-base.md`, and `scc validate` enforces it. This skill is the procedure. ## Ingest — something landed in `docs/raw/` diff --git a/internal/cli/validate_test.go b/internal/cli/validate_test.go index 6bb1413..a1c1198 100644 --- a/internal/cli/validate_test.go +++ b/internal/cli/validate_test.go @@ -67,8 +67,8 @@ func TestScaffoldedSkillsPassTheSkillValidator(t *testing.T) { got = append(got, e.Name()) } } - if len(got) != len(assets.KnowledgeSkills) { - t.Errorf("scaffolded skills = %v, want %v", got, assets.KnowledgeSkills) + if len(got) != len(assets.Skills()) { + t.Errorf("scaffolded skills = %v, want %v", got, assets.Skills()) } stdout, stderr, code := run(t, "skill", "validate", "--root", root, "--json") diff --git a/internal/scaffold/update_test.go b/internal/scaffold/update_test.go index 22d9d7c..8c0843d 100644 --- a/internal/scaffold/update_test.go +++ b/internal/scaffold/update_test.go @@ -2,6 +2,7 @@ package scaffold import ( "os" + "path" "path/filepath" "testing" @@ -243,3 +244,88 @@ func TestApplyUpdateDropsUnmanagedFiles(t *testing.T) { } } } + +// A skill that did not exist when the workspace was scaffolded reaches it through +// `scc update`, in every harness that has the surface for it. +// +// This is the path a new skill actually ships on, and it is the one with something +// to get wrong: the destination is a directory that does not exist yet, so a create +// that only wrote files would fail on precisely the workspaces the update is for. +// Nothing here is specific to plan-run — it is read out of assets.WorkflowSkills, so +// the next workflow skill is covered the day it is added. +func TestApplyUpdateAddsASkillTheWorkspacePredates(t *testing.T) { + for _, h := range paths.Harnesses() { + root := t.TempDir() + applyTo(t, root, h, false) + + // Roll the workspace back to before the skill existed: the files gone, and + // no manifest entry claiming scc ever wrote them. + var rels []string + for _, skill := range assets.WorkflowSkills { + rels = append(rels, path.Join(h.Dir, h.SkillsSeg, skill, "SKILL.md")) + if h.CommandsSeg != "" { + rels = append(rels, path.Join(h.Dir, h.CommandsSeg, "scc-"+skill+".md")) + } + } + m, _, err := manifest.Load(root, h) + if err != nil { + t.Fatalf("%s: Load: %v", h.ID, err) + } + for _, rel := range rels { + if err := os.RemoveAll(filepath.Dir(filepath.Join(root, filepath.FromSlash(rel)))); err != nil { + t.Fatalf("%s: %v", h.ID, err) + } + m.Remove(rel) + } + if err := manifest.Save(root, h, m); err != nil { + t.Fatalf("%s: Save: %v", h.ID, err) + } + + plan := planFor(t, root, h) + for _, rel := range rels { + if got := action(t, plan, rel); got != UpCreate { + t.Errorf("%s: %s planned as %q, want %q", h.ID, rel, got, UpCreate) + } + } + if _, err := ApplyUpdate(root, h, plan, UpdateOptions{SCCVersion: "v0.0.0-test"}); err != nil { + t.Fatalf("%s: ApplyUpdate: %v", h.ID, err) + } + + after, _, err := manifest.Load(root, h) + if err != nil { + t.Fatalf("%s: Load: %v", h.ID, err) + } + for _, rel := range rels { + f := findWorkspaceFile(t, h, rel) + want, err := assets.Render(h, f) + if err != nil { + t.Fatalf("%s: Render %s: %v", h.ID, rel, err) + } + if got := read(t, root, rel); got != want { + t.Errorf("%s: %s is not what this version renders", h.ID, rel) + } + e, ok := after.Get(rel) + if !ok { + t.Errorf("%s: %s was written and not recorded", h.ID, rel) + continue + } + if e.Version != assets.Version { + t.Errorf("%s: %s recorded at version %q, want %q", h.ID, rel, e.Version, assets.Version) + } + } + if plan := planFor(t, root, h); plan.Writes(true) { + t.Errorf("%s: a second update would write again", h.ID) + } + } +} + +func findWorkspaceFile(t *testing.T, h paths.Harness, rel string) assets.File { + t.Helper() + for _, f := range assets.Workspace(h) { + if f.Rel == rel { + return f + } + } + t.Fatalf("%s ships no file at %s", h.ID, rel) + return assets.File{} +} diff --git a/internal/validate/plan.go b/internal/validate/plan.go index 2044c6b..698a51b 100644 --- a/internal/validate/plan.go +++ b/internal/validate/plan.go @@ -67,6 +67,7 @@ func Plan(root, name string) (*finding.Set, error) { return set, nil } checkKickoffAs(set, file, doc.Frontmatter, "plan.kickoff-invalid") + checkLoopAnswers(set, file, doc.Frontmatter) // A plan's tasks carry no requirement citations: a plan has no requirements. The // rest of the grammar is identical, because the methodology is a property of the @@ -105,6 +106,31 @@ func Plan(root, name string) (*finding.Set, error) { return set, nil } +// The answers the `plan-run` skill asks for before it starts a loop, and writes back +// into the plan so a resumed session reads them instead of asking a second time. +// +// Plan-only, and validated on exactly the terms the kickoff answers are: only when +// present, because a plan nobody has ever run a loop over is not a plan with a defect. +// They are checked at all for one reason — the skill writes them and every later +// session reads them, so `worktree: yes` instead of `per-group` would silently decide +// how the next ten groups get built. +var loopValues = map[string]map[string]bool{ + "worktree": {"per-group": true, "in-place": true}, + "merge": {"auto": true, "manual": true}, +} + +func checkLoopAnswers(set *finding.Set, file string, fm mdscan.Frontmatter) { + for _, key := range []string{"worktree", "merge"} { + value, ok := fm.Get(key) + if !ok { + continue + } + if !loopValues[key][value] { + set.Addf(file, 1, "plan.loop-invalid", "`%s: %s` is not one of %s", key, value, allowed(loopValues[key])) + } + } +} + // checkKickoffAs is checkKickoff with the rule slug the caller's subject uses, so a // plan's finding reads `plan.` and a spec's reads `spec.` for the same defect. func checkKickoffAs(set *finding.Set, file string, fm mdscan.Frontmatter, rule string) { diff --git a/internal/validate/plan_test.go b/internal/validate/plan_test.go index de3313d..656079a 100644 --- a/internal/validate/plan_test.go +++ b/internal/validate/plan_test.go @@ -114,6 +114,35 @@ func TestPlanKickoffAnswers(t *testing.T) { } } +// The two answers `plan-run` writes back before it starts a loop. A wrong value is +// worth a finding because the skill writes these and every later session reads them — +// `worktree: yes` would quietly decide how the rest of the plan gets built. +func TestPlanLoopAnswers(t *testing.T) { + root := t.TempDir() + writePlan(t, root, "sweep", + "---\nworktree: yes\nmerge: whenever\n---\n\n# Sweep\n\n- [ ] 1.1 (Unit) Do it\n") + got := planFindings(t, root, "sweep") + if n := count(got, "plan.loop-invalid"); n != 2 { + t.Errorf("rules = %v, want two plan.loop-invalid findings, got %d", got, n) + } +} + +// Absent is not wrong. A plan nobody has run a loop over carries neither key, and +// scaffolded plans never carry them — a validator firing on scc's own output is the +// one defect that teaches users to disbelieve the other seven. +func TestPlanLoopAnswersAreOptional(t *testing.T) { + root := t.TempDir() + writePlan(t, root, "sweep", "---\nautonomy: auto\nci: wait\n---\n\n# Sweep\n\n- [ ] 1.1 (Unit) Do it\n") + if got := planFindings(t, root, "sweep"); contains(got, "plan.loop-invalid") { + t.Errorf("rules = %v, want no plan.loop-invalid", got) + } + writePlan(t, root, "run", + "---\nautonomy: auto\nci: wait\nworktree: per-group\nmerge: auto\n---\n\n# Run\n\n- [ ] 1.1 (Unit) Do it\n") + if got := planFindings(t, root, "run"); len(got) != 0 { + t.Errorf("a plan carrying every valid answer reported %v", got) + } +} + // A spec path inside a fenced block is an example, not a reference. func TestPlanExamplesAreNotReferences(t *testing.T) { root := t.TempDir() diff --git a/internal/validate/spec_test.go b/internal/validate/spec_test.go index d88ebad..3581e1d 100644 --- a/internal/validate/spec_test.go +++ b/internal/validate/spec_test.go @@ -342,10 +342,15 @@ func TestSpecOnAnUnknownFeatureIsAnError(t *testing.T) { } func contains(haystack []string, want string) bool { + return count(haystack, want) > 0 +} + +func count(haystack []string, want string) int { + n := 0 for _, s := range haystack { if s == want { - return true + n++ } } - return false + return n }