Validate OpenFGA models against consumer-declared expectations - #29
Merged
Conversation
Centralizing model ownership made consumers depend on a model another repo provisions, with the contract enforced only downstream. This validates each model in auth/models/ against the expectations its consumers publish, so a breaking change fails here rather than in a consumer's test run later. auth/models/consumers.json starts empty: LiturgicalCalendarAPI's expectations file (Task 8 of the openfga-1182-upgrade plan) doesn't exist yet, and a registry entry pointing at a URL that can't be fetched would fail CI on every model-touching PR before there's anything to actually verify. The empty-registry case is a documented pass, not a silent no-op — see auth/models/consumers.README.md for what's pending and why.
Runs auth/validate-expectations.sh on PRs touching auth/models/** or the validator itself, plus workflow_dispatch. No secrets needed: every expectations_url is a public raw.githubusercontent.com URL.
|
Warning Review limit reached
Next review available in: 2 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a Bash validator for OpenFGA models and consumer expectations. Adds registry and test fixtures for validation cases. Adds a self-test harness and GitHub Actions workflow that runs validation on relevant changes. ChangesModel validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant SelfTest
participant ValidateExpectations
participant ModelRegistry
GitHubActions->>SelfTest: run validator fixtures
SelfTest->>ValidateExpectations: execute test cases
ValidateExpectations->>ModelRegistry: load registry and expectations
ModelRegistry-->>ValidateExpectations: return model contracts
ValidateExpectations-->>SelfTest: return status and output
GitHubActions->>ValidateExpectations: validate registered models
ValidateExpectations->>ModelRegistry: read consumer registry
ModelRegistry-->>ValidateExpectations: return registered consumers
ValidateExpectations-->>GitHubActions: return validation result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
includesRelation searched a relation's entire rewrite subtree for any
computedUserset, including ones nested inside tupleToUserset. Against the
real Martyrology model, edition.can_edit is "editor from governed_by" — a
bare TTU whose computedUserset.relation is "editor" but which grants editor
on a *different* object (the governance_body reached via governed_by), not
editor on the edition itself. relation_includes: {"can_edit": ["editor"]}
passed against that model even though can_edit does not include editor on
the same object — a false pass, which for a contract validator is worse
than a false failure: a false failure gets looked at, a false pass does not.
includesRelation now only descends through union/intersection/difference
combinators looking for a direct computedUserset on the target relation,
and never descends into tupleToUserset. Documented in the header comment so
this boundary doesn't get "fixed" back into a looser match later.
Added auth/models/testdata/expectations-ttu-boundary.json, which targets
Martyrology's edition.can_edit — a TTU-only relation with no union wrapper
at all — and must fail under the corrected function (it passed under the
old one).
…t includes
The difference branch of includesRelation reported inclusion if $target
appeared in EITHER base or subtract:
includesRelation(base; $target) or includesRelation(subtract; $target)
A difference rewrite means "base, but not subtract" — a target reachable
only through subtract is being excluded, not included. The old logic was
the same false-pass shape as the tupleToUserset bug fixed in 26f4aa0: a
consumer asserting "viewer includes admin" would pass against a model that
deliberately subtracts admin from viewer.
Corrected to require inclusion via base AND absence from subtract:
includesRelation(base; $target) and (includesRelation(subtract; $target) | not)
Neither model in this repo uses difference today, so this was latent, not
exploitable yet — but the code already handled the construct, just wrongly,
and a gate that can silently approve a violated contract is worth fixing
regardless. Extended the header and function comments to cover both
relation_includes boundaries (tupleToUserset is a different object;
subtract is an exclusion) so neither gets loosened back later.
Added auth/models/testdata/difference-boundary-model.json (a standalone
test-only model, not a real store: document.viewer is "(this OR admin)
MINUS admin") and expectations-difference-boundary.json, which asserts
viewer includes admin and must fail under the corrected function (it passed
under the old one).
…s, not any relation_includes asserts a SUFFICIENT path: holding the target relation, on its own, is enough to hold the named relation — that's LitCal's real invariant (an admin can edit and view because editor/viewer are unions including admin), a sufficiency claim, not a necessity one. Read that way, intersection's any(...) was wrong. For R = A ∩ B, holding a target found only in A does not grant R — B is still required. any(...) reported that as inclusion anyway: the same false-pass shape as the tupleToUserset and difference.subtract cases already fixed, one level further in. Changed to all(...): a target is a sufficient path through an intersection only if it appears in every branch, since only then does holding it satisfy all of them. Rewrote the header and function comments around the single sufficiency rule the four branches all derive from, instead of describing each as an unrelated special case, so the reasoning is reusable if a fifth combinator ever needs the same treatment. Added auth/models/testdata/intersection-boundary-model.json with two relations: needs_both = admin AND editor (target in only one branch — must now fail, passed under the old any(...)) and admin_gated = admin AND admin (target in every branch — must still pass, proving all(...) isn't simply refusing every intersection). Two matching expectations fixtures.
…un it
A contract validator's only unforgivable failure is reporting a contract
satisfied when it is not: a false violation is noisy and gets investigated,
a false pass ships. Three of those, plus a schema hole with the same shape:
- "*" resolved to "types that have a relations block", so deleting a type's
entire relations block dropped it out of scope and PASSED, while deleting
one relation from it failed — the more destructive edit was the one that
slipped through. "*" now resolves to the consumer's declared
required_types when present and to every type in the model when not, and a
type in scope with no relations block fails every relation required of it.
That still keeps a bare `user` out of scope for a consumer that never
declared it, which is what the old exclusion was actually for.
- relation_includes skipped every type that does not define the named
relation, so naming a relation that exists nowhere in the model was
vacuously "satisfied". Per-type skipping is right — a type that does not
define the relation is not in scope for a claim about it — but if no type
in scope defines it at all, the consumer named something the model does
not have, and that is a violation. Under an explicit type key, a missing
named relation on that type is likewise a violation.
- Single-file mode had no JSON-validity guard: an empty or null expectations
file made every rule's `// {}` default fire and printed "No violations",
exit 0. Registry mode already caught this; both now share one check.
- Nothing validated the expectations schema, so `required_relation` or
`relation_include` — singular, a plausible typo — were ignored and the
file reported as satisfied while asserting nothing. Unknown top-level keys
are rejected, rule values are type-checked, and a file declaring no rule
key at all is rejected outright: a contract that asserts nothing must
never be reported as a contract that holds. Rejection exits distinctly
from a violation.
The boundary fixtures were unrunnable as documented — their standalone
models live under auth/models/testdata/ and have no --store name — so the
regression guards for two earlier fixes were dead code. --model-file PATH
validates a model at an arbitrary path, and auth/validate-expectations.selftest.sh
runs all seventeen cases with the exact exit code and message each must
produce. CI now runs that self-test before the registry check: consumers.json
is legitimately empty, so the job's entire prior behaviour was "an empty
registry passes" and not one line of the rewrite walk ever executed.
Also: jq's own failures are trapped and mapped onto the documented 0/1/2/3/64
scheme rather than aborting with jq's exit 5 (an array-rooted, HTML or
truncated file did exactly that); jq stderr no longer folds into the JSON
violation list it is then parsed as; curl follows redirects, so a consumer
moving their file behind a 301 is not a permanent fetch failure; a
single-file evaluation error no longer reports as a fetch failure when
nothing was fetched; and push/merge_group triggers cover changes that reach
main without a PR run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The C1 fix — resolving "*" to the consumer's required_types — was right for
requirements and wrong for prohibitions, and one shared wildcardTypes applied
it to both. The two are claims about different things:
- required_relations and relation_includes say "the types I use must have
these relations": a claim about the consumer's own declared surface, so
required_types is the right scope.
- forbidden_relations says "no type anywhere defines deleter": a claim
about the whole model. Narrowing it to the declared surface lets a type
the consumer never listed carry the forbidden relation and still pass.
A prohibition can only ever be weakened by shrinking its scope, so it takes
every type in the model unconditionally, whether or not required_types is
present. forbidden_types is a plain list and needed no change.
The shipped expectations-valid.json has exactly the vulnerable shape
(required_types present, forbidden_relations on "*"), so a new LitCal type
carrying `deleter` would have slipped through.
wildcardTypes/typesForKey become requirementScope/prohibitionScope and
typesForRequirement/typesForProhibition, with the asymmetry stated in the
file header and again at the definitions. That comment is the point: this is
the second false pass this function has produced from a plausible-looking
uniformity, and re-unifying the two call sites is exactly the edit it has to
survive.
Two self-test cases pin both halves, deliberately sharing one fixture so no
uniform "*" can satisfy both. forbidden-scope-model.json puts `deleter` on a
type outside required_types (which the prohibition must still catch) and
gives that same type an `editor` that does not include `admin` (which the
requirement must NOT reach). Unify on required_types and the first case
fails; unify on the whole model and the second gains a second violation and
fails. Verified by mutating the script both ways.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
auth/validate-expectations.sh (1)
340-354: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that the walk is not transitive across same-object relations.
The
computedUsersetbranch compares the relation name and stops. It does not resolve that relation's own rewrite. So forviewer = this OR editorandeditor = this OR admin,relation_includesreports thatviewerdoes not includeadmin, even though holdingadminis a sufficient path tovieweron the same object.This errs toward a false violation, which matches the header's stated direction. The header enumerates the three constructs the walk deliberately excludes, and this fourth case is not among them. A consumer will hit it and read the message as a bug. Add it to the header, or resolve named same-object relations with a visited-set guard against cycles.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/validate-expectations.sh` around lines 340 - 354, Update the header/comment block for the relation-walk exclusions to explicitly mention that the includesRelation helper does not recurse through same-object named relations, since the computedUserset branch only compares the relation name and stops. Either add this case to the documented non-transitive behaviors or adjust includesRelation itself to follow same-object relation rewrites with a visited-set guard, keeping the current union/intersection/difference walk behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@auth/models/testdata/expectations-missing-named-relation.json`:
- Around line 5-8: Extend the self-test for expectations-missing-named-relation
to assert the diagnostic for the explicit wider_region relation
nonexistent_rel_2, in addition to the existing wildcard nonexistent_rel check.
Ensure the test fails when validation skips the explicit-type case, or separate
that case into its own fixture and assertion.
In `@auth/validate-expectations.sh`:
- Around line 307-308: Update requirementScope in validate-expectations.sh to
fall back to modelTypes when required_types is absent or an empty list, so
wildcard required_relations rules still evaluate every model type. Preserve the
existing declared-type scope when required_types contains one or more types.
- Line 624: Update the failure message in validate-expectations to replace the
duplicated wording “expectations violation(s) violated” with “expectations
violation(s) found,” matching the wording used at line 627 while preserving the
rest of the message.
- Around line 585-592: Enforce HTTPS for expectations_url values during registry
validation, rejecting non-HTTPS strings before fetching. Update the curl
invocation in the expectations fetch flow to include HTTPS-only protocol and
redirect restrictions, and set an appropriate --max-filesize limit to prevent
unbounded downloads.
---
Nitpick comments:
In `@auth/validate-expectations.sh`:
- Around line 340-354: Update the header/comment block for the relation-walk
exclusions to explicitly mention that the includesRelation helper does not
recurse through same-object named relations, since the computedUserset branch
only compares the relation name and stops. Either add this case to the
documented non-transitive behaviors or adjust includesRelation itself to follow
same-object relation rewrites with a visited-set guard, keeping the current
union/intersection/difference walk behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fc297a72-d61f-4585-a2f6-1f92048decc2
📒 Files selected for processing (24)
.github/workflows/validate-models.ymlauth/models/consumers.README.mdauth/models/consumers.jsonauth/models/testdata/difference-boundary-model.jsonauth/models/testdata/expectations-difference-boundary.jsonauth/models/testdata/expectations-forbidden-scope.jsonauth/models/testdata/expectations-intersection-boundary-fail.jsonauth/models/testdata/expectations-intersection-boundary-pass.jsonauth/models/testdata/expectations-missing-named-relation.jsonauth/models/testdata/expectations-no-rules.jsonauth/models/testdata/expectations-ttu-boundary.jsonauth/models/testdata/expectations-typo-key.jsonauth/models/testdata/expectations-valid.jsonauth/models/testdata/expectations-violating.jsonauth/models/testdata/expectations-wildcard-scope-pass.jsonauth/models/testdata/expectations-wildcard-scope.jsonauth/models/testdata/forbidden-scope-model.jsonauth/models/testdata/intersection-boundary-model.jsonauth/models/testdata/malformed-array.jsonauth/models/testdata/malformed-empty.jsonauth/models/testdata/malformed-null.jsonauth/models/testdata/wildcard-scope-model.jsonauth/validate-expectations.selftest.shauth/validate-expectations.sh
- Pin the C2 self-test's other half: assert the explicit-type "does not define relation" diagnostic, not just the wildcard one, so per-type skipping for a named type can regress and be caught. - Fix requirementScope: an explicit but empty required_types no longer collapses a wildcard requirement's scope to nothing; it falls back to the whole model, same as an absent key. - Reword the mixed violations/fetch-failures summary line to say "found", matching the violations-only line beside it. - Harden the expectations fetch: reject a non-https expectations_url before any fetch (registry entries are consumer-supplied), and add --proto/--proto-redir and a 1 MiB --max-filesize to the curl call. - Document (no behaviour change) that includesRelation's one-hop computedUserset comparison is deliberately non-transitive: it can produce a false violation, never a false pass, and making it transitive would need cycle handling that risks the false-pass shape this script exists to avoid.
Member
Author
|
Re the nitpick in the review summary (auth/validate-expectations.sh:340-354, non-transitive The reasoning: this script's whole design bias, stated in its own header, is "when in doubt, report a problem" — a false violation is noisy and gets investigated, a false pass ships silently. The non-transitive walk ( Making it transitive would require following named same-object relations with a visited-set guard against cycles (an OpenFGA rewrite graph can genuinely cycle). That's a reasonable enhancement, but a mishandled cycle-detection edge case risks flipping this into a false pass instead — reporting a relation as included via a rewrite that doesn't actually resolve, which is exactly the failure shape every other fix in this file (TTU, intersection, difference, the C2/C-empty-required-types fixes) exists to close. Given that asymmetry, I left the walk as-is and added a bullet to the existing "deliberately excluded constructs" block in the header explaining why, rather than reaching for cycle handling to fix a failure mode that's already safe. |
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
1 participant
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.
Summary
Adds the provider-side half of the model contract between cdcf-infra and its
consumers (Phase 2, Tasks 6-7 of
docs/superpowers/plans/2026-08-04-openfga-1182-upgrade.md).Centralizing OpenFGA model ownership in cdcf-infra (#26) turned each
consumer's own contract test into an assertion against a model this repo
provisions. That contract was enforced only downstream, by an integration
test that skips outright when no store is configured — so a breaking model
change could merge here, sync to the VPS, and only surface as a consumer's
test failure later, with that consumer's own CI staying green throughout.
auth/validate-expectations.sh— validates every model inauth/models/against expectations each registered consumer publishes (
required_types,required_relations,forbidden_types,forbidden_relations,relation_includes,"*"wildcard for "every type"). Reports everyviolation, not just the first. A fetch failure fails the run with a
message distinct from a violation.
jq/curlonly, no test framework.auth/models/testdata/expectations-{valid,violating}.json— fixturesproving the validator both passes and catches every rule category.
auth/models/consumers.json— the registry, starting empty. LitCal'sexpectations file (Task 8 of the plan) doesn't exist yet and is blocked on
an unmerged PR; a registry entry pointing at a 404 would fail this check
on every model PR before there's anything to actually verify. See
auth/models/consumers.README.mdfor what's pending. An empty registry isa documented pass, not a silent no-op.
.github/workflows/validate-models.yml— runs the validator on PRstouching
auth/models/**or the validator itself, plusworkflow_dispatch.Test plan
bash -ncleanNot merging yet — opening for review per plan constraints.
Summary by CodeRabbit
New Features
Documentation
Tests