From 57c41368698aa76798de8a2f394e26257faa6f5b Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 4 Sep 2026 14:59:45 +0200 Subject: [PATCH 1/4] DEV-1810: OpenSpec change (plan stage) --- .../.openspec.yaml | 2 + .../design.md | 28 ++++ .../proposal.md | 29 ++++ .../specs/mcp/response-row-cap/spec.md | 145 ++++++++++++++++++ .../tasks.md | 25 +++ 5 files changed, 229 insertions(+) create mode 100644 openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/.openspec.yaml create mode 100644 openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/design.md create mode 100644 openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/proposal.md create mode 100644 openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/specs/mcp/response-row-cap/spec.md create mode 100644 openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/tasks.md diff --git a/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/.openspec.yaml b/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/.openspec.yaml new file mode 100644 index 00000000..1d9aeef9 --- /dev/null +++ b/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-04 diff --git a/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/design.md b/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/design.md new file mode 100644 index 00000000..a3915079 --- /dev/null +++ b/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/design.md @@ -0,0 +1,28 @@ +# Design + +## Context + +See proposal.md — Why. Relevant current state: `_format_output` (slayer/mcp/server.py) dispatches markdown/json/csv rendering, and warnings already render in all three formats (`_format_warnings`, `_csv_warning_comments`, json `{"data","warnings"}` shape). The `query` tool takes a run-by-name shortcut for a bare stored-query-model name with no overrides; passing `limit` disables that shortcut (pre-existing behavior). `SlayerQuery.limit` is emitted as the outermost SQL LIMIT. + +## Goals / Non-Goals + +- Goal: cap lives entirely in the MCP layer; the engine, REST API, Flight, and PG facade are untouched. +- Non-goals: no config/env knob for the default cap; no exact-total row counting; no ceiling on explicit limits. + +## Decisions + +1. **`limit` is the knob; no new tool argument** (over a separate `max_rows` arg). One knob matches Storyline's agent-facing contract and avoids two interacting parameters on an already 16-parameter tool. Explicit `limit` is fully trusted — the maintainer explicitly rejected a hard ceiling (Storyline's 10,000): a caller overriding the default knows what they're doing. +2. **Push-down + universal slice** (over post-hoc slice alone). When no explicit limit, the structured path sets the query limit to cap+1 (21) so the database never ships the full runaway result; the post-execution slice is the universal guarantee for paths push-down can't reach (run-by-name, DAG output, explain plans). Consequence: notices say "more rows exist" uniformly; exact totals are unknowable in the pushed-down case. +3. **Notice rides the warnings union** (over dedicated per-format footers). A `ResponseTruncationWarning` (kind `"truncated"`, fields `returned_rows`, `hint`) joins `AnySlayerWarning` in slayer/core/warnings.py; the MCP layer appends it before formatting. Zero new rendering branches; the three formats stay in sync by construction. The engine never emits this kind — it is additive schema only from the REST API's perspective. +4. **Cap decision uses the caller's arguments, not the executed result.** `len(result.data) > limit` must never be the trigger — with an explicit limit the MCP layer does no slicing at all (even for explain plans whose row count is unrelated to the SQL limit). Only the no-limit paths slice, at 20. +5. **query_nested: root stage only.** The root (last) entry of `queries` controls the cap; other stages' limits are irrelevant. Push-down copies the root dict rather than mutating the caller's input. + +## Risks / Trade-offs + +- [Pushed-down `LIMIT 21` visible in show_sql/dry_run output] → honest: it is the SQL that runs; docstrings note the default cap. +- [Run-by-name notice says "pass a higher 'limit'", but passing `limit` switches to structured execution with different variable precedence] → pre-existing sharp edge, kept out of the one-line notice deliberately; uniform hint text wins. +- [Truncating an explain plan (no-limit case) can mangle plan readability] → accepted for uniformity; a giant EXPLAIN ANALYZE floods context the same way rows do, and `limit` lifts the cap. + +## Migration Plan + +Additive behavior change in one release; no storage or schema migration. Rollback = revert. diff --git a/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/proposal.md b/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/proposal.md new file mode 100644 index 00000000..92e88ea3 --- /dev/null +++ b/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/proposal.md @@ -0,0 +1,29 @@ +# Proposal: MCP query response row cap with truncation notice + +## Why + +The MCP `query` and `query_nested` tools have no response-side row cap: a query without a `limit` returns the full result set, which can flood the calling agent's context window. Storyline's MCP query tool already caps responses; SLayer should behave the same way. + +## What Changes + +- The MCP `query` tool caps returned rows at 20 when the caller passes no `limit`. An explicit `limit` is trusted verbatim — no ceiling, no truncation. +- `query_nested` applies the same rule keyed on the ROOT stage's `limit` (last entry of `queries`). +- When no explicit limit exists, the structured path pushes `LIMIT cap+1` (21) into the generated query so truncation is detectable without fetching the full result; a universal post-execution slice guards paths push-down cannot reach (run-by-name stored queries, DAG stages, explain plans). +- A truncated response carries a `ResponseTruncationWarning` (new kind `"truncated"` in the `AnySlayerWarning` union) stating the returned row count and how to get more rows; it renders through the existing warnings machinery in all three output formats (markdown `Warnings:` block, csv leading `#` comment, json `{"data", "warnings"}`). + +## Capabilities + +### New Capabilities + +- `mcp/response-row-cap`: response-side row capping and truncation notices for the MCP query tools. + +### Modified Capabilities + +(none) + +## Impact + +- `slayer/core/warnings.py` — new `ResponseTruncationWarning` + union member (additive; the engine never emits it, REST unaffected). +- `slayer/mcp/server.py` — default-cap constant, cap/push-down and slice+notice helpers, wiring in `query` and `query_nested`, docstring updates. +- `docs/reference/mcp.md`, `.claude/skills/slayer-query.md` — document the cap, the notice, the root-stage rule, and the json shape change on truncation. +- REST API, Flight, PG facade, stored-query semantics: unchanged. diff --git a/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/specs/mcp/response-row-cap/spec.md b/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/specs/mcp/response-row-cap/spec.md new file mode 100644 index 00000000..55de1ebd --- /dev/null +++ b/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/specs/mcp/response-row-cap/spec.md @@ -0,0 +1,145 @@ +# mcp/response-row-cap + +## Purpose + +Protects the calling agent's context window: MCP query responses are capped at a small default row count unless the caller sets an explicit limit, and a truncated response says so and tells the caller how to get more rows. + +## ADDED Requirements + +### Requirement: Default row cap on MCP query responses + +When the MCP `query` tool is called without a `limit`, the response SHALL contain at most 20 data rows. When the underlying result has more rows than the cap, the response SHALL be truncated to exactly 20 rows and carry a truncation notice. + +#### Scenario: Uncapped query over a large result + +- WHEN `query` runs without `limit` against a model whose result has more than 20 rows +- THEN the response contains exactly 20 data rows and a truncation notice + +#### Scenario: Result exactly at the cap + +- WHEN `query` runs without `limit` and the result has exactly 20 rows +- THEN all 20 rows are returned and no truncation notice appears + +#### Scenario: Result one past the cap + +- WHEN `query` runs without `limit` and the result has exactly 21 rows +- THEN the response contains exactly 20 rows and a truncation notice + +### Requirement: Explicit limit is trusted verbatim + +When the caller passes an explicit `limit`, the MCP layer SHALL return the rows as executed, with no response-side truncation and no truncation notice — regardless of how many rows come back, including `explain` plan rows. + +#### Scenario: Explicit limit honored + +- WHEN `query` runs with `limit=25` against a result with 30 available rows +- THEN 25 rows are returned and no truncation notice appears + +#### Scenario: Explicit small limit + +- WHEN `query` runs with `limit=5` +- THEN 5 rows are returned and no truncation notice appears + +#### Scenario: Rows exceeding an explicit limit are not sliced by the MCP layer + +- WHEN the engine returns more rows than an explicit `limit` (e.g. a mocked execution) +- THEN the MCP layer neither slices the rows nor adds a truncation notice + +#### Scenario: Explain with explicit limit untouched + +- WHEN `query` runs with `explain=True` and an explicit `limit`, and the plan has more rows than the limit +- THEN all plan rows are returned and no truncation notice appears + +### Requirement: Cap push-down into the generated query + +When no explicit `limit` is given on the structured query path, the generated SQL SHALL carry `LIMIT 21` (cap + 1) so truncation is detectable without fetching the full result. Run-by-name execution of a stored query SHALL leave the stored query's SQL untouched. + +#### Scenario: Pushed-down limit visible in SQL + +- WHEN `query` runs without `limit` on the structured path with `show_sql=True` or `dry_run=True` +- THEN the generated SQL contains `LIMIT 21`, not `LIMIT 20` + +#### Scenario: Stored query SQL untouched + +- WHEN a stored query runs by bare name (run-by-name path) without `limit` +- THEN the SQL executed is the stored query's own, with no injected LIMIT + +### Requirement: Run-by-name responses are capped response-side + +A stored query executed by bare name without a `limit` SHALL have its response sliced to 20 rows with a truncation notice when it returns more, and returned whole with no notice when it returns 20 or fewer. + +#### Scenario: Stored query above the cap + +- WHEN a run-by-name stored query returns more than 20 rows +- THEN the response contains exactly 20 rows and a truncation notice + +#### Scenario: Stored query at the cap + +- WHEN a run-by-name stored query returns exactly 20 rows +- THEN all 20 rows are returned and no truncation notice appears + +### Requirement: query_nested capped by the root stage's limit only + +The `query_nested` tool SHALL apply the same rule keyed on the ROOT stage (last entry of `queries`): an explicit root `limit` is trusted verbatim; without one, the final response is capped at 20 with a truncation notice whose hint points at the root query's `limit`. Non-root stages' limits SHALL NOT affect the cap. The tool SHALL NOT mutate the caller's `queries` dicts. + +#### Scenario: Root without limit is capped + +- WHEN `query_nested` runs with a root stage that has no `limit` and the final result has more than 20 rows +- THEN the response contains exactly 20 rows and a truncation notice telling the caller to set a higher `limit` on the root query + +#### Scenario: Non-root limit does not lift the cap + +- WHEN a non-root stage has an explicit `limit` but the root stage has none +- THEN the default cap of 20 still applies to the final response + +#### Scenario: Root limit trusted + +- WHEN the root stage has an explicit `limit` +- THEN no response-side truncation occurs and no notice appears + +#### Scenario: Caller dicts unchanged + +- WHEN `query_nested` pushes the cap into the root stage +- THEN the caller's submitted `queries` dicts are structurally unchanged afterwards (no `limit` key added) + +### Requirement: Truncation notice content and rendering + +The truncation notice SHALL state the returned row count, say that more rows exist, and tell the caller how to get more rows. It SHALL appear in every output format through the warnings channel: the markdown `Warnings:` block, a leading `#` comment line in csv, and a warning entry with kind `"truncated"` in the json `{"data", "warnings"}` payload. It SHALL coexist with other warnings, appended last. + +#### Scenario: Markdown notice + +- WHEN a truncated result is formatted as markdown +- THEN the output ends with a `Warnings:` block containing "showing first 20 rows — more rows exist" and a hint to pass a higher `limit` + +#### Scenario: CSV notice + +- WHEN a truncated result is formatted as csv +- THEN a leading `#` comment line carries the notice and the data rows below keep a uniform column count + +#### Scenario: JSON notice + +- WHEN a truncated result is formatted as json +- THEN the payload has the `{"data", "warnings"}` shape and `warnings` contains an entry with `kind == "truncated"` and the returned row count + +#### Scenario: Coexists with other warnings + +- WHEN a truncated result already carries an engine warning +- THEN both warnings render in every format and the truncation notice comes last + +#### Scenario: Warning round-trips through the union + +- WHEN a response carrying the truncation warning is serialized and re-validated +- THEN the warning deserializes back to the truncation kind with its fields intact + +### Requirement: Explain plan rows capped without a limit + +When `query` runs with `explain=True` and no `limit`, the returned plan rows SHALL be subject to the same 20-row cap and notice. `dry_run` output (SQL only) SHALL never carry a truncation notice. + +#### Scenario: Large explain plan capped + +- WHEN `explain=True` without `limit` yields a plan of more than 20 rows +- THEN 20 plan rows are returned with a truncation notice + +#### Scenario: Dry run unaffected + +- WHEN `dry_run=True` +- THEN the response contains only SQL and never a truncation notice diff --git a/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/tasks.md b/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/tasks.md new file mode 100644 index 00000000..2b38380e --- /dev/null +++ b/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/tasks.md @@ -0,0 +1,25 @@ +# Tasks + +## 1. Failing test suite (spec-tests stage) + +- [ ] 1.1 Write tests for the default cap on `query` (no limit → 20 rows + notice; exactly 20 → no notice; 21 → 20 + notice) and verify they fail against current code +- [ ] 1.2 Write tests that an explicit `limit` is trusted verbatim: limit=5 and limit=25/30 cases; mocked engine returning more rows than the limit → no slice, no notice; `explain=True` with explicit limit → plan untouched +- [ ] 1.3 Write push-down tests: no-limit structured path emits `LIMIT 21` (show_sql and dry_run); run-by-name leaves stored SQL untouched +- [ ] 1.4 Write run-by-name capping tests: stored query >20 rows → 20 + notice; exactly 20 → no notice +- [ ] 1.5 Write `query_nested` tests: root without limit → 20 + notice with root-query hint; non-root limit ignored; root limit trusted; caller `queries` dicts not mutated +- [ ] 1.6 Write notice-rendering tests: markdown `Warnings:` block, csv leading `#` line with uniform column count, json `{"data","warnings"}` with kind `"truncated"`; coexistence with an existing warning (truncation last); union round-trip of `ResponseTruncationWarning` +- [ ] 1.7 Write explain/dry_run tests: explain without limit and >20 plan rows → capped + notice; dry_run never carries a notice + +## 2. Implementation (spec-implement stage) + +- [ ] 2.1 Add `ResponseTruncationWarning` (kind `"truncated"`, `returned_rows`, `hint`) to slayer/core/warnings.py and the `AnySlayerWarning` union; verify union round-trip test passes +- [ ] 2.2 Add default-cap constant and cap helpers to slayer/mcp/server.py (compute cap/pushed-down limit from caller args; slice + append warning); verify helper-level tests pass +- [ ] 2.3 Wire the cap into `query`: push-down on the structured no-limit path, response-side slice for run-by-name and explain; verify tasks 1.1–1.4 and 1.7 tests pass +- [ ] 2.4 Wire the cap into `query_nested` keyed on the root stage, copying the root dict; verify 1.5 tests pass +- [ ] 2.5 Update `limit` docstrings on both tools (concise, one line each); verify rendered tool schema mentions the default cap +- [ ] 2.6 Run the full non-integration suite (`poetry run pytest -m "not integration"`) and ruff; fix all failures + +## 3. Documentation + +- [ ] 3.1 Update docs/reference/mcp.md: default cap, notice, root-stage rule for query_nested, json shape change on truncation; verify by proofread +- [ ] 3.2 Update .claude/skills/slayer-query.md with a one-line mention of the cap; verify by proofread From 21fa3172487d9e151d710996804b7c08bf2ef9d0 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 4 Sep 2026 15:22:49 +0200 Subject: [PATCH 2/4] =?UTF-8?q?DEV-1810:=20MCP=20query=20row=20cap=20?= =?UTF-8?q?=E2=80=94=20default=2020=20rows,=20LIMIT=20push-down,=20truncat?= =?UTF-8?q?ion=20notice?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .claude/skills/slayer-query.md | 2 + docs/reference/mcp.md | 8 +- .../tasks.md | 30 +- slayer/core/warnings.py | 20 +- slayer/mcp/server.py | 42 +- tests/test_mcp_row_cap.py | 415 ++++++++++++++++++ 6 files changed, 494 insertions(+), 23 deletions(-) create mode 100644 tests/test_mcp_row_cap.py diff --git a/.claude/skills/slayer-query.md b/.claude/skills/slayer-query.md index 6d4abf34..e7d1cf29 100644 --- a/.claude/skills/slayer-query.md +++ b/.claude/skills/slayer-query.md @@ -20,6 +20,8 @@ A `SlayerQuery` is a JSON/dict object. The same shape works across the REST API, } ``` +Over MCP, a query without `limit` returns at most 20 rows plus a truncation notice — set an explicit `limit` to get more (`query_nested`: the root stage's `limit`). + `order[].column` uses the short alias (`count`, `revenue_sum`) to order by a measure declared in the same query; undeclared order targets use formula (colon) syntax — see below. **Ordering by something you don't project.** `order` may name an undeclared column/aggregate/expression ("top-N by X, show only Y, Z"). Computed hidden, sorted on, and stripped from the result: an **aggregate** (`amount:sum`, `customers.revenue:sum`), an inline **transform** (`rank(amount:sum)`, `change(...)`, `cumsum`/`lag`/`lead`/`ntile`), an inline **composite** (`revenue:sum / cnt:sum`, `abs(amount:sum)`), and a **windowed** aggregate (`amount:sum(window='90d')`, alone or inside a composite). A **raw row column** sorts directly in a raw-rows query (`distinct_dimension_values: false`); in a grouped/dedup query there is no single value per group, so it sorts **per group** by the extreme the direction puts first — `asc` by each group's `min`, `desc` by each group's `max`. Write `{"column": "created_at:max", "direction": "asc"}` explicitly for the other one. A **joined** row column (`customers.regions.name`), and a derived column whose `sql` reaches through a join, behave the same way — the join is pulled in for the sort, and in a grouped query the wrap is computed per host row-group rather than globally. NULLs sort **last** in both directions on every database (SQL Server excepted: its native ordering is used, because the portable emulation makes the statement fail there). An order target SLayer cannot resolve is an error, never a silently unsorted result. Order expressions must use formula syntax for their operands, not the `name`s of measures declared in the same query: `{"column": "revenue:sum / cnt:sum"}` works, `{"column": "rev / cnt"}` is rejected. diff --git a/docs/reference/mcp.md b/docs/reference/mcp.md index 617fa449..218d8e30 100644 --- a/docs/reference/mcp.md +++ b/docs/reference/mcp.md @@ -91,8 +91,8 @@ claude mcp list | Tool | Description | |------|-------------| -| `query` | Execute a semantic query. See [Queries](../concepts/queries.md) for format. | -| `query_nested` | Execute a multi-stage DAG of named sub-queries that can reference one another via `source_model` or `joins.target_model`. Companion to `query`; the engine auto-sorts the list (Kahn's algorithm), so order doesn't matter. Params: `queries: List[Dict[str, Any]]`, plus `variables` / `show_sql` / `dry_run` / `explain` / `format` mirroring `query`. See [Multistage Queries](../examples/06_multistage_queries/multistage_queries.md). | +| `query` | Execute a semantic query. See [Queries](../concepts/queries.md) for format. Without an explicit `limit` the response is capped at 20 rows (the generated SQL carries `LIMIT 21` so truncation is detectable) and a truncation notice is appended via the warnings channel. | +| `query_nested` | Execute a multi-stage DAG of named sub-queries that can reference one another via `source_model` or `joins.target_model`. Companion to `query`; the engine auto-sorts the list (Kahn's algorithm), so order doesn't matter. Params: `queries: List[Dict[str, Any]]`, plus `variables` / `show_sql` / `dry_run` / `explain` / `format` mirroring `query`. The 20-row cap keys on the ROOT (last) stage's `limit` only — non-root limits neither lift nor lower it. See [Multistage Queries](../examples/06_multistage_queries/multistage_queries.md). | **`query` parameters:** @@ -104,7 +104,7 @@ claude mcp list | `filters` | list[str] | Filter formula strings, e.g. `["status = 'active'", "amount > 100"]`. Supports operators (`=`, `<>`, `>`, `>=`, `<`, `<=`, `IN`, `IS NULL`, `IS NOT NULL`, `LIKE`, `NOT LIKE`), boolean logic (`AND`, `OR`, `NOT`), and inline transform expressions (`"change(revenue) > 0"`). Filters on measures are automatically routed to HAVING. | | `time_dimensions` | list[dict] | Time grouping. Each entry supports an optional `label` for display. | | `order` | list[dict] | Sorting, e.g. `[{"column": "count", "direction": "desc"}]` | -| `limit` | int | Max rows | +| `limit` | int | Max rows, trusted verbatim; without it the response is capped at 20 rows with a truncation notice | | `offset` | int | Skip rows | | `whole_periods_only` | bool | Snap date filters to time bucket boundaries, exclude the current incomplete time bucket | | `distinct_dimension_values` | bool | Default `true` — auto-dedup dim-only queries (`GROUP BY `). Set `false` to emit raw rows (no top-level `GROUP BY`); rejects any measure reference in `measures` / `filters` / `order`. | @@ -112,7 +112,7 @@ claude mcp list | `show_sql` | bool | Include the generated SQL in the response for debugging | | `dry_run` | bool | Generate and return the SQL without executing it | | `explain` | bool | Run EXPLAIN ANALYZE and return the query plan | -| `format` | string | Output format: `"markdown"` (default, compact), `"json"` (structured), or `"csv"` (most compact). Case-insensitive | +| `format` | string | Output format: `"markdown"` (default, compact), `"json"` (structured), or `"csv"` (most compact). Case-insensitive. Warnings (including the truncation notice) render as a trailing `Warnings:` block in markdown, leading `#` comment lines in csv, and turn the json payload into `{"data", "warnings"}` instead of a bare array | ### Memories + semantic search diff --git a/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/tasks.md b/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/tasks.md index 2b38380e..df5d1a8f 100644 --- a/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/tasks.md +++ b/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/tasks.md @@ -2,24 +2,24 @@ ## 1. Failing test suite (spec-tests stage) -- [ ] 1.1 Write tests for the default cap on `query` (no limit → 20 rows + notice; exactly 20 → no notice; 21 → 20 + notice) and verify they fail against current code -- [ ] 1.2 Write tests that an explicit `limit` is trusted verbatim: limit=5 and limit=25/30 cases; mocked engine returning more rows than the limit → no slice, no notice; `explain=True` with explicit limit → plan untouched -- [ ] 1.3 Write push-down tests: no-limit structured path emits `LIMIT 21` (show_sql and dry_run); run-by-name leaves stored SQL untouched -- [ ] 1.4 Write run-by-name capping tests: stored query >20 rows → 20 + notice; exactly 20 → no notice -- [ ] 1.5 Write `query_nested` tests: root without limit → 20 + notice with root-query hint; non-root limit ignored; root limit trusted; caller `queries` dicts not mutated -- [ ] 1.6 Write notice-rendering tests: markdown `Warnings:` block, csv leading `#` line with uniform column count, json `{"data","warnings"}` with kind `"truncated"`; coexistence with an existing warning (truncation last); union round-trip of `ResponseTruncationWarning` -- [ ] 1.7 Write explain/dry_run tests: explain without limit and >20 plan rows → capped + notice; dry_run never carries a notice +- [x] 1.1 Write tests for the default cap on `query` (no limit → 20 rows + notice; exactly 20 → no notice; 21 → 20 + notice) and verify they fail against current code +- [x] 1.2 Write tests that an explicit `limit` is trusted verbatim: limit=5 and limit=25/30 cases; mocked engine returning more rows than the limit → no slice, no notice; `explain=True` with explicit limit → plan untouched +- [x] 1.3 Write push-down tests: no-limit structured path emits `LIMIT 21` (show_sql and dry_run); run-by-name leaves stored SQL untouched +- [x] 1.4 Write run-by-name capping tests: stored query >20 rows → 20 + notice; exactly 20 → no notice +- [x] 1.5 Write `query_nested` tests: root without limit → 20 + notice with root-query hint; non-root limit ignored; root limit trusted; caller `queries` dicts not mutated +- [x] 1.6 Write notice-rendering tests: markdown `Warnings:` block, csv leading `#` line with uniform column count, json `{"data","warnings"}` with kind `"truncated"`; coexistence with an existing warning (truncation last); union round-trip of `ResponseTruncationWarning` +- [x] 1.7 Write explain/dry_run tests: explain without limit and >20 plan rows → capped + notice; dry_run never carries a notice ## 2. Implementation (spec-implement stage) -- [ ] 2.1 Add `ResponseTruncationWarning` (kind `"truncated"`, `returned_rows`, `hint`) to slayer/core/warnings.py and the `AnySlayerWarning` union; verify union round-trip test passes -- [ ] 2.2 Add default-cap constant and cap helpers to slayer/mcp/server.py (compute cap/pushed-down limit from caller args; slice + append warning); verify helper-level tests pass -- [ ] 2.3 Wire the cap into `query`: push-down on the structured no-limit path, response-side slice for run-by-name and explain; verify tasks 1.1–1.4 and 1.7 tests pass -- [ ] 2.4 Wire the cap into `query_nested` keyed on the root stage, copying the root dict; verify 1.5 tests pass -- [ ] 2.5 Update `limit` docstrings on both tools (concise, one line each); verify rendered tool schema mentions the default cap -- [ ] 2.6 Run the full non-integration suite (`poetry run pytest -m "not integration"`) and ruff; fix all failures +- [x] 2.1 Add `ResponseTruncationWarning` (kind `"truncated"`, `returned_rows`, `hint`) to slayer/core/warnings.py and the `AnySlayerWarning` union; verify union round-trip test passes +- [x] 2.2 Add default-cap constant and cap helpers to slayer/mcp/server.py (compute cap/pushed-down limit from caller args; slice + append warning); verify helper-level tests pass +- [x] 2.3 Wire the cap into `query`: push-down on the structured no-limit path, response-side slice for run-by-name and explain; verify tasks 1.1–1.4 and 1.7 tests pass +- [x] 2.4 Wire the cap into `query_nested` keyed on the root stage, copying the root dict; verify 1.5 tests pass +- [x] 2.5 Update `limit` docstrings on both tools (concise, one line each); verify rendered tool schema mentions the default cap +- [x] 2.6 Run the full non-integration suite (`poetry run pytest -m "not integration"`) and ruff; fix all failures ## 3. Documentation -- [ ] 3.1 Update docs/reference/mcp.md: default cap, notice, root-stage rule for query_nested, json shape change on truncation; verify by proofread -- [ ] 3.2 Update .claude/skills/slayer-query.md with a one-line mention of the cap; verify by proofread +- [x] 3.1 Update docs/reference/mcp.md: default cap, notice, root-stage rule for query_nested, json shape change on truncation; verify by proofread +- [x] 3.2 Update .claude/skills/slayer-query.md with a one-line mention of the cap; verify by proofread diff --git a/slayer/core/warnings.py b/slayer/core/warnings.py index a983093d..aa09aabe 100644 --- a/slayer/core/warnings.py +++ b/slayer/core/warnings.py @@ -81,10 +81,28 @@ def human_message(self) -> str: ) +class ResponseTruncationWarning(SlayerWarning): + """A response sliced to a row cap; ``hint`` tells the caller how to get more rows. Emitted by the MCP layer only, never by the engine.""" + + kind: Literal["truncated"] = "truncated" + returned_rows: int + hint: str + + def human_message(self) -> str: + return ( + f"showing first {self.returned_rows} rows — more rows exist; {self.hint}" + ) + + # Discriminated union, not the bare base: a ``List[SlayerWarning]`` would validate # down to the base type and drop subclass fields. Keyed on ``kind``, each round-trips. AnySlayerWarning = Annotated[ - Union[NormalizationWarning, DroppedFilterWarning, BroadcastGrainWarningPayload], + Union[ + NormalizationWarning, + DroppedFilterWarning, + BroadcastGrainWarningPayload, + ResponseTruncationWarning, + ], Field(discriminator="kind"), ] diff --git a/slayer/mcp/server.py b/slayer/mcp/server.py index 6f6e0170..59118e28 100644 --- a/slayer/mcp/server.py +++ b/slayer/mcp/server.py @@ -27,6 +27,7 @@ ) from slayer.core.query import ModelExtension, SlayerQuery from slayer.core.recommend import render_recommendation_markdown +from slayer.core.warnings import ResponseTruncationWarning from slayer import async_utils from slayer.engine import ingestion as engine_ingestion from slayer.engine.ingestion import ( @@ -72,6 +73,11 @@ VALID_DIMENSION_TYPES = {"string", "time", "date", "boolean", "number"} _UNSET = object() # Sentinel to distinguish "not provided" from "explicitly set to None" +# Response row cap when the caller passes no limit; an explicit limit is trusted verbatim. +_MCP_ROW_CAP = 20 +_CAP_HINT = "pass a higher 'limit' to get more rows" +_NESTED_CAP_HINT = "pass a higher 'limit' on the root query to get more rows" + # Shared remedy for every mcp-import failure below. _MCP_REMEDY = ( "Install a supported version: pip install 'mcp>=1.0,<2' " @@ -500,7 +506,7 @@ async def query( # NOSONAR S107 — FastMCP introspects this signature to expos "change(revenue:sum) > 0", "last(change(revenue:sum)) < 0". time_dimensions: Time grouping. Format: {"dimension": "created_at", "granularity": "day|week|month|quarter|year", "date_range": ["2024-01-01", "2024-12-31"]}. order: Sorting. Format: {"column": "measure_or_dim_name", "direction": "asc|desc"}. - limit: Max rows to return. + limit: Max rows to return, trusted verbatim; without it the response is capped at 20 rows with a truncation notice. offset: Number of rows to skip. whole_periods_only: When true, snap date filters to time bucket boundaries based on granularity, exclude the current incomplete time bucket. show_sql: When true, include the generated SQL in the response for debugging. @@ -572,6 +578,8 @@ async def query( # NOSONAR S107 — FastMCP introspects this signature to expos ) if dry_run: return f"SQL:\n{result.sql}" + # Push-down can't reach the stored SQL; cap response-side. + _cap_rows(result, hint=_CAP_HINT) if explain: output = f"SQL:\n{result.sql}\n\nQuery Plan:\n" output += _format_output(result=result, fmt=fmt) @@ -580,6 +588,11 @@ async def query( # NOSONAR S107 — FastMCP introspects this signature to expos if show_sql and result.sql: output = f"SQL:\n{result.sql}\n\n{output}" return output + # No explicit limit: push down cap+1 so truncation is detectable + # without fetching the full result, then slice to the cap. + capped = limit is None + if capped: + data["limit"] = _MCP_ROW_CAP + 1 slayer_query = SlayerQuery.model_validate(data) result = await engine.execute( query=slayer_query, @@ -589,6 +602,8 @@ async def query( # NOSONAR S107 — FastMCP introspects this signature to expos ) if dry_run: return f"SQL:\n{result.sql}" + if capped: + _cap_rows(result, hint=_CAP_HINT) if explain: output = f"SQL:\n{result.sql}\n\nQuery Plan:\n" output += _format_output(result=result, fmt=fmt) @@ -635,7 +650,9 @@ async def query_nested( Args: queries: Ordered list of stage dicts. Earlier stages must be - named; the last stage is the one whose rows return. + named; the last stage is the one whose rows return. Without + a root-stage ``limit`` the response is capped at 20 rows + with a truncation notice. variables: Variable values for ``{var}`` placeholder substitution in filters. Runtime kwarg precedence: ``runtime > stage.variables > outer query.variables > @@ -662,14 +679,22 @@ async def query_nested( raise ValueError(f"Invalid format '{format}'. Must be one of: json, csv, markdown") if not queries: raise ValueError("'queries' must be a non-empty list of query dicts.") + # Cap keys on the root (last) stage only; copy its dict — never + # mutate caller input. + capped = queries[-1].get("limit") is None + exec_queries = list(queries) + if capped: + exec_queries[-1] = {**exec_queries[-1], "limit": _MCP_ROW_CAP + 1} result = await engine.execute( - query=list(queries), + query=exec_queries, variables=variables, dry_run=dry_run, explain=explain, ) if dry_run: return f"SQL:\n{result.sql}" + if capped: + _cap_rows(result, hint=_NESTED_CAP_HINT) if explain: output = f"SQL:\n{result.sql}\n\nQuery Plan:\n" output += _format_output(result=result, fmt=fmt) @@ -2138,6 +2163,17 @@ def _format_csv(data: list[dict[str, Any]], columns: list[str]) -> str: return "\n".join(lines) +def _cap_rows(result: SlayerResponse, *, hint: str) -> None: + """Slice past-cap rows and append the truncation notice. No-limit paths only.""" + if len(result.data) <= _MCP_ROW_CAP: + return + result.data = result.data[:_MCP_ROW_CAP] + result.warnings = [ + *result.warnings, + ResponseTruncationWarning(returned_rows=_MCP_ROW_CAP, hint=hint), + ] + + def _csv_warning_comments(result: SlayerResponse) -> str: """Warnings as leading `#` comment lines for CSV output (uniform column count).""" lines = [f"# warning: {w.human_message()}" for w in (result.warnings or [])] diff --git a/tests/test_mcp_row_cap.py b/tests/test_mcp_row_cap.py new file mode 100644 index 00000000..341647eb --- /dev/null +++ b/tests/test_mcp_row_cap.py @@ -0,0 +1,415 @@ +"""MCP query-tool response row cap: default 20-row cap, LIMIT push-down, +truncation notice via the warnings channel (spec: mcp/response-row-cap).""" + +from __future__ import annotations + +import copy +import csv +import io +import json +import sqlite3 +from pathlib import Path +from typing import Any + +from slayer.core.enums import DataType +from slayer.core.models import Column, DatasourceConfig, SlayerModel +from slayer.core.query import SlayerQuery +from slayer.core.warnings import NormalizationWarning, ResponseTruncationWarning +from slayer.engine.query_engine import SlayerQueryEngine, SlayerResponse +from slayer.mcp.server import create_mcp_server +from slayer.storage.yaml_storage import YAMLStorage + +CAP = 20 +NOTICE = f"showing first {CAP} rows — more rows exist" + + +def _make_db(workspace: Path, *, rows: int = 30) -> Path: + db = workspace / "live.db" + conn = sqlite3.connect(db) + conn.execute("CREATE TABLE nums (id INTEGER PRIMARY KEY, v INTEGER)") + conn.executemany( + "INSERT INTO nums (id, v) VALUES (?, ?)", + [(i, i * 10) for i in range(1, rows + 1)], + ) + conn.commit() + conn.close() + return db + + +async def _seed_storage(workspace: Path) -> YAMLStorage: + """A ``lite`` sqlite datasource with a 30-row ``nums`` model plus two + stored query-backed models returning 25 (``qb_over``) and 20 (``qb_at_cap``) rows.""" + db = _make_db(workspace) + storage = YAMLStorage(base_dir=str(workspace / "store")) + await storage.save_datasource(DatasourceConfig(name="lite", type="sqlite", database=str(db))) + await storage.save_model(SlayerModel( + name="nums", data_source="lite", sql_table="nums", + columns=[ + Column(name="id", sql="id", type=DataType.INT, primary_key=True), + Column(name="v", sql="v", type=DataType.INT), + ], + )) + await storage.save_model(SlayerModel( + name="qb_over", data_source="lite", + source_queries=[SlayerQuery( + source_model="nums", dimensions=["id"], filters=["id <= 25"], + )], + )) + await storage.save_model(SlayerModel( + name="qb_at_cap", data_source="lite", + source_queries=[SlayerQuery( + source_model="nums", dimensions=["id"], filters=["id <= 20"], + )], + )) + return storage + + +async def _make_server(tmp_path: Path): + storage = await _seed_storage(tmp_path) + return create_mcp_server(storage=storage) + + +async def _call(server, *, name: str, arguments: dict[str, Any] | None = None) -> str: + content_blocks, _ = await server.call_tool(name=name, arguments=arguments or {}) + return content_blocks[0].text + + +def _json_payload(text: str) -> Any: + """Decode the leading JSON value, ignoring any trailing footer text.""" + payload, _ = json.JSONDecoder().raw_decode(text) + return payload + + +def _json_after_plan(text: str) -> Any: + return _json_payload(text.split("Query Plan:\n", 1)[1]) + + +def _canned_response(n: int, *, warnings: list | None = None) -> SlayerResponse: + return SlayerResponse( + data=[{"nums.id": i} for i in range(1, n + 1)], + columns=["nums.id"], + sql="SELECT 1", + warnings=warnings or [], + ) + + +def _patch_execute(monkeypatch, *, make_response) -> None: + """Replace engine execution with a canned-response factory.""" + async def fake_execute(self, *args: Any, **kwargs: Any) -> SlayerResponse: + return make_response() + + monkeypatch.setattr(SlayerQueryEngine, "execute", fake_execute) + + +def _assert_truncated(payload: Any, *, rows: int = CAP) -> dict: + """Payload is {"data", "warnings"} with ``rows`` rows and a last + truncation warning; returns that warning entry.""" + assert isinstance(payload, dict), f"expected truncated shape, got {type(payload)}" + assert len(payload["data"]) == rows + warning = payload["warnings"][-1] + assert warning["kind"] == "truncated" + assert warning["returned_rows"] == rows + return warning + + +def _assert_untruncated(payload: Any, *, rows: int) -> None: + assert isinstance(payload, list), f"expected bare array, got {type(payload)}" + assert len(payload) == rows + + +class TestDefaultCap: + """Requirement: default row cap on MCP query responses.""" + + async def test_uncapped_query_over_large_result(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], "format": "json", + }) + _assert_truncated(_json_payload(result)) + + async def test_result_exactly_at_cap(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], + "filters": ["id <= 20"], "format": "json", + }) + _assert_untruncated(_json_payload(result), rows=20) + + async def test_result_one_past_cap(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], + "filters": ["id <= 21"], "format": "json", + }) + _assert_truncated(_json_payload(result)) + + +class TestExplicitLimitTrusted: + """Requirement: an explicit ``limit`` is trusted verbatim — no slice, no notice.""" + + async def test_explicit_limit_honored(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], + "limit": 25, "format": "json", + }) + _assert_untruncated(_json_payload(result), rows=25) + + async def test_explicit_small_limit(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], + "limit": 5, "format": "json", + }) + _assert_untruncated(_json_payload(result), rows=5) + + async def test_rows_exceeding_explicit_limit_not_sliced( + self, tmp_path: Path, monkeypatch, + ) -> None: + """The cap decision keys on caller args, never on the executed row count.""" + server = await _make_server(tmp_path) + _patch_execute(monkeypatch, make_response=lambda: _canned_response(30)) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], + "limit": 5, "format": "json", + }) + _assert_untruncated(_json_payload(result), rows=30) + + async def test_explain_with_explicit_limit_untouched( + self, tmp_path: Path, monkeypatch, + ) -> None: + server = await _make_server(tmp_path) + _patch_execute(monkeypatch, make_response=lambda: _canned_response(25)) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], + "limit": 5, "explain": True, "format": "json", + }) + _assert_untruncated(_json_after_plan(result), rows=25) + + +class TestPushDown: + """Requirement: cap push-down into the generated query (cap + 1).""" + + async def test_dry_run_sql_carries_limit_21(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], "dry_run": True, + }) + assert "LIMIT 21" in result + assert "LIMIT 20" not in result + + async def test_show_sql_carries_limit_21(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], + "show_sql": True, "format": "json", + }) + assert "LIMIT 21" in result + assert "LIMIT 20" not in result + + async def test_run_by_name_stored_sql_untouched(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query", arguments={ + "source_model": "qb_over", "dry_run": True, + }) + assert "SQL:" in result + assert "LIMIT" not in result.upper() + + +class TestRunByNameCap: + """Requirement: run-by-name responses are capped response-side.""" + + async def test_stored_query_above_cap(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query", arguments={ + "source_model": "qb_over", "format": "json", + }) + warning = _assert_truncated(_json_payload(result)) + # Uniform hint even though passing `limit` switches execution paths. + assert "limit" in warning["hint"] + + async def test_stored_query_at_cap(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query", arguments={ + "source_model": "qb_at_cap", "format": "json", + }) + _assert_untruncated(_json_payload(result), rows=20) + + +class TestQueryNestedCap: + """Requirement: query_nested capped by the root stage's limit only.""" + + async def test_root_without_limit_capped(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query_nested", arguments={ + "queries": [{"source_model": "nums", "dimensions": ["id"]}], + "format": "json", + }) + warning = _assert_truncated(_json_payload(result)) + assert "root" in warning["hint"] + assert "limit" in warning["hint"] + + async def test_non_root_limit_does_not_lift_cap(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query_nested", arguments={ + "queries": [ + {"name": "base", "source_model": "nums", + "dimensions": ["id"], "limit": 30}, + {"source_model": "base", "dimensions": ["id"]}, + ], + "format": "json", + }) + _assert_truncated(_json_payload(result)) + + async def test_root_limit_trusted(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query_nested", arguments={ + "queries": [ + {"source_model": "nums", "dimensions": ["id"], "limit": 25}, + ], + "format": "json", + }) + _assert_untruncated(_json_payload(result), rows=25) + + async def test_rows_exceeding_root_limit_not_sliced( + self, tmp_path: Path, monkeypatch, + ) -> None: + server = await _make_server(tmp_path) + _patch_execute(monkeypatch, make_response=lambda: _canned_response(30)) + result = await _call(server, name="query_nested", arguments={ + "queries": [ + {"source_model": "nums", "dimensions": ["id"], "limit": 5}, + ], + "format": "json", + }) + _assert_untruncated(_json_payload(result), rows=30) + + async def test_root_dry_run_sql_carries_limit_21(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query_nested", arguments={ + "queries": [{"source_model": "nums", "dimensions": ["id"]}], + "dry_run": True, + }) + assert "LIMIT 21" in result + assert "LIMIT 20" not in result + + async def test_caller_dicts_not_mutated(self, tmp_path: Path) -> None: + """Push-down must copy the root dict, not add ``limit`` to caller input.""" + server = await _make_server(tmp_path) + # Direct closure call: the wire path would copy the dicts anyway. + fn = server._tool_manager.get_tool("query_nested").fn + queries = [{"source_model": "nums", "dimensions": ["id"]}] + snapshot = copy.deepcopy(queries) + await fn(queries=queries, format="json") + assert queries == snapshot + + +class TestNoticeRendering: + """Requirement: truncation notice content and rendering in all formats.""" + + async def test_markdown_notice(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], + }) + assert "Warnings:" in result + assert NOTICE in result + assert "limit" in result.split("Warnings:", 1)[1] + # The Warnings block ends the output, notice last. + assert NOTICE in result.rstrip().splitlines()[-1] + assert result.index("Warnings:") > result.index("| ") + table_lines = [ln for ln in result.splitlines() if ln.startswith("|")] + assert len(table_lines) == 2 + CAP # header + separator + rows + + async def test_csv_notice(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id", "v"], "format": "csv", + }) + lines = result.splitlines() + assert lines[0].startswith("#") + assert NOTICE in lines[0] + data_lines = [ln for ln in lines if not ln.startswith("#")] + rows = list(csv.reader(io.StringIO("\n".join(data_lines)))) + assert len(rows) == 1 + CAP # header + rows + assert all(len(r) == 2 for r in rows) + + async def test_json_notice(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], "format": "json", + }) + warning = _assert_truncated(_json_payload(result)) + assert "limit" in warning["hint"] + + async def test_coexists_with_other_warnings( + self, tmp_path: Path, monkeypatch, + ) -> None: + server = await _make_server(tmp_path) + norm = NormalizationWarning( + rule_id="R1", original="a", normalized="b", location="filters[0]", + ) + _patch_execute( + monkeypatch, + make_response=lambda: _canned_response(25, warnings=[norm]), + ) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], "format": "json", + }) + payload = _json_payload(result) + _assert_truncated(payload) + kinds = [w["kind"] for w in payload["warnings"]] + assert kinds == ["normalization", "truncated"] + + md = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], + }) + assert "[R1]" in md + assert NOTICE in md + assert md.index("[R1]") < md.index(NOTICE) + + csv_out = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], "format": "csv", + }) + comment_lines = [ln for ln in csv_out.splitlines() if ln.startswith("#")] + assert len(comment_lines) == 2 + assert "[R1]" in comment_lines[0] + assert NOTICE in comment_lines[1] + + def test_union_round_trip(self) -> None: + resp = SlayerResponse( + data=[], + warnings=[ResponseTruncationWarning( + returned_rows=CAP, hint="pass a higher 'limit'", + )], + ) + parsed = SlayerResponse.model_validate_json(resp.model_dump_json()) + warning = parsed.warnings[0] + assert isinstance(warning, ResponseTruncationWarning) + assert warning.kind == "truncated" + assert warning.returned_rows == CAP + assert warning.hint == "pass a higher 'limit'" + + +class TestExplainDryRun: + """Requirement: explain plans capped without a limit; dry_run never truncates.""" + + async def test_large_explain_plan_capped( + self, tmp_path: Path, monkeypatch, + ) -> None: + server = await _make_server(tmp_path) + _patch_execute(monkeypatch, make_response=lambda: _canned_response(25)) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], + "explain": True, "format": "json", + }) + _assert_truncated(_json_after_plan(result)) + + async def test_dry_run_never_truncates(self, tmp_path: Path) -> None: + server = await _make_server(tmp_path) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], "dry_run": True, + }) + assert "SQL:" in result + assert "truncated" not in result + assert "Warnings:" not in result From 73b476ba01b91d48734550e8e681aa7e08895e93 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 4 Sep 2026 16:08:51 +0200 Subject: [PATCH 3/4] DEV-1810: keep truncation notice trailing after attributes in markdown Field attributes were appended after _format_output, which for markdown already carries the trailing Warnings block, so a truncated response with labeled/formatted fields no longer ended with the notice. Render the attributes footer before the markdown warnings via a new _format_output footer param; add a regression test. --- slayer/mcp/server.py | 32 ++++++++++++++++++++------------ tests/test_mcp_row_cap.py | 28 +++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/slayer/mcp/server.py b/slayer/mcp/server.py index 59118e28..6147fe29 100644 --- a/slayer/mcp/server.py +++ b/slayer/mcp/server.py @@ -608,11 +608,11 @@ async def query( # NOSONAR S107 — FastMCP introspects this signature to expos output = f"SQL:\n{result.sql}\n\nQuery Plan:\n" output += _format_output(result=result, fmt=fmt) return output - output = _format_output(result=result, fmt=fmt) + output = _format_output( + result=result, fmt=fmt, footer=_attributes_footer(result.attributes), + ) if show_sql and result.sql: output = f"SQL:\n{result.sql}\n\n{output}" - if result.attributes and (result.attributes.dimensions or result.attributes.measures): - output += "\n\n" + _format_attributes(attributes=result.attributes) return output except Exception as e: if isinstance(e, (sa.exc.OperationalError, sa.exc.DatabaseError)): @@ -699,11 +699,11 @@ async def query_nested( output = f"SQL:\n{result.sql}\n\nQuery Plan:\n" output += _format_output(result=result, fmt=fmt) return output - output = _format_output(result=result, fmt=fmt) + output = _format_output( + result=result, fmt=fmt, footer=_attributes_footer(result.attributes), + ) if show_sql and result.sql: output = f"SQL:\n{result.sql}\n\n{output}" - if result.attributes and (result.attributes.dimensions or result.attributes.measures): - output += "\n\n" + _format_attributes(attributes=result.attributes) return output except Exception as e: if isinstance(e, (sa.exc.OperationalError, sa.exc.DatabaseError)): @@ -2186,24 +2186,25 @@ def _format_warnings(result: SlayerResponse) -> str: return "" if not lines else "\n\nWarnings:\n" + "\n".join(lines) -def _format_output(result: SlayerResponse, fmt: str) -> str: +def _format_output(result: SlayerResponse, fmt: str, *, footer: str = "") -> str: """Format query output in the requested format. Warnings stay machine-safe: inside the json payload, leading `#` lines for - csv, a prose block only for markdown. + csv, a prose block only for markdown. ``footer`` (the attributes block) sits + before the markdown warnings so the warnings stay the trailing block. """ if fmt == "csv": # Leading `#` lines, never trailing prose — trailing rows break the # column count for every CSV reader. return _csv_warning_comments(result) + _format_csv( data=result.data, columns=result.columns, - ) + ) + footer if fmt == "markdown": - return result.to_markdown() + _format_warnings(result) + return result.to_markdown() + footer + _format_warnings(result) return _format_json( data=result.data, warnings=[w.model_dump(mode="json") for w in (result.warnings or [])], - ) + ) + footer def _format_field_meta(entries: dict[str, Any]) -> list[str]: @@ -2236,4 +2237,11 @@ def _format_attributes(attributes) -> str: if measure_lines: lines.append("Measure attributes:") lines.extend(measure_lines) - return "\n".join(lines)if lines else "" \ No newline at end of file + return "\n".join(lines)if lines else "" + + +def _attributes_footer(attributes) -> str: + """Attributes block as a trailing footer, or empty when there's nothing to show.""" + if attributes and (attributes.dimensions or attributes.measures): + return "\n\n" + _format_attributes(attributes=attributes) + return "" \ No newline at end of file diff --git a/tests/test_mcp_row_cap.py b/tests/test_mcp_row_cap.py index 341647eb..74cda13c 100644 --- a/tests/test_mcp_row_cap.py +++ b/tests/test_mcp_row_cap.py @@ -15,7 +15,12 @@ from slayer.core.models import Column, DatasourceConfig, SlayerModel from slayer.core.query import SlayerQuery from slayer.core.warnings import NormalizationWarning, ResponseTruncationWarning -from slayer.engine.query_engine import SlayerQueryEngine, SlayerResponse +from slayer.engine.query_engine import ( + FieldMetadata, + ResponseAttributes, + SlayerQueryEngine, + SlayerResponse, +) from slayer.mcp.server import create_mcp_server from slayer.storage.yaml_storage import YAMLStorage @@ -376,6 +381,27 @@ async def test_coexists_with_other_warnings( assert "[R1]" in comment_lines[0] assert NOTICE in comment_lines[1] + async def test_markdown_notice_trails_attributes( + self, tmp_path: Path, monkeypatch, + ) -> None: + """Field attributes render before the trailing Warnings block, so the + notice stays last even when the response carries metadata.""" + server = await _make_server(tmp_path) + attrs = ResponseAttributes(dimensions={"nums.id": FieldMetadata(label="ID")}) + _patch_execute( + monkeypatch, + make_response=lambda: SlayerResponse( + data=[{"nums.id": i} for i in range(1, 25)], + columns=["nums.id"], sql="SELECT 1", attributes=attrs, + ), + ) + result = await _call(server, name="query", arguments={ + "source_model": "nums", "dimensions": ["id"], + }) + assert "Dimension attributes:" in result + assert result.index("Dimension attributes:") < result.index("Warnings:") + assert NOTICE in result.rstrip().splitlines()[-1] + def test_union_round_trip(self) -> None: resp = SlayerResponse( data=[], From 9bf0fda231bfda37fe5718ed259b230767a6bf11 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 4 Sep 2026 16:32:09 +0200 Subject: [PATCH 4/4] =?UTF-8?q?DEV-1810:=20archive=20OpenSpec=20change=20?= =?UTF-8?q?=E2=80=94=20merge=20response-row-cap=20spec=20into=20specs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../.openspec.yaml | 0 .../design.md | 0 .../proposal.md | 0 .../specs/mcp/response-row-cap/spec.md | 0 .../tasks.md | 0 openspec/specs/mcp/response-row-cap/spec.md | 144 ++++++++++++++++++ 6 files changed, 144 insertions(+) rename openspec/changes/{dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation => archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation}/.openspec.yaml (100%) rename openspec/changes/{dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation => archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation}/design.md (100%) rename openspec/changes/{dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation => archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation}/proposal.md (100%) rename openspec/changes/{dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation => archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation}/specs/mcp/response-row-cap/spec.md (100%) rename openspec/changes/{dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation => archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation}/tasks.md (100%) create mode 100644 openspec/specs/mcp/response-row-cap/spec.md diff --git a/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/.openspec.yaml b/openspec/changes/archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/.openspec.yaml similarity index 100% rename from openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/.openspec.yaml rename to openspec/changes/archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/.openspec.yaml diff --git a/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/design.md b/openspec/changes/archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/design.md similarity index 100% rename from openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/design.md rename to openspec/changes/archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/design.md diff --git a/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/proposal.md b/openspec/changes/archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/proposal.md similarity index 100% rename from openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/proposal.md rename to openspec/changes/archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/proposal.md diff --git a/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/specs/mcp/response-row-cap/spec.md b/openspec/changes/archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/specs/mcp/response-row-cap/spec.md similarity index 100% rename from openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/specs/mcp/response-row-cap/spec.md rename to openspec/changes/archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/specs/mcp/response-row-cap/spec.md diff --git a/openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/tasks.md b/openspec/changes/archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/tasks.md similarity index 100% rename from openspec/changes/dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/tasks.md rename to openspec/changes/archive/2026-09-04-dev-1810-mcp-query-tool-add-a-response-row-cap-with-a-truncation/tasks.md diff --git a/openspec/specs/mcp/response-row-cap/spec.md b/openspec/specs/mcp/response-row-cap/spec.md new file mode 100644 index 00000000..c96cdccb --- /dev/null +++ b/openspec/specs/mcp/response-row-cap/spec.md @@ -0,0 +1,144 @@ +# mcp/response-row-cap Specification + +## Purpose +Protects the calling agent's context window: MCP query responses are capped at a small default row count unless the caller sets an explicit limit, and a truncated response says so and tells the caller how to get more rows. + +## Requirements + +### Requirement: Default row cap on MCP query responses + +When the MCP `query` tool is called without a `limit`, the response SHALL contain at most 20 data rows. When the underlying result has more rows than the cap, the response SHALL be truncated to exactly 20 rows and carry a truncation notice. + +#### Scenario: Uncapped query over a large result + +- WHEN `query` runs without `limit` against a model whose result has more than 20 rows +- THEN the response contains exactly 20 data rows and a truncation notice + +#### Scenario: Result exactly at the cap + +- WHEN `query` runs without `limit` and the result has exactly 20 rows +- THEN all 20 rows are returned and no truncation notice appears + +#### Scenario: Result one past the cap + +- WHEN `query` runs without `limit` and the result has exactly 21 rows +- THEN the response contains exactly 20 rows and a truncation notice + +### Requirement: Explicit limit is trusted verbatim + +When the caller passes an explicit `limit`, the MCP layer SHALL return the rows as executed, with no response-side truncation and no truncation notice — regardless of how many rows come back, including `explain` plan rows. + +#### Scenario: Explicit limit honored + +- WHEN `query` runs with `limit=25` against a result with 30 available rows +- THEN 25 rows are returned and no truncation notice appears + +#### Scenario: Explicit small limit + +- WHEN `query` runs with `limit=5` +- THEN 5 rows are returned and no truncation notice appears + +#### Scenario: Rows exceeding an explicit limit are not sliced by the MCP layer + +- WHEN the engine returns more rows than an explicit `limit` (e.g. a mocked execution) +- THEN the MCP layer neither slices the rows nor adds a truncation notice + +#### Scenario: Explain with explicit limit untouched + +- WHEN `query` runs with `explain=True` and an explicit `limit`, and the plan has more rows than the limit +- THEN all plan rows are returned and no truncation notice appears + +### Requirement: Cap push-down into the generated query + +When no explicit `limit` is given on the structured query path, the generated SQL SHALL carry `LIMIT 21` (cap + 1) so truncation is detectable without fetching the full result. Run-by-name execution of a stored query SHALL leave the stored query's SQL untouched. + +#### Scenario: Pushed-down limit visible in SQL + +- WHEN `query` runs without `limit` on the structured path with `show_sql=True` or `dry_run=True` +- THEN the generated SQL contains `LIMIT 21`, not `LIMIT 20` + +#### Scenario: Stored query SQL untouched + +- WHEN a stored query runs by bare name (run-by-name path) without `limit` +- THEN the SQL executed is the stored query's own, with no injected LIMIT + +### Requirement: Run-by-name responses are capped response-side + +A stored query executed by bare name without a `limit` SHALL have its response sliced to 20 rows with a truncation notice when it returns more, and returned whole with no notice when it returns 20 or fewer. + +#### Scenario: Stored query above the cap + +- WHEN a run-by-name stored query returns more than 20 rows +- THEN the response contains exactly 20 rows and a truncation notice + +#### Scenario: Stored query at the cap + +- WHEN a run-by-name stored query returns exactly 20 rows +- THEN all 20 rows are returned and no truncation notice appears + +### Requirement: query_nested capped by the root stage's limit only + +The `query_nested` tool SHALL apply the same rule keyed on the ROOT stage (last entry of `queries`): an explicit root `limit` is trusted verbatim; without one, the final response is capped at 20 with a truncation notice whose hint points at the root query's `limit`. Non-root stages' limits SHALL NOT affect the cap. The tool SHALL NOT mutate the caller's `queries` dicts. + +#### Scenario: Root without limit is capped + +- WHEN `query_nested` runs with a root stage that has no `limit` and the final result has more than 20 rows +- THEN the response contains exactly 20 rows and a truncation notice telling the caller to set a higher `limit` on the root query + +#### Scenario: Non-root limit does not lift the cap + +- WHEN a non-root stage has an explicit `limit` but the root stage has none +- THEN the default cap of 20 still applies to the final response + +#### Scenario: Root limit trusted + +- WHEN the root stage has an explicit `limit` +- THEN no response-side truncation occurs and no notice appears + +#### Scenario: Caller dicts unchanged + +- WHEN `query_nested` pushes the cap into the root stage +- THEN the caller's submitted `queries` dicts are structurally unchanged afterwards (no `limit` key added) + +### Requirement: Truncation notice content and rendering + +The truncation notice SHALL state the returned row count, say that more rows exist, and tell the caller how to get more rows. It SHALL appear in every output format through the warnings channel: the markdown `Warnings:` block, a leading `#` comment line in csv, and a warning entry with kind `"truncated"` in the json `{"data", "warnings"}` payload. It SHALL coexist with other warnings, appended last. + +#### Scenario: Markdown notice + +- WHEN a truncated result is formatted as markdown +- THEN the output ends with a `Warnings:` block containing "showing first 20 rows — more rows exist" and a hint to pass a higher `limit` + +#### Scenario: CSV notice + +- WHEN a truncated result is formatted as csv +- THEN a leading `#` comment line carries the notice and the data rows below keep a uniform column count + +#### Scenario: JSON notice + +- WHEN a truncated result is formatted as json +- THEN the payload has the `{"data", "warnings"}` shape and `warnings` contains an entry with `kind == "truncated"` and the returned row count + +#### Scenario: Coexists with other warnings + +- WHEN a truncated result already carries an engine warning +- THEN both warnings render in every format and the truncation notice comes last + +#### Scenario: Warning round-trips through the union + +- WHEN a response carrying the truncation warning is serialized and re-validated +- THEN the warning deserializes back to the truncation kind with its fields intact + +### Requirement: Explain plan rows capped without a limit + +When `query` runs with `explain=True` and no `limit`, the returned plan rows SHALL be subject to the same 20-row cap and notice. `dry_run` output (SQL only) SHALL never carry a truncation notice. + +#### Scenario: Large explain plan capped + +- WHEN `explain=True` without `limit` yields a plan of more than 20 rows +- THEN 20 plan rows are returned with a truncation notice + +#### Scenario: Dry run unaffected + +- WHEN `dry_run=True` +- THEN the response contains only SQL and never a truncation notice