feat: coverage short circuit (#837)#2743
Open
matiasmagni wants to merge 24 commits intoPermify:masterfrom
Open
feat: coverage short circuit (#837)#2743matiasmagni wants to merge 24 commits intoPermify:masterfrom
matiasmagni wants to merge 24 commits intoPermify:masterfrom
Conversation
…rmify#837) - Add coverage registry to track visit counts for all logic nodes - Implement AST discovery to register all AND/OR nodes with source positions - Instrument evaluator to track node visits during permission checks - Detect short-circuited nodes (e.g., B in A OR B when A is true) - Report uncovered nodes with exact Line:Column positions - Add comprehensive test coverage for short-circuit detection Closes Permify#837 Co-authored-by: Cursor <cursoragent@cursor.com>
- Introduced PositionInfo message to capture line and column information. - Updated Child message to include PositionInfo for source position tracking. - Modified compiler logic to populate PositionInfo during call compilation. - Adjusted protobuf definitions and generated code accordingly. - Updated docker-compose to change PostgreSQL port mapping. This commit enhances the ability to track source positions in the permission tree, improving debugging and coverage reporting.
4 tasks
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds end-to-end logic coverage: AST discovery with deterministic path IDs and source positions, a thread-safe Registry to register/track/report node visits, CheckEngine instrumentation to propagate coverage paths and detect short-circuits, and CLI/development wiring to surface per-node uncovered logic with line:column locations. Changes
Sequence DiagramsequenceDiagram
participant CLI as "Coverage CLI"
participant Dev as "Development"
participant Disc as "Discovery"
participant Reg as "Registry"
participant Engine as "CheckEngine"
participant Eval as "Evaluator"
CLI->>Dev: RunCoverage(ctx, shape)
Dev->>Reg: NewRegistry()
Dev->>Disc: Discover(parsedSchema, registry)
Disc->>Reg: Register(path, SourceInfo, Type)
Dev->>Engine: SetRegistry(registry)
Engine->>Eval: Check(permission) with ContextWithPath
Eval->>Reg: Track(ctx) / Visit(path)
alt OR short-circuits true
Eval-->>Engine: true (skip other branch)
else OR false
Eval->>Reg: Track(ctx) / Visit(other_path)
end
Dev->>Reg: ReportAll() / Report()
Reg-->>Dev: uncovered nodes + coverage %
Dev-->>CLI: SchemaCoverageInfo + errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/development/development.go (1)
198-212: Wire the registry to the check engine viaSetRegistry.
RunWithShapeinjects the registry into the context and sets it on the Development receiver, but does not callSetRegistryon the check engine. The engine reads coverage data from itsregistryfield (not from context), so coverage tracking will not work in this flow. AddcheckEngine.SetRegistry(registry)or modify the engine to read the registry from context usingRegistryFromContext(ctx).
🤖 Fix all issues with AI agents
In `@docker-compose.yaml`:
- Around line 22-23: The docker-compose service ports mapping was changed to
"5433:5432", which can break developer workflows; revert the ports entry in
docker-compose.yaml back to "5432:5432" unless the change is intentional, and if
intentional add a note in the PR/README explaining why the host port was moved
to 5433; locate the ports block for the PostgreSQL service (the ports: -
"5433:5432" line) and either restore it to - "5432:5432" or keep "5433:5432" and
add documentation describing the rationale and any steps developers must take.
In `@internal/coverage/discovery.go`:
- Around line 24-50: The issue is that discoverEntity registers a PERMISSION
node at a given path, and discoverExpression registers the operator node at the
exact same path, causing a collision in Registry.Register. To fix this, update
discoverExpression to register infix operator nodes at a distinct sub-path such
as path + ".op" or a similarly unique suffix to avoid overwriting the PERMISSION
node. This change ensures both permission and operator nodes are independently
tracked without collision.
In `@internal/engines/check.go`:
- Around line 177-202: In checkRewrite, replace the direct
engine.registry.Visit(...) call with the coverage.Track helper to match
trace()'s pattern: call coverage.Track(ctx, engine.registry,
coverage.GetPath(request.GetPermission(),
request.GetMetadata().GetExclusionPath()), path) so the registry existence and
context-path logic is handled consistently; update the Visit usage in
checkRewrite to use coverage.Track with the existing local variable path and the
computed child path from coverage.GetPath to preserve the correct rewrite-node
coverage semantics.
- Around line 233-239: trace() currently relies on a registry in the context but
none is injected during normal checks; fix by injecting engine.registry into the
context before coverage.Track is called: inside CheckEngine.trace (which returns
a CheckFunction) wrap the incoming ctx with coverage.ContextWithRegistry(ctx,
engine.registry) (so RegistryFromContext(ctx) will return the registry) and then
call coverage.Track with that wrapped context before invoking fn; alternatively,
replace the context lookup by calling engine.registry directly from trace if you
prefer not to mutate ctx—ensure consistency with checkRewrite which already uses
engine.registry.Visit().
In `@internal/engines/coverage_test.go`:
- Around line 41-43: The test setup currently ignores the error returned by
factories.DatabaseFactory which can cause nil dereferences; change the call to
capture the error (e.g., db, err := factories.DatabaseFactory(...)) and fail the
test immediately if err != nil (use t.Fatalf or require.NoError) before calling
factories.SchemaWriterFactory(db) so the test fails fast and avoids using a nil
db.
In `@pkg/development/development.go`:
- Around line 117-166: The RunCoverage function currently uses c.Registry even
when RunWithShape returned errors, causing stale registry data to affect logic
coverage; modify RunCoverage so it clears or nils c.Registry before calling
RunWithShape (or captures the registry snapshot only after a successful run),
then only perform the Registry.Report/ReportAll merge and entity logic coverage
calculations when errors is empty (or nil) and c.Registry is non-nil; update
references in the function to check c.Registry and the result of RunWithShape
(errors) before computing TotalLogicCoverage and iterating ReportAll, and ensure
any early-return on errors avoids using a stale c.Registry.
🧹 Nitpick comments (8)
proto/base/v1/service.proto (1)
856-858: Consider a clearer field name.The field
exclusion_pathis described as "used for coverage tracking," but the name implies exclusion logic rather than coverage instrumentation. A name likecoverage_pathortrace_pathwould better convey its purpose.That said, this is a minor naming concern and doesn't affect functionality.
pkg/dsl/compiler/compiler.go (1)
482-485: AddPositionInfoto Identifier nodes for consistent coverage reporting
PositionInfois only being set forCallnodes (line 482), but not forIdentifiernodes in thecompileIdentifierfunction. Since the PR aims to provide exact source positions for coverage reporting, this inconsistency means relation/attribute references likeownerorparent.adminwill lack position information while rule calls have it.To make coverage position tracking complete across all node types, set
PositionInfoincompileIdentifier:// After setting child.Type in compileIdentifier child.PositionInfo = &base.PositionInfo{ Line: uint32(ident.Idents[0].PositionInfo.LinePosition), Column: uint32(ident.Idents[0].PositionInfo.ColumnPosition), }This should be applied before each return statement that creates an Identifier-based leaf.
internal/coverage/discovery.go (1)
52-73: Handle unexpected leaf expression types explicitly.For non-
Identifier/Callleaf expressions,nodeTypestays empty andSourceInfomay be zeroed, producing ambiguous coverage reports. Consider defaulting to"UNKNOWN"or skipping registration for unsupported leaf types.♻️ Suggested safeguard
switch v := expr.(type) { case *ast.Identifier: if len(v.Idents) > 0 { info = SourceInfo{ Line: int32(v.Idents[0].PositionInfo.LinePosition), Column: int32(v.Idents[0].PositionInfo.ColumnPosition), } } nodeType = "LEAF" case *ast.Call: info = SourceInfo{ Line: int32(v.Name.PositionInfo.LinePosition), Column: int32(v.Name.PositionInfo.ColumnPosition), } nodeType = "CALL" + default: + nodeType = "UNKNOWN" } - r.Register(path, info, nodeType) + if nodeType != "" { + r.Register(path, info, nodeType) + } }pkg/development/coverage/coverage.go (1)
62-82: Registry discovery here is unused.
Runbuilds a registry and callsDiscover, but the registry is never consumed. This adds extra AST traversal and can mislead maintainers into thinking logic coverage is handled here. Either remove it or plumb logic coverage into the returnedSchemaCoverageInfo.🧹 Minimal cleanup (remove unused registry)
- registry := coverage.NewRegistry() - coverage.Discover(p, registry) - - // Logic coverage is handled during scenario execution in calculateEntityCoverages if we wanted to be fully integration-style, - // but since the current coverage tool is static, we'll mark logic nodes as uncovered based on registry report. - // For now, let's just populate the logic coverage in calculateEntityCoverage.internal/coverage/registry.go (4)
9-11: Consider using unexported type for context key.The
contextKeystruct at line 9 is unexported (good), but declaringregistryKeyas a package-level variable at line 11 is fine. However, note thatpathKeyat line 134 uses a different pattern (inline struct type). Consider unifying the approach for consistency.Suggested unified pattern
type contextKey struct{} -var registryKey = contextKey{} +type registryContextKey struct{} +type pathContextKey struct{} ... func ContextWithRegistry(ctx context.Context, r *Registry) context.Context { - return context.WithValue(ctx, registryKey, r) + return context.WithValue(ctx, registryContextKey{}, r) } func RegistryFromContext(ctx context.Context) *Registry { - if r, ok := ctx.Value(registryKey).(*Registry); ok { + if r, ok := ctx.Value(registryContextKey{}).(*Registry); ok { return r } return nil } -type pathKey struct{} func ContextWithPath(ctx context.Context, path string) context.Context { - return context.WithValue(ctx, pathKey{}, path) + return context.WithValue(ctx, pathContextKey{}, path) } func PathFromContext(ctx context.Context) string { - if p, ok := ctx.Value(pathKey{}).(string); ok { + if p, ok := ctx.Value(pathContextKey{}).(string); ok { return p } return "" }
73-81: Non-deterministic ordering in ReportAll().Map iteration order in Go is randomized. If consistent ordering is needed for testing or deterministic output, consider sorting the result by path before returning.
Optional: Sort by path for deterministic output
+import "sort" func (r *Registry) ReportAll() (nodes []NodeInfo) { r.mu.RLock() defer r.mu.RUnlock() for _, node := range r.nodes { nodes = append(nodes, *node) } + sort.Slice(nodes, func(i, j int) bool { + return nodes[i].Path < nodes[j].Path + }) return nodes }
96-103: Silent no-op when visiting unregistered path.
Visit()silently ignores paths that weren't registered. While this may be intentional for graceful handling, it could mask bugs where paths are visited but never registered (e.g., registration/discovery mismatch). Consider logging or adding a debug mode to flag such cases during development.
158-172: Consider consolidating path helper functions.
GetPath(permission, exclusion)has identical logic toAppendPath(permission, exclusion). Consider whether both are needed or if one can be an alias/wrapper of the other for clarity.Optional consolidation
// GetPath helper to build the full coverage path from permission and exclusion. func GetPath(permission, exclusion string) string { - if exclusion == "" { - return permission - } - return fmt.Sprintf("%s.%s", permission, exclusion) + return AppendPath(permission, exclusion) }
Reverts the database port mapping in docker-compose.yaml from 5433:5432 to 5432:5432 to avoid potential conflicts and align with standard developer setups. This addresses the first comment from the coderabbitai bot review.
Updates the 'discoverExpression' function to register infix operator nodes at a distinct sub-path (e.g., 'path.op'). This prevents overwriting the parent PERMISSION node in the coverage registry, ensuring that both the permission and its operator nodes are tracked independently. This addresses the second comment from the coderabbitai bot review.
Updates the `trace` function to inject the coverage registry into the context before tracking visits. This ensures that the `coverage.Track` helper can consistently access the registry from the context, resolving a potential bug where the registry would be missing during normal checks. This addresses the fourth comment from the coderabbitai bot review.
Replaces the direct `engine.registry.Visit` call in `checkRewrite` with the `coverage.Track` helper. This ensures that coverage tracking is handled consistently with the `trace` function, using a context-based approach to manage the registry and path. This addresses the third comment from the coderabbitai bot review.
Updates the coverage test to properly handle the error returned by `factories.DatabaseFactory`. This prevents potential nil dereferences and ensures the test fails fast if the database creation fails. This addresses the fifth comment from the coderabbitai bot review.
Updates the `RunCoverage` function to clear the coverage registry before calling `RunWithShape`. This prevents stale data from affecting logic coverage calculations and ensures that the coverage report is only generated when the run is successful. This addresses the sixth comment from the coderabbitai bot review.
Renames the `exclusion_path` field to `coverage_path` in `PermissionCheckRequestMetadata` within `proto/base/v1/service.proto`. This change provides a clearer name that better reflects its purpose for coverage tracking rather than exclusion logic. The associated comments have also been updated to reflect the new naming. This addresses the seventh comment from the coderabbitai bot review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/engines/check.go`:
- Line 183: Remove the trailing whitespace/extra blank line flagged by gofumpt
inside the Check function (near the end of the function around the reported
spot); delete the stray blank line or trailing spaces so the function ends with
the closing brace immediately and run gofumpt/go fmt to verify formatting.
🧹 Nitpick comments (2)
pkg/development/development.go (1)
139-164: Consider caching entity-filtered nodes for better performance.The current implementation iterates
c.Registry.ReportAll()for each entity, resulting in O(entities × nodes) complexity. For large schemas, consider grouping nodes by entity prefix once before the loop.♻️ Optional optimization
+ // Group nodes by entity once + nodesByEntity := make(map[string][]*coverage.NodeInfo) + for _, node := range c.Registry.ReportAll() { + parts := strings.SplitN(node.Path, "#", 2) + if len(parts) > 0 { + nodesByEntity[parts[0]] = append(nodesByEntity[parts[0]], node) + } + } + // Update entity coverage info with logic nodes for i, entityInfo := range schemaCoverageInfo.EntityCoverageInfo { var entityUncovered []cov.LogicNodeCoverage var entityTotal int var entityUncoveredCount int - for _, node := range c.Registry.ReportAll() { - if strings.HasPrefix(node.Path, entityInfo.EntityName+"#") { + for _, node := range nodesByEntity[entityInfo.EntityName] { entityTotal++ if node.VisitCount == 0 { entityUncoveredCount++ entityUncovered = append(entityUncovered, cov.LogicNodeCoverage{ Path: node.Path, SourceInfo: node.SourceInfo, Type: node.Type, }) } - } }internal/coverage/discovery.go (1)
19-35: Consider defensive type assertion for PermissionStatement.The type assertion at line 21 assumes all elements in
PermissionStatementsare*ast.PermissionStatement. If the AST implementation guarantees this, it's fine; otherwise, a defensive check prevents potential panics.🛡️ Optional defensive assertion
func discoverEntity(es *ast.EntityStatement, r *Registry) { for _, ps := range es.PermissionStatements { - st := ps.(*ast.PermissionStatement) + st, ok := ps.(*ast.PermissionStatement) + if !ok { + continue + } path := fmt.Sprintf("%s#%s", es.Name.Literal, st.Name.Literal)
Updates the `compileIdentifier` function to include `PositionInfo` for `Identifier` nodes. This ensures consistent coverage reporting across all node types, as requested by the bot. This addresses the eighth comment from the coderabbitai bot review.
Adds a default case to the switch statement in `discoverExpression` to explicitly handle unexpected leaf expression types by setting the `nodeType` to "UNKNOWN". This prevents ambiguous coverage reports when non-`Identifier`/`Call` leaf expressions are encountered. This addresses the ninth comment from the coderabbitai bot review.
Removes the unused registry discovery logic from the `Run` function in `pkg/development/coverage/coverage.go`. The registry is now handled in `RunWithShape` and `RunCoverage`, making this redundant. This addresses the tenth comment from the coderabbitai bot review.
Unifies the context key patterns in `internal/coverage/registry.go` by replacing `contextKey{}` with `registryContextKey{}` and `pathKey{}` with `pathContextKey{}`. This improves consistency and adheres to best practices for context key declarations.
This addresses the eleventh comment from the coderabbitai bot review.
Implements sorting by path for the results of `ReportAll()` in `internal/coverage/registry.go`. This ensures a deterministic order for coverage reports, which is beneficial for testing and consistent output. This addresses the twelfth comment from the coderabbitai bot review.
Adds a debug log message to the `Visit()` function in `internal/coverage/registry.go` when an attempt is made to visit an unregistered path. This helps in identifying potential issues with registration/discovery mismatch during development. This addresses the thirteenth comment from the coderabbitai bot review.
Removes the `GetPath` helper function from `internal/coverage/registry.go` and replaces its usage with `AppendPath`. This consolidates the path helper functions and improves code clarity. This addresses the fourteenth comment from the coderabbitai bot review.
Removes the `GetPath` helper function from `internal/coverage/registry.go` and replaces its usage with `AppendPath`. This consolidates the path helper functions and improves code clarity. This addresses the fourteenth comment from the coderabbitai bot review.
Removes the `GetPath` helper function from `internal/coverage/registry.go` and replaces its usage with `AppendPath`. This consolidates the path helper functions and improves code clarity. This addresses the fourteenth comment from the coderabbitai bot review.
Replaces `request.GetMetadata().GetExclusionPath()` with `request.GetMetadata().GetCoveragePath()` in `checkRewrite` within `internal/engines/check.go`. This change reflects the renaming of `exclusion_path` to `coverage_path` in `proto/base/v1/service.proto` for improved clarity and consistency in coverage tracking. This addresses the impact of renaming `exclusion_path` to `coverage_path` in `proto/base/v1/service.proto`.
matiasmagni
added a commit
to matiasmagni/permify
that referenced
this pull request
Jan 31, 2026
…ify#837) - Add internal/coverage package (discovery, registry) for logic node tracking - Add PositionInfo to proto Child, coverage_path/exclusion_path to PermissionCheckRequestMetadata - Instrument CheckEngine with coverage tracking (trace, setChild, checkRewrite, checkLeaf) - Populate child.PositionInfo in compiler for identifiers and calls - Fix lookup test depth (200) for Google Docs Case 2 - Fix loader_test platform-independent assertion - Add coverage artifacts to .gitignore Co-authored-by: Cursor <cursoragent@cursor.com>
matiasmagni
added a commit
to matiasmagni/permify
that referenced
this pull request
Jan 31, 2026
…ify#837) - Add internal/coverage package (discovery, registry) for logic node tracking - Add PositionInfo to proto Child, coverage_path/exclusion_path to PermissionCheckRequestMetadata - Instrument CheckEngine with coverage tracking (trace, setChild, checkRewrite, checkLeaf) - Populate child.PositionInfo in compiler for identifiers and calls - Fix lookup test depth (200) for Google Docs Case 2 - Fix loader_test platform-independent assertion - Add coverage artifacts to .gitignore
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@internal/coverage/discovery.go`:
- Around line 19-33: In discoverEntity, registering the root "PERMISSION" node
for every PermissionStatement (in discoverEntity using r.Register with path
fmt.Sprintf("%s#%s")) overwrites/blocks leaf metadata when
st.ExpressionStatement is a leaf; change the logic in discoverEntity (and the
same pattern in the other block around lines 52-76) to detect leaf expressions
via st.ExpressionStatement / ast.ExpressionStatement and skip creating the
PERMISSION registration for that path so the subsequent call to
discoverExpression can let the leaf registration own the root path
(alternatively, if you prefer, register PERMISSION under a distinct subpath
instead of the root) — update the code paths around discoverEntity, the st
variable handling of st.ExpressionStatement, and calls to r.Register
accordingly.
In `@internal/engines/check.go`:
- Around line 175-199: The coverage path used in checkRewrite repeats the same
trackPath for the rewrite node, which doesn't create the operator segment
expected by discovery (it needs an "op" segment with child indices), so modify
checkRewrite to build an opPath (e.g., coverage.AppendPath(trackPath, "op")) and
pass that opPath into engine.trace/engine.setChild calls so children receive
paths like op.0, op.1; also apply the same opPath propagation in the similar
rewrite-handling code later (the other rewrite block around the setChild/trace
usage). Locate functions checkRewrite, engine.setChild, and engine.trace to
update the path construction and parameter passing accordingly.
- Around line 178-183: The code calls request.GetMetadata().GetCoveragePath
(which doesn't exist) and calls coverage.Track twice; fix by using the already
computed context path (path := coverage.PathFromContext(ctx)) when building
trackPath and remove the duplicate coverage.Track call so you only call
coverage.Track once with coverage.ContextWithPath(trackCtx, trackPath); adjust
references around trackCtx, trackPath, and the use of request.GetPermission() as
needed.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
proto/base/v1/service.proto (2)
2498-2513:⚠️ Potential issue | 🟡 MinorFix
tuple_filterIDs in the Data Delete cURL sample.Lines 2504 and 2509 use singular
idfields, butEntityFilter/SubjectFilterrequireidsarrays. The sample won’t validate against the API schema.Suggested fix
- " \"id\": \"1\"\n" + " \"ids\": [\"1\"]\n" ... - " \"id\": \"1\"\n" + " \"ids\": [\"1\"]\n"
2592-2601:⚠️ Potential issue | 🟡 MinorJS RunBundle sample uses incorrect client reference.
The example uses
client.bundle.runBundle, but the Permify Node.js SDK exposes this method on the Data service asclient.data.runBundle(). The code sample should be updated to use the correct client reference.Suggested fix
- "client.bundle.runBundle({\n" + "client.data.runBundle({\n"
🤖 Fix all issues with AI agents
In `@docs/api-reference/openapiv2/apidocs.swagger.json`:
- Around line 476-479: The Data Delete cURL sample uses singular "id" fields but
the TupleFilter schema requires arrays; update the sample's tuple_filter to use
entity.ids and subject.ids (e.g., "entity": {"type":"organization","ids":["1"]}
and "subject": {"type":"user","ids":["1"]}) and ensure relation remains "admin"
and attribute_filter stays as shown; reference the TupleFilter payload shape
used by the /v1/tenants/{tenant_id}/data/delete example so the request matches
the expected arrays for entity.ids and subject.ids.
- Around line 585-588: The JS example incorrectly calls client.bundle.runBundle;
update the sample to call the Data service runBundle method instead by replacing
client.bundle.runBundle with client.data.runBundle (matching the Go example's
client.Data.RunBundle and the RunBundle operation), so the snippet uses the Data
client for execution rather than the bundle-management client.
In `@internal/engines/check.go`:
- Around line 206-228: checkLeaf is tracing only the parent path, so coverage
never registers the leaf segment; update checkLeaf to append the leaf segment
(e.g., ".leaf" or use the same segment key used by discovery) to the path
returned by coverage.PathFromContext(ctx) before calling engine.trace so that
each returned CheckFunction is traced with the leaf path. Locate the switch in
checkLeaf (function checkLeaf) and modify the path variable or pass a derived
path (path.Child("leaf") or equivalent) into engine.trace for each returned call
(engine.checkTupleToUserSet, engine.checkComputedUserSet,
engine.checkComputedAttribute, engine.checkCall) so the discovered leaf coverage
entries are recorded.
In `@internal/engines/coverage_test.go`:
- Around line 74-90: The test currently ignores errors from tuple.Tuple(...) and
dw.Write(...), which can hide setup failures; update the setup to fail fast by
checking and handling errors returned by tuple.Tuple and dw.Write (and any other
calls used to prepare the test like tuple.E creation and invoker.Check
invocation), e.g., if err != nil then call t.Fatalf or equivalent; ensure you
reference and validate the results of tuple.Tuple, dw.Write, tuple.E and
invoker.Check so the test aborts immediately on setup or parsing failures.
- discovery.go: skip PERMISSION when expression is leaf-only to preserve leaf metadata; use path.leaf for operand leaves - check.go: fix checkRewrite to use opPath instead of GetCoveragePath; pass opPath to setChild; add path.leaf for operand leaves; add sequential execution when concurrency=1 for short-circuit coverage - coverage_test.go: fail fast on tuple/db errors; use CheckConcurrencyLimit(1) for short-circuit test - apidocs: fix tuple_filter ids (use arrays); fix RunBundle to use client.data
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/engines/check.go`:
- Around line 176-203: The coverage tracking in checkRewrite is using opPath
instead of the permission root path, so change the coverage.Track call to track
the original path (variable path) and let engine.trace continue to handle
opPath; specifically, in function checkRewrite replace the
coverage.Track(coverage.ContextWithPath(trackCtx, opPath)) usage with
coverage.Track(coverage.ContextWithPath(trackCtx, path)) so the PERMISSION node
is visited and operator visits aren’t double-counted while leaving the
trace(...) calls that pass opPath unchanged.
In `@internal/engines/coverage_test.go`:
- Around line 115-116: The failure message in the test uses the wrong expected
path string: update the t.Errorf message that currently mentions
"repository#edit.1 (admin)" to match the actual check/path
"repository#edit.op.1.leaf (admin)"; locate the assertion around the foundAdmin
boolean in coverage_test.go and change the error text to include the correct
identifier "repository#edit.op.1.leaf" so the message matches the checked path.
🧹 Nitpick comments (2)
docs/api-reference/openapiv2/apidocs.swagger.json (2)
2597-2601: Clarify thecoverage_pathfield description.The description "Path of the exclusion, used for coverage tracking" is potentially misleading. Based on the PR objectives, this field propagates coverage paths for all logic nodes (AND, OR, leaf), not just exclusions. Consider updating to something like:
"Path identifier used for coverage tracking during permission evaluation."
2790-2802: Consider adding a description to the PositionInfo definition.The
PositionInfotype is correctly defined with line and column properties, but lacks a description. For API documentation consistency, consider adding:"PositionInfo": { "type": "object", "description": "Source position information indicating line and column numbers in the schema definition.", "properties": { ... } }
…Permify#837) - Use parseAndCompileSchema in coverage Run() to fix unused linter - Apply gofumpt to coverage and test files - All tests and linter passing
matiasmagni
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/engines/check.go`:
- Around line 680-693: The sequential branch for limit == 1 forgot to handle
exclusions: update checkExclusion (and similar callers) so it does not call
checkRun(..., limit-1) when limit==1; either clamp the child limit to at least 1
(e.g., childLimit := max(1, limit-1)) before calling checkRun or add a dedicated
sequential exclusion path that runs functions one-by-one (like the
union/intersection sequential loop) to avoid passing 0 into checkRun and
blocking on the unbuffered semaphore; adjust the logic in checkExclusion, and
mirror the same fix at the other occurrence around lines ~746-759 to ensure no
callers pass 0 to checkRun.
🧹 Nitpick comments (1)
internal/coverage/registry.go (1)
110-123: Make Report() deterministic by sorting uncovered paths.
Report()iterates over a map, so the uncovered list order is non-deterministic. Sorting (as inReportAll) avoids flaky output/tests.♻️ Proposed tweak
func (r *Registry) Report() (uncovered []LogicNodeCoverage) { r.mu.RLock() defer r.mu.RUnlock() for _, node := range r.nodes { if node.VisitCount == 0 { uncovered = append(uncovered, LogicNodeCoverage{ Path: node.Path, SourceInfo: node.SourceInfo, Type: node.Type, }) } } + sort.Slice(uncovered, func(i, j int) bool { + return uncovered[i].Path < uncovered[j].Path + }) return uncovered }
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.
/claim #837
Enhance Coverage Command for Detailed Action/Permission Conditions (#837)
Description
This PR implements comprehensive coverage tracking for permission rules, specifically detecting when parts of conditions are skipped due to short-circuit evaluation (e.g., when
A OR BevaluatesAas true,Bis never executed).Problem
Previously, the coverage command would mark a permission as "covered" if any part of the condition was asserted, even if other parts were skipped due to short-circuit logic. This led to incomplete coverage assessments.
Example from issue:
Asserting only
system.viewwould mark the entire permission as covered, even though other components were never tested.Solution
Implemented a comprehensive coverage tracking system that:
Implementation Details
New Components
internal/coverage/registry.go: Thread-safe registry for tracking node visits with SourceInfo (Line:Column)internal/coverage/discovery.go: AST walker that discovers and registers all logic nodes during schema parsinginternal/engines/coverage_test.go: Comprehensive test for short-circuit detectionModified Components
internal/engines/check.go: Addedtrace()wrapper to instrument evaluation paths and track visitspkg/development/development.go: Integrated logic coverage into coverage command outputpkg/development/coverage/coverage.go: Extended coverage info with logic node trackingpkg/cmd/coverage.go: Updated to display logic coverage informationproto/base/v1/base.proto: AddedPositionInfomessage for source position trackingpkg/dsl/compiler/compiler.go: Enhanced to populate PositionInfo during compilationFeatures
✅ Source Position Tracking: Every node includes Line:Column information from source
✅ Unique Node IDs: Deterministic path-based IDs (e.g.,
repository#edit.0,repository#edit.1)✅ Short-Circuit Detection: Correctly identifies skipped nodes in:
✅ Thread-Safe: Registry uses
sync.RWMutexfor concurrent access✅ Comprehensive Reporting: Uncovered nodes reported with exact source positions
✅ Backward Compatible: No breaking changes to existing coverage functionality
Testing
TestCheckEngineCoverage: Verifies short-circuit detection for OR operationsTestRegistry: Validates registry functionality and thread safetyTestDiscover: Confirms AST discovery correctly registers all logic nodesTest Case:
All tests pass successfully.
Example Output
The coverage command now reports:
Example:
Technical Approach
PositionInfoin tokens to track source positionsVisitCount == 0to identify gapsPerformance Considerations
Breaking Changes
None. This is a backward-compatible enhancement.
Checklist
Related Issues
Closes #837
Additional Notes
See
COVERAGE_READINESS.mdfor detailed implementation assessment and verification. The implementation correctly detects when parts of permission rules are skipped due to short-circuit evaluation, providing detailed coverage information with exact source positions for uncovered nodes.Summary by CodeRabbit
New Features
Behavioral
Tests