fix: improve coverage for permission condition branches#2955
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTracks permission assertion components (leaves), computes per-scenario uncovered components and per-key coverage percentages, aggregates component-level percentages into TotalAssertionsCoverage, updates CLI to display uncovered components and percentages, and adds tests for branched, cross-entity, and nested-permission cases. ChangesAssertion Component Coverage Tracking
Sequence Diagram(s)sequenceDiagram
participant Client
participant CoverageCLI
participant CoverageEngine
participant ChecksRunner
participant Aggregator
Client->>CoverageCLI: request coverage report
CoverageCLI->>CoverageEngine: build schema refs & extract assertion components
CoverageEngine->>ChecksRunner: evaluate scenarios (relationships/attributes)
ChecksRunner-->>CoverageEngine: per-scenario coverage contexts
CoverageEngine->>Aggregator: compute uncovered components & component percentages
Aggregator-->>CoverageCLI: aggregated coverage info
CoverageCLI-->>Client: render report (uncovered components, percentages)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Iineman2
changed the title
|
Demo video for the coverage change: https://github.com/Iineman2/permify/releases/download/permify-837-demo/permify-837-demo.mp4 It shows the partial-coverage scenario from the regression test: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/development/coverage/coverage_test.go (1)
250-292: ⚡ Quick winAdd one cross-entity false-positive regression case for components.
This new case is solid, but it won’t catch the scenario where a branch is marked covered from tuples on a different entity instance (same entity type).
A small follow-up case (e.g., check ondocument:1, supporting tuple only ondocument:2) would lock down the branch-coverage guarantee.🤖 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 250 - 292, Add a follow-up test that prevents cross-entity-instance false positives by asserting that coverage recorded for a check on one entity instance is not satisfied by tuples on a different instance of the same entity type: update the test file around the existing Case 2.1 (the Run(file.Shape{...}) block) or add a new It() case that uses a Check with Entity "document:1" while providing the supporting relationship tuple only on "document:2" in the Relationships slice, then assert that the relevant branch/component for "document:1" remains uncovered (use the same assertions you have for EntityCoverageInfo[*].UncoveredAssertionComponents and CoverageAssertionComponentsPercent to verify the branch is not marked covered and TotalAssertionsCoverage reflects the uncovered branch).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/development/coverage/coverage.go`:
- Around line 365-377: The current logic in findUncoveredAssertionComponents
uses extractCoveredAssertions and assertionComponentHasSupportingData to mark
components covered even when supporting relation/attribute data belong to
unrelated entity instances or when tuple-to-userset components only verify the
first hop; fix this by scoping supporting-data checks to the specific
scenario-relevant entity instances derived from the given checks/filters (use
the same subject tuples or filtered entity IDs when calling
assertionComponentHasSupportingData) and update the tuple-to-userset validation
code (the tuple-to-userset branch inside assertionComponentHasSupportingData or
its helper) to resolve and verify the second hop (the computed hop like 'b' in
'a.b') for each candidate subject instance rather than only checking the first
hop; ensure you iterate per-entity-instance, confirm the computed-hop tuple
existence for that instance, and only then mark the assertion component as
covered.
---
Nitpick comments:
In `@pkg/development/coverage/coverage_test.go`:
- Around line 250-292: Add a follow-up test that prevents cross-entity-instance
false positives by asserting that coverage recorded for a check on one entity
instance is not satisfied by tuples on a different instance of the same entity
type: update the test file around the existing Case 2.1 (the
Run(file.Shape{...}) block) or add a new It() case that uses a Check with Entity
"document:1" while providing the supporting relationship tuple only on
"document:2" in the Relationships slice, then assert that the relevant
branch/component for "document:1" remains uncovered (use the same assertions you
have for EntityCoverageInfo[*].UncoveredAssertionComponents and
CoverageAssertionComponentsPercent to verify the branch is not marked covered
and TotalAssertionsCoverage reflects the uncovered branch).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c945891d-9fcb-4e8e-bb04-92204286ca00
📒 Files selected for processing (3)
pkg/cmd/coverage.gopkg/development/coverage/coverage.gopkg/development/coverage/coverage_test.go
|
Addressed CodeRabbit's entity-instance coverage concern in commit 67a9baa. Component support is now scoped to the checked/filtered entity IDs, tuple-to-userset components verify the computed hop against the scenario subject, and Case 2.2 covers the cross-instance false-positive regression. Verification: go test ./pkg/cmd ./pkg/development/coverage. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/development/coverage/coverage_test.go (1)
250-292: ⚡ Quick winConsider adding a comment to explain the TotalAssertionsCoverage calculation.
The test correctly verifies the new component-level coverage behavior. However, the expected
TotalAssertionsCoveragevalue of 66% may not be immediately obvious to future maintainers. Consider adding a brief comment explaining how this value is derived (e.g., averaging component coverage across entities).📝 Suggested comment addition
})).Should(Equal(true)) Expect(sci.EntityCoverageInfo[1].CoverageAssertionComponentsPercent["scenario 1"]).Should(Equal(33)) + // TotalAssertionsCoverage averages component coverage across entities: (user:100% + document:33%) / 2 = 66% Expect(sci.TotalAssertionsCoverage).Should(Equal(66)) })🤖 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 250 - 292, Add a brief clarifying comment above the Expect(sci.TotalAssertionsCoverage).Should(Equal(66)) assertion explaining how 66 is calculated (e.g., derive from component-level coverage: document has two uncovered components out of three across the scenario => scenario component coverage 33% and overall assertion coverage averages to 66%). Locate the check in the test case "Case 2.1: Tracks partially covered permission conditions" where the test inspects sci (the Run result) and the EntityCoverageInfo and TotalAssertionsCoverage fields and insert the one-line comment to document the calculation.
🤖 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 250-292: Add a brief clarifying comment above the
Expect(sci.TotalAssertionsCoverage).Should(Equal(66)) assertion explaining how
66 is calculated (e.g., derive from component-level coverage: document has two
uncovered components out of three across the scenario => scenario component
coverage 33% and overall assertion coverage averages to 66%). Locate the check
in the test case "Case 2.1: Tracks partially covered permission conditions"
where the test inspects sci (the Run result) and the EntityCoverageInfo and
TotalAssertionsCoverage fields and insert the one-line comment to document the
calculation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d06ab12-1728-440e-8c49-8db39d33b92b
📒 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
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
Added one more focused coverage improvement in commit 9b3572d: nested same-entity permissions now expand into their leaf components for the outer permission as well, with Case 2.3 covering |
1 participant
/claim #837
Summary
Fixes #837.
This updates the coverage analyzer so a permission/action assertion can report branch-level condition coverage instead of treating one satisfied branch as complete coverage for the whole permission.
Changes:
system or (viewer or owner)where only thesystembranch is exercised.Verification
Summary by CodeRabbit
New Features
Tests