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/.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 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)) + } +}