feat: connect rbac validator in validator ochestrator#20
Conversation
8dcc865 to
0368043
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR integrates RBAC (Role-Based Access Control) validation into the validator orchestrator and refactors LLM validation to be executed directly within the unified validator instead of as a separate component. This consolidation enables consistent validation flow for all rule types including RBAC permission checks.
Key changes:
- Unified validator now handles RBAC, LLM, and other engine-based validations in a single orchestrator
- RBAC permission checks integrated at the beginning of validation flow for both file and diff-based validation
- LLM validator execution moved from standalone component to orchestrated execution within the main validator
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/validator/validator.go | Added RBAC permission checks, LLM client support, and new ValidateChanges method for unified diff-based validation |
| internal/validator/llm_validator.go | Changed checkRule from private to exported CheckRule method for reuse in validator orchestrator |
| internal/mcp/server.go | Updated to use unified validator instead of standalone LLM validator, added RBAC info display in conventions query |
| internal/git/repo.go | Added GetCurrentUser function to retrieve git user name for RBAC checks |
| internal/cmd/validate.go | Updated CLI command to use unified validator with LLM client instead of standalone LLM validator |
Comments suppressed due to low confidence (1)
internal/mcp/server.go:1
- Line 965 unconditionally writes 'Allowed paths: All files' even when
len(roleConfig.AllowWrite) > 0is true (lines 966-968). The if-else structure is missing, causing both messages to appear. The code should use an if-else block to write only one of these messages.
package mcp
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(addedLines) == 0 && strings.TrimSpace(change.Diff) != "" { | ||
| addedLines = strings.Split(change.Diff, "\n") | ||
| } |
There was a problem hiding this comment.
The fallback logic on lines 489-491 appears to treat the entire diff as added lines when ExtractAddedLines returns empty but diff is non-empty. This could incorrectly include removed lines (prefixed with '-') as added content. Consider removing this fallback or adding a comment explaining when this scenario occurs.
| if len(addedLines) == 0 && strings.TrimSpace(change.Diff) != "" { | |
| addedLines = strings.Split(change.Diff, "\n") | |
| } |
Uh oh!
There was an error while loading. Please reload this page.