diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9af272b..b886338 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,6 +31,12 @@ jobs: - name: Test run: go test -race ./... + # docs/BEHAVIOR.md is generated from the test suite's doc comments, so + # it can only stay true if every change regenerates it. Adding, renaming + # or re-documenting a test without running `make docs` fails here. + - name: Docs up to date + run: make docs-check + - name: Build run: make build diff --git a/Makefile b/Makefile index 4166c07..4e9d38f 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: build test docs diff-test vet all +.PHONY: build test docs docs-check diff-test vet all all: vet test build docs @@ -16,6 +16,13 @@ vet: docs: go run ./tools/gendocs +# CI gate: the committed docs must be exactly what the generator produces. +# gendocs is deterministic, so a diff here means a test doc comment changed +# (or a test was added/renamed) without `make docs` being run. +docs-check: docs + @git diff --exit-code -- docs/BEHAVIOR.md \ + || { echo "docs/BEHAVIOR.md is stale — run 'make docs' and commit the result."; exit 1; } + # Differential fuzz of the pattern matcher against the vendored, unmodified # hmarr/codeowners oracle (500k cases). diff-test: diff --git a/README.md b/README.md index 32b2322..331fd76 100644 --- a/README.md +++ b/README.md @@ -50,7 +50,7 @@ arm64. > `xattr -d com.apple.quarantine ./codeowners-tool`. Homebrew and the install > script above are unaffected — neither quarantines its downloads. -**From source** with Go 1.24+: +**From source** with Go 1.24.7+ (the version pinned in `go.mod`): ```sh go install github.com/jordonpeterson/codeowners-tool/cmd/codeowners-tool@latest @@ -72,9 +72,11 @@ codeowners-tool apply --plan plan.json ``` `plan` re-resolves ownership across your real git tree and refuses anything it -can't prove; `apply` writes only the proven edit and is idempotent, so -re-running the same op is a no-op. To audit a repo for rot or to verify in CI -that a change stayed inside its declared scope, read on. +can't prove; `apply` writes only the proven edit. Both are idempotent: +re-planning the same op against an already-satisfied file changes nothing and +**exits 1** (`nothing to change`) rather than 0, so a CI step that runs `plan` +unconditionally should treat 1 as success. To audit a repo for rot or to +verify in CI that a change stayed inside its declared scope, read on. ## Quick start @@ -95,6 +97,18 @@ GITHUB_TOKEN=... ./codeowners-tool audit --github-repo org/repo --format json ./codeowners-tool verify --before before.json --after after.json --scope /services/api/ ``` +`verify` compares *resolved ownership per path*, independently of the planner — +CI can check the invariant without trusting the tool that made the change. +Two things to know about the recipe above: + +- **Omitting `--scope` asserts that nothing changed at all.** Scopes are the + allowlist; with none, every difference is a violation. +- **Files the branch adds or deletes are reported, never violations.** The two + snapshots come from different refs, so their trees differ. An added path had + no prior ownership for INV-2 to preserve, so it prints as `added:` and does + not fail the check. A real reassignment still fails it — the subtree's + pre-existing files change. + ## Operations (Engine A — mutation) Scope is a directory, file path, or glob — same syntax as CODEOWNERS patterns. @@ -106,6 +120,80 @@ Scope is a directory, file path, or glob — same syntax as CODEOWNERS patterns. | `remove_owner(scope, owner)` | Owner stops owning every path in scope. If a rule's owner set would empty, `--on-empty` is **required** (see below). | | `rename_owner(old, new)` | Global identifier substitution — the only op safe as pure text replacement (it can't change any rule's match set). | +An owner is `@org/team`, `@username`, or an email address. The `@` is +required — a bare `team_a` is rejected with +`error: invalid owner token "team_a"`. Both `@org/team_a` and `@team_a` parse, +but they mean different things to GitHub (a team vs a user account) and the +tool cannot tell you which you meant. + +### Adding an owner: co-owner, or sole owner? + +The most common task, and the one where hand-editing goes wrong. Given: + +``` +/services/ @org/platform @org/sre +``` + +**Add `@org/team_a`, keeping the existing owners:** + +```sh +codeowners-tool plan --op 'add_owner(/services/, @org/team_a)' --out plan.json +``` + +``` +-/services/ @org/platform @org/sre ++/services/ @org/platform @org/sre @org/team_a + +services/api/main.go : [@org/platform, @org/sre] → [@org/platform, @org/sre, @org/team_a] +``` + +**Make `@org/team_a` the sole owner, displacing the others:** + +```sh +codeowners-tool plan --op 'set_owners(/services/, [@org/team_a])' --out plan.json +``` + +``` +-/services/ @org/platform @org/sre ++/services/ @org/team_a + +services/api/main.go : [@org/platform, @org/sre] → [@org/team_a] +``` + +Review `plan.json`, then `codeowners-tool apply --plan plan.json`. + +**Why not just append a line?** Writing `/services/ @org/team_a` at the end of +the file yourself produces the *second* result when you probably wanted the +first. Last match wins and owner sets do not union (S-1), so an appended rule +replaces the owners of everything it matches. `add_owner` re-resolves every +tracked path and proves the prior owners survived; that check is the point of +the tool. + +**Scope is per-path, not "the whole file."** For a repo-wide owner, `*` works, +but look at what it has to do: + +```sh +codeowners-tool plan --op 'add_owner(*, @org/team_a)' --out plan.json +``` + +``` +-/web/ @org/frontend ++/web/ @org/frontend @org/team_a +-/services/ @org/platform @org/sre ++/services/ @org/platform @org/sre @org/team_a ++* @org/team_a + +.github/CODEOWNERS : (unowned) → [@org/team_a] +services/api/main.go : [@org/platform, @org/sre] → [@org/platform, @org/sre, @org/team_a] +web/app.js : [@org/frontend] → [@org/frontend, @org/team_a] +``` + +The catch-all alone would not be enough: every later rule shadows it, so +`@org/team_a` would own nothing under `services/` or `web/`. The planner has +to amend each one. A repo-wide op therefore touches every rule in the file and +can pull previously unowned paths (here the CODEOWNERS file itself) into +ownership — read the `ownership_rows` before applying it. + ### The two invariants - **INV-1 (in scope):** after apply, every in-scope path resolves to exactly @@ -119,6 +207,37 @@ independently computed desired state. Anything unprovable → exit 2, nothing written. Plans are idempotent (re-running is a no-op) and preserve every untouched byte — comments, blank lines, spacing, ordering. +### How the planner edits lines + +You express intent over paths; the planner decides which lines move. It never +reorders or reformats, but it does **add lines you did not write**. When an +existing rule governs your scope *and* paths outside it, amending that rule in +place would violate INV-2 — so the planner inserts a **narrowing rule** +immediately after it. That is why the getting-started example above produces a +line nobody typed: + +``` +/services/ @org/platform +/services/api/ @org/platform @org/team-1 ← synthesized: narrows /services/ +/web/ @org/frontend +``` + +Because the last matching rule wins (S-1), this leaves everything outside +`/services/api/` resolving exactly as before. Two consequences worth knowing: + +- **Refusal when no narrowing is expressible.** For some scope/rule + combinations no CODEOWNERS pattern describes exactly the intersection. + Amending would break INV-2 and appending would break INV-1, so the plan is + refused (exit 2) rather than guessed at. Narrow the scope, or restructure the + offending rule by hand. +- **The inexact-narrowing warning.** A synthesized glob can be exact for every + file tracked today yet not provably confined for files added later. The plan + still applies, and prints a warning naming the pattern — read it, because it + is telling you a future file could land on the wrong side. + +Every synthesized line carries a `reason` in the plan JSON explaining why it +exists. + ### `--on-empty` (R-6) Removing the sole owner of a rule needs an explicit policy — **there is no @@ -163,6 +282,70 @@ Removing a sole owner is presented as a **reassignment** with before → after owners per path, never a bare line deletion (R-14). Lookups are cached in memory per run and optionally on disk (`--cache-dir`, `--cache-ttl`). +## CLI reference + +Five commands. `plan` and `apply` are the only writer path; `audit`, +`snapshot`, and `verify` are read-only. + +**Common to `plan`, `audit`, `snapshot`:** + +| Flag | Default | Meaning | +|---|---|---| +| `--repo DIR` | `.` | Path to the local git repository | +| `--branch REF` | `HEAD` | Ref whose tracked tree governs resolution (S-7) | +| `--file PATH` | discovery | Repo-relative CODEOWNERS override, bypassing `.github/` > root > `docs/` precedence. The escape hatch when A-10 reports more than one file | + +**`plan`** — resolve, synthesize, prove. Writes a plan, never the file. + +| Flag | Default | Meaning | +|---|---|---| +| `--op 'kind(args)'` | required | Operation; repeatable for a batch | +| `--on-empty POLICY` | none | `error`\|`inherit`\|`unowned`; required only when a removal would empty an owner set | +| `--out FILE` | stdout | Where to write the plan JSON | +| `--max-size N` | `3000000` | Hard byte cap; over it, refuse (S-4) | +| `--warn-size N` | `2500000` | Byte threshold that emits a warning (R-9) | + +**`apply`** — write the proven edit, then validate. + +| Flag | Default | Meaning | +|---|---|---| +| `--plan FILE` | required | Plan JSON from `plan` | +| `--repo DIR` | plan's repo | Override the repository recorded in the plan | + +The plan pins the input file's SHA-256; if the file changed since planning, +apply refuses rather than clobber the other edit. A write that introduces new +syntax errors is rolled back (exit 6). + +**`audit`** — find rot. Never writes. + +| Flag | Default | Meaning | +|---|---|---| +| `--checks LIST` | all | Comma-separated subset, e.g. `a1,a3,a6`. `a1`, `A1`, `a-1`, `A-1` all parse; an unknown name is a hard error, so a typo can't make an audit pass vacuously | +| `--format FMT` | `text` | `text` or `json` | +| `--github-repo owner/name` | none | Required, with a token, for the API checks A-1…A-3 | +| `--token T` | `$GITHUB_TOKEN` | GitHub PAT | +| `--api-url URL` | `https://api.github.com` | API base, for GHES | +| `--cache-dir D` | memory only | Persist API lookups to disk (R-15) | +| `--cache-ttl DUR` | `24h` | Disk cache lifetime | + +Without a token and `--github-repo`, audit runs the offline checks (A-4…A-12) +and says so. If you *explicitly* request A-1/A-2/A-3 via `--checks` and they +can't run, that is exit 5 — inconclusive, not a silent skip. + +**`snapshot`** — write resolved ownership for every tracked path at a ref. + +| Flag | Default | Meaning | +|---|---|---| +| `--out FILE` | stdout | Where to write the snapshot JSON | + +**`verify`** — compare two snapshots. + +| Flag | Default | Meaning | +|---|---|---| +| `--before FILE` | required | Baseline snapshot | +| `--after FILE` | required | Snapshot to check | +| `--scope PATTERN` | none | Where change is allowed; repeatable. **With none, any change is a violation** | + ## Exit codes | Code | Meaning | diff --git a/docs/BEHAVIOR.md b/docs/BEHAVIOR.md index 80515d8..c0ebe08 100644 --- a/docs/BEHAVIOR.md +++ b/docs/BEHAVIOR.md @@ -928,6 +928,11 @@ SPEC S-9: GitHub's official example — a zero-owner rule un-owns a subtree. > be checked in CI from two ownership snapshots WITHOUT trusting the tool > that produced the change. +### `TestR18_AddedFileDoesNotMaskReassignment` + +The gate still bites: an added file does not launder a real reassignment of +the subtree it lands in, because that subtree's pre-existing files change. + ### `TestR18_NoScope_AssertNoChange` SPEC R-18 + INV-2: with no scope given, verify asserts NOTHING changed. @@ -941,9 +946,14 @@ Order of owners on a line is presentation: {"@a","@b"} == {"@b","@a"}. With scopes, changes inside scope pass; any out-of-scope change fails — this is INV-2 checked from raw data. -### `TestR18_TreeChangesSurface` +### `TestR18_TreeChangesSurfaceButDoNotViolate` -A path added or removed from the tree is reported, not silently ignored. +A path added or removed from the tree is reported, not silently ignored — +but it is NOT an invariant violation. INV-2 preserves what a path resolved +to BEFORE; an added path has no before, a removed path has no after. The +two snapshots come from different refs, so counting a tree delta as a +violation made the documented CI recipe fail on any PR that added a file +outside the declared scope, with CODEOWNERS byte-identical. ### `TestR18_UnownedVsZeroOwnersDistinct` @@ -952,4 +962,4 @@ DIFFERENT states; transitioning between them is a real ownership change. --- -144 documented test cases across 11 packages. +145 documented test cases across 11 packages. diff --git a/internal/cli/cli.go b/internal/cli/cli.go index d561b7d..5796b32 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -84,12 +84,14 @@ func usage(w io.Writer) { plan --op 'add_owner(/services/api, @org/team-1)' [--op ...] [--on-empty error|inherit|unowned] [--repo DIR] [--branch REF] [--file PATH] [--out plan.json] + [--max-size N] [--warn-size N] apply --plan plan.json [--repo DIR] audit [--checks a1,a3,a6] [--format json|text] [--github-repo owner/name] [--token T | $GITHUB_TOKEN] [--api-url URL] [--cache-dir D] [--cache-ttl DUR] - [--repo DIR] [--branch REF] - snapshot [--repo DIR] [--branch REF] [--out snap.json] + [--repo DIR] [--branch REF] [--file PATH] + snapshot [--repo DIR] [--branch REF] [--file PATH] [--out snap.json] verify --before before.json --after after.json [--scope PATTERN ...] + (no --scope means: assert NOTHING changed) Exit codes: 0 ok · 1 no-op · 2 refused (invariant/size) · 3 invalid input 4 audit findings · 5 inconclusive (fail-closed) · 6 rolled back @@ -290,6 +292,14 @@ func cmdVerify(args []string, stdout, stderr io.Writer) int { for _, c := range res.Changed { fmt.Fprintf(stdout, "changed: %s %s → %s\n", c.Path, fmtOwners(c.Before), fmtOwners(c.After)) } + // Tree deltas are informational: the refs differ, so files come and go. + // They are never violations (see verify.Compare). + for _, t := range res.Added { + fmt.Fprintf(stdout, "added: %s %s\n", t.Path, fmtOwners(t.Owners)) + } + for _, t := range res.Removed { + fmt.Fprintf(stdout, "removed: %s %s\n", t.Path, fmtOwners(t.Owners)) + } if !res.OK() { fmt.Fprintf(stderr, "INVARIANT VIOLATED: %d path(s) changed outside the declared scope\n", len(res.Violations)) for _, v := range res.Violations { @@ -297,7 +307,8 @@ func cmdVerify(args []string, stdout, stderr io.Writer) int { } return ExitRefused } - fmt.Fprintf(stdout, "ok: %d change(s), all within scope\n", len(res.Changed)) + fmt.Fprintf(stdout, "ok: %d change(s), all within scope (%d path(s) added, %d removed)\n", + len(res.Changed), len(res.Added), len(res.Removed)) return ExitOK } diff --git a/internal/verify/verify.go b/internal/verify/verify.go index 632d7f2..e6e39b3 100644 --- a/internal/verify/verify.go +++ b/internal/verify/verify.go @@ -32,11 +32,23 @@ type Change struct { After []string `json:"owners_after"` } -// Result of a comparison. Changed lists every difference; Violations lists -// the subset outside the declared scopes (all of them, when no scope given). +// TreePath is a path present in only one of the two snapshots — a file the +// branch added or deleted. Reported for visibility, never a violation: see +// Compare. +type TreePath struct { + Path string `json:"path"` + Owners []string `json:"owners"` +} + +// Result of a comparison. Changed lists every ownership difference among +// paths common to both snapshots; Violations lists the subset outside the +// declared scopes (all of them, when no scope given). Added and Removed +// record tree differences and never affect OK. type Result struct { - Changed []Change `json:"changed"` - Violations []Change `json:"violations"` + Changed []Change `json:"changed"` + Violations []Change `json:"violations"` + Added []TreePath `json:"added,omitempty"` + Removed []TreePath `json:"removed,omitempty"` } // OK reports whether the invariant holds: no out-of-scope change. @@ -63,6 +75,19 @@ func Load(path string) (*Snapshot, error) { // violation (INV-2 from raw data). With no scopes, ANY change is a violation. // A scope that fails to compile is a hard error — silently dropping it would // misreport which changes are in scope (found in review). +// +// Only paths present in BOTH snapshots can violate the invariant. INV-2 says +// every out-of-scope path resolves to what it did before; a path the branch +// added has no "before" to preserve, and one it deleted has no "after". The +// two snapshots normally come from different refs, so treating a tree delta +// as a violation failed every real PR that touched a file outside the +// declared scope, with CODEOWNERS byte-identical. Such paths are reported in +// Added/Removed instead — surfaced, never fatal. +// +// This does not weaken the gate: a CODEOWNERS edit that reassigns a subtree +// still shows up on that subtree's pre-existing files. Only a scope whose +// every file is new to the branch goes unchecked, and there the invariant has +// nothing to say. func Compare(before, after *Snapshot, scopes []string) (*Result, error) { var pats []*pattern.Pattern for _, s := range scopes { @@ -98,13 +123,19 @@ func Compare(before, after *Snapshot, scopes []string) (*Result, error) { for _, p := range sorted { b, bok := before.Ownership[p] a, aok := after.Ownership[p] - if bok && aok && resolve.OwnersEqual(b, a) { - continue - } - c := Change{Path: p, Before: b, After: a} - res.Changed = append(res.Changed, c) - if len(pats) == 0 || !inScope(p) { - res.Violations = append(res.Violations, c) + switch { + case !bok: + res.Added = append(res.Added, TreePath{Path: p, Owners: a}) + case !aok: + res.Removed = append(res.Removed, TreePath{Path: p, Owners: b}) + case resolve.OwnersEqual(b, a): + // unchanged + default: + c := Change{Path: p, Before: b, After: a} + res.Changed = append(res.Changed, c) + if len(pats) == 0 || !inScope(p) { + res.Violations = append(res.Violations, c) + } } } return res, nil diff --git a/internal/verify/verify_test.go b/internal/verify/verify_test.go index 806e2f3..da6bc8c 100644 --- a/internal/verify/verify_test.go +++ b/internal/verify/verify_test.go @@ -77,12 +77,40 @@ func TestR18_UnownedVsZeroOwnersDistinct(t *testing.T) { } } -// A path added or removed from the tree is reported, not silently ignored. -func TestR18_TreeChangesSurface(t *testing.T) { - before := snap(map[string][]string{"a.go": {"@a"}}) +// A path added or removed from the tree is reported, not silently ignored — +// but it is NOT an invariant violation. INV-2 preserves what a path resolved +// to BEFORE; an added path has no before, a removed path has no after. The +// two snapshots come from different refs, so counting a tree delta as a +// violation made the documented CI recipe fail on any PR that added a file +// outside the declared scope, with CODEOWNERS byte-identical. +func TestR18_TreeChangesSurfaceButDoNotViolate(t *testing.T) { + before := snap(map[string][]string{"a.go": {"@a"}, "gone.go": {"@a"}}) after := snap(map[string][]string{"a.go": {"@a"}, "new.go": {"@a"}}) res, _ := verify.Compare(before, after, nil) + if !res.OK() { + t.Errorf("tree-only delta must not violate the invariant: %+v", res.Violations) + } + if len(res.Changed) != 0 { + t.Errorf("tree delta is not an ownership change: %+v", res.Changed) + } + if len(res.Added) != 1 || res.Added[0].Path != "new.go" { + t.Errorf("added = %+v, want new.go", res.Added) + } + if len(res.Removed) != 1 || res.Removed[0].Path != "gone.go" { + t.Errorf("removed = %+v, want gone.go", res.Removed) + } +} + +// The gate still bites: an added file does not launder a real reassignment of +// the subtree it lands in, because that subtree's pre-existing files change. +func TestR18_AddedFileDoesNotMaskReassignment(t *testing.T) { + before := snap(map[string][]string{"web/app.js": {"@fe"}}) + after := snap(map[string][]string{"web/app.js": {"@other"}, "web/new.js": {"@other"}}) + res, _ := verify.Compare(before, after, []string{"/services/"}) if res.OK() { - t.Error("new path must surface as a change") + t.Error("out-of-scope reassignment must still fail when the PR also adds files") + } + if len(res.Violations) != 1 || res.Violations[0].Path != "web/app.js" { + t.Errorf("violations = %+v", res.Violations) } } diff --git a/tools/gendocs/main.go b/tools/gendocs/main.go index fab0cc2..1b57a4c 100644 --- a/tools/gendocs/main.go +++ b/tools/gendocs/main.go @@ -77,6 +77,33 @@ func main() { fmt.Printf("wrote %s (%d tests)\n", out, total) } +// sortedPkgNames orders test packages deterministically, external (`x_test`) +// before internal (`x`), then lexically. +func sortedPkgNames(pkgs map[string]*ast.Package) []string { + names := make([]string, 0, len(pkgs)) + for n := range pkgs { + names = append(names, n) + } + sort.Slice(names, func(i, j int) bool { + ei := strings.HasSuffix(names[i], "_test") + ej := strings.HasSuffix(names[j], "_test") + if ei != ej { + return ei + } + return names[i] < names[j] + }) + return names +} + +func sortedKeys[V any](m map[string]V) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + func scan(dir string) *pkgDoc { fset := token.NewFileSet() pkgsMap, err := parser.ParseDir(fset, dir, func(fi os.FileInfo) bool { @@ -88,8 +115,16 @@ func scan(dir string) *pkgDoc { out := &pkgDoc{Dir: filepath.ToSlash(dir)} var names []string byName := map[string]testDoc{} - for _, pkg := range pkgsMap { - for _, f := range pkg.Files { + // parser.ParseDir returns unordered maps, and a directory can hold BOTH + // the internal (`plan`) and external (`plan_test`) test packages — so + // "first doc comment wins" picked a random one per run and `make docs` + // was not reproducible. Walk packages and files in a fixed order, with + // the external test package first: its doc comment is the one written to + // describe the package's behavior, not an internal white-box helper's. + for _, pkgName := range sortedPkgNames(pkgsMap) { + pkg := pkgsMap[pkgName] + for _, fname := range sortedKeys(pkg.Files) { + f := pkg.Files[fname] if f.Doc != nil && out.Doc == "" { out.Doc = strings.TrimSpace(f.Doc.Text()) }