diff --git a/CLAUDE.md b/CLAUDE.md index 9605cf7..42698c0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,7 +18,11 @@ These landed after phase 10, and all are documented in `design/orchestration.md` - **Three harnesses, one template set.** `scc init --claude|--codex|--opencode` (Claude Code is the default, and a terminal with no flag gets a picker). One prose source: paths come from a `paths.Harness` profile and the header each loader parses is synthesized at render time — YAML frontmatter for Claude Code and opencode, a TOML agent role file for Codex. - The profile also carries `PreloadsRules`, because where the rules go is scc's choice but what the harness then does with them is not. Claude Code loads `.claude/rules/*.md` at launch with the same priority as `CLAUDE.md`; Codex and opencode load nothing from `rules/`, which is scc's own directory there. The entry file branches on it: told to "read the rule when the concern is live", an agent that already has all nine in context re-reads them, putting the same ~26KB in twice — and learns that the one document it is meant to trust is wrong about its own environment. Confirm per harness with `/context`. + The profile also carries `PreloadsRules`, because where the rules go is scc's choice but what the harness then does with them is not. Claude Code loads `.claude/rules/*.md` at launch with the same priority as `CLAUDE.md`; Codex and opencode load nothing from `rules/`, which is scc's own directory there. The entry file branches on it: told to "read the rule when the concern is live", an agent that already has the set in context re-reads it, putting the same bytes in twice — and learns that the one document it is meant to trust is wrong about its own environment. Confirm per harness with `/context`. + + **`ScopedRules` is the second half of that, and it is the only lever there is on what the methodology costs.** Measured on a freshly scaffolded Claude Code workspace, the entry file plus 15 rules came to 47KB in every request of the session — this file said nine rules and ~26KB, and had said so since the set was that size. Three rules now carry a `paths:` header, so the harness loads them when the agent touches the tree they govern: `specs.md` (`specs/**`), `tasks.md` (`specs/**`, `plans/**`), `knowledge-base.md` (`docs/**`). That is 10.4KB, about a quarter, moved off the standing cost without moving a word of the methodology. + + **Which three is mechanical, not taste: a rule may be scoped only where a validator reports what it prevents.** A scope trades "in context before the decision" for "in context when the file is open", and that trade is survivable exactly when something else catches the result — a malformed EARS line, a task with no methodology annotation, a broken wikilink are all `scc validate` exit 2 before the commit. `TestOnlyRulesAValidatorBacksAreScoped` holds the bar. The rules that look most temptingly scopable are the clearest refusals: `artifacts.md` would scope to `plans/**` and its instruction is *never open the plan*, so it would only ever load to say the thing it prevents has happened; `prior-art.md` and `routing.md` fire before any file exists; `caveman.md`, `ladder.md` and `delivery.md` govern how the agent works rather than what it edits, and nothing mechanical reports a failure to follow them. - **`scc update` (phase 11), as replace-or-keep rather than the planned three-way merge.** It hashes every managed file against this build and against the manifest, prints the plan grouped by outcome, asks, and then replaces what is safe to replace. An edited file is kept and named; `--force` is the separate decision. `internal/merge` is still unbuilt. - **`scc rtk`, and the block `scc init` writes.** Wires in [RTK](https://github.com/rtk-ai/rtk), the CLI proxy that filters command output: `cargo install` when the binary is missing, plus a splice of RTK's marker-delimited usage block into the entry file. **scc's block wins by default**, replacing whatever sits between the markers; `--keep` is the separate decision. The reason is size, not authorship: `rtk init` (measured on 0.42.4) writes 139 lines / 5140 bytes, scc ships 18 lines / ~900, both stamped `v2`, both saying the same thing — and the entry file is preloaded into every request of the session, so the difference is paid continuously rather than once. Between two blocks of the same version the condensed one is simply better, and leaving the larger one because it got there first is a standing cost. Where that costs something is a block claiming a *newer* version, which is a real downgrade: the run names the version it displaced and points at `--keep`. `--check` reports without writing and exits 2 when the block is missing. @@ -168,6 +172,10 @@ These landed after phase 10, and all are documented in `design/orchestration.md` **They never refuse — but on `Stop`, speaking is itself a continuation, and getting that wrong shipped a loop.** The harness treats `additionalContext` on Stop as guidance the conversation *continues* for, under the same loop protections as an outright block; returning 0 is not a passive report there. v0.24.0 asserted the opposite in this very paragraph, never read `stop_hook_active`, and put a branch-scoped finding on a stage that re-fires until the consecutive-block cap — measured in a real session, with the agent reduced to answering `.` to itself nine times. Two rules now hold it: **`stop_hook_active` is honoured unconditionally**, and **Stop may only carry what the next turn can resolve** — a finding clears when fixed, unpushed work clears when pushed, and a fact about the branch clears for nobody, which is why drift moved to `SessionStart`. Refusal still belongs at the commit, which is a decision with a natural place to stand in front of. + **That second rule had two more violations of its own, and both are the same shape as the loop it was written for.** The stage ran every validator, `attribution` included — whose subject is commits already made, so the fix is a rebase and the line re-appears at the end of every turn for the life of the branch. It is out of this run entirely (`validate.WithoutAttribution`); `commit-msg` catches a signature at the one moment it is still a file. And it reported findings anywhere in the workspace, so a migrated plan or a stale ADR the task never touched asked the agent, every turn, to go and fix a file it had no business in — the findings are now intersected with `git.Changed`, and `scc validate` is where the whole workspace still answers. The undelivered-work line had a third: it fired from the *first* commit of a unit of work, which on a continuation stage means a push per turn, each paying for the pre-push gate. It waits for a clean tree now, because uncommitted changes are the cheapest evidence there is that the work is still in hand. + + **And the stage stopped re-indexing on every turn.** `syncGraph` started a CodeGraph process at the end of each turn whether or not anything had changed; it now compares the graph's mtime against the files this branch touched — a stat per changed file rather than a walk — and skips when nothing is newer. It errs toward syncing: an unnecessary index costs a subprocess, and a skipped one leaves the agent an index that answers confidently about code that changed. + **`Stop` also reports methodology drift, and that is the one place this design improves on ponytail's.** ponytail assumes an agent drifts and re-asserts its instruction on every prompt and every subagent; that works because its instruction is a *disposition* — reach for the smallest thing that works — and a disposition has no shape, so re-asserting is the only move available. scc's rules are not dispositions. Source changed with no box ticked, a marker left in code, commits sitting on the base branch: each leaves evidence on disk, in a range `internal/git` already computes. So the answer is not to re-assert a rule at somebody who has read it — it is to say what actually happened. Three signals, **each a line only when it is true**, and `TestTheDriftStageIsSilentOnACleanBranch` is what holds that, because a stage that speaks every turn is one the reader learns to skip and then the turn it had something to say is the turn nobody read it. The marker scan requires a comment opener earlier on the line, which is what separates the annotation the rule forbids from a string literal containing the word — including the list the check itself is built from. **`UserPromptSubmit` is the third stage, and it is bounded by what it may speak about.** It fires on every prompt of every session — the worst cost profile in the set — so the only thing that makes it defensible is that it almost never speaks: it says something *only* when the prompt names an artifact this workspace actually has, and then says the one thing cheapest to say at that moment, which is how to read that artifact through `scc map` instead of with `Read`. ponytail can afford to fire on every prompt because its instruction is a disposition with no shape; scc's rules are routed by moment, so a stage that spoke unprompted would not be re-asserting anything — it would be interrupting. Three details are load-bearing: it runs **no subprocess** (two `ReadDir` calls and a string scan, with a 10-second ceiling it never expects to use, because it sits between the keystroke and the agent while the other two run when nobody is waiting); a name shorter than four characters is matched **only by its path**, since a spec called `ui` would otherwise fire on "build the ui" and on "guide"; and `-`/`.` are *inside* a name rather than boundaries around it, so `auth` does not match the spec `auth-flow`. A requirement id is deliberately not a trigger — `R1.2` names a requirement without naming a spec, and a real workspace defines it in nine. This is also the one stage where a non-zero exit discards the user's prompt before the agent sees it, which `TestThePromptStageNeverBlocksATurn` pins. **It was planned as a decision to be made on benchmark evidence and was built without it** (see `design/ponytail-port.md` 3.7), so it is conservative by construction rather than by measurement. @@ -298,17 +306,27 @@ Five packages sit off to the side of that tree — `rtk`, `headroom`, `codegraph **Exit codes are the contract.** `0` ok · `1` usage/runtime error · `2` validation findings. Every lint/validate command returns `2` on findings so CI and agents can branch on it. A finding is a legitimate answer to a lint question, not a failure of the tool — don't collapse `2` into `1`. +**`--help` is `0`**, and it took 52 call sites to be wrong about that at once. Every handler routed `flag.ErrHelp` to `ExitError`, so `scc map tasks --help` reported the same code as a command that could not run — to an agent branching on this contract, help that says the command is broken. `parseFlags` returns the error and `exitFor` maps it; `helpWord` turns a lone `help` positional on a leaf command into the same thing, because `scc validate help` is what somebody types after `scc map help`. + +**A positional never ends the flags.** Go's `flag` stops at the first non-flag argument, so a single pass reads everything after it as more positionals — measured, `scc notes add --tag gotcha "text" --path internal/x` wrote the flag into the note as prose, recorded no path, and exited 0. `parseFlags` alternates between peeling positionals and parsing flags until nothing is left, and honours a bare `--` for a value spelled like a flag. Misfiling an argument while reporting success is worse than rejecting it. + `scc launch` is the single exception, and it has to be: it returns whatever the agent it started returned. A launcher that flattened the exit status of what it launched into its own vocabulary would be unusable in the scripts people actually write. scc's own failures — no workspace, unknown harness, binary not on PATH — still happen before anything starts and still report `1`. **A validator that fires on scc's own output is the worst bug in the product.** The templates carry their instructions in HTML comments and fenced examples, which is exactly what `mdscan` excludes — and `TestFreshArtifactsPassTheirOwnValidators` in `internal/cli` is the gate. Treat it as required reading before changing a template or a validator: one wrong finding teaches the user to disbelieve all eight. **Machine-readable output.** Bind the `--json` flag via `addJSON` and emit through `emitJSON` (`internal/cli/jsonout.go`) so the flag name, help text, and stdout/stderr split stay identical across commands — stdout is a clean JSON stream, diagnostics go to stderr. +**Indented for a person, compact for everything else** — the same test `render` already makes about color. Indentation exists to be read, and the overwhelming reader of a `--json` document here is an agent paying by the byte: measured on a six-task plan, `map tasks --json` was 2198 bytes indented, about a third of it leading whitespace, against 599 for the human listing of the same tasks. It is 1558 now and a terminal still gets the readable form. The corollary is a testing rule: a test may not assert on the *spelling* of a document — `decode` in `cli_test.go` unmarshals it, because a `strings.Contains(out, "\"count\": 0")` is an assertion about the reader rather than about the command, and four of them broke on this change while the commands were correct. + **Hostile input.** Any positional name that becomes a path segment must pass `workspace.SafeName` *before* `filepath.Join` — without it `scc spec delete .. --force` resolves to the workspace root. Resource names are kebab-case (`workspace.KebabCheck`). +**And an artifact address is hostile input too.** `artifact.Resolve` accepts a path relative to the working directory, which is what makes `scc map show tasks.md` work from inside a spec directory — and, until `artifact.Within` was put in front of it, what made `scc patch append ../outside.md L3 --text X` write to a file outside the repository and exit 0, reporting only "no validator for this artifact". `requireWorkspace` does not cover this: it checks where scc is, not where the write lands. The reason it belongs in `internal/artifact` rather than in a handler is that an address reaches scc from a file the agent read as readily as from the user, so every command that resolves one needs the same boundary — `map show` dumping a file from outside the tree into context is the read half of the same hole. Symlinks are evaluated on both sides before the comparison, because a `t.TempDir()` on macOS is itself a symlink and a lexical test would call every artifact in it an escape. + **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. +**The rules are a standing budget, not a place to explain things.** Every unscoped 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. A scoped rule is capped on its prose, not on the `paths:` header that took it out of the preloaded set. + +**A skill's description is on that same budget and its body is not.** The harness loads every `description` at session start to decide when to invoke the skill, and leaves the body until it does — so the body is paid once by the run that needed it, and the description by every request whether the skill runs or not. Eight of them had grown to 4087 characters, mostly enumerating findings `scc validate` already names by rule; `TestSkillDescriptionsStayShortEnoughToPreload` caps one at 340 and the set at 2400. The body is where the detail belongs, on exactly the reasoning that sends a rule's detail into a `--help` string. 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. diff --git a/internal/artifact/artifact.go b/internal/artifact/artifact.go index 1b6aea9..94872ff 100644 --- a/internal/artifact/artifact.go +++ b/internal/artifact/artifact.go @@ -450,18 +450,32 @@ func (a *Artifact) Prose(from, to int) string { // that exists relative to the working directory, a bare plan name, and a bare // feature name — which resolves to all three of that spec's files, because a spec // is three files and asking to map one is asking to map the spec. +// +// Every one of those has to land inside the workspace. The working-directory form +// is what makes `scc map show tasks.md` work from inside a spec directory, and +// without the boundary it is also what made `scc patch append ../../secrets.env` +// work from anywhere — measured, exit 0, file written, "no validator for this +// artifact" as the only sign. An address arrives from a file the agent read as +// readily as from the user, so the guard belongs here rather than in one handler. func Resolve(root, arg string) ([]string, error) { if arg == "" { return nil, fmt.Errorf("no artifact named") } clean := filepath.FromSlash(strings.TrimSuffix(arg, "/")) + escapes := false for _, cand := range []string{filepath.Join(root, clean), clean} { if abs, err := filepath.Abs(cand); err == nil { if info, err := os.Stat(abs); err == nil && info.Mode().IsRegular() { - return []string{abs}, nil + if Within(root, abs) { + return []string{abs}, nil + } + escapes = true } } } + if escapes { + return nil, fmt.Errorf("%q is outside the workspace at %s", arg, root) + } if err := workspace.SafeName(arg, "artifact"); err == nil { if p := paths.Plan(root, arg); isFile(p) { return []string{p}, nil @@ -495,6 +509,36 @@ func Resolve(root, arg string) ([]string, error) { return nil, fmt.Errorf("no artifact %q under %s/ or %s/", arg, paths.PlansSeg, paths.SpecsSeg) } +// Within reports whether target sits inside root. It is the boundary between an +// artifact address and the rest of the filesystem. +// +// Symlinks are evaluated on both sides before the comparison, because a temporary +// directory is itself a symlink on macOS (/var → /private/var) and a purely +// lexical test would call every file under one an escape. Where a side cannot be +// evaluated its cleaned absolute path is used, which still catches the `..` that +// this exists for. +func Within(root, target string) bool { + r, err := filepath.Abs(root) + if err != nil { + return false + } + t, err := filepath.Abs(target) + if err != nil { + return false + } + if p, err := filepath.EvalSymlinks(r); err == nil { + r = p + } + if p, err := filepath.EvalSymlinks(t); err == nil { + t = p + } + rel, err := filepath.Rel(r, t) + if err != nil { + return false + } + return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} + // Scan loads every artifact in the workspace: each plan, then each spec's three // files in phase order. It is the index every other read is a narrowing of. func Scan(root string) ([]*Artifact, error) { diff --git a/internal/artifact/artifact_test.go b/internal/artifact/artifact_test.go index af0eeb5..363a376 100644 --- a/internal/artifact/artifact_test.go +++ b/internal/artifact/artifact_test.go @@ -232,6 +232,47 @@ func TestResolveRefusesEscape(t *testing.T) { } } +// A path that exists outside the workspace resolves to nothing. +// +// Resolve accepts a path relative to the working directory, which is what makes +// `scc map show tasks.md` work from inside a spec directory — and, before this, +// what made `scc patch append ../outside.md L3 --text X` write to a file outside +// the repository and exit 0. An address reaches scc from a file the agent read as +// readily as from the user, so an escape is reachable without anybody typing it. +func TestResolveRefusesAFileOutsideTheWorkspace(t *testing.T) { + root, _ := writeWorkspace(t) + outside := filepath.Join(filepath.Dir(root), "outside.md") + if err := os.WriteFile(outside, []byte("# outside\n"), 0o644); err != nil { + t.Fatal(err) + } + for _, arg := range []string{outside, "../outside.md", filepath.Join("..", "outside.md")} { + got, err := Resolve(root, arg) + if err == nil { + t.Errorf("Resolve(%q) = %v, want an error", arg, got) + continue + } + if !strings.Contains(err.Error(), "outside the workspace") { + t.Errorf("Resolve(%q): %v, want it to name the boundary", arg, err) + } + } +} + +// The boundary has to hold on a root that is itself a symlink, which every +// t.TempDir() on macOS is (/var → /private/var). A lexical comparison alone would +// call every artifact in the workspace an escape. +func TestWithinFollowsSymlinks(t *testing.T) { + root, planPath := writeWorkspace(t) + if !Within(root, planPath) { + t.Errorf("Within(%q, %q) = false, want true", root, planPath) + } + if Within(root, filepath.Join(filepath.Dir(root), "elsewhere.md")) { + t.Error("Within accepted a sibling of the workspace") + } + if !Within(root, root) { + t.Error("Within rejected the root itself") + } +} + func sameFile(a, b string) (bool, error) { ai, err := os.Stat(a) if err != nil { diff --git a/internal/assets/assets.go b/internal/assets/assets.go index 1031948..364ef7e 100644 --- a/internal/assets/assets.go +++ b/internal/assets/assets.go @@ -226,7 +226,30 @@ import ( // None of them is wrong about its own subject, which is why they survived review: a // rule is read one file at a time and audited the same way. The cost is the same as a // command that does not exist — the agent does what the line says, once. -const Version = "29" +// +// 30: the standing cost, measured and cut. What a scaffolded Claude Code workspace +// preloads into every request had grown to 47KB — 15 rules and the entry file — while +// this file's own prose still said nine rules and ~26KB. Nothing about the +// methodology changed here; what changed is when three of its files arrive. +// `specs.md`, `tasks.md` and `knowledge-base.md` carry a `paths:` header, so the +// harness loads them when the agent touches the tree they govern rather than at +// session start: 10.4KB, about a quarter, off every request. The bar for scoping a +// rule is in Rules() and it is mechanical — a validator has to report what the rule +// prevents, so a rule arriving late is a rule whose failure `scc validate` catches +// before the commit. The entry file says which three, because it is the one document +// the agent is meant to trust about its own environment. +// +// The skill descriptions are the other half and were the unmeasured one: a body is +// paid by the run that invokes it, a description by every request whether the skill +// runs or not. Eight came to 4087 characters, mostly enumerating findings the +// validator already names — now 2084, with a budget test so the next one cannot +// quietly restore them. +// +// And the two review agents become `scc-code-review` and `scc-security-review`. +// Claude Code ships its own under the unprefixed names, so `delivery.md` naming them +// in prose resolved to whichever the harness picked — a collision nobody would ever +// see, because both produce a review and the one that ran is the one that answers. +const Version = "30" // The embedded tree. "all:" so nothing is silently dropped for having a name the // default embed pattern skips. @@ -253,8 +276,84 @@ const ( // Command is a slash command, on the same terms as Agent: shared body, // per-harness frontmatter. Command Kind = "command" + // Rule is one file of the methodology. Copied through like Plain, except on a + // harness that understands a `paths:` header, where a rule carrying one is + // rendered with it so the harness loads that rule on demand instead of at + // session start. See Rules for which rules carry one and why. + Rule Kind = "rule" ) +// rule is one file of the methodology and when the harness should load it. +type rule struct { + // name is the file under rules/. + name string + // scope is the globs this rule governs, or nil for a rule that is always on. + // + // A rule with a scope is one a harness may load only when the agent works with + // a matching file. That is a real change in when the agent has it — it arrives + // as the file is opened rather than before the decision to open it — so the + // bar for putting a scope here is stated once, in Rules, and applies to every + // entry. + scope []string +} + +// Rules is the methodology, in the order a unit of work meets it, and which of +// them the harness may leave until they are needed. +// +// **Everything here is preloaded on Claude Code and paid in every request of the +// session.** That is what the rules are for — a rule you have to decide to read is +// one the session that skips it does not have — and it is also the whole of what +// this product costs to run. Fifteen files come to ~44KB, and nothing about the +// methodology gets cheaper by being explained better. +// +// **So three of them carry a scope, and the bar is mechanical: a rule may be +// scoped only where a validator reports what it prevents.** The failure a scope +// introduces is the rule arriving after the decision it governs, and that is +// survivable exactly when something else catches the result — an EARS clause +// missing a part, a task with no methodology annotation, a broken wikilink, a +// dependency nobody recorded are all `scc validate` exit 2 before the commit. The +// three scoped rules are the three whose subject a validator reads. +// +// Every other rule stays on whatever it costs, and the ones that look most +// temptingly scopable are the clearest cases: +// +// - `artifacts.md` would scope to `plans/**`, and its instruction is **never +// open the plan**. A rule that loads when the agent opens the file is a rule +// that only ever arrives to say the thing it was meant to prevent has happened. +// - `prior-art.md` and `routing.md` fire before the first artifact exists, so +// there is no file for a glob to match. +// - `caveman.md`, `ladder.md`, `methodology.md`, `delivery.md` and +// `verification.md` govern how the agent works rather than what it edits, and +// nothing mechanical reports a failure to follow them. +// - `notes.md` is triggered by writing a comment in *source*, which is every +// file in the repository — a scope covering everything is a preloaded rule +// with extra steps. +func Rules() []rule { + return []rule{ + {name: "caveman.md"}, + {name: "routing.md"}, + {name: "autonomy.md"}, + {name: "prior-art.md"}, + {name: "methodology.md"}, + {name: "ladder.md"}, + // The task grammar: `(Unit)`/`(TDD)`, the number, the citation, the four + // flags. `spec.task-*` and `plan.task-*` report every part of it. + {name: "tasks.md", scope: []string{"specs/**", "plans/**"}}, + {name: "verification.md"}, + {name: "delivery.md"}, + // EARS, the delta form, the conditional design sections. The `spec` + // validator grades the requirement lines and the traceability both ways. + {name: "specs.md", scope: []string{"specs/**"}}, + // The wiki, ADRs, codewiki, glossary and stack — five validators between + // them, reporting broken links, orphans, numbering gaps, unresolved + // citations, avoided synonyms and undocumented dependencies. + {name: "knowledge-base.md", scope: []string{"docs/**"}}, + {name: "notes.md"}, + {name: "code-search.md"}, + {name: "artifacts.md"}, + } +} + // File is one workspace file scc scaffolds and then tracks. type File struct { // Name is the path inside the embedded tree, e.g. "rules/routing.md". @@ -268,6 +367,11 @@ type File struct { // Kind selects the rendering. Kind Kind + // Scope is a Rule's `paths:` list, empty for a rule that is always on. It + // reaches the file only on a harness whose ScopedRules says the header means + // something there. + Scope []string + // Owned marks a file the user owns from their first edit. scc writes it once // and records it, then leaves it alone: an upgrade reports that a new version // exists rather than merging into it. The entry file and the project rule are @@ -289,26 +393,12 @@ func Workspace(h paths.Harness) []File { } // The methodology. Every one of these is scc's own content: an upgrade should // deliver improvements to them, so none is Owned. - for _, rule := range []string{ - "caveman.md", - "routing.md", - "autonomy.md", - "prior-art.md", - "methodology.md", - "ladder.md", - "tasks.md", - "verification.md", - "delivery.md", - "specs.md", - "knowledge-base.md", - "notes.md", - "code-search.md", - "artifacts.md", - } { + for _, r := range Rules() { set = append(set, File{ - Name: "rules/" + rule, - Rel: under(h.RulesSeg, rule), - Kind: Plain, + Name: "rules/" + r.name, + Rel: under(h.RulesSeg, r.name), + Kind: Rule, + Scope: r.scope, }) } for _, agent := range ReviewAgents { @@ -439,7 +529,21 @@ func CodeGraphBlock() (string, error) { return Content(CodeGraphTemplate) } // ReviewAgents names the two subagents scc ships. Both read and neither writes: // review is where a cold context is worth paying for, and authorship is not. -var ReviewAgents = []string{"code-review", "security-review"} +// +// **Prefixed, because the unprefixed names are taken.** Claude Code ships its own +// `code-review` and `security-review`, so a workspace scaffolded with those names +// hands the model two different things under each one — and `delivery.md` names +// them in prose, which resolves to whichever the harness picked. That is not a +// collision anybody would see: both names produce a review, and the one that ran +// is the one that answers. The prefix is the same `scc-` the commands already +// carry, and it is what makes the rule's instruction name exactly one thing. +// +// Keeping them at all is a separate question with a separate answer: these two +// check what the built-in cannot, because they know the methodology. Gate 1 is +// ticked boxes against the code that was actually written, the standard is the +// artifact rather than taste, and the verdict comes back in a fixed shape the +// orchestrator branches on. Codex and opencode have no built-in either. +var ReviewAgents = []string{"scc-code-review", "scc-security-review"} // KnowledgeSkills names the skills that author a `docs/` artifact a validator // checks — one per artifact, so a workspace shipping the eight validators never @@ -546,6 +650,15 @@ type layout struct { // paths.Harness.PreloadsRules. RulesPreloaded bool + // RulesScoped says some of the rules carry a `paths:` header this harness + // reads, so "everything is already in context" is not true of the whole set. + // + // It is a field for the same reason RulesPreloaded is one: the entry file is + // the document the agent is meant to trust about its own environment, and a + // blanket "nothing to open" in a workspace where three rules are on demand + // teaches it that the file is wrong. See paths.Harness.ScopedRules. + RulesScoped bool + // The same three paths, trailing slash included, padded to the column the // entry file's layout block puts its descriptions in. // @@ -587,6 +700,7 @@ func layoutOf(h paths.Harness) layout { Agents: path.Join(h.Dir, h.AgentsSeg), Manifest: path.Join(h.Dir, paths.ManifestSeg), RulesPreloaded: h.PreloadsRules, + RulesScoped: h.ScopedRules, } l.RulesCol = column(l.Rules) l.SkillsCol = column(l.Skills) @@ -615,11 +729,33 @@ func Render(h paths.Harness, f File) (string, error) { return renderAgent(h, f, expanded) case Command: return renderCommand(h, expanded) + case Rule: + return renderRule(h, f, expanded), nil default: return expanded, nil } } +// renderRule puts a rule's `paths:` header on where the harness reads one. +// +// Only there, and that is the point of the field rather than a switch: on Codex +// and opencode nothing preloads the rules in the first place, so the header would +// buy nothing and cost every reader of that file the two lines it takes to work +// out that they are not YAML frontmatter it should act on. +func renderRule(h paths.Harness, f File, body string) string { + if !h.ScopedRules || len(f.Scope) == 0 { + return body + } + var b strings.Builder + b.WriteString("---\npaths:\n") + for _, p := range f.Scope { + fmt.Fprintf(&b, " - %q\n", p) + } + b.WriteString("---\n\n") + b.WriteString(body) + return b.String() +} + // expand runs a workspace template through text/template with the layout as its // only data. missingkey=error turns a typo'd field into a test failure rather // than the string "" landing in somebody's rules. diff --git a/internal/assets/assets_test.go b/internal/assets/assets_test.go index 9a8a36f..78a5b01 100644 --- a/internal/assets/assets_test.go +++ b/internal/assets/assets_test.go @@ -304,7 +304,11 @@ func TestRulesStayShortEnoughToBePreloaded(t *testing.T) { t.Fatalf("%s: %v", f.Name, err) } name := path.Base(f.Rel) - lines := strings.Count(strings.TrimRight(raw, "\n"), "\n") + 1 + // The prose, not the `paths:` header in front of it. The header is what + // takes a rule *out* of the preloaded set, so counting it against the + // budget the preloaded set exists to hold would charge a rule for the lines + // that stopped it being charged at all. + lines := strings.Count(strings.TrimRight(ruleBody(raw), "\n"), "\n") + 1 limit, capped := grandfathered[name] if !capped { @@ -974,7 +978,7 @@ var ladderInvariants = []string{ // have to survive the header synthesis as well as the editing. func TestTheLadderKeepsItsCarveOuts(t *testing.T) { for _, h := range paths.Harnesses() { - raw, err := renderRule(h, ladderRule) + raw, err := ruleAsShipped(h, ladderRule) if err != nil { t.Fatalf("%s: %v", h.ID, err) } @@ -1004,7 +1008,7 @@ func TestTheLadderKeepsItsCarveOuts(t *testing.T) { // elsewhere, and this is what keeps the route from being edited away separately // from the invariant that needs it. func TestTheLadderPointsTheRequirementSomewhereElse(t *testing.T) { - raw, err := renderRule(paths.Claude, ladderRule) + raw, err := ruleAsShipped(paths.Claude, ladderRule) if err != nil { t.Fatalf("%v", err) } @@ -1020,9 +1024,9 @@ func TestTheLadderPointsTheRequirementSomewhereElse(t *testing.T) { } } -// renderRule is one rule as a harness receives it, found by the name it ships +// ruleAsShipped is one rule as a harness receives it, found by the name it ships // under rather than by the path it lands at — which differs per harness. -func renderRule(h paths.Harness, name string) (string, error) { +func ruleAsShipped(h paths.Harness, name string) (string, error) { for _, f := range Workspace(h) { if f.Name == name { return Render(h, f) @@ -1058,7 +1062,7 @@ func (e errNoSuchRule) Error() string { // uses wherever a change is cheap to make and expensive to make by accident. func TestTheTemplateVersionMovesWithTheTemplates(t *testing.T) { // Bump Version, then replace this with the digest the failure prints. - const fingerprint = "d8eb76d69f2b335b93f20d492661f882e80ad7815d7a863a27c2631db05a7504" + const fingerprint = "32ec486ca64c66ba775896f352adf72c33cf50256770a4eb9e4b8480140d93f8" sum := sha256.New() // Version goes into the hash, and without it this test does not do the job its @@ -1093,3 +1097,115 @@ under one number, and the manifest can no longer tell a pristine file from a stale one.`, Version, got) } } + +// ruleBody is a rendered rule without the `paths:` header, when it has one. +func ruleBody(raw string) string { + const fence = "---\n" + if !strings.HasPrefix(raw, fence) { + return raw + } + rest := raw[len(fence):] + end := strings.Index(rest, "\n"+fence) + if end < 0 { + return raw + } + return strings.TrimLeft(rest[end+len("\n"+fence):], "\n") +} + +// A scoped rule is one whose failure something else reports, and that is the whole +// bar — so it is checked rather than trusted to the comment that states it. +// +// A scope moves a rule from "in context before the decision" to "in context when +// the file is opened", which is a real loss and is only survivable because +// `scc validate` catches the result. A rule scoped without that safety net is a +// rule the session that never opens the file simply does not have, with nothing +// downstream to notice. +func TestOnlyRulesAValidatorBacksAreScoped(t *testing.T) { + // The validator that reports what each scoped rule prevents. Adding a scope + // means adding a line here, which is the moment to check there really is one. + backed := map[string]string{ + "tasks.md": "spec and plan report a task line that breaks the grammar", + "specs.md": "spec reports EARS and traceability", + "knowledge-base.md": "wiki, adr, glossary, stack and codewiki report docs/", + } + for _, r := range Rules() { + if len(r.scope) == 0 { + continue + } + if _, ok := backed[r.name]; !ok { + t.Errorf("%s is scoped, so it reaches the agent only once a matching file is open — "+ + "name the validator that reports what it prevents, or take the scope off", r.name) + } + } + // And the scope has to reach the files the rule is about, or it is a rule + // nothing ever loads. + for _, r := range Rules() { + for _, g := range r.scope { + if !strings.Contains(g, "*") { + t.Errorf("%s: scope %q matches one path rather than a tree", r.name, g) + } + } + } +} + +// The scoped rules are scoped on Claude Code and nowhere else. +// +// On Codex and opencode nothing preloads rules/ at all, so there is nothing for a +// scope to save and the header would be two lines of a dialect their loaders do +// not read, in front of a file the agent was told to open. +func TestOnlyAHarnessThatReadsAScopeGetsOne(t *testing.T) { + for _, h := range paths.Harnesses() { + for _, f := range Workspace(h) { + if f.Kind != Rule || len(f.Scope) == 0 { + continue + } + raw, err := Render(h, f) + if err != nil { + t.Fatalf("%s/%s: %v", h.ID, f.Name, err) + } + got := strings.HasPrefix(raw, "---\npaths:\n") + if got != h.ScopedRules { + t.Errorf("%s: %s has a paths: header = %v, want %v (ScopedRules is %v)", + h.ID, f.Name, got, h.ScopedRules, h.ScopedRules) + } + } + } +} + +// A skill's description is the only part of it that is never free. +// +// The harness loads every skill's `description` at session start to decide when to +// invoke it, and leaves the body until it does — so the body is paid once, by the +// run that needed it, and the description is paid by every request of every +// session whether the skill is used or not. Eight of them at ~500 characters each +// is a preloaded rule nobody counted. +// +// 340 is generous for what a description has to do: say what the skill owns, and +// say when it fires. What it does not have to do is enumerate the findings it +// clears — `scc validate` names the rule, the skill's own body has the table, and +// a description listing three of six shapes is worse than one saying "any adr.* +// finding", which is both shorter and complete. +func TestSkillDescriptionsStayShortEnoughToPreload(t *testing.T) { + const budget = 340 + total := 0 + for _, skill := range Skills() { + raw, err := Content("skills/" + skill + "/SKILL.md") + if err != nil { + t.Fatalf("%s: %v", skill, err) + } + m, err := splitMeta(skill, raw) + if err != nil { + t.Fatalf("%s: %v", skill, err) + } + total += len(m.description) + if n := len(m.description); n > budget { + t.Errorf("skills/%s: description is %d characters, over the %d-character budget — "+ + "it is loaded into every request of every session, used or not", skill, n, budget) + } + } + // And the set as a whole, because eight descriptions each just under the + // budget is the same bill arriving in instalments. + if total > 2400 { + t.Errorf("the skill descriptions come to %d characters preloaded; keep the set under 2400", total) + } +} diff --git a/internal/assets/templates/agents/code-review.md b/internal/assets/templates/agents/scc-code-review.md similarity index 99% rename from internal/assets/templates/agents/code-review.md rename to internal/assets/templates/agents/scc-code-review.md index b9c80ba..470de65 100644 --- a/internal/assets/templates/agents/code-review.md +++ b/internal/assets/templates/agents/scc-code-review.md @@ -1,5 +1,5 @@ --- -name: code-review +name: scc-code-review description: Reviews a diff for correctness and quality against the task list, re-runs the feature's tests and lint, and reports. Use it after the last task of a spec or plan is verified and before opening the PR — never on your own work as the author. --- diff --git a/internal/assets/templates/agents/security-review.md b/internal/assets/templates/agents/scc-security-review.md similarity index 96% rename from internal/assets/templates/agents/security-review.md rename to internal/assets/templates/agents/scc-security-review.md index 3a3fc97..4a84829 100644 --- a/internal/assets/templates/agents/security-review.md +++ b/internal/assets/templates/agents/scc-security-review.md @@ -1,12 +1,12 @@ --- -name: security-review -description: Reviews a diff for exploitable weaknesses, attack-class agnostic — traces attacker-controlled input to effect and reports reachable paths. Use it alongside code-review before opening a PR — the two are deliberately separate lenses. +name: scc-security-review +description: Reviews a diff for exploitable weaknesses, attack-class agnostic — traces attacker-controlled input to effect and reports reachable paths. Use it alongside scc-code-review before opening a PR — the two are deliberately separate lenses. --- You review a diff for security defects and nothing else. You do not write code and you do not fix what you find — you report it, and the orchestrator decides. -Separate from `code-review` on purpose: **one reviewer asked for "everything" +Separate from `scc-code-review` on purpose: **one reviewer asked for "everything" reliably under-weights security**, because correctness findings are easier to produce and crowd it out. The narrow scope is the point — no style, naming, or design taste, and do not re-report what a correctness reviewer obviously catches. diff --git a/internal/assets/templates/entry.md b/internal/assets/templates/entry.md index 2f6e421..a127481 100644 --- a/internal/assets/templates/entry.md +++ b/internal/assets/templates/entry.md @@ -5,8 +5,9 @@ Spec-driven development, scaffolded and checked by `scc`. The methodology lives ## Rules — `{{.Rules}}/.md` {{if .RulesPreloaded -}} -{{.Label}} loads `{{.Rules}}/` at session start — nothing to open. The triggers say *when* -each governs: the failure is not a rule you never read, it is one you had and misapplied. +{{.Label}} loads `{{.Rules}}/` at session start — nothing to open{{if .RulesScoped}}, bar `specs.md`, +`tasks.md` and `knowledge-base.md`, read when you touch their tree{{end}}. The failure is not a +rule you never read, it is one you had and misapplied — so the triggers say *when* each governs. {{- else -}} Nothing loads these for you. Open the file whose moment has arrived, and open it again in a new session: a rule you read yesterday is not a rule you have read. @@ -35,8 +36,7 @@ Triggered by what you are about to touch: ## Ask the index before you read the file -**Code** — `scc graph query|explore `, or `codegraph_explore` where registered. -Read the source when you are about to change it, not to find it. +**Code** — `scc graph query|explore `; read the source to change it, not to find it. **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 diff --git a/internal/assets/templates/rules/delivery.md b/internal/assets/templates/rules/delivery.md index 9efe8d6..c9b110b 100644 --- a/internal/assets/templates/rules/delivery.md +++ b/internal/assets/templates/rules/delivery.md @@ -24,7 +24,7 @@ Once the last task is done: 1. **`scc validate --checks --pr`** on the integrated branch — artifacts, record and the four gates together, build first and stopping at the first failure; `2` is not done. Per-task runs cannot see breakage between tasks, and this is what `pre-push` runs. -2. **`code-review` and `security-review`** subagents on the diff, dispatched together. +2. **`scc-code-review` and `scc-security-review`** subagents on the diff, dispatched together. **Both, every time, on every PR** — not "when the change looks risky", which is the judgment call that ends with neither being run. Each returns a verdict, what it checked, and findings by severity; you fix from that report and do not re-review. diff --git a/internal/assets/templates/skills/adr/SKILL.md b/internal/assets/templates/skills/adr/SKILL.md index fd9adb5..12bfe0d 100644 --- a/internal/assets/templates/skills/adr/SKILL.md +++ b/internal/assets/templates/skills/adr/SKILL.md @@ -1,6 +1,6 @@ --- name: adr -description: Record a decision that is hard to reverse as a numbered ADR under docs/adr/, and supersede an old record rather than editing it. Use it when a choice is made that would be expensive to undo — storage, wire protocol, a module boundary, a vendor holding data, a framework the code will shape itself around — when someone asks why the project is built this way, and when `scc validate` reports adr.* findings such as a numbering gap, a superseded record with no successor, or a citation resolving to no record. +description: Record a decision that is hard to reverse as a numbered ADR under docs/adr/, and supersede rather than edit an old one. Use it when a choice would be expensive to undo — storage, a wire protocol, a module boundary, a vendor — and on any adr.* finding. --- You write the record of a decision, so that the reasoning survives the people who diff --git a/internal/assets/templates/skills/codewiki/SKILL.md b/internal/assets/templates/skills/codewiki/SKILL.md index 3e2616f..f0f0369 100644 --- a/internal/assets/templates/skills/codewiki/SKILL.md +++ b/internal/assets/templates/skills/codewiki/SKILL.md @@ -1,6 +1,6 @@ --- name: codewiki -description: Narrate an area of this codebase into docs/codewiki/, where every section cites the exact lines it explains. Use it when a subsystem is hard to enter cold and the code does not say why it is shaped that way, when someone asks for onboarding notes or an architecture walkthrough, and when `scc validate` reports codewiki.* findings — a citation that no longer resolves, one that runs past the end of its file, or a section that cites nothing at all. +description: Narrate an area of this codebase into docs/codewiki/, every section citing the lines it explains. Use it when a subsystem is hard to enter cold, when someone asks for an architecture walkthrough, and on any codewiki.* finding. --- You write prose that explains code, and every section of it points at the lines it diff --git a/internal/assets/templates/skills/glossary/SKILL.md b/internal/assets/templates/skills/glossary/SKILL.md index f127bc9..865a354 100644 --- a/internal/assets/templates/skills/glossary/SKILL.md +++ b/internal/assets/templates/skills/glossary/SKILL.md @@ -1,6 +1,6 @@ --- name: glossary -description: Own docs/glossary.md — one canonical term per concept, and the synonyms nobody should use for it. Use it when a domain term is coined, contested, fuzzy, or renamed; before naming a spec, a plan, a wiki page, or an identifier that will outlive the session; and when `scc validate` reports glossary.* findings — a duplicate entry, a term listed as both canonical and avoided, or an avoided synonym used somewhere in docs/. +description: Own docs/glossary.md — one canonical term per concept, and the synonyms nobody should use for it. Use it when a domain term is coined, contested or renamed, before naming anything that will outlive the session, and on any glossary.* finding. --- You own the project's vocabulary. Domain language drifts by default: two people diff --git a/internal/assets/templates/skills/init/SKILL.md b/internal/assets/templates/skills/init/SKILL.md index e1edc61..8ab4eba 100644 --- a/internal/assets/templates/skills/init/SKILL.md +++ b/internal/assets/templates/skills/init/SKILL.md @@ -1,6 +1,6 @@ --- name: init -description: Bootstrap this project's knowledge base from the code that already exists — survey the repository, then write docs/stack.md, docs/glossary.md, the wiki, the codewiki, the ADRs for decisions already taken, and the project rule's real build, test, and lint commands, and the delivery gate's four. Use it on a workspace whose docs/ is still the four seeded anchors, when someone asks to document an existing codebase, when a repository has just been scaffolded and nothing under docs/ is filled in, or when someone runs /scc-init. Not for one page or one new decision — the wiki, glossary, stack, codewiki, and adr skills each own their own artifact, and this run is what calls them. +description: Bootstrap this project's knowledge base from the code that already exists: survey the repository, then write the stack, glossary, wiki, codewiki and ADRs, and the project's real build, test, lint and format commands. Use it on a workspace whose docs/ is still the seeded anchors, or to document an existing codebase. --- You fill an empty knowledge base from a repository that already exists. diff --git a/internal/assets/templates/skills/plan-run/SKILL.md b/internal/assets/templates/skills/plan-run/SKILL.md index 1643936..4436222 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 — 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. +description: Drive a whole plan under plans/ to completion — brief it once, then implement group by group, asking scc for the next task rather than opening the file, and deliver per group or at the end. Resumes from the repository, not from memory. Use it to implement or finish an entire plan. --- You run a plan to the end. diff --git a/internal/assets/templates/skills/prd/SKILL.md b/internal/assets/templates/skills/prd/SKILL.md index 3077fc8..109dfab 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 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`. +description: Turn an initiative spanning more than one feature into a plan under plans/: a short header, a checklist, and references to the specs built separately. Use it for a PRD, a roadmap item, an epic, or an idea too large for one spec. --- You take an initiative that is too big for one spec and turn it into a plan: one diff --git a/internal/assets/templates/skills/stack/SKILL.md b/internal/assets/templates/skills/stack/SKILL.md index efc4de1..19a5daa 100644 --- a/internal/assets/templates/skills/stack/SKILL.md +++ b/internal/assets/templates/skills/stack/SKILL.md @@ -1,6 +1,6 @@ --- name: stack -description: Own docs/stack.md — every adopted technology with one line on why it earned its place. Use it before adding a dependency to go.mod, package.json, or any other manifest; when a technology is being swapped, dropped, or evaluated; and when `scc validate` reports stack.missing or stack.undocumented-dependency, which means something is installed that nobody recorded a decision about. +description: Own docs/stack.md — every adopted technology with one line on why it earned its place. Use it before adding a dependency to any manifest, when a technology is swapped or dropped, and on any stack.* finding. --- You own `docs/stack.md`. The rule it enforces is in @@ -8,9 +8,10 @@ You own `docs/stack.md`. The rule it enforces is in not listed here is an open decision, never something adopted silently.** This is checkable because dependency manifests are structured data. `scc validate` -reads the direct dependencies out of `go.mod` and `package.json` and reports any -that `stack.md` does not mention. Indirect dependencies are skipped — nobody decided -those. +reads the direct dependencies out of every manifest it knows — `go.mod`, +`package.json`, `requirements.txt`, `pyproject.toml`, `Cargo.toml`, +`composer.json`, `pom.xml` — and reports any that `stack.md` does not mention. +Indirect dependencies are skipped: nobody decided those. ## Adding a dependency is two steps diff --git a/internal/assets/templates/skills/wiki/SKILL.md b/internal/assets/templates/skills/wiki/SKILL.md index 87e0fee..6ccc3d2 100644 --- a/internal/assets/templates/skills/wiki/SKILL.md +++ b/internal/assets/templates/skills/wiki/SKILL.md @@ -1,6 +1,6 @@ --- name: wiki -description: Build and maintain docs/wiki/ — one page per concept, linked with wikilinks and reachable from index.md. Use it when a source lands in docs/raw/ and has to be distilled into a page, when a question is better answered from accumulated knowledge than from re-reading the code, and when `scc validate` reports wiki.* findings — a broken wikilink, an orphan page, a changelog naming a page that no longer exists, or a source still sitting unprocessed in docs/raw/. +description: Build and maintain docs/wiki/ — one page per concept, linked with wikilinks and reachable from index.md. Use it to distil a source from docs/raw/, to answer from accumulated knowledge, and on any wiki.* finding. --- You own `docs/wiki/`: the durable half of what this project knows. A spec says what diff --git a/internal/cli/brief_test.go b/internal/cli/brief_test.go index 1817507..87baa25 100644 --- a/internal/cli/brief_test.go +++ b/internal/cli/brief_test.go @@ -172,7 +172,11 @@ func TestNextSaysWhyThereIsNothingToDo(t *testing.T) { if code != ExitOK { t.Fatalf("--next on a finished plan = %d", code) } - if !strings.Contains(stdout, `"done": true`) { + var next struct { + Done bool `json:"done"` + } + decode(t, stdout, &next) + if !next.Done { t.Errorf("a finished plan must say so:\n%s", stdout) } diff --git a/internal/cli/check.go b/internal/cli/check.go index bc38946..dc50f8b 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -130,7 +130,7 @@ func runCheckRun(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } target, ok := resolveRoot(*root) if !ok || !requireWorkspace(target) { @@ -257,7 +257,7 @@ func runCheckSet(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } target, ok := resolveRoot(*root) if !ok || !requireWorkspace(target) { @@ -290,7 +290,11 @@ func runCheckSet(args []string) int { } // A --min with no command is how a floor is raised on a gate that already has // one; a command with no --min leaves the floor where it was. - if fs.Lookup("min").Value.String() != "0" { + // Whether the caller typed it, not whether the value came out non-zero: + // reading the parsed value makes `--min 0` indistinguishable from an absent + // flag, and 0 is the one floor somebody sets deliberately — a project turning + // the floor off while keeping the gate. + if isSet(fs, "min") { cfg.MinCoverage = *min } if cfg.Command(k) == gate.Skipped { @@ -331,7 +335,7 @@ func decide(args []string, cmd, value, said string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } target, ok := resolveRoot(*root) if !ok || !requireWorkspace(target) { @@ -371,7 +375,7 @@ func saveConfig(root string, cfg gate.Config, jsonOut bool, said string) int { gate.Config Floor float64 `json:"floor"` Files []string `json:"files"` - }{cfg, cfg.Floor(), relAll(wrote)}) + }{cfg, cfg.Floor(), relAll(root, wrote)}) } for _, f := range wrote { render.OK(fmt.Sprintf("%s — %s", finding.Rel(workspace.Relative(mustCwd(), f)), said)) @@ -386,7 +390,7 @@ func runCheckShow(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "check show") { return ExitError @@ -428,14 +432,16 @@ func runCheckShow(args []string) int { // relAll makes a list of absolute paths printable and stable across machines, // which is what a JSON consumer needs from a field naming files in a repository. -func relAll(paths []string) []string { +// +// Relative to the workspace root, which is what every other command's relPath +// answers. Against the working directory the same manifest came back as +// `.claude/scc-manifest.json` from the root and `../.claude/scc-manifest.json` +// from `specs/` — one field naming one file two ways, which is exactly what a +// consumer matching it against the manifest cannot do. +func relAll(root string, paths []string) []string { out := make([]string, 0, len(paths)) - cwd := mustCwd() for _, p := range paths { - if cwd != "" { - p = workspace.Relative(cwd, p) - } - out = append(out, finding.Rel(p)) + out = append(out, relPath(root, p)) } return out } diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 848485b..8c1dba8 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -76,6 +76,29 @@ func runStdin(t *testing.T, stdin string, args ...string) (stdout, stderr string return stdout, stderr, code } +// decode unmarshals a command's --json document into v. +// +// What these assertions are about is the shape of the document, never the +// whitespace between its keys: emitJSON indents for a terminal and writes compact +// for everything else, so a test capturing a pipe sees the compact spelling and a +// string match on `"count": 0` would be asserting on the reader rather than on the +// command. +func decode(t *testing.T, stdout string, v any) { + t.Helper() + if err := json.Unmarshal([]byte(stdout), v); err != nil { + t.Fatalf("could not decode the JSON document: %v\n%s", err, stdout) + } +} + +// emptyList is the shape `plan list` and `spec list` promise on an empty +// workspace: the key present, the array empty rather than null, and a zero count — +// so a caller can index it without a nil check. +type emptyList struct { + Plans []json.RawMessage `json:"plans"` + Specs []json.RawMessage `json:"specs"` + Count int `json:"count"` +} + func TestVersionPrintsStampedVersion(t *testing.T) { stdout, _, code := run(t, "version") if code != ExitOK { diff --git a/internal/cli/flags.go b/internal/cli/flags.go index c3bee85..7dfd706 100644 --- a/internal/cli/flags.go +++ b/internal/cli/flags.go @@ -1,34 +1,92 @@ package cli import ( + "errors" "flag" "fmt" "path/filepath" - "strings" "github.com/protonspy/spec-claude-code/internal/render" "github.com/protonspy/spec-claude-code/internal/workspace" ) -// parseFlags parses args and returns the positionals, allowing them to appear -// before the flags as well as after. +// parseFlags parses args and returns the positionals, wherever they sit among the +// flags. // // Go's flag package stops parsing at the first non-flag argument, so // `scc spec new user-auth --json` would otherwise read "--json" as a second // positional and silently ignore the flag. Every user and every agent writes the -// name first, so this is not a preference: leading positionals are collected, the -// remainder is parsed, and anything left over is appended. +// name first, so collecting the leading positionals and parsing the rest is the +// minimum. It is not enough on its own: a flag *after* a positional stops the +// parse just as dead, and the measured result was `scc notes add --tag gotcha +// "text" --path internal/x` writing "--path internal/x" into the note as prose, +// with no path recorded and exit 0. Silently misfiling an argument is worse than +// rejecting it, so this alternates — peel positionals, parse flags, repeat — until +// nothing is left. +// +// A bare `--` ends scc's own arguments. Everything after it is a positional +// verbatim, even spelled like a flag, which is what makes a literal `--text -x` +// reachable at all. func parseFlags(fs *flag.FlagSet, args []string) ([]string, error) { + var verbatim []string + for i, a := range args { + if a == "--" { + args, verbatim = args[:i], args[i+1:] + break + } + } var positionals []string - i := 0 - for i < len(args) && !strings.HasPrefix(args[i], "-") { - positionals = append(positionals, args[i]) - i++ + for len(args) > 0 { + i := 0 + for i < len(args) && !isFlag(args[i]) { + positionals = append(positionals, args[i]) + i++ + } + if err := fs.Parse(args[i:]); err != nil { + return nil, err + } + // flag stops at the first argument it does not recognize as a flag, which + // by isFlag's definition is one this loop will consume — so the next pass + // always makes progress and this terminates. + args = fs.Args() + } + return append(positionals, verbatim...), nil +} + +// isFlag reports whether flag would treat this argument as one. +// +// It matches flag's own test rather than approximating it with a "-" prefix, +// because the two disagree on exactly the argument that would hang the loop above: +// a bare "-" looks like a flag and is parsed as a positional. +func isFlag(a string) bool { + return len(a) > 1 && a[0] == '-' +} + +// helpWord rewrites a lone `help` positional into the flag that prints the usage. +// +// `scc validate help` is what somebody types having just typed `scc map help` and +// `scc check help`, which work because those commands have a subcommand to +// dispatch on. A leaf command has none, so the word arrived as a stray positional +// and the answer was "validate takes no arguments, got \"help\"" — correct, exit +// 1, and no use to anybody. +func helpWord(args []string) []string { + if len(args) == 1 && args[0] == "help" { + return []string{"-h"} } - if err := fs.Parse(args[i:]); err != nil { - return nil, err + return args +} + +// exitFor turns a parse failure into a command's exit code. +// +// `--help` is not a failure. flag reports it as one because it has nothing else to +// return, but the user got exactly what they asked for, and exit 1 there says the +// command could not run — which an agent branching on the 0/1/2 contract reads as +// a broken command rather than as the help it just printed. +func exitFor(err error) int { + if errors.Is(err, flag.ErrHelp) { + return ExitOK } - return append(positionals, fs.Args()...), nil + return ExitError } // rootFlagHelp is the single wording every command shows for --root, for the same diff --git a/internal/cli/flags_test.go b/internal/cli/flags_test.go new file mode 100644 index 0000000..d4794e4 --- /dev/null +++ b/internal/cli/flags_test.go @@ -0,0 +1,140 @@ +package cli + +import ( + "flag" + "os" + "path/filepath" + "strings" + "testing" +) + +// Help is what the user asked for, so it is exit 0. +// +// Every command here binds its own FlagSet and every one of them routed +// flag.ErrHelp to ExitError, which put `scc map tasks --help` and `scc validate +// --help` on the same exit code as a command that could not run. The 0/1/2 +// contract is what an agent branches on, so help reading as a failure is help an +// agent stops asking for. +func TestHelpIsNotAFailure(t *testing.T) { + root := mapWorkspace(t) + for _, args := range [][]string{ + {"map", "tasks", "--help"}, + {"map", "show", "--help"}, + {"validate", "--help"}, + {"patch", "check", "--help"}, + {"notes", "find", "--help"}, + {"notes", "add", "--help"}, + {"spec", "new", "--help"}, + {"plan", "new", "--help"}, + {"check", "set", "--help"}, + {"graph", "query", "--help"}, + {"hooks", "install", "--help"}, + {"init", "--help"}, + {"update", "--help"}, + {"launch", "--help"}, + {"rtk", "--help"}, + {"spec", "track", "--help"}, + } { + t.Run(strings.Join(args, " "), func(t *testing.T) { + _, stderr, code := run(t, append(args, "--root", root)...) + if code != ExitOK { + t.Errorf("%v: exit %d, want %d", args, code, ExitOK) + } + if !strings.Contains(stderr, "-") { + t.Errorf("%v: printed no usage:\n%s", args, stderr) + } + }) + } +} + +// A genuine parse failure is still exit 1. Without this the fix above would turn +// every unknown flag into a silent success. +func TestAnUnknownFlagIsStillAnError(t *testing.T) { + root := mapWorkspace(t) + if _, _, code := run(t, "map", "tasks", "sample", "--nonesuch", "--root", root); code != ExitError { + t.Errorf("exit %d, want %d for an unknown flag", code, ExitError) + } +} + +// A flag written after a positional is a flag, not prose. +// +// Go's flag package stops at the first non-flag argument, so the old single-pass +// parse read everything after the artifact name as more positionals. Measured +// before the fix: `notes add --tag gotcha "text" --path internal/x` wrote "--path +// internal/x" into the note body, recorded no path, and exited 0 — a misfiled +// record that reports success, which is worse than a rejected one. +func TestAFlagAfterAPositionalIsStillAFlag(t *testing.T) { + root := mapWorkspace(t) + if _, stderr, code := run(t, "notes", "add", "--tag", "gotcha", "a measured note", + "--path", "internal/cli/flags.go", "--root", root); code != ExitOK { + t.Fatalf("notes add: exit %d\n%s", code, stderr) + } + b, err := os.ReadFile(filepath.Join(root, "docs", "notes.md")) + if err != nil { + t.Fatal(err) + } + // The note that was just written, not the seeded guidance above it — which + // quotes `--path` in its own worked example. + var line string + for _, l := range strings.Split(string(b), "\n") { + if strings.Contains(l, "a measured note") { + line = l + } + } + if line == "" { + t.Fatalf("the note was not appended:\n%s", b) + } + if !strings.Contains(line, "@internal/cli/flags.go") { + t.Errorf("the --path was not recorded as a path:\n%s", line) + } + if strings.Contains(line, "--path") { + t.Errorf("the --path flag was written into the note as prose:\n%s", line) + } +} + +// parseFlags is driven directly for the shapes a command surface cannot reach: +// the terminator, a bare dash, and flags on both sides of a positional. +func TestParseFlagsFindsFlagsWhereverTheySit(t *testing.T) { + for _, tc := range []struct { + name string + args []string + want []string + tag string + on bool + fails bool + }{ + {name: "leading positional", args: []string{"x", "--tag", "a"}, want: []string{"x"}, tag: "a"}, + {name: "trailing positional", args: []string{"--tag", "a", "x"}, want: []string{"x"}, tag: "a"}, + {name: "flag after positional", args: []string{"--tag", "a", "x", "--on"}, want: []string{"x"}, tag: "a", on: true}, + {name: "positional between flags", args: []string{"--on", "x", "--tag", "a", "y"}, want: []string{"x", "y"}, tag: "a", on: true}, + {name: "terminator keeps a flag verbatim", args: []string{"x", "--", "--tag"}, want: []string{"x", "--tag"}}, + {name: "a bare dash is a positional", args: []string{"-", "--tag", "a"}, want: []string{"-"}, tag: "a"}, + {name: "unknown flag fails", args: []string{"x", "--nonesuch"}, fails: true}, + } { + t.Run(tc.name, func(t *testing.T) { + fs := flag.NewFlagSet("t", flag.ContinueOnError) + fs.SetOutput(&strings.Builder{}) + tag := fs.String("tag", "", "") + on := fs.Bool("on", false, "") + got, err := parseFlags(fs, tc.args) + if tc.fails { + if err == nil { + t.Fatalf("parseFlags(%v) = %v, want an error", tc.args, got) + } + return + } + if err != nil { + t.Fatalf("parseFlags(%v): %v", tc.args, err) + } + if strings.Join(got, " ") != strings.Join(tc.want, " ") { + t.Errorf("positionals = %v, want %v", got, tc.want) + } + if *tag != tc.tag { + t.Errorf("--tag = %q, want %q", *tag, tc.tag) + } + if *on != tc.on { + t.Errorf("--on = %v, want %v", *on, tc.on) + } + }) + } +} diff --git a/internal/cli/graph.go b/internal/cli/graph.go index 6e0d057..4cb1a52 100644 --- a/internal/cli/graph.go +++ b/internal/cli/graph.go @@ -64,7 +64,7 @@ func runGraphBuild(args []string) int { force := fs.Bool("force", false, "rebuild from scratch even when a graph is already there") rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "graph build") { return ExitError @@ -107,7 +107,7 @@ func runGraphSync(args []string) int { root := addRoot(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "graph sync") { return ExitError @@ -134,7 +134,7 @@ func runGraphStatus(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "graph status") { return ExitError @@ -204,7 +204,7 @@ func runGraphQuery(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } query, ok := graphQueryArg(rest, "query") if !ok { @@ -234,7 +234,7 @@ func runGraphExplore(args []string) int { root := addRoot(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } query, ok := graphQueryArg(rest, "explore") if !ok { diff --git a/internal/cli/graph_scope.go b/internal/cli/graph_scope.go index 6ffb998..01616cc 100644 --- a/internal/cli/graph_scope.go +++ b/internal/cli/graph_scope.go @@ -132,7 +132,7 @@ func runGraphScope(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "graph scope") { return ExitError @@ -185,7 +185,7 @@ func runGraphScopeSet(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } target, ok := resolveRoot(*root) if !ok || !requireWorkspace(target) { @@ -230,7 +230,7 @@ func writeScope(root string, scope []string, jsonOut bool) int { return emitJSON(struct { Scope []string `json:"scope"` Files []string `json:"files"` - }{orEmpty(scope), relAll(wrote)}) + }{orEmpty(scope), relAll(root, wrote)}) } said := fmt.Sprintf("scope: %s", strings.Join(scope, ", ")) if len(scope) == 0 { diff --git a/internal/cli/hooks.go b/internal/cli/hooks.go index 1baa63d..dfa0420 100644 --- a/internal/cli/hooks.go +++ b/internal/cli/hooks.go @@ -3,6 +3,7 @@ package cli import ( "flag" "fmt" + "io" "os" "path/filepath" "strings" @@ -10,6 +11,7 @@ import ( "github.com/protonspy/spec-claude-code/internal/attribution" "github.com/protonspy/spec-claude-code/internal/finding" "github.com/protonspy/spec-claude-code/internal/gate" + "github.com/protonspy/spec-claude-code/internal/git" "github.com/protonspy/spec-claude-code/internal/hooks" "github.com/protonspy/spec-claude-code/internal/render" "github.com/protonspy/spec-claude-code/internal/validate" @@ -114,7 +116,7 @@ func runHooksInstall(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "hooks install") { return ExitError @@ -168,7 +170,7 @@ func runHooksCheck(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "hooks check") { return ExitError @@ -201,7 +203,7 @@ func runHooksRemove(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "hooks remove") { return ExitError @@ -304,7 +306,7 @@ func runHooksRun(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if len(rest) == 0 { render.Err(fmt.Sprintf("hooks run: name a stage (%s)", strings.Join([]string{ @@ -325,18 +327,72 @@ func runHooksRun(args []string) int { case hooks.CommitMsg: return runCommitMsg(rest[1:], *jsonOut) case hooks.PrePush: + if !pushingAnything(hookStdin()) { + // git runs pre-push for a deletion and for a push that turns out to have + // nothing new, and it says which on stdin. Without reading it, `git push + // --delete` and `git push --tags` both paid for a full build, lint and + // test run to gate a push that carries no commits — minutes, for nothing, + // on the command somebody reaches for to tidy up. + return ExitOK + } return runGate(target, *jsonOut, true) case hooks.StageSessionStart, hooks.StageUserPrompt, hooks.StageStop: // A different protocol downstream of the same command: the harness hands // the event in on stdin and reads a document back, where git reads an exit // code. --json is ignored here because the output is already the harness's // JSON and there is no second shape to ask for. - return runAgentStage(target, stage, os.Stdin) + return runAgentStage(target, stage, hookStdin()) default: return runGate(target, *jsonOut, false) } } +// hookStdin is the hook's input, or an empty one when nobody is piping. +// +// The stages here are written for git and for the harness, both of which hand +// their input in on a pipe. A person running `scc hooks run stop` to see what it +// does — which the usage text invites — has a terminal on stdin, and a read of +// that blocks until they work out that it wants a Ctrl-D. An empty reader is the +// honest answer: there is no event, and every stage already degrades to silence +// when the event is not what it expected. +func hookStdin() io.Reader { + if isTerminal(os.Stdin) { + return strings.NewReader("") + } + return os.Stdin +} + +// pushingAnything reads git's pre-push input and reports whether any ref is +// actually being updated. +// +// One line per ref, ` `, and a +// local oid of all zeros is a deletion. No lines at all means git found nothing to +// push. Unreadable input, or a shape this does not recognize, answers yes: the +// gate running when it need not have costs time, and the gate not running when it +// should have is the thing it exists to prevent. +func pushingAnything(in io.Reader) bool { + raw, err := io.ReadAll(in) + if err != nil { + return true + } + body := strings.TrimSpace(string(raw)) + if body == "" { + // A person at a terminal, or a git that passed nothing. Not evidence that + // there is nothing to push. + return true + } + for _, line := range strings.Split(body, "\n") { + fields := strings.Fields(line) + if len(fields) < 2 { + return true + } + if strings.Trim(fields[1], "0") != "" { + return true + } + } + return false +} + // runGate is the pre-commit and pre-push check: the same run `scc validate` does, // reported the same way, exiting on the same contract. func runGate(root string, jsonOut, pr bool) int { @@ -365,9 +421,9 @@ func runGate(root string, jsonOut, pr bool) int { // repository that follows scc's rules without being one of its workspaces — scc's // own, for one — where the artifact validators have nothing to read and the rule // about what a commit may say still binds. -func gateFindings(root string, pr bool) (*finding.Set, error) { +func gateFindings(root string, pr bool, extra ...validate.Option) (*finding.Set, error) { if workspace.IsWorkspace(root) { - var opts []validate.Option + opts := append([]validate.Option{}, extra...) if pr { opts = append(opts, validate.WithPR()) // The suite runs here and only here. A push is where a branch becomes a @@ -425,7 +481,7 @@ func runCommitMsg(rest []string, jsonOut bool) int { return ExitError } set := &finding.Set{} - for _, hit := range attribution.Scan(commitMessage(string(raw))) { + for _, hit := range attribution.Scan(commitMessage(string(raw), git.CommentChar(filepath.Dir(path)))) { set.Addf(finding.Rel(path), hit.Line, "attribution."+hit.Rule, "%s — the work is the user's, and the record says so", hit.Match) } @@ -450,13 +506,28 @@ func runCommitMsg(rest []string, jsonOut bool) int { // the comment character and a verbose commit pastes the entire diff below the // scissors — a check reading either would report findings on text that never // reaches a commit, and report them on every commit. -func commitMessage(raw string) string { +// The comment character is asked of git rather than assumed to be `#`. A +// repository that sets `core.commentChar` to something else — `;` is the common +// choice, for people who write `#123` at the start of a line — keeps its template +// lines and its scissors line spelled with that character, and a check that went +// on looking for `#` read git's own template as part of the message and scanned +// the whole appended diff for signatures. +// +// `auto` is the one value that cannot be resolved by asking: git picks a character +// at write time from the ones the message does not already use. It falls back to +// `#`, which is what git picks unless the message has a line starting with one — +// and in that case the cost is a comment line read as content, which checks more +// text rather than less. +func commitMessage(raw, comment string) string { + if comment == "" || comment == "auto" { + comment = "#" + } var out []string for _, line := range strings.Split(strings.ReplaceAll(raw, "\r\n", "\n"), "\n") { - if strings.HasPrefix(line, scissors) { + if strings.HasPrefix(line, comment) && strings.Contains(line, scissorsBody) { break } - if strings.HasPrefix(line, "#") { + if strings.HasPrefix(line, comment) { continue } out = append(out, line) @@ -464,5 +535,7 @@ func commitMessage(raw string) string { return strings.Join(out, "\n") } -// scissors is what `git commit --verbose` puts above the diff it appends. -const scissors = "# ------------------------ >8 ------------------------" +// scissorsBody is what `git commit --verbose` writes between the comment +// character and the end of the line, above the diff it appends. Matched without +// the character in front of it, because that character is configuration. +const scissorsBody = "------------------------ >8 ------------------------" diff --git a/internal/cli/hooks_agent.go b/internal/cli/hooks_agent.go index 0e2dd43..1d3f26a 100644 --- a/internal/cli/hooks_agent.go +++ b/internal/cli/hooks_agent.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "os" + "path/filepath" "strings" "github.com/protonspy/spec-claude-code/internal/codegraph" @@ -12,6 +13,7 @@ import ( "github.com/protonspy/spec-claude-code/internal/gate" "github.com/protonspy/spec-claude-code/internal/git" "github.com/protonspy/spec-claude-code/internal/hooks" + "github.com/protonspy/spec-claude-code/internal/validate" "github.com/protonspy/spec-claude-code/internal/workspace" ) @@ -234,16 +236,61 @@ func stopContext(root string) []string { return out } -// stopFindings runs what a turn can afford: every validator that reads a file, -// and neither of the two that do not. The pull request is a network call and the -// test suite is a test suite — both belong at the push, which is where the hook -// that runs them is. +// stopFindings runs what a turn can afford and reports what a turn can act on. +// +// What it can afford: every validator that reads a file, and neither of the two +// that do not. The pull request is a network call and the test suite is a test +// suite — both belong at the push, which is where the hook that runs them is. +// +// What it can act on is the narrower half, and it is the Stop contract rather +// than a nicety, because speaking here continues the conversation. Two kinds of +// finding can never clear in the next turn and so would re-fire at the end of +// every turn for the life of the branch: +// +// - **a signature in a commit that is already made** — a rebase, not an edit, +// so `attribution` comes out of this run entirely. +// - **a finding in a file this branch never touched** — a migrated plan, a +// stale ADR, whatever the workspace was carrying before this work started. +// The agent is told to go and fix a file its task has nothing to do with, +// every turn, and correctly declines. +// +// Both are reported in full by `scc validate`, which is where somebody asking +// about the workspace gets the whole answer. Here the question is narrower: what +// did this turn just break. func stopFindings(root string) *finding.Set { - set, err := gateFindings(root, false) + set, err := gateFindings(root, false, validate.WithoutAttribution()) if err != nil { return nil } - return set + return touchedHere(root, set) +} + +// touchedHere keeps the findings that sit in a file this branch has changed. +// +// Its answer when git cannot say — no repository, an unborn HEAD, a base that +// will not resolve — is the whole set, unfiltered. That direction is deliberate: +// the filter exists to keep the stage from nagging about work nobody here did, +// and a workspace where the question cannot be asked is one where every finding +// is as likely as not to be this session's. +func touchedHere(root string, set *finding.Set) *finding.Set { + if set == nil || set.Empty() { + return set + } + changes, err := git.Changed(root, "") + if err != nil || len(changes) == 0 { + return set + } + touched := make(map[string]bool, len(changes)) + for _, c := range changes { + touched[finding.Rel(c.Path)] = true + } + out := &finding.Set{} + for _, f := range set.Sorted() { + if touched[finding.Rel(f.File)] { + out.Add(f) + } + } + return out } // stopFindingLines is the findings themselves, capped. @@ -275,7 +322,18 @@ func stopFindingLines(set *finding.Set) []string { // undeliveredLine says that finished work has not left the machine, and stops // there. It names the commands rather than running them: publishing is the // agent's act, in its next turn, where a person can see it happen. +// +// **Finished is the word that does the work, and a dirty tree is not it.** Said on +// the first commit of a unit of work, this asks for a push in the middle of it — +// and because Stop is a continuation, the ask is acted on: a push per turn, each +// one paying for the pre-push gate's build, lint and suite, and a pull request +// opened on a third of a feature. Uncommitted changes are the cheapest available +// evidence that the work is still in hand, so the line waits for them to be gone. +// A clean tree with commits the remote does not have is the state this is for. func undeliveredLine(root string) string { + if git.Dirty(root) { + return "" + } u := git.UndeliveredWork(root) if !u.Any() { return "" @@ -301,6 +359,45 @@ func firstLine(s string) string { return s } +// stale reports whether dir's graph is older than something inside it. +// +// It exists because this runs at the end of every turn, and CodeGraph is a node +// process: an unconditional sync spends one on a turn that answered a question and +// wrote nothing, and on Windows that is most of a second of the user waiting for a +// re-index of a tree nobody edited. The comparison is a stat per changed file +// rather than a walk, which is what keeps the check cheaper than the thing it is +// avoiding. +// +// The answer when it cannot tell — no `.codegraph` to stat, no list of changed +// files, a file that has since been deleted — is stale. Re-indexing needlessly +// costs a subprocess; skipping a sync that was needed leaves the agent with an +// index that answers confidently about code that changed, and the rule this +// implements says that is the worse of the two. +func stale(dir string, changed []git.Change) bool { + info, err := os.Stat(filepath.Join(dir, codegraph.Dir)) + if err != nil { + return true + } + if len(changed) == 0 { + return true + } + indexed := info.ModTime() + for _, c := range changed { + p := c.Path + if !filepath.IsAbs(p) { + p = filepath.Join(dir, filepath.FromSlash(p)) + } + fi, err := os.Stat(p) + if err != nil { + continue + } + if fi.ModTime().After(indexed) { + return true + } + } + return false +} + // syncGraph brings every scoped graph up to date, silently. // // This is the deterministic half of the staleness problem, and it is deliberately @@ -333,6 +430,11 @@ func syncGraph(root string) { if err != nil { return } + // What this branch has touched, asked once and reused per root. It is what + // makes the sync conditional: an index is re-run when a file it covers is + // newer than it is, and skipped otherwise. + changed, _ := git.Changed(root, "") + roots, _ := codegraph.Roots(root, scope) for _, r := range roots { // Only a tree that already has a graph. Building one from a hook would turn @@ -342,6 +444,9 @@ func syncGraph(root string) { if !r.Indexed() { continue } + if !stale(r.Dir, changed) { + continue + } // Output discarded rather than forwarded: stdout belongs to the hook's own // JSON document, and CodeGraph's progress bars are not an event the agent // needs. A failure is a stale graph, which the rule already covers. diff --git a/internal/cli/hooks_test.go b/internal/cli/hooks_test.go index 7cf2624..279f292 100644 --- a/internal/cli/hooks_test.go +++ b/internal/cli/hooks_test.go @@ -127,7 +127,7 @@ func TestCommitMsgIgnoresWhatGitStrips(t *testing.T) { path := filepath.Join(t.TempDir(), "COMMIT_EDITMSG") msg := "feat: a real subject\n" + "# Co-Authored-By: Claude \n" + - scissors + "\n" + + "# " + scissorsBody + "\n" + "diff --git a/x b/x\n+\U0001F916 Generated with [Claude Code](https://claude.com/claude-code)\n" if err := os.WriteFile(path, []byte(msg), 0o644); err != nil { t.Fatalf("write: %v", err) @@ -137,6 +137,63 @@ func TestCommitMsgIgnoresWhatGitStrips(t *testing.T) { } } +// The comment character is configuration, and a repository that changed it still +// has to have its template and its scissors recognized. +// +// `core.commentChar=;` is what somebody sets who writes `#123` at the start of a +// line. With `#` compiled in, git's template read as part of the message and the +// whole appended diff was scanned for signatures — so a commit whose *diff* +// happened to add a generated-with footer to a file was refused, and a real +// signature under `;` went through. +func TestCommitMessageReadsTheConfiguredCommentCharacter(t *testing.T) { + msg := "feat: a real subject\n" + + "; Co-Authored-By: Claude \n" + + "; " + scissorsBody + "\n" + + "diff --git a/x b/x\n+Generated with Claude Code\n" + got := commitMessage(msg, ";") + if strings.Contains(got, "Co-Authored-By") { + t.Errorf("a comment line was read as the message:\n%s", got) + } + if strings.Contains(got, "diff --git") { + t.Errorf("the verbose diff was read as the message:\n%s", got) + } + if !strings.Contains(got, "a real subject") { + t.Errorf("the subject was stripped:\n%s", got) + } + + // Unset and "auto" both fall back to git's default. + hash := "feat: x\n# a comment\n# " + scissorsBody + "\ndiff --git a/x b/x\n" + for _, comment := range []string{"", "auto", "#"} { + got := commitMessage(hash, comment) + if strings.Contains(got, "a comment") || strings.Contains(got, "diff --git") { + t.Errorf("commitMessage(_, %q) kept what git strips:\n%s", comment, got) + } + } +} + +// git says on stdin what a push carries, and a push carrying nothing is not worth +// a build, a lint and a full suite. `git push --delete` and a push with nothing +// new both paid for one before this. +func TestPrePushSkipsAPushThatCarriesNothing(t *testing.T) { + zero := strings.Repeat("0", 40) + for _, tc := range []struct { + name string + in string + want bool + }{ + {"a deletion", "(delete) " + zero + " refs/heads/gone abc123\n", false}, + {"two deletions", "(delete) " + zero + " refs/heads/a abc\n(delete) " + zero + " refs/heads/b def\n", false}, + {"a real push", "refs/heads/main abc123 refs/heads/main def456\n", true}, + {"a delete beside a real push", "(delete) " + zero + " refs/heads/a abc\nrefs/heads/b abc123 refs/heads/b def\n", true}, + {"nothing on stdin", "", true}, + {"a shape this does not know", "garbage\n", true}, + } { + if got := pushingAnything(strings.NewReader(tc.in)); got != tc.want { + t.Errorf("%s: pushingAnything = %v, want %v", tc.name, got, tc.want) + } + } +} + // TestInstalledHooksSurviveARemove, and the workspace says so at every step. The // point is that check and remove agree with install about what is there. func TestHooksInstallCheckRemoveAgree(t *testing.T) { diff --git a/internal/cli/init.go b/internal/cli/init.go index d753698..448a6ef 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -45,9 +45,9 @@ func runInit(args []string) int { picks[h.ID] = fs.Bool(h.ID, false, "scaffold for "+h.ID+" ("+h.EntryFile+", "+h.Dir+"/)") } jsonOut := addJSON(fs) - rest, err := parseFlags(fs, args) + rest, err := parseFlags(fs, helpWord(args)) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "init") { return ExitError diff --git a/internal/cli/jsonout.go b/internal/cli/jsonout.go index 6c35c15..8318281 100644 --- a/internal/cli/jsonout.go +++ b/internal/cli/jsonout.go @@ -4,6 +4,7 @@ import ( "encoding/json" "flag" "fmt" + "os" "github.com/protonspy/spec-claude-code/internal/render" ) @@ -19,14 +20,28 @@ func addJSON(fs *flag.FlagSet) *bool { return fs.Bool("json", false, jsonFlagHelp) } -// emitJSON writes v as indented JSON to stdout and returns the exit code. +// emitJSON writes v as JSON to stdout and returns the exit code. // // stdout carries nothing but the JSON document: diagnostics go to stderr (see // package render), so a caller can pipe stdout straight into jq while still // seeing warnings on the terminal. A marshal failure is the tool's own bug, so // it reports on stderr and exits 1 rather than emitting half a document. +// +// Indented for a person, compact for everything else — the same test render +// already makes about color, and for the same reason. The indentation is there to +// be read, and the overwhelming reader of a `--json` document here is an agent +// that pays for every byte of it: measured on a six-task plan, `map tasks --json` +// is 2198 bytes indented and about a third of that is leading whitespace, against +// 599 bytes for the human listing of the same tasks. A terminal still gets the +// readable form, so nothing anybody looks at changes. func emitJSON(v any) int { - b, err := json.MarshalIndent(v, "", " ") + var b []byte + var err error + if isTerminal(os.Stdout) { + b, err = json.MarshalIndent(v, "", " ") + } else { + b, err = json.Marshal(v) + } if err != nil { render.Err(fmt.Sprintf("could not encode JSON output: %v", err)) return ExitError diff --git a/internal/cli/launch.go b/internal/cli/launch.go index d5cc41f..4e47c31 100644 --- a/internal/cli/launch.go +++ b/internal/cli/launch.go @@ -82,9 +82,9 @@ func runLaunch(args []string) int { yes := fs.Bool("yes", false, "answer the install prompts with yes, for an unattended run") dryRun := fs.Bool("dry-run", false, "print the command this would run, and run nothing") jsonOut := addJSON(fs) - rest, err := parseFlags(fs, own) + rest, err := parseFlags(fs, helpWord(own)) if err != nil { - return ExitError + return exitFor(err) } mcpMode, err := headroom.ParseMCPMode(*mcp) if err != nil { diff --git a/internal/cli/map.go b/internal/cli/map.go index 7476169..228599e 100644 --- a/internal/cli/map.go +++ b/internal/cli/map.go @@ -125,7 +125,7 @@ func runMapIndex(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "map index") { return ExitError @@ -196,7 +196,7 @@ func runMapOutline(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if len(rest) == 0 { render.Err("map outline needs an artifact: a path, a plan name, or a feature name") @@ -315,7 +315,7 @@ 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, "the one task to work on now: eligible, most urgent, lowest number") + next := fs.Bool("next", false, "the one task to work on now: eligible, most urgent, lowest number (named no artifact, the first with an open task, plans before specs — the answer names the file it came from)") 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") @@ -325,7 +325,7 @@ func runMapTasks(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if *open && *done { render.Err("--open and --done ask for opposite things") @@ -675,7 +675,7 @@ func runMapBrief(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if len(rest) == 0 { render.Err("map brief needs an artifact: a path, a plan name, or a feature name") @@ -796,7 +796,7 @@ func runMapShow(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if len(rest) < 2 { render.Err("map show needs an artifact and at least one address: `map show plans/x.md 1.2`") @@ -862,7 +862,7 @@ func runMapBlocks(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if len(rest) == 0 { render.Err("map blocks needs an artifact") @@ -918,7 +918,7 @@ func runMapFind(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if len(rest) == 0 { render.Err("map find needs something to look for") @@ -974,7 +974,7 @@ func runMapTrace(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if len(rest) != 1 { render.Err("map trace takes one reference: a requirement (R1.2) or a spec (specs//)") diff --git a/internal/cli/migrate.go b/internal/cli/migrate.go index acbc354..f692c87 100644 --- a/internal/cli/migrate.go +++ b/internal/cli/migrate.go @@ -93,7 +93,7 @@ func runPlanMigrate(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } name, ok := artifactName(rest, "plan") if !ok { diff --git a/internal/cli/notes.go b/internal/cli/notes.go index 631d6c3..e2fbab4 100644 --- a/internal/cli/notes.go +++ b/internal/cli/notes.go @@ -94,7 +94,7 @@ func runNotesAdd(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } // The text is the positionals joined, so an unquoted sentence arrives as the // sentence it was typed as. `scc graph explore` does the same, for the same @@ -297,7 +297,7 @@ func runNotesFind(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } target, ok := resolveRoot(*root) if !ok || !requireWorkspace(target) { @@ -363,7 +363,7 @@ func runNotesShow(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if len(rest) != 1 { render.Err(fmt.Sprintf("expected exactly one note id, got %d", len(rest))) @@ -411,7 +411,7 @@ func runNotesIndex(args []string, which string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "notes "+which) { return ExitError @@ -453,7 +453,7 @@ func runNotesRemove(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if len(rest) != 1 { render.Err(fmt.Sprintf("expected exactly one note id, got %d", len(rest))) diff --git a/internal/cli/patch.go b/internal/cli/patch.go index 0892f78..73cea87 100644 --- a/internal/cli/patch.go +++ b/internal/cli/patch.go @@ -118,7 +118,7 @@ func runPatchCheck(args []string, done bool) int { pf := addPatchFlags(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if len(rest) < 2 { render.Err(name + " needs an artifact and at least one task number") @@ -144,7 +144,7 @@ func runPatchTask(args []string) int { 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 + return exitFor(err) } if len(rest) != 2 { render.Err("patch task needs an artifact and one task number") @@ -240,7 +240,7 @@ func runPatchAdd(args []string) int { reason := fs.String("reason", "", "why this turned up after the plan was approved") rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if len(rest) != 1 { render.Err("patch add needs an artifact") @@ -316,7 +316,7 @@ func runPatchRemove(args []string) int { 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 + return exitFor(err) } if len(rest) != 2 { render.Err("patch rm needs an artifact and one task number") @@ -344,7 +344,7 @@ func runPatchText(op string, args []string) int { file := fs.String("file", "", "read the text from this `path` instead") rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if len(rest) != 2 { render.Err(fmt.Sprintf("patch %s needs an artifact and one address", op)) @@ -376,7 +376,7 @@ func runPatchFrontmatter(args []string) int { pf := addPatchFlags(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if len(rest) < 2 { render.Err("patch fm needs an artifact and at least one key=value") diff --git a/internal/cli/plan.go b/internal/cli/plan.go index e9dd886..c505e2b 100644 --- a/internal/cli/plan.go +++ b/internal/cli/plan.go @@ -65,7 +65,7 @@ func runPlanNew(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if !kick.validate() { return ExitError @@ -146,7 +146,7 @@ func runPlanApprove(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } name, ok := artifactName(rest, "plan") if !ok { @@ -217,7 +217,7 @@ func runPlanReseal(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } name, ok := artifactName(rest, "plan") if !ok { @@ -302,7 +302,7 @@ func runPlanList(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "plan list") { return ExitError @@ -358,7 +358,7 @@ func runPlanDelete(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } name, ok := artifactName(rest, "plan") if !ok { diff --git a/internal/cli/plan_test.go b/internal/cli/plan_test.go index 6c5facd..fe7533f 100644 --- a/internal/cli/plan_test.go +++ b/internal/cli/plan_test.go @@ -137,7 +137,9 @@ func TestPlanListOnAnEmptyWorkspace(t *testing.T) { if code != ExitOK { t.Fatalf("exit = %d", code) } - if !strings.Contains(stdout, `"plans": []`) || !strings.Contains(stdout, `"count": 0`) { + var got emptyList + decode(t, stdout, &got) + if got.Plans == nil || len(got.Plans) != 0 || got.Count != 0 { t.Errorf("empty list = %s, want an empty array and a zero count", stdout) } } @@ -156,7 +158,9 @@ func TestPlanListIgnoresNonPlans(t *testing.T) { if code != ExitOK { t.Fatalf("exit = %d", code) } - if !strings.Contains(stdout, `"count": 0`) { + var got emptyList + decode(t, stdout, &got) + if got.Count != 0 { t.Errorf("list picked up something that is not a plan: %s", stdout) } } diff --git a/internal/cli/rtk.go b/internal/cli/rtk.go index 02653f5..97bfeb7 100644 --- a/internal/cli/rtk.go +++ b/internal/cli/rtk.go @@ -28,9 +28,9 @@ func runRTK(args []string) int { noInstall := fs.Bool("no-install", false, "never run cargo; only write the block") keep := fs.Bool("keep", false, "leave an existing block alone (default: replace it with the one this scc ships)") jsonOut := addJSON(fs) - rest, err := parseFlags(fs, args) + rest, err := parseFlags(fs, helpWord(args)) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "rtk") { return ExitError diff --git a/internal/cli/seal.go b/internal/cli/seal.go index 7894fab..2078857 100644 --- a/internal/cli/seal.go +++ b/internal/cli/seal.go @@ -20,8 +20,16 @@ import ( // resealed on top, and the evidence would be gone in the same command that should // have reported it. +// addNoVerify binds the flag that skips the seal check. +// +// Spelled `--no-seal-check` rather than `--no-verify`, which is what it was: git +// spells "skip the hooks" `--no-verify`, scc installs git hooks, and an agent that +// knows git reaches for it here expecting to get past `scc validate`. What it +// would actually get is a plan's tamper-evidence turned off while every validator +// still ran — the opposite of the ask, silently. The name says which check it +// means; `SCC_SKIP_HOOKS=1` is the one that skips hooks. func addNoVerify(fs *flag.FlagSet) *bool { - return fs.Bool("no-verify", false, "skip the seal check on an approved plan (for diagnosis)") + return fs.Bool("no-seal-check", false, "skip the seal check on an approved plan (for diagnosis)") } // loadVerified is loadMany plus the seal check — the read path for every command diff --git a/internal/cli/seal_test.go b/internal/cli/seal_test.go index b96952c..8c19522 100644 --- a/internal/cli/seal_test.go +++ b/internal/cli/seal_test.go @@ -110,9 +110,9 @@ func TestDriftIsReportedAndActionable(t *testing.T) { 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) + // --no-seal-check is the diagnosis escape hatch, and nothing else. + if _, _, code := run(t, "map", "tasks", "sweep", "--root", root, "--no-seal-check"); code != ExitOK { + t.Errorf("--no-seal-check = %d", code) } if _, _, code := run(t, "plan", "reseal", "sweep", "--root", root); code != ExitError { diff --git a/internal/cli/spec.go b/internal/cli/spec.go index 3ea8f91..281fc7a 100644 --- a/internal/cli/spec.go +++ b/internal/cli/spec.go @@ -71,7 +71,7 @@ func runSpecNew(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if !kick.validate() { return ExitError @@ -175,7 +175,7 @@ func runSpecList(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "spec list") { return ExitError @@ -242,7 +242,7 @@ func runSpecShow(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } name, ok := artifactName(rest, "spec") if !ok { @@ -287,7 +287,7 @@ func runSpecDelete(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } name, ok := artifactName(rest, "spec") if !ok { diff --git a/internal/cli/spec_test.go b/internal/cli/spec_test.go index a948f69..9fdd99e 100644 --- a/internal/cli/spec_test.go +++ b/internal/cli/spec_test.go @@ -184,7 +184,9 @@ func TestSpecListOnAnEmptyWorkspace(t *testing.T) { if code != ExitOK { t.Fatalf("exit = %d", code) } - if !strings.Contains(stdout, `"specs": []`) || !strings.Contains(stdout, `"count": 0`) { + var got emptyList + decode(t, stdout, &got) + if got.Specs == nil || len(got.Specs) != 0 || got.Count != 0 { t.Errorf("empty list = %s, want an empty array and a zero count", stdout) } } diff --git a/internal/cli/track.go b/internal/cli/track.go index 6852a7b..012fd27 100644 --- a/internal/cli/track.go +++ b/internal/cli/track.go @@ -48,7 +48,7 @@ func runSpecTrack(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } name, ok := artifactName(rest, "spec") if !ok { @@ -277,7 +277,7 @@ func runSpecSync(args []string) int { jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { - return ExitError + return exitFor(err) } target, ok := resolveRoot(*root) if !ok || !requireWorkspace(target) { diff --git a/internal/cli/update.go b/internal/cli/update.go index 82f79c0..4e17c9e 100644 --- a/internal/cli/update.go +++ b/internal/cli/update.go @@ -41,9 +41,9 @@ func runUpdate(args []string) int { picks[h.ID] = fs.Bool(h.ID, false, "update only the "+h.ID+" tree") } jsonOut := addJSON(fs) - rest, err := parseFlags(fs, args) + rest, err := parseFlags(fs, helpWord(args)) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "update") { return ExitError diff --git a/internal/cli/validate.go b/internal/cli/validate.go index b448b8b..912d2bc 100644 --- a/internal/cli/validate.go +++ b/internal/cli/validate.go @@ -27,9 +27,9 @@ func runValidation(subject string, args []string, fn func(root string, rest []st fs.SetOutput(os.Stderr) root := addRoot(fs) jsonOut := addJSON(fs) - rest, err := parseFlags(fs, args) + rest, err := parseFlags(fs, helpWord(args)) if err != nil { - return ExitError + return exitFor(err) } target, ok := resolveRoot(*root) if !ok || !requireWorkspace(target) { diff --git a/internal/cli/validate_all.go b/internal/cli/validate_all.go index 7f8059a..5b85dd2 100644 --- a/internal/cli/validate_all.go +++ b/internal/cli/validate_all.go @@ -32,9 +32,9 @@ func runValidateAll(args []string) int { // pull request — and a bare pre-commit run never waits on a build. withChecks := fs.Bool("checks", false, "also run this workspace's build, format, lint and test commands") jsonOut := addJSON(fs) - rest, err := parseFlags(fs, args) + rest, err := parseFlags(fs, helpWord(args)) if err != nil { - return ExitError + return exitFor(err) } if !noPositionals(rest, "validate") { return ExitError @@ -70,13 +70,20 @@ func runValidateAll(args []string) int { // Counts per validator first, then the findings themselves. "Few findings, each // fixable" is easiest to violate right here, where every check reports at once — // so the shape of the output has to carry the summary before the detail. + // + // Only the validators that found something, though. A clean run printed twelve + // lines to say nothing happened, eleven of them a validator's name beside a + // zero, and this is the command the pre-commit hook runs and an agent reads back + // on every commit. What a reader needs from a clean run is the one line saying + // it was clean; what they need from a dirty one is which check fired, and that + // line is still here. for _, r := range results { - line := fmt.Sprintf("%-12s %d", r.Name, r.Findings) - if r.Findings == 0 { - render.Info(line) - continue + if r.Findings > 0 { + render.Warn(fmt.Sprintf("%-12s %d", r.Name, r.Findings)) } - render.Warn(line) + } + if set.Empty() { + render.Info(fmt.Sprintf("%d validators · 0 findings", len(results))) } set.Report("validate") return set.ExitCode() diff --git a/internal/git/git.go b/internal/git/git.go index 093e017..914e214 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -395,10 +395,44 @@ func parseLog(out string) []Commit { // it. Absence is a normal answer here as everywhere else in this package: a // directory that is not a repository has no hooks directory, which is not an // error to propagate. -func HooksDir(dir string) (string, error) { - if p, err := run(Bin, dir, "config", "--get", "core.hooksPath"); err == nil && p != "" { - return absolute(dir, p), nil +// Dirty reports whether the working tree has uncommitted changes, tracked or not. +// +// One `status --porcelain`, which is the cheapest question in this package that +// reads the tree at all. Absence is a normal answer as everywhere else here: no +// git, no repository, a status that will not run — all answer false, because a +// caller asking "is this work finished" gets a less useful answer from an error +// than from "as far as can be told, yes". +func Dirty(dir string) bool { + out, err := run(Bin, dir, "status", "--porcelain", "--untracked-files=normal") + if err != nil { + return false + } + return strings.TrimSpace(out) != "" +} + +// CommentChar is what this repository puts in front of a comment line in a commit +// message template, from `core.commentChar`. +// +// Empty when it is unset, when git is not here, or when the question cannot be +// asked — all of which the caller reads as the default. The literal string "auto" +// comes back as itself rather than as a guess: git resolves that at write time +// against the message being written, and a caller that has the message is in a +// better position to decide what to do about it than this is. +func CommentChar(dir string) string { + out, err := run(Bin, dir, "config", "--get", "core.commentChar") + if err != nil { + return "" } + return strings.TrimSpace(out) +} + +func HooksDir(dir string) (string, error) { + // `rev-parse --git-path hooks` already honours core.hooksPath, and it is the + // only one of the two that expands the `~` people actually write there. Asking + // `config --get` first looked like the more direct question and answered a + // configured `~/.githooks` with the literal string, which joined to + // `/~/.githooks` — a directory scc would create, write three hooks into, + // and report success for, while git went on reading the real one. out, err := run(Bin, dir, "rev-parse", "--git-path", "hooks") if err != nil { return "", err diff --git a/internal/hooks/harness.go b/internal/hooks/harness.go index b530b21..e644625 100644 --- a/internal/hooks/harness.go +++ b/internal/hooks/harness.go @@ -399,7 +399,24 @@ func mine(raw json.RawMessage) bool { return false } for _, h := range obj.Hooks { - if strings.Contains(h.Command, Prog+" hooks run ") { + if ours(h.Command) { + return true + } + } + return false +} + +// ours reports whether a settings entry's command is one scc wrote. +// +// This decides what a remove takes out and what an install replaces, so a loose +// match is scc editing somebody else's entry. A substring test claimed +// `my-scc hooks run stop` and `scc hooks run stop | tee log` alike; the command is +// one scc composes itself, so it can be recognized exactly — trimmed for the +// whitespace a hand-edited settings file may carry, and nothing else. +func ours(cmd string) bool { + cmd = strings.TrimSpace(cmd) + for _, st := range AgentStages() { + if cmd == command(st) { return true } } diff --git a/internal/hooks/hooks.go b/internal/hooks/hooks.go index be4962f..31f9b9c 100644 --- a/internal/hooks/hooks.go +++ b/internal/hooks/hooks.go @@ -35,6 +35,7 @@ import ( "errors" "fmt" "os" + "path" "path/filepath" "strings" @@ -326,14 +327,34 @@ func write(path, body string) error { // posix reports whether a script scc is about to append to is one git will run // with a POSIX shell. +// It reads the interpreter out of the shebang rather than matching the line's +// tail, because a shebang carries arguments: `#!/bin/sh -e` and `#!/usr/bin/env +// bash -eu` are both ordinary hooks, and both were called non-POSIX and refused +// even under --force. `env` is stepped over, since that is where the interpreter +// name sits when it is used. func posix(doc string) bool { line, _, _ := strings.Cut(strings.TrimSpace(doc), "\n") if !strings.HasPrefix(line, "#!") { // No shebang: git runs it with sh, which is what the block expects. return true } + fields := strings.Fields(strings.TrimPrefix(line, "#!")) + if len(fields) == 0 { + return true + } + interp := path.Base(filepath.ToSlash(fields[0])) + if interp == "env" && len(fields) > 1 { + // `env` may carry its own options before the program it runs. + for _, f := range fields[1:] { + if strings.HasPrefix(f, "-") || strings.Contains(f, "=") { + continue + } + interp = path.Base(filepath.ToSlash(f)) + break + } + } for _, sh := range []string{"sh", "bash", "dash", "zsh", "ksh"} { - if strings.HasSuffix(strings.TrimSpace(line), "/"+sh) || strings.HasSuffix(strings.TrimSpace(line), " "+sh) { + if interp == sh { return true } } diff --git a/internal/hooks/hooks_test.go b/internal/hooks/hooks_test.go index b2ba48e..733b91c 100644 --- a/internal/hooks/hooks_test.go +++ b/internal/hooks/hooks_test.go @@ -307,3 +307,37 @@ func TestTheHarnessStagesOnlyEverReport(t *testing.T) { } } } + +// A shebang carries arguments, and an interpreter with one is still that +// interpreter. +// +// posix matched the tail of the shebang line, so `#!/bin/sh -e` — an ordinary +// hardening flag on an ordinary hook — read as a script written in something scc +// must not append shell to, and `--force` refused it. The test states the +// interpreters rather than the spellings, because the spellings are what was +// wrong. +func TestPosixReadsTheInterpreterNotTheLineEnding(t *testing.T) { + for _, tc := range []struct { + line string + want bool + }{ + {"#!/bin/sh", true}, + {"#!/bin/sh -e", true}, + {"#!/bin/sh -eu", true}, + {"#!/usr/bin/env sh", true}, + {"#!/usr/bin/env bash", true}, + {"#!/usr/bin/env bash -eu", true}, + {"#!/usr/bin/env -S bash -eu", true}, + {"#!/bin/bash", true}, + {"#!/usr/local/bin/zsh -f", true}, + {"", true}, // no shebang: git runs it with sh + {"#!/usr/bin/env python3", false}, + {"#!/usr/bin/python", false}, + {"#!/usr/bin/env node", false}, + {"#!/usr/bin/perl -w", false}, + } { + if got := posix(tc.line + "\necho hi\n"); got != tc.want { + t.Errorf("posix(%q) = %v, want %v", tc.line, got, tc.want) + } + } +} diff --git a/internal/paths/paths.go b/internal/paths/paths.go index 63637e2..e117826 100644 --- a/internal/paths/paths.go +++ b/internal/paths/paths.go @@ -109,6 +109,28 @@ type Harness struct { // Verifiable per harness: `/context` in Claude Code lists what loaded. PreloadsRules bool + // ScopedRules is whether this harness understands a `paths:` list in a rule's + // frontmatter — load this rule only when working with files that match, rather + // than at session start with everything else. + // + // It is the only lever there is on what the methodology costs. A preloaded + // rule is paid in every request of the session, and the set is 15 files and + // ~44KB; three of them govern a file the agent is going to have open anyway, + // and moving those three to on-demand takes roughly a quarter off the standing + // cost without moving a word of the methodology. + // + // Which three is not a judgment call, and the bar is in Workspace: a rule may + // be scoped only where a validator reports what it prevents. That is what makes + // arriving late survivable — the agent that wrote an EARS line wrong without + // the rule in context is told so by `scc validate`, exit 2, before the commit. + // A rule whose violation nothing mechanical catches stays preloaded whatever it + // costs, because for that one the rule is the only check there is. + // + // False for Codex and opencode, where it would be noise rather than a saving: + // nothing preloads RulesSeg there, so every rule is already on demand and a + // `paths:` header is a key their loaders do not read. + ScopedRules bool + // SubagentsInheritRules is the same question one level down: does a subagent // this harness spawns arrive with the methodology already in context, or does // it start without the rules the session it was spawned from is following? @@ -194,6 +216,7 @@ var ( SkillsSeg: "skills", CommandsSeg: "commands", RulesSeg: "rules", SettingsSeg: "settings.json", PreloadsRules: true, + ScopedRules: true, // Yes, and documented as such: a non-fork subagent inherits the whole // CLAUDE.md hierarchy, project rules included. SubagentsInheritRules: true, diff --git a/internal/scaffold/scaffold_test.go b/internal/scaffold/scaffold_test.go index 2b7e18c..01b2e64 100644 --- a/internal/scaffold/scaffold_test.go +++ b/internal/scaffold/scaffold_test.go @@ -372,7 +372,7 @@ func TestStaleManifestEntriesAreDropped(t *testing.T) { func TestApplyRestoresADeletedFile(t *testing.T) { root := t.TempDir() apply(t, root, false) - rel := ".claude/agents/code-review.md" + rel := ".claude/agents/scc-code-review.md" if err := os.Remove(filepath.Join(root, filepath.FromSlash(rel))); err != nil { t.Fatalf("Remove: %v", err) } @@ -380,7 +380,7 @@ func TestApplyRestoresADeletedFile(t *testing.T) { if res.Created != 1 { t.Errorf("created = %d, want 1", res.Created) } - want := renderRel(t, ".claude/agents/code-review.md") + want := renderRel(t, ".claude/agents/scc-code-review.md") if got := read(t, root, rel); got != want { t.Error("the deleted file was not restored from the template") } @@ -451,8 +451,8 @@ func TestASecondHarnessLeavesTheFirstAlone(t *testing.T) { if got := workspace.Harnesses(root); len(got) != 2 { t.Errorf("initialized harnesses = %v, want two", got) } - read(t, root, ".codex/agents/code-review.toml") - read(t, root, ".opencode/agent/code-review.md") + read(t, root, ".codex/agents/scc-code-review.toml") + read(t, root, ".opencode/agent/scc-code-review.md") } func splitLines(s string) []string { diff --git a/internal/validate/all.go b/internal/validate/all.go index 264e6c6..b13aa74 100644 --- a/internal/validate/all.go +++ b/internal/validate/all.go @@ -24,7 +24,21 @@ type Validator struct { // workspace with findings. That is what lets the aggregate command run unconditionally // instead of asking the user which validators apply. func All(opts ...Option) []Validator { - return append(base(), extra(opts)...) + var o options + for _, fn := range opts { + fn(&o) + } + out := base() + if o.noAttribution { + kept := out[:0] + for _, v := range out { + if v.Name != "attribution" { + kept = append(kept, v) + } + } + out = kept + } + return append(out, extra(opts)...) } // Option turns on a check that a default run leaves off. @@ -39,10 +53,23 @@ func All(opts ...Option) []Validator { type Option func(*options) type options struct { - pr bool - checks bool + pr bool + checks bool + noAttribution bool } +// WithoutAttribution drops the history check from the run. +// +// It exists for the Stop hook, which may only carry what the next turn can +// resolve. Every other validator reports something an edit fixes; a signature in a +// commit this branch has already made comes out with a rebase, so the line would +// re-appear at the end of every turn for the life of the branch and never clear — +// which is the exact shape of the loop the Stop stage was fixed for once already. +// The commit-msg hook catches a signature at the one moment it is still a file, +// `scc validate` reports it whenever somebody asks, and pre-push holds the branch. +// Nothing is lost by leaving it out of the one place it cannot be acted on. +func WithoutAttribution() Option { return func(o *options) { o.noAttribution = true } } + // WithPR adds the pull-request check: the title and body of the PR open on this // branch, read through `gh`. Off by default, on in the pre-push hook and under // `scc validate --pr`, which are the two moments the pull request exists.