feat: WIP[DO NOT REVIEW] pkg/go semantic validation port to proto generated structs and more#616
Draft
SoulPancake wants to merge 30 commits into
Draft
feat: WIP[DO NOT REVIEW] pkg/go semantic validation port to proto generated structs and more#616SoulPancake wants to merge 30 commits into
SoulPancake wants to merge 30 commits into
Conversation
…pe migration Adds a complete semantic validation engine for OpenFGA authorization models. Migrates all validation code from go-sdk types to openfga/api proto types, resolving merge conflicts with main while preserving all intentional validation logic from the feature branch. - ValidationEngine with 5-phase pipeline: schema, duplicates, semantic, complex ops, wildcards, multi-file, and condition validation - SemanticValidator, CycleDetector, DuplicateDetector, ConditionValidator, ComplexOperationValidator, WildcardValidator, MultiFileValidator - Full proto type migration: AuthorizationModel, TypeDefinition, Userset, RelationReference, Condition using getter chains - Fix infinite recursion in complex operation validator via visited-set tracking - Fix yaml_test_integration to use TransformDSLToProto instead of Must variant
…ion cruft The validation package was the only consumer of github.com/openfga/go-sdk, which the feature branch introduced. Every other pkg/go package (transformer, graph, utils) uses the proto-generated types from openfga/api/proto, which is also what TransformDSLToProto outputs. With validation migrated to proto, go-sdk has no remaining importers, so go mod tidy removes it. - Remove unused strings import and the var _ = strings.Join keep-alive hack - Restore descriptive comments on checkSubsumingUnionMembers / checkRedundantIntersectionMembers stubs explaining what they would check - go mod tidy drops go-sdk and its transitive deps (otel, go-logr, xxhash)
…to the engine ValidateTypeName/ValidateRelationName/ValidateConditionName existed but nothing called them — RunAllValidations had no name-validation phase, so reserved names (self/this) and naming-rule violations produced zero errors. Add ValidateNames, which walks every type, relation, and condition name and applies the reserved-keyword and naming-rule checks, mirroring the JS reference implementation's populateRelations. Wire it as a phase in RunAllValidations after schema validation. This fixes the reserved-name reference cases (self/this for types and relations) in dsl-semantic-validation-cases.yaml. The naming-rule cases still differ on the error-message rule format (anchored vs unanchored regex) — tracked separately as a message-wording reconciliation item.
Document the gap between the Go validator's error messages and the canonical JS reference (pkg/js/util/exceptions.ts), measured against the shared fixture tests/data/dsl-semantic-validation-cases.yaml (currently 6/82 passing). Categorizes the remaining failures into three classes — message wording, wrong raise method/errorType, and cascading/duplicate errors — with concrete JS↔Go message mappings and a suggested order of work. Captures that the YAML harness is currently log-only and should only be made enforcing once messages are reconciled.
…with canonical reference Three message templates in error_collector.go now match the canonical JS reference (pkg/js/util/exceptions.ts) verbatim: - duplicate type: 'the type `x` is a duplicate.' - duplicate restriction: 'the type restriction `x` is a duplicate in the relation `y`.' - undefined condition: '`x` is not a defined condition in the model.' These cases already matched on error count, errorType, and line/column — only the wording differed — so the change is text-only with no logic impact. Updates the unit tests that pinned the old strings. Moves the shared semantic-validation fixture suite from 6/82 to 15/82 passing.
…name errors ValidateTypeName/ValidateRelationName/ValidateConditionName matched names against the anchored pattern (^...$) but reported the unanchored rule string in the error message. The canonical JS reference (pkg/js/validator/validate-dsl.ts) reports the anchored form. Build the anchored pattern once and pass it as the displayed clause so the reported rule matches the rule actually used for matching. Fixes the 'invalid type name' and 'invalid relation name' cases in dsl-semantic-validation-cases.yaml. No logic change; unit tests that pass their own clause directly are unaffected.
The validator collapsed two distinct failures into one: any non-1.1/1.2 version
was reported as 'the schema version X is not supported.' with errorType
schema-version-unsupported. The canonical JS reference distinguishes three cases
via a switch on the version:
- empty -> 'schema version required' (schema-version-required)
- '1.0' -> 'schema version no longer supported' (schema-version-unsupported)
- anything else (0.9, 2.0, ...) -> 'invalid schema X' (invalid-schema)
'1.0' is a recognized-but-retired version; '0.9'/'2.0' were never valid. These are
semantically different and carry different errorTypes and messages.
Changes:
- ValidateSchemaVersion now mirrors the reference switch (empty / 1.0 / default)
- RaiseInvalidSchemaVersion emits 'invalid schema X' with errorType invalid-schema
(previously emitted the 'not supported' wording with the wrong errorType)
- add RaiseSchemaVersionUnsupported ('schema version no longer supported') for the
1.0 path, which previously had no dedicated method
- RaiseSchemaVersionRequired message aligned to 'schema version required'
- update unit tests pinning the old behavior; add a 1.0 retired-version case and a
RaiseSchemaVersionUnsupported test
Fixes the 'unsupported schema version', 'schema required error', and
'allows extend in modular model' cases in dsl-semantic-validation-cases.yaml.
ValidateUnusedConditions was implemented but never invoked by the engine, so a condition defined and never referenced produced no error. The original branch had left the call commented out. Wire it into runConditionValidation alongside the reference and consistency checks. Also align the message to the canonical JS reference: before: condition 'x' is defined but not used. after: `x` condition is not used in the model. Updates the unit tests that pinned the old wording. Fixes the 'extraneous condition' case in dsl-semantic-validation-cases.yaml. The 'handle single name duplicated type and unused condition' case still differs on a cascading entry-point error and error line numbers, addressed separately.
…kind
The validator funneled several distinct reference errors through two generic ones
(RaiseUndefinedType / RaiseUndefinedRelation) with non-canonical wording. The
reference distinguishes them by where the bad reference appears:
- direct restriction [X], X undefined -> '`X` is not a valid type.' (invalid-type)
- direct restriction [X#r], r not on X -> '`r` is not a valid relation for `X`.' (invalid-relation-type)
- computed userset 'define a: b', b missing -> 'the relation `b` does not exist.' (missing-definition)
- TTU 'a from b', b not a relation on type -> '`b` is not a valid relation for `type`.' (invalid-relation-type)
- TTU 'a from b', a missing on b's targets -> 'the `a` relation definition on type `T` is not valid: ...' (invalid-relation-on-tupleset)
Changes:
- validateTypeRestrictions: type-undefined -> RaiseInvalidType; type#relation
undefined -> RaiseInvalidTypeRelation (was RaiseUndefinedType/Relation)
- computed userset references -> RaiseInvalidRelationError
- new validateTupleToUsersetReferences mirrors the reference's two-step TTU check
(from-relation exists and is a plain direct assignment; computed relation
exists on at least one assignable type), with GetRelationNames /
GetDirectlyAssignableTypes helpers
- align RaiseInvalidType / RaiseInvalidTypeRelation / RaiseInvalidRelationOnTupleset
message text to the canonical reference
- remove duplicate undefined-relation TTU checks from the wildcard validator;
it now only checks direct-assignability of an existing tupleset relation
- update unit tests pinning the old errorTypes/messages
Fixes the 'invalid type is used', 'direct relation assignment not found' cases.
Cases that also trigger the cycle/entry-point cascade remain blocked on that
separate reconciliation.
…ference
The validator ran two separate passes — a cycle detector and an entry-point
checker — and emitted two errors per impossible relation ('contains a cycle' and
'has no entry point'). The reference performs a single traversal per relation
that yields exactly one outcome.
Rewrite cycle_detection.go as a faithful port of the reference's
hasEntryPointOrLoop: walk the rewrite tree with a per-relation visited set and
return {hasEntry, loop}. A relation with no entry point produces one error:
- loop reached -> ' is an impossible relation for (potential loop).'
- otherwise -> ' is an impossible relation for (no entrypoint).'
Both carry errorType relation-no-entry-point; the standalone 'contains a cycle'
error and its method are removed.
Two correctness fixes uncovered along the way:
- Direct-assignment usersets are detected via a type switch on the oneof
wrapper (*openfgav1.Userset_This) rather than GetThis() != nil. The DSL
transformer emits Userset_This with a nil inner DirectUserset, so GetThis()
returns nil and every '[user]' relation was wrongly flagged as impossible.
Same fix applied to GetDirectlyAssignableTypes.
- GetRelationLineNumber now treats skipIndex as a start offset (search from
that line onward), matching the reference's slice-based getRelationLineNumber,
and the entry-point caller anchors the search to the type's line so the right
'define' occurrence is found when multiple types share a relation name.
Rewrites the cycle-detection unit tests around the single-outcome model. Fixture:
23/82 -> 45/82.
…rect-assignment check Two changes that suppress duplicate errors for a single root cause, matching the reference's modelValidation structure. Cascade gating in RunAllValidations: relation-reference validation always runs, but duplicate detection, entry-point/cycle detection, complex-operation and wildcard validation now run only when no error has been recorded yet. A model with a bad reference or a duplicate would otherwise also be reported as having no entry point, a redundant union member, an empty difference, etc. — many errors for one cause. The reference skips these later passes once any error exists. Complete the tuple-to-userset direct-assignment check in the relation-reference pass to mirror the reference: each type the 'from' relation is assignable to must be a concrete type — a wildcard or type#relation target is reported as '`from` relation used inside from allows only direct relation.' — and the computed relation must exist on at least one assignable type. GetDirectlyAssignableTypes now returns the full type restrictions rather than just type names so the wildcard/relation cases can be detected. Also align RaiseDuplicateType wording to the reference: 'the partial relation definition `X` is a duplicate in the relation `Y`.' Fixture: 45/82 -> 66/82.
…eference validation When a type is declared more than once, the relation maps are built last-wins, so a relation on an earlier (shadowed) duplicate could be validated against the winning type's definition and produce a spurious 'not a valid relation' error. The reference resolves every relation through its deduped typeMap, effectively validating only the winning definition. Skip type-definition array elements that are shadowed by a later definition of the same name; the winning definition is validated on its own pass, and the duplicate-type error is reported by duplicate detection. Fixes the 'duplicate in type name' case. Fixture: 66/82 -> 67/82.
…aration Relation-reference and duplicate errors reported the line of the first 'define X' in the file, so when several types declared a relation of the same name, every error pointed at the first occurrence. Thread the type's line index into the relation-reference and duplicate-detection passes and use it as the search start offset for GetRelationLineNumber, so each error points at the occurrence inside the correct type block. Fixture: 67/82 -> 73/82.
…tch the reference Three precision fixes so reported positions line up with the canonical reference. - Column resolution in addError now matches the symbol on word boundaries (\bsymbol\b) instead of a plain substring, so e.g. a type named 't' is found at the type position rather than inside 'type'. Symbols containing non-word characters (such as 'user:*') fall back to a substring search. - RaiseInvalidType resolves its column to the assignable-types list after the colon, so when a relation and its restriction share a name (define customer: [customer]) the error marks the type, not the relation key. - GetConditionLineNumber matches the 'condition <name>' declaration rather than any line containing the name as a substring, and treats skipIndex as a start offset like the other line lookups. Every non-skipped case in dsl-semantic-validation-cases.yaml now matches the reference on message, error type, line, and column (75 passed, 0 failed, 7 skipped).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds validation primitives, schema and naming rules, relation, condition, duplicate, cycle, and multi-file checks, a validation engine with YAML-driven harnesses, and matching docs plus JavaScript schema-version support. ChangesOpenFGA validation framework
Sequence Diagram(s)sequenceDiagram
participant ValidateDSL
participant ValidationEngine
participant ErrorCollector
participant ValidateSchemaVersion
participant ValidateNames
participant ValidateRelationReferences
participant ValidateCyclesAndEntryPoints
participant ValidateComplexOperations
participant ValidateWildcardUsage
participant ValidateMultiFileConsistency
participant ConditionValidator
ValidateDSL->>ValidationEngine: RunAllValidations
ValidationEngine->>ValidateSchemaVersion: validate schema version
ValidationEngine->>ValidateNames: validate names
ValidationEngine->>ValidateRelationReferences: validate relation references
ValidationEngine->>ValidateCyclesAndEntryPoints: check cycles and entry points
ValidationEngine->>ValidateComplexOperations: check complex usersets
ValidationEngine->>ValidateWildcardUsage: check wildcard and tuple-to-userset rules
ValidationEngine->>ValidateMultiFileConsistency: check file/module consistency
ValidationEngine->>ConditionValidator: validate unused, referenced, and named conditions
ValidationEngine->>ErrorCollector: collect ValidationErrors
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
hasEntryPointOrLoop previously deep-copied the visited map at the top of every recursive call, mirroring the reference's per-call deepCopy. On a deep linear chain of computed-userset relations this is O(n^2): each of the n frames copies the growing map. The copy only needs to isolate sibling branches from each other — the this/ttu type loops, union/intersection children, and a difference's base/subtract. The single linear successor, the computed-userset tail call, has no sibling and can share the map directly; sharing is in fact required so the visited set accumulates down the chain and a back-edge is still detected as a loop. Move the copy to each branch call site and share on the computed-userset tail. A 500-deep chain drops from ~493ms / 914MB / 500k allocs to ~19ms / 11MB / 7.2k allocs, with identical entry-point and loop outcomes. cycle_detection_stress_test.go locks the behavior in: a 1000-deep chain that must resolve with an entry point and terminate, a 1000-wide union, a self-referential chain that must still be a loop, union sibling isolation, and a stable per-relation no-entry-point count.
RunAllValidations built a fresh SemanticValidator in five phases (ValidateRelationReferences, ValidateCyclesAndEntryPoints, ValidateTupleToUsersetRequirements, ValidateComplexOperations, ValidateWildcardUsage) and a fresh ConditionValidator in two (ValidateConditionReferences, ValidateUnusedConditions). Each constructor walks every type and relation to build its index, so a single validation run re-indexed the whole model seven times. NewValidationEngine now builds one SemanticValidator and one ConditionValidator up front and the engine passes them into each phase. Every exported phase function keeps its (collector, model, lines) signature for external and test callers by delegating to an unexported worker that takes the prebuilt validator; the engine calls the workers directly with the shared instances. Behavior is unchanged (semantic suite stays 75/0/7, race-clean). Indexing work on a 100-type model drops accordingly.
…rror
Three hot paths compiled a regexp on each call:
- name_validation.go compiled the anchored type/relation/condition name rule
for every name. The rules are fixed, so compile them once into a cache
keyed by the anchored rule string (which is also the clause reported in the
error) and have validateFieldValue serve fixed rules from it.
- schema_validation.go recompiled the `\s{2,}` whitespace-collapsing pattern
inside the per-line loop; hoist it to a package-level var.
- error_collector.go wordIndex compiled `\bsymbol\b` per error to find a
symbol's column. The pattern is symbol-dependent, so replace the regexp
with a direct word-boundary scan: walk substring matches and accept the
first whose flanking characters aren't word characters, falling back to a
plain substring search for symbols with non-word characters (e.g. user:*).
This keeps identical results without per-call compilation or shared state.
TestWordIndex locks the boundary and fallback semantics. Behavior unchanged
(semantic suite 75/0/7, columns intact, race-clean).
…rgence The this-branch of hasEntryPointOrLoop returns on the first assignable type whose referenced relation is missing instead of trying later types, matching validate-dsl.ts. The path is unreachable today (the reference pass and cascade gate run first), so it kept the reference's behavior; add a short comment saying so.
TestYAMLTestRunner_ComprehensiveValidation logged FAIL and ERROR cases via t.Logf and never failed, so a regression against the shared cross-language fixtures stayed green. Now that the suite is at 75/0/7, flip those to t.Errorf (naming the offending case) so any mismatch breaks the build, and require at least one passing case so an empty or all-skipped run is caught too. The seven skip:true fixtures remain skipped. Verified by injecting a fault into entry-point detection: the harness reports the failing cases and fails, and passes again once reverted.
Fix lint failures in the semantic validation package: - rename builtin-shadowing variables (error, copy) - remove unused complex-operation wrapper functions - align ErrorCollector/ValidationErrors receiver names - drop unused function parameters (unparam) - use require for error assertions in tests - silence errname for the ValidationErrors collection type
SoulPancake
changed the title
- keywords: route IsReservedTypeName/RelationName through IsReservedKeyword - context/multi_file_validation: preallocate result slices - duplicate_detection: merge union/intersection duplicate checks into checkDuplicatesInOperands - condition_validation: drop dead scanUsersetForConditions (conditions only attach to direct type restrictions) - wildcard_validation: build meta only inside the TTU branch that uses it - semantic_validation: add map size hints when building type/relation indexes - validation_engine: hoist criticalErrorTypes to a package-level var - cycle_detection_test: add TupleToUserset entry-point coverage - rename yaml_test_integration.go to *_test.go so test types stay out of the prod build
Match the adjacent default block's block-scoping style in validate-dsl.ts.
- remove two phantom error-type rows from the README table - drop two dead Related-Errors links in multiple-modules-in-file - repoint allowed-type-schema-10 links to schema-version-unsupported
Jest writes coverage to pkg/js/coverage, not tests/coverage; update the gitignore rule so the generated report is no longer tracked.
There was a problem hiding this comment.
Pull request overview
This PR is a large in-progress port/expansion of OpenFGA model validation, primarily adding a new Go validation engine and related validation phases (schema/name/semantic/cycle/wildcard/conditions), while also tightening JS validation by explicitly rejecting retired schema version 1.0 and adding substantial end-user documentation for validation error codes.
Changes:
- JS: Treat schema
1.0as explicitly unsupported and add a corresponding validation error. - Go: Introduce a new validation engine and port/implement multiple validation phases (schema, names, semantic references, cycles/entrypoints, wildcard + tuple-to-userset requirements, multi-file/module checks, condition checks).
- Docs: Add a comprehensive error-code reference and troubleshooting guide for model validation errors.
Reviewed changes
Copilot reviewed 64 out of 65 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/js/validator/validate-dsl.ts | Rejects schema 1.0 early as unsupported during JSON validation. |
| pkg/js/util/exceptions.ts | Adds schema-version-unsupported error construction + collector method. |
| pkg/js/errors.ts | Removes unused schema-1.0-specific error enum values. |
| pkg/js/.gitignore | Updates ignored coverage output path. |
| pkg/go/validation/wildcard_validation.go | Adds wildcard and tuple-to-userset directness validation in Go. |
| pkg/go/validation/validation_engine.go | Introduces a phased validation engine orchestrating validation passes. |
| pkg/go/validation/test_helpers_test.go | Adds small pointer helpers for tests. |
| pkg/go/validation/semantic_validation.go | Adds semantic validator maps + reference validation for types/relations/ttu. |
| pkg/go/validation/semantic_validation_test.go | Adds tests for semantic validator + reference validation behavior. |
| pkg/go/validation/schema_validation.go | Adds schema version support/retirement handling + schema line lookup. |
| pkg/go/validation/name_validation.go | Adds name validation + line-number helpers with anchored searching. |
| pkg/go/validation/multi_file_validation.go | Adds multi-file/module mapping and validation helpers. |
| pkg/go/validation/keywords.go | Adds reserved keyword handling helpers. |
| pkg/go/validation/keywords_test.go | Adds tests for reserved keyword behavior. |
| pkg/go/validation/errors.go | Defines Go-side validation error types and error structs. |
| pkg/go/validation/errors_test.go | Adds tests for error formatting/types/metadata. |
| pkg/go/validation/duplicate_detection.go | Adds duplicate detection for types/restrictions/operands. |
| pkg/go/validation/cycle_detection.go | Adds cycle/entrypoint detection ported from JS logic. |
| pkg/go/validation/cycle_detection_stress_test.go | Adds stress tests for deep chains and wide unions. |
| pkg/go/validation/context.go | Adds validation context structs/types (supporting types for validation). |
| pkg/go/validation/context_test.go | Adds tests for validation context helpers. |
| pkg/go/validation/condition_validation.go | Adds condition reference/unused/consistency validation. |
| pkg/go/validation/complex_operation_validation.go | Adds initial complex-operation validation scaffolding. |
| pkg/go/go.sum | Updates Go dependency checksums. |
| pkg/go/go.mod | Updates Go module metadata (incl. toolchain line) and indirect deps. |
| pkg/go/CHANGELOG.md | Adds an “Unreleased” section header for pkg/go. |
| docs/validation/model/undefined-type.md | Adds documentation for undefined-type. |
| docs/validation/model/undefined-relation.md | Adds documentation for undefined-relation. |
| docs/validation/model/type-wildcard-relation.md | Adds documentation for type-wildcard-relation. |
| docs/validation/model/tupleuserset-not-direct.md | Adds documentation for tupleuserset-not-direct. |
| docs/validation/model/TROUBLESHOOTING_GUIDE.md | Adds a quick-reference troubleshooting guide for validation errors. |
| docs/validation/model/self-error.md | Adds documentation for self-error. |
| docs/validation/model/schema-version-required.md | Adds documentation for schema-version-required. |
| docs/validation/model/reserved-type-keywords.md | Adds documentation for reserved-type-keywords. |
| docs/validation/model/reserved-relation-keywords.md | Adds documentation for reserved-relation-keywords. |
| docs/validation/model/relation-no-entry-point.md | Adds documentation for relation-no-entry-point. |
| docs/validation/model/README.md | Adds an index of model validation error codes and references. |
| docs/validation/model/multiple-modules-in-file.md | Adds documentation for multiple-modules-in-file. |
| docs/validation/model/missing-definition.md | Adds documentation for missing-definition. |
| docs/validation/model/invalid-wildcard-error.md | Adds documentation for invalid-wildcard-error. |
| docs/validation/model/invalid-type.md | Adds documentation for invalid-type. |
| docs/validation/model/invalid-syntax.md | Adds documentation for invalid-syntax. |
| docs/validation/model/invalid-schema.md | Adds documentation for invalid-schema. |
| docs/validation/model/invalid-schema-version.md | Adds documentation for invalid-schema-version. |
| docs/validation/model/invalid-relation-type.md | Adds documentation for invalid-relation-type. |
| docs/validation/model/invalid-relation-on-tupleset.md | Adds documentation for invalid-relation-on-tupleset. |
| docs/validation/model/invalid-name.md | Adds documentation for invalid-name. |
| docs/validation/model/duplicated-error.md | Adds documentation for duplicated-error. |
| docs/validation/model/different-nested-condition-name.md | Adds documentation for different-nested-condition-name. |
| docs/validation/model/cyclic-relation.md | Adds documentation for cyclic-relation. |
| docs/validation/model/cyclic-error.md | Adds documentation for cyclic-error. |
| docs/validation/model/condition-not-used.md | Adds documentation for condition-not-used. |
| docs/validation/model/condition-not-defined.md | Adds documentation for condition-not-defined. |
| docs/validation/model/assignable-relation-must-have-type.md | Adds documentation for assignable-relation-must-have-type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (28)
pkg/go/validation/yaml_test_integration_test.go-224-228 (1)
224-228: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmpty expected message matches everything.
strings.Contains(..., "")returns true, so an expected error with an emptymsgmatches any actual error. Combined with the count check this may mask incorrect matches. Consider requiring a non-empty message (or matching onErrorTypewhenmsgis blank).🤖 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 `@pkg/go/validation/yaml_test_integration_test.go` around lines 224 - 228, The message check in errorsMatch currently treats an empty expected Message as a wildcard because strings.Contains always succeeds on an empty string, which can hide bad matches. Update YAMLTestRunner.errorsMatch to explicitly handle blank expected.Message by either rejecting it unless another field like ErrorType is used for matching, or by requiring a non-empty message before doing the substring comparison. Keep the existing logic around ValidationError and YAMLExpectedError so the match remains strict when msg is provided.pkg/go/go.mod-5-6 (1)
5-6: 📐 Maintainability & Code Quality | 🟡 Minor
go.sumcontains a stale checksum forgithub.com/kr/textThe
go.sumfile still referencesgithub.com/kr/text v0.2.0, yet the module is absent fromgo.modand no source code imports it. This discrepancy indicatesgo mod tidyhas not been run since the removal. Please rungo mod tidyto clean up the stale entry.🤖 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 `@pkg/go/go.mod` around lines 5 - 6, The Go module metadata is out of sync: go.sum still has a stale github.com/kr/text entry even though go.mod no longer references it. Run go mod tidy for the pkg/go module to remove the obsolete checksum entry and refresh the module files, using go.mod and go.sum as the targets to verify afterward.docs/validation/model/TROUBLESHOOTING_GUIDE.md-100-119 (1)
100-119: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLabel the remaining fenced blocks.
The three prose blocks here are still unlabelled fences, so markdownlint will keep flagging MD040. Use
text/mdor convert them back to regular lists.🤖 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 `@docs/validation/model/TROUBLESHOOTING_GUIDE.md` around lines 100 - 119, The fenced blocks under the troubleshooting headings are still unlabeled, which will keep triggering markdownlint MD040. Update the three code fences in TROUBLESHOOTING_GUIDE to include an explicit language label such as text or md, or convert those sections back into regular markdown lists while keeping the content the same. Focus on the fenced blocks in the “Model Won’t Parse at All”, “Multiple Undefined Errors”, and “Authorization Not Working” sections.Source: Linters/SAST tools
pkg/go/validation/yaml_integration_test.go-271-337 (1)
271-337: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the scenario outcomes, and fix the schema error type.
Both scenario subtests only log the result, so a broken matcher can still pass. The schema case also uses
invalid-schema, which is the wrong error family for aschema 0.9failure; switch this to the schema-version error emitted by the validator.🤖 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 `@pkg/go/validation/yaml_integration_test.go` around lines 271 - 337, The two subtests in yaml_integration_test’s Test error matching functionality and Test schema version validation only log results, so they need real assertions on the returned result fields from runner.RunTestCase to verify the expected status, message, and matched error metadata instead of allowing failures to pass silently. In addition, update the schema-version test case’s expected YAMLErrorMetadata.ErrorType from invalid-schema to the actual schema-version error family emitted by the validator for schema 0.9 so the matcher checks the correct classification.pkg/go/validation/yaml_integration_test.go-84-123 (1)
84-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTurn this smoke test into an assertion.
This loop only logs results, so it can still pass when validation behavior regresses. Please assert the sampled cases instead of treating log output as verification.
🤖 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 `@pkg/go/validation/yaml_integration_test.go` around lines 84 - 123, The “Run sample semantic validation tests” smoke test in yaml_integration_test should assert behavior instead of only logging it, since the current loop can pass even when validation regresses. Update the test around runner.RunTestCase and result.Status to use assertions for the sampled cases, especially for the expected PASS/FAIL outcomes, and keep the t.Logf calls only as diagnostics. Use the existing symbols suite.TestCases, runner.RunTestCase, and result.Status to locate the loop and replace the log-only verification with explicit test assertions.docs/validation/model/undefined-relation.md-25-35 (1)
25-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced code blocks.
These fences are missing language tags (
md040), which reduces markdown tooling quality.Suggested patch
-``` +```fga model @@ -``` +``` -``` +```fga model @@ -``` +``` -``` +```fga model @@ -``` +```Also applies to: 47-58, 62-72
🤖 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 `@docs/validation/model/undefined-relation.md` around lines 25 - 35, Add a language identifier to every fenced code block in this markdown document so the validator stops flagging md040. Update the affected fences in the undefined-relation examples to use the appropriate FGA code fence consistently, and check the other matching examples referenced in the comment to ensure all blocks use the same tagged fence style.Source: Linters/SAST tools
docs/validation/model/undefined-type.md-24-35 (1)
24-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced code blocks.
These fences are missing language tags (
md040), which hurts markdown lint compliance.Suggested patch
-``` +```fga model @@ -``` +``` -``` +```fga model @@ -``` +``` -``` +```fga model @@ -``` +``` -``` +```fga module current_module @@ -``` +```Also applies to: 47-65, 69-79, 83-94
🤖 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 `@docs/validation/model/undefined-type.md` around lines 24 - 35, Add the missing language identifier to each fenced example block in the undefined-type validation docs so the markdown satisfies md040. Update the relevant fenced code blocks in the validation examples to use the same language tag consistently, and make sure both the opening and closing fences remain balanced across all affected snippets.Source: Linters/SAST tools
pkg/go/validation/error_collector_test.go-377-398 (1)
377-398: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a fixture that actually exercises the custom resolver.
This line never places
userafter thefromclause, so the assertion mostly checks the fallback arithmetic instead of the resolver path. A regression in the resolver could still pass.🤖 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 `@pkg/go/validation/error_collector_test.go` around lines 377 - 398, The test in TestErrorCollector_CustomResolver is using input that does not truly exercise the custom resolver path, so update the fixture lines to place the target user after the from clause in a way that RaiseTupleUsersetRequiresDirect must use the custom resolver. Keep the assertions on errors[0].Column and expectedStart, but make the line content and lineIndex in that test align with the actual resolver behavior so the test fails if the resolver regresses.docs/validation/model/invalid-wildcard-error.md-24-34 (1)
24-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the contradictory wildcard example.
[user:*]is later documented here as a valid wildcard pattern, so the “Invalid wildcard syntax” example is self-contradictory. Replace that snippet with a genuinely invalid form or clarify the surrounding rule.🤖 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 `@docs/validation/model/invalid-wildcard-error.md` around lines 24 - 34, The wildcard example under the invalid syntax section is contradictory because [user:*] is documented elsewhere as valid. Update the example in the invalid-wildcard-error documentation to use a truly invalid wildcard form, or adjust the surrounding explanation so it clearly distinguishes invalid wildcard syntax from the valid [user:*] pattern. Use the existing “Invalid wildcard syntax” example block and the related wildcard validation wording to keep the docs consistent.pkg/go/validation/name_validation.go-177-183 (1)
177-183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch condition names on token boundaries.
HasPrefix(trimmedLine, "condition "+conditionName)will also match longer names that share the same prefix (foovsfoobar) and return the wrong line. Parse the declaration token or check the boundary before returning.🤖 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 `@pkg/go/validation/name_validation.go` around lines 177 - 183, The condition-name lookup in the relevant name validation logic matches by simple prefix, so shorter names can incorrectly match longer declarations. Update the matching in the condition-scanning code to use token-boundary-aware parsing or an explicit boundary check before returning, and keep the fix localized around the existing condition declaration search in the validation helper.docs/validation/model/tupleuserset-not-direct.md-9-18 (1)
9-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify that the tupleset must be direct-only.
The prose says the tupleset just needs direct-assignment capability, but the examples and
RaiseTupleUsersetRequiresDirectinpkg/go/validation/error_collector.goreject anyfromrelation that also contains computed paths. Reword this so readers don’t think[type] or ...is allowed.🤖 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 `@docs/validation/model/tupleuserset-not-direct.md` around lines 9 - 18, Update the tuple-to-userset validation text to say the tupleset relation must be direct-only, not merely direct-assignment capable. In the referenced documentation section, revise the explanation and examples so they clearly match the behavior enforced by RaiseTupleUsersetRequiresDirect in error_collector.go: the relation used after from must be a plain [type] assignment and must not include computed paths like [type] or .... Keep the wording aligned with tuple-to-userset and tupleset relation terminology so readers don’t infer that mixed direct/computed relations are valid.docs/validation/model/invalid-relation-on-tupleset.md-58-58 (1)
58-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocumented error message doesn't match the emitted message.
RaiseInvalidRelationOnTupleset(pkg/go/validation/error_collector.go) emits a message of the formthe `<rel>` relation definition on type `<typeDef>` is not valid: `<rel>` does not exist on `<parent>`, which is of type `<type>`.— not the string shown here. Update the doc so it reflects the real output.🤖 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 `@docs/validation/model/invalid-relation-on-tupleset.md` at line 58, The documented error text is incorrect and should match the actual output from RaiseInvalidRelationOnTupleset in error_collector.go. Update the invalid-relation-on-tupleset doc’s Error Message to reflect the emitted wording for the tuple-to-userset validation failure, using the same relation/type placeholders and phrasing currently produced by the validator.docs/validation/model/assignable-relation-must-have-type.md-54-54 (1)
54-54: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocumented error message doesn't match the emitted message.
The actual error raised by
RaiseAssignableRelationMustHaveTypes(pkg/go/validation/error_collector.go) isthe assignable relation '%s' must have at least one assignable type., not the string shown here. Align the doc to avoid misleading users grepping logs.📝 Suggested correction
-**Error Message:** `Assignable relation '`#member`' must specify a type` +**Error Message:** `the assignable relation '`#member`' must have at least one assignable type.`🤖 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 `@docs/validation/model/assignable-relation-must-have-type.md` at line 54, The documented error text for the assignable-relation validation is outdated and does not match the message emitted by RaiseAssignableRelationMustHaveTypes in error_collector.go. Update the error message shown in assignable-relation-must-have-type.md to match the actual emitted wording exactly, using the same phrasing as the validation error so users can grep logs reliably.docs/validation/model/invalid-type.md-30-38 (1)
30-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove invalid type-name examples out of
invalid-type.
ValidateTypeNameraisesInvalidNamefortype User,type my-type, etc. This page should focus on malformed type references, otherwise it duplicatesinvalid-name.mdand mislabels the emitted error code.🤖 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 `@docs/validation/model/invalid-type.md` around lines 30 - 38, Move the `type User`, `type my-type`, and `type 123invalid` examples out of this page because `ValidateTypeName` emits `InvalidName` for those cases, not the type-reference error this document should cover. Keep `invalid-type.md` focused on malformed references in the `document` block and `define viewer/editor` examples, and relocate the invalid type-name samples to `invalid-name.md` so the examples and emitted error code match.docs/validation/model/reserved-relation-keywords.md-100-110 (1)
100-110: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrim the reserved list to actual relation keywords.
RaiseReservedRelationNameonly coversself/this. Keywords likedefine,union,from, andschemaare parser tokens, not relation identifiers, so listing them here sends readers to the wrong error path.🤖 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 `@docs/validation/model/reserved-relation-keywords.md` around lines 100 - 110, The reserved relation keywords list is too broad and includes parser tokens that are not handled by RaiseReservedRelationName. Update the table in reserved-relation-keywords.md to only include actual relation identifiers covered by the validation path, especially self and this, and remove entries like define, union, from, and schema so the doc matches the behavior of RaiseReservedRelationName.docs/validation/model/invalid-schema-version.md-13-17 (1)
13-17: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse schema-version wording, not SemVer wording.
OpenFGA schema versions here are
X.Yreleases, not full semantic versions. Calling them "semantic versioning format" is misleading and makes the examples harder to compare with validator behavior.🤖 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 `@docs/validation/model/invalid-schema-version.md` around lines 13 - 17, The schema version wording in the OpenFGA validation docs is misleading because it describes versions as semantic versioning instead of schema-version releases. Update the text in the invalid-schema-version documentation to use schema-version terminology, keeping the focus on X.Y style releases and adjusting the examples and bullet wording to match the validator behavior. Use the existing schema-version validation section as the reference point when revising the phrasing.docs/validation/model/invalid-schema.md-15-19 (1)
15-19: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
invalid-schemaseparate frominvalid-syntax.This section mixes structural schema failures with indentation/keyword/brace syntax errors. The runtime taxonomy already has a distinct
invalid-syntaxcode, so this page should stay focused on malformed or missing model/schema structure.🤖 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 `@docs/validation/model/invalid-schema.md` around lines 15 - 19, The invalid-schema documentation is mixing schema-structure failures with syntax/formatting errors, so update the taxonomy text in this section to keep it focused on malformed or missing model/schema structure only. Adjust the wording in the invalid-schema content so it references structure-related cases and remove any mentions that belong under invalid-syntax, using the existing invalid-schema section as the place to separate the two categories clearly.docs/validation/model/invalid-name.md-57-57 (1)
57-57: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch the runtime invalid-name message.
ErrorCollector.RaiseInvalidNameemitstype '%s' does not match naming rule: '%s'./relation '%s' of type '%s' does not match naming rule: '%s'.The custom string here doesn't match what users will actually see.🤖 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 `@docs/validation/model/invalid-name.md` at line 57, The invalid-name example text is using a custom message that does not match the runtime output from ErrorCollector.RaiseInvalidName. Update the message in the invalid-name docs example to mirror the actual wording emitted by the invalid-name path, using the same naming-rule phrasing seen in ErrorCollector.RaiseInvalidName for both type and relation cases so the documentation matches what users will see.docs/validation/model/schema-version-unsupported.md-21-38 (1)
21-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExample triggers the wrong error code.
Per
ValidateSchemaVersioninpkg/go/validation/schema_validation.go, only1.0maps toRaiseSchemaVersionUnsupported(thisschema-version-unsupporteddoc). An unrecognized version like2.0falls into thedefaultbranch and raisesRaiseInvalidSchemaVersion(invalid-schema), notschema-version-unsupported. The example here should useschema 1.0(the recognized-but-retired version) to actually trigger this error.📝 Suggested example correction
-## Example 1: Using a version that is not yet supported +## Example: Using a retired schema version The following model would trigger this error:model
- schema 2.0 # Error: Version 2.0 not yet supported
- schema 1.0 # Error: schema 1.0 is no longer supported
type user
type document
relations
define viewer: [user]-**Error Location:** Line 2, `schema 2.0` declaration. +**Error Location:** Line 2, `schema 1.0` declaration.🤖 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 `@docs/validation/model/schema-version-unsupported.md` around lines 21 - 38, Update the schema-version-unsupported example so it matches the behavior of ValidateSchemaVersion in schema_validation.go: use the recognized-but-retired schema version that actually maps to RaiseSchemaVersionUnsupported, not an unrecognized version that would hit RaiseInvalidSchemaVersion. Adjust the sample model in the docs to use schema 1.0 in the example and update the surrounding error message and error location text accordingly.docs/validation/model/cyclic-error.md-25-151 (1)
25-151: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language to fenced code blocks (MD040).
markdownlint flags the DSL fences at Lines 25, 38, 52, 71, 86, 99, 134, and 151 for missing a language specifier. Tag them (e.g.
```fgaor```text) to satisfy the linter.🤖 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 `@docs/validation/model/cyclic-error.md` around lines 25 - 151, Add a language identifier to every fenced code block in this document to satisfy MD040. Update the fences in the cyclic-error markdown examples to use a consistent specifier such as fga or text, and keep the changes limited to the code blocks shown in the cyclical dependency examples and the valid/invalid pattern sections.Source: Linters/SAST tools
pkg/go/validation/condition_validation.go-98-117 (1)
98-117: 🎯 Functional Correctness | 🟡 MinorCorrect the
symbolargument inRaiseInvalidConditionNameInParametercall.The function signature requires
symbol(the context where the error occurred) andconditionNameas distinct parameters. The current invocation passesconditionNamefor both arguments, which loses the context of the relation name. Update the first argument toref.RelationName.Code Change
- collector.RaiseInvalidConditionNameInParameter(conditionName, ref.TypeName, ref.RelationName, conditionName, meta, lineIndex) + collector.RaiseInvalidConditionNameInParameter(ref.RelationName, ref.TypeName, ref.RelationName, conditionName, meta, lineIndex)🤖 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 `@pkg/go/validation/condition_validation.go` around lines 98 - 117, The call to RaiseInvalidConditionNameInParameter in validateConditionReferences is passing the wrong symbol/context value: it uses conditionName for both the symbol and conditionName arguments, which drops the relation context. Update the first argument to use ref.RelationName while keeping conditionName as the invalid condition name, so the error is reported against the correct relation context.pkg/go/validation/multi_file_validation.go-65-75 (1)
65-75: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep file-path normalization symmetric.
addFileModuleMappingstoresfilepath.Clean(file), butGetModulesForFilelooks up the raw argument. A caller passing the original source-info path will miss unless they pre-clean it. Normalize on read too, or stop cleaning the stored key.Suggested fix
func (mfv *MultiFileValidator) GetModulesForFile(filePath string) []string { + filePath = filepath.Clean(filePath) modules := make([]string, 0, len(mfv.moduleToFileMap[filePath]))Also applies to: 144-151
🤖 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 `@pkg/go/validation/multi_file_validation.go` around lines 65 - 75, The file-path mapping is asymmetric: addFileModuleMapping stores a cleaned path, but GetModulesForFile and the related lookup path still use the raw input, so callers can miss matches unless they pre-normalize. Update the read side to apply the same filepath.Clean normalization before looking up fileToModuleMap, or otherwise make the stored and queried keys use the same form throughout MultiFileValidator.docs/validation/model/condition-not-used.md-45-48 (1)
45-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the collector's exact unused-condition wording.
pkg/go/validation/error_collector.go:311-315emits`unused_condition` condition is not used in the model., so the current sentence will not match the runtime error string.Suggested fix
-**Error Message:** `Condition 'unused_condition' is defined but not used in any relation` +**Error Message:** `unused_condition` condition is not used in the model.🤖 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 `@docs/validation/model/condition-not-used.md` around lines 45 - 48, Update the wording in the unused-condition documentation to match the exact runtime message emitted by error_collector.go in the unused-condition path. In the condition-not-used note, replace the current sentence with the collector’s exact phrasing for the `unused_condition` case so the docs and validation output align precisely.docs/validation/model/different-nested-condition-name.md-56-56 (1)
56-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the nested-condition mismatch message consistent.
pkg/go/validation/error_collector.go:317-321emitsthe 'low_access_count' condition has a different nested condition name ('small_access_count').; the currentexpected/foundwording doesn't match that contract.Suggested fix
-**Error Message:** `Condition name mismatch: expected 'low_access_count' but found 'small_access_count'` +**Error Message:** `the 'low_access_count' condition has a different nested condition name ('small_access_count').`🤖 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 `@docs/validation/model/different-nested-condition-name.md` at line 56, The nested-condition mismatch error text is inconsistent with the established contract. Update the message produced by the validation path in error handling so it matches the existing wording used by the relevant collector logic, keeping the same “the 'low_access_count' condition has a different nested condition name ('small_access_count').” style instead of the current expected/found phrasing. Make the change in the error formatting used by the collector so all nested-condition name mismatches report consistently.docs/validation/model/cyclic-relation.md-26-126 (1)
26-126: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language tags to the fenced examples.
markdownlint-cli2is already flagging the plain fences here (for example lines 26, 49, 64, 99, and 107). Adding a language tag liketextto each DSL example will clear MD040.Suggested cleanup
-``` +```textApply the same tag to the other plain fences in this file.
🤖 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 `@docs/validation/model/cyclic-relation.md` around lines 26 - 126, The fenced DSL examples in the cyclic relation doc are missing language identifiers, which triggers markdownlint MD040. Update each plain fenced block in this document to use a language tag such as text, including the examples under the main scenario, the resolution options, the valid/invalid patterns, and the implementation notes. Use the existing fenced blocks as the targets and keep the example content unchanged.Source: Linters/SAST tools
docs/validation/model/condition-not-defined.md-37-40 (1)
37-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch the documented error text to the validator.
pkg/go/validation/error_collector.go:304-309emits`undefined_condition` is not a defined condition in the model., so this relation-scoped wording will drift from the actual contract and any exact-message assertions.Suggested fix
-**Error Message:** `Condition 'undefined_condition' is not defined (referenced in relation 'viewer' of type 'document')` +**Error Message:** `undefined_condition` is not a defined condition in the model.🤖 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 `@docs/validation/model/condition-not-defined.md` around lines 37 - 40, The documented error text does not match the validator output for undefined conditions, so update the wording in the condition-not-defined example to align with the exact message emitted by error_collector.go’s relation validation path. Use the same phrasing produced for the viewer relation in the model validation flow, and keep the documentation consistent with the actual contract so exact-message checks do not drift.pkg/go/validation/multi_file_validation.go-86-95 (1)
86-95: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSort module names before raising the multi-module error.
moduleNamescomes from map iteration, soRaiseMultipleModulesInSingleFilewill emit the module list in a different order across runs. That makes the validator message and any exact-message tests flaky. As perpkg/go/validation/error_collector.go:323-328, the collector joins the slice as-is.Suggested fix
import ( "path/filepath" + "sort" @@ for module := range modules { moduleNames = append(moduleNames, module) } + sort.Strings(moduleNames) collector.RaiseMultipleModulesInSingleFile(file, moduleNames)🤖 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 `@pkg/go/validation/multi_file_validation.go` around lines 86 - 95, The multi-module validation error is using module names collected from map iteration, so the order is nondeterministic and can make messages/tests flaky. In validateMultipleModulesInFile on MultiFileValidator, sort the moduleNames slice before calling ErrorCollector.RaiseMultipleModulesInSingleFile so the joined output is stable; this should align with how RaiseMultipleModulesInSingleFile in ErrorCollector formats the list.docs/validation/model/relation-no-entry-point.md-25-137 (1)
25-137: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language tags to the fenced examples.
markdownlint-cli2is already flagging the plain fences here (for example lines 25, 48, 63, 99, and 112). Adding a language tag liketextto each DSL example will clear MD040.Suggested cleanup
-``` +```textApply the same tag to the other plain fences in this file.
🤖 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 `@docs/validation/model/relation-no-entry-point.md` around lines 25 - 137, The markdown examples in the relation-no-entry-point documentation use plain fenced code blocks, which triggers markdownlint MD040. Update each fenced DSL example in this document to include a language tag such as text, and apply the same fix consistently to all remaining unlabeled fences while keeping the example content unchanged.Source: Linters/SAST tools
🧹 Nitpick comments (10)
pkg/go/validation/validation_engine_test.go (2)
171-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSubtest does not verify the skip behavior.
"Skip complex operation validation"only assertsNotNil; it never compares against the non-skipped run, so it would pass even ifSkipComplexOperationValidationwere ignored. Consider asserting a difference (or specific absence) relative to default options, mirroring the semantic-skip subtest.🤖 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 `@pkg/go/validation/validation_engine_test.go` around lines 171 - 197, The “Skip complex operation validation” subtest in ValidateDSL only checks that errors are non-nil, so it does not prove SkipComplexOperationValidation is honored. Update this test to compare ValidateDSL(model, "", options) against the default behavior or assert the specific complex-operation error is absent when EngineOptions.SkipComplexOperationValidation is true, using the existing ValidateDSL and EngineOptions symbols to keep the skip behavior verified.
274-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest can silently pass without asserting anything.
The model uses
SchemaVersion: "invalid"and is expected to produce errors, but all assertions live insideif report.ValidationErrors.Count() > 0. If validation ever stops emitting errors here (regression), the test passes as a no-op. Assert that errors exist first, then proceed.♻️ Make the expectation explicit
report := CreateValidationReport(model, "", DefaultEngineOptions()) - if report.ValidationErrors.Count() > 0 { - assert.False(t, report.IsValid(), "Invalid model should fail IsValid()") + require.Greater(t, report.ValidationErrors.Count(), 0, "Invalid schema version should produce errors") + assert.False(t, report.IsValid(), "Invalid model should fail IsValid()")🤖 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 `@pkg/go/validation/validation_engine_test.go` around lines 274 - 288, The test in validation_engine_test.go can pass without checking anything because all assertions are guarded by report.ValidationErrors.Count() > 0. Make the expectation explicit by asserting that CreateValidationReport returns validation errors first, then keep the existing IsValid, Summary, and GetErrorsByType checks using report, ValidationErrors, and summary so regressions where no errors are emitted will fail the test.pkg/go/validation/context.go (1)
66-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn modules in deterministic order.
GetModulesForFilebuilds a slice from a map iteration, so output ordering is non-deterministic. If this slice is used in error text, tests/messages can become flaky across runs. Sorting before return will stabilize behavior.Suggested patch
import ( + "slices" openfgav1 "github.com/openfga/api/proto/openfga/v1" ) @@ func (ctx *ValidationContext) GetModulesForFile(filename string) []string { modules := make([]string, 0, len(ctx.FileToModuleMap[filename])) if moduleMap := ctx.FileToModuleMap[filename]; moduleMap != nil { for module := range moduleMap { modules = append(modules, module) } } + slices.Sort(modules) return modules }🤖 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 `@pkg/go/validation/context.go` around lines 66 - 74, The GetModulesForFile method currently returns modules in map iteration order, so its output is non-deterministic. Update ValidationContext.GetModulesForFile to collect the keys from FileToModuleMap[filename] and sort the resulting slice before returning it, so any callers that use this in error text or tests get stable ordering.docs/validation/model/assignable-relation-must-have-type.md (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: add a language to fenced code blocks (MD040).
markdownlint flags the DSL fences at lines 26, 42, 62, 80, 116, and 135. Tagging them (e.g.
```fga/```text) clears the warnings and enables highlighting.🤖 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 `@docs/validation/model/assignable-relation-must-have-type.md` at line 26, Add a fence language tag to the DSL code blocks in the validation docs so markdownlint no longer flags MD040; update the fenced blocks in assignable-relation-must-have-type.md (the ones around the documented examples) to use an appropriate label such as fga for DSL examples or text for plain output, and keep the changes consistent across the same sections so highlighting and linting both pass.Source: Linters/SAST tools
docs/validation/model/invalid-relation-on-tupleset.md (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: add a language to fenced code blocks (MD040).
markdownlint flags the fences at lines 25, 42, 66, 84, 101, 138, and 153. Tagging them clears the warnings.
🤖 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 `@docs/validation/model/invalid-relation-on-tupleset.md` at line 25, The fenced code blocks in the markdown doc are missing language tags, which triggers MD040 warnings. Update the affected fenced blocks in the invalid-relation-on-tupleset document to include an appropriate language identifier (for example, matching the content type) so markdownlint no longer flags them. Focus on the fence sections used throughout the document rather than any surrounding prose.Source: Linters/SAST tools
docs/validation/model/reserved-type-keywords.md (1)
100-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOpen TODO: comprehensive reserved-keyword list is unfinished.
This documents a real gap (the alternatives table at Lines 91-99 only covers a handful of keywords). Worth tracking so the docs don't ship incomplete.
Want me to open an issue to track completing the reserved-keyword list, or draft the full list from
pkg/go/validation/keywords.go?🤖 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 `@docs/validation/model/reserved-type-keywords.md` around lines 100 - 108, The reserved-keywords documentation is still marked as TODO and the list is incomplete. Update the reserved-type-keywords section to replace the placeholder in the reserved keywords list area with the full set of terms, and keep it aligned with the source of truth in pkg/go/validation/keywords.go so the alternatives table and documented keywords are complete and consistent.pkg/go/validation/semantic_validation_test.go (1)
110-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding tuple-to-userset coverage.
The tests thoroughly cover type restrictions and computed-userset references, but
validateTupleToUsersetReferences(the most intricate path insemantic_validation.go, including the mixed-wildcard edge case) is untested here. A couple ofTupleToUsersetcases would harden the suite.Want me to draft tuple-to-userset test cases (valid, missing
from, non-directfrom, missing target)?🤖 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 `@pkg/go/validation/semantic_validation_test.go` around lines 110 - 276, Add tuple-to-userset coverage in TestValidateRelationReferences by introducing cases that exercise ValidateRelationReferences’ tuple-to-userset path, especially validateTupleToUsersetReferences. Use the existing model/setup patterns in semantic_validation_test.go and add tests for a valid TupleToUserset, a missing from relation, a non-direct from relation, and a missing target relation; assert the expected ErrorType and message for each so the mixed-wildcard and tuple-to-userset edge cases are covered.pkg/go/validation/duplicate_detection_test.go (1)
390-512: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an integration case for duplicate operands in a computed-only relation.
TestValidateDuplicatesonly covers duplicate type names and metadata-backed type restrictions. AValidateDuplicatescase with a relation likedefine editor: admin or admin(noDirectlyRelatedUserTypes) would catch the gap noted induplicate_detection.gowhereCheckForDuplicatesInRelationis gated behind the metadata-relations loop.🤖 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 `@pkg/go/validation/duplicate_detection_test.go` around lines 390 - 512, Add a duplicate-detection test for computed-only relations in TestValidateDuplicates. The current coverage misses cases where a relation like define editor: admin or admin has no Metadata.Relations entry, so ValidateDuplicates never exercises CheckForDuplicatesInRelation there. Extend the table-driven tests with an authorization model that includes a computed relation containing duplicate operands and assert it produces a DuplicatedError via NewErrorCollector and ValidateDuplicates.docs/validation/model/multiple-modules-in-file.md (1)
107-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnfinished TODO section left in published doc.
The "Module Organization Best Practices" section is an empty placeholder. Consider filling it in or removing the heading before release so the published doc doesn't surface an empty section.
Want me to draft the best-practices content or open a tracking issue?
🤖 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 `@docs/validation/model/multiple-modules-in-file.md` around lines 107 - 115, The “Module Organization Best Practices” section is still a placeholder in the published documentation. Either replace the TODO block with real guidance content or remove the heading and comment entirely so no empty section remains. Update the markdown in the multiple-modules-in-file doc around the Module Organization Best Practices section, keeping the section structure consistent with the rest of the document.pkg/go/validation/complex_operation_validation.go (1)
196-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStubbed semantic checks acknowledged.
checkSubsumingUnionMembersandcheckRedundantIntersectionMembersare intentional no-ops; fine as placeholders, but the same root concern as the intersection check applies — these won't catch anything until type-restriction analysis is wired in.Happy to open a tracking issue for completing these semantic checks.
🤖 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 `@pkg/go/validation/complex_operation_validation.go` around lines 196 - 204, `checkSubsumingUnionMembers` and `checkRedundantIntersectionMembers` are still intentional no-ops, but they should be clearly marked as placeholder semantic checks so it’s obvious they won’t detect issues until type-restriction analysis exists. Update these methods in `ComplexOperationValidator` to reference the planned type-restriction analysis path and add a tracking reference or TODO for the missing implementation, keeping the stub behavior explicit and discoverable for future completion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e73c3c33-b275-4788-8306-1c02269771e6
⛔ Files ignored due to path filters (1)
pkg/go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (64)
docs/validation/model/README.mddocs/validation/model/TROUBLESHOOTING_GUIDE.mddocs/validation/model/assignable-relation-must-have-type.mddocs/validation/model/condition-not-defined.mddocs/validation/model/condition-not-used.mddocs/validation/model/cyclic-error.mddocs/validation/model/cyclic-relation.mddocs/validation/model/different-nested-condition-name.mddocs/validation/model/duplicated-error.mddocs/validation/model/invalid-name.mddocs/validation/model/invalid-relation-on-tupleset.mddocs/validation/model/invalid-relation-type.mddocs/validation/model/invalid-schema-version.mddocs/validation/model/invalid-schema.mddocs/validation/model/invalid-syntax.mddocs/validation/model/invalid-type.mddocs/validation/model/invalid-wildcard-error.mddocs/validation/model/missing-definition.mddocs/validation/model/multiple-modules-in-file.mddocs/validation/model/relation-no-entry-point.mddocs/validation/model/reserved-relation-keywords.mddocs/validation/model/reserved-type-keywords.mddocs/validation/model/schema-version-required.mddocs/validation/model/schema-version-unsupported.mddocs/validation/model/self-error.mddocs/validation/model/tupleuserset-not-direct.mddocs/validation/model/type-wildcard-relation.mddocs/validation/model/undefined-relation.mddocs/validation/model/undefined-type.mdpkg/go/CHANGELOG.mdpkg/go/go.modpkg/go/validation/complex_operation_validation.gopkg/go/validation/condition_validation.gopkg/go/validation/condition_validation_test.gopkg/go/validation/context.gopkg/go/validation/context_test.gopkg/go/validation/cycle_detection.gopkg/go/validation/cycle_detection_stress_test.gopkg/go/validation/cycle_detection_test.gopkg/go/validation/duplicate_detection.gopkg/go/validation/duplicate_detection_test.gopkg/go/validation/error_collector.gopkg/go/validation/error_collector_test.gopkg/go/validation/errors.gopkg/go/validation/errors_test.gopkg/go/validation/keywords.gopkg/go/validation/keywords_test.gopkg/go/validation/multi_file_validation.gopkg/go/validation/name_validation.gopkg/go/validation/name_validation_test.gopkg/go/validation/schema_validation.gopkg/go/validation/schema_validation_test.gopkg/go/validation/semantic_validation.gopkg/go/validation/semantic_validation_test.gopkg/go/validation/test_helpers_test.gopkg/go/validation/validation_engine.gopkg/go/validation/validation_engine_test.gopkg/go/validation/wildcard_validation.gopkg/go/validation/yaml_integration_test.gopkg/go/validation/yaml_test_integration_test.gopkg/js/.gitignorepkg/js/errors.tspkg/js/util/exceptions.tspkg/js/validator/validate-dsl.ts
💤 Files with no reviewable changes (1)
- pkg/js/errors.ts
…reference
Three parity fixes against the JS validator (validate-dsl.ts):
- ValidateConditionConsistency compared the wrong thing: it raised when a
condition's name was empty, producing false positives and missing real
mismatches. The reference compares each condition's nested name property to
its map key (conditionName != condition.name); match that, and update the
DifferentNestedConditionName message and symbol to the reference's form
("condition key is `X` but nested name property is Y").
- Condition-reference errors now anchor the relation line lookup to the
referencing type's declaration (GetTypeLineNumber -> GetRelationLineNumber),
so the correct `define` is found when several types share a relation name.
- Wildcard validation likewise anchors its relation line lookups to the type
declaration for both the undefined-type and wildcard-with-relation reports.
Tests updated to cover the key-vs-nested-name mismatch and the consistent
empty/empty case. Semantic suite stays 75/0/7.
GetValidationSummary dereferenced err.Metadata unconditionally. The collector always sets it, but a directly-constructed ValidationError (consumer or test) could leave the pointer nil and panic. Skip such entries instead. Also add InvalidSchema to the critical-error set: that is the error type the schema-version check actually raises (matching the reference), so an invalid schema version is now correctly flagged as critical in the summary.
The "invalid wildcard syntax" example used [user:*], which the same doc later lists as a valid pattern. Replace it with [*] (a wildcard missing its type), which is a genuinely invalid form.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Comment on lines
+219
to
+223
| fromRelation := ttu.GetTupleset().GetRelation() | ||
| targetRelation := ttu.GetComputedUserset().GetRelation() | ||
| if fromRelation == "" || targetRelation == "" { | ||
| return | ||
| } |
Comment on lines
+118
to
+123
| func validateTupleToUsersetOperation(collector *ErrorCollector, validator *SemanticValidator, | ||
| typeName, relationName string, ttu *openfgav1.TupleToUserset, meta *Meta, lines []string) { | ||
| tuplesetRelation := ttu.GetTupleset().GetRelation() | ||
| if tuplesetRelation == "" { | ||
| return | ||
| } |
Comment on lines
+137
to
+146
| if ttu := userset.GetTupleToUserset(); ttu != nil { | ||
| target := ttu.GetComputedUserset().GetRelation() | ||
| from := ttu.GetTupleset().GetRelation() | ||
| if target != "" && from != "" { | ||
| return target + " from " + from | ||
| } | ||
| if target != "" { | ||
| return target | ||
| } | ||
| } |
Comment on lines
+152
to
+157
| ttu := rewrite.GetTupleToUserset() | ||
| tupleset := ttu.GetTupleset().GetRelation() | ||
| computed := ttu.GetComputedUserset().GetRelation() | ||
| if tupleset == "" || computed == "" { | ||
| return entryPointResult{} | ||
| } |
Comment on lines
+178
to
+182
| if ttu := userset.GetTupleToUserset(); ttu != nil { | ||
| tuplesetRel := ttu.GetTupleset().GetRelation() | ||
| computedRel := ttu.GetComputedUserset().GetRelation() | ||
| return "ttu:" + tuplesetRel + ":" + computedRel | ||
| } |
Comment on lines
+141
to
+144
| if len(rm.GetDirectlyRelatedUserTypes()) == 0 { | ||
| lineIndex := GetRelationLineNumber(parentRelation, lines, nil) | ||
| collector.RaiseTuplesetNotDirect(tuplesetRelation, typeName, parentRelation, meta, lineIndex) | ||
| } |
Comment on lines
+96
to
+107
| func (cov *ComplexOperationValidator) checkRedundantUnionMembers(collector *ErrorCollector, typeName, relationName string, union *openfgav1.Usersets, lines []string) { | ||
| seenOperations := make(map[string]bool) | ||
| for _, child := range union.GetChild() { | ||
| operationKey := cov.getUsersetOperationKey(child) | ||
| if operationKey == "" { | ||
| continue | ||
| } | ||
| if seenOperations[operationKey] { | ||
| lineIndex := GetRelationLineNumber(relationName, lines, nil) | ||
| meta := cov.getTypeMeta(typeName) | ||
| collector.RaiseRedundantUnionMember(operationKey, relationName, typeName, meta, lineIndex) | ||
| } |
Comment on lines
+1
to
+10
| # Self Error | ||
|
|
||
| **Error Code:** `self-error` | ||
|
|
||
| **Category:** Naming Validation | ||
|
|
||
| ## Summary | ||
|
|
||
| Type or relation names cannot use reserved keywords 'self' or 'this' as they have special meaning in OpenFGA's authorization logic. | ||
|
|
Comment on lines
+25
to
+40
| ``` | ||
| model | ||
| schema 1.1 | ||
|
|
||
| type model # Error: 'model' is a reserved keyword | ||
| relations | ||
| define viewer: [user] | ||
|
|
||
| type schema # Error: 'schema' is a reserved keyword | ||
| relations | ||
| define member: [user] | ||
|
|
||
| type relation # Error: 'relation' is a reserved keyword | ||
| relations | ||
| define owner: [user] | ||
| ``` |
Comment on lines
+25
to
+37
| ``` | ||
| model | ||
| schema 1.1 | ||
|
|
||
| type user | ||
|
|
||
| type document | ||
| relations | ||
| define define: [user] # Error: 'define' is a reserved keyword | ||
| define relation: [user] # Error: 'relation' is a reserved keyword | ||
| define union: [user] # Error: 'union' is a reserved keyword | ||
| define from: [user] # Error: 'from' is a reserved keyword | ||
| ``` |
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.
Description
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
1.0handling to report the correct “schema version unsupported” error.