MotleyAI · ZmeiGorynych · Aug 16, 2026 · Aug 12, 2026 · Aug 12, 2026 · Aug 16, 2026
diff --git a/DECISIONS.md b/DECISIONS.md
@@ -73,3 +73,4 @@ implementation detail. Include issue refs when known.
 - 2026-08-06 — Dialect-aware identifier-length fitting (DEV-1756): every dialect declares a conservative universal budget as `SqlDialect.max_identifier_bytes` (postgres 63, mysql 64, redshift 127, oracle/tsql 128, snowflake 255, duckdb 256, bigquery 300, `None` = unbounded for sqlite/clickhouse/trino/presto/databricks/spark), and an over-limit identifier is shortened at emission to `<head>_<hash8>_<tail>` via `slayer/sql/dialects/_identifier_fit.py`. Postgres is the binding case and the reason this is a correctness fix rather than cosmetics: it truncates over-length identifiers **silently** (a NOTICE, never an error), so two 3-hop aliases sharing a 63-byte prefix either blow up as `AmbiguousColumnError` under the DEV-1444 outer wrap or — with no sibling to collide with — quietly return the column under a name the engine never looks up. Shorten **only when over the limit**, never uniformly: `decode_result_keys` restores the canonical dotted alias, so the dialect-dependence the issue worried about is invisible to consumers and the 99% case keeps byte-identical SQL (pinned by pre-change goldens, not merely by idempotence). Both ends of the alias are kept because the reported colliding pair differs ONLY in its final segment — a head-only truncation would render the two indistinguishable in `dry_run` output. The digest is sha256 of the FULL original, which is what makes `fit_identifier` a pure function of the name and lets the read side rebuild the emitted→canonical map by re-running it, with no map threaded through generation (the alternative the issue sketched). Write side is an EXACT-match replacement over the query's own alias set (`all_projection_aliases`, unfiltered — hidden ORDER-BY hoists and `_inner_*`/`_ft*`/`_ts*` entries are projected in the inner SELECT and truncate identically), never a length regex over arbitrary SQL, so a long quoted-looking span inside a string literal can never be corrupted; substitution is two-phase (canonical→sentinel→final) as defence-in-depth, though today's key set (over-limit) and value set (within-limit) are provably disjoint. BigQuery/T-SQL compose by running their existing dot-mangle regex AFTER the base length pass: an under-limit alias makes the length pass a genuine no-op so their output is unchanged, and an over-limit one arrives still-dotted and gets mangled by the same regex — no double-encoding — with the budget sized against `encode_alias` so the post-mangle form still fits. Scope is the three OUTPUT-name surfaces: projection aliases, CTE names (`_cte_name_from_alias` fits the whole result, prefix included, allocated through a per-statement collision-checked `SQLGenerator._cte_name`), and `_query_as_model` virtual-model shorts. Join-path TABLE aliases are deferred to DEV-1743, whose plan already owns the `__` path-alias allocator; their failure mode (silent wrong joins) is worse but the fix requires decoupling `EnrichedDimension.model_name` from the emitted qualifier across ~8 sites that split it back on `__`. Collisions raise `IdentifierCollisionError` rather than emitting ambiguous SQL — the check covers identity entries too, since an already-short alias equal to another's fitted form is a duplicate no hash width can prevent. Fixed alongside, in the same code path: `_query_as_model` no longer decides its short alias's quoting with a hand-written predicate. The contract is AGREEMENT between the emitted `AS <short>` and the downstream `Column(sql=short)` reference — they must resolve to the same column — so the short is now run through the same two mechanisms the reference side uses: `SQLGenerator._maybe_quote_ident` (DEV-1645: quote iff the name contains an uppercase letter) followed by `Identifier.sql(dialect=...)`, which lets sqlglot add quotes for words reserved IN THE TARGET DIALECT and for names that are not safe bare identifiers. Emitted bare, a mixed-case short was case-folded by Postgres while the outer stage referenced it quoted, making any query-backed model with a mixed-case join path unqueryable (`UndefinedColumnError`, reproduced on a live server). Two wrong answers were tried and rejected on the way: quoting UNCONDITIONALLY defines a case-sensitive `"status"` on upper-folding backends (Snowflake, Oracle) while the bare reference still resolves as `STATUS`, trading a Postgres bug for a Snowflake one; and quoting on `SLAYER_RESERVED_KEYWORDS` alone misses words reserved only NATIVELY (`index`, `int`, `rows` on MySQL; `rows` on BigQuery), where the reference side quotes but a bare `AS index` is a syntax error — `install_reserved_keywords` unions our set INTO each generator's, so only the generator knows the full set. Shape matters too: `Column.name` forbids only `.` and `:`, so `1abc` or `foo bar` needs quoting on form alone. Both axes must therefore be dialect-driven, and the rule is pinned by a 12-dialect × 7-name test asserting the two spellings are byte-identical. Because an all-lowercase short stays bare and still shares a case-folded namespace, the short-uniqueness check remains case-insensitive.
 - 2026-08-06 — `mcp` capped at `>=1.0,<2` (DEV-1757). mcp 2.0.0 renamed `mcp.server.fastmcp` → `mcp.server.mcpserver` (`FastMCP` → `MCPServer`), so the unbounded `mcp = ">=1.0"` let every lockfile-free install (`pip install motley-slayer`, `uv tool install`) resolve a major `slayer/mcp/server.py` cannot import — a broken MCP server, SLayer's primary agent-facing interface, on every fresh install. `poetry.lock` pinned a 1.x, so CI was green throughout and only users saw it; the guard added here (`tests/test_mcp_dependency_pin.py`) is therefore **declaration-level** — it parses the pyproject constraint and asserts 2.0.0 falls outside it, since no in-repo test can execute an mcp major the environment does not have. Two consequences accepted deliberately: users needing mcp≥2 for another package in the same environment now hit a resolver conflict instead of a runtime crash, and other core deps were left unbounded — capping the rest was considered and rejected (caps rot and produce unresolvable trees downstream), which is the scope of this change and NOT a standing no-caps policy. The import failure now diagnoses itself: absent package, wrong major, and a 1.x that failed to import for some other reason are three distinct messages, all offering `pip install 'mcp>=1.0,<2'` first and "upgrade SLayer" second — the old "Reinstall SLayer: pip install motley-slayer" text told users to do the one thing that reproduced the failure, and leading with the upgrade hint would recreate that for anyone already on the latest release. Separately, `serverInfo.version` now reports SLayer's own version: FastMCP 1.x exposes no `version` kwarg and never forwards one to the lowlevel `Server`, which falls back to `pkg_version("mcp")`, so SLayer was announcing the SDK's version as its own. The stamp writes the private `_mcp_server.version` (the only route in 1.x; the file already sets `_slayer_engine` on the same object per DEV-1656) and tolerates both a missing attribute and a read-only one, so a future SDK cannot abort server construction over a cosmetic field. Migrating to the 2.x `MCPServer` API — which would retire that private write via its public `version=` kwarg, and also pulls in `Context` injection, worker-thread sync handlers, snake_case `mcp.types`, and an httpx→httpx2 / pydantic≥2.12 / opentelemetry dependency shift — is deferred, as is removing the deprecated `inspect_model` tool.
 - 2026-08-06 — Recognised ELT/migration housekeeping tables ingest hidden (DEV-1759). An unfiltered ingest modelled `_dlt_loads`/`_dlt_pipeline_state`/`_dlt_version` as first-class models; the model list is the menu handed to an agent over MCP, so junk entries burn tokens every session, invite an agent to aggregate or join them (agents lack the human instinct that a `_`-prefixed table is off-limits), and dump kilobytes of serialized state into context on one exploratory query. **Hidden, not skipped** — the model exists, stays queryable by name and remains a valid join target, so DEV-1741's no-silent-omissions principle holds and the deliberate use the issue names (`_dlt_loads` answering "when did this last load?") keeps working; all it loses is presence in `models_summary`, `models list`, the REST listing, semantic search, and the BI catalogs, each of which already gated on `model.hidden`. Rules live in a pure, DB-free `slayer/engine/internal_tables.py`; matching is case-insensitive (Liquibase upper-cases, EF Core writes `__EFMigrationsHistory`, Sequelize `SequelizeMeta`) and runs on the **live object name**, never the model name, since `_assign_model_names` collapses `__` runs and matching a derived string is matching something the database never had. PREFIX rules are admitted only where the namespace is reserved by contract in every warehouse the vendor loads into, so the rule needs no dialect: `_dlt_` and `_airbyte_` (which is also how `_airbyte_raw_*`'s arbitrary stream suffix is covered); everything else is an exact name. No `_fivetran_`/`_sdc_` prefix rule: both vendors' real surface is *columns* on real tables (`_fivetran_synced`, `_sdc_batched_at`), so a table-level prefix would match nothing and imply coverage we do not have — Fivetran's only destination-schema tables are the two audit ones, listed exactly. **No `sqlite_` prefix rule either**, for the same reason plus a worse one, and this took a second pass to see: SQLite genuinely reserves the namespace (`CREATE TABLE sqlite_foo` is a hard error), but the objects it reserves it FOR never reach the scan, because SQLAlchemy's SQLite inspector defaults to `sqlite_include_internal=False` and filters `sqlite_sequence`/`sqlite_stat1`…`sqlite_stat4` out of `get_table_names()`. So the rule was unreachable on the one dialect that justified it, and its only reachable effect was on a NON-SQLite datasource — where nothing reserves the prefix and `sqlite_backup` is an ordinary table — i.e. hiding real user data. Dialect-scoping it was considered and rejected: it would have left machinery gating a rule that then fires nowhere. Both halves of that argument are pinned by tests (the inspector really does drop a live `sqlite_sequence`; SQLite really does reject the `CREATE TABLE`), since deleting a rule on the strength of upstream behaviour is only safe while that behaviour holds. PostGIS and `pg_stat_statements` are deliberately excluded: bookkeeping, but not engine-reserved namespaces, and admitting them opens "which extensions count?" with no principled stopping point. One accepted risk remains: `schema_version` (legacy Flyway ≤4) is the most collision-prone exact entry — recoverable precisely because hiding is not omitting. **Creation-only**: `hidden`/`meta` are absent from `_additive_merge_existing`'s update dict, so an un-hide sticks forever and a pre-existing visible model is never retro-hidden; no migration heals old stores, because a "was this ever edited?" heuristic cannot tell a deliberate un-hide from a pristine model and would re-hide it on every run — the exact fight hidden-not-skipped exists to avoid. Flag is `--surface-internals`, not the issue's `--include-internals`, which would read as "add internals to the `--include` list" next to two flags that genuinely control inclusion; `--include _dlt_loads` therefore still hides it, keeping CLI and REST (which has no flags) identical. **Reporting is split by path, and this is the subtle part**: `IngestionScanReport.hidden_internals` is what the scan CONSTRUCTED (correct for `datasources create --ingest`, which never reads storage), while `IdempotentIngestResult.hidden_internals` is EFFECTIVE post-merge state, re-derived by re-reading each matched model and keeping only those actually hidden — looked up by `model_name`, not `table_name`, or every `__`-sanitized model silently vanishes from the report. Forwarding the scan's verdict lies in both directions: it reports a user-un-hidden model as hidden, and it prints nothing for a still-hidden model on the very run where `--surface-internals` was passed to see it. Candidates are re-derived from `scan.models` rather than reused from `scan.hidden_internals` for that second reason (the latter is empty under the flag by construction) and, being keyed on successfully-built models, exclude skipped objects for free. The `HiddenInternal` entry is appended only AFTER `_build_one_model` returns, so a name collision or a construction failure lands in `skipped` alone — never two contradictory verdicts on one table. Exit code is unchanged (hiding is the intended outcome; a skip is a capability failure with `--exclude` as its remedy) and REST never 422s on it, matching the `skipped` contract. `datasources create --ingest` switched from `ingest_datasource` to `ingest_datasource_report` so it stops being silent about both internals and skips; that forced `_print_ingest_drift_and_errors` to read every field through `getattr`, since an `IngestionScanReport` has no `to_delete` and no `errors` at all. **MCP was the last silent surface, and the one that mattered most**: `ingest_datasource_models` renders through its own `_render_ingest_result` rather than `_print_ingest_drift_and_errors`, and that renderer read only `additions`/`to_delete`/`errors` — so the agent-facing tool this feature exists for was the one place that never said what it hid (nor, since DEV-1741, what it skipped). Both sections were added there in the CLI's order. The subtle half is the EARLY RETURN, not the sections: a steady-state re-ingest produces no additions, no drift and no errors, so the "already in sync" branch would have swallowed both on every run after the first — the guard therefore has to test `skipped` and `hidden_internals` too, while still leaving the empty-schema probe reachable for a genuinely empty schema. The hint is `edit_model(name, hidden=false)` rather than `--surface-internals`, which is a CLI flag this tool does not accept; likewise the skip line names no `--exclude`. `surface_internals` is deliberately NOT exposed as an MCP tool argument: handing the agent a knob to un-hide the junk the feature exists to hide it from inverts the point, and `edit_model` already covers the deliberate single-model case. That un-hide hint is DATASOURCE-QUALIFIED on every surface (shared `_unhide_hint`), because internals are the one model class that collides across datasources *by construction* rather than by accident — `_dlt_loads` exists verbatim in every dlt-loaded database, so a bare `edit_model("_dlt_loads", hidden=false)` resolves by priority list and either raises `AmbiguousModelError` or silently un-hides the other pipeline's model, precisely in the multi-pipeline setup this feature targets. The parameter stays optional because neither result shape carries the datasource — it has to come from the caller, and all four in-tree ones pass it. Column-level internals (`_dlt_id`, `_dlt_load_id`, `_fivetran_synced`, `_sdc_*`) are deferred — `_fivetran_deleted` is a soft-delete flag an agent must SEE or it silently returns deleted rows, which needs its own design pass rather than a rider. Glob `--exclude`, internal schemas (`airbyte_internal`, `fivetran_metadata`), and Airbyte V2's `airbyte_internal.<ns>_raw__stream_<name>` naming are out of scope.
+- 2026-08-12 — Formula measure referencing a sibling saved measure now emits valid SQL regardless of measure order (DEV-1779). A saved formula (`habit_score = order_count / unique_customers`) inline-expands at parse time to leaf colon refs (`id:count / customer:count_distinct`), so when the formula measure is enriched BEFORE a referenced sibling, its expression SQL freezes the sibling's canonical alias (`orders.id_count`); the later direct selection of that sibling renames the base-CTE column to the declared name (`orders.order_count`) and the frozen reference dangled — invalid SQL on Postgres, silently-NULL on SQLite (double-quote-as-string-literal). The DEV-1444 provenance-merge only reconciled the forward order (sibling declared first). Fix makes the rename atomic via one `_repoint_alias(prev, new)` helper called at BOTH rename sites (local-agg and cross-model-intercept): it sweeps every `known_aliases` value, the `measure_canonical_key_to_alias` provenance index, and — the new part — the already-frozen carriers `EnrichedExpression.sql` (exact quoted-token replace; the closing quote makes `"orders.id_count"` never match `"orders.id_count_2"`) and `EnrichedTransform.measure_alias` (so `cumsum(order_count)` and `change_pct` desugaring follow the rename too). Quoted-token string replacement is SQL-token-blind but safe here because arithmetic expression SQL is compiler-produced and never embeds a single-quoted literal containing a double-quoted alias — same invariant `_resolve_sql` already relies on. Defense-in-depth: the SQL generator's CTE-layering loop previously emitted an unresolved expression (and silently DROPPED an unresolved self-join `time_shift`) when it stalled, so a regression of this class reached the DB as broken SQL; it now raises a precise `ValueError` naming the computed column / transform and the missing alias for expressions AND all transform types. `_deps_available` gates in-loop addition, so anything still pending is genuinely unresolved — no false-positive raise.