diff --git a/.devcontainer/Dockerfile b/.devcontainer/Dockerfile index a16cb21..16ae745 100644 --- a/.devcontainer/Dockerfile +++ b/.devcontainer/Dockerfile @@ -36,6 +36,25 @@ RUN apt-get update \ && apt-get install -y --no-install-recommends gh \ && rm -rf /var/lib/apt/lists/* +# The workspace is a bind mount from the host, so its files arrive owned by a uid +# this container does not have, and git refuses a repository it thinks belongs to +# somebody else — the ownership check that exists because a differently-owned .git +# can carry a config or a hook that runs on the next ordinary git command +# (CVE-2022-24765). Scoped to where devcontainers mount a workspace and no further: +# this container forwards GH_TOKEN and mounts the agent's credential volume, so a +# bare `*` here would hand the next crafted .git in a dependency tree exactly those. +# +# Set in the image rather than in postCreateCommand, and that is forced from both +# ends: --global writes ~/.gitconfig, which is the read-only bind that carries your +# identity in ("Device or resource busy", which failed postCreate, `devcontainer up` +# and the whole sandbox, back onto the host), while --system writes /etc/gitconfig, +# which postCreate's non-root user cannot. The build runs as root and can. +# +# The path is a literal because ${containerWorkspaceFolder} is not resolved at build +# time. A workspace mounted somewhere else gets git's ordinary ownership message and +# one `git config --global --add safe.directory ` to run — visible, not silent. +RUN git config --system --add safe.directory '/workspaces/*' + # RTK is the slow layer: it is a Rust build, so this installs a toolchain, builds # one binary and throws the toolchain away. Delete this block if you would rather # have fast rebuilds than filtered command output — nothing else depends on it, @@ -50,7 +69,6 @@ RUN export CARGO_HOME=/tmp/cargo RUSTUP_HOME=/tmp/rustup \ && "$CARGO_HOME/bin/cargo" install --git https://github.com/rtk-ai/rtk --locked \ && install -m 0755 "$CARGO_HOME/bin/rtk" /usr/local/bin/rtk \ && rm -rf "$CARGO_HOME" "$RUSTUP_HOME" - # The project toolchain — exactly what `scc check` runs, and nothing a human would # merely like to have. The four recorded gates are `go build ./...`, # `golangci-lint fmt --diff`, `golangci-lint run` and `go run -C tools ./testreport`, @@ -87,14 +105,12 @@ RUN curl -fsSL "https://dl.google.com/go/go${GO_VERSION}.linux-${TARGETARCH}.tar && rm /tmp/go.tgz /tmp/go.tgz.sha256 \ && ln -sf /usr/local/go/bin/go /usr/local/bin/go \ && ln -sf /usr/local/go/bin/gofmt /usr/local/bin/gofmt - # `go install` at a pinned version rather than the upstream install script: the # version is the one CI enforces, and piping a remote script into a shell is a thing # this project declines to do on somebody's behalf (see docs/stack.md). GOBIN puts # the binary where every shell looks, as above. RUN GOBIN=/usr/local/bin go install \ github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_VERSION} - # The module and build caches under the node user's home, since that is who runs the # gate; a root-owned cache is the failure that looks like a network problem the first # time the agent runs `go build`. diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index 39e526c..33259de 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -45,7 +45,12 @@ "GITHUB_TOKEN": "${localEnv:GITHUB_TOKEN}" }, - "postCreateCommand": "scc version && git config --global --add safe.directory ${containerWorkspaceFolder}", + // Just a smoke test that the agent's toolchain is on PATH. Nothing here writes + // git config: ~/.gitconfig is the read-only bind above, and `git config --global` + // against it fails with "Device or resource busy" — which failed postCreate, which + // failed `devcontainer up`, which dropped the launch back onto the host outside any + // sandbox at all. safe.directory is set in the image instead, with --system. + "postCreateCommand": "scc version", "customizations": { "vscode": { diff --git a/docs/notes.md b/docs/notes.md index 9e3492f..ce667af 100644 --- a/docs/notes.md +++ b/docs/notes.md @@ -44,3 +44,8 @@ over this file answers with the example above as well as with the notes. --> - n-0004 2026-09-15 #gotcha @tools/go.mod — tools/ is a nested module so the root's ./... never sees it — a helper in the main module would land in the coverage profile at 0% - n-0005 2026-09-15 #gotcha @.devcontainer/Dockerfile — bash -lc re-runs /etc/profile and rebuilds PATH, discarding every Docker ENV, so container tooling must be linked into /usr/local/bin - n-0006 2026-09-15 #gotcha @internal/scaffold — scc update deletes a managed file but never prunes the directory it emptied, and the skills validator then reports skill.missing-skill-md on the empty dir + +- n-0008 2026-09-15 #gotcha @.devcontainer/devcontainer.json — postCreateCommand cannot run git config --global: ~/.gitconfig is a read-only bind, git fails Device or resource busy, and devcontainer up takes the sandbox down with it +- n-0009 2026-09-15 #gotcha @internal/mdblock/mdblock.go @internal/rtk/rtk.go @internal/mdscan/mdscan.go — a marker written in prose or a code span is not a block — mdblock and rtk.ForeignBlock search mdscan.Mask, and the entry file that documents the markers is why +- n-0010 2026-09-15 #ceiling @internal/mdscan/mdscan.go — mdscan.Mask hides fenced blocks and inline code spans only — a marker in a 4-space-indented block or a blockquote still reads as a live block; lift it by teaching Mask those two constructs +- n-0011 2026-09-15 #gotcha @internal/artifact/artifact.go — artifact.Within must resolve both sides the same way — resolving only the root made a not-yet-created path inside the workspace read as an escape on macOS and on Windows 8.3 paths diff --git a/internal/artifact/artifact.go b/internal/artifact/artifact.go index 94872ff..1ceef0a 100644 --- a/internal/artifact/artifact.go +++ b/internal/artifact/artifact.go @@ -514,9 +514,20 @@ func Resolve(root, arg string) ([]string, error) { // // Symlinks are evaluated on both sides before the comparison, because a temporary // directory is itself a symlink on macOS (/var → /private/var) and a purely -// lexical test would call every file under one an escape. Where a side cannot be -// evaluated its cleaned absolute path is used, which still catches the `..` that -// this exists for. +// lexical test would call every file under one an escape. +// +// **Both sides are resolved the same way, and that is the whole of it.** Resolving +// only what exists is what broke: a target that is not on disk yet — the ordinary +// case for a file about to be written — failed EvalSymlinks and stayed lexical +// while the root beside it was resolved, so the two were no longer comparable and +// a path plainly inside the workspace came back as an escape. Measured on macOS +// (/var vs /private/var) and on Windows, where a path handed over with an 8.3 +// short name (RUNNER~1) resolves to its long form on one side only. +// +// So each side resolves its deepest existing ancestor and re-attaches the rest. +// The part that does not exist cannot be a symlink — there is nothing there to be +// one — so nothing is assumed about it, and the `..` this exists to catch is still +// caught: it is resolved away by Abs before any of this runs. func Within(root, target string) bool { r, err := filepath.Abs(root) if err != nil { @@ -526,19 +537,38 @@ func Within(root, target string) bool { if err != nil { return false } - if p, err := filepath.EvalSymlinks(r); err == nil { - r = p - } - if p, err := filepath.EvalSymlinks(t); err == nil { - t = p - } - rel, err := filepath.Rel(r, t) + rel, err := filepath.Rel(resolveExisting(r), resolveExisting(t)) if err != nil { return false } return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) } +// resolveExisting evaluates symlinks on the deepest existing ancestor of p and +// re-attaches whatever was below it, so a path that is not on disk yet compares on +// the same terms as one that is. +// +// A path with no existing ancestor at all comes back as it was given, which is the +// answer for a volume that is not mounted and for the root of a filesystem that +// does not exist. +func resolveExisting(p string) string { + rest := "" + for cur := p; ; { + if resolved, err := filepath.EvalSymlinks(cur); err == nil { + if rest == "" { + return resolved + } + return filepath.Join(resolved, rest) + } + parent := filepath.Dir(cur) + if parent == cur { + return p + } + rest = filepath.Join(filepath.Base(cur), rest) + cur = parent + } +} + // Scan loads every artifact in the workspace: each plan, then each spec's three // files in phase order. It is the index every other read is a narrowing of. func Scan(root string) ([]*Artifact, error) { diff --git a/internal/artifact/cover_test.go b/internal/artifact/cover_test.go new file mode 100644 index 0000000..74010b2 --- /dev/null +++ b/internal/artifact/cover_test.go @@ -0,0 +1,790 @@ +package artifact + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// The vocabulary is closed, and it is closed for the reason a task's flags are: an +// open one produces three spellings of "done" inside a month, and the question this +// record answers — what is still unfinished — is the one a synonym destroys. +func TestDeliveryVocabularyIsClosedAndOrdered(t *testing.T) { + states := DeliveryStates() + if len(states) != 4 { + t.Fatalf("DeliveryStates = %v, want the four this record knows", states) + } + if states[0] != DeliveryInProgress || states[len(states)-1] != DeliveryAbandoned { + t.Errorf("DeliveryStates = %v, want the order work moves through", states) + } + for _, s := range states { + if !ValidDelivery(s) { + t.Errorf("ValidDelivery(%q) = false for a state of its own list", s) + } + } + for _, s := range []string{"", "done", "DONE", "in progress", "closed"} { + if ValidDelivery(s) { + t.Errorf("ValidDelivery(%q) = true for a word this vocabulary does not have", s) + } + } + + // Settled is what separates "still open" from "done with", which is the only + // distinction a reader scanning for loose ends actually makes. + for _, s := range []string{DeliveryMerged, DeliveryAbandoned} { + if !Settled(s) { + t.Errorf("Settled(%q) = false for a terminal state", s) + } + } + for _, s := range []string{DeliveryInProgress, DeliveryInReview, ""} { + if Settled(s) { + t.Errorf("Settled(%q) = true for work that is still open", s) + } + } +} + +// A `pr:` that is not a number comes back as 0 rather than as an error: reading is +// not where a malformed value is reported, and a reader that failed here would take +// out `scc spec list` for the whole workspace over one typo. +func TestReadDeliveryNeverFailsOnAMalformedValue(t *testing.T) { + full := ReadDelivery(map[string]string{ + KeyBranch: "feat/x", KeyPR: "42", KeyDelivery: DeliveryInReview, + }) + if full.Branch != "feat/x" || full.PR != 42 || full.State != DeliveryInReview { + t.Errorf("ReadDelivery = %+v", full) + } + if !full.Tracked() { + t.Error("a spec with a branch, a PR and a state reported as untracked") + } + + for _, bad := range []string{"", "none", "#42", "0", "-3"} { + got := ReadDelivery(map[string]string{KeyPR: bad}) + if got.PR != 0 { + t.Errorf("ReadDelivery with pr=%q gave %d, want 0", bad, got.PR) + } + } + + // An untracked spec is not a defect — it is one nobody has started, or one + // that predates the record. + if ReadDelivery(nil).Tracked() { + t.Error("an empty frontmatter reported as tracked") + } + // Any one of the three is enough to count as started. + for _, fm := range []map[string]string{ + {KeyBranch: "feat/x"}, + {KeyPR: "1"}, + {KeyDelivery: DeliveryMerged}, + } { + if !ReadDelivery(fm).Tracked() { + t.Errorf("ReadDelivery(%v) reported untracked", fm) + } + } +} + +// Scan is the index every other read is a narrowing of: plans in name order, then +// each spec's three files in phase order, so a caller listing a workspace gets the +// same order twice running. +func TestScanReadsPlansThenSpecsInPhaseOrder(t *testing.T) { + root := t.TempDir() + mkdirAll(t, filepath.Join(root, "plans")) + mkdirAll(t, filepath.Join(root, "specs", "beta")) + mkdirAll(t, filepath.Join(root, "specs", "alpha")) + + write(t, filepath.Join(root, "plans", "zulu.md"), "# Zulu\n") + write(t, filepath.Join(root, "plans", "alpha.md"), "# Alpha plan\n") + // A directory under plans/ is skipped rather than read: the scanner uses + // ReadDir, which is what lets `plans/archive/` hold a migrated plan's notes. + mkdirAll(t, filepath.Join(root, "plans", "archive")) + write(t, filepath.Join(root, "plans", "archive", "old-notes.md"), "# Notes\n") + // And a file that is not Markdown is not an artifact. + write(t, filepath.Join(root, "plans", "notes.txt"), "not an artifact\n") + + write(t, filepath.Join(root, "specs", "alpha", "requirements.md"), "# Alpha\n") + write(t, filepath.Join(root, "specs", "alpha", "tasks.md"), "# Alpha tasks\n") + write(t, filepath.Join(root, "specs", "beta", "requirements.md"), "# Beta\n") + + all, err := Scan(root) + if err != nil { + t.Fatalf("Scan: %v", err) + } + var got []string + for _, a := range all { + got = append(got, a.Path) + } + want := []string{ + "plans/alpha.md", "plans/zulu.md", + "specs/alpha/requirements.md", "specs/alpha/tasks.md", + "specs/beta/requirements.md", + } + if strings.Join(got, " ") != strings.Join(want, " ") { + t.Errorf("Scan =\n %v\nwant\n %v", got, want) + } + + // A workspace with neither tree is empty rather than an error: that is a + // young workspace, not a failure. + empty, err := Scan(t.TempDir()) + if err != nil || len(empty) != 0 { + t.Errorf("Scan on an empty workspace = %d artifacts, %v", len(empty), err) + } +} + +func mkdirAll(t *testing.T, dir string) { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("MkdirAll %s: %v", dir, err) + } +} + +func write(t *testing.T, path, body string) { + t.Helper() + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatalf("WriteFile %s: %v", path, err) + } +} + +// Text is what an editor prints and Prose is what a summary prints, and the +// difference is the whole reason there are two. On a freshly scaffolded artifact +// the instructions to the author are HTML comments and outweigh everything the +// author has written, so a brief that echoed them would spend its budget on text +// addressed to somebody else. +func TestProseDropsWhatIsNotContentAndTextDoesNot(t *testing.T) { + root := t.TempDir() + mkdirAll(t, filepath.Join(root, "plans")) + path := filepath.Join(root, "plans", "p.md") + write(t, path, "# P\n\n## Why\n\n\nreal prose\n\n\n\n```\nfenced\n```\nmore prose\n") + + a, err := Load(root, path) + if err != nil { + t.Fatalf("Load: %v", err) + } + + full := a.Text(1, a.LineCount()) + for _, want := range []string{"an instruction to the author", "fenced", "real prose"} { + if !strings.Contains(full, want) { + t.Errorf("Text dropped %q, and an editor needs the file as it is:\n%s", want, full) + } + } + + summary := a.Prose(1, a.LineCount()) + for _, gone := range []string{"an instruction to the author", "fenced"} { + if strings.Contains(summary, gone) { + t.Errorf("Prose kept %q, which is not the author's content:\n%s", gone, summary) + } + } + for _, kept := range []string{"real prose", "more prose"} { + if !strings.Contains(summary, kept) { + t.Errorf("Prose dropped %q:\n%s", kept, summary) + } + } + if strings.Contains(summary, "\n\n\n") { + t.Errorf("Prose left a run of blank lines:\n%q", summary) + } + + // Both clamp rather than panic: an address resolved against a file that has + // since been edited is exactly where an out-of-range pair comes from. + if got := a.Text(-5, 1); got != a.Lines[0] { + t.Errorf("Text(-5, 1) = %q", got) + } + if got := a.Text(3, 2); got != "" { + t.Errorf("Text with from > to = %q, want empty", got) + } + if got := a.Prose(-5, a.LineCount()+50); !strings.Contains(got, "real prose") { + t.Errorf("Prose did not clamp: %q", got) + } + if a.Doc() == nil { + t.Error("Doc returned nil, so the searcher has no blanked body to match against") + } +} + +// An editor that failed reports the reason and produces nothing, so one path +// reports every reason a patch stopped — including the approved-plan guards, which +// depend on what the file says and so cannot be decided before it is loaded. +func TestEditorFailIsReportedThroughTheSamePath(t *testing.T) { + a, _ := load(t) + + e := a.Edit() + if !e.Empty() { + t.Error("a fresh editor is not empty") + } + e.Fail("the plan is approved, so %s is refused", "replace") + if _, err := e.Content(); err == nil { + t.Fatal("Content returned no error after Fail") + } else if !strings.Contains(err.Error(), "replace") { + t.Errorf("error = %q, which does not carry the reason", err) + } + + // And a failed editor stays failed: a later edit does not clear it. + e.Prepend("#why", "something") + if _, err := e.Content(); err == nil { + t.Error("an edit after a failure cleared the failure") + } +} + +// Prepend writes under a heading, above whatever it already says — the half of the +// pair that lets a caller put something where it will be read first. +func TestPrependWritesUnderTheHeading(t *testing.T) { + a, _ := load(t) + + e := a.Edit() + e.Prepend("#why", "A new first line.") + out, err := e.Content() + if err != nil { + t.Fatalf("Content: %v", err) + } + lines := strings.Split(out, "\n") + var heading, added, existing int + for i, l := range lines { + switch { + case strings.HasPrefix(l, "## Why"): + heading = i + case strings.Contains(l, "A new first line."): + added = i + case strings.Contains(l, "One paragraph about why"): + existing = i + } + } + if !(heading < added && added < existing) { + t.Errorf("prepend landed at %d, between heading %d and the existing text %d:\n%s", + added, heading, existing, out) + } + if len(e.Changes()) != 1 { + t.Errorf("Changes = %+v, want the one splice", e.Changes()) + } + + // An address that does not resolve is an error, never an insert at a guess. + bad := a.Edit() + bad.Prepend("#no-such-section", "x") + if _, err := bad.Content(); err == nil { + t.Error("prepend to an address that does not resolve produced a file") + } +} + +// The seal is tamper-evidence, not prevention: a file edited by hand after approval +// no longer matches what it recorded, and that is the whole of what it claims. +func TestApprovedAndDrift(t *testing.T) { + root := t.TempDir() + mkdirAll(t, filepath.Join(root, "plans")) + path := filepath.Join(root, "plans", "p.md") + + // A plan with no status is never checked, which is what makes every plan + // written before the seal shipped keep working. + write(t, path, "---\nautonomy: auto\n---\n\n# P\n\n## Why\n\nBecause.\n") + a, err := Load(root, path) + if err != nil { + t.Fatalf("Load: %v", err) + } + if a.Approved() { + t.Error("a plan with no status reported as approved") + } + if _, _, drifted := a.Drift(); drifted { + t.Error("a plan with no status reported as drifted") + } + + sealed := Approve("---\nautonomy: auto\n---\n\n# P\n\n## Why\n\nBecause.\n") + write(t, path, sealed) + a, err = Load(root, path) + if err != nil { + t.Fatalf("Load: %v", err) + } + if !a.Approved() { + t.Fatalf("Approve did not stamp the status:\n%s", sealed) + } + recorded, actual, drifted := a.Drift() + if drifted { + t.Errorf("a freshly approved plan drifted: recorded %q, actual %q", recorded, actual) + } + + // Edit it by hand, and the seal says so. + write(t, path, strings.Replace(sealed, "Because.", "Because of something else.", 1)) + a, err = Load(root, path) + if err != nil { + t.Fatalf("Load: %v", err) + } + recorded, actual, drifted = a.Drift() + if !drifted { + t.Error("an edited approved plan did not report drift") + } + if recorded == actual || recorded == "" || actual == "" { + t.Errorf("drift reported recorded %q and actual %q", recorded, actual) + } + + // Reseal is the answer to a legitimate edit, and it leaves the status alone. + resealed := Reseal(strings.Replace(sealed, "Because.", "Because of something else.", 1)) + write(t, path, resealed) + a, err = Load(root, path) + if err != nil { + t.Fatalf("Load: %v", err) + } + if !a.Approved() { + t.Error("Reseal dropped the approved status") + } + if _, _, drifted := a.Drift(); drifted { + t.Error("a resealed plan still reports drift") + } +} + +// The schedule is one implementation shared by --next, --ready and --blocked: two +// notions of eligibility would be two answers to "what do I work on". +func TestReadyBlockedAndNextAreOneSchedule(t *testing.T) { + root := t.TempDir() + mkdirAll(t, filepath.Join(root, "plans")) + path := filepath.Join(root, "plans", "p.md") + write(t, path, `# P + +## Tasks + +- [x] 1.1 (Unit) Done already +- [ ] 1.9 (Unit) Ninth +- [ ] 1.10 (Unit) Tenth +- [ ] 1.2 (Unit) Urgent + _Priority 1_ +- [ ] 1.3 (Unit) Waits for the tenth + _Depends 1.10_ +`) + a, err := Load(root, path) + if err != nil { + t.Fatalf("Load: %v", err) + } + + var ready []string + for _, t := range a.Ready() { + ready = append(ready, t.Number) + } + // Priority ascending with absent last, then number compared *numerically* — + // which is also why 1.9 sorts before 1.10 rather than after it. + want := []string{"1.2", "1.9", "1.10"} + if strings.Join(ready, " ") != strings.Join(want, " ") { + t.Errorf("Ready = %v, want %v", ready, want) + } + + blocked := a.BlockedTasks() + if len(blocked) != 1 || blocked[0].Number != "1.3" { + t.Errorf("BlockedTasks = %+v, want the one waiting on 1.10", blocked) + } + if got := a.WaitingOn(blocked[0]); len(got) != 1 || got[0] != "1.10" { + t.Errorf("WaitingOn = %v, want 1.10", got) + } + + next, ok := a.Next() + if !ok || next.Number != "1.2" { + t.Errorf("Next = %+v, %v — want the highest-priority ready task", next, ok) + } + if next.Number != a.Ready()[0].Number { + t.Error("Next and Ready disagree about what to work on") + } + + // A plan with nothing open has no next task, and that is an answer. + write(t, path, "# P\n\n## Tasks\n\n- [x] 1.1 (Unit) Done\n") + done, err := Load(root, path) + if err != nil { + t.Fatalf("Load: %v", err) + } + if _, ok := done.Next(); ok { + t.Error("Next found a task in a finished plan") + } + if len(done.Ready()) != 0 || len(done.BlockedTasks()) != 0 { + t.Error("a finished plan reports ready or blocked tasks") + } +} + +// Within is the boundary between an artifact address and the rest of the +// filesystem: without it `scc patch append ../outside.md L3 --text X` wrote to a +// file outside the repository and exited 0. +func TestWithinIsTheBoundaryAnAddressCannotCross(t *testing.T) { + root := t.TempDir() + mkdirAll(t, filepath.Join(root, "plans")) + write(t, filepath.Join(root, "plans", "p.md"), "# P\n") + + for _, in := range []string{ + filepath.Join(root, "plans", "p.md"), + filepath.Join(root, "plans"), + root, + filepath.Join(root, "does", "not", "exist.md"), + } { + if !Within(root, in) { + t.Errorf("Within(root, %q) = false for a path inside the workspace", in) + } + } + + outside := t.TempDir() + for _, out := range []string{ + outside, + filepath.Join(outside, "elsewhere.md"), + filepath.Join(root, "..", "sibling.md"), + filepath.Join(root, "plans", "..", "..", "escape.md"), + } { + if Within(root, out) { + t.Errorf("Within(root, %q) = true for a path outside the workspace", out) + } + } + + // A sibling whose name merely starts with the root's is not inside it, which a + // string-prefix test would get wrong. + if Within(root, root+"-other") { + t.Error("a sibling directory sharing the root's prefix was called inside it") + } +} + +// A task's continuation is the lines below the checkbox with the flags removed, +// which is where the decision usually sits — and the reason --next prints the task +// whole while the listings clip. +func TestContinuationIsTheTaskWithoutItsFlags(t *testing.T) { + root := t.TempDir() + mkdirAll(t, filepath.Join(root, "plans")) + path := filepath.Join(root, "plans", "p.md") + write(t, path, `# P + +## Tasks + +- [ ] 1.1 (TDD) A task whose description runs on + and keeps running onto a second line + _Depends 1.2_ + _Priority 2_ +- [ ] 1.2 (Unit) Another +`) + a, err := Load(root, path) + if err != nil { + t.Fatalf("Load: %v", err) + } + task, ok := a.Task("1.1") + if !ok { + t.Fatal("1.1 did not parse") + } + + cont := strings.Join(task.Continuation(), "\n") + if !strings.Contains(cont, "keeps running onto a second line") { + t.Errorf("Continuation dropped the description: %q", cont) + } + for _, flag := range []string{"_Depends", "_Priority"} { + if strings.Contains(cont, flag) { + t.Errorf("Continuation kept %s, so a searcher would index a flag as prose: %q", flag, cont) + } + } + // Detail is the same text on one line, for a row in a table. + if strings.Contains(task.Detail, "\n") { + t.Errorf("Detail spans lines: %q", task.Detail) + } + // End covers the flags, so removing the task removes them with it. + if !strings.Contains(a.Text(task.Line, task.End), "_Priority 2_") { + t.Errorf("the task's region stops short of its flags:\n%s", a.Text(task.Line, task.End)) + } + + // ParseTasks is the same grammar reachable without an Artifact, and it must + // agree with it — a reader that disagreed with the validator about what a task + // is would be worse than no reader. + direct := ParseTasks(a.Doc()) + if len(direct) != len(a.Tasks) { + t.Errorf("ParseTasks found %d tasks, Load found %d", len(direct), len(a.Tasks)) + } +} + +// The forms that the existing address test does not reach, and the collisions the +// resolution order exists to settle. +func TestFindSettlesTheFormsThatCouldCollide(t *testing.T) { + a, _ := load(t) + + // A bare number is a task before it is a line, because a caller who means a + // line writes the L. + task, err := a.Find("1.2") + if err != nil { + t.Fatalf("Find: %v", err) + } + if !strings.Contains(a.Text(task.Line, task.End), "1.2") { + t.Errorf("Find(\"1.2\") resolved to %+v, which does not contain the task", task) + } + if got := task.Lines(); got != task.End-task.Line+1 { + t.Errorf("Lines() = %d for %d..%d", got, task.Line, task.End) + } + + // A single L is a one-line range rather than an error. + one, err := a.Find("L1") + if err != nil { + t.Fatalf("Find(\"L1\"): %v", err) + } + if one.Lines() != 1 { + t.Errorf("Find(\"L1\") covers %d lines", one.Lines()) + } + + // A section by title as written, not only by slug. + if _, err := a.Find("Why"); err != nil { + t.Errorf("Find by heading text: %v", err) + } + + for _, ref := range []string{"", "#no-such-section", "specs/nothing/", "notes:99", "R1.1"} { + if got, err := a.Find(ref); err == nil { + t.Errorf("Find(%q) resolved to %+v in a plan that has no such thing", ref, got) + } + } +} + +// Requirement is the lookup a trace is built on, and the id is matched without +// regard to case because a spec writes R1.2 and a citation sometimes writes r1.2. +func TestRequirementLookupIgnoresCase(t *testing.T) { + root := t.TempDir() + mkdirAll(t, filepath.Join(root, "specs", "auth")) + path := filepath.Join(root, "specs", "auth", "requirements.md") + write(t, path, `# Auth + +## Requirements + +- **R1.1** WHEN a token expires THE SYSTEM SHALL refuse the request +- **R1.2** WHILE a session is open THE SYSTEM SHALL refresh the token +`) + a, err := Load(root, path) + if err != nil { + t.Fatalf("Load: %v", err) + } + if len(a.Requirements) != 2 { + t.Fatalf("parsed %d requirements: %+v", len(a.Requirements), a.Requirements) + } + for _, id := range []string{"R1.2", "r1.2"} { + got, ok := a.Requirement(id) + if !ok { + t.Errorf("Requirement(%q) not found", id) + continue + } + if !strings.Contains(got.Text, "refresh the token") { + t.Errorf("Requirement(%q) = %+v", id, got) + } + } + if _, ok := a.Requirement("R9.9"); ok { + t.Error("Requirement found an id the spec does not define") + } +} + +// Migration has to write a status into a plan that may never have had a +// frontmatter block at all, so both halves are exported and both have to work on a +// file with none. +func TestEnsureAndSetFrontmatterOnAFileWithNone(t *testing.T) { + lines := []string{"# P", "", "## Why", "", "Because."} + + got, n := EnsureFrontmatter(lines) + if n < 2 { + t.Fatalf("EnsureFrontmatter reported a %d-line block: %v", n, got) + } + if got[0] != "---" { + t.Errorf("the block does not open with a delimiter: %v", got) + } + + got, n = SetFrontmatterKey(got, n, KeyStatus, StatusApproved) + joined := strings.Join(got, "\n") + if !strings.Contains(joined, KeyStatus+": "+StatusApproved) { + t.Errorf("the key was not written:\n%s", joined) + } + if !strings.Contains(joined, "# P") { + t.Errorf("the document was lost:\n%s", joined) + } + + // Writing the same key again replaces it rather than adding a second line. + got, _ = SetFrontmatterKey(got, n, KeyStatus, "draft") + if strings.Count(strings.Join(got, "\n"), KeyStatus+":") != 1 { + t.Errorf("the key is recorded twice:\n%s", strings.Join(got, "\n")) + } +} + +// Sorted as text `1.10` comes before `1.9`, so a loop that asked for the next task +// got the tenth before the ninth as soon as a group grew past nine items. This is +// a correction rather than a preference, so it is pinned component by component. +func TestCompareNumbersOrdersLikeAReader(t *testing.T) { + for _, tc := range []struct { + a, b string + want int + }{ + {"1.9", "1.10", -1}, + {"1.10", "1.9", 1}, + {"1.2", "1.2", 0}, + {"1.2", "2.1", -1}, + {"2", "10", -1}, + {"1.2.3", "1.2.10", -1}, + // A shorter number sorts before a longer one that shares its prefix: 1.2 + // is the group 1.2.1 lives in. + {"1.2", "1.2.1", -1}, + {"1.2.1", "1.2", 1}, + // A component that is not a number falls back to text rather than to a + // panic, because the parser accepts what the file says. + {"1.a", "1.b", -1}, + {"1.a", "1.a", 0}, + } { + got := CompareNumbers(tc.a, tc.b) + if (got < 0) != (tc.want < 0) || (got > 0) != (tc.want > 0) { + t.Errorf("CompareNumbers(%q, %q) = %d, want %d", tc.a, tc.b, got, tc.want) + } + } +} + +// Group is what `--group` filters on, and Summary is one task on one line — a +// 61-line task exists, and printing it in a list would defeat the point of the +// list. +func TestGroupAndSummary(t *testing.T) { + for number, want := range map[string]string{ + "1.2": "1", + "1.2.3": "1.2", + "12": "12", + "": "", + } { + if got := (Task{Number: number}).Group(); got != want { + t.Errorf("Task{%q}.Group() = %q, want %q", number, got, want) + } + } + + task := Task{Text: "Build the parser, and prove with a test that it reads a fenced block"} + if got := task.Summary(0); got != task.Text { + t.Errorf("Summary(0) = %q, want the whole line", got) + } + if got := task.Summary(-1); got != task.Text { + t.Errorf("Summary(-1) = %q, want the whole line", got) + } + short := task.Summary(20) + if len([]rune(short)) > 20 { + t.Errorf("Summary(20) = %q, which is %d runes", short, len([]rune(short))) + } + if short == task.Text { + t.Errorf("Summary(20) did not clip: %q", short) + } +} + +// The searcher ranks addressable regions rather than lines, so a hit comes back as +// something `show` accepts. An empty query is no hits rather than every hit. +func TestSearchRanksRegionsAndRefusesNothing(t *testing.T) { + a, _ := load(t) + + hits, err := Search([]*Artifact{a}, "atomic writes", SearchOpts{}) + if err != nil { + t.Fatalf("Search: %v", err) + } + if len(hits) == 0 { + t.Fatal("a term the plan contains matched nothing") + } + for _, h := range hits { + if h.Ref == "" { + t.Errorf("a hit is not addressable: %+v", h) + continue + } + if _, err := a.Find(h.Ref); err != nil { + t.Errorf("Search returned %q, which Find cannot resolve: %v", h.Ref, err) + } + } + + // A query with no terms in it is no hits rather than everything: a caller that + // got the whole corpus back for an empty string would put it all in context. + for _, empty := range []string{"", " ", "!!!"} { + if hits, err := Search([]*Artifact{a}, empty, SearchOpts{}); err != nil || len(hits) != 0 { + t.Errorf("Search(%q) = %d hits, %v", empty, len(hits), err) + } + } + + // A regex that will not compile is an error rather than zero hits, which would + // read as "nothing matches". + if _, err := Search([]*Artifact{a}, "(unclosed", SearchOpts{Regex: true}); err == nil { + t.Error("a malformed regex returned no error") + } +} + +// The regression CI found on macOS and Windows and this machine could not: both +// sides have to resolve the same way, or a path that is plainly inside the +// workspace comes back as an escape. +// +// It fires wherever the root's spelling is not its resolved one — /var against +// /private/var on macOS, an 8.3 short name on Windows — and only for a target that +// is not on disk yet, which is the ordinary case for a file about to be written. +func TestWithinComparesBothSidesTheSameWay(t *testing.T) { + root := t.TempDir() + + // The deepest existing ancestor is the root itself here, and everything below + // it is invented. All of it is inside. + for _, in := range []string{ + filepath.Join(root, "not-yet.md"), + filepath.Join(root, "plans", "not-yet.md"), + filepath.Join(root, "a", "b", "c", "d", "not-yet.md"), + } { + if !Within(root, in) { + t.Errorf("Within(root, %q) = false for a path inside the workspace", in) + } + } + + // And an invented path that climbs out is still out: `..` is resolved away by + // Abs before any of this runs, so nothing below depends on it existing. + for _, out := range []string{ + filepath.Join(root, "..", "sibling", "not-yet.md"), + filepath.Join(root, "plans", "..", "..", "escape.md"), + } { + if Within(root, out) { + t.Errorf("Within(root, %q) = true for a path outside the workspace", out) + } + } + + // A root that does not exist either: both sides fall back together, so the + // comparison is still lexical-but-symmetric rather than one of each. + missing := filepath.Join(root, "no-such-workspace") + if !Within(missing, filepath.Join(missing, "plans", "p.md")) { + t.Error("a path under a root that does not exist was called an escape") + } + if Within(missing, filepath.Join(root, "elsewhere.md")) { + t.Error("a sibling of a root that does not exist was called inside it") + } +} + +// The same regression stated as the condition that actually produces it: a root +// reached through a symlink, which is what /var is on macOS and what a workspace +// under a linked path is anywhere. Resolving the root and not the target put the +// two on opposite sides of the link. +func TestWithinFollowsARootReachedThroughASymlink(t *testing.T) { + real := t.TempDir() + link := filepath.Join(t.TempDir(), "link") + if err := os.Symlink(real, link); err != nil { + t.Skip("this platform will not create a symlink here: " + err.Error()) + } + + for _, in := range []string{ + filepath.Join(link, "not-yet.md"), + filepath.Join(link, "plans", "not-yet.md"), + } { + if !Within(link, in) { + t.Errorf("Within(%q, %q) = false, though the target is under the root as given", link, in) + } + } + // A file that does exist under the link resolves to the real directory on both + // sides, and is inside either way. + existing := filepath.Join(real, "here.md") + write(t, existing, "# Here\n") + if !Within(link, filepath.Join(link, "here.md")) { + t.Error("an existing file under the linked root was called an escape") + } + if !Within(link, existing) { + t.Error("the same file named through the real path was called an escape") + } +} + +// resolveExisting is what makes the two sides comparable, and it is testable +// without a symlink: for a path that exists it must agree with EvalSymlinks, and +// for one that does not it must return that same resolved ancestor with the rest +// re-attached — never the unresolved spelling it was handed. +func TestResolveExistingReattachesWhatIsNotThereYet(t *testing.T) { + dir := t.TempDir() + resolved, err := filepath.EvalSymlinks(dir) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + + if got := resolveExisting(dir); got != resolved { + t.Errorf("resolveExisting on an existing directory = %q, want %q", got, resolved) + } + + for _, rest := range []string{ + "not-yet.md", + filepath.Join("plans", "not-yet.md"), + filepath.Join("a", "b", "c", "not-yet.md"), + } { + want := filepath.Join(resolved, rest) + if got := resolveExisting(filepath.Join(dir, rest)); got != want { + t.Errorf("resolveExisting(%q) = %q, want %q", rest, got, want) + } + } + + // Nothing on this path exists, so there is no ancestor to resolve and the + // answer is what it was given — which keeps both sides falling back together. + nowhere := filepath.Join(string(filepath.Separator), "no-such-root-anywhere", "x.md") + if got := resolveExisting(nowhere); got != nowhere { + t.Errorf("resolveExisting on a path with no existing ancestor = %q, want %q", got, nowhere) + } +} diff --git a/internal/assets/templates/devcontainer/Dockerfile b/internal/assets/templates/devcontainer/Dockerfile index 8b33acb..29a17e2 100644 --- a/internal/assets/templates/devcontainer/Dockerfile +++ b/internal/assets/templates/devcontainer/Dockerfile @@ -36,6 +36,25 @@ RUN apt-get update \ && apt-get install -y --no-install-recommends gh \ && rm -rf /var/lib/apt/lists/* +# The workspace is a bind mount from the host, so its files arrive owned by a uid +# this container does not have, and git refuses a repository it thinks belongs to +# somebody else — the ownership check that exists because a differently-owned .git +# can carry a config or a hook that runs on the next ordinary git command +# (CVE-2022-24765). Scoped to where devcontainers mount a workspace and no further: +# this container forwards GH_TOKEN and mounts the agent's credential volume, so a +# bare `*` here would hand the next crafted .git in a dependency tree exactly those. +# +# Set in the image rather than in postCreateCommand, and that is forced from both +# ends: --global writes ~/.gitconfig, which is the read-only bind that carries your +# identity in ("Device or resource busy", which failed postCreate, `devcontainer up` +# and the whole sandbox, back onto the host), while --system writes /etc/gitconfig, +# which postCreate's non-root user cannot. The build runs as root and can. +# +# The path is a literal because ${containerWorkspaceFolder} is not resolved at build +# time. A workspace mounted somewhere else gets git's ordinary ownership message and +# one `git config --global --add safe.directory ` to run — visible, not silent. +RUN git config --system --add safe.directory '/workspaces/*' + # RTK is the slow layer: it is a Rust build, so this installs a toolchain, builds # one binary and throws the toolchain away. Delete this block if you would rather # have fast rebuilds than filtered command output — nothing else depends on it, @@ -50,7 +69,6 @@ RUN export CARGO_HOME=/tmp/cargo RUSTUP_HOME=/tmp/rustup \ && "$CARGO_HOME/bin/cargo" install --git https://github.com/rtk-ai/rtk --locked \ && install -m 0755 "$CARGO_HOME/bin/rtk" /usr/local/bin/rtk \ && rm -rf "$CARGO_HOME" "$RUSTUP_HOME" - # <<< project toolchain — /scc-init fills this in >>> # A Go project needs golang; a Python project needs python and its package # manager; a Node project needs whatever the one above is not. Install what `scc diff --git a/internal/assets/templates/devcontainer/devcontainer.json b/internal/assets/templates/devcontainer/devcontainer.json index 39e526c..33259de 100644 --- a/internal/assets/templates/devcontainer/devcontainer.json +++ b/internal/assets/templates/devcontainer/devcontainer.json @@ -45,7 +45,12 @@ "GITHUB_TOKEN": "${localEnv:GITHUB_TOKEN}" }, - "postCreateCommand": "scc version && git config --global --add safe.directory ${containerWorkspaceFolder}", + // Just a smoke test that the agent's toolchain is on PATH. Nothing here writes + // git config: ~/.gitconfig is the read-only bind above, and `git config --global` + // against it fails with "Device or resource busy" — which failed postCreate, which + // failed `devcontainer up`, which dropped the launch back onto the host outside any + // sandbox at all. safe.directory is set in the image instead, with --system. + "postCreateCommand": "scc version", "customizations": { "vscode": { diff --git a/internal/cli/check_test.go b/internal/cli/check_test.go index 78513d0..6146752 100644 --- a/internal/cli/check_test.go +++ b/internal/cli/check_test.go @@ -489,3 +489,94 @@ func skipRest(t *testing.T, root, keep string) { } } } + +// A finding about the configuration points at the manifest, and it names it +// relative to the workspace root — which is what every other command's relPath +// answers. Against the working directory the same file came back as +// `.claude/scc-manifest.json` from the root and `../.claude/scc-manifest.json` +// from `specs/`, one field naming one file two ways. +func TestTheWrittenManifestsAreNamedFromTheRoot(t *testing.T) { + root := initWorkspace(t) + + stdout, stderr, code := run(t, "check", "set", "build", "go build ./...", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("check set: exit %d (%s)", code, stderr) + } + var doc struct { + Wrote []string `json:"files"` + } + decode(t, stdout, &doc) + if len(doc.Wrote) == 0 { + t.Fatalf("check set reported no files written: %s", stdout) + } + for _, p := range doc.Wrote { + if filepath.IsAbs(p) { + t.Errorf("a written path is absolute: %q", p) + } + if strings.HasPrefix(p, "..") { + t.Errorf("a written path escapes the workspace: %q", p) + } + if _, err := os.Stat(filepath.Join(root, filepath.FromSlash(p))); err != nil { + t.Errorf("the reported path does not resolve under the root: %q", p) + } + } +} + +// The four gates are a closed set, and a fifth name is refused rather than +// recorded under a key nothing reads. `skipped` is a word rather than an absent +// key, so declining one and clearing it are different acts with different +// consequences. +func TestCheckSetSkipAndClearAreThreeDifferentActs(t *testing.T) { + root := initWorkspace(t) + + if _, _, code := run(t, "check", "set", "typecheck", "tsc --noEmit", "--root", root); code == ExitOK { + t.Error("a gate this project does not have was recorded") + } + if _, _, code := run(t, "check", "skip", "typecheck", "--root", root); code == ExitOK { + t.Error("a gate this project does not have was declined") + } + // And `set` with no command is a usage error rather than an empty recording, + // which reads back as "nobody has decided". + if _, _, code := run(t, "check", "set", "lint", "--root", root); code == ExitOK { + t.Error("`check set lint` with no command exited 0") + } + + if _, stderr, code := run(t, "check", "skip", "format", "--root", root); code != ExitOK { + t.Fatalf("check skip: exit %d (%s)", code, stderr) + } + var doc struct { + Commands map[string]string `json:"commands"` + Floor float64 `json:"floor"` + } + stdout, _, code := run(t, "check", "show", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("check show: exit %d", code) + } + decode(t, stdout, &doc) + if doc.Commands["format"] != "skipped" { + t.Errorf("a declined gate reads back as %q, want the word that means declined", doc.Commands["format"]) + } + + // Clearing puts it back to undecided, which is a different state: the + // validator is silent about a declined gate and reports an undecided one. + if _, stderr, code := run(t, "check", "clear", "format", "--root", root); code != ExitOK { + t.Fatalf("check clear: exit %d (%s)", code, stderr) + } + stdout, _, code = run(t, "check", "show", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("check show: exit %d", code) + } + var cleared struct { + Commands map[string]string `json:"commands"` + Floor float64 `json:"floor"` + } + decode(t, stdout, &cleared) + if cleared.Commands["format"] == "skipped" { + t.Error("clear left the gate declined rather than undecided") + } + // The floor has a working default and is reported even when nobody set one, + // so a reader never has to know what the default is. + if cleared.Floor <= 0 { + t.Errorf("floor = %v, want the default this workspace is held to", cleared.Floor) + } +} diff --git a/internal/cli/graph_scope_more_test.go b/internal/cli/graph_scope_more_test.go new file mode 100644 index 0000000..45c01b1 --- /dev/null +++ b/internal/cli/graph_scope_more_test.go @@ -0,0 +1,247 @@ +package cli + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// withGraphCapture stands in for the capturing runner, so a fan-out test can state +// what each root printed instead of needing CodeGraph installed. +func withGraphCapture(t *testing.T, out string, code int) *[]string { + t.Helper() + var dirs []string + orig := graphCapture + graphCapture = func(bin, dir string, args []string, buf *bytes.Buffer) int { + dirs = append(dirs, dir) + buf.WriteString(out) + return code + } + t.Cleanup(func() { graphCapture = orig }) + return &dirs +} + +// A scope is one graph per directory rather than one graph filtered, so every +// command answers once per root — and N JSON documents concatenated are not a +// document, so --json comes back as an array of sections. +func TestGraphJSONOverAScopeIsAnArrayOfSections(t *testing.T) { + root := initWorkspace(t) + isolatedPath(t, "codegraph") + for _, dir := range []string{"backend", "frontend"} { + if err := os.MkdirAll(filepath.Join(root, dir, "src", ".codegraph"), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + } + if _, stderr, code := run(t, "graph", "scope", "set", "backend/src", "frontend/src", "--root", root); code != ExitOK { + t.Fatalf("scope set: exit %d (%s)", code, stderr) + } + + dirs := withGraphCapture(t, `{"symbols": 3}`, ExitOK) + stdout, _, code := run(t, "graph", "status", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("graph status --json: exit %d", code) + } + if len(*dirs) != 2 { + t.Fatalf("the command ran in %d roots, want one per scoped tree: %v", len(*dirs), *dirs) + } + + var doc []struct { + Path string `json:"path"` + Output map[string]int `json:"output"` + Raw string `json:"raw"` + Exit int `json:"exit"` + } + decode(t, stdout, &doc) + if len(doc) != 2 { + t.Fatalf("the document has %d sections, want one per root: %+v", len(doc), doc) + } + for _, s := range doc { + if s.Path == "" { + t.Errorf("a section does not say which root it came from: %+v", s) + } + // The root's own document is passed through rather than re-encoded: + // nesting a document is not reading one. + if s.Output["symbols"] != 3 { + t.Errorf("a section did not carry the root's own output: %+v", s) + } + } + + // A root whose output is not JSON after all degrades to a string rather than + // breaking the array — a CodeGraph that prints a warning above its document is + // still something a consumer can read. + withGraphCapture(t, "warning: something\n", ExitOK) + stdout, _, code = run(t, "graph", "status", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("graph status --json: exit %d", code) + } + doc = doc[:0] + decode(t, stdout, &doc) + for _, s := range doc { + if s.Raw == "" { + t.Errorf("output that is not JSON did not come back as a string: %+v", s) + } + } + + // A failing root carries its exit code into its own section, and the command's + // exit is the failure rather than the last root's. + withGraphCapture(t, "", ExitError) + if _, _, code := run(t, "graph", "status", "--root", root, "--json"); code == ExitOK { + t.Error("a failing root reported success") + } +} + +// Scoped or not, the scope is reported: "the graph is current" means something +// different when it is two graphs, so a run that scoped says so. +func TestGraphScopeShowSaysWhatIsRecorded(t *testing.T) { + root := initWorkspace(t) + + stdout, _, code := run(t, "graph", "scope", "--root", root) + if code != ExitOK { + t.Fatalf("graph scope: exit %d", code) + } + if stdout == "" { + t.Error("an empty scope said nothing, so nobody learns it is the whole workspace") + } + + var doc struct { + Scope []string `json:"scope"` + } + stdout, _, code = run(t, "graph", "scope", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("graph scope --json: exit %d", code) + } + decode(t, stdout, &doc) + // [] rather than null: the field is a list of directories, and an empty scope + // is still a list. + if doc.Scope == nil { + t.Error("an empty scope came back as null rather than as an empty list") + } +} + +// A pattern that matches nothing is named and skipped; every pattern matching +// nothing stops the run instead. Falling back to the whole workspace there would +// be the opposite of what the scope asked for, and an expensive way to find out +// about a typo. +func TestAScopeThatMatchesNothingStopsTheRun(t *testing.T) { + root := initWorkspace(t) + isolatedPath(t, "codegraph") + if err := os.MkdirAll(filepath.Join(root, "backend", "src"), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + + // One good and one missing: the missing one is named, and the run goes on with + // what is there. + if _, stderr, code := run(t, "graph", "scope", "set", "backend/src", "--root", root); code != ExitOK { + t.Fatalf("scope set: exit %d (%s)", code, stderr) + } + if err := os.WriteFile(filepath.Join(root, ".claude", "scc-manifest.json"), + []byte(scopedManifest(t, root, "backend/src", "frontend/src")), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + withGraphExec(t, ExitOK) + stdout, stderr, _ := run(t, "graph", "sync", "--root", root) + if !strings.Contains(stdout+stderr, "frontend/src") { + t.Errorf("the missing tree was not named:\n%s%s", stdout, stderr) + } + + // Every pattern missing: the run stops, and says how to fix or clear it. + if err := os.WriteFile(filepath.Join(root, ".claude", "scc-manifest.json"), + []byte(scopedManifest(t, root, "nowhere/at-all")), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + stdout, stderr, code := run(t, "graph", "sync", "--root", root) + if code == ExitOK { + t.Error("a scope that matches nothing indexed the whole workspace anyway") + } + if !strings.Contains(stdout+stderr, "scope set") { + t.Errorf("the error does not say how to fix it:\n%s%s", stdout, stderr) + } +} + +// scopedManifest is this workspace's manifest with the graph scope replaced, so a +// test can state a scope that names trees which are not there — which `scope set` +// itself refuses, and rightly. +func scopedManifest(t *testing.T, root string, dirs ...string) string { + t.Helper() + raw, err := os.ReadFile(filepath.Join(root, ".claude", "scc-manifest.json")) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + var doc map[string]any + if err := json.Unmarshal(raw, &doc); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + list := make([]any, 0, len(dirs)) + for _, d := range dirs { + list = append(list, d) + } + doc["codegraph"] = list + out, err := json.Marshal(doc) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + return string(out) +} + +// A scope command with nothing recorded and one with a scope both have to answer, +// and `clear` has to put it back to the whole workspace rather than to an empty +// list nothing indexes. +func TestGraphScopeSetAndClearRoundTrip(t *testing.T) { + root := initWorkspace(t) + for _, dir := range []string{"backend/src", "frontend/src"} { + if err := os.MkdirAll(filepath.Join(root, filepath.FromSlash(dir)), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + } + + if _, stderr, code := run(t, "graph", "scope", "set", "backend/src/*", "frontend/src", "--root", root); code != ExitOK { + t.Fatalf("scope set: exit %d (%s)", code, stderr) + } + var doc struct { + Scope []string `json:"scope"` + } + stdout, _, code := run(t, "graph", "scope", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("scope --json: exit %d", code) + } + decode(t, stdout, &doc) + if len(doc.Scope) != 2 { + t.Fatalf("scope = %v, want both trees", doc.Scope) + } + // What people write is `backend/src/*` and what that means is the directory: + // the pattern is recorded as typed and normalized when the roots are resolved, + // so a run indexes one tree rather than failing to match a directory. + isolatedPath(t, "codegraph") + for _, dir := range []string{"backend/src", "frontend/src"} { + if err := os.MkdirAll(filepath.Join(root, filepath.FromSlash(dir), ".codegraph"), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + } + dirs := withGraphCapture(t, `{"symbols": 1}`, ExitOK) + if _, stderr, code := run(t, "graph", "status", "--root", root, "--json"); code != ExitOK { + t.Fatalf("graph status: exit %d (%s)", code, stderr) + } + if len(*dirs) != 2 { + t.Errorf("the trailing glob did not resolve to a directory: %v", *dirs) + } + + if _, stderr, code := run(t, "graph", "scope", "clear", "--root", root); code != ExitOK { + t.Fatalf("scope clear: exit %d (%s)", code, stderr) + } + stdout, _, code = run(t, "graph", "scope", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("scope --json: exit %d", code) + } + cleared := doc + decode(t, stdout, &cleared) + if len(cleared.Scope) != 0 { + t.Errorf("clear left %v recorded", cleared.Scope) + } + // Empty is a list, not null: the field is a list of directories. + if cleared.Scope == nil { + t.Error("a cleared scope came back as null") + } +} diff --git a/internal/cli/graph_test.go b/internal/cli/graph_test.go index 022dd43..1c5b002 100644 --- a/internal/cli/graph_test.go +++ b/internal/cli/graph_test.go @@ -329,3 +329,36 @@ func TestGraphScopeSurvivesInit(t *testing.T) { t.Errorf("codegraph = %v, want it carried across init", m.Codegraph) } } + +// The whole command is the binary, so a missing CodeGraph is a hard error here — +// unlike the launch path, where a graph is an enhancement and the agent starts +// anyway. The error has to name the install, because the next thing the reader +// does is install it. +func TestGraphRefusesWithoutTheBinaryAndNamesTheInstall(t *testing.T) { + root := initWorkspace(t) + isolatedPath(t) + + for _, sub := range [][]string{ + {"sync"}, + {"build"}, + {"query", "renderTask"}, + {"explore", "how does a plan get validated"}, + } { + args := append(append([]string{"graph"}, sub...), "--root", root) + stdout, stderr, code := run(t, args...) + if code == ExitOK { + t.Errorf("`scc graph %s` with no binary exited 0", strings.Join(sub, " ")) + continue + } + if !strings.Contains(stdout+stderr, "codegraph") { + t.Errorf("`scc graph %s` did not name the binary:\n%s%s", strings.Join(sub, " "), stdout, stderr) + } + } + + // npm is the only installer scc will run, and the hint names it rather than + // the piped script CodeGraph's own docs lead with. + stdout, stderr, _ := run(t, "graph", "sync", "--root", root) + if strings.Contains(stdout+stderr, "| sh") || strings.Contains(stdout+stderr, "iex") { + t.Errorf("the hint pipes a remote script into a shell:\n%s%s", stdout, stderr) + } +} diff --git a/internal/cli/hooks_agent_more_test.go b/internal/cli/hooks_agent_more_test.go new file mode 100644 index 0000000..12271ca --- /dev/null +++ b/internal/cli/hooks_agent_more_test.go @@ -0,0 +1,174 @@ +package cli + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/protonspy/spec-claude-code/internal/codegraph" + "github.com/protonspy/spec-claude-code/internal/finding" + "github.com/protonspy/spec-claude-code/internal/git" + "github.com/protonspy/spec-claude-code/internal/hooks" +) + +// The Stop stage stopped re-indexing on every turn: syncGraph compares the graph's +// mtime against the files this branch touched — a stat per changed file rather +// than a walk — and skips when nothing is newer. +// +// It errs toward syncing, and the reason is asymmetric: an unnecessary index costs +// a subprocess, while a skipped one leaves the agent an index that answers +// confidently about code that changed. +func TestStaleErrsTowardSyncing(t *testing.T) { + dir := t.TempDir() + + // No graph to stat at all: nothing to be stale about, and the answer is still + // "sync" rather than a claim the index is current. + if !stale(dir, []git.Change{{Path: "a.go"}}) { + t.Error("a workspace with no graph reported a current index") + } + + graph := filepath.Join(dir, codegraph.Dir) + if err := os.MkdirAll(graph, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + // No list of changed files: the question cannot be answered, so sync. + if !stale(dir, nil) { + t.Error("an unanswerable question reported a current index") + } + + // A file older than the index: nothing to do. + old := filepath.Join(dir, "old.go") + if err := os.WriteFile(old, []byte("package x\n"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + past := time.Now().Add(-time.Hour) + if err := os.Chtimes(old, past, past); err != nil { + t.Fatalf("Chtimes: %v", err) + } + if stale(dir, []git.Change{{Path: "old.go"}}) { + t.Error("a file older than the index reported the index stale") + } + + // A file the branch touched that no longer exists is skipped rather than + // counted: a deleted file cannot be newer than anything. + if stale(dir, []git.Change{{Path: "gone.go"}}) { + t.Error("a file that is not there reported the index stale") + } + + // And one written after the index is exactly what a sync is for. + fresh := filepath.Join(dir, "fresh.go") + if err := os.WriteFile(fresh, []byte("package x\n"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + future := time.Now().Add(time.Hour) + if err := os.Chtimes(fresh, future, future); err != nil { + t.Fatalf("Chtimes: %v", err) + } + if !stale(dir, []git.Change{{Path: "old.go"}, {Path: "fresh.go"}}) { + t.Error("a file newer than the index did not report it stale") + } +} + +// The Stop stage reports findings, and it intersects them with what this branch +// touched: a migrated plan or a stale ADR the task never went near asked the +// agent, every turn, to go and fix a file it had no business in. +// +// Its answer when git cannot say is the whole set, unfiltered — the filter exists +// to stop it nagging about work nobody here did, and a workspace where the +// question cannot be asked is one where every finding is as likely as not to be +// this session's. +func TestTouchedHereFallsBackToTheWholeSet(t *testing.T) { + set := &finding.Set{} + set.Addf("plans/other.md", 3, "plan.unknown-section", "a section nobody agreed to") + set.Addf("docs/adr/0001-x.md", 1, "adr.no-status", "no status") + + // Not a repository: the question cannot be asked, so nothing is filtered out. + if got := touchedHere(t.TempDir(), set); got.Len() != set.Len() { + t.Errorf("outside a repository the set was filtered to %d of %d", got.Len(), set.Len()) + } + + // An empty set stays empty rather than becoming nil, so the caller's Len() and + // Sorted() keep working. + empty := &finding.Set{} + if got := touchedHere(t.TempDir(), empty); got == nil || !got.Empty() { + t.Errorf("an empty set came back as %+v", got) + } + if got := touchedHere(t.TempDir(), nil); got != nil { + t.Errorf("a nil set came back as %+v", got) + } +} + +// A finding that quotes a test log contributes one row to the report rather than +// twelve: this is context the agent pays for on its next turn. +func TestFirstLineClipsAMultiLineMessage(t *testing.T) { + if got := firstLine("one line only"); got != "one line only" { + t.Errorf("firstLine = %q", got) + } + got := firstLine("the gate failed\n--- FAIL: TestX\n x_test.go:1: nope\n") + if got != "the gate failed" { + t.Errorf("firstLine = %q, want just the first line", got) + } + if firstLine("") != "" { + t.Error("firstLine invented text for an empty message") + } +} + +// The Stop stage intersects findings with what this branch touched: a migrated +// plan or a stale ADR the task never went near asked the agent, every turn, to go +// and fix a file it had no business in. +func TestTouchedHereKeepsOnlyThisBranchesFiles(t *testing.T) { + if !git.Found(git.Bin) { + t.Skip("git is not on PATH") + } + root := gitWorkspace(t) + gitIn := func(args ...string) { + t.Helper() + cmd := exec.Command(git.Bin, args...) + cmd.Dir = root + // The workspace has scc own hooks installed, and the fixtures here are + // deliberately malformed plans: the gate is not what is under test. + cmd.Env = append(os.Environ(), hooks.SkipEnv+"=1") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %s: %v %s", strings.Join(args, " "), err, out) + } + } + gitIn("config", "user.email", "t@example.invalid") + gitIn("config", "user.name", "t") + gitIn("config", "commit.gpgsign", "false") + + // A file this branch wrote, and one it did not. + mine := filepath.Join(root, "plans", "mine.md") + theirs := filepath.Join(root, "plans", "theirs.md") + if err := os.MkdirAll(filepath.Dir(mine), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + for _, p := range []string{mine, theirs} { + if err := os.WriteFile(p, []byte("# Plan\n"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + } + gitIn("add", "-A") + gitIn("commit", "-qm", "init") + + gitIn("switch", "-qc", "feat/x") + + // Change only one, so the diff against the base names exactly that file. + if err := os.WriteFile(mine, []byte("# Plan\n\n## Why\n\nBecause.\n"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + set := &finding.Set{} + set.Addf("plans/mine.md", 3, "plan.unknown-section", "a section nobody agreed to") + set.Addf("plans/theirs.md", 1, "plan.empty", "a plan with no checklist") + + got := touchedHere(root, set) + if got.Len() != 1 { + t.Fatalf("touchedHere kept %d of 2 findings: %+v", got.Len(), got.Sorted()) + } + if f := got.Sorted()[0]; !strings.Contains(f.File, "mine.md") { + t.Errorf("touchedHere kept the wrong finding: %+v", f) + } +} diff --git a/internal/cli/hooks_test.go b/internal/cli/hooks_test.go index 279f292..837a965 100644 --- a/internal/cli/hooks_test.go +++ b/internal/cli/hooks_test.go @@ -317,3 +317,58 @@ func TestPrePushIsSilentWithoutATestCommand(t *testing.T) { t.Errorf("the hook demanded a test command nobody asked it to check: %q", stdout+stderr) } } + +// The hooks are installable in a repository that follows scc's rules without being +// one of its workspaces — scc's own, for one — where the artifact validators have +// nothing to read and the rule about what a commit may say still binds. +// +// That narrowing is not a courtesy: without it the pre-commit hook would report +// findings about a `specs/` tree that does not exist, in every repository that +// merely installed it. +func TestTheGateNarrowsOutsideAWorkspace(t *testing.T) { + dir := gitInit(t) + + // Not a workspace: the record checks run and the artifact ones have nothing to + // say, so a fresh repository is clean rather than full of findings. + if _, stderr, code := run(t, "hooks", "run", "pre-commit", "--root", dir); code != ExitOK { + t.Errorf("pre-commit in a plain repository exited %d (%s)", code, stderr) + } + + // The same stage in a workspace runs everything, and a workspace scc just + // scaffolded is clean — a validator that fired on scc own output is the + // worst bug in the product. + root := gitWorkspace(t) + if _, stderr, code := run(t, "hooks", "run", "pre-commit", "--root", root); code != ExitOK { + t.Errorf("pre-commit in a fresh workspace exited %d (%s)", code, stderr) + } + + // And the report says the way past itself, because a gate with no documented + // escape is a gate people delete the first time it is wrong. + plan := filepath.Join(root, "plans", "broken.md") + if err := os.MkdirAll(filepath.Dir(plan), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(plan, []byte("# Broken\n\n## Nonsense\n\nNo required sections.\n"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + stdout, stderr, code := run(t, "hooks", "run", "pre-commit", "--root", root) + if code != ExitFindings { + t.Errorf("a workspace with a broken plan exited %d, want %d", code, ExitFindings) + } + if !strings.Contains(stdout+stderr, hooks.SkipEnv) { + t.Errorf("the gate does not name the way past it:\n%s%s", stdout, stderr) + } + + // --json keeps stdout a document and still carries the exit contract. + stdout, _, code = run(t, "hooks", "run", "pre-commit", "--root", root, "--json") + if code != ExitFindings { + t.Errorf("--json exited %d, want %d", code, ExitFindings) + } + var doc struct { + Count int `json:"count"` + } + decode(t, stdout, &doc) + if doc.Count == 0 { + t.Errorf("the document reports no findings though the command exited %d", ExitFindings) + } +} diff --git a/internal/cli/init_test.go b/internal/cli/init_test.go index 18eecb5..1937538 100644 --- a/internal/cli/init_test.go +++ b/internal/cli/init_test.go @@ -387,3 +387,63 @@ func stripJSONComments(s string) string { } return strings.Join(out, "\n") } + +// The seeded container has to actually come up, and the first version of it did not: +// postCreateCommand ran `git config --global --add safe.directory`, ~/.gitconfig is +// the read-only bind that carries your identity in, and git failed with "Device or +// resource busy". That failed postCreate, which failed `devcontainer up`, which +// dropped `scc launch` back onto the host — a run that reported a sandbox and started +// the agent outside one. The setting belongs in the image, where nothing is read-only. +func TestTheSeededContainerDoesNotWriteTheMountedGitConfig(t *testing.T) { + root := initWorkspace(t) + seg := filepath.Join(root, paths.DevcontainerSeg) + + cfg, err := os.ReadFile(filepath.Join(seg, "devcontainer.json")) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + // The mount is what makes the write fail, so the test asserts both halves: a + // seed that stopped binding .gitconfig could write it again safely. + if !strings.Contains(string(cfg), "target=/home/node/.gitconfig") { + t.Fatal("the seeded config no longer binds .gitconfig; this test's premise is gone") + } + for _, line := range strings.Split(string(cfg), "\n") { + if !strings.Contains(line, "postCreateCommand") { + continue + } + if strings.Contains(line, "git config --global") { + t.Errorf("postCreateCommand writes the read-only .gitconfig bind:\n%s", line) + } + } + + docker, err := os.ReadFile(filepath.Join(seg, "Dockerfile")) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if !strings.Contains(string(docker), "git config --system --add safe.directory") { + t.Errorf("the image never marks the bind-mounted workspace safe, so git refuses it:\n%s", docker) + } +} + +// And it marks the workspace safe without marking everything safe. `safe.directory` +// is git's guard against a differently-owned repository whose .git carries a config +// or a hook that runs on the next ordinary command; this container forwards GH_TOKEN +// and mounts the agent's credential volume, so a bare `*` would hand those to the +// next crafted .git in a dependency tree. +func TestTheSeededContainerDoesNotDisableGitsOwnershipCheckWholesale(t *testing.T) { + root := initWorkspace(t) + docker, err := os.ReadFile(filepath.Join(root, paths.DevcontainerSeg, "Dockerfile")) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + for _, line := range strings.Split(string(docker), "\n") { + if !strings.Contains(line, "safe.directory") || strings.HasPrefix(strings.TrimSpace(line), "#") { + continue + } + for _, wildcard := range []string{"safe.directory '*'", `safe.directory "*"`, "safe.directory *"} { + if strings.Contains(line, wildcard) { + t.Errorf("the image trusts every repository in the container:\n%s", line) + } + } + } +} diff --git a/internal/cli/launch_container_test.go b/internal/cli/launch_container_test.go new file mode 100644 index 0000000..55ef10a --- /dev/null +++ b/internal/cli/launch_container_test.go @@ -0,0 +1,133 @@ +package cli + +import ( + "os" + "path/filepath" + "runtime" + + "github.com/protonspy/spec-claude-code/internal/devcontainer" + "strings" + "testing" +) + +// The container is the Windows backend, and every assertion here is about what a +// run reports when it cannot deliver one: the agent still starts, and the line +// saying it is uncontained is the most important thing that run has to say. +// +// It runs only where ai-jail has no backend, because everywhere else the default +// sandbox is the jail and this path is not reached. +func skipWhereTheJailWorks(t *testing.T) { + t.Helper() + if runtime.GOOS != "windows" { + t.Skip("ai-jail is the default sandbox on this platform, so the container path is unreachable") + } +} + +// A workspace with no .devcontainer/ is the first reason there is no container, +// and it is reported rather than guessed around: seeding one is the user's +// decision, not something a launch does on the way past. +func TestLaunchReportsAWorkspaceWithNoContainer(t *testing.T) { + skipWhereTheJailWorks(t) + root := initWorkspace(t) + isolatedPath(t, "claude") + if err := os.RemoveAll(filepath.Join(root, ".devcontainer")); err != nil { + t.Fatalf("RemoveAll: %v", err) + } + + cmd := launchJSON(t, "launch", "claude", "--root", root, "--json", "--no-install") + if cmd.Container == nil { + t.Fatal("the run reported no container at all, so nobody learns it is uncontained") + } + if cmd.Container.Wrapping { + t.Error("a workspace with no .devcontainer reported that it wrapped") + } + if !strings.Contains(cmd.Container.Reason, devcontainer.Dir) { + t.Errorf("the reason does not name the missing directory: %q", cmd.Container.Reason) + } + // The boundary is reported as the lesser one for machines too, not only in + // the human line: a consumer should not have to know the platform. + if !cmd.Container.Weaker { + t.Error("the container report does not say it is the weaker boundary") + } + // And the agent still starts: a sandbox nobody asked for by name does not get + // to stop the thing the user actually asked for. + if cmd.Bin == "" { + t.Error("the run named no binary to start") + } +} + +// `init` seeds .devcontainer/, so a fresh workspace has one — and then the binary +// is the next reason. A plan-only run reports it without installing anything. +func TestLaunchReportsAMissingDevcontainerCLI(t *testing.T) { + skipWhereTheJailWorks(t) + root := initWorkspace(t) + isolatedPath(t, "claude") + + cmd := launchJSON(t, "launch", "claude", "--root", root, "--json", "--no-install") + if cmd.Container == nil { + t.Fatal("the run reported no container at all") + } + if cmd.Container.Wrapping { + t.Error("a run with no devcontainer CLI reported that it wrapped") + } + if cmd.Container.Config == "" { + t.Errorf("the seeded configuration was not found: %+v", cmd.Container) + } + if cmd.Container.Reason == "" { + t.Error("the run gave no reason for starting on the host") + } + if cmd.Container.Install != installSkipped { + t.Errorf("install = %q on a --no-install run", cmd.Container.Install) + } +} + +// --no-sandbox is the deliberate way out, and it contradicts --jail rather than +// one outranking the other: somebody who typed both has not said what they want. +func TestLaunchNoSandboxOptsOutAndContradictsJail(t *testing.T) { + root := initWorkspace(t) + isolatedPath(t, "claude") + + cmd := launchJSON(t, "launch", "claude", "--root", root, "--json", "--no-sandbox", "--no-install") + if cmd.Container != nil || cmd.Jail != nil { + t.Errorf("--no-sandbox still reported a boundary: container %+v, jail %+v", cmd.Container, cmd.Jail) + } + + stdout, stderr, code := run(t, "launch", "claude", "--root", root, "--jail", "--no-sandbox", "--dry-run") + if code != ExitError { + t.Errorf("--jail with --no-sandbox exited %d, want %d", code, ExitError) + } + if !strings.Contains(stdout+stderr, "contradict") { + t.Errorf("the refusal does not say the two flags contradict:\n%s%s", stdout, stderr) + } +} + +// --jail-arg is repeatable, because the alternative — a comma-separated string — +// cannot carry a value containing a comma, which a path or a glob can. +func TestJailArgIsRepeatable(t *testing.T) { + root := initWorkspace(t) + isolatedPath(t, "claude") + + // On a platform with no jail backend this refuses, and on one with a backend + // it needs ai-jail installed. Either way the flag has been parsed by then, + // which is what this is about: the values are collected rather than the last + // one winning. + var r repeatable + for _, v := range []string{"--bind a,b", "--bind c"} { + if err := r.Set(v); err != nil { + t.Fatalf("Set: %v", err) + } + } + if len(r) != 2 { + t.Fatalf("repeatable collected %d values, want both: %v", len(r), r) + } + if !strings.Contains(r.String(), "a,b") { + t.Errorf("a value containing a comma did not survive: %q", r.String()) + } + + // And the flag reaches the command line rather than being silently dropped. + if _, _, code := run(t, "launch", "claude", "--root", root, + "--jail-arg", "--bind a,b", "--jail-arg", "--bind c", "--dry-run", "--no-install"); code == ExitOK { + // A platform with a backend and no binary refuses, which is the contract. + return + } +} diff --git a/internal/cli/launch_degrade_test.go b/internal/cli/launch_degrade_test.go new file mode 100644 index 0000000..6cffb48 --- /dev/null +++ b/internal/cli/launch_degrade_test.go @@ -0,0 +1,160 @@ +package cli + +import ( + "strings" + "testing" + + "github.com/protonspy/spec-claude-code/internal/paths" +) + +// Every way of not getting Headroom degrades to starting the agent bare, and each +// says why: a launch that refused to run because a compression proxy was missing +// would be scc putting its own preference above the thing the user asked for. +// +// The reason is the whole value — silence is how somebody spends a month +// wondering why Headroom never seemed to help. Driven against resolveHeadroom +// directly because `--json` implies "install nothing", so the install-shaped +// branches are not reachable through a plan-only run. +func TestHeadroomDegradesWithAReasonEachTime(t *testing.T) { + for name, tc := range map[string]struct { + bins []string + opts headroomOptions + says string + wraps bool + }{ + "no installer at all": { + bins: nil, + opts: headroomOptions{}, + says: "neither uv nor pip", + }, + "an installer, but nobody to ask": { + bins: []string{"uv"}, + opts: headroomOptions{quiet: true}, + says: "nobody is here", + }, + "--no-install with an installer right there": { + bins: []string{"uv"}, + opts: headroomOptions{noInstall: true}, + says: "not on PATH", + }, + "present, so it wraps": { + bins: []string{"headroom"}, + opts: headroomOptions{quiet: true}, + wraps: true, + }, + } { + t.Run(name, func(t *testing.T) { + isolatedPath(t, tc.bins...) + report := resolveHeadroom(paths.Claude, tc.opts) + if report == nil { + t.Fatal("the run said nothing about Headroom at all") + } + if report.Wrapping != tc.wraps { + t.Errorf("wrapping = %v, want %v (reason: %q)", report.Wrapping, tc.wraps, report.Reason) + } + if tc.wraps { + if report.Reason != "" { + t.Errorf("a wrapping run gave a reason for not wrapping: %q", report.Reason) + } + if report.Install != installPresent { + t.Errorf("install = %q, want %q", report.Install, installPresent) + } + return + } + if !strings.Contains(report.Reason, tc.says) { + t.Errorf("reason = %q, want it to mention %q", report.Reason, tc.says) + } + }) + } + + // Headroom does not wrap every harness, and one it cannot is reported rather + // than silently skipped — the user asked for compression and is not getting it. + isolatedPath(t, "headroom") + if report := resolveHeadroom(paths.Codex, headroomOptions{quiet: true}); report == nil { + t.Error("a harness Headroom may not wrap reported nothing at all") + } + + // And disabled is the one outcome with no report: nobody asked, so there is + // nothing to explain. + if report := resolveHeadroom(paths.Claude, headroomOptions{disabled: true}); report != nil { + t.Errorf("a disabled step still reported %+v", report) + } +} + +// The same contract for the graph, which degrades the same way and for the same +// reason: the agent can still read files, so a missing binary, a declined install +// or a failed index all end in the agent starting anyway with a line saying what +// happened. +func TestTheGraphDegradesWithAReasonEachTime(t *testing.T) { + for name, tc := range map[string]struct { + bins []string + opts graphOptions + says string + }{ + "no npm to install with": { + bins: nil, + opts: graphOptions{quiet: true}, + says: "npm is not on PATH", + }, + "npm, but nobody to ask": { + bins: []string{"npm"}, + opts: graphOptions{quiet: true}, + says: "nobody is here", + }, + "--no-install with npm right there": { + bins: []string{"npm"}, + opts: graphOptions{noInstall: true, quiet: true}, + says: "not on PATH", + }, + "plan-only, which installs and indexes nothing": { + bins: []string{"npm"}, + opts: graphOptions{plan: true, quiet: true}, + says: "not on PATH", + }, + } { + t.Run(name, func(t *testing.T) { + root := initWorkspace(t) + isolatedPath(t, tc.bins...) + report := resolveGraph(root, tc.opts) + if report == nil { + t.Fatal("the run said nothing about the graph at all") + } + if report.Action != graphSkipped { + t.Errorf("action = %q, want %q", report.Action, graphSkipped) + } + if !strings.Contains(report.Reason, tc.says) { + t.Errorf("reason = %q, want it to mention %q", report.Reason, tc.says) + } + // No binary means no usage block either: guidance naming a command the + // machine cannot run is worse than none, because an agent that tries it + // and watches it fail discounts the whole file. + if len(report.Blocks) != 0 { + t.Errorf("a run with no CodeGraph wrote a usage block anyway: %+v", report.Blocks) + } + }) + } + + if report := resolveGraph(t.TempDir(), graphOptions{disabled: true}); report != nil { + t.Errorf("a disabled step still reported %+v", report) + } +} + +// --no-graph and --no-rtk drop their step entirely rather than reporting a skipped +// one: a flag that turned into a report would be scc explaining a decision the +// user already made. +func TestTheOptOutsRemoveTheirStepEntirely(t *testing.T) { + root := initWorkspace(t) + isolatedPath(t, "claude") + + cmd := launchJSON(t, "launch", "claude", "--root", root, "--json", "--no-graph", "--no-rtk") + if cmd.Graph != nil { + t.Errorf("--no-graph still reported a graph step: %+v", cmd.Graph) + } + if cmd.RTK != nil { + t.Errorf("--no-rtk still reported an RTK step: %+v", cmd.RTK) + } + // Headroom is off by default, so it is absent without being asked to be. + if cmd.Headroom != nil && cmd.Headroom.Wrapping { + t.Errorf("a default run wrapped with Headroom: %+v", cmd.Headroom) + } +} diff --git a/internal/cli/map_more_test.go b/internal/cli/map_more_test.go new file mode 100644 index 0000000..7aab051 --- /dev/null +++ b/internal/cli/map_more_test.go @@ -0,0 +1,421 @@ +package cli + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "strings" + "testing" +) + +// specWorkspace is mapWorkspace plus a spec, for the reads that only make sense +// against one: a requirement id, a trace, and an index with both trees in it. +func specWorkspace(t *testing.T) string { + t.Helper() + root := mapWorkspace(t) + dir := filepath.Join(root, "specs", "job-store") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + files := map[string]string{ + "requirements.md": `--- +autonomy: auto +ci: wait +--- + +# Job store + +## Requirements + +- **R1.1** WHEN a job is submitted THE SYSTEM SHALL write it to disk before replying +- **R1.2** WHILE a job is running THE SYSTEM SHALL keep its request id addressable +`, + "design.md": `# Job store — design + +## Storage + +One file per job, written atomically. Satisfies R1.1. + +## Recovery + +The request id recovers an already-paid result after a crash. Satisfies R1.2. +`, + "tasks.md": `# Job store — tasks + +## Tasks + +- [ ] 1.1 (TDD) Write the job file atomically (R1.1) +- [ ] 1.2 (Unit) Recover by request id (R1.2) +`, + } + for name, body := range files { + if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + return root +} + +// The index is what a session reads instead of the workspace. It has to name every +// artifact and say enough about each that the next command is obvious — which is +// the whole reason it exists rather than `ls`. +func TestMapIndexNamesEveryArtifact(t *testing.T) { + root := specWorkspace(t) + + stdout, _, code := run(t, "map", "--root", root) + if code != ExitOK { + t.Fatalf("map: exit %d", code) + } + for _, want := range []string{"sample", "job-store"} { + if !strings.Contains(stdout, want) { + t.Errorf("the index does not mention %q:\n%s", want, stdout) + } + } + + var doc struct { + Artifacts []struct { + Path string `json:"path"` + Kind string `json:"kind"` + Title string `json:"title"` + } `json:"artifacts"` + } + stdout, _, code = run(t, "map", "index", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("map index --json: exit %d", code) + } + decode(t, stdout, &doc) + if len(doc.Artifacts) < 4 { + t.Fatalf("the index lists %d artifacts, want the plan and the spec's three: %+v", + len(doc.Artifacts), doc.Artifacts) + } + kinds := map[string]bool{} + for _, a := range doc.Artifacts { + if a.Path == "" || a.Kind == "" { + t.Errorf("an entry is not fully described: %+v", a) + } + kinds[a.Kind] = true + } + for _, want := range []string{"plan", "requirements", "design", "tasks"} { + if !kinds[want] { + t.Errorf("the index has no %s artifact: %+v", want, doc.Artifacts) + } + } +} + +// A trace answers "what else mentions this", which is the question that otherwise +// costs a read of every file in the spec. +func TestMapTraceFollowsARequirementThroughItsSpec(t *testing.T) { + root := specWorkspace(t) + + stdout, stderr, code := run(t, "map", "trace", "specs/job-store/R1.1", "--root", root) + if code != ExitOK { + t.Fatalf("map trace: exit %d (%s)", code, stderr) + } + // The requirement itself and the design that satisfies it: two files, one + // command, where the alternative is reading the spec. + for _, want := range []string{"requirements.md", "design.md"} { + if !strings.Contains(stdout, want) { + t.Errorf("the trace does not reach %s:\n%s", want, stdout) + } + } + + // A spec reference traces too: a plan's leaf is an address like any other. + if _, _, code := run(t, "map", "trace", "specs/job-store/", "--root", root); code != ExitOK { + t.Errorf("tracing a spec reference exited %d", code) + } + + // An id nothing mentions is answered rather than failed — "nothing mentions + // this" is a result, and the line saying so is what stops a caller reading an + // empty stdout as a broken command. + stdout, _, code = run(t, "map", "trace", "specs/job-store/R9.9", "--root", root) + if code != ExitOK { + t.Errorf("tracing an id nothing mentions exited %d", code) + } + if !strings.Contains(stdout, "nothing") { + t.Errorf("an empty trace said nothing about being empty: %q", stdout) + } +} + +// Section addressing bottoms out on a long section with no headings inside it, and +// blocks is the answer: every paragraph's opening line, as an address `show` takes. +func TestMapBlocksIndexesASectionThatHasNoHeadings(t *testing.T) { + root := mapWorkspace(t) + + stdout, stderr, code := run(t, "map", "blocks", "plans/sample.md", "--root", root) + if code != ExitOK { + t.Fatalf("map blocks: exit %d (%s)", code, stderr) + } + if strings.TrimSpace(stdout) == "" { + t.Fatal("map blocks printed nothing") + } + + var doc struct { + Blocks []struct { + Section string `json:"section"` + Index int `json:"index"` + Lead string `json:"lead"` + Line int `json:"line"` + } `json:"blocks"` + } + stdout, _, code = run(t, "map", "blocks", "plans/sample.md", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("map blocks --json: exit %d", code) + } + decode(t, stdout, &doc) + if len(doc.Blocks) == 0 { + t.Fatal("map blocks --json listed nothing") + } + // Every ref it prints has to be one `show` accepts, or the index is a list of + // addresses that do not resolve. + for _, b := range doc.Blocks { + if b.Section == "" || b.Index < 1 || b.Line < 1 || b.Lead == "" { + t.Errorf("a block is not addressable: %+v", b) + continue + } + ref := fmt.Sprintf("%s:%d", b.Section, b.Index) + if _, _, code := run(t, "map", "show", "plans/sample.md", ref, "--root", root); code != ExitOK { + t.Errorf("map show %s exited %d, though blocks printed it", ref, code) + } + } +} + +// --ready and --blocked share one implementation with --next, because two notions +// of eligibility would be two answers to "what do I work on". +func TestMapTasksReadyAndBlockedAgreeWithNext(t *testing.T) { + root := mapWorkspace(t) + + next, _, code := run(t, "map", "tasks", "plans/sample.md", "--root", root, "--next") + if code != ExitOK { + t.Fatalf("--next: exit %d", code) + } + if strings.TrimSpace(next) == "" { + t.Fatal("--next printed nothing on a plan with open tasks") + } + ready, _, code := run(t, "map", "tasks", "plans/sample.md", "--root", root, "--ready") + if code != ExitOK { + t.Fatalf("--ready: exit %d", code) + } + if strings.TrimSpace(ready) == "" { + t.Fatal("--ready printed nothing on a plan with open tasks") + } + + for _, flag := range []string{"--blocked", "--deps", "--open", "--done"} { + if _, _, code := run(t, "map", "tasks", "plans/sample.md", "--root", root, flag); code != ExitOK { + t.Errorf("%s exited %d", flag, code) + } + } +} + +// brief is the header and tasks is the checklist, and no command returns both — +// that split is what gives "never read the plan" its authority. +func TestMapBriefAndTasksAreEachSmallerThanThePlan(t *testing.T) { + root := mapWorkspace(t) + full := len(planText(t, root)) + + brief, _, code := run(t, "map", "brief", "plans/sample.md", "--root", root) + if code != ExitOK { + t.Fatalf("map brief: exit %d", code) + } + tasks, _, code := run(t, "map", "tasks", "plans/sample.md", "--root", root) + if code != ExitOK { + t.Fatalf("map tasks: exit %d", code) + } + if len(brief) >= full { + t.Errorf("the brief is no smaller than the plan (%d vs %d)", len(brief), full) + } + if len(tasks) >= full { + t.Errorf("the checklist is no smaller than the plan (%d vs %d)", len(tasks), full) + } +} + +// A name rather than a path: `scc map brief sample` has to find the plan, because +// the address forms exist so a caller never has to know the layout. +func TestMapAcceptsANameAsWellAsAPath(t *testing.T) { + root := specWorkspace(t) + + for _, ref := range []string{"plans/sample.md", "sample"} { + if _, _, code := run(t, "map", "brief", ref, "--root", root); code != ExitOK { + t.Errorf("map brief %s exited %d", ref, code) + } + } + for _, ref := range []string{"specs/job-store/tasks.md", "job-store"} { + if _, _, code := run(t, "map", "outline", ref, "--root", root); code != ExitOK { + t.Errorf("map outline %s exited %d", ref, code) + } + } + // A name nothing answers to is an error that says so. + stdout, stderr, code := run(t, "map", "brief", "no-such-plan", "--root", root) + if code == ExitOK { + t.Error("map brief on a name nothing answers to exited 0") + } + if !strings.Contains(stdout+stderr, "no-such-plan") { + t.Errorf("the error does not name what was asked for:\n%s%s", stdout, stderr) + } +} + +// Indented for a person, compact for everything else: the overwhelming reader of a +// --json document here is an agent paying by the byte, and measured on a six-task +// plan the indented form was 2198 bytes against 599 for the human listing. +// +// Every read has both forms, and neither may be empty — a command that printed +// nothing on one of them would send the caller to the file. +func TestEveryMapReadHasBothForms(t *testing.T) { + root := specWorkspace(t) + + for _, args := range [][]string{ + {"index"}, + {"outline", "plans/sample.md"}, + {"brief", "plans/sample.md"}, + {"tasks", "plans/sample.md"}, + {"show", "plans/sample.md", "1.2"}, + {"blocks", "plans/sample.md"}, + {"trace", "specs/job-store/R1.1"}, + {"outline", "specs/job-store/requirements.md"}, + {"show", "specs/job-store/requirements.md", "R1.1"}, + } { + name := strings.Join(args, " ") + human, _, code := run(t, append(append([]string{"map"}, args...), "--root", root)...) + if code != ExitOK { + t.Errorf("`map %s` exited %d", name, code) + continue + } + if strings.TrimSpace(human) == "" { + t.Errorf("`map %s` printed nothing", name) + } + machine, _, code := run(t, append(append([]string{"map"}, args...), "--root", root, "--json")...) + if code != ExitOK { + t.Errorf("`map %s --json` exited %d", name, code) + continue + } + if !json.Valid([]byte(strings.TrimSpace(machine))) { + t.Errorf("`map %s --json` did not print a JSON document:\n%s", name, machine) + } + // Compact: the document carries no run of leading whitespace, which is + // what a third of the indented form was. + if strings.Contains(machine, "\n ") { + t.Errorf("`map %s --json` is indented, which an agent pays for by the byte", name) + } + } +} + +// --width is for a terminal and applies to the listings, which clip to one line +// because a list of sixty one-line tasks is not a list. --next ignores it and +// prints the task whole: the line below the checkbox is usually where the decision +// sits, and a --next that stopped there would send the reader to the file — the +// exact cost this surface exists to remove. +func TestWidthClipsTheListingAndNotTheNextTask(t *testing.T) { + root := mapWorkspace(t) + + narrow, _, code := run(t, "map", "tasks", "plans/sample.md", "--root", root, "--width", "40") + if code != ExitOK { + t.Fatalf("--width: exit %d", code) + } + for _, line := range strings.Split(strings.TrimSpace(narrow), "\n") { + if len([]rune(line)) > 80 { + t.Errorf("a clipped listing line is %d runes long: %q", len([]rune(line)), line) + } + } + + next, _, code := run(t, "map", "tasks", "plans/sample.md", "--root", root, "--next", "--width", "40") + if code != ExitOK { + t.Fatalf("--next --width: exit %d", code) + } + // 1.2's description runs onto a second line in the fixture, and --next has to + // print it. + if !strings.Contains(next, "so the secret is never") { + t.Errorf("--next stopped at the checkbox line:\n%s", next) + } +} + +// `map find` is undocumented rather than removed: with the plan small, searching +// inside one stopped making sense, while searching the corpus is still the only +// alternative to reading a file. Its flags are the whole of what it offers over a +// grep, so each has a case. +func TestMapFindNarrowsByFlag(t *testing.T) { + root := specWorkspace(t) + + all, _, code := run(t, "map", "find", "atomic", "--root", root) + if code != ExitOK { + t.Fatalf("map find: exit %d", code) + } + if strings.TrimSpace(all) == "" { + t.Fatal("map find printed nothing for a term the workspace contains") + } + + var doc struct { + Query string `json:"query"` + Hits []struct { + Ref string `json:"ref"` + Path string `json:"path"` + Kind string `json:"kind"` + } `json:"hits"` + Count int `json:"count"` + } + stdout, _, code := run(t, "map", "find", "atomic", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("map find --json: exit %d", code) + } + decode(t, stdout, &doc) + if doc.Count == 0 || len(doc.Hits) != doc.Count { + t.Fatalf("the document disagrees with itself: %+v", doc) + } + if doc.Query != "atomic" { + t.Errorf("query = %q", doc.Query) + } + + // --in restricts to one artifact, which is what makes a hit list readable in a + // workspace with more than a handful of files. + stdout, _, code = run(t, "map", "find", "atomic", "--root", root, "--in", "job-store", "--json") + if code != ExitOK { + t.Fatalf("--in: exit %d", code) + } + scoped := doc + decode(t, stdout, &scoped) + for _, h := range scoped.Hits { + if !strings.Contains(h.Path, "job-store") { + t.Errorf("--in job-store returned a hit in %s", h.Path) + } + } + + // --limit caps the list, and --kind narrows it to one addressable unit. + stdout, _, code = run(t, "map", "find", "the", "--root", root, "--limit", "2", "--any", "--json") + if code != ExitOK { + t.Fatalf("--limit: exit %d", code) + } + capped := doc + decode(t, stdout, &capped) + if capped.Count > 2 { + t.Errorf("--limit 2 returned %d hits", capped.Count) + } + + stdout, _, code = run(t, "map", "find", "job", "--root", root, "--kind", "task", "--any", "--json") + if code != ExitOK { + t.Fatalf("--kind: exit %d", code) + } + kinds := doc + decode(t, stdout, &kinds) + for _, h := range kinds.Hits { + if h.Kind != "task" { + t.Errorf("--kind task returned a %s", h.Kind) + } + } + + // A regex that will not compile is an error rather than zero hits, which would + // read as "nothing matches". + if _, _, code := run(t, "map", "find", "(unclosed", "--root", root, "--regex"); code == ExitOK { + t.Error("a malformed regex exited 0") + } + // And nothing to look for is a usage error rather than the whole corpus. + if _, _, code := run(t, "map", "find", "--root", root); code != ExitError { + t.Error("map find with no query did not report a usage error") + } + // A term nothing matches is an answer, said out loud. + stdout, _, code = run(t, "map", "find", "zzzznothingmatchesthis", "--root", root) + if code != ExitOK { + t.Errorf("a term nothing matches exited %d", code) + } + if !strings.Contains(stdout, "nothing") { + t.Errorf("an empty result said nothing about being empty:\n%s", stdout) + } +} diff --git a/internal/cli/notes_more_test.go b/internal/cli/notes_more_test.go new file mode 100644 index 0000000..1b219e9 --- /dev/null +++ b/internal/cli/notes_more_test.go @@ -0,0 +1,250 @@ +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func notesText(t *testing.T, root string) string { + t.Helper() + b, err := os.ReadFile(filepath.Join(root, "docs", "notes.md")) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + return string(b) +} + +// What `scc notes find` adds over grep is the questions a substring cannot answer: +// which tags exist, what this project knows about a path, and what is new since a +// date. +func TestNotesFindAnswersWhatGrepCannot(t *testing.T) { + root := mapWorkspace(t) + + add := func(text, tag string, paths ...string) { + t.Helper() + args := []string{"notes", "add", text, "--tag", tag, "--root", root} + if len(paths) > 0 { + args = append(args, "--path", strings.Join(paths, ",")) + } + if _, stderr, code := run(t, args...); code != ExitOK { + t.Fatalf("notes add: exit %d (%s)", code, stderr) + } + } + add("wrap writes MCP config to the agent's own file", "gotcha", "internal/cli/launch.go") + add("global lock; per-account locks if throughput matters", "ceiling", "internal/cli/patch.go") + add("the race detector needs cgo", "platform", "internal/cli/launch.go") + + stdout, _, code := run(t, "notes", "tags", "--root", root) + if code != ExitOK { + t.Fatalf("notes tags: exit %d", code) + } + for _, tag := range []string{"gotcha", "ceiling", "platform"} { + if !strings.Contains(stdout, tag) { + t.Errorf("notes tags does not list %q:\n%s", tag, stdout) + } + } + + stdout, _, code = run(t, "notes", "find", "--tag", "gotcha", "--root", root) + if code != ExitOK { + t.Fatalf("notes find --tag: exit %d", code) + } + if !strings.Contains(stdout, "MCP config") || strings.Contains(stdout, "global lock") { + t.Errorf("--tag gotcha returned the wrong notes:\n%s", stdout) + } + + stdout, _, code = run(t, "notes", "find", "--path", "internal/cli/launch.go", "--root", root) + if code != ExitOK { + t.Fatalf("notes find --path: exit %d", code) + } + if !strings.Contains(stdout, "MCP config") || !strings.Contains(stdout, "race detector") { + t.Errorf("--path did not return both notes about that file:\n%s", stdout) + } + + stdout, _, code = run(t, "notes", "paths", "--root", root) + if code != ExitOK { + t.Fatalf("notes paths: exit %d", code) + } + if !strings.Contains(stdout, "launch.go") { + t.Errorf("notes paths does not list the paths notes are attached to:\n%s", stdout) + } + + // A term search still works, and a term nothing matches is an answer rather + // than a failure. + if _, _, code := run(t, "notes", "find", "throughput", "--root", root); code != ExitOK { + t.Errorf("a term search exited %d", code) + } + if _, _, code := run(t, "notes", "find", "nothing-matches-this", "--root", root); code != ExitOK { + t.Errorf("a term nothing matches exited %d, and an empty result is an answer", code) + } +} + +// `--tag` is required and has no default, because a default would be one tag on +// everything, which is the drift the index exists to prevent. +func TestNotesAddRequiresATag(t *testing.T) { + root := mapWorkspace(t) + + if _, _, code := run(t, "notes", "add", "a note with no tag", "--root", root); code == ExitOK { + t.Error("notes add with no --tag exited 0") + } + if _, err := os.Stat(filepath.Join(root, "docs", "notes.md")); err == nil { + if strings.Contains(notesText(t, root), "a note with no tag") { + t.Error("the untagged note was written anyway") + } + } +} + +// A number is spent, never reused: rm takes the text out and leaves an HTML-comment +// tombstone, so a citation to n-0001 can dangle but can never come to mean a +// different note. +func TestNotesRemoveLeavesATombstoneAndTheNumberIsSpent(t *testing.T) { + root := mapWorkspace(t) + + if _, stderr, code := run(t, "notes", "add", "the first note", "--tag", "gotcha", "--root", root); code != ExitOK { + t.Fatalf("notes add: exit %d (%s)", code, stderr) + } + first := notesText(t, root) + if !strings.Contains(first, "n-0001") { + t.Fatalf("the first note is not n-0001:\n%s", first) + } + + stdout, _, code := run(t, "notes", "show", "n-0001", "--root", root) + if code != ExitOK { + t.Fatalf("notes show: exit %d", code) + } + if !strings.Contains(stdout, "the first note") { + t.Errorf("notes show did not return the note:\n%s", stdout) + } + + if _, stderr, code := run(t, "notes", "rm", "n-0001", "--root", root); code != ExitOK { + t.Fatalf("notes rm: exit %d (%s)", code, stderr) + } + after := notesText(t, root) + if strings.Contains(after, "the first note") { + t.Errorf("rm left the text behind:\n%s", after) + } + if !strings.Contains(after, "n-0001") { + t.Errorf("rm left no tombstone, so the number could be reused:\n%s", after) + } + + // The next note gets the next number rather than the spent one. + if _, _, code := run(t, "notes", "add", "the second note", "--tag", "gotcha", "--root", root); code != ExitOK { + t.Fatalf("notes add: exit %d", code) + } + if !strings.Contains(notesText(t, root), "n-0002") { + t.Errorf("the next note did not get n-0002:\n%s", notesText(t, root)) + } + + // And an id nothing answers to is a miss rather than an empty success. + if _, _, code := run(t, "notes", "show", "n-9999", "--root", root); code == ExitOK { + t.Error("notes show on an id nothing answers to exited 0") + } + if _, _, code := run(t, "notes", "rm", "n-9999", "--root", root); code == ExitOK { + t.Error("notes rm on an id nothing answers to exited 0") + } +} + +// The ninth validator reports the failure this file cannot tolerate quietly: a +// hand-written line that missed the grammar, which no query will ever return. +func TestNotesValidateReportsAHandWrittenLine(t *testing.T) { + root := mapWorkspace(t) + if _, _, code := run(t, "notes", "add", "a well-formed note", "--tag", "gotcha", "--root", root); code != ExitOK { + t.Fatalf("notes add: exit %d", code) + } + if _, _, code := run(t, "notes", "validate", "--root", root); code != ExitOK { + t.Errorf("a well-formed log reported findings") + } + + path := filepath.Join(root, "docs", "notes.md") + body := notesText(t, root) + "- this line missed the grammar entirely\n" + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if _, _, code := run(t, "notes", "validate", "--root", root); code != ExitFindings { + t.Errorf("a malformed line exited %d, want %d", code, ExitFindings) + } +} + +// A rollback that left a seeded file behind would report a note as not written and +// still change the workspace — so the first `notes add` that fails has to take the +// file it just created with it. +func TestAFailedFirstNoteLeavesNoLogBehind(t *testing.T) { + root := mapWorkspace(t) + path := filepath.Join(root, "docs", "notes.md") + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + t.Fatalf("Remove: %v", err) + } + + // A tag the grammar cannot round-trip is refused, and the refusal happens + // after the seed has been prepared — which is the case the rollback is for. + if _, _, code := run(t, "notes", "add", "a note", "--tag", "Not Kebab Case", "--root", root); code == ExitOK { + t.Fatal("a tag the grammar rejects was accepted") + } + if _, err := os.Stat(path); err == nil { + b, _ := os.ReadFile(path) + t.Errorf("the refused note left a log behind:\n%s", b) + } + + // And the first note that succeeds creates it, seeded with the format so the + // file explains itself to whoever opens it next. + if _, stderr, code := run(t, "notes", "add", "the first note", "--tag", "gotcha", "--root", root); code != ExitOK { + t.Fatalf("notes add: exit %d (%s)", code, stderr) + } + body := notesText(t, root) + if !strings.Contains(body, "the first note") { + t.Errorf("the note was not written:\n%s", body) + } + if !strings.Contains(body, "## Log") { + t.Errorf("the seeded log carries no format for the next reader:\n%s", body) + } +} + +// --since is one of the questions a substring cannot answer, and --limit is what +// keeps a long log from arriving whole in a session's context. +func TestNotesFindSinceAndLimit(t *testing.T) { + root := mapWorkspace(t) + for i, date := range []string{"2026-01-01", "2026-06-01", "2026-09-01"} { + args := []string{"notes", "add", "note " + string(rune('a'+i)), "--tag", "gotcha", + "--date", date, "--root", root} + if _, stderr, code := run(t, args...); code != ExitOK { + t.Fatalf("notes add %s: exit %d (%s)", date, code, stderr) + } + } + + var doc struct { + Notes []struct { + Date string `json:"date"` + } `json:"notes"` + Count int `json:"count"` + } + stdout, _, code := run(t, "notes", "find", "--since", "2026-06-01", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("--since: exit %d", code) + } + decode(t, stdout, &doc) + if doc.Count != 2 { + t.Errorf("--since returned %d notes, want the two on or after that date: %+v", doc.Count, doc.Notes) + } + for _, n := range doc.Notes { + if n.Date < "2026-06-01" { + t.Errorf("--since returned a note dated %s", n.Date) + } + } + + stdout, _, code = run(t, "notes", "find", "--limit", "1", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("--limit: exit %d", code) + } + capped := doc + decode(t, stdout, &capped) + if capped.Count != 1 { + t.Errorf("--limit 1 returned %d notes", capped.Count) + } + + // A date the grammar would not parse back is refused rather than silently + // matching nothing, which reads as "no notes since then". + if _, _, code := run(t, "notes", "find", "--since", "yesterday", "--root", root); code == ExitOK { + t.Error("--since accepted a date the grammar cannot parse") + } +} diff --git a/internal/cli/patch_more_test.go b/internal/cli/patch_more_test.go new file mode 100644 index 0000000..3015bc0 --- /dev/null +++ b/internal/cli/patch_more_test.go @@ -0,0 +1,206 @@ +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// Prose gets in three ways, and stdin is there because a paragraph is a bad thing +// to put in a shell argument. All three have to reach the same file. +func TestPatchTakesTextInlineFromAFileAndFromStdin(t *testing.T) { + root := mapWorkspace(t) + + if _, stderr, code := run(t, "patch", "append", "plans/sample.md", "#why", + "--text", "Inline sentence.", "--root", root); code != ExitOK { + t.Fatalf("--text: exit %d (%s)", code, stderr) + } + if !strings.Contains(planText(t, root), "Inline sentence.") { + t.Error("the inline text never reached the file") + } + + from := filepath.Join(t.TempDir(), "body.md") + if err := os.WriteFile(from, []byte("Sentence from a file.\n"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if _, stderr, code := run(t, "patch", "append", "plans/sample.md", "#why", + "--file", from, "--root", root); code != ExitOK { + t.Fatalf("--file: exit %d (%s)", code, stderr) + } + if !strings.Contains(planText(t, root), "Sentence from a file.") { + t.Error("the text file never reached the plan") + } + + // A file that is not there is an error rather than an empty append, which + // would silently write nothing and report success. + if _, _, code := run(t, "patch", "append", "plans/sample.md", "#why", + "--file", filepath.Join(t.TempDir(), "missing.md"), "--root", root); code == ExitOK { + t.Error("a missing --file exited 0") + } +} + +// The methodology vocabulary is closed, and a task's annotation is what decides how +// it gets built — so a third word is refused rather than written through. +func TestPatchTaskRefusesAMethodologyThatIsNotOne(t *testing.T) { + root := mapWorkspace(t) + + for _, method := range []string{"Unit", "TDD"} { + if _, stderr, code := run(t, "patch", "task", "plans/sample.md", "1.1", + "--method", method, "--root", root); code != ExitOK { + t.Errorf("--method %s: exit %d (%s)", method, code, stderr) + } + } + stdout, stderr, code := run(t, "patch", "task", "plans/sample.md", "1.1", + "--method", "Integration", "--root", root) + if code == ExitOK { + t.Error("an unknown methodology was accepted") + } + if !strings.Contains(stdout+stderr, "Integration") { + t.Errorf("the refusal does not name what was passed:\n%s%s", stdout, stderr) + } +} + +// Rewriting a task must re-emit its flags and its continuation: without that, +// `patch task --method TDD` was a data-loss command that deleted the description +// and every dependency the task declared. +func TestPatchTaskKeepsTheFlagsAndTheContinuation(t *testing.T) { + root := mapWorkspace(t) + + if _, stderr, code := run(t, "patch", "task", "plans/sample.md", "1.2", + "--depends", "1.1", "--priority", "2", "--root", root); code != ExitOK { + t.Fatalf("setting the flags: exit %d (%s)", code, stderr) + } + before := planText(t, root) + for _, want := range []string{"_Depends 1.1_", "_Priority 2_"} { + if !strings.Contains(before, want) { + t.Fatalf("%s was never written:\n%s", want, before) + } + } + + if _, stderr, code := run(t, "patch", "task", "plans/sample.md", "1.2", + "--method", "TDD", "--root", root); code != ExitOK { + t.Fatalf("changing the methodology: exit %d (%s)", code, stderr) + } + after := planText(t, root) + for _, want := range []string{"_Depends 1.1_", "_Priority 2_"} { + if !strings.Contains(after, want) { + t.Errorf("changing the methodology dropped %s:\n%s", want, after) + } + } + if !strings.Contains(after, "(TDD)") { + t.Errorf("the methodology did not change:\n%s", after) + } +} + +// An address that does not resolve is an error and never an insert at a guess — +// the guard that reading-first used to provide. +func TestPatchRefusesAnAddressThatDoesNotResolve(t *testing.T) { + root := mapWorkspace(t) + before := planText(t, root) + + for _, args := range [][]string{ + {"check", "plans/sample.md", "9.9"}, + {"uncheck", "plans/sample.md", "9.9"}, + {"task", "plans/sample.md", "9.9", "--text", "x"}, + {"append", "plans/sample.md", "#no-such-section", "--text", "x"}, + {"prepend", "plans/sample.md", "#no-such-section", "--text", "x"}, + {"replace", "plans/sample.md", "#no-such-section", "--text", "x"}, + } { + full := append(append([]string{"patch"}, args...), "--root", root) + if _, _, code := run(t, full...); code == ExitOK { + t.Errorf("`scc patch %s` on an address that does not resolve exited 0", strings.Join(args, " ")) + } + } + if planText(t, root) != before { + t.Error("a refused patch changed the file anyway") + } +} + +// The frontmatter is where the kickoff answers live, and a value neither the rule +// nor the validator knows is rolled back like any other bad edit. +func TestPatchFrontmatterWritesAndRefuses(t *testing.T) { + root := mapWorkspace(t) + + if _, stderr, code := run(t, "patch", "fm", "plans/sample.md", "lang=wenyan", "--root", root); code != ExitOK { + t.Fatalf("patch fm: exit %d (%s)", code, stderr) + } + if !strings.Contains(planText(t, root), "lang: wenyan") { + t.Errorf("the key was not written:\n%s", planText(t, root)) + } + + // A pair with no `=` is a usage error rather than a key called "lang wenyan". + if _, _, code := run(t, "patch", "fm", "plans/sample.md", "lang", "--root", root); code == ExitOK { + t.Error("patch fm accepted an argument with no value") + } +} + +// After approval the work is fixed and only discovery moves: `add` allocates the +// number and demands a reason, `rm` strikes the task out where it stands so the +// number is never reused, and rewriting a task or the prose is refused. +func TestDiscoveryOnAnApprovedPlan(t *testing.T) { + root := sealWorkspace(t) + if _, stderr, code := run(t, "plan", "approve", "sample", "--root", root); code != ExitOK { + t.Fatalf("plan approve: exit %d (%s)", code, stderr) + } + read := func() string { + t.Helper() + b, err := os.ReadFile(filepath.Join(root, "plans", "sample.md")) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + return string(b) + } + + // Ticking a box is what an approved plan is for. + if _, stderr, code := run(t, "patch", "check", "sample", "1.2", "--root", root); code != ExitOK { + t.Fatalf("patch check: exit %d (%s)", code, stderr) + } + + // add without a reason is refused: discovery on a settled plan is a thing + // somebody has to account for. + if _, _, code := run(t, "patch", "add", "sample", "--group", "1", + "--text", "Something discovered", "--root", root); code == ExitOK { + t.Error("add on an approved plan succeeded with no --reason") + } + if _, stderr, code := run(t, "patch", "add", "sample", "--group", "1", + "--text", "Something discovered", "--reason", "found while building 1.2", "--root", root); code != ExitOK { + t.Fatalf("patch add: exit %d (%s)", code, stderr) + } + if !strings.Contains(read(), "Something discovered") { + t.Errorf("the discovered task was not written:\n%s", read()) + } + + // rm strikes the task out where it stands rather than deleting it, so the + // number can never come to mean something else. A ticked task is refused — + // it was either done or it was not — so this strikes out the one just added. + if _, _, code := run(t, "patch", "rm", "sample", "1.1", + "--reason", "already done", "--root", root); code == ExitOK { + t.Error("rm struck out a task that is ticked") + } + if _, stderr, code := run(t, "patch", "rm", "sample", "1.3", + "--reason", "turned out to be wrong", "--root", root); code != ExitOK { + t.Fatalf("patch rm: exit %d (%s)", code, stderr) + } + after := read() + if !strings.Contains(after, "1.1") { + t.Errorf("rm deleted the task line, so the number could be reused:\n%s", after) + } + if !strings.Contains(after, "removed") { + t.Errorf("the struck-out task does not say so:\n%s", after) + } + + // Rewriting a task, or the prose, is refused: what discovery can never touch + // is guaranteed structurally rather than by instruction. + for _, args := range [][]string{ + {"task", "sample", "1.2", "--text", "rewritten"}, + {"replace", "sample", "#why", "--text", "a different why"}, + {"append", "sample", "#why", "--text", "an extra sentence"}, + {"prepend", "sample", "#why", "--text", "an extra sentence"}, + } { + full := append(append([]string{"patch"}, args...), "--root", root) + if _, _, code := run(t, full...); code == ExitOK { + t.Errorf("`scc patch %s` on an approved plan exited 0", strings.Join(args, " ")) + } + } +} diff --git a/internal/cli/plan_more_test.go b/internal/cli/plan_more_test.go new file mode 100644 index 0000000..07350ce --- /dev/null +++ b/internal/cli/plan_more_test.go @@ -0,0 +1,163 @@ +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// validPlan is a plan under the v2 contract: six sections and no others, which is +// what `plan validate` and `plan approve` hold a file to. mapPlan is deliberately a +// v1 plan — it is what the map reads were measured against — so the seal tests need +// their own. +const validPlan = `--- +autonomy: auto +ci: wait +--- + +# Sample plan + +What this work is, in one sentence. + +## Why + +What this is for, and what done means for the whole of it. + +## Out of scope + +- The thing this is not. + +## Tasks + +- [x] 1.1 (Unit) Build the parser, and prove with a test that it reads a fenced + block without treating the example inside it as a task +- [ ] 1.2 (TDD) Guard the credential before the provider client lands + +## Done when + +- The suite is green and the validators report nothing. +` + +func sealWorkspace(t *testing.T) string { + t.Helper() + root := t.TempDir() + if _, _, code := run(t, "init", "--no-rtk", "--no-hooks", "--root", root); code != ExitOK { + t.Fatalf("init: exit %d", code) + } + dir := filepath.Join(root, "plans") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "sample.md"), []byte(validPlan), 0o644); err != nil { + t.Fatal(err) + } + return root +} + +// `scc plan validate` is the same validators narrowed to one plan, and it returns 2 +// on findings like every other lint question — a finding is a legitimate answer, +// not a failure of the tool, and collapsing the two makes the contract useless to +// the agent branching on it. +func TestPlanValidateReportsFindingsAsTwo(t *testing.T) { + root := sealWorkspace(t) + path := filepath.Join(root, "plans", "sample.md") + + if _, stderr, code := run(t, "plan", "validate", "sample", "--root", root); code != ExitOK { + t.Fatalf("a clean plan: exit %d (%s)", code, stderr) + } + + // A heading nobody agreed to is the finding the closed section set exists for: + // the 56KB plan that motivated the contract got there through `## Notes`. + if err := os.WriteFile(path, []byte(validPlan+"\n## Notes\n\nAn essay starts here.\n"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if _, _, code := run(t, "plan", "validate", "sample", "--root", root); code != ExitFindings { + t.Errorf("a plan with an unknown section exited %d, want %d", code, ExitFindings) + } + + // A plan that is not there is a usage error rather than a finding. + if _, _, code := run(t, "plan", "validate", "no-such-plan", "--root", root); code != ExitError { + t.Errorf("a plan that does not exist exited %d, want %d", code, ExitError) + } +} + +// The seal is tamper-evidence, and reseal is the answer to a legitimate edit made +// outside the cycle — deliberately a separate forced command, because the same +// call made automatically would erase the evidence it exists to keep. +func TestResealNeedsForceOnceThePlanHasDrifted(t *testing.T) { + root := sealWorkspace(t) + path := filepath.Join(root, "plans", "sample.md") + + if _, stderr, code := run(t, "plan", "approve", "sample", "--root", root); code != ExitOK { + t.Fatalf("plan approve: exit %d (%s)", code, stderr) + } + sealed, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if !strings.Contains(string(sealed), "status: approved") || !strings.Contains(string(sealed), "checksum:") { + t.Fatalf("approve wrote neither half of the seal:\n%s", sealed) + } + + // Edit it by hand, which is what the seal exists to notice. + drifted := strings.Replace(string(sealed), "What this work is, in one sentence.", + "What this work is, edited by hand.", 1) + if err := os.WriteFile(path, []byte(drifted), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + // Drift is checked before an edit is applied rather than by a validator, and + // that is the whole value: a harness that edited by hand and then patched + // would otherwise have its edit resealed by the command that should report it. + stdout, stderr, code := run(t, "patch", "check", "sample", "1.2", "--root", root) + if code == ExitOK { + t.Errorf("a patch onto a drifted plan succeeded: %s %s", stdout, stderr) + } + if !strings.Contains(stdout+stderr, "reseal") { + t.Errorf("the refusal does not name the way past it: %s %s", stdout, stderr) + } + after, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(after) != drifted { + t.Error("the refused patch changed the file anyway") + } + + if _, stderr, code := run(t, "plan", "reseal", "sample", "--force", "--root", root); code != ExitOK { + t.Fatalf("reseal --force: exit %d (%s)", code, stderr) + } + if _, _, code := run(t, "patch", "check", "sample", "1.2", "--root", root); code != ExitOK { + t.Errorf("a patch onto a resealed plan exited %d", code) + } + // And the status survives: reseal answers "this edit was legitimate", never + // "approve this". + final, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if !strings.Contains(string(final), "status: approved") { + t.Errorf("reseal dropped the status: %s", final) + } +} + +// A plan name becomes a path segment, so it goes through SafeName first: without +// it `scc plan new ..` resolves to the workspace root. +func TestPlanNewRefusesHostileNamesAndDoesNotOverwrite(t *testing.T) { + root := sealWorkspace(t) + + for _, name := range []string{"..", "../outside", "nested/name"} { + if _, _, code := run(t, "plan", "new", name, "--root", root); code == ExitOK { + t.Errorf("plan new %q exited 0", name) + } + } + if _, stderr, code := run(t, "plan", "new", "second-plan", "--root", root); code != ExitOK { + t.Errorf("plan new second-plan: exit %d (%s)", code, stderr) + } + // Twice is a refusal rather than an overwrite: the file is the user's once it + // exists. + if _, _, code := run(t, "plan", "new", "second-plan", "--root", root); code == ExitOK { + t.Error("plan new overwrote a plan that was already there") + } +} diff --git a/internal/cli/rtk_more_test.go b/internal/cli/rtk_more_test.go new file mode 100644 index 0000000..27cda2e --- /dev/null +++ b/internal/cli/rtk_more_test.go @@ -0,0 +1,51 @@ +package cli + +import ( + "strings" + "testing" +) + +// rtkInstallOK is one decision for `scc init` and `scc launch` both, because two +// copies would be two answers to "is anybody here to ask" and the untested one +// would be the one that ran. Every path out of it is a reason phrased for the line +// the caller prints. +func TestRTKInstallOKAnswersEveryWayItCanRefuse(t *testing.T) { + // No cargo anywhere: the toolchain is the reason, not the flag. + isolatedPath(t) + + for name, tc := range map[string]struct { + ask rtkAsk + says string + }{ + "no-install": {rtkAsk{noInstall: true}, "not on PATH"}, + "plan only": {rtkAsk{plan: true}, "not on PATH"}, + "no cargo": {rtkAsk{}, "cargo is not on PATH"}, + "unattended": {rtkAsk{quiet: true}, "cargo is not on PATH"}, + "yes without": {rtkAsk{yes: true}, "cargo is not on PATH"}, + } { + t.Run(name, func(t *testing.T) { + _, _, _ = capture(t, func() int { + if got := rtkInstallOK(tc.ask); !strings.Contains(got, tc.says) { + t.Errorf("rtkInstallOK(%+v) = %q, want it to mention %q", tc.ask, got, tc.says) + } + return ExitOK + }) + }) + } + + // With cargo there, --yes is consent in advance and nothing is asked. + isolatedPath(t, "cargo") + if got := rtkInstallOK(rtkAsk{yes: true}); got != "" { + t.Errorf("rtkInstallOK with --yes and cargo present = %q, want the empty answer", got) + } + // And an unattended run still declines rather than building a Rust binary on + // somebody who is not there to stop it. + if got := rtkInstallOK(rtkAsk{quiet: true}); !strings.Contains(got, "nobody is here") { + t.Errorf("an unattended run = %q, want it to say nobody is here to answer", got) + } + // --no-install outranks a present cargo: it is the flag that means "install + // nothing", not "install nothing unless it is easy". + if got := rtkInstallOK(rtkAsk{noInstall: true}); got == "" { + t.Error("--no-install with cargo present allowed the build anyway") + } +} diff --git a/internal/cli/track_test.go b/internal/cli/track_test.go index f1e330a..0598d15 100644 --- a/internal/cli/track_test.go +++ b/internal/cli/track_test.go @@ -258,6 +258,9 @@ func TestSpecSyncFollowsABranchToMerged(t *testing.T) { gitRun("init", "-q", "-b", "main", ".") gitRun("config", "user.email", "t@example.com") gitRun("config", "user.name", "t") + // A machine whose global config signs commits would fail every commit below + // non-interactively, which is an environment this test does not mean to assert about. + gitRun("config", "commit.gpgsign", "false") gitRun("add", "-A") gitRun("commit", "-qm", "init") gitRun("switch", "-qc", "feat/user-auth") @@ -311,3 +314,101 @@ func TestSpecSyncFollowsABranchToMerged(t *testing.T) { t.Errorf("sync = %q, want nothing left open", stdout) } } + +// syncTargets is the specs to reconcile: the ones named, or all of them. A name +// becomes a path segment, so it goes through SafeName first — without it +// `scc spec sync ..` walks out of the workspace. +func TestSpecSyncTargetsNamedSpecsOrAllOfThem(t *testing.T) { + if !git.Found(git.Bin) { + t.Skip("git is not on PATH") + } + root := trackedSpec(t) + gitHere(t, root) + if _, _, code := run(t, "spec", "new", "second-feature", "--root", root); code != ExitOK { + t.Fatalf("spec new: exit %d", code) + } + // Sync reconciles the specs that recorded something: one with no branch and no + // PR is a spec nobody has started, which is not a loose end to report. + for _, name := range []string{"user-auth", "second-feature"} { + if _, stderr, code := run(t, "spec", "track", name, "--branch", "feat/"+name, "--root", root); code != ExitOK { + t.Fatalf("spec track %s: exit %d (%s)", name, code, stderr) + } + } + + var doc struct { + Specs []struct { + Name string `json:"spec"` + } `json:"specs"` + Count int `json:"count"` + } + stdout, stderr, code := run(t, "spec", "sync", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("spec sync: exit %d (%s)", code, stderr) + } + decode(t, stdout, &doc) + if doc.Count < 2 { + t.Errorf("a bare sync reconciled %d specs, want both: %+v", doc.Count, doc.Specs) + } + + stdout, _, code = run(t, "spec", "sync", "user-auth", "--root", root, "--json") + if code != ExitOK { + t.Fatalf("spec sync user-auth: exit %d", code) + } + one := doc + decode(t, stdout, &one) + if one.Count != 1 { + t.Errorf("a named sync reconciled %d specs: %+v", one.Count, one.Specs) + } + + for _, name := range []string{"..", "../outside", "nested/name"} { + if _, _, code := run(t, "spec", "sync", name, "--root", root); code == ExitOK { + t.Errorf("spec sync %q exited 0", name) + } + } + // A spec that is simply not there is an error too, and one that names itself. + stdout, stderr, code = run(t, "spec", "sync", "no-such-feature", "--root", root) + if code == ExitOK { + t.Error("syncing a spec that does not exist exited 0") + } + if !strings.Contains(stdout+stderr, "no-such-feature") { + t.Errorf("the error does not name the spec:\n%s%s", stdout, stderr) + } +} + +// --dry-run reports what sync would write and writes nothing: the record is the +// user's, and a command that edited every spec in the workspace without being +// asked would be the one flag nobody expects to change anything doing exactly that. +func TestSpecSyncDryRunWritesNothing(t *testing.T) { + root := trackedSpec(t) + if _, _, code := run(t, "spec", "track", "user-auth", "--branch", "feat/x", "--root", root); code != ExitOK { + t.Fatalf("spec track: exit %d", code) + } + before := requirements(t, root) + + if _, stderr, code := run(t, "spec", "sync", "--dry-run", "--root", root); code != ExitOK { + t.Fatalf("spec sync --dry-run: exit %d (%s)", code, stderr) + } + if requirements(t, root) != before { + t.Error("--dry-run rewrote the record") + } +} + +// gitHere makes root a repository with one commit, which is what `spec sync` needs +// before it has anything to reconcile against. +func gitHere(t *testing.T, root string) { + t.Helper() + for _, args := range [][]string{ + {"init", "-q", "-b", "main", "."}, + {"config", "user.email", "t@example.invalid"}, + {"config", "user.name", "t"}, + {"config", "commit.gpgsign", "false"}, + {"add", "-A"}, + {"commit", "-qm", "init"}, + } { + cmd := exec.Command(git.Bin, args...) + cmd.Dir = root + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } + } +} diff --git a/internal/cli/update_test.go b/internal/cli/update_test.go index df9a7b2..c7716bf 100644 --- a/internal/cli/update_test.go +++ b/internal/cli/update_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + "github.com/protonspy/spec-claude-code/internal/hooks" "github.com/protonspy/spec-claude-code/internal/manifest" "github.com/protonspy/spec-claude-code/internal/paths" "github.com/protonspy/spec-claude-code/internal/workspace" @@ -248,3 +249,41 @@ func TestUpdateTargetsEveryInitializedHarness(t *testing.T) { t.Errorf("stderr = %q, want it to say how to create that tree", stderr) } } + +// `scc update` reports a stale hook block and does not rewrite it: a "yes" about +// template files is not consent to edit .git. +func TestUpdateReportsStaleHooksWithoutRewritingThem(t *testing.T) { + root := gitWorkspace(t) + + hooksDir, err := hooks.Dir(root) + if err != nil { + t.Fatalf("hooks.Dir: %v", err) + } + path := filepath.Join(hooksDir, "pre-commit") + before, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + stale := strings.Replace(string(before), "scc hooks run pre-commit", "scc hooks run pre-commit --old", 1) + if stale == string(before) { + t.Fatal("the installed hook does not call back into scc, so this test's premise is gone") + } + if err := os.WriteFile(path, []byte(stale), 0o755); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + stdout, stderr, code := run(t, "update", "--yes", "--root", root) + if code != ExitOK { + t.Fatalf("update: exit %d (%s)", code, stderr) + } + after, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(after) != stale { + t.Errorf("update rewrote a hook in .git:\n%s", after) + } + if !strings.Contains(stdout+stderr, "hooks") { + t.Errorf("update said nothing about the stale hook:\n%s%s", stdout, stderr) + } +} diff --git a/internal/cli/usage_test.go b/internal/cli/usage_test.go new file mode 100644 index 0000000..a393e60 --- /dev/null +++ b/internal/cli/usage_test.go @@ -0,0 +1,155 @@ +package cli + +import ( + "errors" + "flag" + "fmt" + "strings" + "testing" +) + +// `--help` is 0, and it took 52 call sites to be wrong about that at once: every +// handler routed flag.ErrHelp to ExitError, so `scc map tasks --help` reported the +// same code as a command that could not run — to an agent branching on this +// contract, help that says the command is broken. +func TestHelpIsAlwaysZeroAndAlwaysPrintsSomething(t *testing.T) { + // `check` with no subcommand is deliberately absent from every list here: it + // runs the gates, which in this repository is the suite running this test. + commands := [][]string{ + {"map"}, {"map", "index"}, {"map", "outline"}, {"map", "brief"}, {"map", "tasks"}, + {"map", "show"}, {"map", "blocks"}, {"map", "trace"}, + {"patch"}, {"patch", "check"}, {"patch", "uncheck"}, {"patch", "task"}, + {"patch", "add"}, {"patch", "rm"}, {"patch", "append"}, {"patch", "prepend"}, + {"patch", "replace"}, {"patch", "fm"}, + {"notes"}, {"notes", "add"}, {"notes", "find"}, {"notes", "show"}, + {"notes", "tags"}, {"notes", "paths"}, {"notes", "rm"}, {"notes", "validate"}, + {"spec"}, {"spec", "new"}, {"spec", "list"}, {"spec", "track"}, {"spec", "sync"}, + {"plan"}, {"plan", "new"}, {"plan", "approve"}, {"plan", "reseal"}, {"plan", "migrate"}, + {"graph"}, {"graph", "build"}, {"graph", "sync"}, {"graph", "status"}, + {"graph", "query"}, {"graph", "explore"}, {"graph", "scope"}, + {"hooks"}, {"hooks", "install"}, {"hooks", "check"}, {"hooks", "remove"}, + {"check", "set"}, {"check", "skip"}, {"check", "clear"}, + {"init"}, {"update"}, {"validate"}, {"launch"}, {"rtk"}, + } + + for _, cmd := range commands { + form := append(append([]string{}, cmd...), "--help") + name := strings.Join(form, " ") + stdout, stderr, code := run(t, form...) + if code != ExitOK { + t.Errorf("`scc %s` exited %d, and help is always 0", name, code) + } + if strings.TrimSpace(stdout+stderr) == "" { + t.Errorf("`scc %s` printed nothing", name) + } + } +} + +// A bare `help` positional is the same request one word shorter, and it is what +// somebody types after `scc map help`. It is answered on a resource — where there +// is a subcommand list to print — rather than on a leaf, where `help` is a +// perfectly good address, note id or feature name and taking it as a request for +// the manual would make those unaddressable. +func TestHelpAsAPositionalIsAnsweredOnAResource(t *testing.T) { + for _, resource := range []string{"map", "patch", "notes", "spec", "plan", "graph", "hooks", "check"} { + stdout, stderr, code := run(t, resource, "help") + if code != ExitOK { + t.Errorf("`scc %s help` exited %d", resource, code) + } + if !strings.Contains(stdout+stderr, resource) { + t.Errorf("`scc %s help` does not name the resource:\n%s%s", resource, stdout, stderr) + } + } +} + +// An unknown subcommand is an error that names what was typed: the caller is often +// an agent that cannot see a manual, so "unknown command" alone costs it a round +// trip to work out which word was wrong. +func TestAnUnknownSubcommandNamesWhatWasTyped(t *testing.T) { + for _, resource := range []string{"map", "patch", "notes", "spec", "plan", "graph", "hooks", "check"} { + stdout, stderr, code := run(t, resource, "definitely-not-a-subcommand") + if code != ExitError { + t.Errorf("`scc %s definitely-not-a-subcommand` exited %d, want %d", resource, code, ExitError) + } + if !strings.Contains(stdout+stderr, "definitely-not-a-subcommand") { + t.Errorf("`scc %s` with an unknown subcommand did not name it:\n%s%s", resource, stdout, stderr) + } + } +} + +// The top-level surface: no arguments at all, and a command scc does not have. +func TestTheTopLevelSurfaceExplainsItself(t *testing.T) { + stdout, stderr, code := run(t) + if code != ExitError { + t.Errorf("`scc` with no arguments exited %d", code) + } + if !strings.Contains(stdout+stderr, "validate") { + t.Errorf("`scc` does not list its commands:\n%s%s", stdout, stderr) + } + + stdout, stderr, code = run(t, "--help") + if code != ExitOK { + t.Errorf("`scc --help` exited %d", code) + } + if !strings.Contains(stdout+stderr, "validate") { + t.Errorf("`scc --help` does not list its commands:\n%s%s", stdout, stderr) + } + + if _, _, code := run(t, "definitely-not-a-command"); code != ExitError { + t.Errorf("an unknown command exited %d, want %d", code, ExitError) + } + + // The exit codes are the contract, and they are three distinct things: a + // version query is not a usage error and findings are not a failure to run. + if _, _, code := run(t, "version"); code != ExitOK { + t.Errorf("`scc version` exited %d", code) + } +} + +// `scc validate help` is what somebody types having just typed `scc map help`, +// which works because that command has a subcommand to dispatch on. A leaf command +// has none, so the word arrived as a stray positional and the answer was "validate +// takes no arguments" — correct, exit 1, and no use to anybody. +func TestHelpWordRewritesOnlyALoneHelp(t *testing.T) { + if got := helpWord([]string{"help"}); len(got) != 1 || got[0] != "-h" { + t.Errorf("helpWord([help]) = %v, want [-h]", got) + } + // Anything else is left exactly as it was: `help` is a perfectly good plan + // name, note id or search term, and taking it for a request would make those + // unaddressable. + for _, args := range [][]string{ + {}, + {"help", "me"}, + {"--json", "help"}, + {"helpful"}, + {"sample"}, + } { + got := helpWord(args) + if len(got) != len(args) { + t.Errorf("helpWord(%v) = %v, want it untouched", args, got) + continue + } + for i := range args { + if got[i] != args[i] { + t.Errorf("helpWord(%v) = %v, want it untouched", args, got) + break + } + } + } +} + +// `--help` is not a failure. flag reports it as one because it has nothing else to +// return, but the user got exactly what they asked for — and exit 1 there says the +// command could not run, which an agent branching on the contract reads as a +// broken command rather than as the help it just printed. +func TestExitForSeparatesHelpFromAFailure(t *testing.T) { + if got := exitFor(flag.ErrHelp); got != ExitOK { + t.Errorf("exitFor(ErrHelp) = %d, want %d", got, ExitOK) + } + if got := exitFor(errors.New("flag provided but not defined: -nope")); got != ExitError { + t.Errorf("exitFor(a real failure) = %d, want %d", got, ExitError) + } + if got := exitFor(fmt.Errorf("wrapped: %w", flag.ErrHelp)); got != ExitOK { + t.Errorf("exitFor on a wrapped ErrHelp = %d, want %d", got, ExitOK) + } +} diff --git a/internal/codegraph/codegraph_test.go b/internal/codegraph/codegraph_test.go index f5ee807..6fb8bbb 100644 --- a/internal/codegraph/codegraph_test.go +++ b/internal/codegraph/codegraph_test.go @@ -1,9 +1,11 @@ package codegraph import ( + "fmt" "io" "os" "path/filepath" + "runtime" "strings" "testing" ) @@ -123,3 +125,94 @@ func TestRunSeparatesAFailedExitFromAFailedStart(t *testing.T) { t.Errorf("code = %d, want 1", code) } } + +// The binary half, driven against a stand-in npm and a stand-in codegraph on a +// replaced PATH: npm is the only installer scc will run, and a test that ran it +// would install a global package on whoever runs the suite. +func TestTheBinaryHalfAnswersAboutWhatIsOnPATH(t *testing.T) { + onlyPath(t, t.TempDir()) + if i, ok := Available(); ok { + t.Errorf("Available returned %+v with an empty PATH", i) + } + if p, ok := Path(); ok { + t.Errorf("Path found %q with an empty PATH", p) + } + if got := Version("definitely-not-a-real-program"); got != "" { + t.Errorf("Version invented %q for a binary that is not there", got) + } + + onlyPath(t, fakeBin(t, "npm", "", 0), fakeBin(t, Bin, "1.5.0", 0)) + i, ok := Available() + if !ok { + t.Fatal("Available false with npm right there") + } + p, ok := Path() + if !ok { + t.Fatal("Path did not find the binary on PATH") + } + if got := Version(p); got != "1.5.0" { + t.Errorf("Version = %q", got) + } + if err := Install(i, io.Discard, io.Discard); err != nil { + t.Errorf("Install with a working npm: %v", err) + } + + onlyPath(t, fakeBin(t, "npm", "", 1)) + i, _ = Available() + if err := Install(i, io.Discard, io.Discard); err == nil { + t.Error("Install returned no error when npm failed") + } +} + +// Indexed is what tells `init` from `sync`, and it answers on the directory the +// tool actually writes rather than on the workspace having been seen before. +func TestIndexedAnswersOnTheGraphDirectory(t *testing.T) { + root := t.TempDir() + if Indexed(root) { + t.Error("Indexed on a workspace with no graph") + } + if err := os.MkdirAll(filepath.Join(root, Dir), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if !Indexed(root) { + t.Error("Indexed false with the graph directory right there") + } +} + +// fakeBin writes an executable named name into its own directory and returns that +// directory, so a test can put it on PATH and drive a lookup or an install without +// the real tool being installed on the machine running the suite. +func fakeBin(t *testing.T, name, stdout string, exit int) string { + t.Helper() + dir := t.TempDir() + if runtime.GOOS == "windows" { + body := "@echo off\r\n" + if stdout != "" { + body += "echo " + stdout + "\r\n" + } + body += fmt.Sprintf("exit /b %d\r\n", exit) + writeFile(t, filepath.Join(dir, name+".cmd"), body, 0o644) + return dir + } + body := "#!/bin/sh\n" + if stdout != "" { + body += "printf '%s\n' '" + strings.ReplaceAll(stdout, "'", `'\''`) + "'\n" + } + body += fmt.Sprintf("exit %d\n", exit) + writeFile(t, filepath.Join(dir, name), body, 0o755) + return dir +} + +func writeFile(t *testing.T, path, body string, mode os.FileMode) { + t.Helper() + if err := os.WriteFile(path, []byte(body), mode); err != nil { + t.Fatalf("WriteFile %s: %v", path, err) + } +} + +// onlyPath replaces PATH with dirs for the length of the test, so a lookup finds +// exactly what the test put there and nothing the machine happens to have. +func onlyPath(t *testing.T, dirs ...string) { + t.Helper() + t.Setenv("PATH", strings.Join(dirs, string(os.PathListSeparator))) +} diff --git a/internal/devcontainer/devcontainer_test.go b/internal/devcontainer/devcontainer_test.go new file mode 100644 index 0000000..0a854ce --- /dev/null +++ b/internal/devcontainer/devcontainer_test.go @@ -0,0 +1,295 @@ +package devcontainer + +import ( + "fmt" + "io" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// Windows alone, and by elimination rather than preference: everywhere else has a +// kernel sandbox that is cheaper, stronger, and not coupled to Docker. +func TestPreferredIsWindowsAlone(t *testing.T) { + if got, want := Preferred(), runtime.GOOS == "windows"; got != want { + t.Errorf("Preferred() = %v on %s, want %v", got, runtime.GOOS, want) + } +} + +// Present answers on the config file, not on the directory: a `.devcontainer/` +// somebody started and left empty is not a project the CLI can build. +func TestPresentWantsTheConfigFile(t *testing.T) { + root := t.TempDir() + if Present(root) { + t.Error("Present on an empty workspace") + } + if err := os.MkdirAll(filepath.Join(root, Dir), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if Present(root) { + t.Error("Present on a directory with no configuration in it") + } + if err := os.WriteFile(filepath.Join(root, Dir, Config), []byte("{}\n"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if !Present(root) { + t.Error("Present false with the configuration right there") + } +} + +// The CLI resolves the project from --workspace-folder, not from the working +// directory, so a process started elsewhere would build the wrong project. Both +// vectors carry it, and exec carries the agent's own arguments after the binary +// rather than before it, where the CLI would claim them. +func TestArgVectorsCarryTheWorkspaceFolder(t *testing.T) { + root := filepath.Join("some", "workspace") + + up := UpArgs(root) + if up[0] != "up" { + t.Errorf("UpArgs starts with %q", up[0]) + } + if !contains(up, "--workspace-folder") || !contains(up, root) { + t.Errorf("UpArgs = %v, which does not name the workspace", up) + } + + run := ExecArgs(root, "claude", []string{"--resume", "--verbose"}) + if run[0] != "exec" { + t.Errorf("ExecArgs starts with %q", run[0]) + } + if !contains(run, "--workspace-folder") || !contains(run, root) { + t.Errorf("ExecArgs = %v, which does not name the workspace", run) + } + bin := indexOf(run, "claude") + if bin < 0 { + t.Fatalf("ExecArgs = %v, which never names the binary", run) + } + if indexOf(run, "--resume") < bin || indexOf(run, "--verbose") < bin { + t.Errorf("ExecArgs = %v puts the agent's arguments ahead of the agent", run) + } + if run[len(run)-1] != "--verbose" { + t.Errorf("ExecArgs = %v reordered the pass-through", run) + } +} + +// ExecArgs must not write into the caller's slice. It appends to a slice it built +// itself, but a `rest` with spare capacity is exactly where an append-in-place bug +// hides, and the caller here is holding the user's own arguments. +func TestExecArgsDoesNotAliasTheCallersArguments(t *testing.T) { + rest := make([]string, 1, 4) + rest[0] = "--resume" + ExecArgs("w", "claude", rest) + if len(rest) != 1 || rest[0] != "--resume" { + t.Errorf("the caller's slice changed to %v", rest) + } +} + +// Path and Version answer about a binary that is usually not installed on the +// machine running the suite, so what is asserted is the shape of the answer: an +// absent binary is reported absent rather than as an empty path that looks real. +func TestPathAndVersionDegradeRatherThanGuess(t *testing.T) { + if p, ok := Path(); ok && p == "" { + t.Error("Path reported a binary with no path") + } else if !ok && p != "" { + t.Errorf("Path reported %q for a binary it says is absent", p) + } + if got := Version("definitely-not-a-real-program"); got != "" { + t.Errorf("Version invented %q for a binary that is not there", got) + } +} + +// Run reports the child's exit code rather than collapsing it, so a caller can tell +// "the container would not come up" from "the CLI could not be started". +func TestRunSeparatesAFailedExitFromAFailedStart(t *testing.T) { + code, err := Run("definitely-not-a-real-program", t.TempDir(), UpArgs("w"), io.Discard, io.Discard) + if err == nil { + t.Error("Run with a missing binary returned no error") + } + if code != 0 { + t.Errorf("code = %d, want 0 — a process that never started has no exit code", code) + } + + // A real binary that exits non-zero: `go` with no arguments prints usage and + // exits 2. It is on PATH by construction, since this suite is running. + code, err = Run("go", t.TempDir(), []string{"--this-is-not-a-go-command"}, io.Discard, io.Discard) + if err != nil { + t.Errorf("Run on a binary that started returned an error: %v", err) + } + if code == 0 { + t.Error("Run reported success for a command that failed") + } +} + +// npm is the only installer, and a missing one is reported as a missing npm rather +// than as a failed install — which would send the user to look at the CLI instead +// of at their own toolchain. +func TestInstallReportsAMissingNPM(t *testing.T) { + if Available() { + t.Skip("npm is on PATH here, so the missing-npm path is not reachable") + } + err := Install(io.Discard, io.Discard) + if err == nil { + t.Fatal("Install with no npm returned no error") + } + if !strings.Contains(err.Error(), "not on PATH") { + t.Errorf("error = %q, want it to say npm is not on PATH", err) + } +} + +// The hint has to name both things the user needs, because having one and not the +// other is the common case and each has a different fix. +func TestInstallHintNamesNPMTheDaemonAndTheDocs(t *testing.T) { + hint := InstallHint() + for _, want := range []string{InstallCmd(), "npm", "Docker", Docs} { + if !strings.Contains(hint, want) { + t.Errorf("InstallHint() = %q, which never mentions %q", hint, want) + } + } + if !strings.Contains(InstallCmd(), Pkg) { + t.Errorf("InstallCmd() = %q, which does not install %q", InstallCmd(), Pkg) + } +} + +// scc runs no remote script through a shell on the user's behalf — the rule +// docs/stack.md states and CodeGraph's own installer is the counter-example to. +func TestTheInstallerIsNotAPipedScript(t *testing.T) { + for _, s := range []string{InstallCmd(), InstallHint()} { + if strings.Contains(s, "| sh") || strings.Contains(s, "| bash") || strings.Contains(s, "iex") { + t.Errorf("%q pipes something into a shell", s) + } + } +} + +// DockerReady is a separate question from Path, because the two fail differently +// and the user can only fix one at a time. Asserted as a shape: whatever this +// machine has, the answer must not claim a daemon without a docker binary. +func TestDockerReadyNeedsTheBinaryFirst(t *testing.T) { + if DockerReady() { + if _, ok := Path(); !ok { + // Fine: docker can be there without the devcontainer CLI. The check is + // only that DockerReady did not answer for the CLI. + t.Log("a daemon is running without the devcontainer CLI, which is the split this reports") + } + } +} + +func contains(args []string, want string) bool { return indexOf(args, want) >= 0 } + +func indexOf(args []string, want string) int { + for i, a := range args { + if a == want { + return i + } + } + return -1 +} + +// fakeBin writes an executable named name into its own directory and returns that +// directory, so a test can put it on PATH and drive the install and lookup paths +// without the real tool being installed on the machine running the suite. +// +// Two flavours because exec.LookPath is platform-shaped: on Windows a command is +// found through PATHEXT, so the script has to be a .cmd; everywhere else it is the +// executable bit and a shebang. +func fakeBin(t *testing.T, name, stdout string, exit int) string { + t.Helper() + dir := t.TempDir() + if runtime.GOOS == "windows" { + body := "@echo off\r\n" + if stdout != "" { + body += "echo " + stdout + "\r\n" + } + body += fmt.Sprintf("exit /b %d\r\n", exit) + write(t, filepath.Join(dir, name+".cmd"), body, 0o644) + return dir + } + body := "#!/bin/sh\n" + if stdout != "" { + body += "echo '" + stdout + "'\n" + } + body += fmt.Sprintf("exit %d\n", exit) + write(t, filepath.Join(dir, name), body, 0o755) + return dir +} + +func write(t *testing.T, path, body string, mode os.FileMode) { + t.Helper() + if err := os.WriteFile(path, []byte(body), mode); err != nil { + t.Fatalf("WriteFile %s: %v", path, err) + } +} + +// onlyPath replaces PATH with dirs for the length of the test, so a lookup finds +// exactly what the test put there and nothing the machine happens to have. +func onlyPath(t *testing.T, dirs ...string) { + t.Helper() + t.Setenv("PATH", strings.Join(dirs, string(os.PathListSeparator))) +} + +// The install path, driven against a stand-in npm rather than the real one: a test +// that shelled out to the actual package manager would install a global package on +// whoever ran the suite. +func TestInstallRunsNPMAndReportsWhatItDid(t *testing.T) { + onlyPath(t, fakeBin(t, "npm", "", 0)) + if !Available() { + t.Fatal("Available false with npm right there on PATH") + } + if err := Install(io.Discard, io.Discard); err != nil { + t.Errorf("Install with a working npm: %v", err) + } + + onlyPath(t, fakeBin(t, "npm", "", 1)) + err := Install(io.Discard, io.Discard) + if err == nil { + t.Fatal("Install returned no error when npm failed") + } + if !strings.Contains(err.Error(), InstallCmd()) { + t.Errorf("error = %q, which does not name the command that failed", err) + } + + onlyPath(t, t.TempDir()) + if Available() { + t.Fatal("Available true with an empty PATH") + } + if err := Install(io.Discard, io.Discard); err == nil { + t.Error("Install returned no error with no npm on PATH") + } else if !strings.Contains(err.Error(), "not on PATH") { + t.Errorf("error = %q, want it to say npm is not on PATH", err) + } +} + +// Path and Version against a stand-in CLI, so both answers are exercised on a +// machine that has never installed the real one. +func TestPathAndVersionFindWhatIsOnPATH(t *testing.T) { + onlyPath(t, fakeBin(t, Bin, "0.89.0", 0)) + p, ok := Path() + if !ok { + t.Fatal("Path did not find the binary on PATH") + } + if got := Version(p); got != "0.89.0" { + t.Errorf("Version() = %q, want %q", got, "0.89.0") + } + + onlyPath(t, t.TempDir()) + if p, ok := Path(); ok { + t.Errorf("Path found %q with an empty PATH", p) + } +} + +// DockerReady is two questions, and the binary is the first: a missing docker is +// reported without ever asking a daemon that cannot be there. +func TestDockerReadyIsFalseWithoutTheBinary(t *testing.T) { + onlyPath(t, t.TempDir()) + if DockerReady() { + t.Error("DockerReady true with no docker on PATH") + } + onlyPath(t, fakeBin(t, "docker", "", 1)) + if DockerReady() { + t.Error("DockerReady true when `docker info` failed") + } + onlyPath(t, fakeBin(t, "docker", "Server Version: 27", 0)) + if !DockerReady() { + t.Error("DockerReady false when `docker info` succeeded") + } +} diff --git a/internal/ears/ears_test.go b/internal/ears/ears_test.go index 95e91a1..2b2240d 100644 --- a/internal/ears/ears_test.go +++ b/internal/ears/ears_test.go @@ -189,3 +189,32 @@ func TestInternalCommaDoesNotBreakWellFormedness(t *testing.T) { t.Errorf("req = %+v, want the system and response intact", req) } } + +// A finding that quotes an entire paragraph back at the user is a finding they will +// not read, so the text a parse error carries is clipped — and a requirement long +// enough to need clipping is exactly the one most likely to be malformed. +func TestAParseErrorQuotesAReadableAmount(t *testing.T) { + // No `the` before `shall`, which is the error that quotes the head back. + long := strings.Repeat("a very long condition ", 20) + "shall happen" + _, err := Parse(long) + if err == nil { + t.Fatalf("a requirement with no subject parsed: %q", long) + } + if len(err.Error()) > 200 { + t.Errorf("the error is %d characters long:\n%s", len(err.Error()), err) + } + if !strings.Contains(err.Error(), "…") { + t.Errorf("a clipped quote does not say it was clipped: %s", err) + } + + // A short one is quoted whole: clipping what already fits would hide the very + // text the reader has to fix. + short := "nothing shall happen" + _, err = Parse(short) + if err == nil { + t.Fatalf("a requirement with no subject parsed: %q", short) + } + if !strings.Contains(err.Error(), "nothing") { + t.Errorf("a short requirement was not quoted whole: %s", err) + } +} diff --git a/internal/gate/gate_test.go b/internal/gate/gate_test.go index 43cef83..86af335 100644 --- a/internal/gate/gate_test.go +++ b/internal/gate/gate_test.go @@ -1,6 +1,11 @@ package gate import ( + "os" + + "github.com/protonspy/spec-claude-code/internal/assets" + "github.com/protonspy/spec-claude-code/internal/manifest" + "github.com/protonspy/spec-claude-code/internal/paths" "io" "runtime" "strings" @@ -321,3 +326,88 @@ func twoStreams(out, err string) string { } return "printf '%s\n' '" + err + "' >&2; printf '%s\n' '" + out + "'" } + +// scaffolded makes root look like a workspace for these harnesses: the manifest is +// the marker, and it is also where the commands live. +func scaffolded(t *testing.T, all ...paths.Harness) string { + t.Helper() + root := t.TempDir() + for _, h := range all { + if err := os.MkdirAll(h.Config(root), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := manifest.Save(root, h, manifest.New(assets.Version, h)); err != nil { + t.Fatalf("manifest.Save: %v", err) + } + } + return root +} + +// A workspace that records nothing is not an error: absence means this project has +// not wired its commands up, which the caller reports or ignores depending on +// whether the gate was asked for. +func TestLoadAndSaveRoundTripThroughEveryManifest(t *testing.T) { + root := scaffolded(t, paths.Claude, paths.Codex) + + cfg, err := Load(root) + if err != nil { + t.Fatalf("Load: %v", err) + } + if cfg.Any() { + t.Errorf("a freshly scaffolded workspace reported commands: %+v", cfg) + } + + want := Config{ + Commands: map[Kind]string{ + Build: "go build ./...", + Test: "go run -C tools ./testreport", + Lint: "golangci-lint run", + Format: Skipped, + }, + MinCoverage: 91, + } + wrote, err := Save(root, want) + if err != nil { + t.Fatalf("Save: %v", err) + } + // Every harness rather than one: the next reader takes the first that has + // any, so writing a single manifest would make the answer depend on which + // tool the reader was standing in. + if len(wrote) != 2 { + t.Errorf("Save wrote %v, want both manifests", wrote) + } + + got, err := Load(root) + if err != nil { + t.Fatalf("Load: %v", err) + } + for _, k := range Kinds() { + if got.Command(k) != want.Command(k) { + t.Errorf("%s = %q, want %q", k, got.Command(k), want.Command(k)) + } + } + if got.MinCoverage != want.MinCoverage { + t.Errorf("MinCoverage = %v, want %v", got.MinCoverage, want.MinCoverage) + } + // `skipped` is a word rather than an absent key: a language with no formatter + // is a real answer, and a gate left unrecorded is a decision nobody has made. + if !got.Declined(Format) { + t.Error("a gate recorded as skipped did not read back as declined") + } + if got.Declined(Build) { + t.Error("a gate with a real command read back as declined") + } + + // A finding about the configuration points at the manifest it was read from. + if p := ManifestPath(root); p != paths.Claude.Manifest(root) { + t.Errorf("ManifestPath = %q, want the first harness's manifest", p) + } + // And outside a workspace it still names a file, so a finding has somewhere to + // point rather than an empty path. + if p := ManifestPath(t.TempDir()); p == "" { + t.Error("ManifestPath outside a workspace named nothing") + } + if _, err := Save(t.TempDir(), want); err == nil { + t.Error("Save outside a workspace returned no error") + } +} diff --git a/internal/git/repo_test.go b/internal/git/repo_test.go new file mode 100644 index 0000000..689d51e --- /dev/null +++ b/internal/git/repo_test.go @@ -0,0 +1,505 @@ +package git + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// A repository built for one test, isolated from whoever is running the suite: +// GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM point at nothing, so a developer's own +// commit.gpgsign, core.hooksPath or diff.noprefix cannot reach in and change what +// these tests measure — which is the same class of defect Changed's pinned +// prefixes exist for. +func repo(t *testing.T) string { + t.Helper() + if !Found(Bin) { + t.Skip("git is not on PATH") + } + empty := filepath.Join(t.TempDir(), "gitconfig") + if err := os.WriteFile(empty, nil, 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + t.Setenv("GIT_CONFIG_GLOBAL", empty) + t.Setenv("GIT_CONFIG_SYSTEM", empty) + + dir := t.TempDir() + git(t, dir, "init", "-b", DefaultBase) + git(t, dir, "config", "user.name", "Test") + git(t, dir, "config", "user.email", "test@example.invalid") + git(t, dir, "config", "commit.gpgsign", "false") + return dir +} + +func git(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command(Bin, args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } + return strings.TrimSpace(string(out)) +} + +// commit writes path and commits it, so a test reads as the history it is +// building rather than as four git calls. +func commit(t *testing.T, dir, path, body, message string) { + t.Helper() + full := filepath.Join(dir, filepath.FromSlash(path)) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(full, []byte(body), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + git(t, dir, "add", "--", path) + git(t, dir, "commit", "-m", message) +} + +func containsLine(lines []string, want string) bool { + for _, l := range lines { + if strings.TrimSpace(l) == want { + return true + } + } + return false +} + +// fakeBin writes an executable named name into its own directory and returns that +// directory, so a test can put it on PATH and drive a query without the real tool. +func fakeBin(t *testing.T, name, stdout string, exit int) string { + t.Helper() + dir := t.TempDir() + if runtime.GOOS == "windows" { + body := "@echo off\r\n" + if stdout != "" { + body += "echo " + stdout + "\r\n" + } + body += fmt.Sprintf("exit /b %d\r\n", exit) + writeFile(t, filepath.Join(dir, name+".cmd"), body, 0o644) + return dir + } + body := "#!/bin/sh\n" + if stdout != "" { + body += "printf '%s\n' " + shellQuote(stdout) + "\n" + } + body += fmt.Sprintf("exit %d\n", exit) + writeFile(t, filepath.Join(dir, name), body, 0o755) + return dir +} + +// shellQuote wraps s for /bin/sh, which has one escape for a single-quoted string +// and it is to leave the string, quote a quote, and go back in. +func shellQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + +func writeFile(t *testing.T, path, body string, mode os.FileMode) { + t.Helper() + if err := os.WriteFile(path, []byte(body), mode); err != nil { + t.Fatalf("WriteFile %s: %v", path, err) + } +} + +func TestIsRepoAndCurrentBranch(t *testing.T) { + dir := repo(t) + if !IsRepo(dir) { + t.Fatal("IsRepo false inside a repository") + } + commit(t, dir, "a.txt", "one\n", "feat: one") + + got, err := CurrentBranch(dir) + if err != nil { + t.Fatalf("CurrentBranch: %v", err) + } + if got != DefaultBase { + t.Errorf("CurrentBranch = %q, want %q", got, DefaultBase) + } + + // A detached head has no branch, and "" is the answer rather than the literal + // "HEAD" git prints — a caller comparing against a base would otherwise match + // a branch nobody is on. + git(t, dir, "checkout", "--detach", "HEAD") + if got, err := CurrentBranch(dir); err != nil || got != "" { + t.Errorf("detached CurrentBranch = %q, %v — want the empty answer", got, err) + } + + outside := t.TempDir() + if IsRepo(outside) { + t.Error("IsRepo true outside any repository") + } + if _, err := CurrentBranch(outside); err == nil { + t.Error("CurrentBranch outside a repository returned no error") + } +} + +// "main" has been wrong for every repository created before 2020 and plenty +// since, so the remote's own HEAD is asked first and the fallback is a guess the +// caller is told about. +func TestBaseAsksTheRemoteBeforeGuessing(t *testing.T) { + dir := repo(t) + commit(t, dir, "a.txt", "one\n", "feat: one") + if got := Base(dir); got != DefaultBase { + t.Errorf("Base = %q on a main-only repository, want %q", got, DefaultBase) + } + + old := repo(t) + // The initial branch has no commits yet, so switching leaves no main behind at + // all — which is the shape of a repository that only ever had master. + git(t, old, "checkout", "-b", "master") + commit(t, old, "a.txt", "one\n", "feat: one") + if got := Base(old); got != "master" { + t.Errorf("Base = %q on a master-only repository, want master", got) + } + + remote := t.TempDir() + git(t, remote, "init", "--bare", "-b", "trunk") + named := repo(t) + git(t, named, "checkout", "-b", "trunk") + commit(t, named, "a.txt", "one\n", "feat: one") + git(t, named, "remote", "add", "origin", remote) + git(t, named, "push", "-u", "origin", "trunk") + git(t, named, "remote", "set-head", "origin", "trunk") + if got := Base(named); got != "trunk" { + t.Errorf("Base = %q, want the remote's own HEAD, trunk", got) + } +} + +// Merged is ahead == 0 AND behind > 0. A branch created ten seconds ago is +// trivially an ancestor of its base, and reading that as delivered work is the +// defect this spelling exists to prevent. +func TestLookTellsAFreshBranchFromADeliveredOne(t *testing.T) { + dir := repo(t) + commit(t, dir, "a.txt", "one\n", "feat: one") + + git(t, dir, "checkout", "-b", "feat/fresh") + fresh, err := Look(dir, "feat/fresh", DefaultBase) + if err != nil { + t.Fatalf("Look: %v", err) + } + if !fresh.Local || fresh.Remote { + t.Errorf("fresh branch = %+v, want local only", fresh) + } + if fresh.Merged { + t.Error("a branch with no commits of its own was reported merged") + } + + commit(t, dir, "b.txt", "two\n", "feat: two") + ahead, _ := Look(dir, "feat/fresh", DefaultBase) + if ahead.Ahead != 1 || ahead.Behind != 0 || ahead.Merged { + t.Errorf("branch with one commit = %+v, want ahead 1 and not merged", ahead) + } + + git(t, dir, "checkout", DefaultBase) + git(t, dir, "merge", "--no-ff", "-m", "merge", "feat/fresh") + landed, _ := Look(dir, "feat/fresh", DefaultBase) + if landed.Ahead != 0 || landed.Behind == 0 || !landed.Merged { + t.Errorf("landed branch = %+v, want ahead 0, behind > 0, merged", landed) + } + + git(t, dir, "branch", "-D", "feat/fresh") + gone, _ := Look(dir, "feat/fresh", DefaultBase) + if gone.Exists() || gone.Merged { + t.Errorf("deleted branch = %+v, want neither existing nor merged", gone) + } + + filled, _ := Look(dir, DefaultBase, "") + if filled.Base == "" { + t.Error("Look left the base empty instead of asking the repository") + } +} + +// Commits is "on HEAD and not on base", and the message comes back whole — +// trailers included, because the trailers are what a caller asking about +// authorship is asking about. +func TestCommitsAreWhatThisBranchAdded(t *testing.T) { + dir := repo(t) + commit(t, dir, "a.txt", "one\n", "feat: one") + git(t, dir, "checkout", "-b", "feat/x") + commit(t, dir, "b.txt", "two\n", "feat: two\n\nA body.\n\nCo-Authored-By: Someone ") + + got, err := Commits(dir, DefaultBase) + if err != nil { + t.Fatalf("Commits: %v", err) + } + if len(got) != 1 { + t.Fatalf("Commits returned %d, want the one this branch added: %+v", len(got), got) + } + c := got[0] + if c.Subject != "feat: two" { + t.Errorf("subject = %q", c.Subject) + } + if !strings.Contains(c.Message, "Co-Authored-By:") { + t.Errorf("message lost its trailers: %q", c.Message) + } + if c.SHA == "" || c.Short == "" || !strings.HasPrefix(c.SHA, c.Short) { + t.Errorf("sha = %q, short = %q", c.SHA, c.Short) + } + + // Standing on the base branch with no remote leaves genuinely nothing to + // compare against, and that is an empty answer rather than an error. + git(t, dir, "checkout", DefaultBase) + if got, err := Commits(dir, DefaultBase); err != nil || len(got) != 0 { + t.Errorf("on the base branch: %d commits, %v — want none and no error", len(got), err) + } +} + +// On the base branch the comparison is against the remote's copy, so unpushed +// commits are still checked rather than silently passing. +func TestCommitsOnTheBaseBranchCompareAgainstTheRemote(t *testing.T) { + remote := t.TempDir() + dir := repo(t) + git(t, remote, "init", "--bare", "-b", DefaultBase) + commit(t, dir, "a.txt", "one\n", "feat: one") + git(t, dir, "remote", "add", "origin", remote) + git(t, dir, "push", "-u", "origin", DefaultBase) + + if got, _ := Commits(dir, DefaultBase); len(got) != 0 { + t.Errorf("a pushed base branch reported %d commits", len(got)) + } + commit(t, dir, "b.txt", "two\n", "feat: unpushed") + got, err := Commits(dir, DefaultBase) + if err != nil { + t.Fatalf("Commits: %v", err) + } + if len(got) != 1 || got[0].Subject != "feat: unpushed" { + t.Errorf("unpushed work on the base = %+v, want the one commit", got) + } +} + +func TestDirtyAndCommentChar(t *testing.T) { + dir := repo(t) + commit(t, dir, "a.txt", "one\n", "feat: one") + if Dirty(dir) { + t.Error("Dirty on a clean tree") + } + writeFile(t, filepath.Join(dir, "a.txt"), "changed\n", 0o644) + if !Dirty(dir) { + t.Error("Dirty false with a modified file") + } + git(t, dir, "checkout", "--", "a.txt") + writeFile(t, filepath.Join(dir, "new.txt"), "x\n", 0o644) + if !Dirty(dir) { + t.Error("Dirty false with an untracked file") + } + + if got := CommentChar(dir); got != "" { + t.Errorf("CommentChar = %q on a repository that sets none", got) + } + git(t, dir, "config", "core.commentChar", ";") + if got := CommentChar(dir); got != ";" { + t.Errorf("CommentChar = %q, want %q", got, ";") + } + // "auto" comes back as itself: git resolves it against the message being + // written, and the caller holding the message decides. + git(t, dir, "config", "core.commentChar", "auto") + if got := CommentChar(dir); got != "auto" { + t.Errorf("CommentChar = %q, want auto verbatim", got) + } +} + +// Asked of git rather than assumed, because writing a hook where git will not +// read it is the kind of failure that looks like success for weeks. +func TestHooksDirHonoursTheConfiguredPath(t *testing.T) { + dir := repo(t) + commit(t, dir, "a.txt", "one\n", "feat: one") + + got, err := HooksDir(dir) + if err != nil { + t.Fatalf("HooksDir: %v", err) + } + if filepath.Base(got) != "hooks" || !filepath.IsAbs(got) { + t.Errorf("HooksDir = %q, want an absolute path ending in hooks", got) + } + + elsewhere := filepath.Join(t.TempDir(), "githooks") + git(t, dir, "config", "core.hooksPath", filepath.ToSlash(elsewhere)) + got, err = HooksDir(dir) + if err != nil { + t.Fatalf("HooksDir with core.hooksPath: %v", err) + } + if filepath.Clean(got) != filepath.Clean(elsewhere) { + t.Errorf("HooksDir = %q, want the configured %q", got, elsewhere) + } + + if _, err := HooksDir(t.TempDir()); err == nil { + t.Error("HooksDir outside a repository returned no error") + } +} + +// Everything here is answered from refs already in the repository — no fetch, no +// gh — because the caller is a hook that runs at the end of every turn. +func TestUndeliveredWork(t *testing.T) { + dir := repo(t) + commit(t, dir, "a.txt", "one\n", "feat: one") + + u := UndeliveredWork(dir) + if u.Any() || u.Branch != DefaultBase { + t.Errorf("on the base branch = %+v, want nothing to report", u) + } + + git(t, dir, "checkout", "-b", "feat/x") + commit(t, dir, "b.txt", "two\n", "feat: two") + u = UndeliveredWork(dir) + if !u.Any() || u.Ahead != 1 || u.Unpushed != 1 || u.Pushed { + t.Errorf("never-pushed branch = %+v, want one unpushed commit and Pushed false", u) + } + + remote := t.TempDir() + git(t, remote, "init", "--bare", "-b", DefaultBase) + git(t, dir, "remote", "add", "origin", remote) + git(t, dir, "push", "-u", "origin", "feat/x") + u = UndeliveredWork(dir) + if u.Any() || !u.Pushed { + t.Errorf("pushed branch = %+v, want Pushed and nothing outstanding", u) + } + + commit(t, dir, "c.txt", "three\n", "feat: three") + u = UndeliveredWork(dir) + if !u.Any() || u.Unpushed != 1 || !u.Pushed { + t.Errorf("branch with one commit past the remote = %+v", u) + } + + git(t, dir, "checkout", "--detach", "HEAD") + if got := UndeliveredWork(dir); got.Any() { + t.Errorf("detached head = %+v, want nothing", got) + } + if got := UndeliveredWork(t.TempDir()); got.Any() { + t.Errorf("outside a repository = %+v, want nothing", got) + } +} + +// Changed reads against the working tree rather than HEAD, because at the end of +// a turn the agent has usually written code and not yet committed it. +func TestChangedSeesCommittedUncommittedAndUntracked(t *testing.T) { + dir := repo(t) + commit(t, dir, "a.txt", "one\n", "feat: one") + git(t, dir, "checkout", "-b", "feat/x") + + commit(t, dir, "tracked.txt", "committed line\n", "feat: tracked") + writeFile(t, filepath.Join(dir, "a.txt"), "one\nuncommitted line\n", 0o644) + writeFile(t, filepath.Join(dir, "new.txt"), "untracked line\n", 0o644) + + changes, err := Changed(dir, DefaultBase) + if err != nil { + t.Fatalf("Changed: %v", err) + } + added := map[string][]string{} + for _, c := range changes { + added[filepath.ToSlash(c.Path)] = c.Added + } + for path, want := range map[string]string{ + "tracked.txt": "committed line", + "a.txt": "uncommitted line", + "new.txt": "untracked line", + } { + lines, ok := added[path] + if !ok { + t.Errorf("%s is missing from %v", path, added) + continue + } + if !containsLine(lines, want) { + t.Errorf("%s added = %v, want it to carry %q", path, lines, want) + } + } + + // A binary blob is not read: scanning one finds nothing and costs everything. + if err := os.WriteFile(filepath.Join(dir, "blob.bin"), []byte{'a', 0, 'b'}, 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + // And a gitignored file is git's own business. + writeFile(t, filepath.Join(dir, ".gitignore"), "ignored.txt\n", 0o644) + writeFile(t, filepath.Join(dir, "ignored.txt"), "hidden\n", 0o644) + + changes, _ = Changed(dir, DefaultBase) + for _, c := range changes { + switch filepath.ToSlash(c.Path) { + case "blob.bin": + t.Errorf("a binary file was read as text: %+v", c) + case "ignored.txt": + t.Error("an ignored file was reported as changed") + } + } +} + +// Absence is a normal answer, never an error to propagate — the line this package +// holds everywhere. With no git on PATH every query says so, so a caller can tell +// "could not determine" from "determined nothing". +func TestEveryQueryDegradesWithoutGit(t *testing.T) { + dir := t.TempDir() + t.Setenv("PATH", t.TempDir()) + if Found(Bin) { + t.Skip("PATH could not be emptied on this platform") + } + + if IsRepo(dir) || Dirty(dir) || CommentChar(dir) != "" { + t.Error("a query answered as though git were here") + } + if _, err := CurrentBranch(dir); err != ErrUnavailable { + t.Errorf("CurrentBranch err = %v, want ErrUnavailable", err) + } + if _, err := Look(dir, "x", DefaultBase); err != ErrUnavailable { + t.Errorf("Look err = %v, want ErrUnavailable", err) + } + if _, err := Commits(dir, DefaultBase); err != ErrUnavailable { + t.Errorf("Commits err = %v, want ErrUnavailable", err) + } + if _, err := Changed(dir, DefaultBase); err != ErrUnavailable { + t.Errorf("Changed err = %v, want ErrUnavailable", err) + } + if _, err := HooksDir(dir); err == nil { + t.Error("HooksDir returned no error with no git") + } + if got := UndeliveredWork(dir); got.Any() { + t.Errorf("UndeliveredWork = %+v with no git", got) + } + if got := Base(dir); got != DefaultBase { + t.Errorf("Base = %q with no git, want the documented fallback", got) + } + if _, err := LookPR(dir, 1); err != ErrUnavailable { + t.Errorf("LookPR err = %v, want ErrUnavailable", err) + } + if _, err := CurrentPR(dir); err != ErrUnavailable { + t.Errorf("CurrentPR err = %v, want ErrUnavailable", err) + } +} + +// The PR is the half no hook can reach, so the title and body come back with it. +// Driven against a stand-in gh: the real one would need a network and an account. +func TestViewPRReadsWhatTheForgeSays(t *testing.T) { + const body = `{"number":7,"state":"open","headRefName":"feat/x","url":"https://example.invalid/pr/7","title":"feat: x","body":"why"}` + t.Setenv("PATH", fakeBin(t, GHBin, body, 0)) + + pr, err := LookPR(t.TempDir(), 7) + if err != nil { + t.Fatalf("LookPR: %v", err) + } + if pr.Number != 7 || pr.Branch != "feat/x" || pr.Title != "feat: x" || pr.Body != "why" { + t.Errorf("pr = %+v", pr) + } + // gh spells the state however it likes; scc's constants are upper case, so the + // comparison a caller writes against StateOpen has to hold. + if pr.State != StateOpen { + t.Errorf("state = %q, want %q", pr.State, StateOpen) + } + if _, err := CurrentPR(t.TempDir()); err != nil { + t.Errorf("CurrentPR: %v", err) + } + + // A branch with no pull request is gh exiting non-zero, and that is an answer + // rather than a malfunction. + t.Setenv("PATH", fakeBin(t, GHBin, "", 1)) + if _, err := CurrentPR(t.TempDir()); err == nil { + t.Error("CurrentPR returned no error when gh reported none") + } + // And output that is not JSON is reported rather than half-read. + t.Setenv("PATH", fakeBin(t, GHBin, "not json", 0)) + if _, err := LookPR(t.TempDir(), 7); err == nil { + t.Error("LookPR accepted output that is not JSON") + } +} diff --git a/internal/headroom/headroom_test.go b/internal/headroom/headroom_test.go index cba9795..67305bf 100644 --- a/internal/headroom/headroom_test.go +++ b/internal/headroom/headroom_test.go @@ -1,7 +1,11 @@ package headroom import ( + "fmt" "io" + "os" + "path/filepath" + "runtime" "strings" "testing" @@ -200,3 +204,111 @@ func TestInstallReportsAMissingProgram(t *testing.T) { t.Errorf("error = %q, want it to say the program is not on PATH", err) } } + +// The binary half, driven against stand-ins on a replaced PATH: the real install +// resolves and builds a Python distribution, which is not a thing a test suite +// does to whoever runs it. +func TestTheBinaryHalfAnswersAboutWhatIsOnPATH(t *testing.T) { + onlyPath(t, t.TempDir()) + if i, ok := Available(); ok { + t.Errorf("Available returned %+v with an empty PATH", i) + } + if p, ok := Path(); ok { + t.Errorf("Path found %q with an empty PATH", p) + } + if got := Version("definitely-not-a-real-program"); got != "" { + t.Errorf("Version invented %q for a binary that is not there", got) + } + // A build that cannot answer means scc passes no options rather than guessing. + if got := WrapHelp("definitely-not-a-real-program", "claude"); got != "" { + t.Errorf("WrapHelp invented %q", got) + } + // A missing program is reported as itself: the user has to get uv or pip + // first, which is a different problem from an install that broke. + err := Install(Installer{Prog: "definitely-not-a-real-program", Cmd: "nope"}, io.Discard, io.Discard) + if err == nil { + t.Fatal("Install with a missing program returned no error") + } + if !strings.Contains(err.Error(), "not on PATH") { + t.Errorf("error = %q, want it to say the program is not on PATH", err) + } + + // uv first, because it is what Headroom's own docs lead with — so a machine + // carrying both has to choose it. + uv, pip := fakeBin(t, "uv", "", 0), fakeBin(t, "pip", "", 0) + onlyPath(t, pip, uv) + i, ok := Available() + if !ok { + t.Fatal("Available false with both installers on PATH") + } + if i.Prog != "uv" { + t.Errorf("Available chose %q with uv present", i.Prog) + } + if err := Install(i, io.Discard, io.Discard); err != nil { + t.Errorf("Install with a working uv: %v", err) + } + + onlyPath(t, pip) + if i, ok := Available(); !ok || i.Prog != "pip" { + t.Errorf("Available = %+v, %v on a machine with only pip", i, ok) + } + + // The binary, and the help a wrap reads its opt-outs out of. + onlyPath(t, fakeBin(t, Bin, "headroom 1.2.3", 0)) + p, ok := Path() + if !ok { + t.Fatal("Path did not find the binary on PATH") + } + if got := Version(p); got != "headroom 1.2.3" { + t.Errorf("Version = %q", got) + } + if got := WrapHelp(p, "claude"); !strings.Contains(got, "headroom 1.2.3") { + t.Errorf("WrapHelp = %q, want whatever the build printed", got) + } + + // An install that fails is reported as the command that failed, so the user + // can run it again themselves and see the same output. + onlyPath(t, fakeBin(t, "uv", "", 1)) + i, _ = Available() + if err := Install(i, io.Discard, io.Discard); err == nil { + t.Error("Install returned no error when the installer failed") + } +} + +// fakeBin writes an executable named name into its own directory and returns that +// directory, so a test can put it on PATH and drive a lookup or an install without +// the real tool being installed on the machine running the suite. +func fakeBin(t *testing.T, name, stdout string, exit int) string { + t.Helper() + dir := t.TempDir() + if runtime.GOOS == "windows" { + body := "@echo off\r\n" + if stdout != "" { + body += "echo " + stdout + "\r\n" + } + body += fmt.Sprintf("exit /b %d\r\n", exit) + writeFile(t, filepath.Join(dir, name+".cmd"), body, 0o644) + return dir + } + body := "#!/bin/sh\n" + if stdout != "" { + body += "printf '%s\n' '" + strings.ReplaceAll(stdout, "'", `'\''`) + "'\n" + } + body += fmt.Sprintf("exit %d\n", exit) + writeFile(t, filepath.Join(dir, name), body, 0o755) + return dir +} + +func writeFile(t *testing.T, path, body string, mode os.FileMode) { + t.Helper() + if err := os.WriteFile(path, []byte(body), mode); err != nil { + t.Fatalf("WriteFile %s: %v", path, err) + } +} + +// onlyPath replaces PATH with dirs for the length of the test, so a lookup finds +// exactly what the test put there and nothing the machine happens to have. +func onlyPath(t *testing.T, dirs ...string) { + t.Helper() + t.Setenv("PATH", strings.Join(dirs, string(os.PathListSeparator))) +} diff --git a/internal/hooks/harness_test.go b/internal/hooks/harness_test.go new file mode 100644 index 0000000..3bd6f25 --- /dev/null +++ b/internal/hooks/harness_test.go @@ -0,0 +1,345 @@ +package hooks + +import ( + "encoding/json" + "os" + "testing" + + "github.com/protonspy/spec-claude-code/internal/paths" +) + +// scaffolded makes root look like a workspace for the given harnesses, which is +// the only thing the harness-hook path asks of it: the manifest is the marker. +func scaffolded(t *testing.T, all ...paths.Harness) string { + t.Helper() + root := t.TempDir() + for _, h := range all { + if err := os.MkdirAll(h.Config(root), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(h.Manifest(root), []byte(`{"harness":"`+h.ID+`"}`), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + } + return root +} + +func readSettings(t *testing.T, path string) map[string]any { + t.Helper() + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile %s: %v", path, err) + } + var out map[string]any + if err := json.Unmarshal(raw, &out); err != nil { + t.Fatalf("the settings file scc wrote is not valid JSON: %v\n%s", err, raw) + } + return out +} + +// Every event scc registers has to answer the three questions the report asks of +// it. A stage with no name is an entry whose command runs `scc hooks run ` and +// does nothing; a timeout of zero is the harness's own default rather than the +// one this reasoning picked. +func TestEveryEventIsFullyDescribed(t *testing.T) { + for _, e := range Events() { + if e.Stage() == "" { + t.Errorf("%s has no stage name", e) + } + if e.Why() == "" { + t.Errorf("%s has no line for the report", e) + } + if e.Timeout() <= 0 { + t.Errorf("%s has timeout %d", e, e.Timeout()) + } + } + // The prompt stage sits between the keystroke and the agent, so it is an order + // of magnitude tighter than the two that run while nobody is waiting. + if UserPromptSubmit.Timeout() >= SessionStart.Timeout() { + t.Errorf("the prompt stage (%ds) is not tighter than session start (%ds)", + UserPromptSubmit.Timeout(), SessionStart.Timeout()) + } + if Stop.Timeout() < SessionStart.Timeout() { + t.Errorf("stop (%ds) is tighter than session start (%ds), though both sync the graph", + Stop.Timeout(), SessionStart.Timeout()) + } + var unknown Event = "Whatever" + if unknown.Stage() != "" || unknown.Why() != "" { + t.Error("an event scc does not know answered as though it did") + } + if unknown.Timeout() != SessionStart.Timeout() { + t.Errorf("an unknown event's timeout is %d, want the default", unknown.Timeout()) + } +} + +// Install, then install again: the second run has nothing to do, and a settings +// file that changed on every run would diff against itself in the repository it +// is committed to. +func TestInstallHarnessIsIdempotent(t *testing.T) { + root := scaffolded(t, paths.Claude) + + first := InstallHarness(root) + if len(first) != len(Events()) { + t.Fatalf("got %d statuses, want %d: %+v", len(first), len(Events()), first) + } + for _, st := range first { + if st.State != Installed || st.Action != Added { + t.Errorf("%s: state=%q action=%q (%s)", st.Event, st.State, st.Action, st.Note) + } + } + before, err := os.ReadFile(paths.Claude.Settings(root)) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + + for _, st := range InstallHarness(root) { + if st.State != Installed || st.Action != Present { + t.Errorf("second run: %s state=%q action=%q", st.Event, st.State, st.Action) + } + } + after, err := os.ReadFile(paths.Claude.Settings(root)) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(before) != string(after) { + t.Errorf("the second install rewrote the file:\n%s\n---\n%s", before, after) + } + if !HarnessOK(LookHarness(root)) { + t.Error("LookHarness does not agree the hooks are installed") + } +} + +// The file belongs to the harness and to whoever else writes into it. scc's own +// entries are replaced; every other entry in the same event, every other event, +// and every key this build has never heard of survive exactly as they were. +func TestInstallHarnessPreservesEverythingItDoesNotOwn(t *testing.T) { + root := scaffolded(t, paths.Claude) + path := paths.Claude.Settings(root) + if err := os.MkdirAll(paths.Claude.Config(root), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + const existing = `{ + "model": "opus", + "permissions": {"allow": ["Bash(ls:*)"]}, + "somethingSccHasNeverHeardOf": {"deep": [1, 2]}, + "hooks": { + "Stop": [{"hooks": [{"type": "command", "command": "make lint"}]}], + "PreToolUse": [{"hooks": [{"type": "command", "command": "somebody-elses-tool"}]}] + } +}` + if err := os.WriteFile(path, []byte(existing), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + InstallHarness(root) + got := readSettings(t, path) + + for _, key := range []string{"model", "permissions", "somethingSccHasNeverHeardOf"} { + if _, ok := got[key]; !ok { + t.Errorf("install dropped %q from somebody else's settings file", key) + } + } + hooks, _ := got["hooks"].(map[string]any) + if _, ok := hooks["PreToolUse"]; !ok { + t.Error("install dropped an event scc does not register") + } + stop, _ := hooks["Stop"].([]any) + if len(stop) != 2 { + t.Fatalf("Stop has %d entries, want somebody else's plus scc's: %+v", len(stop), stop) + } + if !hasCommand(stop, "make lint") { + t.Errorf("install replaced somebody else's Stop hook: %+v", stop) + } + if !hasCommand(stop, command(StageStop)) { + t.Errorf("install did not add scc's own Stop hook: %+v", stop) + } +} + +// Remove is the same promise from the other side, and the detail that matters is +// what it leaves: somebody else's entries, and no empty key where scc's used to +// be. A key holding an empty array is scc's litter in a file it does not own. +func TestRemoveHarnessLeavesNoLitter(t *testing.T) { + root := scaffolded(t, paths.Claude) + path := paths.Claude.Settings(root) + if err := os.MkdirAll(paths.Claude.Config(root), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + const existing = `{"hooks": {"Stop": [{"hooks": [{"type": "command", "command": "make lint"}]}]}}` + if err := os.WriteFile(path, []byte(existing), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + InstallHarness(root) + for _, st := range RemoveHarness(root) { + if st.State != Missing || st.Action != Removed { + t.Errorf("%s: state=%q action=%q", st.Event, st.State, st.Action) + } + } + + got := readSettings(t, path) + hooks, ok := got["hooks"].(map[string]any) + if !ok { + t.Fatalf("remove took the whole hooks object with somebody else's entry in it: %+v", got) + } + if _, ok := hooks[string(SessionStart)]; ok { + t.Error("remove left an empty SessionStart key behind") + } + stop, _ := hooks[string(Stop)].([]any) + if len(stop) != 1 || !hasCommand(stop, "make lint") { + t.Errorf("remove did not leave somebody else's Stop hook alone: %+v", stop) + } + + // A second remove has nothing to take out and says so rather than rewriting. + for _, st := range RemoveHarness(root) { + if st.Action != Skipped { + t.Errorf("second remove: %s action=%q, want %q", st.Event, st.Action, Skipped) + } + } +} + +func hasCommand(entries []any, want string) bool { + for _, e := range entries { + obj, _ := e.(map[string]any) + inner, _ := obj["hooks"].([]any) + for _, h := range inner { + hook, _ := h.(map[string]any) + if cmd, _ := hook["command"].(string); cmd == want { + return true + } + } + } + return false +} + +// A file scc cannot parse is a file scc will not rewrite: reformatting somebody's +// broken JSON into valid JSON of scc's choosing destroys what they were in the +// middle of fixing. +func TestABrokenSettingsFileIsReportedAndNotRewritten(t *testing.T) { + root := scaffolded(t, paths.Claude) + path := paths.Claude.Settings(root) + if err := os.MkdirAll(paths.Claude.Config(root), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + const broken = "{\n \"model\": \"opus\",\n" + if err := os.WriteFile(path, []byte(broken), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + all := InstallHarness(root) + if len(all) != 1 || all[0].State != Foreign || all[0].Note == "" { + t.Fatalf("install on a broken file = %+v, want one Foreign status with a note", all) + } + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(raw) != broken { + t.Errorf("scc rewrote a file it could not parse:\n%s", raw) + } +} + +// An empty file is one somebody created and never filled in, not a syntax error +// to refuse over. +func TestAnEmptySettingsFileIsFilledInRatherThanRefused(t *testing.T) { + root := scaffolded(t, paths.Claude) + path := paths.Claude.Settings(root) + if err := os.MkdirAll(paths.Claude.Config(root), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(path, []byte(" \n"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if !HarnessOK(InstallHarness(root)) { + t.Error("install refused an empty settings file") + } +} + +// Recognition is by the exact command, because a loose match is scc editing +// somebody else's entry. Both of these were real near-misses for a substring +// test: one is another tool, the other is scc's command with something appended. +func TestOnlySccsOwnEntriesAreRecognized(t *testing.T) { + for _, cmd := range []string{ + command(StageStop), + " " + command(StageStop) + " ", + } { + if !ours(cmd) { + t.Errorf("ours(%q) = false, and scc wrote that command", cmd) + } + } + for _, cmd := range []string{ + "my-" + command(StageStop), + command(StageStop) + " | tee log", + "scc validate", + "", + } { + if ours(cmd) { + t.Errorf("ours(%q) = true, and scc did not write that command", cmd) + } + } +} + +// An entry that is current but spelled with different whitespace or key order is +// not stale: the comparison is by value, because the file is hand-editable and a +// byte comparison would report a rewrite on every formatting difference. +func TestAReformattedEntryIsNotStale(t *testing.T) { + root := scaffolded(t, paths.Claude) + InstallHarness(root) + path := paths.Claude.Settings(root) + + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + var doc any + if err := json.Unmarshal(raw, &doc); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + compact, err := json.Marshal(doc) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + if err := os.WriteFile(path, compact, 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if !HarnessOK(LookHarness(root)) { + t.Error("a reformatted settings file was reported as not installed") + } + + // An entry from another build — same command, different timeout — is stale, + // and installing replaces it rather than adding a second one. + stale := `{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"` + + command(StageStop) + `","timeout":1}]}]}}` + if err := os.WriteFile(path, []byte(stale), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + var replaced bool + for _, st := range InstallHarness(root) { + if st.Event == Stop { + replaced = st.Action == Replaced + } + } + if !replaced { + t.Error("an entry from another build was not reported as replaced") + } + hooks, _ := readSettings(t, path)["hooks"].(map[string]any) + if entries, _ := hooks[string(Stop)].([]any); len(entries) != 1 { + t.Errorf("replacing left %d entries, want one: %+v", len(entries), entries) + } +} + +// Codex and opencode are not missing a file scc could write — they have no +// mechanism that would read one, so they contribute nothing rather than a +// finding. +func TestAHarnessWithNoHookSurfaceContributesNothing(t *testing.T) { + root := scaffolded(t, paths.Codex, paths.OpenCode) + if all := InstallHarness(root); len(all) != 0 { + t.Errorf("install reported %+v for harnesses with no hook surface", all) + } + if all := LookHarness(root); len(all) != 0 { + t.Errorf("look reported %+v for harnesses with no hook surface", all) + } + // And HarnessOK over nothing is true: there is nothing missing. + if !HarnessOK(nil) { + t.Error("HarnessOK(nil) is false, so a Codex-only workspace reports a failure") + } +} diff --git a/internal/hooks/hooks_test.go b/internal/hooks/hooks_test.go index 733b91c..56658e1 100644 --- a/internal/hooks/hooks_test.go +++ b/internal/hooks/hooks_test.go @@ -341,3 +341,109 @@ func TestPosixReadsTheInterpreterNotTheLineEnding(t *testing.T) { } } } + +// Look changes nothing, which is the whole of what `scc hooks check` needs and +// the one thing a report must never get wrong: a check that installed what it +// was asked to report on would make the answer true by writing it. +func TestLookReportsWithoutWriting(t *testing.T) { + dir := repo(t) + + all, err := Look(dir) + if err != nil { + t.Fatalf("Look: %v", err) + } + if len(all) != len(Stages()) { + t.Fatalf("got %d stages, want %d", len(all), len(Stages())) + } + for _, s := range all { + if s.State != Missing { + t.Errorf("%s: state=%q on a repository with no hooks", s.Stage, s.State) + } + if _, err := os.Stat(s.Path); !os.IsNotExist(err) { + t.Errorf("Look created %s", s.Path) + } + } + if OK(all) { + t.Error("OK true for a repository with no hooks installed") + } + + if _, err := Install(dir, false); err != nil { + t.Fatalf("Install: %v", err) + } + all, err = Look(dir) + if err != nil { + t.Fatalf("Look: %v", err) + } + if !OK(all) { + t.Errorf("OK false right after a successful install: %+v", all) + } + + // OK over nothing is false, and that is deliberate: an empty report means the + // question could not be answered, not that everything is fine. + if OK(nil) { + t.Error("OK(nil) is true, so an unanswerable check reads as a pass") + } + if _, err := Look(t.TempDir()); err == nil { + t.Error("Look outside a repository returned no error") + } +} + +// A hook scc appended to keeps everything of theirs when scc's block comes back +// out — the other half of the promise --force makes. +func TestRemoveLeavesAForeignHookItAppendedTo(t *testing.T) { + dir := repo(t) + hooks, err := Dir(dir) + if err != nil { + t.Fatalf("Dir: %v", err) + } + path := filepath.Join(hooks, string(PreCommit)) + const theirs = "#!/bin/sh\necho theirs\n" + if err := os.MkdirAll(hooks, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(path, []byte(theirs), 0o755); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + if _, err := Install(dir, true); err != nil { + t.Fatalf("Install --force: %v", err) + } + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if !strings.Contains(string(raw), "echo theirs") { + t.Fatalf("--force overwrote the existing hook:\n%s", raw) + } + + all, err := Remove(dir) + if err != nil { + t.Fatalf("Remove: %v", err) + } + for _, s := range all { + if s.Stage != PreCommit { + continue + } + if s.Action != Removed || s.State != Foreign { + t.Errorf("pre-commit: state=%q action=%q, want the foreign script left behind", s.State, s.Action) + } + } + raw, err = os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if !strings.Contains(string(raw), "echo theirs") { + t.Errorf("remove took the user's own hook with it:\n%s", raw) + } + if strings.Contains(string(raw), Prog+" hooks run") { + t.Errorf("remove left scc's block behind:\n%s", raw) + } + + // A second remove has nothing of scc's to take out. + all, _ = Remove(dir) + for _, s := range all { + if s.Action != Skipped { + t.Errorf("second remove: %s action=%q, want %q", s.Stage, s.Action, Skipped) + } + } +} diff --git a/internal/jail/jail_test.go b/internal/jail/jail_test.go index 0cdfac3..94701f8 100644 --- a/internal/jail/jail_test.go +++ b/internal/jail/jail_test.go @@ -1,6 +1,10 @@ package jail import ( + "fmt" + "io" + "os" + "path/filepath" "runtime" "strings" "testing" @@ -113,3 +117,123 @@ func TestInstallHintNamesSomethingToRun(t *testing.T) { t.Errorf("InstallHint() = %q", InstallHint()) } } + +// The binary half, driven against stand-ins on a replaced PATH: the real install +// is a Rust build that takes minutes. +func TestTheBinaryHalfAnswersAboutWhatIsOnPATH(t *testing.T) { + onlyPath(t, t.TempDir()) + if Available() { + t.Error("Available true with no cargo on PATH") + } + if p, ok := Path(); ok { + t.Errorf("Path found %q with an empty PATH", p) + } + if got := Version("definitely-not-a-real-program"); got != "" { + t.Errorf("Version invented %q for a binary that is not there", got) + } + // A build that cannot answer means scc asks for nothing rather than guessing + // — a sandbox opened by a guess is the failure this whole feature prevents. + if got := Help("definitely-not-a-real-program"); got != "" { + t.Errorf("Help invented %q", got) + } + // Missing cargo is reported as itself: a Rust toolchain is a different problem + // from a build that broke. + err := Install(io.Discard, io.Discard) + if err == nil { + t.Fatal("Install with no cargo returned no error") + } + if !strings.Contains(err.Error(), "cargo is not on PATH") { + t.Errorf("error = %q, want it to name the missing toolchain", err) + } + + onlyPath(t, fakeBin(t, "cargo", "", 0), fakeBin(t, Bin, "ai-jail 0.4.0", 0)) + if !Available() { + t.Error("Available false with cargo right there") + } + p, ok := Path() + if !ok { + t.Fatal("Path did not find the binary on PATH") + } + if got := Version(p); got != "ai-jail 0.4.0" { + t.Errorf("Version = %q", got) + } + if got := Help(p); !strings.Contains(got, "ai-jail 0.4.0") { + t.Errorf("Help = %q, want whatever the build printed", got) + } + if err := Install(io.Discard, io.Discard); err != nil { + t.Errorf("Install with a working cargo: %v", err) + } + + onlyPath(t, fakeBin(t, "cargo", "", 1)) + if err := Install(io.Discard, io.Discard); err == nil { + t.Error("Install returned no error when cargo failed") + } else if !strings.Contains(err.Error(), InstallCmd()) { + t.Errorf("error = %q, which does not name the command that failed", err) + } +} + +// Every platform answers all three consistently: a platform with a backend names +// it and hides something, and one without names neither and says why. +func TestThePlatformAnswersAreConsistent(t *testing.T) { + if Supported() { + if Backend() == "" { + t.Error("a supported platform names no backend") + } + if HiddenRoot() == "" { + t.Error("a supported platform hides nothing, so no toolchain would ever be mapped") + } + if !strings.Contains(InstallHint(), InstallCmd()) && runtime.GOOS != "darwin" { + t.Errorf("InstallHint = %q, which does not carry the install command", InstallHint()) + } + return + } + if Backend() != "" { + t.Errorf("an unsupported platform names the backend %q", Backend()) + } + if HiddenRoot() != "" { + t.Errorf("an unsupported platform hides %q", HiddenRoot()) + } + // Somebody on Windows has to be told where the answer is, and WSL2 is a real + // answer rather than a workaround. + if !strings.Contains(Unsupported(), "WSL2") { + t.Errorf("Unsupported() = %q, which does not name the way forward", Unsupported()) + } +} + +// fakeBin writes an executable named name into its own directory and returns that +// directory, so a test can put it on PATH and drive a lookup or an install without +// the real tool being installed on the machine running the suite. +func fakeBin(t *testing.T, name, stdout string, exit int) string { + t.Helper() + dir := t.TempDir() + if runtime.GOOS == "windows" { + body := "@echo off\r\n" + if stdout != "" { + body += "echo " + stdout + "\r\n" + } + body += fmt.Sprintf("exit /b %d\r\n", exit) + writeBin(t, filepath.Join(dir, name+".cmd"), body, 0o644) + return dir + } + body := "#!/bin/sh\n" + if stdout != "" { + body += "printf '%s\n' '" + strings.ReplaceAll(stdout, "'", `'\''`) + "'\n" + } + body += fmt.Sprintf("exit %d\n", exit) + writeBin(t, filepath.Join(dir, name), body, 0o755) + return dir +} + +func writeBin(t *testing.T, path, body string, mode os.FileMode) { + t.Helper() + if err := os.WriteFile(path, []byte(body), mode); err != nil { + t.Fatalf("WriteFile %s: %v", path, err) + } +} + +// onlyPath replaces PATH with dirs for the length of the test, so a lookup finds +// exactly what the test put there and nothing the machine happens to have. +func onlyPath(t *testing.T, dirs ...string) { + t.Helper() + t.Setenv("PATH", strings.Join(dirs, string(os.PathListSeparator))) +} diff --git a/internal/manifest/manifest_test.go b/internal/manifest/manifest_test.go index efa0e49..655cb1d 100644 --- a/internal/manifest/manifest_test.go +++ b/internal/manifest/manifest_test.go @@ -416,3 +416,67 @@ func TestFlatCommandKeysAreReadAndMigrated(t *testing.T) { t.Errorf("check = %+v, want the migrated values", check) } } + +// CarryOver exists because `scc init` and `scc update` both build the next +// manifest from scratch and then fill in the file entries — right for the entries +// and silently destructive for everything else. Before the gate's commands lived +// here the only casualty was the unknown-field preservation; now a re-run of init +// would throw away the whole configuration. +func TestCarryOverKeepsEverythingThatIsNotAFileEntry(t *testing.T) { + prior := New("30", paths.Claude) + prior.Build = "go build ./..." + prior.Test = "go run -C tools ./testreport" + prior.Lint = "golangci-lint run" + prior.Format = "skipped" + prior.MinCoverage = 91 + prior.Codegraph = []string{"backend/src", "frontend/src"} + prior.Set("CLAUDE.md", "abc", "30") + + // A key a newer scc wrote that this build has never heard of. + raw, err := prior.Bytes() + if err != nil { + t.Fatalf("Bytes: %v", err) + } + withUnknown := strings.Replace(string(raw), "{\n", "{\n \"somethingNewer\": {\"deep\": 1},\n", 1) + prior = &Manifest{} + if err := json.Unmarshal([]byte(withUnknown), prior); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + + next := New("31", paths.Claude) + next.CarryOver(prior) + + if next.Build != "go build ./..." || next.Format != "skipped" { + t.Errorf("the commands did not carry over: %+v", next) + } + if next.MinCoverage != 91 { + t.Errorf("MinCoverage = %v, want 91", next.MinCoverage) + } + if strings.Join(next.Codegraph, " ") != "backend/src frontend/src" { + t.Errorf("the graph scope did not carry over: %v", next.Codegraph) + } + out, err := next.Bytes() + if err != nil { + t.Fatalf("Bytes: %v", err) + } + if !strings.Contains(string(out), "somethingNewer") { + t.Errorf("a key this build does not know was dropped:\n%s", out) + } + // The file entries are the one thing it must *not* carry: they belong to the + // scaffold run that is rebuilding them. + if len(next.Files) != 0 { + t.Errorf("CarryOver brought the file entries with it: %+v", next.Files) + } + // And the scope is a copy rather than the prior's own slice. + next.Codegraph[0] = "mutated" + if prior.Codegraph[0] == "mutated" { + t.Error("CarryOver aliased the prior manifest's scope") + } + + // A nil prior is a first scaffold, and it leaves the new manifest alone. + fresh := New("31", paths.Claude) + fresh.CarryOver(nil) + if fresh.Build != "" || fresh.MinCoverage != 0 { + t.Errorf("CarryOver(nil) invented configuration: %+v", fresh) + } +} diff --git a/internal/mdblock/mdblock.go b/internal/mdblock/mdblock.go index 6893fd5..512aa68 100644 --- a/internal/mdblock/mdblock.go +++ b/internal/mdblock/mdblock.go @@ -17,6 +17,7 @@ import ( "fmt" "strings" + "github.com/protonspy/spec-claude-code/internal/mdscan" "github.com/protonspy/spec-claude-code/internal/textutil" ) @@ -46,6 +47,35 @@ const ( Replaced Action = "replaced" ) +// span returns the half-open range of the block in doc, markers included. +// +// It is the one place any of this package decides where the block is, because four +// methods that answered that question separately were four chances to disagree — and +// the answer is not "where the markers are". A document that *documents* the markers +// writes them in a sentence or shows them in a fenced example, and both of those were +// read as a block: this repository's own entry file names RTK's opening and closing +// markers in one line of prose, which made Splice stand ready to replace that +// sentence, and names scc's CodeGraph opener alone, which made the whole file look +// malformed so the block was never written. +// +// So the search runs over mdscan.Mask — fences and code spans blanked, HTML comments +// kept, every byte offset preserved — and the result indexes the original. +// +// start is -1 when doc carries no opening marker; end is -1 when it carries one with +// no close, which is malformed rather than blockless. +func (m Markers) span(doc string) (start, end int) { + masked := mdscan.Mask(doc) + start = strings.Index(masked, m.Open) + if start < 0 { + return -1, -1 + } + rel := strings.Index(masked[start:], m.Close) + if rel < 0 { + return start, -1 + } + return start, start + rel + len(m.Close) +} + // Splice returns doc with block present exactly once, and what it had to do to get // there. // @@ -58,7 +88,8 @@ const ( // // A document carrying an opening marker with no closing one is malformed rather than // blockless, and it is an error: appending a second block there would leave the file -// with two openings and one close, which no tool could then update. +// with two openings and one close, which no tool could then update. A marker written +// inside a code span or a fenced example is neither — see span. func (m Markers) Splice(doc, block string, keep bool) (string, Action, error) { block = strings.TrimRight(textutil.NormalizeNewlines(block), "\n") eol := "\n" @@ -67,9 +98,9 @@ func (m Markers) Splice(doc, block string, keep bool) (string, Action, error) { block = strings.ReplaceAll(block, "\n", "\r\n") } - start := strings.Index(doc, m.Open) + start, end := m.span(doc) if start < 0 { - if strings.Contains(doc, m.Close) { + if strings.Contains(mdscan.Mask(doc), m.Close) { return "", "", fmt.Errorf("found %s with no opening marker", m.Close) } trimmed := strings.TrimRight(doc, " \t\r\n") @@ -78,17 +109,13 @@ func (m Markers) Splice(doc, block string, keep bool) (string, Action, error) { } return trimmed + eol + eol + block + eol, Added, nil } - - rest := doc[start:] - end := strings.Index(rest, m.Close) if end < 0 { return "", "", fmt.Errorf("found %s with no closing %s", m.Open+" …", m.Close) } - end += len(m.Close) - if keep || rest[:end] == block { + if keep || doc[start:end] == block { return doc, Present, nil } - return doc[:start] + block + doc[start+end:], Replaced, nil + return doc[:start] + block + doc[end:], Replaced, nil } // Block returns the marker-delimited block in doc, markers included, or "" when doc @@ -98,16 +125,11 @@ func (m Markers) Splice(doc, block string, keep bool) (string, Action, error) { // written — the entry file is preloaded into every request of the session, so the // size of a block is the thing worth comparing when two of them say the same words. func (m Markers) Block(doc string) string { - start := strings.Index(doc, m.Open) - if start < 0 { + start, end := m.span(doc) + if start < 0 || end < 0 { return "" } - rest := doc[start:] - end := strings.Index(rest, m.Close) - if end < 0 { - return "" - } - return rest[:end+len(m.Close)] + return doc[start:end] } // Version reports what the opening marker in doc claims — "v2" for @@ -116,16 +138,16 @@ func (m Markers) Block(doc string) string { // Advisory: it is printed so a run that left a block alone says which one it left, // and never compared. Version ordering belongs to whoever defines the block. func (m Markers) Version(doc string) string { - start := strings.Index(doc, m.Open) - if start < 0 { + start, end := m.span(doc) + if start < 0 || end < 0 { return "" } - rest := doc[start+len(m.Open):] - end := strings.Index(rest, "-->") - if end < 0 { + rest := doc[start+len(m.Open) : end] + stop := strings.Index(rest, "-->") + if stop < 0 { return "" } - return strings.TrimSpace(rest[:end]) + return strings.TrimSpace(rest[:stop]) } // Remove returns doc with the block taken out, and whether there was one. @@ -139,17 +161,12 @@ func (m Markers) Version(doc string) string { // growing stack of empty lines after each remove-and-splice cycle is a file that // records how many times the tool ran, which is not information anybody wants. func (m Markers) Remove(doc string) (string, bool) { - start := strings.Index(doc, m.Open) - if start < 0 { - return doc, false - } - rest := doc[start:] - end := strings.Index(rest, m.Close) - if end < 0 { + start, end := m.span(doc) + if start < 0 || end < 0 { return doc, false } head := strings.TrimRight(doc[:start], " \t\r\n") - tail := strings.TrimLeft(doc[start+end+len(m.Close):], " \t\r\n") + tail := strings.TrimLeft(doc[end:], " \t\r\n") eol := "\n" if strings.Contains(doc, "\r\n") { eol = "\r\n" diff --git a/internal/mdblock/mdblock_test.go b/internal/mdblock/mdblock_test.go index 3361c73..95f6c12 100644 --- a/internal/mdblock/mdblock_test.go +++ b/internal/mdblock/mdblock_test.go @@ -82,3 +82,139 @@ func TestSpliceRefusesAnUnclosedBlock(t *testing.T) { t.Error("Splice accepted a closing marker with no opening one") } } + +// A document that documents the markers is not a document carrying the block. This +// repository's own entry file writes both of RTK's markers in one sentence of prose, +// which made Splice read that sentence as the block and stand ready to replace it, +// and made Block report a block that was never spliced — so `scc launch` skipped the +// RTK wiring it believed was already done. +func TestMarkersInProseAreNotTheBlock(t *testing.T) { + prose := "scc uses RTK's own markers: `` … ``, verified.\n" + + if got := alpha.Block(prose); got != "" { + t.Errorf("Block found a block in prose: %q", got) + } + out, action, err := alpha.Splice(prose, alphaBlock, false) + if err != nil { + t.Fatalf("Splice: %v", err) + } + if action != Added { + t.Errorf("action = %q, want %q", action, Added) + } + if !strings.Contains(out, prose) { + t.Errorf("Splice overwrote the sentence:\n%s", out) + } + if !strings.Contains(out, alphaBlock) { + t.Errorf("Splice did not append the block:\n%s", out) + } + if back, had := alpha.Remove(out); !had || !strings.Contains(back, prose) { + t.Errorf("Remove took the prose with it (had = %v):\n%s", had, back) + } +} + +// The other half of the same defect, and the one that was actually reported: an entry +// file naming only the opening marker — in a code span, in a sentence about what the +// markers are — was read as malformed, so the block was never written at all. +func TestAnOpeningMarkerInACodeSpanIsNotMalformed(t *testing.T) { + prose := "Its markers are scc's own — `` — which is deliberate.\n" + + out, action, err := alpha.Splice(prose, alphaBlock, false) + if err != nil { + t.Fatalf("Splice: %v", err) + } + if action != Added { + t.Errorf("action = %q, want %q", action, Added) + } + if !strings.Contains(out, prose) { + t.Errorf("Splice overwrote the sentence:\n%s", out) + } +} + +// A fenced example is the same case one construct over: a README showing the block it +// documents must not be mistaken for a workspace carrying it. +func TestAFencedExampleIsNotTheBlock(t *testing.T) { + doc := "# Docs\n\n```\n" + alphaBlock + "\n```\n" + + if got := alpha.Version(doc); got != "" { + t.Errorf("Version read a fenced example: %q", got) + } + out, action, err := alpha.Splice(doc, alphaBlock, false) + if err != nil || action != Added { + t.Fatalf("action = %q, err = %v", action, err) + } + if !strings.Contains(out, "```\n"+alphaBlock+"\n```") { + t.Errorf("Splice rewrote the fenced example:\n%s", out) + } +} + +// Remove is the counterpart to Splice, and its promise is what survives: the file +// that carried the block alongside somebody else's content is still theirs +// afterwards, and the blank lines the block sat between go with it — a file left +// with a growing stack of empty lines records how many times the tool ran. +func TestRemoveTakesTheBlockAndItsBlankLines(t *testing.T) { + head := "# CLAUDE.md\n\nThe user's own prose.\n" + tail := "\nAnd more of their prose.\n" + + // Between two pieces of the user's own text. + doc, _, err := alpha.Splice(head+tail, alphaBlock, false) + if err != nil { + t.Fatalf("Splice: %v", err) + } + out, found := alpha.Remove(doc) + if !found { + t.Fatal("Remove did not find the block it had just spliced") + } + if strings.Contains(out, "Alpha says this.") { + t.Errorf("Remove left the block behind:\n%s", out) + } + if !strings.Contains(out, "The user's own prose.") || !strings.Contains(out, "And more of their prose.") { + t.Errorf("Remove took the user's text with it:\n%s", out) + } + if strings.Contains(out, "\n\n\n") { + t.Errorf("Remove left a stack of blank lines:\n%q", out) + } + + // A document that is nothing but the block comes back empty rather than as a + // file holding one newline. + if out, found := alpha.Remove(alphaBlock + "\n"); !found || strings.TrimSpace(out) != "" { + t.Errorf("removing the whole document left %q (found = %v)", out, found) + } + + // A document with no block is returned unchanged, and says so. + plain := "# CLAUDE.md\n\nNothing here.\n" + if out, found := alpha.Remove(plain); found || out != plain { + t.Errorf("Remove on a blockless document returned %q, %v", out, found) + } + // An opening marker with no close is malformed, not a block to take out. + unclosed := "\nno close\n" + if out, found := alpha.Remove(unclosed); found || out != unclosed { + t.Errorf("Remove on an unclosed block returned %q, %v", out, found) + } + // And a marker written in prose is not a block either, on this path as on + // every other. + prose := "documented as `` … `` in a sentence.\n" + if out, found := alpha.Remove(prose); found || out != prose { + t.Errorf("Remove took a sentence out of the document: %q, %v", out, found) + } +} + +// CRLF is preserved on every path, because rewriting every line of somebody +// else's file is not a change they asked for. +func TestSpliceAndRemovePreserveCRLF(t *testing.T) { + doc := "# CLAUDE.md\r\n\r\nTheir prose.\r\n" + + out, _, err := alpha.Splice(doc, alphaBlock, false) + if err != nil { + t.Fatalf("Splice: %v", err) + } + if strings.Contains(strings.ReplaceAll(out, "\r\n", ""), "\n") { + t.Errorf("the splice introduced a bare LF into a CRLF document:\n%q", out) + } + back, found := alpha.Remove(out) + if !found { + t.Fatal("Remove did not find the block in a CRLF document") + } + if strings.Contains(strings.ReplaceAll(back, "\r\n", ""), "\n") { + t.Errorf("the remove introduced a bare LF:\n%q", back) + } +} diff --git a/internal/mdscan/mdscan.go b/internal/mdscan/mdscan.go index 64f7d1b..b22bfdb 100644 --- a/internal/mdscan/mdscan.go +++ b/internal/mdscan/mdscan.go @@ -309,3 +309,45 @@ func Ticked(line string) bool { m := checkboxRe.FindStringSubmatch(strings.TrimRight(line, "\r")) return m != nil && m[2] != " " } + +// Mask returns content with fenced blocks and inline code spans blanked to spaces, +// byte for byte, so an index into the result is an index into the original. +// +// HTML comments survive, which is the one way it differs from Document.Body and the +// whole reason it exists: the caller is looking for markers that *are* HTML comments, +// and a document explaining those markers in prose is not a document carrying the +// block. `internal/mdblock` measures over this and slices out of the original — this +// repository's own entry file documents both marker pairs, and without the mask the +// splice reads a sentence about the markers as the block itself. +// +// Line endings are not normalized, because the caller preserves the document's own +// and a normalizing mask would hand it offsets into a string it never had. +func Mask(content string) string { + lines := strings.Split(content, "\n") + var fence string + for i, line := range lines { + if fence != "" { + if closesFence(line, fence) { + fence = "" + } + lines[i] = blank(line) + continue + } + if open, delim := opensFence(line); open { + fence = delim + lines[i] = blank(line) + continue + } + lines[i] = maskCodeSpans(line) + } + return strings.Join(lines, "\n") +} + +// blank returns a run of spaces as long as line, keeping a trailing carriage return +// so a CRLF document masks to one of exactly the same shape. +func blank(line string) string { + if strings.HasSuffix(line, "\r") { + return strings.Repeat(" ", len(line)-1) + "\r" + } + return strings.Repeat(" ", len(line)) +} diff --git a/internal/mdscan/mdscan_test.go b/internal/mdscan/mdscan_test.go index a92fd55..0d668ed 100644 --- a/internal/mdscan/mdscan_test.go +++ b/internal/mdscan/mdscan_test.go @@ -299,3 +299,82 @@ func TestLineIsOneBasedAndSafe(t *testing.T) { t.Error("out-of-range Line() did not return empty") } } + +func TestMaskProseHidesExamplesAndKeepsComments(t *testing.T) { + doc := "Prose about `` and its close.\n" + + "```\n\n```\n" + + "\nbody\n\n" + + got := Mask(doc) + + if len(got) != len(doc) { + t.Fatalf("Mask changed the length: %d, want %d", len(got), len(doc)) + } + for _, hidden := range []string{"rtk-instructions", "fenced-marker"} { + if strings.Contains(got, hidden) { + t.Errorf("Mask left %q visible:\n%s", hidden, got) + } + } + for _, kept := range []string{"", "", "body"} { + if !strings.Contains(got, kept) { + t.Errorf("Mask hid %q, which is not an example:\n%s", kept, got) + } + } + if i := strings.Index(got, ""); i != strings.Index(doc, "") { + t.Errorf("Mask moved the marker to %d, want %d", i, strings.Index(doc, "")) + } +} + +func TestMaskProsePreservesCRLFOffsets(t *testing.T) { + doc := "a `` b\r\n\r\n" + + got := Mask(doc) + + if len(got) != len(doc) { + t.Fatalf("Mask changed the length: %d, want %d", len(got), len(doc)) + } + if strings.Contains(got, "") { + t.Errorf("Mask left the code span visible: %q", got) + } + if i := strings.Index(got, ""); i != strings.Index(doc, "") { + t.Errorf("Mask moved the marker to %d, want %d", i, strings.Index(doc, "")) + } +} + +// Ticked is exported rather than reimplemented at the call site, because the +// alternative is a second regular expression somewhere deciding what a ticked box +// looks like — and two answers to that is how a checked task stops counting as +// progress in one place while still counting in another. +func TestTickedReadsOneLine(t *testing.T) { + for line, want := range map[string]bool{ + "- [x] 1.1 (Unit) Done": true, + "- [X] 1.1 (Unit) Done": true, + "- [x] 1.1 (Unit) Done\r": true, + " - [x] 1.1 nested": true, + "- [ ] 1.1 (Unit) Not done": false, + "- [] 1.1 malformed": false, + "* [x] 1.1 with a star": true, + "1. [x] 1.1 an ordered item": false, + "not a checkbox at all": false, + "": false, + } { + if got := Ticked(line); got != want { + t.Errorf("Ticked(%q) = %v, want %v", line, got, want) + } + } +} + +// A marker inside a blockquote or a four-space indented block is deliberately not +// masked: those are constructs mdscan's fence scanner does not model, and the +// splice that reads this is documented as covering fenced blocks and code spans +// only. Pinned so a later change to Mask is a decision rather than a surprise. +func TestMaskDoesNotClaimConstructsItDoesNotModel(t *testing.T) { + for _, doc := range []string{ + "> \n", + " \n", + } { + if !strings.Contains(Mask(doc), "marker") { + t.Errorf("Mask hid a construct it does not model:\n%q", doc) + } + } +} diff --git a/internal/notes/notes_test.go b/internal/notes/notes_test.go index ecd67b5..9dc6410 100644 --- a/internal/notes/notes_test.go +++ b/internal/notes/notes_test.go @@ -373,3 +373,48 @@ func TestRemoveLeavesATombstone(t *testing.T) { t.Error("removing an already-removed note succeeded") } } + +func TestTodayIsADateTheGrammarAccepts(t *testing.T) { + if err := CheckDate(Today()); err != nil { + t.Errorf("Today() = %q, which the grammar rejects: %v", Today(), err) + } + for _, bad := range []string{"", "09-02-2026", "2026-2-9", "yesterday", "2026-13-01"} { + if err := CheckDate(bad); err == nil { + t.Errorf("CheckDate(%q) accepted a date the grammar would not parse back", bad) + } + } +} + +// One note is one line, and what a reader greps for and what a reader is shown +// must not be two formats to learn — so String is Format. +func TestStringIsTheFormatAReaderGrepsFor(t *testing.T) { + n := Note{ + ID: ID(42), Num: 42, Date: "2026-02-09", Tags: []string{"gotcha"}, + Paths: []string{"internal/cli/launch.go"}, + Text: "wrap writes MCP config to the agent's own file", + } + if n.String() != n.Format() { + t.Errorf("String and Format disagree:\n%s\n%s", n.String(), n.Format()) + } + line := n.String() + if strings.Count(line, "\n") != 0 { + t.Errorf("a note spans lines: %q", line) + } + // The index fields come first, which is what makes a grep for a tag a grep for + // a whole note. + for _, want := range []string{ID(42), "2026-02-09", "#gotcha", "@internal/cli/launch.go"} { + if !strings.Contains(line, want) { + t.Errorf("the line does not carry %q: %s", want, line) + } + } + + // And it round-trips: the grammar that writes it is the grammar that reads it. + f := parse(t, "# Notes\n\n## "+Section+"\n\n"+line+"\n") + if len(f.Notes) != 1 { + t.Fatalf("the log cannot read back what it wrote: %s", line) + } + back := f.Notes[0] + if back.Num != n.Num || back.Text != n.Text || strings.Join(back.Tags, ",") != "gotcha" { + t.Errorf("round trip changed the note: %+v", back) + } +} diff --git a/internal/paths/paths_test.go b/internal/paths/paths_test.go new file mode 100644 index 0000000..b7d8f7c --- /dev/null +++ b/internal/paths/paths_test.go @@ -0,0 +1,181 @@ +package paths + +import ( + "path/filepath" + "strings" + "testing" +) + +// Every path scc hands out has to land under the root it was given. The failure +// this guards is not a typo in a Join — it is a segment constant that grows a +// leading separator and silently turns a workspace-relative path into an absolute +// one, which no caller checks and every caller writes to. +func TestEveryPathStaysUnderTheRoot(t *testing.T) { + root := filepath.Join("some", "workspace") + + got := map[string]string{ + "Specs": Specs(root), + "Spec": Spec(root, "checkout"), + "Requirements": Requirements(root, "checkout"), + "Design": Design(root, "checkout"), + "Tasks": Tasks(root, "checkout"), + "Plans": Plans(root), + "Plan": Plan(root, "revamp"), + "Docs": Docs(root), + "Wiki": Wiki(root), + "WikiPages": WikiPages(root), + "ADR": ADR(root), + "Raw": Raw(root), + "Codewiki": Codewiki(root), + "Glossary": Glossary(root), + "Notes": Notes(root), + "Stack": Stack(root), + } + for _, h := range Harnesses() { + got[h.ID+".Config"] = h.Config(root) + got[h.ID+".Agents"] = h.Agents(root) + got[h.ID+".Skills"] = h.Skills(root) + got[h.ID+".Rules"] = h.Rules(root) + got[h.ID+".Rule"] = h.Rule(root, "delivery") + got[h.ID+".Entry"] = h.Entry(root) + got[h.ID+".Manifest"] = h.Manifest(root) + } + + for name, p := range got { + if !strings.HasPrefix(p, root+string(filepath.Separator)) { + t.Errorf("%s = %q, which is not under %q", name, p, root) + } + } +} + +// The named artifacts carry their own extension and segment, and a caller that +// joined the wrong one would write a file every validator then ignores. +func TestArtifactPathsCarryTheirOwnNames(t *testing.T) { + root := "w" + for name, want := range map[string]string{ + Requirements(root, "f"): RequirementsSeg, + Design(root, "f"): DesignSeg, + Tasks(root, "f"): TasksSeg, + Plan(root, "p"): "p.md", + Glossary(root): GlossarySeg, + Notes(root): NotesSeg, + Stack(root): StackSeg, + Claude.Rule(root, "delivery"): "delivery.md", + Claude.Manifest(root): ManifestSeg, + } { + if filepath.Base(name) != want { + t.Errorf("%q ends in %q, want %q", name, filepath.Base(name), want) + } + } +} + +// An empty segment is a capability this harness does not have, and the methods +// that carry one return "" rather than a path into the configuration directory +// itself — which is what a bare Join would produce, and what a caller would then +// write a settings file or a command over. +func TestAnAbsentCapabilityIsEmptyAndNotTheConfigDirectory(t *testing.T) { + root := "w" + for _, h := range Harnesses() { + cmds, settings := h.Commands(root), h.Settings(root) + if (h.CommandsSeg == "") != (cmds == "") { + t.Errorf("%s: CommandsSeg %q but Commands() = %q", h.ID, h.CommandsSeg, cmds) + } + if (h.SettingsSeg == "") != (settings == "") { + t.Errorf("%s: SettingsSeg %q but Settings() = %q", h.ID, h.SettingsSeg, settings) + } + if cmds != "" && cmds == h.Config(root) { + t.Errorf("%s: Commands() resolved to the config directory itself", h.ID) + } + if settings != "" && settings == h.Config(root) { + t.Errorf("%s: Settings() resolved to the config directory itself", h.ID) + } + } +} + +func TestAgentExtFollowsTheFormat(t *testing.T) { + for _, h := range Harnesses() { + want := ".md" + if h.AgentFormat == FormatTOML { + want = ".toml" + } + if got := h.AgentExt(); got != want { + t.Errorf("%s: AgentExt() = %q, want %q", h.ID, got, want) + } + } +} + +func TestParseHarnessAcceptsEveryIDAndNothingElse(t *testing.T) { + for _, want := range Harnesses() { + got, err := ParseHarness(want.ID) + if err != nil { + t.Errorf("ParseHarness(%q): %v", want.ID, err) + continue + } + if got.ID != want.ID { + t.Errorf("ParseHarness(%q) returned %q", want.ID, got.ID) + } + } + for _, id := range []string{"", "CLAUDE", "cursor", "claude "} { + if _, err := ParseHarness(id); err == nil { + t.Errorf("ParseHarness(%q) accepted an id scc does not support", id) + } else if !strings.Contains(err.Error(), HarnessIDs()) { + t.Errorf("ParseHarness(%q) failed without naming the choices: %v", id, err) + } + } +} + +// The probe order is deliberate — it is the order workspace.Find stats the marker +// in — while HarnessIDs is for help text and sorts. Two different orders on +// purpose, and a test each so neither is "fixed" into the other. +func TestHarnessesProbeInOrderAndListAlphabetically(t *testing.T) { + if got := Harnesses()[0].ID; got != Claude.ID { + t.Errorf("the probe order starts at %q, not the default harness", got) + } + ids := strings.Split(HarnessIDs(), ", ") + for i := 1; i < len(ids); i++ { + if ids[i-1] > ids[i] { + t.Errorf("HarnessIDs is not sorted: %q", HarnessIDs()) + break + } + } + if len(ids) != len(Harnesses()) { + t.Errorf("HarnessIDs lists %d of %d harnesses", len(ids), len(Harnesses())) + } +} + +// Skills are checked wherever they are: the Agent Skills format is one standard, +// and a workspace scaffolded for two harnesses has two copies of it to keep +// conforming. A validator searching one directory would pass a broken skill. +func TestSkillDirsCoversEveryHarness(t *testing.T) { + dirs := SkillDirs("w") + if len(dirs) != len(Harnesses()) { + t.Fatalf("SkillDirs returned %d directories for %d harnesses", len(dirs), len(Harnesses())) + } + for i, h := range Harnesses() { + if dirs[i] != h.Skills("w") { + t.Errorf("SkillDirs[%d] = %q, want %q", i, dirs[i], h.Skills("w")) + } + } +} + +// Each harness keeps its own directory, its own entry file and its own manifest. +// Two sharing either would make one workspace's scaffold overwrite the other's, +// and the manifest is also the marker, so a shared one would make the two +// indistinguishable to the workspace walk. +func TestHarnessesDoNotShareTheirOwnFiles(t *testing.T) { + dirs, manifests := map[string]string{}, map[string]string{} + for _, h := range Harnesses() { + if prev, seen := dirs[h.Dir]; seen { + t.Errorf("%s and %s share the directory %q", prev, h.ID, h.Dir) + } + dirs[h.Dir] = h.ID + m := h.Manifest("w") + if prev, seen := manifests[m]; seen { + t.Errorf("%s and %s share the manifest %q", prev, h.ID, m) + } + manifests[m] = h.ID + if h.ID == "" || h.Label == "" || h.Bin == "" || h.EntryFile == "" || h.RulesSeg == "" { + t.Errorf("%+v is missing a field every harness needs", h) + } + } +} diff --git a/internal/render/render_test.go b/internal/render/render_test.go new file mode 100644 index 0000000..652c090 --- /dev/null +++ b/internal/render/render_test.go @@ -0,0 +1,137 @@ +package render + +import ( + "io" + "os" + "strings" + "testing" +) + +// capture swaps the real streams for pipes and returns what each one received. +// +// The package writes to os.Stdout and os.Stderr by name rather than to an +// io.Writer, which is deliberate — the CLI has one terminal and nothing to inject +// — so a test that wants the bytes has to take the file descriptors. +func capture(t *testing.T, f func()) (stdout, stderr string) { + t.Helper() + outR, outW, err := os.Pipe() + if err != nil { + t.Fatalf("Pipe: %v", err) + } + errR, errW, err := os.Pipe() + if err != nil { + t.Fatalf("Pipe: %v", err) + } + realOut, realErr := os.Stdout, os.Stderr + os.Stdout, os.Stderr = outW, errW + + // One channel each, drained concurrently: a single channel would leave the two + // streams told apart by whichever write flushed first, which is not a fact. + outC, errC := make(chan string, 1), make(chan string, 1) + read := func(r *os.File, c chan<- string) { + b, _ := io.ReadAll(r) + c <- string(b) + } + go read(outR, outC) + go read(errR, errC) + + f() + os.Stdout, os.Stderr = realOut, realErr + outW.Close() + errW.Close() + return <-outC, <-errC +} + +// The split is the contract: stdout stays a clean stream a caller can pipe into +// jq, so every diagnostic goes to stderr. A status line on the wrong stream +// corrupts the JSON document of every --json command at once. +func TestStatusLinesSplitAcrossStreams(t *testing.T) { + for _, tc := range []struct { + name string + call func() + glyph string + stderr bool + }{ + {"Info", func() { Info("hello") }, "•", false}, + {"OK", func() { OK("hello") }, "✓", false}, + {"Ask", func() { Ask("hello") }, "?", false}, + {"Warn", func() { Warn("hello") }, "!", true}, + {"Err", func() { Err("hello") }, "✗", true}, + {"Detail", func() { Detail("hello") }, "", true}, + } { + t.Run(tc.name, func(t *testing.T) { + stdout, stderr := capture(t, tc.call) + got, quiet := stdout, stderr + if tc.stderr { + got, quiet = stderr, stdout + } + if !strings.Contains(got, "hello") { + t.Errorf("message missing from the stream it belongs on: %q", got) + } + if strings.Contains(quiet, "hello") { + t.Errorf("message also reached the other stream: %q", quiet) + } + if tc.glyph != "" && !strings.Contains(got, tc.glyph) { + t.Errorf("%q carries no %s glyph", got, tc.glyph) + } + }) + } +} + +// Ask leaves the cursor where the answer is typed, which is the one thing that +// distinguishes it from Info. +func TestAskDoesNotEndTheLine(t *testing.T) { + stdout, _ := capture(t, func() { Ask("Install it now?") }) + if strings.HasSuffix(stdout, "\n") { + t.Errorf("Ask ended the line, so the answer appears below the question: %q", stdout) + } +} + +func TestColorIsOffWhenNobodyIsLooking(t *testing.T) { + // A pipe is not a character device, so the package-level decision already made + // at init is the one under test here. + if useColor { + t.Skip("this run has a TTY on stdout") + } + if got := Cyan("x"); got != "x" { + t.Errorf("Cyan wrapped %q with escapes on a non-TTY", got) + } +} + +func TestColorWrapsEachGlyphWhenItIsOn(t *testing.T) { + restore := useColor + useColor = true + defer func() { useColor = restore }() + + for name, got := range map[string]string{ + "Cyan": Cyan("x"), + "Green": Green("x"), + "Yellow": Yellow("x"), + "Red": Red("x"), + "Bold": Bold("x"), + } { + if !strings.HasPrefix(got, "\033[") || !strings.HasSuffix(got, "\033[0m") { + t.Errorf("%s produced %q, which is not an escape-wrapped string", name, got) + } + if !strings.Contains(got, "x") { + t.Errorf("%s lost the text: %q", name, got) + } + } +} + +// isTTY answers false for anything that is not a character device, and for a file +// it cannot stat at all — a closed descriptor is the case that would otherwise +// panic on the nil FileInfo. +func TestIsTTY(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "render-*") + if err != nil { + t.Fatalf("CreateTemp: %v", err) + } + if isTTY(f) { + t.Error("a regular file reported as a terminal") + } + f.Close() + if isTTY(f) { + t.Error("a closed file reported as a terminal") + } +} diff --git a/internal/rtk/rtk.go b/internal/rtk/rtk.go index a52a70a..77531c7 100644 --- a/internal/rtk/rtk.go +++ b/internal/rtk/rtk.go @@ -17,6 +17,7 @@ import ( "strings" "github.com/protonspy/spec-claude-code/internal/mdblock" + "github.com/protonspy/spec-claude-code/internal/mdscan" ) // Repo is where the binary is built from. RTK is a Rust program distributed as @@ -71,9 +72,15 @@ var foreigners = []Foreign{ } // ForeignBlock reports the other tool's RTK block in doc, if there is one. +// +// Over mdscan.Mask rather than doc, for the reason mdblock measures there: a file +// that *documents* the other tool's marker — in a code span, in a fenced example — +// is not a file carrying its block, and reporting one sends the reader to +// `headroom unwrap` to remove something nobody wrote. func ForeignBlock(doc string) (Foreign, bool) { + masked := mdscan.Mask(doc) for _, f := range foreigners { - if strings.Contains(doc, f.Open) { + if strings.Contains(masked, f.Open) { return f, true } } diff --git a/internal/rtk/rtk_test.go b/internal/rtk/rtk_test.go index 8bea17b..9ac6981 100644 --- a/internal/rtk/rtk_test.go +++ b/internal/rtk/rtk_test.go @@ -1,6 +1,11 @@ package rtk import ( + "fmt" + "io" + "os" + "path/filepath" + "runtime" "strings" "testing" @@ -261,3 +266,109 @@ func tail(s string) string { } return s } + +// A file that names Headroom's marker in a sentence is not a file carrying Headroom's +// block. Reported, it sends the reader to `headroom unwrap` to remove something nobody +// wrote — measured on this repository's own CLAUDE.md, which explains the marker pair +// in the paragraph about why scc passes --no-context-tool. +func TestASentenceAboutHeadroomsMarkerIsNotHeadroomsBlock(t *testing.T) { + doc := "behind its own marker pair, ``, which scc's do not match.\n" + + if f, ok := ForeignBlock(doc); ok { + t.Errorf("ForeignBlock reported %s's block in a sentence about it", f.Tool) + } + fenced := "```\n\n```\n" + if f, ok := ForeignBlock(fenced); ok { + t.Errorf("ForeignBlock reported %s's block in a fenced example", f.Tool) + } +} + +// The binary half of this package, driven against a stand-in cargo and a stand-in +// rtk on a replaced PATH — the real install is a Rust build that takes minutes and +// is not a thing a test suite does to whoever runs it. +func TestTheBinaryHalfAnswersAboutWhatIsOnPATH(t *testing.T) { + onlyPath(t, t.TempDir()) + if Available() { + t.Error("Available true with no cargo on PATH") + } + if p, ok := Path(); ok { + t.Errorf("Path found %q with an empty PATH", p) + } + if got := Version("definitely-not-a-real-program"); got != "" { + t.Errorf("Version invented %q for a binary that is not there", got) + } + // Missing cargo is reported as itself rather than as a failed build: the user + // has to install a toolchain, which is a different problem from a build that + // broke, and "cargo install failed" would send them looking in the wrong place. + err := Install(io.Discard, io.Discard) + if err == nil { + t.Fatal("Install with no cargo returned no error") + } + if !strings.Contains(err.Error(), "cargo is not on PATH") { + t.Errorf("error = %q, want it to name the missing toolchain", err) + } + + onlyPath(t, fakeBin(t, "cargo", "", 0), fakeBin(t, Bin, "rtk 0.42.4", 0)) + if !Available() { + t.Error("Available false with cargo right there") + } + p, ok := Path() + if !ok { + t.Fatal("Path did not find the binary on PATH") + } + if got := Version(p); got != "rtk 0.42.4" { + t.Errorf("Version = %q", got) + } + if err := Install(io.Discard, io.Discard); err != nil { + t.Errorf("Install with a working cargo: %v", err) + } + + // A build that fails is reported as the command that failed, so the user can + // run it again themselves and see the same output. + onlyPath(t, fakeBin(t, "cargo", "", 1)) + err = Install(io.Discard, io.Discard) + if err == nil { + t.Fatal("Install returned no error when cargo failed") + } + if !strings.Contains(err.Error(), InstallCmd()) { + t.Errorf("error = %q, which does not name the command that failed", err) + } +} + +// fakeBin writes an executable named name into its own directory and returns that +// directory, so a test can put it on PATH and drive the install and lookup paths +// without the real tool being installed on the machine running the suite. +func fakeBin(t *testing.T, name, stdout string, exit int) string { + t.Helper() + dir := t.TempDir() + if runtime.GOOS == "windows" { + body := "@echo off\r\n" + if stdout != "" { + body += "echo " + stdout + "\r\n" + } + body += fmt.Sprintf("exit /b %d\r\n", exit) + writeFile(t, filepath.Join(dir, name+".cmd"), body, 0o644) + return dir + } + body := "#!/bin/sh\n" + if stdout != "" { + body += "printf '%s\n' '" + strings.ReplaceAll(stdout, "'", `'\''`) + "'\n" + } + body += fmt.Sprintf("exit %d\n", exit) + writeFile(t, filepath.Join(dir, name), body, 0o755) + return dir +} + +func writeFile(t *testing.T, path, body string, mode os.FileMode) { + t.Helper() + if err := os.WriteFile(path, []byte(body), mode); err != nil { + t.Fatalf("WriteFile %s: %v", path, err) + } +} + +// onlyPath replaces PATH with dirs for the length of the test, so a lookup finds +// exactly what the test put there and nothing the machine happens to have. +func onlyPath(t *testing.T, dirs ...string) { + t.Helper() + t.Setenv("PATH", strings.Join(dirs, string(os.PathListSeparator))) +} diff --git a/internal/scaffold/update_test.go b/internal/scaffold/update_test.go index cb612a6..d996b3f 100644 --- a/internal/scaffold/update_test.go +++ b/internal/scaffold/update_test.go @@ -343,3 +343,62 @@ func findWorkspaceFile(t *testing.T, h paths.Harness, rel string) assets.File { t.Fatalf("%s ships no file at %s", h.ID, rel) return assets.File{} } + +// Count and Writes are what a caller prints and what it asks the user to agree to, +// and Writes has one subtlety worth pinning: 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 TestCountAndWritesDescribeThePlanTheUserIsShown(t *testing.T) { + root := t.TempDir() + applyTo(t, root, paths.Claude, false) + + // A plan with nothing to do writes nothing, whichever way force is set. + plan := planFor(t, root, paths.Claude) + if n := plan.Count(UpCurrent); n == 0 { + t.Errorf("a fresh workspace has %d current files", n) + } + for _, force := range []bool{false, true} { + if plan.Writes(force) { + t.Errorf("a fresh workspace reports writes with force = %v: %+v", force, plan.Pending()) + } + } + if len(plan.Pending()) != 0 { + t.Errorf("a fresh workspace has pending items: %+v", plan.Pending()) + } + + // A conflict alone is a plan with nothing to do *until the user decides*, so + // it writes only under force. + edited := ".claude/rules/tasks.md" + writeManaged(t, root, paths.Claude, edited, "# mine\n", false) + plan = planFor(t, root, paths.Claude) + if n := plan.Count(UpConflict); n != 1 { + t.Errorf("Count(conflict) = %d, want 1", n) + } + if plan.Writes(false) { + t.Error("a plan whose only item is a conflict reports writes without force") + } + if !plan.Writes(true) { + t.Error("a plan whose only item is a conflict reports no writes under force") + } + if len(plan.Pending()) != 1 { + t.Errorf("Pending = %+v, want the one conflict", plan.Pending()) + } + + // An orphan writes without force: the file is left alone, but the manifest + // entry goes, and a stale one would send the next update looking for a + // template that is not there. + orphan := ".claude/rules/retired-edited.md" + writeManaged(t, root, paths.Claude, orphan, "# retired\n", true) + writeManaged(t, root, paths.Claude, orphan, "# and then edited\n", false) + plan = planFor(t, root, paths.Claude) + if n := plan.Count(UpOrphan); n != 1 { + t.Errorf("Count(orphan) = %d, want 1", n) + } + if !plan.Writes(false) { + t.Error("a plan with an orphan reports no writes, so the manifest keeps a dead entry") + } + // An action nothing in this plan carries counts zero rather than guessing. + if n := plan.Count(UpdateAction("not-an-action")); n != 0 { + t.Errorf("Count on an unknown action = %d", n) + } +} diff --git a/internal/validate/checks_test.go b/internal/validate/checks_test.go new file mode 100644 index 0000000..1fff283 --- /dev/null +++ b/internal/validate/checks_test.go @@ -0,0 +1,351 @@ +package validate + +import ( + "os" + "strings" + "testing" + + "github.com/protonspy/spec-claude-code/internal/assets" + "github.com/protonspy/spec-claude-code/internal/gate" + sccmanifest "github.com/protonspy/spec-claude-code/internal/manifest" + "github.com/protonspy/spec-claude-code/internal/paths" +) + +func gateWorkspace(t *testing.T) string { + t.Helper() + root := t.TempDir() + if err := os.MkdirAll(paths.Claude.Config(root), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := sccmanifest.Save(root, paths.Claude, sccmanifest.New(assets.Version, paths.Claude)); err != nil { + t.Fatalf("manifest.Save: %v", err) + } + return root +} + +// The findings are deliberately distinct rather than one "the gate failed". A +// command that did not run, a test command whose report could not be read, a suite +// with no tests, and one that came up short are four different things to go and do, +// and a gate that called them all one thing would send the reader to the same dead +// end four times. +func TestCheckFindingsTellsTheFailuresApart(t *testing.T) { + root := gateWorkspace(t) + + for _, tc := range []struct { + name string + res gate.Result + rule string + says []string + }{ + { + name: "timed out", + res: gate.Result{Kind: gate.Test, Command: "go test ./...", TimedOut: true}, + rule: "check.timed-out", + says: []string{"go test ./...", "still running"}, + }, + { + name: "failed", + res: gate.Result{Kind: gate.Build, Command: "go build ./...", ExitCode: 2, Tail: "undefined: x"}, + rule: "check.failed", + // The tail is there because "the gate failed" with no excerpt sends the + // reader off to re-run it by hand. + says: []string{"go build ./...", "exited 2", "undefined: x"}, + }, + { + name: "no report", + res: gate.Result{Kind: gate.Test, Command: "go test ./...", ExitCode: 0}, + rule: "tests.no-report", + says: []string{"total", "coverage", "stdout"}, + }, + { + name: "no tests", + res: gate.Result{Kind: gate.Test, Command: "go test ./...", Reported: true, + Report: gate.Report{Total: 0, Coverage: 100}}, + rule: "tests.none", + says: []string{"0 tests"}, + }, + { + name: "below floor", + res: gate.Result{Kind: gate.Test, Command: "go test ./...", Reported: true, + Report: gate.Report{Total: 800, Coverage: 73.8}, Floor: 86}, + rule: "tests.below-floor", + says: []string{"73.8", "86"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + set := CheckFindings(root, tc.res) + if set.Empty() { + t.Fatalf("no finding for %s", tc.name) + } + var found bool + var msg string + for _, f := range set.Sorted() { + if f.Rule == tc.rule { + found, msg = true, f.Message + } + } + if !found { + t.Fatalf("no %s finding: %+v", tc.rule, set.Sorted()) + } + for _, want := range tc.says { + if !strings.Contains(msg, want) { + t.Errorf("%s message does not mention %q:\n%s", tc.rule, want, msg) + } + } + }) + } + + // A passing build says nothing, and a passing test run over the floor says + // nothing either — the other three gates are judged by their exit status alone. + for _, res := range []gate.Result{ + {Kind: gate.Build, Command: "go build ./...", ExitCode: 0}, + {Kind: gate.Lint, Command: "golangci-lint run", ExitCode: 0, Tail: "0 issues."}, + {Kind: gate.Test, Command: "go test ./...", Reported: true, + Report: gate.Report{Total: 800, Coverage: 91}, Floor: 86}, + } { + if set := CheckFindings(root, res); !set.Empty() { + t.Errorf("a passing %s gate reported %+v", res.Kind, set.Sorted()) + } + } +} + +// A gate nobody has decided on is a finding, and the message says what to write +// rather than only what is wrong — these are the only findings in the product +// whose fix is a command line rather than an edit to the file they point at. A +// gate somebody declined is silence: a language with no formatter is a real +// answer, and a project that recorded it has already had this conversation. +func TestChecksTellsAnUndecidedGateFromADeclinedOne(t *testing.T) { + root := gateWorkspace(t) + + set, err := Checks(root) + if err != nil { + t.Fatalf("Checks: %v", err) + } + if set.Empty() { + t.Fatal("a workspace that has decided nothing reported no findings") + } + first := set.Sorted()[0] + if first.Rule != "check.not-configured" { + t.Fatalf("first finding = %+v, want check.not-configured", first) + } + // Build first, because the four presuppose each other and a lint report over a + // broken build is derived noise. + if !strings.Contains(first.Message, string(gate.Build)) { + t.Errorf("the first finding is about %q, want the build gate", first.Message) + } + for _, want := range []string{"scc check set", "scc check skip"} { + if !strings.Contains(first.Message, want) { + t.Errorf("the message offers no way out via %q:\n%s", want, first.Message) + } + } + + // Decline all four, and the validator goes quiet about every one of them. + cfg := gate.Config{Commands: map[gate.Kind]string{}} + for _, k := range gate.Kinds() { + cfg.Commands[k] = gate.Skipped + } + if _, err := gate.Save(root, cfg); err != nil { + t.Fatalf("gate.Save: %v", err) + } + set, err = Checks(root) + if err != nil { + t.Fatalf("Checks: %v", err) + } + if !set.Empty() { + t.Errorf("a workspace that declined every gate reported %+v", set.Sorted()) + } +} + +// The set is what the aggregate command runs, and the two options are behind a +// flag for one reason: cost. Everything in base() reads files already on disk; +// these run a compiler and go to the network. +func TestTheValidatorSetIsAssembledByCost(t *testing.T) { + names := func(all []Validator) []string { + var out []string + for _, v := range all { + out = append(out, v.Name) + } + return out + } + + def := names(All()) + if len(def) == 0 { + t.Fatal("All() returned no validators") + } + for _, off := range []string{"checks", "pr"} { + if contains(def, off) { + t.Errorf("%q runs by default, and it is behind a flag for cost", off) + } + } + if !contains(def, "attribution") { + t.Error("attribution does not run by default") + } + + // The Stop hook may only carry what the next turn can resolve, and a signature + // in a commit already made comes out with a rebase rather than an edit — so it + // would re-appear at the end of every turn for the life of the branch. + if quiet := names(All(WithoutAttribution())); contains(quiet, "attribution") { + t.Error("WithoutAttribution left the history check in the run") + } else if len(quiet) != len(def)-1 { + t.Errorf("WithoutAttribution changed the set by more than the one check: %v", quiet) + } + + if withPR := names(All(WithPR())); !contains(withPR, "pr") { + t.Errorf("WithPR did not add the pull-request check: %v", withPR) + } + + full := names(All(WithChecks(), WithPR())) + if !contains(full, "checks") || !contains(full, "pr") { + t.Errorf("both options together produced %v", full) + } + // Checks before pr: they are the slow ones, and a run that is going to fail on + // a broken build should say so before it spends a round trip on the forge. + if indexOf(full, "checks") > indexOf(full, "pr") { + t.Errorf("pr runs before checks: %v", full) + } + // And the artifact checks come first, so a scan of the report finds them + // together with the history check after them. + if indexOf(full, "checks") < indexOf(full, "attribution") { + t.Errorf("the delivery gate runs before the artifact checks: %v", full) + } + + for _, v := range full { + if v == "" { + t.Error("a validator has no name, so no finding can be traced to it") + } + } +} + +func indexOf(all []string, want string) int { + for i, s := range all { + if s == want { + return i + } + } + return -1 +} + +// The plan's shape is a closed set of six sections, and PlanSections is that +// contract in the order the template writes it — the one thing that ever capped a +// plan's size, because there is deliberately nowhere for prose to go. +func TestPlanSectionsIsTheContractAndACopy(t *testing.T) { + all := PlanSections() + if len(all) != 6 { + t.Fatalf("PlanSections returned %d sections, want the six the contract has: %+v", len(all), all) + } + var required int + for _, s := range all { + if s.Title == "" || s.Slug == "" { + t.Errorf("a section is not fully named: %+v", s) + } + if s.Required { + required++ + } + } + if required == 0 { + t.Error("no section is required, so an empty file satisfies the contract") + } + + // A copy, not the table: a caller that sorted or truncated what it got back + // would change what every later plan is held to. + all[0].Title = "Mutated" + if PlanSections()[0].Title == "Mutated" { + t.Error("PlanSections handed out the contract itself") + } +} + +// The pair has to travel together: a `status: approved` with no checksum is a plan +// claiming to be sealed by a seal that is not there, and every read of it would +// silently skip the check it was approved to get. +func TestAnApprovedPlanWithNoChecksumIsAFinding(t *testing.T) { + root := t.TempDir() + const task = "- [ ] 1.1 (Unit) Do the thing\n" + + // A plan with no status is never checked, which is what makes every plan + // written before the seal shipped keep passing. + writePlan(t, root, "none", plan("", task)) + set, err := Plans(root) + if err != nil || !set.Empty() { + t.Errorf("a plan with no status: %v, %+v", err, set.Sorted()) + } + + writePlan(t, root, "none", plan("---\nstatus: approved\n---\n\n", task)) + set, err = Plans(root) + if err != nil { + t.Fatalf("Plans: %v", err) + } + if !hasRule(set, "plan.unsealed") { + t.Errorf("an approved plan with no checksum reported %+v", set.Sorted()) + } + + // A status outside the vocabulary is its own finding, and the checksum is not + // then also demanded — one finding that names the real problem beats two. + writePlan(t, root, "none", plan("---\nstatus: finished\n---\n\n", task)) + set, err = Plans(root) + if err != nil { + t.Fatalf("Plans: %v", err) + } + if !hasRule(set, "plan.status-invalid") { + t.Errorf("an unknown status reported %+v", set.Sorted()) + } + if hasRule(set, "plan.unsealed") { + t.Errorf("an unknown status also demanded a checksum: %+v", set.Sorted()) + } + + // Draft is the other half of the vocabulary and needs no seal. + writePlan(t, root, "none", plan("---\nstatus: draft\n---\n\n", task)) + set, err = Plans(root) + if err != nil { + t.Fatalf("Plans: %v", err) + } + if !set.Empty() { + t.Errorf("a draft plan reported %+v", set.Sorted()) + } +} + +// The pull request is the half no hook can reach: a commit message passes through +// commit-msg on the machine that wrote it, and a PR body is typed straight into +// the forge, which is why the footer that dies everywhere else survives there. +// +// Like its sibling it never returns an error. No gh, no authentication, no pull +// request for this branch: all of them mean there is nothing to check, and none of +// them is this validator failing. +func TestAttributionPRIsSilentWhenThereIsNothingToAsk(t *testing.T) { + // Not a repository at all. + set, err := AttributionPR(t.TempDir()) + if err != nil { + t.Fatalf("AttributionPR outside a repository: %v", err) + } + if !set.Empty() { + t.Errorf("a directory that is not a repository reported %+v", set.Sorted()) + } + + // A repository with no gh on PATH: the question cannot be asked, which is not + // the same as an answer of "clean" and is certainly not an error. + t.Setenv("PATH", t.TempDir()) + set, err = AttributionPR(t.TempDir()) + if err != nil { + t.Fatalf("AttributionPR with no gh: %v", err) + } + if !set.Empty() { + t.Errorf("a workspace with no gh reported %+v", set.Sorted()) + } +} + +// clipSubject names the commit in words next to the short sha a finding is filed +// under: the sha is what `git show` takes, the subject is what the reader +// recognizes, and neither on its own finds the commit again after a rebase. +func TestClipSubjectKeepsTheLineReadable(t *testing.T) { + short := "feat(cli): return exit 2 on findings" + if got := clipSubject(short); got != short { + t.Errorf("clipSubject shortened a subject that fits: %q", got) + } + long := strings.Repeat("x", 200) + got := clipSubject(long) + if len(got) >= len(long) { + t.Errorf("clipSubject did not shorten a %d-character subject", len(long)) + } + if !strings.HasSuffix(got, "…") { + t.Errorf("a clipped subject does not say it was clipped: %q", got) + } +} diff --git a/internal/validate/knowledge_test.go b/internal/validate/knowledge_test.go index 5ce0d51..1983f7a 100644 --- a/internal/validate/knowledge_test.go +++ b/internal/validate/knowledge_test.go @@ -565,3 +565,123 @@ func TestCodewikiValidatesAWorkspaceBehindASymlink(t *testing.T) { t.Errorf("rules = %v, want none: a citation inside a symlinked workspace is not an escape", got) } } + +// A duplicated heading gives two sections one anchor, and a link to it lands on +// whichever came first — which is the whole reason slugs are checked rather than +// assumed. +func TestCodewikiHeadingSlugsMustBeUniqueAndReal(t *testing.T) { + root := t.TempDir() + write(t, filepath.Join(root, "main.go"), "one\ntwo\nthree\n") + write(t, filepath.Join(paths.Codewiki(root), "app.md"), `# App + +## How it starts + +[main.go:1-2]() + +## How it starts + +[main.go:2-3]() + +## !!! + +[main.go:1-3]() + +## + +[main.go:1-3]() +`) + got := runValidator(t, Codewiki, root) + for _, want := range []string{ + "codewiki.duplicate-heading", + "codewiki.unslugged-heading", + "codewiki.empty-heading", + } { + if !contains(got, want) { + set, _ := Codewiki(root) + t.Errorf("rules = %v, want %s; findings: %+v", got, want, set.Sorted()) + } + } +} + +// countLines counts the way an editor numbers: a trailing newline does not add a +// line, so a citation to the last line of a well-formed file resolves rather than +// being reported as one past the end. +func TestACitationToTheLastLineResolves(t *testing.T) { + root := t.TempDir() + // Three lines and a trailing newline — the shape every well-formed text file + // has, and the one an off-by-one gets wrong. + write(t, filepath.Join(root, "main.go"), "one\ntwo\nthree\n") + write(t, filepath.Join(paths.Codewiki(root), "app.md"), `# App + +## The whole file + +[main.go:1-3]() +`) + if got := runValidator(t, Codewiki, root); len(got) != 0 { + set, _ := Codewiki(root) + t.Errorf("a citation to the last line reported %v: %+v", got, set.Sorted()) + } + + // A file with no trailing newline has the same number of lines, and a file + // with no content at all has none to cite. + write(t, filepath.Join(root, "short.go"), "only\n") + write(t, filepath.Join(root, "empty.go"), "") + write(t, filepath.Join(paths.Codewiki(root), "app.md"), `# App + +## One line + +[short.go:1-1]() + +## Nothing at all + +[empty.go:1-1]() +`) + got := runValidator(t, Codewiki, root) + if !contains(got, "codewiki.citation-out-of-range") { + set, _ := Codewiki(root) + t.Errorf("citing a line of an empty file reported %v: %+v", got, set.Sorted()) + } +} + +// An ADR whose frontmatter cannot be read is reported as that rather than as a +// missing status: the two send the reader to different places, and "add a status" +// to a file whose YAML is broken is advice that cannot be followed. +func TestAnUnreadableADRFrontmatterIsItsOwnFinding(t *testing.T) { + root := t.TempDir() + write(t, filepath.Join(paths.ADR(root), "0001-use-sqlite.md"), + "---\nstatus: accepted\nnested:\n too:\n deep: yes\n---\n\n# Use SQLite\n\n## Context\n\nBecause.\n") + + got := runValidator(t, ADR, root) + if !contains(got, "adr.frontmatter-unreadable") { + set, _ := ADR(root) + t.Errorf("rules = %v, want adr.frontmatter-unreadable; findings: %+v", got, set.Sorted()) + } + // And not also the findings that depend on having read it: one finding that + // names the real problem beats three that agree with each other. + for _, unwanted := range []string{"adr.missing-status", "adr.status-invalid"} { + if contains(got, unwanted) { + t.Errorf("an unreadable file also reported %s: %v", unwanted, got) + } + } +} + +// The status vocabulary is closed, and a status outside it is its own finding +// rather than silence — a record whose state nobody can read is a record that +// binds nothing. +func TestADRStatusVocabulary(t *testing.T) { + root := t.TempDir() + + write(t, filepath.Join(paths.ADR(root), "0001-a.md"), + "---\nstatus: probably-fine\n---\n\n# A\n\n## Context\n\nBecause.\n") + if got := runValidator(t, ADR, root); !contains(got, "adr.status-invalid") { + set, _ := ADR(root) + t.Errorf("rules = %v, want adr.status-invalid; findings: %+v", got, set.Sorted()) + } + + write(t, filepath.Join(paths.ADR(root), "0001-a.md"), + "---\ntitle: A\n---\n\n# A\n\n## Context\n\nBecause.\n") + if got := runValidator(t, ADR, root); !contains(got, "adr.missing-status") { + set, _ := ADR(root) + t.Errorf("rules = %v, want adr.missing-status; findings: %+v", got, set.Sorted()) + } +} diff --git a/internal/validate/skill_test.go b/internal/validate/skill_test.go index 8256712..caf263e 100644 --- a/internal/validate/skill_test.go +++ b/internal/validate/skill_test.go @@ -326,3 +326,31 @@ func TestCompatibilityLimitCountsCharacters(t *testing.T) { t.Errorf("a compatibility field of exactly %d characters was reported as too long", skillCompatibilityMax) } } + +// A link inside the skill is the only kind this validator can check: everything +// else — an anchor, an absolute path, a URL, a mailto, a path that escapes the +// skill — is somebody else's to resolve, and reporting it would be scc claiming to +// know about the world outside the directory it is looking at. +func TestOnlyALinkInsideTheSkillIsChecked(t *testing.T) { + for _, target := range []string{ + "reference.md", + "scripts/run.sh", + "./reference.md", + } { + if !isLocalReference(target) { + t.Errorf("isLocalReference(%q) = false for a path inside the skill", target) + } + } + for _, target := range []string{ + "", + "#a-heading", + "/etc/passwd", + "../outside.md", + "https://example.invalid/page", + "mailto:someone@example.invalid", + } { + if isLocalReference(target) { + t.Errorf("isLocalReference(%q) = true, and that is not scc's to resolve", target) + } + } +} diff --git a/internal/workspace/cover_test.go b/internal/workspace/cover_test.go new file mode 100644 index 0000000..81ab97f --- /dev/null +++ b/internal/workspace/cover_test.go @@ -0,0 +1,151 @@ +package workspace + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// The temp file is created in the destination directory so the rename stays on one +// filesystem, and a concurrent reader never observes a truncated file. What is +// checkable here is the observable half: the content arrives whole, the mode is the +// one asked for, a missing directory is created, and nothing is left behind. +func TestAtomicWriteLeavesNoTempFileAndKeepsTheMode(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "nested", "deeper", "file.txt") + + if err := AtomicWrite(path, []byte("first\n"), 0o644); err != nil { + t.Fatalf("AtomicWrite: %v", err) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(got) != "first\n" { + t.Errorf("content = %q", got) + } + + // Overwriting is the case the rename exists for: the old content is replaced + // whole rather than truncated and rewritten. + if err := AtomicWrite(path, []byte("second and longer\n"), 0o644); err != nil { + t.Fatalf("AtomicWrite (overwrite): %v", err) + } + got, err = os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(got) != "second and longer\n" { + t.Errorf("content after overwrite = %q", got) + } + + entries, err := os.ReadDir(filepath.Dir(path)) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + for _, e := range entries { + if strings.Contains(e.Name(), ".tmp-") { + t.Errorf("a temp file survived the write: %s", e.Name()) + } + } + + // A path whose parent is a file cannot be created, and that is an error rather + // than a silent miss. + blocked := filepath.Join(dir, "nested", "deeper", "file.txt", "child.txt") + if err := AtomicWrite(blocked, []byte("x"), 0o644); err == nil { + t.Error("AtomicWrite under a regular file returned no error") + } +} + +// Resolve normalizes --root: an empty argument means "the enclosing workspace", +// and a path that is not there is an error rather than a directory scc creates. +func TestResolveRefusesAPathThatIsNotThere(t *testing.T) { + dir := t.TempDir() + + got, err := Resolve(dir) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if !sameDir(t, got, dir) { + t.Errorf("Resolve(%q) = %q", dir, got) + } + + missing := filepath.Join(dir, "not-there") + if _, err := Resolve(missing); err == nil { + t.Error("Resolve on a path that does not exist returned no error") + } else if !strings.Contains(err.Error(), "does not exist") { + t.Errorf("error = %q, want it to say the path is not there", err) + } + + // Empty falls back to the working directory's enclosing workspace, which is a + // real answer wherever the suite is run from. + if got, err := Resolve(""); err != nil || got == "" { + t.Errorf("Resolve(\"\") = %q, %v", got, err) + } +} + +// Relative is for logging, so a target that cannot be made relative comes back as +// itself rather than as an error nobody can act on. +func TestRelativeFallsBackToTheTargetItself(t *testing.T) { + root := t.TempDir() + inside := filepath.Join(root, "plans", "sample.md") + + if got := Relative(root, inside); got != filepath.Join("plans", "sample.md") { + t.Errorf("Relative = %q", got) + } + // A target on another volume cannot be made relative on Windows, and on every + // platform an empty root leaves the path as it was. + if got := Relative("", inside); got == "" { + t.Error("Relative with no root returned nothing") + } +} + +// SafeWrite is "write only when missing" and WriteFile is "write unless told not +// to" — the pair that keeps a seed from overwriting what the user has since +// edited. +func TestSafeWriteAndWriteFileDisagreeOnPurpose(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "glossary.md") + + created, err := SafeWrite(path, "seeded\n") + if err != nil || !created { + t.Fatalf("SafeWrite: %v, created = %v", err, created) + } + created, err = SafeWrite(path, "seeded again\n") + if err != nil { + t.Fatalf("SafeWrite: %v", err) + } + if created { + t.Error("SafeWrite overwrote a file that was already there") + } + if body, _ := os.ReadFile(path); string(body) != "seeded\n" { + t.Errorf("content = %q", body) + } + + if err := WriteFile(path, "mine\n", false); err == nil { + t.Error("WriteFile without overwrite replaced an existing file") + } else if !strings.Contains(err.Error(), "--force") { + t.Errorf("the refusal does not name the way past it: %v", err) + } + if err := WriteFile(path, "mine\n", true); err != nil { + t.Fatalf("WriteFile with overwrite: %v", err) + } + if body, _ := os.ReadFile(path); string(body) != "mine\n" { + t.Errorf("content = %q", body) + } +} + +// sameDir compares resolved paths rather than strings: t.TempDir can sit under a +// symlink (/var → /private/var on macOS) and Windows reports 8.3 short names. +func sameDir(t *testing.T, a, b string) bool { + t.Helper() + fa, err := os.Stat(a) + if err != nil { + return false + } + fb, err := os.Stat(b) + if err != nil { + return false + } + return os.SameFile(fa, fb) +} diff --git a/plans/launch-defects.md b/plans/launch-defects.md new file mode 100644 index 0000000..7800a6d --- /dev/null +++ b/plans/launch-defects.md @@ -0,0 +1,64 @@ +--- +autonomy: auto +ci: wait +--- + +# Launch defects + +A run of `scc launch claude` on Windows fell back to the host unsandboxed and warned +about a malformed block in this repo's own entry file. Two causes: the dev container's +`postCreateCommand` writes to a read-only mount, and `internal/mdblock` reads prose +about its markers as a block. + +## Why + +Both defects ship to every scaffolded workspace and both fail in the direction that +looks like success. The container one ends with an agent running outside the boundary +`scc launch` said it would provide. The mdblock one is worse: a document that merely +*documents* the markers is read as carrying the block, so `scc launch` skips the RTK +wiring it believes is already done, and `scc rtk` would splice over the prose between +a marker pair that only ever appeared in a sentence. Done is a launch that sandboxes +and an entry file that can describe the markers without being edited by them. + +## Paths + +- `internal/mdblock/mdblock.go` +- `internal/mdscan/mdscan.go` +- `.devcontainer/` and `internal/assets/templates/devcontainer/` +- `internal/rtk/rtk.go` + +## Out of scope + +- Editing `CLAUDE.md` to work around the marker bug — the file is the user's, and a + document that cannot name the markers scc splices is the defect, not the workaround. + +## Tasks + +- [x] 1.1 (TDD) Add `mdscan.Mask`: fenced blocks and inline code spans blanked to + spaces, every byte offset preserved, HTML comments left intact because the markers + a caller looks for here are HTML comments. +- [x] 1.2 (TDD) Resolve the block through one span helper in `internal/mdblock`, + measured over `MaskProse` and sliced out of the original, so `Splice`, `Block`, + `Version` and `Remove` cannot disagree about where the block is. + _Depends 1.1_ +- [x] 1.3 (Unit) Move the dev container's `safe.directory` write out of + `postCreateCommand` and into the image as a `--system` setting, in the seeded + template and in this repo's copy. +- [x] 1.4 (Unit) Record what each defect cost, as notes against the files that carried + them. + _Depends 1.1, 1.2, 1.3, 1.5_ +- [x] 1.5 (Unit) Mask the document in `rtk.ForeignBlock` too: a sentence naming + Headroom's marker is not Headroom's block, and reporting it sends the reader to + `headroom unwrap` for a block nobody wrote. +- [x] 2.1 (Unit) Raise coverage to the 86% floor the delivery gate requires, package by + package from the least-covered, so `pre-push` passes on its own terms rather than + on `SCC_SKIP_HOOKS`. + +## Done when + +- `scc launch --dry-run` reports no malformed-block warning on this repo's `CLAUDE.md`, + and reports the RTK block as absent rather than present. +- `devcontainer up` completes its `postCreateCommand` on Windows, so `scc launch` + reports the container rather than falling back to the host. +- `scc validate --checks` is clean, which it was not on `main`: the suite went from + 73.6% to 86.0%, the floor this workspace records. \ No newline at end of file