feat: add permission condition coverage details#2954
Open
hieu-lee wants to merge 6 commits into
Open
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-permission, per-scenario condition-component coverage: new types and storage, threads compiled schema through coverage computation, extracts and evaluates leaf condition components for coverage, displays sorted/color-coded results in the CLI, and adds tests validating component coverage. ChangesPermission Condition Coverage Feature
Sequence Diagram(s)sequenceDiagram
participant CLI as coverage CLI
participant Runner as calculateEntityCoverage
participant Compiler as schema compiler
participant Extractor as condition extractor
participant Indexer as scenario indexer
participant Evaluator as coverage evaluator
CLI->>Runner: request coverage display
Runner->>Compiler: use compiled entity definitions
Runner->>Extractor: extract leaf condition components per permission
Extractor->>Indexer: build scenario-local relationship/attribute index
Indexer->>Evaluator: evaluate each component against asserted targets
Evaluator->>Runner: return ConditionCoverageInfo
Runner->>CLI: render sorted/color-coded results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
pkg/development/coverage/coverage.go (1)
789-805: 💤 Low valueConsider documenting the always-covered/always-uncovered branches.
componentCallalways returns covered andcomponentPermissionalways returns uncovered. The former is a reasonable simplification (rule logic is opaque to coverage), and the latter is a fallback for recursive same-entity permission references (caught byvisitedPermissionsupstream). A short comment here would help future readers understand the design intent and avoid mistaking these for bugs.📝 Proposed comment
func (data componentCoverageData) isComponentCovered(entityName string, component ConditionComponent, targets []assertionTarget) bool { switch component.Type { case componentRelation: return data.hasRelationship(entityName, component.Name, targets) case componentTupleToUserset: tupleSetRelation, _, _ := strings.Cut(component.Name, ".") return data.hasRelationship(entityName, tupleSetRelation, targets) case componentAttribute: return data.hasAttribute(entityName, component.Name, targets) case componentCall: + // Rule bodies are opaque to coverage; the call itself is considered covered. + // Any attribute arguments are tracked separately as componentAttribute leaves. return true case componentPermission: + // Fallback for recursive same-entity permission references that cannot be + // expanded further; conservatively reported as uncovered. return false default: return false } }🤖 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/development/coverage/coverage.go` around lines 789 - 805, Add concise comments inside the isComponentCovered function explaining why componentCall always returns true (rule logic is opaque so we treat calls as covered) and why componentPermission always returns false (this is a deliberate fallback for recursive/same-entity permission references handled by visitedPermissions upstream). Locate the isComponentCovered function and add short inline comments next to the case labels for componentCall and componentPermission mentioning these design intentions and referencing visitedPermissions as the upstream guard so future readers do not treat these branches as bugs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/development/coverage/coverage.go`:
- Around line 789-805: Add concise comments inside the isComponentCovered
function explaining why componentCall always returns true (rule logic is opaque
so we treat calls as covered) and why componentPermission always returns false
(this is a deliberate fallback for recursive/same-entity permission references
handled by visitedPermissions upstream). Locate the isComponentCovered function
and add short inline comments next to the case labels for componentCall and
componentPermission mentioning these design intentions and referencing
visitedPermissions as the upstream guard so future readers do not treat these
branches as bugs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f0aa0b4-3e72-4221-9119-47007cfe691b
📒 Files selected for processing (3)
pkg/cmd/coverage.gopkg/development/coverage/coverage.gopkg/development/coverage/coverage_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 594a9038-845c-48d9-ad8d-599dcb54e0aa
📒 Files selected for processing (4)
pkg/cmd/coverage.gopkg/cmd/flags/coverage.gopkg/development/coverage/coverage.gopkg/development/coverage/coverage_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/development/coverage/coverage_test.go
4 tasks
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/development/coverage/coverage_test.go (1)
678-685: ⚡ Quick winFail fast when entity coverage is missing.
Returning an empty
EntityCoverageInfodefers the failure and makes diagnostics less clear. In tests, it’s better to fail at lookup time with a precise message.Proposed refactor
-func findEntityCoverage(sci SchemaCoverageInfo, entityName string) EntityCoverageInfo { +func findEntityCoverage(sci SchemaCoverageInfo, entityName string) (EntityCoverageInfo, bool) { for _, entityCoverage := range sci.EntityCoverageInfo { if entityCoverage.EntityName == entityName { - return entityCoverage + return entityCoverage, true } } - return EntityCoverageInfo{} + return EntityCoverageInfo{}, false }And at call sites:
documentCoverage, ok := findEntityCoverage(sci, "document") Expect(ok).Should(BeTrue(), "expected coverage info for entity 'document'")🤖 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/development/coverage/coverage_test.go` around lines 678 - 685, Change findEntityCoverage to fail fast by returning an explicit presence flag instead of silently returning an empty EntityCoverageInfo: update the signature of findEntityCoverage(sci SchemaCoverageInfo, entityName string) to return (EntityCoverageInfo, bool), iterate as before and return (entityCoverage, true) when found and (EntityCoverageInfo{}, false) when not; then update all call sites to check the bool (e.g., documentCoverage, ok := findEntityCoverage(...); Expect(ok).Should(BeTrue(), "expected coverage info for entity 'document'")) so tests fail immediately with a clear assertion when coverage is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/development/coverage/coverage_test.go`:
- Around line 678-685: Change findEntityCoverage to fail fast by returning an
explicit presence flag instead of silently returning an empty
EntityCoverageInfo: update the signature of findEntityCoverage(sci
SchemaCoverageInfo, entityName string) to return (EntityCoverageInfo, bool),
iterate as before and return (entityCoverage, true) when found and
(EntityCoverageInfo{}, false) when not; then update all call sites to check the
bool (e.g., documentCoverage, ok := findEntityCoverage(...);
Expect(ok).Should(BeTrue(), "expected coverage info for entity 'document'")) so
tests fail immediately with a clear assertion when coverage is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d120abdd-a15f-4b67-b4f2-98b8262cb067
📒 Files selected for processing (2)
pkg/development/coverage/coverage.gopkg/development/coverage/coverage_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/development/coverage/coverage.go
Author
|
I have read the CLA Document and I hereby sign the CLA |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/claim #837
Fixes #837.
Adds scenario-level permission condition coverage to the existing
coveragecommand. A scenario no longer gets only permission-name coverage for a complex expression such as:The coverage report now walks the compiled permission tree and reports relation, attribute, tuple-to-userset, rule-call, and nested same-entity permission components that do not have matching test data for the asserted scenario.
Changes
base.Childpermission trees.EntityCoverageInfo.company.maintainis not covered bydocument#companyalone.--coverage-conditionsto fail coverage runs below a minimum permission-condition coverage threshold.system.viewbranch case from the bounty issue.Demo
Short demo video: https://github.com/hieu-lee/permify/blob/permify-837-demo/demos/permify-837/condition-coverage-demo.mp4
The demo shape and transcript are also on that branch:
Test Plan
go test ./pkg/development/coverage ./pkg/cmdgo test ./pkg/development/... ./pkg/cmdgo run ./cmd/permify coverage /Users/hieu.leduc/Desktop/codex-money-maker/artifacts/permify-837/condition-coverage-demo.yamlgo run ./cmd/permify coverage --coverage-conditions 100 /Users/hieu.leduc/Desktop/codex-money-maker/artifacts/permify-837/condition-coverage-demo.yamlexits withpermission condition coverage < 100%.I also started
go test ./...; it hit unrelated integration-test failures because this local environment does not provide apermifyhost, then stayed idle in later environment-dependent packages, so I stopped that run after the relevant package tests had passed.Summary by CodeRabbit
New Features
Tests