DEV-1450 principled redesign of syntax (WIP)

[ZmeiGorynych]

Summary

Work-in-progress on DEV-1450 — replacing today's tangled multi-path resolution with a typed pipeline of composable stages: raw → slack normalize → parse_expr → bind_expr → ProjectionPlanner → plan_query → PlannedQuery.

• 23 commits since main, broken into ~25 numbered stages.
• The typed pipeline is in place and dormant — generator + engine cutover not yet wired in.
• Per-stage rhythm: failing tests first → Codex review of tests → implement → unit suite + ruff → Codex review of impl → commit.

See the DEV-1450 progress checkpoints for stage-by-stage detail.

Test plan

• All non-integration tests green at each stage (currently 3500 passing).
• SQLite + DuckDB integration tests run by user.
• Generator slices (7b.7–7b.13) assert SQL parity against the legacy generator via a temporary parity adapter.
• Engine cutover (7b.15) adds DEV-1445 / DEV-1446 / DEV-1448 / DEV-1449 end-to-end acceptance tests.
• Bit-identical result keys (orders.revenue_sum, orders._count, orders.customers.regions.name, renamed orders.<user_name>).

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Typed multi-stage planning and projection pipeline with cross-model aggregate planning and deterministic filter routing
  • First-class time-dimension support including derived temporal expressions
  • Query transforms (cumsum, change, change_pct, rank, lag/lead) and automatic function-style → colon normalization
  • Structured normalization warnings surfaced in responses

• Bug Fixes

  • Parametric cross-model aggregate result-key collisions resolved
  • Nested transform/aggregate deduplication improved
  • Model-filter handling for derived SQL fixed; windowed filters rejected

• Documentation

  • Comprehensive architecture and pipeline docs added/updated

• Tests

  • Extensive new unit and end-to-end coverage across parsing, binding, planning, and execution

[linear]

DEV-1450

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a typed resolution pipeline: typed keys/phases, Mode‑B parser and Mode‑A SQL canonicalizer, measure expansion, binder, projection/planning (ValueRegistry, ProjectionPlanner), cross‑model planning, source‑bundle builder, normalization rules, engine wiring, response metadata, sync derived‑column expander, structured errors/warnings, docs, and extensive tests.

Changes

DEV-1450 Typed Pipeline and Planning

Layer / File(s)	Summary
Core types and scope slayer/core/keys.py, slayer/core/scope.py, slayer/core/refs.py	Adds ValueKey family with phases, TimeTruncKey, Aggregate/Transform/Arithmetic/Scalar keys, scalar normalization, StageColumn/StageSchema/ModelScope, and agg-kwarg canonicalization helpers.
Errors, warnings, model validators slayer/core/errors.py, slayer/core/warnings.py, slayer/core/models.py	Stable _format_error_message and many typed errors, NormalizationWarning/SlayerNormalizationWarning, UnreachableFilterDroppedWarning; removes multi-dot auto-rewrite from model validators and validates filters via sql predicate parsing.
Parsing (Mode‑B) and parsed-ref walker slayer/engine/syntax.py, tests/test_syntax.py	Introduces Python-AST-based Mode‑B parser, colon preprocessing, parse_filter_expr, and walk_parsed_refs for scope‑free reference extraction.
SQL canonicalization (Mode‑A) slayer/sql/sql_expr.py, tests/test_sql_expr.py	sqlglot wrapper producing SqlExprKey, dialect rewrites (SQLite json_extract, log10/log2), window detection and assert_no_window_in_filter.
Slack-normalization slayer/engine/normalization.py, slayer/core/warnings.py, tests/test_slack_normalization.py	FUNC_STYLE_AGG, MISPLACED_MEASURE, DOT_PATH_IN_SQL rules; normalize_query/normalize_model produce NormalizationResult and structured warnings surfaced via warnings and SlayerResponse.warnings.
Measure pre-expansion & schema-drift extraction slayer/engine/measure_expansion.py, slayer/engine/schema_drift.py, tests/test_model_measure_expansion.py	expand_model_measures with recursion/cycle limits and AST-based formula ref extraction replacing legacy parsing for schema drift.
Binding slayer/engine/binding.py, tests/test_binding.py	bind_expr/bind_filter/bind_time_dimension produce BoundExpr/BoundFilter, scope-dependent ref resolution (ModelScope vs StageSchema), phase computation, transform/agg binding and validations, window-in-filter rejection.
Planned shapes & projection slayer/engine/planned.py, slayer/engine/planning.py, tests/test_planned.py, tests/test_projection_planner.py	PlannedQuery/ValueSlot/FilterPhase shapes, ValueRegistry interning, desugar_change/desugar_change_pct, lower_sugar_transforms, ProjectionPlanner, hidden-slot materialization, and filter→slot mapping.
Cross-model planning slayer/engine/cross_model_planner.py, tests/test_cross_model_planner.py, tests/test_cross_model_planner_wiring.py	CrossModelPlanner protocol, IsolatedCteCrossModelPlanner with host-filter routing, shared-grain computation, CTE schema construction, and optional rerooting.
Source bundle & path resolution slayer/engine/source_bundle.py, slayer/engine/path_resolution.py, tests/test_source_bundle.py, tests/test_path_resolution.py	ResolvedSourceBundle builder, sibling/synthetic model wiring, transitive referenced_models collection, and async walk_join_chain with NoJoinError.
Engine wiring & response metadata slayer/engine/query_engine.py, slayer/engine/response_meta.py, tests/*	Integrates normalization into engine pipeline, propagates structured warnings, computes touched models, persists normalized models, builds response metadata/expected columns for rendered SQL.
SQL helpers & derived expansion slayer/engine/column_expansion.py, tests/test_column_expansion_sync.py	Adds synchronous expand_derived_refs_sync for derived Column.sql inlining and related sync utilities for derived-column expansion.
Docs & navigation docs/**, mkdocs.yml	New architecture docs (parsing, binding, planning, cross-model aggregates, slack-normalization, errors & warnings, orchestration, typed keys) and updated mkdocs nav.
Tests (comprehensive) tests/**	Large test surface covering keys, parser, binding, measure expansion, planning, cross-model planner wiring, normalization, response metadata, integration/acceptance tests for DEV-1445/1446/1448/1449/1450 behaviors.

Sequence Diagram(s)

sequenceDiagram
  rect rgba(102, 153, 255, 0.5)
  participant Client
  participant Engine
  end
  rect rgba(153, 204, 255, 0.5)
  participant Storage
  participant SourceBundle
  end
  rect rgba(204, 255, 204, 0.5)
  participant Parser
  participant Binder
  participant Planner
  end
  rect rgba(255, 204, 153, 0.5)
  participant SQLGen
  participant DB
  end

  Client->>Engine: execute(query)
  Engine->>Storage: build_resolved_source_bundle
  Storage-->>Engine: models
  Engine->>Parser: normalize + parse
  Parser-->>Engine: ParsedExpr
  Engine->>Binder: bind_expr/bind_filter/bind_time_dimension
  Binder-->>Engine: BoundExpr/BoundFilter
  Engine->>Planner: plan_query/plan_stages
  Planner-->>Engine: PlannedQuery
  Engine->>SQLGen: generate_from_planned
  SQLGen->>DB: run SQL
  DB-->>Engine: rows
  Engine-->>Client: SlayerResponse + warnings

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• MotleyAI/slayer#34: Variables-in-filters merging/apply helpers and related wiring.
• MotleyAI/slayer#89: Derived-column expansion behavior (sync/async expansion overlap).
• MotleyAI/slayer#123: Error vocabulary and cycle/alias-collision handling.

Suggested reviewers

• AivanF

Poem

A rabbit taps keys with gentle might,
Parsing colons into tidy light—
It binds and plans through day and night,
Warnings hop and errors write,
Thump—ship the pipeline into flight!

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch egor/dev-1450-principled-redesign-of-syntax

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (8)

tests/test_time_trunc_key.py (1)

109-112: ⚡ Quick win

Specify the exact exception type for frozen instance mutation.

The test uses pytest.raises(Exception) which is too broad. Specify the exact exception type that's raised when attempting to mutate a frozen Pydantic model or dataclass field (typically ValidationError for Pydantic v2 or FrozenInstanceError for frozen dataclasses).

♻️ Proposed fix to specify exception type

+from pydantic import ValidationError
+
 class TestTimeTruncKeyImmutability:
     def test_is_frozen(self) -> None:
         k = TimeTruncKey(column=ColumnKey(leaf="ordered_at"), granularity="month")
-        with pytest.raises(Exception):
+        with pytest.raises(ValidationError):
             k.granularity = "day"  # type: ignore[misc]

Note: Use ValidationError if TimeTruncKey is a Pydantic model, or dataclasses.FrozenInstanceError if it's a frozen dataclass.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_time_trunc_key.py` around lines 109 - 112, The test test_is_frozen
should assert the specific exception type instead of pytest.raises(Exception):
replace the broad Exception with the concrete exception thrown when mutating a
frozen TimeTruncKey (use pydantic.errors.ValidationError /
pydantic.ValidationError if TimeTruncKey is a Pydantic model, or
dataclasses.FrozenInstanceError if it is a frozen dataclass), update the test
import(s) accordingly, and keep the mutating line k.granularity = "day" and
references to TimeTruncKey and ColumnKey unchanged.

slayer/engine/path_resolution.py (1)

75-75: ⚡ Quick win

Prefer more specific type annotation.

The nq variable is assigned a dict value, so nq: dict[str, Any] would be clearer than nq: Any.

♻️ Suggested improvement

-    nq: Any = named_queries if named_queries is not None else {}
+    nq: dict[str, Any] = named_queries if named_queries is not None else {}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@slayer/engine/path_resolution.py` at line 75, The local variable `nq` is
annotated too broadly as `Any`; change its annotation to a more specific mapping
type (e.g., `dict[str, Any]`) so callers and linters know it holds a dict
produced from `named_queries`. Update the assignment `nq: Any = named_queries if
named_queries is not None else {}` to use `nq: dict[str, Any] = ...` (or use
`cast(dict[str, Any], named_queries)` if necessary) and ensure `typing` imports
include `Any`/`dict` typing forms compatible with the project's Python version.

tests/test_path_resolution.py (1)

27-34: 💤 Low value

Consider using keyword arguments consistently in test calls.

The _make_model helper has 2 parameters. Per coding guidelines, functions with more than 1 parameter should be called using keyword arguments. Several calls use positional arguments (e.g., lines 160, 175, 191).

Example:

# Current
return _make_model(model_name)

# Preferred per guideline
return _make_model(name=model_name)

As per coding guidelines: "Use keyword arguments for functions with more than 1 parameter"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_path_resolution.py` around lines 27 - 34, The test helper
_make_model accepts parameters (name, joins=None) but several test calls use
positional args; update all usages to use keyword arguments per guideline: call
_make_model(name=..., joins=...) instead of positional calls (e.g., replace
_make_model(model_name) with _make_model(name=model_name) and any two-argument
positional calls with explicit joins=...), locating usages by the function name
_make_model in the tests and editing each call.

tests/test_source_bundle.py (1)

20-29: 💤 Low value

Consider using keyword arguments consistently in test calls.

The _model helper has 2 parameters. Per coding guidelines, functions with more than 1 parameter should be called using keyword arguments for clarity. Multiple calls throughout this file use positional arguments (e.g., lines 39, 49, 50, 56, 64, 74, 83, etc.).

Example:

# Current
m = _model("orders")
m = _model("orders", ds="warehouse")

# Preferred per guideline
m = _model(name="orders")
m = _model(name="orders", ds="warehouse")

As per coding guidelines: "Use keyword arguments for functions with more than 1 parameter"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_source_bundle.py` around lines 20 - 29, The test helper
_model(name: str, ds: str = "prod") is being invoked with positional arguments
in multiple places; update all calls to use keyword arguments (e.g.,
_model(name="orders") or _model(name="orders", ds="warehouse")) so they follow
the guideline for functions with more than one parameter; search for usages of
_model in tests/test_source_bundle.py and replace positional invocations with
keyword form, leaving the SlayerModel construction (and related Column/DataType
references) unchanged.

tests/test_boundexpr_unification.py (1)

161-161: ⚡ Quick win

Move function-local imports to module scope.

Line 161 and Line 177 import inside test methods; this violates the repo rule to keep imports at the top of the file.

Suggested diff

 from slayer.core.enums import DataType
-from slayer.core.keys import AggregateKey, ColumnKey
+from slayer.core.keys import AggregateKey, ArithmeticKey, ColumnKey
 from slayer.core.models import Column, SlayerModel
 from slayer.core.query import SlayerQuery
 from slayer.engine.binding import BoundExpr as BinderBoundExpr
+from slayer.engine.binding import walk_value_keys
 from slayer.engine.planned import BoundExpr as PlannedBoundExpr
 from slayer.engine.source_bundle import ResolvedSourceBundle
 from slayer.engine.stage_planner import plan_query
@@
-        from slayer.core.keys import ArithmeticKey
         assert isinstance(fp.expression.value_key, ArithmeticKey)
@@
-        from slayer.engine.binding import walk_value_keys
         keys = list(walk_value_keys(fp.expression.value_key))

As per coding guidelines: "**/*.py: Place imports at the top of files".

Also applies to: 177-177

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_boundexpr_unification.py` at line 161, Move the function-local
imports to module scope: remove the in-test import of ArithmeticKey (from
slayer.core.keys) and any other imports currently performed inside test
functions and place them at the top of tests/test_boundexpr_unification.py;
update the module-level import section to include "from slayer.core.keys import
ArithmeticKey" (and any other symbols currently imported inside tests) so tests
reference those symbols without local imports.

tests/test_slack_normalization.py (2)

249-257: ⚡ Quick win

Avoid run_until_complete in pytest async suites.

The fixture manually drives the loop; this is fragile when a loop is already managed by pytest-asyncio. Prefer an async fixture and await the setup calls directly.

As per coding guidelines "**/tests/**/*.py: Tests must use pytest-asyncio with asyncio_mode = \"auto\" so test functions can be async def and await directly."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_slack_normalization.py` around lines 249 - 257, The test uses
asyncio.get_event_loop().run_until_complete to call storage.save_datasource and
storage.save_model, which conflicts with pytest-asyncio-managed loops; change
the fixture to be async (make the fixture function async def) and replace those
run_until_complete calls with direct awaits: await
storage.save_datasource(DatasourceConfig(...)) and await
storage.save_model(_orders()), ensuring the fixture yields/returns as before so
pytest-asyncio (asyncio_mode="auto") manages the event loop correctly.

---

225-229: ⚡ Quick win

Move test imports to module scope.

These in-function imports make the test module inconsistent with repo style and harder to scan. Please hoist them to the top-level import block.

As per coding guidelines "**/*.py: Place imports at the top of files."

Also applies to: 249-257, 263-271, 287-295, 314-318, 343-351

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_slack_normalization.py` around lines 225 - 229, Several tests in
test_slack_normalization.py contain in-function imports (e.g., DatasourceConfig,
SlayerQueryEngine, YAMLStorage, sqlite3) — hoist these imports to the
module-level import block at the top of the file, remove the now-redundant local
imports inside the test functions, and consolidate/organize them with the
existing top imports; repeat this change for the other in-function import sites
flagged (around the commented ranges) to comply with the project import style
and keep test-scoped symbols available to all functions.

tests/test_dot_path_in_sql.py (1)

474-474: ⚡ Quick win

Hoist local imports to the module import section.

These test-local imports should be moved to top-level to match project conventions.

As per coding guidelines "**/*.py: Place imports at the top of files."

Also applies to: 489-490

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_dot_path_in_sql.py` at line 474, The test contains local imports
(e.g., "from slayer.core.models import ModelMeasure" and the other imports
around the later test block) that should be hoisted to the module-level import
section; move these local imports to the top of the file with the other imports,
remove the in-function/local import statements, and run the tests to ensure no
circular import issues—if a circular dependency appears, refactor the import to
a lazy import helper or import only the needed attribute at top-level.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@slayer/engine/binding.py`:
- Around line 155-156: The call to _bind uses a positional first argument;
change it to use keyword arguments for all parameters (e.g.,
_bind(parsed=parsed, scope=scope, bundle=bundle, in_filter=False)) and update
every other _bind invocation in this module (including the ones around the other
mentioned call sites) to the same keyword style so it conforms to the repo rule;
keep the surrounding return (BoundExpr(value_key=...)) unchanged.

In `@slayer/engine/syntax.py`:
- Around line 180-188: The current check uses _OVER_RE.search(text) and flags
any "OVER(" even when inside string literals (e.g. "status == 'OVER('"); change
the check to ignore matches inside quotes by scanning text
character-by-character tracking single-quote, double-quote, and escape state and
only applying _OVER_RE when not inside a quoted string, then raise
IllegalWindowInFilterError(filter_expr=text, source=..., suggestion=...) as
before; update the code path that currently calls _OVER_RE.search(text) (the
block raising IllegalWindowInFilterError) so it uses the new quote-aware
detection routine on the variable text.
- Around line 409-412: The code silently ignores keyword unpacking (**kwargs)
because the comprehension in _convert_call filters out ast.keyword entries with
arg is None; update _convert_call to detect any kw in node.keywords with kw.arg
is None and raise a clear error (e.g., ValueError or a parsing-specific
exception) rejecting **kwargs in Mode-B call parsing instead of dropping them,
then proceed to build kwargs by converting remaining keywords via _convert;
reference node.keywords, _convert_call, and _convert when implementing the check
and error raise.

In `@tests/test_agg_registry.py`:
- Around line 62-64: Update the test calls in tests/test_agg_registry.py to use
keyword arguments instead of positional arguments for helper functions with more
than one parameter: change calls to collect_reachable_agg_names(...),
resolve_aggregation(...), merge_agg_params(...), and any other multi-parameter
helper invocations (including the occurrences at lines noted in the comment) so
that each argument is passed as param_name=value (e.g., m=...,
resolve_join_target=..., named_queries=...) rather than by position; ensure you
update all instances mentioned (around the ranges 62-64, 79-81, 99-101, 117,
177, 183, 230, 238, 247, 257, 264) and run tests to confirm no signature
mismatches.

In `@tests/test_binding.py`:
- Around line 145-147: Update all calls to bind_expr and bind_filter (and
similar multi-parameter helpers) to pass the parsed expression/result as a
keyword instead of a positional first argument; e.g., replace
bind_expr(parse_expr("amount"), scope=_scope(), bundle=_bundle()) with a keyword
form like bind_expr(parsed=parse_expr("amount"), scope=_scope(),
bundle=_bundle()) (and do the same for bind_filter), applying this change at
every location listed in the comment so all multi-parameter calls use keyword
arguments for clarity and to follow the repository standard.

In `@tests/test_stage_planner.py`:
- Around line 203-220: The test test_stages_in_dependency_order currently
constructs stage1 and stage2 but passes them to plan_stages in dependency order,
so it doesn't verify reordering; update the test to pass stages in reverse order
(pass stage2 then stage1) to force the planner to reorder, and add an assertion
that the planned result is topologically correct (e.g., check planned[0].name ==
"stage1" and planned[1].name == "stage2" or equivalent checks on the planned
stages returned by plan_stages) so the test fails if plan_stages does not
reorder dependencies; references: test_stages_in_dependency_order, plan_stages,
SlayerQuery, planned.

---

Nitpick comments:
In `@slayer/engine/path_resolution.py`:
- Line 75: The local variable `nq` is annotated too broadly as `Any`; change its
annotation to a more specific mapping type (e.g., `dict[str, Any]`) so callers
and linters know it holds a dict produced from `named_queries`. Update the
assignment `nq: Any = named_queries if named_queries is not None else {}` to use
`nq: dict[str, Any] = ...` (or use `cast(dict[str, Any], named_queries)` if
necessary) and ensure `typing` imports include `Any`/`dict` typing forms
compatible with the project's Python version.

In `@tests/test_boundexpr_unification.py`:
- Line 161: Move the function-local imports to module scope: remove the in-test
import of ArithmeticKey (from slayer.core.keys) and any other imports currently
performed inside test functions and place them at the top of
tests/test_boundexpr_unification.py; update the module-level import section to
include "from slayer.core.keys import ArithmeticKey" (and any other symbols
currently imported inside tests) so tests reference those symbols without local
imports.

In `@tests/test_dot_path_in_sql.py`:
- Line 474: The test contains local imports (e.g., "from slayer.core.models
import ModelMeasure" and the other imports around the later test block) that
should be hoisted to the module-level import section; move these local imports
to the top of the file with the other imports, remove the in-function/local
import statements, and run the tests to ensure no circular import issues—if a
circular dependency appears, refactor the import to a lazy import helper or
import only the needed attribute at top-level.

In `@tests/test_path_resolution.py`:
- Around line 27-34: The test helper _make_model accepts parameters (name,
joins=None) but several test calls use positional args; update all usages to use
keyword arguments per guideline: call _make_model(name=..., joins=...) instead
of positional calls (e.g., replace _make_model(model_name) with
_make_model(name=model_name) and any two-argument positional calls with explicit
joins=...), locating usages by the function name _make_model in the tests and
editing each call.

In `@tests/test_slack_normalization.py`:
- Around line 249-257: The test uses asyncio.get_event_loop().run_until_complete
to call storage.save_datasource and storage.save_model, which conflicts with
pytest-asyncio-managed loops; change the fixture to be async (make the fixture
function async def) and replace those run_until_complete calls with direct
awaits: await storage.save_datasource(DatasourceConfig(...)) and await
storage.save_model(_orders()), ensuring the fixture yields/returns as before so
pytest-asyncio (asyncio_mode="auto") manages the event loop correctly.
- Around line 225-229: Several tests in test_slack_normalization.py contain
in-function imports (e.g., DatasourceConfig, SlayerQueryEngine, YAMLStorage,
sqlite3) — hoist these imports to the module-level import block at the top of
the file, remove the now-redundant local imports inside the test functions, and
consolidate/organize them with the existing top imports; repeat this change for
the other in-function import sites flagged (around the commented ranges) to
comply with the project import style and keep test-scoped symbols available to
all functions.

In `@tests/test_source_bundle.py`:
- Around line 20-29: The test helper _model(name: str, ds: str = "prod") is
being invoked with positional arguments in multiple places; update all calls to
use keyword arguments (e.g., _model(name="orders") or _model(name="orders",
ds="warehouse")) so they follow the guideline for functions with more than one
parameter; search for usages of _model in tests/test_source_bundle.py and
replace positional invocations with keyword form, leaving the SlayerModel
construction (and related Column/DataType references) unchanged.

In `@tests/test_time_trunc_key.py`:
- Around line 109-112: The test test_is_frozen should assert the specific
exception type instead of pytest.raises(Exception): replace the broad Exception
with the concrete exception thrown when mutating a frozen TimeTruncKey (use
pydantic.errors.ValidationError / pydantic.ValidationError if TimeTruncKey is a
Pydantic model, or dataclasses.FrozenInstanceError if it is a frozen dataclass),
update the test import(s) accordingly, and keep the mutating line k.granularity
= "day" and references to TimeTruncKey and ColumnKey unchanged.

🪄 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: 2ce23cea-7e91-49fa-a808-715b179c12a0

📥 Commits

Reviewing files that changed from the base of the PR and between a8b8f2c and b65fab8.

📒 Files selected for processing (43)

• slayer/core/errors.py
• slayer/core/keys.py
• slayer/core/scope.py
• slayer/core/warnings.py
• slayer/engine/agg_registry.py
• slayer/engine/binding.py
• slayer/engine/cross_model_planner.py
• slayer/engine/measure_expansion.py
• slayer/engine/normalization.py
• slayer/engine/path_resolution.py
• slayer/engine/planned.py
• slayer/engine/planning.py
• slayer/engine/query_engine.py
• slayer/engine/source_bundle.py
• slayer/engine/stage_planner.py
• slayer/engine/syntax.py
• slayer/engine/variables.py
• slayer/sql/sql_expr.py
• tests/test_agg_registry.py
• tests/test_binding.py
• tests/test_boundexpr_unification.py
• tests/test_cross_model_planner.py
• tests/test_cross_model_planner_wiring.py
• tests/test_dot_path_in_sql.py
• tests/test_error_messages.py
• tests/test_keys.py
• tests/test_model_measure_expansion.py
• tests/test_path_resolution.py
• tests/test_planned.py
• tests/test_projection_planner.py
• tests/test_scope_schema.py
• tests/test_slack_normalization.py
• tests/test_source_bundle.py
• tests/test_sql_expr.py
• tests/test_stage_planner.py
• tests/test_syntax.py
• tests/test_time_dimensions_filters.py
• tests/test_time_dimensions_planner.py
• tests/test_time_trunc_key.py
• tests/test_transform_lowerer.py
• tests/test_transforms_planner.py
• tests/test_value_registry.py
• tests/test_variables_planner.py

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Convert multi-parameter helper calls to keyword arguments.

Several calls use positional arguments for functions with multiple parameters (for example collect_reachable_agg_names(...), resolve_aggregation(...), merge_agg_params(...)). Please convert these to keyword style throughout this test module.

As per coding guidelines: **/*.py: Use keyword arguments for functions with more than 1 parameter.

Also applies to: 79-81, 99-101, 117-117, 177-177, 183-183, 230-230, 238-238, 247-247, 257-257, 264-264

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_agg_registry.py` around lines 62 - 64, Update the test calls in
tests/test_agg_registry.py to use keyword arguments instead of positional
arguments for helper functions with more than one parameter: change calls to
collect_reachable_agg_names(...), resolve_aggregation(...),
merge_agg_params(...), and any other multi-parameter helper invocations
(including the occurrences at lines noted in the comment) so that each argument
is passed as param_name=value (e.g., m=..., resolve_join_target=...,
named_queries=...) rather than by position; ensure you update all instances
mentioned (around the ranges 62-64, 79-81, 99-101, 117, 177, 183, 230, 238, 247,
257, 264) and run tests to confirm no signature mismatches.

[coderabbitai]

Actionable comments posted: 12

🧹 Nitpick comments (1)

tests/test_dot_path_in_sql.py (1)

478-478: ⚡ Quick win

Move function-local imports to module scope.

These imports should be at the top of the file to match repository rules and keep dependency visibility consistent.

Proposed fix

 from slayer.core.enums import DataType
-from slayer.core.models import Column, ModelJoin, SlayerModel
+from slayer.core.models import Column, ModelJoin, ModelMeasure, SlayerModel
+from slayer.core.query import SlayerQuery
 from slayer.core.warnings import SlayerNormalizationWarning
 from slayer.engine.normalization import (
     _apply_dot_path_in_sql,
     normalize_model,
+    normalize_query,
 )
@@
     def test_model_measure_formula_not_rewritten(self):
@@
-        from slayer.core.models import ModelMeasure
         m = _orders_with_customers_join()
@@
     def test_query_filters_mode_b_not_rewritten(self):
@@
-        from slayer.core.query import SlayerQuery
-        from slayer.engine.normalization import normalize_query
-
         m = _orders_with_customers_join()

As per coding guidelines, "Place imports at the top of files".

Also applies to: 493-494

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_dot_path_in_sql.py` at line 478, Move the function-local imports
into module scope: locate the local import statements (e.g., "from
slayer.core.models import ModelMeasure" and the two other imports referenced
near the same area) and relocate them to the top of
tests/test_dot_path_in_sql.py alongside the other imports; ensure you remove the
duplicate local import lines inside functions and run tests to confirm no
circular import issues, adjusting import order if needed to satisfy linting and
repository import ordering rules.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/architecture/errors-and-warnings.md`:
- Around line 14-19: The fenced code block containing the example error snippet
is missing a language tag which triggers markdownlint MD040; update the block
fence so it includes a language identifier (e.g., "text") directly after the
opening backticks for the existing example error block (the block that starts
with "<ErrorName>: <one-line summary>") so the code fence becomes a typed block
and satisfies the lint rule.

In `@slayer/core/refs.py`:
- Around line 116-181: The agg_kwarg_canonical_str helper currently handles
ColumnKey but not ColumnSqlKey, so derived-column params emitted by
_bind_agg_arg raise TypeError; add a branch in agg_kwarg_canonical_str that
checks isinstance(value, ColumnSqlKey) and returns the SQL identifier/text for
that key (the same string form used when rendering ColumnKey, e.g. value.sql or
the attribute your ColumnSqlKey exposes for its SQL text), mirroring the
existing ColumnKey logic so positional/kwarg derived-column aggregates render
without error.

In `@slayer/engine/binding.py`:
- Around line 638-696: _resolve_dotted_star currently walks hop_path without
checking for cycles, allowing cyclic joins like a.b.a.* to succeed; update
_resolve_dotted_star to mirror _resolve_dotted's cycle guard by tracking visited
model names (starting from host) as you iterate hops and raising
UnknownReferenceError when you encounter a model already seen (or otherwise
detect a cycle) before calling bundle.get_referenced_model; reference symbols to
change: _resolve_dotted_star, hop_path/current/host, current.joins,
bundle.get_referenced_model, and raise UnknownReferenceError with the same kind
of scope_summary/suggestion used elsewhere when a cycle is detected.

In `@slayer/engine/cross_model_planner.py`:
- Around line 650-659: _local_agg_formula currently flattens column-valued
args/kwargs into bare leaves by calling _scalar_formula_literal for positional
args and emitting v.leaf / v.column_name for keyword args, which loses path info
during rerooting; change the loops that build parts so that when an arg or kwarg
value is a ColumnKey or ColumnSqlKey you preserve its full path-shaped
representation (i.e., treat them the same way _aggregate_alias expects
ColumnKey-shaped values) instead of converting to scalar/leaf—either call the
existing path-aware serializer used by _aggregate_alias or keep the
ColumnKey/ColumnSqlKey structure and emit a path-aware token, and only fall back
to _scalar_formula_literal for truly scalar values.

In `@slayer/engine/query_engine.py`:
- Around line 783-799: The current model resolution uses bundle.source_model for
inline/non-string query.source_model and falls back to root model for named
stages; change it to resolve a stage's own model from bundle.stage_source_models
when query.name is present so normalization (normalize_query) and
custom-aggregation logic use the stage-local model. Specifically, when resolving
model for a query (symbols: query.source_model, query.name,
bundle.get_referenced_model, bundle.source_model) prefer
bundle.stage_source_models.get(query.name) if query.name exists and that entry
is present, then compute custom_aggs and call
normalize_query(model=that_model,...); keep existing fallback to
bundle.get_referenced_model or bundle.source_model only if the stage-specific
entry is absent.

In `@slayer/engine/response_meta.py`:
- Around line 182-197: The aggregate-handling code in
_measure_format/_measure_label uses _model_for_path(bundle, path) with src.path
which ignores ColumnSqlKey.model; update the logic that handles AggregateKey
(and the corresponding block at lines ~215-227) to prefer using src.model when
present (i.e., if getattr(src, "model", None) is not None use that model
directly) instead of resolving via _model_for_path(src.path), and only fall back
to _model_for_path when src.model is missing; keep the existing behavior for
StarKey and continue to call _infer_aggregated_format(model=...,
measure_name=..., aggregation=key.agg) once you have the correct model.

In `@slayer/engine/schema_drift.py`:
- Around line 1293-1297: The loop over model.measures is calling
_measure_formula_refs(measure.formula) which only rewrites builtin
function-style aggs and thus misses persisted/custom aggregations; update the
call to pass the model (or its custom aggregations set) into the normalization
so persisted measures like weighted_avg(amount, weight=qty) are recognized —
e.g., call _measure_formula_refs(measure.formula, model=model) or
_measure_formula_refs(measure.formula, custom_aggs=model.custom_aggregations)
and adjust _measure_formula_refs signature/logic accordingly so it consults
model-defined aggregations before returning refs used by _first_dropped_cause.

In `@slayer/engine/source_bundle.py`:
- Around line 257-272: The current code merges all variable scopes into a single
query_variables dict via merge_query_variables and returns it on
ResolvedSourceBundle, which causes sibling stages to lose their own
runtime>stage>outer>model precedence; instead, compute and store merged
variables per-stage (e.g., produce a mapping like stage_query_variables:
Dict[stage_name, Dict[str, Any>] or attach the merged vars to each entry in
stage_source_models) using merge_query_variables for each stage (referencing
merge_query_variables, query.variables, source_model.query_variables, and
stage_source_models), and return that per-stage mapping on ResolvedSourceBundle
rather than collapsing everything into a single root-scoped query_variables dict
so named stages continue to resolve {variable} placeholders with their own
precedence.

In `@slayer/engine/stage_planner.py`:
- Around line 943-950: The current return builds a public projection name by
passing agg_kwargs into canonical_agg_name, which changes exported result.data
keys; revert the public alias to the original stable form by calling
canonical_agg_name without the agg_kwargs/agg_args suffix (use measure_name,
aggregation_name=key.agg and only include agg_args if necessary for public
stability), and move the kwarg-uniquing logic (agg_kwarg_canonical_str(...)
values) into the internal/render alias used for CTEs (create or reuse an
internal alias variable for canonical_agg_name(..., agg_kwargs=...) and use that
for collision avoidance in internal SQL generation while keeping the
returned/exported name unchanged).
- Around line 362-371: The fallback currently parses only col_name which strips
table/qualifier information and causes rebinding errors; change the else-branch
to parse and bind the ORDER object's original reference (the full reference
string stored on o, e.g. the attribute that holds the original column reference
rather than the leaf name) instead of col_name — i.e. call parse_expr(...) with
the ORDER object's full reference (the original reference attribute on o) and
then bind_expr(scope=scope, bundle=bundle) so the ORDER keeps its
table/qualifier context; keep using parse_expr, bind_expr, scope and bundle as
in the surrounding code and only replace col_name with o's original reference
attribute.

In `@tests/test_generator2_multistage.py`:
- Line 257: In the test functions where you unpack harness into engine, storage,
db_path (e.g., the statement "engine, storage, db_path = harness"), replace the
unused binding "engine" with "_" so it becomes "_, storage, db_path = harness"
to silence static-analysis warnings; do the same in the other occurrence
mentioned (the second unpack around the later test), leaving "storage" and
"db_path" unchanged.
- Around line 108-115: The helper _run_sqlite should be changed to require
keyword-only arguments to follow call-style rules: update its signature to place
a bare * before parameters (e.g., def _run_sqlite(*, db_path: str, sql: str) ->
List[dict]) so callers must use explicit names, then update all tests and call
sites that invoke _run_sqlite positionally to call it with named arguments
(db_path=..., sql=...). Ensure references to _run_sqlite in test files are
adjusted accordingly.

---

Nitpick comments:
In `@tests/test_dot_path_in_sql.py`:
- Line 478: Move the function-local imports into module scope: locate the local
import statements (e.g., "from slayer.core.models import ModelMeasure" and the
two other imports referenced near the same area) and relocate them to the top of
tests/test_dot_path_in_sql.py alongside the other imports; ensure you remove the
duplicate local import lines inside functions and run tests to confirm no
circular import issues, adjusting import order if needed to satisfy linting and
repository import ordering rules.

🪄 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: ea473c17-b9ba-43a4-959a-bacb0dbb291b

📥 Commits

Reviewing files that changed from the base of the PR and between 6ca3cb4 and 7a2824a.

📒 Files selected for processing (52)

• CLAUDE.md
• docs/architecture/binding.md
• docs/architecture/cross-model-aggregates.md
• docs/architecture/engine-orchestration.md
• docs/architecture/errors-and-warnings.md
• docs/architecture/index.md
• docs/architecture/parsing.md
• docs/architecture/planning.md
• docs/architecture/scopes-and-bundle.md
• docs/architecture/slack-normalization.md
• docs/architecture/sql-generation.md
• docs/architecture/stage-planning.md
• docs/architecture/typed-keys.md
• docs/concepts/queries.md
• mkdocs.yml
• slayer/core/errors.py
• slayer/core/keys.py
• slayer/core/models.py
• slayer/core/refs.py
• slayer/engine/binding.py
• slayer/engine/column_expansion.py
• slayer/engine/cross_model_planner.py
• slayer/engine/normalization.py
• slayer/engine/planned.py
• slayer/engine/planning.py
• slayer/engine/query_engine.py
• slayer/engine/response_meta.py
• slayer/engine/schema_drift.py
• slayer/engine/source_bundle.py
• slayer/engine/stage_planner.py
• slayer/engine/syntax.py
• slayer/memories/resolver.py
• slayer/sql/generator.py
• tests/test_column_expansion_sync.py
• tests/test_cross_model_planner.py
• tests/test_dev1445_cross_model_alias_filter.py
• tests/test_dev1446_nested_transform_dedup.py
• tests/test_dev1448_named_join_measure_alias.py
• tests/test_dev1449_flat_stage_schema_refs.py
• tests/test_dev1450fix_cross_model_parametric_keys.py
• tests/test_dev1450fix_derived_time_dimension.py
• tests/test_dev1450fix_model_filter_derived.py
• tests/test_dev1450fix_reroot_strategy.py
• tests/test_dot_path_in_sql.py
• tests/test_generator2_multistage.py
• tests/test_memories_resolver_typed.py
• tests/test_models.py
• tests/test_response_meta.py
• tests/test_schema_drift_typed.py
• tests/test_source_bundle_builder.py
• tests/test_time_dimensions_planner.py
• tests/test_time_trunc_key.py

💤 Files with no reviewable changes (1)

• tests/test_models.py

✅ Files skipped from review due to trivial changes (5)

• docs/architecture/cross-model-aggregates.md
• CLAUDE.md
• docs/architecture/engine-orchestration.md
• docs/architecture/planning.md
• docs/architecture/index.md

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve column-valued aggregate parameters during rerooting.

_local_agg_formula() currently serializes positional column refs with _scalar_formula_literal() and collapses keyword column refs to bare leaf / column_name. For rerooted parametric cross-model aggregates, that loses the bound path information and can make the nested subquery bind the wrong column or fail to bind at all. _aggregate_alias() already treats aggregate args/kwargs as ColumnKey-shaped values, so rerooting needs the same path-aware handling here.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@slayer/engine/cross_model_planner.py` around lines 650 - 659,
_local_agg_formula currently flattens column-valued args/kwargs into bare leaves
by calling _scalar_formula_literal for positional args and emitting v.leaf /
v.column_name for keyword args, which loses path info during rerooting; change
the loops that build parts so that when an arg or kwarg value is a ColumnKey or
ColumnSqlKey you preserve its full path-shaped representation (i.e., treat them
the same way _aggregate_alias expects ColumnKey-shaped values) instead of
converting to scalar/leaf—either call the existing path-aware serializer used by
_aggregate_alias or keep the ColumnKey/ColumnSqlKey structure and emit a
path-aware token, and only fall back to _scalar_formula_literal for truly scalar
values.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Make _run_sqlite keyword-only to satisfy repo call-style rules.

This helper has 2 parameters and is used positionally in several tests; make it keyword-only and call it with explicit names.

♻️ Proposed fix

-def _run_sqlite(db_path: str, sql: str) -> List[dict]:
+def _run_sqlite(*, db_path: str, sql: str) -> List[dict]:
@@
-    new_rows = _run_sqlite(db_path, new_sql)
+    new_rows = _run_sqlite(db_path=db_path, sql=new_sql)
@@
-    new_rows = _run_sqlite(db_path, new_sql)
+    new_rows = _run_sqlite(db_path=db_path, sql=new_sql)
@@
-    new_rows = _run_sqlite(db_path, new_sql)
+    new_rows = _run_sqlite(db_path=db_path, sql=new_sql)
@@
-    new_rows = _run_sqlite(db_path, new_sql)
+    new_rows = _run_sqlite(db_path=db_path, sql=new_sql)
@@
-    new_rows = _run_sqlite(db_path, new_sql)
+    new_rows = _run_sqlite(db_path=db_path, sql=new_sql)
@@
-    new_rows = _run_sqlite(db_path, new_sql)
+    new_rows = _run_sqlite(db_path=db_path, sql=new_sql)

As per coding guidelines "Use keyword arguments for functions with more than 1 parameter".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_generator2_multistage.py` around lines 108 - 115, The helper
_run_sqlite should be changed to require keyword-only arguments to follow
call-style rules: update its signature to place a bare * before parameters
(e.g., def _run_sqlite(*, db_path: str, sql: str) -> List[dict]) so callers must
use explicit names, then update all tests and call sites that invoke _run_sqlite
positionally to call it with named arguments (db_path=..., sql=...). Ensure
references to _run_sqlite in test files are adjusted accordingly.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

slayer/engine/syntax.py (1)

189-191: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

The OVER( guard still misclassifies some valid Python strings.

_STRING_LITERAL_RE understands doubled-quote SQL literals, not Python backslash escapes, so inputs like status == "x\" OVER(" still leave OVER( visible and raise IllegalWindowInFilterError even though it is inside the string literal. This needs a quote-aware scanner/tokenizer that follows Python string rules instead of _STRING_LITERAL_RE.sub("", text).

#!/bin/bash
set -euo pipefail

python - <<'PY'
import re

OVER_RE = re.compile(r"\bOVER\s*\(", re.IGNORECASE)
STRING_LITERAL_RE = re.compile(r"'(?:[^']|'')*'|\"(?:[^\"]|\"\")*\"")

cases = [
    r'status == "OVER("',
    r'status == "x\" OVER("',
]

for text in cases:
    masked = STRING_LITERAL_RE.sub("", text)
    print(f"text   : {text}")
    print(f"masked : {masked}")
    print(f"match? : {bool(OVER_RE.search(masked))}")
    print("-" * 40)
PY

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@slayer/engine/syntax.py` around lines 189 - 191, The current guard uses
_STRING_LITERAL_RE to mask literals but fails for Python-style escapes; replace
that masking with a Python-aware tokenizer: use the tokenize module
(tokenize.generate_tokens / tokenize.tokenize) on the input text to produce a
new string where tokens of type STRING are replaced with equal-length whitespace
(so positions remain consistent), then run _OVER_RE.search on that
tokenized/masked string instead of _STRING_LITERAL_RE.sub("", text); update the
conditional using the new helper (e.g., _mask_python_strings(text) and call it
where _STRING_LITERAL_RE.sub(...) is used) and ensure imports for tokenize and
io are added and IllegalWindowInFilterError behavior remains unchanged.

slayer/engine/stage_planner.py (1)

368-374: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Flatten dotted ORDER refs before rebinding in stage scope.

This fallback still parses customers.region verbatim when scope is a StageSchema, but downstream stage columns are emitted as customers__region. Structured ORDER refs without raw_formula will still fail or rebind to the wrong slot in multi-stage queries.

Suggested fix

         else:
             bo = bind_expr(
-                parse_expr(full_name, allow_dunder=flat_scope),
+                parse_expr(
+                    _flatten_dotted(full_name) if flat_scope else full_name,
+                    allow_dunder=flat_scope,
+                ),
                 scope=scope, bundle=bundle,
             )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@slayer/engine/stage_planner.py` around lines 368 - 374, The bind is parsing
the dotted ORDER ref verbatim (parse_expr(full_name,...)) so when scope is a
StageSchema downstream columns are emitted flattened (customers__region) and the
current code rebinds incorrectly; fix by detecting StageSchema (or stage scope)
and transform dotted full_name into the stage-flattened form before calling
parse_expr/bind_expr (e.g. replace dots with the stage separator used by the
StageSchema) so bind_expr receives the flattened identifier (use the same
flattening logic/utility the StageSchema uses) while preserving
allow_dunder=flat_scope and passing scope and bundle to bind_expr.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@slayer/engine/schema_drift.py`:
- Around line 866-870: The comprehension that builds custom_agg_names scans all
models via graph.models_by_name but should only consider models reachable from
the stage source; change the comprehension to iterate over graph.reachable and
then look up models in graph.models_by_name (e.g. for name in graph.reachable
for m in (graph.models_by_name.get(name),) if present) so only reachable models'
aggregations (m.aggregations) contribute to custom_agg_names; update the
expression that defines custom_agg_names accordingly (refer to the existing
variable name custom_agg_names and usages in schema_drift.py).

---

Duplicate comments:
In `@slayer/engine/stage_planner.py`:
- Around line 368-374: The bind is parsing the dotted ORDER ref verbatim
(parse_expr(full_name,...)) so when scope is a StageSchema downstream columns
are emitted flattened (customers__region) and the current code rebinds
incorrectly; fix by detecting StageSchema (or stage scope) and transform dotted
full_name into the stage-flattened form before calling parse_expr/bind_expr
(e.g. replace dots with the stage separator used by the StageSchema) so
bind_expr receives the flattened identifier (use the same flattening
logic/utility the StageSchema uses) while preserving allow_dunder=flat_scope and
passing scope and bundle to bind_expr.

In `@slayer/engine/syntax.py`:
- Around line 189-191: The current guard uses _STRING_LITERAL_RE to mask
literals but fails for Python-style escapes; replace that masking with a
Python-aware tokenizer: use the tokenize module (tokenize.generate_tokens /
tokenize.tokenize) on the input text to produce a new string where tokens of
type STRING are replaced with equal-length whitespace (so positions remain
consistent), then run _OVER_RE.search on that tokenized/masked string instead of
_STRING_LITERAL_RE.sub("", text); update the conditional using the new helper
(e.g., _mask_python_strings(text) and call it where _STRING_LITERAL_RE.sub(...)
is used) and ensure imports for tokenize and io are added and
IllegalWindowInFilterError behavior remains unchanged.

🪄 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: 3ff7eb9f-2ec5-4535-92cf-1e1db5ba5448

📥 Commits

Reviewing files that changed from the base of the PR and between 7a2824a and 2d9729e.

📒 Files selected for processing (22)

• docs/architecture/errors-and-warnings.md
• slayer/core/keys.py
• slayer/core/refs.py
• slayer/engine/binding.py
• slayer/engine/cross_model_planner.py
• slayer/engine/normalization.py
• slayer/engine/planning.py
• slayer/engine/query_engine.py
• slayer/engine/response_meta.py
• slayer/engine/schema_drift.py
• slayer/engine/stage_planner.py
• slayer/engine/syntax.py
• slayer/memories/resolver.py
• slayer/sql/generator.py
• tests/test_agg_registry.py
• tests/test_dev1445_cross_model_alias_filter.py
• tests/test_dev1450fix_cross_model_derived.py
• tests/test_dev1450fix_derived_time_dimension.py
• tests/test_dev1450fix_group2_correctness.py
• tests/test_generator2_multistage.py
• tests/test_path_resolution.py
• tests/test_stage_planner.py

💤 Files with no reviewable changes (1)

• slayer/engine/planning.py

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@slayer/engine/syntax.py`:
- Around line 143-147: Replace the non-escape-aware _STRING_LITERAL_RE with the
new escape-aware _PY_STRING_LITERAL_RE in both _normalize_sql_filter_operators
and _preprocess_colons so rewrites (colon preprocessing and SQL operator
normalization in functions _preprocess_colons and
_normalize_sql_filter_operators) skip over Python-style string literals that
contain backslash-escaped quotes; update any local variable or parameter
references in those functions to use _PY_STRING_LITERAL_RE and run existing
literal-masking logic unchanged so behavior remains identical except now
honoring escaped quotes.

🪄 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: 3c882c1d-4806-4406-9510-0f60db0e0ca6

📥 Commits

Reviewing files that changed from the base of the PR and between 2d9729e and 9e7e59e.

📒 Files selected for processing (6)

• slayer/engine/cross_model_planner.py
• slayer/engine/schema_drift.py
• slayer/engine/syntax.py
• slayer/sql/generator.py
• tests/test_dev1450fix_cross_model_derived.py
• tests/test_dev1450fix_group2_correctness.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

python - <<'PY'
import re

text = r'label == "x \" revenue:sum AND foo"'

sql_style = re.compile(r"'(?:[^']|'')*'|\"(?:[^\"]|\"\")*\"")
py_style = re.compile(r"'(?:\\.|[^'\\])*'|\"(?:\\.|[^\"\\])*\"")

print("input:", text)
print("sql_style matches:", [m.group(0) for m in sql_style.finditer(text)])
print("py_style matches:", [m.group(0) for m in py_style.finditer(text)])
print("sql_style masked:", sql_style.sub("", text))
print("py_style masked:", py_style.sub("", text))
PY

Repository: MotleyAI/slayer

Length of output: 258

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

ls
fd -a "syntax.py" slayer | head
sed -n '1,240p' slayer/engine/syntax.py | nl -ba | sed -n '120,220p'
sed -n '240,340p' slayer/engine/syntax.py | nl -ba | sed -n '220,320p'
sed -n '500,620p' slayer/engine/syntax.py | nl -ba | sed -n '500,620p'

Repository: MotleyAI/slayer

Length of output: 298

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "_STRING_LITERAL_RE|_PY_STRING_LITERAL_RE" slayer/engine/syntax.py

Repository: MotleyAI/slayer

Length of output: 456

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

python -c "print('sanity')"

echo "== matcher definitions =="
rg -n "_PY_STRING_LITERAL_RE|_STRING_LITERAL_RE" slayer/engine/syntax.py

echo
echo "== _normalize_sql_filter_operators block =="
rg -n "def _normalize_sql_filter_operators" -n slayer/engine/syntax.py
start=$(rg -n "def _normalize_sql_filter_operators" slayer/engine/syntax.py | cut -d: -f1 | head -n1)
# print 80 lines around
python - <<PY
import itertools,sys
path="slayer/engine/syntax.py"
start=int(open(path).read().splitlines()[:1] or 1)
PY

Repository: MotleyAI/slayer

Length of output: 777

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

path="slayer/engine/syntax.py"

echo "== matcher definitions / usages =="
rg -n "_PY_STRING_LITERAL_RE|_STRING_LITERAL_RE" "$path"

echo
echo "== _normalize_sql_filter_operators context =="
ln=$(rg -n "def _normalize_sql_filter_operators" "$path" | head -n1 | cut -d: -f1)
sed -n "$((ln-20)),$((ln+120))p" "$path" | nl -ba | sed -n "1,200p"

echo
echo "== _preprocess_colons context =="
ln2=$(rg -n "def _preprocess_colons" "$path" | head -n1 | cut -d: -f1)
sed -n "$((ln2-20)),$((ln2+160))p" "$path" | nl -ba | sed -n "1,240p"

Repository: MotleyAI/slayer

Length of output: 600

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

path="slayer/engine/syntax.py"

echo "== matcher definitions / usages =="
rg -n "_PY_STRING_LITERAL_RE|_STRING_LITERAL_RE" "$path"

echo
echo "== _normalize_sql_filter_operators context =="
ln=$(rg -n "def _normalize_sql_filter_operators" "$path" | head -n1 | cut -d: -f1)
sed -n "$((ln-20)),$((ln+140))p" "$path" | nl -ba | sed -n "1,220p"

echo
echo "== _preprocess_colons context =="
ln2=$(rg -n "def _preprocess_colons" "$path" | head -n1 | cut -d: -f1)
sed -n "$((ln2-20)),$((ln2+200))p" "$path" | nl -ba | sed -n "1,260p"

Repository: MotleyAI/slayer

Length of output: 600

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
path="slayer/engine/syntax.py"

echo "== _normalize_sql_filter_operators (lines ~230-290) =="
sed -n '220,320p' "$path" | cat -n

echo
echo "== _preprocess_colons (lines ~320-420) =="
sed -n '320,460p' "$path" | cat -n

Repository: MotleyAI/slayer

Length of output: 11587

---

Reuse the escape-aware _PY_STRING_LITERAL_RE in colon preprocessing and SQL-operator normalization

_normalize_sql_filter_operators() and _preprocess_colons() still use _STRING_LITERAL_RE, which is not escape-aware, so escaped quotes can cause :sum / AND / IN rewrites to occur inside Python string literals.

Suggested fix

 def _normalize_sql_filter_operators(text: str) -> str:
@@
-    parts = _STRING_LITERAL_RE.split(text)
-    literals = _STRING_LITERAL_RE.findall(text)
+    parts = _PY_STRING_LITERAL_RE.split(text)
+    literals = _PY_STRING_LITERAL_RE.findall(text)
@@
 def _preprocess_colons(
     text: str,
 ) -> Tuple[str, Dict[int, Tuple[Union[Ref, DottedRef, StarSource], str]]]:
@@
-    literal_spans = [
-        (m.start(), m.end()) for m in _STRING_LITERAL_RE.finditer(text)
-    ]
+    literal_spans = [
+        (m.start(), m.end()) for m in _PY_STRING_LITERAL_RE.finditer(text)
+    ]

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@slayer/engine/syntax.py` around lines 143 - 147, Replace the non-escape-aware
_STRING_LITERAL_RE with the new escape-aware _PY_STRING_LITERAL_RE in both
_normalize_sql_filter_operators and _preprocess_colons so rewrites (colon
preprocessing and SQL operator normalization in functions _preprocess_colons and
_normalize_sql_filter_operators) skip over Python-style string literals that
contain backslash-escaped quotes; update any local variable or parameter
references in those functions to use _PY_STRING_LITERAL_RE and run existing
literal-masking logic unchanged so behavior remains identical except now
honoring escaped quotes.

[sonarqubecloud]

Quality Gate passed

Issues
61 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.9% Duplication on New Code

See analysis details on SonarQube Cloud