From fbe6a4467215456e6f771d0d789a2cb580afdea3 Mon Sep 17 00:00:00 2001 From: prode Date: Wed, 5 Aug 2026 01:15:41 -0300 Subject: [PATCH 1/2] feat(plan): make the plan a contract of closed sections and scheduled tasks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A plan stops being a document and becomes a header plus a checklist. Six sections and no others — Why, Paths, References, Out of scope, Tasks, Done when — because a closed set is the only thing that ever capped a plan's size: the 56KB plan we measured got there through `## Notes`, which nothing forbade, growing to half the file. There is no line limit anywhere in the contract except on the description, since a limit that fires on a legitimate plan is worse than the growth it prevents; what there is instead is nowhere for prose to go. `## Decomposition` becomes `## References`, and that cost almost nothing: parseLeaves recognizes a leaf by its `specs//` citation anywhere in the file rather than by the heading above it, so Leaf, the `specs/foo/` address and `map trace` all keep working untouched. A task gains four flags and no more — _Depends_, _Priority_, _Status removed_, _Reason_ — and the vocabulary is closed because an italic one-liner is a shape prose also uses: a parser that absorbed any of them would eat a sentence and hand the task a region that is not the task. Three consequences had to land together or the result is worse than before: Task.End covers the flags, Detail excludes them, and renderTask re-emits them along with the continuation. Without the last one, `patch task --method TDD` was a data-loss command that deleted a sixty-line description and every dependency the task declared. `map brief` is the header, `map tasks` is the checklist, and no command returns both — which is what gives "never read the plan" the authority to be a rule. A session pays brief once and --next per task instead of ~14k tokens per reread. --next is now determined (eligible, then priority ascending with absent last, then number compared numerically, which also fixes 1.10 sorting before 1.9) and --ready/--blocked/--deps share that one implementation. `plan approve` writes status and a checksum over the file minus its own checksum line. It is tamper-evidence, not prevention, and it is checked before an edit is applied — a harness that edited by hand and then ran `patch check` would otherwise have its edit resealed by the command that should have reported it. After approval only discovery moves: add allocates the number from a high-water mark that counts struck-out tasks, rm strikes in place, and rewriting a task or the prose is refused. `plan migrate` moves a v1 plan across without deleting anything. Budget, measured because this is where the gain could be lost: entry.md + rules/artifacts.md + rules/tasks.md went from 175 lines / 8539 bytes to 177 / 9248. The caps the tests already imposed still hold. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DmGyL6QvamMBKdHYyrx7Uq --- CLAUDE.md | 22 +- design/plan-format-v2.md | 719 ++++++++++++++++++ internal/artifact/artifact.go | 132 +++- internal/artifact/edit.go | 77 +- internal/artifact/flags.go | 276 +++++++ internal/artifact/flags_test.go | 343 +++++++++ internal/artifact/parse.go | 52 +- internal/artifact/schedule.go | 213 ++++++ internal/artifact/seal.go | 139 ++++ internal/artifact/seal_test.go | 74 ++ internal/assets/assets.go | 11 +- internal/assets/templates/artifacts/plan.md | 72 +- .../assets/templates/commands/scc-plan-run.md | 14 +- internal/assets/templates/entry.md | 6 +- internal/assets/templates/rules/artifacts.md | 60 +- internal/assets/templates/rules/routing.md | 2 +- internal/assets/templates/rules/tasks.md | 62 +- .../assets/templates/rules/verification.md | 2 +- .../assets/templates/skills/plan-run/SKILL.md | 84 +- internal/assets/templates/skills/prd/SKILL.md | 47 +- internal/cli/brief_test.go | 205 +++++ internal/cli/map.go | 504 ++++++++++-- internal/cli/migrate.go | 289 +++++++ internal/cli/migrate_test.go | 116 +++ internal/cli/patch.go | 224 +++++- internal/cli/plan.go | 182 ++++- internal/cli/seal.go | 75 ++ internal/cli/seal_test.go | 231 ++++++ internal/validate/flags_test.go | 99 +++ internal/validate/plan.go | 148 ++++ internal/validate/plan_test.go | 147 +++- internal/validate/tasks.go | 95 +++ 32 files changed, 4456 insertions(+), 266 deletions(-) create mode 100644 design/plan-format-v2.md create mode 100644 internal/artifact/flags.go create mode 100644 internal/artifact/flags_test.go create mode 100644 internal/artifact/schedule.go create mode 100644 internal/artifact/seal.go create mode 100644 internal/artifact/seal_test.go create mode 100644 internal/cli/brief_test.go create mode 100644 internal/cli/migrate.go create mode 100644 internal/cli/migrate_test.go create mode 100644 internal/cli/seal.go create mode 100644 internal/cli/seal_test.go create mode 100644 internal/validate/flags_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 3f36b0a..15fd27b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -40,7 +40,21 @@ These landed after phase 10, and all are documented in `design/orchestration.md` What scc adds over typing `codegraph` directly is the two things it already knows: the workspace root, so `scc graph build` from `specs/` indexes the repo rather than a subtree, and whether the binary is there at all. The graph itself is *not* an scc artifact — not in the manifest, never touched by `scc update`, and `.codegraph/` stays CodeGraph's directory on CodeGraph's schedule. Unlike the launch path, a missing binary in `scc graph` is a hard error: the whole command is the binary. npm is the only installer scc will run. CodeGraph's headline install pipes a remote script into a shell (`curl … | sh`, `irm … | iex`), which is a fine thing for a person to type and not a thing scc executes on their behalf — `InstallHint` names it and leaves the decision where it belongs. -- **`scc map` and `scc patch`.** The artifacts are structured documents that happen to be Markdown, and the cost of treating them as prose is paid on every request rather than once: measured on a real workspace, one plan is 56KB and its 31 specs bring the corpus to ~90k tokens, so an agent answering "what is the next open task?" by reading the file carries the whole plan for the rest of the session. `map` turns a file into addressable pieces — `index | outline | tasks | show | blocks | find | trace` — and `patch` changes one of them — `check | uncheck | task | add | rm | append | prepend | replace | fm`. +- **The plan as a contract (`design/plan-format-v2.md`).** A plan stopped being a document and became a header plus a checklist. Six sections and no others — `Why`, `Paths`, `References`, `Out of scope`, `Tasks`, `Done when` — because a closed set is the *only* thing that ever capped a plan's size: the 56KB plan measured below got there through `## Notes`, which nothing forbade, growing to half the file. There is no line limit anywhere in the contract except on the description, since a limit that fires on a legitimate plan is worse than the growth it prevents; what there is instead is nowhere for prose to go. + + **`## Decomposition` became `## References`, and that cost almost nothing** — `parseLeaves` recognizes a leaf by the `specs//` citation *anywhere in the file*, not by the heading above it, so `Leaf`, the `specs/foo/` address and `map trace` all kept working untouched. What closes the door on leaves appearing elsewhere is `plan.unknown-section` itself. + + **A task gains four flags and no more** — `_Depends_`, `_Priority_`, `_Status removed_`, `_Reason_` — and the vocabulary is closed because an italic one-liner is a shape prose also uses: a parser that absorbed any of them would silently eat a sentence and hand the task a region that is not the task. An unknown one is `task.unknown-flag`, reported and left where it sits — since the box is the state, a flag that could restate it is the `item-has-two-records` defect arriving by another door, and there is no `_Blocked_` because that is derived from `_Depends_`. `(Unit)`/`(TDD)` is reused as the test strategy rather than a new `_Test_` flag — zero migration, and the concept already had a name. `_Status_` never takes `open` or `completed`. + + Three consequences had to land together or the result is worse than before: `Task.End` covers the flags (so `map show` returns them and `patch rm` removes them), `Detail` excludes them (so the searcher does not index `_Priority 2_` as prose), and **`renderTask` re-emits them plus the continuation** — without that, `patch task --method TDD` was a data-loss command that deleted a sixty-line description and every dependency the task declared. + + **The reading surface is what gives "never read the plan" its authority.** `map brief` is the header, `map tasks` is the checklist, and no command returns both — so a session pays `brief` once and `--next` per task instead of ~14k tokens per reread. Forbidding the read without offering the equivalent query produces an agent that disobeys the rule, correctly — so the surface shipped in the phase before the rule did. `--next` is now determined (eligible → priority ascending, absent last → number compared *numerically*, which is also the fix for `1.10` sorting before `1.9`), and `--ready`/`--blocked`/`--deps` share that one implementation, because two notions of eligibility would be two answers to "what do I work on". +- **`scc plan approve|reseal|migrate`, and the seal.** `approve` validates, then writes `status: approved` and a `checksum:` over the file minus its own checksum line, LF-normalized. It is **tamper-evidence, not prevention** — `reseal --force` is one command away and sha256 is public — and it is recorded that way here so nobody builds a guarantee on it later. The check runs before an edit is applied, which is the whole value: a harness that edited by hand and then ran `patch check` would otherwise have its edit resealed by the command that should have reported it. A plan with no `status:` is never checked, which is what makes every pre-existing plan keep working. + + After approval the work is fixed and only discovery moves: `add` allocates the number (high-water mark including removed tasks, so nothing is stored anywhere) and demands `--reason`; `rm` strikes the task out where it stands rather than deleting it; rewriting a task or the prose is refused. What discovery can never touch is guaranteed structurally rather than by instruction — `Why`, `Out of scope`, `Done when` and the title are reachable only through `append`/`prepend`/`replace`, and those are exactly the three refused. + + `migrate` moves a v1 plan across: it renames `Decomposition`, moves every other heading to `plans/archive/-notes.md` (safe because the plan scanner uses `ReadDir` and skips directories), creates the missing required sections **empty and lets the findings appear** — a placeholder that satisfied the validator would be a plan that lies — and writes `status: draft`, never `approved`. +- **`scc map` and `scc patch`.** The artifacts are structured documents that happen to be Markdown, and the cost of treating them as prose is paid on every request rather than once: measured on a real workspace, one plan is 56KB and its 31 specs bring the corpus to ~90k tokens, so an agent answering "what is the next open task?" by reading the file carries the whole plan for the rest of the session. `map` turns a file into addressable pieces — `index | outline | brief | tasks | show | blocks | find | trace` — and `patch` changes one of them — `check | uncheck | task | add | rm | append | prepend | replace | fm`. **The addresses are the design.** A task is `1.2`, a requirement `R1.2`, a section `#notes`, a leaf `specs//`, a paragraph `notes:7`; `L120-160` is the escape hatch and the only form that is a line number. That is what lets `patch` write into a file nobody read: a line number stops being true the moment anything above it moves, so an editor addressing by line has to read first — which is the cost the package exists to remove. The guard that reading-first was providing is replaced by three that are stronger for a structured file: an address that does not resolve is an error and never an insert at a guess, the file is re-validated afterwards and **rolled back if the edit introduced a finding** (exit 2), and the displaced and written lines are printed back — elided past a few lines, because a confirmation that echoed 400 lines would put the file in context by the back door. @@ -49,6 +63,8 @@ These landed after phase 10, and all are documented in `design/orchestration.md` **`internal/artifact` owns the grammars, and `internal/validate` consumes them.** The task grammar used to live in the validator; a reader that disagreed with the validator about what a task is would be worse than no reader. The parser now states facts about a line (`Methodologies`, `Loose`, `HasCitation`) and turning a fact into a finding stays in `validate` — which is also what lets `map` read a malformed artifact instead of refusing exactly the file a user most needs to inspect. **No search engine.** The obvious reach for `find` is an inverted index; at 352KB and 94 artifacts a linear pass ranks the whole workspace in 55ms, and Tantivy or its kin would cost a CGO surface or a second binary against a stdlib-only `go.mod` and a six-platform cross-compile. What precision needed here was not a better index but a better *unit*: BM25 over addressable regions rather than lines, so a hit comes back as something `show` accepts. The seam is `artifact.Search` — it takes artifacts and returns hits, and nothing outside that file knows how it found them. + + **`map find` is now undocumented rather than removed.** Its stated reason was the whole corpus (94 artifacts, 352KB), not the plan — and with the plan small, searching *inside* one stopped making sense, while searching `design.md` and the knowledge base is still the only alternative to reading a file. So `runMapFind` and `search.go` stay and the line comes out of `rules/artifacts.md`, `entry.md` and `mapUsage()`: deleting the code saves nothing, and deleting the line saves tokens in every request of every session. - **`caveman.md`, the register the agent answers in.** The output budget belongs to the code: prose about the work is written once and then carried in every later request of the session, so narration is the part of a long run that can be cut without losing a fact. It ships as a *rule* rather than a skill because it is on by default, and a default the model has to decide to load is not one — the cost is what every rule costs, since the harnesses that read `rules/` preload it. One level (ultra) rather than a dial, because three descriptions of the register are three things to keep true instead of one, and nobody turns the dial. What it must never compress is the line that keeps it honest: artifacts under `specs/`, `plans/`, `docs/`, anything a validator parses or a shell runs, quoted output, commit and PR bodies, and questions asked of the user. A denser EARS line is a finding, not a saving. @@ -115,7 +131,7 @@ Three packages sit off to the side of that tree — `rtk`, `headroom`, `codegrap | `internal/assets` | The embedded template set — rules, review agents, skills, slash commands, artifact templates. **Workspace templates are data-free except for the harness profile** (a `(version, harness)` pair still renders byte-identically everywhere, and the manifest records both, so the future three-way merge can still reconstruct the old side); **artifact templates take data** (`spec new` renders them and the user owns the result); **seeds are the `docs/` anchors** — data-free like a workspace file, untracked like an artifact. `Render(h, file)` is the only way to get a workspace file's bytes: it expands paths and synthesizes the per-harness header for agents and commands. `Version` is the template-set version and must be bumped whenever a workspace template changes. | | `internal/scaffold` | Applies the template set to a root (`Apply`) and brings an existing one current (`PlanUpdate`/`ApplyUpdate`). Idempotent, never overwrites without being told to, manifest written last. | | `internal/mdscan` | The only Markdown parser: fence- and HTML-comment-aware headings, checkboxes, links, wikilinks, slugs, plus a small frontmatter reader. `Body` is the comment/fence-stripped text every validator applies its grammar to. | -| `internal/artifact` | The navigable model of one artifact, layered on `mdscan`: sections (two ends — the subtree, and the body before the first child), tasks with their continuation, requirements, decomposition leaves, paragraph blocks. Owns **every grammar** (task, requirement, spec reference), `Find` for address resolution, `Editor` for line splices resolved against the original and applied bottom-up, and `Search`. Knows nothing about findings or exit codes. | +| `internal/artifact` | The navigable model of one artifact, layered on `mdscan`: sections (two ends — the subtree, and the body before the first child), tasks with their continuation *and their flags*, requirements, spec-reference leaves, paragraph blocks. Owns **every grammar** (task, requirement, spec reference, flag), `Find` for address resolution, `Editor` for line splices resolved against the original and applied bottom-up, `Search`, the schedule (`Ready`/`BlockedTasks`/`Next`/`Cycles`, one implementation shared by `--next`, `--ready` and `--blocked`), and the seal. Knows nothing about findings or exit codes. | | `internal/ears` | EARS requirement parsing, all five patterns plus complex. | | `internal/validate` | The eight validators, one file each, sharing `mdscan` and `finding`. The exception is `stack_manifests.go`: the seven dependency-file readers age on their own schedule, so they sit beside the rule rather than inside it. | | `internal/rtk` | RTK's marker pair and the idempotent splice of its block into the entry file, plus finding or `cargo install`ing the binary. | @@ -141,6 +157,8 @@ Three packages sit off to the side of that tree — `rtk`, `headroom`, `codegrap **Writes are atomic.** Use `workspace.AtomicWrite` for anything a concurrent reader might see. +**The rules are a standing budget, not a place to explain things.** Every file under `/rules/` is preloaded into every request on a harness that reads them, so a line added there is paid continuously rather than once. `TestRulesStayShortEnoughToBePreloaded` caps a rule at 55 lines (three predate the budget and are capped where they stand; they may shrink, never grow), and `TestScaffoldedEntryFileStaysShort` caps the entry file at 60. When a feature does not fit, the answer is to move the detail into a `--help` string — read only when consulted — and keep the question→command table in the rule. The plan-format change was measured in and out on this basis: it landed at +2 lines and +709 bytes across `entry.md` + `artifacts.md` + `tasks.md`, against ~14k tokens saved per plan reread. + **A command that edits an artifact verifies it afterwards.** `scc patch` snapshots the file, writes, re-runs the validator that owns it, and restores the snapshot if the edit introduced a finding the file did not already have. Two details are load-bearing: the comparison is on `rule + message` and deliberately **not on line number**, because an insertion moves every finding below it and comparing on line would blame this edit for the whole tail of a pre-existing problem; and an artifact scc has no validator for is written and *reported as unverified* rather than silently claimed clean. **The marker is the file `/scc-manifest.json`, never the harness directory.** Two reasons, and both are load-bearing: every harness has a global twin in the user's home (`~/.claude`, `~/.codex`, `~/.config/opencode`) that exists on any machine running that tool, so an upward walk accepting the *directory* would resolve the root to `$HOME` for any command run outside a workspace — every command would then read and write the user's global configuration. And those directories exist in every repo that merely *uses* the tool, where scc was never initialized. `workspace.Find` therefore stats a regular file, for each harness in turn. diff --git a/design/plan-format-v2.md b/design/plan-format-v2.md new file mode 100644 index 0000000..e9cf466 --- /dev/null +++ b/design/plan-format-v2.md @@ -0,0 +1,719 @@ +# Plan v2 — o plano como contrato de execução + +**Status: implementado.** As oito fases da §13 estão no binário e verdes em `make check`. +O que ficou diferente do proposto, e por quê: + +- **`## Paths` não valida existência de caminho.** O aviso da §3.2 exigiria uma + severidade que `internal/finding` não tem — a forma JSON é congelada — e um plano + nomeia por construção arquivos que ainda não existem, então a regra dispararia em + entrada correta. Sob 0/1/2 isso vira erro, não aviso. A seção é validada como + seção; o conteúdo não. +- **O slug do achado de ciclo é `task.dependency-cycle`, não `plan.dependency-cycle`.** + A §8.3 e a §14 se contradiziam; vence a §14, porque a regra vive em + `validate/tasks.go` junto das outras nove de flag e vale igualmente para o + `tasks.md` de uma spec. +- **`plan reseal` não imprime diff contra o selo antigo** (§8.1): o selo é um hash, e o + conteúdo antigo não é recuperável a partir dele. Imprime os dois hashes e aponta + `git diff`, que é quem tem o diff. +- **Duas regras a mais que a §14 previa**: `plan.status-invalid` e `plan.unsealed` — + um `status: approved` sem `checksum:` é um plano que diz estar selado por um selo + que não existe, e toda leitura dele pularia em silêncio a verificação. +- **`_Reason_` também marca uma task *acrescentada* depois da aprovação**, não só uma + removida. Responde à mesma pergunta: por que esta linha não é o que o plano + aprovado dizia. +- **Orçamento da §9, medido**: `entry.md` + `rules/artifacts.md` + `rules/tasks.md` + passaram de 175 linhas / 8539 bytes para 177 / 9248 — +2 linhas, +709 bytes. Os + tetos que os testes já impunham (55 linhas por regra, 60 no entry) continuam + respeitados; foi preciso comprimir prosa existente para caber. + +O documento abaixo é o plano como foi aprovado. + +--- + +## 1. Ponto de partida — o que já existe hoje + +Antes da análise, os fatos verificados no código (não suposições): + +| Fato | Onde | +|---|---| +| O template de plano **já não tem `## Notes`** — e diz explicitamente que a ausência é deliberada | `internal/assets/templates/artifacts/plan.md:42-48` | +| A estrutura atual é `frontmatter(autonomy, ci)` + `# Título` + `## Why` + `## Decomposition` + `## Tasks` | mesmo arquivo | +| A gramática de task é `- [ ] 1.1 (Unit) descrição — R1.2` e vive em `internal/artifact/parse.go` | `taskNumberRe`, `MethodologyRe`, `RequirementIDRe` | +| O validador de plano tem 5 regras: `kickoff-invalid`, `loop-invalid`, `item-has-two-records`, `unknown-spec`, `empty` — mais as 5 de task | `internal/validate/plan.go`, `tasks.go` | +| `map` tem 7 verbos, `patch` tem 9 | `internal/cli/map.go`, `patch.go` | +| `Block`/`map blocks`/`Search`/`map find` existem por causa de um `## Notes` de 411 linhas medido num plano real | comentários em `parse.go:93-102`, `search.go:10-27` | +| `mdscan` lê `Checked: m[2] != " "` — **qualquer** caractere que não seja espaço é "feito" | `internal/mdscan/mdscan.go:141` | +| `blockEnd` só absorve continuação **mais indentada** que o marcador da task | `parse.go:284-297` | +| `manifest.Hash` = sha256 sobre conteúdo com quebras normalizadas | `internal/manifest/manifest.go:105` | +| `assets.Version = "13"`; qualquer mudança de template obriga bump | `internal/assets/assets.go:94` | + +Três consequências imediatas dessas linhas, que decidem boa parte do desenho abaixo: + +1. **"Remover Notes" é quase todo trabalho de migração e de *proibição*, não de template.** + O template já não escreve Notes. O que falta é o validador recusar seções fora do + contrato — hoje um plano pode ter qualquer heading e passa limpo. +2. **Flags na coluna 0 não pertencem à task pelo parser atual.** No exemplo do escopo + (`- [ ] 2.3 …` / linha em branco / `_Depends 2.1_`), `blockEnd` para na primeira + linha com indentação ≤ 0. Sem mudança no parser, `map show 2.3` não devolve as flags, + `patch rm 2.3` deixa as flags órfãs e `patch task 2.3 --text` apaga a continuação + (`SetTask` zera `Detail`) mas não as flags — que ficam grudadas na task errada. +3. **`- [-]` não serve para "removed".** Seria lido como concluída. O estado `Removed` + tem de ser uma flag, não um caractere na caixa. + +--- + +## 2. Análise da alteração + +### 2.1 O que muda de fato + +O plano deixa de ser um documento híbrido (prosa + decomposição + checklist) e passa a +ser **uma tabela de execução com um cabeçalho curto**. A mudança real não é tirar uma +seção: é passar o validador de *permissivo* (aceita qualquer seção) para *fechado* +(aceita só as oito), e mover todo metadado de task para um vocabulário único de flags. + +### 2.2 Onde o ganho de token realmente aparece + +Vale medir antes de prometer. O ganho por chamada vem de três lugares, em ordem de +tamanho: + +1. **O plano em si** — de ~56KB medidos para o teto que o contrato impõe. É o maior + ganho e é permanente, porque a seção fechada impede o crescimento. +2. **A regra `artifacts.md`** — pré-carregada em toda requisição no Claude Code + (`PreloadsRules`). Cuidado: o contrato novo tem **mais** conceitos a explicar + (flags, lifecycle, discovery, selo). Se a regra crescer 40 linhas, parte do ganho + volta para o prejuízo, em todas as requisições da sessão. §9 trata isso como + orçamento, não como consequência. +3. **A superfície de leitura procedural** (§8.4) — troca "abrir o plano para entender o + trabalho" por `brief` uma vez e `--next` por task. É o ganho de *releitura*: hoje o + custo de 14k tokens é pago toda vez que a sessão perde contexto ou troca de grupo. + Com o cabeçalho e as tasks separados por comando, nenhuma pergunta sobre o plano tem + como resposta "abra o arquivo". + +### 2.3 O que a mudança **não** resolve + +- Não reduz o tamanho das specs — e as specs são ~90k tokens do corpus medido. O escopo + diz que a spec fica inalterada; então o corpus continua dominado por specs. +- O selo (checksum) **não impede** edição direta. Ele torna a edição direta *visível*. + Um harness que queira burlar roda `scc plan approve --force` ou calcula sha256. O + valor é evidência e disciplina, não segurança. Está registrado aqui para que ninguém + construa uma garantia em cima disso depois. + +--- + +## 3. Novo contrato do Plan + +### 3.1 Layout + +```md +--- +autonomy: auto +ci: wait +status: approved +checksum: 9f2c… # sha256, escrito só pelo scc +--- + +# + + + +## Why + + +## Paths +- `internal/artifact/parse.go` +- `internal/cli/map.go` + +## References +- specs/plan-format/ +- docs/adr/0007-plan-seal.md + +## Out of scope +- Reescrever o formato de spec + +## Tasks +- [ ] 1.1 (Unit) Descrição imperativa + _Depends 1.0_ + _Priority 2_ + +## Done when +- `scc validate` limpo e `make check` verde +``` + +### 3.2 Regras do contrato + +| Seção | Obrigatória | Conteúdo | Regra de validação | +|---|---|---|---| +| `# Título` (H1) | sim | uma linha | `plan.missing-title` | +| descrição | sim | prosa antes do primeiro H2, ≤ N linhas | `plan.missing-description`, `plan.description-too-long` | +| `## Why` | sim | um parágrafo | `plan.missing-section` | +| `## Paths` | não | lista de caminhos | itens são lista; caminho inexistente → *aviso*, não erro | +| `## References` | não | lista de links/specs/ADRs | `plan.unknown-spec` reaproveitado | +| `## Out of scope` | não | lista | — | +| `## Tasks` | sim | só tasks e suas flags | `plan.missing-section`, `plan.empty` | +| `## Done when` | sim | lista de critérios verificáveis | `plan.missing-section` | +| qualquer outro H2 | — | **proibido** | `plan.unknown-section` (mensagem especial para `Notes`) | + +Decisões embutidas, e o motivo: + +- **Ordem das seções não é validada.** Presença sim, ordem não. Validar ordem custa uma + regra e uma migração a mais e não muda nenhuma leitura — tudo é endereçado por slug. +- **Sem limite de linhas por seção, exceto na descrição.** O limite real é a seção + fechada: sem `Notes` e sem heading livre, não há onde a prosa crescer. Um limite + numérico em `Why` seria uma regra que dispara em plano legítimo — e o pior bug do + produto é validador que dispara na saída correta. +- **`## Decomposition` sai do contrato.** As referências de spec passam a viver em + `## References` (decisão **D1**, §11). O parser as reconhece por citarem + `specs//`, não pelo heading, então `Leaf`, o endereço `specs/foo/` e + `map trace` seguem funcionando sem alteração. + +--- + +## 4. Contrato da Task + +### 4.1 Formato + +``` +- [ ] () + _Depends [, …]_ + _Priority _ + _Status removed_ + _Reason _ +``` + +Mapeamento para o que o escopo pediu: + +| Pedido | Como | Novo? | +|---|---|---| +| identificador único | `` = `.` | não — já existe, já validado | +| descrição objetiva | texto após a anotação | não | +| **estratégia de teste** | `(Unit)` \| `(TDD)` | **não** — é exatamente a anotação de metodologia existente | +| zero ou mais dependências | `_Depends_` | sim | +| zero ou uma prioridade | `_Priority_` | sim | +| zero ou mais flags | vocabulário fechado abaixo | sim | + +**Reaproveitar `(Unit)`/`(TDD)` como estratégia de teste é a decisão de menor custo do +plano inteiro**: zero migração, zero mudança de parser, `patch task --method` e o +validador `task.missing-methodology` continuam valendo, e a regra `methodology.md` já +explica quando é cada uma. Inventar um `_Test_` novo duplicaria o conceito. + +### 4.2 Vocabulário de flags — fechado + +| Flag | Valor | Cardinalidade | Semântica | +|---|---|---|---| +| `_Depends_` | lista de ids separados por vírgula | 0..1 linha | a task só é elegível quando **todas** estiverem `[x]` | +| `_Priority_` | inteiro ≥ 1 | 0..1 | menor = mais urgente; ausente = menos urgente que qualquer explícita | +| `_Status_` | **apenas** `removed` | 0..1 | remoção lógica por Discovery | +| `_Reason_` | texto livre de uma linha | 0..1 | obrigatório com `_Status removed_` | + +Três decisões aqui, e as três são para evitar bug conhecido: + +- **`_Status` não aceita `open`/`completed`.** A caixa é o estado. Aceitar os dois + criaria dois registros de um fato — exatamente o defeito que + `plan.item-has-two-records` existe para pegar. Regra: `task.status-duplicates-box`. +- **Vocabulário fechado.** Um `_Qualquer coisa_` em itálico logo abaixo de uma task + **não** é absorvido como flag: vira `task.unknown-flag`. Sem isso, prosa em itálico + seria engolida silenciosamente pela task anterior — o modo de falha do §1.2. +- **Nada de `_Blocked_`.** É derivável de `_Depends_`. Duas fontes para um fato + divergem; a derivada ganha. + +Avaliadas e **rejeitadas** (registrado para não voltarem sem argumento novo): +`_Owner_` (o dono é o git), `_Estimate_` (não é verificável, e o produto só valida o +que é verificável), `_Tags_` (é busca, e busca é `map`), `_Parallel_` — já existe +decisão registrada em `rules/tasks.md:20` de que não há marcador de paralelismo. +`_Spec_` foi rejeitada com **D1**: uma task que aponta para uma spec teria a caixa e o +`tasks.md` daquela spec como dois registros de um mesmo estado. A referência fica em +`## References` e a task que a consome é marcada quando a spec fecha. + +### 4.3 Parsing — o ponto técnico que decide o resto + +Uma flag é uma linha que casa `^\s*_(Depends|Priority|Status|Reason)\b(.*)_\s*$`, +imediatamente após o bloco da task (continuação incluída), tolerando **no máximo uma** +linha em branco antes do bloco de flags e nenhuma dentro dele. O bloco de flags para na +primeira linha que não é flag. + +Consequências que **precisam** ser implementadas juntas, ou o resultado é pior que hoje: + +1. `Task.End` passa a cobrir as flags → `map show 1.1` devolve task + flags, + `patch rm 1.1` remove as duas coisas, o rollback funciona. +2. `Task.Detail` **exclui** as linhas de flag → a busca não indexa `_Priority 2_` como + texto, e `--width` continua honesto. +3. `renderTask` reemite as flags em ordem canônica (`Depends`, `Priority`, `Status`, + `Reason`) → `patch task --text` deixa de destruir metadado. **Hoje isso seria um bug + de perda de dados**: `SetTask` zera `Detail` e re-renderiza só o cabeçalho. +4. Indentação canônica das flags = 2 espaços. O exemplo do escopo usa coluna 0; aceitar + os dois na leitura e normalizar para 2 na escrita é o meio-termo: coluna 0 sobrevive + se alguém escrever à mão, mas o que o scc escreve é sempre indentado — e indentado + já pertence ao bloco pelas regras que o resto do produto usa. + +Tudo isso vive em `internal/artifact`, que já é o dono declarado das gramáticas +(`CLAUDE.md`: "`internal/artifact` owns the grammars, and `internal/validate` consumes +them"). O parser afirma fatos; transformar fato em finding continua em `validate` — +que é o que mantém `map` legível sobre arquivo malformado. + +### 4.4 Serialização + +- Escrita canônica: cabeçalho quebrado em `LineWidth = 88` (já existe), flags uma por + linha, ordem fixa, 2 espaços de indentação, sem linha em branco entre task e flags. +- **Idempotência é requisito de teste**: `parse(render(parse(x))) == parse(x)` para todo + plano do corpus. É o que garante que `patch` duas vezes não produz diff na segunda. +- JSON: `Task` ganha `depends []string`, `priority *int`, `status string`, + `reason string`, `blocked bool` (derivado), `eligible bool` (derivado). Campos + derivados entram no JSON porque o consumidor é um agente que não deve recalcular + regra de elegibilidade — se recalcular, temos duas implementações do `--next`. + +--- + +## 5. Lifecycle + +Três estados, como pedido. **Nenhum outro** — avaliei e rejeito os candidatos: + +| Estado | Representação | Quem escreve | +|---|---|---| +| `Open` | `- [ ]` sem `_Status_` | autor (draft) ou Discovery | +| `Completed` | `- [x]` | `scc patch check` | +| `Removed` | `- [ ]` + `_Status removed_` + `_Reason_` | `scc patch rm` em plano aprovado | + +Rejeitados e por quê: +- **`InProgress`** — o progresso dentro de uma task é o todo-list do harness, e + `rules/tasks.md:39-49` já decidiu que o arquivo guarda o durável e o harness guarda o + agora. Um terceiro estado teria de ser escrito e limpo por sessão que pode morrer no + meio, deixando lixo que ninguém reconcilia. +- **`Blocked`** — derivado de `_Depends_`. +- **`Skipped`** — é `Removed` com outra `_Reason_`. + +Invariantes: +- `Removed` + `[x]` → `task.removed-but-checked`. +- Task `Removed` nunca conta em `Done()`, nunca aparece em `--next`, **conta** para a + alocação de número (§7). +- Dependência apontando para task `Removed` → `task.depends-on-removed` (achado, não + aviso: é um deadlock silencioso em `--next`). + +--- + +## 6. Discovery + +**Discovery é um modo de escrita, não um comando novo.** Reutiliza `patch add` / `patch +rm`, que já resolvem endereço, revalidam e fazem rollback. Um par de verbos novos +(`scc plan discover add`) seria uma segunda superfície fazendo a mesma coisa. + +Em plano com `status: approved`: + +| Operação | Comportamento | +|---|---| +| `patch add` | exige `--reason`; **recusa `--number`** (o scc aloca, §7); exige `--group N` ou `--new-group` | +| `patch rm` | **remoção lógica**: reescreve a task com `_Status removed_` + `_Reason`; nunca apaga linhas | +| `patch check`/`uncheck` | permitido | +| `patch fm` | permitido (respostas do loop: `pr`, `worktree`, `merge`) | +| `patch task --text/--method/--number` | **recusado** — muda conteúdo funcional | +| `patch append`/`prepend`/`replace` | **recusado em plano aprovado** | + +Em plano `status: draft`: tudo permitido, `rm` apaga de verdade, `add` aceita +`--number`. É a fase de autoria. + +O que Discovery **nunca** pode tocar está garantido estruturalmente, não por instrução: +`Why`, `Out of scope`, `Done when` e o título só são alcançáveis por +`append`/`prepend`/`replace`, e esses três estão recusados em plano aprovado. Não +existe caminho por `add`/`rm`/`check`, que só endereçam tasks. + +Histórico preservado em duas camadas: a task removida **fica no arquivo** com sua razão, +e o git guarda o resto. Não há terceiro log — um log que ninguém mantém contradiz os +outros dois. + +--- + +## 7. Numeração + +- Formato `.`, ambos inteiros ≥ 1. +- Grupos sequenciais a partir de 1; itens sequenciais dentro do grupo. +- **Imutável**: `patch task --number` recusado em plano aprovado. +- **Nunca reutilizado**: a próxima vaga é `max(item) + 1` no grupo, contando tasks + `Removed`. Como a removida permanece no arquivo, o high-water mark é derivável — **sem + estado extra em lugar nenhum**, o que preserva a convenção "um arquivo por harness e + nenhum arquivo de configuração". +- `--new-group` aloca `max(grupo) + 1`. +- Regra nova `plan.number-gap`? **Não.** Buraco de numeração é consequência normal de + remoção; validar geraria achado em plano correto. + +--- + +## 8. SCC — impacto comando a comando + +### 8.1 Novos + +| Comando | O que faz | Saída | +|---|---|---| +| `scc plan approve ` | valida; se limpo, escreve `status: approved` + `checksum`; se sujo, recusa | 0 / 2 | +| `scc plan reseal ` | recalcula o selo após edição legítima fora do ciclo (ex.: resolução de conflito de merge). Exige `--force` e **imprime o diff contra o selo antigo** | 0 / 1 | +| `scc plan migrate ` | converte plano v1 → v2 (§10) | 0 / 1 | +| `scc map brief ` | o cabeçalho do plano sem as tasks — o "o que é este trabalho" (§8.4) | 0 | +| `scc map tasks … --ready` | todas as tasks elegíveis, na ordem canônica | 0 | +| `scc map tasks … --blocked` | abertas não elegíveis, cada uma nomeando o que espera | 0 | +| `scc map tasks … --deps` | só as arestas de dependência, uma linha por task | 0 | +| `scc map tasks … --next` | passa a ter algoritmo determinístico (§8.3) | 0 | + +Sem comando `plan status`: `map ` já responde. + +### 8.2 Removidos / depreciados + +| Comando | Proposta | Justificativa | +|---|---|---| +| `scc map find` | **manter o código; tirar da documentação de plano** (**D3**, fechada) | O motivo declarado do `find` é o corpus inteiro (94 artefatos, 352KB), não o plano. Com o plano pequeno, buscar *no plano* deixa de fazer sentido — mas buscar em `design.md` e no knowledge base continua sendo a única alternativa a ler arquivo. Remover o binário não economiza token; remover a linha da regra economiza, em toda requisição. | +| `scc map blocks` | manter, escopo reduzido a specs/docs | Existia pelo `## Notes` de 411 linhas. Sem Notes, um plano não tem parágrafo endereçável — mas `design.md` tem. | +| `patch append/prepend/replace` | manter para specs/docs; **recusar em plano aprovado** | São a única forma de escrever prosa em `design.md`. Remover quebraria a autoria de spec, que o escopo diz não mudar. | +| `patch task --number` | recusar em plano aprovado | numeração imutável | +| `map tasks --method` | manter | filtro por estratégia de teste continua útil | + +**Nenhum comando é removido do binário nesta proposta.** Toda a redução de superfície +acontece na documentação e nas guardas por estado. Isso é deliberado: o custo de um +subcomando não documentado é ~0 token; o custo de quebrar `scc patch replace` para quem +escreve spec é alto e imediato. + +### 8.3 `--next` — algoritmo determinístico + +``` +entrada: um artefato (plano), suas tasks +1. candidatos = tasks com !Checked ∧ status ≠ removed +2. elegíveis = candidatos cujas deps existem e estão todas Checked +3. ordenar elegíveis por: + a) priority ascendente (ausente = +∞, ou seja, por último) + b) id natural (grupo asc, depois item asc — comparação numérica, não textual) + c) ordem de arquivo (desempate total; na prática igual a (b)) +4. devolver o primeiro +``` + +Casos de borda, todos com resposta definida: + +| Situação | Resposta | Exit | +|---|---|---| +| há elegível | a task, `--json` com `depends`, `priority`, endereço | 0 | +| há abertas, nenhuma elegível | `{"task":null,"blocked":[{id, waiting_on:[…]}]}` + texto nomeando os bloqueadores | 0 | +| nenhuma aberta | `{"task":null,"done":true}` | 0 | +| ciclo de dependência | achado `plan.dependency-cycle`, nomeando o ciclo | 2 | +| dependência inexistente/removida | achado; a task é tratada como não-elegível | 2 | +| plano com drift de selo | achado `plan.drift`, **nada é devolvido** | 2 | + +`--next` com vários artefatos mantém o comportamento atual (primeiro artefato com +elegível, na ordem do scan). Priorizar entre planos diferentes seria inventar uma +ordenação global que ninguém pediu. + +Comparação numérica de id é uma correção necessária: hoje a ordem é a do arquivo, e +`1.10` < `1.9` em ordenação textual. + +### 8.4 A superfície de leitura procedural + +O contrato acima só vale se **existir uma consulta para cada pergunta que hoje leva a +abrir o arquivo**. Proibir a leitura direta sem oferecer a consulta equivalente produz +um agente que desobedece a regra — corretamente, porque a alternativa era não trabalhar. + +O modelo é um só, e é o que torna a garantia verificável: + +> **O plano é cabeçalho + tasks. `brief` lê o cabeçalho, `tasks` lê as tasks, e nenhum +> comando devolve os dois.** Não existe chamada de scc que retorne o plano inteiro. + +| Pergunta do agente | Comando | O que volta | Frequência | Custo estimado | +|---|---|---|---|---| +| O que é este trabalho, e quando está pronto? | `scc map brief ` | título, descrição, `Why`, `Paths`, `References`, `Out of scope`, `Done when` — **sem tasks** | **1× por sessão** | ~500–800 tokens | +| O que eu faço agora? | `scc map tasks --next` | uma task: id, estratégia, descrição, deps, prioridade, endereço | 1× por task | ~100 tokens | +| Qual é a frente de trabalho? | `scc map tasks --ready` | todas as elegíveis, na ordem de §8.3 | eventual | ~30 tokens/task | +| Por que não há nada elegível? | `scc map tasks --blocked` | abertas não elegíveis, cada uma nomeando o que espera | em impasse | ~30 tokens/task | +| Qual é a ordem geral? | `scc map tasks --deps` | só as arestas: `1.3 ← 1.1, 1.2` | eventual | ~1 linha/task | +| Me mostra exatamente essa task | `scc map show 1.2` | a task com suas flags e continuação | sob demanda | ~80 tokens | +| Quanto falta? | `scc map ` | contagens por seção e por grupo: feitas / abertas / bloqueadas / removidas | por grupo | ~20 linhas | + +Custo total de uma sessão que executa N tasks: **`brief` uma vez + N × `--next`**. Para +um plano de 30 tasks isso é ~800 + 3.000 tokens, contra 14k **por releitura**. + +Decisões embutidas: + +- **`brief` é um verbo novo em `map`, não em `plan`.** `map` já é a metade de leitura + declarada do produto, e `brief` serve igualmente a uma spec (`Why` + contagem de + requisitos) — pôr em `plan` daria dois lugares para ler artefato. +- **`--ready`, `--blocked` e `--deps` são flags de `map tasks`, não verbos.** A pergunta + é a mesma ("quais tasks"), muda o filtro. Verbo novo por filtro infla o `--help`, que + é superfície documentada e portanto custa token. +- **Todos aceitam `--json`** por `addJSON`/`emitJSON`, como manda a convenção. O JSON é + o contrato para o harness; o texto é para a pessoa. +- **`brief` é limitado por construção, não por truncamento.** O cabeçalho é limitado + pelo contrato de seções fechadas (§3). Um `brief` que precisasse truncar seria sinal + de que o validador falhou, não de que o comando precisa de `--width`. +- **Nenhum desses comandos imprime prosa de task por padrão.** A descrição vai clipada + em `--width` (já existe); a continuação inteira só sai por `map show`, que é a + pergunta explícita "me mostra essa". + +Isto fecha a exigência do escopo: com `brief` + `tasks` + `show`, não sobra pergunta +sobre o plano cuja única resposta seja abrir o arquivo — e é isso que dá autoridade à +regra "nunca leia o plano" em `rules/artifacts.md`. + +### 8.5 O selo (checksum) + +**Canonicalização** — o que entra no hash: +- o arquivo inteiro, **menos a linha `checksum:`** (senão o hash referencia a si mesmo), +- com quebras normalizadas para LF (`textutil.NormalizeNewlines`, já usado), +- sha256 hex — reaproveitando `manifest.Hash`, que já faz as duas coisas. + +**Onde é verificado**: em *todo* comando que lê ou escreve tasks de um plano com +`status: approved` — `map tasks`, `map show`, `map `, `map index`, todos os +`patch`. Custo real ≈ 0: o arquivo já é lido; sobra um sha256 sobre ~4KB. + +**Ordem de operações no `patch` — isto é load-bearing**: +``` +1. carregar 5. revalidar +2. VERIFICAR selo 6. rollback se introduziu achado +3. aplicar edição 7. RESELAR (recalcular e escrever checksum) +4. escrever 8. reportar +``` +Verificar **antes** de aplicar é o que impede o cenário em que o harness edita à mão, +roda `patch check` e o resela por cima, apagando a evidência. + +**Erro de drift** — precisa ser acionável, porque o leitor é um agente que não pode ver +o arquivo: +``` +✗ plans/x.md — drift: o arquivo mudou fora do scc + selo: 9f2c… (status: approved) + atual: 41ab… + o conteúdo funcional de um plano aprovado só muda por scc patch. + → reverta com git, ou `scc plan reseal x --force` se a edição foi intencional +``` + +**Limites, ditos aqui e não descobertos depois:** +- É evidência, não impedimento. `reseal --force` está a um comando de distância e o + sha256 é público. O valor é que a violação vira ruidosa e auditável. +- **Conflito de merge**: cada tick reescreve a linha `checksum:` → dois branches que + marcam tasks diferentes conflitam **sempre** nessa linha. Sob `pr: per-group` com + worktrees paralelos isso é atrito real. Mitigações consideradas: selo fora do arquivo + (rejeitado — cria arquivo por plano, contra a convenção "nenhum arquivo de + configuração"); selo só sobre `## Tasks` sem os estados (rejeitado — deixaria a + alteração de `Why` passar batida, que é justamente o que se quer pegar). **Aceito o + conflito**, com `scc plan reseal` documentado como a resolução. O `plan-run` é + sequencial por desenho, então o caso é raro. +- Plano sem `status`/`checksum` = não selado = nenhuma verificação. É o que dá + compatibilidade retroativa de graça. + +--- + +## 9. Impacto em templates, regras e skills + +| Arquivo | Mudança | Risco | +|---|---|---| +| `artifacts/plan.md` | reescrito no contrato v2 | `TestFreshArtifactsPassTheirOwnValidators` tem de passar — o pior bug do produto é validador que dispara na própria saída | +| `rules/artifacts.md` | tabela de perguntas, endereços, e **"nunca leia o plano"** | **orçamento**: o arquivo é pré-carregado em toda requisição no Claude Code | +| `rules/tasks.md` | gramática de flags + lifecycle | mesmo orçamento | +| `rules/autonomy.md` | menciona `status:`/`checksum:` no frontmatter | pequena | +| `skills/plan-run/SKILL.md` | trocar "mapeie e escolha o grupo" por laço sobre `--next --json`; remover a permissão de ler o plano ("Read the plan itself only where the map is not enough" → nunca); **grupo passa a ser só a família de número maior** — a tabela de duas linhas em `SKILL.md:31-35` vira uma | média — a skill é longa e tem duas formas de PR, mas **D1 a encurta** | +| `commands/scc-plan-run.md` | idem | pequena | +| `entry.md` | linhas de `map`/`patch` | pré-carregado — cada linha conta | +| `agents/code-review.md` | já usa `map show`; conferir | pequena | + +**Orçamento explícito, porque é onde a meta pode ser perdida**: o conjunto +`entry.md` + `rules/artifacts.md` + `rules/tasks.md` **não pode crescer**. Medir antes e +depois (linhas e bytes) e registrar no PR. Se o contrato novo não couber, a saída é +mover a explicação de flags para dentro do `--help` do `scc patch` (que só é lido quando +consultado) e deixar na regra apenas a tabela pergunta→comando. + +`assets.Version` vai para `"14"`. `scc update` já trata isso com replace-or-keep; planos +são artefatos do usuário e não são tocados por `update` — a migração é o §10. + +--- + +## 10. Compatibilidade e migração + +### 10.1 Classificação das quebras + +| Mudança | Tipo | Quem sente | +|---|---|---| +| Seções fechadas | **breaking** para planos v1 com `Notes`/`Decomposition` | `scc validate` → exit 2 | +| Flags no parser | aditivo | ninguém: plano sem flag continua válido | +| `_Status removed_` | aditivo | — | +| `--next` com deps/prioridade | **comportamental** | sem flags o resultado é idêntico ao de hoje (primeira aberta) | +| Selo | aditivo e opt-in | plano sem `status:` não é verificado | +| Guardas de escrita em plano aprovado | breaking **por opção** — só depois de `approve` | — | +| Contrato de spec | **inalterado** | — | + +Ordem numérica correta em `--next` (`1.9` antes de `1.10`) é a única alteração de +comportamento que aparece sem o usuário pedir. É correção de defeito; vai no changelog. + +### 10.2 `scc plan migrate ` + +Automática no mecânico, **nunca destrutiva** no conteúdo: + +1. `--dry-run` por padrão? Não — `--dry-run` disponível, execução normal escreve, mas + nada é apagado (ver 3). +2. Garante as seções obrigatórias, criando as vazias com um marcador `` + que o validador aceita? **Não**: cria a seção vazia e deixa o achado aparecer. + Um placeholder que passa no validador é um plano que mente. +3. Seções fora do contrato (`Notes`, e quaisquer outras) são **movidas** para + `plans/archive/-notes.md`. `plans/archive/` é seguro: o scanner de planos usa + `ReadDir` e ignora diretórios, então o arquivo não vira um plano fantasma. +4. Normaliza a indentação das flags e reescreve as tasks na forma canônica. +5. Escreve `status: draft`. **Nunca aprova automaticamente** — aprovar é ato humano. +6. Relatório: seções movidas, tasks reescritas, achados restantes. + +Migração automática no momento da leitura foi considerada e rejeitada: reescrever +arquivo do usuário como efeito colateral de um `map` viola "never author what the user +owns" e produz diff que ninguém pediu. + +### 10.3 Caminho para o workspace já existente + +O plano de 56KB medido é o caso real. Com **D1** fechada, `migrate` faz três coisas +nele: move `## Notes` para `plans/archive/`, **renomeia `## Decomposition` para +`## References`** — o conteúdo é o mesmo, são itens de lista citando `specs//`, +então nenhuma linha muda — e cria vazias as seções obrigatórias que faltam +(`Out of scope`, `Done when`), deixando os achados aparecerem em vez de preenchê-las com +placeholder. As 31 referências de spec sobrevivem intactas e `map trace` continua +respondendo sobre elas. + +--- + +## 11. Decisões — fechadas + +**D1 — `## Decomposition` sai; as referências de spec vão para `## References`.** +Decidido. O contrato fica com as oito seções e nada mais. + +A consequência é mais barata do que parecia, e vale registrar por quê: `parseLeaves` +reconhece um item de lista que cita `specs//` **em qualquer lugar do arquivo**, +não por estar sob um heading específico. Então o tipo `Leaf`, o `TargetLeaf`, o endereço +`specs/foo/` e o `map trace` continuam funcionando **sem alteração de código** — passam +apenas a encontrar seus itens sob `## References`. O que fecha a porta para eles +aparecerem em outro lugar é a própria regra `plan.unknown-section`. + +O que de fato muda: +- `plan.unknown-spec` continua valendo, agora sobre `## References`. +- `plan.item-has-two-records` continua valendo e continua necessária: uma task não pode + citar uma spec. +- **`plan-run` perde "um grupo = um leaf".** Grupo passa a ser só a família de número + maior (`1.1`, `1.2` → grupo 1). Isso *simplifica* a skill: a tabela de duas linhas em + `SKILL.md:31-35` vira uma. Um trabalho grande o bastante para ser uma spec vira uma + task que aponta para a spec em `## References` e é marcada quando a spec fecha. +- O rótulo "decomposition" some do vocabulário: `map outline` deixa de imprimir + `N leaves` como categoria própria de plano. + +**D2 — O selo é verificado na leitura e na escrita.** Segue o pedido literal. `--no-verify` +existe para diagnóstico. Custo real ≈ 0: o arquivo já é lido; sobra um sha256 sobre ~4KB. + +**D3 — `map find` sai da documentação, não do binário.** Decidido. `runMapFind` e +`internal/artifact/search.go` ficam; a linha some de `rules/artifacts.md` e de +`entry.md`, que é onde o token é pago em toda requisição. Continua sendo a única +alternativa a ler `design.md` inteiro. + +**D4 — Flags indentadas com 2 espaços.** Decidido. A leitura aceita coluna 0 (um humano +que escreveu à mão não é punido); a escrita normaliza sempre para 2. Assim o bloco de +flags pertence à task pelas regras de indentação que o produto já usa, e `blockEnd` não +precisa de regra nova. + +**D5 — `migrate` move as seções fora do contrato para `plans/archive/-notes.md`.** +Decidido. Nada é apagado e nada exige `--force`. É seguro porque o scanner de planos usa +`ReadDir` e ignora diretórios: o arquivo arquivado não vira um plano fantasma nem é +validado. + +--- + +## 12. Riscos + +| # | Risco | Prob. | Impacto | Mitigação | +|---|---|---|---|---| +| R1 | `patch task --text` destrói flags | alta se não tratado | perda de dados | `renderTask` reemite flags; teste de round-trip é P0 | +| R2 | Flag parser engole prosa em itálico | média | task com região errada | vocabulário fechado; desconhecido vira achado, não conteúdo | +| R3 | Regras crescem e comem o ganho de token | **alta** | meta principal perdida | orçamento medido antes/depois no PR (§9) | +| R4 | Conflito de merge na linha `checksum:` | média | atrito em worktree paralelo | `plan reseal` documentado; `plan-run` é sequencial | +| R5 | Selo lido como garantia de segurança | média | decisão errada no futuro | registrado como tamper-*evidence* aqui e na regra | +| R6 | Validador dispara no template novo | média | pior bug do produto | `TestFreshArtifactsPassTheirOwnValidators` é gate obrigatório | +| R7 | Deadlock de `--next` por dep em task removida | média | loop trava sem explicação | achado `task.depends-on-removed` + saída `blocked` explícita | +| R8 | `plan-run` continua lendo o plano por hábito | alta | meta de contexto perdida | a superfície de §8.4 tem de existir **antes** da regra proibir a leitura — proibir sem oferecer a consulta produz desobediência justificada. Fase 3 antes da fase 7 | +| R9 | ~~D1 quebra `map trace`~~ — fechada: o parser reconhece o leaf pela citação, não pelo heading | baixa | — | resta só reescrever a noção de grupo do `plan-run` (fase 7) | +| R10 | CRLF/Windows muda o hash | baixa | drift falso | `manifest.Hash` já normaliza; teste específico no CI do Windows | + +--- + +## 13. Estratégia de implementação + +Oito fases, ordenadas para que **nada quebre antes de existir substituto**. Cada fase é +um PR com `make check` verde. + +| Fase | Conteúdo | Quebra algo? | +|---|---|---| +| **0** | ~~Decidir D1–D5~~ — **fechada**, §11 | — | +| **1** | `internal/artifact`: parse de flags, `Task.End` cobrindo flags, `Detail` sem flags, `renderTask` canônico, ordenação numérica de id. Testes de round-trip | não — aditivo | +| **2** | `internal/validate/plan.go`: seções fechadas, flags, deps, ciclo, lifecycle. Template v2 + `assets.Version = "14"` | sim — planos v1 passam a ter achados | +| **3** | A superfície de leitura: `map brief`, `--next` determinístico, `--ready`, `--blocked`, `--deps`, flags no JSON | comportamental | +| **4** | Selo: `plan approve`, `plan reseal`, verificação em leitura e escrita | aditivo (opt-in) | +| **5** | Discovery: guardas por `status`, `patch rm` lógico, alocação de número | breaking só em plano aprovado | +| **6** | `scc plan migrate` | aditivo | +| **7** | Regras, `entry.md`, `plan-run`, comandos — **com medição de orçamento** | — | +| **8** | Depreciações: tirar `find` da doc de plano, ajustar `--help`, changelog | — | + +Com D1–D5 fechadas, a fase 1 pode começar assim que o plano for aprovado. + +--- + +## 14. ToDos por prioridade + +### P0 — bloqueantes (nada começa sem) + +1. ~~Fechar D1–D5~~ — feito, §11. +2. Definir a canonicalização exata do hash e congelá-la em teste (`golden`), incluindo + caso CRLF. +3. Escrever `TestPlanV2TemplatePassesItsOwnValidator` **antes** do template. +4. Medir e registrar a linha de base do orçamento: linhas e bytes de `entry.md`, + `rules/artifacts.md`, `rules/tasks.md` **hoje** — o número contra o qual a fase 7 é + julgada. + +### P1 — o núcleo + +4. `artifact`: `Flag`, `Task.Depends/Priority/Status/Reason`, parse do bloco de flags. +5. `artifact`: `Task.End` inclui flags; `Detail` exclui; teste de que `map show` devolve + task+flags. +6. `artifact`: `renderTask` reemite flags em ordem canônica — **fecha R1**. +7. `artifact`: comparação numérica de id (`1.9` < `1.10`), com teste. +8. `artifact`: `Eligible()`/`Blocked()` + detecção de ciclo. +9. `validate/plan.go`: `unknown-section` (mensagem especial p/ `Notes`), `missing-section`, + `missing-description`. +10. `validate/tasks.go`: `unknown-flag`, `invalid-priority`, `invalid-status`, + `status-duplicates-box`, `removed-without-reason`, `removed-but-checked`, + `unknown-dependency`, `self-dependency`, `depends-on-removed`, `dependency-cycle`. +11. Template `artifacts/plan.md` v2 + `assets.Version = "14"`. +12. `map tasks --next`: algoritmo do §8.3, com tabela de testes cobrindo os 6 casos de borda. +13. `map tasks --json`: expor `depends`, `priority`, `status`, `blocked`, `eligible`. +14. `scc map brief ` — o cabeçalho sem as tasks (§8.4), com `--json`. +15. `map tasks --ready` / `--blocked` / `--deps`, compartilhando a ordenação de §8.3 + com `--next` — **uma implementação só**, ou teremos duas noções de elegibilidade. +16. `map `: contagens passam a distinguir feitas / abertas / **bloqueadas** / + **removidas**. +17. Teste de orçamento: `map brief` sobre o plano de 56KB migrado tem de caber no teto + declarado em §8.4 — se não couber, é o validador de seções que está frouxo. + +### P2 — imutabilidade e discovery + +18. `internal/artifact` (ou `internal/seal`): canonicalizar + hash + verificar. +19. `scc plan approve` — valida, exige limpo, sela. +20. `scc plan reseal --force` — com diff contra o selo antigo. +21. Verificação de drift nos caminhos de leitura e escrita, **antes** de aplicar (§8.5). +22. Guardas por `status: approved` em `patch task/append/prepend/replace`. +23. `patch rm` lógico + `patch add` com `--reason`, `--group`/`--new-group` e alocação + de número por high-water mark. +24. `Done()` e `map index` ignorando tasks `Removed`. + +### P3 — migração, documentação, limpeza + +25. `scc plan migrate` (§10.2) — incluindo a renomeação `## Decomposition` → `## References` + — + testes com plano v1 real como fixture. +26. Reescrever `rules/artifacts.md` e `rules/tasks.md` **medindo linhas antes/depois**; + a tabela pergunta→comando passa a ser a de §8.4. +27. Reescrever `plan-run` para o laço `brief` (1×) + `--next` (N×) e remover a permissão + de ler o plano. +28. `entry.md`, `commands/scc-plan-run.md`, `agents/code-review.md`. +29. Tirar `map find` da documentação de plano; ajustar `mapUsage()`/`patchUsage()`. +30. Changelog: quebras, `plan migrate`, ordenação numérica corrigida. +31. Reavaliar `map blocks` e `Search` sobre o corpus pós-migração — decidir com número, + não com impressão. + +--- + +## 15. Critério de pronto deste plano + +- ✅ D1–D5 respondidas (§11). +- ✅ Existe uma consulta para cada pergunta que hoje leva a abrir o plano (§8.4) — sem + isso a regra "nunca leia o plano" não teria autoridade. +- ✅ Orçamento de tokens das regras definido como número, não como intenção (P0 #4): + 55 linhas por regra e 60 no entry, impostos por `TestRulesStayShortEnoughToBePreloaded` + e `TestScaffoldedEntryFileStaysShort`. Medição antes/depois no topo deste arquivo. +- ✅ Aprovação do plano, e implementação — ver o topo deste arquivo. diff --git a/internal/artifact/artifact.go b/internal/artifact/artifact.go index f2ac043..1b6aea9 100644 --- a/internal/artifact/artifact.go +++ b/internal/artifact/artifact.go @@ -97,6 +97,8 @@ type Section struct { Words int `json:"words"` Tasks int `json:"tasks,omitempty"` Done int `json:"done,omitempty"` + Blocked int `json:"blocked,omitempty"` + Removed int `json:"removed,omitempty"` Leaves int `json:"leaves,omitempty"` } @@ -135,6 +137,7 @@ func Load(root, abs string) (*Artifact, error) { a.Tasks = parseTasks(doc) a.Requirements = parseRequirements(doc) a.Leaves = parseLeaves(doc) + a.resolveTaskStates() a.attribute() return a, nil } @@ -227,11 +230,19 @@ func (a *Artifact) attribute() { for i := range a.Sections { s := &a.Sections[i] for _, t := range a.Tasks { - if t.Line >= s.Line && t.Line <= s.End { - s.Tasks++ - if t.Checked { - s.Done++ - } + if t.Line < s.Line || t.Line > s.End { + continue + } + if t.Removed() { + s.Removed++ + continue + } + s.Tasks++ + switch { + case t.Checked: + s.Done++ + case t.Blocked: + s.Blocked++ } } for _, l := range a.Leaves { @@ -301,14 +312,83 @@ func (a *Artifact) Leaf(ref string) (Leaf, bool) { return Leaf{}, false } -// Done reports how many tasks are checked. +// Done reports how many tasks are checked, out of the tasks that are still work. +// +// A removed task is in neither number. It stays in the file so its number is never +// reused and its reason survives, but counting it would make a plan that dropped +// three items report as permanently unfinished. func (a *Artifact) Done() (done, total int) { for _, t := range a.Tasks { + if t.Removed() { + continue + } + total++ if t.Checked { done++ } } - return done, len(a.Tasks) + return done, total +} + +// Counts is the state of a plan's checklist in one shape: what is finished, what can +// be started, what is waiting, and what was struck out. +type Counts struct { + Total int `json:"total"` + Done int `json:"done"` + Ready int `json:"ready"` + Blocked int `json:"blocked"` + Removed int `json:"removed"` + Priority int `json:"-"` +} + +// Counts totals the checklist. +func (a *Artifact) Counts() Counts { + var c Counts + for _, t := range a.Tasks { + switch { + case t.Removed(): + c.Removed++ + case t.Checked: + c.Total++ + c.Done++ + case t.Eligible: + c.Total++ + c.Ready++ + default: + c.Total++ + c.Blocked++ + } + } + return c +} + +// HighWater is the largest item number used in a group, removed tasks included. +// +// Counting the removed is the whole point: a number is never reused, and because the +// removed task stays in the file the mark is derivable from the file itself — no +// counter, no state, and nothing extra for `scc update` to keep current. +func (a *Artifact) HighWater(group string) int { + high := 0 + for _, t := range a.Tasks { + if t.Group() != group { + continue + } + if n := itemOf(t.Number); n > high { + high = n + } + } + return high +} + +// HighGroup is the largest group number the plan has used. +func (a *Artifact) HighGroup() int { + high := 0 + for _, t := range a.Tasks { + if n := itemOf(t.Group()); n > high { + high = n + } + } + return high } // Text returns lines [from, to] inclusive, 1-based and clamped, as they appear in @@ -326,6 +406,44 @@ func (a *Artifact) Text(from, to int) string { return strings.Join(a.Lines[from-1:to], "\n") } +// Prose returns lines [from, to] as prose: the lines mdscan does not count as +// content — HTML comments and fenced blocks — dropped, and runs of blank lines +// collapsed to one. +// +// It is what a summary prints, and Text is what an editor prints. The difference +// matters most on a freshly scaffolded artifact, where the instructions to the author +// are HTML comments and outweigh everything the author has written so far: a brief +// that echoed them would spend its whole budget on text addressed to somebody else. +// The cost is that a fenced example inside a section does not survive a summary, +// which is the right trade for a summary and the wrong one for an edit. +func (a *Artifact) Prose(from, to int) string { + if from < 1 { + from = 1 + } + if to > len(a.Lines) { + to = len(a.Lines) + } + var out []string + for n := from; n <= to; n++ { + raw := a.Lines[n-1] + if n <= len(a.doc.Body) && strings.TrimSpace(a.doc.Body[n-1]) == "" { + if strings.TrimSpace(raw) != "" { + continue // a comment or a fence: not content + } + if len(out) == 0 || out[len(out)-1] == "" { + continue + } + out = append(out, "") + continue + } + out = append(out, raw) + } + for len(out) > 0 && out[len(out)-1] == "" { + out = out[:len(out)-1] + } + return strings.Join(out, "\n") +} + // Resolve turns whatever the caller typed into the artifact files it names. // // It accepts, in order: a path that exists relative to the workspace root, a path diff --git a/internal/artifact/edit.go b/internal/artifact/edit.go index 53d6d7d..83287f7 100644 --- a/internal/artifact/edit.go +++ b/internal/artifact/edit.go @@ -74,6 +74,11 @@ func (e *Editor) fail(format string, args ...any) { } } +// Fail records a refusal from the caller's own policy — the approved-plan guards, +// which depend on what the file says and so cannot be decided before it is loaded. +// It reads back as an editor error so one path reports every reason a patch stopped. +func (e *Editor) Fail(format string, args ...any) { e.fail(format, args...) } + // Empty reports whether nothing would change. func (e *Editor) Empty() bool { return len(e.splices) == 0 } @@ -175,6 +180,15 @@ type TaskEdit struct { Requirements *[]string Number *string Checked *bool + + // The flags. Priority is two fields rather than one **int: "set it to 3" and + // "it has none" are different edits, and a pointer to a pointer expresses that + // at the cost of every caller having to read it twice. + Depends *[]string + Priority *int + ClearPriority bool + Status *string + Reason *string } // SetTask rewrites one task in place, re-rendering it from its parts. @@ -208,10 +222,56 @@ func (e *Editor) SetTask(number string, edit TaskEdit) { if edit.Text != nil { next.Text = strings.TrimSpace(*edit.Text) next.Detail = "" + next.detail = nil + } + if edit.Depends != nil { + next.Depends = *edit.Depends + } + if edit.ClearPriority { + next.Priority, next.BadPriority = nil, "" + } + if edit.Priority != nil { + p := *edit.Priority + next.Priority, next.BadPriority = &p, "" + } + if edit.Status != nil { + next.Status, next.BadStatus = *edit.Status, "" + } + if edit.Reason != nil { + next.Reason = strings.TrimSpace(*edit.Reason) } e.add("set", number, t.Line, t.End-t.Line+1, renderTask(next)) } +// StrikeTask is discovery's removal: the task stays where it is, carrying the reason +// it stopped being work. +// +// A logical removal rather than a deletion, because the number is the address every +// other artifact and every earlier commit used, and because a plan that silently +// shrank would be a plan whose history is only in git. The high-water mark that +// stops the number being reused is derived from this line staying put. +func (e *Editor) StrikeTask(number, reason string) { + if e.err != nil { + return + } + t, ok := e.a.Task(number) + if !ok { + e.fail("%v", e.a.unknown("task", number)) + return + } + if strings.TrimSpace(reason) == "" { + e.fail("removing task %s needs a reason: it is the whole record of why the work went away", number) + return + } + if t.Removed() { + return + } + next := t + next.Status, next.BadStatus = StatusRemoved, "" + next.Reason = strings.TrimSpace(reason) + e.add("strike", number, t.Line, t.End-t.Line+1, renderTask(next)) +} + // NewTask is a task about to be written. Section is where it lands: the slug of the // heading whose task list it joins. type NewTask struct { @@ -221,6 +281,12 @@ type NewTask struct { Text string Requirements []string Checked bool + Depends []string + Priority *int + // Reason is why this task turned up after the plan was approved. It is the same + // flag a removal carries, because it answers the same question — why this line is + // not what the plan somebody agreed to said. + Reason string } // AddTask appends a task to a section's list. @@ -259,6 +325,7 @@ func (e *Editor) AddTask(t NewTask) { lines := renderTask(Task{ Number: t.Number, Methodology: t.Methodology, Text: t.Text, Requirements: t.Requirements, Checked: t.Checked, + Depends: t.Depends, Priority: t.Priority, Reason: t.Reason, }) e.add("add", t.Number, at, 0, lines) } @@ -371,6 +438,12 @@ func (e *Editor) SetFrontmatter(key, value string) { // renderTask writes a task back out in the grammar, wrapped the way the artifacts // wrap: the continuation indented to sit under the description rather than under the // bullet, which is what makes a multi-line task read as one item. +// +// It re-emits the continuation and the flags, and both are load-bearing. A rewrite +// that dropped them would make `patch task --method TDD` a data-loss command: +// changing one word would silently delete a sixty-line description and every +// dependency the task declared. The flags come last, in the canonical order, so a +// file that has been through this twice produces no diff the second time. func renderTask(t Task) []string { head := "- [ ] " if t.Checked { @@ -386,7 +459,9 @@ func renderTask(t Task) []string { if len(t.Requirements) > 0 { body += " " + CitationSeparator + " " + strings.Join(t.Requirements, ", ") } - return wrap(head, strings.Repeat(" ", len(head)), body, LineWidth) + out := wrap(head, strings.Repeat(" ", len(head)), body, LineWidth) + out = append(out, t.detail...) + return append(out, renderFlags(t)...) } // wrap breaks text into lines that fit width, with prefix on the first and indent on diff --git a/internal/artifact/flags.go b/internal/artifact/flags.go new file mode 100644 index 0000000..5f05a81 --- /dev/null +++ b/internal/artifact/flags.go @@ -0,0 +1,276 @@ +package artifact + +import ( + "regexp" + "sort" + "strconv" + "strings" + + "github.com/protonspy/spec-claude-code/internal/mdscan" +) + +// A task's flags: the metadata that does not fit on the checkbox line. +// +// The vocabulary is closed, and that is the whole design. An italic one-liner is a +// shape prose uses too, so a parser that absorbed any of them would quietly eat a +// sentence and hand the task a region that is not the task. Four names are flags; +// anything else in that position is a fact the validator turns into a finding, and +// the parser leaves it exactly where it found it. +const ( + FlagDepends = "Depends" + FlagPriority = "Priority" + FlagStatus = "Status" + FlagReason = "Reason" +) + +// StatusRemoved is the only value `_Status_` accepts. +// +// Not `open`, and not `completed`: the box is the state, and a flag that could +// restate it would be a second record of one fact — the defect +// `plan.item-has-two-records` already exists to catch. +const StatusRemoved = "removed" + +// FlagIndent is what scc writes. Reading accepts column 0 as well, so a person who +// wrote the flag by hand is not punished for it, but everything scc emits is +// indented — and an indented line already belongs to the task under the same +// indentation rules the rest of the product uses. +const FlagIndent = " " + +// FlagNames is the canonical order flags are written back in. One order, so a file +// that has been through `scc patch` twice produces no diff the second time. +var FlagNames = []string{FlagDepends, FlagPriority, FlagStatus, FlagReason} + +// flagLineRe matches a line that is nothing but one italic annotation: `_Depends 1.1_`. +// The whole line, because a flag is a line — an italic span inside a sentence is +// emphasis, and emphasis is prose. +var flagLineRe = regexp.MustCompile(`^[ \t]*_([A-Za-z][A-Za-z-]*)[ \t]*([^_]*)_[ \t]*$`) + +// Flag is one annotation line as written, kept in file order so the validator can +// report a duplicate at the line that duplicated it. +type Flag struct { + Name string `json:"name"` + Value string `json:"value,omitempty"` + Line int `json:"line"` +} + +// ItalicLine reads a line shaped like a flag, whatever it is called. It is what +// separates "a flag with a name nobody defined" from "a line that is not a flag at +// all" — the first is a typo worth reporting, the second is prose. +func ItalicLine(line string) (name, value string, ok bool) { + m := flagLineRe.FindStringSubmatch(line) + if m == nil { + return "", "", false + } + return m[1], strings.TrimSpace(m[2]), true +} + +// KnownFlag reads a line that is a flag in the vocabulary, returning the canonical +// spelling of its name. +func KnownFlag(line string) (name, value string, ok bool) { + n, v, ok := ItalicLine(line) + if !ok { + return "", "", false + } + for _, want := range FlagNames { + if strings.EqualFold(want, n) { + return want, v, true + } + } + return "", "", false +} + +// Removed reports whether the task was struck out by discovery rather than done. +// A removed task keeps its line — the number it consumed is never reused, and the +// reason it went away is the record — so it is neither open nor complete. +func (t Task) Removed() bool { return t.Status == StatusRemoved } + +// parseTaskFlags reads the flag block that belongs to t, extends t.End to cover it, +// and reports back which lines it claimed so they stay out of the description. +// +// Two runs, because a flag can be written two ways and both have to end up in the +// same region. An indented flag is already inside the task's block by the +// indentation rules that govern every list item, so it is found by walking back from +// the end of that block; a flag at column 0 belongs to nobody under those rules, so +// it is found by walking forward past at most one blank line. Either way the task's +// End covers its flags — which is what makes `map show 1.1` return them, `patch rm +// 1.1` remove them, and the rollback able to put them back. +func parseTaskFlags(doc *mdscan.Document, t *Task) map[int]bool { + claimed := map[int]bool{} + + for n := t.End; n > t.Line; n-- { + text := doc.Body[n-1] + if strings.TrimSpace(text) == "" { + continue + } + if _, _, ok := KnownFlag(text); !ok { + // The line that stopped the run is prose — unless it is shaped like a flag, + // in which case it is a flag whose name nobody defined, and saying so is + // better than swallowing it into the description. + if name, value, italic := ItalicLine(text); italic { + t.UnknownFlags = append(t.UnknownFlags, Flag{Name: name, Value: value, Line: n}) + } + break + } + claimed[n] = true + } + + n := t.End + 1 + if n <= len(doc.Body) && strings.TrimSpace(doc.Body[n-1]) == "" { + n++ + } + for n <= len(doc.Body) { + if _, _, ok := KnownFlag(doc.Body[n-1]); !ok { + break + } + claimed[n] = true + t.End = n + n++ + } + for n <= len(doc.Body) { + name, value, italic := ItalicLine(doc.Body[n-1]) + if !italic { + break + } + t.UnknownFlags = append(t.UnknownFlags, Flag{Name: name, Value: value, Line: n}) + n++ + } + + nums := make([]int, 0, len(claimed)) + for line := range claimed { + nums = append(nums, line) + } + sort.Ints(nums) + for _, line := range nums { + name, value, _ := KnownFlag(doc.Body[line-1]) + t.Flags = append(t.Flags, Flag{Name: name, Value: value, Line: line}) + } + applyFlags(t) + sort.Slice(t.UnknownFlags, func(i, j int) bool { return t.UnknownFlags[i].Line < t.UnknownFlags[j].Line }) + return claimed +} + +// applyFlags turns the annotations into the fields, first occurrence winning. A +// value the vocabulary does not accept is kept as the fact that it was written — +// the validator reports it, and renderTask writes it back rather than deleting +// something a person meant. +func applyFlags(t *Task) { + seen := map[string]bool{} + for _, f := range t.Flags { + if seen[f.Name] { + continue + } + seen[f.Name] = true + switch f.Name { + case FlagDepends: + t.Depends = splitIDs(f.Value) + case FlagPriority: + n, err := strconv.Atoi(strings.TrimSpace(f.Value)) + if err != nil || n < 1 { + t.BadPriority = f.Value + continue + } + t.Priority = &n + case FlagStatus: + if !strings.EqualFold(strings.TrimSpace(f.Value), StatusRemoved) { + t.BadStatus = f.Value + continue + } + t.Status = StatusRemoved + case FlagReason: + t.Reason = f.Value + } + } +} + +// splitIDs reads a comma-separated dependency list. +func splitIDs(s string) []string { + var out []string + for _, part := range strings.FieldsFunc(s, func(r rune) bool { return r == ',' || r == ' ' || r == '\t' }) { + if p := strings.TrimSpace(part); p != "" { + out = append(out, p) + } + } + return out +} + +// renderFlags writes a task's flags in the canonical order, one per line, indented. +// +// An invalid value is written back as it was found rather than dropped: the +// validator has already said it is wrong, and a rewrite that silently deleted it +// would turn a reported defect into a lost sentence. +func renderFlags(t Task) []string { + var out []string + if len(t.Depends) > 0 { + out = append(out, flagLine(FlagDepends, strings.Join(t.Depends, ", "))) + } + switch { + case t.Priority != nil: + out = append(out, flagLine(FlagPriority, strconv.Itoa(*t.Priority))) + case t.BadPriority != "": + out = append(out, flagLine(FlagPriority, t.BadPriority)) + } + switch { + case t.Status != "": + out = append(out, flagLine(FlagStatus, t.Status)) + case t.BadStatus != "": + out = append(out, flagLine(FlagStatus, t.BadStatus)) + } + if t.Reason != "" { + out = append(out, flagLine(FlagReason, t.Reason)) + } + return out +} + +func flagLine(name, value string) string { + if value == "" { + return FlagIndent + "_" + name + "_" + } + return FlagIndent + "_" + name + " " + value + "_" +} + +// itemOf is the last component of a dotted number, read as an integer. A component +// that is not a number is 0, which is below every real one and therefore never the +// high-water mark. +func itemOf(number string) int { + if i := strings.LastIndex(number, "."); i >= 0 { + number = number[i+1:] + } + n, err := strconv.Atoi(strings.TrimSpace(number)) + if err != nil { + return 0 + } + return n +} + +// CompareNumbers orders two task numbers the way a reader does: component by +// component, numerically. +// +// This is a correction, not a preference. Sorted as text `1.10` comes before `1.9`, +// so a loop that asked for the next task got the tenth before the ninth as soon as a +// group grew past nine items. +func CompareNumbers(a, b string) int { + as, bs := strings.Split(a, "."), strings.Split(b, ".") + for i := 0; i < len(as) && i < len(bs); i++ { + x, errA := strconv.Atoi(as[i]) + y, errB := strconv.Atoi(bs[i]) + if errA != nil || errB != nil { + if as[i] != bs[i] { + return strings.Compare(as[i], bs[i]) + } + continue + } + if x != y { + if x < y { + return -1 + } + return 1 + } + } + switch { + case len(as) < len(bs): + return -1 + case len(as) > len(bs): + return 1 + } + return 0 +} diff --git a/internal/artifact/flags_test.go b/internal/artifact/flags_test.go new file mode 100644 index 0000000..9ea6fc3 --- /dev/null +++ b/internal/artifact/flags_test.go @@ -0,0 +1,343 @@ +package artifact + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// flaggedPlan carries every shape the flag block can take: the canonical indented +// form, the column-0 form a person writes by hand, a removal, and a dependency +// nothing satisfies. +const flaggedPlan = `--- +autonomy: auto +ci: wait +--- + +# Flagged + +## Why + +One paragraph. + +## Tasks + +- [x] 1.1 (Unit) Lay the foundation +- [ ] 1.2 (TDD) Build on it, with a description that runs onto a second line so the + continuation is exercised too + _Depends 1.1_ + _Priority 2_ +- [ ] 1.3 (Unit) Wait for something that is not done + +_Depends 1.2_ + +- [ ] 1.4 (Unit) Dropped after the fact + _Status removed_ + _Reason the upstream API made it unnecessary_ + +## Done when + +- everything above is ticked +` + +func loadFlagged(t *testing.T) *Artifact { + t.Helper() + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "plans"), 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(root, "plans", "flagged.md") + if err := os.WriteFile(path, []byte(flaggedPlan), 0o644); err != nil { + t.Fatal(err) + } + a, err := Load(root, path) + if err != nil { + t.Fatal(err) + } + return a +} + +func task(t *testing.T, a *Artifact, number string) Task { + t.Helper() + got, ok := a.Task(number) + if !ok { + t.Fatalf("no task %s", number) + } + return got +} + +func TestFlagsParseInBothIndentations(t *testing.T) { + a := loadFlagged(t) + + two := task(t, a, "1.2") + if len(two.Depends) != 1 || two.Depends[0] != "1.1" { + t.Fatalf("1.2 depends = %v, want [1.1]", two.Depends) + } + if two.Priority == nil || *two.Priority != 2 { + t.Fatalf("1.2 priority = %v, want 2", two.Priority) + } + + // Column 0, one blank line above it: the form the hand-written example uses. + three := task(t, a, "1.3") + if len(three.Depends) != 1 || three.Depends[0] != "1.2" { + t.Fatalf("1.3 depends = %v, want [1.2]", three.Depends) + } +} + +// The flags have to be inside the task's region, or every command that addresses a +// task by number addresses only part of it: `show` hides the dependency, `rm` leaves +// it behind pointing at nothing, and the rollback cannot restore what it never saw. +func TestTaskRegionCoversItsFlags(t *testing.T) { + a := loadFlagged(t) + for _, tc := range []struct{ number, want string }{ + {"1.2", "_Priority 2_"}, + {"1.3", "_Depends 1.2_"}, + {"1.4", "_Reason the upstream API made it unnecessary_"}, + } { + got := task(t, a, tc.number) + text := a.Text(got.Line, got.End) + if !strings.Contains(text, tc.want) { + t.Errorf("task %s region %q does not carry %q", tc.number, text, tc.want) + } + } +} + +// A flag is metadata, not description. Leaving `_Priority 2_` in Detail would index +// it as prose in the searcher and spend the caller's --width budget on it. +func TestDescriptionExcludesFlags(t *testing.T) { + a := loadFlagged(t) + for _, number := range []string{"1.2", "1.3", "1.4"} { + if d := task(t, a, number).Detail; strings.Contains(d, "_") { + t.Errorf("task %s detail carries a flag line: %q", number, d) + } + } + if d := task(t, a, "1.2").Detail; !strings.Contains(d, "continuation is exercised") { + t.Errorf("task 1.2 lost its continuation: %q", d) + } +} + +func TestRemovedTaskIsNeitherOpenNorDone(t *testing.T) { + a := loadFlagged(t) + four := task(t, a, "1.4") + if !four.Removed() { + t.Fatal("1.4 is not removed") + } + if four.Eligible || four.Blocked { + t.Errorf("1.4 eligible=%v blocked=%v; a removed task is neither", four.Eligible, four.Blocked) + } + done, total := a.Done() + if done != 1 || total != 3 { + t.Errorf("Done() = %d/%d, want 1/3 — the removed task is in neither number", done, total) + } + if c := a.Counts(); c.Removed != 1 || c.Ready != 1 || c.Blocked != 1 || c.Done != 1 { + t.Errorf("counts = %+v", c) + } +} + +func TestEligibilityFollowsDependencies(t *testing.T) { + a := loadFlagged(t) + if !task(t, a, "1.2").Eligible { + t.Error("1.2 depends on 1.1, which is done, so it is eligible") + } + if !task(t, a, "1.3").Blocked { + t.Error("1.3 depends on 1.2, which is open, so it is blocked") + } + next, ok := a.Next() + if !ok || next.Number != "1.2" { + t.Fatalf("next = %v %q, want 1.2", ok, next.Number) + } + if waiting := a.WaitingOn(task(t, a, "1.3")); len(waiting) != 1 || waiting[0] != "1.2" { + t.Errorf("1.3 waiting on %v, want [1.2]", waiting) + } +} + +// R1: the failure this whole phase exists to prevent. Rewriting one field of a task +// must not take its dependencies with it. +func TestRewritingATaskKeepsItsFlagsAndContinuation(t *testing.T) { + a := loadFlagged(t) + method := "Unit" + e := a.Edit() + e.SetTask("1.2", TaskEdit{Methodology: &method}) + out, err := e.Content() + if err != nil { + t.Fatal(err) + } + for _, want := range []string{"_Depends 1.1_", "_Priority 2_", "continuation is exercised"} { + if !strings.Contains(out, want) { + t.Errorf("rewriting 1.2 lost %q", want) + } + } +} + +// parse(render(parse(x))) == parse(x): patching twice must not produce a diff the +// second time, which is what makes a canonical form worth having. +func TestRenderingATaskIsIdempotent(t *testing.T) { + a := loadFlagged(t) + root := filepath.Dir(filepath.Dir(a.Abs)) + + rewrite := func(in *Artifact) *Artifact { + t.Helper() + e := in.Edit() + for _, task := range in.Tasks { + text := task.Text + e.SetTask(task.Number, TaskEdit{Text: &text}) + } + content, err := e.Content() + if err != nil { + t.Fatal(err) + } + path := filepath.Join(root, "plans", "flagged.md") + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + next, err := Load(root, path) + if err != nil { + t.Fatal(err) + } + return next + } + + once := rewrite(a) + first := once.Text(1, once.LineCount()) + twice := rewrite(once) + if second := twice.Text(1, twice.LineCount()); second != first { + t.Errorf("a second rewrite changed the file:\n--- first\n%s\n--- second\n%s", first, second) + } +} + +func TestStrikeTaskKeepsTheLineAndTheNumber(t *testing.T) { + a := loadFlagged(t) + e := a.Edit() + e.StrikeTask("1.3", "the requirement was withdrawn") + out, err := e.Content() + if err != nil { + t.Fatal(err) + } + if !strings.Contains(out, "1.3 (Unit) Wait for something") { + t.Error("striking 1.3 deleted it; a removed task keeps its line") + } + if !strings.Contains(out, "_Status removed_") || !strings.Contains(out, "_Reason the requirement was withdrawn_") { + t.Errorf("striking 1.3 did not record the removal:\n%s", out) + } + if e := a.Edit(); func() error { e.StrikeTask("1.3", " "); return e.Err() }() == nil { + t.Error("a removal with no reason should be refused") + } +} + +func TestHighWaterCountsRemovedTasks(t *testing.T) { + a := loadFlagged(t) + if got := a.HighWater("1"); got != 4 { + t.Errorf("HighWater(1) = %d, want 4 — the removed 1.4 still holds its number", got) + } + if got := a.HighGroup(); got != 1 { + t.Errorf("HighGroup() = %d, want 1", got) + } +} + +// 1.10 comes after 1.9. Sorted as text it does not, which is what made the loop +// hand out the tenth task before the ninth as soon as a group grew. +func TestNumbersCompareNumerically(t *testing.T) { + for _, tc := range []struct { + a, b string + want int + }{ + {"1.9", "1.10", -1}, + {"1.10", "1.9", 1}, + {"2.1", "10.1", -1}, + {"1.2", "1.2", 0}, + {"1", "1.1", -1}, + } { + if got := CompareNumbers(tc.a, tc.b); got != tc.want { + t.Errorf("CompareNumbers(%q, %q) = %d, want %d", tc.a, tc.b, got, tc.want) + } + } +} + +func TestPriorityOrdersBeforeNumber(t *testing.T) { + tasks := []Task{ + {Number: "1.9", Line: 3}, + {Number: "1.10", Line: 4}, + {Number: "2.1", Line: 5, Priority: ptr(1)}, + } + SortTasks(tasks) + want := []string{"2.1", "1.9", "1.10"} + for i, t2 := range tasks { + if t2.Number != want[i] { + t.Fatalf("order = %v, want %v", numbersOf(tasks), want) + } + } +} + +func TestDependencyCycleIsFound(t *testing.T) { + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "plans"), 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(root, "plans", "cyclic.md") + content := `# Cyclic + +## Tasks + +- [ ] 1.1 (Unit) One + _Depends 1.2_ +- [ ] 1.2 (Unit) Two + _Depends 1.1_ +` + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + a, err := Load(root, path) + if err != nil { + t.Fatal(err) + } + cycles := a.DependencyCycles() + if len(cycles) != 1 { + t.Fatalf("cycles = %v, want one", cycles) + } + if cycles[0][0] != "1.1" { + t.Errorf("cycle = %v, want it to start at its smallest member", cycles[0]) + } + if task(t, a, "1.1").Eligible || task(t, a, "1.2").Eligible { + t.Error("neither task on a cycle can be eligible") + } +} + +func TestUnknownItalicLineIsReportedNotSwallowed(t *testing.T) { + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "plans"), 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(root, "plans", "typo.md") + content := `# Typo + +## Tasks + +- [ ] 1.1 (Unit) One + _Depend 1.0_ +` + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + a, err := Load(root, path) + if err != nil { + t.Fatal(err) + } + one := task(t, a, "1.1") + if len(one.UnknownFlags) != 1 || one.UnknownFlags[0].Name != "Depend" { + t.Fatalf("unknown flags = %+v, want the typo reported", one.UnknownFlags) + } + if len(one.Depends) != 0 { + t.Errorf("a flag nobody defined must not be read as one: %v", one.Depends) + } +} + +func ptr(n int) *int { return &n } + +func numbersOf(tasks []Task) []string { + out := make([]string, len(tasks)) + for i, t := range tasks { + out[i] = t.Number + } + return out +} diff --git a/internal/artifact/parse.go b/internal/artifact/parse.go index 2c5f36b..daa7824 100644 --- a/internal/artifact/parse.go +++ b/internal/artifact/parse.go @@ -61,9 +61,29 @@ type Task struct { Indent int `json:"-"` Section string `json:"section,omitempty"` + // The flags, read from the annotation lines under the checkbox. See flags.go + // for why the vocabulary is closed. + Depends []string `json:"depends,omitempty"` + Priority *int `json:"priority,omitempty"` + Status string `json:"status,omitempty"` + Reason string `json:"reason,omitempty"` + + // Derived, and in the JSON on purpose: the consumer is an agent, and an agent + // that recomputed eligibility would be a second implementation of `--next`. + Blocked bool `json:"blocked"` + Eligible bool `json:"eligible"` + Methodologies int `json:"-"` // how many annotations the line carried Loose string `json:"-"` // a near-miss annotation, when there is no valid one HasCitation bool `json:"-"` // the line carried the em dash that opens citations + Flags []Flag `json:"-"` // every flag line, in file order, duplicates included + UnknownFlags []Flag `json:"-"` // italic one-liners in flag position that are not vocabulary + BadPriority string `json:"-"` // a `_Priority_` value that is not a positive integer + BadStatus string `json:"-"` // a `_Status_` value that is not `removed` + + // detail is the continuation verbatim, flag lines excluded, so a rewrite that + // was not asked to change the description does not destroy it. + detail []string } // Group is the task's number with its last component removed — the heading it @@ -143,7 +163,9 @@ func parseTasks(doc *mdscan.Document) []Task { t.HasCitation = hasTail t.End = blockEnd(doc, box.Line, box.Indent) - t.Detail = joinLines(doc, box.Line+1, t.End) + claimed := parseTaskFlags(doc, &t) + t.Detail = joinLinesExcept(doc, box.Line+1, t.End, claimed) + t.detail = rawLinesExcept(doc, box.Line+1, t.End, claimed) tasks = append(tasks, t) } return tasks @@ -324,11 +346,22 @@ func indentOf(s string) int { // joinLines collapses a line range into one whitespace-normalized string. func joinLines(doc *mdscan.Document, from, to int) string { + return joinLinesExcept(doc, from, to, nil) +} + +// joinLinesExcept is joinLines with a set of lines held back — a task's flags, which +// belong to its region but not to its description. Keeping `_Priority 2_` out of +// Detail is what stops the searcher from indexing it as prose and keeps `--width` +// honest about how much of the description it clipped. +func joinLinesExcept(doc *mdscan.Document, from, to int, skip map[int]bool) string { if from > to { return "" } var parts []string for n := from; n <= to && n <= len(doc.Body); n++ { + if skip[n] { + continue + } if f := strings.Fields(doc.Body[n-1]); len(f) > 0 { parts = append(parts, strings.Join(f, " ")) } @@ -336,6 +369,23 @@ func joinLines(doc *mdscan.Document, from, to int) string { return strings.Join(parts, " ") } +// rawLinesExcept is the same range as it sits in the file, flags removed and +// trailing blanks trimmed. It is what a rewrite puts back when it was not asked to +// change the description. +func rawLinesExcept(doc *mdscan.Document, from, to int, skip map[int]bool) []string { + var out []string + for n := from; n <= to && n <= len(doc.Lines); n++ { + if skip[n] { + continue + } + out = append(out, doc.Lines[n-1]) + } + for len(out) > 0 && strings.TrimSpace(out[len(out)-1]) == "" { + out = out[:len(out)-1] + } + return out +} + // clip shortens s to n runes, marking that it was shortened. It counts runes rather // than bytes because the artifacts are full of em dashes and box-drawing characters, // and a byte clip would cut one in half. diff --git a/internal/artifact/schedule.go b/internal/artifact/schedule.go new file mode 100644 index 0000000..fe6fc58 --- /dev/null +++ b/internal/artifact/schedule.go @@ -0,0 +1,213 @@ +package artifact + +import "sort" + +// Which task comes next, decided once. +// +// `--next`, `--ready` and `--blocked` are three views of one question, so they share +// one implementation. Two notions of eligibility would be two answers to "what do I +// work on", and the loop would take whichever it asked first. + +// resolveTaskStates fills in the derived half of every task: whether its +// dependencies are satisfied, and therefore whether it can be started. +// +// A task that is done or removed is neither eligible nor blocked — it is finished +// with, and reporting it as blocked would put it in the impasse listing forever. +func (a *Artifact) resolveTaskStates() { + byNumber := make(map[string]int, len(a.Tasks)) + for i := range a.Tasks { + if _, seen := byNumber[a.Tasks[i].Number]; !seen { + byNumber[a.Tasks[i].Number] = i + } + } + for i := range a.Tasks { + t := &a.Tasks[i] + t.Eligible, t.Blocked = false, false + if t.Checked || t.Removed() { + continue + } + ready := true + for _, d := range t.Depends { + j, found := byNumber[d] + if !found || !a.Tasks[j].Checked { + ready = false + break + } + } + t.Eligible, t.Blocked = ready, !ready + } +} + +// WaitingOn is what a blocked task is waiting for: the dependencies that are not +// done, plus the ones that do not exist. A blocked task that could not name its +// blocker would be an impasse with no way out. +func (a *Artifact) WaitingOn(t Task) []string { + var out []string + for _, d := range t.Depends { + dep, ok := a.Task(d) + if !ok || !dep.Checked { + out = append(out, d) + } + } + return out +} + +// Ready is the eligible tasks in the order a loop should take them: by priority, +// then by number read as numbers, then by position in the file. +// +// Priority is ascending and absent sorts last, which is the reading a person +// expects — "priority 1" is the urgent one, and a task nobody prioritized is not +// more urgent than one somebody did. +func (a *Artifact) Ready() []Task { + var out []Task + for _, t := range a.Tasks { + if t.Eligible { + out = append(out, t) + } + } + SortTasks(out) + return out +} + +// BlockedTasks is the open tasks that are not eligible, in the same order. +func (a *Artifact) BlockedTasks() []Task { + var out []Task + for _, t := range a.Tasks { + if t.Blocked { + out = append(out, t) + } + } + SortTasks(out) + return out +} + +// Next is the task to work on, if there is one. +func (a *Artifact) Next() (Task, bool) { + ready := a.Ready() + if len(ready) == 0 { + return Task{}, false + } + return ready[0], true +} + +// SortTasks orders tasks the way §"what do I work on next" defines: priority +// ascending with absent last, then number numerically, then file order as a total +// tie-break so the result never depends on the sort's stability. +func SortTasks(tasks []Task) { + sort.SliceStable(tasks, func(i, j int) bool { + a, b := tasks[i], tasks[j] + pa, pb := priorityOf(a), priorityOf(b) + if pa != pb { + return pa < pb + } + if c := CompareNumbers(a.Number, b.Number); c != 0 { + return c < 0 + } + return a.Line < b.Line + }) +} + +// priorityOf is the task's priority, or a value past every real one. +const noPriority = int(^uint(0) >> 1) + +func priorityOf(t Task) int { + if t.Priority == nil { + return noPriority + } + return *t.Priority +} + +// DependencyCycles returns every cycle in the task graph, each as the numbers on it +// starting from its smallest member so the same cycle reports identically twice. +// +// A cycle is reported rather than worked around because there is no right answer to +// work around it with: every task on it is waiting for another one on it, and a +// `--next` that picked one anyway would start work whose dependency will never be +// satisfied. +func (a *Artifact) DependencyCycles() [][]string { return Cycles(a.Tasks) } + +// Cycles is DependencyCycles over a bare task list, so the validator can report one +// without building an Artifact around the document it is already holding. +func Cycles(tasks []Task) [][]string { + const ( + white = 0 + grey = 1 + black = 2 + ) + color := map[string]int{} + index := make(map[string]Task, len(tasks)) + var order []string + for _, t := range tasks { + if t.Number == "" { + continue + } + if _, seen := index[t.Number]; seen { + continue + } + index[t.Number] = t + order = append(order, t.Number) + } + + seen := map[string]bool{} + var cycles [][]string + var stack []string + + var visit func(string) + visit = func(n string) { + color[n] = grey + stack = append(stack, n) + for _, d := range index[n].Depends { + if _, ok := index[d]; !ok { + continue + } + switch color[d] { + case white: + visit(d) + case grey: + at := len(stack) - 1 + for at >= 0 && stack[at] != d { + at-- + } + if at < 0 { + continue + } + cycle := append([]string(nil), stack[at:]...) + if key := cycleKey(cycle); !seen[key] { + seen[key] = true + cycles = append(cycles, normalizeCycle(cycle)) + } + } + } + stack = stack[:len(stack)-1] + color[n] = black + } + for _, n := range order { + if color[n] == white { + visit(n) + } + } + sort.Slice(cycles, func(i, j int) bool { return CompareNumbers(cycles[i][0], cycles[j][0]) < 0 }) + return cycles +} + +// normalizeCycle rotates a cycle so it starts at its smallest number, which is what +// makes the same loop report the same way whichever node the walk entered it from. +func normalizeCycle(cycle []string) []string { + at := 0 + for i, n := range cycle { + if CompareNumbers(n, cycle[at]) < 0 { + at = i + } + } + return append(append([]string(nil), cycle[at:]...), cycle[:at]...) +} + +func cycleKey(cycle []string) string { + sorted := append([]string(nil), cycle...) + sort.Strings(sorted) + key := "" + for _, n := range sorted { + key += n + "\x00" + } + return key +} diff --git a/internal/artifact/seal.go b/internal/artifact/seal.go new file mode 100644 index 0000000..4dcd27f --- /dev/null +++ b/internal/artifact/seal.go @@ -0,0 +1,139 @@ +package artifact + +import ( + "strings" + + "github.com/protonspy/spec-claude-code/internal/manifest" + "github.com/protonspy/spec-claude-code/internal/mdscan" +) + +// The seal: tamper-evidence for an approved plan. +// +// What it is and is not, said here so nobody builds a guarantee on it later. It does +// not prevent an edit — `scc plan reseal --force` is one command away and sha256 is +// public. What it does is make an edit made outside scc *visible*: an approved plan +// whose content no longer hashes to its recorded checksum says so, by name, at the +// next command that touches it. The value is evidence and discipline, not security. +// +// It is opt-in by construction. A plan with no `status:` is not sealed and nothing is +// ever checked, which is what makes every plan written before this existed keep +// working untouched. + +// The frontmatter keys the seal lives in. +const ( + KeyStatus = "status" + KeyChecksum = "checksum" +) + +// StatusDraft and StatusApproved are the two phases of a plan's life. Draft is +// authorship, where everything is editable; approved is execution, where the content +// is fixed and only the checklist's state moves. +const ( + StatusDraft = "draft" + StatusApproved = "approved" +) + +// Seal is the checksum recorded for content: sha256 over the file with its own +// `checksum:` line removed — otherwise the hash would have to describe itself — and +// with line endings normalized, so a checkout with CRLF seals identically to one +// with LF and Windows does not report permanent drift. +func Seal(content string) string { + return manifest.Hash(withoutChecksum(content)) +} + +// Approved reports whether this artifact has been sealed for execution. +func (a *Artifact) Approved() bool { return a.Frontmatter[KeyStatus] == StatusApproved } + +// Drift reports whether an approved artifact's content no longer matches its seal. +// A file that is not approved never drifts: there is nothing recorded to differ from. +func (a *Artifact) Drift() (recorded, actual string, drifted bool) { + if !a.Approved() { + return "", "", false + } + recorded = a.Frontmatter[KeyChecksum] + actual = Seal(strings.Join(a.Lines, "\n")) + return recorded, actual, recorded != "" && recorded != actual +} + +// Approve stamps content as approved and seals it. +// +// The order is fixed and load-bearing: the status is written first, then the old +// checksum is dropped, then the hash is taken, then the checksum is written last. +// Taking the hash before the status was set would seal a file that no longer exists. +func Approve(content string) string { return reseal(content, StatusApproved) } + +// Reseal recomputes the seal over content as it now stands, leaving the status +// alone. It is the answer to a legitimate edit made outside the cycle — a merge +// conflict resolved by hand — and it is deliberately a separate, forced command, +// because the same call made automatically would erase the evidence it exists to keep. +func Reseal(content string) string { return reseal(content, "") } + +func reseal(content, status string) string { + lines := strings.Split(strings.ReplaceAll(content, "\r\n", "\n"), "\n") + lines, n := ensureFrontmatter(lines) + if status != "" { + lines, n = setKey(lines, n, KeyStatus, status) + } + lines, n = dropKey(lines, n, KeyChecksum) + sum := manifest.Hash(strings.Join(lines, "\n")) + lines, _ = setKey(lines, n, KeyChecksum, sum) + return strings.Join(lines, "\n") +} + +// withoutChecksum is the canonical form the hash covers. +func withoutChecksum(content string) string { + lines := strings.Split(strings.ReplaceAll(content, "\r\n", "\n"), "\n") + fm, err := mdscan.ParseFrontmatter(strings.Join(lines, "\n")) + if err != nil || !fm.Present { + return strings.Join(lines, "\n") + } + out, _ := dropKey(lines, fm.Lines, KeyChecksum) + return strings.Join(out, "\n") +} + +// EnsureFrontmatter returns the lines with a leading `---` block guaranteed, and how +// many lines that block occupies. Exported for migration, which has to write a plan's +// status into a file that may never have had a frontmatter block at all. +func EnsureFrontmatter(lines []string) ([]string, int) { return ensureFrontmatter(lines) } + +// SetFrontmatterKey writes one key inside a frontmatter block whose extent the caller +// already knows, and returns the block's new length. +func SetFrontmatterKey(lines []string, fmLines int, key, value string) ([]string, int) { + return setKey(lines, fmLines, key, value) +} + +// ensureFrontmatter returns the lines with a leading `---` block guaranteed, and how +// many lines that block occupies. +func ensureFrontmatter(lines []string) ([]string, int) { + fm, err := mdscan.ParseFrontmatter(strings.Join(lines, "\n")) + if err == nil && fm.Present { + return lines, fm.Lines + } + return append([]string{"---", "---", ""}, lines...), 2 +} + +// setKey writes one key inside the frontmatter block, replacing it in place if it is +// there and appending it just above the closing fence if it is not. +func setKey(lines []string, fmLines int, key, value string) ([]string, int) { + for n := 2; n < fmLines; n++ { + k, _, ok := strings.Cut(lines[n-1], ":") + if ok && strings.TrimSpace(k) == key { + lines[n-1] = key + ": " + value + return lines, fmLines + } + } + out := append([]string{}, lines[:fmLines-1]...) + out = append(out, key+": "+value) + return append(out, lines[fmLines-1:]...), fmLines + 1 +} + +func dropKey(lines []string, fmLines int, key string) ([]string, int) { + for n := 2; n < fmLines; n++ { + k, _, ok := strings.Cut(lines[n-1], ":") + if ok && strings.TrimSpace(k) == key { + out := append([]string{}, lines[:n-1]...) + return append(out, lines[n:]...), fmLines - 1 + } + } + return lines, fmLines +} diff --git a/internal/artifact/seal_test.go b/internal/artifact/seal_test.go new file mode 100644 index 0000000..0e4648e --- /dev/null +++ b/internal/artifact/seal_test.go @@ -0,0 +1,74 @@ +package artifact + +import ( + "strings" + "testing" +) + +const unsealed = "---\nautonomy: auto\nci: wait\n---\n\n# Sweep\n\nWhat this is.\n\n## Tasks\n\n- [ ] 1.1 (Unit) Do it\n" + +// The canonicalization, frozen. Two properties, and both are load-bearing. +// +// The seal covers the file *minus its own checksum line* — a hash that included the +// line it is written on could never be satisfied. And it covers the LF-normalized +// text, so the same plan checked out with CRLF on Windows seals identically to the LF +// copy on Linux; without that, half the CI matrix would report permanent drift on a +// file nobody touched. +func TestSealCanonicalizationIsFrozen(t *testing.T) { + // Golden, so a change to the canonicalization is a decision rather than an + // accident: it would make every sealed plan in every workspace report drift. + const golden = "335453e2799c7f47f40d84b3afc9b76f4c3af3181940d7f0f5bb907cbdfe5043" + sum := Seal(unsealed) + if sum != golden { + t.Fatalf("the seal of a known plan changed: %s, was %s — every sealed plan now drifts", sum, golden) + } + + if crlf := Seal(strings.ReplaceAll(unsealed, "\n", "\r\n")); crlf != sum { + t.Errorf("CRLF sealed to %s and LF to %s — Windows would report permanent drift", crlf, sum) + } + + sealed := Approve(unsealed) + if !strings.Contains(sealed, "status: approved") { + t.Fatalf("Approve did not write the status:\n%s", sealed) + } + if !strings.Contains(sealed, "checksum: "+Seal(sealed)) { + t.Errorf("the written checksum does not describe the file it is in:\n%s", sealed) + } + if Seal(sealed) != Seal(strings.Replace(sealed, "checksum: "+Seal(sealed), "checksum: deadbeef", 1)) { + t.Error("the checksum line is inside its own hash; no value could ever satisfy it") + } +} + +func TestApproveIsIdempotentAndResealFollowsTheContent(t *testing.T) { + once := Approve(unsealed) + if twice := Approve(once); twice != once { + t.Errorf("approving twice changed the file:\n%s\n---\n%s", once, twice) + } + + edited := strings.Replace(once, "- [ ] 1.1", "- [x] 1.1", 1) + if Seal(edited) == Seal(once) { + t.Fatal("an edit that changed a checkbox did not change the seal") + } + resealed := Reseal(edited) + if !strings.Contains(resealed, "checksum: "+Seal(resealed)) { + t.Error("Reseal did not record the content it was given") + } + if !strings.Contains(resealed, "status: approved") { + t.Error("Reseal changed the status; it only recomputes the hash") + } +} + +// A file with no frontmatter still has to be sealable — `plan new` writes one, but a +// plan somebody wrote by hand may not have. +func TestApproveAddsAFrontmatterBlockWhenThereIsNone(t *testing.T) { + out := Approve("# Sweep\n\nWhat this is.\n") + if !strings.HasPrefix(out, "---\n") { + t.Fatalf("no frontmatter block was added:\n%s", out) + } + if !strings.Contains(out, "status: approved") || !strings.Contains(out, "checksum: ") { + t.Errorf("the seal was not written:\n%s", out) + } + if !strings.Contains(out, "# Sweep") { + t.Error("the body did not survive") + } +} diff --git a/internal/assets/assets.go b/internal/assets/assets.go index 3856fba..8a73a88 100644 --- a/internal/assets/assets.go +++ b/internal/assets/assets.go @@ -94,7 +94,16 @@ import ( // Also: the wiki's pages move to docs/wiki/pages/, so index.md and changelog.md are // told apart from content by where they sit rather than by their names — which is what // stopped any other .md dropped into wiki/ from becoming a page, and then an orphan. -const Version = "13" +// 14: the plan is a contract rather than a document. Its sections are closed — a +// header and a checklist, and nowhere for prose to grow, which is the only thing that +// ever capped a plan's size; `## Decomposition` becomes `## References`, since the +// parser recognizes a leaf by the citation and never by the heading above it. A task +// gains four flags and no more (`_Depends_`, `_Priority_`, `_Status removed_`, +// `_Reason_`), so "what do I work on next" has a determined answer instead of a +// file-order one. And the rules stop offering to read the plan at all: `map brief` +// once plus `map tasks --next` per task is the whole reading surface, which is what +// gives "never read the plan" the authority to be a rule. +const Version = "14" // The embedded tree. "all:" so nothing is silently dropped for having a name the // default embed pattern skips. diff --git a/internal/assets/templates/artifacts/plan.md b/internal/assets/templates/artifacts/plan.md index bc2be58..c8337e4 100644 --- a/internal/assets/templates/artifacts/plan.md +++ b/internal/assets/templates/artifacts/plan.md @@ -5,44 +5,68 @@ ci: {{.CI}} # {{.Title}} - + Delete the sections you do not need — Paths, References and Out of scope are + optional — and delete this comment. --> ## Why - + + +## Paths + + -## Decomposition +- `path/to/the/thing` - +## References + + - `specs//` — +## Out of scope + + + +- + ## Tasks - + + +- [ ] 1.1 (Unit) - [ ] 1.2 (TDD) + _Depends 1.1_ - - A constraint that is none of those — "cannot merge before the migration window" — - goes on the item's own line, where whoever reads that item will see it. --> +- diff --git a/internal/assets/templates/commands/scc-plan-run.md b/internal/assets/templates/commands/scc-plan-run.md index 83d9eec..99e553c 100644 --- a/internal/assets/templates/commands/scc-plan-run.md +++ b/internal/assets/templates/commands/scc-plan-run.md @@ -7,14 +7,14 @@ Use the `plan-run` skill. Plan, and how to run it: $ARGUMENTS -Map the plan — `scc map ` — 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. +Brief the plan — `scc map brief ` — then `scc map ` for the counts, 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. -**Map it; do not open it.** A real plan runs to tens of kilobytes, and opening one as -the first act of a run puts all of it in context for every turn of a loop that lasts -hours. `map` answers the question you actually have here — the sections, the leaves, -the task counts, what is still open — and `map show` fetches the one part it did not. +**Never open the plan file.** `brief` is its header and `tasks` is its checklist; +there is nothing else in it, and opening one as the first act of a run puts all of it +in context for every turn of a loop that lasts hours. Inside a group, ask `scc map +tasks --next` for the one task to do, and ask again once it is ticked. Then take every answer the line above already gave and ask only for what is left. "Implement the whole plan, one PR at the end, delivered when CI is green" has settled diff --git a/internal/assets/templates/entry.md b/internal/assets/templates/entry.md index 7d3d48c..33cd792 100644 --- a/internal/assets/templates/entry.md +++ b/internal/assets/templates/entry.md @@ -37,9 +37,9 @@ Triggered by what you are about to touch: **Code** — `scc graph query|explore `, or `codegraph_explore` where registered. Read the source when you are about to change it, not to find it. -**Plans and specs** — `scc map` · `map ` · `map tasks --next` · -`map find ""` · `map show
` · `map trace`. An address is a -name — `1.2` `R1.2` `#risks` `risks:2` `specs//` — never a line number. +**Plans and specs** — a plan is a header and a checklist: `map brief ` once, then +`map tasks --next` per task; **never open the plan**. Also `scc map` and `map +show
`. An address is a name, never a line number: `1.2` `#risks`. **Changing one** — `scc patch check 1.2`, plus `task` `add` `append` `fm`. Not an editor: it resolves the address, re-validates, and rolls back an edit that adds a diff --git a/internal/assets/templates/rules/artifacts.md b/internal/assets/templates/rules/artifacts.md index b76368f..b924598 100644 --- a/internal/assets/templates/rules/artifacts.md +++ b/internal/assets/templates/rules/artifacts.md @@ -1,19 +1,16 @@ # Plans and specs — address them, do not read them -A plan is a structured document that happens to be Markdown. Reading one end to end -to answer a question about its structure is the most wasteful thing this workspace can -ask of you: a plan decomposing into thirty specs is tens of kilobytes of prose wrapped -around a dozen checkboxes, and once it is in context you carry it all session. `scc -map` answers those questions without loading the file; `scc patch` changes them -without loading it either. +A plan is a header and a checklist, and `scc` answers every question about it without +loading the file. Reading one end to end is the most wasteful thing this workspace can +ask of you: once it is in context you carry it for the rest of the session. **Never +open a plan** — `brief` is the header, `tasks` is the checklist, no command returns +both, so no question about a plan has the file as its answer. | The question | Ask | |---|---| -| What is here, and how far along? | `scc map` | -| What is the shape of this one? | `scc map ` | -| What do I work on next? | `scc map tasks --next` | -| What is left in group 4? | `scc map tasks --open --group 4` | -| Where is the note about X? | `scc map find ""` | +| What is here, and how far along? | `scc map` · `scc map ` | +| What is this work, and when is it done? | `scc map brief ` — once, per session | +| What do I work on now? | `scc map tasks --next` · `--ready` · `--blocked` | | Show me exactly that piece | `scc map show
` | | What else mentions this requirement? | `scc map trace specs//R1.2` | @@ -21,35 +18,38 @@ without loading it either. a line number**, so it survives an edit above it: ``` -1.2 a task #risks a section, by anchor slug -R1.2 a requirement risks:2 the 2nd paragraph of that section -specs/foo/ a leaf L120-160 an explicit range, the escape hatch +1.2 a task #risks a section, by anchor slug +R1.2 a requirement risks:2 the 2nd paragraph of that section +specs/foo/ a spec reference L120-160 an explicit range, the escape hatch ``` -`find` returns addresses, which is what makes the pair work: search, then `show` only -the hit. A long section with no headings inside it is still navigable — `scc map -blocks` indexes its paragraphs by their opening sentence. Read the file directly only -when the question is about *this exact text*: prose you are about to rewrite. +Read a file directly only when the question is about *this exact text* — prose you are +about to rewrite, which is a spec's design and never a plan. **A plan's shape is +closed**: the title, one to three sentences, then `## Why`, `## Paths`, `## References`, +`## Out of scope`, `## Tasks`, `## Done when`, and any other heading is a finding. +`## References` names the specs this decomposes into and carries no checkbox — that +spec's state lives in that spec. ## Writing **Tick boxes and amend tasks with `scc patch`, not with an editor.** ``` -scc patch check 1.1 1.2 -scc patch task 1.2 --text "…" --method TDD --req R1.1,R1.2 -scc patch add --section tasks --number 1.3 --method Unit --text "…" -scc patch append '#risks' --text - reads stdin, for paragraphs -scc patch fm pr=per-plan +scc patch check 1.1 1.2 · patch fm pr=per-plan +scc patch task 1.2 --text "…" --method TDD --depends 1.1 --priority 2 +scc patch add --group 1 --text "…" --reason "…" +scc patch rm 1.4 --reason "…" ``` Each resolves its address with the parser that read the file, so a miss is an error rather than a write to the wrong place. It then re-runs the validators and **rolls the change back if it introduced a finding** — exit `2`, file untouched. `--dry-run` shows -the lines first; deleting more than a screenful stops and asks for `--force`. - -That is why you need not read a plan to change one line of it. Do not defeat it by -reading "to be safe": the printed before/after is the confirmation. - -A requirement id is scoped to its own spec — `R2.5` in one feature is not `R2.5` in -another — so cite it as `specs//R2.5` when the spec is not obvious. +the lines first; deleting more than a screenful stops and asks for `--force`. That is +why you need not read a plan to change one line of it: do not defeat it by reading "to +be safe", since the printed before/after is the confirmation. + +After `scc plan approve` the work is settled: `add` needs `--group` and `--reason` and +is given its number, `rm` strikes the task out where it stands so the number is never +reused, and rewriting a task or the prose is refused — a task that turned out wrong is +struck out and replaced. An edit made outside `scc` shows up as drift. A requirement id +is scoped to its own spec, so cite it as `specs//R2.5` when that is not obvious. diff --git a/internal/assets/templates/rules/routing.md b/internal/assets/templates/rules/routing.md index e72b6e1..57f4849 100644 --- a/internal/assets/templates/rules/routing.md +++ b/internal/assets/templates/rules/routing.md @@ -41,7 +41,7 @@ checklist item, not three ceremonial files under `specs/`. state. Where an item *references a spec*, the state is derived from that spec and never copied — an item must not do both. Two records of one fact disagree, and the copy is the one that goes stale. -- **A plan's leaves are ordinary specs.** `plans/checkout-revamp.md` references +- **A plan's referenced specs are ordinary specs.** `plans/checkout-revamp.md` names `specs/cart-totals/`; that spec is not nested under the plan and is built by exactly the same rules as one a human asked for directly. - **A plan is work, not knowledge**, so it lives in `plans/` — never in `docs/`, diff --git a/internal/assets/templates/rules/tasks.md b/internal/assets/templates/rules/tasks.md index d28e81d..0237e21 100644 --- a/internal/assets/templates/rules/tasks.md +++ b/internal/assets/templates/rules/tasks.md @@ -5,49 +5,51 @@ The methodology is a property of the task, not of the vehicle that carried it. ``` - [ ] 1.1 (Unit) Parse the manifest file — R1.2, R1.4 -- [ ] 1.2 (TDD) Calculate the pro-rata split across accounts — R2.1 +- [ ] 1.2 (TDD) Calculate the pro-rata split — R2.1 + _Depends 1.1_ ``` -- `- [ ]` / `- [x]` — the checkbox is the state. -- `1.1` — a unique number, `.`. +- `- [ ]` / `- [x]` — the checkbox is the state. Nothing else records it. +- `1.1` — `.`, unique, and never reused once it has been handed out. - `(Unit)` or `(TDD)` — **required, exactly one.** A task with no methodology is a task where nobody decided, which is the failure this practice exists to prevent. `scc` exits `2` on a task missing it. -- The description, in the imperative. -- `— R1.2, R1.4` — the requirements this task satisfies, after an em dash. Required in - a spec's `tasks.md`; that citation is what makes traceability checkable. +- The description, in the imperative, then `— R1.2, R1.4` — the requirements it + satisfies. Required in a spec's `tasks.md`: that citation is traceability. -There is no parallel-dispatch marker. Implementation is sequential — see -[delivery.md](delivery.md) for why, and for the parallelism that *is* supported. +## Flags -Requirements are numbered `R.` and cited by that ID: it greps cleanly, it -never collides with a task's own number, and a reader who has never seen this document -can follow it. +Four, at most one of each, on their own lines under the task — and no others: an italic +line that is not one of these is a finding rather than prose. `_Depends 1.1, 1.2_` all +of them ticked before this can start · `_Priority 2_` a whole number 1 or greater, lower +is more urgent, absent is last · `_Status removed_` struck out, the line and the number +stay and the work does not · `_Reason …_` required with `_Status removed_`, and on a +task added after approval. + +`_Status_` never restates the box — two records of one fact disagree. There is no +`_Blocked_` (derived from `_Depends_`) and no parallel-dispatch marker; implementation +is sequential, see [delivery.md](delivery.md) for why and for the parallelism that +*is* supported. **`scc map tasks --next` is the order** — eligible first, +then priority, then number; `--blocked` names what an impasse waits on. ## How big is a task **A task is the right size when it can be verified on its own.** Not "one file", not "an hour" — verifiable alone, which is what makes the per-task loop in -[verification.md](verification.md) possible at all. - -Granularity is not tidiness; it decides what a failure costs. Agents complete -individual steps far more reliably than whole workflows, and structuring work so a -failure can be retried at the subtask level cut retry cost by ~73% against retrying a -whole plan. Too coarse and a red result tells you only that a feature is broken; too -fine and the checklist becomes bookkeeping about work smaller than recording it. +[verification.md](verification.md) possible at all. Granularity decides what a failure +costs, not tidiness: agents complete individual steps far more reliably than whole +workflows, and retrying at the subtask level cut retry cost by ~73% against retrying a +whole plan. Too coarse and a red result says only that a feature is broken; too fine +and the checklist is bookkeeping. ## Two checklists, one truth -Your harness's todo list tracks the task you are on right now. The file — -`specs//tasks.md`, or the checklist in `plans/.md` — is the durable -record: it survives the session, it gets reviewed, it gets committed, and it is what -`scc` validates. - +Your harness's todo list tracks the task you are on right now. The file — a spec's +`tasks.md`, or the checklist in `plans/.md` — is the durable record: it survives +the session, it gets reviewed, it gets committed, and it is what `scc` validates. **Checking an item off in the session means checking the `- [ ]` box in the file too.** -A session ending with its todo list complete and the file untouched has lost -everything except the code: neither the next session nor the reviewer knows which -tasks were done. - -Check it with `scc patch check 1.2` rather than by editing the file. It -addresses the task by its number, so the file never has to be read to change one box, -and it re-validates afterwards — see [code-search.md](code-search.md). +A session ending with its todo list complete and the file untouched has lost everything +except the code: neither the next session nor the reviewer knows which tasks were done. +Use `scc patch check 1.2` rather than an editor — it addresses the task by +number, so the file is never read to change one box, and it re-validates afterwards. +See [artifacts.md](artifacts.md). diff --git a/internal/assets/templates/rules/verification.md b/internal/assets/templates/rules/verification.md index 3cff135..ecb566d 100644 --- a/internal/assets/templates/rules/verification.md +++ b/internal/assets/templates/rules/verification.md @@ -16,7 +16,7 @@ Per-task feedback has to be fast and attributable. The failure a full run catche and a scoped run misses is breakage *between* tasks, and that is worth looking for once the work is integrated — not N times along the way. -The full suite runs at the end of the spec, or of each of a plan's leaves. See +The full suite runs at the end of the spec, or of each of a plan's groups. See [delivery.md](delivery.md). ## Tests and lint both, because they answer different questions diff --git a/internal/assets/templates/skills/plan-run/SKILL.md b/internal/assets/templates/skills/plan-run/SKILL.md index 5b53ed0..13bc4ca 100644 --- a/internal/assets/templates/skills/plan-run/SKILL.md +++ b/internal/assets/templates/skills/plan-run/SKILL.md @@ -1,6 +1,6 @@ --- name: plan-run -description: Drive a whole plan under plans/ to completion — map the plan, report the groups, take whatever the invocation already decided and ask only for the rest, then implement group by group and deliver either one PR per group or one at the end, settling CI before calling the plan delivered. Resumes from the repository 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. +description: Drive a whole plan under plans/ to completion — brief the plan once, report the groups, take whatever the invocation already decided and ask only for the rest, then implement group by group, asking scc for the next task rather than opening the file, and deliver either one PR per group or one at the end, settling CI before calling the plan delivered. Resumes from the repository 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. @@ -26,28 +26,29 @@ say the PR is open, say where, and say what CI is doing. ## What a group is -A **group** is the smallest part of the plan that can merge on its own. +A **group** is one family of task numbers sharing a major number — `1.1`, `1.2` → group +1 — and it is the smallest part of the plan that can merge on its own. Implementing it +means those tasks, in order. -| 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 | +A task that names a spec under `## References` is that spec: run the ordinary cycle for +it, and tick the task when the spec closes. The reference itself is never ticked — +its state lives in that spec and is read from there. -**The order is the order they are written in.** There is no prose anywhere overriding -it, and you do not go looking for any: a plan that wanted a different order would have -been reordered. `scc map ` gives you that sequence without the file. +**You do not decide the order and you do not look for prose that overrides it.** +`scc map tasks --next` is the order: eligible first, then priority, then number. +A task that names a dependency waits for it; `--blocked` says what an impasse is on. 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 what is still open +## Before the first group — brief, report, then ask what is still open -1. **Map the plan and work out the groups.** `scc map ` gives you the sections, - the leaves, the task counts and the open numbers — the whole shape, without the - prose. Read the plan itself only where the map is not enough; a plan that - decomposes into thirty specs is tens of kilobytes you would otherwise carry for the - rest of the loop. Ask nothing yet: the questions below are only answerable by - someone who can see what they are agreeing to. +1. **Brief the plan.** `scc map brief ` gives you the title, why it exists, the + paths, the references and what "done" means — the header and nothing else. Then + `scc map ` for the group counts. **Never open the plan file**: `brief` reads + the header, `tasks` reads the checklist, and there is nothing else in it. 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. **Take every answer the invocation already gave, and ask only for what is left.** @@ -131,15 +132,16 @@ you stop to deliver. 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. +3. **Implement the group, one `--next` at a time.** `scc map tasks --next + --group N --json` gives you the one task to do; do it, verify it, tick it, ask + again. That loop — one call per task — is why the plan never has to be in context. + A task naming a spec gets the spec cycle. 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 — `scc patch check 1.1 1.2 …`, which addresses - each task by number and re-validates the file. A leaf is never ticked: its state - lives in the spec and is read from there. +5. **Record the group's state in that same PR.** The 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 — `scc patch check 1.1 1.2 …`, which addresses each task by + number and re-validates the file. 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 @@ -152,8 +154,8 @@ you stop to deliver. Branch once from a green `main`, then for each group in order: -1. **Implement the group**, exactly as above — a leaf gets the spec cycle, a task - family its tasks in order. +1. **Implement the group**, exactly as above — one `--next` at a time, each verified + before the next. 2. **Run the suite, the lint, and `scc validate` before moving on.** These stay per group and are not deferred with the rest. They are cheap, and they are what makes a later failure attributable: a break caught at group 3 is group 3's, while the same @@ -185,6 +187,18 @@ Stopping part-way is a legitimate outcome, and continuing past any of these is n 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. +- `--next` reports nothing eligible while open tasks remain. `--blocked` names what + each is waiting on; a cycle or a dependency on a struck-out task is a defect in the + plan and exits `2`. +- The plan reports **drift** — it was edited outside `scc` after being approved. Say + so and stop: something changed the work the developer signed off on, and `git diff` + is the answer, not `plan reseal`. + +**Work that turns up mid-loop is discovery, not an edit.** In an approved plan, +`scc patch add --group N --text "…" --reason "…"` allocates the next number +and records why it was not in the plan; `scc patch rm 2.3 --reason "…"` strikes +a task out where it stands. Neither touches the prose, and neither is a reason to open +the file. Under `autonomy: gated`, stop at every group boundary and wait, having reported what merged. @@ -203,19 +217,19 @@ developer's call once; asking a second time because your context died makes them for your problem. Resuming is a question about state, not about prose, so read it as state: `scc map -` for the shape and `scc map tasks --open` for what is left. Re-reading a +` for the counts and `scc map tasks --next` for what to do. Re-reading a whole plan to find one unticked box is the cost this loop would otherwise pay every -time a session dies. +time a session dies. `brief` again only if you have lost what the plan is for. Which checkout you read that from depends on the shape: - **`pr: per-group` — read `main`.** Pull it and map the plan there; the copy in an - old worktree is stale by construction. A task group whose boxes are ticked on `main` - is done, as is a leaf whose spec's `tasks.md` is fully ticked there — `scc map trace - specs//` answers that in one call, without opening either file. An open PR - means that group is mid-flight; under `merge: manual` that is the expected resting - state. Finish it before starting another — two open groups is the fan-out this loop - exists to avoid. + old worktree is stale by construction. A group whose boxes are ticked on `main` is + done, as is a task naming a spec whose `tasks.md` is fully ticked there — `scc map + trace specs//` answers that in one call, without opening either file. An + open PR means that group is mid-flight; under `merge: manual` that is the expected + resting state. Finish it before starting another — two open groups is the fan-out + this loop exists to avoid. - **`pr: per-plan` — read the plan's branch.** Nothing reaches `main` until the end, so `main` will say no group is done and it will be wrong. Find the branch, read its log for the per-group commits, and map the plan **there**. If every group is committed @@ -243,3 +257,7 @@ run over. Ask what the invocation did not already answer. most tempts you to reach for the file instead. 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. +- **The plan is still a draft.** `scc plan approve ` before the first group: it + validates, fixes the content, and seals it, so a later edit made outside `scc` is + visible rather than silent. A plan that will not approve has findings — report them + and stop, rather than running a plan whose own validator rejects it. diff --git a/internal/assets/templates/skills/prd/SKILL.md b/internal/assets/templates/skills/prd/SKILL.md index 99d3143..3077fc8 100644 --- a/internal/assets/templates/skills/prd/SKILL.md +++ b/internal/assets/templates/skills/prd/SKILL.md @@ -1,6 +1,6 @@ --- name: prd -description: Turn an initiative that spans more than one feature into a plan under plans/ — a decomposition where each leaf is either a task you will do here or a reference to a spec that will be built separately. Use it when someone arrives with a PRD, a roadmap item, an epic, or a rough idea too large for one spec, and the first job is to find out what it actually decomposes into. For a single feature whose shape is already clear, skip this and run `scc spec new`. +description: Turn an initiative that spans more than one feature into a plan under plans/ — a short header, a checklist of tasks, and references to the specs that will be built separately. Use it when someone arrives with a PRD, a roadmap item, an epic, or a rough idea too large for one spec, and the first job is to find out what it actually decomposes into. For a single feature whose shape is already clear, skip this and run `scc spec new`. --- You take an initiative that is too big for one spec and turn it into a plan: one @@ -50,21 +50,24 @@ decomposition, do not ask it.** scc plan new ``` -Then fill it in. The rules the file must hold to: - -- **A leaf is either a spec reference or a task with a checkbox — never both.** Where - an item is a task, the box is its state. Where it references a spec, the state - lives in that spec and is read from there. Two records of one fact disagree, and - the copy is the one that goes stale. `scc validate` reports this as - `plan.item-has-two-records`. -- **A leaf that is a spec is an ordinary spec.** `specs//` is not nested - under the plan and is built by exactly the same rules as one somebody asked for - directly. -- **Each spec-sized leaf is one coherent feature** — something a person could - describe in a sentence and verify on its own. If a leaf needs three sentences and - an "and", it is two leaves. -- **Put them in the order they have to happen.** The order is the list — a plan does - not carry prose restating it, so a leaf that must come first is written first. +Then fill it in. **The plan's sections are closed** — the title, one to three +sentences, then `## Why`, `## Paths`, `## References`, `## Out of scope`, `## Tasks`, +`## Done when`, and nothing else. There is deliberately nowhere for prose to grow: the +file is carried by every session that runs it. The rules it must hold to: + +- **`## Tasks` holds the work; `## References` names the specs.** A task carries a + checkbox and the box is its state. A reference carries none — that spec's state + lives in that spec and is read from there. An item that does both keeps two records + of one fact, and `scc validate` reports it as `plan.item-has-two-records`. +- **A referenced spec is an ordinary spec.** `specs//` is not nested under + the plan and is built by exactly the same rules as one somebody asked for directly. + A task that consumes one is ticked when that spec closes. +- **Each spec-sized piece is one coherent feature** — something a person could + describe in a sentence and verify on its own. If it needs three sentences and an + "and", it is two. +- **Order is `_Depends_`, not position.** A task that cannot start until another is + done says so: `_Depends 1.1_`. `_Priority 2_` breaks a tie. Nothing else records + order, and nothing restates it in prose. - **A plan is work, not knowledge.** It lives in `plans/`, never in `docs/`. Referenced specs must exist, or `scc validate` reports `plan.unknown-spec`. Create @@ -73,11 +76,13 @@ placeholder; a reference to nothing is a broken plan. ## Before you hand it over -- `scc validate` — exit 0, or fix what it names. -- **Read the leaf list back as a whole and ask what is missing.** Migration, - backfill, the switch-over, the thing that has to keep working while this ships, - and how it gets turned off if it goes wrong. Decompositions fail at the seams, not - in the middle of a feature. +- `scc validate` — exit 0, or fix what it names. Then `scc plan approve `, + which fixes the content and seals it, so a later edit outside `scc` is visible + rather than silent. Approving is the hand-over. +- **Read the list back as a whole and ask what is missing.** Migration, backfill, the + switch-over, the thing that has to keep working while this ships, and how it gets + turned off if it goes wrong. Decompositions fail at the seams, not in the middle of + a feature. - **Check the vocabulary.** Every term the plan coins is a term three specs will inherit. If any of them is contested or new, use the `glossary` skill now, while it costs one edit. diff --git a/internal/cli/brief_test.go b/internal/cli/brief_test.go new file mode 100644 index 0000000..1817507 --- /dev/null +++ b/internal/cli/brief_test.go @@ -0,0 +1,205 @@ +package cli + +import ( + "encoding/json" + "strings" + "testing" +) + +// schedulePlan exercises every answer --next has to have: an eligible task, a blocked +// one, a removed one, and a priority that beats file order. +const schedulePlan = `--- +autonomy: auto +ci: wait +--- + +# Sweep + +Replace the legacy path, one group at a time. + +## Why + +The old path cannot be extended without a rewrite. + +## References + +- ` + "`specs/cart-totals/`" + ` — the totals engine + +## Out of scope + +- the payment provider itself + +## Tasks + +- [x] 1.1 (Unit) Lay the foundation +- [ ] 1.2 (TDD) Build on it + _Depends 1.1_ +- [ ] 1.3 (Unit) Wait for 1.2 + _Depends 1.2_ +- [ ] 1.4 (Unit) Dropped after the fact + _Status removed_ + _Reason the upstream API made it unnecessary_ +- [ ] 2.1 (Unit) The urgent one + _Priority 1_ + +## Done when + +- the legacy path is gone +` + +// The guarantee that gives "never read the plan" its authority: brief is the header, +// tasks is the checklist, and neither returns the other. +func TestBriefReturnsTheHeaderAndNoTasks(t *testing.T) { + root := initWorkspace(t) + writePlanFile(t, root, "sweep", schedulePlan) + + stdout, stderr, code := run(t, "map", "brief", "sweep", "--root", root) + if code != ExitOK { + t.Fatalf("brief = %d (%s)", code, stderr) + } + out := stdout + stderr + for _, want := range []string{"Sweep", "Replace the legacy path", "Why", "cart-totals", + "payment provider", "the legacy path is gone"} { + if !strings.Contains(out, want) { + t.Errorf("brief is missing %q:\n%s", want, out) + } + } + for _, unwanted := range []string{"Lay the foundation", "Build on it", "[ ]", "[x]"} { + if strings.Contains(out, unwanted) { + t.Errorf("brief returned a task (%q); brief is the header only:\n%s", unwanted, out) + } + } + if !strings.Contains(out, "1 done") || !strings.Contains(out, "1 removed") { + t.Errorf("brief does not count the checklist:\n%s", out) + } +} + +func TestBriefJSONCarriesTheCounts(t *testing.T) { + root := initWorkspace(t) + writePlanFile(t, root, "sweep", schedulePlan) + stdout, _, code := run(t, "map", "brief", "sweep", "--json", "--root", root) + if code != ExitOK { + t.Fatalf("brief --json = %d", code) + } + var got brief + if err := json.Unmarshal([]byte(stdout), &got); err != nil { + t.Fatalf("brief --json is not JSON: %v\n%s", err, stdout) + } + if got.Tasks.Done != 1 || got.Tasks.Removed != 1 || got.Tasks.Blocked != 1 || got.Tasks.Ready != 2 { + t.Errorf("counts = %+v", got.Tasks) + } + if len(got.Leaves) != 1 { + t.Errorf("leaves = %v, want the one spec reference", got.Leaves) + } + for _, s := range got.Sections { + if s.Slug == "tasks" { + t.Error("brief returned the tasks section") + } + } +} + +// Priority beats position, and a dependency beats both. +func TestNextIsDeterministic(t *testing.T) { + root := initWorkspace(t) + writePlanFile(t, root, "sweep", schedulePlan) + + stdout, _, code := run(t, "map", "tasks", "sweep", "--next", "--json", "--root", root) + if code != ExitOK { + t.Fatalf("--next = %d", code) + } + var got struct { + Task *struct { + Number string `json:"number"` + Depends []string `json:"depends"` + Priority *int `json:"priority"` + Eligible bool `json:"eligible"` + } `json:"task"` + } + if err := json.Unmarshal([]byte(stdout), &got); err != nil { + t.Fatalf("not JSON: %v\n%s", err, stdout) + } + if got.Task == nil || got.Task.Number != "2.1" { + t.Fatalf("--next = %+v, want 2.1 (priority 1 outranks 1.2's file position)", got.Task) + } + if !got.Task.Eligible { + t.Error("--next returned a task it does not consider eligible") + } +} + +func TestReadyBlockedAndDeps(t *testing.T) { + root := initWorkspace(t) + writePlanFile(t, root, "sweep", schedulePlan) + + stdout, _, _ := run(t, "map", "tasks", "sweep", "--ready", "--json", "--root", root) + var ready struct { + Tasks []struct { + Number string `json:"number"` + } `json:"tasks"` + } + if err := json.Unmarshal([]byte(stdout), &ready); err != nil { + t.Fatal(err) + } + if len(ready.Tasks) != 2 || ready.Tasks[0].Number != "2.1" || ready.Tasks[1].Number != "1.2" { + t.Errorf("--ready = %+v, want 2.1 then 1.2", ready.Tasks) + } + + stdout, _, _ = run(t, "map", "tasks", "sweep", "--blocked", "--json", "--root", root) + var blocked struct { + Blocked []blockedRow `json:"blocked"` + } + if err := json.Unmarshal([]byte(stdout), &blocked); err != nil { + t.Fatal(err) + } + if len(blocked.Blocked) != 1 || blocked.Blocked[0].Number != "1.3" { + t.Fatalf("--blocked = %+v, want just 1.3", blocked.Blocked) + } + if len(blocked.Blocked[0].WaitingOn) != 1 || blocked.Blocked[0].WaitingOn[0] != "1.2" { + t.Errorf("an impasse that cannot name its blocker is not actionable: %+v", blocked.Blocked[0]) + } + + stdout, _, _ = run(t, "map", "tasks", "sweep", "--deps", "--root", root) + if !strings.Contains(stdout, "1.3") || !strings.Contains(stdout, "←") { + stdout, stderr, _ := run(t, "map", "tasks", "sweep", "--deps", "--root", root) + t.Errorf("--deps printed no edges:\n%s%s", stdout, stderr) + } +} + +// The two answers a loop must be able to tell apart: finished, and stuck. +func TestNextSaysWhyThereIsNothingToDo(t *testing.T) { + root := initWorkspace(t) + writePlanFile(t, root, "done", plan2(`- [x] 1.1 (Unit) Only task`)) + stdout, _, code := run(t, "map", "tasks", "done", "--next", "--json", "--root", root) + if code != ExitOK { + t.Fatalf("--next on a finished plan = %d", code) + } + if !strings.Contains(stdout, `"done": true`) { + t.Errorf("a finished plan must say so:\n%s", stdout) + } + + writePlanFile(t, root, "stuck", plan2( + "- [ ] 1.1 (Unit) One\n _Depends 1.2_\n- [ ] 1.2 (Unit) Two\n _Depends 1.3_\n- [ ] 1.3 (Unit) Three\n _Depends 1.1_")) + if _, _, code := run(t, "map", "tasks", "stuck", "--next", "--root", root); code != ExitFindings { + t.Errorf("a dependency cycle = %d, want %d — a loop cannot recover from it silently", code, ExitFindings) + } +} + +// A removed task is not work: out of every listing except the one that asks for it. +func TestRemovedTasksAreOutOfTheListings(t *testing.T) { + root := initWorkspace(t) + writePlanFile(t, root, "sweep", schedulePlan) + stdout, stderr, _ := run(t, "map", "tasks", "sweep", "--open", "--root", root) + if strings.Contains(stdout+stderr, "Dropped after the fact") { + t.Error("a removed task appeared in --open") + } + stdout, stderr, _ = run(t, "map", "tasks", "sweep", "--removed", "--root", root) + out := stdout + stderr + if !strings.Contains(out, "Dropped after the fact") || !strings.Contains(out, "upstream API") { + t.Errorf("--removed did not report the struck-out task with its reason:\n%s", out) + } +} + +// plan2 wraps a checklist in the sections the contract requires. +func plan2(tasks string) string { + return "# Sweep\n\nWhat this work is.\n\n## Why\n\nBecause.\n\n## Tasks\n\n" + + tasks + "\n\n## Done when\n\n- it is done\n" +} diff --git a/internal/cli/map.go b/internal/cli/map.go index ba76eb4..92b26c8 100644 --- a/internal/cli/map.go +++ b/internal/cli/map.go @@ -29,6 +29,8 @@ func runMap(args []string) int { return runMapOutline(args[1:]) case "tasks": return runMapTasks(args[1:]) + case "brief": + return runMapBrief(args[1:]) case "show": return runMapShow(args[1:]) case "blocks": @@ -119,6 +121,7 @@ func runMapIndex(args []string) int { fs := flag.NewFlagSet("map index", flag.ContinueOnError) fs.SetOutput(os.Stderr) root := addRoot(fs) + noVerify := addNoVerify(fs) jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { @@ -136,6 +139,9 @@ func runMapIndex(args []string) int { render.Err(err.Error()) return ExitError } + if code := sealGuard(arts, *noVerify); code != ExitOK { + return code + } entries := make([]indexEntry, 0, len(arts)) totalBytes := 0 @@ -186,6 +192,7 @@ func runMapOutline(args []string) int { fs.SetOutput(os.Stderr) root := addRoot(fs) depth := fs.Int("depth", 6, "deepest heading level to print") + noVerify := addNoVerify(fs) jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { @@ -199,9 +206,9 @@ func runMapOutline(args []string) int { if !ok || !requireWorkspace(target) { return ExitError } - arts, ok := loadMany(target, rest) - if !ok { - return ExitError + arts, code := loadVerified(target, rest, *noVerify) + if code != ExitOK { + return code } if *jsonOut { return emitJSON(struct { @@ -241,6 +248,12 @@ func printOutline(a *artifact.Artifact, depth int) { if s.Tasks > 0 { facts = append(facts, fmt.Sprintf("%d/%d tasks", s.Done, s.Tasks)) } + if s.Blocked > 0 { + facts = append(facts, fmt.Sprintf("%d blocked", s.Blocked)) + } + if s.Removed > 0 { + facts = append(facts, fmt.Sprintf("%d removed", s.Removed)) + } if s.Leaves > 0 { facts = append(facts, fmt.Sprintf("%d leaves", s.Leaves)) } @@ -250,17 +263,20 @@ func printOutline(a *artifact.Artifact, depth int) { facts = append(facts, fmt.Sprintf("L%d-%d", s.Line, s.End)) render.Info(fmt.Sprintf("%s%-32s %s", indent, s.Title, strings.Join(facts, " · "))) } - if done, total := a.Done(); total > 0 { - render.Info(fmt.Sprintf("tasks: %d/%d done · open: %s", done, total, openNumbers(a))) + c := a.Counts() + if c.Total > 0 || c.Removed > 0 { + render.Info(fmt.Sprintf("tasks: %d/%d done · %d ready · %d blocked · %d removed · ready: %s", + c.Done, c.Total, c.Ready, c.Blocked, c.Removed, openNumbers(a))) } } +// openNumbers is what could be started now, in the order --next would take it. It +// used to be every open task in file order, which answered a question one dependency +// makes wrong. func openNumbers(a *artifact.Artifact) string { var open []string - for _, t := range a.Tasks { - if !t.Checked { - open = append(open, t.Number) - } + for _, t := range a.Ready() { + open = append(open, t.Number) } if len(open) == 0 { return "none" @@ -271,16 +287,25 @@ func openNumbers(a *artifact.Artifact) string { return strings.Join(open, " ") } -func frontmatterLine(a *artifact.Artifact) string { +func frontmatterLine(a *artifact.Artifact) string { return frontmatterOf(a.Frontmatter) } + +func frontmatterOf(fm map[string]string) string { var parts []string - for _, k := range []string{"autonomy", "ci", "pr", "worktree", "merge"} { - if v, ok := a.Frontmatter[k]; ok { + for _, k := range []string{"status", "autonomy", "ci", "lang", "pr", "worktree", "merge"} { + if v, ok := fm[k]; ok { parts = append(parts, k+":"+v) } } return strings.Join(parts, " · ") } +// taskRow is one task with the file it came from — the shape every listing here +// emits, so a caller that has parsed one has parsed all of them. +type taskRow struct { + Path string `json:"path"` + artifact.Task +} + func runMapTasks(args []string) int { fs := flag.NewFlagSet("map tasks", flag.ContinueOnError) fs.SetOutput(os.Stderr) @@ -290,8 +315,13 @@ func runMapTasks(args []string) int { group := fs.String("group", "", "only tasks in this numbering `group` (1, or 1.2)") req := fs.String("req", "", "only tasks citing this `requirement` (R1.2)") method := fs.String("method", "", "only tasks annotated `Unit` or TDD") - next := fs.Bool("next", false, "only the first open task — what a loop asks for, and it implies --open") + next := fs.Bool("next", false, "the one task to work on now: eligible, most urgent, lowest number") + ready := fs.Bool("ready", false, "every eligible task, in the order --next would take them") + blocked := fs.Bool("blocked", false, "open tasks that are not eligible, each naming what it waits on") + deps := fs.Bool("deps", false, "the dependency edges alone, one line per task that has any") + removed := fs.Bool("removed", false, "the tasks discovery struck out, with their reasons") width := fs.Int("width", 96, "clip each description to this many `runes`") + noVerify := addNoVerify(fs) jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { @@ -301,31 +331,39 @@ func runMapTasks(args []string) int { render.Err("--open and --done ask for opposite things") return ExitError } - // --next means the next task to work on, which is the first one nobody has done. - // Without this it would mean "the first task", and answer a question nobody asked. - if *next { - if *done { - render.Err("--next asks for the first open task; --done asks for finished ones") - return ExitError - } - *open = true + if *next && *done { + render.Err("--next asks for the next task to do; --done asks for finished ones") + return ExitError + } + if picked := countTrue(*next, *ready, *blocked, *deps); picked > 1 { + render.Err("--next, --ready, --blocked and --deps are four views of the schedule; ask for one") + return ExitError } target, ok := resolveRoot(*root) if !ok || !requireWorkspace(target) { return ExitError } - arts, ok := loadMany(target, rest) - if !ok { - return ExitError + arts, code := loadVerified(target, rest, *noVerify) + if code != ExitOK { + return code } - type row struct { - Path string `json:"path"` - artifact.Task + switch { + case *next: + return runMapNext(arts, *jsonOut, *width) + case *ready, *blocked, *deps: + return runMapSchedule(arts, scheduleView{ready: *ready, blocked: *blocked, deps: *deps}, + *jsonOut, *width) } - rows := []row{} + + rows := []taskRow{} for _, a := range arts { for _, t := range a.Tasks { + // A removed task is not work, so it is out of every listing unless it is the + // listing asked for. It stays in the file for its number and its reason. + if t.Removed() != *removed { + continue + } switch { case *open && t.Checked, *done && !t.Checked: continue @@ -337,20 +375,14 @@ func runMapTasks(args []string) int { if *req != "" && !cites(t, *req) { continue } - rows = append(rows, row{a.Path, t}) - if *next { - break - } - } - if *next && len(rows) > 0 { - break + rows = append(rows, taskRow{a.Path, t}) } } if *jsonOut { return emitJSON(struct { - Tasks []row `json:"tasks"` - Count int `json:"count"` + Tasks []taskRow `json:"tasks"` + Count int `json:"count"` }{rows, len(rows)}) } if len(rows) == 0 { @@ -358,20 +390,235 @@ func runMapTasks(args []string) int { return ExitOK } for _, r := range rows { - box := "[ ]" - if r.Checked { - box = render.Green("[x]") + render.Info(taskLine(r, *width)) + } + return ExitOK +} + +// taskLine is one task as a line: the box, the number, how it gets built, what it +// says, and the flags that decide when it may start. +func taskLine(r taskRow, width int) string { + box := "[ ]" + switch { + case r.Removed(): + box = render.Red("[-]") + case r.Checked: + box = render.Green("[x]") + } + var tail []string + if len(r.Requirements) > 0 { + tail = append(tail, "— "+strings.Join(r.Requirements, ", ")) + } + if r.Priority != nil { + tail = append(tail, fmt.Sprintf("P%d", *r.Priority)) + } + if len(r.Depends) > 0 { + tail = append(tail, "after "+strings.Join(r.Depends, ", ")) + } + if r.Reason != "" { + tail = append(tail, "removed: "+r.Reason) + } + suffix := "" + if len(tail) > 0 { + suffix = " " + strings.Join(tail, " · ") + } + return fmt.Sprintf("%s %-6s %-6s %s%s %s", box, r.Number, r.Methodology, + r.Summary(width), suffix, render.Cyan(fmt.Sprintf("%s:%d", r.Path, r.Line))) +} + +// blockedRow is one open task that cannot start, and what it is waiting for. An +// impasse that could not name its blocker would stop a loop with nothing to act on. +type blockedRow struct { + Path string `json:"path"` + Number string `json:"id"` + Text string `json:"text"` + WaitingOn []string `json:"waiting_on"` + Line int `json:"line"` +} + +// runMapNext answers "what do I do now" — one task, or a reason there is none. +// +// The reason is the part that had to be designed rather than fallen into. A loop +// that got an empty answer could not tell "the plan is finished" from "everything +// left is waiting on something", and those call for opposite next moves. +func runMapNext(arts []*artifact.Artifact, jsonOut bool, width int) int { + if code := reportUnrunnable(arts); code != ExitOK { + return code + } + var blocked []blockedRow + for _, a := range arts { + if t, ok := a.Next(); ok { + row := taskRow{a.Path, t} + if jsonOut { + return emitJSON(struct { + Task *taskRow `json:"task"` + }{&row}) + } + render.Info(taskLine(row, width)) + return ExitOK + } + blocked = append(blocked, blockedRowsFor(a)...) + } + if jsonOut { + return emitJSON(struct { + Task *taskRow `json:"task"` + Done bool `json:"done"` + Blocked []blockedRow `json:"blocked,omitempty"` + }{nil, len(blocked) == 0, blocked}) + } + if len(blocked) == 0 { + render.OK("nothing open — every task is done or removed") + return ExitOK + } + render.Warn("nothing is eligible: every open task is waiting on another one") + for _, b := range blocked { + render.Info(fmt.Sprintf("%-6s waits on %-16s %s", b.Number, strings.Join(b.WaitingOn, ", "), + render.Cyan(fmt.Sprintf("%s:%d", b.Path, b.Line)))) + } + return ExitOK +} + +type scheduleView struct{ ready, blocked, deps bool } + +func runMapSchedule(arts []*artifact.Artifact, view scheduleView, jsonOut bool, width int) int { + if code := reportUnrunnable(arts); code != ExitOK { + return code + } + switch { + case view.blocked: + rows := []blockedRow{} + for _, a := range arts { + rows = append(rows, blockedRowsFor(a)...) + } + if jsonOut { + return emitJSON(struct { + Blocked []blockedRow `json:"blocked"` + Count int `json:"count"` + }{rows, len(rows)}) + } + if len(rows) == 0 { + render.Info("nothing is blocked") + return ExitOK + } + for _, b := range rows { + render.Info(fmt.Sprintf("%-6s waits on %-16s %s %s", b.Number, strings.Join(b.WaitingOn, ", "), + clipRunes(b.Text, width), render.Cyan(fmt.Sprintf("%s:%d", b.Path, b.Line)))) + } + return ExitOK + + case view.deps: + type edge struct { + Path string `json:"path"` + Number string `json:"id"` + Depends []string `json:"depends"` + } + edges := []edge{} + for _, a := range arts { + for _, t := range a.Tasks { + if len(t.Depends) > 0 { + edges = append(edges, edge{a.Path, t.Number, t.Depends}) + } + } } - cite := "" - if len(r.Requirements) > 0 { - cite = " — " + strings.Join(r.Requirements, ", ") + if jsonOut { + return emitJSON(struct { + Deps []edge `json:"deps"` + Count int `json:"count"` + }{edges, len(edges)}) } - render.Info(fmt.Sprintf("%s %-6s %-6s %s%s %s", box, r.Number, r.Methodology, - r.Summary(*width), cite, render.Cyan(fmt.Sprintf("%s:%d", r.Path, r.Line)))) + if len(edges) == 0 { + render.Info("no task declares a dependency — the order is the list") + return ExitOK + } + for _, e := range edges { + render.Info(fmt.Sprintf("%-6s ← %s", e.Number, strings.Join(e.Depends, ", "))) + } + return ExitOK + } + + rows := []taskRow{} + for _, a := range arts { + for _, t := range a.Ready() { + rows = append(rows, taskRow{a.Path, t}) + } + } + if jsonOut { + return emitJSON(struct { + Tasks []taskRow `json:"tasks"` + Count int `json:"count"` + }{rows, len(rows)}) + } + if len(rows) == 0 { + render.Info("nothing is eligible") + return ExitOK + } + for _, r := range rows { + render.Info(taskLine(r, width)) } return ExitOK } +func blockedRowsFor(a *artifact.Artifact) []blockedRow { + var out []blockedRow + for _, t := range a.BlockedTasks() { + out = append(out, blockedRow{a.Path, t.Number, t.Summary(72), a.WaitingOn(t), t.Line}) + } + return out +} + +// reportUnrunnable stops the schedule commands on the two defects that make the +// schedule meaningless rather than merely wrong: a cycle, and a dependency on a task +// that will never be ticked. +// +// It is exit 2 and not a silent omission because the alternative is a loop that +// reports "nothing to do" while open work remains, and no way for its operator to +// see why. The rules are the same ones `scc validate` reports, so fixing the finding +// fixes both. +func reportUnrunnable(arts []*artifact.Artifact) int { + var problems []string + for _, a := range arts { + for _, cycle := range a.DependencyCycles() { + problems = append(problems, fmt.Sprintf("%s task.dependency-cycle %s", + a.Path, strings.Join(append(append([]string{}, cycle...), cycle[0]), " → "))) + } + for _, t := range a.Tasks { + if t.Checked || t.Removed() { + continue + } + for _, d := range t.Depends { + dep, ok := a.Task(d) + switch { + case !ok: + problems = append(problems, fmt.Sprintf("%s task.unknown-dependency %s depends on %s, which is not in this file", + a.Path, t.Number, d)) + case dep.Removed(): + problems = append(problems, fmt.Sprintf("%s task.depends-on-removed %s depends on %s, which was removed", + a.Path, t.Number, d)) + } + } + } + } + if len(problems) == 0 { + return ExitOK + } + render.Err("the schedule cannot be computed: a dependency will never be satisfied") + for _, p := range problems { + render.Detail(" " + p) + } + render.Detail(fmt.Sprintf(" fix them with `%s patch`, then ask again", prog())) + return ExitFindings +} + +func countTrue(flags ...bool) int { + n := 0 + for _, f := range flags { + if f { + n++ + } + } + return n +} + func cites(t artifact.Task, id string) bool { for _, r := range t.Requirements { if strings.EqualFold(r, id) { @@ -381,11 +628,162 @@ func cites(t artifact.Task, id string) bool { return false } +// briefSection is one section of the header, in full. +type briefSection struct { + Slug string `json:"slug"` + Title string `json:"title"` + Text string `json:"text"` +} + +// brief is the answer to "what is this work, and when is it done" — the whole header +// and none of the checklist. +type brief struct { + Path string `json:"path"` + Kind string `json:"kind"` + Title string `json:"title"` + Frontmatter map[string]string `json:"frontmatter,omitempty"` + Description string `json:"description,omitempty"` + Sections []briefSection `json:"sections,omitempty"` + Tasks artifact.Counts `json:"tasks"` + Requirements int `json:"requirements,omitempty"` + Leaves []string `json:"leaves,omitempty"` +} + +// runMapBrief prints an artifact's header without its items. +// +// It is the other half of the guarantee that makes "never read the plan" a rule +// rather than a wish: `brief` reads the header, `tasks` reads the checklist, and no +// command returns both. A session pays for this once and then asks `--next` per task, +// which is what turns a per-reread cost into a per-run one. +// +// A section that carries items is counted rather than printed, which is the whole +// rule — it is what keeps `brief` on a spec's requirements.md from being the file. +func runMapBrief(args []string) int { + fs := flag.NewFlagSet("map brief", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + root := addRoot(fs) + noVerify := addNoVerify(fs) + jsonOut := addJSON(fs) + rest, err := parseFlags(fs, args) + if err != nil { + return ExitError + } + if len(rest) == 0 { + render.Err("map brief needs an artifact: a path, a plan name, or a feature name") + return ExitError + } + target, ok := resolveRoot(*root) + if !ok || !requireWorkspace(target) { + return ExitError + } + arts, code := loadVerified(target, rest, *noVerify) + if code != ExitOK { + return code + } + + briefs := make([]brief, 0, len(arts)) + for _, a := range arts { + briefs = append(briefs, briefOf(a)) + } + if *jsonOut { + if len(briefs) == 1 { + return emitJSON(briefs[0]) + } + return emitJSON(struct { + Artifacts []brief `json:"artifacts"` + }{briefs}) + } + for i, b := range briefs { + if i > 0 { + render.Info("") + } + printBrief(b) + } + return ExitOK +} + +func briefOf(a *artifact.Artifact) brief { + b := brief{ + Path: a.Path, Kind: string(a.Kind), Title: a.Title, + Frontmatter: a.Frontmatter, Tasks: a.Counts(), Requirements: len(a.Requirements), + } + first := a.LineCount() + 1 + for _, s := range a.Sections { + if s.Level > 1 { + first = s.Line + break + } + } + start := 1 + for _, s := range a.Sections { + if s.Level == 1 { + start = s.Line + 1 + break + } + } + b.Description = a.Prose(start, first-1) + + for _, s := range a.Sections { + if s.Level != 2 || s.Tasks > 0 || s.Removed > 0 { + continue + } + if holdsRequirements(a, s) { + continue + } + b.Sections = append(b.Sections, briefSection{ + Slug: s.Slug, Title: s.Title, Text: a.Prose(s.Line+1, s.End), + }) + } + for _, l := range a.Leaves { + b.Leaves = append(b.Leaves, l.Ref) + } + return b +} + +func holdsRequirements(a *artifact.Artifact, s artifact.Section) bool { + for _, r := range a.Requirements { + if r.Line >= s.Line && r.Line <= s.End { + return true + } + } + return false +} + +func printBrief(b brief) { + head := render.Bold(b.Title) + if fm := frontmatterOf(b.Frontmatter); fm != "" { + head += " " + fm + } + render.Info(head) + render.Info(fmt.Sprintf("%s · %s", b.Path, b.Kind)) + if b.Description != "" { + fmt.Println() + fmt.Println(b.Description) + } + for _, s := range b.Sections { + if s.Text == "" { + continue // an empty section says nothing; printing its heading says less + } + fmt.Println() + render.Info(render.Bold("## " + s.Title)) + fmt.Println(s.Text) + } + fmt.Println() + if c := b.Tasks; c.Total > 0 || c.Removed > 0 { + render.Info(fmt.Sprintf("tasks: %d done · %d ready · %d blocked · %d removed — `%s map tasks --next`", + c.Done, c.Ready, c.Blocked, c.Removed, prog())) + } + if b.Requirements > 0 { + render.Info(fmt.Sprintf("requirements: %d", b.Requirements)) + } +} + func runMapShow(args []string) int { fs := flag.NewFlagSet("map show", flag.ContinueOnError) fs.SetOutput(os.Stderr) root := addRoot(fs) numbers := fs.Bool("numbers", false, "prefix each line with its line number") + noVerify := addNoVerify(fs) jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { @@ -403,6 +801,9 @@ func runMapShow(args []string) int { if !ok { return ExitError } + if code := sealGuard([]*artifact.Artifact{a}, *noVerify); code != ExitOK { + return code + } type piece struct { artifact.Target @@ -717,20 +1118,25 @@ func sizeOf(b int) string { func mapUsage() { fmt.Fprintf(os.Stderr, `Usage: %s map every plan and spec, one line each - %s map the shape of one file: sections, counts, open tasks - %s map tasks […] [filters] --open --done --next --group N --req R1.2 --method TDD + %s map the shape of one file: sections, counts, what is ready + %s map brief the header — why, paths, references, done when. No tasks. + %s map tasks […] [filters] --next --ready --blocked --deps --open --done + --group N --req R1.2 --method TDD --removed %s map show
… exactly that piece, and nothing else %s map blocks [
] the lead sentence of every paragraph, with its address - %s map find [--in ] ranked search over addressable units %s map trace /> everything in the workspace that mentions it An is a path (plans/x.md), a plan name, or a feature name. +A plan is a header and a checklist. "brief" reads the header, "tasks" reads the +checklist, and nothing returns both — one "brief" per session plus one "--next" per +task is the whole of it, so the file never has to be opened. + Addresses, none of which is a line number — which is why one survives an edit above it: 1.2 a task, by its number R1.2 a requirement, by its id - specs/foo/ a decomposition leaf + specs/foo/ a spec this plan references #risks a section, by anchor slug (or by its title as written) risks:2 the 2nd paragraph of that section L120-160 an explicit line range, the escape hatch diff --git a/internal/cli/migrate.go b/internal/cli/migrate.go new file mode 100644 index 0000000..acbc354 --- /dev/null +++ b/internal/cli/migrate.go @@ -0,0 +1,289 @@ +package cli + +import ( + "flag" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/protonspy/spec-claude-code/internal/artifact" + "github.com/protonspy/spec-claude-code/internal/finding" + "github.com/protonspy/spec-claude-code/internal/paths" + "github.com/protonspy/spec-claude-code/internal/render" + "github.com/protonspy/spec-claude-code/internal/validate" + "github.com/protonspy/spec-claude-code/internal/workspace" +) + +// planSectionSlugs and requiredPlanSections read the contract from the validator that +// enforces it. Restating it here would let migration produce a file the validator it +// was written for then rejects. +func planSectionSlugs() []string { + var out []string + for _, s := range validate.PlanSections() { + out = append(out, s.Slug) + } + return out +} + +func requiredPlanSections() []string { + var out []string + for _, s := range validate.PlanSections() { + if s.Required { + out = append(out, s.Title) + } + } + return out +} + +func planFindingsFor(root, name string) ([]finding.Finding, error) { + set, err := validate.Plan(root, name) + if err != nil { + return nil, err + } + return set.Sorted(), nil +} + +// Moving a plan onto the v2 contract. +// +// Mechanical where it can be, and never destructive where it cannot. Two rules +// decide everything below: +// +// - **Nothing is deleted.** A section the contract does not have is moved to +// plans/archive/-notes.md, not dropped. The plan scanner reads plans/ with +// ReadDir and skips directories, so the archived file is neither a phantom plan +// nor something a validator reports on. +// +// - **No placeholder passes the validator.** A missing required section is created +// empty and the finding is left to appear. A `` that satisfied the +// check would be a plan that lies about being complete, which is worse than a +// plan that says it is not. +// +// Migrating on read was considered and rejected: rewriting a user's file as a side +// effect of `scc map` violates "never author what the user owns" and produces a diff +// nobody asked for. + +// archiveSeg is where the sections that are not in the contract go. +const archiveSeg = "archive" + +// renames is the one heading whose content is already in the contract under another +// name. `## Decomposition` held list items citing `specs//`, and the parser +// recognizes those by the citation rather than by the heading above them — so this is +// a rename and not a move, and not one line of content changes. +var renames = map[string]string{"decomposition": "References"} + +type migration struct { + Plan string `json:"plan"` + Path string `json:"path"` + Renamed []string `json:"renamed,omitempty"` + Archived []string `json:"archived,omitempty"` + Created []string `json:"created,omitempty"` + Rewrote int `json:"tasks_rewritten"` + Archive string `json:"archive,omitempty"` + Status string `json:"status"` + Findings []string `json:"findings,omitempty"` + Written bool `json:"written"` +} + +func runPlanMigrate(args []string) int { + fs := flag.NewFlagSet("plan migrate", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + root := addRoot(fs) + dry := fs.Bool("dry-run", false, "report what would change and write nothing") + jsonOut := addJSON(fs) + rest, err := parseFlags(fs, args) + if err != nil { + return ExitError + } + name, ok := artifactName(rest, "plan") + if !ok { + return ExitError + } + target, ok := resolveRoot(*root) + if !ok || !requireWorkspace(target) { + return ExitError + } + path := paths.Plan(target, name) + if !isFile(path) { + render.Err(fmt.Sprintf("no plan %q under %s", name, paths.PlansSeg)) + return ExitError + } + a, err := artifact.Load(target, path) + if err != nil { + render.Err(err.Error()) + return ExitError + } + if a.Approved() { + render.Err(fmt.Sprintf("%s is approved; migrating rewrites its content", a.Path)) + render.Detail(" it is already on a contract somebody signed off — there is nothing to migrate") + return ExitError + } + + plan, archived, m := migrate(a, name) + m.Path = a.Path + if *dry { + m.Written = false + return reportMigration(m, *jsonOut, true) + } + + if archived != "" { + dir := filepath.Join(paths.Plans(target), archiveSeg) + if err := os.MkdirAll(dir, 0o755); err != nil { + render.Err(err.Error()) + return ExitError + } + file := filepath.Join(dir, name+"-notes.md") + if err := workspace.AtomicWrite(file, []byte(archived), 0o644); err != nil { + render.Err(err.Error()) + return ExitError + } + m.Archive = relPath(target, file) + } + if err := workspace.AtomicWrite(path, []byte(plan), 0o644); err != nil { + render.Err(err.Error()) + return ExitError + } + m.Written = true + + // The findings that are left are the point of the report: the sections migration + // created empty are exactly the decisions a person still has to make. + if set, err := planFindingsFor(target, name); err == nil { + for _, f := range set { + m.Findings = append(m.Findings, fmt.Sprintf("%d %s %s", f.Line, f.Rule, f.Message)) + } + } + return reportMigration(m, *jsonOut, false) +} + +// migrate returns the rewritten plan, the archive file (empty when there is nothing +// to archive), and what it did. +func migrate(a *artifact.Artifact, name string) (plan, archive string, m migration) { + m.Plan = name + m.Status = artifact.StatusDraft + + known := map[string]bool{} + for _, s := range planSectionSlugs() { + known[s] = true + } + + // Which lines belong to a section the contract does not have. Whole subtrees, so + // a `### Detail` under `## Notes` travels with it. + drop := map[int]bool{} + var archived []artifact.Section + for _, s := range a.Sections { + if s.Level != 2 || known[s.Slug] { + continue + } + if _, renamed := renames[s.Slug]; renamed { + continue + } + archived = append(archived, s) + for n := s.Line; n <= s.End; n++ { + drop[n] = true + } + } + + var body []string + for i, line := range a.Lines { + n := i + 1 + if drop[n] { + continue + } + if to, ok := renameAt(a, n); ok { + m.Renamed = append(m.Renamed, to) + body = append(body, "## "+to) + continue + } + body = append(body, line) + } + for _, s := range archived { + m.Archived = append(m.Archived, s.Title) + } + + out := strings.Join(body, "\n") + out = withStatus(out, artifact.StatusDraft) + out, created := ensureSections(out) + m.Created = created + m.Rewrote = len(a.Tasks) + + if len(archived) == 0 { + return withTrailingNewline(out), "", m + } + var buf strings.Builder + fmt.Fprintf(&buf, "# %s — sections moved out of the plan\n\n", a.Title) + buf.WriteString("These were in `" + a.Path + "` before it moved to the v2 contract, where a plan is a\n") + buf.WriteString("header and a checklist. Nothing here was changed; it was moved so it could be read,\n") + buf.WriteString("split into ADRs under `" + paths.DocsSeg + "/" + paths.ADRSeg + "/`, or deleted deliberately.\n") + for _, s := range archived { + buf.WriteString("\n") + buf.WriteString(a.Text(s.Line, s.End)) + buf.WriteString("\n") + } + return withTrailingNewline(out), withTrailingNewline(buf.String()), m +} + +func renameAt(a *artifact.Artifact, line int) (string, bool) { + for _, s := range a.Sections { + if s.Line == line && s.Level == 2 { + if to, ok := renames[s.Slug]; ok { + return to, true + } + } + } + return "", false +} + +// ensureSections appends the required headings the plan does not have, empty. +func ensureSections(content string) (string, []string) { + var created []string + lower := strings.ToLower(content) + for _, s := range requiredPlanSections() { + if strings.Contains(lower, "\n## "+strings.ToLower(s)+"\n") || + strings.HasPrefix(lower, "## "+strings.ToLower(s)+"\n") { + continue + } + created = append(created, s) + content = strings.TrimRight(content, "\n") + "\n\n## " + s + "\n" + } + return content, created +} + +func withStatus(content, status string) string { + lines := strings.Split(strings.ReplaceAll(content, "\r\n", "\n"), "\n") + lines, n := artifact.EnsureFrontmatter(lines) + lines, _ = artifact.SetFrontmatterKey(lines, n, artifact.KeyStatus, status) + return strings.Join(lines, "\n") +} + +func reportMigration(m migration, jsonOut, dry bool) int { + if jsonOut { + return emitJSON(m) + } + for _, s := range m.Renamed { + render.Info("renamed → ## " + s) + } + for _, s := range m.Archived { + render.Info("archived ## " + s) + } + for _, s := range m.Created { + render.Info("created ## " + s + " (empty — fill it in)") + } + if m.Archive != "" { + render.Info(" " + m.Archive) + } + if dry { + render.Info("--dry-run: nothing written") + return ExitOK + } + render.OK(m.Path + " — migrated, status: " + m.Status) + if len(m.Findings) > 0 { + render.Warn(fmt.Sprintf("%d finding(s) remain — they are what is left for a person to decide", len(m.Findings))) + for _, f := range m.Findings { + render.Detail(" " + f) + } + render.Detail(fmt.Sprintf(" fix them, then `%s plan approve %s`", prog(), m.Plan)) + return ExitFindings + } + render.Info(fmt.Sprintf("`%s plan approve %s` seals it", prog(), m.Plan)) + return ExitOK +} diff --git a/internal/cli/migrate_test.go b/internal/cli/migrate_test.go new file mode 100644 index 0000000..5dcf392 --- /dev/null +++ b/internal/cli/migrate_test.go @@ -0,0 +1,116 @@ +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// v1Plan is shaped like the plan this contract was written against: a decomposition, +// a checklist, and a Notes section that grew into half the file. +const v1Plan = `--- +autonomy: auto +ci: wait +--- + +# Sweep + +## Why + +The old path cannot be extended. + +## Decomposition + +- ` + "`specs/cart-totals/`" + ` — the totals engine + +## Tasks + +- [ ] 1.1 (Unit) Lay the foundation +- [x] 1.2 (TDD) Build on it + +## Notes + +**Order matters.** cart-totals first, because everything writes through it. + +**The free path wins.** The expensive command knows about the cheap one. +` + +func TestMigrateMovesAPlanOntoTheContract(t *testing.T) { + root := initWorkspace(t) + if err := os.MkdirAll(filepath.Join(root, "specs", "cart-totals"), 0o755); err != nil { + t.Fatal(err) + } + path := writePlanFile(t, root, "sweep", v1Plan) + + stdout, stderr, code := run(t, "plan", "migrate", "sweep", "--root", root) + // The plan is missing `## Done when` and a description, so migration creates the + // section empty and lets the findings appear. A placeholder that satisfied the + // validator would be a plan that lies about being complete. + if code != ExitFindings { + t.Fatalf("migrate = %d, want the remaining findings reported (%s%s)", code, stdout, stderr) + } + + body := readFile(t, path) + if strings.Contains(body, "## Notes") { + t.Error("Notes survived the migration") + } + if strings.Contains(body, "## Decomposition") || !strings.Contains(body, "## References") { + t.Errorf("Decomposition was not renamed:\n%s", body) + } + if !strings.Contains(body, "`specs/cart-totals/`") { + t.Error("the spec reference did not survive the rename") + } + if !strings.Contains(body, "## Done when") { + t.Error("the missing required section was not created") + } + if !strings.Contains(body, "status: draft") { + t.Error("migration must never approve; approving is a human act") + } + if !strings.Contains(body, "- [x] 1.2") { + t.Error("migration lost a task's state") + } + + // Nothing is deleted. plans/archive/ is safe because the plan scanner reads + // plans/ with ReadDir and skips directories. + archived := readFile(t, filepath.Join(root, "plans", "archive", "sweep-notes.md")) + for _, want := range []string{"Order matters", "The free path wins"} { + if !strings.Contains(archived, want) { + t.Errorf("the archive lost %q", want) + } + } + if _, _, code := run(t, "plan", "list", "--root", root); code != ExitOK { + t.Error("the archive directory broke the plan listing") + } + if _, _, code := run(t, "validate", "--root", root); code == ExitError { + t.Error("the archive directory broke validation") + } +} + +func TestMigrateDryRunWritesNothing(t *testing.T) { + root := initWorkspace(t) + path := writePlanFile(t, root, "sweep", v1Plan) + before := readFile(t, path) + if _, _, code := run(t, "plan", "migrate", "sweep", "--dry-run", "--root", root); code != ExitOK { + t.Fatal("dry run failed") + } + if readFile(t, path) != before { + t.Error("--dry-run wrote to the file") + } + if _, err := os.Stat(filepath.Join(root, "plans", "archive")); err == nil { + t.Error("--dry-run created the archive") + } +} + +// An approved plan is already on a contract somebody signed off, so there is nothing +// to migrate and rewriting it would be the one thing approval exists to prevent. +func TestMigrateRefusesAnApprovedPlan(t *testing.T) { + root := initWorkspace(t) + writePlanFile(t, root, "sweep", v2Plan) + if _, _, code := run(t, "plan", "approve", "sweep", "--root", root); code != ExitOK { + t.Fatal("approve failed") + } + if _, _, code := run(t, "plan", "migrate", "sweep", "--root", root); code != ExitError { + t.Error("migrating an approved plan should be refused") + } +} diff --git a/internal/cli/patch.go b/internal/cli/patch.go index 29abcab..0892f78 100644 --- a/internal/cli/patch.go +++ b/internal/cli/patch.go @@ -56,10 +56,11 @@ func runPatch(args []string) int { // patchFlags is the set every patch subcommand shares, so the safety valves are // spelled and behave identically across the surface. type patchFlags struct { - root *string - dry *bool - force *bool - jsonOut *bool + root *string + dry *bool + force *bool + noVerify *bool + jsonOut *bool } func addPatchFlags(fs *flag.FlagSet) patchFlags { @@ -68,7 +69,42 @@ func addPatchFlags(fs *flag.FlagSet) patchFlags { dry: fs.Bool("dry-run", false, "show the change and write nothing"), force: fs.Bool("force", false, "write even when the change introduces a validation finding"), - jsonOut: addJSON(fs), + noVerify: addNoVerify(fs), + jsonOut: addJSON(fs), + } +} + +// patchOp is what a subcommand does to an artifact, which is the only thing the +// approved-plan guard needs to know. +// +// Approval is where authorship ends and execution begins. Before it, a plan is a +// draft and everything is editable; after it, the *work* is fixed and only the +// checklist's state moves — ticked, struck out with a reason, or added with one. +// What discovery can never touch is guaranteed structurally rather than by +// instruction: Why, Out of scope, Done when and the title are reachable only through +// append/prepend/replace, and those three are the ones refused. +type patchOp string + +const ( + opState patchOp = "state" // check, uncheck, fm — the checklist's state + opContent patchOp = "content" // append, prepend, replace — authorship + opTask patchOp = "task" // rewriting a task, which is authorship of one item + opAdd patchOp = "add" + opRemove patchOp = "rm" +) + +// guardApproved refuses the operations that would rewrite work somebody approved. +func guardApproved(a *artifact.Artifact, op patchOp) error { + if !a.Approved() { + return nil + } + switch op { + case opContent: + return fmt.Errorf("%s is approved: its prose is fixed, and this would rewrite it\n"+ + " discovery adds and strikes tasks; it never edits Why, Out of scope, or Done when.\n"+ + " → `%s plan reseal %s --force` after an edit you made deliberately", a.Path, prog(), a.Name) + default: + return nil } } @@ -88,7 +124,7 @@ func runPatchCheck(args []string, done bool) int { render.Err(name + " needs an artifact and at least one task number") return ExitError } - return withEditor(pf, rest[0], func(e *artifact.Editor) { + return withEditor(pf, rest[0], opState, func(_ *artifact.Artifact, e *artifact.Editor) { for _, number := range rest[1:] { e.Check(number, done) } @@ -104,6 +140,8 @@ func runPatchTask(args []string) int { req := fs.String("req", "", "replace the citations: a comma-separated `list` of ids") number := fs.String("number", "", "renumber the task") state := fs.String("state", "", "set the box: `open` or done") + depends := fs.String("depends", "", "replace the dependencies: a comma-separated `list` of task numbers") + priority := fs.Int("priority", 0, "set the priority: a whole `number` 1 or greater; 0 clears it") rest, err := parseFlags(fs, args) if err != nil { return ExitError @@ -113,6 +151,21 @@ func runPatchTask(args []string) int { return ExitError } edit := artifact.TaskEdit{} + if isSet(fs, "depends") { + ids := splitList(*depends) + edit.Depends = &ids + } + if isSet(fs, "priority") { + switch { + case *priority == 0: + edit.ClearPriority = true + case *priority < 0: + render.Err("a priority is a whole number 1 or greater; lower is more urgent") + return ExitError + default: + edit.Priority = priority + } + } if isSet(fs, "text") { edit.Text = text } @@ -143,22 +196,48 @@ func runPatchTask(args []string) int { } } if edit.Text == nil && edit.Methodology == nil && edit.Requirements == nil && - edit.Number == nil && edit.Checked == nil { - render.Err("patch task changes nothing: pass --text, --method, --req, --number, or --state") + edit.Number == nil && edit.Checked == nil && edit.Depends == nil && + edit.Priority == nil && !edit.ClearPriority { + render.Err("patch task changes nothing: pass --text, --method, --req, --number, --state, --depends or --priority") return ExitError } - return withEditor(pf, rest[0], func(e *artifact.Editor) { e.SetTask(rest[1], edit) }) + // Which of these an approved plan still accepts is decided by what they change. + // --depends and --priority reorder work that is already agreed; --text, --method + // and --number change what the work *is*, and that is what approval fixed. + rewrites := edit.Text != nil || edit.Methodology != nil || edit.Number != nil || edit.Requirements != nil + return withEditor(pf, rest[0], opTask, func(a *artifact.Artifact, e *artifact.Editor) { + if a.Approved() && rewrites { + e.Fail("%s is approved: --text, --method, --req and --number rewrite the work itself.\n"+ + " a task that turned out wrong is struck out with `%s patch rm %s %s --reason \"…\"` "+ + "and replaced by a new one.", a.Path, prog(), a.Name, rest[1]) + return + } + e.SetTask(rest[1], edit) + }) } +// runPatchAdd writes a new task — authorship while a plan is a draft, discovery once +// it is approved. +// +// Under approval it stops taking `--number` and starts allocating one, because a +// number is an address: every commit, every branch name and every earlier report used +// it, so reusing one would make two different pieces of work share a name. The next +// slot is `max(item) + 1` in the group, counting the tasks discovery struck out — +// which is derivable from the file itself, so nothing anywhere has to remember it. func runPatchAdd(args []string) int { fs := flag.NewFlagSet("patch add", flag.ContinueOnError) fs.SetOutput(os.Stderr) pf := addPatchFlags(fs) - section := fs.String("section", "", "the `section` slug whose task list this joins") - number := fs.String("number", "", "the task's `number`") + section := fs.String("section", "tasks", "the `section` slug whose task list this joins") + number := fs.String("number", "", "the task's `number` — a draft only; an approved plan allocates it") + group := fs.String("group", "", "allocate the next number in this `group`") + newGroup := fs.Bool("new-group", false, "allocate the first number of a new group") method := fs.String("method", "Unit", "`Unit` or TDD") text := fs.String("text", "", "the description") req := fs.String("req", "", "the requirements it satisfies, comma-separated") + depends := fs.String("depends", "", "the tasks that have to be done first, comma-separated") + priority := fs.Int("priority", 0, "a whole `number` 1 or greater; lower is more urgent") + reason := fs.String("reason", "", "why this turned up after the plan was approved") rest, err := parseFlags(fs, args) if err != nil { return ExitError @@ -167,24 +246,74 @@ func runPatchAdd(args []string) int { render.Err("patch add needs an artifact") return ExitError } - if *section == "" || *number == "" || strings.TrimSpace(*text) == "" { - render.Err("patch add needs --section, --number and --text") + if strings.TrimSpace(*text) == "" { + render.Err("patch add needs --text: a task nobody described is not one") + return ExitError + } + if *group != "" && *newGroup { + render.Err("--group names a group and --new-group makes one; ask for one of them") return ExitError } if !validMethod(*method) { return ExitError } - t := artifact.NewTask{ - Section: *section, Number: *number, Methodology: *method, - Text: *text, Requirements: splitList(*req), + if *priority < 0 { + render.Err("a priority is a whole number 1 or greater; lower is more urgent") + return ExitError } - return withEditor(pf, rest[0], func(e *artifact.Editor) { e.AddTask(t) }) + + return withEditor(pf, rest[0], opAdd, func(a *artifact.Artifact, e *artifact.Editor) { + id := *number + if a.Approved() { + if id != "" { + e.Fail("%s is approved, so it allocates the number: pass --group N or --new-group.\n"+ + " a number is an address, and reusing one makes two pieces of work share a name.", a.Path) + return + } + if strings.TrimSpace(*reason) == "" { + e.Fail("%s is approved: a task added now needs --reason, which is the record of "+ + "why it was not in the plan somebody agreed to.", a.Path) + return + } + if *group == "" && !*newGroup { + e.Fail("%s is approved: say where this lands with --group N or --new-group.", a.Path) + return + } + } + if id == "" { + switch { + case *newGroup: + id = fmt.Sprintf("%d.1", a.HighGroup()+1) + case *group != "": + id = fmt.Sprintf("%s.%d", *group, a.HighWater(*group)+1) + default: + e.Fail("patch add needs --number, --group N, or --new-group") + return + } + } + t := artifact.NewTask{ + Section: *section, Number: id, Methodology: *method, + Text: *text, Requirements: splitList(*req), Depends: splitList(*depends), + Reason: strings.TrimSpace(*reason), + } + if *priority > 0 { + t.Priority = priority + } + e.AddTask(t) + }) } +// runPatchRemove takes a task out of the running. +// +// In a draft that means deleting it. In an approved plan it means striking it out: +// the line stays, carrying `_Status removed_` and the reason, because the plan +// somebody agreed to is a record and a record that silently shrank is only in git. +// It is also what keeps the number from being handed out again. func runPatchRemove(args []string) int { fs := flag.NewFlagSet("patch rm", flag.ContinueOnError) fs.SetOutput(os.Stderr) pf := addPatchFlags(fs) + reason := fs.String("reason", "", "why the work went away — required once the plan is approved") rest, err := parseFlags(fs, args) if err != nil { return ExitError @@ -193,7 +322,18 @@ func runPatchRemove(args []string) int { render.Err("patch rm needs an artifact and one task number") return ExitError } - return withEditor(pf, rest[0], func(e *artifact.Editor) { e.RemoveTask(rest[1]) }) + return withEditor(pf, rest[0], opRemove, func(a *artifact.Artifact, e *artifact.Editor) { + if !a.Approved() && strings.TrimSpace(*reason) == "" { + e.RemoveTask(rest[1]) + return + } + if strings.TrimSpace(*reason) == "" { + e.Fail("%s is approved: removing task %s needs --reason, and the task keeps its line so "+ + "the reason survives and the number is never reused.", a.Path, rest[1]) + return + } + e.StrikeTask(rest[1], *reason) + }) } func runPatchText(op string, args []string) int { @@ -218,7 +358,7 @@ func runPatchText(op string, args []string) int { render.Err("nothing to write: pass --text, --text - to read stdin, or --file") return ExitError } - return withEditor(pf, rest[0], func(e *artifact.Editor) { + return withEditor(pf, rest[0], opContent, func(_ *artifact.Artifact, e *artifact.Editor) { switch op { case "append": e.Append(rest[1], body) @@ -251,8 +391,15 @@ func runPatchFrontmatter(args []string) int { } pairs = append(pairs, [2]string{strings.TrimSpace(k), strings.TrimSpace(v)}) } - return withEditor(pf, rest[0], func(e *artifact.Editor) { + return withEditor(pf, rest[0], opState, func(_ *artifact.Artifact, e *artifact.Editor) { for _, p := range pairs { + // The seal is scc's to write. Letting `patch fm` set it would make the + // checksum a value anyone can type, which is the same as not having one. + if p[0] == artifact.KeyChecksum { + e.Fail("`%s` is written by `%s plan approve` and `%s plan reseal`, not by hand", + artifact.KeyChecksum, prog(), prog()) + return + } e.SetFrontmatter(p[0], p[1]) } }) @@ -266,7 +413,7 @@ func runPatchFrontmatter(args []string) int { // than left on disk. An artifact scc has no validator for — a wiki page — is written // and said to be unverified, because claiming a check that did not happen is worse // than not checking. -func withEditor(pf patchFlags, target string, apply func(*artifact.Editor)) int { +func withEditor(pf patchFlags, target string, op patchOp, apply func(*artifact.Artifact, *artifact.Editor)) int { root, ok := resolveRoot(*pf.root) if !ok || !requireWorkspace(root) { return ExitError @@ -275,9 +422,19 @@ func withEditor(pf patchFlags, target string, apply func(*artifact.Editor)) int if !ok { return ExitError } + // The seal is checked before the edit is applied, and that order is the whole + // value of it: a harness that edited the file by hand and then ran `patch check` + // would otherwise have its edit resealed by the same command that should have + // reported it. + if code := sealGuard([]*artifact.Artifact{a}, *pf.noVerify); code != ExitOK { + return code + } e := a.Edit() - apply(e) + if err := guardApproved(a, op); err != nil { + e.Fail("%v", err) + } + apply(a, e) content, err := e.Content() if err != nil { render.Err(err.Error()) @@ -348,6 +505,17 @@ func withEditor(pf patchFlags, target string, apply func(*artifact.Editor)) int } } + // Re-seal last, and only over what actually landed. A rollback restored the file + // the seal already describes, so there is nothing to recompute. + if report.Written && a.Approved() { + sealed := withTrailingNewline(artifact.Reseal(content)) + if err := workspace.AtomicWrite(a.Abs, []byte(sealed), 0o644); err != nil { + render.Err(err.Error()) + return ExitError + } + report.Checksum = artifact.Seal(sealed) + } + if *pf.jsonOut { code := ExitOK if len(report.Introduced) > 0 { @@ -367,6 +535,7 @@ type patchReport struct { Changes []artifact.Change `json:"changes"` Written bool `json:"written"` Verified string `json:"verified"` // clean | rolled-back | forced | refused | no-validator | not-run | unchanged + Checksum string `json:"checksum,omitempty"` Introduced []finding.Finding `json:"introduced,omitempty"` } @@ -556,8 +725,10 @@ func patchUsage() { %s patch check … mark tasks done %s patch uncheck … mark tasks not done %s patch task [--text|--method|--req|--number|--state] - %s patch add --section --number N --text "…" [--method|--req] - %s patch rm + [--depends 1.1,1.2|--priority N] + %s patch add --text "…" [--number N|--group N|--new-group] + [--method|--req|--depends|--priority|--reason] + %s patch rm [--reason "…"] %s patch append
--text "…"|--text -|--file %s patch prepend
… %s patch replace
… @@ -571,6 +742,11 @@ After writing, the file is re-validated. A change that introduces a finding is r back and reported; --force writes it anyway. --dry-run shows the lines and writes nothing. +Once a plan is approved its work is fixed, and only discovery moves: check and uncheck, +fm, add with a --reason and an allocated number, and rm — which strikes the task out +where it stands rather than deleting it, so the reason survives and the number is never +handed out twice. Rewriting a task or its prose is refused. + Exit codes: 0 written · 1 usage or runtime error · 2 rolled back, or forced with findings. `, prog(), prog(), prog(), prog(), prog(), prog(), prog(), prog(), prog(), prog()) } diff --git a/internal/cli/plan.go b/internal/cli/plan.go index 541a4a2..e9dd886 100644 --- a/internal/cli/plan.go +++ b/internal/cli/plan.go @@ -9,6 +9,7 @@ import ( "sort" "strings" + "github.com/protonspy/spec-claude-code/internal/artifact" "github.com/protonspy/spec-claude-code/internal/assets" "github.com/protonspy/spec-claude-code/internal/finding" "github.com/protonspy/spec-claude-code/internal/mdscan" @@ -39,6 +40,12 @@ func runPlan(args []string) int { return runPlanDelete(args[1:]) case "validate": return runPlanValidate(args[1:]) + case "approve": + return runPlanApprove(args[1:]) + case "reseal": + return runPlanReseal(args[1:]) + case "migrate": + return runPlanMigrate(args[1:]) case "help", "-h", "--help": planUsage() return ExitOK @@ -126,6 +133,161 @@ func runPlanValidate(args []string) int { }) } +// runPlanApprove closes authorship and opens execution. +// +// It refuses a plan with findings, and that refusal is the point of the command: an +// approved plan is one nothing may rewrite, so approving one that is already wrong +// would freeze the defect and make fixing it require `--force`. Everything after this +// is `patch check`, `patch fm`, and discovery. +func runPlanApprove(args []string) int { + fs := flag.NewFlagSet("plan approve", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + root := addRoot(fs) + jsonOut := addJSON(fs) + rest, err := parseFlags(fs, args) + if err != nil { + return ExitError + } + name, ok := artifactName(rest, "plan") + if !ok { + return ExitError + } + target, ok := resolveRoot(*root) + if !ok || !requireWorkspace(target) { + return ExitError + } + path := paths.Plan(target, name) + if !isFile(path) { + render.Err(fmt.Sprintf("no plan %q under %s", name, paths.PlansSeg)) + return ExitError + } + + set, err := validate.Plan(target, name) + if err != nil { + render.Err(err.Error()) + return ExitError + } + if !set.Empty() { + if *jsonOut { + emitJSON(set.Document()) + return ExitFindings + } + set.Report(relPath(target, path)) + render.Detail(" an approved plan is one nothing may rewrite; fix these first") + return ExitFindings + } + + content, err := os.ReadFile(path) + if err != nil { + render.Err(err.Error()) + return ExitError + } + sealed := withTrailingNewline(artifact.Approve(string(content))) + if err := workspace.AtomicWrite(path, []byte(sealed), 0o644); err != nil { + render.Err(err.Error()) + return ExitError + } + sum := artifact.Seal(sealed) + if *jsonOut { + return emitJSON(struct { + Plan string `json:"plan"` + Path string `json:"path"` + Status string `json:"status"` + Checksum string `json:"checksum"` + }{name, relPath(target, path), artifact.StatusApproved, sum}) + } + render.OK(fmt.Sprintf("%s — approved", relPath(target, path))) + render.Info("seal: " + sum) + render.Info("its content is fixed now: tick boxes with `" + prog() + " patch check`, and discover with `" + + prog() + " patch add|rm --reason`") + return ExitOK +} + +// runPlanReseal records a legitimate edit made outside the cycle — a merge conflict +// resolved by hand is the case it was written for. +// +// It demands --force and names both hashes, because the honest description of what it +// does is "erase the evidence that this file was edited outside scc". A command that +// did that quietly would make the seal worth nothing. +func runPlanReseal(args []string) int { + fs := flag.NewFlagSet("plan reseal", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + root := addRoot(fs) + force := fs.Bool("force", false, "required: reselling accepts an edit made outside scc as intended") + jsonOut := addJSON(fs) + rest, err := parseFlags(fs, args) + if err != nil { + return ExitError + } + name, ok := artifactName(rest, "plan") + if !ok { + return ExitError + } + target, ok := resolveRoot(*root) + if !ok || !requireWorkspace(target) { + return ExitError + } + path := paths.Plan(target, name) + if !isFile(path) { + render.Err(fmt.Sprintf("no plan %q under %s", name, paths.PlansSeg)) + return ExitError + } + a, err := artifact.Load(target, path) + if err != nil { + render.Err(err.Error()) + return ExitError + } + if !a.Approved() { + render.Err(fmt.Sprintf("%s is not approved, so it carries no seal to recompute", relPath(target, path))) + render.Detail(fmt.Sprintf(" `%s plan approve %s` is what seals it", prog(), name)) + return ExitError + } + recorded, actual, drifted := a.Drift() + if !drifted { + if *jsonOut { + return emitJSON(struct { + Plan string `json:"plan"` + Checksum string `json:"checksum"` + Changed bool `json:"changed"` + }{name, recorded, false}) + } + render.OK(relPath(target, path) + " — the seal already matches; nothing to do") + return ExitOK + } + if !*force { + render.Err(fmt.Sprintf("%s has drifted; reselling accepts that edit as intended", relPath(target, path))) + render.Detail(" seal: " + recorded) + render.Detail(" actual: " + actual) + render.Detail(" → `git diff " + relPath(target, path) + "` is the change you are about to bless") + render.Detail(" → pass --force once you have read it, or revert it with git") + return ExitError + } + + content, err := os.ReadFile(path) + if err != nil { + render.Err(err.Error()) + return ExitError + } + sealed := withTrailingNewline(artifact.Reseal(string(content))) + if err := workspace.AtomicWrite(path, []byte(sealed), 0o644); err != nil { + render.Err(err.Error()) + return ExitError + } + if *jsonOut { + return emitJSON(struct { + Plan string `json:"plan"` + Path string `json:"path"` + Was string `json:"was"` + Checksum string `json:"checksum"` + Changed bool `json:"changed"` + }{name, relPath(target, path), recorded, artifact.Seal(sealed), true}) + } + render.OK(relPath(target, path) + " — resealed") + render.Info("was: " + recorded) + render.Info("now: " + artifact.Seal(sealed)) + return ExitOK +} + type planEntry struct { Name string `json:"name"` Path string `json:"path"` @@ -229,14 +391,28 @@ func runPlanDelete(args []string) int { return ExitOK } +// withTrailingNewline is how every file scc writes ends. A sealed plan that lost its +// final newline would hash differently from the same plan a text editor saved. +func withTrailingNewline(s string) string { + return strings.TrimRight(s, "\n") + "\n" +} + func planUsage() { fmt.Fprintf(os.Stderr, `Usage: %s plan new [--autonomy=auto|gated] [--ci=wait|no-wait] [--force] %s plan list %s plan delete --force %s plan validate [] + %s plan approve validate, then fix the content and seal it + %s plan reseal --force accept an edit made outside scc, and re-seal + %s plan migrate move a plan onto the closed-section contract + +A plan is everything that is not worth a spec: plans/.md, holding a short +header and a checklist. Its sections are closed — Why, Paths, References, Out of +scope, Tasks, Done when — which is the only thing that caps its size. -A plan is everything that is not worth a spec: plans/.md, holding a checklist -of tasks, references to the specs it decomposes into, or both. -`, prog(), prog(), prog(), prog()) +Approving it makes the content fixed: after that, `+"`%s patch check`"+` moves the boxes and +discovery adds or strikes tasks with a reason, and anything that would rewrite the +work is refused. +`, prog(), prog(), prog(), prog(), prog(), prog(), prog(), prog()) } diff --git a/internal/cli/seal.go b/internal/cli/seal.go new file mode 100644 index 0000000..7894fab --- /dev/null +++ b/internal/cli/seal.go @@ -0,0 +1,75 @@ +package cli + +import ( + "flag" + "fmt" + + "github.com/protonspy/spec-claude-code/internal/artifact" + "github.com/protonspy/spec-claude-code/internal/render" +) + +// Checking the seal, on the way in and on the way out. +// +// An approved plan's content is fixed: only the checklist's state moves, and only +// through `scc patch`. So every command that reads or writes one hashes it first and +// says so when the hash disagrees. The cost is a sha256 over a few kilobytes of a +// file that was going to be read anyway, which is why it can be on by default. +// +// Checking *before* applying an edit is the part that matters. A harness that edited +// the file by hand and then ran `scc patch check` would otherwise have its edit +// resealed on top, and the evidence would be gone in the same command that should +// have reported it. + +func addNoVerify(fs *flag.FlagSet) *bool { + return fs.Bool("no-verify", false, "skip the seal check on an approved plan (for diagnosis)") +} + +// loadVerified is loadMany plus the seal check — the read path for every command +// that reports on a plan's content or its checklist. It returns the exit code, so a +// drifted plan stops the command instead of being answered from. +func loadVerified(root string, args []string, skip bool) ([]*artifact.Artifact, int) { + arts, ok := loadMany(root, args) + if !ok { + return nil, ExitError + } + if code := sealGuard(arts, skip); code != ExitOK { + return nil, code + } + return arts, ExitOK +} + +// sealGuard reports every artifact whose content no longer matches its seal, and +// returns the exit code the caller should use. +// +// The message names both hashes and both ways out, because the reader is an agent +// that cannot see the file: an error it cannot act on is the same as no error. +func sealGuard(arts []*artifact.Artifact, skip bool) int { + if skip { + return ExitOK + } + code := ExitOK + for _, a := range arts { + recorded, actual, drifted := a.Drift() + if !drifted { + continue + } + code = ExitFindings + render.Err(fmt.Sprintf("%s — drift: the file changed outside scc", a.Path)) + render.Detail(fmt.Sprintf(" seal: %s (status: %s)", short(recorded), artifact.StatusApproved)) + render.Detail(fmt.Sprintf(" actual: %s", short(actual))) + render.Detail(" an approved plan's content only changes through `" + prog() + " patch`.") + render.Detail(fmt.Sprintf(" → revert it with git, or `%s plan reseal %s --force` if the edit was meant", + prog(), a.Name)) + } + return code +} + +func short(sum string) string { + if len(sum) > 12 { + return sum[:12] + "…" + } + if sum == "" { + return "(none recorded)" + } + return sum +} diff --git a/internal/cli/seal_test.go b/internal/cli/seal_test.go new file mode 100644 index 0000000..b96952c --- /dev/null +++ b/internal/cli/seal_test.go @@ -0,0 +1,231 @@ +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// v2Plan is a plan on the closed-section contract, ready to approve. +const v2Plan = `--- +autonomy: auto +ci: wait +--- + +# Sweep + +Replace the legacy path, one group at a time. + +## Why + +The old path cannot be extended without a rewrite. + +## Tasks + +- [ ] 1.1 (Unit) Lay the foundation +- [ ] 1.2 (TDD) Build on it + _Depends 1.1_ + +## Done when + +- the suite is green +` + +func writePlanFile(t *testing.T, root, name, content string) string { + t.Helper() + dir := filepath.Join(root, "plans") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(dir, name+".md") + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + return path +} + +func TestPlanApproveSealsAValidPlan(t *testing.T) { + root := initWorkspace(t) + path := writePlanFile(t, root, "sweep", v2Plan) + + stdout, stderr, code := run(t, "plan", "approve", "sweep", "--root", root) + if code != ExitOK { + t.Fatalf("approve = %d (%s)", code, stderr) + } + if !strings.Contains(stdout+stderr, "approved") { + t.Errorf("output = %q / %q", stdout, stderr) + } + body := readFile(t, path) + if !strings.Contains(body, "status: approved") || !strings.Contains(body, "checksum: ") { + t.Fatalf("the plan was not sealed:\n%s", body) + } + + // And an unchanged sealed plan reads without complaint. + if _, _, code := run(t, "map", "tasks", "sweep", "--root", root, "--next"); code != ExitOK { + t.Errorf("reading a sealed, unchanged plan = %d", code) + } +} + +// Approving a plan that is already wrong would freeze the defect, and unfreezing it +// would then need --force. So approval is where the validator has to be clean. +func TestPlanApproveRefusesFindings(t *testing.T) { + root := initWorkspace(t) + writePlanFile(t, root, "broken", "# Broken\n\nWhat this is.\n\n## Tasks\n\n- [ ] 1.1 Do it\n") + if _, _, code := run(t, "plan", "approve", "broken", "--root", root); code != ExitFindings { + t.Errorf("approve of a plan with findings = %d, want %d", code, ExitFindings) + } + if body := readFile(t, filepath.Join(root, "plans", "broken.md")); strings.Contains(body, "status:") { + t.Error("a refused approval still wrote the status") + } +} + +// Drift is the whole point: an edit made outside scc has to become visible at the +// next command that touches the file. +func TestDriftIsReportedAndActionable(t *testing.T) { + root := initWorkspace(t) + path := writePlanFile(t, root, "sweep", v2Plan) + if _, _, code := run(t, "plan", "approve", "sweep", "--root", root); code != ExitOK { + t.Fatalf("approve = %d", code) + } + + body := readFile(t, path) + if err := os.WriteFile(path, []byte(strings.Replace(body, "- [ ] 1.1", "- [x] 1.1", 1)), 0o644); err != nil { + t.Fatal(err) + } + + stdout, stderr, code := run(t, "map", "tasks", "sweep", "--root", root) + if code != ExitFindings { + t.Errorf("reading a drifted plan = %d, want %d", code, ExitFindings) + } + for _, want := range []string{"drift", "plan reseal"} { + if !strings.Contains(stdout+stderr, want) { + t.Errorf("the drift report does not mention %q:\n%s%s", want, stdout, stderr) + } + } + + // A patch has to refuse *before* it applies, or it reseals the hand edit on top + // and destroys the evidence in the same command that should have reported it. + if _, _, code := run(t, "patch", "check", "sweep", "1.2", "--root", root); code != ExitFindings { + t.Errorf("patching a drifted plan = %d, want %d", code, ExitFindings) + } + + // --no-verify is the diagnosis escape hatch, and nothing else. + if _, _, code := run(t, "map", "tasks", "sweep", "--root", root, "--no-verify"); code != ExitOK { + t.Errorf("--no-verify = %d", code) + } + + if _, _, code := run(t, "plan", "reseal", "sweep", "--root", root); code != ExitError { + t.Error("reseal without --force should refuse") + } + if _, _, code := run(t, "plan", "reseal", "sweep", "--force", "--root", root); code != ExitOK { + t.Error("reseal --force should accept the edit") + } + if _, _, code := run(t, "map", "tasks", "sweep", "--root", root); code != ExitOK { + t.Error("after reselling, the plan reads clean") + } +} + +// Ticking a box through scc has to leave the plan sealed, or the next read reports +// drift the tool itself caused. +func TestPatchReSealsWhatItWrites(t *testing.T) { + root := initWorkspace(t) + writePlanFile(t, root, "sweep", v2Plan) + if _, _, code := run(t, "plan", "approve", "sweep", "--root", root); code != ExitOK { + t.Fatalf("approve = %d", code) + } + if _, stderr, code := run(t, "patch", "check", "sweep", "1.1", "--root", root); code != ExitOK { + t.Fatalf("patch check = %d (%s)", code, stderr) + } + if _, _, code := run(t, "map", "tasks", "sweep", "--root", root, "--next"); code != ExitOK { + t.Error("a plan scc itself ticked reported drift") + } +} + +// What discovery may and may not do, once a plan is approved. +func TestApprovedPlanGuards(t *testing.T) { + root := initWorkspace(t) + writePlanFile(t, root, "sweep", v2Plan) + if _, _, code := run(t, "plan", "approve", "sweep", "--root", root); code != ExitOK { + t.Fatalf("approve = %d", code) + } + path := filepath.Join(root, "plans", "sweep.md") + + refused := [][]string{ + {"patch", "task", "sweep", "1.1", "--text", "something else"}, + {"patch", "task", "sweep", "1.1", "--method", "TDD"}, + {"patch", "task", "sweep", "1.1", "--number", "1.9"}, + {"patch", "append", "sweep", "#why", "--text", "and another reason"}, + {"patch", "replace", "sweep", "#done-when", "--text", "- nothing"}, + {"patch", "rm", "sweep", "1.2"}, + {"patch", "add", "sweep", "--text", "new work", "--number", "1.3", "--reason", "found it"}, + {"patch", "add", "sweep", "--text", "new work", "--group", "1"}, + } + for _, args := range refused { + if _, _, code := run(t, append(args, "--root", root)...); code != ExitError { + t.Errorf("%v on an approved plan = %d, want refused", args, code) + } + } + + allowed := [][]string{ + {"patch", "check", "sweep", "1.1"}, + {"patch", "fm", "sweep", "pr=per-group"}, + {"patch", "task", "sweep", "1.2", "--priority", "1"}, + } + for _, args := range allowed { + if _, stderr, code := run(t, append(args, "--root", root)...); code != ExitOK { + t.Errorf("%v on an approved plan = %d, want allowed (%s)", args, code, stderr) + } + } + + // Discovery: a number is allocated rather than chosen, and the reason is recorded. + if _, stderr, code := run(t, "patch", "add", "sweep", "--text", "the thing nobody saw coming", + "--group", "1", "--reason", "turned up while doing 1.1", "--root", root); code != ExitOK { + t.Fatalf("discovery add = %d (%s)", code, stderr) + } + body := readFile(t, path) + if !strings.Contains(body, "1.3 (Unit) the thing nobody saw coming") { + t.Errorf("the added task did not take the next number:\n%s", body) + } + if !strings.Contains(body, "_Reason turned up while doing 1.1_") { + t.Errorf("the reason was not recorded:\n%s", body) + } + + // And a removal keeps its line, so the number is never handed out twice. + if _, _, code := run(t, "patch", "rm", "sweep", "1.3", "--reason", "it was already covered", "--root", root); code != ExitOK { + t.Fatalf("discovery rm = %d", code) + } + body = readFile(t, path) + if !strings.Contains(body, "1.3 (Unit) the thing nobody saw coming") { + t.Error("a discovery removal deleted the line") + } + if !strings.Contains(body, "_Status removed_") { + t.Error("a discovery removal did not record the status") + } + if _, _, code := run(t, "patch", "add", "sweep", "--text", "another one", + "--group", "1", "--reason", "and another", "--root", root); code != ExitOK { + t.Fatal("second discovery add failed") + } + if body = readFile(t, path); !strings.Contains(body, "1.4 (Unit) another one") { + t.Errorf("the number of a removed task was reused:\n%s", body) + } +} + +// A draft is authorship: everything is editable, rm deletes, and add takes a number. +func TestDraftPlanIsFullyEditable(t *testing.T) { + root := initWorkspace(t) + writePlanFile(t, root, "sweep", v2Plan) + for _, args := range [][]string{ + {"patch", "task", "sweep", "1.1", "--text", "something else"}, + {"patch", "append", "sweep", "#why", "--text", "and another reason"}, + {"patch", "add", "sweep", "--text", "more work", "--number", "1.3"}, + {"patch", "rm", "sweep", "1.3"}, + } { + if _, stderr, code := run(t, append(args, "--root", root)...); code != ExitOK { + t.Errorf("%v on a draft = %d, want allowed (%s)", args, code, stderr) + } + } + if body := readFile(t, filepath.Join(root, "plans", "sweep.md")); strings.Contains(body, "1.3") { + t.Error("rm on a draft should delete the line outright") + } +} diff --git a/internal/validate/flags_test.go b/internal/validate/flags_test.go new file mode 100644 index 0000000..2221329 --- /dev/null +++ b/internal/validate/flags_test.go @@ -0,0 +1,99 @@ +package validate + +import "testing" + +// Each of these is a defect a loop cannot recover from on its own: it either runs on +// a fact nobody wrote or waits forever for one that will never arrive. +func TestTaskFlagFindings(t *testing.T) { + for _, tc := range []struct { + name string + body string + want string + }{ + { + "a flag nobody defined is a typo, not prose", + "- [ ] 1.1 (Unit) Do it\n _Depend 1.0_\n", + "task.unknown-flag", + }, + { + "two of the same flag are two answers to one question", + "- [ ] 1.1 (Unit) Do it\n _Priority 1_\n _Priority 2_\n", + "task.duplicate-flag", + }, + { + "a priority that is not a positive whole number orders nothing", + "- [ ] 1.1 (Unit) Do it\n _Priority soon_\n", + "task.invalid-priority", + }, + { + "the box is the state, so a status that restates it is two records of one fact", + "- [ ] 1.1 (Unit) Do it\n _Status open_\n", + "task.status-duplicates-box", + }, + { + "and any other status is simply not one", + "- [ ] 1.1 (Unit) Do it\n _Status parked_\n", + "task.invalid-status", + }, + { + "a removal with no reason is a line nobody can explain later", + "- [ ] 1.1 (Unit) Do it\n _Status removed_\n", + "task.removed-without-reason", + }, + { + "removed and ticked at once", + "- [x] 1.1 (Unit) Do it\n _Status removed_\n _Reason it went away_\n", + "task.removed-but-checked", + }, + { + "a dependency on a task that is not here never resolves", + "- [ ] 1.1 (Unit) Do it\n _Depends 9.9_\n", + "task.unknown-dependency", + }, + { + "a task waiting for itself", + "- [ ] 1.1 (Unit) Do it\n _Depends 1.1_\n", + "task.self-dependency", + }, + { + "a dependency on a removed task will never be ticked", + "- [ ] 1.1 (Unit) Do it\n _Depends 1.2_\n" + + "- [ ] 1.2 (Unit) Dropped\n _Status removed_\n _Reason not needed_\n", + "task.depends-on-removed", + }, + { + "a cycle is a deadlock --next cannot explain", + "- [ ] 1.1 (Unit) One\n _Depends 1.2_\n- [ ] 1.2 (Unit) Two\n _Depends 1.1_\n", + "task.dependency-cycle", + }, + } { + t.Run(tc.name, func(t *testing.T) { + root := t.TempDir() + writePlan(t, root, "sweep", plan("", tc.body)) + if got := planFindings(t, root, "sweep"); !contains(got, tc.want) { + t.Errorf("rules = %v, want %s", got, tc.want) + } + }) + } +} + +// The flags are additive: a plan that carries none is exactly as valid as it was +// before they existed, which is what makes this half of the change safe to ship. +func TestPlansWithoutFlagsAreUnaffected(t *testing.T) { + root := t.TempDir() + writePlan(t, root, "sweep", plan("", "- [ ] 1.1 (Unit) Do it\n- [ ] 1.2 (TDD) Do the other thing\n")) + if got := planFindings(t, root, "sweep"); len(got) != 0 { + t.Errorf("a plan with no flags reported %v", got) + } +} + +// Italic prose that is not sitting where a flag sits is prose. The parser only looks +// directly under a task, which is what keeps emphasis in a `## Why` paragraph from +// becoming a finding. +func TestEmphasisElsewhereIsNotAFlag(t *testing.T) { + root := t.TempDir() + writePlan(t, root, "sweep", plan("", "- [ ] 1.1 (Unit) Do it\n\nSome prose.\n\n_An emphasized aside._\n")) + if got := planFindings(t, root, "sweep"); contains(got, "task.unknown-flag") { + t.Errorf("emphasis away from a task was read as a flag: %v", got) + } +} diff --git a/internal/validate/plan.go b/internal/validate/plan.go index 2237f71..a2c0984 100644 --- a/internal/validate/plan.go +++ b/internal/validate/plan.go @@ -69,6 +69,8 @@ func Plan(root, name string) (*finding.Set, error) { } checkKickoffAs(set, file, doc.Frontmatter, "plan.kickoff-invalid") checkLoopAnswers(set, file, doc.Frontmatter) + checkSeal(set, file, doc.Frontmatter) + checkPlanShape(set, file, doc) // 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 @@ -107,6 +109,128 @@ func Plan(root, name string) (*finding.Set, error) { return set, nil } +// The plan's sections, and there are no others. +// +// A closed set is the whole mechanism. Nothing else caps a plan's size: the file is +// preloaded by every session that runs it, and a plan measured at 56KB got there +// because a heading nobody forbade — `## Notes` — grew to half the file. There is no +// line limit anywhere in this contract except on the description, because a limit +// that fires on a legitimate plan would be worse than the growth it prevents. What +// there is instead is nowhere for prose to go. +// +// Order is deliberately not checked. Every read of a plan addresses a section by its +// slug, so the order on disk changes no answer, and validating it would cost a rule +// and a migration to buy nothing. +var planSections = []PlanSection{ + {"Why", "why", true}, + {"Paths", "paths", false}, + {"References", "references", false}, + {"Out of scope", "out-of-scope", false}, + {"Tasks", "tasks", true}, + {"Done when", "done-when", true}, +} + +// PlanSection is one heading the contract allows. Exported because migration has to +// create exactly the sections this validator demands — two lists would drift, and the +// drift would show up as a migration whose output fails the validator that asked for it. +type PlanSection struct { + Title string + Slug string + Required bool +} + +// PlanSections is the contract, in the order the template writes it. +func PlanSections() []PlanSection { return append([]PlanSection(nil), planSections...) } + +// maxDescriptionLines is how long the opening prose may run before it stops being a +// description of the work and starts being the essay the closed sections exist to +// prevent. Counted in non-blank lines, which is the unit a writer can see. +const maxDescriptionLines = 6 + +// checkPlanShape holds the plan to its contract: a title, a short description, the +// three required sections, and no heading nobody agreed to. +func checkPlanShape(set *finding.Set, file string, doc *mdscan.Document) { + title := 0 + present := map[string]int{} + for _, h := range doc.Headings { + if h.Level == 1 && title == 0 { + title = h.Line + } + if h.Level != 2 { + continue + } + slug := h.Slug + known := false + for _, s := range planSections { + if slug == s.Slug { + known = true + break + } + } + if !known { + // Notes gets its own sentence because it is the heading this contract was + // written against, and because "unknown section" would read as a typo when + // the answer is that the content belongs somewhere else entirely. + if slug == "notes" { + set.Addf(file, h.Line, "plan.unknown-section", + "a plan has no notes section: what was decided and why is an ADR under %s/%s, "+ + "what changed is git, and a constraint on one item goes on that item's own line", + paths.DocsSeg, paths.ADRSeg) + continue + } + set.Addf(file, h.Line, "plan.unknown-section", + "`## %s` is not one of a plan's sections (%s); a plan is a header and a checklist, and prose has nowhere else to go on purpose", + h.Text, sectionTitles()) + continue + } + if _, dup := present[slug]; !dup { + present[slug] = h.Line + } + } + + if title == 0 { + set.Addf(file, 1, "plan.missing-title", "a plan opens with its title as an `# H1`") + } + for _, s := range planSections { + if s.Required && present[s.Slug] == 0 { + set.Addf(file, 1, "plan.missing-section", "a plan has a `## %s` section", s.Title) + } + } + + if title == 0 { + return + } + stop := len(doc.Body) + for _, h := range doc.Headings { + if h.Line > title { + stop = h.Line - 1 + break + } + } + lines := 0 + for n := title + 1; n <= stop && n <= len(doc.Body); n++ { + if strings.TrimSpace(doc.Body[n-1]) != "" { + lines++ + } + } + switch { + case lines == 0: + set.Addf(file, title, "plan.missing-description", + "a plan says what it is in one to three sentences under the title, before the first section") + case lines > maxDescriptionLines: + set.Addf(file, title, "plan.description-too-long", + "the description runs to %d lines; it is one to three sentences, and the rest belongs in `## Why`", lines) + } +} + +func sectionTitles() string { + out := make([]string, 0, len(planSections)) + for _, s := range planSections { + out = append(out, s.Title) + } + return strings.Join(out, ", ") +} + // 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. // @@ -121,6 +245,30 @@ var loopValues = map[string]map[string]bool{ "pr": {"per-group": true, "per-plan": true}, } +// checkSeal validates the two keys `plan approve` writes. They are checked only when +// present, like every other frontmatter answer: a plan nobody has approved is not a +// plan with a defect. +// +// The pair has to travel together. A `status: approved` with no checksum is a plan +// claiming to be sealed by a seal that is not there, and every read of it would +// silently skip the check it was approved to get. +func checkSeal(set *finding.Set, file string, fm mdscan.Frontmatter) { + status, has := fm.Get("status") + if !has { + return + } + if status != artifact.StatusDraft && status != artifact.StatusApproved { + set.Addf(file, 1, "plan.status-invalid", "`status: %s` is not one of %s, %s", + status, artifact.StatusDraft, artifact.StatusApproved) + return + } + if sum, _ := fm.Get(artifact.KeyChecksum); status == artifact.StatusApproved && sum == "" { + set.Addf(file, 1, "plan.unsealed", + "`status: %s` with no `%s:` — nothing can be checked against, so the approval means nothing", + artifact.StatusApproved, artifact.KeyChecksum) + } +} + func checkLoopAnswers(set *finding.Set, file string, fm mdscan.Frontmatter) { for _, key := range []string{"worktree", "merge", "pr"} { value, ok := fm.Get(key) diff --git a/internal/validate/plan_test.go b/internal/validate/plan_test.go index 45ccef8..5fdea59 100644 --- a/internal/validate/plan_test.go +++ b/internal/validate/plan_test.go @@ -2,6 +2,7 @@ package validate import ( "os" + "strings" "testing" "github.com/protonspy/spec-claude-code/internal/paths" @@ -17,6 +18,28 @@ func writePlan(t *testing.T, root, name, content string) { } } +// plan wraps a body in the sections the contract requires, so a test about one rule +// states that rule and not the whole contract. Anything the body opens with a `##` +// simply adds a section to the ones already here. +func plan(frontmatter, body string) string { + return frontmatter + `# Sweep + +What this work is, in one sentence. + +## Why + +Because the old path cannot be extended. + +## Tasks + +` + body + ` + +## Done when + +- ` + "`scc validate`" + ` is clean +` +} + func planFindings(t *testing.T, root, name string) []string { t.Helper() set, err := Plan(root, name) @@ -36,45 +59,115 @@ ci: wait # Checkout revamp -## Decomposition +Replace the checkout path, one leaf at a time. + +## Why + +The old path cannot be extended without a rewrite of the totals engine. + +## Paths + +- `+"`internal/checkout/`"+` + +## References - `+"`specs/cart-totals/`"+` — the totals engine +## Out of scope + +- the payment provider itself + ## Tasks - [ ] 1.1 (Unit) Rename the legacy endpoint - [ ] 1.2 (TDD) Migrate the rounding helper + _Depends 1.1_ + _Priority 2_ + +## Done when + +- the legacy endpoint is gone and the suite is green `) if got := planFindings(t, root, "checkout-revamp"); len(got) != 0 { t.Errorf("a conforming plan produced findings: %v", got) } } +// The contract is closed, and that is the only thing capping a plan's size. The +// heading it was written against gets its own sentence, because "unknown section" +// would read as a typo when the answer is that the content belongs elsewhere. +func TestPlanSectionsAreClosed(t *testing.T) { + root := t.TempDir() + writePlan(t, root, "sweep", plan("", "- [ ] 1.1 (Unit) Do it\n\n## Notes\n\nA long essay begins here.\n")) + got := planFindings(t, root, "sweep") + if !contains(got, "plan.unknown-section") { + t.Fatalf("rules = %v, want plan.unknown-section", got) + } + set, _ := Plan(root, "sweep") + for _, f := range set.Sorted() { + if f.Rule == "plan.unknown-section" && !strings.Contains(f.Message, paths.ADRSeg) { + t.Errorf("the Notes message does not say where the content goes: %q", f.Message) + } + } + + writePlan(t, root, "other", plan("", "- [ ] 1.1 (Unit) Do it\n\n## Appendix\n\nMore.\n")) + if got := planFindings(t, root, "other"); !contains(got, "plan.unknown-section") { + t.Errorf("rules = %v, want plan.unknown-section for any other heading", got) + } +} + +func TestPlanRequiresItsThreeSections(t *testing.T) { + root := t.TempDir() + writePlan(t, root, "bare", "# Bare\n\nWhat this is.\n\n## Tasks\n\n- [ ] 1.1 (Unit) Do it\n") + got := planFindings(t, root, "bare") + if n := count(got, "plan.missing-section"); n != 2 { + t.Errorf("rules = %v, want Why and Done when reported missing, got %d", got, n) + } +} + +func TestPlanNeedsATitleAndAShortDescription(t *testing.T) { + root := t.TempDir() + writePlan(t, root, "untitled", "## Why\n\nBecause.\n\n## Tasks\n\n- [ ] 1.1 (Unit) Do it\n\n## Done when\n\n- done\n") + if got := planFindings(t, root, "untitled"); !contains(got, "plan.missing-title") { + t.Errorf("rules = %v, want plan.missing-title", got) + } + + writePlan(t, root, "silent", "# Silent\n\n## Why\n\nBecause.\n\n## Tasks\n\n- [ ] 1.1 (Unit) Do it\n\n## Done when\n\n- done\n") + if got := planFindings(t, root, "silent"); !contains(got, "plan.missing-description") { + t.Errorf("rules = %v, want plan.missing-description", got) + } + + essay := strings.Repeat("A line of the essay this contract exists to prevent.\n", maxDescriptionLines+1) + writePlan(t, root, "essay", "# Essay\n\n"+essay+"\n## Why\n\nBecause.\n\n## Tasks\n\n- [ ] 1.1 (Unit) Do it\n\n## Done when\n\n- done\n") + if got := planFindings(t, root, "essay"); !contains(got, "plan.description-too-long") { + t.Errorf("rules = %v, want plan.description-too-long", got) + } +} + // One source of truth per item. An item that both carries a checkbox and references a // spec keeps two records of one fact, and the copy is the one that goes stale. func TestItemCannotBothCheckAndReference(t *testing.T) { root := t.TempDir() writeSpec(t, root, "cart-totals", goodRequirements, goodDesign, goodTasks) - writePlan(t, root, "checkout-revamp", `# Checkout revamp - -- [ ] 1.1 (Unit) Build `+"`specs/cart-totals/`"+` -`) + writePlan(t, root, "checkout-revamp", plan("", "- [ ] 1.1 (Unit) Build `"+"specs/cart-totals/"+"`\n")) if got := planFindings(t, root, "checkout-revamp"); !contains(got, "plan.item-has-two-records") { t.Errorf("rules = %v, want plan.item-has-two-records", got) } } +// D1 moved the spec references out of `## Decomposition` and into `## References`. +// The parser recognizes a leaf by the citation rather than by the heading above it, +// so the check that a referenced spec exists moved with them and nothing else did. func TestPlanReferencesMustResolve(t *testing.T) { root := t.TempDir() - writePlan(t, root, "checkout-revamp", `# Checkout revamp - -## Decomposition - -- specs/does-not-exist/ — nothing is here -`) + writePlan(t, root, "checkout-revamp", + plan("", "- [ ] 1.1 (Unit) Do it\n\n## References\n\n- specs/does-not-exist/ — nothing is here\n")) if got := planFindings(t, root, "checkout-revamp"); !contains(got, "plan.unknown-spec") { t.Errorf("rules = %v, want plan.unknown-spec", got) } + if got := planFindings(t, root, "checkout-revamp"); contains(got, "plan.unknown-section") { + t.Errorf("`## References` is part of the contract: %v", got) + } } // A plan's tasks carry no requirement citations — a plan has no requirements — but the @@ -82,11 +175,9 @@ func TestPlanReferencesMustResolve(t *testing.T) { // to the vehicle that carried it. func TestPlanTasksUseTheSameGrammarWithoutCitations(t *testing.T) { root := t.TempDir() - writePlan(t, root, "sweep", `# Sweep - -- [ ] 1.1 (Unit) A task with no citation, which is fine here + writePlan(t, root, "sweep", plan("", `- [ ] 1.1 (Unit) A task with no citation, which is fine here - [ ] 1.2 A task with no methodology, which is not -`) +`)) got := planFindings(t, root, "sweep") if !contains(got, "task.missing-methodology") { t.Errorf("rules = %v, want task.missing-methodology", got) @@ -100,7 +191,7 @@ func TestPlanTasksUseTheSameGrammarWithoutCitations(t *testing.T) { // the work is the only reason both vehicles write a file. func TestEmptyPlan(t *testing.T) { root := t.TempDir() - writePlan(t, root, "hollow", "# Hollow\n\nSome prose and no items at all.\n") + writePlan(t, root, "hollow", plan("", "Some prose and no items at all.\n")) if got := planFindings(t, root, "hollow"); !contains(got, "plan.empty") { t.Errorf("rules = %v, want plan.empty", got) } @@ -108,7 +199,7 @@ func TestEmptyPlan(t *testing.T) { func TestPlanKickoffAnswers(t *testing.T) { root := t.TempDir() - writePlan(t, root, "sweep", "---\nci: eventually\n---\n\n# Sweep\n\n- [ ] 1.1 (Unit) Do it\n") + writePlan(t, root, "sweep", plan("---\nci: eventually\n---\n\n", "- [ ] 1.1 (Unit) Do it\n")) if got := planFindings(t, root, "sweep"); !contains(got, "plan.kickoff-invalid") { t.Errorf("rules = %v, want plan.kickoff-invalid", got) } @@ -116,13 +207,13 @@ func TestPlanKickoffAnswers(t *testing.T) { // A plan is the vehicle a whole run is driven from, so it carries the language // answer on the same terms a spec does — one key, checked when present. root = t.TempDir() - writePlan(t, root, "wide", "---\nautonomy: auto\nci: wait\nlang: wenyan\n---\n\n# Wide\n\n- [ ] 1.1 (Unit) Do it\n") + writePlan(t, root, "wide", plan("---\nautonomy: auto\nci: wait\nlang: wenyan\n---\n\n", "- [ ] 1.1 (Unit) Do it\n")) if got := planFindings(t, root, "wide"); len(got) != 0 { t.Errorf("a plan carrying every kickoff answer reported %v", got) } root = t.TempDir() - writePlan(t, root, "narrow", "---\nlang: pt-BR\n---\n\n# Narrow\n\n- [ ] 1.1 (Unit) Do it\n") + writePlan(t, root, "narrow", plan("---\nlang: pt-BR\n---\n\n", "- [ ] 1.1 (Unit) Do it\n")) if got := planFindings(t, root, "narrow"); !contains(got, "plan.kickoff-invalid") { t.Errorf("rules = %v, want plan.kickoff-invalid for an undocumented language", got) } @@ -134,7 +225,7 @@ func TestPlanKickoffAnswers(t *testing.T) { func TestPlanLoopAnswers(t *testing.T) { root := t.TempDir() writePlan(t, root, "sweep", - "---\nworktree: yes\nmerge: whenever\npr: sometimes\n---\n\n# Sweep\n\n- [ ] 1.1 (Unit) Do it\n") + plan("---\nworktree: yes\nmerge: whenever\npr: sometimes\n---\n\n", "- [ ] 1.1 (Unit) Do it\n")) got := planFindings(t, root, "sweep") if n := count(got, "plan.loop-invalid"); n != 3 { t.Errorf("rules = %v, want three plan.loop-invalid findings, got %d", got, n) @@ -148,8 +239,7 @@ func TestPlanLoopAnswers(t *testing.T) { func TestPlanPRShapeAcceptsBothLoops(t *testing.T) { root := t.TempDir() for _, shape := range []string{"per-group", "per-plan"} { - writePlan(t, root, "sweep", - "---\npr: "+shape+"\n---\n\n# Sweep\n\n- [ ] 1.1 (Unit) Do it\n") + writePlan(t, root, "sweep", plan("---\npr: "+shape+"\n---\n\n", "- [ ] 1.1 (Unit) Do it\n")) if got := planFindings(t, root, "sweep"); contains(got, "plan.loop-invalid") { t.Errorf("pr: %s reported %v", shape, got) } @@ -161,12 +251,13 @@ func TestPlanPRShapeAcceptsBothLoops(t *testing.T) { // 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") + writePlan(t, root, "sweep", plan("---\nautonomy: auto\nci: wait\n---\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\npr: per-plan\nworktree: per-group\nmerge: auto\n---\n\n# Run\n\n- [ ] 1.1 (Unit) Do it\n") + plan("---\nautonomy: auto\nci: wait\npr: per-plan\nworktree: per-group\nmerge: auto\n---\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) } @@ -175,8 +266,8 @@ func TestPlanLoopAnswersAreOptional(t *testing.T) { // A spec path inside a fenced block is an example, not a reference. func TestPlanExamplesAreNotReferences(t *testing.T) { root := t.TempDir() - writePlan(t, root, "documented", "# Documented\n\n- [ ] 1.1 (Unit) Real work\n\n"+ - "```\n- specs/an-example/ — from the docs\n```\n\n\n") + writePlan(t, root, "documented", plan("", "- [ ] 1.1 (Unit) Real work\n\n"+ + "```\n- specs/an-example/ — from the docs\n```\n\n\n")) if got := planFindings(t, root, "documented"); len(got) != 0 { t.Errorf("examples were treated as references: %v", got) } @@ -184,8 +275,8 @@ func TestPlanExamplesAreNotReferences(t *testing.T) { func TestPlansValidatesEveryPlan(t *testing.T) { root := t.TempDir() - writePlan(t, root, "zebra", "# Zebra\n\n- [ ] 1.1 No methodology\n") - writePlan(t, root, "alpha", "# Alpha\n\n- [ ] 1.1 No methodology\n") + writePlan(t, root, "zebra", plan("", "- [ ] 1.1 No methodology\n")) + writePlan(t, root, "alpha", plan("", "- [ ] 1.1 No methodology\n")) set, err := Plans(root) if err != nil { t.Fatalf("Plans: %v", err) diff --git a/internal/validate/tasks.go b/internal/validate/tasks.go index 5114fa6..00eb560 100644 --- a/internal/validate/tasks.go +++ b/internal/validate/tasks.go @@ -1,6 +1,8 @@ package validate import ( + "strings" + "github.com/protonspy/spec-claude-code/internal/artifact" "github.com/protonspy/spec-claude-code/internal/finding" "github.com/protonspy/spec-claude-code/internal/mdscan" @@ -59,5 +61,98 @@ func parseTasks(set *finding.Set, file string, doc *mdscan.Document, citations b artifact.CitationSeparator) } } + checkFlags(set, file, tasks) return tasks } + +// The values that would make `_Status_` a second record of what the box already +// says. They are called out separately from any other bad value because the mistake +// is a different one: not a typo, but two places recording one fact — the defect +// `plan.item-has-two-records` exists to catch, arriving by another door. +var boxStates = map[string]bool{ + "open": true, "todo": true, "done": true, "completed": true, + "complete": true, "in-progress": true, "wip": true, +} + +// checkFlags turns what the parser found about a task's annotation lines into +// findings, and then checks the graph those annotations describe. +// +// The graph checks are the ones that earn their place: a dependency on a task that +// does not exist, or on one that was struck out, is a deadlock that `--next` cannot +// explain and a loop cannot escape — it simply reports nothing to do while open work +// remains. +func checkFlags(set *finding.Set, file string, tasks []artifact.Task) { + byNumber := map[string]artifact.Task{} + for _, t := range tasks { + if _, seen := byNumber[t.Number]; !seen && t.Number != "" { + byNumber[t.Number] = t + } + } + + for _, t := range tasks { + for _, f := range t.UnknownFlags { + set.Addf(file, f.Line, "task.unknown-flag", + "`_%s_` is not one of the four flags a task carries: %s", + f.Name, strings.Join(artifact.FlagNames, ", ")) + } + seen := map[string]int{} + for _, f := range t.Flags { + if prior, dup := seen[f.Name]; dup { + set.Addf(file, f.Line, "task.duplicate-flag", + "`_%s_` is already on line %d; a task carries at most one of each", f.Name, prior) + continue + } + seen[f.Name] = f.Line + } + + if t.BadPriority != "" { + set.Addf(file, t.Line, "task.invalid-priority", + "`_Priority %s_` is not a whole number 1 or greater — lower is more urgent", t.BadPriority) + } + if t.BadStatus != "" { + if boxStates[strings.ToLower(strings.TrimSpace(t.BadStatus))] { + set.Addf(file, t.Line, "task.status-duplicates-box", + "`_Status %s_` restates the checkbox; the box is the state, and `_Status_` only ever says `%s`", + t.BadStatus, artifact.StatusRemoved) + } else { + set.Addf(file, t.Line, "task.invalid-status", + "`_Status %s_` is not a status: the only one is `%s`", t.BadStatus, artifact.StatusRemoved) + } + } + if t.Removed() { + if t.Reason == "" { + set.Addf(file, t.Line, "task.removed-without-reason", + "a removed task carries `_Reason …_`: the reason is the whole record of why the work went away") + } + if t.Checked { + set.Addf(file, t.Line, "task.removed-but-checked", + "task %s is both removed and ticked; it was either done or it was not", t.Number) + } + } + + for _, d := range t.Depends { + switch dep, ok := byNumber[d]; { + case d == t.Number: + set.Addf(file, t.Line, "task.self-dependency", + "task %s depends on itself, so it can never start", t.Number) + case !ok: + set.Addf(file, t.Line, "task.unknown-dependency", + "task %s depends on %s, which this file does not have", t.Number, d) + case dep.Removed(): + set.Addf(file, t.Line, "task.depends-on-removed", + "task %s depends on %s, which was removed — it will never be ticked, so %s can never start", + t.Number, d, t.Number) + } + } + } + + for _, cycle := range artifact.Cycles(tasks) { + at := 0 + if t, ok := byNumber[cycle[0]]; ok { + at = t.Line + } + set.Addf(file, at, "task.dependency-cycle", + "these tasks wait on each other and none of them can start: %s", + strings.Join(append(append([]string{}, cycle...), cycle[0]), " → ")) + } +} From ac203ffc2dd11b24b20f92799ee57ed74b68ac4c Mon Sep 17 00:00:00 2001 From: prode Date: Wed, 5 Aug 2026 01:31:14 -0300 Subject: [PATCH 2/2] fix(artifact): drop joinLines, which joinLinesExcept replaced The flag-aware version took over every call site, so the wrapper was dead the moment it was written. Caught by golangci-lint's `unused`, which does not run on the machine this was written on. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DmGyL6QvamMBKdHYyrx7Uq --- internal/artifact/parse.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/internal/artifact/parse.go b/internal/artifact/parse.go index daa7824..a990aaf 100644 --- a/internal/artifact/parse.go +++ b/internal/artifact/parse.go @@ -344,15 +344,11 @@ func indentOf(s string) int { return n } -// joinLines collapses a line range into one whitespace-normalized string. -func joinLines(doc *mdscan.Document, from, to int) string { - return joinLinesExcept(doc, from, to, nil) -} - -// joinLinesExcept is joinLines with a set of lines held back — a task's flags, which -// belong to its region but not to its description. Keeping `_Priority 2_` out of -// Detail is what stops the searcher from indexing it as prose and keeps `--width` -// honest about how much of the description it clipped. +// joinLinesExcept collapses a line range into one whitespace-normalized string, with +// a set of lines held back — a task's flags, which belong to its region but not to +// its description. Keeping `_Priority 2_` out of Detail is what stops the searcher +// from indexing it as prose and keeps `--width` honest about how much of the +// description it clipped. A nil set is the plain join. func joinLinesExcept(doc *mdscan.Document, from, to int, skip map[int]bool) string { if from > to { return ""