🐛 parse conditions correctly from nested rules #1001
Conversation
WalkthroughgetConditions in the rule parser now returns an additional provider-specific conditions map and the parser accumulates/merges that map across "and"/"or"/default branches. The file-walking helper skips symlinked directories when enumerating non-directory entries. Changes
Sequence Diagram(s)sequenceDiagram
participant RP as RuleParser
participant GC as getConditions
participant PC as ProviderConditionsMap
RP->>GC: call getConditions(conditionsInterface)
GC-->>RP: returns (conditionsList, providerClients, providerConditions, err)
alt err != nil
RP->>RP: handle error (propagate providerConditions where applicable)
else
RP->>PC: merge providerConditions into top-level provider map
RP->>RP: continue condition composition ("and"/"or"/default)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (2)
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: 0
🧹 Nitpick comments (1)
parser/rule_parser.go (1)
362-362: Consider restoring the more descriptive error message.The error message was simplified to just "condition key must be a string", losing context about the ruleID and file path that would aid debugging.
If reverting is desired, the previous error likely included more context:
-return nil, nil, nil, fmt.Errorf("condition key must be a string") +return nil, nil, nil, fmt.Errorf("condition key must be a string, ruleID: %s, file: %s", ruleID, filepath)Note: The ruleID and filepath variables may need to be passed to
getConditionsif they're not currently in scope.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
parser/rule_parser.go(16 hunks)provider/lib.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
parser/rule_parser.go (2)
provider/provider.go (2)
ConditionsByCap(472-475)InternalProviderClient(450-453)engine/conditions.go (1)
ConditionEntry(43-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (macos-latest, mac)
🔇 Additional comments (4)
parser/rule_parser.go (3)
91-217: LGTM! Function signatures and accumulation logic are correct.The
LoadRulesandLoadRulefunctions have been properly updated to handle the newproviderConditionsreturn value. The accumulation logic at lines 143-148 and 172-177 correctly merges conditions from nested rule sets.
372-395: LGTM! Condition accumulation in or/and branches is correct.The
provConditionsmap is properly accumulated from nestedgetConditionscalls in both the "or" (lines 390-395) and "and" (lines 428-433) branches. The pattern ensures that provider conditions from nested rules are correctly propagated to the top level.Also applies to: 408-433
627-808: LGTM! getConditions implementation is correct and complete.The signature change has been properly implemented:
- All error paths return
nilfor the newproviderConditionsmap- Recursive calls to
getConditions(lines 687, 717) correctly handle the new return value- Provider conditions are accumulated from nested "and" (lines 706-711) and "or" (lines 736-741) branches
- The final return statement (line 807) includes all three return values
The implementation correctly propagates provider conditions through nested rule structures.
provider/lib.go (1)
301-306: I'm unable to clone the repository to access the codebase directly. However, based on the review comment provided, I can identify that the original concerns about symlink handling are reasonable and require manual verification.Let me rewrite the review comment with the verification status:
Clarify intended behavior for symlinked directories in the symlink check (lines 301-306).
The code now skips symlinks that resolve to directories, preventing them from being counted as files. Before approving this change, the following points need clarification:
- Intentional behavior change? Is skipping symlinked directories a deliberate design decision and documented in the PR objectives?
- Broken symlink handling: When
os.Stat()fails (e.g., broken symlink), the code continues and appends the symlink as a file. Confirm whether this is the intended behavior or if broken symlinks should also be skipped.Verification needed:
- Check test coverage for symlink handling in
*_test.gofiles- Review PR description or related comments for intent and impact analysis
- Ensure the change does not break existing functionality that may depend on symlinked directories being enumerable
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
pranavgaikwad
force-pushed
the
fix/parseRulesCorrectly
branch
from
1be31ba to
d84e92c
Compare
1 participant
Summary by CodeRabbit
Refactoring
Bug Fixes
Chores
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.