fix: verify no longer fails on added files; gate docs regeneration in CI - #13
Open
jordonpeterson wants to merge 5 commits into
Open
fix: verify no longer fails on added files; gate docs regeneration in CI#13jordonpeterson wants to merge 5 commits into
jordonpeterson wants to merge 5 commits into
Conversation
parser.ParseDir returns unordered maps, and a directory can hold both the internal and external test packages (plan and plan_test). "First file with a doc comment wins" therefore picked a different package header on different runs: 12 consecutive `make docs` runs produced three distinct files. Walk packages and files in a fixed order, preferring the external _test package — its doc comment describes the package's behavior, where the internal one documents a white-box helper. Output is now byte-identical across runs, which is what makes the README's claim that the docs cannot drift from the tests actually true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`verify` counted any path present in only one snapshot as an ownership
change, and therefore as an INV-2 violation. The two snapshots normally come
from different refs, so this failed the CI recipe documented in the README on
any PR that added a file outside the declared scope — with CODEOWNERS
byte-identical:
changed: web/newfile.js (unowned) -> {@org/frontend}
INVARIANT VIOLATED: 1 path(s) changed outside the declared scope
INV-2 preserves what a path resolved to BEFORE. A path the branch added has
no before, and one it deleted has no after, so neither can violate it. Such
paths now land in Result.Added/Removed and print as added:/removed: lines
without failing the check.
This does not weaken the gate: a CODEOWNERS edit that reassigns a subtree
still shows up on that subtree's pre-existing files, which the new
TestR18_AddedFileDoesNotMaskReassignment pins. Only a scope whose every file
is new to the branch goes unchecked, and there the invariant has nothing to
say.
TestR18_TreeChangesSurface pinned the old behavior and is rewritten as
TestR18_TreeChangesSurfaceButDoNotViolate: tree deltas are still surfaced,
they just no longer fail verification.
The usage banner also now states that omitting --scope asserts nothing
changed, and lists the flags it was missing: --file for audit/snapshot,
--max-size/--warn-size for plan.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
docs/BEHAVIOR.md is generated from the test suite's doc comments, so it is only true as long as every change regenerates it. Nothing enforced that, and the file had already drifted. Adds a `make docs-check` target that regenerates and fails on any diff, wired into the CI test job. This is only meaningful now that gendocs is deterministic — before, the check would have flagged innocent changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Gaps found auditing the README against the implementation: - No CLI reference. --repo, --file, --max-size, --warn-size and --checks were reachable only through --help; --checks is a notable omission next to the prominent A-1..A-12 table, and --file is the only escape hatch when A-10 reports more than one CODEOWNERS file. - Narrowing rules were unexplained. The planner inserts lines the user did not write — the getting-started example itself produces one — and can refuse when no narrowing is expressible, or warn when a synthesized glob is exact today but not provably confined for files added later. New "How the planner edits lines" section covers all three. - The snapshot/verify CI recipe did not say that omitting --scope asserts nothing changed at all, nor how added and deleted files are treated. - "Go 1.24+" understated go.mod, which pins 1.24.7. - Getting started called a repeated plan a no-op without noting it exits 1, which breaks a CI step that runs it unconditionally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes verify so it no longer fails solely due to tree deltas (files added/removed between refs), makes docs/BEHAVIOR.md generation deterministic, and adds a CI gate to ensure generated docs stay in sync with test doc comments. It also updates the README and CLI help text to reflect the audited/updated behavior.
Changes:
- Adjust
verify.Compareto treat paths present in only one snapshot as informationalAdded/Removed, not invariant violations; update tests accordingly. - Make
tools/gendocswalk packages/files deterministically to produce stabledocs/BEHAVIOR.md. - Add
make docs-checkand run it in CI; expand README with CLI reference and clarified semantics.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/gendocs/main.go | Deterministic ordering of parsed packages/files to make doc generation reproducible. |
| internal/verify/verify.go | New Added/Removed reporting for tree-only deltas; invariant violations only for ownership changes on shared paths. |
| internal/verify/verify_test.go | Updates and adds tests to pin the new verify semantics and prevent masking real reassignments. |
| internal/cli/cli.go | Updates usage output and verify printing to surface added/removed paths. |
| Makefile | Adds docs-check target to fail if generated docs differ from committed output. |
| .github/workflows/ci.yml | Runs make docs-check in CI to enforce doc regeneration. |
| docs/BEHAVIOR.md | Regenerated content reflecting updated/added tests and new counts. |
| README.md | Expanded CLI reference and updated documentation for plan/verify semantics and planner behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
73
to
77
| // Compare diffs two snapshots. scopes, when non-empty, are CODEOWNERS | ||
| // patterns declaring where change is allowed; every change outside them is a | ||
| // violation (INV-2 from raw data). With no scopes, ANY change is a violation. | ||
| // A scope that fails to compile is a hard error — silently dropping it would | ||
| // misreport which changes are in scope (found in review). |
| [--repo DIR] [--branch REF] [--file PATH] | ||
| snapshot [--repo DIR] [--branch REF] [--file PATH] [--out snap.json] | ||
| verify --before before.json --after after.json [--scope PATTERN ...] | ||
| (no --scope means: assert NOTHING changed) |
Comment on lines
+104
to
+105
| - **Omitting `--scope` asserts that nothing changed at all.** Scopes are the | ||
| allowlist; with none, every difference is a violation. |
"Add team X to CODEOWNERS" is the most common task and the one where hand-editing silently does the wrong thing, but the README only stated the add_owner/set_owners distinction in a table cell. Adds a worked section with both commands, their real line diffs and resolved-ownership rows, and the reason appending a rule by hand replaces owners rather than adding to them. Also documents owner token syntax, which was absent: the @ is required, and @org/team_a and @team_a both parse but mean different things to GitHub. The repo-wide `*` example shows the whole plan — the catch-all plus the amendment of every rule that would otherwise shadow it, and the previously unowned paths it pulls into ownership. All examples are verbatim output from the current build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audited the README against the implementation and fixed what didn't hold up. Two of the findings were code bugs, not doc bugs.
verifyfailed on any PR that added a fileThe README's CI recipe — snapshot two branches,
verify --scope /services/api/— failed on any PR that added a file outside the declared scope, with CODEOWNERS byte-identical:Comparetreated a path present in only one snapshot as an ownership change. The snapshots come from different refs, so their trees differ on every real PR.INV-2 preserves what a path resolved to before. An added path has no before and a deleted one has no after, so neither can violate it. They now surface as
added:/removed:lines without failing the check.This does not weaken the gate. A CODEOWNERS edit that reassigns a subtree still shows up on that subtree's pre-existing files — pinned by the new
TestR18_AddedFileDoesNotMaskReassignment. Only a scope whose every file is new to the branch goes unchecked, and there the invariant has nothing to say.TestR18_TreeChangesSurfacedeliberately pinned the old semantics. It's rewritten asTestR18_TreeChangesSurfaceButDoNotViolate— deltas are still surfaced, they just no longer fail verification. If the strict tree-identity reading was intentional, this should become a flag instead.make docswas not reproducibledocs/BEHAVIOR.mdis generated from test doc comments, butparser.ParseDirreturns unordered maps andinternal/planholds both theplanandplan_testpackages. "First doc comment wins" picked a different header per run — 12 consecutive runs produced three distinct files. Now walks packages and files in a fixed order, preferring the external_testpackage. Byte-identical across 15 runs.New CI gate
Nothing enforced that
make docswas ever run, and the file had already drifted. Addsmake docs-check(regenerate, fail on diff) to the CI test job. Verified it fails on a stale doc and passes on a clean tree — only meaningful now that generation is deterministic.README
add_ownervsset_ownersexamples with their real diffs and ownership rows, why appending a rule by hand replaces owners, and what a repo-wide*op actually has to touch. Owner token syntax (the@is required) was undocumented too.--repo,--file,--max-size,--warn-sizeand--checkswere reachable only via--help.verifysemantics: omitting--scopeasserts nothing changed; how tree deltas are handled.Go 1.24+→1.24.7, matchinggo.mod.planexits 1, which breaks a CI step running it unconditionally.Verification
make vet,go test -race ./...,make docs-check,make buildall pass. Every command in the README re-run end-to-end against throwaway repos, including the CI recipe in both the benign and the genuinely-violating case.🤖 Generated with Claude Code