feat(policy): let on_zero_match=declare and on_unowned=skip compose (R-40b) - #72
Open
jordonpeterson wants to merge 4 commits into
Open
feat(policy): let on_zero_match=declare and on_unowned=skip compose (R-40b)#72jordonpeterson wants to merge 4 commits into
jordonpeterson wants to merge 4 commits into
Conversation
…R-40b) The pair was refused at exit 3 on the reasoning that a declared rule owns files that do not exist yet, "which are unowned by definition" — which conflates unowned with nonexistent. A file that does not exist is in no op's path universe: scope is derived from the tree. The two settings are in fact the two arms of one branch in Build — declare acts where the scope matches no tracked file, skip acts on tracked files that have no owner — so they can never speak about the same path. Together they state one intent across repo shapes: pre-own what does not exist, never close what is open today. The refusal made exactly that policy unstatable, which is the fleet rule it was asked for. R-30 (declare + except) is a different rule and stays refused: there the declared line really does capture future files under the excepted pattern, so the carve promise is void rather than vacuously satisfied. Removes four coupling sites (the Build defense, the defaults contradictory-pair check, and both default-folding exclusions) plus the now-dead check-echo branch; on_zero_match drops out of R-40's legality table, leaving the op kind as the whole table. Six refusal pins become composition pins, and a new e2e runs the real fleet policy across all three repo shapes. Exit-code change, called out per the changelog's standing convention: the pair moves exit 3 -> runs, and because defaults no longer holds either setting back, an op with on_unowned=skip under a defaulted declare moves exit 2 -> exit 0 in a zero-match repo, while an op with declare under a defaulted skip stops granting to open paths. A policy that never states on_unowned is unaffected. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dy6v3LeK6a7wrCip2u66nJ
There was a problem hiding this comment.
🟡 Changes recommended
plan.Build now claims/assumes R-30 (“declare + except”) remains refused, but does not enforce that pairing for exported/library callers, so an explicit Build-level validation should be added or the comment clarified.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the policy/planner semantics to allow on_zero_match: declare to compose with on_unowned: skip (R-40b), removing prior validation that treated the pair as contradictory, and adds coverage/docs/changelog to reflect the new, reachable behavior across repo shapes.
Changes:
- Removes the R-40-era “declare + skip” contradiction checks (including defaults-folding exclusions) so the settings compose on a single op and via
defaults. - Adds plan/policy/cli tests (including an end-to-end fleet-policy test) to pin outcomes across three repo shapes.
- Updates user documentation (
POLICY-FILE,OPERATIONS,BEHAVIOR) and the changelog, including explicit exit-code behavior changes.
File summaries
| File | Description |
|---|---|
| internal/policy/unowned_test.go | Updates R-40 tests to R-40b composition semantics and defaults folding expectations. |
| internal/policy/policy.go | Removes contradictory-defaults rejection and updates legality logic for on_unowned handling. |
| internal/plan/unowned_test.go | Replaces refusal pins with composition behavior tests across repo shapes. |
| internal/plan/plan.go | Removes Build-time rejection of declare + skip and documents composition intent. |
| internal/cli/unowned_test.go | Adjusts CLI “check” expectations and adds an end-to-end fleet policy test across three repo shapes. |
| internal/cli/sync.go | Updates resolved on_unowned reporting now that defaults reach declared ops. |
| docs/POLICY-FILE.md | Updates policy-file reference text to reflect R-40b composition. |
| docs/OPERATIONS.md | Expands on_unowned documentation and adds an R-40b composition example/table. |
| docs/BEHAVIOR.md | Regenerates behavior docs to include new tests and remove obsolete refusal pins. |
| CHANGELOG.md | Adds a “Changed” entry documenting R-40b and the exit-code behavior changes. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review-bot finding on the R-40b lift. The policy validator refuses the pair, but nothing in Build re-stated it, so a library caller's op with OnZeroMatch=declare and a non-empty Except reached the declare branch — where synthDeclare writes the bare scope and never consults the excepted set. The carve vanished in silence and the declared line went on to govern exactly the paths the except existed to protect. The gap predates this PR, but the comment added here contrasts R-40b's composing pair against "R-30, still refused", which was true only in the validator. This makes it true at this layer too, as a sibling of the Build-level defenses R-40 already added for an exported struct. Tree-independent, so exit 3 identically on every repo; no policy or --op path can reach it, since both are refused earlier. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dy6v3LeK6a7wrCip2u66nJ
…ective-assignment-680gi1
The auto-merge dropped the R-40b entry inside the [1.0.0] section, which would have retroactively rewritten released notes. It belongs under [Unreleased]; nothing in the 1.0.0 section is touched. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dy6v3LeK6a7wrCip2u66nJ
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.
The bug
R-40 refused
on_zero_match: declarealongsideon_unowned: skipat exit 3, on the reasoning that "a declared rule exists to own files that do not exist yet, which are unowned by definition, so the two state opposite intents."That conflates unowned with nonexistent. A file that does not exist is in no op's path universe at all — scope is derived from
tree. And mechanically the two settings are the two arms of oneif/elseonlen(set) == 0inplan.Build:declareacts where the scope matches no tracked file,skipacts on tracked files that have no owner. They cannot both fire on one repo, let alone one path.So the refusal forbade a pairing that cannot conflict — and it was precisely the pairing needed to state the rule the feature exists for: pre-own what does not exist, never close what is open today.
What composing gives you
{ "op": "add_owner(/.github/, @org/platform)", "on_zero_match": "declare", "on_unowned": "skip" }/.github/proven: structural/.github/, some ownedleft_open/.github/, all openskipped, no rule written — nothing is closedThe third row is a limit of the format, not a choice: CODEOWNERS has no negation (S-2), so no single line covers future files in a directory without also capturing today's open ones. The tool declines to fake a distinction the format can't express, and the record makes those repos greppable.
R-30 (
declare+except) is a different rule and stays refused — there the declared line genuinely does capture future files under the excepted pattern, so that carve promise is void rather than vacuously satisfied. Pinned by a test.Exit-code changes
Called out per the changelog's standing convention that exit-class moves are stated explicitly:
defaultsno longer holds either setting back from an op stating the other, two already-legal shapes change: an op withon_unowned: skipunderdefaults: {"on_zero_match": "declare"}previously ran asrequireand refused (exit 2) where the scope matched nothing — it now declares and exits 0; an op withon_zero_match: declareunderdefaults: {"on_unowned": "skip"}previously granted to open paths and now leaves them open (different bytes).on_unownedis unaffected in every respect.Those exclusions existed only because the pairing was illegal — folding a default that produced a refusal would have failed a policy on a combination its author never wrote.
Changes
Four coupling sites deleted (
plan.Build's defense, thedefaultscontradictory-pair check, both default-folding exclusions) plus the now-deadcheck-echo branch.on_zero_matchdrops out of R-40's legality table entirely, leaving the op kind as the whole table — a validation rule defensible only by paraphrasing intent was the symptom.Net: 62 lines of validation removed, and the feature gets simpler rather than larger.
Tests
Six refusal pins become composition pins across
plan/policy/cli, plus a new end-to-end (TestR40b_FleetPolicyAcrossThreeRepoShapes) that runs the real fleet policy throughcheckandsyncagainst all three repo shapes in real git repos — a policy that was exit 3 before this change, so none of those outcomes was reachable. Full suite,go vetandgofmtgreen;BEHAVIOR.mdregenerated viamake docs.One fixture note worth recording: the shape-2 repo deliberately has no
.github/CODEOWNERS, because that file would become the governing file under S-8 and, being empty, would turn the fixture into shape 3 by accident.🤖 Generated with Claude Code
https://claude.ai/code/session_01Dy6v3LeK6a7wrCip2u66nJ
Generated by Claude Code