Add support for violations (alerts) management tools - #153
Conversation
Sorry, something went wrong.
Summary by CodeRabbit
WalkthroughAdds a configurable violations toolset with a read-only list_violations MCP tool, including filtering, pagination, alert mapping, and cluster resolution. Cluster resolution is shared with existing vulnerability tools. ChangesViolations toolset and cluster resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPServer
participant listViolationsTool
participant cluster.ResolveClusterID
participant StackRoxAPI
MCPServer->>listViolationsTool: list_violations request
listViolationsTool->>cluster.ResolveClusterID: resolve cluster ID or name
cluster.ResolveClusterID->>StackRoxAPI: GetClusters by name
listViolationsTool->>StackRoxAPI: ListAlerts with filters and pagination
StackRoxAPI-->>listViolationsTool: alert results
listViolationsTool-->>MCPServer: ViolationResult page and nextCursor
Suggested reviewers: janisz 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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 `@internal/cluster/resolver.go`: - Around line 12-14: Update ResolveClusterID to wrap failures from the GetClusters API call with client.NewError before returning them, while preserving the existing not-found behavior and successful cluster ID resolution. Ensure every error propagated by this exported resolver follows the user-facing client error mapping.
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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 545aa2c1-daa6-4778-a962-6a584471c58e
📥 CommitsReviewing files that changed from the base of the PR and between 48edc39 and da3c0dc.
📒 Files selected for processing (10)- examples/config-read-only.yaml
- internal/app/app.go
- internal/cluster/resolver.go
- internal/cluster/resolver_test.go
- internal/config/config.go
- internal/toolsets/violations/tools.go
- internal/toolsets/violations/toolset.go
- internal/toolsets/vulnerability/clusters.go
- internal/toolsets/vulnerability/deployments.go
- internal/toolsets/vulnerability/nodes.go
Sorry, something went wrong.
| // ResolveClusterID resolves a cluster name to its ID. | ||
| // Returns error if cluster name is not found or if API call fails. | ||
| func resolveClusterID(ctx context.Context, conn *grpc.ClientConn, | ||
| func ResolveClusterID(ctx context.Context, conn *grpc.ClientConn, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Convert cluster API failures with client.NewError.
The exported resolver returns the raw GetClusters gRPC error through every consuming MCP handler, potentially exposing backend details and bypassing consistent error mapping.
Proposed fix import (
+ "github.com/stackrox/stackrox-mcp/internal/client"
)
// ...
if err != nil {
- return "", fmt.Errorf("failed to fetch clusters: %w", err)
+ return "", client.NewError(err, "GetClusters")
}As per path instructions, Go MCP server code requires “Proper error wrapping with client.NewError for user-facing errors.”
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cluster/resolver.go` around lines 12 - 14, Update ResolveClusterID to wrap failures from the GetClusters API call with client.NewError before returning them, while preserving the existing not-found behavior and successful cluster ID resolution. Ensure every error propagated by this exported resolver follows the user-facing client error mapping.
Source: Path instructions
Sorry, something went wrong.
janisz
left a comment
There was a problem hiding this comment.
I think we need E2E for this
Sorry, something went wrong.
| } | ||
|
|
||
| // IsReadOnly returns true as this tool only reads data. | ||
| func (t *listViolationsTool) IsReadOnly() bool { |
There was a problem hiding this comment.
Idea not relevant to this PR: Maybe we can create a struct ReadOnlyTool that would implement this function and all read only tools can inherit from it. This will make navigation more obvious and easy to list all read only tools.
@mtodor
Sorry, something went wrong.
| } | ||
|
|
||
| if alert.GetTime() != nil { | ||
| v.Time = alert.GetTime().AsTime().UTC().Format("2006-01-02T15:04:05Z") |
There was a problem hiding this comment.
why do we need this?
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Description
Adds support for listing violations data. Note that this change also exports ResolveClusterID from a shared location.
Validation
Tested with Claude Code (Opus 4.6) and RHACS 4.11: