Skip to content

feat: fleet automation — sync, check, and policy files - #14

Merged
jordonpeterson merged 9 commits into
mainfrom
feat/sync-policy
Aug 4, 2026
Merged

feat: fleet automation — sync, check, and policy files#14
jordonpeterson merged 9 commits into
mainfrom
feat/sync-policy

Conversation

@jordonpeterson

Copy link
Copy Markdown
Owner

Applying a standard CODEOWNERS policy to 100 repositories is currently three
invocations, a temp file, and a set -e script that dies on the first repo that
was already correct. This makes it one idempotent command per repo, without
weakening the guarantees that make the tool worth using.

codeowners-tool check --policy policy.json     # fail on repo 0, not 100 times
codeowners-tool sync --repo work/foo --policy policy.json --create --format json

The change is purely additive. plan and apply keep their exact semantics and
their R-17 exit codes. All new behavior sits behind sync / check / --policy /
on_zero_match / --create. Every test that existed before this branch passes
unmodified — that was the primary review gate at every stage, and an agent was
tasked with disproving it rather than confirming it.


What's new

sync — plan, gate and apply in one step. No temp file, and the SHA-drift window
closes to zero because there is no handoff.

check — validate a policy with no repository present. It is a verb rather than a
flag because its defining property is that it invalidates every other flag on sync;
as a flag it also sat one token away from --dry-run, where the failure mode is exit 0
across all 100 repos having done nothing.

Policy files — JSON, stdlib-only, so go.mod stays at zero dependencies. The
escalation is monotonic: an op is a bare string until it needs to say something extra,
at which point it becomes a one-field object.

{
  "version": 1,
  "ops": [
    "add_owner(/services/api/, @org/api-team)",
    { "op": "add_owner(**/*.tf, @org/infra)",          "on_zero_match": "skip"    },
    { "op": "add_owner(/.github/workflows/, @org/ci)", "on_zero_match": "declare" }
  ]
}

on_zero_match — zero-match has three legitimate meanings and only the policy
author knows which. require (default) fails this repo; skip moves on
opportunistically; declare writes the rule anyway, for files added later. A global
flag could not express this: a real fleet policy needs skip and declare ops in the
same run.

The exit contract, and the rule behind it

sync returns only 0, 2 or 3.

Code Meaning Fleet script
0 Converged — applied, or already correct continue
2 This repo needs a human record and continue
3 The policy is broken — fails identically everywhere halt

Exit 3 is reserved for repo-independent failures. Everything that depends on
which repo you are standing in is exit 2. That single rule resolved three separate
bugs in the first draft: the default on_zero_match value halted the fleet on the
first repo lacking a targeted path; "no CODEOWNERS without --create" halted it on
roughly repo 3; and 3+ as an open range meant exit 6 (rolled back, a per-repo
anomaly) aborted the run at repo 40 while the operator diagnosed "malformed policy."
It also gives check an exact definition: it validates the exit-3 class and nothing
else.

What declare costs

This is the one place a guarantee weakens, and it is stated in the README rather than
buried.

  • INV-2 is unaffected. A pattern matching nothing tracked cannot move any existing
    path's ownership. Proven over the tree exactly as before. A property test over
    ~430k random (file, tree, declare-op) triples found zero violations.
  • INV-1 weakens into a structural proof (INV-6). There is nothing in the repo to
    check the rule against, so the tool proves the next best thing — that no rule after
    it can override it — and guarantees that by appending at EOF. Ops that took this
    path report "proven": "structural" per op, so a reviewer can find them without
    reading the diff.

One design decision worth flagging: INV-6 originally refused any batch where two
declared scopes overlapped, which made the canonical fleet baseline — CI owns
workflows everywhere, infra owns Terraform where it exists — inexpressible in every
repo forever, and surfaced a policy-level conflict per-repo at exit 2. Now total
shadowing still refuses (a rule that can never win is dead on arrival), partial
overlap between same-batch declares is allowed and disclosed in a warning naming the
winner, and partial overlap with a pre-existing later rule still refuses, because R-1
forbids reordering what is already there.

Two soundness bugs found by adversarial review

An agent was given one question — find a way this writes something unproven — and
found two ways. Both were verified independently before being acted on.

pattern.Contains was unsound on main. Not a regression from this branch:

Compile("**/").Match("a.go") == false   // `**/` matches nothing, ever
Contains("**/", "*")         == true    // but Contains says otherwise
Compile("*").Match("a.go")   == true    // and a.go matches `*`

A concrete path matches the inner pattern and not the outer while Contains answers
true — exactly what TestContainsIsSound forbids, and the defect that test describes
as letting "the planner amend a rule in place and silently hand an owner every future
file that rule will ever match." tokenize did not model the matcher's needSlash
bookkeeping, so a leading ** followed by another compiled to a regex matching
nothing while being modeled as universal. A 384k-pair sweep found 902 unsound pairs,
all in that family; now zero.

anchoredDirPrefix stripped a trailing ** before its wildcard check, so
/src** normalized to /src/ and was declared provably disjoint from /srcx/. It is
not — /src** matches srcx/a.go. One reproducer wrote a declaration that was dead
on arrival while reporting it applied; another accepted an order-dependent batch in
both orders with different resulting files.

Five ways sync could lie to a fleet script

A pre-PR correctness pass built the binary and attacked it with ~30 hostile repos.
All five were invisible until a real run.

  • --file was not contained by --repo. --file ../x/CODEOWNERS --create wrote
    outside the repository, creating directories on the way, at exit 0. An absolute
    --file was silently reinterpreted as repo-relative. plan/apply cannot do this
    because apply requires the file to exist; --create opened it.
  • A successful write could report exit 2 and emit no record, when --out failed
    after the CODEOWNERS write had landed. The fleet's results.jsonl got nothing and
    the script filed a converged repo under needs-human.
  • --repo one level below a repository root wrote a CODEOWNERS GitHub never
    loads, reported applied, and left the real file untouched.
  • --branch REF proved INV-2 against REF's tree and wrote the working tree of
    whatever was checked out.
  • A refused write left an applied-looking record, so summing ops_applied across
    the fleet overcounted changes on repos where nothing was written.

Documentation

The README was rewritten and gated on a 60-second test: a reader given only that
file, and no other context, must be able to say what the tool does, what to type for
one repo, what to type for an org, and what happens when it refuses. Three independent
fresh readers ran it. The second failed — it caught a contradiction where one table
said a zero-match scope was exit 2 and another said 3 — and the third and fourth passed
at 8/10 with all four answers above the reference divider.

Three of them independently asked for the same thing, now fixed: 519 lines about
editing CODEOWNERS never once showed a CODEOWNERS file. It now opens with four lines
before and four after, making the point concrete — @org/api-team survives, where a
hand-written line would have silently replaced it.

tools/gendocs also had a real bug: it picked the package doc by iterating a map,
so which one won was a coin flip, and a 17-line explanation of the fleet-idempotence
failure mode was being dropped entirely. It now emits everything in filename order,
and CI fails on BEHAVIOR.md drift — previously nothing checked, so a stale file
shipped green.

Verification

  • 287 documented tests across 12 packages, all green. 107 were written before any
    implementation existed, by five agents working in parallel against a pinned API.
  • 500k-case differential fuzz against the vendored hmarr oracle: clean.
  • End-to-end against a synthetic fleetcheck passes, four heterogeneous repos
    converge, a repo with no CODEOWNERS gets one created, an opportunistic Terraform op
    skips where there is no Terraform, the declared workflows rule lands at EOF, and the
    catch-all's owner is retained (/services/api/ @org/everyone @org/api-team).
    Three further full passes leave every file byte-identical.
  • Two tests were wrong and were changed, each only after independent verification:
    one asserted a path resolved to an owner that the fixture's own trailing catch-all
    already shadowed, demanding the exact INV-2 violation its comment forbade; the other
    passed main as its "non-HEAD" branch in a repo created with git init -b main.

Rejected alternatives

Rejected Why
Plain-text policy file Nicer for humans, but fleet policies are often generated. Simplicity preserved via the bare-string shorthand.
YAML Would be the first dependency in a zero-dependency repo.
Global --allow-unmatched-scope Cannot express a policy needing skip and declare in the same run.
--json boolean A fourth spelling of "give me JSON" in one binary. --format/--out already existed.
--op composing with --policy An op appended at one call site is invisible to the policy file's reviewers.
--on-empty overriding the policy's on_empty The artifact in git would not be the policy that ran.
Multi-repo iteration inside the tool Cloning, auth, hosts, parallelism and retries are solved by gh/ghorg.
Git writes (branch/commit/PR) Existing non-goal. --summary-out gives the script what it needs for a PR body.
Keeping exit 1 under sync "Already correct" is the common fleet outcome; special-casing it is the tax being removed.
except_repos / per-repo exceptions Real gap, deferred to v2 to keep this focused. Workaround is a second policy file.

Follow-ups, deliberately not in this PR

  • internal/policy is O(n²) in JSON nesting depth (flat 50k-op policies are fine;
    100k-deep nesting takes 24s). Not reachable by any plausible policy.
  • A UTF-8 BOM makes a policy unparseable with a raw encoding/json message — a
    plausible PowerShell artifact given how carefully every other message is worded.
  • ops.checkScope rejects unescaped space and tab but not other control characters, so
    a scope containing NUL can reach the file.
  • pattern.Contains compiles a regex per segment comparison with no cache; it is the
    hot spot in the INV-6 proof and predates this branch.

🤖 Generated with Claude Code

jordonpeterson and others added 8 commits August 3, 2026 17:47
Phase 1 of the fleet-automation work: user experience and documentation,
before any implementation. No behavior change yet.

Design goal: applying a standard CODEOWNERS policy to 100 repos should be
one idempotent command per repo, without weakening the guarantees that make
the tool worth using. The change is purely additive — plan and apply keep
their exact semantics and their R-17 exit codes.

- docs/UX.md: the design record, folded into the PR body and deleted before
  merge. Documents the decisions and, at equal length, what was rejected.
- README.md: rewritten so the four questions a newcomer actually has are all
  answered above the reference divider, with the fleet script moved below it.

Four independent design reviews (CLI ergonomics, config format, README
clarity, and a fresh reader given nothing but the README) found three real
design bugs in the first draft, all fixed here:

- The default zero-match value mapped to "halt the fleet", so a policy naming
  a path some repo lacked died on the first such repo — while the README
  promised, 30 lines earlier, that not-applying was graceful. Exit 3 is now
  reserved for repo-INDEPENDENT failures; anything that depends on which repo
  you are standing in is exit 2. That also gives `check` an exact definition.
- "3+ means halt" was an unsafe open range: exit 6 (rolled back) is a
  per-repo anomaly, and `plan -h` exits 3 today because flag.ErrHelp is not
  special-cased. sync now enumerates its codes exhaustively.
- sync had no --on-empty, but the planner's own error message tells the user
  to pass it — a dead end whose only escape was writing a policy file.

Also renames the zero-match values to require|skip|declare: every op writes,
so "write" did not distinguish that case from the default, and "absent"
collided with the tool's existing notion of absent owners (A-1..A-3).
Three more fresh-reader passes over the README, each with no context but
the file itself. Pass 2 failed the 60-second gate; passes 3 and 4 found
progressively smaller things. Final rating 8/10 with all four newcomer
questions answered above the reference divider.

Contradiction the reader caught and could not resolve without the source:
the zero-match row said exit 2 while the precise taxonomy said exit 3 for
a zero-match scope. Both were right — plan reports 3, sync remaps to 2 —
but nothing said so. There is now an explicit mapping table, and a warning
not to read across the two tables, because sync's code is a function of
the CAUSE behind a precise code rather than of the code itself.

Also drops exit 5 from sync's contract entirely rather than reserving it
for a future API preflight: sync makes no network calls, and the reader
correctly traced that the documented script's catch-all would have turned
a GitHub rate limit into a halted rollout.

Bugs in the documented fleet script, all found by reading it as code:
- an unguarded clone under set -e killed the whole run on one bad clone
- re-running cloned onto populated directories, so every completed repo
  landed in clone-failed on the second pass; there is now a done.txt
  resume list, verified by running the control flow end to end
- wc -l on a happy path where needs-human was never created exited non-zero
  after a completely successful run

Readers 2, 3 and 4 all asked for the same two things, now added: a real
CODEOWNERS file shown before and after (519 lines about editing CODEOWNERS
never once displayed one), and a successful run's output, since the only
terminal transcript in the document was a failure.
107 new tests, 105 of them failing. That is the intended state: the
behavior does not exist yet, and the tests are the specification it will be
built against. The two that pass are deliberate compatibility guarantees —
that OnZeroMatch's zero value preserves R-5 exactly, and the policy error
formatter's shape.

Scaffolding only, no logic: two additive fields on ops.Op, plan.OpResult,
a stubbed internal/policy, and sync/check verbs returning a sentinel 99.
The sentinel is deliberately not one of sync's real codes — a stub
returning 3 would let a test asserting "broken policy" pass vacuously
against unimplemented code.

Five agents wrote the tests in parallel against a pinned Go API, so they
would not each invent a different one. A sixth was told to disprove the
"purely additive, no existing test changes" claim rather than confirm it.
It rebuilt a clean baseline, implemented a realistic slice of Phase 3 in a
scratch copy, and found the claim TRUE — 144/144 still pass — but flagged
that it holds more narrowly than the wording suggests. Its landmines are
now the highest-value thing in this commit:

- Hoisting on_empty enum validation into plan.Build breaks 51 existing
  tests: "" is a legal Options.OnEmpty today and is what nearly every test
  passes. Validation belongs in policy.Parse, on the value the file supplies.
- plan.OpResults silently changes plan's output JSON, which R-16 makes
  apply's sole input — and there is no golden test for that document
  anywhere in the suite. Additivity held here only because nothing checks.
- Implementing skip by filtering opList loses R-16's record of what was
  requested, and an all-skip batch then hits "no operations supplied" and
  exits 3 where the spec requires a per-repo skip and exit 0.
- The R-5 error must hang off the switch's default arm, not a comparison
  against ZeroMatchRequire, or --op-sourced ops (which carry "") break.
  TestT7 and TestExitCodes_NoOpAndInvalid are a real tripwire here.
- rename_owner bypasses the zero-match gate entirely, so rejecting
  on_zero_match on a rename is purely policy.Parse's job; nothing
  downstream will catch a silently ignored field.
…e in CI

The generator picked a package doc by iterating a map, so which file's
comment won was a coin flip once a package had two of them (internal/cli
does now). Free-floating comments -- attached to no declaration -- were
dropped entirely, which quietly lost the best prose in the project: the
R-19 fleet-idempotence preamble, the synthetic-fleet note, the R-21
declare-safety note and the schema-pin rationale are all file preambles
separated from the package clause by a blank line, so go/ast never
recorded them as package docs at all.

Now: packages and files are walked in sorted order, and every file-level
comment group is emitted, attributed to its file, in filename order.
Nothing is dropped, and the choice no longer depends on map iteration.

CI gains a docs job that runs `make docs` and fails on a dirty
`git diff docs/BEHAVIOR.md`, so the anti-drift guarantee is enforced
rather than merely claimed. It is a separate job because the test job is
red by design on this branch and would mask the check.

Regenerated: 144 -> 278 documented tests across 12 packages.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…uite

A cross-file review of the 107 parallel-authored tests found four places
where two agents had asserted incompatible things, and two tests that
already passed against stubs returning 99 — worse than no test, because
they would have gone green on any implementation without pinning anything.

Vacuous passes, both now failing for the right reason:
- fleet_test.go's "exit 2 leaves the file byte-identical" filtered every
  repo with `if code != ExitRefused { continue }`, so the body never ran.
  It now asserts the refusing set is exactly {refuse, zero-require} first.
- policy_test.go's unsupported-version cases asserted only "some error,
  with no Go internals in it", which errNotImplemented satisfies.

The contradictions are resolved in the contract rather than by picking a
side per file:
- plan.Build must return a POPULATED *Plan alongside NoOpError. Today it
  returns nil, and three fleet tests need per-op results from exactly that
  path, because "already correct" is the modal fleet outcome and must still
  report one unchanged result per op.
- Op.ID is "" when the policy gives no id; ops[N] is a display label, never
  a stored value. Half the JSON consumers broke on the other reading.
- An explicitly empty on_zero_match is an error while an ABSENT one means
  require — two states a plain struct field cannot distinguish, so the
  decoder needs key-presence detection. That is contract, not preference.
- --create discovery falls back to the working tree. FindCodeownersPaths
  runs over git ls-tree, so a file created by pass 1 and not yet committed
  was invisible to pass 2: the tool saw "no CODEOWNERS" again, --create
  never overwrites, and no third outcome existed. That is a nightly job
  that can never converge.

Two schema tests close a gap that predates this branch: nothing pinned the
plan JSON document, which R-16 makes apply's sole input, and nothing
pinned SyncRecord's wire format, whose field names are a contract with
users' jq scripts. Both were unprotected because every test unmarshalled
into the same struct that produced them, so a rename kept the suite green
while breaking the documented fleet aggregation. Both were mutation-checked
against deliberate renames in throwaway copies.

Also records SyncRecord.Repo as the --repo argument verbatim: deriving it
from git would be the natural implementation and breaks on macOS, where
t.TempDir() gives /var/... and git gives /private/var/....
internal/policy and internal/plan, implemented in parallel against the tests
written in Phase 2. Both packages are fully green; internal/cli stays red
until sync/check land in Wave 2.

policy: the parser does not use DisallowUnknownFields at all. Two independent
reasons, only the first of which was anticipated — it does not survive a
custom unmarshaler, which the polymorphic string-or-object ops array needs;
and it cannot coexist with the `_`-prefix comment rule, since "ignore THESE
unknown keys, reject THOSE" is not something the struct decode path can
express. So field sets are explicit and per-level, and a single token-level
walk produces the located value tree, the byte offsets and duplicate-key
detection together. That subsumes the traps rather than working around them:
an explicitly-empty enum is distinguishable from an absent key because
presence is a property of the tree, duplicates are caught for every key at
every level rather than the subset encoding/json reports, and Go's raw
"cannot unmarshal into Go value of type" messages cannot escape because no
value is ever decoded into a typed field.

Errors carry file:line:col, name the op by index and id, quote the bad value,
enumerate the legal set, and offer a did-you-mean. They also translate the
revision-1 names — when_absent, write, error — because a policy generated
against the older docs otherwise fails on a name that exists nowhere, leaving
the changelog as the only route to the answer.

plan: declare appends at EOF and its INV-1 obligation is discharged
structurally against the re-parsed output bytes — a rule for that scope
exists, its owners match, and every rule after it is provably disjoint from
the scope. Disjointness is sound-and-incomplete like pattern.Contains: false
means "unproven" and every caller treats unproven as overlapping, so
incompleteness costs a refusal, never a wrong write. INV-2 is untouched and
still proven over the tree exactly as before.

Idempotence works by pattern LANGUAGE rather than text, so spacing, tabs and
inline comments are invisible and an existing rule is amended rather than
duplicated. R-8 gains a second pass for zero-match pairs that decides
commutation by enumerating the owner sets a future path could arrive with,
since the tree-based check is vacuous when neither scope matches anything.

One test was wrong and is fixed: TestR22_DeclareAppendsAtEOFBelowACatchAll
asserted that services/api/main.go resolves to @org/api, but the fixture's
trailing `* @org/everyone` already shadows /services/api/ under S-1, so it
resolves to @org/everyone before any op runs. The assertion demanded the
declare op MOVE a tracked path — the exact INV-2 violation the surrounding
block exists to rule out. Verified independently against the resolver rather
than taken on the implementing agent's word. The test's actual thesis, that
the declared rule lands last and governs files added later, was always fine.
…ound

Wave 2 wires the CLI, and an adversarial review of Wave 1 — asked only to
find a way this writes something unproven — found two ways. Both are fixed
here, and one of them predates this branch entirely.

pattern.Contains was UNSOUND on main. Verified directly:

    Compile("**/").Match("a.go") == false   // `**/` matches nothing, ever
    Contains("**/", "*")         == true    // but Contains says otherwise
    Compile("*").Match("a.go")   == true    // and a.go matches `*`

so a concrete path matches the inner pattern and not the outer while Contains
answers true — exactly what TestContainsIsSound forbids, and the defect that
test says "lets the planner amend a rule in place and silently hand an owner
every future file that rule will ever match". Cause: tokenize does not model
buildPatternRegex's needSlash bookkeeping, so a leading `**` followed by
another `**` compiles to a regex matching nothing while being modeled as
universal. A 384k-pair sweep found 902 unsound pairs, all in that family, now
0. The fix makes Contains answer "cannot prove" for adjacent globstars, which
costs refusals on patterns that match nothing anyway.

anchoredDirPrefix stripped a trailing `**` BEFORE its wildcard check, so
`/src**` normalized to `/src/` and was declared provably disjoint from
`/srcx/`. It is not — `/src**` matches `srcx/a.go`. Two reproducers are now
regression tests: one wrote a declaration that was dead on arrival while
reporting it applied, the other accepted an order-dependent batch in both
orders with different resulting files.

One design decision. INV-6's third obligation refused any batch where a
declared scope overlapped another, which made the canonical fleet baseline —
CI owns workflows everywhere, infra owns Terraform where it exists —
inexpressible in every repo forever, and surfaced a policy-level conflict
per-repo at exit 2, the misclassification the exit-2/exit-3 split exists to
prevent. Now: total shadowing still refuses, because a rule that can never
win is dead on arrival; partial overlap between declares in the SAME batch is
allowed and disclosed in a warning naming both scopes and which one wins,
because the author asked for both and their order expresses the precedence,
exactly as in a hand-written file; partial overlap with a pre-existing later
rule still refuses, since R-1 forbids reordering what is already there.

Verified end to end against a synthetic four-repo fleet: check passes, all
four converge, a repo with no CODEOWNERS gets one created, an opportunistic
Terraform op skips where there is no Terraform, the declared workflows rule
lands at EOF, and the catch-all's owner is RETAINED rather than replaced
(/services/api/ @org/everyone @org/api-team). Three further full passes leave
every file byte-identical.

The README's JSON example was aspirational; it is now real output. It also
now says that ops_applied + ops_skipped need not sum to the op count, since
an already-satisfied op is unchanged and counted by neither.
A correctness review built the binary and attacked it with ~30 hostile
repos. Everything it found was in the new CLI surface, and all five would
have been invisible until a real 100-repo run.

- --file was not contained by --repo, so `--file ../x/CODEOWNERS --create`
  wrote a file OUTSIDE the repository, creating directories on the way, at
  exit 0. An absolute --file was silently reinterpreted as repo-relative and
  built a lookalike tree inside the clone. plan/apply cannot do this because
  apply requires the file to already exist; --create is what opened it.
- A successful write could report exit 2 and emit no record at all: --out
  failing after the CODEOWNERS write had landed took the whole run down with
  it. The fleet's results.jsonl got nothing and the script filed a converged
  repo under needs-human. Sink failures are now warnings; a repo's verdict
  can no longer be flipped by an unwritable log path.
- --repo pointing one level BELOW a repository root wrote a CODEOWNERS that
  GitHub never loads, reported applied, and left the real file untouched.
  A clone layout with one extra directory level would have written 100 dead
  files and reported 100 successes.
- --branch REF proved INV-2 against REF's tree and then wrote the working
  tree of whatever was checked out. Refused rather than silently downgraded
  to --dry-run: a dry run exits 0 having written nothing, which under this
  contract reads as converged, and 100 silently unchanged repos with 100
  green rows is worse than the bug.
- A refused write left an applied-looking record, so summing ops_applied
  across the fleet overcounted changes on repos where nothing was written.

One more test was asserting the bug rather than the behavior.
TestSync_CreateHonorsFileAndRejectsNonHeadBranch passed "main" as its
non-HEAD branch, but initRepo runs `git init -b main` — so it pinned the
rule that naming your own checked-out branch is an error, which halts a
rollout at repo 0 over an argument that was never wrong. The intent was
right and the fixture did not exercise it; it now uses a genuinely
divergent branch, and asserts that --branch main still works. The guard
compares resolved commits instead of the literal string "HEAD", and the
refusal is exit 2 rather than 3, because which ref is checked out is a fact
about the repo rather than about the arguments.

Quality pass alongside: the ops[N] label was spelled in five places across
two packages that form a write/read pair, so a divergence would have made
policy notes silently stop reaching PR bodies with every test still green;
the flag.ErrHelp arm was copy-pasted six times, four of them carrying a
comment naming the wrong command; the INV-6 proof was quadratic in the
declare count via a memo-free helper whose answer never depended on the
caller; and gittree's CODEOWNERS search order had been duplicated verbatim
rather than exported.

docs/UX.md was always marked delete-before-merge — it becomes the PR body.
Copilot AI lite review requested due to automatic review settings August 4, 2026 05:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds new “fleet automation” capabilities to codeowners-tool by introducing policy-file parsing/validation, new sync and check verbs with a fleet-friendly exit-code contract, and additional hardening + schema tests to keep these interfaces stable.

Changes:

  • Introduce JSON policy files (internal/policy) with strict validation (unknown/duplicate keys, enums, helpful suggestions).
  • Add sync (plan+gate+apply in one step) and check (repo-independent policy validation) plus extensive hardening/idempotence and wire-schema tests.
  • Improve determinism and doc drift detection via tools/gendocs changes and a CI job that fails on stale generated docs.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tools/gendocs/main.go Makes doc generation deterministic and includes file-level prose from _test.go files.
README.md Rewrites docs to describe sync/check, policy files, on_zero_match, and fleet scripting patterns.
internal/policy/suggest.go Adds “did you mean” suggestions for policy keys/enum values.
internal/policy/policy.go Implements strict policy parsing/validation and structured, located error reporting.
internal/policy/jsonsrc.go Builds a located JSON value tree with duplicate-key detection and line/col errors.
internal/plan/zeromatch.go Implements on_zero_match semantics, declare structural proof (INV-6), and declare-related warnings/refusals.
internal/plan/schema_test.go Pins the JSON schema for plan output as a contract (R-16).
internal/plan/plan.go Adds per-op results (R-24), integrates zero-match behavior, and runs INV-6 structural proof.
internal/pattern/contains.go Hardens Contains soundness by rejecting adjacent ** globstar segment cases.
internal/pattern/contains_test.go Extends corpus/tests to prevent regressions in Contains soundness for globstar patterns.
internal/ops/ops.go Adds OnZeroMatch and per-op ID to ops plus constants for zero-match policy values.
internal/gittree/gittree.go Exports GitHub CODEOWNERS search order for reuse and consistency.
internal/cli/sync.go Adds sync + check verbs, fleet exit-contract handling, repo/branch/file hardening, and JSONL record emission.
internal/cli/schema_test.go Pins sync --format json record schema (R-24) and status vocabulary.
internal/cli/hardening_test.go Adds regression tests for sink failures, path containment, repo root checks, branch-write refusal, dry-run marking, etc.
internal/cli/fleet_idempotence_test.go Adds fleet-scale idempotence tests to prevent unbounded CODEOWNERS growth (R-19).
internal/cli/cli.go Wires new commands, adds consistent flag parse exit-code mapping, and introduces SyncRecord/status constants.
.github/workflows/ci.yml Adds CI job to regenerate docs and fail if docs/BEHAVIOR.md is stale.
Suppressed comments (1)

internal/cli/sync.go:581

  • Appending the --out write failure warning to rec.Warnings has no effect here: cmdSync already printed warnings before calling emitRecord, stdout has already been written, and renderSummary doesn't include rec.Warnings. Consider removing this append (or render warnings in the summary) to avoid dead state mutation.
	if outPath != "" {
		if werr := os.WriteFile(outPath, line, 0o644); werr != nil {
			w := fmt.Sprintf("write --out %s: %v (the record above is on stdout; this repo's outcome is unaffected)", outPath, werr)
			fmt.Fprintln(stderr, "warning:", w)
			rec.Warnings = append(rec.Warnings, w)
		}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/cli/sync.go
Comment on lines +549 to +552
// belongs to the repository: once the file on disk has converged, no unwritable
// --out path makes that false, and inventing a refusal sends a human to inspect
// a repo that is already correct. The warning goes to stderr immediately and
// into rec.Warnings, so the sinks still downstream (the summary) carry it too.
Writing a demo of the fleet workflow, my own copy of the README's per-op jq
recipe died with "Cannot iterate over null" on the first repo that refused.
Keys with nothing in them are omitted rather than emitted empty, so a refused
record has no .ops at all — and that is precisely the repo an operator most
wants to see in the aggregate. The recipe now guards with // [], and the note
covers ops and changes rather than only warnings.
@jordonpeterson
jordonpeterson merged commit 53c268d into main Aug 4, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants