diff --git a/CLAUDE.md b/CLAUDE.md index 322828f..8f575db 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -110,6 +110,14 @@ These landed after phase 10, and all are documented in `design/orchestration.md` **`@path` is the stale check** — the notes half of the codewiki citation rule, since a note about code that no longer exists is read as current, which is worse than the comment it replaced: that one at least died with the file. At *write* time it is a warning and never a block, because a note about a file this branch has not created yet is the note most worth having; `notes add` otherwise writes under the same verify-and-roll-back contract as `scc patch`. The ninth validator also reports the failure this file cannot tolerate quietly: a hand-written line that missed the grammar, which no query will ever return. +- **The spec records where it is being built: `scc spec track` and `scc spec sync`.** A branch was the one part of this methodology that left no trace in the artifacts — the spec said which boxes were ticked, git said a branch had been unmerged for three weeks, and nothing joined the two, so *which of these actually shipped* was answerable only by somebody holding both halves. Under `autonomy: auto` that is nobody. Three keys on `requirements.md` beside the kickoff answers — `branch:`, `pr:`, `delivery: in-progress|in-review|merged|abandoned` — with the vocabulary closed for the reason a task's flags are, and graded by the validator only when present, so every spec written before this keeps passing. + + **`track` records what the caller knows; `sync` derives what git knows.** `--here` takes the branch from the checkout, `--pr ` the pull request, and `scc spec sync` walks every spec, asks git and (when installed) `gh`, and writes the answer back under the same verify-and-roll-back contract as `scc patch`. `scc spec list` shows the record beside the phases. **Neither guesses**: a deleted branch with no PR to ask about is reported undetermined and left alone, because merged and abandoned are indistinguishable once the ref is gone. + + Two things in `sync` were wrong in the first cut and are worth keeping wrong-proof. **Merged is not "is an ancestor of the base"** — a branch created ten seconds ago satisfies that trivially, and the first run declared a spec delivered before a line of it existed; it is *ahead == 0 and behind > 0*, and the fast-forward case that no ref can resolve is called **not** merged, because this record exists to surface unfinished work. And **a settled record is not re-litigated**: a deleted branch on a spec already `merged` is what a merged branch looks like, and warning about it would put a line on every finished spec forever. + + **Plans are deliberately out for now**, for a naming reason rather than a principle: `pr:` on a plan already means the delivery *shape* plan-run asks for at kickoff (`per-plan`/`per-group`), so one key would carry two meanings on one file. Plan tracking starts by renaming that answer. + - **The five seeded `docs/` anchors** (`assets.Seeds`). `init` writes `glossary.md`, `stack.md`, `notes.md`, `wiki/index.md`, and `wiki/changelog.md` — the knowledge base's only fixed-name documents, each holding the format its validator checks. A seed is written once and tracked nowhere: not in the manifest, not by `scc update`. `scc` is a redesign of `csdd` (`github.com/protonspy/csdd`), narrowed to spec-driven development and deliberately leaner. When reaching for something from there, port the *decision*, not the file. Already decided against: a TUI, an embedded web dashboard, an MCP server, a devcontainer. @@ -156,7 +164,7 @@ cmd/scc/main.go os.Exit(cli.Run(os.Args[1:])) plain files on disk: / · specs/ · plans/ · docs/ · CLAUDE.md|AGENTS.md ``` -Three packages sit off to the side of that tree — `rtk`, `headroom`, `codegraph` — reached only from `internal/cli`. They are the third-party integrations, and they are the only code that starts another process. +Four packages sit off to the side of that tree — `rtk`, `headroom`, `codegraph`, `git` — reached only from `internal/cli`. They are the third-party integrations, and they are the only code that starts another process. `git` is the one scc never installs and never writes with: every call in it is a query. `internal/cli/cli.go` is the whole dispatcher: `Run(args)` switches on `args[0]` and hands off to `run` in a file named for that resource. Each handler owns its own `flag.FlagSet`. Adding a subcommand means adding a case there plus one file — nothing is registered dynamically, so the command set is readable in one place. @@ -178,10 +186,11 @@ Three packages sit off to the side of that tree — `rtk`, `headroom`, `codegrap | `internal/validate` | The nine 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 (`rtk.Markers`, spliced by `internal/mdblock`), the foreign-block detection that names Headroom's copy, and finding or `cargo install`ing the binary. | | `internal/headroom` | Headroom's agent-slug table, the `wrap` argument vector, the MCP opt-out discovered from `wrap --help`, and finding or installing the binary (uv, then pip — never npm, which ships the SDK and no CLI). The slugs live here rather than on `paths.Harness` because they are Headroom's vocabulary, not scc's layout. | +| `internal/git` | `git` and `gh`, read-only: does this branch still exist, has it landed, and what did the forge do with the pull request. Two binaries in one package because the question has two halves and no caller should have to route between them — and nothing here installs or writes, so running it over every spec in a workspace is safe by construction. | | `internal/codegraph` | CodeGraph's argument vectors (`init`/`sync`/`index`/`status`/`query`/`explore`), the `.codegraph/` presence test, `codegraph.Markers` for the usage block `scc launch` splices, and finding or `npm install -g`ing the binary. Composes command lines and reads nothing inside the graph — the database is CodeGraph's schema on CodeGraph's schedule. | | `internal/cli` | The dispatcher and every command handler. | -`internal/rtk`, `internal/headroom`, and `internal/codegraph` are the only packages that shell out to another program. Keep that boundary there rather than in a command handler: a third party's binary name, install command, and argument vocabulary all age on that third party's schedule, and one package per integration is what keeps a version bump from touching the dispatcher. Headroom's renamed MCP flag is the worked example — the fix stayed inside `internal/headroom`, and nothing else in the tree knows the flag exists. +`internal/rtk`, `internal/headroom`, `internal/codegraph`, and `internal/git` are the only packages that shell out to another program. Keep that boundary there rather than in a command handler: a third party's binary name, install command, and argument vocabulary all age on that third party's schedule, and one package per integration is what keeps a version bump from touching the dispatcher. Headroom's renamed MCP flag is the worked example — the fix stayed inside `internal/headroom`, and nothing else in the tree knows the flag exists. `go.mod` is stdlib-only. Keep it that way unless a dependency earns its place — the binary is distributed to six platforms and every dep is a supply-chain surface. @@ -239,3 +248,5 @@ Tests live beside the code and lean on a few package-local helpers rather than a ## Commits Conventional Commits, scoped by package or surface, with a descriptive subject written as a claim about behavior — e.g. `feat(cli): return exit 2 when spec validation reports findings`. Changes land through PRs on `main`. + +**No attribution, in a commit message or a PR body.** No `Co-Authored-By` for an assistant, no session link, no generated-with footer, no naming of a model, vendor, or harness. This is the rule scc ships in `rules/delivery.md`, and it binds scc's own history first — a tool that told every workspace not to sign its work and then signed its own would be worth ignoring on both counts. Harness defaults append these unless told not to, so a session working here overrides that default rather than following it. diff --git a/design/orchestration.md b/design/orchestration.md index ecbd6df..6be916e 100644 --- a/design/orchestration.md +++ b/design/orchestration.md @@ -881,8 +881,86 @@ Once the last task is done: spending their attention on findings a subagent would have caught is the waste this ordering avoids. One fix-and-re-review round; a second means the finding wants a person. -3. **Commit and push** — Conventional Commits, generated from the diff and the spec. -4. **Open the PR.** +3. **Commit and push** — Conventional Commits, generated from the diff and the spec, + and carrying no attribution (below). +4. **Open the PR**, and record it on the spec: `scc spec track --pr `. + +### The record carries no attribution + +A commit message and a PR body say what changed and why. They do not say what typed +it: no `Co-Authored-By` for an assistant, no session link, no generated-with footer, +no naming of a model, a vendor, or a harness — not in the message, not in the PR title +or body, not in a branch name. + +This is not modesty and it is not about hiding the tool. **The work is the user's.** A +trailer is a claim on authorship, and a tool that signs what it did for somebody is +taking a share of something it was hired to produce; the honest signature is the diff, +which is already in the record and already attributable by whoever cares to look. The +practical half matters too: these lines outlive the session that wrote them, they end +up in release notes, `git shortlog`, and every downstream fork, and a repository whose +history names a product from 2026 has dated itself for nothing. + +Harness defaults push the other way — several of them append a footer unless told not +to. So this is stated in the rule that owns delivery rather than left to the tool's +own configuration, and it is why `TestNoTemplateShipsAnAttributionFooter` exists: a +template that demonstrated a footer would teach the example, and the example is what +gets copied. + +### The spec records where it is being built + +A branch is the one part of this methodology that left no trace in the artifacts. The +spec said what the feature does and which boxes were ticked; git said a branch called +`feat/user-auth` had been sitting unmerged for three weeks. Nothing joined the two, so +*which of these actually shipped* was answerable only by somebody holding both halves — +and under `autonomy: auto` there is nobody holding either. + +Three keys on `requirements.md`, beside the kickoff answers, because that file is +already the spec's header: + +```yaml +branch: feat/user-auth +pr: 28 +delivery: in-progress | in-review | merged | abandoned +``` + +`scc spec track --here` writes the branch you are on, `--pr ` the pull +request; `scc spec sync` reads git and the forge back into every spec, and `scc spec +list` shows the result beside the phases. The vocabulary is closed for the same reason +a task's flags are: an open one produces three spellings of *done* inside a month, and +"what is still unfinished" is exactly the question a synonym destroys. Absent is the +fifth state and needs no name — it is what every spec written before this looks like, +and it is why the validator grades these only when they are present. + +**The split between the two commands is the one scc makes everywhere.** `track` records +what the caller knows and cannot get wrong. `sync` derives what git knows and can. And +neither one guesses: a branch that has been deleted, with no pull request to ask about, +is reported undetermined and left exactly as it was, because a merged branch and an +abandoned one are indistinguishable once the ref is gone. + +Two decisions inside `sync` are worth naming, because both were wrong in the first +version and the failures were instructive: + +- **Merged is not "is an ancestor of the base".** A branch created ten seconds ago and + never committed to satisfies that trivially, so the first run declared a spec + delivered before a line of it existed. Merged is *ahead == 0 and behind > 0*: + everything the branch had is on the base, and the base has moved on since. The one + case left ambiguous is a fast-forward merge nothing has advanced past, where no ref + can tell "just branched" from "just landed" — resolved as **not** merged, because + this record exists to surface unfinished work and the error that leaves a loose end + visible is the one to make. +- **A settled record stops being re-litigated.** A deleted branch on a spec already + marked `merged` is not an ambiguity worth reporting: it is what a merged branch is + supposed to look like, and warning about it would put a line on every finished spec + forever, which is how a report stops being read. + +`gh` is the only thing that can tell merged from abandoned once the branch is gone, and +it is optional like every other binary scc talks to: without it those rows say so, once +per run rather than once per spec. + +**Plans are deliberately not tracked this way yet**, and the reason is a name rather +than a principle: `pr:` on a plan already means the delivery *shape* (`per-plan` / +`per-group`) that plan-run asks for at kickoff, so one key would carry two meanings on +one file. The day plan tracking is wanted, that answer gets renamed first. ### Waiting for CI is a question, not a policy diff --git a/internal/artifact/delivery.go b/internal/artifact/delivery.go new file mode 100644 index 0000000..f70852b --- /dev/null +++ b/internal/artifact/delivery.go @@ -0,0 +1,107 @@ +package artifact + +import "strconv" + +// The delivery record: three frontmatter keys that say where a spec is being built +// and how far that has got. +// +// It exists because a branch is the one part of this methodology that leaves no trace +// in the artifacts. A spec says what the feature does and which tasks are ticked; git +// says a branch called `feat/user-auth` has been sitting unmerged for three weeks. +// Nothing joined the two, so "which specs are actually finished" was a question only a +// person holding both halves in their head could answer — and under `autonomy: auto` +// there is no such person. These keys are the join, and `scc spec sync` is what keeps +// them true without anybody remembering to. +// +// They live on `requirements.md`, with the kickoff answers, because that is already +// the spec's header: one file to read for everything about the spec that is not a +// requirement, a design, or a task. +const ( + // KeyBranch is the branch the work is on, as git spells it. + KeyBranch = "branch" + // KeyPR is the pull request's number, digits only. A number rather than a URL + // because it is what `gh pr view` takes and what a person says out loud; the URL + // is reconstructible and the number is not. + KeyPR = "pr" + // KeyDelivery is how far the work has got, from DeliveryStates. + // + // Deliberately not `status:`, which already means the approval seal on a plan + // (see KeyStatus). One key with two meanings across two artifacts is the defect + // this vocabulary exists to avoid, and `delivery:` names its own concern — + // the rule that owns it is delivery.md. + KeyDelivery = "delivery" +) + +// The delivery states, and the whole set of them. +// +// Four, and the set is closed for the same reason a task's flags are: an open +// vocabulary here would produce three spellings of "done" inside a month, and the +// question this record answers — what is still unfinished — is exactly the one a +// synonym destroys. +// +// Absent is the fifth state and needs no name: a spec nobody has started carries no +// branch, no PR, and no delivery line, which is also what every spec written before +// this shipped looks like. +const ( + // DeliveryInProgress — a branch exists and has not landed. + DeliveryInProgress = "in-progress" + // DeliveryInReview — a pull request is open. + DeliveryInReview = "in-review" + // DeliveryMerged — the work is on the base branch. Terminal, and the reason the + // branch and PR are kept rather than cleared: the spec then permanently records + // what delivered it. + DeliveryMerged = "merged" + // DeliveryAbandoned — the pull request was closed unmerged, or the branch was + // dropped. Terminal, and it has to be sayable: a spec that was tried and dropped + // is not the same as one nobody started, and only one of the two is a loose end. + DeliveryAbandoned = "abandoned" +) + +// DeliveryStates returns the vocabulary, in the order work moves through it. +func DeliveryStates() []string { + return []string{DeliveryInProgress, DeliveryInReview, DeliveryMerged, DeliveryAbandoned} +} + +// ValidDelivery reports whether s is one of them. +func ValidDelivery(s string) bool { + for _, v := range DeliveryStates() { + if s == v { + return true + } + } + return false +} + +// Settled reports whether a state is terminal — the work is not coming back. It is +// what separates "still open" from "done with", which is the only distinction a +// reader scanning for loose ends actually makes. +func Settled(state string) bool { + return state == DeliveryMerged || state == DeliveryAbandoned +} + +// Delivery is the record as one value. +type Delivery struct { + Branch string `json:"branch,omitempty"` + // PR is 0 when there is none. A pointer would distinguish "absent" from "zero", + // and there is no pull request number zero, so it would only buy a nil check at + // every use. + PR int `json:"pr,omitempty"` + State string `json:"delivery,omitempty"` +} + +// Tracked reports whether anything has been recorded at all. An untracked spec is not +// a defect — it is a spec nobody has started, or one that predates the record. +func (d Delivery) Tracked() bool { return d.Branch != "" || d.PR != 0 || d.State != "" } + +// ReadDelivery pulls the record out of a parsed frontmatter map. +// +// A `pr:` that is not a number comes back as 0 rather than as an error: reading is +// not where a malformed value gets reported, the validator is, and a reader that +// failed here would take out `scc spec list` for the whole workspace over one typo. +func ReadDelivery(fm map[string]string) Delivery { + d := Delivery{Branch: fm[KeyBranch], State: fm[KeyDelivery]} + if n, err := strconv.Atoi(fm[KeyPR]); err == nil && n > 0 { + d.PR = n + } + return d +} diff --git a/internal/assets/assets.go b/internal/assets/assets.go index 150ed6b..b550157 100644 --- a/internal/assets/assets.go +++ b/internal/assets/assets.go @@ -146,7 +146,22 @@ import ( // rule of its own because its trigger is a keystroke — you are about to type a // comment that is not a docstring — and prior-art.md gains the read side, since this // is now the one corpus under docs/ that does have an index. -const Version = "19" +// 20: delivery.md — the commit and the PR carry no attribution. No `Co-Authored-By` +// for an assistant, no session link, no generated-with footer, no naming of a model, +// vendor or harness, in a message or a PR body or a branch name. The work is the +// user's: a tool that signs what it did for somebody is claiming a share of it, and +// the honest signature is the diff. It lands in delivery.md at no cost to the budget — +// the opening paragraph's fenced one-liner goes inline, which pays for the paragraph +// exactly. +// 21: delivery.md — the spec records where it is being built. A branch was the one +// part of this methodology that left no trace in the artifacts: the spec said which +// boxes were ticked and git said a branch had been unmerged for three weeks, and +// nothing joined the two, so "which of these actually shipped" was answerable only by +// somebody holding both halves — which under `autonomy: auto` is nobody. `scc spec +// track --here` records the branch and `--pr` the pull request; `scc spec sync` reads +// git and the forge back into every spec. The rule pays for the paragraph by +// tightening §8's argument, which the design doc holds in full anyway. +const Version = "21" // The embedded tree. "all:" so nothing is silently dropped for having a name the // default embed pattern skips. diff --git a/internal/assets/assets_test.go b/internal/assets/assets_test.go index c818138..778f382 100644 --- a/internal/assets/assets_test.go +++ b/internal/assets/assets_test.go @@ -882,3 +882,46 @@ func descriptionColumn(line string) int { } return -1 } + +// No template scc ships may carry an attribution footer, because delivery.md tells the +// agent never to write one. A template that demonstrated the thing the rule forbids +// would teach the example and not the rule — and an example is what gets copied. +// +// `Co-Authored-By` is allowed in exactly one place: the sentence that forbids it. +func TestNoTemplateShipsAnAttributionFooter(t *testing.T) { + banned := []string{ + "Generated with [Claude Code]", + "claude.ai/code/session", + "noreply@anthropic.com", + "Co-authored-by:", + "Co-Authored-By:", + } + deliveryRule := paths.Claude.RulesSeg + "/delivery.md" + + for _, h := range paths.Harnesses() { + for _, f := range Workspace(h) { + raw, err := Render(h, f) + if err != nil { + t.Fatalf("%s %s: %v", h.ID, f.Rel, err) + } + for _, b := range banned { + if !strings.Contains(raw, b) { + continue + } + t.Errorf("%s: %s contains %q — the rule forbids attribution, so no template may model it", + h.ID, f.Rel, b) + } + } + } + + // And the prohibition itself is actually there, in the rule that owns delivery. + rule, err := Content("rules/delivery.md") + if err != nil { + t.Fatalf("delivery rule: %v", err) + } + for _, want := range []string{"Co-Authored-By", "harness"} { + if !strings.Contains(rule, want) { + t.Errorf("%s never says %q; the no-attribution rule has gone missing", deliveryRule, want) + } + } +} diff --git a/internal/assets/templates/rules/delivery.md b/internal/assets/templates/rules/delivery.md index 2a8dd90..30f4697 100644 --- a/internal/assets/templates/rules/delivery.md +++ b/internal/assets/templates/rules/delivery.md @@ -1,35 +1,26 @@ # Delivery — branch, PR Work does not happen on `main` and does not end with a green test run. It ends with a -pull request. Each unit of work gets its own branch, in the checkout you are in: - -``` -git switch -c / -``` - -Branch from a green `main`, and leave the checkout back on `main` and clean once the -work lands. Nothing here needs a second directory: how a user runs several sessions -against one repo at once is theirs to set up. +pull request. Each unit of work gets its own branch in the checkout you are in — +`git switch -c /`, from a green `main` — and the checkout goes back to +`main`, clean, once the work lands. Nothing here needs a second directory: how a user +runs several sessions against one repo at once is theirs to set up. ## Implementation is sequential — you write the code -**There is no implementation subagent and no parallel task dispatch.** Designed the -other way first, and rejected: - -- Delegating implementation puts the cheaper model on the hardest work while the - orchestrator keeps the part needing the least capability. That is backwards. -- Every fresh agent re-pays for discovery. Within one spec your accumulated context is - the asset: you use the right parser in 1.2 because you wrote 1.1. -- **File-disjointness is not independence, and a clean merge hides the difference.** - Two tasks touching no common file both need a `Money` type that does not exist yet. - Each invents its own, with different semantics, and the merge is clean. Sequential - execution cannot produce this: the later task sees the earlier task's code. +**There is no implementation subagent and no parallel task dispatch.** It puts the +cheaper model on the hardest work, and every fresh agent re-pays for discovery — within +one spec your accumulated context is the asset: you use the right parser in 1.2 because +you wrote 1.1. Above all, **file-disjointness is not independence, and a clean merge +hides the difference**: two tasks touching no common file both need a `Money` type that +does not exist yet, each invents its own with different semantics, and the merge is +clean. Sequential execution cannot produce that — the later task sees the earlier one's +code. Feature-level parallelism has none of that and is supported — a *human* picks the split -and each session has full context. Separate sessions isolate files, not the world: -suites fighting over a fixed port or one test database must be namespaced or -serialized, and two features green separately can still break together, which only CI -on `main` sees. +and each session has full context. Separate sessions isolate files, not the world: a +suite fighting over a fixed port or one test database must be namespaced or serialized, +and two features green separately can still break together, which only CI on `main` sees. ## The delivery sequence @@ -49,6 +40,15 @@ Once the last task is done: 4. **Commit and push.** Conventional Commits, written from the diff and the spec. 5. **Open the PR.** Body: what changed, which spec or plan, how it was verified. +**The work is the user's, and the record says so.** No `Co-Authored-By` for an assistant, +no session link, no "generated with" footer, no naming of a model, vendor, or harness — +not in a commit message, not in a PR title or body, not in a branch name. Say what +changed and why; what typed it is not part of the record. + +**A branch leaves no trace in the artifacts, so record it.** `scc spec track +--here` when you branch, `--pr ` when the PR opens; `scc spec sync` reads git and the +forge back into every spec, so *unfinished* is a state the workspace reports. + Then CI, using the `ci:` answer from kickoff ([autonomy.md](autonomy.md)) — do not ask now. **`wait`** means watch the checks until they settle, fixing and pushing while they are red: the work is not finished while CI is failing. **`no-wait`** means opening the diff --git a/internal/assets/templates/skills/plan-run/SKILL.md b/internal/assets/templates/skills/plan-run/SKILL.md index 032b02e..1643936 100644 --- a/internal/assets/templates/skills/plan-run/SKILL.md +++ b/internal/assets/templates/skills/plan-run/SKILL.md @@ -131,7 +131,10 @@ you stop to deliver. 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. + validate`, both review subagents, commit, push, open the PR. Then record it on every + spec the group delivered: `scc spec track --here --pr `. A plan run + opens more branches than anything else here, so it is the loop where an unrecorded + one is most likely to be the branch nobody comes back to. 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 diff --git a/internal/cli/spec.go b/internal/cli/spec.go index 770931f..3ea8f91 100644 --- a/internal/cli/spec.go +++ b/internal/cli/spec.go @@ -7,7 +7,9 @@ import ( "os" "path/filepath" "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" @@ -33,6 +35,10 @@ func runSpec(args []string) int { return runSpecShow(args[1:]) case "delete": return runSpecDelete(args[1:]) + case "track": + return runSpecTrack(args[1:]) + case "sync": + return runSpecSync(args[1:]) case "validate": return runSpecValidate(args[1:]) case "help", "-h", "--help": @@ -156,6 +162,10 @@ type specEntry struct { Tasks bool `json:"tasks"` Autonomy string `json:"autonomy,omitempty"` CI string `json:"ci,omitempty"` + // Delivery is where this spec is being built and how far that has got. It is + // listed beside the phases because they answer one question between them: the + // phases say whether the spec is written, this says whether the work shipped. + Delivery artifact.Delivery `json:"delivery,omitempty"` } func runSpecList(args []string) int { @@ -200,11 +210,31 @@ func runSpecList(args []string) int { return ExitOK } for _, s := range specs { - render.Info(fmt.Sprintf("%s %s", s.Name, phases(s))) + render.Info(fmt.Sprintf("%s %s%s", s.Name, phases(s), deliveryNote(s.Delivery))) } return ExitOK } +// deliveryNote is the record on one line, or nothing at all when there is none. A +// spec nobody has started should not have to say so on every listing — the absence is +// already the answer, and a column of "not started" is a column nobody reads. +func deliveryNote(d artifact.Delivery) string { + if !d.Tracked() { + return "" + } + parts := []string{} + if d.State != "" { + parts = append(parts, d.State) + } + if d.Branch != "" { + parts = append(parts, d.Branch) + } + if d.PR != 0 { + parts = append(parts, fmt.Sprintf("#%d", d.PR)) + } + return " " + strings.Join(parts, " · ") +} + func runSpecShow(args []string) int { fs := flag.NewFlagSet("spec show", flag.ContinueOnError) fs.SetOutput(os.Stderr) @@ -233,6 +263,12 @@ func runSpecShow(args []string) int { } render.Info(render.Bold(s.Name) + " " + s.Path) render.Info("phases: " + phases(s)) + if s.Delivery.Tracked() { + render.Info("delivery:" + deliveryNote(s.Delivery)) + } else { + render.Info(fmt.Sprintf("delivery: not recorded — `%s spec track %s --here` once you branch", + prog(), s.Name)) + } if s.Autonomy != "" || s.CI != "" { render.Info(fmt.Sprintf("kickoff: autonomy=%s ci=%s", orUnset(s.Autonomy), orUnset(s.CI))) } else { @@ -300,6 +336,7 @@ func describeSpec(root, name string) specEntry { if fm, err := mdscan.ParseFrontmatter(string(b)); err == nil { s.Autonomy, _ = fm.Get("autonomy") s.CI, _ = fm.Get("ci") + s.Delivery = artifact.ReadDelivery(fm.Values) } } return s @@ -341,9 +378,22 @@ func specUsage() { %s spec list %s spec show %s spec delete --force + %s spec track [--here|--branch ] [--pr ] [--delivery ] + %s spec sync […] [--base ] [--dry-run] %s spec validate [] A spec is for work whose *what* and *how* need settling before code: specs//requirements.md, design.md, tasks.md. Everything else is a plan. -`, prog(), prog(), prog(), prog(), prog()) + +The delivery record — %s: · %s: · %s: — says where a spec is being built and how far +that has got, in requirements.md beside the kickoff answers. "track" writes what you +know the moment you know it; "sync" asks git and the forge and writes back what they +say, so a branch cannot go quietly unfinished. Nothing is guessed: a deleted branch +with no PR to ask about is reported undetermined and left alone, because merged and +abandoned look identical once a branch is gone. + + %s: %s +`, prog(), prog(), prog(), prog(), prog(), prog(), prog(), + artifact.KeyBranch, artifact.KeyPR, artifact.KeyDelivery, + artifact.KeyDelivery, strings.Join(artifact.DeliveryStates(), " · ")) } diff --git a/internal/cli/track.go b/internal/cli/track.go new file mode 100644 index 0000000..6852a7b --- /dev/null +++ b/internal/cli/track.go @@ -0,0 +1,528 @@ +package cli + +import ( + "errors" + "flag" + "fmt" + "os" + "sort" + "strconv" + "strings" + + "github.com/protonspy/spec-claude-code/internal/artifact" + "github.com/protonspy/spec-claude-code/internal/finding" + "github.com/protonspy/spec-claude-code/internal/git" + "github.com/protonspy/spec-claude-code/internal/paths" + "github.com/protonspy/spec-claude-code/internal/render" + "github.com/protonspy/spec-claude-code/internal/workspace" +) + +// This file is the delivery record: `scc spec track` writes it and `scc spec sync` +// reconciles it with git. +// +// The problem it closes is that a branch leaves no trace in the artifacts. A spec says +// what the feature does and which boxes are ticked; git says a branch has been sitting +// unmerged for three weeks. Nothing joined the two, so "which of these is actually +// finished" was answerable only by a person holding both halves — and under +// `autonomy: auto` nobody is holding either. +// +// The split between the two commands is the one the rest of scc makes: `track` records +// what the caller knows and cannot be got wrong, `sync` derives what git knows and can. +// Neither guesses. A branch that no longer exists, with no pull request to ask about, +// is reported as undetermined and left exactly as it was — because a merged branch and +// an abandoned one look identical once they are deleted, and writing either one would +// be inventing the answer this record exists to stop people inventing. + +// runSpecTrack records where a spec is being built. Every field is optional and each +// is written only when passed, so `--pr 28` after the PR opens does not have to +// restate the branch. +func runSpecTrack(args []string) int { + fs := flag.NewFlagSet("spec track", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + root := addRoot(fs) + branch := fs.String("branch", "", "the `branch` this spec is being built on") + pr := fs.Int("pr", 0, "the pull request's `number`") + state := fs.String("delivery", "", "how far it has got: `"+strings.Join(artifact.DeliveryStates(), "|")+"`") + here := fs.Bool("here", false, "take the branch from the checkout you are in") + dry := fs.Bool("dry-run", false, "show what would be written, and write nothing") + jsonOut := addJSON(fs) + rest, err := parseFlags(fs, args) + if err != nil { + return ExitError + } + name, ok := artifactName(rest, "spec") + if !ok { + return ExitError + } + target, ok := resolveRoot(*root) + if !ok || !requireWorkspace(target) { + return ExitError + } + + pairs := [][2]string{} + if *here { + // The branch you are on is the branch the work is on, and asking scc for it + // beats retyping it — a mistyped branch name is a record that points at + // nothing, which is worse than no record because it reads as one. + current, err := git.CurrentBranch(target) + if err != nil || current == "" { + render.Err("--here: could not read the current branch (no git, or a detached HEAD)") + return ExitError + } + pairs = append(pairs, [2]string{artifact.KeyBranch, current}) + } + if *branch != "" { + if strings.ContainsAny(*branch, " \t") { + render.Err(fmt.Sprintf("branch %q contains whitespace", *branch)) + return ExitError + } + pairs = append(pairs, [2]string{artifact.KeyBranch, *branch}) + } + if *pr != 0 { + if *pr < 0 { + render.Err("a pull request number is a positive whole number") + return ExitError + } + pairs = append(pairs, [2]string{artifact.KeyPR, strconv.Itoa(*pr)}) + } + if *state != "" { + if !artifact.ValidDelivery(*state) { + render.Err(fmt.Sprintf("--delivery %q is not one of %s", + *state, strings.Join(artifact.DeliveryStates(), ", "))) + return ExitError + } + pairs = append(pairs, [2]string{artifact.KeyDelivery, *state}) + } + if len(pairs) == 0 { + render.Err("nothing to record: pass --branch, --here, --pr, or --delivery") + render.Detail(fmt.Sprintf(" %s spec show %s shows what is recorded now", prog(), name)) + return ExitError + } + + // A branch is recorded because work has started on it, so the state follows from + // it unless the caller said otherwise. Inferring the obvious here is what keeps + // the common call one flag long; anything less obvious is `sync`'s job. + if *state == "" && hasKey(pairs, artifact.KeyBranch) { + next := artifact.DeliveryInProgress + if *pr != 0 { + next = artifact.DeliveryInReview + } + pairs = append(pairs, [2]string{artifact.KeyDelivery, next}) + } else if *state == "" && *pr != 0 { + pairs = append(pairs, [2]string{artifact.KeyDelivery, artifact.DeliveryInReview}) + } + + res, err := writeDelivery(target, name, pairs, *dry) + if err != nil { + render.Err(err.Error()) + return ExitError + } + if *jsonOut { + code := ExitOK + if len(res.Introduced) > 0 { + code = ExitFindings + } + if emitJSON(res) != ExitOK { + return ExitError + } + return code + } + return reportWrite(res, *dry) +} + +func hasKey(pairs [][2]string, key string) bool { + for _, p := range pairs { + if p[0] == key { + return true + } + } + return false +} + +// writeResult is what one spec's write did, in the shape both renderers read. +type writeResult struct { + Spec string `json:"spec"` + Path string `json:"path"` + Changes []string `json:"changes"` + Written bool `json:"written"` + Verified string `json:"verified"` // clean | rolled-back | unchanged | not-run + Introduced []finding.Finding `json:"introduced,omitempty"` + Delivery artifact.Delivery `json:"record"` +} + +// writeDelivery sets the record on a spec's requirements.md, under the contract every +// scc write obeys: the file is re-validated afterwards and the write is undone when it +// introduced a finding the file did not already have. +// +// It goes through the artifact editor rather than through the frontmatter by hand, so +// a spec that has no frontmatter block at all gets one written correctly instead of +// growing a second. +func writeDelivery(root, name string, pairs [][2]string, dry bool) (writeResult, error) { + rel := paths.SpecsSeg + "/" + name + "/" + paths.RequirementsSeg + abs := paths.Requirements(root, name) + if !isFile(abs) { + return writeResult{}, fmt.Errorf("no spec %q: %s does not exist", name, rel) + } + // Load takes the absolute path; rel is what the report prints, so the same + // artifact reads the same way on every platform. + a, err := artifact.Load(root, abs) + if err != nil { + return writeResult{}, err + } + + e := a.Edit() + for _, p := range pairs { + e.SetFrontmatter(p[0], p[1]) + } + content, err := e.Content() + if err != nil { + return writeResult{}, err + } + res := writeResult{Spec: name, Path: rel, Verified: "unchanged"} + for _, p := range pairs { + res.Changes = append(res.Changes, p[0]+": "+p[1]) + } + if e.Empty() { + res.Delivery = artifact.ReadDelivery(a.Frontmatter) + return res, nil + } + if dry { + res.Verified = "not-run" + res.Delivery = plannedDelivery(a.Frontmatter, pairs) + return res, nil + } + + original, err := os.ReadFile(a.Abs) + if err != nil { + return writeResult{}, err + } + before, checkable := validateArtifact(root, a) + if err := workspace.AtomicWrite(a.Abs, []byte(withTrailingNewline(content)), 0o644); err != nil { + return writeResult{}, err + } + res.Written, res.Verified = true, "clean" + if !checkable { + res.Verified = "no-validator" + } else { + after, _ := validateArtifact(root, a) + if introduced := newFindings(before, after); len(introduced) > 0 { + res.Introduced = introduced + if writeErr := workspace.AtomicWrite(a.Abs, original, 0o644); writeErr != nil { + return res, fmt.Errorf("the edit introduced findings and the rollback failed: %w", writeErr) + } + res.Written, res.Verified = false, "rolled-back" + } + } + res.Delivery = plannedDelivery(a.Frontmatter, pairs) + if !res.Written { + res.Delivery = artifact.ReadDelivery(a.Frontmatter) + } + return res, nil +} + +// plannedDelivery is the record as it will read once these pairs land. +func plannedDelivery(fm map[string]string, pairs [][2]string) artifact.Delivery { + next := map[string]string{} + for k, v := range fm { + next[k] = v + } + for _, p := range pairs { + next[p[0]] = p[1] + } + return artifact.ReadDelivery(next) +} + +func reportWrite(res writeResult, dry bool) int { + switch { + case res.Verified == "unchanged": + render.Info(fmt.Sprintf("%s already records that — nothing to write", res.Spec)) + return ExitOK + case dry: + render.Info(fmt.Sprintf("--dry-run: %s would record %s", res.Path, strings.Join(res.Changes, " · "))) + return ExitOK + case res.Verified == "rolled-back": + render.Err(fmt.Sprintf("not written: the edit would introduce %d finding(s) in %s", + len(res.Introduced), res.Path)) + for _, f := range res.Introduced { + render.Detail(fmt.Sprintf(" %s %s", f.Rule, f.Message)) + } + return ExitFindings + } + render.OK(fmt.Sprintf("%s %s", res.Spec, strings.Join(res.Changes, " · "))) + return ExitOK +} + +// syncResult is one spec's reconciliation. +type syncResult struct { + Spec string `json:"spec"` + Was artifact.Delivery `json:"was"` + Now artifact.Delivery `json:"now"` + Changed bool `json:"changed"` + Written bool `json:"written"` + Why string `json:"why"` + Undecided string `json:"undecided,omitempty"` +} + +// runSpecSync reads git back into the specs: the command that makes the record true +// without anybody remembering to keep it true. +// +// With no feature named it walks every spec, which is the shape the question actually +// takes — "what is still open around here" is never asked about one spec. +func runSpecSync(args []string) int { + fs := flag.NewFlagSet("spec sync", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + root := addRoot(fs) + base := fs.String("base", "", "the `branch` work merges into (default: the repository's own)") + dry := fs.Bool("dry-run", false, "report what git says, and write nothing") + jsonOut := addJSON(fs) + rest, err := parseFlags(fs, args) + if err != nil { + return ExitError + } + target, ok := resolveRoot(*root) + if !ok || !requireWorkspace(target) { + return ExitError + } + if !git.Found(git.Bin) || !git.IsRepo(target) { + // Degrades rather than fails, the way every integration here does: a + // workspace outside a repository is a legitimate workspace, and the record + // still holds whatever `spec track` put in it. + render.Warn("no git repository here — nothing to sync against") + if *jsonOut { + return emitJSON(struct { + Specs []syncResult `json:"specs"` + Count int `json:"count"` + Git bool `json:"git"` + }{[]syncResult{}, 0, false}) + } + return ExitOK + } + + names, ok := syncTargets(target, rest) + if !ok { + return ExitError + } + if *base == "" { + *base = git.Base(target) + } + hasGH := git.Found(git.GHBin) + + results := []syncResult{} + for _, name := range names { + d, err := readSpecDelivery(target, name) + if err != nil { + render.Err(err.Error()) + return ExitError + } + if !d.Tracked() { + continue + } + r := reconcile(name, d, lookUp(target, d, *base, hasGH)) + if r.Changed && !*dry { + res, err := writeDelivery(target, name, + [][2]string{{artifact.KeyDelivery, r.Now.State}}, false) + if err != nil { + render.Err(err.Error()) + return ExitError + } + r.Written = res.Written + } + results = append(results, r) + } + + if *jsonOut { + return emitJSON(struct { + Specs []syncResult `json:"specs"` + Count int `json:"count"` + Base string `json:"base"` + GH bool `json:"gh"` + Git bool `json:"git"` + }{results, len(results), *base, hasGH, true}) + } + return reportSync(results, *base, hasGH, *dry) +} + +// syncTargets is the specs to reconcile: the ones named, or all of them. +func syncTargets(root string, rest []string) ([]string, bool) { + if len(rest) > 0 { + for _, name := range rest { + if err := workspace.SafeName(name, "spec"); err != nil { + render.Err(err.Error()) + return nil, false + } + } + return rest, true + } + entries, err := os.ReadDir(paths.Specs(root)) + if err != nil && !errors.Is(err, os.ErrNotExist) { + render.Err(err.Error()) + return nil, false + } + var names []string + for _, e := range entries { + if e.IsDir() { + names = append(names, e.Name()) + } + } + sort.Strings(names) + return names, true +} + +func readSpecDelivery(root, name string) (artifact.Delivery, error) { + abs := paths.Requirements(root, name) + if !isFile(abs) { + return artifact.Delivery{}, fmt.Errorf("no spec %q under %s", name, paths.SpecsSeg) + } + a, err := artifact.Load(root, abs) + if err != nil { + return artifact.Delivery{}, err + } + return artifact.ReadDelivery(a.Frontmatter), nil +} + +// evidence is what the forge and the repository said about one spec's work. +type evidence struct { + branch git.Branch + pr git.PR + askedPR bool + prErr error +} + +func lookUp(root string, d artifact.Delivery, base string, hasGH bool) evidence { + var ev evidence + if d.Branch != "" { + ev.branch, _ = git.Look(root, d.Branch, base) + } + if d.PR != 0 && hasGH { + ev.askedPR = true + ev.pr, ev.prErr = git.LookPR(root, d.PR) + } + return ev +} + +// reconcile is the whole policy, kept as one pure function so the rules can be read +// and tested without a repository. +// +// The order is by strength of evidence. A pull request states the outcome outright, so +// it wins whenever it answered. Git can only prove the positive — this branch's commits +// are on the base, so it landed — and the negative case is genuinely ambiguous: a +// branch that is gone is equally one deleted after a clean merge and one abandoned. +// That ambiguity is reported, never resolved by preference. +func reconcile(name string, was artifact.Delivery, ev evidence) syncResult { + r := syncResult{Spec: name, Was: was, Now: was} + next, why, undecided := "", "", "" + + switch { + case ev.askedPR && ev.prErr == nil: + switch ev.pr.State { + case git.StateMerged: + next, why = artifact.DeliveryMerged, fmt.Sprintf("PR #%d is merged", ev.pr.Number) + case git.StateClosed: + next, why = artifact.DeliveryAbandoned, fmt.Sprintf("PR #%d was closed unmerged", ev.pr.Number) + case git.StateOpen: + next, why = artifact.DeliveryInReview, fmt.Sprintf("PR #%d is open", ev.pr.Number) + default: + undecided = fmt.Sprintf("gh reported PR #%d as %q", ev.pr.Number, ev.pr.State) + } + case ev.askedPR: + undecided = fmt.Sprintf("gh could not read PR #%d", ev.pr.Number) + case was.PR != 0: + undecided = fmt.Sprintf("PR #%d is recorded and gh is not installed", was.PR) + } + + if next == "" && was.Branch != "" { + switch { + case ev.branch.Merged: + next, why = artifact.DeliveryMerged, + fmt.Sprintf("%s has landed on %s", was.Branch, ev.branch.Base) + case ev.branch.Exists(): + // A branch that exists and has not landed is in progress — unless a PR was + // already recorded, in which case in-review is the more specific truth and + // nothing here is evidence against it. + next = artifact.DeliveryInProgress + why = branchWhy(ev.branch) + if was.PR != 0 && was.State == artifact.DeliveryInReview { + next, why = was.State, why+", and a PR is recorded" + } + default: + undecided = fmt.Sprintf("%s is gone; merged and abandoned look the same once a branch is deleted", + was.Branch) + } + } + + if next != "" && next != was.State { + r.Now.State, r.Changed, r.Why = next, true, why + return r + } + if next != "" { + r.Why = why + } + // A settled record has nothing left to decide, so the ambiguity of a deleted + // branch is not worth reporting: that is what a merged branch is supposed to look + // like. Reporting it anyway would put a warning on every finished spec forever, + // which is how a report stops being read. + if !artifact.Settled(was.State) { + r.Undecided = undecided + } + return r +} + +func reportSync(results []syncResult, base string, hasGH, dry bool) int { + if len(results) == 0 { + render.Info(fmt.Sprintf("no spec records a branch or a PR — `%s spec track --here`", prog())) + return ExitOK + } + changed := 0 + for _, r := range results { + switch { + case r.Changed && dry: + changed++ + render.Info(fmt.Sprintf("%-24s %s → %s (%s)", r.Spec, orUnset(r.Was.State), r.Now.State, r.Why)) + case r.Changed: + changed++ + render.OK(fmt.Sprintf("%-24s %s → %s (%s)", r.Spec, orUnset(r.Was.State), r.Now.State, r.Why)) + case r.Undecided != "": + render.Warn(fmt.Sprintf("%-24s %s — %s", r.Spec, orUnset(r.Was.State), r.Undecided)) + default: + render.Info(fmt.Sprintf("%-24s %s", r.Spec, orUnset(r.Was.State))) + } + } + // What is still open is the answer the command was run for, so it is said rather + // than left to be counted off the rows above. + open := 0 + for _, r := range results { + if !artifact.Settled(r.Now.State) { + open++ + } + } + render.Info(fmt.Sprintf("%d tracked · %d changed · %d still open · base %s%s", + len(results), changed, open, base, ghNote(hasGH))) + if dry && changed > 0 { + render.Info("--dry-run: nothing was written") + } + return ExitOK +} + +func ghNote(hasGH bool) string { + if hasGH { + return "" + } + // Said once per run rather than per spec: without gh, a deleted branch cannot be + // told from a merged one, and the reader should know that is why some rows say so. + return " · no gh, so a deleted branch cannot be resolved" +} + +// branchWhy says what git found, in the terms a reader is actually asking about: +// how much work is sitting there unmerged. +func branchWhy(b git.Branch) string { + switch { + case b.Ahead == 1: + return fmt.Sprintf("%s has 1 commit not on %s", b.Name, b.Base) + case b.Ahead > 1: + return fmt.Sprintf("%s has %d commits not on %s", b.Name, b.Ahead, b.Base) + default: + // Level with the base: branched and nothing done, or fast-forwarded and + // nothing has advanced past it. Neither is delivered work. + return fmt.Sprintf("%s is level with %s", b.Name, b.Base) + } +} diff --git a/internal/cli/track_test.go b/internal/cli/track_test.go new file mode 100644 index 0000000..f1e330a --- /dev/null +++ b/internal/cli/track_test.go @@ -0,0 +1,313 @@ +package cli + +import ( + "encoding/json" + "os" + "os/exec" + "strings" + "testing" + + "github.com/protonspy/spec-claude-code/internal/artifact" + "github.com/protonspy/spec-claude-code/internal/git" + "github.com/protonspy/spec-claude-code/internal/paths" +) + +// reconcile is the whole policy, and it is a pure function precisely so the rules can +// be read here without a repository. Every row is a state somebody's workspace is +// actually in. +func TestReconcile(t *testing.T) { + rec := func(branch string, pr int, state string) artifact.Delivery { + return artifact.Delivery{Branch: branch, PR: pr, State: state} + } + alive := func(ahead int) evidence { + return evidence{branch: git.Branch{Name: "feat/x", Local: true, Base: "main", Ahead: ahead}} + } + landed := evidence{branch: git.Branch{Name: "feat/x", Local: true, Base: "main", Behind: 2, Merged: true}} + gone := evidence{branch: git.Branch{Name: "feat/x", Base: "main"}} + pr := func(state string) evidence { + return evidence{askedPR: true, pr: git.PR{Number: 28, State: state}} + } + + for _, tc := range []struct { + name string + was artifact.Delivery + ev evidence + want string + changed bool + undecided bool + }{ + {"a branch with work on it is in progress", + rec("feat/x", 0, ""), alive(3), artifact.DeliveryInProgress, true, false}, + {"a branch level with the base has not delivered anything", + rec("feat/x", 0, artifact.DeliveryInProgress), alive(0), artifact.DeliveryInProgress, false, false}, + {"a landed branch is merged", + rec("feat/x", 0, artifact.DeliveryInProgress), landed, artifact.DeliveryMerged, true, false}, + {"a deleted branch with nothing to ask is undecided, not guessed", + rec("feat/x", 0, artifact.DeliveryInProgress), gone, artifact.DeliveryInProgress, false, true}, + {"a settled record does not re-report the ambiguity", + rec("feat/x", 0, artifact.DeliveryMerged), gone, artifact.DeliveryMerged, false, false}, + {"an open PR is in review", + rec("feat/x", 28, artifact.DeliveryInProgress), pr(git.StateOpen), artifact.DeliveryInReview, true, false}, + {"a merged PR settles it even with the branch gone", + rec("feat/x", 28, artifact.DeliveryInReview), pr(git.StateMerged), artifact.DeliveryMerged, true, false}, + {"a PR closed unmerged is abandoned", + rec("feat/x", 28, artifact.DeliveryInReview), pr(git.StateClosed), artifact.DeliveryAbandoned, true, false}, + {"a recorded PR with no gh is undecided rather than downgraded", + rec("", 28, artifact.DeliveryInReview), evidence{}, artifact.DeliveryInReview, false, true}, + } { + t.Run(tc.name, func(t *testing.T) { + got := reconcile("feature", tc.was, tc.ev) + if got.Now.State != tc.want { + t.Errorf("state = %q, want %q (why: %q, undecided: %q)", + got.Now.State, tc.want, got.Why, got.Undecided) + } + if got.Changed != tc.changed { + t.Errorf("changed = %v, want %v", got.Changed, tc.changed) + } + if (got.Undecided != "") != tc.undecided { + t.Errorf("undecided = %q, want present=%v", got.Undecided, tc.undecided) + } + }) + } +} + +// The PR wins over the branch whenever it answered: a merged PR whose branch is still +// sitting there locally is merged, not in progress. +func TestReconcilePrefersThePullRequest(t *testing.T) { + was := artifact.Delivery{Branch: "feat/x", PR: 28, State: artifact.DeliveryInReview} + ev := evidence{ + branch: git.Branch{Name: "feat/x", Local: true, Base: "main", Ahead: 4}, + askedPR: true, + pr: git.PR{Number: 28, State: git.StateMerged}, + } + if got := reconcile("feature", was, ev); got.Now.State != artifact.DeliveryMerged { + t.Errorf("state = %q, want %q — the forge stated the outcome outright", + got.Now.State, artifact.DeliveryMerged) + } +} + +func trackedSpec(t *testing.T) (root string) { + t.Helper() + root = initWorkspace(t) + if _, stderr, code := run(t, "spec", "new", "user-auth", "--root", root); code != ExitOK { + t.Fatalf("spec new: %d (%s)", code, stderr) + } + return root +} + +func requirements(t *testing.T, root string) string { + t.Helper() + b, err := os.ReadFile(paths.Requirements(root, "user-auth")) + if err != nil { + t.Fatalf("read requirements: %v", err) + } + return string(b) +} + +func TestSpecTrackWritesTheRecord(t *testing.T) { + root := trackedSpec(t) + + if _, stderr, code := run(t, "spec", "track", "user-auth", "--branch", "feat/user-auth", "--root", root); code != ExitOK { + t.Fatalf("track: %d (%s)", code, stderr) + } + body := requirements(t, root) + for _, want := range []string{"branch: feat/user-auth", "delivery: " + artifact.DeliveryInProgress} { + if !strings.Contains(body, want) { + t.Errorf("requirements.md does not record %q:\n%s", want, body) + } + } + // The kickoff answers are still there: the record is written into the block, not + // over it. + if !strings.Contains(body, "autonomy: auto") { + t.Error("writing the record dropped the kickoff answers") + } + + // A PR moves it on, and does not have to restate the branch. + if _, stderr, code := run(t, "spec", "track", "user-auth", "--pr", "28", "--root", root); code != ExitOK { + t.Fatalf("track --pr: %d (%s)", code, stderr) + } + body = requirements(t, root) + for _, want := range []string{"branch: feat/user-auth", "pr: 28", "delivery: " + artifact.DeliveryInReview} { + if !strings.Contains(body, want) { + t.Errorf("requirements.md does not record %q:\n%s", want, body) + } + } + if _, _, code := run(t, "validate", "--root", root); code != ExitOK { + t.Error("a tracked spec does not pass validation") + } +} + +func TestSpecTrackRejectsWhatSyncCouldNotUse(t *testing.T) { + root := trackedSpec(t) + for _, args := range [][]string{ + {"spec", "track", "user-auth"}, + {"spec", "track", "user-auth", "--delivery", "shipping-soon"}, + {"spec", "track", "user-auth", "--pr", "-3"}, + {"spec", "track", "user-auth", "--branch", "two words"}, + } { + if _, _, code := run(t, append(args, "--root", root)...); code != ExitError { + t.Errorf("%v exited %d, want %d", args, code, ExitError) + } + } + if strings.Contains(requirements(t, root), "delivery:") { + t.Error("a rejected track still wrote something") + } +} + +func TestSpecTrackIsIdempotent(t *testing.T) { + root := trackedSpec(t) + run(t, "spec", "track", "user-auth", "--branch", "feat/x", "--root", root) + first := requirements(t, root) + stdout, _, code := run(t, "spec", "track", "user-auth", "--branch", "feat/x", "--root", root) + if code != ExitOK { + t.Fatalf("exit %d", code) + } + if !strings.Contains(stdout, "already records") { + t.Errorf("stdout = %q, want it to say nothing changed", stdout) + } + if requirements(t, root) != first { + t.Error("re-recording the same values rewrote the file") + } +} + +func TestSpecTrackDryRunWritesNothing(t *testing.T) { + root := trackedSpec(t) + before := requirements(t, root) + if _, _, code := run(t, "spec", "track", "user-auth", "--branch", "feat/x", "--dry-run", "--root", root); code != ExitOK { + t.Fatalf("exit %d", code) + } + if requirements(t, root) != before { + t.Error("--dry-run wrote to the file") + } +} + +// The record has to reach `spec list` and `spec show`, because "what is still open" +// is asked of the listing and never of one spec's frontmatter. +func TestTheRecordShowsUpInListAndShow(t *testing.T) { + root := trackedSpec(t) + run(t, "spec", "track", "user-auth", "--branch", "feat/user-auth", "--pr", "28", "--root", root) + + stdout, _, _ := run(t, "spec", "list", "--root", root) + for _, want := range []string{artifact.DeliveryInReview, "feat/user-auth", "#28"} { + if !strings.Contains(stdout, want) { + t.Errorf("spec list = %q, want it to carry %q", stdout, want) + } + } + + stdout, _, _ = run(t, "spec", "show", "user-auth", "--json", "--root", root) + var got struct { + Delivery artifact.Delivery `json:"delivery"` + } + if err := json.Unmarshal([]byte(stdout), &got); err != nil { + t.Fatalf("show --json is not JSON (%v): %q", err, stdout) + } + if got.Delivery.Branch != "feat/user-auth" || got.Delivery.PR != 28 || + got.Delivery.State != artifact.DeliveryInReview { + t.Errorf("delivery = %+v", got.Delivery) + } +} + +// An untracked spec says so once, where somebody can act on it, and stays silent in +// the listing: a column of "not started" is a column nobody reads. +func TestAnUntrackedSpecIsNotAFinding(t *testing.T) { + root := trackedSpec(t) + if _, _, code := run(t, "validate", "--root", root); code != ExitOK { + t.Error("an untracked spec produced findings") + } + stdout, _, _ := run(t, "spec", "show", "user-auth", "--root", root) + if !strings.Contains(stdout, "not recorded") { + t.Errorf("spec show = %q, want it to say the record is missing", stdout) + } +} + +func TestSpecSyncDegradesOutsideARepository(t *testing.T) { + root := trackedSpec(t) + run(t, "spec", "track", "user-auth", "--branch", "feat/x", "--root", root) + // t.TempDir() is not a git repository, and a workspace outside one is legitimate. + stdout, stderr, code := run(t, "spec", "sync", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("exit %d (%s)", code, stderr) + } + var got struct { + Git bool `json:"git"` + } + if err := json.Unmarshal([]byte(stdout), &got); err != nil { + t.Fatalf("not JSON (%v): %q", err, stdout) + } + if got.Git { + t.Error("sync claimed a git repository where there is none") + } +} + +// The lifecycle against a real repository: branched, worked on, merged. Skipped where +// git is not installed rather than failing — this asserts scc's reading of git, and a +// machine without git has nothing to read. +func TestSpecSyncFollowsABranchToMerged(t *testing.T) { + if !git.Found(git.Bin) { + t.Skip("git is not on PATH") + } + root := trackedSpec(t) + gitRun := func(args ...string) { + t.Helper() + cmd := exec.Command(git.Bin, args...) + cmd.Dir = root + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } + } + gitRun("init", "-q", "-b", "main", ".") + gitRun("config", "user.email", "t@example.com") + gitRun("config", "user.name", "t") + gitRun("add", "-A") + gitRun("commit", "-qm", "init") + gitRun("switch", "-qc", "feat/user-auth") + + if _, stderr, code := run(t, "spec", "track", "user-auth", "--here", "--root", root); code != ExitOK { + t.Fatalf("track --here: %d (%s)", code, stderr) + } + if !strings.Contains(requirements(t, root), "branch: feat/user-auth") { + t.Fatal("--here did not read the checked-out branch") + } + + // Freshly branched: nothing has landed, and the ancestor test alone would have + // called this merged. + if _, _, code := run(t, "spec", "sync", "--root", root); code != ExitOK { + t.Fatal("sync failed") + } + if got := requirements(t, root); !strings.Contains(got, "delivery: "+artifact.DeliveryInProgress) { + t.Errorf("a freshly branched spec is not in progress:\n%s", got) + } + + if err := os.WriteFile(root+"/work.txt", []byte("x\n"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + gitRun("add", "-A") + gitRun("commit", "-qm", "work") + run(t, "spec", "sync", "--root", root) + if got := requirements(t, root); !strings.Contains(got, "delivery: "+artifact.DeliveryInProgress) { + t.Errorf("a branch with unmerged work is not in progress:\n%s", got) + } + + gitRun("switch", "-q", "main") + gitRun("merge", "-q", "--no-ff", "feat/user-auth", "-m", "merge") + stdout, _, code := run(t, "spec", "sync", "--root", root) + if code != ExitOK { + t.Fatal("sync failed after the merge") + } + if !strings.Contains(stdout, artifact.DeliveryMerged) { + t.Errorf("sync = %q, want it to report the merge", stdout) + } + if got := requirements(t, root); !strings.Contains(got, "delivery: "+artifact.DeliveryMerged) { + t.Errorf("the merge was not written back:\n%s", got) + } + + // A branch deleted after the merge leaves the record settled and quiet. + gitRun("branch", "-qD", "feat/user-auth") + stdout, stderr, _ := run(t, "spec", "sync", "--root", root) + if strings.Contains(stderr, "look the same") { + t.Errorf("a settled spec re-reported the deleted-branch ambiguity: %q", stderr) + } + if !strings.Contains(stdout, "0 still open") { + t.Errorf("sync = %q, want nothing left open", stdout) + } +} diff --git a/internal/git/git.go b/internal/git/git.go new file mode 100644 index 0000000..0d2dfb1 --- /dev/null +++ b/internal/git/git.go @@ -0,0 +1,236 @@ +// Package git answers one question for scc: what happened to the branch this spec +// says it is being built on. +// +// It is the fourth integration package, on the same terms as rtk, headroom and +// codegraph — it composes command lines for a binary somebody else ships and reads +// what comes back. Two binaries rather than one, because the question has two halves +// that no caller should have to route between: `git` knows whether a branch exists +// and whether it has landed, and `gh` knows whether a pull request is open, merged, +// or closed unmerged. A caller asking "is this work finished?" would otherwise have +// to know which of the two could answer today. +// +// **Nothing here installs anything, and nothing here writes.** scc will not install +// git, and it runs no command that changes a repository: every call below is a query. +// That is what makes it safe to run this over every spec in a workspace on somebody's +// behalf — the worst outcome of a wrong answer is a frontmatter line that says the +// wrong thing, and `scc spec sync` can be run again. +// +// Absence is a normal answer, never an error to propagate. A workspace with no git, +// no remote, or no `gh` still has specs, and the caller reports what it could not +// determine rather than failing. +package git + +import ( + "encoding/json" + "errors" + "os/exec" + "strconv" + "strings" +) + +// Bin and GHBin are the executables, named here so a caller never spells them. +const ( + Bin = "git" + GHBin = "gh" +) + +// DefaultBase is the branch a repository is assumed to merge into when nothing says +// otherwise. It is a fallback for a query that failed, not a preference: every path +// below asks the repository first. +const DefaultBase = "main" + +// ErrUnavailable is what every query returns when the binary it needs is not on PATH. +// Callers test for it to say "could not determine" instead of "failed". +var ErrUnavailable = errors.New("not available") + +// Found reports whether a binary is on PATH. +func Found(bin string) bool { + _, err := exec.LookPath(bin) + return err == nil +} + +// run executes a query in dir and returns its trimmed stdout. +// +// Stderr is deliberately dropped. Every caller here treats failure as "could not +// determine", and git writes advice to stderr on perfectly ordinary misses — a +// caller that surfaced it would turn "this branch is gone, as expected after a +// merge" into something that reads like a malfunction. +func run(bin, dir string, args ...string) (string, error) { + if !Found(bin) { + return "", ErrUnavailable + } + cmd := exec.Command(bin, args...) + cmd.Dir = dir + out, err := cmd.Output() + if err != nil { + return "", err + } + return strings.TrimSpace(string(out)), nil +} + +// IsRepo reports whether dir is inside a git work tree. +func IsRepo(dir string) bool { + out, err := run(Bin, dir, "rev-parse", "--is-inside-work-tree") + return err == nil && out == "true" +} + +// CurrentBranch is the checked-out branch, or "" in a detached head. +func CurrentBranch(dir string) (string, error) { + out, err := run(Bin, dir, "rev-parse", "--abbrev-ref", "HEAD") + if err != nil { + return "", err + } + if out == "HEAD" { + return "", nil + } + return out, nil +} + +// Base is the branch this repository merges into. +// +// It is read from the remote's own HEAD rather than guessed, because "main" has been +// wrong for every repository created before 2020 and for plenty created since. A +// repository with no remote falls back to whichever of main and master exists, and +// then to DefaultBase — which is a guess, and is why callers say what base they used. +func Base(dir string) string { + if out, err := run(Bin, dir, "symbolic-ref", "--short", "refs/remotes/origin/HEAD"); err == nil { + if _, name, found := strings.Cut(out, "/"); found && name != "" { + return name + } + } + for _, name := range []string{DefaultBase, "master"} { + if ref(dir, name) != "" { + return name + } + } + return DefaultBase +} + +// ref resolves the first ref that exists for a branch name: the local branch, then +// the remote-tracking one. The remote half is what keeps a branch findable after a +// local checkout has been deleted, which is most of them. +func ref(dir, branch string) string { + for _, candidate := range []string{"refs/heads/" + branch, "refs/remotes/origin/" + branch} { + if _, err := run(Bin, dir, "rev-parse", "--verify", "--quiet", candidate); err == nil { + return candidate + } + } + return "" +} + +// Branch is what git knows about one branch. +type Branch struct { + Name string `json:"name"` + // Local and Remote say where it still exists. Both false means the branch is + // gone, which on its own means nothing: it is equally the shape of a branch + // deleted after a clean merge and of one abandoned. + Local bool `json:"local"` + Remote bool `json:"remote"` + // Ahead is the commits on this branch that are not on Base; Behind is the + // reverse. Both are 0 when the two refs are the same commit. + Ahead int `json:"ahead"` + Behind int `json:"behind"` + // Merged is Ahead == 0 with Behind > 0: everything this branch had is on the + // base, and the base has moved on since. That second half is what stops a + // freshly created branch — which is trivially an ancestor of its base, having + // added nothing — from being read as delivered work. + // + // The one case it gets wrong is a fast-forward merge that nothing has advanced + // past, where the two refs are identical and no ref can tell "just branched" + // from "just landed". It resolves that as *not* merged, deliberately: this + // record exists to surface unfinished work, so the error that leaves a loose end + // visible is the one to make. + Merged bool `json:"merged"` + Base string `json:"base"` +} + +// Exists reports whether git can still see the branch at all. +func (b Branch) Exists() bool { return b.Local || b.Remote } + +// Look reports what git knows about a branch. +func Look(dir, branch, base string) (Branch, error) { + if !Found(Bin) { + return Branch{Name: branch}, ErrUnavailable + } + if base == "" { + base = Base(dir) + } + b := Branch{Name: branch, Base: base} + _, localErr := run(Bin, dir, "rev-parse", "--verify", "--quiet", "refs/heads/"+branch) + b.Local = localErr == nil + _, remoteErr := run(Bin, dir, "rev-parse", "--verify", "--quiet", "refs/remotes/origin/"+branch) + b.Remote = remoteErr == nil + if !b.Exists() { + return b, nil + } + // Counted against the base ref that exists: a workspace with no remote has no + // origin/main to compare against, and a count nobody can take is not a zero. + baseRef := ref(dir, base) + if baseRef == "" { + return b, nil + } + b.Behind, b.Ahead = counts(dir, baseRef, ref(dir, branch)) + b.Merged = b.Ahead == 0 && b.Behind > 0 + return b, nil +} + +// counts is how far two refs have diverged: commits on left only, then on right only. +// +// `--left-right --count a...b` is one call for both halves, and it is the honest test +// for "did this land" — an ancestor check alone answers yes for a branch that has +// never had a commit of its own. +func counts(dir, left, right string) (int, int) { + out, err := run(Bin, dir, "rev-list", "--left-right", "--count", left+"..."+right) + if err != nil { + return 0, 0 + } + fields := strings.Fields(out) + if len(fields) != 2 { + return 0, 0 + } + l, err1 := strconv.Atoi(fields[0]) + r, err2 := strconv.Atoi(fields[1]) + if err1 != nil || err2 != nil { + return 0, 0 + } + return l, r +} + +// PR is what the forge knows about a pull request. +type PR struct { + Number int `json:"number"` + State string `json:"state"` // OPEN | MERGED | CLOSED, as gh spells them + Branch string `json:"branch"` + URL string `json:"url"` +} + +// The states gh reports, named so a caller never matches on a string literal. +const ( + StateOpen = "OPEN" + StateMerged = "MERGED" + StateClosed = "CLOSED" +) + +// LookPR asks gh about one pull request. +// +// This is the only query that can tell a merged branch from an abandoned one after +// the branch itself is gone, which is the common case and the reason gh is worth +// shelling out to at all. Without it, a spec whose branch has vanished is reported as +// undetermined rather than guessed at. +func LookPR(dir string, number int) (PR, error) { + out, err := run(GHBin, dir, "pr", "view", strconv.Itoa(number), + "--json", "number,state,headRefName,url") + if err != nil { + return PR{Number: number}, err + } + var raw struct { + Number int `json:"number"` + State string `json:"state"` + HeadRefName string `json:"headRefName"` + URL string `json:"url"` + } + if err := json.Unmarshal([]byte(out), &raw); err != nil { + return PR{Number: number}, err + } + return PR{Number: raw.Number, State: strings.ToUpper(raw.State), Branch: raw.HeadRefName, URL: raw.URL}, nil +} diff --git a/internal/validate/spec.go b/internal/validate/spec.go index cd790a2..84e6d80 100644 --- a/internal/validate/spec.go +++ b/internal/validate/spec.go @@ -4,11 +4,13 @@ import ( "fmt" "os" "sort" + "strconv" "strings" "github.com/protonspy/spec-claude-code/internal/artifact" "github.com/protonspy/spec-claude-code/internal/ears" "github.com/protonspy/spec-claude-code/internal/finding" + "github.com/protonspy/spec-claude-code/internal/mdscan" "github.com/protonspy/spec-claude-code/internal/paths" ) @@ -121,6 +123,7 @@ func checkRequirements(set *finding.Set, root, feature string) (map[string]requi return nil, false, nil } checkKickoffAs(set, file, doc.Frontmatter, "spec.kickoff-invalid") + checkDelivery(set, file, doc.Frontmatter) reqs := map[string]requirement{} for i, line := range doc.Body { @@ -275,3 +278,44 @@ func allowed(values map[string]bool) string { sort.Strings(out) return strings.Join(out, " | ") } + +// checkDelivery grades the three keys `scc spec track` and `scc spec sync` write. +// +// Checked only when present, like every other frontmatter answer: a spec nobody has +// branched for is not a spec with a defect, and every spec written before this record +// existed carries none of these. +// +// What it is really protecting is the one question the record answers — what is still +// unfinished. A `delivery:` value outside the vocabulary makes that question +// unanswerable by anything but a person reading every spec, which is the state this +// record was added to end. A `pr:` that is not a number is worse than absent: `spec +// sync` cannot ask the forge about it, so the spec looks tracked and is not. +func checkDelivery(set *finding.Set, file string, fm mdscan.Frontmatter) { + if state, ok := fm.Get(artifact.KeyDelivery); ok && !artifact.ValidDelivery(state) { + set.Addf(file, 1, "spec.delivery-invalid", "`%s: %s` is not one of %s", + artifact.KeyDelivery, state, strings.Join(artifact.DeliveryStates(), ", ")) + } + if pr, ok := fm.Get(artifact.KeyPR); ok { + if n, err := strconv.Atoi(pr); err != nil || n <= 0 { + set.Addf(file, 1, "spec.pr-invalid", + "`%s: %s` is not a pull request number, so nothing can ask the forge about it", + artifact.KeyPR, pr) + } + } + if branch, ok := fm.Get(artifact.KeyBranch); ok { + if strings.TrimSpace(branch) == "" || strings.ContainsAny(branch, " \t") { + set.Addf(file, 1, "spec.branch-invalid", + "`%s: %s` is not a branch name", artifact.KeyBranch, branch) + } + } + // A branch or a PR with no state is a half-written record: something knows where + // the work is and nothing says whether it landed, which is exactly the loose end + // the record exists to surface. + _, hasBranch := fm.Get(artifact.KeyBranch) + _, hasPR := fm.Get(artifact.KeyPR) + if _, hasState := fm.Get(artifact.KeyDelivery); (hasBranch || hasPR) && !hasState { + set.Addf(file, 1, "spec.delivery-unstated", + "a %s or %s is recorded with no `%s:` — run `scc spec sync` to settle it", + artifact.KeyBranch, artifact.KeyPR, artifact.KeyDelivery) + } +} diff --git a/internal/validate/spec_test.go b/internal/validate/spec_test.go index e4255f2..c8b12d6 100644 --- a/internal/validate/spec_test.go +++ b/internal/validate/spec_test.go @@ -398,3 +398,41 @@ func count(haystack []string, want string) int { } return n } + +// The delivery record is graded when present and silent when absent — the same terms +// as every other frontmatter answer, and what lets every spec written before it +// existed keep passing. +func TestDeliveryRecordIsCheckedWhenPresent(t *testing.T) { + for _, tc := range []struct { + name string + fm string + want string + }{ + {"absent", "autonomy: auto\nci: wait", ""}, + {"a full record", "autonomy: auto\nbranch: feat/billing\npr: 28\ndelivery: in-review", ""}, + {"a branch on its own", "autonomy: auto\nbranch: feat/billing\ndelivery: in-progress", ""}, + {"a state outside the vocabulary", "branch: feat/x\ndelivery: shipping-soon", "spec.delivery-invalid"}, + {"a PR that is not a number", "branch: feat/x\npr: later\ndelivery: in-review", "spec.pr-invalid"}, + {"a PR that is not positive", "branch: feat/x\npr: 0\ndelivery: in-review", "spec.pr-invalid"}, + {"a branch with whitespace in it", "branch: two words\ndelivery: in-progress", "spec.branch-invalid"}, + {"a branch with nothing said about it", "branch: feat/x", "spec.delivery-unstated"}, + {"a PR with nothing said about it", "pr: 28", "spec.delivery-unstated"}, + } { + t.Run(tc.name, func(t *testing.T) { + root := t.TempDir() + reqs := "---\n" + tc.fm + "\n---\n" + + strings.SplitN(goodRequirements, "---\n", 3)[2] + writeSpec(t, root, "billing", reqs, goodDesign, goodTasks) + got := specFindings(t, root, "billing") + if tc.want == "" { + if len(got) != 0 { + t.Errorf("findings on a legitimate record: %v", got) + } + return + } + if !contains(got, tc.want) { + t.Errorf("findings = %v, want %s", got, tc.want) + } + }) + } +}