feat(server): rfc 0027 green tools — query_logs, list_templates, template_drift - #414
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…late_drift The §3.2 tool set over the querier engine: query_logs (DSL parse + run_query, count + limited rows + pruning stats), list_templates (the new Querier::template_registry fold, rendered via format_template), template_drift (the window re-parsed through the same DSL grammar — one boundary rule). Per-call tenant binding re-resolves the request's bearer from the HTTP parts rmcp forwards into the tool context — sessions outlive requests, so the header is the authority. Results are the RFC 0016 JSON shapes re-encoded as MCP content; every tool description carries the treat-as-data warning. RFC0027.2/.3/.4/.5/.7 go green with cross-surface equivalence tests against the JSON API. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 56 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 (1)
📝 WalkthroughWalkthroughAdds a tenant-scoped template registry API, implements RFC 0027 MCP tools with tenant authorization, wires shared querier state into the MCP router, updates startup/shutdown signal handling, and expands integration tests plus a new ChangesMCP Query Surface
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant OuriosMcp
participant TokenStore
participant Querier
Client->>OuriosMcp: tools/call (tenant, tool args)
OuriosMcp->>TokenStore: check_tenant(bearer, tenant)
TokenStore-->>OuriosMcp: ok or invalid_request
OuriosMcp->>Querier: run_query / template_registry / run_drift
Querier-->>OuriosMcp: result or QueryError
OuriosMcp-->>Client: JSON content or tool error
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements RFC 0027 “green slice 2” by adding the §3.2 MCP tool surface to the querier (/mcp) and wiring it to existing RFC 0016/0010/0017 query and derivation logic, along with end-to-end integration tests that assert equivalence with the JSON API.
Changes:
- Add MCP tools:
query_logs,list_templates,template_drift, including per-call tenant binding and JSON-shape re-encoding as MCP text content. - Add RFC 0027 integration tests that drive the full MCP session handshake and assert equivalence with existing JSON API responses.
- Expose/reuse supporting querier types and add
Querier::template_registryas an engine-level API for the template registry derivation.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-server/tests/it/rfc0027_mcp.rs | Adds full MCP handshake helpers and implements RFC 0027 scenarios (.2, .3, .4, .5, .7) as integration tests. |
| crates/ourios-server/tests/it/rfc0016_query_endpoint.rs | Adds shared fixture/seam helpers to support RFC 0027 equivalence testing. |
| crates/ourios-server/src/querier.rs | Adjusts visibilities so MCP can reuse limit logic and response DTOs; wires /mcp router with shared engine. |
| crates/ourios-server/src/mcp.rs | Implements the MCP server handler and the three RFC 0027 tools over the querier engine. |
| crates/ourios-server/Cargo.toml | Enables rmcp macros and adds schemars for tool argument schemas. |
| crates/ourios-querier/src/lib.rs | Adds Querier::template_registry with spawn_blocking offload for registry derivation. |
| Cargo.lock | Locks new transitive deps (rmcp-macros, schemars). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/ourios-querier/src/lib.rs (1)
722-742: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a colocated unit test for the new public API.
This new
pub async fnis only exercised indirectly by anourios-serverintegration test (rfc0027_4_list_templates), not by a test in this crate. As per coding guidelines,**/crates/**/*.rs: "Unit tests must be next to the code and are mandatory for anything non-trivial."🤖 Prompt for 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. In `@crates/ourios-querier/src/lib.rs` around lines 722 - 742, The new public API `template_registry` in `ourios-querier` is only covered indirectly, so add a colocated unit test in this crate. Write a test near `template_registry` (or alongside `derive_template_registry`/`spawn_blocking_audit`) that exercises the successful path and, if practical, an error path by using the existing backend/store test helpers. Keep the test local to the crate so the new `pub async fn template_registry` is directly validated instead of relying on the `ourios-server` integration test.Source: Coding guidelines
🤖 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-querier/src/lib.rs`:
- Around line 722-742: The `collect_records` path is duplicating the same
`spawn_blocking_audit` closure used by `template_registry`, so reuse the
existing `template_registry` method instead of re-implementing
`derive_template_registry` there. Update `collect_records` to call
`template_registry` directly with the tenant, preserving the same async/blocking
behavior while removing the duplicated backend clone and closure logic.
In `@crates/ourios-server/src/mcp.rs`:
- Around line 117-234: The MCP tools in OuriosMcp currently call Querier
directly and never go through the existing QuerierMetrics instrumentation used
by /v1/query, so query duration and failures are not recorded. Plumb the shared
metrics/state into OuriosMcp (or add a wrapper around the #[tool_router]
methods) and ensure query_logs, list_templates, and template_drift each record
success and error outcomes through the same telemetry path as the HTTP query
flow. Use the OuriosMcp constructor and the
querier.run_query/run_drift/template_registry call sites as the integration
points.
---
Nitpick comments:
In `@crates/ourios-querier/src/lib.rs`:
- Around line 722-742: The new public API `template_registry` in
`ourios-querier` is only covered indirectly, so add a colocated unit test in
this crate. Write a test near `template_registry` (or alongside
`derive_template_registry`/`spawn_blocking_audit`) that exercises the successful
path and, if practical, an error path by using the existing backend/store test
helpers. Keep the test local to the crate so the new `pub async fn
template_registry` is directly validated instead of relying on the
`ourios-server` integration 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: ea150a4c-d0c7-4ef1-8691-b6b0d9aef586
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/ourios-querier/src/lib.rscrates/ourios-server/Cargo.tomlcrates/ourios-server/src/mcp.rscrates/ourios-server/src/querier.rscrates/ourios-server/tests/it/rfc0016_query_endpoint.rscrates/ourios-server/tests/it/rfc0027_mcp.rs
… robustness The query_logs limit argument is a hard cap (a DSL limit stage clamps to it); all three tools record on the shared ourios.query.duration histogram (query/drift as their true kinds, the registry fold under a new 'templates' registry member); collect_records reuses Querier::template_registry; the malformed-DSL test runs the full protocol dance and tool_error covers the isError result shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…list test Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Second CI occurrence of rfc0016_5 dying on unix_wait_status(15): the handler registered only when the select! first polled, so a SIGTERM between the readiness print and that poll killed the process by default disposition. Registration now precedes every role start (startup_guards), closing the race for supervisors and tests alike. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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/querier.rs`:
- Around line 186-200: The doc comment above the metrics helpers is attached to
the wrong method in querier.rs: the failed-query description currently sits on
record_duration instead of record_err. Move the stale “Record a failed query”
comment so it documents record_err, and keep the existing record_duration
comment with record_duration; use the record_duration and record_err symbols to
place the comments correctly.
🪄 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: 9e48f6af-1759-4c74-8957-b0a85ce24cd5
📒 Files selected for processing (6)
crates/ourios-querier/src/lib.rscrates/ourios-server/src/main.rscrates/ourios-server/src/mcp.rscrates/ourios-server/src/querier.rscrates/ourios-server/tests/it/rfc0027_mcp.rssemconv/registry/attributes.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/ourios-querier/src/lib.rs
- crates/ourios-server/src/mcp.rs
- crates/ourios-server/tests/it/rfc0027_mcp.rs
Comment on lines
3
to
5
| //! `.1` is green (the transport slice); the remaining stubs are | ||
| //! `#[ignore]`d so the default run stays green while the RFC works | ||
| //! through its slices, each naming the slice that discharges it. |
Comment on lines
185
to
+188
| /// Record a failed query: its duration, tagged with `error.type`. | ||
| fn record_err(&self, kind: &'static str, elapsed: Duration, error_type: &'static str) { | ||
| /// Record a stats-less operation's duration (the RFC 0027 registry | ||
| /// fold has no scan, so no row-group split to add). | ||
| pub(crate) fn record_duration(&self, kind: &'static str, elapsed: Duration) { |
This was referenced Jul 13, 2026
This was referenced Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
RFC 0027 green slice 2 (tools) — Scenarios RFC0027.2, .3, .4, .5, .7:
OuriosMcp(rmcp#[tool_router]/#[tool_handler]):query_logs(DSL parse →run_query; count + up-to-limitrendered rows + scanned/pruned stats; a drift statement is redirected totemplate_driftas a tool error),list_templates(the newQuerier::template_registry— the RFC 0017 fold exposed with the samespawn_blockingoffload as every derivation — rendered viaformat_template, sorted(id, version)), andtemplate_drift(the window re-parsed through the same DSL grammar as the JSON API's statement — one grammar, one RFC0010.2 boundary rule).http::request::Parts— the per-requestAuthorization— is the authority:check_tenantre-resolves the bearer against the store and rejects an out-of-set tenant as a tool error before any data is touched. The transport-layer 401 from slice 1 is unchanged.QueryError's H6-scrubbedDisplay— no DataFusion/SQL on either boundary.Scenario mapping
tests/it/rfc0027_mcp.rsdrives the real router through the full MCP session dance (initialize → session id → initialized → tools/call, SSE parsed):.2: 401 before dispatch (no/unknown bearer), out-of-set tenant as the denial tool error (no token value on the surface), open mode serves..3/.5: equivalence — the MCP payload is asserted==the JSON API's response for the identical statement over the same seeded store (fixture seams shared from the RFC 0016 suite); malformed DSL is a tool error, transport stays 200..4: row-for-row againstQuerier::template_registry, rendering matched toformat_template..7:tools/listis exactly the §3.2 three (no tenant enumeration), every description carries the warning, nothing SQL-shaped advertised..6(the grammar resource) remains the last stub — the resource slice, then the green flip.Invariants / hazards
.7pins it.Checks run locally
cargo fmt --all --check,cargo clippy -p ourios-server --all-targets --all-features -- -D warnings,cargo test -p ourios-server --all-features— green (6/7 RFC 0027 scenarios passing,.6ignored).🤖 Generated with Claude Code
Summary by CodeRabbit
query_logs,list_templates, andtemplate_drift.ourios.query.kind = templates.