feat(mcp): preview_alert_rule dry-runs a rule before it is saved - #1207
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (20)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Maple review🔴 Confidence 2/5 · risky as written Warning This review ended early; what follows is what it established. Adds a read-only
Findings🟠 Warning · F1 · "Every window had no data" never fires for a grouped rulecorrectness · The guard 🟠 Warning · F2 · Unbounded
|
| Change | Kind | Observable | Evidence |
|---|---|---|---|
| preview_alert_rule MCP tool | inbound entrypoint | yes | Runs under the executor span that annotates maple.mcp.tool.arguments (apps/ai/src/mcp/tools/registry.ts); warehouse read is spanned by the query engine |
Files not reviewed (1)
The review ended before it read these diffs, so nothing above vouches for them.
apps/ios/Packages/MapleAPI/Sources/MapleAPI/openapi.json
24e1cbd · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.
![maple-review-bot[bot]](https://avatars.githubusercontent.com/in/4108169?s=60&v=4){kind=small}
| thresholdUpper: preview.thresholdUpper, | ||
| minimumSampleCount: ruleRequest.minimumSampleCount ?? 0, | ||
| groups, | ||
| points: preview.series.flatMap((series) => |
There was a problem hiding this comment.
Warning
Unbounded points array in the structured output
F2 · Warning · performance
Every window of every group is copied into structuredContent with no cap: the service bounds windows at MAX_PREVIEW_BUCKETS = 1500 (AlertsService.ts:294) but nothing bounds groups, and a spec rule grouping on a high-cardinality attribute can return thousands. A 24h preview of a 5-minute-window rule grouped by attr.http.url is groups × 288 point objects on the wire, to the MCP client and to the chat UI's __maple_ui payload, while the text the model reads is capped at SHOWN_GROUPS/SHOWN_POINTS.
Cap the rows the output carries — the first `SHOWN_GROUPS` groups' points, say — and report the cut in `output` (`truncation`/`omitted`) the way the text already does, or take a `limit` parameter like the other list tools.
🤖 Prompt to fix with an AI agent
In `apps/ai/src/mcp/tools/preview-alert-rule.ts:207-217`: Unbounded `points` array in the structured output.
Every window of every group is copied into `structuredContent` with no cap: the service bounds windows at `MAX_PREVIEW_BUCKETS = 1500` (`AlertsService.ts:294`) but nothing bounds groups, and a spec rule grouping on a high-cardinality attribute can return thousands. A 24h preview of a 5-minute-window rule grouped by `attr.http.url` is groups × 288 point objects on the wire, to the MCP client and to the chat UI's `__maple_ui` payload, while the text the model reads is capped at `SHOWN_GROUPS`/`SHOWN_POINTS`.
Suggested fix: Cap the rows the output carries — the first `SHOWN_GROUPS` groups' points, say — and report the cut in `output` (`truncation`/`omitted`) the way the text already does, or take a `limit` parameter like the other list tools.
Verify the problem exists at that location before changing it, and keep the fix to those lines.
24e1cbd to
4259130
Compare
|
Note A newer push replaced |
4259130 to
3308f27
Compare
Maple review🟡 Confidence 3/5 · needs attention Warning This review ended early; what follows is what it established. Adds a read-only
Still open from earlier reviews
Fixed since the last review
What was checked
Observability coverage: 1 of 1 changes observable
Files not reviewed (1)The review ended before it read these diffs, so nothing above vouches for them.
|
An agent writing a raw-SQL rule had no way to see what it would do short of creating it and reading checks later, which is how a rule that matches nothing ships looking quiet. preview_alert_rule replays a saved rule (rule_id, optionally with overrides) or a create_alert_rule definition over past data through AlertsService.previewRule: per window the value, sample count and verdict, skips with their reason, and when it would have fired. It warns when every window had no data or fell below the minimum sample count. Preview points now carry skipReason (skip_reason on the v2 API). create/update/get_alert_rule and list_alert_checks point at it.
…hing The scheduler turns an empty result into one no-data observation per tick (per service in multi-service mode), but the preview only charted that for ungrouped plans, so a raw-SQL rule matching nothing previewed as no series and preview_alert_rule could never warn about it. preview_alert_rule also keeps the latest 200 windows per group in its structured output, labels the window column by its start, and tailors the advice once the range was clamped.
3308f27 to
3bb3e2b
Compare
Maple review🟡 Confidence 3/5 · needs attention Adds the read-only
Still open from earlier reviews
What was checked
Observability coverage: 2 of 2 changes observable
|
1 participant
Stack 3/5. An agent writing a raw-SQL rule could only learn what it does by saving it and reading checks later, which is how a rule that matches nothing ships looking quiet.
New read-only MCP tool
preview_alert_rule:rule_id, optionally with overrides) or acreate_alert_ruledefinition, plus a window.AlertsService.previewRule. Per window: value, sample count, verdict, and for skips the reason. It also reports when the rule would have fired.sampleswarning.Preview points carry
skipReason(skip_reasonon v2). create/update/get_alert_rule and list_alert_checks link to the tool. Registered in the output catalog, web tool metadata, MCP instructions and docs.Test: alert-tools (draft and saved-rule cases), registry contract, previewRule suite.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit