From 2ab1a4d67aaa0c1046beffa8ae3dda44b12e1159 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Tue, 21 Jul 2026 18:09:31 +0100 Subject: [PATCH 1/2] chore: capture the impact write-path gap as iss-111 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 added two blockers requiring an explicit `impact` before a record can move to shipped/ or resolved/, but neither write path can set the field: `abcd capture resolve` writes only the resolution, and `seedDraft` on the intent side emits no impact key. The gates are armed against a value the tool cannot produce, so the field is hand-edited until this is closed. Captured rather than fixed here — the flag surface is a design call, not an integration fix, and phase 2 is the surface guardrail. Assisted-by: Claude:claude-opus-4-8 --- ...resolve-and-intent-seeddraft-cannot-set-impact.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 .abcd/work/issues/open/iss-111-abcd-capture-resolve-and-intent-seeddraft-cannot-set-impact.md diff --git a/.abcd/work/issues/open/iss-111-abcd-capture-resolve-and-intent-seeddraft-cannot-set-impact.md b/.abcd/work/issues/open/iss-111-abcd-capture-resolve-and-intent-seeddraft-cannot-set-impact.md new file mode 100644 index 0000000..90ee0e2 --- /dev/null +++ b/.abcd/work/issues/open/iss-111-abcd-capture-resolve-and-intent-seeddraft-cannot-set-impact.md @@ -0,0 +1,12 @@ +--- +schema_version: 1 +id: "iss-111" +slug: "abcd-capture-resolve-and-intent-seeddraft-cannot-set-impact" +severity: "major" +category: "inconsistency" +source: "agent-finding" +found_during: "itd-73 phase 1 derived versioning" +found_at: "internal/core/capture/workflow.go" +--- + +abcd capture resolve and intent seedDraft cannot set impact, so the tool's own path produces a record the new issue_impact_valid and intent_impact_valid blockers reject \ No newline at end of file From 5a379b287721643a660640153bdde66430d515c5 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Tue, 21 Jul 2026 19:21:15 +0100 Subject: [PATCH 2/2] feat: gate the release on a structural surface diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 of the derived-versioning programme (itd-73 / spc-10). A mislabelled release can no longer ship a compatibility lie: a removed or renamed command or flag, a newly-required flag, or a dropped manifest entry fails the cut unless the cut declares a `breaking` intent, and the failure names the surface that changed. THE SNAPSHOT A new structured JSON emitter over the shared NewRootCommand() tree, not over GenerateReference's Markdown walker. That walker returns early on cmd.Hidden, so the operator-internal `hook` subtree is invisible to it, and it carries neither per-flag required-ness nor the manifest surface — reusing it would have baked those blind spots into a compatibility gate. What is reused is the root command itself. The snapshot captures all 51 commands including the 5 hidden `hook` entries, each flag's type and required-ness, and 16 declared manifest keys. Types and diff live in internal/core/surface (cobra-free); the walk lives in internal/surface/cli. The generator and the drift test share one render path, so they cannot disagree — the same shape cmd/abcd-gen-cli-ref and reference_test.go already use for the CLI reference page. WHAT THE BASELINE IS The committed surface.json is held equal to the working tree by its own drift test, so comparing the committed file against a fresh walk would compare a thing to itself and never fire. The baseline is the snapshot AS OF THE LAST RELEASE TAG, read via git. A tag carrying no snapshot refuses rather than passing: v0.3.0 predates this file, so cuts refuse until a release contains the baseline, which is the deliberate fail-closed direction for the first and highest-risk cut. DETERMINISM Proven at three levels: 50 in-process renders byte-identical, 10 fresh generator processes producing one sha256, and a second `go generate` leaving no diff. Cobra already sorts subcommands, so the live walk is stable even without our sort — the sort in canonical() is the guarantee that does not depend on that default, and the manifest's array-position ordering defect was fixed inside the extractor rather than patched downstream. Each detector was mutation-checked: hiding the hook subtree, hardwiring required-ness false, removing the sort, and deleting a manifest key each make a named test fail. Assisted-by: Claude:claude-opus-4-8 --- .abcd/development/release/README.md | 63 +++ .abcd/development/release/surface.json | 586 +++++++++++++++++++++++++ cmd/abcd-gen-cli-ref/main.go | 27 +- cmd/abcd-gen-surface/main.go | 48 ++ internal/README.md | 7 + internal/core/changelog/guard.go | 261 +++++++++++ internal/core/changelog/guard_test.go | 454 +++++++++++++++++++ internal/core/changelog/shipped.go | 34 +- internal/core/surface/diff.go | 175 ++++++++ internal/core/surface/diff_test.go | 279 ++++++++++++ internal/core/surface/manifest.go | 167 +++++++ internal/core/surface/manifest_test.go | 189 ++++++++ internal/core/surface/snapshot.go | 213 +++++++++ internal/core/surface/snapshot_test.go | 259 +++++++++++ internal/fsutil/moduleroot_test.go | 56 +++ internal/fsutil/paths.go | 30 ++ internal/surface/cli/guard_test.go | 265 +++++++++++ internal/surface/cli/surface.go | 126 ++++++ internal/surface/cli/surface_test.go | 235 ++++++++++ 19 files changed, 3454 insertions(+), 20 deletions(-) create mode 100644 .abcd/development/release/README.md create mode 100644 .abcd/development/release/surface.json create mode 100644 cmd/abcd-gen-surface/main.go create mode 100644 internal/core/changelog/guard.go create mode 100644 internal/core/changelog/guard_test.go create mode 100644 internal/core/surface/diff.go create mode 100644 internal/core/surface/diff_test.go create mode 100644 internal/core/surface/manifest.go create mode 100644 internal/core/surface/manifest_test.go create mode 100644 internal/core/surface/snapshot.go create mode 100644 internal/core/surface/snapshot_test.go create mode 100644 internal/fsutil/moduleroot_test.go create mode 100644 internal/surface/cli/guard_test.go create mode 100644 internal/surface/cli/surface.go create mode 100644 internal/surface/cli/surface_test.go diff --git a/.abcd/development/release/README.md b/.abcd/development/release/README.md new file mode 100644 index 0000000..cce996e --- /dev/null +++ b/.abcd/development/release/README.md @@ -0,0 +1,63 @@ +# Release artefacts — the committed compatibility baseline + +This directory holds the machine-generated artefacts a release is gated against. +Everything here is derived from the code and the manifests: regenerate it, never +hand-edit it. + +## `surface.json` — the compatibility snapshot + +`surface.json` is the structured record of abcd's public compatibility surface at +this commit: + +- **every command**, by its full command path (`abcd intent plan`), with the flags + it declares — name, shorthand, value type, requiredness, and whether the flag is + hidden; +- **the manifest surface** — the declared key paths of `.claude-plugin/plugin.json` + and `.claude-plugin/marketplace.json`. + +Hidden commands are included. The operator-internal `hook` subtree never appears +on the documentation page, but harness wiring invokes it by name, so removing or +renaming it breaks installations exactly as a visible command would. + +A manifest **entry** is one declared key path, and only its presence is recorded. +Objects flatten with dots (`author.name`); a list of objects carrying unique names +is keyed by name (`plugins[abcd].source`), so reordering it changes nothing; any +other list is one entry, so editing its members changes nothing. Values are absent +by design — a removed entry is a break, while a reworded description is not. + +Nothing in the file is treated as expected or required: `plugin.json` declares no +version in the development tree, and the version the rendered release payload +carries reads as one ordinary added entry. + +## What it is for + +The release cut diffs the copy of this file carried by the **last release tag** +against the surface of the tree being released. A removed or renamed command, a +removed or renamed flag, a flag that becomes required, or a removed manifest entry +is a structural break, and a cut carrying one without a `breaking` record added in +that cut fails. Additions, help-text edits, and reordering are not breaks. + +The copy committed here is not the comparison input — a drift test keeps it equal +to the live tree, so diffing it against that tree would compare a file with itself +and could never report a break. It is committed for two other reasons: so that the +NEXT tag carries a correct baseline for the cut after it, and so the guardrail can +check that the binary walking the surface really is this tree. The drift test +regenerates the surface from the live command tree and the live manifests on every +`go test` run and fails the build whenever the committed file disagrees, which is +what keeps both uses honest. + +## Regenerating + +```bash +go generate ./internal/surface/cli # or: go run ./cmd/abcd-gen-surface +``` + +The generator holds no formatting logic of its own — it calls the same +`GenerateSurface` the drift test calls — so the file that is written and the file +that is checked are produced by one code path. Output is deterministic: every +collection is sorted by a stable key, the JSON key order is fixed, and nothing +reads the clock or the environment, so the same tree yields the same bytes on +every machine. + +Regenerate and commit the result in the same change that alters a command, a flag, +or a manifest. diff --git a/.abcd/development/release/surface.json b/.abcd/development/release/surface.json new file mode 100644 index 0000000..93cb507 --- /dev/null +++ b/.abcd/development/release/surface.json @@ -0,0 +1,586 @@ +{ + "schema_version": 1, + "commands": [ + { + "path": "abcd", + "hidden": false, + "flags": [ + { + "name": "json", + "shorthand": "", + "type": "bool", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd ahoy", + "hidden": false, + "flags": [] + }, + { + "path": "abcd ahoy doctor", + "hidden": false, + "flags": [] + }, + { + "path": "abcd ahoy dry-run", + "hidden": false, + "flags": [] + }, + { + "path": "abcd ahoy identity-check", + "hidden": false, + "flags": [] + }, + { + "path": "abcd ahoy install", + "hidden": false, + "flags": [ + { + "name": "adopt", + "shorthand": "", + "type": "bool", + "required": false, + "hidden": false + }, + { + "name": "docs-target", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "oracle-backend", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "refuse-adopt", + "shorthand": "", + "type": "bool", + "required": false, + "hidden": false + }, + { + "name": "scan-deep", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "visibility", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "yes", + "shorthand": "", + "type": "bool", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd ahoy uninstall", + "hidden": false, + "flags": [] + }, + { + "path": "abcd audit", + "hidden": false, + "flags": [ + { + "name": "root", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd capture", + "hidden": false, + "flags": [ + { + "name": "blocked-by", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "category", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "found-at", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "found-during", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "severity", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "slug", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "source", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd capture list", + "hidden": false, + "flags": [ + { + "name": "all", + "shorthand": "", + "type": "bool", + "required": false, + "hidden": false + }, + { + "name": "open", + "shorthand": "", + "type": "bool", + "required": false, + "hidden": false + }, + { + "name": "resolved", + "shorthand": "", + "type": "bool", + "required": false, + "hidden": false + }, + { + "name": "wontfix", + "shorthand": "", + "type": "bool", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd capture resolve", + "hidden": false, + "flags": [] + }, + { + "path": "abcd capture wontfix", + "hidden": false, + "flags": [] + }, + { + "path": "abcd disembark", + "hidden": false, + "flags": [] + }, + { + "path": "abcd disembark coverage", + "hidden": false, + "flags": [] + }, + { + "path": "abcd disembark graveyard", + "hidden": false, + "flags": [ + { + "name": "lessons-json", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd disembark oracle", + "hidden": false, + "flags": [ + { + "name": "oracle-json", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd disembark pack", + "hidden": false, + "flags": [] + }, + { + "path": "abcd disembark plan", + "hidden": false, + "flags": [] + }, + { + "path": "abcd disembark press-release", + "hidden": false, + "flags": [ + { + "name": "press-release-json", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd disembark principles", + "hidden": false, + "flags": [ + { + "name": "principles-json", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd disembark probe", + "hidden": false, + "flags": [] + }, + { + "path": "abcd docs", + "hidden": false, + "flags": [] + }, + { + "path": "abcd docs lint", + "hidden": false, + "flags": [ + { + "name": "config", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "root", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd embark", + "hidden": false, + "flags": [] + }, + { + "path": "abcd embark from", + "hidden": false, + "flags": [] + }, + { + "path": "abcd embark probe", + "hidden": false, + "flags": [] + }, + { + "path": "abcd history", + "hidden": false, + "flags": [] + }, + { + "path": "abcd history capture", + "hidden": false, + "flags": [ + { + "name": "kind", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "session", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd history list", + "hidden": false, + "flags": [] + }, + { + "path": "abcd history show", + "hidden": false, + "flags": [] + }, + { + "path": "abcd hook", + "hidden": true, + "flags": [] + }, + { + "path": "abcd hook prompt-router", + "hidden": false, + "flags": [] + }, + { + "path": "abcd hook prompt-router-reset", + "hidden": false, + "flags": [] + }, + { + "path": "abcd hook session-end", + "hidden": false, + "flags": [] + }, + { + "path": "abcd hook session-start", + "hidden": false, + "flags": [] + }, + { + "path": "abcd intent", + "hidden": false, + "flags": [] + }, + { + "path": "abcd intent link", + "hidden": false, + "flags": [] + }, + { + "path": "abcd intent new", + "hidden": false, + "flags": [] + }, + { + "path": "abcd intent plan", + "hidden": false, + "flags": [] + }, + { + "path": "abcd intent ready", + "hidden": false, + "flags": [] + }, + { + "path": "abcd intent review", + "hidden": false, + "flags": [] + }, + { + "path": "abcd intent review ingest", + "hidden": false, + "flags": [ + { + "name": "verdict-json", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd launch", + "hidden": false, + "flags": [ + { + "name": "dry-run", + "shorthand": "", + "type": "bool", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd memory", + "hidden": false, + "flags": [] + }, + { + "path": "abcd memory ask", + "hidden": false, + "flags": [ + { + "name": "file-back", + "shorthand": "", + "type": "bool", + "required": false, + "hidden": false + }, + { + "name": "page-json", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "top-n", + "shorthand": "", + "type": "int", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd memory ingest", + "hidden": false, + "flags": [ + { + "name": "keep-original", + "shorthand": "", + "type": "bool", + "required": false, + "hidden": false + }, + { + "name": "pages-json", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd memory lint", + "hidden": false, + "flags": [] + }, + { + "path": "abcd rules", + "hidden": false, + "flags": [] + }, + { + "path": "abcd spec", + "hidden": false, + "flags": [] + }, + { + "path": "abcd spec close", + "hidden": false, + "flags": [] + }, + { + "path": "abcd version", + "hidden": false, + "flags": [] + } + ], + "manifest": [ + { + "file": ".claude-plugin/marketplace.json", + "key": "$schema" + }, + { + "file": ".claude-plugin/marketplace.json", + "key": "name" + }, + { + "file": ".claude-plugin/marketplace.json", + "key": "owner.name" + }, + { + "file": ".claude-plugin/marketplace.json", + "key": "owner.url" + }, + { + "file": ".claude-plugin/marketplace.json", + "key": "plugins[abcd].description" + }, + { + "file": ".claude-plugin/marketplace.json", + "key": "plugins[abcd].name" + }, + { + "file": ".claude-plugin/marketplace.json", + "key": "plugins[abcd].source" + }, + { + "file": ".claude-plugin/plugin.json", + "key": "$schema" + }, + { + "file": ".claude-plugin/plugin.json", + "key": "author.name" + }, + { + "file": ".claude-plugin/plugin.json", + "key": "author.url" + }, + { + "file": ".claude-plugin/plugin.json", + "key": "description" + }, + { + "file": ".claude-plugin/plugin.json", + "key": "homepage" + }, + { + "file": ".claude-plugin/plugin.json", + "key": "keywords" + }, + { + "file": ".claude-plugin/plugin.json", + "key": "license" + }, + { + "file": ".claude-plugin/plugin.json", + "key": "name" + }, + { + "file": ".claude-plugin/plugin.json", + "key": "repository" + } + ] +} diff --git a/cmd/abcd-gen-cli-ref/main.go b/cmd/abcd-gen-cli-ref/main.go index 2867501..9f0caaf 100644 --- a/cmd/abcd-gen-cli-ref/main.go +++ b/cmd/abcd-gen-cli-ref/main.go @@ -12,11 +12,12 @@ import ( "os" "path/filepath" + "github.com/REPPL/abcd-cli/internal/fsutil" "github.com/REPPL/abcd-cli/internal/surface/cli" ) func main() { - root, err := repoRoot() + root, err := moduleRoot() if err != nil { fmt.Fprintln(os.Stderr, "abcd-gen-cli-ref:", err) os.Exit(1) @@ -33,22 +34,16 @@ func main() { fmt.Println("wrote", cli.ReferencePagePath) } -// repoRoot walks up from the working directory to the module root (the directory -// holding go.mod), so the generator writes to the same page whether it is invoked -// via `go generate` (cwd is the package dir) or directly from the repo root. -func repoRoot() (string, error) { - dir, err := os.Getwd() +// moduleRoot resolves the module root from the working directory, so the +// generator writes to the same page whether it is invoked via `go generate` (cwd +// is the package dir) or directly from the repo root. The walk itself lives in +// fsutil, shared with abcd-gen-surface: two generators anchoring on go.mod by +// two separate walks is one walk too many, and a divergence between them would +// send the two drift-checked artefacts to different directories. +func moduleRoot() (string, error) { + cwd, err := os.Getwd() if err != nil { return "", err } - for { - if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { - return dir, nil - } - parent := filepath.Dir(dir) - if parent == dir { - return "", fmt.Errorf("go.mod not found above %s", dir) - } - dir = parent - } + return fsutil.ModuleRoot(cwd) } diff --git a/cmd/abcd-gen-surface/main.go b/cmd/abcd-gen-surface/main.go new file mode 100644 index 0000000..cabf42a --- /dev/null +++ b/cmd/abcd-gen-surface/main.go @@ -0,0 +1,48 @@ +// Command abcd-gen-surface writes the committed compatibility snapshot from the +// abcd command tree and the plugin manifests. It is the write half of the +// drift-checked surface: `go generate ./internal/surface/cli` runs it to refresh +// .abcd/development/release/surface.json, and a test +// (internal/surface/cli/surface_test.go) fails the build if the committed +// snapshot ever diverges from the tree. It holds no rendering logic of its own — +// the deterministic walk and encoding live in cli.GenerateSurface, so the +// generator and the drift test render byte-for-byte identically. +package main + +import ( + "fmt" + "os" + "path/filepath" + + "github.com/REPPL/abcd-cli/internal/fsutil" + "github.com/REPPL/abcd-cli/internal/surface/cli" +) + +func main() { + if err := run(); err != nil { + fmt.Fprintln(os.Stderr, "abcd-gen-surface:", err) + os.Exit(1) + } + fmt.Println("wrote", cli.SurfaceSnapshotPath) +} + +// run does the whole job so every failure path returns an error to one reporter, +// rather than each step repeating the exit dance. +func run() error { + cwd, err := os.Getwd() + if err != nil { + return err + } + root, err := fsutil.ModuleRoot(cwd) + if err != nil { + return err + } + data, err := cli.GenerateSurface(root) + if err != nil { + return err + } + dest := filepath.Join(root, filepath.FromSlash(cli.SurfaceSnapshotPath)) + if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { + return err + } + return os.WriteFile(dest, data, 0o644) +} diff --git a/internal/README.md b/internal/README.md index 754705b..1601fd9 100644 --- a/internal/README.md +++ b/internal/README.md @@ -17,6 +17,13 @@ plugin surface, and a future MCP server share one engine. entered the terminal folders since the anchor tag. It owns the enum so the lints that GATE the judgement (`core/lint`), the ledger reader that VALIDATES it (`core/capture`), and the derivation that CONSUMES it cannot drift apart. +- **`core/surface/`** — the compatibility surface as DATA: the snapshot of every + command, flag, and manifest entry a consumer binds to, and the diff that names + what a release narrowed. It shares a word with the `surface/` front-door tier + and nothing else — that tier is about transports, this package is about what + those transports expose — so it is cobra-free like the rest of `core/`. The + walk that reads the live command tree needs cobra and therefore lives in + `surface/cli`, which hands its result in; the dependency never points back. - **`core/lifeboat/`** — the brief↔lifeboat contract. `mapping.go` is the single source of truth for which brief section a lifeboat fills from which source tier, and it is rendered into the brief's `00-meta.md` with a test asserting diff --git a/internal/core/changelog/guard.go b/internal/core/changelog/guard.go new file mode 100644 index 0000000..2dc5738 --- /dev/null +++ b/internal/core/changelog/guard.go @@ -0,0 +1,261 @@ +package changelog + +import ( + "bytes" + "fmt" + "strings" + + "github.com/REPPL/abcd-cli/internal/core/surface" + "github.com/REPPL/abcd-cli/internal/gitutil" +) + +// SurfaceGuardStatus is the guardrail's verdict on a cut. Three states, kept +// apart because a caller that conflated them would ship the wrong release: +// +// SurfaceGuardPassed — the cut may proceed. +// SurfaceGuardFailed — the surface narrowed and nothing in the cut declares +// it; Reason names every changed surface. +// SurfaceGuardRefused — the guardrail could not compare at all; Reason says +// what to do about it. +// +// A refusal is NOT a pass. Failing to distinguish them is the whole risk of the +// first cut, which is the one with no baseline to compare against. +type SurfaceGuardStatus string + +// The three verdicts. The string values are what a rendered preview prints and +// what a machine-readable front door emits, so renaming a constant is safe and +// changing a value is a contract change. +const ( + SurfaceGuardPassed SurfaceGuardStatus = "passed" + SurfaceGuardFailed SurfaceGuardStatus = "failed" + SurfaceGuardRefused SurfaceGuardStatus = "refused" +) + +// SurfaceGuard is the outcome of the surface-break guardrail at a release cut. +// +// Like Derivation, a refusal is modelled as a VALUE rather than an error: "this +// cut cannot be guarded" is a legitimate result a read-only preview must render, +// and errors are reserved for "the repository could not be read at all". +type SurfaceGuard struct { + // BaseTag is the release tag the baseline snapshot was read from, empty + // when no tag resolved. + BaseTag string + // Status is the verdict. + Status SurfaceGuardStatus + // Breaks is every structural narrowing found, in canonical order. It is + // populated on a PASS too when a break was declared: the release notes have + // to describe what broke, so a declared break is reported, not discarded. + Breaks []surface.Break + // BreakingDeclared reports whether the cut ADDS a record whose impact is + // breaking — the author's declaration that this release narrows the surface + // on purpose. See RecordSet.DeclaresBreak for why the removed side does not + // count. + BreakingDeclared bool + // Reason names what to fix; empty on a clean pass. + Reason string +} + +// GuardSurface runs the release surface guardrail over the repository at root +// and writes nothing (spc-10 AC 4, plan outcome 5). +// +// The comparison is the BASELINE AS OF THE LAST RELEASE TAG against the current +// surface the caller hands in. Reading the baseline out of the tag is the single +// load-bearing decision in this function, and the obvious "simplification" is +// what would destroy it: the committed snapshot is kept byte-equal to the live +// command tree by a drift test, so comparing the committed FILE against the +// current tree would compare a file with itself, never report a break, and leave +// a green light where a guardrail was meant to be. +// +// current is passed in rather than built here because building it means walking +// the live cobra command tree, and internal/core must not know cobra. The front +// door (internal/surface/cli) owns the walk; this owns the judgement. +// +// The cut's own records answer "was this break declared?". Both the record set +// and the impact ordering are phase 1's (ShippedSince and RecordSet.DeclaresBreak), +// so the guardrail and the version derivation can never disagree about what +// shipped or about what breaking means. A break passes when — and only when — the +// cut ADDS a record whose impact is breaking; a superseded record leaving the +// terminal folder declares nothing about this release. +// +// It refuses, rather than passing, in three fail-closed cases: +// +// - No release tag. There is no immutable anchor to read a baseline from. +// - The tag's tree carries no snapshot. This is the repository's real state +// until a release is cut that contains the baseline, and it is deliberate: +// the first cut is the highest-risk one and must not sail through unguarded. +// The refusal text names the clean-cutover manual roll, which is what puts a +// baseline into a tag. +// - current disagrees with the snapshot committed at HEAD. See +// currentMatchesHead: the caller walks a compiled-in command tree, and a +// binary older than the tree being released reproduces the LAST release's +// surface, which would diff the baseline against itself. +// +// A snapshot that is present at the tag but cannot be decoded is an ERROR, not a +// refusal: a corrupt baseline read as "no surface" would make every command in +// the release look like surface that never existed. +func GuardSurface(root string, current surface.Snapshot) (SurfaceGuard, error) { + var g SurfaceGuard + + base, hasTag, err := LatestReleaseTag(root) + if err != nil { + return SurfaceGuard{}, err + } + if !hasTag { + return refuseGuard(g, "no release tag found — the surface baseline is read from the last release tag, "+ + "and without one no break can be detected (tag the current release first)"), nil + } + g.BaseTag = base.Tag() + + baseline, hasBaseline, err := surfaceSnapshotAt(root, g.BaseTag) + if err != nil { + return SurfaceGuard{}, err + } + if !hasBaseline { + return refuseGuard(g, noBaselineReason(g.BaseTag)), nil + } + + matches, reason, err := currentMatchesHead(root, current) + if err != nil { + return SurfaceGuard{}, err + } + if !matches { + return refuseGuard(g, reason), nil + } + + records, err := ShippedSince(root, g.BaseTag) + if err != nil { + return SurfaceGuard{}, err + } + g.BreakingDeclared = records.DeclaresBreak() + g.Breaks = surface.Diff(baseline, current) + + if len(g.Breaks) == 0 || g.BreakingDeclared { + g.Status = SurfaceGuardPassed + return g, nil + } + g.Status = SurfaceGuardFailed + g.Reason = failureReason(g.BaseTag, g.Breaks) + return g, nil +} + +// failureReason names every changed surface, because "a break was detected" +// sends the operator hunting through a 500-line snapshot for it. Each break is +// listed on its own line so a long list stays readable in a terminal, and the +// remedy states both ways out: declare it, or restore the surface. +func failureReason(baseTag string, breaks []surface.Break) string { + lines := make([]string, 0, len(breaks)) + for _, b := range breaks { + lines = append(lines, " - "+b.String()) + } + return fmt.Sprintf("the public surface narrowed since %s and no record in the cut declares it:\n%s\n"+ + "either ship a record with `impact: breaking` in this cut, or restore the surface", baseTag, strings.Join(lines, "\n")) +} + +// currentMatchesHead checks the caller's current surface against the snapshot +// committed at HEAD, and is what closes the stale-binary hole. +// +// current is walked from a COMPILED-IN command tree: the front door reflects over +// the cobra tree of the process that is running. An installed release binary +// therefore reports the surface of the release it was built from, not of the tree +// being released — so a command deleted since that build is missing from the +// baseline's counterpart as well, the diff comes back empty, and a real break +// ships labelled additive. Nothing else in the guardrail can see this: every +// other input (the baseline, the records) is read from git and would look +// perfectly consistent. +// +// HEAD's committed snapshot is the tree's own statement of its surface, kept +// honest by the drift test, so equality with it is exactly the proof that the +// walking binary IS the tree being released. A mismatch is a refusal rather than +// a failure: nothing has been shown about compatibility, only that the guardrail +// cannot trust its own input. +// +// HEAD is read rather than the working tree for the reason ShippedSince reads it: +// a release is cut from a commit, and a dirty or half-staged tree must not be +// able to change what a release gate concludes. +func currentMatchesHead(root string, current surface.Snapshot) (bool, string, error) { + head, found, err := surfaceSnapshotAt(root, "HEAD") + if err != nil { + return false, "", err + } + if !found { + return false, fmt.Sprintf("no surface snapshot committed at HEAD — %s is absent from the tree being "+ + "released, so the surface this binary reports cannot be checked against it (%s)", + surface.SnapshotPath, regenerateRemedy), nil + } + + wantBytes, err := surface.Encode(head) + if err != nil { + return false, "", err + } + gotBytes, err := surface.Encode(current) + if err != nil { + return false, "", err + } + if !bytes.Equal(wantBytes, gotBytes) { + return false, fmt.Sprintf("the surface this binary reports does not match %s at HEAD, so the guardrail "+ + "would compare the wrong tree and cannot answer whether the release breaks anything. Either the "+ + "binary predates the tree being released — rebuild it from this tree — or the snapshot is stale (%s).", + surface.SnapshotPath, regenerateRemedy), nil + } + return true, "", nil +} + +// regenerateRemedy names the generator once, so both halves of the stale-surface +// refusal tell the operator the same thing to run. +const regenerateRemedy = "regenerate and commit it with `go generate ./internal/surface/cli`" + +// noBaselineReason is the fail-closed refusal for a tag with no snapshot in its +// tree. It says what the state IS and what ends it, because this is not a +// transient error an operator can retry away: it holds for every cut until a +// release ships that carries the baseline, and an operator who reads it as a bug +// will go looking for one that is not there. +func noBaselineReason(baseTag string) string { + return fmt.Sprintf("no surface baseline in %s — %s is absent from that release's tree, so a break cannot be "+ + "detected and the cut refuses rather than passing unguarded. The baseline was seeded after %s, so every cut "+ + "refuses until a release is cut that CONTAINS it; the clean-cutover manual roll is what puts it into a tag.", + baseTag, surface.SnapshotPath, baseTag) +} + +// refuseGuard stamps a refusal, clearing anything that could read as a verdict +// on the surface. Whatever was resolved before the refusal (the anchor tag) is +// kept, because it is what the operator needs to act on the message. +func refuseGuard(g SurfaceGuard, reason string) SurfaceGuard { + g.Status = SurfaceGuardRefused + g.Reason = reason + g.Breaks = nil + g.BreakingDeclared = false + return g +} + +// maxSnapshotBytes caps the guarded blob read, in the same order and for the +// same reason as maxRecordBytes: the snapshot is a few hundred lines of JSON, so +// a blob that is not one must not stream unbounded input into a release gate. +const maxSnapshotBytes = 4 << 20 + +// surfaceSnapshotAt reads the committed surface snapshot out of ref's tree. +// +// Presence is probed with ls-tree rather than inferred from `cat-file` failing, +// because that command fails identically for "the path is not in this tree" and +// for "this ref does not exist" — and collapsing those would turn an unreadable +// repository into a silent "no baseline" refusal. A missing path returns +// found=false with no error, which the caller turns into the fail-closed +// refusal; anything git could not answer at all is returned as an error. +func surfaceSnapshotAt(root string, ref string) (surface.Snapshot, bool, error) { + listed, err := gitutil.Run(root, "ls-tree", "-z", "--name-only", ref, "--", surface.SnapshotPath) + if err != nil { + return surface.Snapshot{}, false, fmt.Errorf("looking for %s at %s: %w", surface.SnapshotPath, ref, err) + } + if strings.Trim(listed, "\x00") == "" { + return surface.Snapshot{}, false, nil + } + + blob, err := gitutil.RunLimited(root, maxSnapshotBytes, "cat-file", "blob", ref+":"+surface.SnapshotPath) + if err != nil { + return surface.Snapshot{}, false, fmt.Errorf("reading %s at %s: %w", surface.SnapshotPath, ref, err) + } + snap, err := surface.Decode([]byte(blob)) + if err != nil { + return surface.Snapshot{}, false, fmt.Errorf("decoding %s at %s: %w", surface.SnapshotPath, ref, err) + } + return snap, true, nil +} diff --git a/internal/core/changelog/guard_test.go b/internal/core/changelog/guard_test.go new file mode 100644 index 0000000..bc07a40 --- /dev/null +++ b/internal/core/changelog/guard_test.go @@ -0,0 +1,454 @@ +package changelog + +import ( + "strings" + "testing" + + "github.com/REPPL/abcd-cli/internal/core/surface" +) + +// The fixture constructors keep the taxonomy table readable. +func cmdOf(path string, flags ...surface.Flag) surface.Command { + return surface.Command{Path: path, Flags: flags} +} +func optFlag(name string) surface.Flag { return surface.Flag{Name: name, Type: "bool"} } +func reqFlag(name string) surface.Flag { + return surface.Flag{Name: name, Type: "bool", Required: true} +} +func entry(key string) surface.ManifestEntry { + return surface.ManifestEntry{File: ".claude-plugin/plugin.json", Key: key} +} + +// writeSurface writes an encoded snapshot into the fixture's working tree at the +// one canonical path, so a fixture exercises exactly the location the generator +// writes and the guardrail reads. +func writeSurface(r *fixtureRepo, snap surface.Snapshot) { + r.t.Helper() + data, err := surface.Encode(snap) + if err != nil { + r.t.Fatalf("Encode: %v", err) + } + r.write(surface.SnapshotPath, string(data)) +} + +// guardRepo builds the ONLY history shape that makes the guardrail meaningful: +// baseline is committed and TAGGED as the last release, and then the working +// tree's snapshot is replaced by current and committed on top. +// +// That second step is deliberate and is the fixture's whole point. The committed +// snapshot is kept equal to the live tree by the drift test, so a guardrail that +// compared "the committed file" against "the current tree" would be comparing a +// file with itself and could never fire. Every fixture built here reproduces +// that: HEAD's committed snapshot IS current. A break is therefore only visible +// to a guardrail that reads its baseline out of the TAG. +// +// impacts are the records the cut ships, one shipped intent each — the input the +// guardrail asks "is a break declared?" of. +func guardRepo(t *testing.T, baseline, current surface.Snapshot, impacts ...string) *fixtureRepo { + t.Helper() + r := newFixtureRepo(t) + writeSurface(r, baseline) + r.commit("seed the surface baseline") + r.git("tag", "v0.4.0") + + writeSurface(r, current) + r.commit("regenerate the surface snapshot") + for i, impact := range impacts { + id := "itd-" + string(rune('1'+i)) + r.record(".abcd/development/intents/shipped/"+id+"-thing.md", id, impact) + r.commit("ship " + id) + } + return r +} + +// TestGuardSurfaceTaxonomy walks every row of the break taxonomy (spc-10 AC 4, +// plan outcome 5) end to end through real git objects, in BOTH directions: what +// must fail the cut, and what must pass it. Each row gets its own fixture repo +// whose tagged snapshot genuinely differs from the current tree. +func TestGuardSurfaceTaxonomy(t *testing.T) { + tests := []struct { + name string + baseline surface.Snapshot + current surface.Snapshot + impacts []string + wantStatus SurfaceGuardStatus + wantNamed string + }{ + { + name: "removed command fails and names the command", + baseline: surface.NewSnapshot([]surface.Command{cmdOf("abcd"), cmdOf("abcd ghost")}, nil), + current: surface.NewSnapshot([]surface.Command{cmdOf("abcd")}, nil), + impacts: []string{"additive"}, + wantStatus: SurfaceGuardFailed, + wantNamed: "abcd ghost", + }, + { + name: "renamed command fails and names the old path", + baseline: surface.NewSnapshot([]surface.Command{cmdOf("abcd"), cmdOf("abcd ghost")}, nil), + current: surface.NewSnapshot([]surface.Command{cmdOf("abcd"), cmdOf("abcd spirit")}, nil), + impacts: []string{"fix"}, + wantStatus: SurfaceGuardFailed, + wantNamed: "abcd ghost", + }, + { + name: "removed flag fails and names the flag", + baseline: surface.NewSnapshot([]surface.Command{cmdOf("abcd", optFlag("json"), optFlag("quiet"))}, nil), + current: surface.NewSnapshot([]surface.Command{cmdOf("abcd", optFlag("json"))}, nil), + impacts: []string{"additive"}, + wantStatus: SurfaceGuardFailed, + wantNamed: "abcd --quiet", + }, + { + name: "renamed flag fails and names the old flag", + baseline: surface.NewSnapshot([]surface.Command{cmdOf("abcd", optFlag("json"))}, nil), + current: surface.NewSnapshot([]surface.Command{cmdOf("abcd", optFlag("jsonl"))}, nil), + impacts: []string{"additive"}, + wantStatus: SurfaceGuardFailed, + wantNamed: "abcd --json", + }, + { + name: "optional flag becoming required fails", + baseline: surface.NewSnapshot([]surface.Command{cmdOf("abcd launch", optFlag("dry-run"))}, nil), + current: surface.NewSnapshot([]surface.Command{cmdOf("abcd launch", reqFlag("dry-run"))}, nil), + impacts: []string{"additive"}, + wantStatus: SurfaceGuardFailed, + wantNamed: "abcd launch --dry-run", + }, + { + name: "absent flag arriving required on an existing command fails", + baseline: surface.NewSnapshot([]surface.Command{cmdOf("abcd launch")}, nil), + current: surface.NewSnapshot([]surface.Command{cmdOf("abcd launch", reqFlag("token"))}, nil), + impacts: []string{"additive"}, + wantStatus: SurfaceGuardFailed, + wantNamed: "abcd launch --token", + }, + { + name: "removed manifest entry fails and names file and key", + baseline: surface.NewSnapshot(nil, []surface.ManifestEntry{entry("author.name"), entry("description")}), + current: surface.NewSnapshot(nil, []surface.ManifestEntry{entry("author.name")}), + impacts: []string{"additive"}, + wantStatus: SurfaceGuardFailed, + wantNamed: ".claude-plugin/plugin.json:description", + }, + { + name: "new command passes", + baseline: surface.NewSnapshot([]surface.Command{cmdOf("abcd")}, nil), + current: surface.NewSnapshot([]surface.Command{cmdOf("abcd"), cmdOf("abcd brand-new")}, nil), + impacts: []string{"additive"}, + wantStatus: SurfaceGuardPassed, + }, + { + name: "new optional flag passes", + baseline: surface.NewSnapshot([]surface.Command{cmdOf("abcd", optFlag("json"))}, nil), + current: surface.NewSnapshot([]surface.Command{cmdOf("abcd", optFlag("json"), optFlag("verbose"))}, nil), + impacts: []string{"additive"}, + wantStatus: SurfaceGuardPassed, + }, + { + name: "new command carrying a required flag passes", + baseline: surface.NewSnapshot([]surface.Command{cmdOf("abcd")}, nil), + current: surface.NewSnapshot([]surface.Command{cmdOf("abcd"), cmdOf("abcd ship", reqFlag("changelog-json"))}, nil), + impacts: []string{"additive"}, + wantStatus: SurfaceGuardPassed, + }, + { + name: "reordering passes", + baseline: surface.NewSnapshot( + []surface.Command{cmdOf("abcd ghost", optFlag("quiet"), optFlag("json")), cmdOf("abcd")}, + []surface.ManifestEntry{entry("description"), entry("author.name")}), + current: surface.NewSnapshot( + []surface.Command{cmdOf("abcd"), cmdOf("abcd ghost", optFlag("json"), optFlag("quiet"))}, + []surface.ManifestEntry{entry("author.name"), entry("description")}), + impacts: []string{"additive"}, + wantStatus: SurfaceGuardPassed, + }, + { + name: "removed command with a breaking record in the cut passes", + baseline: surface.NewSnapshot([]surface.Command{cmdOf("abcd"), cmdOf("abcd ghost")}, nil), + current: surface.NewSnapshot([]surface.Command{cmdOf("abcd")}, nil), + impacts: []string{"breaking"}, + wantStatus: SurfaceGuardPassed, + }, + { + name: "newly-required flag with a breaking record in the cut passes", + baseline: surface.NewSnapshot([]surface.Command{cmdOf("abcd launch", optFlag("dry-run"))}, nil), + current: surface.NewSnapshot([]surface.Command{cmdOf("abcd launch", reqFlag("dry-run"))}, nil), + impacts: []string{"breaking"}, + wantStatus: SurfaceGuardPassed, + }, + { + name: "removed manifest entry with a breaking record in the cut passes", + baseline: surface.NewSnapshot(nil, []surface.ManifestEntry{entry("author.name"), entry("description")}), + current: surface.NewSnapshot(nil, []surface.ManifestEntry{entry("author.name")}), + impacts: []string{"breaking"}, + wantStatus: SurfaceGuardPassed, + }, + { + name: "a breaking record among several declares the break", + baseline: surface.NewSnapshot([]surface.Command{cmdOf("abcd"), cmdOf("abcd ghost")}, nil), + current: surface.NewSnapshot([]surface.Command{cmdOf("abcd")}, nil), + impacts: []string{"fix", "breaking", "internal"}, + wantStatus: SurfaceGuardPassed, + }, + { + name: "a break with no records at all in the cut fails", + baseline: surface.NewSnapshot([]surface.Command{cmdOf("abcd"), cmdOf("abcd ghost")}, nil), + current: surface.NewSnapshot([]surface.Command{cmdOf("abcd")}, nil), + wantStatus: SurfaceGuardFailed, + wantNamed: "abcd ghost", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + r := guardRepo(t, tc.baseline, tc.current, tc.impacts...) + + got, err := GuardSurface(r.root, tc.current) + if err != nil { + t.Fatalf("GuardSurface: %v", err) + } + if got.Status != tc.wantStatus { + t.Fatalf("Status = %q (reason %q), want %q", got.Status, got.Reason, tc.wantStatus) + } + if got.BaseTag != "v0.4.0" { + t.Errorf("BaseTag = %q, want v0.4.0", got.BaseTag) + } + if tc.wantNamed == "" { + return + } + if !strings.Contains(got.Reason, tc.wantNamed) { + t.Errorf("Reason = %q, want it to name the changed surface %q", got.Reason, tc.wantNamed) + } + }) + } +} + +// TestGuardSurfaceReadsBaselineFromTagNotWorkingTree is the anti-simplification +// test: it fails the moment the guardrail is "simplified" to compare the +// committed snapshot against the current tree. +// +// The drift test keeps the committed snapshot equal to the live tree, so that +// comparison is a file against itself and can never report a break. The fixture +// asserts the trap condition explicitly — HEAD's committed bytes ARE the current +// snapshot — and then asserts the break is still detected, which is only +// possible if the baseline came out of the tag. +func TestGuardSurfaceReadsBaselineFromTagNotWorkingTree(t *testing.T) { + baseline := surface.NewSnapshot([]surface.Command{cmdOf("abcd"), cmdOf("abcd ghost")}, nil) + current := surface.NewSnapshot([]surface.Command{cmdOf("abcd")}, nil) + r := guardRepo(t, baseline, current, "additive") + + wantCommitted, err := surface.Encode(current) + if err != nil { + t.Fatalf("Encode: %v", err) + } + committed := r.git("show", "HEAD:"+surface.SnapshotPath) + if strings.TrimSpace(string(wantCommitted)) != committed { + t.Fatalf("fixture precondition broken: HEAD's snapshot must equal the current tree, "+ + "otherwise this test does not exercise the trap it exists for\ncommitted: %s", committed) + } + + got, err := GuardSurface(r.root, current) + if err != nil { + t.Fatalf("GuardSurface: %v", err) + } + if got.Status != SurfaceGuardFailed { + t.Fatalf("Status = %q, want %q: the baseline must be read from %s, not from the committed file "+ + "(which the drift test keeps equal to the current tree)", got.Status, SurfaceGuardFailed, got.BaseTag) + } + if !strings.Contains(got.Reason, "abcd ghost") { + t.Errorf("Reason = %q, want it to name abcd ghost", got.Reason) + } +} + +// TestGuardSurfaceIgnoresBreakingOnTheRemovedSide pins that only what the cut +// ADDS can declare a break. +// +// The fixture is ordinary maintenance, not an attack: the tag's tree carries a +// shipped intent labelled `breaking` (the historical back-fill puts such labels +// on old intents), and this cut supersedes it — the record leaves shipped/ — while +// removing a command and shipping nothing but an `additive` intent. Judging the +// declaration over the whole cut would let the LAST release's label wave through +// a narrowing that nothing in THIS release declares, silently and with the break +// named in the result. +func TestGuardSurfaceIgnoresBreakingOnTheRemovedSide(t *testing.T) { + baseline := surface.NewSnapshot([]surface.Command{cmdOf("abcd"), cmdOf("abcd ghost")}, nil) + current := surface.NewSnapshot([]surface.Command{cmdOf("abcd")}, nil) + superseded := ".abcd/development/intents/shipped/itd-9-superseded.md" + + r := newFixtureRepo(t) + writeSurface(r, baseline) + r.record(superseded, "itd-9", "breaking") + r.commit("seed the surface baseline and a shipped breaking intent") + r.git("tag", "v0.4.0") + + writeSurface(r, current) + r.remove(superseded) + r.record(".abcd/development/intents/shipped/itd-10-thing.md", "itd-10", "additive") + r.commit("supersede itd-9 and ship itd-10") + + got, err := GuardSurface(r.root, current) + if err != nil { + t.Fatalf("GuardSurface: %v", err) + } + if got.BreakingDeclared { + t.Errorf("BreakingDeclared = true, want false: the only breaking record is LEAVING the release") + } + if got.Status != SurfaceGuardFailed { + t.Fatalf("Status = %q (reason %q), want %q: a superseded record's label must not declare this cut's break", + got.Status, got.Reason, SurfaceGuardFailed) + } + if !strings.Contains(got.Reason, "abcd ghost") { + t.Errorf("Reason = %q, want it to name the undeclared removal", got.Reason) + } +} + +// TestGuardSurfaceRefusesWhenCurrentDiffersFromHead pins the stale-binary +// fail-closed rule. The caller's `current` is walked from the command tree +// compiled into the RUNNING binary; when that binary was built before the tree +// being released, it reproduces the last release's surface — so the guardrail +// would compare the baseline against itself and wave through every removal made +// since. The fixture is exactly that: HEAD commits the narrowed surface while the +// caller hands in the baseline's. +func TestGuardSurfaceRefusesWhenCurrentDiffersFromHead(t *testing.T) { + baseline := surface.NewSnapshot([]surface.Command{cmdOf("abcd"), cmdOf("abcd ghost")}, nil) + current := surface.NewSnapshot([]surface.Command{cmdOf("abcd")}, nil) + r := guardRepo(t, baseline, current, "additive") + + // A binary built before `abcd ghost` was removed still walks the baseline. + got, err := GuardSurface(r.root, baseline) + if err != nil { + t.Fatalf("GuardSurface: %v", err) + } + if got.Status != SurfaceGuardRefused { + t.Fatalf("Status = %q (reason %q), want %q: a surface that disagrees with HEAD cannot be guarded", + got.Status, got.Reason, SurfaceGuardRefused) + } + if !strings.Contains(got.Reason, surface.SnapshotPath) { + t.Errorf("Reason = %q, want it to name the snapshot path", got.Reason) + } + if len(got.Breaks) != 0 { + t.Errorf("Breaks = %v, want none: nothing trustworthy was compared", got.Breaks) + } +} + +// TestGuardSurfaceRefusesWithoutBaseline pins the fail-closed no-baseline rule +// (plan outcome 5): a tag with no snapshot in its tree cannot be diffed, and a +// cut that cannot be diffed refuses instead of passing. That refusal is the +// repo's real state until a release is cut that CONTAINS the baseline, so the +// message has to say what puts it there. +func TestGuardSurfaceRefusesWithoutBaseline(t *testing.T) { + r := newFixtureRepo(t) + r.write("README.md", "# fixture\n") + r.commit("a release that predates the surface baseline") + r.git("tag", "v0.3.0") + current := surface.NewSnapshot([]surface.Command{cmdOf("abcd")}, nil) + writeSurface(r, current) + r.commit("seed the surface baseline after the release") + + got, err := GuardSurface(r.root, current) + if err != nil { + t.Fatalf("GuardSurface: %v", err) + } + if got.Status != SurfaceGuardRefused { + t.Fatalf("Status = %q, want %q: a cut with no baseline must refuse, never silently pass", + got.Status, SurfaceGuardRefused) + } + for _, want := range []string{"v0.3.0", surface.SnapshotPath, "refus"} { + if !strings.Contains(got.Reason, want) { + t.Errorf("Reason = %q, want it to mention %q", got.Reason, want) + } + } + if len(got.Breaks) != 0 { + t.Errorf("Breaks = %v, want none: nothing was compared", got.Breaks) + } +} + +// TestGuardSurfaceRefusesWithoutTag pins the other missing anchor: with no +// release tag there is no baseline to read at all, so the guardrail refuses for +// the same reason derivation does rather than reporting a clean surface. +func TestGuardSurfaceRefusesWithoutTag(t *testing.T) { + r := newFixtureRepo(t) + current := surface.NewSnapshot([]surface.Command{cmdOf("abcd")}, nil) + writeSurface(r, current) + r.commit("seed the surface baseline") + + got, err := GuardSurface(r.root, current) + if err != nil { + t.Fatalf("GuardSurface: %v", err) + } + if got.Status != SurfaceGuardRefused { + t.Fatalf("Status = %q, want %q", got.Status, SurfaceGuardRefused) + } + if !strings.Contains(got.Reason, "release tag") { + t.Errorf("Reason = %q, want it to name the missing release tag", got.Reason) + } +} + +// TestGuardSurfaceReportsEveryBreak pins that one failing cut names every +// changed surface, not just the first. An operator who has to fix, re-run, and +// discover the next break one at a time will stop reading the message. +func TestGuardSurfaceReportsEveryBreak(t *testing.T) { + baseline := surface.NewSnapshot( + []surface.Command{cmdOf("abcd", optFlag("json")), cmdOf("abcd ghost")}, + []surface.ManifestEntry{entry("author.name")}) + current := surface.NewSnapshot([]surface.Command{cmdOf("abcd")}, nil) + r := guardRepo(t, baseline, current, "additive") + + got, err := GuardSurface(r.root, current) + if err != nil { + t.Fatalf("GuardSurface: %v", err) + } + if got.Status != SurfaceGuardFailed { + t.Fatalf("Status = %q, want %q", got.Status, SurfaceGuardFailed) + } + if len(got.Breaks) != 3 { + t.Fatalf("Breaks = %v, want three (a flag, a command, a manifest entry)", got.Breaks) + } + for _, want := range []string{"abcd --json", "abcd ghost", ".claude-plugin/plugin.json:author.name"} { + if !strings.Contains(got.Reason, want) { + t.Errorf("Reason = %q, want it to name %q", got.Reason, want) + } + } +} + +// TestGuardSurfaceKeepsBreaksWhenDeclared pins that a declared break is still +// REPORTED, not discarded: the release notes have to describe what broke, so a +// passing guard still hands its caller the surfaces that changed. +func TestGuardSurfaceKeepsBreaksWhenDeclared(t *testing.T) { + baseline := surface.NewSnapshot([]surface.Command{cmdOf("abcd"), cmdOf("abcd ghost")}, nil) + current := surface.NewSnapshot([]surface.Command{cmdOf("abcd")}, nil) + r := guardRepo(t, baseline, current, "breaking") + + got, err := GuardSurface(r.root, current) + if err != nil { + t.Fatalf("GuardSurface: %v", err) + } + if got.Status != SurfaceGuardPassed || !got.BreakingDeclared { + t.Fatalf("Status = %q, BreakingDeclared = %v, want passed and declared", got.Status, got.BreakingDeclared) + } + if len(got.Breaks) != 1 || got.Breaks[0].Surface != "abcd ghost" { + t.Errorf("Breaks = %v, want the declared break to still be reported", got.Breaks) + } + if got.Reason != "" { + t.Errorf("Reason = %q, want empty on a passing guard", got.Reason) + } +} + +// TestGuardSurfaceRejectsUnreadableBaseline pins the difference between a +// refusal and an error. An ABSENT baseline is a state the operator resolves by +// cutting a release; a baseline that is present but cannot be decoded means the +// tag's artefact is corrupt, and treating it as "no surface" would make every +// command in the release look like surface that never existed. +func TestGuardSurfaceRejectsUnreadableBaseline(t *testing.T) { + r := newFixtureRepo(t) + r.write(surface.SnapshotPath, "{ not json\n") + r.commit("a corrupt baseline") + r.git("tag", "v0.4.0") + current := surface.NewSnapshot([]surface.Command{cmdOf("abcd")}, nil) + writeSurface(r, current) + r.commit("regenerate the surface snapshot") + + if _, err := GuardSurface(r.root, current); err == nil { + t.Fatal("GuardSurface = nil error, want a corrupt baseline to be an error, not a pass") + } +} diff --git a/internal/core/changelog/shipped.go b/internal/core/changelog/shipped.go index b840883..dbb8758 100644 --- a/internal/core/changelog/shipped.go +++ b/internal/core/changelog/shipped.go @@ -129,10 +129,36 @@ func unlabelled(records []Record) []Record { // Impact is the strongest impact in the cut — the one that decides the bump. // An empty or all-internal cut yields ImpactInternal, which reads at the call // site as "nothing to release". -func (s RecordSet) Impact() Impact { - all := s.All() - impacts := make([]Impact, 0, len(all)) - for _, r := range all { +func (s RecordSet) Impact() Impact { return maxImpactOf(s.All()) } + +// DeclaresBreak reports whether the cut declares that it narrows the public +// surface on purpose — the release guardrail's "was this break declared?" test. +// +// It judges the ADDED side only, and that restriction is the whole point of the +// method existing separately from Impact(). A Removed record's blob is read from +// the anchor tag's immutable tree, so its label is what the LAST release +// declared, not what this one does. Supersession is a supported flow — a shipped +// intent leaves the folder when a later one replaces it — and the historical +// impact back-fill puts `breaking` labels on old intents, so a cut that ships +// nothing breaking can easily carry a superseded `breaking` record on its Removed +// side. Counting that as a declaration would wave an undeclared removal through +// the guardrail, silently, which is the exact fail-open the guardrail exists to +// prevent. The same reasoning excludes Removed from UnlabelledAdded. +// +// A withdrawal that genuinely narrows the surface therefore ships its own +// superseding record labelled `breaking`. +// +// Impact() deliberately keeps counting both sides: the version must account for +// every record in the cut, because a record leaving a terminal folder is itself a +// user-visible change. +func (s RecordSet) DeclaresBreak() bool { return maxImpactOf(s.Added) == ImpactBreaking } + +// maxImpactOf is the one place a slice of records is reduced to its strongest +// impact, so the version arithmetic and the guardrail can never disagree about +// what the maximum means. +func maxImpactOf(records []Record) Impact { + impacts := make([]Impact, 0, len(records)) + for _, r := range records { impacts = append(impacts, r.Impact) } return MaxImpact(impacts) diff --git a/internal/core/surface/diff.go b/internal/core/surface/diff.go new file mode 100644 index 0000000..1d2d1d1 --- /dev/null +++ b/internal/core/surface/diff.go @@ -0,0 +1,175 @@ +package surface + +// BreakKind classifies one structural incompatibility between two snapshots. +// +// The set is exactly the break taxonomy of spc-10 and plan outcome 5 — nothing +// more. A kind absent from this list (a changed flag type, a flag losing its +// shorthand, a command becoming hidden) is deliberately NOT a break: the +// taxonomy is what a release contract can be held to, and widening it here +// would make the gate refuse releases the contract permits. Those are named as +// limits in Diff rather than silently folded in. +// +// The string values are stable identifiers a renderer or a machine-readable +// preview can switch on, so renaming a constant is safe and changing a value is +// a contract change. +type BreakKind string + +const ( + // BreakCommandRemoved covers a removal AND a rename: a renamed command is a + // removal at the path callers type, plus an addition nobody depends on yet. + BreakCommandRemoved BreakKind = "command_removed" + // BreakFlagRemoved covers a removed or renamed flag, for the same reason. + BreakFlagRemoved BreakKind = "flag_removed" + // BreakFlagRequired is a flag that was optional or absent becoming + // mandatory — the one narrowing that adds surface rather than deleting it, + // and the one most easily missed, because the flag is still there. + BreakFlagRequired BreakKind = "flag_required" + // BreakManifestRemoved is a declared manifest key path that is gone. + BreakManifestRemoved BreakKind = "manifest_entry_removed" +) + +// Break is one structural incompatibility, named precisely enough to act on. +// +// Surface is the whole point of the type. A gate that reports "a break was +// detected" tells the operator to go hunting; Surface names the command path, +// the command-and-flag, or the manifest file and key that changed, in the form +// the operator reads it in the tree. +type Break struct { + Kind BreakKind + Surface string +} + +// String renders one break as the line a failing gate names it with. It says +// "removed or renamed" for the two deletion kinds because a structural diff +// genuinely cannot tell them apart — the old path is gone either way, and +// claiming to know which happened would be a guess the operator has to check. +func (b Break) String() string { + switch b.Kind { + case BreakCommandRemoved: + return "command removed or renamed: " + b.Surface + case BreakFlagRemoved: + return "flag removed or renamed: " + b.Surface + case BreakFlagRequired: + return "flag is now required: " + b.Surface + case BreakManifestRemoved: + return "manifest entry removed: " + b.Surface + default: + return string(b.Kind) + ": " + b.Surface + } +} + +// Diff reports every way current narrows the compatibility surface base +// declared — the structural half of the release guardrail (spc-10 AC 4). +// +// It is a pure comparison of two values: it reads no git, no files, and no +// clock, so the same pair of snapshots always yields the same breaks in the same +// order. Where the two snapshots come from is the CALLER's decision and is where +// the guardrail is easiest to get wrong: comparing the committed baseline +// against the current tree compares a file against itself, because a drift test +// keeps them equal. The baseline must be read out of the last release tag. +// +// What Diff reports is the taxonomy and only the taxonomy: +// +// - a removed or renamed command; +// - a removed or renamed flag on a command that still exists; +// - a flag that was optional, or absent, becoming required on a command that +// still exists; +// - a removed declared manifest entry. +// +// Additions of any kind are silent — a new command, a new optional flag, a new +// manifest entry — as are reordering and every change the snapshot does not +// model at all (help text, descriptions, summaries), which is precisely why the +// snapshot does not model them. +// +// Two known limits, stated rather than hidden. A flag whose VALUE TYPE changes +// (string to stringSlice) is a compatibility event the taxonomy does not list, +// so it passes here and rests on the author's `breaking` judgement. And a +// behavioural break behind an unchanged surface is invisible to any structural +// diff. Both are the author's call; the gate backstops the structural ones. +// +// Results are ordered: commands in canonical path order (each command's own flag +// breaks before the next command's), then manifest entries in canonical order. +// The whole set is returned, never just the first, so one fix-and-rerun cycle +// shows the operator everything that changed. +func Diff(base, current Snapshot) []Break { + // Canonicalise defensively rather than trusting the caller to have gone + // through NewSnapshot or Decode: a hand-built Snapshot must not be able to + // change the ORDER of a gate's findings. + base, current = canonical(base), canonical(current) + + currentCommands := make(map[string]Command, len(current.Commands)) + for _, c := range current.Commands { + currentCommands[c.Path] = c + } + + var breaks []Break + for _, was := range base.Commands { + now, stillThere := currentCommands[was.Path] + if !stillThere { + // The command is gone; its flags went with it. Reporting them too + // would turn one break into one-per-flag and bury the command that + // is the thing to fix. + breaks = append(breaks, Break{Kind: BreakCommandRemoved, Surface: was.Path}) + continue + } + breaks = append(breaks, flagBreaks(was, now)...) + } + + currentEntries := make(map[ManifestEntry]struct{}, len(current.Manifest)) + for _, e := range current.Manifest { + currentEntries[e] = struct{}{} + } + for _, was := range base.Manifest { + if _, stillThere := currentEntries[was]; !stillThere { + breaks = append(breaks, Break{Kind: BreakManifestRemoved, Surface: was.File + ":" + was.Key}) + } + } + return breaks +} + +// flagBreaks compares the flags of ONE command that exists in both snapshots. +// +// Requiredness is judged here, and only here, which is what keeps the two +// taxonomy rows that meet at this point from contradicting each other: "a new +// command is not a break" and "an absent flag becoming required is a break". +// Every flag of a brand-new command was absent at the baseline, so judging +// requiredness across the whole tree would report each new command carrying a +// required flag as a break. Nobody can depend on surface that did not exist, so +// a new command's flags — required or not — are new surface, not narrowed +// surface. A required flag arriving on an EXISTING command is the real +// narrowing: every call that worked before now fails. +func flagBreaks(was, now Command) []Break { + baseFlags := make(map[string]Flag, len(was.Flags)) + for _, f := range was.Flags { + baseFlags[f.Name] = f + } + currentFlags := make(map[string]Flag, len(now.Flags)) + for _, f := range now.Flags { + currentFlags[f.Name] = f + } + + var breaks []Break + for _, f := range was.Flags { + if _, stillThere := currentFlags[f.Name]; !stillThere { + breaks = append(breaks, Break{Kind: BreakFlagRemoved, Surface: flagSurface(was.Path, f.Name)}) + } + } + for _, f := range now.Flags { + if !f.Required { + continue + } + // Already required at the baseline: the narrowing shipped in an earlier + // release and is not this cut's break to declare again. + if before, existed := baseFlags[f.Name]; existed && before.Required { + continue + } + breaks = append(breaks, Break{Kind: BreakFlagRequired, Surface: flagSurface(now.Path, f.Name)}) + } + return breaks +} + +// flagSurface names a flag the way an operator invokes it, so the failure can be +// pasted into a shell and understood without consulting the snapshot. +func flagSurface(commandPath, flagName string) string { + return commandPath + " --" + flagName +} diff --git a/internal/core/surface/diff_test.go b/internal/core/surface/diff_test.go new file mode 100644 index 0000000..f5e06f1 --- /dev/null +++ b/internal/core/surface/diff_test.go @@ -0,0 +1,279 @@ +package surface + +import ( + "testing" +) + +// The fixture constructors keep the taxonomy table readable: a row should show +// what changed, not how a Command literal is spelt. +func cmdOf(path string, flags ...Flag) Command { return Command{Path: path, Flags: flags} } +func optFlag(name string) Flag { return Flag{Name: name, Type: "bool"} } +func reqFlag(name string) Flag { return Flag{Name: name, Type: "bool", Required: true} } +func entry(key string) ManifestEntry { + return ManifestEntry{File: ".claude-plugin/plugin.json", Key: key} +} + +// describe renders a break set as the lines a failure message would name, so a +// table row can state the expected surface rather than a struct. +func describe(breaks []Break) []string { + out := make([]string, 0, len(breaks)) + for _, b := range breaks { + out = append(out, b.String()) + } + return out +} + +func sameLines(got, want []string) bool { + if len(got) != len(want) { + return false + } + for i := range got { + if got[i] != want[i] { + return false + } + } + return true +} + +// TestBreakKindWireValues pins the four identifiers a machine-readable front door +// emits. The constants are the only part of the taxonomy a consumer switches on, +// and every other test in this file asserts through String(), which would keep +// passing if a value were retyped — so without this, changing the contract is a +// silent edit. Renaming a constant stays safe; changing a value fails here, which +// is the point. +func TestBreakKindWireValues(t *testing.T) { + tests := []struct { + kind BreakKind + want string + }{ + {BreakCommandRemoved, "command_removed"}, + {BreakFlagRemoved, "flag_removed"}, + {BreakFlagRequired, "flag_required"}, + {BreakManifestRemoved, "manifest_entry_removed"}, + } + for _, tc := range tests { + t.Run(tc.want, func(t *testing.T) { + if string(tc.kind) != tc.want { + t.Errorf("BreakKind = %q, want %q: the wire value is a contract a consumer switches on", tc.kind, tc.want) + } + }) + } +} + +// TestDiffReportsTheRightKind pins that each taxonomy row is emitted under its own +// Kind, not merely under the right prose. Every other case here reads String(), +// which cannot tell a mislabelled Kind from a correct one once the renderer is +// changed to switch on Kind instead. +func TestDiffReportsTheRightKind(t *testing.T) { + tests := []struct { + name string + base Snapshot + current Snapshot + want BreakKind + }{ + { + name: "removed command", + base: NewSnapshot([]Command{cmdOf("abcd"), cmdOf("abcd ghost")}, nil), + current: NewSnapshot([]Command{cmdOf("abcd")}, nil), + want: BreakCommandRemoved, + }, + { + name: "removed flag", + base: NewSnapshot([]Command{cmdOf("abcd", optFlag("json"))}, nil), + current: NewSnapshot([]Command{cmdOf("abcd")}, nil), + want: BreakFlagRemoved, + }, + { + name: "flag becoming required", + base: NewSnapshot([]Command{cmdOf("abcd launch", optFlag("dry-run"))}, nil), + current: NewSnapshot([]Command{cmdOf("abcd launch", reqFlag("dry-run"))}, nil), + want: BreakFlagRequired, + }, + { + name: "removed manifest entry", + base: NewSnapshot(nil, []ManifestEntry{entry("description")}), + current: NewSnapshot(nil, nil), + want: BreakManifestRemoved, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := Diff(tc.base, tc.current) + if len(got) != 1 { + t.Fatalf("Diff = %v, want exactly one break", describe(got)) + } + if got[0].Kind != tc.want { + t.Errorf("Kind = %q, want %q", got[0].Kind, tc.want) + } + }) + } +} + +// TestDiffTaxonomy walks every row of the break taxonomy (spc-10, plan outcome +// 5) in BOTH directions: what must be reported as a break, and what must not. +// The table is the specification — a row removed here is a hole in the gate. +func TestDiffTaxonomy(t *testing.T) { + tests := []struct { + name string + base Snapshot + current Snapshot + want []string + }{ + { + name: "removed command is a break", + base: NewSnapshot([]Command{cmdOf("abcd"), cmdOf("abcd ghost")}, nil), + current: NewSnapshot([]Command{cmdOf("abcd")}, nil), + want: []string{"command removed or renamed: abcd ghost"}, + }, + { + name: "renamed command is a break at its old path", + base: NewSnapshot([]Command{cmdOf("abcd"), cmdOf("abcd ghost")}, nil), + current: NewSnapshot([]Command{cmdOf("abcd"), cmdOf("abcd spirit")}, nil), + want: []string{"command removed or renamed: abcd ghost"}, + }, + { + name: "removed flag is a break", + base: NewSnapshot([]Command{cmdOf("abcd", optFlag("json"), optFlag("quiet"))}, nil), + current: NewSnapshot([]Command{cmdOf("abcd", optFlag("json"))}, nil), + want: []string{"flag removed or renamed: abcd --quiet"}, + }, + { + name: "renamed flag is a break at its old name", + base: NewSnapshot([]Command{cmdOf("abcd", optFlag("json"))}, nil), + current: NewSnapshot([]Command{cmdOf("abcd", optFlag("jsonl"))}, nil), + want: []string{"flag removed or renamed: abcd --json"}, + }, + { + name: "optional flag becoming required is a break", + base: NewSnapshot([]Command{cmdOf("abcd launch", optFlag("dry-run"))}, nil), + current: NewSnapshot([]Command{cmdOf("abcd launch", reqFlag("dry-run"))}, nil), + want: []string{"flag is now required: abcd launch --dry-run"}, + }, + { + name: "absent flag arriving required on an existing command is a break", + base: NewSnapshot([]Command{cmdOf("abcd launch")}, nil), + current: NewSnapshot([]Command{cmdOf("abcd launch", reqFlag("token"))}, nil), + want: []string{"flag is now required: abcd launch --token"}, + }, + { + name: "removed manifest entry is a break", + base: NewSnapshot(nil, []ManifestEntry{entry("author.name"), entry("description")}), + current: NewSnapshot(nil, []ManifestEntry{entry("author.name")}), + want: []string{"manifest entry removed: .claude-plugin/plugin.json:description"}, + }, + { + name: "new command is not a break", + base: NewSnapshot([]Command{cmdOf("abcd")}, nil), + current: NewSnapshot([]Command{cmdOf("abcd"), cmdOf("abcd brand-new")}, nil), + want: nil, + }, + { + name: "new optional flag is not a break", + base: NewSnapshot([]Command{cmdOf("abcd", optFlag("json"))}, nil), + current: NewSnapshot([]Command{cmdOf("abcd", optFlag("json"), optFlag("verbose"))}, nil), + want: nil, + }, + { + name: "new manifest entry is not a break", + base: NewSnapshot(nil, []ManifestEntry{entry("author.name")}), + current: NewSnapshot(nil, []ManifestEntry{entry("author.name"), entry("version")}), + want: nil, + }, + { + name: "reordering is not a break", + base: Snapshot{SchemaVersion: SchemaVersion, + Commands: []Command{cmdOf("abcd ghost", optFlag("quiet"), optFlag("json")), cmdOf("abcd")}, + Manifest: []ManifestEntry{entry("description"), entry("author.name")}}, + current: Snapshot{SchemaVersion: SchemaVersion, + Commands: []Command{cmdOf("abcd"), cmdOf("abcd ghost", optFlag("json"), optFlag("quiet"))}, + Manifest: []ManifestEntry{entry("author.name"), entry("description")}}, + want: nil, + }, + { + name: "a flag staying required is not a fresh break", + base: NewSnapshot([]Command{cmdOf("abcd launch", reqFlag("token"))}, nil), + current: NewSnapshot([]Command{cmdOf("abcd launch", reqFlag("token"))}, nil), + want: nil, + }, + { + name: "a required flag becoming optional is not a break", + base: NewSnapshot([]Command{cmdOf("abcd launch", reqFlag("token"))}, nil), + current: NewSnapshot([]Command{cmdOf("abcd launch", optFlag("token"))}, nil), + want: nil, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := describe(Diff(tc.base, tc.current)) + if !sameLines(got, tc.want) { + t.Errorf("Diff = %v, want %v", got, tc.want) + } + }) + } +} + +// TestDiffIgnoresRequirednessOnNewCommands is the edge the taxonomy's two rows +// meet at: "a new command is not a break" and "an absent flag becoming required +// is a break". A brand-new command's flags were all absent at the baseline, so a +// naive requiredness check would report every new command carrying a required +// flag as a break and make adding one impossible without a `breaking` record. +// Nobody can depend on surface that did not exist, so requiredness is only +// judged for commands present in BOTH snapshots. +func TestDiffIgnoresRequirednessOnNewCommands(t *testing.T) { + base := NewSnapshot([]Command{cmdOf("abcd")}, nil) + current := NewSnapshot([]Command{cmdOf("abcd"), cmdOf("abcd ship", reqFlag("changelog-json"))}, nil) + + if got := describe(Diff(base, current)); len(got) != 0 { + t.Errorf("Diff = %v, want no breaks: a new command's required flag is new surface, not a narrowed one", got) + } +} + +// TestDiffReportsRemovedCommandOnce pins that removing a command reports the +// command, not also every flag it carried. A five-flag command would otherwise +// produce six lines for one break and bury the surface that actually matters. +func TestDiffReportsRemovedCommandOnce(t *testing.T) { + base := NewSnapshot([]Command{cmdOf("abcd"), cmdOf("abcd ghost", optFlag("json"), optFlag("quiet"))}, nil) + current := NewSnapshot([]Command{cmdOf("abcd")}, nil) + + want := []string{"command removed or renamed: abcd ghost"} + if got := describe(Diff(base, current)); !sameLines(got, want) { + t.Errorf("Diff = %v, want %v", got, want) + } +} + +// TestDiffIsOrderedAndCumulative pins that a multi-break cut reports every break +// in one deterministic pass: commands before manifest entries, each in canonical +// order. A gate that stopped at the first break would make the operator fix and +// re-run once per break. +func TestDiffIsOrderedAndCumulative(t *testing.T) { + base := NewSnapshot( + []Command{cmdOf("abcd", optFlag("json")), cmdOf("abcd zeta"), cmdOf("abcd alpha")}, + []ManifestEntry{entry("author.name"), entry("description")}, + ) + current := NewSnapshot([]Command{cmdOf("abcd")}, nil) + + want := []string{ + "flag removed or renamed: abcd --json", + "command removed or renamed: abcd alpha", + "command removed or renamed: abcd zeta", + "manifest entry removed: .claude-plugin/plugin.json:author.name", + "manifest entry removed: .claude-plugin/plugin.json:description", + } + if got := describe(Diff(base, current)); !sameLines(got, want) { + t.Errorf("Diff = %v, want %v", got, want) + } +} + +// TestDiffDistinguishesManifestFiles pins that an entry is identified by its file +// AND its key: the same key path in the other manifest is a different entry, so +// moving a key between manifests is a removal from the one that lost it. +func TestDiffDistinguishesManifestFiles(t *testing.T) { + base := NewSnapshot(nil, []ManifestEntry{{File: ".claude-plugin/plugin.json", Key: "name"}}) + current := NewSnapshot(nil, []ManifestEntry{{File: ".claude-plugin/marketplace.json", Key: "name"}}) + + want := []string{"manifest entry removed: .claude-plugin/plugin.json:name"} + if got := describe(Diff(base, current)); !sameLines(got, want) { + t.Errorf("Diff = %v, want %v", got, want) + } +} diff --git a/internal/core/surface/manifest.go b/internal/core/surface/manifest.go new file mode 100644 index 0000000..3e6fb7d --- /dev/null +++ b/internal/core/surface/manifest.go @@ -0,0 +1,167 @@ +package surface + +import ( + "encoding/json" + "fmt" + "path/filepath" + "sort" + + "github.com/REPPL/abcd-cli/internal/fsutil" +) + +// The two plugin manifests that together declare abcd's distribution surface: the +// plugin's own identity and the marketplace listing that points at it. They are +// repo-relative and slash-separated, because the path travels into the committed +// snapshot and an absolute path would both leak the generating machine and make +// the artefact fail to reproduce anywhere else. +const ( + pluginManifestPath = ".claude-plugin/plugin.json" + marketplaceManifestPath = ".claude-plugin/marketplace.json" +) + +// manifestPaths is the ordered list the entries are collected from; the snapshot +// sorts them again, so this order is for readability only. +var manifestPaths = []string{pluginManifestPath, marketplaceManifestPath} + +// maxManifestBytes caps the guarded read. A plugin manifest is a short +// declaration, so a file that is not one must not stream unbounded input into a +// generator that runs in CI. +const maxManifestBytes = 1 << 20 + +// nameKey is the field an array element is keyed by when the array holds objects. +// Both manifests identify list members this way (marketplace.json's plugins[]), +// so the name is the member's identity and its position is not. +const nameKey = "name" + +// ManifestEntries reads both plugin manifests under repoRoot and flattens them +// into the set of declared key paths — the manifest half of the surface +// snapshot. +// +// An ENTRY is one leaf key path, and only its presence is recorded. The +// flattening rules are: +// +// - an object contributes one entry per leaf beneath it, joined with dots +// ("author.name"); an empty object contributes one entry at its own path, so +// that emptying it is still visible; +// - an array whose elements are all objects carrying a unique, non-empty +// "name" contributes entries beneath each element, keyed by that name +// ("plugins[abcd].source") — so reordering the array changes nothing while +// removing or renaming a member is a removed entry; +// - any other array (scalars such as keywords, unnamed objects, ambiguous +// duplicate names) contributes exactly one entry at its own path. Keying +// those by position would report a reorder as a break, and reordering is +// explicitly not one; +// - a scalar, including null, contributes one entry at its own path. +// +// Values are not recorded, deliberately: the break taxonomy makes a removed +// entry a break and a changed description or a reordered list not one, so +// carrying values would report every prose edit as a surface change. +// +// Nothing here treats any particular key as expected or required. plugin.json +// declares no version in the development tree and the rendered release payload +// adds one; both are ordinary entry sets, so the absence of a version is not an +// anomaly and its later presence reads as one added entry. +// +// A manifest that cannot be read or parsed, or whose root is not a JSON object, +// is an error rather than an empty entry set: reporting "no entries" for a +// manifest that failed to load would make every declared entry look like surface +// that was never there, which is exactly the removal the guardrail exists to +// catch. +func ManifestEntries(repoRoot string) ([]ManifestEntry, error) { + var out []ManifestEntry + for _, rel := range manifestPaths { + data, err := fsutil.ReadGuarded(filepath.Join(repoRoot, filepath.FromSlash(rel)), maxManifestBytes) + if err != nil { + return nil, fmt.Errorf("reading %s: %w", rel, err) + } + var doc map[string]any + if err := json.Unmarshal(data, &doc); err != nil { + return nil, fmt.Errorf("parsing %s: %w", rel, err) + } + out = append(out, flatten(rel, "", doc)...) + } + sortEntries(out) + return out, nil +} + +// flatten walks one decoded manifest value and returns the leaf entries beneath +// it. Object keys are visited in sorted order and array elements in document +// order; ManifestEntries sorts the result, so neither the JSON decoder's map +// iteration nor an element's position can reach the artefact. +func flatten(file string, prefix string, value any) []ManifestEntry { + switch v := value.(type) { + case map[string]any: + if len(v) == 0 { + return leaf(file, prefix) + } + var out []ManifestEntry + keys := make([]string, 0, len(v)) + for k := range v { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + out = append(out, flatten(file, join(prefix, k), v[k])...) + } + return out + case []any: + names, ok := elementNames(v) + if !ok { + return leaf(file, prefix) + } + var out []ManifestEntry + for i, elem := range v { + out = append(out, flatten(file, prefix+"["+names[i]+"]", elem)...) + } + return out + default: + return leaf(file, prefix) + } +} + +// elementNames reports the per-element key for an array, and whether the array +// may be keyed at all. It may only be keyed when every element is an object with +// a non-empty string name and no two names collide — anything less makes the key +// ambiguous, and an ambiguous key would either invent entries or silently merge +// two of them. +func elementNames(arr []any) ([]string, bool) { + if len(arr) == 0 { + return nil, false + } + names := make([]string, 0, len(arr)) + seen := make(map[string]bool, len(arr)) + for _, elem := range arr { + obj, isObject := elem.(map[string]any) + if !isObject { + return nil, false + } + name, isString := obj[nameKey].(string) + if !isString || name == "" || seen[name] { + return nil, false + } + seen[name] = true + names = append(names, name) + } + return names, true +} + +// leaf records one entry. An empty prefix means the whole document was a scalar +// or an empty object; that is a manifest with no declared keys, and recording an +// unnamed entry for it would be meaningless, so it contributes nothing. +func leaf(file, prefix string) []ManifestEntry { + if prefix == "" { + return nil + } + return []ManifestEntry{{File: file, Key: prefix}} +} + +// join builds a dotted key path. Keys containing a dot or a bracket would render +// an ambiguous path; neither manifest uses such a key, and the snapshot is a +// comparison artefact rather than a parser input, so the paths stay readable +// instead of escaped. +func join(prefix, key string) string { + if prefix == "" { + return key + } + return prefix + "." + key +} diff --git a/internal/core/surface/manifest_test.go b/internal/core/surface/manifest_test.go new file mode 100644 index 0000000..dabde55 --- /dev/null +++ b/internal/core/surface/manifest_test.go @@ -0,0 +1,189 @@ +package surface + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// writeManifests lays down a throwaway repo root carrying the two plugin +// manifests, so every manifest test states exactly the JSON it is about. +func writeManifests(t *testing.T, plugin, marketplace string) string { + t.Helper() + root := t.TempDir() + dir := filepath.Join(root, ".claude-plugin") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if plugin != "" { + if err := os.WriteFile(filepath.Join(dir, "plugin.json"), []byte(plugin), 0o644); err != nil { + t.Fatalf("write plugin.json: %v", err) + } + } + if marketplace != "" { + if err := os.WriteFile(filepath.Join(dir, "marketplace.json"), []byte(marketplace), 0o644); err != nil { + t.Fatalf("write marketplace.json: %v", err) + } + } + return root +} + +func keysFor(t *testing.T, entries []ManifestEntry, file string) []string { + t.Helper() + var out []string + for _, e := range entries { + if strings.HasSuffix(e.File, file) { + out = append(out, e.Key) + } + } + return out +} + +// TestManifestEntriesFlattensDeclaredKeys pins what an "entry" is: one leaf key +// path per declared value, objects flattened with dots, arrays of named objects +// keyed by their name, and any other array recorded as a single leaf so that +// reordering or editing its members is invisible to the guardrail. +func TestManifestEntriesFlattensDeclaredKeys(t *testing.T) { + root := writeManifests(t, + `{"name":"abcd","author":{"name":"REPPL","url":"https://example.invalid"},"keywords":["a","b"]}`, + `{"name":"abcd-marketplace","owner":{"name":"REPPL"},"plugins":[{"name":"abcd","source":"./"}]}`) + + entries, err := ManifestEntries(root) + if err != nil { + t.Fatalf("ManifestEntries: %v", err) + } + + wantPlugin := []string{"author.name", "author.url", "keywords", "name"} + if got := keysFor(t, entries, "plugin.json"); !equalStrings(got, wantPlugin) { + t.Fatalf("plugin.json keys = %v, want %v", got, wantPlugin) + } + wantMarket := []string{"name", "owner.name", "plugins[abcd].name", "plugins[abcd].source"} + if got := keysFor(t, entries, "marketplace.json"); !equalStrings(got, wantMarket) { + t.Fatalf("marketplace.json keys = %v, want %v", got, wantMarket) + } +} + +// TestManifestEntriesArrayKeying covers the array rules one at a time: named +// objects are keyed by name (so reordering the array is not a change), duplicate +// or missing names collapse to one leaf (the keying is ambiguous, so recording +// per-element entries would invent surface), and an empty container still +// registers its own presence. +func TestManifestEntriesArrayKeying(t *testing.T) { + tests := []struct { + name string + plugin string + want []string + }{ + { + name: "named objects keyed by name", + plugin: `{"items":[{"name":"b","v":1},{"name":"a","v":2}]}`, + want: []string{"items[a].name", "items[a].v", "items[b].name", "items[b].v"}, + }, + { + name: "duplicate names collapse to one leaf", + plugin: `{"items":[{"name":"a"},{"name":"a"}]}`, + want: []string{"items"}, + }, + { + name: "unnamed objects collapse to one leaf", + plugin: `{"items":[{"v":1}]}`, + want: []string{"items"}, + }, + { + name: "scalar array is one leaf", + plugin: `{"items":["a","b"]}`, + want: []string{"items"}, + }, + { + name: "empty containers still register", + plugin: `{"items":[],"obj":{}}`, + want: []string{"items", "obj"}, + }, + { + name: "null is a declared key", + plugin: `{"a":null}`, + want: []string{"a"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + root := writeManifests(t, tc.plugin, `{"name":"m"}`) + entries, err := ManifestEntries(root) + if err != nil { + t.Fatalf("ManifestEntries: %v", err) + } + if got := keysFor(t, entries, "plugin.json"); !equalStrings(got, tc.want) { + t.Fatalf("keys = %v, want %v", got, tc.want) + } + }) + } +} + +// TestManifestEntriesTreatsVersionAsOrdinary is the guard for the release +// payload: the development tree carries no `version` in plugin.json and the +// rendered release payload does. Absence must not be an error or a special case, +// and presence must read as one ordinary added entry. +func TestManifestEntriesTreatsVersionAsOrdinary(t *testing.T) { + without := writeManifests(t, `{"name":"abcd"}`, `{"name":"m"}`) + entries, err := ManifestEntries(without) + if err != nil { + t.Fatalf("ManifestEntries without version: %v", err) + } + if got := keysFor(t, entries, "plugin.json"); !equalStrings(got, []string{"name"}) { + t.Fatalf("keys without version = %v, want [name]", got) + } + + with := writeManifests(t, `{"name":"abcd","version":"0.4.0"}`, `{"name":"m"}`) + entries, err = ManifestEntries(with) + if err != nil { + t.Fatalf("ManifestEntries with version: %v", err) + } + if got := keysFor(t, entries, "plugin.json"); !equalStrings(got, []string{"name", "version"}) { + t.Fatalf("keys with version = %v, want [name version]", got) + } +} + +// TestManifestEntriesRefusesUnreadableManifests keeps the snapshot fail-closed. A +// missing or malformed manifest must be an error: reporting it as "no entries" +// would make every manifest removal look like a surface that was never declared. +func TestManifestEntriesRefusesUnreadableManifests(t *testing.T) { + tests := []struct { + name string + plugin string + marketplace string + want string + }{ + {"plugin missing", "", `{"name":"m"}`, "plugin.json"}, + {"marketplace missing", `{"name":"p"}`, "", "marketplace.json"}, + {"plugin malformed", `{`, `{"name":"m"}`, "plugin.json"}, + {"plugin not an object", `["a"]`, `{"name":"m"}`, "plugin.json"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + root := writeManifests(t, tc.plugin, tc.marketplace) + if _, err := ManifestEntries(root); err == nil { + t.Fatalf("ManifestEntries = nil error, want one naming %s", tc.want) + } else if !strings.Contains(err.Error(), tc.want) { + t.Fatalf("error = %q, want it to name %s", err, tc.want) + } + }) + } +} + +// TestManifestEntriesUsesRepoRelativePaths keeps the artefact machine-independent +// and privacy-safe: an absolute path from the machine that generated it would +// both leak a local path into a committed file and make the drift test fail on +// every other machine. +func TestManifestEntriesUsesRepoRelativePaths(t *testing.T) { + root := writeManifests(t, `{"name":"p"}`, `{"name":"m"}`) + entries, err := ManifestEntries(root) + if err != nil { + t.Fatalf("ManifestEntries: %v", err) + } + for _, e := range entries { + if !strings.HasPrefix(e.File, ".claude-plugin/") { + t.Fatalf("entry file %q is not repo-relative", e.File) + } + } +} diff --git a/internal/core/surface/snapshot.go b/internal/core/surface/snapshot.go new file mode 100644 index 0000000..b19364a --- /dev/null +++ b/internal/core/surface/snapshot.go @@ -0,0 +1,213 @@ +// Package surface models abcd's public compatibility surface — the commands, +// flags, and manifest entries a consumer can bind to — as a transport-agnostic +// value that is snapshotted, committed, and diffed between releases (itd-73, +// spc-10). The snapshot is the artefact the release guardrail compares: a +// removed command, a removed flag, a flag that becomes required, or a removed +// manifest entry is a structural break that a release must declare. +// +// It lives under internal/core, not beside the CLI front door, because the +// snapshot is DATA and the guardrail that diffs two of them is domain logic: +// neither may depend on cobra. The walk that reads the live command tree needs +// cobra, so it lives in internal/surface/cli and hands its result in here. The +// dependency never points the other way. The package name shares a word with the +// internal/surface/* front-door tier and nothing else: that tier is about +// transports, this package is about what those transports expose. +// +// Determinism is this package's contract, not a convenience. The snapshot is a +// committed file gated by a drift test, so the same tree must encode to the same +// bytes on every machine and every run. Every collection is sorted by a stable +// key, nothing is derived from map iteration order, and nothing reads the clock +// or the environment. +package surface + +import ( + "bytes" + "encoding/json" + "fmt" + "sort" +) + +// SchemaVersion is the version of the snapshot's on-disk shape. It is written +// into every encoded snapshot and checked on decode, so a guardrail can never +// silently compare two snapshots written to different shapes — a mismatch there +// would report phantom breaks or, worse, miss real ones. +const SchemaVersion = 1 + +// SnapshotPath is where the committed snapshot lives, repo-relative and +// slash-separated. +// +// It sits with the type rather than with the front door that generates the file, +// because the release guardrail reads the baseline out of a git TREE (`ls-tree`/ +// `cat-file` take slash-separated repo-relative paths) and must name the same +// location the generator writes and the drift test gates. One constant is what +// keeps a guardrail from silently reading a path nothing writes and concluding +// there is no baseline. +const SnapshotPath = ".abcd/development/release/surface.json" + +// Snapshot is the whole compatibility surface at one commit. +// +// The field order is the JSON key order: encoding/json emits struct fields in +// declaration order, so the shape on disk is fixed by this declaration and by +// nothing else. Maps are deliberately absent from the whole type — a map would +// make the encoded bytes depend on iteration order. +type Snapshot struct { + SchemaVersion int `json:"schema_version"` + Commands []Command `json:"commands"` + Manifest []ManifestEntry `json:"manifest"` +} + +// Command is one command in the tree, identified by its full command path +// ("abcd intent plan") because that path is what a caller types and therefore +// what a rename breaks. +// +// Hidden commands are included. The operator-internal `hook` subtree is hidden +// from the documentation page but is still public surface for compatibility +// purposes: harness wiring invokes it by name, so removing or renaming it breaks +// installations even though no user ever reads about it. Recording Hidden lets +// the guardrail report what kind of surface changed without letting it skip any. +// +// Hidden is what the command DECLARES, not whether it is reachable in help: a +// visible subcommand of a hidden parent records Hidden=false, because that is +// what the tree says and the hiding is the help renderer's doing. The field is +// descriptive; presence in the snapshot is what decides a break. +type Command struct { + Path string `json:"path"` + Hidden bool `json:"hidden"` + Flags []Flag `json:"flags"` +} + +// Flag is one flag declared ON a command — its own flags plus the persistent +// flags it declares, never the persistent flags it inherits. A persistent flag +// is therefore recorded exactly once, on the command that declares it, and its +// removal there is one break rather than one per descendant. +type Flag struct { + Name string `json:"name"` + Shorthand string `json:"shorthand"` + // Type is the flag's value type as the flag library names it ("bool", + // "string", "stringSlice"). A type change is a compatibility event in its own + // right: `--since` going from string to stringSlice changes what callers may + // pass. + Type string `json:"type"` + // Required records whether the flag must be supplied. Nothing in the tree + // marks a flag required today, so every entry is false; the field exists so + // that a flag LATER becoming required is diffed as the break it is, rather + // than being invisible because the snapshot never modelled requiredness. + Required bool `json:"required"` + // Hidden mirrors Command.Hidden: an undocumented flag is still a flag a + // script may pass. + Hidden bool `json:"hidden"` +} + +// ManifestEntry is one declared key path in one plugin manifest — the unit the +// guardrail counts as "a manifest surface entry". +// +// An entry is a PRESENCE, not a value. Key paths are leaf paths through the +// manifest JSON (see ManifestEntries for the flattening rules) and the value is +// deliberately not recorded, because the break taxonomy makes a removed entry a +// break while a changed description or a reordered list is not. Recording values +// would make every prose edit to a manifest look like a surface change and would +// bury the removals the guardrail exists to catch. +type ManifestEntry struct { + File string `json:"file"` + Key string `json:"key"` +} + +// NewSnapshot is the one constructor: it stamps the schema version and returns +// the canonical form of the surface it is given. +// +// Callers hand it whatever order their walk produced — cobra's registration +// order, a directory read, a map range — and canonicalisation here is what makes +// two runs over the same tree byte-identical. The input slices are copied, so a +// caller that keeps mutating its working slices cannot reorder a snapshot after +// the fact. +func NewSnapshot(commands []Command, entries []ManifestEntry) Snapshot { + return canonical(Snapshot{SchemaVersion: SchemaVersion, Commands: commands, Manifest: entries}) +} + +// canonical returns s with every collection copied, sorted by a stable key, and +// nil collections normalised to empty ones. +// +// It is the single definition of "canonical" that both the constructor and the +// encoder use, so there is exactly one place where the ordering of the committed +// artefact is decided. Sort keys are the identity fields: a command's path and a +// flag's name are unique within their scope, and a manifest entry is identified +// by its file and key together. +func canonical(s Snapshot) Snapshot { + out := Snapshot{ + SchemaVersion: s.SchemaVersion, + Commands: make([]Command, len(s.Commands)), + Manifest: make([]ManifestEntry, len(s.Manifest)), + } + copy(out.Commands, s.Commands) + copy(out.Manifest, s.Manifest) + + for i := range out.Commands { + flags := make([]Flag, len(out.Commands[i].Flags)) + copy(flags, out.Commands[i].Flags) + sort.Slice(flags, func(a, b int) bool { return flags[a].Name < flags[b].Name }) + out.Commands[i].Flags = flags + } + sort.Slice(out.Commands, func(a, b int) bool { return out.Commands[a].Path < out.Commands[b].Path }) + sortEntries(out.Manifest) + return out +} + +// sortEntries orders manifest entries by file then key — the pair that identifies +// an entry. It is shared with ManifestEntries so the extractor's own output and +// the canonical form agree on one order, rather than the extractor emitting an +// array-positional order that only the constructor happens to repair. +func sortEntries(entries []ManifestEntry) { + sort.Slice(entries, func(a, b int) bool { + if entries[a].File != entries[b].File { + return entries[a].File < entries[b].File + } + return entries[a].Key < entries[b].Key + }) +} + +// Encode renders the snapshot as the committed artefact: canonical order, fixed +// key order, two-space indent, and exactly one trailing newline. +// +// It canonicalises defensively rather than trusting the caller to have gone +// through NewSnapshot, because Encode is the boundary that writes a file a drift +// test then gates — a snapshot that encodes differently depending on how it was +// built would turn that gate into a coin flip. HTML escaping is off so a key +// containing `<`, `>`, or `&` is written literally instead of as an escape that +// a human reader would have to decode. +func Encode(s Snapshot) ([]byte, error) { + var buf bytes.Buffer + enc := json.NewEncoder(&buf) + enc.SetEscapeHTML(false) + enc.SetIndent("", " ") + if err := enc.Encode(canonical(s)); err != nil { + return nil, fmt.Errorf("encoding surface snapshot: %w", err) + } + return buf.Bytes(), nil +} + +// Decode reads a committed snapshot back, so a guardrail can diff the released +// baseline against the current tree. +// +// It is strict in every direction. Unknown fields are rejected because a field +// this binary does not understand means the file describes surface it cannot +// compare, and a schema version other than the one this binary writes is +// rejected for the same reason. Trailing content is rejected because the decoder +// is a STREAM decoder: it stops at the first value and would otherwise accept a +// blob that is a snapshot followed by anything at all. All three are fail-closed +// on purpose: a baseline that parses "successfully" into a partial or empty +// surface would make every removal look like a surface that never existed. +func Decode(data []byte) (Snapshot, error) { + dec := json.NewDecoder(bytes.NewReader(data)) + dec.DisallowUnknownFields() + var s Snapshot + if err := dec.Decode(&s); err != nil { + return Snapshot{}, fmt.Errorf("decoding surface snapshot: %w", err) + } + if dec.More() { + return Snapshot{}, fmt.Errorf("decoding surface snapshot: trailing content after the snapshot") + } + if s.SchemaVersion != SchemaVersion { + return Snapshot{}, fmt.Errorf("surface snapshot schema version %d, want %d", s.SchemaVersion, SchemaVersion) + } + return canonical(s), nil +} diff --git a/internal/core/surface/snapshot_test.go b/internal/core/surface/snapshot_test.go new file mode 100644 index 0000000..a07ba41 --- /dev/null +++ b/internal/core/surface/snapshot_test.go @@ -0,0 +1,259 @@ +package surface + +import ( + "strings" + "testing" +) + +// TestNewSnapshotSortsEveryCollection is the determinism detector for the +// constructor: callers hand it whatever order their walk produced (cobra's +// registration order, a map range), and the snapshot must come back in one +// canonical order regardless. +func TestNewSnapshotSortsEveryCollection(t *testing.T) { + snap := NewSnapshot( + []Command{ + {Path: "abcd intent", Flags: []Flag{{Name: "json"}, {Name: "all"}}}, + {Path: "abcd", Flags: nil}, + {Path: "abcd hook", Hidden: true}, + }, + []ManifestEntry{ + {File: "b.json", Key: "name"}, + {File: "a.json", Key: "zeta"}, + {File: "a.json", Key: "alpha"}, + }, + ) + + if snap.SchemaVersion != SchemaVersion { + t.Fatalf("schema version = %d, want %d", snap.SchemaVersion, SchemaVersion) + } + wantPaths := []string{"abcd", "abcd hook", "abcd intent"} + if got := commandPaths(snap); !equalStrings(got, wantPaths) { + t.Fatalf("command order = %v, want %v", got, wantPaths) + } + wantFlags := []string{"all", "json"} + var gotFlags []string + for _, f := range snap.Commands[2].Flags { + gotFlags = append(gotFlags, f.Name) + } + if !equalStrings(gotFlags, wantFlags) { + t.Fatalf("flag order = %v, want %v", gotFlags, wantFlags) + } + wantEntries := []string{"a.json:alpha", "a.json:zeta", "b.json:name"} + var gotEntries []string + for _, e := range snap.Manifest { + gotEntries = append(gotEntries, e.File+":"+e.Key) + } + if !equalStrings(gotEntries, wantEntries) { + t.Fatalf("manifest order = %v, want %v", gotEntries, wantEntries) + } +} + +// TestNewSnapshotCopiesInput proves the constructor does not alias the caller's +// slices: a snapshot that a later mutation can reorder is not a snapshot. +func TestNewSnapshotCopiesInput(t *testing.T) { + cmds := []Command{{Path: "abcd b"}, {Path: "abcd a"}} + entries := []ManifestEntry{{File: "b.json"}, {File: "a.json"}} + snap := NewSnapshot(cmds, entries) + + cmds[0].Path = "mutated" + entries[0].File = "mutated" + + if snap.Commands[1].Path != "abcd b" { + t.Fatalf("commands aliased the caller's slice: %v", commandPaths(snap)) + } + if snap.Manifest[1].File != "b.json" { + t.Fatalf("manifest aliased the caller's slice: %+v", snap.Manifest) + } +} + +// TestEncodeShape pins the JSON contract the guardrail diffs: fixed key order, +// two-space indent, empty collections as [] rather than null, and exactly one +// trailing newline. Downstream stages read this shape, so a change here is a +// change to a published artefact. +func TestEncodeShape(t *testing.T) { + snap := NewSnapshot( + []Command{ + {Path: "abcd", Flags: []Flag{{Name: "json", Type: "bool"}}}, + {Path: "abcd hook", Hidden: true}, + }, + []ManifestEntry{{File: ".claude-plugin/plugin.json", Key: "name"}}, + ) + + got, err := Encode(snap) + if err != nil { + t.Fatalf("Encode: %v", err) + } + want := `{ + "schema_version": 1, + "commands": [ + { + "path": "abcd", + "hidden": false, + "flags": [ + { + "name": "json", + "shorthand": "", + "type": "bool", + "required": false, + "hidden": false + } + ] + }, + { + "path": "abcd hook", + "hidden": true, + "flags": [] + } + ], + "manifest": [ + { + "file": ".claude-plugin/plugin.json", + "key": "name" + } + ] +} +` + if string(got) != want { + t.Fatalf("Encode shape mismatch:\ngot:\n%s\nwant:\n%s", got, want) + } + if !strings.HasSuffix(string(got), "}\n") || strings.HasSuffix(string(got), "}\n\n") { + t.Fatalf("Encode must end in exactly one trailing newline, got %q", tail(string(got))) + } +} + +// TestEncodeCanonicalisesUnsortedInput is the second determinism detector: Encode +// is the boundary that writes the committed artefact, so it must not depend on +// the caller having gone through NewSnapshot. +func TestEncodeCanonicalisesUnsortedInput(t *testing.T) { + unsorted := Snapshot{ + SchemaVersion: SchemaVersion, + Commands: []Command{{Path: "abcd zeta"}, {Path: "abcd alpha"}}, + Manifest: []ManifestEntry{{File: "b.json", Key: "k"}, {File: "a.json", Key: "k"}}, + } + sorted := NewSnapshot(unsorted.Commands, unsorted.Manifest) + + fromUnsorted, err := Encode(unsorted) + if err != nil { + t.Fatalf("Encode(unsorted): %v", err) + } + fromSorted, err := Encode(sorted) + if err != nil { + t.Fatalf("Encode(sorted): %v", err) + } + if string(fromUnsorted) != string(fromSorted) { + t.Fatalf("Encode is order-sensitive:\nunsorted:\n%s\nsorted:\n%s", fromUnsorted, fromSorted) + } +} + +// TestEncodeIsRepeatable runs the encoder many times over one snapshot: any +// map-ranging or other non-determinism inside the encoder shows up as a diff. +func TestEncodeIsRepeatable(t *testing.T) { + snap := NewSnapshot( + []Command{{Path: "abcd", Flags: []Flag{{Name: "json"}, {Name: "all"}, {Name: "dry-run"}}}}, + []ManifestEntry{{File: "a.json", Key: "x"}, {File: "a.json", Key: "y"}}, + ) + first, err := Encode(snap) + if err != nil { + t.Fatalf("Encode: %v", err) + } + for i := 0; i < 50; i++ { + again, err := Encode(snap) + if err != nil { + t.Fatalf("Encode iteration %d: %v", i, err) + } + if string(again) != string(first) { + t.Fatalf("Encode is not deterministic at iteration %d", i) + } + } +} + +// TestDecodeRoundTrip proves the committed artefact reads back as the value the +// guardrail diffs — without a decoder the JSON shape would be write-only. +func TestDecodeRoundTrip(t *testing.T) { + snap := NewSnapshot( + []Command{{Path: "abcd hook", Hidden: true, Flags: []Flag{{Name: "since", Shorthand: "s", Type: "string", Required: true}}}}, + []ManifestEntry{{File: ".claude-plugin/plugin.json", Key: "author.name"}}, + ) + data, err := Encode(snap) + if err != nil { + t.Fatalf("Encode: %v", err) + } + back, err := Decode(data) + if err != nil { + t.Fatalf("Decode: %v", err) + } + again, err := Encode(back) + if err != nil { + t.Fatalf("Encode(back): %v", err) + } + if string(again) != string(data) { + t.Fatalf("round trip lost data:\ngot:\n%s\nwant:\n%s", again, data) + } +} + +// TestDecodeRefusesBadInput keeps the guardrail fail-closed: a baseline it cannot +// read faithfully must be an error, never a silently empty surface that would +// make every removal invisible. +func TestDecodeRefusesBadInput(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"malformed", `{`, "decoding"}, + {"unknown field", `{"schema_version":1,"commands":[],"manifest":[],"extra":1}`, "extra"}, + {"future schema", `{"schema_version":99,"commands":[],"manifest":[]}`, "schema version"}, + {"missing schema", `{"commands":[],"manifest":[]}`, "schema version"}, + { + // A stream decoder stops at the first value, so anything after the + // snapshot would be accepted silently. The artefact is exactly one + // JSON document; bytes beyond it mean the blob is not the artefact. + "trailing content", + `{"schema_version":1,"commands":[],"manifest":[]}` + "\n{\"schema_version\":1}\n", + "trailing", + }, + { + "trailing garbage", + `{"schema_version":1,"commands":[],"manifest":[]}` + "\n<<>>\n", + "trailing", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := Decode([]byte(tc.in)) + if err == nil { + t.Fatalf("Decode(%q) = nil error, want one", tc.in) + } + if !strings.Contains(err.Error(), tc.want) { + t.Fatalf("Decode(%q) error = %q, want it to mention %q", tc.in, err, tc.want) + } + }) + } +} + +func commandPaths(s Snapshot) []string { + var out []string + for _, c := range s.Commands { + out = append(out, c.Path) + } + return out +} + +func equalStrings(got, want []string) bool { + if len(got) != len(want) { + return false + } + for i := range got { + if got[i] != want[i] { + return false + } + } + return true +} + +func tail(s string) string { + if len(s) < 8 { + return s + } + return s[len(s)-8:] +} diff --git a/internal/fsutil/moduleroot_test.go b/internal/fsutil/moduleroot_test.go new file mode 100644 index 0000000..d7c8eb8 --- /dev/null +++ b/internal/fsutil/moduleroot_test.go @@ -0,0 +1,56 @@ +package fsutil + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestModuleRootWalksUpToGoMod covers the three cases a generator meets: invoked +// from the module root, invoked from a nested package directory (what `go +// generate` does), and invoked from outside any module. +func TestModuleRootWalksUpToGoMod(t *testing.T) { + root := t.TempDir() + // t.TempDir can hand back a symlinked path (/var vs /private/var on macOS); + // resolve it so the comparison is about the walk, not about link spelling. + resolved, err := filepath.EvalSymlinks(root) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + if err := os.WriteFile(filepath.Join(resolved, "go.mod"), []byte("module example.invalid\n"), 0o644); err != nil { + t.Fatalf("write go.mod: %v", err) + } + nested := filepath.Join(resolved, "internal", "surface", "cli") + if err := os.MkdirAll(nested, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + + tests := []struct { + name string + start string + }{ + {"from the module root", resolved}, + {"from a nested package directory", nested}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := ModuleRoot(tc.start) + if err != nil { + t.Fatalf("ModuleRoot(%s): %v", tc.start, err) + } + if got != resolved { + t.Fatalf("ModuleRoot(%s) = %q, want %q", tc.start, got, resolved) + } + }) + } + + t.Run("outside any module", func(t *testing.T) { + outside := t.TempDir() + if _, err := ModuleRoot(outside); err == nil { + t.Fatalf("ModuleRoot(%s) = nil error, want one", outside) + } else if !strings.Contains(err.Error(), "go.mod") { + t.Fatalf("error = %q, want it to name go.mod", err) + } + }) +} diff --git a/internal/fsutil/paths.go b/internal/fsutil/paths.go index 4589aed..57223a4 100644 --- a/internal/fsutil/paths.go +++ b/internal/fsutil/paths.go @@ -2,6 +2,7 @@ package fsutil import ( "errors" + "fmt" "io" "os" "path/filepath" @@ -102,3 +103,32 @@ func DirHasEntries(path string) (bool, error) { } return len(names) > 0, nil } + +// ModuleRoot walks up from start until it finds the directory holding go.mod — +// the module root. +// +// It exists for the repo's code generators, which must write to the same file +// whether they are invoked by `go generate` (the working directory is the package +// being generated) or run directly from the repo root. Anchoring on go.mod rather +// than on .git means a generator also works inside a worktree or an export where +// the git directory is not where the walk expects it. +// +// A start outside any module is an error rather than a fallback to the working +// directory: a generator that silently wrote its artefact into an unrelated +// directory would be worse than one that refuses. +func ModuleRoot(start string) (string, error) { + dir, err := filepath.Abs(start) + if err != nil { + return "", err + } + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir, nil + } + parent := filepath.Dir(dir) + if parent == dir { + return "", fmt.Errorf("fsutil: go.mod not found at or above %s", start) + } + dir = parent + } +} diff --git a/internal/surface/cli/guard_test.go b/internal/surface/cli/guard_test.go new file mode 100644 index 0000000..26b9aa2 --- /dev/null +++ b/internal/surface/cli/guard_test.go @@ -0,0 +1,265 @@ +package cli + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/REPPL/abcd-cli/internal/core/changelog" + "github.com/REPPL/abcd-cli/internal/core/surface" + "github.com/REPPL/abcd-cli/internal/gittest" + "github.com/spf13/cobra" +) + +// guardFixture is a hermetic throwaway repository for the end-to-end guardrail +// test: the front door walks the LIVE cobra tree and reads the LIVE manifests +// under a root, so the only honest exercise of it is a real repo with real +// manifests, a real tag, and a real snapshot blob in that tag's tree. +// +// internal/core/changelog's fixture_test.go carries an equivalent unexported +// helper. This is the second copy, kept because promoting it would mean +// rewriting that package's tests; a third caller should consolidate the two into +// an exported helper in internal/gittest rather than adding another. +type guardFixture struct { + t *testing.T + root string + env []string +} + +func newGuardFixture(t *testing.T) *guardFixture { + t.Helper() + f := &guardFixture{t: t, root: t.TempDir(), env: gittest.Env(t)} + init := exec.Command("git", "-C", f.root, "init", "--initial-branch=main") + init.Env = f.env + if out, err := init.CombinedOutput(); err != nil { + t.Skipf("git init unavailable: %v (%s)", err, out) + } + return f +} + +func (f *guardFixture) git(args ...string) { + f.t.Helper() + full := append([]string{ + "-C", f.root, + "-c", "user.email=fixture@example.invalid", + "-c", "user.name=Fixture", + "-c", "commit.gpgsign=false", + }, args...) + cmd := exec.Command("git", full...) + cmd.Env = f.env + if out, err := cmd.CombinedOutput(); err != nil { + f.t.Fatalf("git %v: %v\n%s", args, err, out) + } +} + +func (f *guardFixture) write(rel, content string) { + f.t.Helper() + path := filepath.Join(f.root, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + f.t.Fatal(err) + } + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + f.t.Fatal(err) + } +} + +func (f *guardFixture) commit(msg string) { + f.t.Helper() + f.git("add", "-A") + f.git("commit", "--allow-empty", "-m", msg) +} + +// writeManifests plants the two plugin manifests the surface walk reads, so the +// fixture's current snapshot is built from real files rather than a stub. +func (f *guardFixture) writeManifests() { + f.t.Helper() + f.write(".claude-plugin/plugin.json", `{"name":"abcd","description":"fixture"}`+"\n") + f.write(".claude-plugin/marketplace.json", `{"name":"abcd","plugins":[{"name":"abcd","source":"./"}]}`+"\n") +} + +// TestGuardSurfaceEndToEnd exercises the whole wired path: the front door walks +// the live cobra tree, reads the live manifests, and hands the result to the +// core guardrail, which reads its baseline out of the release tag. +// +// The baseline planted in the tag is the live surface PLUS a command that does +// not exist ("abcd ghost"), so from the tag's point of view the release removed +// it. The snapshot committed at HEAD is the live surface — exactly what the +// drift test enforces in the real repo — which means a guardrail comparing the +// committed file against the current tree would see nothing at all. Detecting +// the removal proves the baseline came from the tag. +func TestGuardSurfaceEndToEnd(t *testing.T) { + f := newGuardFixture(t) + f.writeManifests() + + live, err := SurfaceSnapshot(f.root) + if err != nil { + t.Fatalf("SurfaceSnapshot: %v", err) + } + tagged := surface.NewSnapshot( + append(append([]surface.Command{}, live.Commands...), surface.Command{Path: "abcd ghost"}), + live.Manifest) + + writeEncoded(t, f, tagged) + f.commit("seed the surface baseline") + f.git("tag", "v0.4.0") + + writeEncoded(t, f, live) + f.write(".abcd/development/intents/shipped/itd-1-thing.md", "---\nid: itd-1\nimpact: additive\n---\n# itd-1\n") + f.commit("ship an additive intent and regenerate the snapshot") + + got, err := GuardSurface(f.root) + if err != nil { + t.Fatalf("GuardSurface: %v", err) + } + if got.Status != changelog.SurfaceGuardFailed { + t.Fatalf("Status = %q (reason %q), want %q", got.Status, got.Reason, changelog.SurfaceGuardFailed) + } + if !strings.Contains(got.Reason, "abcd ghost") { + t.Errorf("Reason = %q, want it to name the removed command", got.Reason) + } + if got.BaseTag != "v0.4.0" { + t.Errorf("BaseTag = %q, want v0.4.0", got.BaseTag) + } +} + +// TestGuardSurfaceEndToEndPassesUnchangedSurface is the other direction of the +// same wiring: a tag whose baseline IS the live surface reports a clean pass, so +// the failing case above is the guardrail firing rather than the front door +// being broken. +func TestGuardSurfaceEndToEndPassesUnchangedSurface(t *testing.T) { + f := newGuardFixture(t) + f.writeManifests() + + live, err := SurfaceSnapshot(f.root) + if err != nil { + t.Fatalf("SurfaceSnapshot: %v", err) + } + writeEncoded(t, f, live) + f.commit("seed the surface baseline") + f.git("tag", "v0.4.0") + + f.write(".abcd/development/intents/shipped/itd-1-thing.md", "---\nid: itd-1\nimpact: additive\n---\n# itd-1\n") + f.commit("ship an additive intent") + + got, err := GuardSurface(f.root) + if err != nil { + t.Fatalf("GuardSurface: %v", err) + } + if got.Status != changelog.SurfaceGuardPassed { + t.Fatalf("Status = %q (reason %q), want %q", got.Status, got.Reason, changelog.SurfaceGuardPassed) + } + if len(got.Breaks) != 0 { + t.Errorf("Breaks = %v, want none", got.Breaks) + } +} + +// TestGuardSurfaceEndToEndRefusesWithoutBaseline pins the fail-closed refusal +// through the front door — the repository's real state today, because v0.3.0 +// predates the snapshot. The message has to be actionable, so it names the tag, +// the missing artefact, and the manual roll that resolves it. +func TestGuardSurfaceEndToEndRefusesWithoutBaseline(t *testing.T) { + f := newGuardFixture(t) + f.writeManifests() + f.commit("a release that predates the surface baseline") + f.git("tag", "v0.3.0") + + live, err := SurfaceSnapshot(f.root) + if err != nil { + t.Fatalf("SurfaceSnapshot: %v", err) + } + writeEncoded(t, f, live) + f.commit("seed the surface baseline after the release") + + got, err := GuardSurface(f.root) + if err != nil { + t.Fatalf("GuardSurface: %v", err) + } + if got.Status != changelog.SurfaceGuardRefused { + t.Fatalf("Status = %q, want %q: the first cut must not sail through unguarded", got.Status, changelog.SurfaceGuardRefused) + } + for _, want := range []string{"v0.3.0", SurfaceSnapshotPath, "manual roll"} { + if !strings.Contains(got.Reason, want) { + t.Errorf("Reason = %q, want it to mention %q", got.Reason, want) + } + } +} + +// TestGuardSurfaceEndToEndRefusesStaleBinary pins the fail-closed rule that +// covers this front door's one structural weakness: the current surface is walked +// from the command tree COMPILED INTO the running binary, while every other input +// is read out of the repository at repoRoot. An installed release binary +// therefore reports the surface of the release it was built from, and a command +// removed since that build would be missing from both sides of the diff — a real +// break shipping as additive, invisibly. +// +// The fixture is exactly that shape: HEAD's committed snapshot has a command +// dropped, so it disagrees with the tree this binary walks. There is no way to +// tell "stale binary" from "stale snapshot" apart from here, so the guardrail +// refuses and the message names both remedies. +func TestGuardSurfaceEndToEndRefusesStaleBinary(t *testing.T) { + f := newGuardFixture(t) + f.writeManifests() + + live, err := SurfaceSnapshot(f.root) + if err != nil { + t.Fatalf("SurfaceSnapshot: %v", err) + } + if len(live.Commands) < 2 { + t.Fatalf("live surface has %d commands, want a tree to drop one from", len(live.Commands)) + } + writeEncoded(t, f, live) + f.commit("seed the surface baseline") + f.git("tag", "v0.4.0") + + // The tree being released removed a command and regenerated the snapshot; + // the binary doing the walking predates that removal. + writeEncoded(t, f, surface.NewSnapshot(live.Commands[1:], live.Manifest)) + f.write(".abcd/development/intents/shipped/itd-1-thing.md", "---\nid: itd-1\nimpact: additive\n---\n# itd-1\n") + f.commit("remove a command and regenerate the snapshot") + + got, err := GuardSurface(f.root) + if err != nil { + t.Fatalf("GuardSurface: %v", err) + } + if got.Status != changelog.SurfaceGuardRefused { + t.Fatalf("Status = %q (reason %q), want %q: a binary that disagrees with the tree cannot guard it", + got.Status, got.Reason, changelog.SurfaceGuardRefused) + } + for _, want := range []string{SurfaceSnapshotPath, "rebuild", "regenerate"} { + if !strings.Contains(got.Reason, want) { + t.Errorf("Reason = %q, want it to mention %q", got.Reason, want) + } + } +} + +// TestHelpTextIsNotSurface pins the taxonomy row the snapshot answers by not +// modelling it: changed help, description, and summary text are not breaks. Two +// trees identical but for their prose must produce identical surface and diff +// clean — which is why the snapshot carries no prose at all. +func TestHelpTextIsNotSurface(t *testing.T) { + build := func(short, long, example string) surface.Snapshot { + root := &cobra.Command{Use: "abcd", Short: short, Long: long} + child := &cobra.Command{Use: "plan", Short: short, Long: long, Example: example} + child.Flags().String("since", "", short) + root.AddCommand(child) + return surface.NewSnapshot(commandSurface(root), nil) + } + + before := build("does a thing", "the long version", "abcd plan") + after := build("REWORDED entirely", "a completely different long description", "abcd plan --since v1") + + if breaks := surface.Diff(before, after); len(breaks) != 0 { + t.Errorf("Diff = %v, want none: reworded help is not a compatibility break", breaks) + } +} + +func writeEncoded(t *testing.T, f *guardFixture, snap surface.Snapshot) { + t.Helper() + data, err := surface.Encode(snap) + if err != nil { + t.Fatalf("Encode: %v", err) + } + f.write(SurfaceSnapshotPath, string(data)) +} diff --git a/internal/surface/cli/surface.go b/internal/surface/cli/surface.go new file mode 100644 index 0000000..7d92ca2 --- /dev/null +++ b/internal/surface/cli/surface.go @@ -0,0 +1,126 @@ +package cli + +//go:generate go run ../../../cmd/abcd-gen-surface + +import ( + "github.com/REPPL/abcd-cli/internal/core/changelog" + "github.com/REPPL/abcd-cli/internal/core/surface" + "github.com/spf13/cobra" + "github.com/spf13/pflag" +) + +// SurfaceSnapshotPath is the committed compatibility snapshot, relative to the +// repo root. The generator writes it, the drift test (surface_test.go) diffs the +// freshly-walked surface against it, and the release guardrail reads the +// baseline copy of it out of the last release tag — so all three agree on one +// location, declared once beside the type in internal/core/surface. It sits +// under the development record rather than under docs/ because it is a +// release-gate input, not something a reader of the documentation consumes. +const SurfaceSnapshotPath = surface.SnapshotPath + +// SurfaceSnapshot builds the current compatibility surface: every command in the +// tree with its flags, plus the declared entries of the two plugin manifests +// under repoRoot. +// +// It walks the shared NewRootCommand() tree — the one canonical root command, +// the same one the CLI executes — rather than the Markdown reference walker. +// GenerateReference emits prose, returns early on hidden commands, and carries no +// structured requiredness or manifest data; reusing it would bake those blind +// spots into a compatibility gate. What is shared is the tree, which is the part +// that must not diverge. +// +// The tree is built fresh here and never executed. Cobra lazily attaches its +// default `help` and `completion` machinery during execution, so snapshotting an +// executed tree would record surface that a freshly-built one does not have, and +// the answer would depend on what else ran first in the process. +func SurfaceSnapshot(repoRoot string) (surface.Snapshot, error) { + entries, err := surface.ManifestEntries(repoRoot) + if err != nil { + return surface.Snapshot{}, err + } + return surface.NewSnapshot(commandSurface(NewRootCommand()), entries), nil +} + +// GenerateSurface renders the current surface as the committed artefact's bytes. +// Both the generator and the drift test call it, so the file that is written and +// the file that is checked are produced by one code path and can never disagree +// on formatting. +func GenerateSurface(repoRoot string) ([]byte, error) { + snap, err := SurfaceSnapshot(repoRoot) + if err != nil { + return nil, err + } + return surface.Encode(snap) +} + +// GuardSurface runs the release surface guardrail for the repository at +// repoRoot: it builds the CURRENT compatibility surface from the live command +// tree and the live manifests, and hands it to the core guardrail, which reads +// the baseline out of the last release tag and answers whether a narrowing was +// declared. +// +// This is the split the architecture requires. Building the current surface +// means walking cobra, which internal/core may not do; judging a break is domain +// logic, which must not depend on a transport. The front door therefore owns the +// walk and core owns the verdict, and the dependency points one way only. +// +// It is the entry point the ship flow calls. There is deliberately no `launch +// ship` verb yet — the write path is a later phase — so today this is reached +// from tests and from whatever front door composes the cut next; the guardrail +// itself is complete and does not change when that verb arrives. +func GuardSurface(repoRoot string) (changelog.SurfaceGuard, error) { + current, err := SurfaceSnapshot(repoRoot) + if err != nil { + return changelog.SurfaceGuard{}, err + } + return changelog.GuardSurface(repoRoot, current) +} + +// commandSurface flattens a command tree into snapshot entries, depth-first from +// cmd. +// +// Hidden commands are recorded, not skipped: the operator-internal `hook` subtree +// is invoked by name from harness wiring, so removing it breaks installations +// even though the documentation never mentions it. Ordering is left to +// surface.NewSnapshot, which sorts by command path — relying on cobra's traversal +// order would make the artefact depend on registration order. +func commandSurface(cmd *cobra.Command) []surface.Command { + out := []surface.Command{{ + Path: cmd.CommandPath(), + Hidden: cmd.Hidden, + Flags: commandFlags(cmd), + }} + for _, child := range cmd.Commands() { + out = append(out, commandSurface(child)...) + } + return out +} + +// commandFlags reads the flags a command declares itself — its own flags plus the +// persistent flags it defines, which is exactly what cobra's LocalFlags excludes +// inherited persistent flags from. A persistent flag is therefore recorded once, +// where it is declared, so removing `--json` from the root reads as one break +// rather than one per command in the tree. +func commandFlags(cmd *cobra.Command) []surface.Flag { + var out []surface.Flag + cmd.LocalFlags().VisitAll(func(f *pflag.Flag) { + out = append(out, surface.Flag{ + Name: f.Name, + Shorthand: f.Shorthand, + Type: f.Value.Type(), + Required: flagRequired(f), + Hidden: f.Hidden, + }) + }) + return out +} + +// flagRequired reports whether cobra considers the flag mandatory. Cobra records +// requiredness as an annotation set by MarkFlagRequired rather than as a field, +// and it reads that annotation as "present and its first value is true"; this +// mirrors that reading exactly, so the snapshot's answer is the one the runtime +// would give. +func flagRequired(f *pflag.Flag) bool { + values, found := f.Annotations[cobra.BashCompOneRequiredFlag] + return found && len(values) > 0 && values[0] == "true" +} diff --git a/internal/surface/cli/surface_test.go b/internal/surface/cli/surface_test.go new file mode 100644 index 0000000..03cc9c0 --- /dev/null +++ b/internal/surface/cli/surface_test.go @@ -0,0 +1,235 @@ +package cli + +import ( + "os" + "path/filepath" + "testing" + + "github.com/REPPL/abcd-cli/internal/core/surface" + "github.com/spf13/cobra" +) + +// testRepoRoot locates the repo root from this package directory +// (internal/surface/cli), so the snapshot tests read the real manifests and the +// real committed baseline. +func testRepoRoot() string { return filepath.Join("..", "..", "..") } + +func findCommand(snap surface.Snapshot, path string) (surface.Command, bool) { + for _, c := range snap.Commands { + if c.Path == path { + return c, true + } + } + return surface.Command{}, false +} + +func findFlag(cmd surface.Command, name string) (surface.Flag, bool) { + for _, f := range cmd.Flags { + if f.Name == name { + return f, true + } + } + return surface.Flag{}, false +} + +// TestSurfaceSnapshotIncludesHiddenCommands is the reason this walk exists rather +// than reusing the Markdown reference walker: the operator-internal `hook` +// subtree is hidden from the docs page but IS public surface for compatibility +// purposes, because harness wiring invokes it by name. A snapshot that skipped it +// would let a hook removal ship as a non-breaking release. +func TestSurfaceSnapshotIncludesHiddenCommands(t *testing.T) { + snap, err := SurfaceSnapshot(testRepoRoot()) + if err != nil { + t.Fatalf("SurfaceSnapshot: %v", err) + } + + // Hidden is recorded as each command DECLARES it, which is why the subtree + // below a hidden parent reports Hidden=false: cobra hides descendants by + // never rendering the parent, not by marking them. Presence is what the + // guardrail diffs, so both must be present either way. + tests := []struct { + path string + wantHidden bool + }{ + {"abcd", false}, + {"abcd hook", true}, + {"abcd hook prompt-router", false}, + } + for _, tc := range tests { + cmd, ok := findCommand(snap, tc.path) + if !ok { + t.Fatalf("%q missing from the snapshot; the walk is skipping hidden commands", tc.path) + } + if cmd.Hidden != tc.wantHidden { + t.Fatalf("%q hidden = %v, want %v", tc.path, cmd.Hidden, tc.wantHidden) + } + } +} + +// TestSurfaceSnapshotRecordsFlagDetail checks the structured per-flag data the +// Markdown reference cannot carry: a persistent flag is recorded once on the +// command that declares it (not repeated on every descendant that inherits it), +// and its type travels with it. +func TestSurfaceSnapshotRecordsFlagDetail(t *testing.T) { + snap, err := SurfaceSnapshot(testRepoRoot()) + if err != nil { + t.Fatalf("SurfaceSnapshot: %v", err) + } + + root, ok := findCommand(snap, "abcd") + if !ok { + t.Fatalf("root command missing") + } + jsonFlag, ok := findFlag(root, "json") + if !ok { + t.Fatalf("--json missing from the root command's flags: %+v", root.Flags) + } + if jsonFlag.Type != "bool" { + t.Fatalf("--json type = %q, want %q", jsonFlag.Type, "bool") + } + if jsonFlag.Required { + t.Fatalf("--json recorded as required") + } + + version, ok := findCommand(snap, "abcd version") + if !ok { + t.Fatalf("`abcd version` missing") + } + if _, inherited := findFlag(version, "json"); inherited { + t.Fatalf("inherited persistent flag recorded on a subcommand; it must be recorded only where it is declared") + } +} + +// TestCommandSurfaceExtractsRequiredness tests requiredness against a SYNTHETIC +// tree, because nothing in the live tree marks a flag required — asserting only +// against the live tree would be asserting that an always-false field is false. +// A flag becoming required is a break, so the extraction must work the day it +// first happens rather than the day someone notices it never did. +func TestCommandSurfaceExtractsRequiredness(t *testing.T) { + root := &cobra.Command{Use: "synth"} + child := &cobra.Command{Use: "child"} + child.Flags().String("must", "", "a required flag") + child.Flags().StringP("optional", "o", "", "an optional flag") + child.Flags().Bool("secret", false, "a hidden flag") + if err := child.MarkFlagRequired("must"); err != nil { + t.Fatalf("MarkFlagRequired: %v", err) + } + if err := child.Flags().MarkHidden("secret"); err != nil { + t.Fatalf("MarkHidden: %v", err) + } + root.AddCommand(child) + + snap := surface.NewSnapshot(commandSurface(root), nil) + cmd, ok := findCommand(snap, "synth child") + if !ok { + t.Fatalf("synthetic child missing from %+v", snap.Commands) + } + + tests := []struct { + flag string + want surface.Flag + label string + }{ + {"must", surface.Flag{Name: "must", Shorthand: "", Type: "string", Required: true, Hidden: false}, "required flag"}, + {"optional", surface.Flag{Name: "optional", Shorthand: "o", Type: "string", Required: false, Hidden: false}, "optional flag with a shorthand"}, + {"secret", surface.Flag{Name: "secret", Shorthand: "", Type: "bool", Required: false, Hidden: true}, "hidden flag"}, + } + for _, tc := range tests { + t.Run(tc.label, func(t *testing.T) { + got, ok := findFlag(cmd, tc.flag) + if !ok { + t.Fatalf("--%s missing from %+v", tc.flag, cmd.Flags) + } + if got != tc.want { + t.Fatalf("--%s = %+v, want %+v", tc.flag, got, tc.want) + } + }) + } +} + +// TestLiveTreeMarksNoFlagRequired records the state the requiredness field is a +// tripwire for. Nothing marks a flag required today; when something does, this +// test fails and the author is pointed at the fact that the change is a break +// needing a `breaking` record in the cut. +func TestLiveTreeMarksNoFlagRequired(t *testing.T) { + snap, err := SurfaceSnapshot(testRepoRoot()) + if err != nil { + t.Fatalf("SurfaceSnapshot: %v", err) + } + for _, cmd := range snap.Commands { + for _, f := range cmd.Flags { + if f.Required { + t.Fatalf("`%s --%s` is now required: a previously optional flag becoming required is a "+ + "surface break and needs a `breaking` record in the release cut", cmd.Path, f.Name) + } + } + } +} + +// TestGenerateSurfaceIsDeterministic is the phase's stop condition made +// executable: a guardrail built on a wobbling snapshot is worse than none, +// because it either cries wolf every release or is switched off. Many runs in one +// process catch ordering that depends on map iteration, on cobra's registration +// order, or on any per-run state the walk accumulates. +func TestGenerateSurfaceIsDeterministic(t *testing.T) { + root := testRepoRoot() + first, err := GenerateSurface(root) + if err != nil { + t.Fatalf("GenerateSurface: %v", err) + } + const runs = 50 + for i := 1; i < runs; i++ { + again, err := GenerateSurface(root) + if err != nil { + t.Fatalf("GenerateSurface run %d: %v", i, err) + } + if string(again) != string(first) { + t.Fatalf("GenerateSurface is not deterministic: run %d differs from run 0 at %s", + i, firstDiff(string(first), string(again))) + } + } +} + +// TestSurfaceSnapshotMatchesCommittedBaseline is the drift gate for the committed +// snapshot, mirroring the CLI reference's gate. It regenerates the surface from +// the live command tree and the live manifests and diffs it against the committed +// artefact, so a command, flag, or manifest entry can never change without the +// baseline moving with it — which is what makes the release guardrail's diff +// meaningful. +func TestSurfaceSnapshotMatchesCommittedBaseline(t *testing.T) { + root := testRepoRoot() + baseline := filepath.Join(root, filepath.FromSlash(SurfaceSnapshotPath)) + committed, err := os.ReadFile(baseline) + if err != nil { + t.Fatalf("cannot read committed surface snapshot %s: %v\n"+ + "regenerate it with `go generate ./internal/surface/cli`", SurfaceSnapshotPath, err) + } + + want, err := GenerateSurface(root) + if err != nil { + t.Fatalf("GenerateSurface: %v", err) + } + if string(committed) != string(want) { + t.Fatalf("%s is stale: the committed surface no longer matches the command tree and manifests.\n"+ + "Regenerate it with `go generate ./internal/surface/cli` and commit the result.\n"+ + "first difference at %s", SurfaceSnapshotPath, firstDiff(string(committed), string(want))) + } +} + +// TestCommittedBaselineDecodes proves the committed artefact is readable by the +// decoder the release guardrail uses, not merely byte-equal to what the generator +// produced. +func TestCommittedBaselineDecodes(t *testing.T) { + data, err := os.ReadFile(filepath.Join(testRepoRoot(), filepath.FromSlash(SurfaceSnapshotPath))) + if err != nil { + t.Fatalf("reading %s: %v", SurfaceSnapshotPath, err) + } + snap, err := surface.Decode(data) + if err != nil { + t.Fatalf("Decode(%s): %v", SurfaceSnapshotPath, err) + } + if len(snap.Commands) == 0 || len(snap.Manifest) == 0 { + t.Fatalf("committed baseline decodes to an empty surface: %d commands, %d manifest entries", + len(snap.Commands), len(snap.Manifest)) + } +}