fix: ingest resolves a schema scope and qualifies non-default schemas (DEV-1758)

.claude/skills/slayer-models.md

@@ -88,6 +88,15 @@ Model names cannot contain `__` (reserved for join-path aliases), but
 `sql_table` can. Ingestion sanitizes only the name: object
 `reports__patient__drug` → model `reports_patient_drug`, `sql_table` unchanged.
 
+`sql_table` is emitted verbatim into the generated SQL, so anything outside the
+connection's default schema **must** be schema-qualified (`analytics.orders`)
+or the query fails with table-not-found. Ingestion qualifies exactly when the
+object's schema differs from the default, or when the schema was named
+explicitly — default-schema objects stay bare, so both forms coexist in one
+datasource by design. The schema is everything before the final dot, so
+`project.dataset.table` works. A missing qualifier is repaired by re-ingesting
+that schema; an existing one is never rewritten.
+
 ## Query-backed models
 
 `create_model_from_query(query, name, variables=None)` saves a query (or list of stages) as a query-backed model. It populates `model.source_queries`, optional `model.query_variables` defaults, and caches `model.columns` + `model.backing_query_sql` from a save-time dry-run (unresolved `{var}` placeholders default to `'0'`).

.claude/skills/slayer-overview.md

@@ -12,7 +12,7 @@ SLayer is a lightweight, agent-first semantic layer. Instead of writing raw SQL,
 - **SQLGenerator** — takes an EnrichedQuery (not SlayerQuery) and converts it to SQL via sqlglot (dialect-aware: postgres, mysql, bigquery, etc.)
 - **SlayerSQLClient** — executes SQL via SQLAlchemy with retry logic and statement timeouts
 - **Storage** — YAML or SQLite backends for model and datasource configs
-- **Ingestion** — auto-generates models from DB schema with rollup-style FK joins (denormalized LEFT JOINs). It can be triggered manually (`slayer ingest`, `ingest_datasource_models`, `POST /ingest`) or **on every server boot** via `slayer serve --ingest-on-startup` / `slayer mcp --ingest-on-startup` (also `SLAYER_INGEST_ON_STARTUP=1`, or `create_app/create_mcp_server(ingest_on_startup=True)` programmatically). It is idempotent and continues on per-datasource failures.
+- **Ingestion** — auto-generates models from DB schema with rollup-style FK joins (denormalized LEFT JOINs). It can be triggered manually (`slayer ingest`, `ingest_datasource_models`, `POST /ingest`) or **on every server boot** via `slayer serve --ingest-on-startup` / `slayer mcp --ingest-on-startup` (also `SLAYER_INGEST_ON_STARTUP=1`, or `create_app/create_mcp_server(ingest_on_startup=True)` programmatically). It is idempotent and continues on per-datasource failures. One pass covers **one schema** — `--schema a,b` / `--all-schemas` (and the `schemas` / `all_schemas` equivalents on MCP, REST and the Python API) opt into more; precedence is explicit flag → `datasource.schema_name` → the connection default. Non-default-schema objects get a schema-qualified `sql_table`.
 - **Interfaces** — MCP server (stdio via `slayer mcp`, SSE via `slayer serve` at `/mcp/sse`), REST API (FastAPI on port 5143), Python SDK, and two read-only wire-protocol facades for BI tools: Arrow Flight SQL (`slayer flight-serve`, port 5144) and Postgres (`slayer pg-serve`, port 5145; the connection `database` selects the SLayer datasource)
 
 ## Key Models

DECISIONS.md

@@ -70,3 +70,4 @@ implementation detail. Include issue refs when known.
 - 2026-08-04 — Dialect-aware / complete escaping for Mode-A `{variable}` substitution (DEV-1727), hardening DEV-1625. `substitute_variables(..., escape="sql")` is now **dialect-aware** and **fail-closed**: it gained a required keyword-only `backslash_escapes` signal (`bool | None`, raises if `None` in sql mode) so a caller rendering raw SQL can never silently under-escape. On backslash-escaping dialects (MySQL/ClickHouse/Snowflake/Redshift/BigQuery/Databricks/Spark) it doubles the backslash before escaping the single quote; on standard dialects it keeps the `''` quote-doubling. The double quote is deliberately left untouched — inside a single-quoted literal `\"` is NOT a recognised escape on 6 of the 7 backslash dialects (only MySQL), so escaping it would corrupt the value. The regime is DERIVED from sqlglot's own tokenizer via `SqlDialect.backslash_escapes_strings` (= `"\\" in tokenizer.STRING_ESCAPES`, guarded + 14-dialect pinned) so our escaping can never drift from the parser that reads the substituted SQL. `escape="python"` (Mode-B) additionally encodes the full C0 control range (`\t`/`\n`/`\r` named, rest `\xNN`) so raw newlines/NUL no longer break `ast.parse`. Engine fail-closed: `_substitute_model_sql_surfaces` / `_render_probe_model` require a `dialect`, threaded from the resolved datasource — no bare bool to forget. Assumes MySQL's default `sql_mode` (backslash escapes on); `NO_BACKSLASH_ESCAPES` servers are a sqlglot-layer-wide limitation, documented not fixed. The SQLite backslash end-to-end gap stays a pinned strict-xfail (pre-existing, out of scope). Bound parameters rejected (don't fit substitute-into-raw-SQL). Nested/join/cross-model lineages remain DEV-1678.
 - 2026-08-04 — Declared list-valued `{variable}` coercion (DEV-1730 follow-up): a scalar supplied for a variable the model declares `list_valued` is wrapped into a one-element list before Mode-A substitution, so an importer-generated `col IN ({var})` renders `IN ('US')` rather than the unquoted `IN (US)`. The generic scalar rule (author writes the quotes, so `{var}` also works in numeric/fragment positions like `amount >= {floor}` and `{d}::TIMESTAMP`) is CORRECT and unchanged — it just presumes an author who can see the SQL position, which a machine-generated fixed template does not have; the caller cannot supply per-element quotes through parentheses the importer wrote. Silent-wrong-answer risk drove the fix over a raise: `region IN (US)` parses as a column reference, so it fails at the database with a confusing message, or resolves against a real column and returns wrong rows. Opt-in is a front-end-NEUTRAL flag: the Cube converter writes `list_valued: ref.kind == "string"` into each `meta.cube_variables` entry (arrow forms splice pre-quoted scalars and stay `False`), and the engine reads only that flag — never Cube's `kind` taxonomy — so a future list-shaped front-end opts in the same way. Coercion lives at the single Mode-A choke point `_substitute_model_sql_surfaces` (execution and the `_render_probe_model` type-probe both route through it, so it cannot be bypassed) via `coerce_declared_list_variables` / `list_valued_variable_names` in `slayer/core/query.py`. Scope is deliberately narrow: only `str`/`int`/`float`/`bool` are wrapped; `list`/`tuple` pass through (the **empty list still raises** — "no filter" belongs to an optional block or a sentinel default); `None`/`dict` are left for `_render_variable_value` to reject with its own naming error; hand-written models declare nothing and are untouched. Follow-on from the same review: `declares_variables(model)` (any non-empty `meta.cube_variables`) now also defeats the DEV-1625 zero-variable fast path, via the shared `_model_needs_substitution_pass` predicate used by both `_substitute_model_sql_surfaces` and `_render_probe_model`. This closes the fast-path hole for a GENERATED model whose pushdowns are all required (no `{? ?}` block to force the pass): such a model used to emit a bare `{var}` into the SQL on a zero-variable call instead of raising the documented missing-variable error. The hole stays open — deliberately — for hand-written models, which declare nothing and keep the raw-brace-literal protection (`'{1,2,3}'`). The `list_valued` flag is matched with `is True`, not truthiness, since `meta` is user-extensible and a stray `1` or the string `"false"` must not switch substitution semantics. The bag is also SELF-IDENTIFYING — an entry counts only with a string `member` (the shape every importer writes) — so a hand-written `meta` that reuses the `cube_variables` key is not mistaken for generated SQL and silently stripped of its brace-literal protection.
 - 2026-08-05 — Ingestion sees views, survives unmodellable names, and stops being silent (DEV-1741). **Views**: `list_ingestable_objects` replaces the bare `get_table_names()` at every introspection site, adding `get_view_names` + `get_materialized_view_names` behind a `NotImplementedError`/`Exception` guard (the base `Inspector` RAISES for matviews on unsupporting dialects rather than returning `[]`), de-duplicated first-classification-wins because some dialects return views from `get_table_names()`, in a deterministic tables→views→matviews order that the name-collision policy depends on. Ingested by default — dbt materializes staging models as views, so opt-in would have left the reported failure in place for a fresh install — with `--no-views` on both `slayer ingest` and `datasources create --ingest`. The drift side (`_live_schema_for_datasource`) and the MCP listing (`_fetch_tables`) take **no flag and are unconditional**: that map is only ever a lookup target (`validate_datasource` iterates the *persisted* models, `available_in_ds` derives from them), so views there cannot manufacture a model or a drift entry — what they fix is a pre-existing **data-loss** bug where a hand-authored model whose `sql_table` named a view resolved to `live_table=None` → `WholeModelDelete` → deleted by `validate-models --force-clean`. Gating that on `--no-views` would re-arm it for exactly the users who opted out. **Names**: model names can't contain `__` (six modules — generator/enrichment/column_expansion/column_dependency/schema_drift/osi — `split("__")` an alias back into a join path, so a model named `a__b` is read as the alias for `a→b` and yields a silently wrong query, not a crash), but `sql_table` can, so a dlt child table `reports__patient__drug` is modelled as `reports_patient_drug` with `sql_table` verbatim. Sanitizer is `re.sub(r"_{2,}", "_")`, NOT `replace("__","_")` — `str.replace` is non-overlapping, so `a___b`→`a__b` would still fail validation. Collisions reserve every unsanitized name first (a real `a_b` always beats a sanitized `a__b`, order-independently) and **skip** rather than suffix, since suffixes shift as the object set changes and would orphan models and churn drift. A per-object `try/except` backstops everything else (`.`/`:`/`/`/`\` names, bad column names, per-object introspection failures); the FK-collection loop and `_get_fk_relationships` are guarded too because they run BEFORE that isolation and would otherwise still kill the run. **Reporting**: skips travel in a new `skipped` list, deliberately NOT folded into `errors` — `slayer ingest` exits 1 on either (we declined a perfectly valid object; `--exclude` is the documented remedy) but `POST /ingest` keeps 422 for `errors` only, because a permanent 422 aimed at a machine that can't act on the hint buries a successful partial ingest behind an error status. An empty scan prints the available schemas and exits 1 (the reporter listed the missing exit code as part of the defect), gated on `objects` not `additions` so a healthy no-op re-ingest stays quiet; `datasources create --ingest` on an empty DB still exits 0, since creating the datasource is that command's job and it succeeded. `in_scope_table_names` switched from model names to `_bare_table_name(sql_table)` — it is compared against table names in `_scoped_models_for_validation`, so the old keying silently dropped from validation scope any model whose name differs from its table (every sanitized model, and already every dbt/OSI hidden model passing `model_name=`). **`source_kind`** (`table`/`view`/`materialized_view`/`None`=unknown) persists on `SlayerModel`, v7→v8 with a no-op migration (mandatory: `migrate()` raises `RuntimeError` on an unregistered step); `None` for pre-v8, hand-authored and sql/query-backed models is the honest value, not a guess. It is a deliberate **exception to the additive-merge contract** — refreshed, not preserved — because it describes the live object rather than user intent, and the transition it exists to capture (dbt `+materialized: table`) usually changes no columns at all; the refresh therefore has to be made in three places (the early return, the `model_copy(update=...)`, and the save gate in `_process_one_table`), since editing only the update dict computes a corrected model and throws it away. A `None` from a non-classifying path never erases a known value. Docs-only fix for the advertised `motley-slayer[duckdb]` extra, which does not exist: `duckdb`/`duckdb-engine` are unconditional core deps (the Postgres facade imports duckdb at top level on every CLI invocation), and adding them under `[tool.poetry.extras]` would *gate* rather than alias them, breaking bare `pip install motley-slayer` → `datasources create demo`.
+- 2026-08-07 — Ingest resolves a schema scope, and non-default schemas are written into `sql_table` (DEV-1758). **The bug**: `duckdb_engine`'s `get_table_names(schema=None)` returns objects from *every* schema as bare names — Postgres/MySQL/MSSQL/Snowflake/BigQuery/ClickHouse/SQLite all restrict it to the connection's default — so `_build_one_model`'s `f"{schema}.{name}" if schema else name` wrote an unqualified `sql_table` for a non-default-schema object and the generator emitted `FROM reports`, which fails table-not-found. The same schema-blindness made `_get_columns_fallback` union two same-named tables' columns. Not literally a #283 regression: that `sql_table` line is byte-identical before and after, and `--schema` always qualified correctly; #283 changed *visibility* by ingesting views by default, and dbt materialises staging models as views, so a dlt+dbt DuckDB file went from a handful of models to dozens, most unqueryable. **Scope**: one pass covers ONE schema — explicit `--schema a,b` / `--all-schemas` (plus `schemas` / `all_schemas` on the Python, REST and MCP surfaces), else `datasource.schema_name`, else the connection default. Multi-schema is opt-in because it changes what `sql_table` holds; when exactly one schema is scanned and others exist, the result carries a hint naming them and the exit code is unchanged (a hint is not a failure). `schema_name` is a **fallback**, never a conflict with an explicit flag; the genuine conflicts (`schema`+`schemas`, `all_schemas`+either) are rejected by one shared helper called from every entry point, so the CLI's mutually-exclusive group is not the only thing holding the line. **Two different strings**: the *discovery token* is carried exactly as `get_schema_names()` yields it — catalog-qualified on DuckDB (`fda.main`), bare elsewhere — and the qualified form is the SAFE one. Measured: with a second catalog `ATTACH`ed, a bare `main` makes `get_table_names` and `has_table` reach into the attached catalog and makes the column fallback return the cross-catalog union, while `att_main.main` is exact. So `is_default` compares the token IN FULL (no last-segment comparison, which is what made `att_main.main` and `other.main` both read as the default), and the INFORMATION_SCHEMA fallbacks filter on `table_catalog` as well as `table_schema` — `table_schema` alone holds the bare name, so a qualified token matched nothing and produced a silently **column-less** model, and retrying bare would resurrect the union. This applies to the PK fallback too, which on DuckDB is the path that actually runs (its Inspector reports no PK even for a declared PRIMARY KEY) — filtering it wrong drops every primary key, and fan-out safety leans on `Column.primary_key`. The *emitted qualifier* is a different string: the bare last segment, since the connection's current catalog is already correct. **What gets qualified** (D-9): only non-default schemas, so widening the scan never rewrites models already on disk and one datasource legitimately mixes both forms; a schema named explicitly as a single value is written verbatim, preserving today's `--schema public` → `public.orders`. A multi-schema list is deliberately NOT verbatim — listing the default alongside another schema would re-qualify every existing model. `--all-schemas` means the **current catalog only**; attached catalogs are dropped loudly with the exact `--schema <catalog>.<schema>` invocation that ingests them, never silently. **Merging**: re-ingest heals a MISSING qualifier (participating in the short-circuit and the save gate, like `source_kind` — a repair usually changes no columns, so a merge that only edits the update dict computes the fix and discards it) but never rewrites an existing one. Two schemas' same-named tables are never fused into one model: a schema mismatch skips, and — the case a schema comparison alone cannot see, because D-9 persists default-schema models *unqualified* — a bare persisted `sql_table` that names a real default-schema object also skips, rather than being repointed at another schema's table by the heal. **Collisions** are resolved in ONE phase over final model names with a 4-key total order (unsanitized beats sanitized, then default schema, then schema name, then object name), not as successive passes, so the mixed case (`s1.a__b` sanitizing onto a real `s2.a_b`) is defined and the outcome never depends on inspector listing order. Losers skip, never suffix. **Validation** derives its schema set from the models being validated rather than gaining a flag, and the live map is keyed on the full `<schema_token>.<object>` identity plus shorter aliases. A contested alias resolves to the DEFAULT schema's entry, mirroring what the database itself does (`FROM orders` and `FROM main.orders` both land in the current catalog); it is dropped only when the default cannot break the tie. Dropping every contested alias — the first cut, and what the plan review asked for — was itself a data-loss bug, because default-schema models are persisted UNQUALIFIED by design, so the moment another schema gained a same-named table the legacy model stopped resolving. Schema names arriving from outside — a `--schema` argument, a persisted `schema_name`, a bare qualifier read back off `sql_table` — are upgraded to the enumerated catalog-qualified token before they reach an Inspector, since a bare token is exactly what sweeps ATTACHed catalogs; what the user typed is kept separately and is what gets emitted, so resolving for discovery can never change the SQL persisted. This is a data-loss path, not a nuisance: an unresolvable model is a `WholeModelDelete` that `validate-models --force-clean` acts on. One dotted-name splitter (`split_sql_table`, everything before the FINAL dot) replaces three disagreeing parsers, so hand-written Snowflake `db.schema.table` and BigQuery `project.dataset.table` stop losing their catalog. No new model field and no v9 migration — the schema lives in `sql_table`, which is where the generator already reads it.