fix(alerts): explain raw-SQL sample counts and warn when they count rows

[Makisuo]

Stack 1/5. Customer feedback from an agent building alert rules: a raw-SQL rule's minimum_sample_count counts returned rows (one per bucket), not events.

The engine already reads an optional samples column (each row counts as 1 without it), but only one MCP parameter description mentioned it.

• The alert SQL editor and the MCP create_alert_rule / update_alert_rule parameters document value, group, samples and what a query returning no rows means.
• rawAlertSampleCountWarning (domain raw-sql) flags a raw-SQL rule with a minimum above 1 and no samples column. It shows in the editor and as a notice on create/update/get_alert_rule output (warnings on the create/update outputs).
• The warehouse catalog notes that service_overview_spans has no SpanKind (PR 5 replaces this).

No behavior change to evaluation.

Test: raw-sql.test.ts, alert-tools.test.ts.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

Summary by CodeRabbit

• New Features
  • Raw-query alerts now warn when a minimum sample threshold is set but the query does not select a samples value. In that case, each returned row counts as one sample; selecting a samples value lets the query provide the sample count.
  • Alert guidance explains how sample thresholds work, including that queries returning no rows are treated as no-data checks.
  • Configuration warnings appear in alert creation, update, and detail results when applicable.
• Documentation
  • Alert settings now describe defaults of two consecutive breaches, two consecutive healthy evaluations, and a 30-minute re-notification interval.

[maple-review-bot]

Maple review

🟢 Confidence 5/5 · safe to merge
Docs-and-warning change with no engine path touched; the only defect is a case-sensitivity mismatch between the new check and the engine's column contract.
_{quality 98/100 · 1 note · tests covered · risk low}

Adds documentation and an advisory warning for raw-SQL alert rules whose minimum sample count counts returned rows. Evaluation is untouched and the wiring type-checks; safe to merge.

• rawAlertSampleCountWarning flags raw-SQL rules whose min sample count counts rows
• create_alert_rule / update_alert_rule return warnings and notice them; get_alert_rule notices too
• Alert SQL editor and MCP parameter text document value/group/samples and no-row behavior
• service_overview_spans catalog note records the missing SpanKind column

Findings

🔵 Note · F1 · rawAlertSqlSelectsSamples accepts Samples, which the engine never reads

correctness · packages/domain/src/raw-sql.ts:291-292

The check matches case-insensitively, but the engine decodes the column key as exactly samples (RawSqlAlertRowSchema, packages/query-engine/src/runtime/query-engine.ts:2323, read at :2559) and the warehouse returns column names as the author wrote them. A query aliasing count() AS Samples therefore suppresses the warning while the min sample count still counts one per row — the case the warning exists for.

Drop the `i` flag so the check matches the engine's contract: ```` \/\\bsamples\\b\/.test(maskLiteralsAndComments(sql)) ````.

🤖 Prompt to fix this finding with an AI agent

Findings from an automated review of commit 1bc30145766825af92130a61488bc70d3bc7c619. Verify each one against the current code before changing anything, fix only those that still apply, and keep each fix to the lines it names.

---

F1 · Note · correctness · packages/domain/src/raw-sql.ts:291-292
`rawAlertSqlSelectsSamples` accepts `Samples`, which the engine never reads
The check matches case-insensitively, but the engine decodes the column key as exactly `samples` (`RawSqlAlertRowSchema`, `packages/query-engine/src/runtime/query-engine.ts:2323`, read at `:2559`) and the warehouse returns column names as the author wrote them. A query aliasing `count() AS Samples` therefore suppresses the warning while the min sample count still counts one per row — the case the warning exists for.
Suggested fix: Drop the `i` flag so the check matches the engine's contract: ```` \/\\bsamples\\b\/.test(maskLiteralsAndComments(sql)) ````.

What was checked

• row.samples == null ? 1 : ... (query-engine.ts:2559) confirms the warning's one-row-per-sample claim
• maskLiteralsAndComments keeps samples inside strings/comments from matching (raw-sql.test.ts)
• serviceOverviewSpans in datasources.ts has no SpanKind, so the new catalog note is accurate

_{1bc3014 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}

[coderabbitai]

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 962bc78f-08da-4593-8e21-340f744c11a2

📥 Commits

Reviewing files that changed from the base of the PR and between 1bc3014 and ff972f2.

📒 Files selected for processing (3)

• apps/ai/src/mcp/tools/create-alert-rule.ts
• packages/domain/src/raw-sql.test.ts
• packages/domain/src/raw-sql.ts

 _____________________________________________
< HD, 4K, 8K...I see bugs in all resolutions. >
 ---------------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: c8ade7a1-3376-41e3-801a-db334a3de741

📥 Commits

Reviewing files that changed from the base of the PR and between c7ee332 and 1bc3014.

📒 Files selected for processing (10)

• apps/ai/src/mcp/lib/alert-rules.ts
• apps/ai/src/mcp/tools/__tests__/alert-tools.test.ts
• apps/ai/src/mcp/tools/create-alert-rule.ts
• apps/ai/src/mcp/tools/get-alert-rule.ts
• apps/ai/src/mcp/tools/update-alert-rule.ts
• apps/web/src/components/alerts/signal-and-threshold-section.tsx
• packages/backend/src/services/warehouse/warehouse-catalog.ts
• packages/domain/src/mcp-outputs/alerts.ts
• packages/domain/src/raw-sql.test.ts
• packages/domain/src/raw-sql.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Raw-query alert rules now produce configuration warnings when their minimum sample count exceeds one and their SQL does not select a samples column. The warnings appear in AI alert-tool outputs and the web alert interface. Warehouse catalog notes also clarify span-kind information.

Changes

Raw alert sample-count warnings

Layer / File(s)	Summary
Detect missing sample counts packages/domain/src/raw-sql.ts, packages/domain/src/raw-sql.test.ts	SQL helpers detect a samples identifier outside comments and literals. They return a warning when the minimum sample count exceeds one and no samples identifier is selected. Tests cover warning conditions and cases that suppress warnings.
Surface warnings in alert tools packages/domain/src/mcp-outputs/alerts.ts, apps/ai/src/mcp/lib/alert-rules.ts, apps/ai/src/mcp/tools/create-alert-rule.ts, apps/ai/src/mcp/tools/get-alert-rule.ts, apps/ai/src/mcp/tools/update-alert-rule.ts, apps/ai/src/mcp/tools/__tests__/alert-tools.test.ts	Create and update output schemas allow optional warnings. The create, get, and update tools compute configuration warnings and pass them to their output or renderer. Parameter descriptions explain raw-query sample counting and no-row checks. Tests check warning output for raw-query rules.
Show raw-query guidance in the web interface apps/web/src/components/alerts/signal-and-threshold-section.tsx	The alert form explains optional group and samples columns, raw-query sample counting, and no-row checks. It displays a warning when the SQL and minimum sample count meet the warning conditions.

Warehouse span catalog note

Layer / File(s)	Summary
Clarify span-kind information packages/backend/src/services/warehouse/warehouse-catalog.ts	The service_overview_spans note states that the table has no SpanKind column and directs queries that need span-kind separation to traces.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant create_alert_rule
  participant ruleConfigWarnings
  participant rawAlertSampleCountWarning
  create_alert_rule->>ruleConfigWarnings: Check rule configuration
  ruleConfigWarnings->>rawAlertSampleCountWarning: Check raw query SQL and minimum sample count
  rawAlertSampleCountWarning-->>ruleConfigWarnings: Warning or null
  ruleConfigWarnings-->>create_alert_rule: Configuration warnings
  create_alert_rule-->>create_alert_rule: Include warnings in output and rendered notices

Loading

Suggested reviewers: jeremyfunk

Merge Risk: 🔵 Low · up to 1bc30

The advisory warning can be missed for nested queries or CTEs. Alert evaluation remains unchanged, so this is mergeable with awareness of the warning's limitation.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 1bc30

The changes are advisory: they do not alter alert evaluation, grant additional permissions, or change how rules are saved. The public output change is additive. Warning detection is heuristic, however, and should not be treated as confirmation that a query returns event counts.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The changed behavior is scoped to advisory information for an existing rule or editor draft. No additional cross-tenant selection, warehouse execution, or privileged action is introduced by the inspected warning paths.

Trust Boundaries and Controls

• observed — Create and update continue passing the current tenant's organization, user, and roles to their existing services. Those services retain admin checks. Update still selects the rule from the tenant-scoped list before writing; warning generation does not replace these controls.

Resilience and Maintainability Implications

• inferred — The added warning path introduces no new write, reservation, cleanup, or recovery step. Create remains non-idempotent, and update completes its existing stale-incident reconciliation before warnings are computed. Existing interruption, repetition, concurrency, and partial-write recovery semantics are not changed by this advisory response addition.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 10 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main changes: documenting raw-SQL sample counts and warning when returned rows count as samples.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[maple-review-bot]

Maple review

🟢 Confidence 5/5 · safe to merge
rawAlertSqlSelectsSamples now matches the engine's exact samples key, and the two new test files exercise both warning states.
_{quality 100/100 · no findings · tests covered · risk low}

Documents what minimum_sample_count counts for raw-SQL rules and adds rawAlertSampleCountWarning, surfaced in the alert editor and as warnings/notices on create, update and get. No evaluation behavior changes; safe to merge.

• rawAlertSampleCountWarning flags a raw-SQL rule whose minimum exceeds 1 with no samples column
• ruleConfigWarnings feeds warnings on create/update outputs and notices in renderRuleWrite
• Editor shows the warning and a raw-SQL samples/no-rows hint
• create_alert_rule parameter descriptions now state the real defaults (2/2/30)

Fixed since the last review

• ✅ F1 · rawAlertSqlSelectsSamples accepts Samples, which the engine never reads

What was checked

• F1: alias match is case-sensitive and rejects Samples (raw-sql.ts:296), matching RawSqlAlertRowSchema's exact samples key in query-engine.ts
• Masked text is used only to locate AS, the alias is read from the original at the same offset (raw-sql.ts:294), so comments and string literals are ignored
• New parameter/schema defaults (2, 2, 30) match packages/db/src/schema/alerts.ts

_{ff972f2 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}