S8: section-level budgeting for inspect_model (closes #54)#70
Merged
Conversation
`inspect_model` now accepts three new params so an agent can drill into
a model without paying for the full envelope every call.
New params:
- `sections: Optional[List[str]] = None` — subset of {columns, measures,
aggregations, joins, reachable_fields, samples}. Default = all six.
- `descriptions_max_chars: Optional[int] = None` — trim long descriptions
with a literal ` ... [truncated]` marker.
- `reachable_fields_depth: int = 5` — exposes the previously-hardcoded
BFS-depth constant.
## Design choices
**`sections` enum is six names, not five**
The issue listed five (`columns`, `measures`, `joins`, `reachable_fields`,
`samples`); `aggregations` is added so an agent can skip a long
custom-aggregations table while keeping the rest. Default = all six,
so default behaviour is preserved.
**Always-on parts (not gated by `sections`)**
Header, metadata bullets (`data_source`, `sql_table`, `row_count`, …),
custom-SQL block (gated by `show_sql`), model-level filters block (gated
by `show_sql`), backing-query *structure* for query-backed models, and
the cached backing-query SQL (gated by `show_sql`). Backing-query
structure is identity-bearing for query-backed models — the equivalent
of `sql_table` for table-backed models — so it stays unconditional.
**`aggregations` is fully gated**
- When in `sections`: full table; with `show_sql=False` the `formula`
column and the `sql` field of each `params[]` entry are dropped (only
the SQL definition is gated by `show_sql`, not the section itself).
- When omitted from `sections`: collapses to a one-line backticked CSV
of aggregation names, regardless of `show_sql`.
**Names-only fallback for `columns`/`measures`/`aggregations`/`joins`**
Reuses `_markdown_table()`'s single-column collapse: heading
`## <Section> (N — names only)` followed by `` `id`, `amount`, … ``. For
`joins` the names are `target_model` strings.
**`reachable_fields` and `samples` fully omit when not selected**
They have no natural "names" to list. The footer documents what was
dropped so the agent knows it can re-call to fetch them.
**Truncation footer is one global block**
Markdown:
> Sections shown: columns.
> Names-only: measures, aggregations, joins.
> Omitted: reachable_fields, samples.
> Re-call inspect_model with `sections=[...]` to fetch.
The footer is suppressed entirely when nothing was trimmed (default
calls produce no footer). Unknown section names produce a leading
`> Warning: ignored unknown sections: …` line; the rest of the footer
renders only if there's also trimming.
**Unknown section names: warn + proceed (no exception)**
`sections=["colums"]` emits a warning line and proceeds with the valid
ones. If every entry is unknown, fall back to all six. Reasoning: an
LLM that mistypes a section should get a useful response plus a
correction signal, not a tool-call exception that may abort a longer
plan.
**JSON parity**
- Included section: full key (`columns: [...]`, etc.).
- Names-only: full key absent; sibling `<section>_names: [...]` array.
- Omitted: key absent.
- Top-level arrays added when non-empty: `omitted_sections`,
`names_only_sections`, `unknown_sections`. Default calls don't carry
these arrays at all.
**`descriptions_max_chars` scope**
Only the four `description` fields (model, column, measure,
aggregation). Labels, SQL, filters, and formulas are left alone —
truncating SQL or formulas mid-token would mislead agents. Marker is
literal ` ... [truncated]` (leading space, three ASCII dots, space,
square-bracketed token). `0` is allowed and yields just the marker for
any non-empty description.
**`reachable_fields_depth`**
Exposes the previously-hardcoded `max_depth=5` in
`_collect_reachable_fields` as a parameter (the issue said "already
exists; reaffirm" but the param didn't actually exist on the MCP
tool). No clamping; `depth=0` yields no reachable fields. The depth
value is ignored when `reachable_fields` isn't in `sections` (BFS
isn't run at all).
**Performance: skip DB calls for omitted sections**
- `_collect_dim_profile` + `_collect_measure_profile`: only run when
`columns` is in `sections` (they feed the `sampled` column).
- `engine.get_column_types`: only run when `samples` is in `sections`
(it feeds the sample-query construction).
- Sample-query execution: only run when `samples` is in `sections`.
- `_collect_reachable_fields`: only run when `reachable_fields` is in
`sections`.
- `_get_row_count`: stays unconditional (cheap, feeds always-on
metadata bullets).
**`num_rows` kept, NOT renamed**
The issue suggested `samples_max_rows: int = 5`. We kept
`num_rows: int = 3` because (a) the issue also says "default behaviour
unchanged" — `3 → 5` contradicts that; (b) the rename would break
existing callers using `num_rows=` keyword. Naming inconsistency is
preferable to a silent default change.
**No token-count footer**
Issue lists it as optional. Skipped to avoid a tiktoken/Claude-SDK
dependency and the "estimate vs. true count" ambiguity. Not a
prerequisite for section budgeting.
## Tests
- 35 new unit tests in `tests/test_mcp_server.py` across 6 classes:
section gating, descriptions truncation, reachable_fields_depth,
aggregations show_sql gating, JSON parity, and direct unit tests
for the new helpers.
- 2 new integration tests in `tests/integration/test_mcp_inspect.py`
exercising end-to-end gating + truncation against the real SQLite
fixture.
- 1287 tests passing (full suite minus postgres integration, which
needs a postgres server the sandbox can't spawn).
- ruff clean.
## Docs
- `docs/reference/mcp.md` and `docs/interfaces/mcp.md` updated with
the new params and gating semantics on the `inspect_model` row.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces query-backed models as a first-class SlayerModel source mode and expands the ChangesQuery-Backed Models & Inspect Tool Enhancements
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI<br/>(slayer query)
participant Engine as SlayerQueryEngine
participant Storage as Storage
participant MCP as MCP Server
participant DB as Database
User->>CLI: slayer query --model my_query --variables VAR=value
CLI->>CLI: Parse --variables into dict
CLI->>Engine: execute_sync(query_input=name, variables={...})
Engine->>Storage: Load model by name
Storage-->>Engine: SlayerModel (query-backed)
Engine->>Engine: Resolve {var} placeholders<br/>by precedence:<br/>runtime > stage > model defaults
Engine->>DB: Execute backing SQL<br/>(async via await)
DB-->>Engine: Results
Engine-->>CLI: SlayerResponse (.data, .sql, .row_count)
CLI->>CLI: Render table or --dry-run SQL
CLI-->>User: Display output
User->>MCP: inspect_model(name, sections=[columns],<br/>descriptions_max_chars=50)
MCP->>Storage: Load model
Storage-->>MCP: SlayerModel
MCP->>MCP: _resolve_inspect_sections(sections)<br/>→ validate & normalize
MCP->>MCP: Short-circuit:<br/>skip measures/reachable/samples
MCP->>DB: Profile only columns
DB-->>MCP: Dimension stats
MCP->>MCP: Render Columns table<br/>+ truncate descriptions
MCP->>MCP: _render_inspect_footer()<br/>→ "Omitted: Sample Data,<br/>Reachable Fields"
MCP-->>User: Markdown + footer warning
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The changes span multiple interconnected systems (MCP server, CLI, storage, query engine) with new branching logic for section gating, query-backed model routing, and variable precedence. The high file diversity, dense helper-function additions, and extensive test coverage for edge cases (unknown sections, truncation boundaries, depth limits, JSON parity) require careful validation of each layer's integration points and test assertions. Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/interfaces/rest-api.md (1)
58-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
orders.countin the response example.
orders._countis the internal enrichment alias; the user-facing response payload should showorders.count. As written, this example teaches the wrong column name for*:count.Suggested doc fix
{ "data": [ - {"orders.status": "completed", "orders._count": 42}, - {"orders.status": "pending", "orders._count": 15} + {"orders.status": "completed", "orders.count": 42}, + {"orders.status": "pending", "orders.count": 15} ], "row_count": 2, - "columns": ["orders.status", "orders._count"] + "columns": ["orders.status", "orders.count"] }Based on learnings: the user-visible output/result column name uses
orders.countwhile only the internalEnrichedMeasure.aliaskeepsorders._count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/interfaces/rest-api.md` around lines 58 - 66, Update the response example to use the user-facing column name `orders.count` instead of the internal enrichment alias `orders._count`: replace all occurrences of `orders._count` in the example JSON `data` objects and in the `columns` array so the docs show the correct user-visible output (the internal `EnrichedMeasure.alias` remains `orders._count`, but the returned payload should display `orders.count`).
🧹 Nitpick comments (6)
CLAUDE.md (2)
55-55: 💤 Low valueConsider clarifying "write-if-changed semantics".
The phrase "write-if-changed semantics" is implementation jargon that may not be immediately clear. Consider rephrasing for clarity:
-The cache refreshes on every `engine.execute` path (real, dry-run, explain) with write-if-changed semantics. +The cache refreshes on every `engine.execute` path (real, dry-run, explain), updating storage only when values have changed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 55, The phrase "write-if-changed semantics" is ambiguous; update the sentence describing cache behavior to plain language that explains when the cache is persisted: change the wording around how the cache refreshes on every engine.execute path (real, dry-run, explain) to explicitly state that the engine will recompute the model cache (columns + backing_query_sql) on each execution and only persist/update the stored cache if the recomputed values differ from the current cached values (no-op if unchanged). Ensure this clarification mentions SlayerModel.query_variables, engine.create_model_from_query, engine.save_model and engine.execute so readers can locate the related behavior.
56-56: ⚡ Quick winAdd context for variable precedence terms.
The variable precedence mentions "stage" and "outer query" without defining them in context. Readers unfamiliar with query-backed models and multi-stage queries may not understand these terms.
Consider adding a brief parenthetical explanation:
-Variable precedence (highest first): runtime kwarg > stage > outer query > model defaults. +Variable precedence (highest first): runtime kwarg > stage (in source_queries) > outer query > model defaults.Alternatively, reference where these concepts are explained: "See query-backed models above for stage and outer query definitions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 56, Update the "Run-by-name execution" paragraph to define or reference the terms "stage" and "outer query" used in variable precedence: either add a short parenthetical after "stage" (e.g., "(a stage in a multi-stage query where variables can be overridden)") and after "outer query" (e.g., "(the enclosing query when executing nested or query-backed models)"), or append "See query-backed models above for stage and outer query definitions" to the sentence; ensure references to engine.execute, execute_sync, the variables= kwarg, and SlayerQuery remain unchanged.tests/test_query_backed_models.py (1)
37-49: 💤 Low valueConsider converting to a pytest fixture for cleaner resource management.
The
_engine_with_ordershelper returns a tuple requiring manualtry/finally/cleanup()in every test. A pytest fixture withyieldwould be cleaner and less error-prone.♻️ Example fixture approach
`@pytest.fixture` async def engine_with_orders(): tmp = tempfile.TemporaryDirectory() storage = YAMLStorage(base_dir=tmp.name) await storage.save_datasource(_ds()) await storage.save_model(_orders_model()) engine = SlayerQueryEngine(storage=storage) yield engine tmp.cleanup() # Then tests become: async def test_missing_model_raises(self, engine_with_orders) -> None: with pytest.raises(ValueError, match="Model 'nope' not found"): await engine_with_orders.execute("nope")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_query_backed_models.py` around lines 37 - 49, Replace the helper function _engine_with_orders with an async pytest fixture (e.g., engine_with_orders) that creates the TemporaryDirectory, constructs YAMLStorage, saves _ds() and _orders_model(), yields a SlayerQueryEngine(storage=storage) and then calls tmp.cleanup() after yield; update tests to accept the fixture (engine_with_orders) instead of calling _engine_with_orders and remove manual try/finally cleanup.tests/test_api_server.py (2)
229-240: 💤 Low valueConsider using
async deftest methods instead ofasyncio.get_event_loop().run_until_complete().Per coding guidelines, tests use
pytest-asynciowithasyncio_mode = "auto", so test methods can beasync def. Theasyncio.get_event_loop()pattern is deprecated in Python 3.10+ and may cause issues. Consider refactoring to async fixtures or helper methods.♻️ Example refactor for test setup
+ `@pytest.fixture` + async def setup_ds_and_upstream(self, storage: YAMLStorage): + from slayer.core.models import DatasourceConfig + await storage.save_datasource(DatasourceConfig( + name="ds", type="sqlite", database=":memory:" + )) + await storage.save_model(SlayerModel( + name="upstream", sql_table="t", data_source="ds", + columns=[Column(name="amount", sql="amount", type=DataType.NUMBER)], + )) + def test_post_models_creates_query_backed_model( - self, client: TestClient, storage: YAMLStorage + self, client: TestClient, storage: YAMLStorage, setup_ds_and_upstream ) -> None: - from slayer.core.models import DatasourceConfig - import asyncio - asyncio.get_event_loop().run_until_complete( - storage.save_datasource(DatasourceConfig( - name="ds", type="sqlite", database=":memory:" - )) - ) - asyncio.get_event_loop().run_until_complete( - storage.save_model(SlayerModel( - name="upstream", sql_table="t", data_source="ds", - columns=[Column(name="amount", sql="amount", type=DataType.NUMBER)], - )) - ) resp = client.post("/models", json={Also applies to: 263-274, 338-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_api_server.py` around lines 229 - 240, The test uses synchronous loop calls; replace asyncio.get_event_loop().run_until_complete(...) with direct awaits inside async test functions (or async fixtures) so the calls to storage.save_datasource(DatasourceConfig(...)) and storage.save_model(SlayerModel(...)) become await storage.save_datasource(...) and await storage.save_model(...); update the test to be declared async def and/or move setup into an async fixture/helper that awaits creation of Column(name="amount", sql="amount", type=DataType.NUMBER) and the DatasourceConfig/SlayerModel saves to comply with pytest-asyncio asyncio_mode="auto".
331-375: 💤 Low valueTest is misplaced in
TestOpenAPI400Documentation.
test_post_query_run_by_name_dry_run_returns_sql_without_executingtests the dry-run execution behavior, not OpenAPI documentation. It should be moved toTestQueryBackedModelsAPIfor logical organization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_api_server.py` around lines 331 - 375, The test function test_post_query_run_by_name_dry_run_returns_sql_without_executing currently lives under the TestOpenAPI400Documentation group but asserts dry-run behavior for query-backed models; move this entire test function into the TestQueryBackedModelsAPI test class in the same tests/test_api_server.py file (or the appropriate query-backed models test class), keeping its body, imports (DatasourceConfig, SlayerModel, Column, DataType, SlayerSQLClient) and async storage setup unchanged, fix indentation to match the destination class, and ensure any class-level fixtures (client, storage) are available there before running the test suite.slayer/mcp/server.py (1)
629-633: 💤 Low valueConsider using typed set hints for clarity.
Minor: using
set[str]instead of baresetwould improve type clarity.- all_placeholders: set = set() + all_placeholders: set[str] = set() stage_dicts: List[dict] = [] - defaulted: set = set(model.query_variables.keys()) + defaulted: set[str] = set(model.query_variables.keys())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@slayer/mcp/server.py` around lines 629 - 633, Update the bare set annotations to typed set hints for clarity: change all_placeholders: set = set() and defaulted: set = set(model.query_variables.keys()) to use set[str] (or Set[str]) so their element type is explicit; if necessary import the appropriate typing names (e.g., Set or use built-in generics) and also consider tightening stage_dicts: List[dict] to List[dict[str, Any]] (with Any imported) for consistency with the typed collections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/slayer-models.md:
- Around line 123-129: The example shows delete_measures=["margin"] as a
dangling fragment; update the bullet to be a full edit_model(...) invocation so
it reads consistently with the other examples — call edit_model with model_name
and the delete_measures argument (e.g. use edit_model(model_name="orders",
delete_measures=["margin"])) and ensure it matches the style of the other
examples like edit_model(..., columns=...) and edit_model(..., measures=...).
In @.claude/skills/slayer-query.md:
- Line 87: Update the documentation to correct the behavior of runtime
variables: clarify that SlayerQueryEngine.execute(...) merges the provided
runtime kwargs into the combined variable set via dictionary unpacking (so keys
from the runtime "variables=" are retained even if unused by the query) rather
than silently dropping unknown keys; reference the variable precedence order
(runtime kwargs > stage.variables > outer query.variables >
model.query_variables) and state that unresolved {var} placeholders still raise
at execute time with model and stage named. Include references to
SlayerQueryEngine.execute, model.query_variables, and stage.variables so readers
can locate the relevant merging logic.
In `@CLAUDE.md`:
- Around line 54-56: The CLAUDE.md section currently documents query-backed
models (SlayerModel source modes, engine.create_model_from_query,
engine.save_model, engine.execute, variables precedence) which conflicts with
this PR's stated objective to add section-level budgeting to inspect_model (new
parameters: sections, descriptions_max_chars, reachable_fields_depth); update
the document so it matches the PR: either replace the query-backed/models text
with a clear description of inspect_model's new parameters and behavior (how
sections are selected, defaults and types for
sections/descriptions_max_chars/reachable_fields_depth, examples of partial
inspection, and where errors are raised) and mention any API/CLI/REST changes,
or if the query-backed content belongs to another PR, remove it from this file
and add it to the appropriate change set; locate references to inspect_model,
sections, descriptions_max_chars, reachable_fields_depth, and to
SlayerModel/engine.execute/engine.save_model in the diff to ensure you edit the
correct paragraph.
In `@docs/interfaces/mcp.md`:
- Line 73: The docs still refer to a "dimensions table" in the inspect_model
description but the actual schema/selector uses "columns"; update the
inspect_model doc text to rename "dimensions table" to "columns table" (and any
occurrences of the term "dimensions" in that sentence) so it matches the
sections param and the rendered "## Columns" heading, ensuring references to the
params `sections` and `reachable_fields_depth` and the `inspect_model` feature
remain unchanged.
In `@docs/reference/mcp.md`:
- Line 82: The inline code spans in the `inspect_model` row contain
leading/trailing spaces (triggers markdownlint MD038); locate the backticked
snippets referenced (e.g., the `format` default values currently shown as `
"markdown"` and ` "json"` and any similar spans in the `sections` list) and
remove the extra space characters inside the backticks so the literals are
backticked exactly (e.g., `"markdown"`, `"json"`, `["columns", ...]`), and scan
the rest of the `inspect_model` text to fix any other code spans with accidental
leading/trailing spaces.
In `@slayer/api/server.py`:
- Around line 88-104: The validation for run-by-name requests is missing
request.whole_periods_only in the disallowed fields check, so requests like
{"name": "...", "whole_periods_only": true} bypass validation and the flag is
never forwarded to engine.execute; update the disallowed collection (the tuple
containing request.source_model, request.measures, request.dimensions,
request.time_dimensions, request.filters, request.order, request.limit,
request.offset) to also include request.whole_periods_only so it is rejected
when name is set (keep the same error behavior in the HTTPException raised).
In `@slayer/cli.py`:
- Around line 451-463: The JSON branch assumes json.loads returns a dict; update
it to handle list payloads: after data = json.loads(query_input) check if
isinstance(data, list) and for each item (ensure it's a dict) inject
args.dry_run/args.explain flags (set item["dry_run"]=True / item["explain"]=True
when args present), then validate each with SlayerQuery.model_validate (or
validate the list if SlayerQuery supports list validation) and pass the
resulting list to engine.execute_sync(query=..., variables=...), ensuring
explain_set/dry_run_set reflect the flags (e.g., True if args set or any
validated query has those flags). If data is a single dict continue current
logic but avoid mutating original input in unexpected ways.
- Around line 691-694: Wrap the call to run_sync(engine.save_model(model))
(where engine is an instance of SlayerQueryEngine and save_model may raise
ValueError) in a try/except that catches ValueError and prints a CLI-style error
message and exits with non-zero status; specifically catch ValueError from
SlayerQueryEngine.save_model, emit a single-line "Error: <message>" to stderr
(matching other CLI paths) and call sys.exit(1) so validation/cache-population
failures become normal CLI errors rather than tracebacks.
In `@slayer/mcp/server.py`:
- Around line 883-885: The code calls storage.get_model(source_model) twice;
instead, call it once and reuse the result by assigning it to a local variable
(e.g., target) before the if-check: replace the first occurrence in the
condition that currently checks await storage.get_model(source_model) with an
awaited assignment to target (target = await storage.get_model(source_model))
and then use no_overrides and target is not None in the if, followed by the
existing target.source_queries check; ensure you keep the existing await
semantics and variable names (no_overrides, storage.get_model, source_model,
target).
---
Outside diff comments:
In `@docs/interfaces/rest-api.md`:
- Around line 58-66: Update the response example to use the user-facing column
name `orders.count` instead of the internal enrichment alias `orders._count`:
replace all occurrences of `orders._count` in the example JSON `data` objects
and in the `columns` array so the docs show the correct user-visible output (the
internal `EnrichedMeasure.alias` remains `orders._count`, but the returned
payload should display `orders.count`).
---
Nitpick comments:
In `@CLAUDE.md`:
- Line 55: The phrase "write-if-changed semantics" is ambiguous; update the
sentence describing cache behavior to plain language that explains when the
cache is persisted: change the wording around how the cache refreshes on every
engine.execute path (real, dry-run, explain) to explicitly state that the engine
will recompute the model cache (columns + backing_query_sql) on each execution
and only persist/update the stored cache if the recomputed values differ from
the current cached values (no-op if unchanged). Ensure this clarification
mentions SlayerModel.query_variables, engine.create_model_from_query,
engine.save_model and engine.execute so readers can locate the related behavior.
- Line 56: Update the "Run-by-name execution" paragraph to define or reference
the terms "stage" and "outer query" used in variable precedence: either add a
short parenthetical after "stage" (e.g., "(a stage in a multi-stage query where
variables can be overridden)") and after "outer query" (e.g., "(the enclosing
query when executing nested or query-backed models)"), or append "See
query-backed models above for stage and outer query definitions" to the
sentence; ensure references to engine.execute, execute_sync, the variables=
kwarg, and SlayerQuery remain unchanged.
In `@slayer/mcp/server.py`:
- Around line 629-633: Update the bare set annotations to typed set hints for
clarity: change all_placeholders: set = set() and defaulted: set =
set(model.query_variables.keys()) to use set[str] (or Set[str]) so their element
type is explicit; if necessary import the appropriate typing names (e.g., Set or
use built-in generics) and also consider tightening stage_dicts: List[dict] to
List[dict[str, Any]] (with Any imported) for consistency with the typed
collections.
In `@tests/test_api_server.py`:
- Around line 229-240: The test uses synchronous loop calls; replace
asyncio.get_event_loop().run_until_complete(...) with direct awaits inside async
test functions (or async fixtures) so the calls to
storage.save_datasource(DatasourceConfig(...)) and
storage.save_model(SlayerModel(...)) become await storage.save_datasource(...)
and await storage.save_model(...); update the test to be declared async def
and/or move setup into an async fixture/helper that awaits creation of
Column(name="amount", sql="amount", type=DataType.NUMBER) and the
DatasourceConfig/SlayerModel saves to comply with pytest-asyncio
asyncio_mode="auto".
- Around line 331-375: The test function
test_post_query_run_by_name_dry_run_returns_sql_without_executing currently
lives under the TestOpenAPI400Documentation group but asserts dry-run behavior
for query-backed models; move this entire test function into the
TestQueryBackedModelsAPI test class in the same tests/test_api_server.py file
(or the appropriate query-backed models test class), keeping its body, imports
(DatasourceConfig, SlayerModel, Column, DataType, SlayerSQLClient) and async
storage setup unchanged, fix indentation to match the destination class, and
ensure any class-level fixtures (client, storage) are available there before
running the test suite.
In `@tests/test_query_backed_models.py`:
- Around line 37-49: Replace the helper function _engine_with_orders with an
async pytest fixture (e.g., engine_with_orders) that creates the
TemporaryDirectory, constructs YAMLStorage, saves _ds() and _orders_model(),
yields a SlayerQueryEngine(storage=storage) and then calls tmp.cleanup() after
yield; update tests to accept the fixture (engine_with_orders) instead of
calling _engine_with_orders and remove manual try/finally cleanup.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2353cf8c-c2dc-4ee8-a21d-92085dd4efeb
📒 Files selected for processing (23)
.claude/skills/slayer-models.md.claude/skills/slayer-overview.md.claude/skills/slayer-query.mdCLAUDE.mddocs/concepts/models.mddocs/concepts/queries.mddocs/interfaces/cli.mddocs/interfaces/mcp.mddocs/interfaces/rest-api.mddocs/reference/mcp.mdslayer/api/server.pyslayer/cli.pyslayer/core/models.pyslayer/core/query.pyslayer/engine/query_engine.pyslayer/mcp/server.pytests/integration/test_mcp_inspect.pytests/test_api_server.pytests/test_cli.pytests/test_mcp_server.pytests/test_migrations.pytests/test_models.pytests/test_query_backed_models.py
Real bugs: - api/server.py: add `whole_periods_only` to run-by-name disallowed guard so the flag isn't silently dropped (matches MCP behavior). - cli.py: handle JSON-list payloads in `slayer query` (the heuristic already accepts `[`, but the handler crashed on dict-key access); stamp dry_run/explain on the final stage and pass the list to `engine.execute_sync`. - cli.py: wrap `engine.save_model()` in try/ValueError so cache / source-mode validation failures surface as `Error: ...` + exit 1 rather than a traceback. Polish: - mcp/server.py: dedupe two consecutive `await storage.get_model(...)` calls. - slayer-query.md + CLAUDE.md: reword "Unknown kwarg variables are silently ignored" — runtime kwargs are merged in; extras simply remain unused. - docs/reference/mcp.md: reword the truncated-suffix code span to silence markdownlint MD038 while preserving accuracy. - skills/slayer-models.md: split the dangling `delete_measures=...` fragment into its own bullet. - docs/interfaces/mcp.md: rename "dimensions table" → "columns table" to match the renamed `## Columns` section / `sections=["columns"]` selector. Tests: added `test_post_query_run_by_name_rejects_whole_periods_only` in tests/test_api_server.py. Full suite: 1288 passed. Dismissed (with reply on the thread): CodeRabbit comment on CLAUDE.md:54-56 claiming PR scope mismatch — egor/S8 is a stack of S-commits (S2a/S2c/S3a/S8), so the query-backed docs are correct. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/interfaces/mcp.md`:
- Line 73: The markdown for the inspect_model parameter descriptions uses an
inline code span with leading/trailing spaces around the truncation marker
(currently shown as ` ... [truncated]`) which triggers MD038; update the
description for descriptions_max_chars in the inspect_model docs to use a code
span without surrounding spaces (e.g., wrap the exact truncation string without
leading/trailing spaces inside backticks) so the marker is rendered as a single
compact code token; ensure any mentions in the JSON/markdown variants and the
footer wording use the same spacing-corrected code span.
In `@tests/test_api_server.py`:
- Around line 360-367: The test currently issues client.post("/models",
json=...) without verifying the response; update the test to capture the
response from client.post (the call that creates the setup model) and assert its
success (e.g., check response.status_code is 200/201 or use
response.raise_for_status equivalent) before proceeding to the dry-run
assertions so failures in model creation are reported immediately; look for the
client.post("/models", json=...) call in tests/test_api_server.py and add the
response variable and an assert on its status/result.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ee1cabd-97c2-462c-ada3-458a88ef7547
📒 Files selected for processing (9)
.claude/skills/slayer-models.md.claude/skills/slayer-query.mdCLAUDE.mddocs/interfaces/mcp.mddocs/reference/mcp.mdslayer/api/server.pyslayer/cli.pyslayer/mcp/server.pytests/test_api_server.py
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- .claude/skills/slayer-models.md
🚧 Files skipped from review as they are similar to previous changes (5)
- .claude/skills/slayer-query.md
- slayer/cli.py
- slayer/mcp/server.py
- docs/reference/mcp.md
- slayer/api/server.py
CodeRabbit follow-ups (auto-flagged after the previous fix commit): - docs/interfaces/mcp.md: same MD038 reword applied to docs/reference/mcp.md last commit; the truncation-marker span exists here too. - tests/test_api_server.py: capture the setup `POST /models` response in the dry-run test and assert status 200 — surfaces setup failures immediately instead of hiding them behind the dry-run assertions. SonarQube S3776 (Cognitive Complexity): two functions flagged in this PR's leak window. Both refactors would either hide a single conceptual unit or churn a pre-existing helper. Suppressing with a justification comment, matching the existing NOSONAR pattern in the same files (query_engine.py L201, L969): - query_engine.py `_enrich`: closure-heavy join-target resolution. - cli.py `_run_datasources_create_demo`: linear demo-bootstrap UX. Quality Gate already passes; this just clears the two open issues. ruff clean, 1288 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@slayer/cli.py`:
- Around line 484-489: The call to engine.execute_sync uses query_input
positionally while other args are keywords, violating the rule to use keyword
arguments for multi-parameter calls; update the call to pass the first argument
as query=query_input and keep the rest as keywords (variables=runtime_kwarg,
dry_run=bool(args.dry_run), explain=bool(args.explain)) so engine.execute_sync
is invoked entirely with keyword arguments.
- Line 402: There is a function-local import "import json as _json" that
violates the top-level import rule; remove this local import and add "import
json as _json" (or plain "import json") at the module top-level with the other
imports, then update any references that use _json in the function to use the
top-level name to avoid shadowing or repetition (search for occurrences of
"_json" in slayer/cli.py and ensure they resolve to the new module-level
import).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9329f60d-8a75-4af3-a833-0f29eb1e2296
📒 Files selected for processing (4)
docs/interfaces/mcp.mdslayer/cli.pyslayer/engine/query_engine.pytests/test_api_server.py
✅ Files skipped from review due to trivial changes (1)
- docs/interfaces/mcp.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_api_server.py
- slayer/engine/query_engine.py
Both flagged in the review pass after the previous fix commit; both are repo-wide coding rules that the file pre-existed in violation of but were only re-evaluated when I touched it. - Move ``json`` to module top-level (CLAUDE.md: imports at top of file). Drops two function-local ``import json`` / ``import json as _json`` blocks in ``_parse_cli_variables`` and ``_run_query`` and renames the local ``_json`` references back to ``json``. Stdlib, near-zero import cost — no startup-perf reason to defer it. Yaml imports left function-local (heavier loader, deferral is defensible). - Pass ``query=query_input`` keyword in ``engine.execute_sync(...)`` for the run-by-name CLI path (CLAUDE.md: kwargs for >1 param). ruff clean, 1288 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
slayer/cli.py (1)
459-461: 💤 Low valueInconsistent handling of empty
runtime_kwargbetween code paths.The JSON paths use
variables=runtime_kwarg or None(lines 460, 472), converting an empty dict toNone. The run-by-name path usesvariables=runtime_kwarg(line 483), passing an empty dict as-is. While the engine likely handles both identically, this inconsistency could cause subtle behavioral differences and is confusing to maintain.Suggested fix for consistency
result = engine.execute_sync( query=query_input, - variables=runtime_kwarg, + variables=runtime_kwarg or None, dry_run=bool(args.dry_run), explain=bool(args.explain), )Also applies to: 471-473, 481-486
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@slayer/cli.py` around lines 459 - 461, The three engine.execute_sync calls are inconsistent in how they pass variables (some use variables=runtime_kwarg or None, one uses variables=runtime_kwarg); make them consistent by normalizing runtime_kwarg before the call (e.g., set vars_arg = runtime_kwarg or None) and use variables=vars_arg in all code paths (the execute_sync invocations in the JSON-path flow and the run-by-name flow) so empty dicts are handled the same everywhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@slayer/cli.py`:
- Around line 438-441: Wrap the file-open/read block that handles query_input
starting with "@" in a try/except that catches FileNotFoundError and other
OSError/IOError, only set query_input and is_json=True after a successful read,
and convert the exception into the CLI's standard error output by raising a
click.ClickException (or the same CLI error helper used elsewhere in this
module) with a clear message like "Could not read file <path>: <error>" so the
error appears as "Error: ..." instead of a traceback; reference the existing
query_input variable and is_json flag when making the change.
---
Nitpick comments:
In `@slayer/cli.py`:
- Around line 459-461: The three engine.execute_sync calls are inconsistent in
how they pass variables (some use variables=runtime_kwarg or None, one uses
variables=runtime_kwarg); make them consistent by normalizing runtime_kwarg
before the call (e.g., set vars_arg = runtime_kwarg or None) and use
variables=vars_arg in all code paths (the execute_sync invocations in the
JSON-path flow and the run-by-name flow) so empty dicts are handled the same
everywhere.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 15fb92f0-92b2-41f8-b812-994b1a7248f4
📒 Files selected for processing (1)
slayer/cli.py
# Conflicts: # slayer/engine/query_engine.py # tests/test_api_server.py
``ingest_datasource_models`` post-success message accessed ``m.dimensions`` on the freshly ingested ``SlayerModel``. v2 unified ``dimensions`` and ``measures`` into a single ``columns`` list, so this would AttributeError on every successful ingest call through MCP — silently broken for ~6 weeks (since v2). Surfaced by the Codex review pass after merging origin/main. - slayer/mcp/server.py: ``len(m.dimensions)`` → ``len(m.columns)`` in the ingest tool's success-line template; wording updated to ``"X columns"``. - tests/integration/test_mcp_inspect.py: new integration test ``TestIngestDatasourceModelsTool`` exercises the tool against a real SQLite fixture and pins the ``"columns"`` wording so any future regression to ``"dims"`` or to ``model.dimensions`` is caught. ruff clean, 1341 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four findings from the Codex review pass after merging origin/main — all in slayer/mcp/server.py around the new section-budgeting surface. ARCHITECTURE - _resolve_inspect_sections: an all-unknown sections= list (e.g. the typo sections=["sample"]) used to fall back to all six sections, silently triggering the full expensive payload on caller error. Now resolves to no sections instead, so the caller gets header + names-only collapses + a footer warning telling them what they have to fix. "All sections" is reserved for the explicit None / [] forms. BUGS - reachable_fields_depth: validate at the tool boundary as 0 <= depth <= 20. Previously unbounded, defeating the section-budgeting goal — a depth of thousands could enumerate vast join paths and hammer storage on dense graphs. - descriptions_max_chars: validate >= 0. Negatives quietly produced "all but last N chars + truncation marker" via Python negative slicing. - _render_inspect_footer: render unknown section names via repr() so caller-supplied values containing newlines or "> " prefix can't forge additional footer lines in the MCP response. Tests added in tests/test_mcp_server.py: rejection paths for both new validations (raises mcp ToolError with the expected message), helper-level test pinning the all-unknown-resolves-to-empty contract, and a control-char sanitisation test on the footer. Existing test_all_unknown_falls_back_to_default rewritten in place to exercise the new contract end-to-end. Docs updated: docs/reference/mcp.md and docs/interfaces/mcp.md inspect_model rows now state the new bounds and the all-unknown-resolves-to-no-sections rule. ruff clean, 1345 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three low-severity findings — one CLI UX bug, two cosmetic
consistency nits.
- slayer/cli.py: ``slayer query @<path>`` wrapped ``open()`` with no
exception handling, so a missing or unreadable file dumped a
Python traceback instead of the CLI's standard ``Error: ...`` +
exit-1 style used by every other path in the file. Wrap in
try/except FileNotFoundError + OSError and raise SystemExit.
- slayer/cli.py: run-by-name execute_sync passed ``variables=
runtime_kwarg`` while the JSON paths passed ``variables=
runtime_kwarg or None``. Engine treats {} and None identically
but the inconsistency is a maintenance smell. Standardised on
the ``or None`` form for all three callsites.
- CLAUDE.md: replaced "with write-if-changed semantics" jargon
with the more direct "updating storage only when values have
changed" phrasing in the Query-backed-models bullet.
Tests: ``TestRunQueryFileLoading`` in tests/test_cli.py covers
both error branches (FileNotFoundError → ``Query file not found``,
OSError via passing a directory → ``Error reading query file``).
ruff clean, 1347 tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This was referenced May 4, 2026
6 tasks
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
1 participant
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.
Summary
inspect_modelnow accepts three new params so an agent can drill into a model without paying for the full envelope every call.sections: Optional[List[str]] = None— subset of{columns, measures, aggregations, joins, reachable_fields, samples}. Default = all six.descriptions_max_chars: Optional[int] = None— trim long descriptions with a literal... [truncated]marker.reachable_fields_depth: int = 5— exposes the previously-hardcoded BFS-depth constant.Default behaviour is unchanged: an unmodified
inspect_model(model_name="orders")produces the same output as before, including no truncation footer.Design choices
sectionsenum is six names, not fiveThe issue listed five (
columns,measures,joins,reachable_fields,samples);aggregationsis added so an agent can skip a long custom-aggregations table while keeping the rest.Always-on parts (not gated by
sections)Header, metadata bullets (
data_source,sql_table,row_count, …), custom-SQL block (gated byshow_sql), model-level filters block (gated byshow_sql), backing-query structure for query-backed models, and the cached backing-query SQL (gated byshow_sql). Backing-query structure is identity-bearing for query-backed models — the equivalent ofsql_tablefor table-backed models — so it stays unconditional.aggregationsis fully gatedsections: full table; withshow_sql=Falsetheformulacolumn and thesqlfield of eachparams[]entry are dropped (only the SQL definition is gated byshow_sql, not the section itself).sections: collapses to a one-line backticked CSV of aggregation names, regardless ofshow_sql.Names-only fallback for
columns/measures/aggregations/joinsReuses
_markdown_table()'s single-column collapse: heading## <Section> (N — names only)followed by`id`, `amount`, …. Forjoinsthe names aretarget_modelstrings.reachable_fieldsandsamplesfully omit when not selectedThey have no natural "names" to list. The footer documents what was dropped so the agent knows it can re-call to fetch them.
Truncation footer is one global block
Markdown:
The footer is suppressed entirely when nothing was trimmed (default calls produce no footer). Unknown section names produce a leading
> Warning: ignored unknown sections: …line; the rest of the footer renders only if there's also trimming.Unknown section names: warn + proceed (no exception)
sections=[\"colums\"]emits a warning line and proceeds with the valid ones. If every entry is unknown, fall back to all six. Reasoning: an LLM that mistypes a section should get a useful response plus a correction signal, not a tool-call exception that may abort a longer plan.JSON parity
columns: [...], etc.).<section>_names: [...]array.omitted_sections,names_only_sections,unknown_sections. Default calls don't carry these arrays at all.descriptions_max_charsscopeOnly the four
descriptionfields (model, column, measure, aggregation). Labels, SQL, filters, and formulas are left alone — truncating SQL or formulas mid-token would mislead agents. Marker is literal... [truncated](leading space, three ASCII dots, space, square-bracketed token).0is allowed and yields just the marker for any non-empty description.reachable_fields_depthExposes the previously-hardcoded
max_depth=5in_collect_reachable_fieldsas a parameter (the issue said "already exists; reaffirm" but the param didn't actually exist on the MCP tool). No clamping;depth=0yields no reachable fields. The depth value is ignored whenreachable_fieldsisn't insections(BFS isn't run at all).Performance: skip DB calls for omitted sections
_collect_dim_profile+_collect_measure_profile: only run whencolumnsis insections(they feed thesampledcolumn).engine.get_column_types: only run whensamplesis insections(it feeds the sample-query construction).samplesis insections._collect_reachable_fields: only run whenreachable_fieldsis insections._get_row_count: stays unconditional (cheap, feeds always-on metadata bullets).num_rowskept, NOT renamedThe issue suggested
samples_max_rows: int = 5. We keptnum_rows: int = 3because (a) the issue also says "default behaviour unchanged" —3 → 5contradicts that; (b) the rename would break existing callers usingnum_rows=keyword. Naming inconsistency is preferable to a silent default change.No token-count footer
Issue lists it as optional. Skipped to avoid a tiktoken/Claude-SDK dependency and the "estimate vs. true count" ambiguity. Not a prerequisite for section budgeting.
Files changed
slayer/mcp/server.py— new constants/helpers (_truncate_description,_resolve_inspect_sections,_render_inspect_footer);inspect_modelsignature + body rewritten to gate per-section; JSON parity additions.tests/test_mcp_server.py— 35 new unit tests across 6 classes.tests/integration/test_mcp_inspect.py— 2 new end-to-end tests against the real SQLite fixture.docs/reference/mcp.md,docs/interfaces/mcp.md—inspect_modelrow updated.Test plan
poetry run ruff check slayer/ tests/— clean.poetry run pytest tests/test_mcp_server.py— 158 passed (123 original + 35 new).poetry run pytest tests/integration/test_mcp_inspect.py -m integration— 26 passed (24 original + 2 new).poetry run pytest --ignore=tests/integration/test_integration_postgres.py— 1287 passed.pg_ctl); CI should cover.slayer datasources create demo --ingest) verifying sections/footer behaviour end-to-end through MCP.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
inspect_modeltool now supports filtering sections (sectionsparameter) and truncating long descriptions (descriptions_max_charsparameter)Documentation