diff --git a/README.md b/README.md index 67836b3..b5372ed 100644 --- a/README.md +++ b/README.md @@ -12,11 +12,8 @@ So you say what you want — "`@team-2` should co-own `/services/api/`" — and works out the lines. Then it checks its own work against every file in your repo, and **refuses to write anything it can't prove correct.** -It also reads: `snapshot` tells you who owns what today, and `audit` finds owners who've -left the company, rules that match no files, and owners who don't actually have -permission to approve. Neither writes anything; `lint` is the command that repairs what -`audit` finds — see **[docs/LINTING.md](docs/LINTING.md)**. - +It also reads: `snapshot` tells you who owns what today, `audit` finds owners who've +left the company, rules that match no files, and owners who can't actually approve. Works with github.com and GitHub Enterprise Server. ## Install @@ -39,28 +36,25 @@ $ codeowners-tool version | You want to… | Go to | |---|---| -| See who owns what in this repo, right now | [Find out what you have](#find-out-what-you-have) | +| See who owns what in this repo, right now | [The 60-second tour](#the-60-second-tour) | | Understand `add_owner` vs `set_owners` before touching anything | [Basic concepts](#basic-concepts) | -| Follow one small end-to-end change | [A basic example](#a-basic-example) | -| **Lint** a CODEOWNERS file, locally or in CI | [How to: lint](#how-to-lint-a-codeowners-file) | -| **Repair** one — dead owners, split handles, stale rules | [docs/LINTING.md](docs/LINTING.md) | -| **Write** a CODEOWNERS file for a repo that has none | [How to: write a new file](#how-to-write-a-new-codeowners-file) | -| **Modify** a CODEOWNERS file that already exists | [How to: modify an existing file](#how-to-modify-an-existing-codeowners-file) | +| Follow worked end-to-end changes | [docs/GUIDE.md](docs/GUIDE.md) | +| **Lint** or **repair** a CODEOWNERS file, locally or in CI | [docs/LINTING.md](docs/LINTING.md) | +| Prove a merged PR changed only the owners it declared | [GUIDE.md#reviewing](docs/GUIDE.md#reviewing-the-change-before-it-lands) | | Roll one policy out over many repos | [docs/FLEET.md](docs/FLEET.md) | | Look up a flag, exit code, or JSON field | [docs/REFERENCE.md](docs/REFERENCE.md) | +| Look up a term | [docs/CONCEPTS.md](docs/CONCEPTS.md) | | Know exactly what is guaranteed, and by which test | [docs/BEHAVIOR.md](docs/BEHAVIOR.md) | -## Find out what you have +## The 60-second tour -Every command is discoverable from the binary, and these are all safe to run against +Every command is discoverable from the binary, and these are safe to run against anything — none of them writes: ```sh -codeowners-tool --help # every command and its flags -codeowners-tool snapshot # who owns each tracked file, as JSON on stdout -codeowners-tool audit # what's broken or rotten in the current file -codeowners-tool lint --dry-run … # what a repair pass would change (writes nothing) -codeowners-tool check --policy p.json # is this policy well-formed? (reads no repo) +codeowners-tool --help # every command and its flags +codeowners-tool snapshot # who owns each tracked file, as JSON on stdout +codeowners-tool audit # what's broken or rotten in the current file ``` `snapshot` is the one to reach for first, because it answers the question CODEOWNERS @@ -69,15 +63,29 @@ itself doesn't: ```console $ codeowners-tool snapshot | jq .ownership { - ".github/CODEOWNERS": ["@org/everyone"], "README.md": ["@org/everyone"], - "docs/guide.md": ["@org/everyone"], "services/api/main.go": ["@org/api-team"], "services/web/app.ts": ["@org/everyone"] } ``` -Nothing below writes anything either until you drop `--dry-run`. +In that map `[]` means a rule matches the path and deliberately assigns no owners; +`null` means no rule matches it at all. `snapshot` reads the CODEOWNERS **committed** +at `--branch` (default `HEAD`) — the file GitHub sees — so commit before snapshotting. +(Every command also has its own help: `codeowners-tool sync --help`.) + +To change ownership, state the intent and preview it — nothing writes until you drop +`--dry-run`: + +```console +$ codeowners-tool sync --op 'add_owner(README.md, @org/docs-team)' --dry-run +applied: 1 op(s) applied, 0 skipped; 1 line change(s), 1 path(s) change owners + ops[0] applied (proven: tree) +``` + +`proven: tree` means the claim was checked against the repo's real files, not just +reasoned about. Runs are idempotent, and every untouched byte survives: comments, blank +lines, spacing, ordering. [docs/GUIDE.md](docs/GUIDE.md) walks this change end to end. ## Basic concepts @@ -89,13 +97,13 @@ derives for you. **You state an intent — an *op*.** Same syntax whether you pass it with `--op` or list it in a policy file. Scope is a directory, file path, or glob, using CODEOWNERS pattern -syntax. +syntax; a space in a path is escaped with a backslash (`docs/release\ notes.md`). | Op | What it means | |---|---| -| `add_owner(scope, owner)` | Owner becomes a **co-owner**. Every pre-existing owner of every path in scope is kept. | +| `add_owner(scope, owner)` | Owner (or `[owners]`) becomes a **co-owner**. Every pre-existing owner of every path in scope is kept. | | `set_owners(scope, [owners])` | This exact set owns every path in scope, displacing whoever owned it. `[]` is legal and deliberately un-owns the scope. | -| `remove_owner(scope, owner)` | Owner stops owning every path in scope. If that would empty a rule, you must say what happens — see [`--on-empty`](docs/REFERENCE.md#--on-empty--on_empty-r-6). | +| `remove_owner(scope, owner)` | Owner (or `[owners]`) stops owning every path in scope. If that would empty a rule, you must say what happens — see [`--on-empty`](docs/REFERENCE.md#--on-empty--on_empty-r-6). | | `rename_owner(old, new)` | Global identifier substitution — the only op that is safe as plain text replacement. | `add_owner` and `set_owners` are the two you'll use most, and picking the wrong one is @@ -115,7 +123,8 @@ $ codeowners-tool sync --op 'set_owners(/services/api/, [@org/team-1])' Editing the file by hand, both of those look like the same one-line edit. That's the trap: adding `/services/api/ @org/team-1` at the bottom of a file *silently* performs the -second one. +second one. (An op can also carve out sub-paths with an `except` clause — see +[REFERENCE.md#operations](docs/REFERENCE.md#operations).) **Two invariants hold on every write**, or the write doesn't happen: @@ -123,403 +132,64 @@ second one. - **INV-2** — after the change, every path *out of scope* is owned exactly as it was before. This is the product. -The tool synthesizes line edits, then re-resolves every file git knows about and compares -against an independently computed desired state. Anything it can't prove → it refuses and -writes nothing. Runs are idempotent, and every untouched byte survives: comments, blank -lines, spacing, ordering. - -## A basic example - -A small repo, with a `README.md` nobody owns in particular: - -``` -README.md -docs/guide.md -services/api/main.go -services/web/app.ts -``` - -```console -$ cat .github/CODEOWNERS -* @org/everyone -/services/api/ @org/api-team -``` - -Docs team should co-own the README. Look before you leap: - -```console -$ codeowners-tool sync --op 'add_owner(README.md, @org/docs-team)' --dry-run -applied: 1 op(s) applied, 0 skipped; 1 line change(s), 1 path(s) change owners - ops[0] applied (proven: tree) -``` - -`proven: tree` means the claim was checked against the repo's real files, not just -reasoned about. Drop `--dry-run` to write it: - -```console -$ codeowners-tool sync --op 'add_owner(README.md, @org/docs-team)' -applied: 1 op(s) applied, 0 skipped; 1 line change(s), 1 path(s) change owners - ops[0] applied (proven: tree) -$ cat .github/CODEOWNERS -* @org/everyone -README.md @org/everyone @org/docs-team -/services/api/ @org/api-team -``` - -Three things happened that are worth noticing. - -`@org/everyone` was **carried onto the new line**. They owned `README.md` via `*`, and -`add_owner` means co-own, so the new rule has to restate them or they'd be dropped. - -The line went **in the middle, not at the end**. Appending it would have placed it after -`/services/api/`, which is harmless here — but the general habit isn't, and putting it -directly after the rule it narrows is what keeps INV-2 true. +Anything it can't prove → it refuses and writes nothing. Refusing is a normal outcome +for some repos, not a bug — [GUIDE.md#when-it-refuses](docs/GUIDE.md#when-it-refuses) +shows what it looks like and what to do. -`/services/api/` was **not touched at all**, including its original spacing. - -Run it again and nothing happens: - -```console -$ codeowners-tool sync --op 'add_owner(README.md, @org/docs-team)' -unchanged: 0 op(s) applied, 0 skipped; 0 line change(s), 0 path(s) change owners - ops[0] unchanged (proven: tree) -``` - -> **Pattern note.** `README.md` is unanchored, so like gitignore it matches a `README.md` -> at *any* depth. Write `/README.md` if you mean only the one at the root. - -## How to: lint a CODEOWNERS file - -`audit` is the linter and it is read-only — where a fix is expressible it prints an op -string for a human to run, and never applies one itself. `lint` -([below](#fixing-what-it-finds-lint)) is the command that applies them. - -```console -$ codeowners-tool audit -note: no token/--github-repo — running offline checks only (A-4..A-12) -[A-4/warning] (line 3) pattern "/nonexistent/" matches zero tracked files (report-only: may be deliberate, R-11) -[A-5/warning] (line 4) pattern "/Docs/" matches zero files ONLY because of case — CODEOWNERS is case-sensitive (S-6); "/docs/" would match -$ echo $? -4 -``` - -**Exit 4 means findings, 0 means clean** — that's your CI gate, and `--fail-on` decides -which findings count toward it. Exit 5 means the audit couldn't reach a conclusion (see -below), which you also want to fail on. - -Offline it checks the file and the git tree: dead patterns, case-only mismatches, shadowed -and duplicate rules, syntax errors, unowned paths, more than one CODEOWNERS file, and the -3 MB cliff. Add a token and a repo and it also checks the owners themselves — that they -exist, are in the org, and have **explicit write access** (org membership is not enough, -and this is the check that catches the most real rot): - -```console -$ GITHUB_TOKEN=... codeowners-tool audit --github-repo org/repo --format json -``` - -Two things to know before you wire it into CI. - -**`audit` reads the CODEOWNERS committed at `--branch`** (default `HEAD`), not the copy in -your working directory — it is asking what GitHub would do, and GitHub only ever sees -committed files. An uncommitted edit will not show up. (`sync`, `plan` and `apply` are the -other way round: they read and write the working-tree file, and resolve ownership against -the ref's tree.) - -**It fails closed.** A 404 from the API can mean deleted, renamed, invisible to your -token, or rate-limited. Anything inconclusive is reported as `unknown`, exits 5, and -**never proposes a removal** — an expired token quietly stripping owners is the worst -thing this tool could do, so it can't. Pin the exit code you gate on accordingly: - -```yaml -- run: codeowners-tool audit --github-repo ${{ github.repository }} --fail-on error - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} -``` - -`--fail-on` chooses which findings exit 4; every finding is printed either way. The -default `any` is the strictest reading and the right one for a file you maintain by hand. -Gate on `error` once you roll a baseline out with `on_zero_match: declare` — a declared -rule matches zero files by construction, and A-4 reports every one of them as a -report-only warning, so the default would fail CI in each repo the rollout touched. -`never` reports without gating. Exit 5 is unaffected: a check that could not run is not a -finding whose severity you can weigh. - -The full check table (A-1 … A-12, which ones the API is needed for, which propose fixes) -is in [docs/REFERENCE.md](docs/REFERENCE.md#audit-checks). Run a subset with -`--checks a1,a3,a6`. - -### Fixing what it finds: `lint` - -`lint` repairs what `audit` reports: handles whitespace has broken, owners that no longer -exist, and — with `--remove-stale-paths` — rules matching no files. Exit 4 means the file -still needs a person, so the dry run is your CI gate. - -```console -$ GITHUB_TOKEN=... codeowners-tool lint --github-repo org/repo --dry-run -``` - -Drop `--dry-run` to write it. **[docs/LINTING.md](docs/LINTING.md)** has the flags, the -exit codes, and what to do about every error it can print. - -There's a second kind of lint that has nothing to do with a repo. If you keep your ops in -a policy file, `check` validates the file itself: - -```console -$ codeowners-tool check --policy policy.json -ok: policy.json — 3 op(s), no policy errors -``` - -```console -$ codeowners-tool check --policy bad.json -error: bad.json:1:21: ops[0]: op "add_owner(/services/api/)" is not valid: add_owner takes (scope, owner) or (scope, [owners]), got 1 args -error: bad.json:1:91: ops[1]: unknown field "on_zero_mtach" (did you mean "on_zero_match"?); an op accepts "op", "id", "on_zero_match", "on_except_zero_match", "except", "note" -this is a policy error — it will fail identically on every repo; fix the policy, do not retry -$ echo $? -3 -``` +> **Pattern note.** `README.md` is unanchored, so like gitignore it matches a +> `README.md` at *any* depth. Write `/README.md` if you mean only the one at the root. -`check` reads no repository and writes nothing. It exits `0` or `3` and never `1`, so -under `set -e` a good policy always lets the script continue. It also echoes the -`on_zero_match` each op resolved to, whenever the policy resolves one at all — `policy.json` -above has no `defaults` block and no per-op value, so there is nothing to echo. +## Audit and lint -## How to: write a new CODEOWNERS file +`audit` reports, `lint` repairs; both leave the file alone until you say otherwise. +Offline, `audit` checks the file against the git tree — dead patterns, case-only +mismatches, shadowed rules, unowned paths. With a token it also checks the owners +themselves: that they exist, are in the org, and have **explicit write access** (the +check that catches the most real rot). -For a repo with no CODEOWNERS at all, `create` writes one at `.github/CODEOWNERS`. It -never overwrites an existing file, and it's off by default. A policy run states it as -`"create": true` in the policy; `--create` is the flag for `--op` runs, and alongside -`--policy` it is exit 3 — a flag must not decide what the reviewed artifact answers. +**Exit 4 means findings, 0 means clean** — that's your CI gate. Exit 5 means a check +couldn't reach a conclusion: the audit **fails closed**, and never proposes a removal +it can't verify — an expired token quietly stripping owners is the worst thing this +tool could do, so it can't. Flags, the full check table, and every error it can print: +**[docs/LINTING.md](docs/LINTING.md)**. -It is permission to create a file, not an instruction to. When there is nothing to write — -every op carrying `on_zero_match: "skip"` matched nothing — it creates no file and no -`.github/` directory, and reports `skipped` at exit 0. Nothing here synthesizes ownership -to make a repo look covered. +## Many repos -A scope that matches nothing under the DEFAULT `require` is the other outcome, and not a -quiet one: that repo is refused at exit 2 and gets no file, because a path you named and -this repo does not have is usually a typo. Which of the two you want is what -`on_zero_match` is for — see [docs/FLEET.md](docs/FLEET.md#your-100-repos-arent-identical). - -The smallest version is one op: - -```console -$ codeowners-tool sync --op 'set_owners(*, [@org/everyone])' --create -applied: 1 op(s) applied, 0 skipped; 1 line change(s), 4 path(s) change owners - ops[0] applied (proven: tree) - created a new CODEOWNERS file -``` - -For a real file, put the ops in a policy so the whole shape is reviewable in one diff: - -```json -{ - "version": 1, - "name": "bootstrap ownership", - "create": true, - "ops": [ - "add_owner(*, @org/everyone)", - "add_owner(/services/api/, @org/api-team)", - "add_owner(/docs/, @org/docs-team)", - { "op": "add_owner(/.github/workflows/, @org/ci)", "on_zero_match": "declare" } - ] -} -``` - -```console -$ codeowners-tool check --policy bootstrap.json -ok: bootstrap.json — 4 op(s), no policy errors - ops[0] on_zero_match: require (built-in) - ops[1] on_zero_match: require (built-in) - ops[2] on_zero_match: require (built-in) - ops[3] on_zero_match: declare -$ codeowners-tool sync --policy bootstrap.json -applied: 4 op(s) applied, 0 skipped; 4 line change(s), 4 path(s) change owners - ops[0] applied (proven: tree) - ops[1] applied (proven: tree) - ops[2] applied (proven: tree) - ops[3] applied (proven: structural) - created a new CODEOWNERS file -$ cat .github/CODEOWNERS -* @org/everyone -/docs/ @org/everyone @org/docs-team -/services/api/ @org/everyone @org/api-team -/.github/workflows/ @org/ci -``` - -Two things that will bite you on the first try. - -**Use `add_owner` for the catch-all, not `set_owners`.** `add_owner` ops commute, so any -number of them can share one run. `set_owners(*, …)` overlaps every other scope and does -*not* commute with them, so the batch is refused rather than silently resolved by order: - -```console -$ codeowners-tool sync --policy bootstrap.json -error: ops "set_owners(*, [@org/everyone])" and "add_owner(/services/api/, @org/api-team)" do not commute, and "*" provably governs every path "/services/api/" does — so the batch is order-dependent on every repository that has one (R-8); run "set_owners(*, [@org/everyone])" on its own first and the narrower op(s) in a second run — but preview that first run with --dry-run or `plan --out`: "set_owners(*, [@org/everyone])" REPLACES the owners of every path in scope, so anyone owning those paths today and not listed in it loses them -this is a policy error — it will fail identically on every repo; fix the policy, do not retry -$ echo $? -3 -``` - -Exit 3, not 2: an order-dependent batch is wrong on every repo, so a fleet stops instead -of recording it a hundred times. - -**A rule for files that don't exist yet needs `on_zero_match: "declare"`.** The default is -`require`: a scope matching nothing is treated as a problem with this repo, because it -usually is a typo. `declare` says you meant it — the rule is written at the end of the -file, ready for files added later. That op reports `proven: structural` rather than -`proven: tree`: with no matching files there is nothing to check the rule against, so the -tool proves only that no later rule can override it. See -[what `declare` costs](docs/REFERENCE.md#what-declare-costs). - -## How to: modify an existing CODEOWNERS file - -Same command; the interesting part is what it protects you from. Starting from: - -``` -* @org/everyone -/services/api/ @org/api-team -``` - -Each of these is one run against that file, and the second column is the line it leaves -behind. Notice that the original spacing survives every one of them. - -| Run | `/services/api/` afterwards | -|---|---| -| `sync --op 'add_owner(/services/api/, @org/platform)'` | `/services/api/ @org/api-team @org/platform` | -| `sync --op 'set_owners(/services/api/, [@org/platform, @org/api-team])'` | `/services/api/ @org/platform @org/api-team` | -| `sync --op 'rename_owner(@org/api-team, @org/platform-api)'` | `/services/api/ @org/platform-api` | - -The first is the common case and the one hand-editing gets wrong. The second is the same -edit stated deliberately. The third is what a reorg needs — it substitutes the identifier -everywhere it appears, and it's the only op that is safe as plain text replacement, -because it can't change any rule's match set. - -**Removing an owner** is the one that stops and asks. If it would empty a rule's owner set -there is deliberately no default: - -```console -$ codeowners-tool sync --op 'remove_owner(/services/api/, @org/api-team)' -error: removing @org/api-team empties the owner set of "/services/api/"; an explicit --on-empty policy (error|inherit|unowned) is required — there is deliberately no default (R-6) (governing file: .github/CODEOWNERS) -refused: 0 op(s) applied, 0 skipped; 0 line change(s), 0 path(s) change owners - removing @org/api-team empties the owner set of "/services/api/"; an explicit --on-empty policy (error|inherit|unowned) is required — there is deliberately no default (R-6) (governing file: .github/CODEOWNERS) -$ echo $? -2 -``` - -Say which you meant, and it proceeds — here `inherit` deletes the rule and lets the -preceding broader one take over: - -```console -$ codeowners-tool sync --op 'remove_owner(/services/api/, @org/api-team)' --on-empty inherit -applied: 1 op(s) applied, 0 skipped; 1 line change(s), 1 path(s) change owners - ops[0] applied (proven: tree) -$ cat .github/CODEOWNERS -* @org/everyone -``` - -`unowned` would instead keep the pattern with zero owners — GitHub's sanctioned substitute -for `!` negation — and `error` refuses outright. The recommendation is `error`. - -### Reviewing the change before it lands - -`sync` is plan-assert-apply-validate in one step. Split it in two when you want the -artifact in the middle — a JSON plan with resolved ownership per path and the literal line -diff: - -```console -$ codeowners-tool plan --op 'add_owner(/services/web/, @org/web-team)' --out plan.json -plan written to plan.json -1 line change(s), 1 path(s) change owners, 58 → 101 bytes -$ jq '.ownership_rows, .diff' plan.json -[ - { - "path": "services/web/app.ts", - "owners_before": ["@org/everyone"], - "owners_after": ["@org/everyone", "@org/web-team"] - } -] -"@ line 2\n+/services/web/ @org/everyone @org/web-team\n" -$ codeowners-tool apply --plan plan.json -applied: .github/CODEOWNERS (58 → 101 bytes) -``` - -Every change also carries the reason it took that shape, which is the part a reviewer -actually wants: - -``` -"rule \"*\" also governs out-of-scope paths; inserted narrowing rule \"/services/web/\" - immediately after it so out-of-scope resolution is untouched (R-2)" -``` - -And to prove after the fact that a merged change moved nothing it didn't declare: - -```sh -codeowners-tool snapshot --branch main --out before.json -codeowners-tool snapshot --branch feature --out after.json -codeowners-tool verify --before before.json --after after.json --scope /services/api/ -``` - -### When it refuses - -Sometimes there is no line that does what you asked and nothing else. Given a repo with -`infra/main.tf` and `infra/README.md`, and a CODEOWNERS of exactly: - -``` -infra/ @org/infra-legacy -``` - -the tool says so and writes nothing: - -```console -$ codeowners-tool sync --op 'add_owner(**/*.tf, @org/infra)' -error: refusing: rule "infra/" also governs paths outside scope "**/*.tf", and no sound narrowing pattern is derivable — amending would violate INV-2, appending would violate INV-1 (governing file: .github/CODEOWNERS) -refused: 0 op(s) applied, 0 skipped; 0 line change(s), 0 path(s) change owners - refusing: rule "infra/" also governs paths outside scope "**/*.tf", and no sound narrowing pattern is derivable — amending would violate INV-2, appending would violate INV-1 (governing file: .github/CODEOWNERS) -$ echo $? -2 -``` - -In English: `infra/` covers `infra/main.tf` *and* `infra/README.md`. Editing that line -would change the README's owners, which you never asked for (INV-2). Adding a `**/*.tf` -line before it would be overridden by it (INV-1). So it stops. - -This is a normal outcome for some repos, not a bug — the tool fails closed rather than -guessing. The fix is usually to replace the over-broad rule the error names with narrower -ones, then re-run. - -Exit `2` means *this repo* needs a human. Exit `3` means *the policy* is broken and will -fail identically everywhere. That split is what makes a hundred-repo run survivable — -see [docs/FLEET.md](docs/FLEET.md). +Exit `2` means *this repo* needs a human. Exit `3` means *the policy* is broken and +will fail identically everywhere, so the fleet stops instead of recording it a hundred +times. `check --policy` validates a policy file without touching any repository — +run it before repo #1. The resumable rollout script and the `jq` habits that stop a +silent no-op looking like success: **[docs/FLEET.md](docs/FLEET.md)**. ## Where everything else is -- **[docs/INSTALL.md](docs/INSTALL.md)** — every install route, provenance verification, - GHES, upgrading, uninstalling. +- **[docs/GUIDE.md](docs/GUIDE.md)** — worked examples: bootstrap a file, modify one, + review a change, understand a refusal. - **[docs/REFERENCE.md](docs/REFERENCE.md)** — commands and flags, policy file fields, - JSON output, exit codes, op semantics, the audit check table, GitHub semantics the tool - encodes, design decisions, prior art. -- **[docs/FLEET.md](docs/FLEET.md)** — rolling one policy across many repos: a resumable - script, `--format json`, and the `jq` habits that stop a silent no-op looking like - success. -- **[docs/BEHAVIOR.md](docs/BEHAVIOR.md)** — generated from the test suite. Every `R-`, - `S-`, `INV-` and `A-` identifier in these docs is a numbered requirement enforced by a - named test, and this is where you look it up. + JSON output, exit codes, op semantics, the audit check table, GitHub semantics the + tool encodes, design decisions, prior art. +- **[docs/LINTING.md](docs/LINTING.md)** — audit and repair: flags, exit codes, every + error and what to do about it. +- **[docs/FLEET.md](docs/FLEET.md)** — rolling one policy across many repos. +- **[docs/CONCEPTS.md](docs/CONCEPTS.md)** — glossary, and habits that save you. +- **[docs/BEHAVIOR.md](docs/BEHAVIOR.md)** — generated from the test suite. Every + `R-`, `S-`, `INV-` and `A-` identifier in these docs is a numbered requirement + enforced by a named test, and this is where you look it up. +- **[docs/INSTALL.md](docs/INSTALL.md)** — every install route, provenance + verification, GHES, upgrading, uninstalling. - **[CONTRIBUTING.md](CONTRIBUTING.md)** — a few unusual rules, each with a reason. +- **[SECURITY.md](SECURITY.md)** — reporting vulnerabilities. +- **[CHANGELOG.md](CHANGELOG.md)** — what changed, release by release. ## Tests as documentation -The test suite is the specification. Every test carries a doc comment naming the +The test suite is the specification: every test carries a doc comment naming the requirement it enforces, and `make docs` regenerates -[docs/BEHAVIOR.md](docs/BEHAVIOR.md) from those comments via `go/ast`, so the docs cannot -drift from what is verified. On top of the unit and end-to-end tests: a vendored pattern -corpus from [hmarr/codeowners](https://github.com/hmarr/codeowners), a 500k-case -differential fuzz against that matcher unmodified as an oracle, and property tests that -prove INV-1/INV-2 and idempotence by independent re-resolution. - -```sh -make build # ./bin/codeowners-tool -make all # vet, test, build, docs -``` +[docs/BEHAVIOR.md](docs/BEHAVIOR.md) from those comments, so the docs cannot drift from +what is verified. On top of the unit and end-to-end tests: a 500k-case differential +fuzz against the [hmarr/codeowners](https://github.com/hmarr/codeowners) matcher as an +oracle, and property tests that prove INV-1/INV-2 and idempotence by independent +re-resolution. ## License diff --git a/docs/BEHAVIOR.md b/docs/BEHAVIOR.md index 69b7e7e..ed6a190 100644 --- a/docs/BEHAVIOR.md +++ b/docs/BEHAVIOR.md @@ -372,6 +372,20 @@ a bare line deletion. > `plan` is deliberately exempt from both: it emits an artifact and writes no > CODEOWNERS, so proving against another ref is exactly its job. +**`lint_offline_test.go`** + +> The offline tree-only mode of `lint --remove-stale-paths` (UAT finding, +> TestOfflineStaleRuleRemovalReportable). +> +> R-12 makes owner existence — an API fact — undecidable offline, so lint +> without credentials refuses at exit 5. Whether a pattern matches zero +> tracked files is a git-TREE fact (audit's A-4/A-5), so when +> --remove-stale-paths names that repair as what the run is for, the run +> proceeds offline: stage 3 alone, owner checks skipped and DISCLOSED, no +> owner verified, repaired, or removed. Everything else about lint's contract +> holds unchanged — exit 4 for pending or spared work, exit 0 for clean, +> never 1; the A-5 sparing of case-only typos; apply as the single writer. + **`lint_test.go`** > `audit --lint` is the first verb in this tool that EDITS a file on the basis @@ -680,6 +694,19 @@ a bare line deletion. > ---------- R-36e: every command validates the whole file ---------- +**`prerelease_bugs_test.go`** + +> Regression guards from the pre-release review: each test began life as a +> failing repro of a confirmed bug and now pins the fixed behavior. The doc +> comments keep the original finding so the guarded failure mode stays legible. + +**`prerelease_fixes_test.go`** + +> Regression guards around the pre-release fixes: each test here pins an edge +> the KnownBug test that motivated the fix does not — the same behavior on the +> verbs the bug report only implied, and the neighboring cases the fix must +> NOT have broken. + **`rollout_test.go`** > The rollout scenarios. @@ -930,6 +957,107 @@ SPEC R-17: exit 1 no-op, exit 3 invalid input. Review finding: a typo'd --scope must be a loud exit-3 error, not silently dropped (which turned every change into an inexplicable violation). +### `TestFix_ApplyBelowRepoRootRefused` + +The repo-root guard reaches `apply` too: --repo can point the apply at a +different clone than the plan's, and pointed below the root the joined +codeowners_path names a file GitHub never reads (checkRepoRoot). + +### `TestFix_ApplyRefusesSymlinkedCodeowners` + +The symlink refusal reaches `apply` too: a plan is reviewed in one place and +applied in another, so the link can appear between the two — the write must +refuse, and the link's in-repo target must keep its bytes. + +### `TestFix_ApplyRefusesSymlinkedParentDir` + +The parent-directory refusal reaches `apply` too: the link can appear +between planning and applying, exactly like the final-component case the +existing guard pins. + +### `TestFix_AuditCleanLineStaysInTextMode` + +The pure-JSON fix must not have taken the human verdict with it: under the +default text format a clean audit still says so on stdout. + +### `TestFix_AuditJSONWithFindingsIsPureJSON` + +`audit --format json` stdout is one JSON document in the findings case too, +not only on the clean repo the KnownBug test pins. + +### `TestFix_CeilingRefusalNamesOnlyAppliedOps` + +R-25's refusal names only the ops that would have APPLIED: an op whose rule +was already satisfied changed zero paths, so naming it sent the operator +narrowing an op that was never behind the number. + +### `TestFix_DetachedHeadLabelIsBareSHA` + +headLabel's contract: on a detached HEAD the label is the bare abbreviated +SHA — the honest answer, since there is no branch name to offer. Before the +fix the echoed `--end-of-options` line meant the name never equalled "HEAD" +and the detached path could not fire. + +### `TestFix_FileFlagSpellingsClassifyByCleanPath` + +The other uncleaned spellings of a governing location: `.github//CODEOWNERS` +and `docs/../CODEOWNERS` name the S-8 files they clean to, so neither draws +the "governs nothing" warning — while a path that genuinely is not an S-8 +location still does. + +### `TestFix_LintRefusesSymlinkedCodeowners` + +lint shares the same write path, and its symlink refusal fires before any +API call — so it is decidable, and tested, offline. + +### `TestFix_LintRefusesSymlinkedParentDir` + +lint shares the helper, so the parent-directory case refuses there too — +offline, before any API call, like its final-component sibling above. + +### `TestFix_NoRecordNoteCoversEverySyncExit3` + +Every sync exit-3 verdict asked for a sink discloses that no record was +written — not only the two paths the first fix covered. A fleet aggregating +--out records otherwise loses these repos silently, the exact hazard the +note exists to disclose. + +### `TestFix_PositionalArgsRejectedOnEveryVerb` + +Positional args are rejected on EVERY verb, not only the audit invocation +that surfaced the bug: the flag package stops at the first non-flag token on +all of them alike, so any verb left unguarded still swallows whole +arguments. Exit 3 — decidable from the arguments alone, like every other +member of that class. + +### `TestFix_StaleCommentWarningLeadingBoundary` + +The stale-comment warning needs a LEADING token boundary too: a rename of +@old-team must stay quiet about `someone@old-team`, where the match is the +tail of an email, while a real mention — space-separated or glued to the +comment glyph — still warns. + +### `TestFix_SymlinkElsewhereStaysIrrelevant` + +A symlink that is NOT the governing CODEOWNERS stays irrelevant: only the +write path's components are Lstat'ed, so an ordinary repo full of links +syncs as before. + +### `TestFix_SymlinkedDirOffWritePathStaysIrrelevant` + +A symlinked DIRECTORY that is not on the write path stays irrelevant, like a +symlinked file elsewhere always has: the walk covers only the components +between the repository root and the CODEOWNERS being written. + +### `TestFix_SyncRefusesSymlinkedParentDir` + +A symlinked PARENT directory is the same dead-on-arrival write one level up: +git tracks `.github -> real-gh` as a link blob, so `.github/CODEOWNERS` does +not exist in the tree GitHub reads — yet Lstat'ing only the final component +(a real file, reached through the link) let sync write through it at exit 0. +The refusal must fire, name WHICH component is the link, and leave the +link's target untouched. + ### `TestFleet_BrokenPolicyHaltsOnTheFirstRepo` SPEC R-20/R-22: a broken policy is exit 3 on the FIRST repo, before a single @@ -1110,6 +1238,85 @@ repos with a mistyped `--out` directory that is 100 real edits reported as The record on stdout is the durable trace and goes first, unconditionally; a sink that cannot be written is a warning on stderr and nothing more. +### `TestLintOffline_CaseOnlyMissIsSparedNotDeleted` + +SPEC A-5/S-6 offline: a rule that misses ONLY because of case is a typo, not +a dead rule, and the offline mode spares it exactly as the online mode does +— deleting it would silently un-own the files it was aimed at. Spared means +reported, and the run exits 4: a typo still needs a person. + +### `TestLintOffline_DryRunReportsThePendingRemovalAndWritesNothing` + +SPEC A-4/R-11 offline: the dry run is the CI-shaped half of the fix — the +pending dead-rule removal is reported at exit 4, the dead pattern is named, +nothing is written, and the output says the owner checks were skipped so +nobody reads the report as "the owners are fine too". + +### `TestLintOffline_InvalidLineFailsTheJSONGate` + +SPEC offline reporting (docs/LINTING.md's exit table): a line GitHub is +silently skipping is reported at exit 4 / needs_human even offline — +"syntactically broken" is a file fact, no API needed — so a CI gate on +`jq -e .needs_human` cannot go green over broken lines. + +### `TestLintOffline_JSONRecordCarriesTheDisclosure` + +SPEC --format json offline: the record carries the disclosure as a field, so +a script consuming a mixed fleet of online and offline records can tell +which ones say nothing about owners — prose in a note line cannot be jq'd. + +### `TestLintOffline_MalformedGitHubRepoIsInvalidEvenWithoutAToken` + +SPEC exit 3: a malformed --github-repo is a misspelled argument the operator +plainly meant to use, and it is diagnosed with or without a token — before +the fix, a garbage value with no token slid into the offline mode with the +flag silently ignored. + +### `TestLintOffline_OwnerRepairsAndRemovalsStayRefused` + +SPEC R-12 offline: the fail-closed contract for OWNERS is untouched. A split +handle (stage 1's repair) and a dead-looking owner (stage 2's removal) both +survive the offline run byte-for-byte while the dead PATTERN on another line +is removed — the run does the tree work without touching a single owner. The +broken line is REPORTED, not repaired: GitHub is skipping it, which is a +file fact, so the run exits 4 and names the credentialed run that can fix it. + +### `TestLintOffline_PartialCredentialsRefuseNamingWhatIsMissing` + +SPEC R-12: the offline mode engages only when NEITHER credential was +offered. A run that named a repo or held a token asked for the credentialed +lint; silently narrowing it to dead patterns would report success over owner +checks the operator believes ran. Refused at exit 5, naming exactly what is +absent — never the credential that was supplied. + +### `TestLintOffline_PolicyRefusalNamesThePolicyFieldNotTheBannedFlag` + +SPEC R-36b: the offline refusal's escape hatch is worded for how THIS run +was configured. --remove-stale-paths is exit-3-banned next to --policy, so +under --policy the remedy is the policy field, not the flag — the old advice +sent a policy-mode operator straight into a second refusal. + +### `TestLintOffline_PolicyRemoveStalePathsEnablesTreeOnlyMode` + +SPEC R-36a offline: `"remove_stale_paths": true` in the policy file's "lint" +block opts in to the tree-only mode exactly as the flag does — the reviewed +artifact IS the configuration, so the offline escape must not require a flag +the same run bans (R-36b). + +### `TestLintOffline_WithoutRemoveStalePathsStillRefuses` + +SPEC R-12: offline WITHOUT --remove-stale-paths keeps the exit-5 refusal — +there is no tree-only repair to run, and quietly doing nothing would report +success over a file full of owners nobody checked. The refusal now names the +one offline escape, so the operator who only wanted the dead rules gone is +told the flag instead of being told to find a token. + +### `TestLintOffline_WriteRemovesTheDeadRuleAndIsIdempotent` + +SPEC R-0 offline: the write path works too, through the same apply machinery +as every other write — and re-running over its own output is a no-op at exit +0 (clean is lint's success, never exit 1), so the mode is schedulable. + ### `TestLintVerb_CaseOnlyMissIsSparedNotDeleted` A rule that misses ONLY because of case is spared by --remove-stale-paths. @@ -3429,6 +3636,89 @@ Every verb, not just audit: a token in the environment must survive any usage render anywhere in the CLI. This is the regression guard for the day a second command grows a credential flag. +### `TestAuditJSONCleanIsPureJSON` + +Pre-release finding, fixed: `audit --format json` on a clean repo prints the literal line +"audit clean" after the JSON object, so the one case CI most wants to pipe +to jq — the healthy repo — is the one case the output isn't parseable. +Under `--format json`, stdout is data. + +### `TestAuditRejectsUnknownFormat` + +Pre-release finding, fixed: `audit` silently accepts an unknown `--format` and falls back to +text. sync, check, and lint all reject unknown formats at exit 3 — "never a +silent fallback to text" — and audit is documented with the same +`--format json|text` contract. + +### `TestBranchMismatchErrorNamesHeadCleanly` + +Pre-release finding, fixed: the S-7 branch-mismatch refusal interpolates raw `git rev-parse +--abbrev-ref --end-of-options HEAD` output, and rev-parse echoes the +`--end-of-options` operator as an output line — so the one-line error (and +the JSON record's error field) reads "HEAD is --end-of-options\nmain (…)". + +### `TestEscapedHashPatternRefused` + +Pre-release finding, fixed: a `\#`-escaped pattern is accepted and written, but S-6/S-2 says +GitHub honors no `\#` escape — on GitHub the written line is dead, so the +tool reports `proven: tree` for a rule that provably does not hold there. +The unescaped spelling `add_owner(#tag.md, …)` is already refused; the +escaped spelling must be refused too, and nothing written. + +### `TestFileFlagSpellingNoFalseGovernsNothing` + +Pre-release finding, fixed: `--file ./.github/CODEOWNERS` (or any uncleaned spelling of a +governing location) triggers a false "governs nothing" warning. trackedAt +cleans the spelling before matching the tracked file; the S-8 location +check compares the raw string, so a live change is reported as dead in the +warning, the --out record, and the --summary-out PR body. + +### `TestOfflineStaleRuleRemovalReportable` + +Pre-release finding (UAT), fixed: a tree-provably-dead rule cannot be repaired +offline. `lint --dry-run --remove-stale-paths` refuses everything at exit 5 +citing R-12 ("owner existence is not decidable offline") — but whether a +pattern matches zero tracked files is a git-tree fact the offline audit +(A-4/A-5) itself proves, no API needed. With --remove-stale-paths as the +requested repair, the dry run should report the pending dead-rule removal +(exit 4) instead of demanding a token; today the only offline remedy is the +hand edit the tool exists to prevent. + +### `TestPlanBelowRepoRootRefused` + +Pre-release finding, fixed: plan and apply skip the repo-root guard that sync enforces. +sync refuses `--repo ` because the CODEOWNERS it would write lands +at a path GitHub never reads (checkRepoRoot). plan happily plans against the +subtree and apply writes the dead file, reporting success — the "applied, +dead on arrival" outcome the guard exists to prevent. plan must refuse +exactly as sync does. + +### `TestPositionalArgsRejected` + +Pre-release finding, fixed: positional arguments are silently discarded, and every flag +after them with them. `audit ../other-repo --checks a999` (note the missing +--repo) audits the CWD with all defaults and exits 0 — the invalid +`--checks a999`, which the parser would reject loudly, is never seen. A +tool this strict about flag values must not swallow whole arguments. + +### `TestSetOwnersDisclosesAuthoredDuplicate` + +Pre-release finding, fixed: set_owners on a scope whose pattern already exists earlier in +the file authors a shadowed duplicate — the old line stays, dead under +last-match-wins but still naming its owners to human readers — and the run +that creates it says nothing. The R-7 duplicate warning fires only on the +NEXT run that touches the file. The run creating the duplicate must +disclose it. + +### `TestSymlinkedCodeownersNotSilentSuccess` + +Pre-release finding, fixed: a symlinked .github/CODEOWNERS inside the clone is written +through and reported applied with no warning. The tool's own docs state +GitHub does not follow a symlinked CODEOWNERS, so the run edited a file +that governs nothing while reporting success. An out-of-repo symlink target +is already refused (containedWritePath); the in-repo case must at minimum +not be a silent success. + ## internal/file **`file_test.go`** @@ -3745,6 +4035,28 @@ an error condition for the caller, never a merge. > ---------- no-op, byte preservation, reporting ---------- +**`offline_test.go`** + +> The tree-only mode behind offline `lint --remove-stale-paths` +> (Options.SkipOwnerChecks). +> +> R-12 makes owner existence — an API fact — undecidable offline, and the +> whole run fails closed on it. But whether a pattern matches zero tracked +> files is a git-TREE fact the offline audit (A-4/A-5) already proves, so a +> run that asks ONLY for the stale-path repair may run with no Verifier at +> all. The contract for that mode, pinned here: +> +> - stage 3 runs exactly as it does online — same staleness judgment, same +> A-5 sparing of case-only misses; +> - the owner WORK of stages 1 and 2 is skipped, not degraded: no owner is +> looked up, repaired, or removed, and no invalid line is rewritten; +> - stage 1's REPORTING still runs: an invalid line GitHub is skipping is a +> file fact, no API needed, so it is reported (NeedsHuman → exit 4) — a +> line the online run would repair with a reason that names the +> credentialed run, any other with the same reason as online; +> - SkipOwnerChecks without RemoveStalePaths is invalid input, because with +> owner work forbidden there is nothing left the run may do (R-11/R-12). + ### `TestBuild_ActionsCarryLineAndReason` SPEC R-16 (reporting): every action carries the 1-based line it happened on @@ -3907,6 +4219,57 @@ destroy it in the one commit an operator is least likely to read closely: the automated one. The correct outcome is a file that still contains the broken line and a report that says so. +### `TestOffline_CaseOnlyMissIsSparedAndNeedsAHuman` + +SPEC A-5/S-6 offline: a rule that matches nothing ONLY because of case is a +typo, not a dead rule, and the offline mode spares it with exactly the +online logic — deleting it would silently un-own the files it was aimed at. +Spared means reported (NeedsHuman), so the caller still exits 4. + +### `TestOffline_DeadRuleIsRemovedWithANilVerifier` + +SPEC A-4/R-11 offline: a rule whose pattern matches nothing tracked and +nothing on disk is deleted with a NIL Verifier. The nil is the proof that no +lookup can possibly have been made — an implementation that touched the +network here would panic, not pass. + +### `TestOffline_EmailOwnersAreNotReportedUnverifiable` + +SPEC R-13 offline: email owners are not looked up online either, but online +they are REPORTED as unverifiable — a statement about a check that ran +around them. Offline no owner check ran at all, so the report would imply +the rest of the file's owners were verified. Nothing is reported. + +### `TestOffline_OwnersAreNeverLookedUpOrRemoved` + +SPEC R-12 offline: owners are never touched, even one a lookup WOULD have +proven dead. Proven from the call log, not the output — an implementation +that asked and ignored the answer is one refactor from believing it. + +### `TestOffline_SkipWithoutRemoveStaleIsInvalid` + +SPEC R-11/R-12: SkipOwnerChecks without RemoveStalePaths is invalid input, +not a clean run. Stages 1 and 2 are owner work the mode forbids, and stage 3 +was not opted into — a "clean" from a run that checked nothing would be a +green check that means nothing. + +### `TestOffline_SplitHandleIsReportedNotRepaired` + +SPEC R-12 offline: stage 1's REPAIR is an owner repair — it puts a +previously skipped line, and the unverified owner on it, into force — so +offline the line is left byte-for-byte. But it IS reported: GitHub skipping +the line is a file fact, and a run that exits 0 over it goes green over rot. +The reason is honest about which run can fix it — a credentialed one, which +repairs it mechanically and verifies the reassembled owner. + +### `TestOffline_UnrepairableLineIsReportedSameAsOnline` + +SPEC offline reporting: a line no run can repair — `@keep /docs` is shaped +exactly like two rules on one line — is reported offline with the SAME +reason as online. GitHub skipping it is a file fact, and docs/LINTING.md +promises exit 4 for it; an offline exit 0 would let a CI gate on +.needs_human go green over a broken line. + ### `TestOwners_CaseFoldedForLookupButNotRewritten` Owner identity is case-folded for the lookup, because GitHub's is. @@ -3994,6 +4357,15 @@ Proven from the call log, not from the output: an implementation that asks about `docs@example.com` and merely ignores the reply is one refactor away from believing it. +### `TestRemoveDeadOwner_MixedCaseSpellingKeepsItsReason` + +SPEC R-38a in stage 2's record: a dead owner spelled @Org/Gone is the same +owner as @org/gone, so the lookup and the `dead` map are both case-folded — +but the Action.Reason was read back with the UNFOLDED spelling, an empty +reason on exactly the removals whose spelling differs from the fold. The +reason is what a reviewer approves the deletion on, so it must survive the +file's own capitalisation. + ### `TestRepairHandle_ConservesBytesAndProducesOnlyHandles` Property: a merge may only ever remove whitespace, and only from inside one @@ -4387,6 +4759,12 @@ paths like /apps/[param]/file.ts only match the literal bracket path). SPEC S-2 (differential): every case in the vendored hmarr/codeowners corpus must match GitHub's observed behavior exactly. +### `TestS2_EscapedLeadingHashRejected` + +SPEC S-2/S-6: GitHub honors no `\#` escape of a leading hash — a line +starting with '#' is always a comment there, so a `\#…` rule is dead on +GitHub. Same standard as `!`: a mutation tool must never accept or emit one. + ### `TestS2_GitHubDocExamples` SPEC S-2: examples taken verbatim from GitHub's "About code owners" docs. @@ -4396,6 +4774,12 @@ SPEC S-2: examples taken verbatim from GitHub's "About code owners" docs. Literal special characters observed accepted by GitHub (hmarr issues #47 caret, #50 tilde, #52 colon): they match themselves. +### `TestS2_MidPatternHashAccepted` + +Only the LEADING position is special-cased: a mid-pattern '#' needs no +escape at all, and a mid-pattern `\#` keeps the reference implementation's +literal-'#' meaning. + ### `TestS2_NegationRejected` SPEC S-2: negation is not supported. A pattern starting with `!` is a @@ -4807,6 +5191,14 @@ the white-box divergence case lives in settle_internal_test.go. SPEC R-7: with duplicate patterns, edit the EFFECTIVE (last) one and report the shadowed duplicate — never silently fix it. +### `TestR7_SetOwnersDisclosesAuthoredDuplicate` + +SPEC R-7, same-run case (pre-release finding): set_owners on a scope whose +pattern already exists earlier inserts after the last intersecting rule, +leaving the earlier byte-equal line permanently shadowed but still naming +its old owners to readers. The run that AUTHORS the duplicate must disclose +it — not only the next run that touches the file. + ### `TestR8_ConflictingBatchRejected` SPEC R-8: order-dependent overlapping batches are rejected, not resolved @@ -5316,6 +5708,23 @@ lack Terraform" is answerable only if each op reports itself, by id, in the order the policy lists them — a bare count cannot answer it, and a reordered list attributes the wrong outcome to the wrong op. +### `TestZeroMatch_UnrecognizedOnExceptZeroMatchIsInvalid` + +Same defense for the sibling switch: an unrecognized on_except_zero_match on +an op whose except bites nothing must be invalid input (exit 3) naming the +value — not silently run as `require`, whose exit-2 refusal reads as a +per-repo problem when the defect is in the policy and identical everywhere. + +### `TestZeroMatch_UnrecognizedOnZeroMatchIsInvalid` + +Regression guard from the pre-release review: an UNRECOGNIZED on_zero_match +value on a zero-match scope must refuse (exit 3), naming the value. Policy +parsing validates the enum, but the field is exported with a json tag, so a +library caller — or a value the policy layer learns before the planner does +— can carry anything. Before the fix no switch arm matched, the op +synthesized nothing, and the run reported the repo converged with a proven +tree: the silent no-op rollout this file exists to prevent. + ### `TestBasenameSpellingsSelectTheSameFiles` The equivalence the test above rests on: nothing in the planner is allowed to @@ -5805,6 +6214,13 @@ Observed before the fix, at exit 0 on a repo holding `my dir/x.txt` and > SPEC INV-3: resolution is computed over the actual tracked file tree, never > over the pattern set. +### `TestA8_EscapedLeadingHashLineIsInvalidAndSkipped` + +A pre-existing `\#…` line takes the same path as `!` negation: the pattern +no longer compiles (S-2/S-6 — GitHub reads such a line as a comment), so the +line is INVALID, skipped in resolution, and surfaced through the same A-8 +invalid-line reporting — never a rule this tool edits or counts on. + ### `TestA8_InvalidLinesDoNotResolve` Invalid lines are skipped during resolution (current GitHub semantics) — @@ -5864,4 +6280,4 @@ DIFFERENT states; transitioning between them is a real ownership change. --- -559 documented test cases across 13 packages. +610 documented test cases across 13 packages. diff --git a/docs/GUIDE.md b/docs/GUIDE.md new file mode 100644 index 0000000..a8cba82 --- /dev/null +++ b/docs/GUIDE.md @@ -0,0 +1,199 @@ +# Guide: making changes end to end + +Worked examples for each kind of change. Concepts: [README](../README.md#basic-concepts); +every flag and exit code: [REFERENCE.md](REFERENCE.md). + +## A basic example + +A small repo, with a `README.md` nobody owns in particular: + +``` +README.md +docs/guide.md +services/api/main.go +services/web/app.ts +``` + +```console +$ cat .github/CODEOWNERS +* @org/everyone +/services/api/ @org/api-team +``` + +Docs team should co-own the README — look before you leap: +```console +$ codeowners-tool sync --op 'add_owner(README.md, @org/docs-team)' --dry-run +applied: 1 op(s) applied, 0 skipped; 1 line change(s), 1 path(s) change owners + ops[0] applied (proven: tree) +``` + +`proven: tree` means the claim was checked against the repo's real files, not just +reasoned about. Drop `--dry-run` to write it: + +```console +$ codeowners-tool sync --op 'add_owner(README.md, @org/docs-team)' +applied: 1 op(s) applied, 0 skipped; 1 line change(s), 1 path(s) change owners + ops[0] applied (proven: tree) +$ cat .github/CODEOWNERS +* @org/everyone +README.md @org/everyone @org/docs-team +/services/api/ @org/api-team +``` + +Three things happened that are worth noticing: + +- `@org/everyone` was **carried onto the new line** — they owned `README.md` via `*`, + and `add_owner` means co-own, so the new rule restates them or they'd be dropped. +- The line went **in the middle, not at the end** — directly after the rule it narrows, + which is what keeps out-of-scope ownership (INV-2) untouched. +- `/services/api/` was **not touched at all**, including its original spacing. + +Run it again and nothing happens: the second run reports `unchanged`, zero bytes change. + +> **Pattern note.** `README.md` is unanchored, so like gitignore it matches a +> `README.md` at *any* depth. Write `/README.md` if you mean only the one at the root. + +## Writing a new CODEOWNERS file + +For a repo with no CODEOWNERS at all, `--create` grants permission to write one at +`.github/CODEOWNERS`. It never overwrites an existing file, it's off by default, and a +run with nothing to write creates nothing. The smallest version is one op: + +```console +$ codeowners-tool sync --op 'set_owners(*, [@org/everyone])' --create +applied: 1 op(s) applied, 0 skipped; 1 line change(s), 4 path(s) change owners + ops[0] applied (proven: tree) + created a new CODEOWNERS file +``` + +For a real file, put the ops in a policy so the whole shape is reviewable in one diff +(a policy run states `"create": true` in the file instead of the flag — +[why](REFERENCE.md#creating-a-file-r-23-and-not-creating-one)): + +```json +{ + "version": 1, + "name": "bootstrap ownership", + "create": true, + "ops": [ + "add_owner(*, @org/everyone)", + "add_owner(/services/api/, @org/api-team)", + "add_owner(/docs/, @org/docs-team)", + { "op": "add_owner(/.github/workflows/, @org/ci)", "on_zero_match": "declare" } + ] +} +``` + +```console +$ codeowners-tool check --policy bootstrap.json +ok: bootstrap.json — 4 op(s), no policy errors +$ codeowners-tool sync --policy bootstrap.json +applied: 4 op(s) applied, 0 skipped; 4 line change(s), 4 path(s) change owners + created a new CODEOWNERS file +``` + +Two things that will bite you on the first try: + +- **Use `add_owner` for the catch-all, not `set_owners`.** `add_owner` ops commute, so + any number can share one run. `set_owners(*, …)` overlaps every other scope and does + not commute with them, so the batch is refused at exit 3 (R-8) — run it on its own + first, previewed with `--dry-run`, since it *replaces* the owners of everything. +- **A rule for files that don't exist yet needs `on_zero_match: "declare"`.** The + default `require` treats a scope matching nothing as a problem with this repo, + because it usually is a typo. `declare` writes the rule at the end of the file for + files added later, and reports `proven: structural` — see + [what `declare` costs](REFERENCE.md#what-declare-costs). + +## Modifying an existing file + +Same command; the interesting part is what it protects you from. Starting from the +two-line file above, each row is one run, and the second column is the line it leaves +behind — original spacing intact: + +| Run | `/services/api/` afterwards | +|---|---| +| `sync --op 'add_owner(/services/api/, @org/platform)'` | `/services/api/ @org/api-team @org/platform` | +| `sync --op 'set_owners(/services/api/, [@org/platform, @org/api-team])'` | `/services/api/ @org/platform @org/api-team` | +| `sync --op 'rename_owner(@org/api-team, @org/platform-api)'` | `/services/api/ @org/platform-api` | + +The first is the common case and the one hand-editing gets wrong. The second is the +same edit stated deliberately. The third is what a reorg needs — a global identifier +substitution that can't change any rule's match set. + +**Removing an owner** stops and asks when it would empty a rule's owner set — there is +deliberately no default: + +```console +$ codeowners-tool sync --op 'remove_owner(/services/api/, @org/api-team)' +error: removing @org/api-team empties the owner set of "/services/api/"; an explicit --on-empty policy (error|inherit|unowned) is required — there is deliberately no default (R-6) (governing file: .github/CODEOWNERS) +``` + +`--on-empty inherit` deletes the rule and lets the preceding broader one take over; +`unowned` keeps the pattern with zero owners (GitHub's sanctioned substitute for `!` +negation); `error` refuses outright, and is the recommendation. + +## Reviewing the change before it lands + +`sync` is plan-assert-apply-validate in one step. Split it when you want the artifact +in the middle — a JSON plan with resolved ownership per path and the literal line diff: + +```console +$ codeowners-tool plan --op 'add_owner(/services/web/, @org/web-team)' --out plan.json +plan written to plan.json +1 line change(s), 1 path(s) change owners, 58 → 101 bytes +$ jq '.ownership_rows, .diff' plan.json +[ + { + "path": "services/web/app.ts", + "owners_before": ["@org/everyone"], + "owners_after": ["@org/everyone", "@org/web-team"] + } +] +"@ line 2\n+/services/web/ @org/everyone @org/web-team\n" +$ codeowners-tool apply --plan plan.json +applied: .github/CODEOWNERS (58 → 101 bytes) +``` + +Every change carries the reason it took that shape — the part a reviewer actually +wants. And to prove after the fact that a merged change moved nothing it didn't +declare: + +```sh +codeowners-tool snapshot --branch main --out before.json +codeowners-tool snapshot --branch feature --out after.json +codeowners-tool verify --before before.json --after after.json --scope /services/api/ +``` + +Two hygiene rules make that proof trustworthy. `snapshot` reads the **committed** +CODEOWNERS at `--branch` (default `HEAD`) — so commit the change before taking the +"after" snapshot. And leave the evidence files themselves uncommitted: a path that +enters the tracked tree counts as an ownership change, so `verify` will rightly flag +your own `before.json` as out of scope. + +## When it refuses + +Sometimes there is no line that does what you asked and nothing else. Given +`infra/main.tf` and `infra/README.md`, and a CODEOWNERS of exactly `infra/ +@org/infra-legacy`: + +```console +$ codeowners-tool sync --op 'add_owner(**/*.tf, @org/infra)' +error: refusing: rule "infra/" also governs paths outside scope "**/*.tf", and no sound narrowing pattern is derivable — amending would violate INV-2, appending would violate INV-1 (governing file: .github/CODEOWNERS) +``` + +In English: `infra/` covers the `.tf` file *and* the README. Editing that line would +change the README's owners, which you never asked for (INV-2). Adding a `**/*.tf` line +before it would be overridden by it (INV-1). So it stops. + +This is a normal outcome for some repos, not a bug — the tool fails closed rather than +guessing. Two ways out, both stated in the op itself: + +- **Narrow the scope** to something a sound rule *can* be written for — the concrete + paths, or a directory-local glob: `add_owner(/infra/*.tf, @org/infra)`. +- **Carve the conflicting paths out** with an `except` clause: + `add_owner(infra/ except infra/README.md, @org/infra)` — see [except.md](except.md). + +(A same-owners `set_owners` does not help: it changes no ownership, so the tool reports +`unchanged` and writes nothing.) Exit `2` means *this repo* needs a human; exit `3` means +*the policy* is broken and will fail identically everywhere — the split that makes a +hundred-repo run survivable ([FLEET.md](FLEET.md)). diff --git a/docs/LINTING.md b/docs/LINTING.md index ba89626..374776e 100644 --- a/docs/LINTING.md +++ b/docs/LINTING.md @@ -7,7 +7,7 @@ Two commands, and the difference between them is whether they write: | What it does | Reports 12 checks (A-1 … A-12) | Repairs 3 of them | | Writes? | **Never** | Yes, unless `--dry-run` | | Reads | the file committed at `--branch` | the file **in your working tree** | -| Needs a token? | Only for the owner checks | **Always** | +| Needs a token? | Only for the owner checks | Yes, bar one offline mode ([below](#errors-you-will-actually-hit)) | `audit --lint` is the older spelling of `lint` and still works — same code path. @@ -39,7 +39,7 @@ Drop `--dry-run` to write it. Every flag is in | Flag | Why | |---|---| | `--dry-run` | Report, write nothing. Exit 4 if anything is pending. | -| `--github-repo owner/name` | Required. Proves the token can see this repo. | +| `--github-repo owner/name` | Required, bar the offline mode. Proves the token can see this repo. | | `--remove-stale-paths` | Also delete rules matching no files. Off by default. | | `--on-empty error\|inherit\|unowned` | Required *only* if a removal would empty a rule. | | `--file PATH` | For a CODEOWNERS you have not committed yet. | @@ -125,7 +125,7 @@ One rule: **0 when the file needs nothing further from a person, 4 when it does. | 2 | Refused — `--on-empty=error`, the size cap, wrong `--branch`, or `--repo` below the repo root | | 3 | Invalid input — a missing `--on-empty`, a bad flag combination, or the file changed under the run | | 4 | Still needs a person — pending fixes under `--dry-run`, an unrepairable line, or a case-only typo | -| 5 | Inconclusive — a lookup could not be answered, or no token. **Nothing written** | +| 5 | Inconclusive — a lookup could not be answered, or credentials missing (bar the offline mode below). **Nothing written** | | 6 | Post-write validation failed; rolled back | `lint` never returns 1. A file needing no repair is this command's success, and exiting @@ -134,7 +134,12 @@ One rule: **0 when the file needs nothing further from a person, 4 when it does. ## Errors you will actually hit **`lint needs a token … and --github-repo`** (exit 5). Owner existence is not decidable -offline, and that is the whole point of the command. It names whichever one you left out. +offline; the refusal names whichever credential you left out. One exception: with +`--remove-stale-paths` and *neither* credential given, the run proceeds offline and does +stage 3 alone — dead rules are removed against the tree, invalid lines are still reported +(exit 4), and no owner is verified, repaired, or removed; the skip is disclosed in the +output and as `owner_checks_skipped` in the JSON record. Supplying one credential without +the other still refuses — a run that named a repo or held a token asked for the full lint. **`inconclusive: … no owner was removed and nothing was written`** (exit 5). One lookup could not be answered — rate limit, expired token, an org your token cannot enumerate — diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index dc8b5f2..f1e3be4 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -36,7 +36,7 @@ audit [--checks a1,a3,a6] [--fail-on any|warning|error|never] [--format json| [--cache-dir D] [--cache-ttl DUR] [--repo DIR] [--branch REF] [--file PATH] lint --github-repo owner/name [--token T | $GITHUB_TOKEN] [--api-url URL] [--remove-stale-paths] [--on-empty error|inherit|unowned] [--dry-run] - [--repo DIR] [--branch REF] [--file PATH] [--format text|json] + [--policy FILE] [--repo DIR] [--branch REF] [--file PATH] [--format text|json] snapshot [--repo DIR] [--branch REF] [--out snap.json] verify --before before.json --after after.json [--scope PATTERN ...] version print the build this binary was stamped with @@ -272,15 +272,28 @@ codeowners-tool snapshot --branch feature --out after.json codeowners-tool verify --before before.json --after after.json --scope /services/api/ ``` +## `snapshot` and `verify` + +`snapshot` resolves the CODEOWNERS **committed at `--branch`** (default `HEAD`) against +that ref's tracked tree — an uncommitted edit is invisible to it, exactly as it is to +GitHub. In the `ownership` map, `[]` means a rule matches the path and deliberately +assigns no owners; `null` means no rule matches it at all. + +`verify` compares two snapshots and exits `0` when every ownership change falls inside +a declared `--scope` (repeatable), `2` — printing each offending path — when any change +falls outside them (with no `--scope`, any change at all violates), and `3` for a +malformed snapshot. A path that enters or leaves the tracked tree counts as a change +(R-18), so don't commit the snapshot files themselves between the two snapshots. + ## Operations Scope is a directory, file path, or glob — same syntax as CODEOWNERS patterns. | Op | Meaning | |---|---| -| `add_owner(scope, owner)` | Owner becomes a **co-owner**; every pre-existing owner of every path in scope is retained. | +| `add_owner(scope, owner)` | Owner — or a bracketed list `[a, b]` (R-33) — becomes a **co-owner**; every pre-existing owner of every path in scope is retained. | | `set_owners(scope, [owners])` | Exact owner set for every path in scope, displacing prior owners. `[]` is legal: it deliberately un-owns the scope. | -| `remove_owner(scope, owner)` | Owner stops owning every path in scope. If a rule's owner set would empty, an `--on-empty` policy is **required**. | +| `remove_owner(scope, owner)` | Owner — or a bracketed list (R-33) — stops owning every path in scope. If a rule's owner set would empty, an `--on-empty` policy is **required**. | | `rename_owner(old, new)` | Global identifier substitution — the only op safe as pure text replacement (it can't change any rule's match set). | The scope of `add_owner`, `set_owners` and `remove_owner` may carry an `except` @@ -405,7 +418,7 @@ Under `inherit`/`unowned` the resulting reassignment is shown in the plan's owne ## Audit checks -Read-only **except `audit --lint`** ([below](#audit---lint)). Plain `audit` never writes — +Read-only **except `audit --lint`** ([below](#lint)). Plain `audit` never writes — where a fix is expressible it emits op strings for a human to review and run through `plan`/`apply`. Even under `--lint` the bytes reach disk only through `apply`, which remains the system's single writer path. @@ -470,8 +483,8 @@ rejected, because a subset of a whole-file repair is ambiguous rather than small | Flag | Meaning | |---|---| -| `--github-repo owner/name` | **Required.** Probed, so a token that cannot see the repo stops the run. | -| `--token` / `$GITHUB_TOKEN` | **Required.** Owner existence is not decidable offline. | +| `--github-repo owner/name` | **Required**, bar the offline mode below. Probed, so a token that cannot see the repo stops the run. | +| `--token` / `$GITHUB_TOKEN` | **Required**, bar the offline mode below. Owner existence is not decidable offline. | | `--dry-run` | Compute and report; write nothing. Exit 4 if anything is pending. | | `--remove-stale-paths` | Stage 3. Deletes rules matching nothing tracked **and** nothing on disk. | | `--on-empty error\|inherit\|unowned` | R-6, required only when a removal would empty a rule. `inherit` deletes the line. | @@ -482,6 +495,11 @@ rejected, because a subset of a whole-file repair is ambiguous rather than small revalidation, and here that answer deletes an owner rather than printing a finding. Lookups are still cached in memory per run. +**Offline tree-only mode:** with `--remove-stale-paths` and *neither* a token nor +`--github-repo`, the run does stage 3 alone — dead rules judged against the tree, invalid +lines still reported at exit 4, no owner verified, repaired, or removed — and the skip is +disclosed. One credential without the other still refuses at exit 5. + | # | Stage | Opt-in | What it does | |---|---|---|---| | 1 | Repair owner spacing | no | Rejoins an `@`handle split by whitespace: `@ org/team`, `@org/ team`, `@ org / team` → `@org/team`. Runs **before** stage 2 — those are one owner nobody has looked up, not two that are missing. | @@ -511,7 +529,7 @@ when the two agree. | 2 | Refused — `--on-empty=error`, size cap, or either repository guard | | 3 | Invalid input — missing `--on-empty`, a rejected flag, an empty tree under `--remove-stale-paths`, or hash drift between read and write | | 4 | Still needs a person — pending fixes under `--dry-run`, an unrepairable line, or a case-only typo | -| 5 | Inconclusive, or no token/`--github-repo`. Nothing written | +| 5 | Inconclusive, or missing credentials (bar the offline mode above). Nothing written | | 6 | Post-write validation failed; rolled back | `lint` never returns 1: a file needing no repair is its success, and "no-op" would make @@ -521,6 +539,8 @@ every healthy repository in a fleet read as a failure under `set -e`. `exit_code`, `actions[]` (`kind`, `line`, `owner`, `pattern`, `reason`), `unverifiable[]`, `changes[]`, `ownership_rows[]`, `diff`, `warnings[]`. `needs_human` and `exit_code` come from the same function that sets the process status, so `jq -e .needs_human` is the gate. +An offline run additionally carries `owner_checks_skipped: true` — the record says +nothing about whether the owners exist. Unlike the `sync` record, `actions`, `changes` and `ownership_rows` are always present (possibly empty). diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 491086d..b7e4221 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -78,6 +78,22 @@ func flagParseCode(err error) int { return ExitInvalid } +// rejectLeftoverArgs refuses positional arguments, which no verb here takes. +// +// Go's flag package stops parsing at the first non-flag token, so `audit +// ../other-repo --checks a999` silently dropped BOTH the stray path and every +// flag after it: the run audited the CWD with all defaults and exited 0, and +// the invalid --checks the parser would have rejected loudly was never seen. +// A tool this strict about flag values must not swallow whole arguments. +// Argument-only and repo-independent, so it is exit 3 in every verb. +func rejectLeftoverArgs(fs *flag.FlagSet) error { + if fs.NArg() == 0 { + return nil + } + return fmt.Errorf("unexpected argument %q: %s takes every input as a flag (--repo DIR, not a positional path), and the parser reads nothing after a positional argument — flags included", + fs.Arg(0), fs.Name()) +} + // isFlagSet reports whether the operator actually PASSED a flag, as opposed to // inheriting its default. --cache-ttl has a non-zero default, so "did you ask // for this?" cannot be read off its value. @@ -325,10 +341,10 @@ func containedRelPath(p string) error { // fleet verbs exist to prevent. // // The check lives here rather than in apply.Apply because apply.Apply has no -// notion of a repository: it is handed one path and one plan, and legitimately -// follows a link within the repo (a CODEOWNERS symlinked to docs/OWNERS is a -// real layout). The CLI is the layer that knows --repo, so it is the layer that -// can tell "inside" from "outside". +// notion of a repository: it is handed one path and one plan, and follows +// whatever link it is handed. The CLI is the layer that knows --repo, so it is +// the layer that can tell "inside" from "outside" — and the layer that refuses +// a link even when it stays inside (see refuseSymlinkedTarget). // // Both sides are resolved before comparison, for the same reason checkRepoRoot // resolves both: on macOS t.TempDir() hands out /var/folders/... while the real @@ -417,6 +433,84 @@ func resolveExistingPrefix(dir string) string { } } +// refuseSymlinkedTarget refuses to write a CODEOWNERS that is a symlink, or +// that sits BEHIND one — any component of the write path below the repository +// root, wherever the link points. +// +// containedWritePath keeps the write INSIDE the clone; this closes the cases +// it lets through. The final component: GitHub does not follow a symlinked +// CODEOWNERS, so writing through one edits a file that governs nothing while +// reporting success — the "applied, dead on arrival" outcome the write verbs +// exist to prevent, only with the dead file hidden behind a live-looking path. +// A PARENT component is the same outcome one level up: git commits a symlinked +// directory as a link BLOB, not a tree, so with `.github -> real-gh` there is +// no `.github/CODEOWNERS` in the tree GitHub reads, and Lstat'ing only the +// final component (a real file, reached through the link) let sync, apply and +// lint all write through it at exit 0. Refusal over a warning, matching the +// tool's fail-closed posture — a warning under a fleet's `--format json >> +// results` is a count nobody reads. +// +// Only the components of the write path below the repository root are +// Lstat'ed, so a symlink anywhere else in the repository stays irrelevant, an +// absent component (the --create case) has no link to refuse, and a --repo +// reached through links above the root (macOS /var -> /private/var) stays +// legitimate. The walk is bounded by the path's own depth. +func refuseSymlinkedTarget(target string) error { + // Abs can only fail when the CWD is gone; degrade to the raw spelling — + // the walk then covers the final component, the pre-walk behavior. + abs, err := filepath.Abs(target) + if err != nil { + abs = filepath.Clean(target) + } + for _, comp := range writePathComponents(abs) { + fi, err := os.Lstat(comp) + if err != nil || fi.Mode()&os.ModeSymlink == 0 { + continue + } + dest, err := os.Readlink(comp) + if err != nil { + dest = "(unreadable link target)" + } + if comp != abs { + return &plan.RefusalError{Msg: fmt.Sprintf( + "refusing to write %s: %s is a symlink to %s, and git records a symlinked directory as a link, not a tree, so no CODEOWNERS exists behind it at the path GitHub reads — the write would edit a file that governs nothing while reporting applied; replace the link with a real directory — nothing was written", + filepath.ToSlash(target), filepath.ToSlash(comp), filepath.ToSlash(dest))} + } + return &plan.RefusalError{Msg: fmt.Sprintf( + "refusing to write %s: it is a symlink to %s, and GitHub does not follow a symlinked CODEOWNERS, so the write would edit a file that governs nothing while reporting applied; replace the link with a real file — nothing was written", + filepath.ToSlash(target), filepath.ToSlash(dest))} + } + return nil +} + +// writePathComponents lists abs and every directory between it and the +// enclosing repository's root, root EXCLUSIVE — those are exactly the +// components a committed symlink can occupy, and the ones GitHub resolves +// through the tracked tree. +// +// The root is found lexically — the nearest ancestor holding a .git entry — +// rather than by asking git, because rev-parse resolves symlinks in its answer +// while this walk must stay on the spelling the caller joined --repo and the +// repo-relative path into; a lexical boundary is what keeps everything at or +// above the root uninspected. Every caller has already passed checkRepoRoot, +// so that ancestor IS the --repo the write is contained to. Found no root at +// all, the walk degrades to the final component alone — the pre-walk behavior, +// and the only thing decidable without a boundary. +func writePathComponents(abs string) []string { + var comps []string + for dir := abs; ; { + parent := filepath.Dir(dir) + if parent == dir { + return []string{abs} + } + comps = append([]string{dir}, comps...) + if _, err := os.Stat(filepath.Join(parent, ".git")); err == nil { + return comps + } + dir = parent + } +} + type multiFlag []string func (m *multiFlag) String() string { return strings.Join(*m, ",") } @@ -438,6 +532,10 @@ func cmdPlan(args []string, stdout, stderr io.Writer) int { if err := fs.Parse(args); err != nil { return flagParseCode(err) } + if err := rejectLeftoverArgs(fs); err != nil { + fmt.Fprintln(stderr, "error:", err) + return ExitInvalid + } if len(opSpecs) == 0 { fmt.Fprintln(stderr, "error: at least one --op is required") return ExitInvalid @@ -446,6 +544,15 @@ func cmdPlan(args []string, stdout, stderr io.Writer) int { if err != nil { return errExit(&plan.InvalidError{Msg: err.Error()}, stderr) } + // The same repo-root guard `sync` enforces (checkRepoRoot), for the verb + // that produces the ARTIFACT: pointed below the root, discovery resolves + // against the ROOT's tree while codeowners_path joins onto the + // subdirectory, so the plan describes an edit at a path GitHub never reads + // — and a human approves it before any downstream refusal fires. Refused + // before --out exists: a refused run writes nothing. + if err := checkRepoRoot(*repo); err != nil { + return errExit(&plan.RefusalError{Msg: err.Error()}, stderr) + } tree, coPath, _, err := locate(*repo, *branch, *filePath) if err != nil { return errExit(err, stderr) @@ -491,6 +598,10 @@ func cmdApply(args []string, stdout, stderr io.Writer) int { if err := fs.Parse(args); err != nil { return flagParseCode(err) } + if err := rejectLeftoverArgs(fs); err != nil { + fmt.Fprintln(stderr, "error:", err) + return ExitInvalid + } if *planPath == "" { fmt.Fprintln(stderr, "error: --plan is required") return ExitInvalid @@ -507,6 +618,14 @@ func cmdApply(args []string, stdout, stderr io.Writer) int { if *repo != "" { repoDir = *repo } + // The same repo-root guard `sync` enforces (checkRepoRoot), re-checked at + // apply time for the same reason containment is: --repo can point this + // verb at a different clone than the one planned against, and pointed + // below the root the join below addresses a file at a path GitHub never + // reads while the file that governs stays untouched. + if err := checkRepoRoot(repoDir); err != nil { + return errExit(&plan.RefusalError{Msg: err.Error()}, stderr) + } target := filepath.Join(repoDir, filepath.FromSlash(pf.CodeownersPath)) // The same containment `sync` enforces, enforced again here. A plan is an // artifact: it is reviewed in one place and applied somewhere else entirely, @@ -515,6 +634,11 @@ func cmdApply(args []string, stdout, stderr io.Writer) int { if err := containedWritePath(repoDir, target); err != nil { return errExit(err, stderr) } + // And the symlink refusal those two do not cover: a link that stays inside + // the clone still lands the write on a file GitHub never reads. + if err := refuseSymlinkedTarget(target); err != nil { + return errExit(err, stderr) + } if err := apply.Apply(&pf.Plan, target); err != nil { return errExit(err, stderr) } @@ -532,6 +656,10 @@ func cmdSnapshot(args []string, stdout, stderr io.Writer) int { if err := fs.Parse(args); err != nil { return flagParseCode(err) } + if err := rejectLeftoverArgs(fs); err != nil { + fmt.Fprintln(stderr, "error:", err) + return ExitInvalid + } tree, coPath, _, err := locate(*repo, *branch, *filePath) if err != nil { return errExit(err, stderr) @@ -569,6 +697,10 @@ func cmdVerify(args []string, stdout, stderr io.Writer) int { if err := fs.Parse(args); err != nil { return flagParseCode(err) } + if err := rejectLeftoverArgs(fs); err != nil { + fmt.Fprintln(stderr, "error:", err) + return ExitInvalid + } if *beforePath == "" || *afterPath == "" { fmt.Fprintln(stderr, "error: --before and --after are required") return ExitInvalid @@ -627,6 +759,17 @@ func cmdAudit(args []string, stdout, stderr io.Writer) int { if err := fs.Parse(args); err != nil { return flagParseCode(err) } + if err := rejectLeftoverArgs(fs); err != nil { + fmt.Fprintln(stderr, "error:", err) + return ExitInvalid + } + // The same validation sync, check and lint apply, and for the same reason: + // never a silent fallback to text. `--format jsn` fell back to prose here, + // and the CI step piping stdout to jq failed on precisely the healthy runs. + if *format != "text" && *format != "json" { + fmt.Fprintf(stderr, "error: unknown --format %q; want text or json\n", *format) + return ExitInvalid + } // The three lint-only flags are rejected rather than ignored when --lint is // absent. `audit --remove-stale-paths` silently reporting instead of // deleting is the shape of mistake that only surfaces months later, when @@ -779,7 +922,12 @@ func cmdAudit(args []string, stdout, stderr io.Writer) int { len(rep.Findings), *failOn) return ExitOK } - fmt.Fprintln(stdout, "audit clean") + // The verdict line is text-mode furniture. Under --format json, stdout is + // exactly one JSON document — printing it after the object broke `| jq` on + // precisely the run CI most wants to pipe: the healthy repo. + if *format != "json" { + fmt.Fprintln(stdout, "audit clean") + } return ExitOK } diff --git a/internal/cli/lint.go b/internal/cli/lint.go index a42d7cd..7a9bd23 100644 --- a/internal/cli/lint.go +++ b/internal/cli/lint.go @@ -48,7 +48,11 @@ func errReason(err error) string { // - It requires a token and --github-repo. Whether an owner exists is not // decidable offline, and stage 2 is the point of the mode; running it // without the ability to answer that question would silently degrade to a -// whitespace tidy while reporting success. +// whitespace tidy while reporting success. One carve-out: with +// --remove-stale-paths and NEITHER credential given, an offline run may +// still make the ONE repair that is a tree fact rather than an API fact — +// deleting dead patterns — with owner checks skipped and disclosed +// (see runLint). type lintRun struct { repo, branch, filePath string githubRepo, token string @@ -60,6 +64,11 @@ type lintRun struct { removeStale bool onEmpty string dryRun bool + // offline marks the tree-only mode: NEITHER a token nor --github-repo was + // given, --remove-stale-paths requested. Set inside runLint, never by + // callers — it is a fact about the run (which credentials were absent), + // not a flag. Partial credentials never reach it: they refuse at exit 5. + offline bool // policy records that a --policy file configured this run, not flags. It // governs only how a refusal is WORDED: the remedy for an unset R-6 policy // is a field in the file here and a flag under `audit --lint`, and naming @@ -77,14 +86,17 @@ type lintDoc struct { // process status, so a CI gate is `jq -e .needs_human` rather than a // two-clause expression that has to know the string "unrepairable-line" — // which a reviewer got wrong on the first try, going green over rot. - NeedsHuman bool `json:"needs_human"` - ExitCode int `json:"exit_code"` - Actions []lint.Action `json:"actions"` - Unverifiable []string `json:"unverifiable,omitempty"` - Changes []plan.Change `json:"changes"` - Rows []plan.Row `json:"ownership_rows"` - Diff string `json:"diff"` - Warnings []string `json:"warnings,omitempty"` + NeedsHuman bool `json:"needs_human"` + ExitCode int `json:"exit_code"` + // OwnerChecksSkipped marks an offline run: only dead patterns were + // judged, and nothing here says the owners exist (R-12). + OwnerChecksSkipped bool `json:"owner_checks_skipped,omitempty"` + Actions []lint.Action `json:"actions"` + Unverifiable []string `json:"unverifiable,omitempty"` + Changes []plan.Change `json:"changes"` + Rows []plan.Row `json:"ownership_rows"` + Diff string `json:"diff"` + Warnings []string `json:"warnings,omitempty"` } // cmdLint is the `lint` verb: the same run, with a flagset that contains only @@ -118,6 +130,10 @@ func cmdLint(args []string, stdout, stderr io.Writer) int { if err := fs.Parse(args); err != nil { return flagParseCode(err) } + if err := rejectLeftoverArgs(fs); err != nil { + fmt.Fprintln(stderr, "error:", err) + return ExitInvalid + } removeStalePaths, onEmptyPolicy := *removeStale, *onEmpty if len(policyPaths) > 0 { var err error @@ -254,13 +270,46 @@ func runLint(r lintRun, stdout, stderr io.Writer) int { if r.githubRepo == "" { missing = append(missing, "--github-repo owner/name") } - if len(missing) > 0 { - fmt.Fprintf(stderr, "error: lint needs %s. Whether an owner still exists is not decidable offline, and removing owners on a guess is what R-12 forbids — nothing was written.\n", strings.Join(missing, " and ")) - return ExitInconclusive + // Checked whenever the flag was TYPED, token or no token: a malformed + // --github-repo is a misspelled argument the operator plainly meant to use, + // and letting the credential refusal (or the offline mode) speak first + // would silently ignore the value they typed. + if r.githubRepo != "" { + if len(strings.SplitN(r.githubRepo, "/", 2)) != 2 || strings.Count(r.githubRepo, "/") != 1 { + fmt.Fprintln(stderr, "error: --github-repo must be owner/name") + return ExitInvalid + } } - if len(strings.SplitN(r.githubRepo, "/", 2)) != 2 || strings.Count(r.githubRepo, "/") != 1 { - fmt.Fprintln(stderr, "error: --github-repo must be owner/name") - return ExitInvalid + // One narrow carve-out: R-12 is about OWNER existence, an API fact. Whether + // a pattern matches zero tracked files is a git-TREE fact the offline audit + // (A-4/A-5) itself proves, so when --remove-stale-paths names that repair + // as what this run is for, the run proceeds OFFLINE — stage 3 alone, owner + // checks skipped and said so, no owner removed, verified, or repaired. + // + // Only when NEITHER credential was offered. A run that named a repo or + // held a token asked for the credentialed lint, and silently narrowing it + // to dead patterns would report success over owner checks the operator + // believes ran — so partial credentials refuse below, naming what is + // absent, exactly as before the mode existed. + r.offline = r.token == "" && r.githubRepo == "" + switch { + case r.offline && !r.removeStale: + // The escape hatch is named in the operator's own dialect: the flag is + // exit-3-banned next to --policy (R-36b), so pointing a policy-mode + // run at it would send them straight into a second refusal. + hatch := "pass --remove-stale-paths" + if r.policy { + hatch = `set "remove_stale_paths": true in the policy file's "lint" block` + } + fmt.Fprintf(stderr, "error: lint needs %s. Whether an owner still exists is not decidable offline, and removing owners on a guess is what R-12 forbids — nothing was written. The one repair decidable offline is removing dead patterns (a tree fact); %s to run that repair alone, with owner checks skipped.\n", strings.Join(missing, " and "), hatch) + return ExitInconclusive + case !r.offline && len(missing) > 0: + msg := fmt.Sprintf("error: lint needs %s. Whether an owner still exists is not decidable offline, and removing owners on a guess is what R-12 forbids — nothing was written.", strings.Join(missing, " and ")) + if r.removeStale { + msg += " The tree-only offline mode (dead-pattern removal alone) engages only when neither a token nor --github-repo is given — supply what is missing, or drop the one you passed." + } + fmt.Fprintln(stderr, msg) + return ExitInconclusive } // The disk cache is refused rather than used, and this is the one place // where lint must NOT inherit audit's behavior. @@ -318,6 +367,12 @@ func runLint(r lintRun, stdout, stderr io.Writer) int { if err := containedWritePath(r.repo, target); err != nil { return errExit(err, stderr) } + // Same refusal as sync and apply: a link that stays INSIDE the clone is + // still a CODEOWNERS GitHub does not follow, so a repair written through it + // repairs a file that governs nothing. See refuseSymlinkedTarget. + if err := refuseSymlinkedTarget(target); err != nil { + return errExit(err, stderr) + } content, err := os.ReadFile(target) if err != nil { return errExit(&plan.InvalidError{Msg: err.Error()}, stderr) @@ -337,24 +392,32 @@ func runLint(r lintRun, stdout, stderr io.Writer) int { } } - client := ghapi.New(r.apiURL, r.token, ghapi.NewMemCache()) - // --github-repo is a precondition, not decoration: it establishes that this - // token can see the repository whose CODEOWNERS is about to be rewritten. - // Without it the flag was required and then never used — the org for every - // team lookup comes from the owner token itself — so a token with no - // relationship to this repo ran to completion, and an operator asking "why - // did lint delete my team" had no signal that the credential was wrong. - // Fails closed, like every other lookup here. - owner, name, _ := strings.Cut(r.githubRepo, "/") - if err := client.ProbeRepo(owner, name); err != nil { - fmt.Fprintf(stderr, "error: cannot confirm this token can see %s (%s) — refusing to remove owners on a credential whose access to the repository is unproven (R-12); nothing was written\n", r.githubRepo, errReason(err)) - return ExitInconclusive + // Offline the client is never built and ProbeRepo never runs: there is no + // credential to prove anything with, and the run removes no owners for it + // to vouch for — lint.Build never calls the Verifier under SkipOwnerChecks. + var verifier lint.Verifier + if !r.offline { + client := ghapi.New(r.apiURL, r.token, ghapi.NewMemCache()) + // --github-repo is a precondition, not decoration: it establishes that this + // token can see the repository whose CODEOWNERS is about to be rewritten. + // Without it the flag was required and then never used — the org for every + // team lookup comes from the owner token itself — so a token with no + // relationship to this repo ran to completion, and an operator asking "why + // did lint delete my team" had no signal that the credential was wrong. + // Fails closed, like every other lookup here. + owner, name, _ := strings.Cut(r.githubRepo, "/") + if err := client.ProbeRepo(owner, name); err != nil { + fmt.Fprintf(stderr, "error: cannot confirm this token can see %s (%s) — refusing to remove owners on a credential whose access to the repository is unproven (R-12); nothing was written\n", r.githubRepo, errReason(err)) + return ExitInconclusive + } + verifier = client } - res, buildErr := lint.Build(content, tree, client, lint.Options{ + res, buildErr := lint.Build(content, tree, verifier, lint.Options{ RemoveStalePaths: r.removeStale, WorkTree: workTree, OnEmpty: r.onEmpty, + SkipOwnerChecks: r.offline, }) var inconclusive *lint.InconclusiveError @@ -423,7 +486,8 @@ func emitLint(res *lint.Result, coPath string, applied bool, r lintRun, stdout i doc := lintDoc{ Path: coPath, Applied: applied, DryRun: r.dryRun, NeedsHuman: code == ExitFindings, ExitCode: code, - Actions: res.Actions, Unverifiable: res.Unverifiable, + OwnerChecksSkipped: r.offline, + Actions: res.Actions, Unverifiable: res.Unverifiable, Changes: res.Plan.Changes, Rows: res.Plan.Rows, Diff: res.Plan.Diff, Warnings: res.Plan.Warnings, } @@ -448,6 +512,11 @@ func emitLint(res *lint.Result, coPath string, applied bool, r lintRun, stdout i // file is fine directly above a nonzero exit code is the one output // that makes a CI log read green over a red result. fmt.Fprintf(stdout, "lint: nothing to repair automatically in %s, but %d line(s) need a person\n", coPath, stuck) + case n == 0 && r.offline: + // The offline mode's success claim is narrower than lint's, and the + // headline must not borrow the wider one: "owners that do not exist" + // names a check this run skipped. + fmt.Fprintf(stdout, "lint: nothing to remove in %s — this offline run covered only dead patterns (--remove-stale-paths); add a token and --github-repo for the owner checks\n", coPath) case n == 0: // Scoped, not "clean". Lint covers three things; `audit` runs twelve // checks, and a bare "lint clean" sends somebody away from a file that @@ -459,6 +528,11 @@ func emitLint(res *lint.Result, coPath string, applied bool, r lintRun, stdout i default: fmt.Fprintf(stdout, "lint: %d fix(es) pending in %s (--dry-run; nothing written)\n", n, coPath) } + if r.offline { + // The disclosure the offline mode is conditioned on: what was skipped, + // and that the fail-closed contract for owners is intact (R-12). + fmt.Fprintf(stdout, " note: owner checks were skipped — this run was given neither a token nor --github-repo, so no owner was verified, repaired, or removed (R-12); dead patterns were judged against the tree, and invalid lines were reported but left as written\n") + } // What lint DID, then what it deliberately did not: a flat list made the // one line that is not a fix look like one, and made the headline count // disagree with the number of bullets under it. diff --git a/internal/cli/lint_offline_test.go b/internal/cli/lint_offline_test.go new file mode 100644 index 0000000..d66831f --- /dev/null +++ b/internal/cli/lint_offline_test.go @@ -0,0 +1,318 @@ +package cli_test + +// The offline tree-only mode of `lint --remove-stale-paths` (UAT finding, +// TestOfflineStaleRuleRemovalReportable). +// +// R-12 makes owner existence — an API fact — undecidable offline, so lint +// without credentials refuses at exit 5. Whether a pattern matches zero +// tracked files is a git-TREE fact (audit's A-4/A-5), so when +// --remove-stale-paths names that repair as what the run is for, the run +// proceeds offline: stage 3 alone, owner checks skipped and DISCLOSED, no +// owner verified, repaired, or removed. Everything else about lint's contract +// holds unchanged — exit 4 for pending or spared work, exit 0 for clean, +// never 1; the A-5 sparing of case-only typos; apply as the single writer. + +import ( + "strings" + "testing" + + "github.com/jordonpeterson/codeowners-tool/internal/cli" +) + +// lintOffline invokes the `lint` verb with NO credentials at all: the ambient +// $GITHUB_TOKEN is cleared so the run is genuinely offline, not accidentally +// authenticated by the test environment. +func lintOffline(t *testing.T, repo string, extra ...string) (int, string, string) { + t.Helper() + t.Setenv("GITHUB_TOKEN", "") + return runCLI(t, append([]string{"lint", "--repo", repo}, extra...)...) +} + +// SPEC A-4/R-11 offline: the dry run is the CI-shaped half of the fix — the +// pending dead-rule removal is reported at exit 4, the dead pattern is named, +// nothing is written, and the output says the owner checks were skipped so +// nobody reads the report as "the owners are fine too". +func TestLintOffline_DryRunReportsThePendingRemovalAndWritesNothing(t *testing.T) { + repo := initRepo(t, map[string]string{ + lintOwnersRel: "* @org/everyone\n/ghost/ @org/ghost-team\n", + "a.md": "", + }) + path := lintOwnersPath(repo) + before := lintRead(t, path) + + code, out, errOut := lintOffline(t, repo, "--dry-run", "--remove-stale-paths") + if code != cli.ExitFindings { + t.Fatalf("exit %d, want 4 — a pending removal under --dry-run\nstdout: %s\nstderr: %s", code, out, errOut) + } + lintUnchanged(t, "offline --dry-run", path, before) + lintMentions(t, "pending removal", out, "/ghost/") + lintMentions(t, "pending removal", out, "remove-stale-rule") + lintMentions(t, "the disclosure", out, "owner checks were skipped") +} + +// SPEC R-0 offline: the write path works too, through the same apply machinery +// as every other write — and re-running over its own output is a no-op at exit +// 0 (clean is lint's success, never exit 1), so the mode is schedulable. +func TestLintOffline_WriteRemovesTheDeadRuleAndIsIdempotent(t *testing.T) { + repo := initRepo(t, map[string]string{ + lintOwnersRel: "* @org/everyone\n/ghost/ @org/ghost-team\n", + "a.md": "", + }) + path := lintOwnersPath(repo) + + code, out, errOut := lintOffline(t, repo, "--remove-stale-paths") + if code != cli.ExitOK { + t.Fatalf("exit %d, want 0 — the removal was computed and written\nstdout: %s\nstderr: %s", code, out, errOut) + } + if got := lintRead(t, path); got != "* @org/everyone\n" { + t.Fatalf("CODEOWNERS = %q, want only the dead rule gone", got) + } + lintMentions(t, "offline write", out, "owner checks were skipped") + + // Second run: byte-identical file, exit 0, and a headline scoped to what + // this run actually established (dead patterns — not owners). + code, out, errOut = lintOffline(t, repo, "--remove-stale-paths") + if code != cli.ExitOK { + t.Fatalf("second run: exit %d, want 0\nstdout: %s\nstderr: %s", code, out, errOut) + } + lintUnchanged(t, "offline idempotence", path, "* @org/everyone\n") + lintMentions(t, "scoped clean headline", out, "nothing to remove") +} + +// SPEC A-5/S-6 offline: a rule that misses ONLY because of case is a typo, not +// a dead rule, and the offline mode spares it exactly as the online mode does +// — deleting it would silently un-own the files it was aimed at. Spared means +// reported, and the run exits 4: a typo still needs a person. +func TestLintOffline_CaseOnlyMissIsSparedNotDeleted(t *testing.T) { + repo := initRepo(t, map[string]string{ + lintOwnersRel: "* @org/everyone\n/Src/ @org/everyone\n", + "src/a.go": "package src\n", + }) + path := lintOwnersPath(repo) + before := lintRead(t, path) + + code, out, errOut := lintOffline(t, repo, "--remove-stale-paths") + if code != cli.ExitFindings { + t.Errorf("exit %d, want 4 (a typo needs a person)\nstdout: %s\nstderr: %s", code, out, errOut) + } + lintUnchanged(t, "offline case-only miss", path, before) + lintMentions(t, "the sparing", out, "kept-case-mismatch") +} + +// SPEC R-12: offline WITHOUT --remove-stale-paths keeps the exit-5 refusal — +// there is no tree-only repair to run, and quietly doing nothing would report +// success over a file full of owners nobody checked. The refusal now names the +// one offline escape, so the operator who only wanted the dead rules gone is +// told the flag instead of being told to find a token. +func TestLintOffline_WithoutRemoveStalePathsStillRefuses(t *testing.T) { + repo := initRepo(t, map[string]string{ + lintOwnersRel: "* @org/everyone\n/ghost/ @org/ghost-team\n", + "a.md": "", + }) + path := lintOwnersPath(repo) + before := lintRead(t, path) + + code, out, errOut := lintOffline(t, repo, "--dry-run") + if code != cli.ExitInconclusive { + t.Errorf("exit %d, want 5 — owner existence is still not decidable offline\nstdout: %s\nstderr: %s", code, out, errOut) + } + lintUnchanged(t, "offline without --remove-stale-paths", path, before) + lintMentions(t, "the escape hatch", out+errOut, "--remove-stale-paths") +} + +// SPEC R-12 offline: the fail-closed contract for OWNERS is untouched. A split +// handle (stage 1's repair) and a dead-looking owner (stage 2's removal) both +// survive the offline run byte-for-byte while the dead PATTERN on another line +// is removed — the run does the tree work without touching a single owner. The +// broken line is REPORTED, not repaired: GitHub is skipping it, which is a +// file fact, so the run exits 4 and names the credentialed run that can fix it. +func TestLintOffline_OwnerRepairsAndRemovalsStayRefused(t *testing.T) { + repo := initRepo(t, map[string]string{ + lintOwnersRel: "* @org/everyone\n/x/ @ org/split\n/ghost/ @org/ghost-team\n", + "x/a.go": "package x\n", + }) + path := lintOwnersPath(repo) + + code, out, errOut := lintOffline(t, repo, "--remove-stale-paths") + if code != cli.ExitFindings { + t.Fatalf("exit %d, want 4 — the broken line GitHub skips still needs a person\nstdout: %s\nstderr: %s", code, out, errOut) + } + got := lintRead(t, path) + if !strings.Contains(got, "/x/ @ org/split") { + t.Errorf("CODEOWNERS = %q: the split handle was repaired offline — an owner repair R-12 reserves for a run that can verify the result", got) + } + if strings.Contains(got, "/ghost/") { + t.Errorf("CODEOWNERS = %q: the dead pattern survived", got) + } + for _, kind := range []string{"repair-owner-spacing", "remove-dead-owner"} { + if strings.Contains(out, kind) { + t.Errorf("output records %q on an offline run — no owner may be repaired or removed, or claimed to be:\n%s", kind, out) + } + } + lintMentions(t, "the broken-line report", out, "unrepairable-line") + lintMentions(t, "the honest remedy", out, "credentialed run") +} + +// SPEC offline reporting (docs/LINTING.md's exit table): a line GitHub is +// silently skipping is reported at exit 4 / needs_human even offline — +// "syntactically broken" is a file fact, no API needed — so a CI gate on +// `jq -e .needs_human` cannot go green over broken lines. +func TestLintOffline_InvalidLineFailsTheJSONGate(t *testing.T) { + repo := initRepo(t, map[string]string{ + lintOwnersRel: "* @org/everyone\n/x/ @org/everyone /docs\n", + "x/a.go": "package x\n", + }) + path := lintOwnersPath(repo) + before := lintRead(t, path) + + code, out, errOut := lintOffline(t, repo, "--dry-run", "--remove-stale-paths", "--format", "json") + if code != cli.ExitFindings { + t.Fatalf("exit %d, want 4 — a broken line needs a person, offline or not\nstdout: %s\nstderr: %s", code, out, errOut) + } + lintUnchanged(t, "offline invalid line", path, before) + doc := lintDecode(t, out) + if !lintBool(t, doc, "needs_human") { + t.Error("needs_human is false over a line GitHub is silently skipping — the CI gate goes green over rot") + } + if !lintHasKind(lintActionKinds(t, doc), "unrepairable-line") { + t.Errorf("actions carry no unrepairable-line entry: %s", out) + } +} + +// SPEC --format json offline: the record carries the disclosure as a field, so +// a script consuming a mixed fleet of online and offline records can tell +// which ones say nothing about owners — prose in a note line cannot be jq'd. +func TestLintOffline_JSONRecordCarriesTheDisclosure(t *testing.T) { + repo := initRepo(t, map[string]string{ + lintOwnersRel: "* @org/everyone\n/ghost/ @org/ghost-team\n", + "a.md": "", + }) + + code, out, errOut := lintOffline(t, repo, "--dry-run", "--remove-stale-paths", "--format", "json") + if code != cli.ExitFindings { + t.Fatalf("exit %d, want 4\nstdout: %s\nstderr: %s", code, out, errOut) + } + doc := lintDecode(t, out) + if !lintBool(t, doc, "owner_checks_skipped") { + t.Error("owner_checks_skipped is absent or false on an offline record") + } + if ec, _ := doc["exit_code"].(float64); int(ec) != code { + t.Errorf("exit_code = %v, want %d", doc["exit_code"], code) + } + kinds := lintActionKinds(t, doc) + if !lintHasKind(kinds, "remove-stale-rule") { + t.Errorf("actions = %v, want a remove-stale-rule entry naming the pending removal", kinds) + } +} + +// SPEC R-12: the offline mode engages only when NEITHER credential was +// offered. A run that named a repo or held a token asked for the credentialed +// lint; silently narrowing it to dead patterns would report success over owner +// checks the operator believes ran. Refused at exit 5, naming exactly what is +// absent — never the credential that was supplied. +func TestLintOffline_PartialCredentialsRefuseNamingWhatIsMissing(t *testing.T) { + newRepo := func(t *testing.T) (string, string, string) { + repo := initRepo(t, map[string]string{ + lintOwnersRel: "* @org/everyone\n/ghost/ @org/ghost-team\n", + "a.md": "", + }) + path := lintOwnersPath(repo) + return repo, path, lintRead(t, path) + } + + t.Run("token but no --github-repo", func(t *testing.T) { + repo, path, before := newRepo(t) + code, out, errOut := lintOffline(t, repo, "--token", "t", "--dry-run", "--remove-stale-paths") + if code != cli.ExitInconclusive { + t.Fatalf("exit %d, want 5 — a run holding a token wanted the credentialed lint\nstdout: %s\nstderr: %s", code, out, errOut) + } + lintUnchanged(t, "token without --github-repo", path, before) + lintMentions(t, "the missing flag", errOut, "--github-repo") + if strings.Contains(errOut, "$GITHUB_TOKEN") { + t.Errorf("stderr asks for a token that was supplied: %q", errOut) + } + if strings.Contains(out, "owner checks were skipped") { + t.Errorf("the run degraded to the tree-only mode with a token in hand:\n%s", out) + } + }) + + t.Run("--github-repo but no token", func(t *testing.T) { + repo, path, before := newRepo(t) + code, out, errOut := lintOffline(t, repo, "--github-repo", "org/repo", "--dry-run", "--remove-stale-paths") + if code != cli.ExitInconclusive { + t.Fatalf("exit %d, want 5 — a run that named a repo wanted the credentialed lint\nstdout: %s\nstderr: %s", code, out, errOut) + } + lintUnchanged(t, "--github-repo without token", path, before) + lintMentions(t, "the missing credential", errOut, "token") + if strings.Contains(out, "owner checks were skipped") { + t.Errorf("the run degraded to the tree-only mode for a NAMED repo:\n%s", out) + } + }) +} + +// SPEC exit 3: a malformed --github-repo is a misspelled argument the operator +// plainly meant to use, and it is diagnosed with or without a token — before +// the fix, a garbage value with no token slid into the offline mode with the +// flag silently ignored. +func TestLintOffline_MalformedGitHubRepoIsInvalidEvenWithoutAToken(t *testing.T) { + repo := initRepo(t, map[string]string{ + lintOwnersRel: "* @org/everyone\n/ghost/ @org/ghost-team\n", + "a.md": "", + }) + path := lintOwnersPath(repo) + before := lintRead(t, path) + + code, out, errOut := lintOffline(t, repo, "--github-repo", "not-owner-name", "--dry-run", "--remove-stale-paths") + if code != cli.ExitInvalid { + t.Fatalf("exit %d, want 3 — the value the operator typed must not be ignored\nstdout: %s\nstderr: %s", code, out, errOut) + } + lintUnchanged(t, "malformed --github-repo", path, before) + lintMentions(t, "the diagnosis", errOut, "must be owner/name") +} + +// SPEC R-36a offline: `"remove_stale_paths": true` in the policy file's "lint" +// block opts in to the tree-only mode exactly as the flag does — the reviewed +// artifact IS the configuration, so the offline escape must not require a flag +// the same run bans (R-36b). +func TestLintOffline_PolicyRemoveStalePathsEnablesTreeOnlyMode(t *testing.T) { + pol := plPolicy(t, `{"version":1,"lint":{"remove_stale_paths":true},"ops":["add_owner(/x/, @org/other)"]}`) + repo := initRepo(t, map[string]string{ + lintOwnersRel: "* @org/everyone\n/ghost/ @org/ghost-team\n", + "a.md": "", + }) + path := lintOwnersPath(repo) + before := lintRead(t, path) + + code, out, errOut := lintOffline(t, repo, "--policy", pol, "--dry-run") + if code != cli.ExitFindings { + t.Fatalf("exit %d, want 4 — the policy opted in to the one offline repair\nstdout: %s\nstderr: %s", code, out, errOut) + } + lintUnchanged(t, "offline --policy --dry-run", path, before) + lintMentions(t, "the pending removal", out, "remove-stale-rule") + lintMentions(t, "the disclosure", out, "owner checks were skipped") +} + +// SPEC R-36b: the offline refusal's escape hatch is worded for how THIS run +// was configured. --remove-stale-paths is exit-3-banned next to --policy, so +// under --policy the remedy is the policy field, not the flag — the old advice +// sent a policy-mode operator straight into a second refusal. +func TestLintOffline_PolicyRefusalNamesThePolicyFieldNotTheBannedFlag(t *testing.T) { + pol := plPolicy(t, `{"version":1,"lint":{"on_empty":"unowned"},"ops":["add_owner(/x/, @org/other)"]}`) + repo := initRepo(t, map[string]string{ + lintOwnersRel: "* @org/everyone\n/ghost/ @org/ghost-team\n", + "a.md": "", + }) + path := lintOwnersPath(repo) + before := lintRead(t, path) + + code, out, errOut := lintOffline(t, repo, "--policy", pol, "--dry-run") + if code != cli.ExitInconclusive { + t.Fatalf("exit %d, want 5 — the policy did not opt in to the offline repair\nstdout: %s\nstderr: %s", code, out, errOut) + } + lintUnchanged(t, "offline --policy without remove_stale_paths", path, before) + lintMentions(t, "the policy-mode remedy", errOut, `"remove_stale_paths"`) + lintMentions(t, "the block it lives in", errOut, `"lint" block`) + if strings.Contains(errOut, "--remove-stale-paths") { + t.Errorf("the remedy names a flag this command line refuses at exit 3 (R-36b):\n%s", errOut) + } +} diff --git a/internal/cli/prerelease_bugs_test.go b/internal/cli/prerelease_bugs_test.go new file mode 100644 index 0000000..b9b35cf --- /dev/null +++ b/internal/cli/prerelease_bugs_test.go @@ -0,0 +1,256 @@ +// Regression guards from the pre-release review: each test began life as a +// failing repro of a confirmed bug and now pins the fixed behavior. The doc +// comments keep the original finding so the guarded failure mode stays legible. +package cli_test + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/jordonpeterson/codeowners-tool/internal/cli" +) + +// Pre-release finding, fixed: plan and apply skip the repo-root guard that sync enforces. +// sync refuses `--repo ` because the CODEOWNERS it would write lands +// at a path GitHub never reads (checkRepoRoot). plan happily plans against the +// subtree and apply writes the dead file, reporting success — the "applied, +// dead on arrival" outcome the guard exists to prevent. plan must refuse +// exactly as sync does. +func TestPlanBelowRepoRootRefused(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/root\n", + "a.md": "", + "sub/.github/CODEOWNERS": "* @org/fixture\n", + "sub/inner.md": "", + }) + sub := filepath.Join(repo, "sub") + + // Baseline: sync refuses this. + if code, _, _ := runCLI(t, "sync", "--repo", sub, "--op", "add_owner(inner.md, @org/a)"); code != cli.ExitRefused { + t.Fatalf("sync below root: want exit 2, got %d", code) + } + + planPath := filepath.Join(t.TempDir(), "plan.json") + code, _, stderr := runCLI(t, "plan", "--repo", sub, "--op", "add_owner(inner.md, @org/a)", "--out", planPath) + if code != cli.ExitRefused { + t.Errorf("plan below root: want exit 2 (same refusal as sync), got %d\nstderr: %s", code, stderr) + } + if _, err := os.Stat(planPath); err == nil { + t.Errorf("plan below root wrote %s; a refused run must write nothing", planPath) + } +} + +// Pre-release finding, fixed: a `\#`-escaped pattern is accepted and written, but S-6/S-2 says +// GitHub honors no `\#` escape — on GitHub the written line is dead, so the +// tool reports `proven: tree` for a rule that provably does not hold there. +// The unescaped spelling `add_owner(#tag.md, …)` is already refused; the +// escaped spelling must be refused too, and nothing written. +func TestEscapedHashPatternRefused(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/every\n", + "#tag.md": "", + }) + before, _ := os.ReadFile(filepath.Join(repo, ".github/CODEOWNERS")) + + code, _, stderr := runCLI(t, "sync", "--repo", repo, "--op", `add_owner(\#tag.md, @org/a)`) + if code != cli.ExitRefused && code != cli.ExitInvalid { + t.Errorf(`add_owner(\#tag.md, …): want refusal (exit 2 or 3), got %d`, code) + } + after, _ := os.ReadFile(filepath.Join(repo, ".github/CODEOWNERS")) + if string(after) != string(before) { + t.Errorf("file was rewritten with a \\#-escaped pattern GitHub will not honor:\n%s\nstderr: %s", after, stderr) + } +} + +// Pre-release finding, fixed: a symlinked .github/CODEOWNERS inside the clone is written +// through and reported applied with no warning. The tool's own docs state +// GitHub does not follow a symlinked CODEOWNERS, so the run edited a file +// that governs nothing while reporting success. An out-of-repo symlink target +// is already refused (containedWritePath); the in-repo case must at minimum +// not be a silent success. +func TestSymlinkedCodeownersNotSilentSuccess(t *testing.T) { + repo := initRepo(t, map[string]string{ + "docs/OWNERS_REAL": "* @org/every\n", + "a.md": "", + }) + if err := os.MkdirAll(filepath.Join(repo, ".github"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink("../docs/OWNERS_REAL", filepath.Join(repo, ".github/CODEOWNERS")); err != nil { + t.Fatal(err) + } + gitRun(t, repo, "add", "-A") + gitRun(t, repo, "commit", "-qm", "symlink") + + code, stdout, stderr := runCLI(t, "sync", "--repo", repo, "--op", "add_owner(a.md, @org/a)") + if code == cli.ExitOK && !strings.Contains(stdout+stderr, "symlink") { + t.Errorf("write through symlinked CODEOWNERS succeeded silently (exit 0, no symlink warning)\nstdout: %s\nstderr: %s", stdout, stderr) + } +} + +// Pre-release finding, fixed: the S-7 branch-mismatch refusal interpolates raw `git rev-parse +// --abbrev-ref --end-of-options HEAD` output, and rev-parse echoes the +// `--end-of-options` operator as an output line — so the one-line error (and +// the JSON record's error field) reads "HEAD is --end-of-options\nmain (…)". +func TestBranchMismatchErrorNamesHeadCleanly(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/e\n", + "a.md": "", + }) + gitRun(t, repo, "switch", "-qc", "feature") + if err := os.WriteFile(filepath.Join(repo, "b.md"), nil, 0o644); err != nil { + t.Fatal(err) + } + gitRun(t, repo, "add", "-A") + gitRun(t, repo, "commit", "-qm", "f") + gitRun(t, repo, "switch", "-q", "main") + + code, stdout, stderr := runCLI(t, "sync", "--repo", repo, "--branch", "feature", "--op", "add_owner(a.md, @org/x)") + if code != cli.ExitRefused { + t.Fatalf("branch mismatch: want exit 2, got %d", code) + } + if out := stdout + stderr; strings.Contains(out, "--end-of-options") { + t.Errorf("refusal leaks git plumbing into the error text (want \"HEAD is main (…)\"):\n%s", out) + } +} + +// Pre-release finding, fixed: `--file ./.github/CODEOWNERS` (or any uncleaned spelling of a +// governing location) triggers a false "governs nothing" warning. trackedAt +// cleans the spelling before matching the tracked file; the S-8 location +// check compares the raw string, so a live change is reported as dead in the +// warning, the --out record, and the --summary-out PR body. +func TestFileFlagSpellingNoFalseGovernsNothing(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/e\n", + "a.md": "", + }) + code, stdout, stderr := runCLI(t, "sync", "--repo", repo, "--file", "./.github/CODEOWNERS", "--op", "add_owner(a.md, @org/y)") + if code != cli.ExitOK { + t.Fatalf("sync --file ./.github/CODEOWNERS: want exit 0, got %d\nstderr: %s", code, stderr) + } + if out := stdout + stderr; strings.Contains(out, "governs nothing") { + t.Errorf("false warning for an alternate spelling of the governing file:\n%s", out) + } +} + +// Pre-release finding, fixed: set_owners on a scope whose pattern already exists earlier in +// the file authors a shadowed duplicate — the old line stays, dead under +// last-match-wins but still naming its owners to human readers — and the run +// that creates it says nothing. The R-7 duplicate warning fires only on the +// NEXT run that touches the file. The run creating the duplicate must +// disclose it. +func TestSetOwnersDisclosesAuthoredDuplicate(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "/x/ @org/a\n* @org/e\n", + "x/a.go": "", + "README.md": "", + }) + code, stdout, stderr := runCLI(t, "sync", "--repo", repo, "--op", "set_owners(/x/, [@org/n])") + if code != cli.ExitOK { + t.Fatalf("set_owners: want exit 0, got %d\nstderr: %s", code, stderr) + } + after, _ := os.ReadFile(filepath.Join(repo, ".github/CODEOWNERS")) + if strings.Count(string(after), "/x/ ") != 2 { + t.Skipf("file no longer contains a duplicate /x/ rule; bug shape changed:\n%s", after) + } + if out := stdout + stderr; !strings.Contains(out, "duplicate") && !strings.Contains(out, "shadow") { + t.Errorf("run authored a shadowed duplicate of /x/ without disclosing it\nfile:\n%s\noutput:\n%s", after, out) + } +} + +// Pre-release finding, fixed: `audit --format json` on a clean repo prints the literal line +// "audit clean" after the JSON object, so the one case CI most wants to pipe +// to jq — the healthy repo — is the one case the output isn't parseable. +// Under `--format json`, stdout is data. +func TestAuditJSONCleanIsPureJSON(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/t\n", + "f.md": "", + }) + code, stdout, _ := runCLI(t, "audit", "--repo", repo, "--checks", "a4", "--format", "json") + if code != cli.ExitOK { + t.Fatalf("clean audit: want exit 0, got %d", code) + } + var v any + if err := json.Unmarshal([]byte(stdout), &v); err != nil { + t.Errorf("audit --format json stdout is not one JSON document: %v\nstdout:\n%s", err, stdout) + } +} + +// Pre-release finding, fixed: positional arguments are silently discarded, and every flag +// after them with them. `audit ../other-repo --checks a999` (note the missing +// --repo) audits the CWD with all defaults and exits 0 — the invalid +// `--checks a999`, which the parser would reject loudly, is never seen. A +// tool this strict about flag values must not swallow whole arguments. +func TestPositionalArgsRejected(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/t\n", + "f.md": "", + }) + // t.Chdir is process-wide: this test must never gain t.Parallel(). From + // inside a healthy repo, the dropped args make audit run on "." with all + // defaults and exit 0 — hiding both the bogus path and the bad --checks. + t.Chdir(repo) + code, stdout, stderr := runCLI(t, "audit", filepath.Join(repo, "does-not-exist"), "--checks", "a999") + if code != cli.ExitInvalid { + t.Errorf("audit with positional arg and invalid --checks: want exit 3, got %d\nstdout: %s\nstderr: %s", + code, stdout, stderr) + } +} + +// Pre-release finding, fixed: `audit` silently accepts an unknown `--format` and falls back to +// text. sync, check, and lint all reject unknown formats at exit 3 — "never a +// silent fallback to text" — and audit is documented with the same +// `--format json|text` contract. +func TestAuditRejectsUnknownFormat(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/t\n", + "f.md": "", + }) + code, _, _ := runCLI(t, "audit", "--repo", repo, "--format", "xml") + if code != cli.ExitInvalid { + t.Errorf("audit --format xml: want exit 3, got %d", code) + } +} + +// Pre-release finding (UAT), fixed: a tree-provably-dead rule cannot be repaired +// offline. `lint --dry-run --remove-stale-paths` refuses everything at exit 5 +// citing R-12 ("owner existence is not decidable offline") — but whether a +// pattern matches zero tracked files is a git-tree fact the offline audit +// (A-4/A-5) itself proves, no API needed. With --remove-stale-paths as the +// requested repair, the dry run should report the pending dead-rule removal +// (exit 4) instead of demanding a token; today the only offline remedy is the +// hand edit the tool exists to prevent. +func TestOfflineStaleRuleRemovalReportable(t *testing.T) { + // The tree-only mode engages only when NEITHER credential is given, and + // this test is the "neither" case — an ambient $GITHUB_TOKEN would turn it + // into the partial-credential refusal instead. + t.Setenv("GITHUB_TOKEN", "") + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/everyone\n/ghost/ @org/ghost-team\n", + "a.md": "", + }) + code, stdout, stderr := runCLI(t, "lint", "--repo", repo, "--dry-run", "--remove-stale-paths") + if code != cli.ExitFindings { + t.Errorf("offline lint --dry-run --remove-stale-paths on a tree-provably-dead rule: want exit 4 (pending fix), got %d\nstdout: %s\nstderr: %s", + code, stdout, stderr) + } + if out := stdout + stderr; !strings.Contains(out, "/ghost/") { + t.Errorf("the pending removal should name the dead pattern /ghost/\noutput:\n%s", out) + } +} + +// gitRun is a helper for tests that need extra git steps after initRepo. +func gitRun(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t", "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } +} diff --git a/internal/cli/prerelease_fixes_test.go b/internal/cli/prerelease_fixes_test.go new file mode 100644 index 0000000..929653d --- /dev/null +++ b/internal/cli/prerelease_fixes_test.go @@ -0,0 +1,487 @@ +// Regression guards around the pre-release fixes: each test here pins an edge +// the KnownBug test that motivated the fix does not — the same behavior on the +// verbs the bug report only implied, and the neighboring cases the fix must +// NOT have broken. +package cli_test + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/jordonpeterson/codeowners-tool/internal/cli" +) + +// fixGitOut runs git and returns its trimmed stdout, for tests that need a +// value (a SHA) rather than a side effect (which is gitRun's job). +func fixGitOut(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + out, err := cmd.Output() + if err != nil { + t.Fatalf("git %v: %v", args, err) + } + return strings.TrimSpace(string(out)) +} + +// Positional args are rejected on EVERY verb, not only the audit invocation +// that surfaced the bug: the flag package stops at the first non-flag token on +// all of them alike, so any verb left unguarded still swallows whole +// arguments. Exit 3 — decidable from the arguments alone, like every other +// member of that class. +func TestFix_PositionalArgsRejectedOnEveryVerb(t *testing.T) { + for _, verb := range []string{"sync", "check", "plan", "apply", "audit", "lint", "verify", "snapshot"} { + code, _, stderr := runCLI(t, verb, "stray-arg", "--repo", ".") + if code != cli.ExitInvalid { + t.Errorf("%s with positional arg: want exit 3, got %d\nstderr: %s", verb, code, stderr) + } + if !strings.Contains(stderr, "stray-arg") { + t.Errorf("%s: the error must name the stray argument\nstderr: %s", verb, stderr) + } + } +} + +// The pure-JSON fix must not have taken the human verdict with it: under the +// default text format a clean audit still says so on stdout. +func TestFix_AuditCleanLineStaysInTextMode(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/t\n", + "f.md": "", + }) + code, stdout, _ := runCLI(t, "audit", "--repo", repo, "--checks", "a4") + if code != cli.ExitOK { + t.Fatalf("clean audit: want exit 0, got %d", code) + } + if !strings.Contains(stdout, "audit clean") { + t.Errorf("text mode lost its verdict line:\n%s", stdout) + } +} + +// `audit --format json` stdout is one JSON document in the findings case too, +// not only on the clean repo the KnownBug test pins. +func TestFix_AuditJSONWithFindingsIsPureJSON(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/e\n/ghost/ @org/gone\n", + "a.md": "", + }) + code, stdout, _ := runCLI(t, "audit", "--repo", repo, "--checks", "a4", "--format", "json") + if code != cli.ExitFindings { + t.Fatalf("audit with dead rule: want exit 4, got %d", code) + } + var v any + if err := json.Unmarshal([]byte(stdout), &v); err != nil { + t.Errorf("stdout is not one JSON document: %v\n%s", err, stdout) + } +} + +// headLabel's contract: on a detached HEAD the label is the bare abbreviated +// SHA — the honest answer, since there is no branch name to offer. Before the +// fix the echoed `--end-of-options` line meant the name never equalled "HEAD" +// and the detached path could not fire. +func TestFix_DetachedHeadLabelIsBareSHA(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/e\n", + "a.md": "", + }) + gitRun(t, repo, "switch", "-qc", "feature") + if err := os.WriteFile(filepath.Join(repo, "b.md"), nil, 0o644); err != nil { + t.Fatal(err) + } + gitRun(t, repo, "add", "-A") + gitRun(t, repo, "commit", "-qm", "f") + gitRun(t, repo, "switch", "-q", "main") + gitRun(t, repo, "checkout", "-q", "--detach") + short := fixGitOut(t, repo, "rev-parse", "HEAD")[:7] + + code, stdout, stderr := runCLI(t, "sync", "--repo", repo, "--branch", "feature", "--op", "add_owner(a.md, @org/x)") + if code != cli.ExitRefused { + t.Fatalf("branch mismatch on detached HEAD: want exit 2, got %d", code) + } + out := stdout + stderr + if !strings.Contains(out, "HEAD is "+short) { + t.Errorf("detached HEAD should be named by its bare SHA %q:\n%s", short, out) + } + if strings.Contains(out, "HEAD is HEAD") || strings.Contains(out, "--end-of-options") { + t.Errorf("refusal still leaks a placeholder or git plumbing:\n%s", out) + } +} + +// The other uncleaned spellings of a governing location: `.github//CODEOWNERS` +// and `docs/../CODEOWNERS` name the S-8 files they clean to, so neither draws +// the "governs nothing" warning — while a path that genuinely is not an S-8 +// location still does. +func TestFix_FileFlagSpellingsClassifyByCleanPath(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/e\n", + "a.md": "", + }) + code, stdout, stderr := runCLI(t, "sync", "--repo", repo, "--file", ".github//CODEOWNERS", "--op", "add_owner(a.md, @org/y)") + if code != cli.ExitOK { + t.Fatalf("--file .github//CODEOWNERS: want exit 0, got %d\nstderr: %s", code, stderr) + } + if out := stdout + stderr; strings.Contains(out, "governs nothing") || strings.Contains(out, "govern nothing") { + t.Errorf("false warning for a doubled-slash spelling of the governing file:\n%s", out) + } + + rootRepo := initRepo(t, map[string]string{ + "CODEOWNERS": "* @org/e\n", + "a.md": "", + }) + code, stdout, stderr = runCLI(t, "sync", "--repo", rootRepo, "--file", "docs/../CODEOWNERS", "--op", "add_owner(a.md, @org/y)") + if code != cli.ExitOK { + t.Fatalf("--file docs/../CODEOWNERS: want exit 0, got %d\nstderr: %s", code, stderr) + } + if out := stdout + stderr; strings.Contains(out, "governs nothing") || strings.Contains(out, "govern nothing") { + t.Errorf("false warning for a dot-dot spelling of the governing root file:\n%s", out) + } + + // The warning itself must survive the fix: a path that cleans to something + // GitHub never loads still governs nothing however it is spelled. + offRepo := initRepo(t, map[string]string{ + "build/OWNERS": "* @org/e\n", + "a.md": "", + }) + code, stdout, stderr = runCLI(t, "sync", "--repo", offRepo, "--file", "./build/OWNERS", "--op", "add_owner(a.md, @org/y)") + if code != cli.ExitOK { + t.Fatalf("--file ./build/OWNERS: want exit 0, got %d\nstderr: %s", code, stderr) + } + if out := stdout + stderr; !strings.Contains(out, "governs nothing") { + t.Errorf("a genuinely non-governing --file lost its S-8 warning:\n%s", out) + } +} + +// The symlink refusal reaches `apply` too: a plan is reviewed in one place and +// applied in another, so the link can appear between the two — the write must +// refuse, and the link's in-repo target must keep its bytes. +func TestFix_ApplyRefusesSymlinkedCodeowners(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/every\n", + "docs/OWNERS_REAL": "* @org/every\n", + "a.md": "", + }) + planPath := filepath.Join(t.TempDir(), "plan.json") + if code, _, stderr := runCLI(t, "plan", "--repo", repo, "--op", "add_owner(a.md, @org/a)", "--out", planPath); code != cli.ExitOK { + t.Fatalf("plan: want exit 0, got %d\nstderr: %s", code, stderr) + } + if err := os.Remove(filepath.Join(repo, ".github/CODEOWNERS")); err != nil { + t.Fatal(err) + } + if err := os.Symlink("../docs/OWNERS_REAL", filepath.Join(repo, ".github/CODEOWNERS")); err != nil { + t.Fatal(err) + } + before, _ := os.ReadFile(filepath.Join(repo, "docs/OWNERS_REAL")) + + code, _, stderr := runCLI(t, "apply", "--plan", planPath) + if code != cli.ExitRefused { + t.Errorf("apply through in-repo symlinked CODEOWNERS: want exit 2, got %d\nstderr: %s", code, stderr) + } + if !strings.Contains(stderr, "symlink") { + t.Errorf("the refusal must name the symlink:\n%s", stderr) + } + after, _ := os.ReadFile(filepath.Join(repo, "docs/OWNERS_REAL")) + if string(after) != string(before) { + t.Errorf("the link's target was written anyway:\n%s", after) + } +} + +// lint shares the same write path, and its symlink refusal fires before any +// API call — so it is decidable, and tested, offline. +func TestFix_LintRefusesSymlinkedCodeowners(t *testing.T) { + repo := initRepo(t, map[string]string{ + "docs/OWNERS_REAL": "* @org/every\n", + "a.md": "", + }) + if err := os.MkdirAll(filepath.Join(repo, ".github"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink("../docs/OWNERS_REAL", filepath.Join(repo, ".github/CODEOWNERS")); err != nil { + t.Fatal(err) + } + gitRun(t, repo, "add", "-A") + gitRun(t, repo, "commit", "-qm", "symlink") + + code, _, stderr := runCLI(t, "lint", "--repo", repo, "--github-repo", "o/r", "--token", "t") + if code != cli.ExitRefused { + t.Errorf("lint through in-repo symlinked CODEOWNERS: want exit 2, got %d\nstderr: %s", code, stderr) + } + if !strings.Contains(stderr, "symlink") { + t.Errorf("the refusal must name the symlink:\n%s", stderr) + } +} + +// A symlink that is NOT the governing CODEOWNERS stays irrelevant: only the +// write path's components are Lstat'ed, so an ordinary repo full of links +// syncs as before. +func TestFix_SymlinkElsewhereStaysIrrelevant(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/every\n", + "a.md": "", + }) + if err := os.Symlink("../a.md", filepath.Join(repo, ".github/NOTES")); err != nil { + t.Fatal(err) + } + gitRun(t, repo, "add", "-A") + gitRun(t, repo, "commit", "-qm", "link elsewhere") + + code, _, stderr := runCLI(t, "sync", "--repo", repo, "--op", "add_owner(a.md, @org/a)") + if code != cli.ExitOK { + t.Errorf("sync with an unrelated symlink in the repo: want exit 0, got %d\nstderr: %s", code, stderr) + } +} + +// The repo-root guard reaches `apply` too: --repo can point the apply at a +// different clone than the plan's, and pointed below the root the joined +// codeowners_path names a file GitHub never reads (checkRepoRoot). +func TestFix_ApplyBelowRepoRootRefused(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/root\n", + "a.md": "", + "sub/.github/CODEOWNERS": "* @org/fixture\n", + "sub/a.md": "", + }) + planPath := filepath.Join(t.TempDir(), "plan.json") + if code, _, stderr := runCLI(t, "plan", "--repo", repo, "--op", "add_owner(a.md, @org/a)", "--out", planPath); code != cli.ExitOK { + t.Fatalf("plan: want exit 0, got %d\nstderr: %s", code, stderr) + } + sub := filepath.Join(repo, "sub") + before, _ := os.ReadFile(filepath.Join(sub, ".github/CODEOWNERS")) + + code, _, stderr := runCLI(t, "apply", "--plan", planPath, "--repo", sub) + if code != cli.ExitRefused { + t.Errorf("apply below root: want exit 2, got %d\nstderr: %s", code, stderr) + } + after, _ := os.ReadFile(filepath.Join(sub, ".github/CODEOWNERS")) + if string(after) != string(before) { + t.Errorf("the dead subdirectory file was written anyway:\n%s", after) + } +} + +// A symlinked PARENT directory is the same dead-on-arrival write one level up: +// git tracks `.github -> real-gh` as a link blob, so `.github/CODEOWNERS` does +// not exist in the tree GitHub reads — yet Lstat'ing only the final component +// (a real file, reached through the link) let sync write through it at exit 0. +// The refusal must fire, name WHICH component is the link, and leave the +// link's target untouched. +func TestFix_SyncRefusesSymlinkedParentDir(t *testing.T) { + repo := initRepo(t, map[string]string{ + "real-gh/CODEOWNERS": "/src/ @org/team\n", + "src/a.go": "", + }) + if err := os.Symlink("real-gh", filepath.Join(repo, ".github")); err != nil { + t.Fatal(err) + } + gitRun(t, repo, "add", "-A") + gitRun(t, repo, "commit", "-qm", "dir link") + before, _ := os.ReadFile(filepath.Join(repo, "real-gh/CODEOWNERS")) + + code, _, stderr := runCLI(t, "sync", "--repo", repo, "--op", "add_owner(/src/, @org/extra)") + if code != cli.ExitRefused { + t.Fatalf("sync through symlinked .github/: want exit 2, got %d\nstderr: %s", code, stderr) + } + want := filepath.ToSlash(filepath.Join(repo, ".github")) + " is a symlink" + if !strings.Contains(stderr, want) { + t.Errorf("the refusal must name the symlinked COMPONENT (%q):\n%s", want, stderr) + } + after, _ := os.ReadFile(filepath.Join(repo, "real-gh/CODEOWNERS")) + if string(after) != string(before) { + t.Errorf("the link's target directory was written anyway:\n%s", after) + } +} + +// The parent-directory refusal reaches `apply` too: the link can appear +// between planning and applying, exactly like the final-component case the +// existing guard pins. +func TestFix_ApplyRefusesSymlinkedParentDir(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/every\n", + "real-gh/CODEOWNERS": "* @org/every\n", + "a.md": "", + }) + planPath := filepath.Join(t.TempDir(), "plan.json") + if code, _, stderr := runCLI(t, "plan", "--repo", repo, "--op", "add_owner(a.md, @org/a)", "--out", planPath); code != cli.ExitOK { + t.Fatalf("plan: want exit 0, got %d\nstderr: %s", code, stderr) + } + if err := os.RemoveAll(filepath.Join(repo, ".github")); err != nil { + t.Fatal(err) + } + if err := os.Symlink("real-gh", filepath.Join(repo, ".github")); err != nil { + t.Fatal(err) + } + before, _ := os.ReadFile(filepath.Join(repo, "real-gh/CODEOWNERS")) + + code, _, stderr := runCLI(t, "apply", "--plan", planPath) + if code != cli.ExitRefused { + t.Fatalf("apply through symlinked .github/: want exit 2, got %d\nstderr: %s", code, stderr) + } + if !strings.Contains(stderr, filepath.ToSlash(filepath.Join(repo, ".github"))+" is a symlink") { + t.Errorf("the refusal must name the symlinked component:\n%s", stderr) + } + after, _ := os.ReadFile(filepath.Join(repo, "real-gh/CODEOWNERS")) + if string(after) != string(before) { + t.Errorf("the link's target directory was written anyway:\n%s", after) + } +} + +// lint shares the helper, so the parent-directory case refuses there too — +// offline, before any API call, like its final-component sibling above. +func TestFix_LintRefusesSymlinkedParentDir(t *testing.T) { + repo := initRepo(t, map[string]string{ + "real-gh/CODEOWNERS": "* @org/every\n", + "a.md": "", + }) + if err := os.Symlink("real-gh", filepath.Join(repo, ".github")); err != nil { + t.Fatal(err) + } + gitRun(t, repo, "add", "-A") + gitRun(t, repo, "commit", "-qm", "dir link") + + // --file, because discovery cannot see .github/CODEOWNERS in a tree where + // .github is a link blob — which is the point of the refusal. + code, _, stderr := runCLI(t, "lint", "--repo", repo, "--github-repo", "o/r", "--token", "t", "--file", ".github/CODEOWNERS") + if code != cli.ExitRefused { + t.Fatalf("lint through symlinked .github/: want exit 2, got %d\nstderr: %s", code, stderr) + } + if !strings.Contains(stderr, "is a symlink") { + t.Errorf("the refusal must name the symlinked component:\n%s", stderr) + } +} + +// A symlinked DIRECTORY that is not on the write path stays irrelevant, like a +// symlinked file elsewhere always has: the walk covers only the components +// between the repository root and the CODEOWNERS being written. +func TestFix_SymlinkedDirOffWritePathStaysIrrelevant(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "* @org/every\n", + "static/logo.txt": "x", + "a.md": "", + }) + if err := os.Symlink("static", filepath.Join(repo, "assets")); err != nil { + t.Fatal(err) + } + gitRun(t, repo, "add", "-A") + gitRun(t, repo, "commit", "-qm", "dir link elsewhere") + + code, _, stderr := runCLI(t, "sync", "--repo", repo, "--op", "add_owner(a.md, @org/a)") + if code != cli.ExitOK { + t.Errorf("sync with a symlinked dir off the write path: want exit 0, got %d\nstderr: %s", code, stderr) + } +} + +// R-25's refusal names only the ops that would have APPLIED: an op whose rule +// was already satisfied changed zero paths, so naming it sent the operator +// narrowing an op that was never behind the number. +func TestFix_CeilingRefusalNamesOnlyAppliedOps(t *testing.T) { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": "/src/ @org/team\n", + "src/a.go": "", + }) + code, _, stderr := runCLI(t, "sync", "--repo", repo, + "--op", "add_owner(/src/, @org/team)", // already satisfied → unchanged + "--op", "add_owner(/src/, @org/extra)", // would apply → behind the count + "--max-paths-changed", "0") + if code != cli.ExitRefused { + t.Fatalf("over the ceiling: want exit 2, got %d\nstderr: %s", code, stderr) + } + if !strings.Contains(stderr, "ops[1]") { + t.Errorf("the refusal must name the op behind the number (ops[1]):\n%s", stderr) + } + if strings.Contains(stderr, "ops[0]") { + t.Errorf("the refusal names ops[0], which changed zero paths:\n%s", stderr) + } +} + +// Every sync exit-3 verdict asked for a sink discloses that no record was +// written — not only the two paths the first fix covered. A fleet aggregating +// --out records otherwise loses these repos silently, the exact hazard the +// note exists to disclose. +func TestFix_NoRecordNoteCoversEverySyncExit3(t *testing.T) { + polPath := filepath.Join(t.TempDir(), "p.json") + if err := os.WriteFile(polPath, []byte(`{"version":1,"ops":["add_owner(/x/, @a)"]}`), 0o644); err != nil { + t.Fatal(err) + } + op := "add_owner(/x/, @a)" + cases := []struct { + name string + args []string // --out / --summary-out / --format json appended per case + }{ + {"bad --format", []string{"sync", "--op", op, "--format", "jsn", "--out", ""}}, + {"bad --on-empty", []string{"sync", "--op", op, "--on-empty", "typo", "--out", ""}}, + {"--on-empty with --policy", []string{"sync", "--policy", polPath, "--on-empty", "error", "--format", "json"}}, + {"--file escape", []string{"sync", "--op", op, "--file", "../esc/CODEOWNERS", "--out", ""}}, + {"--file in .git", []string{"sync", "--op", op, "--file", ".git/CODEOWNERS", "--summary-out", ""}}, + {"bad --branch", []string{"sync", "--op", op, "--branch", "-bad", "--out", ""}}, + {"stray positional", []string{"sync", "--op", op, "--out", "", "stray-arg"}}, + {"no ops", []string{"sync", "--out", ""}}, + {"negative ceiling", []string{"sync", "--op", op, "--max-paths-changed", "-5", "--out", ""}}, + {"--create with --policy", []string{"sync", "--policy", polPath, "--create", "--out", ""}}, + {"ceiling with --policy", []string{"sync", "--policy", polPath, "--max-paths-changed", "5", "--out", ""}}, + {"invalid scope", []string{"sync", "--op", "add_owner(!x, @a)", "--out", ""}}, + {"static conflict", []string{"sync", "--op", "set_owners(/x/, @a)", "--op", "set_owners(/x/, @b)", "--out", ""}}, + } + for _, tc := range cases { + outPath := filepath.Join(t.TempDir(), "rec.json") + args := make([]string, len(tc.args)) + copy(args, tc.args) + for i := range args { + if args[i] == "" && i > 0 && (args[i-1] == "--out" || args[i-1] == "--summary-out") { + args[i] = outPath + } + } + code, _, stderr := runCLI(t, args...) + if code != cli.ExitInvalid { + t.Errorf("%s: want exit 3, got %d\nstderr: %s", tc.name, code, stderr) + continue + } + if !strings.Contains(stderr, "no record was written") { + t.Errorf("%s: exit 3 with a sink asked for must carry the no-record note\nstderr: %s", tc.name, stderr) + } + if _, err := os.Stat(outPath); err == nil { + t.Errorf("%s: a record file was written for an exit-3 verdict", tc.name) + } + } + + // The gate is unchanged: with no sink asked for there is no aggregation to + // protect, so the note stays quiet. + code, _, stderr := runCLI(t, "sync", "--op", op, "--format", "jsn") + if code != cli.ExitInvalid { + t.Fatalf("bad --format without sinks: want exit 3, got %d", code) + } + if strings.Contains(stderr, "no record was written") { + t.Errorf("the note fired with no sink asked for:\n%s", stderr) + } +} + +// The stale-comment warning needs a LEADING token boundary too: a rename of +// @old-team must stay quiet about `someone@old-team`, where the match is the +// tail of an email, while a real mention — space-separated or glued to the +// comment glyph — still warns. +func TestFix_StaleCommentWarningLeadingBoundary(t *testing.T) { + renameOp := "rename_owner(@old-team, @new-team)" + cases := []struct { + name string + content string + wantWarn bool + }{ + {"email tail", "# email someone@old-team about escalations\n/src/ @old-team\n", false}, + {"real mention", "# ping @old-team about escalations\n/src/ @old-team\n", true}, + {"glued to comment glyph", "#@old-team\n/src/ @old-team\n", true}, + } + for _, tc := range cases { + repo := initRepo(t, map[string]string{ + ".github/CODEOWNERS": tc.content, + "src/a.go": "", + }) + code, _, stderr := runCLI(t, "sync", "--repo", repo, "--op", renameOp) + if code != cli.ExitOK { + t.Errorf("%s: rename should apply at exit 0, got %d\nstderr: %s", tc.name, code, stderr) + continue + } + gotWarn := strings.Contains(stderr, `still names "@old-team"`) + if gotWarn != tc.wantWarn { + t.Errorf("%s: warning fired=%v, want %v\nstderr: %s", tc.name, gotWarn, tc.wantWarn, stderr) + } + } +} diff --git a/internal/cli/sync.go b/internal/cli/sync.go index 7b64310..3fadb41 100644 --- a/internal/cli/sync.go +++ b/internal/cli/sync.go @@ -66,7 +66,10 @@ func validOnEmpty(s string) bool { // records/$repo.json` and aggregating the directory afterwards sees the // affected repos DISAPPEAR rather than appear as refused, so the count of repos // needing attention goes down. Silence about that is what made it dangerous; -// the behavior itself is correct. +// the behavior itself is correct. Every exit-3 return in cmdSync goes through +// this (via the exit3s choke point there): the hazard is the same whichever +// pre-repo verdict fired, and covering only some of them lost the rest of the +// repos silently — the exact failure the note exists to disclose. func noRecordNote(stderr io.Writer, format, outPath, summaryPath string, code int) int { if format != "json" && outPath == "" && summaryPath == "" { return code @@ -202,6 +205,22 @@ func cmdSync(args []string, stdout, stderr io.Writer) int { if err := fs.Parse(args); err != nil { return flagParseCode(err) } + // The one choke point for sync's exit-3 class: every verdict in it is + // reached before the repository is opened, so NONE of them produces a + // record — and when --format json, --out or --summary-out asked for one, + // that absence has to be said out loud (see noRecordNote). Routing every + // exit-3 return through this closure is what keeps a refusal added later + // from silently dropping a repo out of a fleet's aggregation — the exact + // hazard the note discloses; the first fix covered only two of the ten + // paths, and the other eight lost repos silently. + exit3s := func(err error) int { + return noRecordNote(stderr, *format, *out, *summaryOut, exit3(stderr, err)) + } + // Argument-only, like every exit-3 verdict below: a stray positional arg + // means the parser read nothing after it, flags included. + if err := rejectLeftoverArgs(fs); err != nil { + return exit3s(err) + } // Which flags were TYPED, as opposed to which hold a non-zero value. Every // "not allowed with --policy" ban below is a ban on the flag being // PRESENT: `--create=false` overrides a reviewed `"create": true` exactly @@ -213,7 +232,7 @@ func cmdSync(args []string, stdout, stderr io.Writer) int { // Never a silent fallback to text: the fleet script's `>> results.jsonl` // would then collect human prose that `jq -s` cannot read, after the // whole rollout has already written its CODEOWNERS files. - return exit3(stderr, fmt.Errorf("unknown --format %q; want text or json", *format)) + return exit3s(fmt.Errorf("unknown --format %q; want text or json", *format)) } // Validated here, not on whichever repo first has a removal empty an owner // set. A flag value is decidable from the arguments alone, so it belongs to @@ -223,10 +242,10 @@ func cmdSync(args []string, stdout, stderr io.Writer) int { // reported exit 2 ("this repo needs a human") on the one repo that happened // to trip it, naming a CODEOWNERS that had nothing to do with the mistake. if *onEmpty != "" && !validOnEmpty(*onEmpty) { - return exit3(stderr, fmt.Errorf("unknown --on-empty %q; want error, inherit or unowned (R-6)", *onEmpty)) + return exit3s(fmt.Errorf("unknown --on-empty %q; want error, inherit or unowned (R-6)", *onEmpty)) } if *onEmpty != "" && len(policyPaths) > 0 { - return exit3(stderr, errors.New("--on-empty is not allowed with --policy: set \"on_empty\" in the policy file instead, or the artifact in git is not the policy that ran (R-20)")) + return exit3s(errors.New("--on-empty is not allowed with --policy: set \"on_empty\" in the policy file instead, or the artifact in git is not the policy that ran (R-20)")) } // A non-HEAD --branch may not write — checkBranchIsWritable enforces that // for creates and edits alike, by comparing RESOLVED COMMITS rather than @@ -239,17 +258,17 @@ func cmdSync(args []string, stdout, stderr io.Writer) int { // and it is checked BEFORE the repository is opened, because with --create // the write happens outside the repository the moment we get that far. if err := containedRelPath(*filePath); err != nil { - return exit3(stderr, err) + return exit3s(err) } // The third spelling of the same mistake, and the one containment cannot // see because it stays inside --repo: see refuseGitDirPath. if err := refuseGitDirPath(*filePath); err != nil { - return exit3(stderr, err) + return exit3s(err) } // Argument-only, hence exit 3: a fleet run halts at repo 0 rather than // recording the same refusal 100 times. if err := gittree.ValidateRef(*branch); err != nil { - return exit3(stderr, err) + return exit3s(err) } // Whether the ceiling flag was TYPED, not merely whether its value looks // set. -1 is the internal "no ceiling" sentinel, so guarding on `>= 0` let @@ -263,10 +282,10 @@ func cmdSync(args []string, stdout, stderr io.Writer) int { // rather than hidden behind this one. pol, opList, err := opSource(opSpecs, policyPaths) if err != nil { - return noRecordNote(stderr, *format, *out, *summaryOut, exit3(stderr, err)) + return exit3s(err) } if maxPathsSet && *maxPaths < 0 { - return exit3(stderr, fmt.Errorf("--max-paths-changed %d must be zero or positive; omit the flag to set no ceiling (R-25)", *maxPaths)) + return exit3s(fmt.Errorf("--max-paths-changed %d must be zero or positive; omit the flag to set no ceiling (R-25)", *maxPaths)) } // The third member of that family (R-34b), and the one whose false default // hid it: `--policy p.json --create` used to create the file at exit 0 @@ -277,7 +296,7 @@ func cmdSync(args []string, stdout, stderr io.Writer) int { // problem in the same command line, and reporting the flag first would let // "the broken policy halted the fleet" be proven by the flag instead. if passed["create"] && len(policyPaths) > 0 { - return exit3(stderr, errors.New("--create is not allowed with --policy: set \"create\" in the policy file instead, or the artifact in git is not the policy that ran (R-20/R-34)")) + return exit3s(errors.New("--create is not allowed with --policy: set \"create\" in the policy file instead, or the artifact in git is not the policy that ran (R-20/R-34)")) } if maxPathsSet && len(policyPaths) > 0 { // Mirrors --on-empty exactly, and for the same reason: the ceiling is a @@ -286,17 +305,17 @@ func cmdSync(args []string, stdout, stderr io.Writer) int { // A flag that could override the file would let one call site quietly // loosen a reviewed policy, and any "lower of the two wins" scheme is a // new precedence rule for operators to learn. - return exit3(stderr, errors.New("--max-paths-changed is not allowed with --policy: set \"max_paths_changed\" in the policy file instead, or the artifact in git is not the policy that ran (R-20/R-25)")) + return exit3s(errors.New("--max-paths-changed is not allowed with --policy: set \"max_paths_changed\" in the policy file instead, or the artifact in git is not the policy that ran (R-20/R-25)")) } if err := validateScopes(opList); err != nil { - return exit3(stderr, err) + return exit3s(err) } // The statically provable half of R-8, settled here with no repository // open, so both verbs give the same verdict for the same policy on every // repo. plan.Build keeps the other half — an overlap only a real tree // reveals stays exit 2, per repo, and the fleet loop still steps over it. if err := ops.StaticConflict(opList); err != nil { - return noRecordNote(stderr, *format, *out, *summaryOut, exit3(stderr, err)) + return exit3s(err) } run := &syncRun{ @@ -388,7 +407,17 @@ func (r *syncRun) execute() (SyncRecord, int) { // keeps that choice inside the clone. Refusal, not error: the repo was read // fine and the tool is declining to write into it, and it is a fact about // THIS clone, so exit 2 and the fleet loop steps to the next one. - if err := containedWritePath(r.repoArg, filepath.Join(r.repoArg, filepath.FromSlash(rel))); err != nil { + target := filepath.Join(r.repoArg, filepath.FromSlash(rel)) + if err := containedWritePath(r.repoArg, target); err != nil { + rec.Status = StatusRefused + rec.Error = err.Error() + return rec, ExitRefused + } + // The refusal containment cannot make: a symlink whose target stays INSIDE + // the clone is still a CODEOWNERS GitHub does not follow, so writing + // through it is the same dead-on-arrival outcome with a live-looking path. + // See refuseSymlinkedTarget. + if err := refuseSymlinkedTarget(target); err != nil { rec.Status = StatusRefused rec.Error = err.Error() return rec, ExitRefused @@ -628,7 +657,12 @@ func headLabel(repoDir, head string) string { if len(short) > 7 { short = short[:7] } - name, err := gitLine(repoDir, "rev-parse", "--abbrev-ref", "--end-of-options", "HEAD") + // No --end-of-options here: in --abbrev-ref (filter) mode rev-parse ECHOES + // the operator as an output line, so the S-7 refusal read "HEAD is + // --end-of-options\nmain (sha)" and the detached check below could never + // fire. "HEAD" is a literal this function supplies, never operator input, + // so there is nothing for the operator to smuggle past. + name, err := gitLine(repoDir, "rev-parse", "--abbrev-ref", "HEAD") if err != nil || name == "" || name == "HEAD" { return short } @@ -750,11 +784,22 @@ func (e *unreadableCodeownersError) Error() string { func (e *unreadableCodeownersError) Unwrap() error { return e.err } +// relClean is the canonical spelling of a repo-relative path: slash-separated +// and cleaned, so `./.github/CODEOWNERS`, `.github//CODEOWNERS` and +// `docs/../CODEOWNERS` all spell the file they name. Every comparison against +// a git-reported path goes through it — git lists clean slash paths, and +// comparing an operator's raw --file spelling against one reported a live +// change as "governs nothing" (S-8) in the warning, the --out record and the +// --summary-out PR body. +func relClean(rel string) string { + return filepath.ToSlash(filepath.Clean(filepath.FromSlash(rel))) +} + // trackedAt reports whether rel is one of the paths git lists for the ref. // Comparison is against the cleaned, slash-separated spelling because rel can // come from --file, where `./docs/CODEOWNERS` names the tracked `docs/CODEOWNERS`. func trackedAt(tree []string, rel string) bool { - want := filepath.ToSlash(filepath.Clean(filepath.FromSlash(rel))) + want := relClean(rel) for _, p := range tree { if p == want { return true @@ -855,7 +900,8 @@ func governingWarnings(tree []string, rel string, content []byte) []string { rel, strings.Join(gittree.CodeownersLocations, ", "))) } if present := gittree.FindCodeownersPaths(tree); len(present) > 0 { - if rel != present[0] && isCodeownersLocation(rel) { + // relClean on the --file side only: present comes from git, already clean. + if relClean(rel) != present[0] && isCodeownersLocation(rel) { out = append(out, fmt.Sprintf( "this run writes %s, but GitHub resolves ownership from %s (S-8: .github/ > root > docs/, first found wins, never merged) — the rules written here govern nothing until that is the file being edited", rel, present[0])) @@ -876,10 +922,14 @@ func governingWarnings(tree []string, rel string, content []byte) []string { } // isCodeownersLocation reports whether a repo-relative path is one of the three -// GitHub actually reads (S-8). +// GitHub actually reads (S-8). Classified over the cleaned spelling, like +// trackedAt: `./.github/CODEOWNERS` governs exactly what `.github/CODEOWNERS` +// governs, and classifying the raw string drew the false "governs nothing" +// warning for an alternate spelling of a governing file. func isCodeownersLocation(rel string) bool { + clean := relClean(rel) for _, loc := range gittree.CodeownersLocations { - if rel == loc { + if clean == loc { return true } } @@ -976,6 +1026,17 @@ func commentLinesNaming(content, owner string) []commentMatch { // in any case. Advancing by one byte rather than past the match is // what lets a longer handle be rejected and a real one later on the // same line still be found. + // + // Both ends need it. The LEADING boundary is what keeps `# email + // someone@old-team` quiet on a rename of @old-team: the match is + // the tail of a longer token, betrayed by the byte before it being + // an owner-token byte ('e') — or '@' itself, which ownerTokenByte + // deliberately excludes as a continuation byte but which glued to + // the front (`a@@old-team`) still means "embedded, not a mention". + // `#@old-team` stays a real mention: '#' is neither. + if at != 0 && (ownerTokenByte(text[at-1]) || text[at-1] == '@') { + continue + } end := at + len(owner) if end != len(text) && ownerTokenByte(text[end]) { continue @@ -1006,10 +1067,16 @@ func commentStart(line string) int { } // blockedOpLabels names the ops that would have applied, for R-25's refusal. +// +// Only status "applied" belongs: the caller reads the statuses BEFORE the +// applied→unchanged rewrite, so at that point "unchanged" means "already +// satisfied, changed zero paths" — an op that contributed nothing to the +// number the ceiling refused on. Naming it sent the operator narrowing an op +// that was never behind the count. func blockedOpLabels(results []plan.OpResult) string { var labels []string for i, o := range results { - if o.Status == "applied" || o.Status == "unchanged" && o.Reason == "" { + if o.Status == "applied" { labels = append(labels, policy.OpLabel(o.ID, i)) } } @@ -1301,6 +1368,9 @@ func cmdCheck(args []string, stdout, stderr io.Writer) int { if err := fs.Parse(args); err != nil { return flagParseCode(err) } + if err := rejectLeftoverArgs(fs); err != nil { + return exit3(stderr, err) + } if *format != "text" && *format != "json" { return exit3(stderr, fmt.Errorf("unknown --format %q; want text or json", *format)) } diff --git a/internal/lint/deadreason_test.go b/internal/lint/deadreason_test.go new file mode 100644 index 0000000..859fde8 --- /dev/null +++ b/internal/lint/deadreason_test.go @@ -0,0 +1,35 @@ +package lint_test + +import ( + "strings" + "testing" + + "github.com/jordonpeterson/codeowners-tool/internal/lint" +) + +// SPEC R-38a in stage 2's record: a dead owner spelled @Org/Gone is the same +// owner as @org/gone, so the lookup and the `dead` map are both case-folded — +// but the Action.Reason was read back with the UNFOLDED spelling, an empty +// reason on exactly the removals whose spelling differs from the fold. The +// reason is what a reviewer approves the deletion on, so it must survive the +// file's own capitalisation. +func TestRemoveDeadOwner_MixedCaseSpellingKeepsItsReason(t *testing.T) { + v := fcNew() + v.missingTeams["org/gone"] = true + + content := []byte("* @keep @Org/Gone\n") + res, err := lint.Build(content, []string{"a.md"}, v, fcOpts()) + if err != nil { + t.Fatalf("Build: %v", err) + } + removed := fcActionsOfKind(res, lint.ActionRemoveOwner) + if len(removed) != 1 || removed[0].Owner != "@Org/Gone" { + t.Fatalf("remove actions = %+v, want exactly one for @Org/Gone", res.Actions) + } + if removed[0].Reason == "" { + t.Fatal("Reason is empty — the dead map is keyed by the folded spelling and the action looked it up unfolded") + } + if !strings.Contains(removed[0].Reason, "does not exist") { + t.Errorf("Reason = %q, want the lookup's verdict on the record", removed[0].Reason) + } +} diff --git a/internal/lint/lint.go b/internal/lint/lint.go index 8a2addd..5f26ac7 100644 --- a/internal/lint/lint.go +++ b/internal/lint/lint.go @@ -87,6 +87,19 @@ type Options struct { // its owners are deleted out from under it, at exit 0 (found by adversarial // review). Required whenever RemoveStalePaths is set. WorkTree []string + // SkipOwnerChecks is the tree-only mode: stage 3 alone, no lookups, no + // owner repairs, no owner removals. It exists for the offline + // `lint --remove-stale-paths` path — whether a pattern matches zero files + // is a git-tree fact (audit's A-4/A-5), decidable with no API, while owner + // existence is not (R-12). Stage 2 is skipped outright, and stage 1's + // REPAIR is too: a split handle is an owner repair (it puts an unverified + // owner into force), so offline it is left exactly as written, like every + // other owner. Stage 1's REPORTING still runs — an invalid line GitHub is + // skipping is a file fact, and it is reported (NeedsHuman) with a reason + // that says whether a credentialed run could repair it. Requires + // RemoveStalePaths — without it there is no repair this mode is allowed to + // make. The Verifier may be nil when this is set; it is never called. + SkipOwnerChecks bool // OnEmpty is R-6's policy for the case where removing a dead owner would // leave a rule with no owners: "error", "inherit", or "unowned". "" is the // ordinary default and is perfectly valid — it becomes a *plan.InvalidError @@ -479,6 +492,14 @@ func Build(content []byte, tree []string, v Verifier, opts Options) (*Result, er // initial commit or a tag on an empty tree all reach it). plan.Build // refuses a zero-match scope under R-5 for the same reason; this is lint's // analogue. + // SkipOwnerChecks without RemoveStalePaths has no work it is allowed to + // do: stages 1 and 2 repair and remove OWNERS, which is exactly what R-12 + // forbids without lookups, and stage 3 was not opted into (R-11). Refused + // rather than returned clean — a "clean" from a run that checked nothing + // is the green check that means nothing. + if opts.SkipOwnerChecks && !opts.RemoveStalePaths { + return nil, &plan.InvalidError{Msg: "refusing SkipOwnerChecks without RemoveStalePaths: with owner checks skipped, deleting dead patterns is the only repair left, and it was not opted into (R-11/R-12) — there is nothing this run may do"} + } if opts.RemoveStalePaths && len(opts.WorkTree) == 0 { return nil, &plan.InvalidError{Msg: "refusing --remove-stale-paths: Options.WorkTree is empty, so staleness would be judged against the committed tree alone while the edit lands on the working-tree file — a directory created but not yet committed would read as dead and lose its owners; supply the checkout's file list (gittree.ListWorkTree)"} } @@ -497,12 +518,32 @@ func Build(content []byte, tree []string, v Verifier, opts Options) (*Result, er // ---- Stage 1: repair owner spacing. ------------------------------------- // Before any lookup: `@ org/team` is one owner nobody has asked about yet, // not two owners that do not exist. + // + // Under SkipOwnerChecks the REPAIR half is skipped — it puts a previously + // skipped line, and the owner on it, into force, which the run only gets + // to trust because stage 2 then verifies that owner. The REPORTING still + // happens: "GitHub is skipping this broken line" is a file fact, no API + // needed, and exiting 0 over it would be a green check over rot. A line + // the online run would repair is reported as needing that credentialed + // run, never as flatly unrepairable. for i, ln := range f.Lines { if ln.Kind != file.LineInvalid { continue } old := ln.Raw fixed, ok := RepairLine(old) + if opts.SkipOwnerChecks { + reason := fmt.Sprintf("%s: %s — left exactly as written; GitHub skips this line, so nothing here owns anything", + ln.Err.Kind, ln.Err.Message) + if ok { + reason = fmt.Sprintf("%s: %s — a split @handle a credentialed run repairs mechanically; left exactly as written here, because the repair puts the reassembled owner into force and offline nothing can verify that owner exists (R-12)", + ln.Err.Kind, ln.Err.Message) + } + res.Actions = append(res.Actions, Action{ + Kind: ActionUnrepairable, Line: i + 1, Before: old, Reason: reason, + }) + continue + } if !ok { // Reported, never touched. A line the tool does not understand is // a rule somebody wrote and believes is in force; guessing at it or @@ -550,24 +591,31 @@ func Build(content []byte, tree []string, v Verifier, opts Options) (*Result, er } dead := map[string]string{} // owner -> why it is dead var reasons []string - for _, o := range owners { - // R-13: an email owner resolves via a verified address the API cannot - // see. Unverifiable is not inconclusive — it is permanent, so treating - // it as R-12 would wedge lint forever on any file that has one. - if file.IsEmailOwner(o) { - res.Unverifiable = append(res.Unverifiable, o) - continue - } - gone, reason, err := ownerIsGone(v, o) - if err != nil { - reasons = appendUnique(reasons, fmt.Sprintf("%s: %s", o, errReason(err))) - continue - } - if gone { - // Keyed by the folded spelling for the same reason the lookup is: - // two capitalisations of one dead team must both go, and a file - // that names it both ways must not keep one of them. - dead[ops.FoldOwner(o)] = reason + // Under SkipOwnerChecks no owner is looked up at all, so `dead` stays + // empty and every owner — email owners included, which is why the R-13 + // reporting sits inside the gate — is left exactly as written. This is + // R-12 kept intact rather than relaxed: offline, "does this owner exist" + // has no answer, and an unanswered question authorizes nothing. + if !opts.SkipOwnerChecks { + for _, o := range owners { + // R-13: an email owner resolves via a verified address the API cannot + // see. Unverifiable is not inconclusive — it is permanent, so treating + // it as R-12 would wedge lint forever on any file that has one. + if file.IsEmailOwner(o) { + res.Unverifiable = append(res.Unverifiable, o) + continue + } + gone, reason, err := ownerIsGone(v, o) + if err != nil { + reasons = appendUnique(reasons, fmt.Sprintf("%s: %s", o, errReason(err))) + continue + } + if gone { + // Keyed by the folded spelling for the same reason the lookup is: + // two capitalisations of one dead team must both go, and a file + // that names it both ways must not keep one of them. + dead[ops.FoldOwner(o)] = reason + } } } // R-12, applied to the whole run rather than to one owner. Partial @@ -705,9 +753,12 @@ func Build(content []byte, tree []string, v Verifier, opts Options) (*Result, er } for _, o := range removed { + // The folded key, matching how the map is written: `dead[o]` on a + // dead owner spelled @Org/Team was a lookup of a key that is not + // there, and the action shipped with an empty Reason. res.Actions = append(res.Actions, Action{ Kind: ActionRemoveOwner, Line: i + 1, Owner: o, Pattern: r.PatternText, - Before: old, Reason: dead[o], + Before: old, Reason: dead[ops.FoldOwner(o)], }) } if deletedLine { @@ -787,6 +838,11 @@ func Build(content []byte, tree []string, v Verifier, opts Options) (*Result, er // line is invalid — so a flat "every owner exists" would assert a check // that never ran on precisely the lines a human still has to fix. msg := "nothing to lint: no repairable owner spacing, and every owner named by a valid rule exists" + if opts.SkipOwnerChecks { + // The scoped claim for the tree-only mode. The owner sentence above + // would assert checks that never ran (R-12). + msg = "nothing to remove: every rule's pattern matches at least one tracked or on-disk file — owner checks were skipped, so this run says nothing about whether the owners exist" + } if n := res.NeedsHuman(); n > 0 { // Two unrelated categories, and one sentence written for only one // of them said the other was invalid, unchecked and skipped by diff --git a/internal/lint/offline_test.go b/internal/lint/offline_test.go new file mode 100644 index 0000000..50b5797 --- /dev/null +++ b/internal/lint/offline_test.go @@ -0,0 +1,210 @@ +package lint_test + +// The tree-only mode behind offline `lint --remove-stale-paths` +// (Options.SkipOwnerChecks). +// +// R-12 makes owner existence — an API fact — undecidable offline, and the +// whole run fails closed on it. But whether a pattern matches zero tracked +// files is a git-TREE fact the offline audit (A-4/A-5) already proves, so a +// run that asks ONLY for the stale-path repair may run with no Verifier at +// all. The contract for that mode, pinned here: +// +// - stage 3 runs exactly as it does online — same staleness judgment, same +// A-5 sparing of case-only misses; +// - the owner WORK of stages 1 and 2 is skipped, not degraded: no owner is +// looked up, repaired, or removed, and no invalid line is rewritten; +// - stage 1's REPORTING still runs: an invalid line GitHub is skipping is a +// file fact, no API needed, so it is reported (NeedsHuman → exit 4) — a +// line the online run would repair with a reason that names the +// credentialed run, any other with the same reason as online; +// - SkipOwnerChecks without RemoveStalePaths is invalid input, because with +// owner work forbidden there is nothing left the run may do (R-11/R-12). + +import ( + "errors" + "strings" + "testing" + + "github.com/jordonpeterson/codeowners-tool/internal/lint" + "github.com/jordonpeterson/codeowners-tool/internal/plan" +) + +// offlineOpts is the option set the offline CLI path builds: stage 3 opted in, +// owner checks skipped, an --on-empty that could never matter (stage 2 is off). +func offlineOpts(workTree []string) lint.Options { + return lint.Options{RemoveStalePaths: true, WorkTree: workTree, SkipOwnerChecks: true} +} + +// SPEC A-4/R-11 offline: a rule whose pattern matches nothing tracked and +// nothing on disk is deleted with a NIL Verifier. The nil is the proof that no +// lookup can possibly have been made — an implementation that touched the +// network here would panic, not pass. +func TestOffline_DeadRuleIsRemovedWithANilVerifier(t *testing.T) { + content := []byte("* @org/everyone\n/ghost/ @org/ghost-team\n") + tree := []string{"a.md"} + + res, err := lint.Build(content, tree, nil, offlineOpts(tree)) + if err != nil { + t.Fatalf("Build: %v", err) + } + after := fcAfter(res) + if after != "* @org/everyone\n" { + t.Errorf("after = %q, want only the dead rule gone", after) + } + stale := fcActionsOfKind(res, lint.ActionRemoveStale) + if len(stale) != 1 || stale[0].Pattern != "/ghost/" { + t.Errorf("stale actions = %+v, want exactly one for /ghost/", res.Actions) + } + // A stale rule wins no tracked path by construction, so its deletion must + // change no ownership — the same property the end-of-run gate relies on. + if len(res.Plan.Rows) != 0 { + t.Errorf("ownership rows = %+v, want none: deleting a dead rule changes no current ownership", res.Plan.Rows) + } +} + +// SPEC R-12 offline: owners are never touched, even one a lookup WOULD have +// proven dead. Proven from the call log, not the output — an implementation +// that asked and ignored the answer is one refactor from believing it. +func TestOffline_OwnersAreNeverLookedUpOrRemoved(t *testing.T) { + v := fcNew() + v.missingUsers["gone"] = true // dead if anyone asked; nobody may ask + + content := []byte("* @keep @gone\n/ghost/ @keep\n") + tree := []string{"a.md"} + res, err := lint.Build(content, tree, v, offlineOpts(tree)) + if err != nil { + t.Fatalf("Build: %v", err) + } + if calls := v.fcCalls(); len(calls) != 0 { + t.Errorf("offline run made API calls: %v (R-12: owner existence was not asked for and must not be answered)", calls) + } + fcAssertNotRemoved(t, res, "@gone") + if after := fcAfter(res); !strings.Contains(after, "@keep @gone") { + t.Errorf("after = %q: an owner was touched by a run that could not know anything about owners", after) + } +} + +// SPEC R-12 offline: stage 1's REPAIR is an owner repair — it puts a +// previously skipped line, and the unverified owner on it, into force — so +// offline the line is left byte-for-byte. But it IS reported: GitHub skipping +// the line is a file fact, and a run that exits 0 over it goes green over rot. +// The reason is honest about which run can fix it — a credentialed one, which +// repairs it mechanically and verifies the reassembled owner. +func TestOffline_SplitHandleIsReportedNotRepaired(t *testing.T) { + content := []byte("* @keep\n/x/ @ org/team\n/ghost/ @keep\n") + tree := []string{"x/a.go"} + + res, err := lint.Build(content, tree, nil, offlineOpts(tree)) + if err != nil { + t.Fatalf("Build: %v", err) + } + after := fcAfter(res) + if !strings.Contains(after, "/x/ @ org/team") { + t.Errorf("after = %q: the invalid line was rewritten or deleted offline", after) + } + if n := len(fcActionsOfKind(res, lint.ActionRepairOwner)); n != 0 { + t.Errorf("%d owner repair(s) in an offline run — repairing an owner offline is what R-12 forbids", n) + } + stuck := fcActionsOfKind(res, lint.ActionUnrepairable) + if len(stuck) != 1 { + t.Fatalf("unrepairable reports = %+v, want exactly one for the split handle — GitHub is skipping that line, and silence over it is exit 0 over rot", res.Actions) + } + if !strings.Contains(stuck[0].Reason, "credentialed run") { + t.Errorf("reason = %q: a line the online run repairs mechanically must be reported as needing that run, not as flatly unrepairable", stuck[0].Reason) + } + if res.NeedsHuman() != 1 { + t.Errorf("NeedsHuman = %d, want 1 — the broken line keeps the run at exit 4", res.NeedsHuman()) + } + if strings.Contains(after, "/ghost/") { + t.Errorf("after = %q: the genuinely dead rule survived — sparing owners is not a blanket refusal to do the tree work", after) + } +} + +// SPEC offline reporting: a line no run can repair — `@keep /docs` is shaped +// exactly like two rules on one line — is reported offline with the SAME +// reason as online. GitHub skipping it is a file fact, and docs/LINTING.md +// promises exit 4 for it; an offline exit 0 would let a CI gate on +// .needs_human go green over a broken line. +func TestOffline_UnrepairableLineIsReportedSameAsOnline(t *testing.T) { + content := []byte("* @keep\n/x/ @keep /docs\n/ghost/ @keep\n") + tree := []string{"x/a.go"} + + res, err := lint.Build(content, tree, nil, offlineOpts(tree)) + if err != nil { + t.Fatalf("Build: %v", err) + } + stuck := fcActionsOfKind(res, lint.ActionUnrepairable) + if len(stuck) != 1 { + t.Fatalf("unrepairable reports = %+v, want exactly one", res.Actions) + } + if !strings.Contains(stuck[0].Reason, "GitHub skips this line") { + t.Errorf("reason = %q, want the online wording — nothing about this line's diagnosis needed an API", stuck[0].Reason) + } + if strings.Contains(stuck[0].Reason, "credentialed run") { + t.Errorf("reason = %q: this line is ambiguous for EVERY run, and pointing at a credentialed one promises a repair it will also refuse", stuck[0].Reason) + } + if res.NeedsHuman() != 1 { + t.Errorf("NeedsHuman = %d, want 1", res.NeedsHuman()) + } + if after := fcAfter(res); !strings.Contains(after, "/x/ @keep /docs") { + t.Errorf("after = %q: the invalid line was touched", after) + } +} + +// SPEC A-5/S-6 offline: a rule that matches nothing ONLY because of case is a +// typo, not a dead rule, and the offline mode spares it with exactly the +// online logic — deleting it would silently un-own the files it was aimed at. +// Spared means reported (NeedsHuman), so the caller still exits 4. +func TestOffline_CaseOnlyMissIsSparedAndNeedsAHuman(t *testing.T) { + content := []byte("* @keep\n/Src/ @keep\n") + tree := []string{"src/a.go"} + + res, err := lint.Build(content, tree, nil, offlineOpts(tree)) + var noop *plan.NoOpError + if !errors.As(err, &noop) { + t.Fatalf("err = %v (%T), want *plan.NoOpError — the spared rule is the only candidate, so nothing changes", err, err) + } + if got := fcActionsOfKind(res, lint.ActionKeptCaseMismatch); len(got) != 1 { + t.Fatalf("kept-case-mismatch actions = %+v, want exactly one for /Src/", res.Actions) + } + if res.NeedsHuman() != 1 { + t.Errorf("NeedsHuman = %d, want 1 — a spared typo still needs a person (exit 4)", res.NeedsHuman()) + } + // The no-op message must not claim the owner checks ran. + if msg := noop.Error(); strings.Contains(msg, "every owner named by a valid rule exists") { + t.Errorf("no-op message asserts owner checks that never ran: %q", msg) + } +} + +// SPEC R-11/R-12: SkipOwnerChecks without RemoveStalePaths is invalid input, +// not a clean run. Stages 1 and 2 are owner work the mode forbids, and stage 3 +// was not opted into — a "clean" from a run that checked nothing would be a +// green check that means nothing. +func TestOffline_SkipWithoutRemoveStaleIsInvalid(t *testing.T) { + content := []byte("* @keep\n") + _, err := lint.Build(content, []string{"a.md"}, nil, lint.Options{SkipOwnerChecks: true}) + var inv *plan.InvalidError + if !errors.As(err, &inv) { + t.Fatalf("err = %v (%T), want *plan.InvalidError", err, err) + } +} + +// SPEC R-13 offline: email owners are not looked up online either, but online +// they are REPORTED as unverifiable — a statement about a check that ran +// around them. Offline no owner check ran at all, so the report would imply +// the rest of the file's owners were verified. Nothing is reported. +func TestOffline_EmailOwnersAreNotReportedUnverifiable(t *testing.T) { + content := []byte("* @keep docs@example.com\n/ghost/ @keep\n") + tree := []string{"a.md"} + + res, err := lint.Build(content, tree, nil, offlineOpts(tree)) + if err != nil { + t.Fatalf("Build: %v", err) + } + if len(res.Unverifiable) != 0 { + t.Errorf("Unverifiable = %v, want empty — offline, EVERY owner is unverified, and singling out the email owner implies the others were checked", res.Unverifiable) + } + if after := fcAfter(res); !strings.Contains(after, "docs@example.com") { + t.Errorf("after = %q: the email owner was touched", after) + } +} diff --git a/internal/pattern/pattern.go b/internal/pattern/pattern.go index db375e5..83aec19 100644 --- a/internal/pattern/pattern.go +++ b/internal/pattern/pattern.go @@ -9,9 +9,14 @@ // Copyright (c) 2020 Harry Marr — the actively maintained reference // implementation whose semantics are differentially tested against GitHub's // observed behavior. The vendored corpus in testdata/patterns.json comes from -// the same project. Local divergences from the port, both deliberate: +// the same project. Local divergences from the port, all deliberate: // - Compile rejects patterns starting with `!` (GitHub: negation "doesn't // work"; a mutation tool must never accept or emit one). +// - Compile rejects patterns starting with `\#` (S-2/S-6: GitHub honors no +// `\#` escape — a line starting with `#` is always a comment, so the rule +// would be dead there; same standard as `!`). Only the LEADING position is +// special-cased: a mid-pattern `#` needs no escape at all, so `\#` after +// the first character keeps the oracle's literal-`#` meaning. // - Compile rejects empty patterns. package pattern @@ -39,6 +44,9 @@ func Compile(patternStr string) (*Pattern, error) { if patternStr[0] == '!' { return nil, fmt.Errorf("negation (%q) is not supported in CODEOWNERS", patternStr) } + if strings.HasPrefix(patternStr, `\#`) { + return nil, fmt.Errorf(`pattern %q needs a \# escape GitHub does not honor (S-2/S-6): a line starting with '#' is always a comment there`, patternStr) + } p := &Pattern{raw: patternStr} if !strings.ContainsAny(patternStr, "*?\\") && patternStr[0] == '/' { p.leftAnchoredLiteral = true diff --git a/internal/pattern/pattern_test.go b/internal/pattern/pattern_test.go index 56252eb..f42f603 100644 --- a/internal/pattern/pattern_test.go +++ b/internal/pattern/pattern_test.go @@ -124,6 +124,33 @@ func TestS2_NegationRejected(t *testing.T) { } } +// SPEC S-2/S-6: GitHub honors no `\#` escape of a leading hash — a line +// starting with '#' is always a comment there, so a `\#…` rule is dead on +// GitHub. Same standard as `!`: a mutation tool must never accept or emit one. +func TestS2_EscapedLeadingHashRejected(t *testing.T) { + if _, err := pattern.Compile(`\#tag.md`); err == nil { + t.Error(`pattern \#tag.md must be rejected: GitHub reads the written line as a comment`) + } +} + +// Only the LEADING position is special-cased: a mid-pattern '#' needs no +// escape at all, and a mid-pattern `\#` keeps the reference implementation's +// literal-'#' meaning. +func TestS2_MidPatternHashAccepted(t *testing.T) { + for _, c := range []struct{ pat, path string }{ + {"a#b.md", "a#b.md"}, + {`a\#b.md`, "a#b.md"}, + } { + p, err := pattern.Compile(c.pat) + if err != nil { + t.Fatalf("Compile(%q): %v", c.pat, err) + } + if !p.Match(c.path) { + t.Errorf("pattern %q must match %q literally", c.pat, c.path) + } + } +} + // Empty and comment-like inputs are the parser's problem, not the matcher's; // compiling them is an error so bugs surface loudly. func TestCompile_RejectsEmpty(t *testing.T) { diff --git a/internal/plan/plan.go b/internal/plan/plan.go index 583ddb8..960052a 100644 --- a/internal/plan/plan.go +++ b/internal/plan/plan.go @@ -251,6 +251,15 @@ func Build(content []byte, tree []string, opList []ops.Op, opts Options) (*Plan, "scope %q matches %d tracked file(s), but every one of them is excepted — the except clause empties the op in this repo (R-28)", op.Scope, raw)} } return nil, &InvalidError{Msg: fmt.Sprintf("scope %q matches zero tracked files (R-5: refusing to create a dead rule)", op.Scope)} + default: + // Policy parsing validates the field, but the struct is + // exported: a library caller (or a future value) can carry + // anything here, and falling through would synthesize + // nothing while reporting the repo converged — the silent + // no-op rollout this switch exists to prevent. + return nil, &InvalidError{Msg: fmt.Sprintf( + "unknown on_zero_match value %q on %s; legal values are %q, %q, or %q", + op.OnZeroMatch, op.Raw, ops.ZeroMatchRequire, ops.ZeroMatchSkip, ops.ZeroMatchDeclare)} } } else if len(zeroPats) > 0 { // R-28's second question, reached only when the op WILL write. @@ -268,7 +277,7 @@ func Build(content []byte, tree []string, opList []ops.Op, opts Options) (*Plan, allowWarnings = append(allowWarnings, fmt.Sprintf( "except pattern %q matches zero tracked files; on_except_zero_match=allow writes the grant with NO carve for it, so a matching file created later falls under the grant — a declare-class weakening of INV-1, marked proven=structural (R-28)", e)) } - default: + case ops.ExceptZeroMatchRequire, "": // "" and "require" are one state, exactly as above. An // except that bites nothing means the carve-out this // policy promises does not exist here — in the motivating @@ -278,6 +287,13 @@ func Build(content []byte, tree []string, opList []ops.Op, opts Options) (*Plan, // /.github/CODEOWNERS. return nil, &RefusalError{Msg: fmt.Sprintf( "refusing: except pattern %q matches zero tracked files — the carve-out this policy promises does not exist in this repo, and writing the grant without it would reopen the hole the except exists to close (R-28); normalize this repo first, or set on_except_zero_match=allow to accept the weakening", zeroPats[0])} + default: + // Same defense as on_zero_match: an unrecognized value is + // bad input that fails identically everywhere (exit 3), + // not a per-repo refusal masquerading as require. + return nil, &InvalidError{Msg: fmt.Sprintf( + "unknown on_except_zero_match value %q on %s; legal values are %q or %q", + op.OnExceptZeroMatch, op.Raw, ops.ExceptZeroMatchRequire, ops.ExceptZeroMatchAllow)} } } } @@ -825,6 +841,19 @@ func synthSet(f *file.File, tree []string, op ops.Op, scope map[string]bool, des NewOwners: op.Owners, NewLine: f.LineText(at), Reason: "inserted after the last rule whose match set intersects the scope, so no later rule recaptures any in-scope path (R-3)", }) + // R-7 disclosure for a duplicate THIS run authors: the insert lands + // after the last intersecting rule, so an earlier byte-equal pattern is + // left permanently shadowed while still naming its old owners to human + // readers. warnShadowedDuplicates above ran on the pre-op file and + // cannot see it — without this, the run creating the dead line is the + // one run that says nothing about it (pre-release finding). + for _, r := range f.Rules() { + if r.LineIndex != at && r.PatternText == op.Scope { + pl.addWarning(fmt.Sprintf( + "line %d: duplicate pattern %q is shadowed by line %d, which this run inserted — the earlier line is dead under last-match-wins (R-7); run `audit` to clean up duplicates", + r.LineIndex+1, op.Scope, at+1)) + } + } } for p := range scope { diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go index c61f7f4..4a75bad 100644 --- a/internal/plan/plan_test.go +++ b/internal/plan/plan_test.go @@ -252,6 +252,29 @@ func TestR7_DuplicatePatternsEditEffective(t *testing.T) { } } +// SPEC R-7, same-run case (pre-release finding): set_owners on a scope whose +// pattern already exists earlier inserts after the last intersecting rule, +// leaving the earlier byte-equal line permanently shadowed but still naming +// its old owners to readers. The run that AUTHORS the duplicate must disclose +// it — not only the next run that touches the file. +func TestR7_SetOwnersDisclosesAuthoredDuplicate(t *testing.T) { + tree := []string{"x/a.go", "README.md"} + p, err := build(t, "/x/ @a\n* @e\n", tree, plan.Options{}, "set_owners(/x/, [@n])") + if err != nil { + t.Fatal(err) + } + if got := strings.Count(p.AfterContent, "/x/ "); got != 2 { + t.Fatalf("both /x/ lines must remain (R-1: only disclose, never reorder):\n%s", p.AfterContent) + } + joined := strings.Join(p.Warnings, " ") + if !strings.Contains(joined, "duplicate") || !strings.Contains(joined, "shadow") { + t.Errorf("run that authored the duplicate must warn, warnings=%v", p.Warnings) + } + if !strings.Contains(joined, "line 1") || !strings.Contains(joined, "line 3") { + t.Errorf("warning must name the shadowed line and the inserted line, warnings=%v", p.Warnings) + } +} + // SPEC R-8: order-dependent overlapping batches are rejected, not resolved // by input order. Commuting batches are fine. func TestR8_ConflictingBatchRejected(t *testing.T) { diff --git a/internal/plan/zeromatch_test.go b/internal/plan/zeromatch_test.go index 6d07978..d07ba57 100644 --- a/internal/plan/zeromatch_test.go +++ b/internal/plan/zeromatch_test.go @@ -943,3 +943,45 @@ func TestINV6_PartialOverlapWithAPreexistingLaterRuleIsStillRefused(t *testing.T t.Errorf("refusal must name the declared scope, got %q", err.Error()) } } + +// Regression guard from the pre-release review: an UNRECOGNIZED on_zero_match +// value on a zero-match scope must refuse (exit 3), naming the value. Policy +// parsing validates the enum, but the field is exported with a json tag, so a +// library caller — or a value the policy layer learns before the planner does +// — can carry anything. Before the fix no switch arm matched, the op +// synthesized nothing, and the run reported the repo converged with a proven +// tree: the silent no-op rollout this file exists to prevent. +func TestZeroMatch_UnrecognizedOnZeroMatchIsInvalid(t *testing.T) { + tree := []string{"README.md"} + op := ops.Op{Kind: ops.AddOwner, Scope: "/ghost/", Owners: []string{"@b"}, + Raw: "add_owner(/ghost/, @b)", OnZeroMatch: "frob", ID: "bad"} + + _, err := plan.Build([]byte("* @a\n"), tree, []ops.Op{op}, plan.Options{}) + var inv *plan.InvalidError + if !errors.As(err, &inv) { + t.Fatalf("unrecognized on_zero_match on a zero-match scope must be invalid input (exit 3), got %v", err) + } + if !strings.Contains(err.Error(), `"frob"`) { + t.Errorf("refusal must name the unrecognized value, got %q", err.Error()) + } +} + +// Same defense for the sibling switch: an unrecognized on_except_zero_match on +// an op whose except bites nothing must be invalid input (exit 3) naming the +// value — not silently run as `require`, whose exit-2 refusal reads as a +// per-repo problem when the defect is in the policy and identical everywhere. +func TestZeroMatch_UnrecognizedOnExceptZeroMatchIsInvalid(t *testing.T) { + tree := []string{"src/main.go", "README.md"} + op := ops.Op{Kind: ops.AddOwner, Scope: "/src/", Owners: []string{"@b"}, + Raw: "add_owner(/src/, @b)", Except: []string{"/src/ghost/"}, + OnExceptZeroMatch: "frob", ID: "bad"} + + _, err := plan.Build([]byte("* @a\n"), tree, []ops.Op{op}, plan.Options{}) + var inv *plan.InvalidError + if !errors.As(err, &inv) { + t.Fatalf("unrecognized on_except_zero_match must be invalid input (exit 3), got %v", err) + } + if !strings.Contains(err.Error(), `"frob"`) { + t.Errorf("refusal must name the unrecognized value, got %q", err.Error()) + } +} diff --git a/internal/resolve/resolve_test.go b/internal/resolve/resolve_test.go index 5252f6b..f42031c 100644 --- a/internal/resolve/resolve_test.go +++ b/internal/resolve/resolve_test.go @@ -63,6 +63,21 @@ func TestA8_InvalidLinesDoNotResolve(t *testing.T) { } } +// A pre-existing `\#…` line takes the same path as `!` negation: the pattern +// no longer compiles (S-2/S-6 — GitHub reads such a line as a comment), so the +// line is INVALID, skipped in resolution, and surfaced through the same A-8 +// invalid-line reporting — never a rule this tool edits or counts on. +func TestA8_EscapedLeadingHashLineIsInvalidAndSkipped(t *testing.T) { + f := mustParse(t, "* @global\n\\#tag.md @dead\n") + if got := len(f.InvalidLines()); got != 1 { + t.Fatalf("invalid lines = %d, want 1 (the \\# line)", got) + } + res := resolve.All(f, []string{"#tag.md"}) + if got := res["#tag.md"].Owners; !reflect.DeepEqual(got, []string{"@global"}) { + t.Errorf("owners = %v, want [@global] (the \\# line must be skipped)", got) + } +} + // SPEC INV-3: a pattern matching no tracked file resolves nothing; resolution // answers are only ever about real paths in the tree. func TestINV3_ResolutionIsOverTree(t *testing.T) { diff --git a/tools/fleet/co-own.sh b/tools/fleet/co-own.sh index 28a9b16..0f29fc3 100755 --- a/tools/fleet/co-own.sh +++ b/tools/fleet/co-own.sh @@ -41,8 +41,18 @@ for bin in git jq awk; do done command -v "$TOOL" > /dev/null || { echo "co-own.sh: codeowners-tool not found (set CODEOWNERS_TOOL)" >&2; exit 3; } -# Scope is compared against snapshot paths with slashes normalized away. +# SCOPE reaches the tool (sync --op, verify --scope) exactly as typed: the op +# and its proof must carry the operator's spelling — stripping the anchoring +# slash from /docs/ would widen both to any-depth docs. S is the slash-stripped +# spelling for mechanics only: the branch name, the dedicated-line match (awk +# strips the line's slashes the same way), and jq path-prefix checks. +# ANCHORED mirrors CODEOWNERS matching: any slash before a trailing one anchors +# the pattern to the root; a bare single segment matches at any depth. S="${SCOPE#/}"; S="${S%/}" +case "${SCOPE%/}" in + */*) ANCHORED=1 ;; + *) ANCHORED=0 ;; +esac CFILE="" TMP="$(mktemp -d)" @@ -64,12 +74,23 @@ CFILE="$(jq -r '.codeowners_path // empty' "$TMP/before.json")" [ -n "$CFILE" ] || skip "no CODEOWNERS file" # state SNAPSHOT -> none|no-owner|shared|exclusive, over the in-scope paths. +# In-scope follows the typed spelling: anchored scopes prefix-match from the +# root, unanchored ones also match under any parent. Wildcards are not modeled +# here — such a scope selects no paths and is skipped as "nothing in scope"; +# the tool's snapshot/verify stay the authority on any edit regardless. +# Owners compare under the tool's identity (R-38): @handles fold case, an +# email stays byte-exact. Snapshot preserves file spellings, so byte equality +# here would call @Org/Platform a different owner than --owner @org/platform. state() { - jq -r --arg s "$S" --arg o "$OWNER" ' - [.ownership | to_entries[] | select(.key == $s or (.key | startswith($s + "/"))) + jq -r --arg s "$S" --arg o "$OWNER" --arg anchored "$ANCHORED" ' + def fold: if startswith("@") then ascii_downcase else . end; + [.ownership | to_entries[] + | select(.key as $k | ($k == $s) or ($k | startswith($s + "/")) + or (($anchored == "0") + and (($k | endswith("/" + $s)) or ($k | contains("/" + $s + "/"))))) | .value // []] as $sets | if ($sets | length) == 0 then "none" - elif [$sets[] | index($o)] | any(. == null) then "no-owner" + elif [$sets[] | map(fold) | index($o | fold)] | any(. == null) then "no-owner" elif [$sets[] | length >= 2] | all then "shared" else "exclusive" end' "$1" } @@ -81,13 +102,15 @@ esac # Exclusivity must come from a dedicated "SCOPE OWNER" line; anything else # (a broad sole-owner rule, extra owners, an inline comment) has no move that -# provably stays inside the scope, so it goes to a human instead. +# provably stays inside the scope, so it goes to a human instead. The owner +# match folds like state() does: @handles case-insensitively, emails byte-exact. if ! awk -v s="$S" -v o="$OWNER" ' + function fold(x) { return x ~ /^@/ ? tolower(x) : x } { line = $0; sub(/\r$/, "", line) n = split(line, f, /[ \t]+/); i = 1 if (n > 0 && f[1] == "") i = 2 p = f[i]; sub(/^\/+/, "", p); sub(/\/+$/, "", p) - if (n - i + 1 == 2 && p == s && f[i+1] == o) { deleted = 1; next } + if (n - i + 1 == 2 && p == s && fold(f[i+1]) == fold(o)) { deleted = 1; next } print } END { exit deleted ? 0 : 1 } ' "$REPO/$CFILE" > "$TMP/without-line"; then @@ -123,19 +146,23 @@ if [ "$(state "$TMP/fallback.json")" != shared ]; then # The broader rule drops or loses $OWNER, so deleting alone breaks "stays an # owner". Invert: keep the line, add the broader team(s) to it — one # add_owner with an owner list is one line change (R-33b). - FALLBACK="$(jq -r --arg s "$S" --arg o "$OWNER" ' - [.ownership | to_entries[] | select(.key == $s or (.key | startswith($s + "/"))) + FALLBACK="$(jq -r --arg s "$S" --arg o "$OWNER" --arg anchored "$ANCHORED" ' + def fold: if startswith("@") then ascii_downcase else . end; + [.ownership | to_entries[] + | select(.key as $k | ($k == $s) or ($k | startswith($s + "/")) + or (($anchored == "0") + and (($k | endswith("/" + $s)) or ($k | contains("/" + $s + "/"))))) | .value // []] | unique as $sets | if ($sets | length) != 1 then "" - else [$sets[0][] | select(. != $o)] | join(", ") end' "$TMP/fallback.json")" + else [$sets[0][] | select(fold != ($o | fold))] | join(", ") end' "$TMP/fallback.json")" [ -n "$FALLBACK" ] || restore_and_human "nothing co-ownable behind the line; a human decides who shares $SCOPE" git -C "$REPO" reset -q --hard "$BASE" RC=0 - "$TOOL" sync --repo "$REPO" --op "add_owner($S, [$FALLBACK])" >&2 || RC=$? + "$TOOL" sync --repo "$REPO" --op "add_owner($SCOPE, [$FALLBACK])" >&2 || RC=$? case "$RC" in 0) git -C "$REPO" commit -qam "probe: line amended" ;; - 2) restore_and_human "tool refused to amend: add_owner($S, [$FALLBACK])" ;; - *) restore_and_human "tool exited $RC on add_owner($S, [$FALLBACK])" ;; + 2) restore_and_human "tool refused to amend: add_owner($SCOPE, [$FALLBACK])" ;; + *) restore_and_human "tool exited $RC on add_owner($SCOPE, [$FALLBACK])" ;; esac fi @@ -146,7 +173,7 @@ git -C "$REPO" commit -qm "chore: co-own $SCOPE ($OWNER no longer exclusive)" # every in-scope path is owned by $OWNER plus at least one other team. "$TOOL" snapshot --repo "$REPO" --out "$TMP/after.json" >&2 \ || restore_and_human "snapshot failed after edit" -"$TOOL" verify --before "$TMP/before.json" --after "$TMP/after.json" --scope "$S" >&2 \ +"$TOOL" verify --before "$TMP/before.json" --after "$TMP/after.json" --scope "$SCOPE" >&2 \ || restore_and_human "ownership moved outside $SCOPE; edit discarded" [ "$(state "$TMP/after.json")" = shared ] \ || restore_and_human "end state is not co-owned; edit discarded" diff --git a/tools/fleet/coown_fold_test.go b/tools/fleet/coown_fold_test.go new file mode 100644 index 0000000..0174e7d --- /dev/null +++ b/tools/fleet/coown_fold_test.go @@ -0,0 +1,90 @@ +// Owner-identity tests for co-own.sh: owner comparisons follow the tool's +// R-38 rule — @handles are the same owner regardless of case, emails are +// byte-exact — and snapshot preserves file spellings, so the script must fold +// its own comparisons rather than trust byte equality. +package fleet + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// A dedicated line spelled @Org/Platform is the same owner as +// --owner @org/platform. Byte comparison called this "no-owner" (skipped) or +// missed the dedicated line entirely; the fold must land it in the ordinary +// shared lane, deleting the line. +func TestCoOwn_CaseVariantHandleOnDedicatedLineIsRecognized(t *testing.T) { + needBinaries(t) + repo := mkRepo(t, + "* @org/team_a\n.github @org/team_a @Org/Platform\n.github/workflows @Org/Platform\n", + map[string]string{".github/workflows/ci.yml": "x", ".github/dependabot.yml": "x", "src/main.go": "x"}) + + res, code := runScriptScoped(t, repo, ".github/workflows", "@org/platform") + if code != 0 { + t.Fatalf("exit %d, want 0 (result: %+v)", code, res) + } + if res.Status != "shared" { + t.Fatalf("status %q, want \"shared\" — a case-variant handle is the same owner (R-38)", res.Status) + } + git(t, repo, "checkout", "-q", res.Branch) + // Snapshot preserves the file's spelling, so the surviving broader rule's + // @Org/Platform is what resolution reports. + if got := owners(t, repo, ".github/workflows/ci.yml"); !sameOwners(got, "@org/team_a", "@Org/Platform") { + t.Fatalf("workflows owners after: %v, want exactly {@org/team_a, @Org/Platform}", got) + } + body, err := os.ReadFile(filepath.Join(repo, ".github", "CODEOWNERS")) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(body), ".github/workflows") { + t.Fatalf("the dedicated line survived — the awk match did not fold the handle:\n%s", body) + } +} + +// An email owner is NOT folded (R-38: the local part's case is not ours to +// change). A case-variant email is a different owner — skipped, untouched — +// while a byte-identical one still flows through the shared lane. +func TestCoOwn_EmailOwnerStaysByteExact(t *testing.T) { + needBinaries(t) + + t.Run("case-variant email is a different owner", func(t *testing.T) { + repo := mkRepo(t, + "* @org/team_a\n.github/workflows Ops@Example.com\n", + map[string]string{".github/workflows/ci.yml": "x", "src/main.go": "x"}) + base := git(t, repo, "rev-parse", "HEAD") + + res, code := runScriptScoped(t, repo, ".github/workflows", "ops@example.com") + if code != 0 { + t.Fatalf("exit %d, want 0 (result: %+v)", code, res) + } + if res.Status != "skipped" { + t.Fatalf("status %q, want \"skipped\" — ops@example.com does not own what Ops@Example.com owns", res.Status) + } + if head := git(t, repo, "rev-parse", "HEAD"); head != base { + t.Fatal("HEAD moved on a repo the script had no mandate in") + } + if got := owners(t, repo, ".github/workflows/ci.yml"); !sameOwners(got, "Ops@Example.com") { + t.Fatalf("ownership moved to %v — emails must compare byte-exactly", got) + } + }) + + t.Run("byte-identical email takes the shared lane", func(t *testing.T) { + repo := mkRepo(t, + "* @org/team_a\n.github ops@example.com @org/team_a\n.github/workflows ops@example.com\n", + map[string]string{".github/workflows/ci.yml": "x", "src/main.go": "x"}) + + res, code := runScriptScoped(t, repo, ".github/workflows", "ops@example.com") + if code != 0 { + t.Fatalf("exit %d, want 0 (result: %+v)", code, res) + } + if res.Status != "shared" { + t.Fatalf("status %q, want \"shared\"", res.Status) + } + git(t, repo, "checkout", "-q", res.Branch) + if got := owners(t, repo, ".github/workflows/ci.yml"); !sameOwners(got, "ops@example.com", "@org/team_a") { + t.Fatalf("workflows owners after: %v, want exactly {ops@example.com, @org/team_a}", got) + } + }) +} diff --git a/tools/fleet/coown_scope_test.go b/tools/fleet/coown_scope_test.go new file mode 100644 index 0000000..d03a247 --- /dev/null +++ b/tools/fleet/coown_scope_test.go @@ -0,0 +1,88 @@ +// Scope-spelling tests for co-own.sh: the pattern the operator types must +// flow through to the op the script runs and the verify that proves it — +// anchored stays anchored, unanchored stays unanchored. +package fleet + +import ( + "strings" + "testing" +) + +// An anchored single-segment scope (/docs/) matches only at the root. Here the +// broader rule already lists the owner, so the fix is deleting the dedicated +// line — and the committed diff must remove exactly that anchored rule, adding +// nothing, so deeper directories that merely share the name (sub/docs/) are +// untouched by construction. +func TestCoOwn_AnchoredScopeDiffTouchesOnlyAnchoredRule(t *testing.T) { + needBinaries(t) + repo := mkRepo(t, + "* @org/broad @org/platform\n/sub/docs/ @org/other\n/docs/ @org/platform\n", + map[string]string{"docs/a.md": "a\n", "sub/docs/b.md": "b\n", "src/main.go": "x\n"}) + + res, code := runScriptScoped(t, repo, "/docs/", "@org/platform") + if code != 0 { + t.Fatalf("exit %d, want 0 (result: %+v)", code, res) + } + if res.Status != "shared" { + t.Fatalf("status %q, want \"shared\"", res.Status) + } + // Branch names cannot carry the slashes; the sanitized spelling is for + // naming only, never for the op or its proof. + if res.Branch != "co-own/docs" { + t.Fatalf("branch %q, want \"co-own/docs\"", res.Branch) + } + git(t, repo, "checkout", "-q", res.Branch) + if got := owners(t, repo, "docs/a.md"); !sameOwners(got, "@org/broad", "@org/platform") { + t.Fatalf("docs/a.md owners after: %v, want exactly {@org/broad, @org/platform}", got) + } + if got := owners(t, repo, "sub/docs/b.md"); !sameOwners(got, "@org/other") { + t.Fatalf("sub/docs/b.md owners became %v — the anchored scope leaked to a deeper docs/", got) + } + diff := git(t, repo, "show", "--pretty=format:", "--unified=0", "HEAD") + var removed, added []string + for _, line := range strings.Split(diff, "\n") { + switch { + case strings.HasPrefix(line, "---") || strings.HasPrefix(line, "+++"): + case strings.HasPrefix(line, "-"): + removed = append(removed, strings.TrimSpace(strings.TrimPrefix(line, "-"))) + case strings.HasPrefix(line, "+"): + added = append(added, line) + } + } + if len(removed) != 1 || removed[0] != "/docs/ @org/platform" || len(added) != 0 { + t.Fatalf("committed diff must delete exactly the anchored rule and add nothing:\n%s", diff) + } +} + +// An unanchored single-segment scope (docs) matches at any depth — that is +// what the operator typed, and the script must pass it through rather than +// anchor it: every docs/ directory in the tree ends co-owned, and only the +// dedicated unanchored line goes. +func TestCoOwn_UnanchoredScopePassesThroughUnmodified(t *testing.T) { + needBinaries(t) + repo := mkRepo(t, + "* @org/broad @org/platform\ndocs @org/platform\n", + map[string]string{"docs/a.md": "a\n", "sub/docs/b.md": "b\n", "src/main.go": "x\n"}) + + res, code := runScriptScoped(t, repo, "docs", "@org/platform") + if code != 0 { + t.Fatalf("exit %d, want 0 (result: %+v)", code, res) + } + if res.Status != "shared" { + t.Fatalf("status %q, want \"shared\"", res.Status) + } + if res.Branch != "co-own/docs" { + t.Fatalf("branch %q, want \"co-own/docs\"", res.Branch) + } + git(t, repo, "checkout", "-q", res.Branch) + // Both depths were in the typed scope, so both end co-owned. + if got := owners(t, repo, "docs/a.md"); !sameOwners(got, "@org/broad", "@org/platform") { + t.Fatalf("docs/a.md owners after: %v, want exactly {@org/broad, @org/platform}", got) + } + if got := owners(t, repo, "sub/docs/b.md"); !sameOwners(got, "@org/broad", "@org/platform") { + t.Fatalf("sub/docs/b.md owners after: %v — the unanchored scope was narrowed", got) + } + if got := owners(t, repo, "src/main.go"); !sameOwners(got, "@org/broad", "@org/platform") { + t.Fatalf("src/main.go owners moved to %v — the script edited outside its scope", got) + } +} diff --git a/tools/fleet/prerelease_bugs_test.go b/tools/fleet/prerelease_bugs_test.go new file mode 100644 index 0000000..751cabd --- /dev/null +++ b/tools/fleet/prerelease_bugs_test.go @@ -0,0 +1,69 @@ +// Regression guard from the pre-release review of co-own.sh: the test began +// life as a failing repro of a confirmed bug and now pins the fixed behavior. +package fleet + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// runScriptScoped is runScript with a caller-chosen scope, for tests about +// scope spelling rather than the fleet rollout shape. +func runScriptScoped(t *testing.T, repo, scope, owner string) (result, int) { + t.Helper() + script, err := filepath.Abs("co-own.sh") + if err != nil { + t.Fatal(err) + } + cmd := exec.Command("bash", script, "--repo", repo, "--scope", scope, "--owner", owner) + cmd.Env = append(os.Environ(), "CODEOWNERS_TOOL="+toolPath, + "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t", + "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t", + ) + stdout, runErr := cmd.Output() + code := 0 + if runErr != nil { + ee, ok := runErr.(*exec.ExitError) + if !ok { + t.Fatalf("run co-own.sh: %v", runErr) + } + code = ee.ExitCode() + t.Logf("stderr:\n%s", ee.Stderr) + } + var res result + if line := strings.TrimSpace(string(stdout)); line != "" { + if err := json.Unmarshal([]byte(line), &res); err != nil { + t.Fatalf("stdout is not one JSON object: %q (%v)", line, err) + } + } + return res, code +} + +// Pre-release finding, fixed: co-own.sh strips the anchoring slash from a single-segment scope +// (`/docs/` becomes the unanchored `docs`, which matches at any depth), so +// both the op it runs and the `verify --scope` that is supposed to prove the +// edit use a broader scope than the operator typed. Ownership changes outside +// the declared scope, and verify blesses it as "all within scope". +func TestCoOwnAnchoredScopeStaysAnchored(t *testing.T) { + needBinaries(t) + repo := mkRepo(t, + "* @org/broad\n/sub/docs/ @org/other\n/docs/ @org/platform\n", + map[string]string{"docs/a.md": "a\n", "sub/docs/b.md": "b\n"}, + ) + before := owners(t, repo, "sub/docs/b.md") + + res, code := runScriptScoped(t, repo, "/docs/", "@org/platform") + if code != 0 { + t.Fatalf("co-own.sh: exit %d, status %q", code, res.Status) + } + + git(t, repo, "checkout", "-q", res.Branch) + after := owners(t, repo, "sub/docs/b.md") + if strings.Join(before, ",") != strings.Join(after, ",") { + t.Errorf("scope /docs/ is anchored to the root, but sub/docs/b.md changed owners: %v -> %v", before, after) + } +}