From 3416e86f62fc92f2c3c7b4c26c0991639dcc240e Mon Sep 17 00:00:00 2001 From: prode Date: Sat, 1 Aug 2026 20:31:46 -0300 Subject: [PATCH 1/5] feat(assets): scaffold the methodology into Codex and opencode, not only Claude Code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scc init` takes --claude (the default), --codex, or --opencode, and asks with a numbered picker when a person is at the terminal and named no flag. Unattended callers keep getting Claude Code silently, so an agent or a CI job still drives init exactly as it did. One prose source, three harnesses. The rules, skills, and review agents are written once and re-addressed per harness: 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. Shipping a copy of the template set per harness would drift, and the drift would be silent. The review agents change with it: both now run five named gates (the ticked boxes are true, the code matches the tasks, the tests, the lint, the practices no linter has an opinion about) and report in a fixed shape the orchestrator can branch on. security-review works from a method — surface, trust boundaries, reachability, deliberate attack — with the known attack classes demoted to prompts rather than the definition of done. Effort is pinned high wherever the harness expresses one; the model tier only where there is a stable alias for it. The manifest moves into the harness directory and records which harness it is: a template renders differently per harness, so an upgrade that re-renders the recorded version to reconstruct a merge base has to render it for the same one. workspace.Find accepts any harness marker, and the skills validator checks every harness directory, because the Agent Skills format is one standard. Template version 5. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019yCM51izVEvKywpKDgYvNM --- internal/assets/assets.go | 319 ++++++++-- internal/assets/assets_test.go | 582 +++++++++++++----- .../assets/templates/agents/code-review.md | 117 ++++ .../templates/agents/security-review.md | 122 ++++ .../templates/claude/agents/code-review.md | 57 -- .../claude/agents/security-review.md | 54 -- .../{claude => }/commands/scc-adr.md | 0 .../{claude => }/commands/scc-codewiki.md | 0 .../{claude => }/commands/scc-glossary.md | 0 .../{claude => }/commands/scc-prd.md | 0 .../{claude => }/commands/scc-stack.md | 0 .../{claude => }/commands/scc-wiki.md | 0 .../assets/templates/{CLAUDE.md => entry.md} | 33 +- .../templates/{claude => }/rules/autonomy.md | 0 .../templates/{claude => }/rules/delivery.md | 17 +- .../{claude => }/rules/knowledge-base.md | 2 +- .../{claude => }/rules/methodology.md | 0 .../templates/{claude => }/rules/project.md | 0 .../templates/{claude => }/rules/routing.md | 0 .../templates/{claude => }/rules/specs.md | 0 .../templates/{claude => }/rules/tasks.md | 2 +- .../{claude => }/rules/verification.md | 0 .../{claude => }/skills/adr/SKILL.md | 0 .../{claude => }/skills/codewiki/SKILL.md | 0 .../{claude => }/skills/glossary/SKILL.md | 2 +- .../{claude => }/skills/prd/SKILL.md | 0 .../{claude => }/skills/stack/SKILL.md | 0 .../{claude => }/skills/wiki/SKILL.md | 0 internal/cli/harness.go | 129 ++++ internal/cli/harness_test.go | 162 +++++ internal/cli/init.go | 24 +- internal/cli/init_test.go | 24 +- internal/cli/validate_test.go | 36 +- internal/manifest/manifest.go | 46 +- internal/manifest/manifest_test.go | 57 +- internal/paths/paths.go | 264 +++++--- internal/scaffold/scaffold.go | 31 +- internal/scaffold/scaffold_test.go | 145 ++++- internal/validate/skill.go | 40 +- internal/validate/skill_test.go | 4 +- internal/workspace/workspace.go | 45 +- internal/workspace/workspace_test.go | 41 +- 42 files changed, 1825 insertions(+), 530 deletions(-) create mode 100644 internal/assets/templates/agents/code-review.md create mode 100644 internal/assets/templates/agents/security-review.md delete mode 100644 internal/assets/templates/claude/agents/code-review.md delete mode 100644 internal/assets/templates/claude/agents/security-review.md rename internal/assets/templates/{claude => }/commands/scc-adr.md (100%) rename internal/assets/templates/{claude => }/commands/scc-codewiki.md (100%) rename internal/assets/templates/{claude => }/commands/scc-glossary.md (100%) rename internal/assets/templates/{claude => }/commands/scc-prd.md (100%) rename internal/assets/templates/{claude => }/commands/scc-stack.md (100%) rename internal/assets/templates/{claude => }/commands/scc-wiki.md (100%) rename internal/assets/templates/{CLAUDE.md => entry.md} (50%) rename internal/assets/templates/{claude => }/rules/autonomy.md (100%) rename internal/assets/templates/{claude => }/rules/delivery.md (80%) rename internal/assets/templates/{claude => }/rules/knowledge-base.md (97%) rename internal/assets/templates/{claude => }/rules/methodology.md (100%) rename internal/assets/templates/{claude => }/rules/project.md (100%) rename internal/assets/templates/{claude => }/rules/routing.md (100%) rename internal/assets/templates/{claude => }/rules/specs.md (100%) rename internal/assets/templates/{claude => }/rules/tasks.md (96%) rename internal/assets/templates/{claude => }/rules/verification.md (100%) rename internal/assets/templates/{claude => }/skills/adr/SKILL.md (100%) rename internal/assets/templates/{claude => }/skills/codewiki/SKILL.md (100%) rename internal/assets/templates/{claude => }/skills/glossary/SKILL.md (97%) rename internal/assets/templates/{claude => }/skills/prd/SKILL.md (100%) rename internal/assets/templates/{claude => }/skills/stack/SKILL.md (100%) rename internal/assets/templates/{claude => }/skills/wiki/SKILL.md (100%) create mode 100644 internal/cli/harness.go create mode 100644 internal/cli/harness_test.go diff --git a/internal/assets/assets.go b/internal/assets/assets.go index 8748ac8..8eb5c23 100644 --- a/internal/assets/assets.go +++ b/internal/assets/assets.go @@ -5,19 +5,29 @@ // There are two kinds of template here and the difference matters: // // - **Workspace files** — what `scc init` writes and the manifest tracks. These -// are DATA-FREE: no project name, no date, no path is interpolated into any of -// them. That is what makes an upgrade possible without storing rendered text -// anywhere. A given template version renders byte-identically in every -// workspace on earth, so its content hash identifies it globally and the -// three-way merge can reconstruct the old side from the version alone. The two -// files that genuinely want per-project content — CLAUDE.md and the project -// rule — are exactly the files an upgrade excludes because the user owns them, -// so the two rules agree instead of fighting. +// are DATA-FREE except for the harness: no project name, no date, no path +// from the machine is interpolated into any of them, and the only value a +// template can reach is the profile in internal/paths that says where this +// harness keeps its files. That is what makes an upgrade possible without +// storing rendered text anywhere. A given (template version, harness) pair +// renders byte-identically in every workspace on earth, so its content hash +// identifies it globally and the three-way merge can reconstruct the old side +// from the version and the harness alone — both of which the manifest +// records. The two files that genuinely want per-project content — the entry +// file and the project rule — are exactly the files an upgrade excludes +// because the user owns them, so the two rules agree instead of fighting. // // - **Artifact templates** — what `spec new` and `plan new` render. These take // data, because they are authored from birth: the user owns the result // immediately, nothing tracks them in the manifest, and no upgrade ever touches // them. +// +// One prose source, three harnesses. The methodology does not change with the +// tool reading it, so the rules, the skills, and the review agents are written +// once and re-addressed per harness: paths come from the profile, and the +// frontmatter each loader parses is synthesized at render time. Shipping a copy +// of the template set per harness would guarantee they drift, and the drift would +// be silent. package assets import ( @@ -43,7 +53,10 @@ import ( // // 2: the knowledge-base skills and their commands. // 3: CLAUDE.md documents the npx fallback for running scc uninstalled. -const Version = "3" +// 4: the review agents pin a model and effort, run gates instead of reading, and +// report in a fixed shape the orchestrator can act on. +// 5: one template set for three harnesses — Claude Code, Codex, opencode. +const Version = "5" // The embedded tree. "all:" so nothing is silently dropped for having a name the // default embed pattern skips. @@ -56,9 +69,25 @@ const ( artifactsDir = "artifacts" ) +// Kind says how a template becomes a file in a workspace. +type Kind string + +const ( + // Plain is copied through after path expansion: rules, skills, the entry file. + Plain Kind = "plain" + // Agent is a subagent definition. Its template carries harness-neutral + // frontmatter (a name and a description) and the reviewer's prose; the + // header the harness's loader actually parses is synthesized per harness, + // because the three disagree on both the dialect and the keys. + Agent Kind = "agent" + // Command is a slash command, on the same terms as Agent: shared body, + // per-harness frontmatter. + Command Kind = "command" +) + // File is one workspace file scc scaffolds and then tracks. type File struct { - // Name is the path inside the embedded tree, e.g. "claude/rules/routing.md". + // Name is the path inside the embedded tree, e.g. "rules/routing.md". Name string // Rel is where it goes: slash-separated and relative to the workspace root. @@ -66,23 +95,27 @@ type File struct { // machines as slashes and never as the host's separator. Rel string + // Kind selects the rendering. + Kind Kind + // 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. CLAUDE.md and the project rule are the - // two files whose whole purpose is to be edited, and merging a new template - // into someone's own prose produces a mess no one asked for. + // exists rather than merging into it. The entry file and the project rule are + // the two files whose whole purpose is to be edited, and merging a new + // template into someone's own prose produces a mess no one asked for. Owned bool } -// Workspace returns every file `scc init` writes, sorted by destination so init's -// output and the manifest are in the same order on every run. -func Workspace() []File { - claude := func(seg ...string) string { - return path.Join(append([]string{paths.ClaudeDir}, seg...)...) +// Workspace returns every file `scc init` writes for a harness, sorted by +// destination so init's output and the manifest are in the same order on every +// run. +func Workspace(h paths.Harness) []File { + under := func(seg ...string) string { + return path.Join(append([]string{h.Dir}, seg...)...) } set := []File{ - {Name: "CLAUDE.md", Rel: paths.EntryFile, Owned: true}, - {Name: "claude/rules/project.md", Rel: claude(paths.RulesSeg, "project.md"), Owned: true}, + {Name: "entry.md", Rel: h.EntryFile, Kind: Plain, Owned: true}, + {Name: "rules/project.md", Rel: under(h.RulesSeg, "project.md"), Kind: Plain, Owned: true}, } // The methodology. Every one of these is scc's own content: an upgrade should // deliver improvements to them, so none is Owned. @@ -97,14 +130,16 @@ func Workspace() []File { "knowledge-base.md", } { set = append(set, File{ - Name: "claude/rules/" + rule, - Rel: claude(paths.RulesSeg, rule), + Name: "rules/" + rule, + Rel: under(h.RulesSeg, rule), + Kind: Plain, }) } - for _, agent := range []string{"code-review.md", "security-review.md"} { + for _, agent := range ReviewAgents { set = append(set, File{ - Name: "claude/agents/" + agent, - Rel: claude(paths.AgentsSeg, agent), + Name: "agents/" + agent + ".md", + Rel: under(h.AgentsSeg, agent+h.AgentExt()), + Kind: Agent, }) } // The knowledge base's authors. Every one of these produces an artifact @@ -113,30 +148,45 @@ func Workspace() []File { // nobody was told how to write. for _, skill := range KnowledgeSkills { set = append(set, File{ - Name: "claude/skills/" + skill + "/SKILL.md", - Rel: claude(paths.SkillsSeg, skill, "SKILL.md"), + Name: "skills/" + skill + "/SKILL.md", + Rel: under(h.SkillsSeg, skill, "SKILL.md"), + Kind: Plain, }) // One command per skill, so the human has an explicit entry point where the // model has a description. Namespaced, because slash commands share a flat - // namespace with every other source Claude Code loads them from and `/adr` + // namespace with every other source the harness loads them from and `/adr` // would collide on contact. + // + // Codex is the exception and gets none: its custom prompts live in the + // user's home directory and are deprecated in favor of skills, so there is + // nothing project-scoped to write. The skills above are the whole surface + // there, which is what Codex itself now recommends. + if h.CommandsSeg == "" { + continue + } cmd := commandPrefix + skill + ".md" set = append(set, File{ - Name: "claude/commands/" + cmd, - Rel: claude(paths.CommandsSeg, cmd), + Name: "commands/" + cmd, + Rel: under(h.CommandsSeg, cmd), + Kind: Command, }) } sort.Slice(set, func(i, j int) bool { return set[i].Rel < set[j].Rel }) return set } +// 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"} + // KnowledgeSkills names the skills scc ships, each one the author of a `docs/` // artifact a validator checks. Both the skill directory and its slash command are // derived from this list, so the two cannot drift apart. // // The methodology is deliberately absent from it: the cycles, verification, and -// delivery are rules under `.claude/rules/`, read when the concern is live. A skill -// restating a rule is a second copy of one fact, and the copy goes stale. +// delivery are rules under the harness's rules directory, read when the concern +// is live. A skill restating a rule is a second copy of one fact, and the copy +// goes stale. var KnowledgeSkills = []string{"adr", "codewiki", "glossary", "prd", "stack", "wiki"} // commandPrefix namespaces the scaffolded slash commands. @@ -145,19 +195,23 @@ const commandPrefix = "scc-" // Dirs returns the directories `scc init` creates even when it has no file to put // in them. An agent that can see specs/, plans/, and docs/wiki/ knows where its // output goes; one that has to infer the layout from a rule file guesses. -func Dirs() []string { - return []string{ - path.Join(paths.ClaudeDir, paths.RulesSeg), - path.Join(paths.ClaudeDir, paths.AgentsSeg), - path.Join(paths.ClaudeDir, paths.SkillsSeg), - path.Join(paths.ClaudeDir, paths.CommandsSeg), +func Dirs(h paths.Harness) []string { + dirs := []string{ + path.Join(h.Dir, h.RulesSeg), + path.Join(h.Dir, h.AgentsSeg), + path.Join(h.Dir, h.SkillsSeg), + } + if h.CommandsSeg != "" { + dirs = append(dirs, path.Join(h.Dir, h.CommandsSeg)) + } + return append(dirs, paths.SpecsSeg, paths.PlansSeg, path.Join(paths.DocsSeg, paths.WikiSeg), path.Join(paths.DocsSeg, paths.RawSeg), path.Join(paths.DocsSeg, paths.ADRSeg), path.Join(paths.DocsSeg, paths.CodewikiSeg), - } + ) } // Content returns an embedded template verbatim, normalized to LF. @@ -173,6 +227,193 @@ func Content(name string) (string, error) { return textutil.NormalizeNewlines(string(b)), nil } +// layout is what a workspace template can address: where this harness keeps +// things, and nothing else. Every field is derived from the profile, so a +// template cannot come to depend on the machine, the project, or the clock. +type layout struct { + Harness string + Dir string + Entry string + Rules string + Skills string + Agents string + Commands string + HasCommands bool + Manifest string +} + +func layoutOf(h paths.Harness) layout { + l := layout{ + Harness: h.ID, + Dir: h.Dir, + Entry: h.EntryFile, + Rules: path.Join(h.Dir, h.RulesSeg), + Skills: path.Join(h.Dir, h.SkillsSeg), + Agents: path.Join(h.Dir, h.AgentsSeg), + Manifest: path.Join(h.Dir, paths.ManifestSeg), + } + if h.CommandsSeg != "" { + l.Commands = path.Join(h.Dir, h.CommandsSeg) + l.HasCommands = true + } + return l +} + +// Render produces the exact bytes f takes in a workspace scaffolded for h: paths +// expanded, and for an agent or a command, the frontmatter that harness's loader +// parses. +func Render(h paths.Harness, f File) (string, error) { + raw, err := Content(f.Name) + if err != nil { + return "", err + } + expanded, err := expand(f.Name, raw, layoutOf(h)) + if err != nil { + return "", err + } + switch f.Kind { + case Agent: + return renderAgent(h, f, expanded) + case Command: + return renderCommand(h, expanded) + default: + return expanded, nil + } +} + +// 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. +func expand(name, raw string, l layout) (string, error) { + tpl, err := template.New(name).Option("missingkey=error").Parse(raw) + if err != nil { + return "", fmt.Errorf("template %q: %w", name, err) + } + var buf bytes.Buffer + if err := tpl.Execute(&buf, l); err != nil { + return "", fmt.Errorf("template %q: %w", name, err) + } + return textutil.NormalizeNewlines(buf.String()), nil +} + +// meta is the harness-neutral header a shared template carries: what this thing +// is called and when to use it. Everything else in a real header is dialect. +type meta struct { + name string + description string + extra map[string]string // keys only some harnesses understand + body string +} + +// splitMeta reads the leading `---` block as flat `key: value` pairs. This is not +// a YAML parser and does not need to be: scc writes these templates, the tests +// assert the shape, and a template that grows a nested value here would be +// telling us it wants to be per-harness data instead. +func splitMeta(name, raw string) (meta, error) { + const fence = "---\n" + if !strings.HasPrefix(raw, fence) { + return meta{}, fmt.Errorf("template %q: expected a leading --- block", name) + } + rest := raw[len(fence):] + end := strings.Index(rest, "\n"+fence) + if end < 0 { + return meta{}, fmt.Errorf("template %q: unterminated --- block", name) + } + m := meta{extra: map[string]string{}, body: strings.TrimLeft(rest[end+len("\n"+fence):], "\n")} + for _, line := range strings.Split(rest[:end], "\n") { + key, value, ok := strings.Cut(line, ": ") + if !ok { + return meta{}, fmt.Errorf("template %q: %q is not a `key: value` line", name, line) + } + switch key { + case "name": + m.name = value + case "description": + m.description = value + default: + m.extra[key] = value + } + } + if m.description == "" { + return meta{}, fmt.Errorf("template %q: no description", name) + } + return m, nil +} + +// The reasoning budget both reviewers run on. Review is chains-of-inference work +// — tracing a value from an argument to a shell, or a ticked box to the code +// behind it — and that is what effort buys. Every harness that expresses it gets +// it; the model tier is pinned only where the harness has a stable alias for one +// (Claude Code's "sonnet"), because a pinned `gpt-5.6` or `anthropic/claude-x` +// would be a guess about a name that churns and a provider the user may not have +// configured. +const reviewEffort = "high" + +func renderAgent(h paths.Harness, f File, raw string) (string, error) { + m, err := splitMeta(f.Name, raw) + if err != nil { + return "", err + } + if m.name == "" { + return "", fmt.Errorf("template %q: an agent needs a name", f.Name) + } + switch h.ID { + case paths.Codex.ID: + // A Codex agent role file: flat TOML, the prose in developer_instructions. + // A literal string ('''), so no character in the reviewer's prose needs + // escaping and none is silently mangled — the tests hold the body to that. + var b strings.Builder + fmt.Fprintf(&b, "name = %q\n", m.name) + fmt.Fprintf(&b, "description = %q\n", m.description) + fmt.Fprintf(&b, "model_reasoning_effort = %q\n", reviewEffort) + fmt.Fprintf(&b, "developer_instructions = '''\n%s'''\n", m.body) + return b.String(), nil + case paths.OpenCode.ID: + // opencode takes the agent's name from the filename, so the header carries + // only what it cannot infer. `edit: deny` is the "you do not write code, + // you report it" rule made mechanical; bash stays allowed because the + // reviewer has to run this project's tests and lint itself. + var b strings.Builder + b.WriteString("---\n") + fmt.Fprintf(&b, "description: %s\n", m.description) + b.WriteString("mode: subagent\n") + b.WriteString("temperature: 0.1\n") + b.WriteString("permission:\n edit: deny\n") + b.WriteString("---\n\n") + b.WriteString(m.body) + return b.String(), nil + default: + var b strings.Builder + b.WriteString("---\n") + fmt.Fprintf(&b, "name: %s\n", m.name) + fmt.Fprintf(&b, "description: %s\n", m.description) + b.WriteString("tools: Read, Grep, Glob, Bash\n") + b.WriteString("model: sonnet\n") + fmt.Fprintf(&b, "effort: %s\n", reviewEffort) + b.WriteString("---\n\n") + b.WriteString(m.body) + return b.String(), nil + } +} + +func renderCommand(h paths.Harness, raw string) (string, error) { + m, err := splitMeta("command", raw) + if err != nil { + return "", err + } + var b strings.Builder + b.WriteString("---\n") + fmt.Fprintf(&b, "description: %s\n", m.description) + // argument-hint is Claude Code's; opencode's command schema does not define it + // and there is no value in feeding a loader a key it will only ignore. + if hint, ok := m.extra["argument-hint"]; ok && h.ID == paths.Claude.ID { + fmt.Fprintf(&b, "argument-hint: %s\n", hint) + } + b.WriteString("---\n\n") + b.WriteString(m.body) + return b.String(), nil +} + // ArtifactData is what an artifact template can interpolate. Deliberately tiny: // every field is something the user supplied on the command line, so a template // cannot come to depend on the machine it was rendered on. diff --git a/internal/assets/assets_test.go b/internal/assets/assets_test.go index ebfb1d0..143f091 100644 --- a/internal/assets/assets_test.go +++ b/internal/assets/assets_test.go @@ -3,6 +3,8 @@ package assets import ( "strings" "testing" + + "github.com/protonspy/spec-claude-code/internal/paths" ) // Every embedded template is checked for the properties a manifest hash depends @@ -39,12 +41,12 @@ func TestEveryTemplateIsLFCleanAndNonEmpty(t *testing.T) { // still delivered as LF. Assert on the raw bytes to prove the guarantee is in the // code and not in the checkout. func TestContentIsNormalized(t *testing.T) { - raw, err := Content("CLAUDE.md") + raw, err := Content("entry.md") if err != nil { t.Fatalf("Content: %v", err) } if strings.Contains(raw, "\r") { - t.Error("CLAUDE.md carries CR after Content()") + t.Error("entry.md carries CR after Content()") } } @@ -55,8 +57,8 @@ func TestContentRejectsUnknownTemplate(t *testing.T) { } // The workspace set and the embedded tree must agree in both directions: a File -// pointing at nothing breaks init, and an embedded file no File points at ships in -// the binary and reaches no workspace. +// pointing at nothing breaks init, and an embedded file no harness's File points +// at ships in the binary and reaches no workspace. func TestWorkspaceSetAndTreeAgree(t *testing.T) { embedded, err := walk() if err != nil { @@ -68,18 +70,20 @@ func TestWorkspaceSetAndTreeAgree(t *testing.T) { } referenced := map[string]bool{} - for _, f := range Workspace() { - if _, err := Content(f.Name); err != nil { - t.Errorf("Workspace() references %q which is not embedded", f.Name) + for _, h := range paths.Harnesses() { + for _, f := range Workspace(h) { + if _, err := Content(f.Name); err != nil { + t.Errorf("Workspace(%s) references %q which is not embedded", h.ID, f.Name) + } + referenced[f.Name] = true } - referenced[f.Name] = true } for _, name := range []string{"requirements.md", "design.md", "tasks.md", "plan.md"} { referenced["artifacts/"+name] = true } for name := range inTree { if !referenced[name] { - t.Errorf("embedded template %q is not in Workspace() and is not an artifact template", name) + t.Errorf("embedded template %q is in no harness's Workspace() and is not an artifact template", name) } } } @@ -87,17 +91,36 @@ func TestWorkspaceSetAndTreeAgree(t *testing.T) { // Destinations must be unique and slash-separated: they are written verbatim into // the manifest, and two Files claiming one path means one silently wins. func TestWorkspaceDestinationsAreUniqueAndSlashed(t *testing.T) { - seen := map[string]bool{} - for _, f := range Workspace() { - if seen[f.Rel] { - t.Errorf("duplicate destination %q", f.Rel) - } - seen[f.Rel] = true - if strings.Contains(f.Rel, `\`) { - t.Errorf("destination %q is not slash-separated", f.Rel) + for _, h := range paths.Harnesses() { + seen := map[string]bool{} + for _, f := range Workspace(h) { + if seen[f.Rel] { + t.Errorf("%s: duplicate destination %q", h.ID, f.Rel) + } + seen[f.Rel] = true + if strings.Contains(f.Rel, `\`) { + t.Errorf("%s: destination %q is not slash-separated", h.ID, f.Rel) + } + if strings.HasPrefix(f.Rel, "/") || strings.Contains(f.Rel, "..") { + t.Errorf("%s: destination %q escapes the workspace root", h.ID, f.Rel) + } } - if strings.HasPrefix(f.Rel, "/") || strings.Contains(f.Rel, "..") { - t.Errorf("destination %q escapes the workspace root", f.Rel) + } +} + +// Every managed file lands inside the harness it was scaffolded for, except the +// entry file the harness itself loads from the project root. A file written into +// another harness's directory would be invisible to its own tools and would be +// clobbered by that harness's own init. +func TestWorkspaceStaysInsideItsHarness(t *testing.T) { + for _, h := range paths.Harnesses() { + for _, f := range Workspace(h) { + if f.Rel == h.EntryFile { + continue + } + if !strings.HasPrefix(f.Rel, h.Dir+"/") { + t.Errorf("%s: %q is outside %s/", h.ID, f.Rel, h.Dir) + } } } } @@ -105,76 +128,397 @@ func TestWorkspaceDestinationsAreUniqueAndSlashed(t *testing.T) { // The set is sorted by destination so init's report and the manifest are in the // same order on every run and on every platform. func TestWorkspaceIsSortedByDestination(t *testing.T) { - set := Workspace() - for i := 1; i < len(set); i++ { - if set[i-1].Rel >= set[i].Rel { - t.Fatalf("not sorted: %q before %q", set[i-1].Rel, set[i].Rel) + for _, h := range paths.Harnesses() { + set := Workspace(h) + for i := 1; i < len(set); i++ { + if set[i-1].Rel >= set[i].Rel { + t.Fatalf("%s: not sorted: %q before %q", h.ID, set[i-1].Rel, set[i].Rel) + } } } } -// The upgrade path depends on this: a data-free template renders identically in -// every workspace, so its hash identifies its content globally. A template with a -// placeholder in it could not be reconstructed from the manifest alone. -func TestWorkspaceTemplatesAreDataFree(t *testing.T) { - for _, f := range Workspace() { - raw, err := Content(f.Name) - if err != nil { - t.Fatalf("%s: %v", f.Name, err) +// The upgrade path depends on this: a template's only variable is the harness +// profile, which the manifest records, so re-rendering the recorded version +// reconstructs the merge base exactly. A leftover action or a "" means +// something reached for data that is not the layout, and that data would not be +// reconstructible from anything scc stores. +func TestRenderLeavesNoActionsAndNoMissingKeys(t *testing.T) { + for _, h := range paths.Harnesses() { + for _, f := range Workspace(h) { + out, err := Render(h, f) + if err != nil { + t.Fatalf("%s %s: %v", h.ID, f.Name, err) + } + if strings.Contains(out, "{{") || strings.Contains(out, "") { + t.Errorf("%s %s: unrendered action or missing key", h.ID, f.Rel) + } + if strings.Contains(out, "\r") { + t.Errorf("%s %s: rendered with CR", h.ID, f.Rel) + } + // "" is shorthand this project's own docs use for "whichever + // of the three". It is prose for a human reading about scc, and it + // means nothing to an agent reading a scaffolded rule — a template + // carrying it wanted {{.Dir}} and would ship a path that resolves to + // nowhere. + if strings.Contains(out, "") { + t.Errorf("%s %s: carries the literal ; use the layout fields", h.ID, f.Rel) + } } - if i := strings.Index(raw, "{{"); i >= 0 { - t.Errorf("%s contains a template action at byte %d — workspace templates must be data-free", f.Name, i) + } +} + +// Same inputs, same bytes — the property the manifest hash is built on. +func TestRenderIsDeterministic(t *testing.T) { + for _, h := range paths.Harnesses() { + for _, f := range Workspace(h) { + first, err := Render(h, f) + if err != nil { + t.Fatalf("%s %s: %v", h.ID, f.Name, err) + } + second, err := Render(h, f) + if err != nil { + t.Fatalf("%s %s: %v", h.ID, f.Name, err) + } + if first != second { + t.Errorf("%s %s: two renders differ", h.ID, f.Rel) + } } } } -// Exactly the two files whose purpose is to be edited are Owned. If a third -// appears, it is either a mistake or a decision that belongs in the design. -func TestOnlyTheUserOwnedFilesAreOwned(t *testing.T) { - var owned []string - for _, f := range Workspace() { - if f.Owned { - owned = append(owned, f.Rel) +// No rendered file may name a harness directory other than its own. This is the +// guard against the failure this refactor exists to prevent: a rule that still +// says ".claude/rules/project.md" reads correctly to a Claude Code session and +// sends a Codex session to a path that is not there. +func TestNoRenderedFileNamesAnotherHarnessDirectory(t *testing.T) { + for _, h := range paths.Harnesses() { + for _, f := range Workspace(h) { + out, err := Render(h, f) + if err != nil { + t.Fatalf("%s %s: %v", h.ID, f.Name, err) + } + for _, other := range paths.Harnesses() { + if other.Dir == h.Dir { + continue + } + if strings.Contains(out, other.Dir+"/") { + t.Errorf("%s %s mentions %s/", h.ID, f.Rel, other.Dir) + } + } } } - want := []string{".claude/rules/project.md", "CLAUDE.md"} - if strings.Join(owned, ",") != strings.Join(want, ",") { - t.Errorf("owned files = %v, want %v", owned, want) +} + +// Exactly the two files whose purpose is to be edited are Owned, in every +// harness. If a third appears, it is either a mistake or a decision that belongs +// in the design. +func TestOnlyTheUserOwnedFilesAreOwned(t *testing.T) { + for _, h := range paths.Harnesses() { + var owned []string + for _, f := range Workspace(h) { + if f.Owned { + owned = append(owned, f.Rel) + } + } + want := []string{h.Dir + "/" + h.RulesSeg + "/project.md", h.EntryFile} + if h.EntryFile < want[0] { + want = []string{h.EntryFile, h.Dir + "/" + h.RulesSeg + "/project.md"} + } + if strings.Join(owned, ",") != strings.Join(want, ",") { + t.Errorf("%s: owned files = %v, want %v", h.ID, owned, want) + } } } -// CLAUDE.md is paid for in context by every session, and accuracy degrades as +// The entry file is paid for in context by every session, and accuracy degrades as // context grows. The rules exist so it can stay short; this is the guard that keeps // someone from "helpfully" inlining them back into it. func TestScaffoldedEntryFileStaysShort(t *testing.T) { - raw, err := Content("CLAUDE.md") - if err != nil { - t.Fatalf("Content: %v", err) - } - if n := strings.Count(raw, "\n"); n > 60 { - t.Errorf("scaffolded CLAUDE.md is %d lines; keep it under 60 and link to .claude/rules/ instead", n) + for _, h := range paths.Harnesses() { + raw, err := Render(h, entryFile(t, h)) + if err != nil { + t.Fatalf("%s: %v", h.ID, err) + } + if n := strings.Count(raw, "\n"); n > 60 { + t.Errorf("%s: scaffolded %s is %d lines; keep it under 60 and link to the rules instead", + h.ID, h.EntryFile, n) + } } } -// Every rule the scaffolded CLAUDE.md points at has to exist, or the entry file -// sends the agent to a file that isn't there. +// Every rule the scaffolded entry file points at has to exist under this +// harness's own rules directory, or the entry file sends the agent to a file that +// isn't there. func TestEntryFileLinksResolve(t *testing.T) { - raw, err := Content("CLAUDE.md") - if err != nil { - t.Fatalf("Content: %v", err) + for _, h := range paths.Harnesses() { + raw, err := Render(h, entryFile(t, h)) + if err != nil { + t.Fatalf("%s: %v", h.ID, err) + } + for _, f := range Workspace(h) { + if !strings.HasPrefix(f.Rel, h.Dir+"/"+h.RulesSeg+"/") { + continue + } + if !strings.Contains(raw, "("+f.Rel+")") { + t.Errorf("%s: %s does not link to %s", h.ID, h.EntryFile, f.Rel) + } + } + } +} + +func entryFile(t *testing.T, h paths.Harness) File { + t.Helper() + for _, f := range Workspace(h) { + if f.Rel == h.EntryFile { + return f + } } - dests := map[string]bool{} - for _, f := range Workspace() { - dests[f.Rel] = true + t.Fatalf("%s: no entry file in the workspace set", h.ID) + return File{} +} + +// The review agents are read by each harness's own loader, and every one of them +// refuses a definition whose header it cannot parse. Assert the dialect per +// harness, at the source. +func TestReviewAgentsCarryTheHeaderTheirHarnessParses(t *testing.T) { + for _, h := range paths.Harnesses() { + for _, name := range ReviewAgents { + f := findFile(t, h, h.Dir+"/"+h.AgentsSeg+"/"+name+h.AgentExt()) + out, err := Render(h, f) + if err != nil { + t.Fatalf("%s %s: %v", h.ID, name, err) + } + switch h.AgentFormat { + case paths.FormatTOML: + for _, want := range []string{ + "name = \"" + name + "\"\n", + "description = \"", + "model_reasoning_effort = \"high\"\n", + "developer_instructions = '''\n", + } { + if !strings.Contains(out, want) { + t.Errorf("%s %s: missing %q", h.ID, name, want) + } + } + if !strings.HasSuffix(out, "'''\n") { + t.Errorf("%s %s: developer_instructions is not closed", h.ID, name) + } + default: + if !strings.HasPrefix(out, "---\n") { + t.Errorf("%s %s: does not open with frontmatter", h.ID, name) + } + if !strings.Contains(out, "\ndescription: ") { + t.Errorf("%s %s: no description", h.ID, name) + } + } + } } - for _, f := range Workspace() { - if !strings.HasPrefix(f.Rel, ".claude/rules/") { +} + +// The reasoning budget is pinned wherever the harness expresses one, because +// review is chains-of-inference work and that is what effort buys. The model tier +// is pinned only where the harness has a stable alias for one: a hardcoded +// provider-prefixed model would name something the user may not have configured. +func TestReviewAgentsPinTheirEffort(t *testing.T) { + want := map[string][]string{ + paths.Claude.ID: {"\nmodel: sonnet\n", "\neffort: high\n"}, + paths.Codex.ID: {"model_reasoning_effort = \"high\"\n"}, + paths.OpenCode.ID: {"\nmode: subagent\n", "\n edit: deny\n"}, + } + for _, h := range paths.Harnesses() { + for _, name := range ReviewAgents { + f := findFile(t, h, h.Dir+"/"+h.AgentsSeg+"/"+name+h.AgentExt()) + out, err := Render(h, f) + if err != nil { + t.Fatalf("%s %s: %v", h.ID, name, err) + } + for _, w := range want[h.ID] { + if !strings.Contains(out, w) { + t.Errorf("%s %s: missing %q", h.ID, name, strings.TrimSpace(w)) + } + } + } + } +} + +// The Codex header is TOML, so the reviewer's prose has to survive being embedded +// in it. A literal string ends at the first ”' and escapes nothing, so the two +// things that could corrupt the file are a ”' in the body and a quote or +// backslash in the description. +func TestAgentProseIsSafeToEmbedInTOML(t *testing.T) { + for _, name := range ReviewAgents { + raw, err := Content("agents/" + name + ".md") + if err != nil { + t.Fatalf("%s: %v", name, err) + } + m, err := splitMeta(name, raw) + if err != nil { + t.Fatalf("%s: %v", name, err) + } + if strings.Contains(m.body, "'''") { + t.Errorf("%s: the body contains ''' and would end the TOML literal string early", name) + } + if strings.ContainsAny(m.description, "\"\\") { + t.Errorf("%s: the description contains a quote or backslash", name) + } + // A YAML scalar carrying ": " is a mapping, not a string, in the two + // harnesses that read this description as frontmatter. + if strings.Contains(m.description, ": ") { + t.Errorf("%s: the description contains \": \" and would parse as a YAML mapping", name) + } + } +} + +// Every skill in KnowledgeSkills ships a SKILL.md in every harness, and its slash +// command wherever the harness has a command surface. The two are derived from one +// list so they cannot drift, and this is the assertion that keeps the derivation +// honest — including for Codex, where the answer is deliberately no commands at +// all. +func TestEverySkillShipsWithItsCommand(t *testing.T) { + for _, h := range paths.Harnesses() { + skills, commands := map[string]bool{}, map[string]bool{} + for _, f := range Workspace(h) { + switch { + case strings.HasPrefix(f.Rel, h.Dir+"/"+h.SkillsSeg+"/"): + name := strings.TrimSuffix(strings.TrimPrefix(f.Rel, h.Dir+"/"+h.SkillsSeg+"/"), "/SKILL.md") + skills[name] = true + case h.CommandsSeg != "" && strings.HasPrefix(f.Rel, h.Dir+"/"+h.CommandsSeg+"/"): + name := strings.TrimSuffix(strings.TrimPrefix(f.Rel, h.Dir+"/"+h.CommandsSeg+"/"), ".md") + commands[name] = true + } + } + if len(skills) != len(KnowledgeSkills) { + t.Fatalf("%s: %d skills, want %d", h.ID, len(skills), len(KnowledgeSkills)) + } + wantCommands := len(KnowledgeSkills) + if h.CommandsSeg == "" { + wantCommands = 0 + } + if len(commands) != wantCommands { + t.Fatalf("%s: %d commands, want %d", h.ID, len(commands), wantCommands) + } + for _, name := range KnowledgeSkills { + if !skills[name] { + t.Errorf("%s: %s is in KnowledgeSkills and ships no SKILL.md", h.ID, name) + } + if wantCommands > 0 && !commands[commandPrefix+name] { + t.Errorf("%s: %s ships no %s%s command", h.ID, name, commandPrefix, name) + } + } + } +} + +// The Agent Skills contract is what `scc skill validate` enforces on everyone else, +// and the two fields it can break loading on are checked here at the source. The +// full conformance run happens against a scaffolded workspace in internal/cli — a +// tool that ships non-conforming skills has no standing to check anyone else's. +func TestSkillsCarryTheirFrontmatter(t *testing.T) { + for _, h := range paths.Harnesses() { + for _, f := range Workspace(h) { + if !strings.HasSuffix(f.Rel, "/SKILL.md") { + continue + } + raw, err := Render(h, f) + if err != nil { + t.Fatalf("%s %s: %v", h.ID, f.Rel, err) + } + if !strings.HasPrefix(raw, "---\n") { + t.Errorf("%s: %s does not open with frontmatter", h.ID, f.Rel) + continue + } + // The name must equal the parent directory or the skill does not load at + // all, in any tool that reads this format. + dir := path2(f.Rel) + if !strings.Contains(raw, "\nname: "+dir+"\n") { + t.Errorf("%s: %s frontmatter name does not match the directory %q", h.ID, f.Rel, dir) + } + if !strings.Contains(raw, "\ndescription: ") { + t.Errorf("%s: %s has no description", h.ID, f.Rel) + } + } + } +} + +// path2 returns the directory name a SKILL.md sits in. +func path2(rel string) string { + parts := strings.Split(strings.TrimSuffix(rel, "/SKILL.md"), "/") + return parts[len(parts)-1] +} + +// A slash command with no description is one that cannot be found in the picker. +// Claude Code additionally documents argument-hint; opencode's schema does not +// define it, so it must not be written there. +func TestCommandsCarryTheirDescription(t *testing.T) { + for _, h := range paths.Harnesses() { + if h.CommandsSeg == "" { continue } - if !strings.Contains(raw, "("+f.Rel+")") { - t.Errorf("CLAUDE.md does not link to %s", f.Rel) + for _, f := range Workspace(h) { + if f.Kind != Command { + continue + } + raw, err := Render(h, f) + if err != nil { + t.Fatalf("%s %s: %v", h.ID, f.Rel, err) + } + if !strings.HasPrefix(raw, "---\n") || !strings.Contains(raw, "\ndescription: ") { + t.Errorf("%s: %s needs frontmatter carrying a description", h.ID, f.Rel) + } + if h.ID != paths.Claude.ID && strings.Contains(raw, "argument-hint:") { + t.Errorf("%s: %s carries argument-hint, which this harness does not define", h.ID, f.Rel) + } + } + } +} + +// Directories init creates on its own must be inside the workspace and +// slash-separated, for the same reason destinations must be. +func TestDirsAreRelativeAndSlashed(t *testing.T) { + for _, h := range paths.Harnesses() { + for _, d := range Dirs(h) { + if strings.HasPrefix(d, "/") || strings.Contains(d, "..") || strings.Contains(d, `\`) { + t.Errorf("%s: directory %q is not a slash-separated path inside the workspace", h.ID, d) + } + } + } +} + +// Every file the set writes has a directory init created, or the write is left +// depending on AtomicWrite's MkdirAll to cover for a layout the package was +// supposed to declare. +func TestDirsCoverEveryDestination(t *testing.T) { + for _, h := range paths.Harnesses() { + dirs := map[string]bool{} + for _, d := range Dirs(h) { + dirs[d] = true + } + for _, f := range Workspace(h) { + parent := f.Rel[:strings.LastIndex(f.Rel, "/")+1] + if parent == "" { // the entry file, at the root + continue + } + parent = strings.TrimSuffix(parent, "/") + // A skill lives one level deeper than the directory init creates. + if strings.HasSuffix(f.Rel, "/SKILL.md") { + parent = parent[:strings.LastIndex(parent, "/")] + } + if !dirs[parent] { + t.Errorf("%s: %s lands in %q, which Dirs() does not create", h.ID, f.Rel, parent) + } + } + } +} + +func findFile(t *testing.T, h paths.Harness, rel string) File { + t.Helper() + for _, f := range Workspace(h) { + if f.Rel == rel { + return f } } + t.Fatalf("%s: no file at %s", h.ID, rel) + return File{} } func TestArtifactRendersItsData(t *testing.T) { @@ -234,110 +578,16 @@ func TestTitle(t *testing.T) { } } -// The review agents are read by Claude Code's subagent loader, which needs the -// frontmatter contract: a name matching the file and a description saying when to -// use it. -func TestAgentsCarryTheirFrontmatter(t *testing.T) { - for _, f := range Workspace() { - if !strings.HasPrefix(f.Rel, ".claude/agents/") { - continue - } - raw, err := Content(f.Name) - if err != nil { - t.Fatalf("%s: %v", f.Name, err) - } - if !strings.HasPrefix(raw, "---\n") { - t.Errorf("%s does not open with frontmatter", f.Rel) - } - base := strings.TrimSuffix(strings.TrimPrefix(f.Rel, ".claude/agents/"), ".md") - if !strings.Contains(raw, "\nname: "+base+"\n") { - t.Errorf("%s: frontmatter name does not match the filename", f.Rel) - } - if !strings.Contains(raw, "\ndescription: ") { - t.Errorf("%s: no description in frontmatter", f.Rel) - } - } -} - -// Every skill in KnowledgeSkills ships both a SKILL.md and a slash command, and -// nothing else lands in either directory. The two are derived from one list so they -// cannot drift, and this is the assertion that keeps the derivation honest. -func TestEverySkillShipsWithItsCommand(t *testing.T) { - skills, commands := map[string]bool{}, map[string]bool{} - for _, f := range Workspace() { - switch { - case strings.HasPrefix(f.Rel, ".claude/skills/"): - name := strings.TrimSuffix(strings.TrimPrefix(f.Rel, ".claude/skills/"), "/SKILL.md") - skills[name] = true - case strings.HasPrefix(f.Rel, ".claude/commands/"): - name := strings.TrimSuffix(strings.TrimPrefix(f.Rel, ".claude/commands/"), ".md") - commands[name] = true - } - } - if len(skills) != len(KnowledgeSkills) || len(commands) != len(KnowledgeSkills) { - t.Fatalf("%d skills and %d commands, want %d of each", len(skills), len(commands), len(KnowledgeSkills)) - } - for _, name := range KnowledgeSkills { - if !skills[name] { - t.Errorf("%s is in KnowledgeSkills and ships no SKILL.md", name) - } - if !commands[commandPrefix+name] { - t.Errorf("%s is in KnowledgeSkills and ships no %s%s command", name, commandPrefix, name) - } - } -} - -// The Agent Skills contract is what `scc skill validate` enforces on everyone else, -// and the two fields it can break loading on are checked here at the source. The -// full conformance run happens against a scaffolded workspace in internal/cli — a -// tool that ships non-conforming skills has no standing to check anyone else's. -func TestSkillsCarryTheirFrontmatter(t *testing.T) { - for _, f := range Workspace() { - if !strings.HasPrefix(f.Rel, ".claude/skills/") { - continue - } - raw, err := Content(f.Name) - if err != nil { - t.Fatalf("%s: %v", f.Name, err) - } - if !strings.HasPrefix(raw, "---\n") { - t.Errorf("%s does not open with frontmatter", f.Rel) - continue - } - // The name must equal the parent directory or the skill does not load at - // all, in any tool that reads this format. - dir := strings.TrimSuffix(strings.TrimPrefix(f.Rel, ".claude/skills/"), "/SKILL.md") - if !strings.Contains(raw, "\nname: "+dir+"\n") { - t.Errorf("%s: frontmatter name does not match the directory %q", f.Rel, dir) - } - if !strings.Contains(raw, "\ndescription: ") { - t.Errorf("%s: no description in frontmatter", f.Rel) - } - } -} - -// A slash command with no description is one that cannot be found in the picker. -func TestCommandsCarryTheirDescription(t *testing.T) { - for _, f := range Workspace() { - if !strings.HasPrefix(f.Rel, ".claude/commands/") { - continue - } - raw, err := Content(f.Name) - if err != nil { - t.Fatalf("%s: %v", f.Name, err) - } - if !strings.HasPrefix(raw, "---\n") || !strings.Contains(raw, "\ndescription: ") { - t.Errorf("%s: a command needs frontmatter carrying a description", f.Rel) - } - } -} - -// Directories init creates on its own must be inside the workspace and -// slash-separated, for the same reason destinations must be. -func TestDirsAreRelativeAndSlashed(t *testing.T) { - for _, d := range Dirs() { - if strings.HasPrefix(d, "/") || strings.Contains(d, "..") || strings.Contains(d, `\`) { - t.Errorf("directory %q is not a slash-separated path inside the workspace", d) +func TestSplitMetaRejectsAMalformedHeader(t *testing.T) { + cases := map[string]string{ + "no header": "# just prose\n", + "unterminated": "---\nname: x\ndescription: y\n", + "not key-value": "---\nname: x\ndescription: y\nnonsense\n---\n\nbody\n", + "no description": "---\nname: x\n---\n\nbody\n", + } + for label, raw := range cases { + if _, err := splitMeta(label, raw); err == nil { + t.Errorf("%s: splitMeta accepted it", label) } } } diff --git a/internal/assets/templates/agents/code-review.md b/internal/assets/templates/agents/code-review.md new file mode 100644 index 0000000..3b34ff3 --- /dev/null +++ b/internal/assets/templates/agents/code-review.md @@ -0,0 +1,117 @@ +--- +name: 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. +--- + +You review a diff. You do not write code, and you do not fix what you find — you +report it. The orchestrator decides what to do with the report. + +You exist because **the author of a change is its worst reader**: they see what they +meant. A cold context is the entire value you add, so do not ask the author what they +intended, and do not take "it's done" as evidence. Read the diff, read the task list, +and run the checks yourself. + +## Scope + +The changed lines, plus enough of the surrounding code to judge them. Start with: + +```bash +git diff main...HEAD # or the base branch the work targets +git diff --stat main...HEAD +``` + +Then read what the work was supposed to be: `specs//requirements.md`, +`design.md` and `tasks.md`, or the checklist in `plans/.md`. **The artifact is +the standard the code is held to** — not the implementation's own apparent intent. +Read `{{.Rules}}/project.md` for this project's build, test, and lint commands. + +## The five gates — run every one, in this order + +Run all five even after one fails. Bailing at the first red gate gives the +orchestrator one problem when there were four, and it pays for a second review round +to learn the rest. + +**1 · The ticked boxes are true.** For every task marked `[x]`, find the code that +implements it in the diff. A box ticked with nothing behind it is the most expensive +defect you can find here, because everything downstream — the PR body, the spec, the +next session's assumptions — is built on it. Report the reverse too: code in the diff +that no task claims, and unticked boxes that are in fact implemented. + +**2 · The code does what the task says.** Not something adjacent, not a superset. +Trace each changed behavior back to a requirement or a task line, and read the +acceptance criteria as written. Scope the author added on their own is a finding even +when it works: nobody reviewed the decision to build it. + +**3 · The feature's tests run green — because you ran them.** Use this project's test +command, scoped to what the diff touches, and quote the exact command and the tail of +its output. Then judge the tests themselves: + +- **Tests that assert the implementation instead of the requirement.** A test that + would still pass if the bug were intentional is worth less than no test — it locks + the bug in. This is the most common defect in agent-written tests: look for + assertions that read like a transcript of the code. +- **Missing tests.** Unit tasks owe a test per function; TDD tasks owe a test that was + seen to fail first. Untested new functions are a finding. + +**4 · The lint runs clean — because you ran it.** The project's linter, quoted the +same way. Lint is the automated half of the best-practices check: unused code, +unchecked errors, shadowed variables, unsafe conversions. Do not re-derive by eye what +the linter already answers. + +**5 · Best practices, read by hand.** The half no linter has an opinion about, in +rough order of what actually bites: + +- **Correctness at the edges** — empty, zero, nil, one, many, concurrent, the error + path. Errors dropped, wrapped without context, or returned but not handled. +- **Behavior changed by accident.** A modified function whose existing callers were + not looked at. +- **Consistency with the codebase.** A new way of doing something the project already + does one way is a cost paid by everyone who reads it next. +- **Complexity that is not paying for itself** — an abstraction with one caller, a + layer that only forwards, a config knob nobody asked for. +- **Naming and comments that lie.** A comment describing the previous behavior is + worse than no comment. + +If a gate cannot be run — no test command, a suite that needs a service you do not +have — report it as `not-run` with the reason. **Never report a gate you skipped as +passing**, and never infer green from the author having said so. + +Security is a different lens and has its own reviewer. Note anything alarming in one +line, but do not try to be that reviewer. + +## The report + +End with this, and nothing after it: + +``` +## Verdict + — one sentence saying why. + +## Gates +| # | Gate | Result | Evidence | +|---|---|---|---| +| 1 | ticked boxes are true | pass/fail | 7/7 tasks traced to code | +| 2 | code matches the tasks | pass/fail | ... | +| 3 | tests | pass/fail/not-run | `` → 42 ok, 0 failed | +| 4 | lint | pass/fail/not-run | `` → clean | +| 5 | best practices | pass/fail | 2 findings | + +## Findings +### 1 · blocker — path/to/file.go:118 +What is wrong, and what specifically makes it wrong: the input, the state, or the +caller that breaks. Which task or requirement it violates. What to do instead. + +### 2 · major — path/to/other.go:40 +... + +## Notes +Anything the author may reasonably ignore, one line each. +``` + +Severity is `blocker` (do not open the PR), `major` (fix before merge), or `minor` +(the author's call). A red gate 1, 3, or 4 is always a blocker. + +**A finding the author cannot act on is noise.** If you are not sure something is a +defect, put it under Notes with what would make it one, rather than reporting it as +one. `clean` with an empty Findings section is a legitimate and useful answer; padding +a review with style preferences is how a reviewer stops being read. diff --git a/internal/assets/templates/agents/security-review.md b/internal/assets/templates/agents/security-review.md new file mode 100644 index 0000000..f70f97e --- /dev/null +++ b/internal/assets/templates/agents/security-review.md @@ -0,0 +1,122 @@ +--- +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. +--- + +You review a diff for security defects, and for nothing else. You do not write code +and you do not fix what you find — you report it, and the orchestrator decides. + +This is a separate agent from `code-review` on purpose. **A single reviewer asked for +"everything" reliably under-weights security**, because correctness findings are +easier to produce and crowd it out. Your narrow scope is the point: do not report +style, naming, or design taste, and do not re-report what a correctness reviewer would +obviously catch. + +**You are not a checklist runner.** The question is not "does this diff contain any of +the ten bugs I know the names of" — it is **"what can someone make this code do that +it was not built to do?"** A weakness with no name is still a weakness; a named class +with no reachable path is not a finding. Work from the code outward, not from a list +inward. + +## Scope + +```bash +git diff main...HEAD # or the base branch the work targets +git diff --stat main...HEAD +``` + +Judge what the change makes *possible*, not what the codebase already was. +Pre-existing issues in untouched code are worth one line at the end, not the body of +the review. + +## The method — four passes, in this order + +**1 · Map what the change adds to the attack surface.** Before judging anything, list +it: new inputs, new outputs, new files or paths touched, new privileges or +capabilities exercised, new state that persists, new dependencies, new network calls, +new places a secret can flow. This list is what the rest of the review works through — +if it is empty, say so and stop. + +**2 · Find the trust boundaries.** For each input on that list: who controls it, and +what is assumed about it. A boundary is anywhere data crosses from someone else's +control into yours — request bodies, CLI arguments, filenames, environment, file +contents, database rows written by users, anything over the network, and the output of +any other system. **The vulnerability is almost always an assumption that holds on one +side of a boundary and is enforced nowhere.** + +**3 · Trace reachability, source to effect.** For each boundary crossing, follow the +value until it either gets validated or reaches something that acts on it: a shell, a +query, a path, a template, a deserializer, an allocation, a permission check, a +redirect, a model prompt. Write the path down — file to file, call to call. **A +finding without a path from an attacker-controlled source to an effect is a +hypothesis, and you must label it as one.** + +**4 · Attack it deliberately.** Ask what you would try if you wanted this code to +misbehave, and answer concretely: the input, the sequence, the race, the state you +would put it in first. Consider order of operations (check before use, use before +check), what happens on the error path, what happens twice, and what happens when a +value is at its boundary — empty, huge, negative, encoded, or a lookalike. + +### Known classes, as prompts and not as a scope + +Use these to jog the passes above — never as the definition of "done". Absence of +every class below is not evidence of safety. + +- **Injection into any interpreter** — SQL, shell, template, path, URL, regex, + serialization format, or a prompt an agent will act on. +- **Path traversal.** A caller-supplied name that becomes a path segment without being + validated first: `..`, absolute paths, separators, symlinks, Windows device names. + This is the one that turns a delete command into deleting the project. +- **Authorization.** A new endpoint, command, or branch that skips the check its + neighbors make. Missing authorization is far more common than broken authentication. +- **Secrets.** Credentials, tokens, or keys added to source, config, a test fixture, a + log line, an error message, or a cache. +- **Crypto and randomness.** `math/rand` where unpredictability matters, a hand-rolled + comparison of secrets, a hash chosen for speed where it needed to be slow. +- **Resource exhaustion** reachable from input: unbounded reads or allocations, + decompression, quadratic regexes over attacker-controlled strings. +- **Time-of-check to time-of-use**, and anything else where two operations assume the + world did not move between them. +- **Dependencies added in this diff.** New attack surface and a new maintainer to + trust. Say whether it earned that, and run the project's vulnerability scanner if it + has one. +- **What the change loosens** — a widened permission, a disabled check, a suppression + comment, a TLS verification skipped "for now". + +## The report + +End with this, and nothing after it: + +``` +## Verdict + — one sentence saying why. + +## Surface reviewed +| What the change adds | Trust boundary | Traced | +|---|---|---| +| `scc spec delete ` argument | user/CLI | yes → findings 1 | +| new dep `example/foo` | third party | yes → no issue | + +## Findings +### 1 · critical — path/to/file.go:64 +**Path:** attacker-controlled `` → `` → ``, quoted line by +line. +**Impact:** what an attacker gets, stated concretely. +**Fix:** what to do instead. + +## Hypotheses +Suspicions with no reachable path yet, and what would confirm each one. + +## Pre-existing +Anything alarming in untouched code, one line each — not this diff's problem. +``` + +Severity is `critical` (reachable now, high impact), `high` (reachable, bounded +impact), `medium` (needs a precondition an attacker may well have), or `low` +(defense-in-depth). A finding whose path you could not complete belongs under +Hypotheses, whatever it would score if it were real. + +**Do not inflate severity to be heard.** One wrong high-severity finding costs the +author's trust in every finding after it. "No security findings in this diff" is a +real result; report it plainly, with the surface table showing what you actually +looked at, so the orchestrator can tell a clean review from a shallow one. diff --git a/internal/assets/templates/claude/agents/code-review.md b/internal/assets/templates/claude/agents/code-review.md deleted file mode 100644 index ed470de..0000000 --- a/internal/assets/templates/claude/agents/code-review.md +++ /dev/null @@ -1,57 +0,0 @@ ---- -name: code-review -description: Reviews a diff for correctness and quality. 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. -tools: Read, Grep, Glob, Bash ---- - -You review a diff. You do not write code, and you do not fix what you find — you -report it. - -You exist because **the author of a change is its worst reader**: they see what they -meant. A cold context is the entire value you add, so do not ask the author what they -intended. Read the diff and the code around it. - -## Scope - -The changed lines, plus enough of the surrounding code to judge them. Start with: - -```bash -git diff main...HEAD # or the base branch the work targets -git diff --stat main...HEAD -``` - -If the work has a spec, read `specs//requirements.md` and `tasks.md`. The -requirement is the standard the code is held to — not the implementation's own -apparent intent. - -## What to look for, roughly in order of what actually bites - -1. **Does it do what the requirement says?** Trace each changed behavior back to a - requirement or a task. Code doing something nobody asked for is a finding. -2. **Tests that assert the implementation instead of the requirement.** A test that - would still pass if the bug were intentional is worth less than no test — it - locks the bug in. This is the most common defect in agent-written tests: look for - assertions that read like a transcript of the code. -3. **Missing tests.** Unit tasks owe a test per function; TDD tasks owe a test that - was seen to fail first. Untested new functions are a finding. -4. **Correctness at the edges** — empty, zero, nil, one, many, concurrent, the error - path. Errors dropped, wrapped without context, or returned but not handled. -5. **Behavior changed by accident.** A modified function whose existing callers were - not looked at. -6. **Consistency with the codebase.** A new way of doing something the project - already does one way is a cost paid by everyone who reads it next. -7. **Complexity that is not paying for itself** — an abstraction with one caller, a - layer that only forwards, a config knob nobody asked for. - -Security is a different lens and has its own reviewer. Note anything alarming, but -do not try to be that reviewer. - -## How to report - -Findings only, most severe first. For each one: the file and line, what is wrong, and -what specifically makes it wrong — the input, the state, the caller that breaks. - -**A finding the author cannot act on is noise.** If you are not sure something is a -defect, say what would make it one rather than reporting it as one. Ending with "no -findings" is a legitimate and useful answer; padding a review with style preferences -is how a reviewer stops being read. diff --git a/internal/assets/templates/claude/agents/security-review.md b/internal/assets/templates/claude/agents/security-review.md deleted file mode 100644 index 07d16eb..0000000 --- a/internal/assets/templates/claude/agents/security-review.md +++ /dev/null @@ -1,54 +0,0 @@ ---- -name: security-review -description: Reviews a diff for security defects specifically. Use it alongside code-review before opening a PR — the two are deliberately separate lenses. -tools: Read, Grep, Glob, Bash ---- - -You review a diff for security defects, and for nothing else. - -This is a separate agent from `code-review` on purpose. **A single reviewer asked for -"everything" reliably under-weights security**, because correctness findings are -easier to produce and crowd it out. Your narrow scope is the point: do not report -style, naming, or design taste, and do not re-report what a correctness reviewer -would obviously catch. - -## Scope - -```bash -git diff main...HEAD # or the base branch the work targets -``` - -Judge what the change makes possible, not what the codebase already was. Pre-existing -issues in untouched code are worth one line at the end, not the body of the review. - -## What to look for - -- **Untrusted input reaching something that acts on it.** Trace it: request bodies, - CLI arguments, filenames, environment, file contents, anything over the network. - Interpolation into SQL, a shell, a template, a path, a URL, or a deserializer. -- **Path traversal.** A caller-supplied name that becomes a path segment without - being validated first — `..`, absolute paths, separators, Windows device names. - This is the one that turns a delete command into deleting the project. -- **Authentication and authorization.** A new endpoint, command, or branch that skips - the check its neighbors make. Missing authorization is far more common than broken - authentication. -- **Secrets.** Credentials, tokens, or keys added to source, to a config file, to a - test fixture, to a log line, or to an error message. -- **Crypto and randomness.** `math/rand` where unpredictability matters, a hand-rolled - comparison of secrets, a hash chosen for speed where it needed to be slow. -- **Resource exhaustion** reachable from input: unbounded reads, unbounded - allocations, decompression, regexes over attacker-controlled strings. -- **Dependencies added in this diff.** A new dependency is new attack surface and a - new maintainer to trust. Say whether it earned that. -- **What the change loosens** — a widened permission, a disabled check, a `#nosec`, a - TLS verification skipped "for now". - -## How to report - -Findings only, most severe first. For each one: the file and line, the concrete path -from an attacker-controlled input to the effect, and what to do instead. A finding -with no reachable path is a hypothesis — say so, or leave it out. - -**Do not inflate severity to be heard.** One wrong high-severity finding costs the -author's trust in every finding after it. "No security findings in this diff" is a -real result; report it plainly when it is true. diff --git a/internal/assets/templates/claude/commands/scc-adr.md b/internal/assets/templates/commands/scc-adr.md similarity index 100% rename from internal/assets/templates/claude/commands/scc-adr.md rename to internal/assets/templates/commands/scc-adr.md diff --git a/internal/assets/templates/claude/commands/scc-codewiki.md b/internal/assets/templates/commands/scc-codewiki.md similarity index 100% rename from internal/assets/templates/claude/commands/scc-codewiki.md rename to internal/assets/templates/commands/scc-codewiki.md diff --git a/internal/assets/templates/claude/commands/scc-glossary.md b/internal/assets/templates/commands/scc-glossary.md similarity index 100% rename from internal/assets/templates/claude/commands/scc-glossary.md rename to internal/assets/templates/commands/scc-glossary.md diff --git a/internal/assets/templates/claude/commands/scc-prd.md b/internal/assets/templates/commands/scc-prd.md similarity index 100% rename from internal/assets/templates/claude/commands/scc-prd.md rename to internal/assets/templates/commands/scc-prd.md diff --git a/internal/assets/templates/claude/commands/scc-stack.md b/internal/assets/templates/commands/scc-stack.md similarity index 100% rename from internal/assets/templates/claude/commands/scc-stack.md rename to internal/assets/templates/commands/scc-stack.md diff --git a/internal/assets/templates/claude/commands/scc-wiki.md b/internal/assets/templates/commands/scc-wiki.md similarity index 100% rename from internal/assets/templates/claude/commands/scc-wiki.md rename to internal/assets/templates/commands/scc-wiki.md diff --git a/internal/assets/templates/CLAUDE.md b/internal/assets/templates/entry.md similarity index 50% rename from internal/assets/templates/CLAUDE.md rename to internal/assets/templates/entry.md index 55bbfe1..d731326 100644 --- a/internal/assets/templates/CLAUDE.md +++ b/internal/assets/templates/entry.md @@ -1,26 +1,26 @@ -# CLAUDE.md +# {{.Entry}} This repo practises spec-driven development, scaffolded and checked by `scc`. **This file is deliberately short, and keeping it short is a requirement.** Every session pays for it in context, and model accuracy degrades as context grows — non-uniformly, well before any documented limit. The methodology lives in -`.claude/rules/`, one file per concern, read when the concern is live. Do not +`{{.Rules}}/`, one file per concern, read when the concern is live. Do not inline those rules here. ## The rules | Read | When | |---|---| -| [routing.md](.claude/rules/routing.md) | Work arrives and needs a vehicle — a spec, or a plan. | -| [autonomy.md](.claude/rules/autonomy.md) | At kickoff, before writing anything. | -| [methodology.md](.claude/rules/methodology.md) | Starting any task: which cycle, and what to run first. | -| [tasks.md](.claude/rules/tasks.md) | Writing or reading a task list. | -| [verification.md](.claude/rules/verification.md) | A task's code is written and you think it is done. | -| [delivery.md](.claude/rules/delivery.md) | The last task is done: branch, review, PR. | -| [specs.md](.claude/rules/specs.md) | Writing or changing anything under `specs/`. | -| [knowledge-base.md](.claude/rules/knowledge-base.md) | Something learned is worth keeping, or a decision was made. | -| [project.md](.claude/rules/project.md) | You need this project's build, test, or lint commands. | +| [routing.md]({{.Rules}}/routing.md) | Work arrives and needs a vehicle — a spec, or a plan. | +| [autonomy.md]({{.Rules}}/autonomy.md) | At kickoff, before writing anything. | +| [methodology.md]({{.Rules}}/methodology.md) | Starting any task: which cycle, and what to run first. | +| [tasks.md]({{.Rules}}/tasks.md) | Writing or reading a task list. | +| [verification.md]({{.Rules}}/verification.md) | A task's code is written and you think it is done. | +| [delivery.md]({{.Rules}}/delivery.md) | The last task is done: branch, review, PR. | +| [specs.md]({{.Rules}}/specs.md) | Writing or changing anything under `specs/`. | +| [knowledge-base.md]({{.Rules}}/knowledge-base.md) | Something learned is worth keeping, or a decision was made. | +| [project.md]({{.Rules}}/project.md) | You need this project's build, test, or lint commands. | ## The layout @@ -28,15 +28,20 @@ inline those rules here. specs// requirements.md · design.md · tasks.md plans/.md one file: structure, plus a checklist and/or spec references docs/ the knowledge base — wiki, adr, codewiki, glossary, stack -.claude/rules/ the methodology above -.claude/skills/ how to author each part of docs/ — invoked when it applies -.claude/commands/ the same skills on demand, as /scc-wiki, /scc-adr, /scc-prd, … + +{{.Rules}}/ — the methodology above +{{.Skills}}/ — how to author each part of docs/, invoked when it applies +{{.Agents}}/ — code-review and security-review, run before the PR +{{- if .HasCommands}} +{{.Commands}}/ — the same skills on demand, as /scc-wiki, /scc-adr, /scc-prd, … +{{- end}} ``` ## Checking your work ``` scc validate # every applicable validator; exit 2 means findings +scc update # take a newer scc's rules and agents; shows the plan, then asks ``` If `scc` is not installed on this machine, run it with no install — same binary, diff --git a/internal/assets/templates/claude/rules/autonomy.md b/internal/assets/templates/rules/autonomy.md similarity index 100% rename from internal/assets/templates/claude/rules/autonomy.md rename to internal/assets/templates/rules/autonomy.md diff --git a/internal/assets/templates/claude/rules/delivery.md b/internal/assets/templates/rules/delivery.md similarity index 80% rename from internal/assets/templates/claude/rules/delivery.md rename to internal/assets/templates/rules/delivery.md index ac22503..525552a 100644 --- a/internal/assets/templates/claude/rules/delivery.md +++ b/internal/assets/templates/rules/delivery.md @@ -12,7 +12,7 @@ own **git worktree**: git worktree add ../- -b / ``` -The reason is that the user may run several Claude Code sessions at once, one per +The reason is that the user may run several agent sessions at once, one per feature, and merge them into `main` as they land. A worktree is what makes that possible: each session gets its own directory, none disturbs the others, and none touches the checkout the user is sitting in. A shared tree with `git switch` cannot @@ -56,9 +56,18 @@ Once the last task is done: see breakage between tasks. 2. **`scc validate`** — the artifacts have to be in shape too, and exit `2` is not done. -3. **`code-review` and `security-review`** subagents on the diff, and fix what they - find. The PR should arrive already reviewed: a PR is for the human, and spending - their attention on findings a subagent would have caught is pure waste. +3. **`code-review` and `security-review`** subagents on the diff, dispatched + together. Each returns a verdict, a table of what it actually checked, and + findings by severity — you fix from that report, you do not re-review. `blocked` + or any `blocker`/`critical` finding means the PR does not open yet; `major`/`high` + is fixed before merge; `minor`/`low` is your call, and saying "not doing this, and + why" in the PR body is a legitimate answer. A gate reported `not-run` is not a + pass: run it yourself or say in the PR that it was not run. + + The PR should arrive already reviewed: a PR is for the human, and spending their + attention on findings a subagent would have caught is pure waste. Fixing then + re-running the two agents is worth one round; a third round means the finding + needs a person, not another review. 4. **Commit and push.** Conventional Commits, written from the diff and the spec. 5. **Open the PR.** Body: what changed, which spec or plan, how it was verified. diff --git a/internal/assets/templates/claude/rules/knowledge-base.md b/internal/assets/templates/rules/knowledge-base.md similarity index 97% rename from internal/assets/templates/claude/rules/knowledge-base.md rename to internal/assets/templates/rules/knowledge-base.md index 1e7aeb5..2ae823f 100644 --- a/internal/assets/templates/claude/rules/knowledge-base.md +++ b/internal/assets/templates/rules/knowledge-base.md @@ -85,7 +85,7 @@ One canonical term per concept, and the synonyms to avoid. One entry per line: ```markdown - **order total** — the amount charged, in minor units. Avoid: grand total, sum -- **workspace** — a directory holding .claude/scc-manifest.json. Avoid: project root +- **workspace** — a directory holding {{.Manifest}}. Avoid: project root ``` Domain vocabulary drifts by default — three names for one thing appear within a week diff --git a/internal/assets/templates/claude/rules/methodology.md b/internal/assets/templates/rules/methodology.md similarity index 100% rename from internal/assets/templates/claude/rules/methodology.md rename to internal/assets/templates/rules/methodology.md diff --git a/internal/assets/templates/claude/rules/project.md b/internal/assets/templates/rules/project.md similarity index 100% rename from internal/assets/templates/claude/rules/project.md rename to internal/assets/templates/rules/project.md diff --git a/internal/assets/templates/claude/rules/routing.md b/internal/assets/templates/rules/routing.md similarity index 100% rename from internal/assets/templates/claude/rules/routing.md rename to internal/assets/templates/rules/routing.md diff --git a/internal/assets/templates/claude/rules/specs.md b/internal/assets/templates/rules/specs.md similarity index 100% rename from internal/assets/templates/claude/rules/specs.md rename to internal/assets/templates/rules/specs.md diff --git a/internal/assets/templates/claude/rules/tasks.md b/internal/assets/templates/rules/tasks.md similarity index 96% rename from internal/assets/templates/claude/rules/tasks.md rename to internal/assets/templates/rules/tasks.md index 571dca2..11796fc 100644 --- a/internal/assets/templates/claude/rules/tasks.md +++ b/internal/assets/templates/rules/tasks.md @@ -45,7 +45,7 @@ a whole plan. ## Two checklists, one truth -Claude Code's own todo list is where you track the task you are on right now. The +Your harness's own todo list is where you track the task you are on right now. The file — `specs//tasks.md`, or the checklist in `plans/.md` — is the durable record: it survives the session, it gets reviewed, it gets committed, and it is what `scc` validates. diff --git a/internal/assets/templates/claude/rules/verification.md b/internal/assets/templates/rules/verification.md similarity index 100% rename from internal/assets/templates/claude/rules/verification.md rename to internal/assets/templates/rules/verification.md diff --git a/internal/assets/templates/claude/skills/adr/SKILL.md b/internal/assets/templates/skills/adr/SKILL.md similarity index 100% rename from internal/assets/templates/claude/skills/adr/SKILL.md rename to internal/assets/templates/skills/adr/SKILL.md diff --git a/internal/assets/templates/claude/skills/codewiki/SKILL.md b/internal/assets/templates/skills/codewiki/SKILL.md similarity index 100% rename from internal/assets/templates/claude/skills/codewiki/SKILL.md rename to internal/assets/templates/skills/codewiki/SKILL.md diff --git a/internal/assets/templates/claude/skills/glossary/SKILL.md b/internal/assets/templates/skills/glossary/SKILL.md similarity index 97% rename from internal/assets/templates/claude/skills/glossary/SKILL.md rename to internal/assets/templates/skills/glossary/SKILL.md index 830df93..74dbba0 100644 --- a/internal/assets/templates/claude/skills/glossary/SKILL.md +++ b/internal/assets/templates/skills/glossary/SKILL.md @@ -17,7 +17,7 @@ One entry per line, in `docs/glossary.md`: ```markdown - **order total** — the amount charged, in minor units. Avoid: grand total, sum -- **workspace** — a directory holding .claude/scc-manifest.json. Avoid: project root +- **workspace** — a directory holding {{.Manifest}}. Avoid: project root ``` - The term is bold. The definition follows an em dash. diff --git a/internal/assets/templates/claude/skills/prd/SKILL.md b/internal/assets/templates/skills/prd/SKILL.md similarity index 100% rename from internal/assets/templates/claude/skills/prd/SKILL.md rename to internal/assets/templates/skills/prd/SKILL.md diff --git a/internal/assets/templates/claude/skills/stack/SKILL.md b/internal/assets/templates/skills/stack/SKILL.md similarity index 100% rename from internal/assets/templates/claude/skills/stack/SKILL.md rename to internal/assets/templates/skills/stack/SKILL.md diff --git a/internal/assets/templates/claude/skills/wiki/SKILL.md b/internal/assets/templates/skills/wiki/SKILL.md similarity index 100% rename from internal/assets/templates/claude/skills/wiki/SKILL.md rename to internal/assets/templates/skills/wiki/SKILL.md diff --git a/internal/cli/harness.go b/internal/cli/harness.go new file mode 100644 index 0000000..8afb777 --- /dev/null +++ b/internal/cli/harness.go @@ -0,0 +1,129 @@ +package cli + +import ( + "bufio" + "fmt" + "io" + "os" + "strconv" + "strings" + + "github.com/protonspy/spec-claude-code/internal/paths" + "github.com/protonspy/spec-claude-code/internal/render" +) + +// chooseHarness resolves which harness `scc init` scaffolds for, in this order: +// an explicit flag, then a prompt, then the default. +// +// The prompt only happens when a person is actually there to answer it — stdin +// is a terminal and the caller did not ask for JSON. That condition is the whole +// design: scc's contract is that an agent or a CI job drives it exactly as well +// as a human does, and a command that blocks on a read no one will answer breaks +// that contract far worse than a wrong default would. Non-interactive callers +// get Claude Code silently, which is what every existing script already assumed. +func chooseHarness(picks map[string]*bool, jsonOut bool) (paths.Harness, error) { + var chosen []paths.Harness + for _, h := range paths.Harnesses() { + if p, ok := picks[h.ID]; ok && *p { + chosen = append(chosen, h) + } + } + switch len(chosen) { + case 1: + return chosen[0], nil + case 0: + if jsonOut || !interactive() { + return paths.Claude, nil + } + return promptHarness(promptIn), nil + default: + // Exclusive rather than additive because each run writes one manifest and + // one entry file: two harnesses in one invocation would have to pick which + // one the shared AGENTS.md describes, and guessing that is worse than + // asking for a second run. + names := make([]string, 0, len(chosen)) + for _, h := range chosen { + names = append(names, "--"+h.ID) + } + return paths.Harness{}, fmt.Errorf( + "%s: pick one harness per run, and run init again for the other", + strings.Join(names, " and ")) + } +} + +// promptHarness shows the supported harnesses and reads a choice. Claude Code is +// first and is the default, because it is the harness the methodology was +// designed against and the only one whose subagent, skill, and slash-command +// surfaces all exist at project scope. +// +// A numbered list read line by line, not a full-screen selector: scc is a +// headless CLI that happens to be talkative when a human is present, and a raw +// terminal mode would be a second interaction model to maintain — and to get +// wrong on Windows — for one question asked once per repo. +func promptHarness(in io.Reader) paths.Harness { + all := paths.Harnesses() + + fmt.Println() + render.Ask("Which harness is this workspace for?\n") + fmt.Println() + width := 0 + for _, h := range all { + if len(h.Label) > width { + width = len(h.Label) + } + } + for i, h := range all { + line := fmt.Sprintf(" %s %-*s %s · %s/", + render.Bold(strconv.Itoa(i+1)+")"), width, h.Label, h.EntryFile, h.Dir) + if i == 0 { + line += render.Cyan(" (default)") + } + fmt.Println(line) + } + fmt.Println() + + scanner := bufio.NewScanner(in) + for { + render.Ask(fmt.Sprintf("Pick 1-%d, or Enter for %s: ", len(all), all[0].Label)) + if !scanner.Scan() { + // EOF: the input ended without an answer, so take the default rather + // than looping forever on a closed pipe. + fmt.Println() + return all[0] + } + answer := strings.TrimSpace(scanner.Text()) + if answer == "" { + return all[0] + } + if n, err := strconv.Atoi(answer); err == nil && n >= 1 && n <= len(all) { + return all[n-1] + } + // A name works too — somebody who typed "codex" meant it. + if h, err := paths.ParseHarness(strings.ToLower(answer)); err == nil { + return h + } + render.Warn(fmt.Sprintf("not one of the choices: %q", answer)) + } +} + +// Where an interactive prompt reads from, and whether anybody is there to answer +// it. Two variables rather than a direct os.Stdin read, so the tests can drive +// both halves: the prompts are a real part of the surface and would otherwise be +// checkable only by hand, or worse, would behave differently under `go test` +// depending on whether the runner attached a console. +var ( + promptIn io.Reader = os.Stdin + interactive = func() bool { return isTerminal(os.Stdin) } +) + +// isTerminal reports whether f is a character device, i.e. a real terminal rather +// than a pipe or a file. Same test render uses to decide on color, and stdlib +// only — the binary ships to six platforms and every dependency is a supply-chain +// surface. +func isTerminal(f *os.File) bool { + info, err := f.Stat() + if err != nil { + return false + } + return info.Mode()&os.ModeCharDevice != 0 +} diff --git a/internal/cli/harness_test.go b/internal/cli/harness_test.go new file mode 100644 index 0000000..6a4747d --- /dev/null +++ b/internal/cli/harness_test.go @@ -0,0 +1,162 @@ +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/protonspy/spec-claude-code/internal/assets" + "github.com/protonspy/spec-claude-code/internal/paths" + "github.com/protonspy/spec-claude-code/internal/workspace" +) + +// withPrompt makes the process look interactive and answers whatever is asked +// with input. Without it the tests inherit whatever stdin the runner attached, +// which differs between a terminal, a CI job, and an IDE — and a prompt is not +// testable at all if it reads the real one. +func withPrompt(t *testing.T, input string) { + t.Helper() + origIn, origTTY := promptIn, interactive + promptIn, interactive = strings.NewReader(input), func() bool { return true } + t.Cleanup(func() { promptIn, interactive = origIn, origTTY }) +} + +// withoutTerminal is the unattended case: an agent or a CI job, where nobody can +// answer a prompt. +func withoutTerminal(t *testing.T) { + t.Helper() + origIn, origTTY := promptIn, interactive + promptIn, interactive = strings.NewReader(""), func() bool { return false } + t.Cleanup(func() { promptIn, interactive = origIn, origTTY }) +} + +// Each harness flag scaffolds that harness's whole tree, entry file included. +func TestInitScaffoldsTheChosenHarness(t *testing.T) { + for _, h := range paths.Harnesses() { + root := t.TempDir() + if _, stderr, code := run(t, "init", "--"+h.ID, "--root", root); code != ExitOK { + t.Fatalf("%s: exit = %d (%s)", h.ID, code, stderr) + } + if !workspace.IsWorkspace(root) { + t.Fatalf("%s: no marker", h.ID) + } + for _, f := range assets.Workspace(h) { + if _, err := os.Stat(filepath.Join(root, filepath.FromSlash(f.Rel))); err != nil { + t.Errorf("%s: %s was not written: %v", h.ID, f.Rel, err) + } + } + got := workspace.Harnesses(root) + if len(got) != 1 || got[0].ID != h.ID { + t.Errorf("%s: initialized harnesses = %v", h.ID, got) + } + } +} + +// No flag and no terminal is the CI and agent case, and it must stay silent and +// deterministic rather than blocking on a prompt nobody will answer. +func TestInitWithoutAFlagDefaultsToClaude(t *testing.T) { + withoutTerminal(t) + root := t.TempDir() + if _, stderr, code := run(t, "init", "--root", root); code != ExitOK { + t.Fatalf("exit = %d (%s)", code, stderr) + } + got := workspace.Harnesses(root) + if len(got) != 1 || got[0].ID != paths.Claude.ID { + t.Errorf("initialized harnesses = %v, want claude", got) + } +} + +// With a person at the terminal and no flag, init asks — and scaffolds what they +// picked. +func TestInitAsksWhenNobodyPassedAFlag(t *testing.T) { + withPrompt(t, "2\n") + root := t.TempDir() + stdout, stderr, code := run(t, "init", "--root", root) + if code != ExitOK { + t.Fatalf("exit = %d (%s)", code, stderr) + } + if !strings.Contains(stdout, "Which harness") { + t.Errorf("init did not ask:\n%s", stdout) + } + got := workspace.Harnesses(root) + if len(got) != 1 || got[0].ID != paths.Codex.ID { + t.Errorf("initialized harnesses = %v, want codex", got) + } +} + +// --json is a machine reading the output, so the prompt must not appear even on a +// terminal: it would land in the middle of the document. +func TestInitDoesNotAskUnderJSON(t *testing.T) { + withPrompt(t, "2\n") + root := t.TempDir() + stdout, stderr, code := run(t, "init", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("exit = %d (%s)", code, stderr) + } + if strings.Contains(stdout, "Which harness") { + t.Errorf("init prompted under --json:\n%s", stdout) + } + got := workspace.Harnesses(root) + if len(got) != 1 || got[0].ID != paths.Claude.ID { + t.Errorf("initialized harnesses = %v, want claude", got) + } +} + +// Two harnesses in one run would have to pick which one the shared AGENTS.md +// describes. Refusing is better than guessing. +func TestInitRefusesTwoHarnessesAtOnce(t *testing.T) { + root := t.TempDir() + _, stderr, code := run(t, "init", "--codex", "--opencode", "--root", root) + if code != ExitError { + t.Fatalf("exit = %d, want %d", code, ExitError) + } + if !strings.Contains(stderr, "--codex") || !strings.Contains(stderr, "--opencode") { + t.Errorf("stderr does not name both flags: %q", stderr) + } + if workspace.IsWorkspace(root) { + t.Error("a refused init still wrote a workspace") + } +} + +// The picker: a number, a name, or Enter for the default, and an unusable answer +// asks again rather than picking something the user did not choose. +func TestPromptHarnessReadsAChoice(t *testing.T) { + cases := map[string]string{ + "": paths.Claude.ID, // EOF + "\n": paths.Claude.ID, // Enter + "1\n": paths.Claude.ID, + "2\n": paths.Codex.ID, + "3\n": paths.OpenCode.ID, + "opencode\n": paths.OpenCode.ID, + " Codex \n": paths.Codex.ID, + "9\nnope\n2\n": paths.Codex.ID, + } + for in, want := range cases { + var got paths.Harness + _, _, _ = capture(t, func() int { + got = promptHarness(strings.NewReader(in)) + return 0 + }) + if got.ID != want { + t.Errorf("promptHarness(%q) = %q, want %q", in, got.ID, want) + } + } +} + +// The prompt lists every supported harness with Claude Code first, so the choice +// is visible rather than something the user has to know to ask about. +func TestPromptHarnessListsEveryHarness(t *testing.T) { + stdout, _, _ := capture(t, func() int { + promptHarness(strings.NewReader("\n")) + return 0 + }) + for _, h := range paths.Harnesses() { + if !strings.Contains(stdout, h.Label) || !strings.Contains(stdout, h.Dir+"/") { + t.Errorf("the prompt does not offer %s:\n%s", h.Label, stdout) + } + } + if i, j := strings.Index(stdout, paths.Claude.Label), strings.Index(stdout, paths.Codex.Label); i > j { + t.Error("Claude Code is not listed first") + } +} diff --git a/internal/cli/init.go b/internal/cli/init.go index 51ec260..f3c5e67 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -4,7 +4,9 @@ import ( "flag" "fmt" "os" + "path" + "github.com/protonspy/spec-claude-code/internal/paths" "github.com/protonspy/spec-claude-code/internal/render" "github.com/protonspy/spec-claude-code/internal/scaffold" "github.com/protonspy/spec-claude-code/internal/workspace" @@ -14,6 +16,11 @@ import ( // skills and their commands, the directory layout, and the manifest that makes the // directory findable as a workspace. // +// One harness per run, selected by flag and defaulting to Claude Code. Running it +// twice with different harnesses is supported and is how a repo worked on from two +// tools gets both trees — the second run leaves the first's files alone, including +// the entry file when the two harnesses share one. +// // It is safe to re-run. Without --force it never overwrites anything, so "run init // again" is the honest upgrade story until `scc update` exists — additive by // construction, and it simply does not deliver improved templates to files that @@ -23,6 +30,10 @@ func runInit(args []string) int { fs.SetOutput(os.Stderr) root := addRoot(fs) force := fs.Bool("force", false, "overwrite existing files, naming every edited file it clobbers") + picks := map[string]*bool{} + for _, h := range paths.Harnesses() { + picks[h.ID] = fs.Bool(h.ID, false, "scaffold for "+h.ID+" ("+h.EntryFile+", "+h.Dir+"/)") + } jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { @@ -32,12 +43,18 @@ func runInit(args []string) int { return ExitError } + harness, err := chooseHarness(picks, *jsonOut) + if err != nil { + render.Err(err.Error()) + return ExitError + } + target, ok := resolveRoot(*root) if !ok { return ExitError } - res, err := scaffold.Apply(target, scaffold.Options{SCCVersion: version, Force: *force}) + res, err := scaffold.Apply(target, scaffold.Options{SCCVersion: version, Harness: harness, Force: *force}) if err != nil { render.Err(fmt.Sprintf("init failed: %v", err)) return ExitError @@ -63,8 +80,9 @@ func runInit(args []string) int { case res.AlreadyPresent: render.Info(fmt.Sprintf("%d created, %d replaced, %d left alone", res.Created, res.Replaced, res.Skipped)) default: - render.OK(fmt.Sprintf("workspace ready in %s", workspace.Relative(mustCwd(), target))) - render.Info("read CLAUDE.md, then fill in .claude/rules/project.md with this project's commands") + render.OK(fmt.Sprintf("%s workspace ready in %s", harness.Label, workspace.Relative(mustCwd(), target))) + render.Info(fmt.Sprintf("read %s, then fill in %s/project.md with this project's commands", + harness.EntryFile, path.Join(harness.Dir, harness.RulesSeg))) } return ExitOK } diff --git a/internal/cli/init_test.go b/internal/cli/init_test.go index c7224c2..07effe8 100644 --- a/internal/cli/init_test.go +++ b/internal/cli/init_test.go @@ -19,7 +19,7 @@ import ( func initWorkspace(t *testing.T) string { t.Helper() root := t.TempDir() - if _, stderr, code := run(t, "init", "--root", root); code != ExitOK { + if _, stderr, code := run(t, "init", "--claude", "--root", root); code != ExitOK { t.Fatalf("init: exit = %d (stderr: %s)", code, stderr) } return root @@ -30,7 +30,7 @@ func TestInitCreatesAWorkspace(t *testing.T) { if !workspace.IsWorkspace(root) { t.Fatal("init did not leave the workspace marker") } - for _, f := range assets.Workspace() { + for _, f := range assets.Workspace(paths.Claude) { if _, err := os.Stat(filepath.Join(root, filepath.FromSlash(f.Rel))); err != nil { t.Errorf("%s missing after init: %v", f.Rel, err) } @@ -60,8 +60,8 @@ func TestInitJSONReportsEveryAction(t *testing.T) { if err := json.Unmarshal([]byte(stdout), &got); err != nil { t.Fatalf("stdout is not valid JSON (%v): %q", err, stdout) } - if got.Created != len(assets.Workspace()) || len(got.Changes) != len(assets.Workspace()) { - t.Errorf("created = %d, changes = %d, want %d of each", got.Created, len(got.Changes), len(assets.Workspace())) + if got.Created != len(assets.Workspace(paths.Claude)) || len(got.Changes) != len(assets.Workspace(paths.Claude)) { + t.Errorf("created = %d, changes = %d, want %d of each", got.Created, len(got.Changes), len(assets.Workspace(paths.Claude))) } if stderr != "" { t.Errorf("stderr = %q, want empty so stdout can be piped", stderr) @@ -72,7 +72,7 @@ func TestInitJSONReportsEveryAction(t *testing.T) { // to be free: nothing written, nothing reported as changed, exit 0. func TestInitIsIdempotent(t *testing.T) { root := initWorkspace(t) - before, err := os.ReadFile(paths.Manifest(root)) + before, err := os.ReadFile(paths.Claude.Manifest(root)) if err != nil { t.Fatalf("ReadFile: %v", err) } @@ -91,7 +91,7 @@ func TestInitIsIdempotent(t *testing.T) { if got.Created != 0 || got.ManifestWritten { t.Errorf("second init wrote something: %+v", got) } - after, err := os.ReadFile(paths.Manifest(root)) + after, err := os.ReadFile(paths.Claude.Manifest(root)) if err != nil { t.Fatalf("ReadFile: %v", err) } @@ -108,7 +108,7 @@ func TestInitLeavesEditedFilesAlone(t *testing.T) { if err := workspace.AtomicWrite(rule, []byte(mine), 0o644); err != nil { t.Fatalf("AtomicWrite: %v", err) } - if _, stderr, code := run(t, "init", "--root", root); code != ExitOK { + if _, stderr, code := run(t, "init", "--claude", "--root", root); code != ExitOK { t.Fatalf("exit = %d (stderr: %s)", code, stderr) } got, err := os.ReadFile(rule) @@ -128,7 +128,7 @@ func TestInitForceNamesWhatItClobbered(t *testing.T) { if err := workspace.AtomicWrite(rule, []byte("# mine\n"), 0o644); err != nil { t.Fatalf("AtomicWrite: %v", err) } - _, stderr, code := run(t, "init", "--root", root, "--force") + _, stderr, code := run(t, "init", "--claude", "--root", root, "--force") if code != ExitOK { t.Fatalf("exit = %d (stderr: %s)", code, stderr) } @@ -146,7 +146,7 @@ func TestInitForceNamesWhatItClobbered(t *testing.T) { func TestInitRejectsAMissingRoot(t *testing.T) { missing := filepath.Join(t.TempDir(), "nope") - _, stderr, code := run(t, "init", "--root", missing) + _, stderr, code := run(t, "init", "--claude", "--root", missing) if code != ExitError { t.Errorf("exit = %d, want %d", code, ExitError) } @@ -159,7 +159,7 @@ func TestInitRejectsAMissingRoot(t *testing.T) { // Ignoring it would report success for something scc did not do. func TestInitRejectsPositionals(t *testing.T) { root := t.TempDir() - if _, _, code := run(t, "init", "--root", root, "user-auth"); code != ExitError { + if _, _, code := run(t, "init", "--claude", "--root", root, "user-auth"); code != ExitError { t.Errorf("exit = %d, want %d", code, ExitError) } } @@ -168,11 +168,11 @@ func TestInitRejectsPositionals(t *testing.T) { // regular file (the marker contract), and every managed path is inside the root. func TestScaffoldedWorkspaceIsSelfConsistent(t *testing.T) { root := initWorkspace(t) - info, err := os.Stat(paths.Manifest(root)) + info, err := os.Stat(paths.Claude.Manifest(root)) if err != nil || !info.Mode().IsRegular() { t.Fatalf("the marker is not a regular file: %v", err) } - for _, f := range assets.Workspace() { + for _, f := range assets.Workspace(paths.Claude) { if strings.HasPrefix(f.Rel, "/") || strings.Contains(f.Rel, "..") { t.Errorf("%s escapes the workspace root", f.Rel) } diff --git a/internal/cli/validate_test.go b/internal/cli/validate_test.go index 61683b9..6bb1413 100644 --- a/internal/cli/validate_test.go +++ b/internal/cli/validate_test.go @@ -57,7 +57,7 @@ func TestFreshArtifactsPassTheirOwnValidators(t *testing.T) { func TestScaffoldedSkillsPassTheSkillValidator(t *testing.T) { root := initWorkspace(t) - entries, err := os.ReadDir(paths.Skills(root)) + entries, err := os.ReadDir(paths.Claude.Skills(root)) if err != nil { t.Fatalf("init scaffolded no skills directory: %v", err) } @@ -78,6 +78,38 @@ func TestScaffoldedSkillsPassTheSkillValidator(t *testing.T) { } } +// Every harness's tree is validated on its own terms: a workspace scaffolded for +// Codex or opencode has to pass the same gate, and the skills validator has to +// find the skills where that harness keeps them rather than only under .claude/. +func TestAFreshWorkspacePassesValidateInEveryHarness(t *testing.T) { + for _, h := range paths.Harnesses() { + root := t.TempDir() + if _, stderr, code := run(t, "init", "--"+h.ID, "--root", root); code != ExitOK { + t.Fatalf("%s: init: %d (%s)", h.ID, code, stderr) + } + if _, stderr, code := run(t, "spec", "new", "user-auth", "--root", root); code != ExitOK { + t.Fatalf("%s: spec new: %d (%s)", h.ID, code, stderr) + } + stdout, stderr, code := run(t, "validate", "--root", root, "--json") + if code != ExitOK { + t.Errorf("%s: validate on a fresh workspace exited %d\nstdout: %s\nstderr: %s", + h.ID, code, stdout, stderr) + } + // And the skills really were looked at: a broken one in this harness's + // directory has to be found, or the pass above means nothing. + bad := filepath.Join(h.Skills(root), "Bad_Name") + if err := os.MkdirAll(bad, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(filepath.Join(bad, "SKILL.md"), []byte("---\nname: Bad_Name\n---\nbody\n"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if _, _, code := run(t, "skill", "validate", "--root", root); code != ExitFindings { + t.Errorf("%s: a broken skill under %s went unreported (exit %d)", h.ID, h.Skills(root), code) + } + } +} + // Exit 2 is the contract: a finding is a legitimate answer to a lint question, not a // failure of the tool, and CI branches on the difference. func TestValidateExitsTwoOnFindings(t *testing.T) { @@ -152,7 +184,7 @@ func TestSkillValidate(t *testing.T) { t.Errorf("skill validate on a workspace with no skills exited %d, want %d", code, ExitOK) } - dir := filepath.Join(paths.Skills(root), "pdf-processing") + dir := filepath.Join(paths.Claude.Skills(root), "pdf-processing") if err := os.MkdirAll(dir, 0o755); err != nil { t.Fatalf("MkdirAll: %v", err) } diff --git a/internal/manifest/manifest.go b/internal/manifest/manifest.go index 4f53b5c..df5ecdd 100644 --- a/internal/manifest/manifest.go +++ b/internal/manifest/manifest.go @@ -1,5 +1,5 @@ -// Package manifest reads and writes .claude/scc-manifest.json — scc's only file -// and, by existing at all, the workspace marker. +// Package manifest reads and writes /scc-manifest.json — scc's only +// file and, by existing at all, the workspace marker. // // It records one entry per scc-managed file: a content hash and the template // version that produced it. The two answer different questions and an upgrade @@ -70,6 +70,13 @@ type Manifest struct { // workspace behaves like a different version than the user expects. SCC string + // Harness is the harness this tree was scaffolded for: "claude", "codex", + // "opencode". Unlike SCC it is load-bearing, because a template renders + // differently per harness — an upgrade that re-renders the recorded version + // to reconstruct the merge base has to render it for the same harness or the + // base is wrong and the merge silently clobbers. + Harness string + // Files are the managed entries, kept sorted by Path by every operation that // mutates them. Files []Entry @@ -82,6 +89,7 @@ type Manifest struct { // entry as absent, which is the failure this constant exists to make visible. const ( keySCC = "scc" + keyHarness = "harness" keyFiles = "files" keyPath = "path" keyHash = "hash" @@ -109,16 +117,17 @@ func HashFile(path string) (string, error) { return Hash(string(b)), nil } -// New returns an empty manifest stamped with the given scc version. -func New(sccVersion string) *Manifest { - return &Manifest{SCC: sccVersion} +// New returns an empty manifest stamped with the given scc version and harness. +func New(sccVersion string, h paths.Harness) *Manifest { + return &Manifest{SCC: sccVersion, Harness: h.ID} } -// Load reads the manifest at root. A missing file is not an error: it means the -// directory is not a workspace yet, and the caller gets an empty manifest plus -// found=false so it can tell "no workspace" from "empty workspace". -func Load(root string) (m *Manifest, found bool, err error) { - b, err := os.ReadFile(paths.Manifest(root)) +// Load reads the manifest for h at root. A missing file is not an error: it means +// the directory is not a workspace for that harness yet, and the caller gets an +// empty manifest plus found=false so it can tell "no workspace" from "empty +// workspace". +func Load(root string, h paths.Harness) (m *Manifest, found bool, err error) { + b, err := os.ReadFile(h.Manifest(root)) if errors.Is(err, fs.ErrNotExist) { return &Manifest{}, false, nil } @@ -127,20 +136,20 @@ func Load(root string) (m *Manifest, found bool, err error) { } m = &Manifest{} if err := json.Unmarshal([]byte(textutil.NormalizeNewlines(string(b))), m); err != nil { - return nil, true, fmt.Errorf("%s is not valid scc manifest JSON: %w", paths.Manifest(root), err) + return nil, true, fmt.Errorf("%s is not valid scc manifest JSON: %w", h.Manifest(root), err) } m.sortFiles() return m, true, nil } -// Save writes the manifest under root atomically, so a concurrent reader — or a +// Save writes h's manifest under root atomically, so a concurrent reader — or a // crash — never observes a half-written marker. -func Save(root string, m *Manifest) error { +func Save(root string, h paths.Harness, m *Manifest) error { b, err := m.Bytes() if err != nil { return err } - return workspace.AtomicWrite(paths.Manifest(root), b, 0o644) + return workspace.AtomicWrite(h.Manifest(root), b, 0o644) } // Bytes serializes the manifest exactly as Save would write it: sorted, indented @@ -227,6 +236,9 @@ func (m Manifest) MarshalJSON() ([]byte, error) { if err := putJSON(out, keySCC, m.SCC); err != nil { return nil, err } + if err := putJSON(out, keyHarness, m.Harness); err != nil { + return nil, err + } files := m.Files if files == nil { files = []Entry{} // an empty manifest serializes as [], never null @@ -251,6 +263,12 @@ func (m *Manifest) UnmarshalJSON(b []byte) error { } delete(raw, keySCC) } + if v, ok := raw[keyHarness]; ok { + if err := json.Unmarshal(v, &m.Harness); err != nil { + return fmt.Errorf("field %q: %w", keyHarness, err) + } + delete(raw, keyHarness) + } if v, ok := raw[keyFiles]; ok { if err := json.Unmarshal(v, &m.Files); err != nil { return fmt.Errorf("field %q: %w", keyFiles, err) diff --git a/internal/manifest/manifest_test.go b/internal/manifest/manifest_test.go index e537764..6a56c2e 100644 --- a/internal/manifest/manifest_test.go +++ b/internal/manifest/manifest_test.go @@ -43,11 +43,11 @@ func TestHashIsStableAndDistinguishing(t *testing.T) { // Byte-identical output on every machine is what keeps an upgrade's diff limited // to what actually changed. Insertion order must not survive into the file. func TestBytesAreDeterministicAndSorted(t *testing.T) { - a := New("v1.0.0") + a := New("v1.0.0", paths.Claude) a.Set("specs/z.md", Hash("z"), "v1") a.Set(".claude/rules/routing.md", Hash("r"), "v1") - b := New("v1.0.0") + b := New("v1.0.0", paths.Claude) b.Set(".claude/rules/routing.md", Hash("r"), "v1") b.Set("specs/z.md", Hash("z"), "v1") @@ -76,7 +76,7 @@ func TestBytesAreDeterministicAndSorted(t *testing.T) { // A manifest written on Windows is read on Linux — a committed workspace is a // shared workspace — so the recorded separator cannot be the host's. func TestPathsAreStoredSlashSeparated(t *testing.T) { - m := New("v1.0.0") + m := New("v1.0.0", paths.Claude) m.Set(filepath.Join(".claude", "rules", "routing.md"), Hash("r"), "v1") b, err := m.Bytes() if err != nil { @@ -91,7 +91,7 @@ func TestPathsAreStoredSlashSeparated(t *testing.T) { } func TestEmptyManifestSerializesFilesAsArray(t *testing.T) { - b, err := New("v1.0.0").Bytes() + b, err := New("v1.0.0", paths.Claude).Bytes() if err != nil { t.Fatalf("Bytes: %v", err) } @@ -126,7 +126,7 @@ func TestUnknownFieldsSurviveARoundTrip(t *testing.T) { } func TestGetSetRemove(t *testing.T) { - m := New("v1.0.0") + m := New("v1.0.0", paths.Claude) if _, ok := m.Get("a.md"); ok { t.Error("Get on an empty manifest reported a hit") } @@ -153,7 +153,7 @@ func TestStatusDistinguishesPristineEditedMissing(t *testing.T) { root := t.TempDir() pristine := "the shipped template\n" rel := ".claude/rules/routing.md" - m := New("v1.0.0") + m := New("v1.0.0", paths.Claude) m.Set(rel, Hash(pristine), "v1.0.0") e, _ := m.Get(rel) @@ -188,7 +188,7 @@ func TestStatusDistinguishesPristineEditedMissing(t *testing.T) { // A missing manifest is "not a workspace", which is a different answer from "an // empty workspace" — and Find/IsWorkspace depend on the distinction. func TestLoadOnADirectoryWithoutAManifest(t *testing.T) { - m, found, err := Load(t.TempDir()) + m, found, err := Load(t.TempDir(), paths.Claude) if err != nil { t.Fatalf("Load: %v", err) } @@ -202,16 +202,16 @@ func TestLoadOnADirectoryWithoutAManifest(t *testing.T) { func TestSaveLoadRoundTrip(t *testing.T) { root := t.TempDir() - m := New("v1.2.3") + m := New("v1.2.3", paths.Claude) m.Set(".claude/rules/routing.md", Hash("r"), "v1.2.3") m.Set(".claude/CLAUDE.md", Hash("c"), "v1.0.0") - if err := Save(root, m); err != nil { + if err := Save(root, paths.Claude, m); err != nil { t.Fatalf("Save: %v", err) } if !workspace.IsWorkspace(root) { t.Error("Save did not create the workspace marker") } - got, found, err := Load(root) + got, found, err := Load(root, paths.Claude) if err != nil || !found { t.Fatalf("Load = (%v, %v), want found", found, err) } @@ -224,21 +224,48 @@ func TestSaveLoadRoundTrip(t *testing.T) { } } +// The harness is load-bearing state rather than a diagnostic like the scc +// version: an upgrade re-renders the recorded template version to reconstruct the +// merge base, and a template rendered for the wrong harness produces a base the +// file never had — which is how a merge silently clobbers. +func TestHarnessRoundTripsAndIsPerHarness(t *testing.T) { + root := t.TempDir() + for _, h := range paths.Harnesses() { + m := New("v1.2.3", h) + m.Set(h.Dir+"/rules/routing.md", Hash("r"), "5") + if err := Save(root, h, m); err != nil { + t.Fatalf("%s: Save: %v", h.ID, err) + } + } + for _, h := range paths.Harnesses() { + got, found, err := Load(root, h) + if err != nil || !found { + t.Fatalf("%s: Load = (%v, %v), want found", h.ID, found, err) + } + if got.Harness != h.ID { + t.Errorf("%s: harness = %q, want %q", h.ID, got.Harness, h.ID) + } + if _, ok := got.Get(h.Dir + "/rules/routing.md"); !ok { + t.Errorf("%s: manifest does not carry its own harness's entry", h.ID) + } + } +} + // Save writes what Bytes reports, so a caller can compare against the file on // disk and skip an identical write — which is what makes a second `scc init` a // no-op rather than a rewrite. func TestSaveWritesExactlyBytes(t *testing.T) { root := t.TempDir() - m := New("v1.0.0") + m := New("v1.0.0", paths.Claude) m.Set("a.md", Hash("a"), "v1.0.0") want, err := m.Bytes() if err != nil { t.Fatalf("Bytes: %v", err) } - if err := Save(root, m); err != nil { + if err := Save(root, paths.Claude, m); err != nil { t.Fatalf("Save: %v", err) } - got, err := os.ReadFile(paths.Manifest(root)) + got, err := os.ReadFile(paths.Claude.Manifest(root)) if err != nil { t.Fatalf("ReadFile: %v", err) } @@ -249,10 +276,10 @@ func TestSaveWritesExactlyBytes(t *testing.T) { func TestLoadRejectsInvalidJSON(t *testing.T) { root := t.TempDir() - if err := workspace.AtomicWrite(paths.Manifest(root), []byte("{not json"), 0o644); err != nil { + if err := workspace.AtomicWrite(paths.Claude.Manifest(root), []byte("{not json"), 0o644); err != nil { t.Fatalf("AtomicWrite: %v", err) } - if _, found, err := Load(root); err == nil { + if _, found, err := Load(root, paths.Claude); err == nil { t.Errorf("Load = (found %v, nil error), want an error naming the file", found) } } diff --git a/internal/paths/paths.go b/internal/paths/paths.go index 103f072..86e8ef4 100644 --- a/internal/paths/paths.go +++ b/internal/paths/paths.go @@ -2,24 +2,154 @@ // directory and file name the CLI reads or writes is defined here so the layout // lives in exactly one place — never hardcode ".claude" or "specs" elsewhere. // -// The layout is Claude Code-native: no conversion layer, no parallel config -// format, and no directory of its own. Everything scc keeps lives under -// .claude/ — Claude Code's own configuration directory — alongside the agents, -// skills, and commands Claude Code already reads from there. +// The layout is harness-native: no conversion layer, no parallel config format, +// and no directory of scc's own. Everything scc keeps for a harness lives under +// that harness's own configuration directory — .claude/, .codex/, .opencode/ — +// alongside the agents, skills, and commands the harness already reads from +// there. See Harness. // -// Every segment the product governs is defined here now: Claude Code's own -// directories, scc's manifest, the rules directory the scaffolded CLAUDE.md -// points at, the two work trees (specs/, plans/), and the knowledge base under -// docs/. +// The work trees are the opposite: specs/, plans/, and docs/ are the same in +// every workspace, because they are the product, not the tool. Nothing about a +// requirement or a wiki page changes with the agent reading it, and a repo that +// swapped harnesses would otherwise have to move its own knowledge base. package paths -import "path/filepath" +import ( + "fmt" + "path/filepath" + "sort" + "strings" +) // ClaudeDir is Claude Code's own configuration directory at the project root. // scc writes into it — agents, skills, commands, hooks, and its own two files — // but it does not own it. const ClaudeDir = ".claude" +// The other harnesses' configuration directories, on the same terms. +const ( + CodexDir = ".codex" + OpenCodeDir = ".opencode" +) + +// Harness is one agent harness's on-disk conventions — the whole of what scc has +// to know to scaffold into it. +// +// It exists because the methodology is not Claude Code's. The rules, the review +// agents, and the knowledge-base skills are Markdown that says how to work; what +// differs between harnesses is only where each file goes, what the entry file is +// called, and what dialect of frontmatter the loader parses. Encoding that as +// data — rather than as a second copy of the template set per harness — is what +// keeps one prose source honest across all three. +type Harness struct { + // ID is the name on the command line and in the manifest: "claude", + // "codex", "opencode". + ID string + + // Label is the tool's own name, for anything a person reads. The IDs are + // flags and JSON; "Claude Code" is what the user actually installed. + Label string + + // Dir is the harness's configuration directory at the project root. + Dir string + + // EntryFile is the instruction file the harness loads on its own, at the + // project root. Claude Code reads CLAUDE.md; Codex and opencode both read + // AGENTS.md, which is the same idea under the name the wider ecosystem + // standardized on. + EntryFile string + + // AgentsSeg is where subagent definitions go under Dir. opencode's loader + // accepts agent/ and agents/ both; Claude Code and Codex each accept one. + AgentsSeg string + + // AgentFormat is the dialect a subagent definition is written in: + // FormatMarkdown for the YAML-frontmatter files Claude Code and opencode + // read, FormatTOML for Codex's agent role files. + AgentFormat Format + + // SkillsSeg is where SKILL.md directories go under Dir. All three harnesses + // implement the same Agent Skills format, which is why `scc validate` can + // check skills without knowing which harness wrote them. + SkillsSeg string + + // CommandsSeg is where slash commands go under Dir, or "" when the harness + // has no project-level command surface. Codex is the "" case: its custom + // prompts are user-global and deprecated in favor of skills, so scc ships + // the skills alone there rather than writing into the user's home directory. + CommandsSeg string + + // RulesSeg is where the methodology goes under Dir. No harness loads it on + // its own — the entry file's table is what sends the agent to a rule when + // the concern is live — so this is scc's choice in all three, kept parallel + // so one layout is learned once. + RulesSeg string +} + +// Format is the dialect a harness's subagent definitions are written in. +type Format string + +const ( + FormatMarkdown Format = "md" + FormatTOML Format = "toml" +) + +// AgentExt is the file extension an agent definition takes in this harness. +func (h Harness) AgentExt() string { + if h.AgentFormat == FormatTOML { + return ".toml" + } + return ".md" +} + +// The harnesses scc can scaffold into. +// +// Claude Code is the default and the reference implementation: it is where the +// methodology was designed, and the one whose subagent, skill, and slash-command +// surfaces all exist at project scope. +var ( + Claude = Harness{ + ID: "claude", Label: "Claude Code", Dir: ClaudeDir, EntryFile: "CLAUDE.md", + AgentsSeg: "agents", AgentFormat: FormatMarkdown, + SkillsSeg: "skills", CommandsSeg: "commands", RulesSeg: "rules", + } + Codex = Harness{ + ID: "codex", Label: "Codex", Dir: CodexDir, EntryFile: "AGENTS.md", + AgentsSeg: "agents", AgentFormat: FormatTOML, + SkillsSeg: "skills", CommandsSeg: "", RulesSeg: "rules", + } + OpenCode = Harness{ + ID: "opencode", Label: "opencode", Dir: OpenCodeDir, EntryFile: "AGENTS.md", + AgentsSeg: "agent", AgentFormat: FormatMarkdown, + SkillsSeg: "skills", CommandsSeg: "command", RulesSeg: "rules", + } +) + +// Harnesses returns every supported harness, in a fixed order. The order is the +// probe order for the workspace marker, so it is deliberate rather than +// alphabetical: Claude Code first because it is the default. +func Harnesses() []Harness { return []Harness{Claude, Codex, OpenCode} } + +// ParseHarness resolves an ID from the command line. +func ParseHarness(id string) (Harness, error) { + for _, h := range Harnesses() { + if h.ID == id { + return h, nil + } + } + return Harness{}, fmt.Errorf("unknown harness %q: expected one of %s", id, HarnessIDs()) +} + +// HarnessIDs lists the IDs for help text and errors. +func HarnessIDs() string { + ids := make([]string, 0, len(Harnesses())) + for _, h := range Harnesses() { + ids = append(ids, h.ID) + } + sort.Strings(ids) + return strings.Join(ids, ", ") +} + // ManifestSeg is scc's only file, and it does two jobs. // // It records, per scc-managed file, a content hash and the template version that @@ -31,44 +161,25 @@ const ClaudeDir = ".claude" // Its presence is also the workspace marker: only scc writes it, so it exists // exactly when scc has scaffolded this directory — the question a marker answers. // -// The marker is deliberately this FILE and not .claude/ itself. ~/.claude is -// Claude Code's global configuration directory and exists on every machine that -// runs Claude Code, so an upward walk for the directory resolves the root to -// $HOME for any command run outside a workspace — and every command would then -// read and write the user's global configuration. .claude/ also exists in every -// repo that merely *uses* Claude Code, where scc was never initialized. See -// workspace.Find. +// The marker is deliberately this FILE and not the harness directory. Every one +// of the three has a global twin in the user's home — ~/.claude, ~/.codex, +// ~/.config/opencode — and exists on every machine that runs that tool, so an +// upward walk accepting the directory would resolve the root to $HOME for any +// command run outside a workspace, and every command would then read and write +// the user's global configuration. The directories also exist in every repo that +// merely *uses* the harness, where scc was never initialized. Only scc writes +// this file. See workspace.Find. +// +// One manifest per harness, in that harness's own directory. A workspace +// scaffolded for two harnesses is two managed trees that happen to share a repo, +// and each has to be upgradable on its own. // // There is no scc config file. scc runs no tests and no linters (see the design // doc's §5 and §7 — that is the orchestrator's job), so it has nothing to -// configure; a project's test and lint commands belong in a rule under -// .claude/rules/, which is Markdown the orchestrator already reads. +// configure; a project's test and lint commands belong in a rule under the +// harness's rules directory, which is Markdown the orchestrator already reads. const ManifestSeg = "scc-manifest.json" -// Entry-point files at the project root. -const ( - // EntryFile is the agent entry point. Keeping it small is a product - // requirement, not a preference: every Claude Code session pays for it in - // context, so it links out to steering/knowledge rather than inlining them. - EntryFile = "CLAUDE.md" -) - -// Directory segments under .claude/. All but RulesSeg are Claude Code -// conventions. -const ( - AgentsSeg = "agents" // subagent definitions (review, security) - SkillsSeg = "skills" // model-invoked skills - CommandsSeg = "commands" // slash commands - HooksSeg = "hooks" // deterministic automation scripts - SettingsSeg = "settings.json" // hooks + permissions - - // RulesSeg holds the methodology, one file per concern. It is where scc's - // product actually lives: CLAUDE.md stays small and links here, because every - // session pays for CLAUDE.md in context and a rule only needs reading when it - // is relevant. Length has a measured cost in accuracy, not just in tokens. - RulesSeg = "rules" -) - // Project-root directory segments that scc governs. The split is strict and each // name means exactly one thing: specs/ holds features, plans/ holds initiatives, // docs/ is the knowledge base. csdd kept plans under docs/plans/; a plan is work, @@ -102,27 +213,54 @@ const ( WikiLog = "changelog.md" // what changed in the wiki, and when ) -// Claude returns the .claude/ directory under root. -func Claude(root string) string { return filepath.Join(root, ClaudeDir) } +// Config returns the harness's configuration directory under root. +func (h Harness) Config(root string) string { return filepath.Join(root, h.Dir) } -// Agents returns .claude/agents/. -func Agents(root string) string { return filepath.Join(root, ClaudeDir, AgentsSeg) } +// Agents returns the harness's subagent directory. +func (h Harness) Agents(root string) string { return filepath.Join(root, h.Dir, h.AgentsSeg) } -// Skills returns .claude/skills/. -func Skills(root string) string { return filepath.Join(root, ClaudeDir, SkillsSeg) } +// Skills returns the harness's skills directory. +func (h Harness) Skills(root string) string { return filepath.Join(root, h.Dir, h.SkillsSeg) } -// Commands returns .claude/commands/. -func Commands(root string) string { return filepath.Join(root, ClaudeDir, CommandsSeg) } +// Commands returns the harness's slash-command directory, or "" when it has no +// project-level command surface. A caller that joins onto "" would write into +// the configuration directory itself, so check before using it. +func (h Harness) Commands(root string) string { + if h.CommandsSeg == "" { + return "" + } + return filepath.Join(root, h.Dir, h.CommandsSeg) +} -// Hooks returns .claude/hooks/. -func Hooks(root string) string { return filepath.Join(root, ClaudeDir, HooksSeg) } +// Rules returns the harness's rules directory. +func (h Harness) Rules(root string) string { return filepath.Join(root, h.Dir, h.RulesSeg) } -// Settings returns .claude/settings.json. -func Settings(root string) string { return filepath.Join(root, ClaudeDir, SettingsSeg) } +// Rule returns /.md. Callers must pass name through +// workspace.SafeName first when it arrives from CLI args. +func (h Harness) Rule(root, name string) string { + return filepath.Join(root, h.Dir, h.RulesSeg, name+".md") +} -// Manifest returns .claude/scc-manifest.json, scc's only file and the workspace -// marker. -func Manifest(root string) string { return filepath.Join(root, ClaudeDir, ManifestSeg) } +// Entry returns the harness's project-root instruction file. +func (h Harness) Entry(root string) string { return filepath.Join(root, h.EntryFile) } + +// Manifest returns /scc-manifest.json, scc's only file and the +// workspace marker for this harness. +func (h Harness) Manifest(root string) string { + return filepath.Join(root, h.Dir, ManifestSeg) +} + +// SkillDirs returns every harness's skills directory under root, in Harnesses +// order. `scc validate` checks skills wherever they are, because the Agent +// Skills format is one standard and a workspace scaffolded for two harnesses +// has two copies of it to keep conforming. +func SkillDirs(root string) []string { + dirs := make([]string, 0, len(Harnesses())) + for _, h := range Harnesses() { + dirs = append(dirs, h.Skills(root)) + } + return dirs +} // Specs returns the project-root specs/ directory. func Specs(root string) string { return filepath.Join(root, SpecsSeg) } @@ -157,15 +295,6 @@ func Plan(root, name string) string { return filepath.Join(root, PlansSeg, name+".md") } -// Rules returns .claude/rules/. -func Rules(root string) string { return filepath.Join(root, ClaudeDir, RulesSeg) } - -// Rule returns .claude/rules/.md. Callers must pass name through -// workspace.SafeName first when it arrives from CLI args. -func Rule(root, name string) string { - return filepath.Join(root, ClaudeDir, RulesSeg, name+".md") -} - // Docs returns the project-root docs/ knowledge base. func Docs(root string) string { return filepath.Join(root, DocsSeg) } @@ -189,6 +318,3 @@ func Glossary(root string) string { return filepath.Join(root, DocsSeg, Glossary // never something adopted silently — and because dependency manifests are // structured data, that rule is checkable without reading any source. func Stack(root string) string { return filepath.Join(root, DocsSeg, StackSeg) } - -// Entry returns the project-root CLAUDE.md. -func Entry(root string) string { return filepath.Join(root, EntryFile) } diff --git a/internal/scaffold/scaffold.go b/internal/scaffold/scaffold.go index 1462b8d..0e98a70 100644 --- a/internal/scaffold/scaffold.go +++ b/internal/scaffold/scaffold.go @@ -55,6 +55,7 @@ type Change struct { // Result is what a run did, in a shape both the human report and --json read from. type Result struct { Root string `json:"root"` + Harness string `json:"harness"` Changes []Change `json:"changes"` Created int `json:"created"` Skipped int `json:"skipped"` @@ -70,6 +71,10 @@ type Options struct { // worth knowing when a workspace behaves like a different version than the user // expects. No logic branches on it; only assets.Version decides behavior. SCCVersion string + // Harness is which agent harness to scaffold for. The zero value is not a + // harness, so a caller that forgets it gets a refusal rather than a tree + // written into the wrong directory. + Harness paths.Harness // Force overwrites existing files instead of leaving them alone. Force bool } @@ -83,21 +88,25 @@ type Options struct { // so a second run is a true no-op — the marker's mtime is not noise a reviewer // should have to explain. func Apply(root string, opts Options) (*Result, error) { - prior, found, err := manifest.Load(root) + h := opts.Harness + if h.ID == "" { + return nil, fmt.Errorf("no harness given: expected one of %s", paths.HarnessIDs()) + } + prior, found, err := manifest.Load(root, h) if err != nil { return nil, err } - res := &Result{Root: root, AlreadyPresent: found} + res := &Result{Root: root, Harness: h.ID, AlreadyPresent: found} - for _, dir := range assets.Dirs() { + for _, dir := range assets.Dirs(h) { if err := os.MkdirAll(filepath.Join(root, filepath.FromSlash(dir)), 0o755); err != nil { return nil, err } } - next := manifest.New(opts.SCCVersion) - for _, f := range assets.Workspace() { - pristine, err := assets.Content(f.Name) + next := manifest.New(opts.SCCVersion, h) + for _, f := range assets.Workspace(h) { + pristine, err := assets.Render(h, f) if err != nil { return nil, err } @@ -124,7 +133,7 @@ func Apply(root string, opts Options) (*Result, error) { // is exactly the set of files scc manages, and a stale entry would send a later // upgrade looking for a template that does not exist. managed := map[string]bool{} - for _, f := range assets.Workspace() { + for _, f := range assets.Workspace(h) { managed[f.Rel] = true } for _, e := range prior.Files { @@ -133,12 +142,12 @@ func Apply(root string, opts Options) (*Result, error) { } } - same, err := manifestUnchanged(root, next) + same, err := manifestUnchanged(root, h, next) if err != nil { return nil, err } if !same { - if err := manifest.Save(root, next); err != nil { + if err := manifest.Save(root, h, next); err != nil { return nil, err } res.ManifestWritten = true @@ -175,12 +184,12 @@ func write(root string, f assets.File, pristine string, prior *manifest.Manifest return action, workspace.AtomicWrite(dest, []byte(pristine), 0o644) } -func manifestUnchanged(root string, m *manifest.Manifest) (bool, error) { +func manifestUnchanged(root string, h paths.Harness, m *manifest.Manifest) (bool, error) { next, err := m.Bytes() if err != nil { return false, err } - current, err := os.ReadFile(paths.Manifest(root)) + current, err := os.ReadFile(h.Manifest(root)) if errors.Is(err, fs.ErrNotExist) { return false, nil } diff --git a/internal/scaffold/scaffold_test.go b/internal/scaffold/scaffold_test.go index 33fec0d..4037a9c 100644 --- a/internal/scaffold/scaffold_test.go +++ b/internal/scaffold/scaffold_test.go @@ -13,13 +13,42 @@ import ( func apply(t *testing.T, root string, force bool) *Result { t.Helper() - res, err := Apply(root, Options{SCCVersion: "v0.0.0-test", Force: force}) + return applyTo(t, root, paths.Claude, force) +} + +func applyTo(t *testing.T, root string, h paths.Harness, force bool) *Result { + t.Helper() + res, err := Apply(root, Options{SCCVersion: "v0.0.0-test", Harness: h, Force: force}) if err != nil { t.Fatalf("Apply: %v", err) } return res } +// renderRel returns the bytes the template set produces for a destination, which +// is what "still pristine" means: the manifest hashes the render, not the +// template file. +func renderRel(t *testing.T, rel string) string { + t.Helper() + return renderRelFor(t, paths.Claude, rel) +} + +// renderRelFor is renderRel for a harness other than Claude Code. +func renderRelFor(t *testing.T, h paths.Harness, rel string) string { + t.Helper() + for _, f := range assets.Workspace(h) { + if f.Rel == rel { + out, err := assets.Render(h, f) + if err != nil { + t.Fatalf("Render %s: %v", rel, err) + } + return out + } + } + t.Fatalf("no managed file at %s", rel) + return "" +} + func read(t *testing.T, root, rel string) string { t.Helper() b, err := os.ReadFile(filepath.Join(root, filepath.FromSlash(rel))) @@ -36,8 +65,8 @@ func TestApplyWritesTheWholeSet(t *testing.T) { if res.AlreadyPresent { t.Error("AlreadyPresent = true for a fresh directory") } - for _, f := range assets.Workspace() { - want, err := assets.Content(f.Name) + for _, f := range assets.Workspace(paths.Claude) { + want, err := assets.Render(paths.Claude, f) if err != nil { t.Fatalf("Content: %v", err) } @@ -45,10 +74,10 @@ func TestApplyWritesTheWholeSet(t *testing.T) { t.Errorf("%s: content does not match the template", f.Rel) } } - if res.Created != len(assets.Workspace()) { - t.Errorf("created = %d, want %d", res.Created, len(assets.Workspace())) + if res.Created != len(assets.Workspace(paths.Claude)) { + t.Errorf("created = %d, want %d", res.Created, len(assets.Workspace(paths.Claude))) } - for _, dir := range assets.Dirs() { + for _, dir := range assets.Dirs(paths.Claude) { info, err := os.Stat(filepath.Join(root, filepath.FromSlash(dir))) if err != nil || !info.IsDir() { t.Errorf("directory %s was not created: %v", dir, err) @@ -88,17 +117,17 @@ func TestApplyRecordsEveryManagedFile(t *testing.T) { root := t.TempDir() apply(t, root, false) - m, found, err := manifest.Load(root) + m, found, err := manifest.Load(root, paths.Claude) if err != nil || !found { t.Fatalf("Load = (%v, %v), want found", found, err) } if m.SCC != "v0.0.0-test" { t.Errorf("scc stamp = %q, want the version passed in", m.SCC) } - if len(m.Files) != len(assets.Workspace()) { - t.Fatalf("entries = %d, want %d", len(m.Files), len(assets.Workspace())) + if len(m.Files) != len(assets.Workspace(paths.Claude)) { + t.Fatalf("entries = %d, want %d", len(m.Files), len(assets.Workspace(paths.Claude))) } - for _, f := range assets.Workspace() { + for _, f := range assets.Workspace(paths.Claude) { e, ok := m.Get(f.Rel) if !ok { t.Errorf("%s is not in the manifest", f.Rel) @@ -120,7 +149,7 @@ func TestApplyRecordsEveryManagedFile(t *testing.T) { func TestManifestRecordsOnlySccManagedFiles(t *testing.T) { root := t.TempDir() apply(t, root, false) - m, _, err := manifest.Load(root) + m, _, err := manifest.Load(root, paths.Claude) if err != nil { t.Fatalf("Load: %v", err) } @@ -136,7 +165,7 @@ func TestManifestRecordsOnlySccManagedFiles(t *testing.T) { func TestApplyIsIdempotent(t *testing.T) { root := t.TempDir() apply(t, root, false) - before, err := os.ReadFile(paths.Manifest(root)) + before, err := os.ReadFile(paths.Claude.Manifest(root)) if err != nil { t.Fatalf("ReadFile: %v", err) } @@ -148,13 +177,13 @@ func TestApplyIsIdempotent(t *testing.T) { if res.Created != 0 || res.Replaced != 0 { t.Errorf("second run wrote files: created=%d replaced=%d", res.Created, res.Replaced) } - if res.Skipped != len(assets.Workspace()) { - t.Errorf("skipped = %d, want %d", res.Skipped, len(assets.Workspace())) + if res.Skipped != len(assets.Workspace(paths.Claude)) { + t.Errorf("skipped = %d, want %d", res.Skipped, len(assets.Workspace(paths.Claude))) } if res.ManifestWritten { t.Error("second run rewrote the manifest") } - after, err := os.ReadFile(paths.Manifest(root)) + after, err := os.ReadFile(paths.Claude.Manifest(root)) if err != nil { t.Fatalf("ReadFile: %v", err) } @@ -183,7 +212,7 @@ func TestApplyNeverOverwritesWithoutForce(t *testing.T) { if len(res.Clobbered) != 0 { t.Errorf("clobbered = %v on a run without --force", res.Clobbered) } - m, _, err := manifest.Load(root) + m, _, err := manifest.Load(root, paths.Claude) if err != nil { t.Fatalf("Load: %v", err) } @@ -212,10 +241,7 @@ func TestForceNamesEveryEditedFileItClobbers(t *testing.T) { if len(res.Clobbered) != 1 || res.Clobbered[0] != edited { t.Errorf("clobbered = %v, want exactly [%s]", res.Clobbered, edited) } - want, err := assets.Content("claude/rules/routing.md") - if err != nil { - t.Fatalf("Content: %v", err) - } + want := renderRel(t, ".claude/rules/routing.md") if got := read(t, root, edited); got != want { t.Error("--force did not restore the template") } @@ -232,12 +258,12 @@ func TestForceNamesEveryEditedFileItClobbers(t *testing.T) { // default. func TestForceReportsAdoptedFilesAsClobbered(t *testing.T) { root := t.TempDir() - if err := workspace.AtomicWrite(paths.Entry(root), []byte("# their own CLAUDE.md\n"), 0o644); err != nil { + if err := workspace.AtomicWrite(paths.Claude.Entry(root), []byte("# their own CLAUDE.md\n"), 0o644); err != nil { t.Fatalf("AtomicWrite: %v", err) } res := apply(t, root, true) - if len(res.Clobbered) != 1 || res.Clobbered[0] != paths.EntryFile { - t.Errorf("clobbered = %v, want [%s]", res.Clobbered, paths.EntryFile) + if len(res.Clobbered) != 1 || res.Clobbered[0] != paths.Claude.EntryFile { + t.Errorf("clobbered = %v, want [%s]", res.Clobbered, paths.Claude.EntryFile) } } @@ -246,19 +272,19 @@ func TestForceReportsAdoptedFilesAsClobbered(t *testing.T) { func TestApplyAdoptsAnExistingFileWithoutLosingIt(t *testing.T) { root := t.TempDir() theirs := "# their own CLAUDE.md\n" - if err := workspace.AtomicWrite(paths.Entry(root), []byte(theirs), 0o644); err != nil { + if err := workspace.AtomicWrite(paths.Claude.Entry(root), []byte(theirs), 0o644); err != nil { t.Fatalf("AtomicWrite: %v", err) } apply(t, root, false) - if got := read(t, root, paths.EntryFile); got != theirs { + if got := read(t, root, paths.Claude.EntryFile); got != theirs { t.Errorf("adoption overwrote their file:\n%s", got) } - m, _, err := manifest.Load(root) + m, _, err := manifest.Load(root, paths.Claude) if err != nil { t.Fatalf("Load: %v", err) } - e, ok := m.Get(paths.EntryFile) + e, ok := m.Get(paths.Claude.EntryFile) if !ok { t.Fatal("the adopted file is not in the manifest") } @@ -304,12 +330,12 @@ func TestStaleManifestEntriesAreDropped(t *testing.T) { root := t.TempDir() apply(t, root, false) - m, _, err := manifest.Load(root) + m, _, err := manifest.Load(root, paths.Claude) if err != nil { t.Fatalf("Load: %v", err) } m.Set(".claude/rules/removed-in-a-later-version.md", "deadbeef", "0") - if err := manifest.Save(root, m); err != nil { + if err := manifest.Save(root, paths.Claude, m); err != nil { t.Fatalf("Save: %v", err) } @@ -323,7 +349,7 @@ func TestStaleManifestEntriesAreDropped(t *testing.T) { if len(unmanaged) != 1 || unmanaged[0] != ".claude/rules/removed-in-a-later-version.md" { t.Errorf("unmanaged = %v, want the stale entry", unmanaged) } - after, _, err := manifest.Load(root) + after, _, err := manifest.Load(root, paths.Claude) if err != nil { t.Fatalf("Load: %v", err) } @@ -345,7 +371,7 @@ func TestApplyRestoresADeletedFile(t *testing.T) { if res.Created != 1 { t.Errorf("created = %d, want 1", res.Created) } - want, _ := assets.Content("claude/agents/code-review.md") + want := renderRel(t, ".claude/agents/code-review.md") if got := read(t, root, rel); got != want { t.Error("the deleted file was not restored from the template") } @@ -363,6 +389,63 @@ func TestChangesAreSortedByPath(t *testing.T) { } } +// Every harness gets a complete, self-contained tree: its own manifest, its own +// rules, and an entry file its own tool loads. +func TestApplyScaffoldsEveryHarness(t *testing.T) { + for _, h := range paths.Harnesses() { + root := t.TempDir() + res := applyTo(t, root, h, false) + if res.Harness != h.ID { + t.Errorf("%s: result harness = %q", h.ID, res.Harness) + } + if !workspace.IsWorkspace(root) { + t.Errorf("%s: no marker written", h.ID) + } + m, found, err := manifest.Load(root, h) + if err != nil || !found { + t.Fatalf("%s: Load = (%v, %v)", h.ID, found, err) + } + if m.Harness != h.ID { + t.Errorf("%s: manifest records harness %q", h.ID, m.Harness) + } + if len(m.Files) != len(assets.Workspace(h)) { + t.Errorf("%s: %d entries, want %d", h.ID, len(m.Files), len(assets.Workspace(h))) + } + read(t, root, h.EntryFile) + read(t, root, h.Dir+"/"+h.RulesSeg+"/routing.md") + } +} + +// Running init twice for two harnesses is how a repo worked on from two tools +// gets both trees. The second run must not touch the first's files — and Codex +// and opencode share one entry file, which is exactly where an overwrite would +// destroy the user's own prose. +func TestASecondHarnessLeavesTheFirstAlone(t *testing.T) { + root := t.TempDir() + applyTo(t, root, paths.Codex, false) + + mine := read(t, root, paths.Codex.EntryFile) + "\n## my own section\n" + if err := workspace.AtomicWrite(paths.Codex.Entry(root), []byte(mine), 0o644); err != nil { + t.Fatalf("AtomicWrite: %v", err) + } + + res := applyTo(t, root, paths.OpenCode, false) + if got := read(t, root, paths.OpenCode.EntryFile); got != mine { + t.Error("the second harness overwrote the shared entry file") + } + for _, c := range res.Changes { + if c.Path == paths.OpenCode.EntryFile && c.Action != Skipped { + t.Errorf("entry file action = %q, want %q", c.Action, Skipped) + } + } + // Both trees are present and each is findable on its own manifest. + 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") +} + func splitLines(s string) []string { var out []string start := 0 diff --git a/internal/validate/skill.go b/internal/validate/skill.go index 57f0a54..489d6a4 100644 --- a/internal/validate/skill.go +++ b/internal/validate/skill.go @@ -39,7 +39,12 @@ var skillNameRe = regexp.MustCompile(`^[a-z0-9]+(-[a-z0-9]+)*$`) // skillFile is the file the spec requires in every skill directory. const skillFile = "SKILL.md" -// Skills validates every skill under .claude/skills/. +// Skills validates every skill in every harness's skills directory. +// +// It checks all of them rather than the one harness this workspace was +// scaffolded for, because the Agent Skills format is one published standard and a +// repo worked on from two tools carries two copies of it — a skill that stopped +// conforming in the copy scc did not write is still a broken skill. // // Implemented in Go rather than by delegating to the reference `skills-ref` // validator: scc is a single binary on six platforms, and shelling out to Node for a @@ -49,25 +54,26 @@ const skillFile = "SKILL.md" // workspace with no skills. func Skills(root string) (*finding.Set, error) { set := &finding.Set{} - dir := paths.Skills(root) - if !isDir(dir) { - return set, nil - } - entries, err := os.ReadDir(dir) - if err != nil { - return nil, err - } - names := make([]string, 0, len(entries)) - for _, e := range entries { - if e.IsDir() { - names = append(names, e.Name()) + for _, dir := range paths.SkillDirs(root) { + if !isDir(dir) { + continue } - } - sort.Strings(names) - for _, name := range names { - if err := skill(set, root, filepath.Join(dir, name), name); err != nil { + entries, err := os.ReadDir(dir) + if err != nil { return nil, err } + names := make([]string, 0, len(entries)) + for _, e := range entries { + if e.IsDir() { + names = append(names, e.Name()) + } + } + sort.Strings(names) + for _, name := range names { + if err := skill(set, root, filepath.Join(dir, name), name); err != nil { + return nil, err + } + } } return set, nil } diff --git a/internal/validate/skill_test.go b/internal/validate/skill_test.go index 2a154b6..8256712 100644 --- a/internal/validate/skill_test.go +++ b/internal/validate/skill_test.go @@ -13,7 +13,7 @@ import ( // writeSkill lays out .claude/skills//SKILL.md with the given content. func writeSkill(t *testing.T, root, name, content string) string { t.Helper() - dir := filepath.Join(paths.Skills(root), name) + dir := filepath.Join(paths.Claude.Skills(root), name) if err := os.MkdirAll(dir, 0o755); err != nil { t.Fatalf("MkdirAll: %v", err) } @@ -203,7 +203,7 @@ func TestBodyBudget(t *testing.T) { func TestMissingSkillMD(t *testing.T) { root := t.TempDir() - if err := os.MkdirAll(filepath.Join(paths.Skills(root), "empty-dir"), 0o755); err != nil { + if err := os.MkdirAll(filepath.Join(paths.Claude.Skills(root), "empty-dir"), 0o755); err != nil { t.Fatalf("MkdirAll: %v", err) } set := skills(t, root) diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index 4a73f6c..9617940 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -75,17 +75,20 @@ func AtomicWrite(path string, data []byte, perm os.FileMode) error { return os.Rename(tmpName, path) } -// Find walks upward from start looking for the workspace marker and returns the -// directory holding it, or start unchanged when there is none. +// Find walks upward from start looking for a workspace marker and returns the +// directory holding it, or start unchanged when there is none. Any harness's +// marker answers: a workspace is a workspace whichever tool it was scaffolded +// for, and specs/, plans/, and docs/ sit at that root regardless. // -// The marker is the file .claude/scc-manifest.json, not the .claude/ directory. -// That distinction is the whole point, for two reasons: ~/.claude is Claude -// Code's own global configuration directory and exists on every machine that -// runs Claude Code, so a walk accepting the directory would resolve the root to -// $HOME for any command run outside a workspace — and every command would then -// read and write the user's global configuration. And .claude/ exists in every -// repo that merely uses Claude Code, where scc was never initialized. Only scc -// writes the manifest, so its presence answers exactly the right question. +// The marker is the file /scc-manifest.json, never the harness +// directory. That distinction is the whole point, for two reasons: every one of +// the three has a global twin in the user's home — ~/.claude, ~/.codex, +// ~/.config/opencode — that exists on any machine running that tool, so a walk +// accepting the directory would resolve the root to $HOME for any command run +// outside a workspace, and every command would then read and write the user's +// global configuration. And those directories exist in every repo that merely +// uses the harness, where scc was never initialized. Only scc writes the +// manifest, so its presence answers exactly the right question. func Find(start string) string { abs, err := filepath.Abs(start) if err != nil { @@ -93,7 +96,7 @@ func Find(start string) string { } cur := abs for { - if isFile(paths.Manifest(cur)) { + if IsWorkspace(cur) { return cur } parent := filepath.Dir(cur) @@ -111,9 +114,23 @@ func isFile(path string) bool { return err == nil && info.Mode().IsRegular() } -// IsWorkspace reports whether root holds the marker — i.e. whether it is an -// initialized workspace rather than just the directory the walk fell back to. -func IsWorkspace(root string) bool { return isFile(paths.Manifest(root)) } +// IsWorkspace reports whether root holds any harness's marker — i.e. whether it +// is an initialized workspace rather than just the directory the walk fell back +// to. +func IsWorkspace(root string) bool { return len(Harnesses(root)) > 0 } + +// Harnesses returns the harnesses root has been initialized for, in +// paths.Harnesses order. Usually one; more when a repo is worked on from two +// tools, which is a supported outcome of running `scc init` twice. +func Harnesses(root string) []paths.Harness { + var found []paths.Harness + for _, h := range paths.Harnesses() { + if isFile(h.Manifest(root)) { + found = append(found, h) + } + } + return found +} // Resolve normalizes the user-supplied --root flag. An empty arg falls back to // Find(cwd). A non-empty arg must exist on disk, otherwise an error is returned. diff --git a/internal/workspace/workspace_test.go b/internal/workspace/workspace_test.go index 1902f33..28f805e 100644 --- a/internal/workspace/workspace_test.go +++ b/internal/workspace/workspace_test.go @@ -94,7 +94,7 @@ func TestAtomicWriteOverwrites(t *testing.T) { // markWorkspace creates the .claude/scc-manifest.json marker under root. func markWorkspace(t *testing.T, root string) { t.Helper() - if err := AtomicWrite(paths.Manifest(root), []byte("{}\n"), 0o644); err != nil { + if err := AtomicWrite(paths.Claude.Manifest(root), []byte("{}\n"), 0o644); err != nil { t.Fatalf("writing marker: %v", err) } } @@ -120,7 +120,7 @@ func TestFindWalksUpToMarker(t *testing.T) { // repo that merely uses Claude Code, where scc was never initialized. func TestFindIgnoresClaudeDirWithoutMarker(t *testing.T) { root := t.TempDir() - if err := os.MkdirAll(paths.Claude(root), 0o755); err != nil { + if err := os.MkdirAll(paths.Claude.Config(root), 0o755); err != nil { t.Fatalf("MkdirAll: %v", err) } deep := filepath.Join(root, "a") @@ -137,7 +137,7 @@ func TestFindIgnoresClaudeDirWithoutMarker(t *testing.T) { // not a workspace, and must not be silently accepted as one. func TestFindRejectsMarkerThatIsADirectory(t *testing.T) { root := t.TempDir() - if err := os.MkdirAll(paths.Manifest(root), 0o755); err != nil { + if err := os.MkdirAll(paths.Claude.Manifest(root), 0o755); err != nil { t.Fatalf("MkdirAll: %v", err) } assertSameDir(t, Find(root), root) // falls back to start, not "found" @@ -164,6 +164,41 @@ func TestFindWithoutMarkerReturnsStart(t *testing.T) { assertSameDir(t, Find(dir), dir) } +// A workspace is a workspace whichever harness scaffolded it: specs/, plans/, and +// docs/ sit at that root either way, so every command has to resolve it. +func TestFindAcceptsAnyHarnessMarker(t *testing.T) { + for _, h := range paths.Harnesses() { + root := t.TempDir() + if err := AtomicWrite(h.Manifest(root), []byte("{}\n"), 0o644); err != nil { + t.Fatalf("%s: writing marker: %v", h.ID, err) + } + deep := filepath.Join(root, "a", "b") + if err := os.MkdirAll(deep, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + assertSameDir(t, Find(deep), root) + got := Harnesses(root) + if len(got) != 1 || got[0].ID != h.ID { + t.Errorf("Harnesses = %v, want just %s", got, h.ID) + } + } +} + +// Running init twice for two harnesses leaves two managed trees in one repo, and +// both have to be visible: each is upgraded on its own manifest. +func TestHarnessesReportsEveryInitializedTree(t *testing.T) { + root := t.TempDir() + for _, h := range []paths.Harness{paths.Claude, paths.OpenCode} { + if err := AtomicWrite(h.Manifest(root), []byte("{}\n"), 0o644); err != nil { + t.Fatalf("%s: writing marker: %v", h.ID, err) + } + } + got := Harnesses(root) + if len(got) != 2 || got[0].ID != paths.Claude.ID || got[1].ID != paths.OpenCode.ID { + t.Errorf("Harnesses = %v, want [claude opencode] in that order", got) + } +} + func TestResolveRejectsMissingRoot(t *testing.T) { missing := filepath.Join(t.TempDir(), "nope") if _, err := Resolve(missing); err == nil { From 7b60eb33c6fd5a6a246be219019cc9796c1ef9b9 Mon Sep 17 00:00:00 2001 From: prode Date: Sat, 1 Aug 2026 20:32:26 -0300 Subject: [PATCH 2/5] feat(cli): add `scc update`, which shows its plan and asks before writing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every managed file is hashed against what this build renders and against what the manifest says scc last wrote. That splits it six ways — current, create, update, conflict, delete, orphan — plus owned for the two files that exist to be edited, and the plan is printed grouped by outcome before anything is touched. The confirmation is the feature, not decoration: update is the one command that can destroy work, and a summary the user agrees to is the difference between a tool and a surprise. --yes is that agreement given up front and is required when stdin is not a terminal, --dry-run reports and stops, and --json emits one shape whatever the run decided to do. A conflict is kept and named, never merged and never clobbered; --force is the separate decision. The kept file stays recorded at the version scc last wrote, which is precisely the base revision the three-way merge will need when it lands. That merge — plan.md phase 11.1, internal/merge — is still unbuilt: this is the honest subset of it, and reporting a conflict is strictly safer than resolving one badly. Without a flag it updates every harness the workspace was initialized for, since a repo worked on from two tools has two managed trees and leaving one behind would let them drift apart. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019yCM51izVEvKywpKDgYvNM --- internal/cli/cli.go | 5 +- internal/cli/update.go | 266 +++++++++++++++++++++++++++ internal/cli/update_test.go | 225 +++++++++++++++++++++++ internal/scaffold/update.go | 304 +++++++++++++++++++++++++++++++ internal/scaffold/update_test.go | 245 +++++++++++++++++++++++++ 5 files changed, 1044 insertions(+), 1 deletion(-) create mode 100644 internal/cli/update.go create mode 100644 internal/cli/update_test.go create mode 100644 internal/scaffold/update.go create mode 100644 internal/scaffold/update_test.go diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 33059aa..a841a76 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -58,6 +58,8 @@ func Run(args []string) int { return runVersion(args[1:]) case "init": return runInit(args[1:]) + case "update": + return runUpdate(args[1:]) case "spec": return runSpec(args[1:]) case "plan": @@ -94,13 +96,14 @@ func runVersion(args []string) int { } func usage() { - fmt.Fprintf(os.Stderr, `%s — spec-driven development for Claude Code + fmt.Fprintf(os.Stderr, `%s — spec-driven development for Claude Code, Codex, and opencode Usage: %s [flags] Commands: init Scaffold a workspace: rules, agents, skills, commands, layout, manifest + update Bring the managed files onto this build's templates, after showing the plan spec Create and inspect specs — new | list | show | delete | validate plan Create and inspect plans — new | list | delete | validate skill Agent Skills conformance — validate diff --git a/internal/cli/update.go b/internal/cli/update.go new file mode 100644 index 0000000..766cbf0 --- /dev/null +++ b/internal/cli/update.go @@ -0,0 +1,266 @@ +package cli + +import ( + "bufio" + "flag" + "fmt" + "io" + "os" + "strings" + + "github.com/protonspy/spec-claude-code/internal/assets" + "github.com/protonspy/spec-claude-code/internal/paths" + "github.com/protonspy/spec-claude-code/internal/render" + "github.com/protonspy/spec-claude-code/internal/scaffold" + "github.com/protonspy/spec-claude-code/internal/workspace" +) + +// runUpdate brings a workspace's managed files onto this build's templates. +// +// It reports before it acts, always. The plan is computed by hashing — every +// managed rule, agent, skill, and command is compared against what this version +// renders and against what the manifest says scc last wrote — so the three +// outcomes are distinguishable and are named separately in the summary: a file +// scc may replace because nothing was authored into it, a file the user changed, +// and a file this version no longer ships. +// +// Nothing is written until the user agrees. --yes is that agreement given up +// front, which is also what a non-interactive caller must pass: a command that +// blocked on a read nobody will answer would break the contract that an agent or +// a CI job drives scc exactly as well as a person does. +func runUpdate(args []string) int { + fs := flag.NewFlagSet("update", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + root := addRoot(fs) + yes := fs.Bool("yes", false, "apply without asking (required when stdin is not a terminal)") + force := fs.Bool("force", false, "also overwrite files you have edited, naming each one") + dryRun := fs.Bool("dry-run", false, "report the plan and write nothing") + picks := map[string]*bool{} + for _, h := range paths.Harnesses() { + picks[h.ID] = fs.Bool(h.ID, false, "update only the "+h.ID+" tree") + } + jsonOut := addJSON(fs) + rest, err := parseFlags(fs, args) + if err != nil { + return ExitError + } + if !noPositionals(rest, "update") { + return ExitError + } + + target, ok := resolveRoot(*root) + if !ok { + return ExitError + } + + harnesses, err := updateTargets(target, picks) + if err != nil { + render.Err(err.Error()) + return ExitError + } + + plans := make([]*scaffold.UpdatePlan, 0, len(harnesses)) + for _, h := range harnesses { + plan, err := scaffold.PlanUpdate(target, h) + if err != nil { + render.Err(fmt.Sprintf("update failed: %v", err)) + return ExitError + } + plans = append(plans, plan) + } + + if *jsonOut && *dryRun { + return emitJSON(struct { + Plans []*scaffold.UpdatePlan `json:"plans"` + }{plans}) + } + if !*jsonOut { + reportPlans(harnesses, plans, *force) + } + + pending := 0 + writes := false + for _, p := range plans { + pending += len(p.Pending()) + writes = writes || p.Writes(*force) + } + // The three ways an update ends without writing. Under --json they all emit the + // same document the applying path does, with an empty result list: one command, + // one output shape, whatever it decided to do. + if pending == 0 { + if *jsonOut { + return emitResults(nil) + } + render.OK(fmt.Sprintf("already on template version %s — nothing to do", assets.Version)) + return ExitOK + } + if *dryRun { + if *jsonOut { + return emitResults(nil) + } + render.Info("dry run: nothing was written") + return ExitOK + } + if !writes { + if *jsonOut { + return emitResults(nil) + } + render.Info("nothing to apply — the differences are all in files scc will not touch") + return ExitOK + } + + if !*yes { + if !interactive() { + render.Err("stdin is not a terminal: re-run with --yes to apply, or --dry-run to see the plan") + return ExitError + } + if !confirm(promptIn, "Apply these changes?") { + render.Info("nothing was written") + return ExitOK + } + } + + results := make([]*scaffold.UpdateResult, 0, len(harnesses)) + for i, h := range harnesses { + res, err := scaffold.ApplyUpdate(target, h, plans[i], scaffold.UpdateOptions{ + SCCVersion: version, + Force: *force, + }) + if err != nil { + render.Err(fmt.Sprintf("update failed: %v", err)) + return ExitError + } + results = append(results, res) + } + + if *jsonOut { + return emitResults(results) + } + applied := 0 + for _, res := range results { + for _, it := range res.Applied { + render.OK(fmt.Sprintf("%-8s %s", it.Action, it.Path)) + applied++ + } + } + render.OK(fmt.Sprintf("%d file(s) updated to template version %s", applied, assets.Version)) + for _, res := range results { + for _, it := range res.Kept { + if it.Action == scaffold.UpConflict { + render.Warn(fmt.Sprintf("kept your edited %s — pass --force to take the new version", it.Path)) + } + if it.Action == scaffold.UpOwned { + render.Warn(fmt.Sprintf("%s is yours and was left alone; a newer template exists", it.Path)) + } + } + } + return ExitOK +} + +// emitResults writes the one document `update --json` ever emits. Never null: a +// caller iterating `.results` must not have to special-case the no-op run. +func emitResults(results []*scaffold.UpdateResult) int { + if results == nil { + results = []*scaffold.UpdateResult{} + } + return emitJSON(struct { + Results []*scaffold.UpdateResult `json:"results"` + }{results}) +} + +// updateTargets resolves which trees to update: the ones named by flag, or every +// harness the workspace was initialized for. Updating all of them by default is +// the honest reading of "bring this workspace up to date" — a repo worked on from +// two tools has two managed trees, and leaving one behind would quietly let them +// drift apart. +func updateTargets(root string, picks map[string]*bool) ([]paths.Harness, error) { + initialized := workspace.Harnesses(root) + if len(initialized) == 0 { + return nil, fmt.Errorf("%s is not an scc workspace: run `%s init` first", root, prog()) + } + var wanted []paths.Harness + for _, h := range paths.Harnesses() { + if p, ok := picks[h.ID]; ok && *p { + wanted = append(wanted, h) + } + } + if len(wanted) == 0 { + return initialized, nil + } + have := map[string]bool{} + for _, h := range initialized { + have[h.ID] = true + } + for _, h := range wanted { + if !have[h.ID] { + return nil, fmt.Errorf("this workspace has no %s tree: run `%s init --%s` to create one", + h.Label, prog(), h.ID) + } + } + return wanted, nil +} + +// reportPlans prints the summary the user is agreeing to. Grouped by what happens +// to the file rather than by directory, because the question being answered is +// "what does this cost me", and the answer is entirely in the conflict group. +func reportPlans(harnesses []paths.Harness, plans []*scaffold.UpdatePlan, force bool) { + groups := []struct { + action scaffold.UpdateAction + label string + }{ + {scaffold.UpCreate, "create"}, + {scaffold.UpUpdate, "update"}, + {scaffold.UpDelete, "delete"}, + {scaffold.UpOrphan, "no longer managed, left on disk"}, + {scaffold.UpConflict, "you edited these"}, + {scaffold.UpOwned, "yours — never overwritten"}, + } + for i, h := range harnesses { + plan := plans[i] + if len(plan.Pending()) == 0 { + render.Info(fmt.Sprintf("%s: already on template version %s", h.Label, assets.Version)) + continue + } + fmt.Println() + render.Info(fmt.Sprintf("%s — %s/ and %s", render.Bold(h.Label), h.Dir, h.EntryFile)) + for _, g := range groups { + var listed []string + for _, it := range plan.Items { + if it.Action == g.action { + listed = append(listed, it.Path) + } + } + if len(listed) == 0 { + continue + } + head := fmt.Sprintf(" %s (%d)", g.label, len(listed)) + switch g.action { + case scaffold.UpConflict: + if force { + head += render.Yellow(" — --force will overwrite them") + } else { + head += render.Yellow(" — kept as they are") + } + } + fmt.Println(head) + for _, rel := range listed { + fmt.Println(" " + rel) + } + } + } + fmt.Println() +} + +// confirm asks a yes/no question, defaulting to no. Anything other than an +// explicit yes leaves the workspace untouched: the cost of a wrong "no" is +// running the command again, and the cost of a wrong "yes" is somebody's work. +func confirm(in io.Reader, question string) bool { + render.Ask(question + " [y/N]: ") + scanner := bufio.NewScanner(in) + if !scanner.Scan() { + fmt.Println() + return false + } + answer := strings.ToLower(strings.TrimSpace(scanner.Text())) + return answer == "y" || answer == "yes" +} diff --git a/internal/cli/update_test.go b/internal/cli/update_test.go new file mode 100644 index 0000000..1799acc --- /dev/null +++ b/internal/cli/update_test.go @@ -0,0 +1,225 @@ +package cli + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/protonspy/spec-claude-code/internal/manifest" + "github.com/protonspy/spec-claude-code/internal/paths" + "github.com/protonspy/spec-claude-code/internal/workspace" +) + +// A workspace this build just scaffolded is current, so update says so and exits +// 0 without asking anything. +func TestUpdateOnACurrentWorkspaceDoesNothing(t *testing.T) { + root := initWorkspace(t) + stdout, stderr, code := run(t, "update", "--root", root) + if code != ExitOK { + t.Fatalf("exit = %d (%s)", code, stderr) + } + if !strings.Contains(stdout, "nothing to do") { + t.Errorf("stdout = %q, want it to report there is nothing to do", stdout) + } +} + +func TestUpdateNeedsAWorkspace(t *testing.T) { + _, stderr, code := run(t, "update", "--root", t.TempDir()) + if code != ExitError { + t.Fatalf("exit = %d, want %d", code, ExitError) + } + if !strings.Contains(stderr, "not an scc workspace") { + t.Errorf("stderr = %q", stderr) + } +} + +// The summary is the point of the command: what would be created, replaced, and +// deleted, before anything is written. +func TestUpdateReportsThePlanAndWritesNothingOnADryRun(t *testing.T) { + root := initWorkspace(t) + missing := ".claude/skills/adr/SKILL.md" + if err := os.Remove(filepath.Join(root, filepath.FromSlash(missing))); err != nil { + t.Fatalf("Remove: %v", err) + } + edited := ".claude/rules/tasks.md" + mine := "# mine\n" + if err := workspace.AtomicWrite(filepath.Join(root, filepath.FromSlash(edited)), []byte(mine), 0o644); err != nil { + t.Fatalf("AtomicWrite: %v", err) + } + + stdout, stderr, code := run(t, "update", "--root", root, "--dry-run") + if code != ExitOK { + t.Fatalf("exit = %d (%s)", code, stderr) + } + for _, want := range []string{missing, edited, "create", "you edited these", "dry run"} { + if !strings.Contains(stdout+stderr, want) { + t.Errorf("the report does not mention %q:\n%s%s", want, stdout, stderr) + } + } + if _, err := os.Stat(filepath.Join(root, filepath.FromSlash(missing))); !os.IsNotExist(err) { + t.Error("--dry-run wrote a file") + } + if got, _ := os.ReadFile(filepath.Join(root, filepath.FromSlash(edited))); string(got) != mine { + t.Error("--dry-run touched an edited file") + } +} + +// Without a terminal there is nobody to confirm, so an unattended run refuses +// rather than applying silently — the confirmation is the feature. +func TestUpdateWithoutATerminalRequiresYes(t *testing.T) { + withoutTerminal(t) + root := initWorkspace(t) + rel := ".claude/rules/specs.md" + if err := os.Remove(filepath.Join(root, filepath.FromSlash(rel))); err != nil { + t.Fatalf("Remove: %v", err) + } + _, stderr, code := run(t, "update", "--root", root) + if code != ExitError { + t.Fatalf("exit = %d, want %d", code, ExitError) + } + if !strings.Contains(stderr, "--yes") { + t.Errorf("stderr = %q, want it to name --yes", stderr) + } + if _, err := os.Stat(filepath.Join(root, filepath.FromSlash(rel))); !os.IsNotExist(err) { + t.Error("the refused update still wrote the file") + } +} + +func TestUpdateWithYesRestoresAndRecords(t *testing.T) { + root := initWorkspace(t) + rel := ".claude/rules/specs.md" + if err := os.Remove(filepath.Join(root, filepath.FromSlash(rel))); err != nil { + t.Fatalf("Remove: %v", err) + } + stdout, stderr, code := run(t, "update", "--root", root, "--yes") + if code != ExitOK { + t.Fatalf("exit = %d (%s)", code, stderr) + } + if !strings.Contains(stdout, rel) { + t.Errorf("stdout does not name the restored file: %q", stdout) + } + if _, err := os.Stat(filepath.Join(root, filepath.FromSlash(rel))); err != nil { + t.Errorf("%s was not restored: %v", rel, err) + } + m, found, err := manifest.Load(root, paths.Claude) + if err != nil || !found { + t.Fatalf("Load = (%v, %v)", found, err) + } + if _, ok := m.Get(rel); !ok { + t.Error("the restored file is not in the manifest") + } +} + +// An edited file survives an update and is named in the report, so the user finds +// out rather than discovering it in a diff later. +func TestUpdateKeepsEditedFilesAndSaysSo(t *testing.T) { + root := initWorkspace(t) + edited := ".claude/rules/delivery.md" + mine := "# mine\n" + if err := workspace.AtomicWrite(filepath.Join(root, filepath.FromSlash(edited)), []byte(mine), 0o644); err != nil { + t.Fatalf("AtomicWrite: %v", err) + } + // Something else has to be applicable, or the command correctly finds nothing + // to do and never reaches the report. + missing := ".claude/rules/specs.md" + if err := os.Remove(filepath.Join(root, filepath.FromSlash(missing))); err != nil { + t.Fatalf("Remove: %v", err) + } + + stdout, stderr, code := run(t, "update", "--root", root, "--yes") + if code != ExitOK { + t.Fatalf("exit = %d (%s)", code, stderr) + } + if got, _ := os.ReadFile(filepath.Join(root, filepath.FromSlash(edited))); string(got) != mine { + t.Error("update overwrote an edited file without --force") + } + if !strings.Contains(stderr, edited) || !strings.Contains(stderr, "--force") { + t.Errorf("the run does not report the kept file and how to take the new one:\n%s%s", stdout, stderr) + } +} + +// The confirmation is the feature: shown the plan, a person who says no gets a +// workspace nothing happened to. +func TestUpdateAsksBeforeWritingAndHonorsNo(t *testing.T) { + for answer, wantRestored := range map[string]bool{"n\n": false, "y\n": true, "\n": false} { + withPrompt(t, answer) + root := initWorkspace(t) + rel := ".claude/rules/specs.md" + if err := os.Remove(filepath.Join(root, filepath.FromSlash(rel))); err != nil { + t.Fatalf("Remove: %v", err) + } + stdout, stderr, code := run(t, "update", "--root", root) + if code != ExitOK { + t.Fatalf("%q: exit = %d (%s)", answer, code, stderr) + } + if !strings.Contains(stdout, "Apply these changes?") { + t.Errorf("%q: update did not ask:\n%s", answer, stdout) + } + _, err := os.Stat(filepath.Join(root, filepath.FromSlash(rel))) + if restored := err == nil; restored != wantRestored { + t.Errorf("%q: restored = %v, want %v", answer, restored, wantRestored) + } + } +} + +func TestUpdateJSONDryRunEmitsThePlan(t *testing.T) { + root := initWorkspace(t) + stdout, stderr, code := run(t, "update", "--root", root, "--dry-run", "--json") + if code != ExitOK { + t.Fatalf("exit = %d (%s)", code, stderr) + } + var doc struct { + Plans []struct { + Harness string `json:"harness"` + Items []struct { + Path string `json:"path"` + Action string `json:"action"` + } `json:"items"` + } `json:"plans"` + } + if err := json.Unmarshal([]byte(stdout), &doc); err != nil { + t.Fatalf("stdout is not valid JSON (%v): %q", err, stdout) + } + if len(doc.Plans) != 1 || doc.Plans[0].Harness != paths.Claude.ID { + t.Fatalf("plans = %+v, want one for claude", doc.Plans) + } + if len(doc.Plans[0].Items) == 0 { + t.Error("the plan has no items") + } +} + +// A workspace with two trees updates both by default; a flag narrows it, and +// naming a harness the workspace does not have is an error rather than a no-op +// that reads like success. +func TestUpdateTargetsEveryInitializedHarness(t *testing.T) { + root := t.TempDir() + for _, h := range []paths.Harness{paths.Claude, paths.OpenCode} { + if _, stderr, code := run(t, "init", "--"+h.ID, "--root", root); code != ExitOK { + t.Fatalf("init --%s: %d (%s)", h.ID, code, stderr) + } + } + for _, rel := range []string{".claude/rules/specs.md", ".opencode/rules/specs.md"} { + if err := os.Remove(filepath.Join(root, filepath.FromSlash(rel))); err != nil { + t.Fatalf("Remove: %v", err) + } + } + stdout, stderr, code := run(t, "update", "--root", root, "--yes") + if code != ExitOK { + t.Fatalf("exit = %d (%s)", code, stderr) + } + for _, rel := range []string{".claude/rules/specs.md", ".opencode/rules/specs.md"} { + if _, err := os.Stat(filepath.Join(root, filepath.FromSlash(rel))); err != nil { + t.Errorf("%s was not restored: %v (%s)", rel, err, stdout) + } + } + + _, stderr, code = run(t, "update", "--root", root, "--codex", "--yes") + if code != ExitError { + t.Fatalf("exit = %d for a harness the workspace does not have, want %d", code, ExitError) + } + if !strings.Contains(stderr, "init --codex") { + t.Errorf("stderr = %q, want it to say how to create that tree", stderr) + } +} diff --git a/internal/scaffold/update.go b/internal/scaffold/update.go new file mode 100644 index 0000000..9cbfc35 --- /dev/null +++ b/internal/scaffold/update.go @@ -0,0 +1,304 @@ +package scaffold + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + + "github.com/protonspy/spec-claude-code/internal/assets" + "github.com/protonspy/spec-claude-code/internal/manifest" + "github.com/protonspy/spec-claude-code/internal/paths" + "github.com/protonspy/spec-claude-code/internal/workspace" +) + +// UpdateAction is what an update would do to one managed file. +// +// The set is deliberately larger than "write / don't write": the difference +// between a file scc may replace and one the user has authored into is the whole +// product decision here, and a plan that collapsed them would be a plan the user +// cannot review before agreeing to it. +type UpdateAction string + +const ( + // UpCurrent means the file on disk already is what this version renders. + UpCurrent UpdateAction = "current" + // UpCreate means the template set ships it and the workspace does not have + // it — either it is new in this version, or somebody deleted it. + UpCreate UpdateAction = "create" + // UpUpdate means the file is still exactly what scc last wrote, and scc now + // writes something different. Replacing it loses nothing. + UpUpdate UpdateAction = "update" + // UpConflict means the file differs from what scc recorded — the user edited + // it, or it arrived from somewhere scc has no record of — and the template + // also moved. Only --force overwrites it. + UpConflict UpdateAction = "conflict" + // UpOwned means the file is one the user owns from their first edit (the + // entry file, the project rule). A newer version exists and scc says so, but + // it never merges into somebody's own prose. + UpOwned UpdateAction = "owned" + // UpDelete means the manifest carries it, this version no longer ships it, + // and it is still pristine — so removing it destroys nothing. + UpDelete UpdateAction = "delete" + // UpOrphan means the same, except the file was edited. The entry is dropped + // and the file stays, now the user's. + UpOrphan UpdateAction = "orphan" +) + +// UpdateItem is one file's place in the plan. +type UpdateItem struct { + Path string `json:"path"` + Action UpdateAction `json:"action"` + // From is the template version the manifest recorded, when it had one. + From string `json:"from,omitempty"` + // To is the template version that would apply. + To string `json:"to,omitempty"` +} + +// UpdatePlan is what an update would do to one harness's tree, computed without +// writing anything. Every command that changes a workspace reports before it +// acts; this is the type that makes "report, then ask, then act" possible rather +// than aspirational. +type UpdatePlan struct { + Root string `json:"root"` + Harness string `json:"harness"` + Items []UpdateItem `json:"items"` +} + +// PlanUpdate compares the workspace against what this build renders for h. +// +// It reads and hashes; it writes nothing, so it is safe to run for a report and +// then throw away. +func PlanUpdate(root string, h paths.Harness) (*UpdatePlan, error) { + prior, found, err := manifest.Load(root, h) + if err != nil { + return nil, err + } + if !found { + return nil, fmt.Errorf("%s is not an scc workspace for %s: run `init --%s` first", root, h.Label, h.ID) + } + plan := &UpdatePlan{Root: root, Harness: h.ID} + + managed := map[string]bool{} + for _, f := range assets.Workspace(h) { + managed[f.Rel] = true + want, err := assets.Render(h, f) + if err != nil { + return nil, err + } + item, err := planOne(root, f, want, prior) + if err != nil { + return nil, fmt.Errorf("%s: %w", f.Rel, err) + } + plan.Items = append(plan.Items, item) + } + + // Entries this version no longer ships. The manifest is exactly the set of + // files scc manages, so a stale entry would send the next update looking for a + // template that does not exist. + for _, e := range prior.Files { + if managed[e.Path] { + continue + } + action := UpDelete + got, err := manifest.HashFile(filepath.Join(root, filepath.FromSlash(e.Path))) + switch { + case errors.Is(err, fs.ErrNotExist): + // Already gone: nothing to remove but the entry. + case err != nil: + return nil, fmt.Errorf("%s: %w", e.Path, err) + case got != e.Hash: + action = UpOrphan + } + plan.Items = append(plan.Items, UpdateItem{Path: e.Path, Action: action, From: e.Version}) + } + return plan, nil +} + +func planOne(root string, f assets.File, want string, prior *manifest.Manifest) (UpdateItem, error) { + item := UpdateItem{Path: f.Rel, To: assets.Version} + if e, ok := prior.Get(f.Rel); ok { + item.From = e.Version + } + + got, err := manifest.HashFile(filepath.Join(root, filepath.FromSlash(f.Rel))) + if errors.Is(err, fs.ErrNotExist) { + item.Action = UpCreate + return item, nil + } + if err != nil { + return item, err + } + if got == manifest.Hash(want) { + item.Action = UpCurrent + return item, nil + } + // From here the file differs from what this version renders. Who last wrote it + // decides what may be done about it. + if f.Owned { + item.Action = UpOwned + return item, nil + } + if e, ok := prior.Get(f.Rel); ok && got == e.Hash { + item.Action = UpUpdate + return item, nil + } + item.Action = UpConflict + return item, nil +} + +// Count returns how many items carry an action. +func (p *UpdatePlan) Count(a UpdateAction) int { + n := 0 + for _, it := range p.Items { + if it.Action == a { + n++ + } + } + return n +} + +// Pending lists the items that are not already current, in plan order — what a +// user is actually being asked to agree to. +func (p *UpdatePlan) Pending() []UpdateItem { + var out []UpdateItem + for _, it := range p.Items { + if it.Action != UpCurrent { + out = append(out, it) + } + } + return out +} + +// Writes reports whether applying the plan would change anything — on disk or in +// the manifest — given whether the caller is forcing edited files. A plan whose +// only items are conflicts and owned files is a plan with nothing to do until the +// user decides something. +// +// An orphan counts: nothing is written to the file, but it stops being managed, +// and a stale entry left in the manifest would send the next update looking for a +// template that no longer exists. +func (p *UpdatePlan) Writes(force bool) bool { + for _, it := range p.Items { + switch it.Action { + case UpCreate, UpUpdate, UpDelete, UpOrphan: + return true + case UpConflict: + if force { + return true + } + } + } + return false +} + +// UpdateOptions are the knobs of applying a plan. +type UpdateOptions struct { + // SCCVersion is stamped into the manifest as a diagnostic. + SCCVersion string + // Force also overwrites the files reported as conflicts — the ones scc can + // see somebody edited. It is a separate decision from agreeing to the plan, + // and the CLI asks for it separately, because it is the only part of an + // update that destroys work. + Force bool +} + +// UpdateResult is what applying the plan actually did. +type UpdateResult struct { + Root string `json:"root"` + Harness string `json:"harness"` + Applied []UpdateItem `json:"applied"` + Kept []UpdateItem `json:"kept"` + ManifestWritten bool `json:"manifestWritten"` +} + +// ApplyUpdate carries out a plan and rewrites the manifest to match what is now +// on disk. +// +// The manifest is written last and only when it changed, for the same reasons +// Apply does it: a crash midway leaves a workspace whose marker still describes a +// state that really existed, and an update that changed nothing does not touch +// the file's mtime. +func ApplyUpdate(root string, h paths.Harness, plan *UpdatePlan, opts UpdateOptions) (*UpdateResult, error) { + prior, found, err := manifest.Load(root, h) + if err != nil { + return nil, err + } + if !found { + return nil, fmt.Errorf("%s is not an scc workspace for %s", root, h.Label) + } + res := &UpdateResult{Root: root, Harness: h.ID} + next := manifest.New(opts.SCCVersion, h) + + rendered := map[string]string{} + for _, f := range assets.Workspace(h) { + want, err := assets.Render(h, f) + if err != nil { + return nil, err + } + rendered[f.Rel] = want + } + + for _, it := range plan.Items { + dest := filepath.Join(root, filepath.FromSlash(it.Path)) + want, shipped := rendered[it.Path] + switch it.Action { + case UpCreate, UpUpdate: + if err := workspace.AtomicWrite(dest, []byte(want), 0o644); err != nil { + return nil, fmt.Errorf("%s: %w", it.Path, err) + } + res.Applied = append(res.Applied, it) + next.Set(it.Path, manifest.Hash(want), assets.Version) + case UpConflict: + if !opts.Force { + // Left exactly as the user has it, and still tracked at the version + // scc last wrote — which is the base revision a later merge needs. + res.Kept = append(res.Kept, it) + carry(next, prior, it.Path) + continue + } + if err := workspace.AtomicWrite(dest, []byte(want), 0o644); err != nil { + return nil, fmt.Errorf("%s: %w", it.Path, err) + } + res.Applied = append(res.Applied, it) + next.Set(it.Path, manifest.Hash(want), assets.Version) + case UpOwned: + res.Kept = append(res.Kept, it) + carry(next, prior, it.Path) + case UpDelete: + if err := os.Remove(dest); err != nil && !errors.Is(err, fs.ErrNotExist) { + return nil, fmt.Errorf("%s: %w", it.Path, err) + } + res.Applied = append(res.Applied, it) + case UpOrphan: + // The file stays and stops being managed: it is the user's now. + res.Applied = append(res.Applied, it) + case UpCurrent: + if shipped { + next.Set(it.Path, manifest.Hash(want), assets.Version) + } + } + } + + same, err := manifestUnchanged(root, h, next) + if err != nil { + return nil, err + } + if !same { + if err := manifest.Save(root, h, next); err != nil { + return nil, err + } + res.ManifestWritten = true + } + return res, nil +} + +// carry keeps a file's existing entry — hash and version both — so a file scc did +// not rewrite is still recorded as what it actually is. Claiming today's version +// for content scc never wrote would destroy the merge base an upgrade needs. +func carry(next, prior *manifest.Manifest, rel string) { + if e, ok := prior.Get(rel); ok { + next.Set(rel, e.Hash, e.Version) + } +} diff --git a/internal/scaffold/update_test.go b/internal/scaffold/update_test.go new file mode 100644 index 0000000..22d9d7c --- /dev/null +++ b/internal/scaffold/update_test.go @@ -0,0 +1,245 @@ +package scaffold + +import ( + "os" + "path/filepath" + "testing" + + "github.com/protonspy/spec-claude-code/internal/assets" + "github.com/protonspy/spec-claude-code/internal/manifest" + "github.com/protonspy/spec-claude-code/internal/paths" + "github.com/protonspy/spec-claude-code/internal/workspace" +) + +func planFor(t *testing.T, root string, h paths.Harness) *UpdatePlan { + t.Helper() + plan, err := PlanUpdate(root, h) + if err != nil { + t.Fatalf("PlanUpdate: %v", err) + } + return plan +} + +func action(t *testing.T, plan *UpdatePlan, rel string) UpdateAction { + t.Helper() + for _, it := range plan.Items { + if it.Path == rel { + return it.Action + } + } + t.Fatalf("no plan item for %s", rel) + return "" +} + +// write puts content at rel and, when recorded is true, tells the manifest that +// is what scc last wrote — the difference between a stale file scc may replace +// and one the user edited. +func writeManaged(t *testing.T, root string, h paths.Harness, rel, content string, recorded bool) { + t.Helper() + if err := workspace.AtomicWrite(filepath.Join(root, filepath.FromSlash(rel)), []byte(content), 0o644); err != nil { + t.Fatalf("AtomicWrite: %v", err) + } + if !recorded { + return + } + m, _, err := manifest.Load(root, h) + if err != nil { + t.Fatalf("Load: %v", err) + } + m.Set(rel, manifest.Hash(content), "1") + if err := manifest.Save(root, h, m); err != nil { + t.Fatalf("Save: %v", err) + } +} + +// A workspace scaffolded by this build is already current, in every harness. If +// this fails, `scc update` would offer to rewrite files it just wrote. +func TestPlanOnAFreshWorkspaceIsAllCurrent(t *testing.T) { + for _, h := range paths.Harnesses() { + root := t.TempDir() + applyTo(t, root, h, false) + plan := planFor(t, root, h) + if got := plan.Pending(); len(got) != 0 { + t.Errorf("%s: fresh workspace has pending items: %+v", h.ID, got) + } + if plan.Writes(true) { + t.Errorf("%s: fresh workspace would be written to", h.ID) + } + } +} + +func TestPlanUpdateNeedsAWorkspace(t *testing.T) { + if _, err := PlanUpdate(t.TempDir(), paths.Claude); err == nil { + t.Error("PlanUpdate on a bare directory returned no error") + } +} + +// The four states an update has to tell apart, and the reason the plan is worth +// showing before anything is written. +func TestPlanNamesEveryState(t *testing.T) { + root := t.TempDir() + applyTo(t, root, paths.Claude, false) + + deleted := ".claude/rules/specs.md" + if err := os.Remove(filepath.Join(root, filepath.FromSlash(deleted))); err != nil { + t.Fatalf("Remove: %v", err) + } + // Stale but pristine: the content scc recorded, which is not what it renders now. + stale := ".claude/rules/routing.md" + writeManaged(t, root, paths.Claude, stale, "# an older template\n", true) + // Edited: differs from both the render and the record. + edited := ".claude/rules/tasks.md" + writeManaged(t, root, paths.Claude, edited, "# mine\n", false) + // Owned, and changed by the user — the file whose whole purpose is to be edited. + owned := paths.Claude.EntryFile + writeManaged(t, root, paths.Claude, owned, "# my own entry file\n", false) + // Managed once, not shipped now, still exactly as scc left it. + gone := ".claude/rules/retired.md" + writeManaged(t, root, paths.Claude, gone, "# retired\n", true) + // Same, but the user changed it, so it is theirs to keep. + goneEdited := ".claude/rules/retired-edited.md" + writeManaged(t, root, paths.Claude, goneEdited, "# retired\n", true) + writeManaged(t, root, paths.Claude, goneEdited, "# and then edited\n", false) + + plan := planFor(t, root, paths.Claude) + want := map[string]UpdateAction{ + deleted: UpCreate, + stale: UpUpdate, + edited: UpConflict, + owned: UpOwned, + gone: UpDelete, + goneEdited: UpOrphan, + } + for rel, w := range want { + if got := action(t, plan, rel); got != w { + t.Errorf("%s: action = %q, want %q", rel, got, w) + } + } + if !plan.Writes(false) { + t.Error("a plan with a create and an update reports no writes") + } +} + +// Applying it: everything scc may touch is brought current, the manifest ends up +// describing what is actually on disk, and a second run has nothing to do. +func TestApplyUpdateBringsTheTreeCurrent(t *testing.T) { + root := t.TempDir() + applyTo(t, root, paths.OpenCode, false) + rel := ".opencode/rules/routing.md" + writeManaged(t, root, paths.OpenCode, rel, "# an older template\n", true) + missing := ".opencode/skills/wiki/SKILL.md" + if err := os.Remove(filepath.Join(root, filepath.FromSlash(missing))); err != nil { + t.Fatalf("Remove: %v", err) + } + + plan := planFor(t, root, paths.OpenCode) + res, err := ApplyUpdate(root, paths.OpenCode, plan, UpdateOptions{SCCVersion: "v0.0.0-test"}) + if err != nil { + t.Fatalf("ApplyUpdate: %v", err) + } + if len(res.Applied) != 2 { + t.Errorf("applied = %+v, want the stale file and the missing one", res.Applied) + } + for _, r := range []string{rel, missing} { + want := renderRelFor(t, paths.OpenCode, r) + if got := read(t, root, r); got != want { + t.Errorf("%s was not brought current", r) + } + } + m, _, err := manifest.Load(root, paths.OpenCode) + if err != nil { + t.Fatalf("Load: %v", err) + } + for _, e := range m.Files { + if e.Version != assets.Version { + t.Errorf("%s recorded version %q, want %q", e.Path, e.Version, assets.Version) + } + } + if got := planFor(t, root, paths.OpenCode); len(got.Pending()) != 0 { + t.Errorf("a second update still has work: %+v", got.Pending()) + } +} + +// The product's first rule: never author over what the user owns. An edited file +// survives an update, and keeps the version it was recorded at — that recorded +// version is the base a later merge would need, and claiming today's would +// describe content scc never wrote. +func TestApplyUpdateKeepsEditedFilesUnlessForced(t *testing.T) { + root := t.TempDir() + applyTo(t, root, paths.Claude, false) + edited := ".claude/rules/verification.md" + mine := "# mine, and I meant it\n" + writeManaged(t, root, paths.Claude, edited, mine, false) + entry := paths.Claude.EntryFile + myEntry := "# my own entry file\n" + writeManaged(t, root, paths.Claude, entry, myEntry, false) + + res, err := ApplyUpdate(root, paths.Claude, planFor(t, root, paths.Claude), UpdateOptions{SCCVersion: "v0.0.0-test"}) + if err != nil { + t.Fatalf("ApplyUpdate: %v", err) + } + if got := read(t, root, edited); got != mine { + t.Error("an update overwrote an edited file without --force") + } + if got := read(t, root, entry); got != myEntry { + t.Error("an update overwrote a user-owned file") + } + if len(res.Kept) != 2 { + t.Errorf("kept = %+v, want the edited file and the owned one", res.Kept) + } + m, _, err := manifest.Load(root, paths.Claude) + if err != nil { + t.Fatalf("Load: %v", err) + } + if e, ok := m.Get(edited); !ok || e.Hash == manifest.Hash(mine) { + t.Errorf("the kept file's entry was rewritten to its edited content: %+v", e) + } + + // --force is the separate, explicit decision — the only one that destroys work. + forced, err := ApplyUpdate(root, paths.Claude, planFor(t, root, paths.Claude), UpdateOptions{ + SCCVersion: "v0.0.0-test", Force: true, + }) + if err != nil { + t.Fatalf("ApplyUpdate --force: %v", err) + } + if got := read(t, root, edited); got != renderRelFor(t, paths.Claude, edited) { + t.Error("--force did not take the new template") + } + if got := read(t, root, entry); got != myEntry { + t.Error("--force overwrote a user-owned file, which it must never do") + } + if len(forced.Applied) != 1 { + t.Errorf("applied = %+v, want only the forced file", forced.Applied) + } +} + +// A file this version no longer ships is removed when it is still pristine, and +// kept — but untracked — when the user changed it. +func TestApplyUpdateDropsUnmanagedFiles(t *testing.T) { + root := t.TempDir() + applyTo(t, root, paths.Claude, false) + gone := ".claude/rules/retired.md" + writeManaged(t, root, paths.Claude, gone, "# retired\n", true) + kept := ".claude/rules/retired-edited.md" + writeManaged(t, root, paths.Claude, kept, "# retired\n", true) + writeManaged(t, root, paths.Claude, kept, "# and then edited\n", false) + + if _, err := ApplyUpdate(root, paths.Claude, planFor(t, root, paths.Claude), UpdateOptions{SCCVersion: "v0.0.0-test"}); err != nil { + t.Fatalf("ApplyUpdate: %v", err) + } + if _, err := os.Stat(filepath.Join(root, filepath.FromSlash(gone))); !os.IsNotExist(err) { + t.Error("a pristine file scc no longer ships was left behind") + } + if got := read(t, root, kept); got != "# and then edited\n" { + t.Error("an edited file scc no longer ships was deleted") + } + m, _, err := manifest.Load(root, paths.Claude) + if err != nil { + t.Fatalf("Load: %v", err) + } + for _, rel := range []string{gone, kept} { + if _, ok := m.Get(rel); ok { + t.Errorf("%s is still in the manifest", rel) + } + } +} From d13595b745adcc5b2326572bc7528b2790b7403c Mon Sep 17 00:00:00 2001 From: prode Date: Sat, 1 Aug 2026 20:32:51 -0300 Subject: [PATCH 3/5] docs: record the three-harness layout and what `scc update` actually shipped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit orchestration.md §6 gains the layout table and the two things that fall out of it: Codex ships no slash commands, because its custom prompts are user-global and deprecated in favor of skills, and a repo initialized for both Codex and opencode has one AGENTS.md written by whichever ran first. §12 keeps the three-way merge as the destination and states plainly what shipped instead — replace-or-keep, with the plan confirmed first — and why the conflicted file's recorded version is preserved rather than stamped forward. The "No AGENTS.md" decision is gone with its reason: it argued against buying portability to tools this product isn't for, and the product is now for them. plan.md's phase 11 note is rewritten from "deliberately not started" to what is built and what is not, and CLAUDE.md and the README follow the code: the marker is per harness, paths are methods on paths.Harness, and adding a harness is one value in paths.Harnesses(). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019yCM51izVEvKywpKDgYvNM --- CLAUDE.md | 33 +++++---- README.md | 47 +++++++++---- design/orchestration.md | 150 +++++++++++++++++++++++++++++++--------- design/plan.md | 39 +++++++++-- 4 files changed, 206 insertions(+), 63 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 24b1652..0d57d8f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -4,15 +4,20 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co ## What this is -`scc` is a single Go binary (`github.com/protonspy/spec-claude-code`) that enforces Spec-Driven Development inside a Claude Code repo. One surface: a headless CLI, so an AI agent or a CI job drives it exactly as well as a human does. +`scc` is a single Go binary (`github.com/protonspy/spec-claude-code`) that enforces Spec-Driven Development inside an agent-driven repo — Claude Code, Codex, or opencode. One surface: a headless CLI, so an AI agent or a CI job drives it exactly as well as a human does. -The artifacts it governs (`.claude/`, `specs/`, `docs/`) are plain Markdown/JSON. There is no server and no database — the files *are* the API. scc has no directory of its own: its state lives in one file under `.claude/`, alongside what Claude Code already keeps there. +The artifacts it governs (the harness's own directory, `specs/`, `docs/`) are plain Markdown/JSON. There is no server and no database — the files *are* the API. scc has no directory of its own: its state lives in one file under the harness's directory, alongside what that tool already keeps there. -This file covers working *on* scc. The product's own rules and methodology are not documented here: they live in `design/` while being designed, and ship as templates the binary scaffolds into `.claude/rules/`. +This file covers working *on* scc. The product's own rules and methodology are not documented here: they live in `design/` while being designed, and ship as templates the binary scaffolds into `/rules/`. -Note: this repo is not itself an scc workspace (no `.claude/`, `specs/`, or `docs/` are committed) — those trees only exist in workspaces the binary scaffolds, and in test temp dirs. +Note: this repo is not itself an scc workspace (no harness directory, `specs/`, or `docs/` are committed) — those trees only exist in workspaces the binary scaffolds, and in test temp dirs. -**Status: v0.3.0-shaped.** Everything through `design/plan.md` phase 10 is built and green: scaffolding (`init`), artifact creation (`spec`, `plan`), and all eight validators behind `scc validate`. `init` also scaffolds the six knowledge-base skills named in `design/orchestration.md` §6 — one per `docs/` artifact a validator checks, plus `prd` — each with a `scc-`-prefixed slash command derived from the same list (`assets.KnowledgeSkills`). `scc update` (phase 11) is not started, deliberately — a three-way merge is untestable until a second template version exists, so until then the upgrade story is "re-run `init`; it never overwrites what you edited". +**Status: v0.4.0-shaped.** Everything through `design/plan.md` phase 10 is built and green: scaffolding (`init`), artifact creation (`spec`, `plan`), and all eight validators behind `scc validate`. `init` also scaffolds the six knowledge-base skills named in `design/orchestration.md` §6 — one per `docs/` artifact a validator checks, plus `prd` — each with a `scc-`-prefixed slash command derived from the same list (`assets.KnowledgeSkills`), wherever the harness has a command surface. + +Two things landed after phase 10 and both are documented in `design/orchestration.md` §6 and §12: + +- **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. +- **`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` is a redesign of `csdd` (`github.com/protonspy/csdd`), narrowed to spec-driven development and deliberately leaner. When reaching for something from there, port the *decision*, not the file. Already decided against: a TUI, an embedded web dashboard, an MCP server, a devcontainer. @@ -55,21 +60,21 @@ cmd/scc/main.go os.Exit(cli.Run(os.Args[1:])) | paths · workspace · render · textutil · finding | - plain files on disk: .claude/ · specs/ · plans/ · docs/ · CLAUDE.md + plain files on disk: / · specs/ · plans/ · docs/ · CLAUDE.md|AGENTS.md ``` `internal/cli/cli.go` is the whole dispatcher: `Run(args)` switches on `args[0]` and hands off to `run` in a file named for that resource. Each handler owns its own `flag.FlagSet`. Adding a subcommand means adding a case there plus one file — nothing is registered dynamically, so the command set is readable in one place. | Package | Role | |---|---| -| `internal/paths` | Every directory/file name in the on-disk layout, in one place. Never hardcode `".claude"` or `"specs"` elsewhere. | -| `internal/workspace` | Resolves the root by walking up for the `.claude/scc-manifest.json` marker; owns `KebabCheck`, `SafeName`, `AtomicWrite`. Knows nothing about specs or wikis. | +| `internal/paths` | Every directory/file name in the on-disk layout, in one place, plus the `Harness` profile (`Claude`, `Codex`, `OpenCode`) that says where each tool keeps things. Never hardcode `".claude"` or `"specs"` elsewhere — the harness-relative paths are methods on `Harness`. | +| `internal/workspace` | Resolves the root by walking up for *any* harness's `scc-manifest.json` marker; `Harnesses(root)` says which trees exist. Owns `KebabCheck`, `SafeName`, `AtomicWrite`. Knows nothing about specs or wikis. | | `internal/render` | CLI terminal output (`✓ ✗ ! •`, `NO_COLOR`/TTY aware), split across stdout/stderr. | | `internal/textutil` | Line-ending and BOM normalization, in exactly one place. | | `internal/finding` | One finding type and one frozen JSON shape (`{findings, count}`) for every validator, plus the grouped human report. | -| `internal/manifest` | `.claude/scc-manifest.json`: `{path, hash, version}` per managed file, deterministic serialization, `Status → pristine\|edited\|missing`. Unknown fields are preserved. | -| `internal/assets` | The embedded template set — rules, review agents, skills, slash commands, artifact templates. **Workspace templates are data-free** (that is what makes a future three-way merge possible); **artifact templates take data** (`spec new` renders them and the user owns the result). `Version` is the template-set version and must be bumped whenever a workspace template changes. | -| `internal/scaffold` | Applies the template set to a root. Idempotent, never overwrites, manifest written last. | +| `internal/manifest` | `/scc-manifest.json`: `{path, hash, version}` per managed file plus the harness, deterministic serialization, `Status → pristine\|edited\|missing`. Unknown fields are preserved. Every call takes the `paths.Harness` whose manifest it means. | +| `internal/assets` | The embedded template set — rules, review agents, skills, slash commands, artifact templates. **Workspace templates are data-free except for the harness profile** (a `(version, harness)` pair still renders byte-identically everywhere, and the manifest records both, so the future three-way merge can still reconstruct the old side); **artifact templates take data** (`spec new` renders them and the user owns the result). `Render(h, file)` is the only way to get a workspace file's bytes: it expands paths and synthesizes the per-harness header for agents and commands. `Version` is the template-set version and must be bumped whenever a workspace template changes. | +| `internal/scaffold` | Applies the template set to a root (`Apply`) and brings an existing one current (`PlanUpdate`/`ApplyUpdate`). Idempotent, never overwrites without being told to, manifest written last. | | `internal/mdscan` | The only Markdown parser: fence- and HTML-comment-aware headings, checkboxes, links, wikilinks, slugs, plus a small frontmatter reader. `Body` is the comment/fence-stripped text every validator applies its grammar to. | | `internal/ears` | EARS requirement parsing, all five patterns plus complex. | | `internal/validate` | The eight validators, one file each, sharing `mdscan` and `finding`. | @@ -89,9 +94,11 @@ cmd/scc/main.go os.Exit(cli.Run(os.Args[1:])) **Writes are atomic.** Use `workspace.AtomicWrite` for anything a concurrent reader might see. -**The marker is the file `.claude/scc-manifest.json`, never the `.claude/` directory.** Two reasons, and both are load-bearing: `~/.claude` is Claude Code's own global config directory and exists on every machine that runs it, so an upward walk accepting the *directory* would resolve the root to `$HOME` for any command run outside a workspace — every command would then read and write the user's global configuration. And `.claude/` exists in every repo that merely *uses* Claude Code, where scc was never initialized. `workspace.Find` therefore stats a regular file. +**The marker is the file `/scc-manifest.json`, never the harness directory.** Two reasons, and both are load-bearing: every harness has a global twin in the user's home (`~/.claude`, `~/.codex`, `~/.config/opencode`) that exists on any machine running that tool, so an upward walk accepting the *directory* would resolve the root to `$HOME` for any command run outside a workspace — every command would then read and write the user's global configuration. And those directories exist in every repo that merely *uses* the tool, where scc was never initialized. `workspace.Find` therefore stats a regular file, for each harness in turn. + +**scc has exactly one file per harness and no config file.** The manifest is it — content hashes, doubling as the marker. scc runs no tests and no linters, so it has nothing to configure; a project's test and lint commands are a rule under `/rules/`, which is Markdown the orchestrator already reads. Resist adding `scc.json`: a JSON schema to version, read by nothing inside the binary, is dead weight. -**scc has exactly one file and no config file.** The manifest is it — content hashes, doubling as the marker. scc runs no tests and no linters, so it has nothing to configure; a project's test and lint commands are a rule under `.claude/rules/`, which is Markdown the orchestrator already reads. Resist adding `scc.json`: a JSON schema to version, read by nothing inside the binary, is dead weight. +**Adding a harness touches one place.** A new `paths.Harness` value plus its entry in `paths.Harnesses()` is the whole change — `init`'s flags, the picker, `update`'s targets, the skills validator's search path, and the workspace walk all derive from that list. If a change needs a `switch h.ID` outside `assets.renderAgent`/`renderCommand`, the profile is missing a field. **Never author what the user owns.** Upgrades preserve user-edited files rather than overwriting them; the manifest of content hashes is what makes a pristine file distinguishable from an edited one. diff --git a/README.md b/README.md index cf2dcce..65c0ce0 100644 --- a/README.md +++ b/README.md @@ -1,23 +1,25 @@ # scc -Spec-driven development for Claude Code — a single Go binary that turns the SDD -workflow into a mechanically validated contract for humans and AI agents. +Spec-driven development for Claude Code, Codex, and opencode — a single Go binary +that turns the SDD workflow into a mechanically validated contract for humans and +AI agents. -> **Status: pre-1.0.** Scaffolding, artifact creation, and validation all work. -> `scc update` — bringing an existing workspace onto improved templates through a -> three-way merge — is not built yet; until it is, re-running `scc init` is the -> upgrade path. It is additive and never overwrites a file you edited, so it is safe, -> but it also will not deliver improved templates to files that already exist. +> **Status: pre-1.0.** Scaffolding, artifact creation, validation, and updating all +> work. `scc update` currently replaces what is safe to replace and *keeps* anything +> you edited, naming it; the three-way merge that would resolve those in place is +> still ahead. ## Use it No install — run it straight from npm inside the repo you want to govern: ```bash -npx @protonspy/scc init # scaffold .claude/rules/, the review agents, the layout +npx @protonspy/scc init # asks which harness, then scaffolds the rules, agents, and layout +npx @protonspy/scc init --codex # or name it: --claude (default), --codex, --opencode npx @protonspy/scc spec new user-auth # specs/user-auth/: requirements.md, design.md, tasks.md npx @protonspy/scc plan new checkout-revamp # plans/checkout-revamp.md npx @protonspy/scc validate # every check; exit 2 means it found something +npx @protonspy/scc update # show what a newer scc would change, then confirm ``` Installed globally (`npm i -g @protonspy/scc`) the same commands are just `scc init`, @@ -26,11 +28,31 @@ Installed globally (`npm i -g @protonspy/scc`) the same commands are just `scc i | Command | What it does | |---|---| | `init` | Scaffolds the workspace and records what it wrote. Idempotent; never overwrites your edits. | +| `update` | Compares every managed file against this build, shows the plan, and applies it once you agree. | | `spec new\|list\|show\|delete\|validate` | The three-artifact vehicle for work whose *what* and *how* need settling first. | | `plan new\|list\|delete\|validate` | One file, for everything else: a checklist, a decomposition into specs, or both. | | `skill validate` | Conformance to the published [Agent Skills](https://agentskills.io/specification) spec. | | `validate` | Every applicable validator, one exit code, one JSON document. | +### Three harnesses, one methodology + +The same rules, review agents, and knowledge-base skills are scaffolded into +whichever tool you work in — only the paths and the frontmatter dialect change. + +| | Claude Code | Codex | opencode | +|---|---|---|---| +| entry file | `CLAUDE.md` | `AGENTS.md` | `AGENTS.md` | +| rules | `.claude/rules/` | `.codex/rules/` | `.opencode/rules/` | +| review agents | `.claude/agents/*.md` | `.codex/agents/*.toml` | `.opencode/agent/*.md` | +| skills | `.claude/skills/` | `.codex/skills/` | `.opencode/skills/` | +| slash commands | `.claude/commands/` | — (skills are the surface) | `.opencode/command/` | + +`specs/`, `plans/`, and `docs/` are identical everywhere: they are the product, not +the tool. Running `init` twice with different flags gives one repo two managed trees, +and `update` keeps both current. + +## What it checks + What gets checked: EARS grammar across all five patterns, requirement numbering, one methodology annotation per task, traceability in both directions, plan one-source-of-truth, skill conformance, wiki link/orphan graph, ADR numbering and @@ -74,10 +96,11 @@ exactly as well as a human does. | `1` | usage or runtime error | | `2` | the command ran and reported validation findings | -The artifacts it governs are plain Markdown and JSON in your repo — `.claude/`, -`specs/`, `docs/`. No server, no database, no conversion layer: the files *are* the -API, and they are the ones Claude Code already reads. `scc` adds no directory of its -own and no config file — it owns a single manifest inside `.claude/`. +The artifacts it governs are plain Markdown and JSON in your repo — the harness's own +directory, `specs/`, `docs/`. No server, no database, no conversion layer: the files +*are* the API, and they are the ones your harness already reads. `scc` adds no +directory of its own and no config file — it owns a single manifest inside the +harness's directory, which doubles as the workspace marker. ## Development diff --git a/design/orchestration.md b/design/orchestration.md index 0b42640..697eb9c 100644 --- a/design/orchestration.md +++ b/design/orchestration.md @@ -11,22 +11,24 @@ several of them are evidence-backed rather than preferences. ## 0 · What scc ships Same philosophy as csdd: **scc is a template plus a CLI.** The binary scaffolds a -workspace — rules, skills, agents, spec templates — into `.claude/`, `specs/`, and -`docs/`, all of it plain Markdown compiled into the binary via `//go:embed`, and then -validates what lives there. There is no conversion layer and no runtime: the files -Claude Code already reads *are* the API, and the CLI is how they are created, -checked, and upgraded. +workspace — rules, skills, agents, spec templates — into the harness's own +configuration directory plus `specs/` and `docs/`, all of it plain Markdown compiled +into the binary via `//go:embed`, and then validates what lives there. There is no +conversion layer and no runtime: the files the harness already reads *are* the API, +and the CLI is how they are created, checked, and upgraded. scc adds **no directory of its own** and **no config file**. It owns exactly one file, -`.claude/scc-manifest.json`: content hashes, so an upgrade can tell a pristine file -from an edited one — and, by existing at all, the workspace marker. It has nothing to -configure because it runs nothing; the project's test and lint commands (§7) are a rule -under `.claude/rules/`, Markdown the orchestrator already reads. - -The marker is that file rather than `.claude/` itself because `~/.claude` is Claude -Code's global config on every machine — a walk accepting the directory would resolve -any out-of-workspace command's root to `$HOME` — and because `.claude/` also exists in -repos that merely use Claude Code, where scc was never initialized. +`/scc-manifest.json`: content hashes and the template version per managed +file, so an upgrade can tell a pristine file from an edited one — and, by existing at +all, the workspace marker. It has nothing to configure because it runs nothing; the +project's test and lint commands (§7) are a rule under the harness's rules directory, +Markdown the orchestrator already reads. + +The marker is that file rather than the directory itself because every harness has a +global twin in the user's home — `~/.claude`, `~/.codex`, `~/.config/opencode` — so a +walk accepting the directory would resolve any out-of-workspace command's root to +`$HOME`, and because those directories also exist in repos that merely use the tool, +where scc was never initialized. That is why everything below describes both a rule and its artifact. A rule the binary cannot scaffold is documentation; a rule it cannot check is a suggestion. @@ -362,12 +364,12 @@ claimed TDD and check them. ## 6 · Where the rules live -`.claude/rules/` is the home of the rules, as in csdd. The scaffolded `CLAUDE.md` +`/rules/` is the home of the rules, as in csdd. The scaffolded entry file stays small and points at them. -This is a token-budget decision, not organization: every Claude Code session pays -for `CLAUDE.md` in context, so the routing table and the two cycles do not belong -inline there. `.claude/rules/` files are read when the rule is relevant. +This is a token-budget decision, not organization: every session pays for the entry +file in context, so the routing table and the two cycles do not belong inline there. +The rules are read when the rule is relevant. The evidence for keeping `CLAUDE.md` short is **context rot**, not the frequently-cited `AGENTS.md` efficiency study. Chroma's measurement across 18 frontier models found accuracy @@ -377,9 +379,43 @@ sooner on more complex tasks. Length has a cost in correctness, not just in toke of scope; the "three sections, under 150 lines" prescription attributed to it is not in it. Don't cite it for a length rule.) -**No `AGENTS.md`.** Claude Code doesn't read it natively, so shipping one means shipping an -import from `CLAUDE.md` too — a second file and an indirection to buy portability to tools -this product isn't for. Anyone who wants cross-tool context can write it themselves. +### Three harnesses, one template set + +`scc init` scaffolds for Claude Code (the default), Codex, or opencode — one per run, +`--claude` / `--codex` / `--opencode`, and it asks when a person is at the terminal +and passed no flag. What differs between them is entirely *where files go and what +dialect the loader parses*, never what the methodology says: + +| | Claude Code | Codex | opencode | +|---|---|---|---| +| entry file | `CLAUDE.md` | `AGENTS.md` | `AGENTS.md` | +| rules | `.claude/rules/` | `.codex/rules/` | `.opencode/rules/` | +| review agents | `.claude/agents/*.md` | `.codex/agents/*.toml` | `.opencode/agent/*.md` | +| skills | `.claude/skills//SKILL.md` | `.codex/skills//SKILL.md` | `.opencode/skills//SKILL.md` | +| slash commands | `.claude/commands/scc-*.md` | — | `.opencode/command/scc-*.md` | +| manifest | `.claude/scc-manifest.json` | `.codex/scc-manifest.json` | `.opencode/scc-manifest.json` | + +**One prose source, addressed three ways.** The rules, the skills, and the reviewers +are written once; the paths in them come from a harness profile and the header each +loader parses is synthesized at render time. A copy of the template set per harness +would drift, and the drift would be silent — the same argument that keeps the +methodology out of skills two paragraphs up. + +This is the one thing templates may interpolate, and it does not cost the upgrade +path: a `(template version, harness)` pair still renders byte-identically everywhere, +and the manifest records both, so re-rendering the recorded version to reconstruct a +merge base stays exact. + +**Codex ships no slash commands, deliberately.** Its custom prompts are user-global +and now deprecated in favor of skills, so there is nothing project-scoped to write +and scc does not write into anyone's home directory. The six skills are the whole +surface there, which is what Codex itself recommends. + +**No entry file is shared across harness *layouts*.** Codex and opencode both read +`AGENTS.md`, so a repo initialized for both has one entry file, written by whichever +ran first and never overwritten — it points at that harness's rules, which are the +same bytes as the other's. That is a rough edge, not a bug: the alternative is +inventing a neutral rules directory neither tool knows, to save one duplicated tree. ### Which skills ship — the authors of `docs/` @@ -421,8 +457,8 @@ Two subagents ship with the workspace, and both of them read rather than write: | Agent | Role | |---|---| -| **code-review** | Reviews the diff for correctness and quality. | -| **security-review** | Reviews the diff for security defects specifically. | +| **code-review** | Runs five gates over the diff: the ticked boxes are true, the code matches the tasks, the feature's tests, the lint, and the best practices no linter has an opinion about. | +| **security-review** | Looks for what the change makes possible, attack-class agnostic: surface, trust boundaries, reachability, deliberate attack. | Splitting review by lens is deliberate: a single reviewer asked for "everything" reliably under-weights security, because the correctness findings are easier to @@ -434,6 +470,30 @@ change is its worst reader — they see what they meant. And a reviewer reads in of writes, so running one on a cheaper model costs far less than delegating authorship would. +**Both pin `model: sonnet` and `effort: high`.** The mid tier is the right trade for +work that reads and judges rather than authors, and the reasoning budget is where the +quality actually comes from here: tracing a value from an argument to a shell, or a +ticked box to the code behind it, is chains-of-inference work, not knowledge work. A +cheap model at low effort produces the review's *shape* without its content, which is +worse than no review because it reads like one. + +**A checklist for correctness, a method for security.** They differ because the +questions differ. Correctness has a finite, knowable set of things the orchestrator +already claimed to have done — so the reviewer re-does them as named gates and +re-runs the commands itself, since "the author said it was green" is exactly the +evidence a cold reader exists to distrust. Security has no such set: a checklist of +attack names bounds the review at the names, and the interesting weakness is the one +nobody named. So that agent gets passes over the code — surface, boundaries, +reachability, attack — with the known classes demoted to prompts that jog the passes +and explicitly not the definition of done. + +**Both report in a fixed shape, and the shape is the handoff.** A verdict, a table of +what was actually checked (including what could not be run, as `not-run` and never as +a pass), findings with severities that map to actions, and a place for the +unconfirmed suspicion so it is not smuggled in as a finding. The orchestrator branches +on that report in §9 without re-reading the diff — which is the whole point of paying +for a second context. + ### The orchestrator closes the loop on every task A task is not done when the code is written. Whoever built it — which is always the @@ -577,10 +637,13 @@ Once the last task is done: 1. **Full suite + lint** on the integrated branch — the per-task scoped runs (§7) cannot see breakage between tasks. -2. **`code-review` and `security-review`** on the diff (§7), and fix what they find. - The PR should arrive already reviewed; a PR is for the human, and spending their - attention on findings a subagent would have caught is the waste this ordering - avoids. +2. **`code-review` and `security-review`** on the diff (§7), dispatched together, and + fix from their reports by severity: `blocker`/`critical` stops the PR, + `major`/`high` is fixed before merge, `minor`/`low` is a judgment call stated in + the PR body. The PR should arrive already reviewed; a PR is for the human, and + spending their attention on findings a subagent would have caught is the waste + this ordering avoids. One fix-and-re-review round; a second means the finding + wants a person. 3. **Commit and push** — Conventional Commits, generated from the diff and the spec. 4. **Open the PR.** @@ -770,14 +833,39 @@ conflict idiom in the whole product instead of two. **This constrains the manifest.** A three-way merge needs the old rendered *text*, not just its hash. scc embeds its templates, so it can re-render any past version — but only if it -knows which version produced each file. So `.claude/scc-manifest.json` records **a version -per entry alongside the content hash**: the hash answers "did the user edit this?", the -version answers "what did it look like before?" Both are required, and a manifest carrying -only hashes cannot support this. +knows which version produced each file, and for which harness. So `/scc-manifest.json` +records **a version per entry alongside the content hash**, and the harness once for the +file: the hash answers "did the user edit this?", the version and harness answer "what did +it look like before?" All three are required, and a manifest carrying only hashes cannot +support this. Files that exist to be edited once and then owned by the user are **excluded from the merge** rather than merged badly. +### What shipped first: replace-or-keep, with the plan shown + +The merge above is the destination. What `scc update` does today is the honest subset +of it, and it is deliberately not a merge: + +- Every managed file is hashed against what this build renders and against what the + manifest says scc last wrote. That splits it four ways — **current**, **create** + (new in this version, or deleted), **update** (still exactly what scc wrote, and + the template moved), **conflict** (the user changed it) — plus **delete** for a + file this version no longer ships and **owned** for the two files that exist to be + edited. +- **The plan is printed and confirmed before anything is written.** Not a nicety: an + update is the one command that can destroy work, and a summary the user agrees to + is what makes the difference between a tool and a surprise. `--yes` is that + agreement given up front and is required when stdin is not a terminal, `--dry-run` + reports and stops. +- **A conflict is kept, not merged and not clobbered**, and is reported by name with + what to do about it. `--force` is a separate, explicit decision. The entry it keeps + in the manifest stays at the version scc last wrote — which is precisely the base + revision the three-way merge will need when it lands. + +The merge is what upgrades this into: same plan, same confirmation, but a conflict +gets resolved instead of deferred. + ## Open questions - **Does a Plan's decomposition get validated for coverage** — i.e. can scc tell diff --git a/design/plan.md b/design/plan.md index 8e4d340..b6c08ca 100644 --- a/design/plan.md +++ b/design/plan.md @@ -11,8 +11,9 @@ scc can scaffold itself, this file migrates. Section references (§n) are to `orchestration.md` throughout. -**Status: phases 1–10 are built and green; phase 11 (`scc update`) is not started.** -The command surface is `init`, `spec new|list|show|delete|validate`, +**Status: phases 1–10 are built and green; phase 11 (`scc update`) shipped as +replace-or-keep rather than as the three-way merge described below.** +The command surface is `init`, `update`, `spec new|list|show|delete|validate`, `plan new|list|delete|validate`, `skill validate`, `validate`, `version`, `help`. §9 records what the build settled that the plan left open, and where it deviated. Every decision listed in §5 has been folded back into `orchestration.md`. @@ -563,11 +564,35 @@ a validator needs a written convention to check against: ADR frontmatter (`statu format (`- **term** — definition. Avoid: synonym`), the wiki's `index.md`/`changelog.md` pair, and the codewiki citation form. -**Phase 11 is deliberately not started.** §3's reason now literally applies: only one -template version exists, so `scc update` could only be tested against synthetic history. -It becomes real work the moment a second template version ships. Until then the honest -upgrade story is the one §6 already states — re-run `init`, which never overwrites what -you edited. +**Phase 11 shipped in two halves, and only the first half is built.** 11.2's command +exists; 11.1's `internal/merge` does not. What `scc update` does is hash every managed +file against what this build renders and against what the manifest recorded, print the +resulting plan grouped by outcome — create, update, delete, conflict, owned — ask for +confirmation, and then replace what is safe to replace. An edited file is *kept and +named*, not merged and not silently overwritten; `--force` is the separate decision, +and `--yes` is the confirmation given up front for unattended callers. + +That is less than the plan promised and it is on purpose. The three-way merge needs a +store of historical renderings keyed by template version, and every argument for it +assumes the user wants a conflict resolved in place rather than reported. Reporting is +strictly safer, it is testable today, and it is the part users would need first. The +manifest keeps a conflicted file's *recorded* hash and version rather than today's, +so the merge base the future merge needs is being preserved from now on rather than +retrofitted later. + +**Three harnesses, one template set** — not in any phase, and the largest change since. +`scc init --claude|--codex|--opencode` scaffolds the same methodology into Claude Code, +Codex, or opencode, and the picker asks when a person is at the terminal and named no +flag. The template tree lost its per-harness directory in the process: rules, skills, +and reviewers are written once, the paths in them come from a harness profile, and the +frontmatter each loader parses is synthesized at render time — TOML for a Codex agent +role file, YAML for the other two. §6 records the layout and the two things that fall +out of it: Codex ships no slash commands (its prompts are user-global and deprecated in +favor of skills), and the manifest now records the harness, because a template rendered +for the wrong one would be a wrong merge base. + +The data-free rule from Phase 1.2 survives this, restated: the harness profile is the +only value a workspace template may interpolate, and it is data scc records. One thing the build found that no phase predicted, worth recording because it is the class of bug this whole design is about: the first end-to-end run of From 638fb0cb17bc50420e42c94d4664dca7a33834f0 Mon Sep 17 00:00:00 2001 From: prode Date: Sat, 1 Aug 2026 20:53:53 -0300 Subject: [PATCH 4/5] fix(manifest): refuse a recorded path that escapes the workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A manifest arrives with a clone, and `scc update` joins each recorded path onto the root and then calls os.Remove on the ones this version no longer ships. An entry reading "../../.ssh/authorized_keys" therefore deletes outside the workspace — the exact class this project's SafeName rule exists for, reached through the one file scc reads back rather than through a CLI argument. Load refuses the whole file instead of skipping the bad entry: a manifest scc did not write is not a manifest, and silently dropping part of it would leave a workspace whose state nobody can account for. Two related hardenings from the same review: `update --json` without --yes is refused rather than prompting. confirm writes through render.Ask to stdout, so asking would emit a question into the document the caller is piping into jq; --dry-run and --yes both still work. The workspace marker is checked with Lstat. Its whole claim is "scc wrote this here", and a symlink pointing at somebody else's manifest satisfies Stat while making that claim false. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019yCM51izVEvKywpKDgYvNM --- internal/cli/update.go | 7 ++++++ internal/cli/update_test.go | 25 ++++++++++++++++++++ internal/manifest/manifest.go | 23 ++++++++++++++++++ internal/manifest/manifest_test.go | 35 ++++++++++++++++++++++++++++ internal/workspace/workspace.go | 15 ++++++++---- internal/workspace/workspace_test.go | 26 +++++++++++++++++++++ 6 files changed, 126 insertions(+), 5 deletions(-) diff --git a/internal/cli/update.go b/internal/cli/update.go index 766cbf0..002b618 100644 --- a/internal/cli/update.go +++ b/internal/cli/update.go @@ -110,6 +110,13 @@ func runUpdate(args []string) int { } if !*yes { + // --json has no way to ask: the prompt and the "nothing was written" line + // both go to stdout, which is the document the caller is piping into jq. + // Refuse the combination rather than emitting something that is not JSON. + if *jsonOut { + render.Err("--json cannot ask: pass --yes to apply, or --dry-run to see the plan") + return ExitError + } if !interactive() { render.Err("stdin is not a terminal: re-run with --yes to apply, or --dry-run to see the plan") return ExitError diff --git a/internal/cli/update_test.go b/internal/cli/update_test.go index 1799acc..be4ee9a 100644 --- a/internal/cli/update_test.go +++ b/internal/cli/update_test.go @@ -164,6 +164,31 @@ func TestUpdateAsksBeforeWritingAndHonorsNo(t *testing.T) { } } +// --json has no way to ask: the prompt goes to stdout, which is the document the +// caller is piping into jq. The combination is refused rather than answered with +// something that is not JSON. +func TestUpdateJSONWithoutYesIsRefused(t *testing.T) { + withPrompt(t, "y\n") + root := initWorkspace(t) + rel := ".claude/rules/specs.md" + if err := os.Remove(filepath.Join(root, filepath.FromSlash(rel))); err != nil { + t.Fatalf("Remove: %v", err) + } + stdout, stderr, code := run(t, "update", "--root", root, "--json") + if code != ExitError { + t.Fatalf("exit = %d, want %d", code, ExitError) + } + if strings.TrimSpace(stdout) != "" { + t.Errorf("stdout is not empty: %q", stdout) + } + if !strings.Contains(stderr, "--yes") || !strings.Contains(stderr, "--dry-run") { + t.Errorf("stderr = %q, want it to name both ways out", stderr) + } + if _, err := os.Stat(filepath.Join(root, filepath.FromSlash(rel))); !os.IsNotExist(err) { + t.Error("the refused run still wrote the file") + } +} + func TestUpdateJSONDryRunEmitsThePlan(t *testing.T) { root := initWorkspace(t) stdout, stderr, code := run(t, "update", "--root", root, "--dry-run", "--json") diff --git a/internal/manifest/manifest.go b/internal/manifest/manifest.go index df5ecdd..c3d58c8 100644 --- a/internal/manifest/manifest.go +++ b/internal/manifest/manifest.go @@ -138,10 +138,33 @@ func Load(root string, h paths.Harness) (m *Manifest, found bool, err error) { if err := json.Unmarshal([]byte(textutil.NormalizeNewlines(string(b))), m); err != nil { return nil, true, fmt.Errorf("%s is not valid scc manifest JSON: %w", h.Manifest(root), err) } + // Every recorded path is joined onto the root by somebody — and `scc update` + // joins it and then calls os.Remove. A manifest is a committed file that + // arrives with a clone, so an entry reading "../../.ssh/authorized_keys" is + // exactly the hostile input this project's rule about SafeName exists for. + // Refuse the whole file rather than skipping the bad entry: a manifest scc did + // not write is not a manifest, and silently dropping part of it would leave a + // workspace whose state nobody can account for. + for _, e := range m.Files { + if !isLocalPath(e.Path) { + return nil, true, fmt.Errorf("%s records the path %q, which escapes the workspace — refusing to use it", + h.Manifest(root), e.Path) + } + } m.sortFiles() return m, true, nil } +// isLocalPath reports whether a recorded path stays inside the workspace when +// joined onto the root: relative, no "..", no root, and — on Windows — not a +// reserved device name. +func isLocalPath(rel string) bool { + if rel == "" || strings.ContainsRune(rel, '\\') { + return false + } + return filepath.IsLocal(filepath.FromSlash(rel)) +} + // Save writes h's manifest under root atomically, so a concurrent reader — or a // crash — never observes a half-written marker. func Save(root string, h paths.Harness, m *Manifest) error { diff --git a/internal/manifest/manifest_test.go b/internal/manifest/manifest_test.go index 6a56c2e..2a83ccc 100644 --- a/internal/manifest/manifest_test.go +++ b/internal/manifest/manifest_test.go @@ -251,6 +251,41 @@ func TestHarnessRoundTripsAndIsPerHarness(t *testing.T) { } } +// A manifest arrives with a clone, and `scc update` joins its recorded paths onto +// the root and then calls os.Remove. An entry that escapes the workspace is the +// hostile input this project's SafeName rule exists for, so Load refuses the file +// rather than acting on it. +func TestLoadRefusesAPathThatEscapesTheWorkspace(t *testing.T) { + for _, bad := range []string{ + "../outside.md", + ".claude/../../outside.md", + "/etc/passwd", + `..\outside.md`, + "", + } { + root := t.TempDir() + doc := `{"scc":"v1","harness":"claude","files":[{"path":"` + bad + `","hash":"x","version":"5"}]}` + if err := workspace.AtomicWrite(paths.Claude.Manifest(root), []byte(doc), 0o644); err != nil { + t.Fatalf("AtomicWrite: %v", err) + } + if _, _, err := Load(root, paths.Claude); err == nil { + t.Errorf("Load accepted the entry %q", bad) + } + } +} + +func TestLoadAcceptsOrdinaryPaths(t *testing.T) { + root := t.TempDir() + doc := `{"scc":"v1","harness":"claude","files":[{"path":".claude/rules/routing.md","hash":"x","version":"5"}]}` + if err := workspace.AtomicWrite(paths.Claude.Manifest(root), []byte(doc), 0o644); err != nil { + t.Fatalf("AtomicWrite: %v", err) + } + m, found, err := Load(root, paths.Claude) + if err != nil || !found || len(m.Files) != 1 { + t.Fatalf("Load = (%+v, %v, %v)", m, found, err) + } +} + // Save writes what Bytes reports, so a caller can compare against the file on // disk and skip an identical write — which is what makes a second `scc init` a // no-op rather than a rewrite. diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index 9617940..f8c0901 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -76,8 +76,10 @@ func AtomicWrite(path string, data []byte, perm os.FileMode) error { } // Find walks upward from start looking for a workspace marker and returns the -// directory holding it, or start unchanged when there is none. Any harness's -// marker answers: a workspace is a workspace whichever tool it was scaffolded +// directory holding it. With no marker anywhere above it, the answer is start +// made absolute — every caller joins paths onto this, so returning a relative +// path would make the result depend on a working directory that may have moved +// by then. Any harness's marker answers: a workspace is a workspace whichever tool it was scaffolded // for, and specs/, plans/, and docs/ sit at that root regardless. // // The marker is the file /scc-manifest.json, never the harness @@ -107,10 +109,13 @@ func Find(start string) string { } } -// isFile reports whether path exists and is a regular file. A directory that -// happens to carry the manifest's name is not a marker. +// isFile reports whether path is itself a regular file. A directory that happens +// to carry the manifest's name is not a marker — and neither is a symlink, which +// is why this is Lstat and not Stat: the marker's whole claim is "scc wrote this +// here", and a link pointing at somebody else's manifest (the user's global one, +// say) satisfies Stat while making that claim false. func isFile(path string) bool { - info, err := os.Stat(path) + info, err := os.Lstat(path) return err == nil && info.Mode().IsRegular() } diff --git a/internal/workspace/workspace_test.go b/internal/workspace/workspace_test.go index 28f805e..529cca4 100644 --- a/internal/workspace/workspace_test.go +++ b/internal/workspace/workspace_test.go @@ -146,6 +146,32 @@ func TestFindRejectsMarkerThatIsADirectory(t *testing.T) { } } +// A symlink is not a marker. The marker's whole claim is "scc wrote this here", +// and a link pointing at somebody else's manifest — the user's global one, say — +// satisfies Stat while making that claim false. +func TestFindRejectsASymlinkedMarker(t *testing.T) { + elsewhere := filepath.Join(t.TempDir(), "real-manifest.json") + if err := AtomicWrite(elsewhere, []byte("{}\n"), 0o644); err != nil { + t.Fatalf("AtomicWrite: %v", err) + } + root := t.TempDir() + marker := paths.Claude.Manifest(root) + if err := os.MkdirAll(filepath.Dir(marker), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.Symlink(elsewhere, marker); err != nil { + // Unprivileged Windows without developer mode cannot make one; the + // behavior under test is unreachable there rather than wrong. + t.Skipf("symlinks unavailable: %v", err) + } + if IsWorkspace(root) { + t.Error("IsWorkspace = true for a symlinked marker, want false") + } + if len(Harnesses(root)) != 0 { + t.Error("a symlinked marker was reported as an initialized harness") + } +} + // The nearest marker wins, so a workspace nested inside another resolves to // itself rather than to its parent. func TestFindStopsAtNearestMarker(t *testing.T) { From 4f61d5d79613ecd4e4ef9ef6fb8756f89a3907e5 Mon Sep 17 00:00:00 2001 From: prode Date: Sat, 1 Aug 2026 20:53:53 -0300 Subject: [PATCH 5/5] docs: name plans/ in the workspace overview and correct the shared AGENTS.md note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit found both: the governed-artifact lists said specs/ and docs/ but not plans/, and §6's claim that the two AGENTS.md harnesses have byte-identical rules was not quite true — eight of nine are, and the ninth differs in a glossary example naming a manifest path. The conclusion is unchanged; the reason it holds is now stated accurately. The report-format fences in both review agents get a language tag. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019yCM51izVEvKywpKDgYvNM --- CLAUDE.md | 4 ++-- design/orchestration.md | 19 +++++++++++++------ .../assets/templates/agents/code-review.md | 2 +- .../templates/agents/security-review.md | 2 +- 4 files changed, 17 insertions(+), 10 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 0d57d8f..7f6f9ef 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,11 +6,11 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co `scc` is a single Go binary (`github.com/protonspy/spec-claude-code`) that enforces Spec-Driven Development inside an agent-driven repo — Claude Code, Codex, or opencode. One surface: a headless CLI, so an AI agent or a CI job drives it exactly as well as a human does. -The artifacts it governs (the harness's own directory, `specs/`, `docs/`) are plain Markdown/JSON. There is no server and no database — the files *are* the API. scc has no directory of its own: its state lives in one file under the harness's directory, alongside what that tool already keeps there. +The artifacts it governs (the harness's own directory, `specs/`, `plans/`, `docs/`) are plain Markdown/JSON. There is no server and no database — the files *are* the API. scc has no directory of its own: its state lives in one file under the harness's directory, alongside what that tool already keeps there. This file covers working *on* scc. The product's own rules and methodology are not documented here: they live in `design/` while being designed, and ship as templates the binary scaffolds into `/rules/`. -Note: this repo is not itself an scc workspace (no harness directory, `specs/`, or `docs/` are committed) — those trees only exist in workspaces the binary scaffolds, and in test temp dirs. +Note: this repo is not itself an scc workspace (no harness directory, `specs/`, `plans/`, or `docs/` are committed) — those trees only exist in workspaces the binary scaffolds, and in test temp dirs. **Status: v0.4.0-shaped.** Everything through `design/plan.md` phase 10 is built and green: scaffolding (`init`), artifact creation (`spec`, `plan`), and all eight validators behind `scc validate`. `init` also scaffolds the six knowledge-base skills named in `design/orchestration.md` §6 — one per `docs/` artifact a validator checks, plus `prd` — each with a `scc-`-prefixed slash command derived from the same list (`assets.KnowledgeSkills`), wherever the harness has a command surface. diff --git a/design/orchestration.md b/design/orchestration.md index 697eb9c..9208ab9 100644 --- a/design/orchestration.md +++ b/design/orchestration.md @@ -12,7 +12,7 @@ several of them are evidence-backed rather than preferences. Same philosophy as csdd: **scc is a template plus a CLI.** The binary scaffolds a workspace — rules, skills, agents, spec templates — into the harness's own -configuration directory plus `specs/` and `docs/`, all of it plain Markdown compiled +configuration directory plus `specs/`, `plans/`, and `docs/`, all of it plain Markdown compiled into the binary via `//go:embed`, and then validates what lives there. There is no conversion layer and no runtime: the files the harness already reads *are* the API, and the CLI is how they are created, checked, and upgraded. @@ -411,11 +411,18 @@ and now deprecated in favor of skills, so there is nothing project-scoped to wri and scc does not write into anyone's home directory. The six skills are the whole surface there, which is what Codex itself recommends. -**No entry file is shared across harness *layouts*.** Codex and opencode both read -`AGENTS.md`, so a repo initialized for both has one entry file, written by whichever -ran first and never overwritten — it points at that harness's rules, which are the -same bytes as the other's. That is a rough edge, not a bug: the alternative is -inventing a neutral rules directory neither tool knows, to save one duplicated tree. +**Codex and opencode share `AGENTS.md`, and the first one initialized wins it.** A +repo scaffolded for both has one entry file, written by whichever ran first and never +overwritten afterwards, pointing at that harness's rules directory. The second +harness's session follows those links and gets the right methodology: eight of the +nine rules are byte-identical between the two, and the ninth differs only in a +glossary *example* naming a manifest path. So the second tree is a duplicate nothing +links to — a wart, not a broken workspace, and the design is choosing it over the +alternatives: inventing a neutral rules directory neither tool knows about, or +rewriting a file the user owns. + +`scc update` keeps both trees current regardless, because it works from each +harness's own manifest rather than from the entry file. ### Which skills ship — the authors of `docs/` diff --git a/internal/assets/templates/agents/code-review.md b/internal/assets/templates/agents/code-review.md index 3b34ff3..d0906a1 100644 --- a/internal/assets/templates/agents/code-review.md +++ b/internal/assets/templates/agents/code-review.md @@ -83,7 +83,7 @@ line, but do not try to be that reviewer. End with this, and nothing after it: -``` +```text ## Verdict — one sentence saying why. diff --git a/internal/assets/templates/agents/security-review.md b/internal/assets/templates/agents/security-review.md index f70f97e..205c04c 100644 --- a/internal/assets/templates/agents/security-review.md +++ b/internal/assets/templates/agents/security-review.md @@ -87,7 +87,7 @@ every class below is not evidence of safety. End with this, and nothing after it: -``` +```text ## Verdict — one sentence saying why.