fix(querier): let the MCP query_logs tool run count aggregations - #607
Conversation
The MCP query_logs handler unconditionally injected its row-cap limit stage, but `count [by …]` and `limit` are mutually exclusive (compile::validate) — so every aggregation through the MCP surface was rejected with "does not support limit". The JSON /v1/query path already guards this behind an is_aggregation check (handle_query); mirror it in the MCP handler so an agent can aggregate its own telemetry (e.g. `count by attr.model`), not just page raw rows. Adds rfc0027_3_query_logs_count_aggregation: runs a count-by through the full MCP protocol with a limit argument set (proving it is ignored for aggregations) and asserts byte-equivalence with the JSON API. Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe MCP ChangesCount aggregation limit handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a parity bug in the Ourios querier’s MCP query_logs tool: MCP previously always injected a row-cap limit, which made count [by …] aggregations fail validation because count and limit are mutually exclusive. The change mirrors the JSON API behavior by skipping apply_limit when the parsed statement includes a Stage::Count, and adds an integration regression test that exercises the full MCP protocol and asserts MCP/JSON output equivalence.
Changes:
- Update the MCP
query_logshandler to detectStage::Countand avoid injecting the row-caplimitfor aggregations. - Add an integration test that runs
count bythrough MCP with a non-nulllimitargument and asserts the response matches the JSON API byte-for-byte.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/ourios-server/src/mcp.rs | Skip MCP-injected limit when the query includes Stage::Count, matching the JSON API guard. |
| crates/ourios-server/tests/it/rfc0027_mcp.rs | Add regression test proving query_logs count aggregations work over MCP and match JSON API output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify 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 `@crates/ourios-server/src/mcp.rs`:
- Around line 303-310: Add colocated unit tests in mcp.rs for the query-building
logic around is_aggregation and apply_limit, covering a normal row query, count,
and count by. Assert that limits are applied to row queries but omitted for both
aggregation forms, while preserving the existing integration regression test.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9154a956-c27e-43a2-8231-7b8ea14a40bf
📒 Files selected for processing (2)
crates/ourios-server/src/mcp.rscrates/ourios-server/tests/it/rfc0027_mcp.rs
Extract the count-vs-limit guard into cap_rows_unless_aggregation and unit-test it next to the code (per §6.2): a row query gets the cap injected as a limit stage; both `count` and `count by` are left uncapped. Complements the RFC 0027 integration regression. Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
Both query_logs equivalence tests now share the identical scenario heading, so the scenario→test grep stays unambiguous; the regression-specific wording moves below the heading. Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
Let the MCP
query_logstool runcountaggregationsThe MCP
query_logshandler unconditionally injected its row-caplimitstage (mcp.rs), butcount [by …]andlimitare mutually exclusive (compile::validate, RFC 0002 amendment 2026-07-15). So every aggregation submitted through the MCP surface was rejected with"a query with a count stage does not support limit"— an agent could page raw rows but couldn't askcount by attr.modelabout its own data.The JSON
/v1/querypath already handles this correctly:handle_querycomputesis_aggregationand skipsapply_limitforcountqueries (querier.rs:481). This mirrors that guard into the MCP handler, so the two surfaces behave identically.Change
mcp.rs: skip the injectedlimitcap when the statement carries aStage::Count— same guard as the JSON API, referenced in the comment.Test (RFC 0027 §5, regression)
rfc0027_3_query_logs_count_aggregation: runstemplate_id == 1 | count by template_idthrough the full MCP protocol with a non-nulllimitargument set — proving the cap is ignored for aggregations rather than injected — and asserts the tool's answer is byte-identical to the JSON API's for the same statement.Invariants
No hazard/pillar surface touched — this is a query-surface parity fix (hazard #6: the DSL layer must behave the same across surfaces). No schema, WAL, or miner change.
Verification
cargo clippy -p ourios-server --all-targets --all-features -- -D warnings→ cleancargo test -p ourios-server --test it rfc0027→ 8 passed, 0 failed (incl. the new regression)cargo fmt --all --check→ clean🤖 Generated with Claude Code
Summary by CodeRabbit
count byaggregations with a specified limit.