Sync upstream v0.62.1 (merge conflicts)#29
Merged
Conversation
* Use transform timeout for queries inside transforms Previously, queries executed within a transform were subject to the lower of MB_DB_QUERY_TIMEOUT_MINUTES (default 20 min) and MB_TRANSFORM_TIMEOUT (default 4 h). This made MB_TRANSFORM_TIMEOUT effectively unusable for its stated purpose: long-running transforms always hit the query timeout first. Bind *query-timeout-ms* to the transform timeout inside run-cancelable-transform! so transform queries respect MB_TRANSFORM_TIMEOUT, while regular Metabase queries keep using MB_DB_QUERY_TIMEOUT_MINUTES. Fixes GDGT-2173. * Enforce query timeouts per-statement, not via pool leak-detector The c3p0 unreturnedConnectionTimeout was doing double duty: leak detection AND query-timeout enforcement. That pool-wide conflation meant transforms running longer than db-query-timeout always got their connection destroyed first, defeating the transform-timeout rebinding introduced earlier in this branch. This commit: - Calls Statement.setQueryTimeout uniformly in prepared-statement*/ statement* so every SQL-JDBC query carries its own server-side timeout derived from *query-timeout-ms*. Applied after the multimethod returns, so driver overrides that replace the base method also get it. - Raises the pool's unreturnedConnectionTimeout default to max(db-query-timeout, transform-timeout) so the safety net does not undercut a legitimate long transform; per-statement timeouts remain the real query killer. - Also rebinds *network-timeout-ms* inside run-cancelable-transform! so a single slow socket read does not kill a long transform earlier than its own deadline. - Replaces the prior tautological test with two real ones: the pool setting default correctly maxes query-timeout and transform-timeout, and a real H2 statement created through statement-or-prepared-statement observes the rebound timeout via .getQueryTimeout. GDGT-2173 * Revert "Enforce query timeouts per-statement, not via pool leak-detector" This reverts commit 5e01c82. * Reapply "Enforce query timeouts per-statement, not via pool leak-detector" This reverts commit a9f2942. * Simplify: strip ticket refs from source, hoist transform-timeout let * Stronger tests: proxy-based unit test + multi-driver sweep for setQueryTimeout * Route transform-timeout into driver pool default via registry, not cross-module lookup Replaces the requiring-resolve from driver/settings.clj into transforms with a register-long-running-timeout-provider! registry that transforms self-populates. Keeps the dependency direction correct (transforms -> driver) and silences the :metabase/modules kondo check. Also drops ^:parallel from the set-statement-query-timeout! unit test, which tripped the destructive-fn-in-parallel-test linter. * Route transforms through a dedicated :transform connection pool Replaces the cross-feature `register-long-running-timeout-provider!` registry with a new `:transform` value on the existing `*connection-type*` enum, so transforms reuse the pool-cache key `[db-id, connection-type]` mechanism that already isolates `:write-data`. The `:default` pool's `unreturnedConnectionTimeout` is now unchanged from pre-PR — only the `:transform` pool sees the longer leak-detector window — addressing reviewer concerns about widening the safety net for non-transform queries. * resolve-connection-type: take connection-type as arg, drop dynamic-var read Pure-function refactor — caller passes *connection-type* (or any value) explicitly. Easier to test, no implicit dependency on the dynamic binding. * :transform also merges write-data-details; restore catch Throwable Transforms route through `with-write-connection` (query_impl.clj:30), so they need write-data credentials when configured. My earlier refactor's `with-transform-connection` rebind dropped those credentials by treating `:transform` as a separate write-context from `:write-data`. Fixed by having `effective-details` merge `:write-data-details` when *connection-type* is either `:write-data` or `:transform` — the pool key still differs so pool properties (unreturnedConnectionTimeout) can be tuned per workload. Updated `transform-creates-write-pool-test` -> `transform-creates-transform-pool-test` to assert the new `[db-id :transform]` cache key. Restored `catch Throwable` in `set-statement-query-timeout!` to match the original PR — Spark/Hive may throw `AbstractMethodError` (an `Error`, not `Exception`); the previous PR's `catch Throwable` swallowed it. * Gate Statement.setQueryTimeout behind :jdbc/set-query-timeout feature flag SparkSQL's Hive JDBC driver appears to handle Statement.setQueryTimeout by closing the Thrift transport on the server side; subsequent statement close() then throws "Failed to close statement" with TTransportException: Socket is closed by peer. The catch in set-statement-query-timeout! silences the setQueryTimeout call itself, but the session is already poisoned and any explicit-joins test errors on .close(). Adds :jdbc/set-query-timeout feature flag (defaults true on :sql-jdbc, overridden false on :sparksql). set-statement-query-timeout! takes the driver as an arg and skips the call entirely when the feature is off. * Skip getQueryTimeout assertion for drivers that opt out of setQueryTimeout Spark/Hive opts out via :jdbc/set-query-timeout=false, so getQueryTimeout() returns the JDBC default (0), not the bound *query-timeout-ms*. Branch the assertion: drivers that support setQueryTimeout still round-trip; opt-outs only assert a Statement was created. * Clarify :transform connection-type docstrings per review The with-transform-connection docstring claimed :transform shares primary :details with :default, but effective-details merges :write-data-details for :transform just like :write-data. Reconcile *connection-type*, with-transform-connection, and resolve-connection-type docstrings to that truth: :transform resolves details like :write-data but routes through a separate pool key so c3p0 can carry transform-tolerant properties.
…test (metabase#74344) * Add JUnit hook-failure rewrite postprocessor Adds e2e/support/fix-junit-hooks.ts, a Bun script that rewrites Mocha/Cypress JUnit XML so failures from before/after hooks are attributed to the underlying test instead of '<suite> "before each" hook for "<test>"'. This keeps Trunk's flake tracking keyed on stable test names. Supports directory mode for performance, --dry-run, and --include substring filtering for gradual rollout. * Wire JUnit hook-failure rewrite into e2e workflows (dry-run) * Log per-testcase rewrites under each file's summary line
* Add tables redux thunks and retire-ready reducer scaffolding to prepare to remove it entirely * Migrate simple table consumers off the Tables entity * Migrate permissions table loading off the Tables entity * Migrate Tables HOC loaders and list invalidation off the entity * Remove tables entity * Match noWrapper semantics * Avoid mutating redux state in-place * Avoid double-normalizing the tables data * Await table metadata fetch * Add unit test for tables-reducer * Remove relyance on EntityName for tables * Avoid forcing request Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…e#74407) * Fix double vertical scrollbar in filter dropdown Make the accordion list the scroll container inside clause popovers so the search bar can stay sticky, instead of the popover dropdown's content box scrolling around it. * Fix typo in comment
fix: allow adding rows to editable tables with a date column The date input declared its props as Omit<..., "onChange"> and emitted onEnter on pick instead of onChange, so a picked date never reached the create/update form state. With a required (NOT NULL) DATE column the submit button stayed disabled, blocking record creation (metabase#70647). Emit onChange (modal form state) and onBlur (inline commit) on pick, matching the boolean/dropdown selects, and close the calendar after a pick. Adds an e2e regression covering record creation with a required date column.
* wip metaplow analytics # Conflicts: # frontend/src/metabase/utils/analytics-untyped.js * payload fix * update settings * fix hostname * update metaplow * update mock type * update types * add unit tests * Metaplow/backend events (metabase#74516) * feat: metabase.analytics.event Raise track-event! to new namespace matching FE, in preparation to add a second event tracker. * feat: metabase.analytics.metaplow Add new analytics namespace to send events to metaplow. Semantics are similar to snowplow: - events are sent in up to 50 parallel workers sharing a HTTP connection pool - event queue can have up to 10k pending events, after which new ones are dropped Metaplow does not have a batch endpoint currently so that is not used. * feat: metrics for metaplow errors * rfct: delay metaplow resources # Conflicts: # src/metabase/analytics/prometheus.clj * return promise --------- Co-authored-by: Filipe Silva <filipematossilva@gmail.com>
…etabase#74035) * BOT-1495 Trim all metabot append-only tables to MB_AI_USAGE_MAX_RETENTION_DAYS As of BOT-1409 we now trim the ai_usage_log table to prevent unbounded growth. The same arguments for trimming the ai_usage_log table apply to other metabot append-only tables, like metabot_conversation, metabot_message, metabot_feedback, and metabot_source_feedback. - Update the fk on `metabot_message` pointing to `metabot_conversation.id` to `ON DELETE CASCADE` - Add a trimmer task for `metabot_conversation` to also respect `MB_AI_USAGE_MAX_RETENTION_DAYS`. Note that if we don't do this, older conversations will not have any source or profile_id associated, since these come from the ai_usage_log table. This is not the only cause, but one possible cause of issues like BOT-1415 See also * metabase#73400 * metabase#73518 * Shorten the gap between the ai_usage_log and metabot_conversation trimmer tasks
Reimporting warehouse metadata could fail with a uniqueness constraint violation on `metabase_table`. The code was matching existing IDs while filtering out inactive tables, which was wrong. To reproduce, import the metadata, deactivate a table, and then reimport that metadata. A regression test is included.
drewr/postal uses Jakarta Mail, and supports a debug option when sending messages. However, it requires callers of the library to manually pass the debug option, and doesn't look at any JVM options. This change makes it so that setting the mail.debug JVM property to true enables debug logging for all outgoing SMTP messages. No credentials are logged.
`:lib/uuid` keys were leaking into serialized MBQL for column references that use string column names. These UUIDs are non-portable. This adds matcher clauses for the string-id cases of `:field`/`:metric`/`:segment`/`:measure`, so that `export-mbql-ref` walks them appropriately. UUIDs for aggregation reference targets are preserved.
* Update `actions/upload-artifact@v4` -> v7 * Upgrade `actions/download-artifact@v4` -> v8
* Stop dropping old datasets from snowflake. There's a couple bugs here; first of all a race condition when inserting datasets which can cause data to get triple-inserted. We could improve locking around this, but it shouldn't be happening anyway since datasets should only be removed when they haven't been accessed in 5 days. Local testing in the repl shows the correct behavior, but randomly in CI sometimes it will decide to drop the datasets anyway. Have not been able to determine why. * Remove snowflake from "needs" section of drivers-tests-result. We want to allow these to run in order to gain data, but we can't afford to make them disrupt CI.
* agent-lib: representations YAML parser + schema validator
First step of migrating construct_notebook_query from the sexp-in-array DSL to
the canonical MBQL 5 representations format (see repr-plan.md).
Adds metabase.agent-lib.representations with:
- parse-yaml: wraps metabase.util.yaml with :keywords false so 'lib/type'
string keys survive intact (we lose the 'lib/' namespace segment if
clj-yaml keywordizes them). Throws :invalid-representations-yaml with
:agent-error? false for malformed input.
- validate-query: Malli schema covering the Phase-1 subset (mbql/query top
level, single-stage mbql.stage/mbql, portable FK vectors for source-table
and field refs, clause shape [op {opts} ...args]). Structural only; deep
clause content is validated by lib.schema/Query after resolution.
- parse-and-validate: convenience combinator.
Implementation notes captured in the plan:
- The Malli schema avoids :cat+:ref recursion (triggers
:malli.core/potentially-recursive-seqex). Clause-shape is encoded as a
flat [:and vector? [:fn ...]] structural predicate.
- clj-yaml returns #ordered/map (OrderedMap) which satisfies map?
- no special handling needed in downstream passes.
* models.serialization.resolve: MetadataProvider-backed resolver
Adds metabase.models.serialization.resolve.mp with import-resolver and
export-resolver factories that satisfy SerdesImportResolver /
SerdesExportResolver via a lib.metadata.protocols/MetadataProvider rather
than the application DB.
This lets the representations resolver (next commit) translate portable
FK paths like [Sample, PUBLIC, ORDERS, CREATED_AT] to numeric IDs without
round-tripping through toucan2, so it works for any MetadataProvider the
caller supplies (application DB, mock, saved-question graph, etc.).
Implemented methods (sufficient for Phase 1):
- import-table-fk / export-table-fk
- import-field-fk / export-field-fk (handles JSON-unfolded nested fields
by walking :parent-id upward on export and descending name+parent-id
on import)
Other protocol methods throw :not-implemented-yet so unsupported serdes
entry points fail loudly rather than silently dropping through.
Implementation notes:
- Uses lib.metadata.protocols/table and /field directly, not the mu/defn
wrappers in lib.metadata - those throw Malli validation errors on nil
returns; the protocol methods return nil cleanly on a miss, which is
what we want for 'unknown-table' / 'unknown-field' errors.
* agent-lib: representations resolver (portable YAML -> canonical MBQL 5)
Step 3 of the representations migration. Adds
metabase.agent-lib.representations.resolve with:
- keywordize-query: recursively converts string map keys to keywords
while preserving string *values* (so 'lib/type: mbql/query' becomes
:lib/type :mbql/query but portable FK strings like 'ORDERS' stay as
strings).
- resolve-query: main entry point. Keywordizes -> binds
resolve/*import-resolver* to the mp-backed resolver -> calls
resolve/import-mbql -> normalizes through lib.schema/query with the
MetadataProvider attached.
- parse-validate-resolve: YAML string + MP -> MBQL 5 query in one call.
Also extends .../resolve/mp.clj: implements import-fk-keyed for
:model/Database by name (needed because import-mbql dispatches on the
top-level :database key). Throws :unknown-database with :agent-error?
true when the YAML references a database other than the MetadataProvider's.
Kondo module config updates:
- Adds metabase.agent-lib.representations{.repair,.resolve} to the
agent-lib module's :api.
- Adds 'models' to agent-lib's :uses (resolver lives under
metabase.models.serialization.resolve.*).
- Exposes metabase.models.serialization.resolve.mp on the models :api.
Tests cover aggregation+breakout with :temporal-unit, filters+limit+
order-by, fields projection, explicit join with alias+strategy+join-alias,
and three error paths (unknown-table, unknown-field, database-mismatch).
* agent-lib: representations repair pass (fills in missing {} options and lib/type markers)
Step 4 of the representations migration. LLMs reliably drop two things
when writing repr YAML:
1. The empty options map in clauses - writing [count] instead of
[count, {}], or [field, [Sample, PUBLIC, ORDERS, ID]] with no
options slot.
2. The lib/type markers on the top-level query and on stages.
Rather than surfacing these as validation errors to the LLM (which wastes
turns and tokens), we normalize them in a repair pass before validation.
metabase.agent-lib.representations.repair composes two idempotent passes:
- ensure-clause-options*: inserts {} at position 2 of clause-like vectors.
Three cases handled:
[op] -> [op, {}]
[op, nil, ...] -> [op, {}, ...]
[op, <non-nil-non-map>] -> [op, {}, <non-nil-non-map>] (insert,
never replace)
The clause-like? predicate distinguishes real clauses from portable FK
paths via looks-like-fk-path? (length >= 3, all string?/nil?). It also
excludes map-entry? so clojure.walk/postwalk doesn't misfire on map
traversal.
- ensure-lib-types*: fills 'lib/type': 'mbql/query' on top-level query
maps and 'lib/type': 'mbql.stage/mbql' on stage-like maps. Uses string
keys because repair runs pre-keywordization.
Both passes are idempotent - running them twice is equivalent to running
them once. The test suite proves this generatively (defspec, 100
iterations) in addition to 19 example-based assertions.
* metabot: flip construct_notebook_query to representations YAML format
Step 5 of the representations migration. Wires the parse->repair->
validate->resolve pipeline into the construct_notebook_query tool,
replacing the sexp-in-array program format.
metabase.metabot.tools.construct:
- New execute-representations-query: resolves source-entity's database ->
builds an application-DB-backed MetadataProvider -> runs the repr
pipeline -> wraps result in the same {:structured-output {...},
:instructions ...} shape as execute-program. Any ExceptionInfo from the
pipeline is re-thrown with :agent-error? true so the tool wrapper
surfaces a helpful message to the LLM instead of a stack trace.
- Tool arg schema: :program is replaced by :query :string (YAML string).
Kept flat at :string to sidestep MCP flatten-root-schema pitfalls;
structural validation happens inside execute-representations-query.
- construct-notebook-query-tool now calls execute-representations-query.
- execute-program and construct-program-schema remain in place; they're
still used by slackbot-construct-notebook-query-tool during the
migration. Slackbot moves to repr in a later phase.
- resolve-source-database-id is now defn (was defn-) so tests can stub it
with with-redefs.
metabase.metabot.tools.util:
- ->result-column now emits a :portable_fk key alongside :field_id when
the column maps to a real database field. Walks the :parent-id chain
to build the path for JSON-unfolded nested fields. Degrades to nil for
expression/aggregation columns or when the MetadataProvider can't
resolve the path. Numeric ids are preserved so existing callers
(chart creation) keep working.
resources/metabot/prompts/tools/construct_notebook_query.md:
- Fully rewritten for the Phase-1 MVP subset. Documents top-level shape,
stage shape, the 'options map at position 2 is always present' rule,
portable FK format (3-element table, 4+ element field, null-schema for
schemaless DBs, JSON-unfolded nested paths), examples for each MVP op
(filter incl. 'and', aggregation, breakout with :temporal-unit,
order-by, limit, fields projection), explicit joins with :join-alias
propagation, implicit joins via repair, and a Phase-1 scope-out list.
Tests:
- test/metabase/metabot/tools/construct_representations_test.clj: new,
4 tests / 12 assertions. Happy path (YAML -> fully-resolved MBQL 5),
malformed YAML surfaces :agent-error?, unknown table surfaces
:agent-error?, and a broken LLM-style YAML with missing lib/type and
missing {} options round-trips cleanly through repair.
- test/metabase/metabot/tools_test.clj: migrated
construct-notebook-query-tool-test from :program / execute-program to
:query (YAML string) / execute-representations-query.
* metabot: expose portable FK paths in entity_details; disable implicit-joins claim in prompt
Two small follow-ups needed to actually use construct_notebook_query in
its new representations form:
1. entity_details' table-details output now includes:
- :database_name at the top level
- :portable_fk: [db-name, schema-or-null, table-name] matching the
format the representations tool expects for source-table
Per-field portable FKs were already exposed via ->result-column in
the previous commit. Without the table-level db-name, the LLM had
no way to construct the 3-element table FK path.
2. The construct_notebook_query prompt previously promised implicit
joins via a repair pass ('Metabase will fill in source-field for
you'). That repair pass isn't implemented yet - it's Phase 1 Step 6.
Flipped the prompt language to explicitly say implicit joins aren't
supported yet, and added it to the Phase-1 scope-out list. Users who
want cross-table fields must write an explicit joins: section.
* metabot: migrate remaining integration tests + document_construct_model_chart to representations
After flipping construct_notebook_query to the representations format,
three other call sites were still sending the legacy :program payload:
1. metabase.metabot.tools.document/document-construct-model-chart-tool
wraps construct-notebook-query-tool and forwarded its :program arg
through unchanged. Renamed its schema field :program -> :query and
updated the inner call. This was an actually-broken call site, not
just a test.
2. test/metabase/metabot/agent/core_test.clj integration-search-query-
chart-flow-test drives the real agent loop with a scripted LLM and
exercises construct_notebook_query end-to-end against the H2 test
database. Migrated its scripted tool-call to the representations
YAML format. The test looks up the real database name, schema, and
table name at runtime to build portable FK paths, so it stays
resilient to the test DB details. JSON-encoded flow-sequence
literals embedded in the YAML string (valid YAML since YAML is a
superset of JSON) avoid the pr-str pitfall where Clojure writes
[nil ...] instead of [null, ...].
3. test/metabase/metabot/tools/charts/edit_test.clj's
edit-chart-of-constructed-query-test is another real integration
test - it calls construct-notebook-query-tool against the products
table and then exercises edit-chart-tool on the resulting chart.
Migrated to use the portable_fk fields from entity_details instead
of numeric field-ids.
4. test/metabase/metabot/tools/document_test.clj's
document-construct-model-chart-tool-test was a unit test using
with-redefs; just updated the arg shape to match the new schema.
All three integration tests now pass against the representations
pipeline end-to-end. The last two are especially valuable because they
exercise the full parse -> repair -> validate -> resolve -> QP path
with a real MetadataProvider, which is what will happen in production.
Uses metabase.util.json instead of cheshire directly, per kondo lint.
* agent-lib: auto-wire source-field for implicit joins in representations repair
Phase 1 step 6 of repr-plan.md.
Extends the representations repair pass with a third pass that walks the
first stage's clauses and fills in the `source-field` option on any
`[field, {...}, [db, schema, OTHER_TABLE, col, ...]]` reference whose target
table is reachable from `source-table` via exactly one foreign key. The QP
interprets a field with `source-field` as an implicit join, so the LLM no
longer needs to author an explicit `joins:` entry for the common
single-FK case.
* `repair` now takes a `MetadataProvider` as its second argument. The
shape passes (ensure clause options, lib/type markers) are unchanged and
don't need the MP; the new implicit-join pass does. Public surface stays
one function.
* When the source-table can't be resolved in the MP, the pass silently
no-ops and leaves the error reporting to the downstream validate /
resolve stages (which have better messages for that case).
* When there is no FK at all, or more than one FK, the pass throws
ex-info with `:agent-error? true` and `:error :no-fk-path` or
`:ambiguous-fk`. `:ambiguous-fk` surfaces the candidate FK portable
paths so the LLM can pick one and retry with an explicit
`source-field` option.
* Field clauses that already carry `source-field` or `join-alias` are
left alone. The `joins` subtree of a stage is not descended \u2014 its
field clauses live in a join context and are expected to carry a
`join-alias` already.
* Adds `resolve.mp/outbound-fks-from-table` to return the FK edges
leaving a given table, so the repair pass can precompute a
target-table-id -> candidate-FKs map once per stage.
* `tools/util/->result-column` now emits a `:fk_target_portable_fk`
alongside `:portable_fk` for columns with an `:fk-target-field-id`,
so the LLM can discover candidate source-fields via `entity_details`.
* Prompt: replaces the "Implicit joins \u2014 not yet supported" section
with concrete rules and examples; removes the scope-out entry.
Test counts (before \u2192 after):
* agent-lib/representations.repair-test: 14 \u2192 19 tests, 17 \u2192 35 assertions
(happy-path, skip-on-source-field, skip-on-join-alias, skip-on-source-table,
no-fk, ambiguous, skip-joins-subtree, idempotence, skip-on-unresolvable-mp)
* serialization/resolve.mp-test: 7 \u2192 10 tests, 34 \u2192 37 assertions
(outbound-fks happy path, no-FK table, simple MP)
* metabot.tools.construct-representations-test: 4 \u2192 6 tests, 12 \u2192 17
assertions (implicit-join happy path, ambiguous surfaces agent error)
* All existing metabot/agent-lib tests stay green: 100 tests, 629 assertions.
* agent-lib: rewrite portable database name to match metadata provider in repair pass
Our prompt examples for construct_notebook_query consistently use `Sample`
as the database name. The LLM follows them literally, producing a query like:
database: Sample
source-table: [Sample, PUBLIC, ORDERS]
against the real application DB whose name is `Sample Database`. The
metadata-provider-backed resolver requires exact string equality on the DB
name, so the canonical demo flow (e.g. "total revenue per product category")
fails with :unknown-database before the query can even be validated.
Per repr-plan.md step 13, source_entity (and thus the metadata provider) is
the ground truth for the target database -- the YAML's `database:` key and
the DB component of every portable FK are redundant w.r.t. the MP. Add a
new repair pass that overwrites both with the MP's actual DB name before
validation/resolution.
The pass is permissive: any DB name (real, prompt-derived, hallucinated)
gets normalised to the MP's name. Cross-DB queries are not supported by
this tool, so the MP unambiguously identifies the only legal DB.
* Unit tests in repair_test.clj: top-level rewrite, portable-FK rewrite,
no-op when names already match, no-op when mp=nil, idempotency.
* Reproducer in construct_representations_test.clj: verbatim YAML from the
bug report against an MP named "Sample Database".
* End-to-end test in charts/edit_test.clj: same YAML against the real
application sample DB, asserting full chart construction + SQL compile.
* agent-lib: rewrite inline order-by aggregations to aggregation refs in repair pass
When the user asks for something like "top categories by total revenue",
the LLM tends to write `order-by` by re-stating the aggregation expression
inline:
aggregation: [[sum, {}, [field, {}, [..., TOTAL]]]]
order-by: [[desc, {}, [sum, {}, [field, {}, [..., TOTAL]]]]]
In MBQL 5 / legacy MBQL the `order-by` direction must wrap an aggregation
**reference**, not the aggregation expression itself. `lib.normalize`
accepts the inline form, but `lib/->legacy-MBQL` produces a query that
fails legacy-schema validation the moment something downstream re-loads
it (e.g. the chart is opened, the QP round-trips through legacy MBQL and
explodes with `Invalid output: {:query {:order-by ...}}`). The agent
reports success but the user sees a broken chart.
Add a repair pass that walks each stage's `order-by`, structurally
matches each inner clause against the stage's `aggregation` list
(ignoring options-map differences), and rewrites the inline expression
to `["aggregation" {} "<uuid>"]` -- synthesising the matching
aggregation's `lib/uuid` if it doesn't already have one. Idempotent:
existing aggregation references and unmatched expressions are left alone
so validation/normalize can surface real errors.
This does NOT introduce general placeholder-UUID handling (Phase 2 step
10's `@agg-N` references); we only resolve this specific recurring
mistake.
* Update prompt with a canonical "Order by an aggregation" example
showing that the inner expression must match an entry in
`aggregation:`.
* Unit tests in repair_test.clj: happy path, multiple aggregations
matched independently, reuses pre-existing lib/uuid, leaves existing
aggregation refs alone, leaves field-orderings alone, leaves
no-match aggregations alone, idempotency.
* Reproducer in construct_representations_test.clj: verifies the
resolved query round-trips through legacy MBQL cleanly.
* End-to-end test in charts/edit_test.clj: same YAML against the real
application sample DB, asserting full chart construction + SQL
execution returning rows in descending sum order.
* agent-lib: drop `source_entity` and `referenced_entities` from construct_notebook_query (repr-plan step 13)
The YAML query is self-describing: it carries `database: <name>` at the
top level and full portable FK paths (`[<db>, <schema>, <table>]`)
everywhere else. The separate `source_entity` parameter was redundant
and could disagree with the YAML, forcing the LLM to keep two things in
sync for no reason.
* `construct-notebook-query-args-schema`: down to `{reasoning?, query,
visualization?}`. Removed the now-unused `source-entity-schema`.
* `execute-representations-query`: signature is now `[yaml-string]`.
Database id is derived from the parsed YAML's `database:` field by
the new public helper `resolve-database-id-from-yaml`. Three
`:agent-error?` paths:
- `:missing-database-name` -- YAML lacks a `database:` field.
- `:unknown-database` -- no `:model/Database` matches that name;
error message instructs the LLM to use the canonical name reported
by `entity_details`.
- `:ambiguous-database-name` -- two databases share that name; LLM
has no way to disambiguate, so this is a hard error.
* `resolve-source-database-id` kept untouched (still used by
`execute-program` for slackbot until step 14 migration).
* `document-construct-model-chart-tool` lost its `:source_entity`
schema entry too -- it's a thin pass-through to the construct tool.
The `rewrite-database-name*` repair pass (Pass 2.5, added during the
step-7 smoke test as a tactical bridge) is now obsolete: the MP is
*built from* the YAML's `database:`, so by construction the names
agree, leaving the pass nothing to do. Per repr-plan.md option (b)
("strict-with-good-error"), the pass and its 5 unit tests are deleted;
the LLM gets a clear error message instead of a silent rewrite. The
`metabase.lib.metadata` import in repair.clj goes with it.
Prompt update (`construct_notebook_query.md`): the `source_entity` /
`referenced_entities` bullets are removed; the `database:` field
description now reads "the exact database name as reported by
`entity_details`" with an explicit warning that misses fail loudly.
Tests:
* All 9 call sites in `construct_representations_test.clj` switched to
the new single-arg signature. The old DB-name reproducer is rewritten
to assert the new loud-failure behaviour. Added 3 new tests: missing
`database:`, non-string `database:`, and `database:` /
`source-table[0]` mismatch.
* `charts/edit_test.clj`: 4 call sites switched. The DB-name E2E test
now asserts the agent-targeted error (no chart, just a clear
`Unknown database` message). Added a sibling
`construct-notebook-query-llm-uses-canonical-db-name-end-to-end-test`
for the post-step-13 happy path. The order-by E2E test switched to
the canonical name (would otherwise short-circuit at the lookup
before exercising the order-by repair pass).
* `tools_test.clj` and `document_test.clj`: stub signatures and call
sites updated.
* Removed 5 obsolete `rewrite-database-name-*` unit tests; replaced
with a comment block pointing at the new test coverage.
86 tests / 529 assertions across all touched namespaces; kondo clean.
* agent-lib: migrate slackbot notebook-query tool to representations pipeline (repr-plan step 14)
The slackbot variant of `construct_notebook_query` now consumes the same
MBQL 5 representations YAML as the main notebook tool, going through
`execute-representations-query` instead of the legacy
`execute-program` + sexp-in-array flow. The slackbot-specific parts
(`:title` and `:display` options, `adhoc_viz` data parts, Slack
visualization link) are unchanged — only the query-construction engine
underneath moved.
Schema change (`slackbot-query-schema`):
* Added: `:query` (a plain `:string`; flat shape for MCP compat — YAML
validation happens inside `execute-representations-query`).
* Dropped: `:source_entity`, `:referenced_entities`, `:program`. Per
repr-plan step 13, the YAML is self-describing (`database: <name>`
at the top level, full portable FK paths everywhere else), so the
separate entity identifiers are redundant.
Follow-on cleanup in `construct.clj`:
* Deleted `construct-program-schema` — slackbot was its last consumer.
* Marked the legacy sexp-pipeline scaffolding
(`source-entity->model-str`, `program-source->source-entity`,
`source-metadata-for`, `surrounding-tables-for`,
`available-measure-ids`, `build-evaluation-context`, and
`execute-program` itself) with a section comment naming step 15 as
the delete point. These functions are kept alive for now because the
HTTP `/v2/construct-query` endpoint in `metabase.agent-api.api` still
calls them; they die with the endpoint migration.
* `resolve-source-database-id` stays as-is — `execute-program` uses it,
and kept in the "live" layer of the namespace rather than the sexp
block because its signature is friendly enough to potentially live on
as a public helper after step 15 (TBD).
Tests (`slackbot_query_test.clj`):
* Added `schema-shape-post-step-14-test`: asserts the new keys are
present and the three legacy keys are gone. Acts as the schema-level
contract guard.
* Added 4 stubbed-pipeline unit tests for the slackbot wrapper:
- happy path (YAML pass-through, adhoc_viz data part carries title +
display + link + query);
- no-optional-fields path (title/display absent → not included in
the part, but the link is still built);
- `:agent-error?` passthrough (bare `:output` with the message,
no structured-output or data-parts);
- non-agent exception wrapping (generic `Failed to construct notebook
query:` prefix so stack traces don't leak to the LLM).
* Added 2 end-to-end tests against the real sample DB:
- happy path exercising the full repr pipeline + real
`query->question-url`;
- unknown-database error path, asserting the step-13
loud-failure behaviour via the slackbot contract.
Prompt (`construct_notebook_query.md`) is shared between the two tools
and describes `:visualization` (notebook) rather than `:title`/`:display`
(slackbot). This asymmetry is pre-existing — the old slackbot schema
had `:program`, also not documented in the shared prompt — not
introduced by this commit. Left as a follow-up.
All 93 tests / 575 assertions across touched namespaces pass
(representations parser/repair/resolve + construct + slackbot +
charts/edit + document + metabot.tools). Kondo 0/0 on the three
modified files.
* agent-lib: multi-stage representations queries (repr-plan step 8)
Adds a new repair pass that lets LLMs write multi-stage queries the
way they naturally want to: a `[field, {}, "<column-name>"]` clause in
stage N referencing an output of stage N-1, with no `base-type` option
set. The lib-schema requires `base-type` on any string-named field
ref, so these queries used to blow up at `lib/query` time with a
`missing required key, got: nil` error.
## Changes
### `agent_lib/representations/repair.clj`
Adds **Pass 5 — `infer-cross-stage-field-types*`**:
* When `stages.length >= 2`, walk stages left-to-right. Before
processing stage `i`, build a truncated query of stages[0..i-1],
resolve it through the existing pipeline, and call
`lib/returned-columns` on the result. Index those columns by name.
* Walk stage `i` (skipping descent into its `joins` subtree — join
stages have their own resolution context). Every
`[field, opts, "<name>"]` clause whose opts lack `base-type` gets
its opts merged with the inferred `{base-type, effective-type}`.
* Best-effort and idempotent: if the mini-resolve fails, the pass
logs at debug and lets the downstream resolver surface the real
error; clauses whose name isn't produced by the previous stage are
left alone so the lib-schema error points at the typo; clauses that
already have `base-type` are never overwritten.
Also tightens the docstring on `resolve-implicit-joins*` to explain
why that pass stays stage-0-only (implicit FK joins require a
`source-table`, which only the first stage has).
### `metabot/tools/construct.clj`
Widens the `:agent-error?` catch in `execute-representations-query`:
the previous catch only wrapped parse/repair/validate/resolve, but
cross-stage field-type errors actually surface in
`result-columns-for-query`'s `lib/query` call. The new catch wraps
the entire pipeline from the MP forward, so any lib-schema violation
reaches the LLM with `:agent-error? true` set and no stack trace.
### Prompt (`construct_notebook_query.md`)
* Removes "Phase 1 MVP supports a single stage; multi-stage is coming"
and the matching "not yet supported" bullet.
* Adds a "Multi-stage queries" section documenting the cross-stage
ref shape (`[field, {}, <column-name>]` as a string), two concrete
examples (post-aggregation filter `count > 10`, post-aggregation
aggregation `avg(daily sum) by month`), and the rules (first-stage
`source-table`, valid output column names, joins in later stages).
### Tests
* **`repair_test.clj`** — 8 new unit tests: happy path, existing
`base-type` preserved, breakout-column inheritance, string-FK-path
isolation, no-previous-stage no-op, unknown-column leave-alone,
idempotency, repair+resolve smoke.
* **`construct_representations_test.clj`** — 4 new E2E tests: two-stage
post-aggregation filter, explicit-base-type preservation, three-stage
chain (filter + order-by both reference the aggregate), and
unknown-column `:agent-error?` propagation.
All tests pass: 105 tests / 600 assertions across representations
parser/repair/resolve + construct + slackbot + charts/edit + document
+ metabot.tools + serialization resolver. Kondo 0/0 on the 4 modified
source/test files.
## What's NOT changed
* Resolver (`agent_lib/representations/resolve.clj`) — already handled
multi-stage for free; `import-mbql` walks the structure recursively
and cross-stage string refs don't match any of its vector-shape
patterns, so they pass through untouched.
* Representations YAML schema — no new shape was needed; a
cross-stage ref is just an ordinary `[field, opts, _]` clause with a
string instead of an FK vector in position 2.
* Implicit-join pass — stays stage-0-only. Implicit FK joins are only
meaningful from a stage that owns its `source-table`.
* agent-lib: support expressions in representations queries (repr-plan step 9)
- Added pass 1.5 `normalize-expressions-shape*` in repair: accepts the
LLM-friendly map form `expressions: {Name: clause}` and normalises to the
canonical MBQL 5 sequential form with `lib/expression-name` stamped from the
map key. Canonical sequential input passes through unchanged; authored
`lib/expression-name` in options is never overwritten. Idempotent.
- Resolver unchanged: `["expression" {} "<name>"]` refs are 3-element
vectors with a string in the last slot and don't match any portable-FK
pattern, so they pass through the resolver untouched.
- Prompt: new "Expressions (custom columns)" section with arithmetic
(Subtotal = TOTAL + TAX) and string-concat (FullName) examples, plus a
rules block on YAML quoting of arithmetic operators and the map-key
shortcut. Removed expressions from the Phase 1 'not yet supported' list.
- Tests: 7 new unit tests in repair_test (map shape, sequential, sequential
without name, existing-name preserved, absent, idempotency, multi-stage) +
3 new end-to-end tests in construct_representations_test (map shape,
canonical sequential, expression in breakout).
Totals: repair 34\u219241 / 59\u219270 assertions; construct 15\u219218 / 45\u219256.
Kondo 0/0 on touched files.
* agent-lib: resolve integer-indexed aggregation refs to UUID form (repr-plan step 10)
- Added pass 2.8 `resolve-aggregation-ref-indexes*` in repair: walks each
stage, stamps `lib/uuid` on aggregation clauses that lack one, and rewrites
every `[aggregation, opts, <int>]` ref to the canonical MBQL 5
`[aggregation, {lib/uuid, base-type, effective-type, \u2026}, "<uuid>"]` shape.
0-based integer index mirrors legacy MBQL 4 prior art (`[:aggregation 0]`,
see `legacy_mbql/schema.cljc`); no new invented sigil.
- Type inference by aggregation head: count / distinct / cum-count /
count-where \u2192 type/BigInteger; avg / median / stddev / var / share \u2192
type/Float; sum / sum-where / cum-sum / min / max inherit from the inner
field's annotated base-type (fallback type/Float); unknown heads \u2192 type/*.
Authored base-type / effective-type / name / etc. on the ref's options are
never overwritten.
- Out-of-range index raises `:agent-error?` with code
`:aggregation-ref-out-of-range` and a message listing every available
aggregation as `<head> at <index>`. Integer ref in a stage with no
`aggregation:` raises `:aggregation-ref-no-aggregations`.
- Same-stage only. Cross-stage aggregation refs are out of scope here because
pMBQL forbids that shape \u2014 the correct downstream form is a cross-stage
field ref by column name, handled by the existing Pass 4.
- Idempotent: refs whose last slot is already a UUID-shaped string pass
through untouched. Resolver: no change.
- Prompt: new "Aggregation references" section with the two LLM-facing
forms (inline vs by-index) and an example. Removed "Aggregation references
with UUIDs" from the Phase 1 'not yet supported' list.
- Tests: +9 unit tests (happy path, authored options preserved,
out-of-range agent-error, UUID-string idempotent, no-aggs agent-error,
no integer refs no-op, full idempotency, type inference table,
multi-stage same-stage ref) + 2 end-to-end tests (execute-representations-query
resolving to valid pMBQL; out-of-range surfacing clear error).
Totals: repair 41\u219250 / 70\u219298 assertions; construct 18\u219220 / 56\u219263 assertions.
Kondo 0/0 on touched files.
* resolve/mp: import Card FK by entity_id (repr-plan step 11.1)
The metadata-provider-backed serdes import resolver now handles
`import-fk` for `Card` / `:model/Card`, looking up the saved question /
model by its portable `entity_id` via `serdes/lookup-by-id`. Previously this
threw `:not-implemented-yet` and blocked the agent-lib representations
pipeline from handling `source-card` stages.
Why touch the app DB here (when the rest of the MP resolver is warehouse-only)?
Cards live in the application DB independently of whose warehouse the metadata
provider points at, and the lib metadata protocol's `metadatas` filter does
not support `entity_id` \u2014 a proper implementation would need a new metadata
spec. A one-row DB hit by entity_id is fine in practice.
Cross-database guard: if the resolved card's `database_id` does not match the
metadata provider's current DB, we throw a clear `:agent-error?` with code
`:cross-database-card` so the LLM can recover. Not-found likewise surfaces
as `:agent-error?` with code `:unknown-card`.
Other models (`Segment`, `Metric`, `NativeQuerySnippet`, etc.) continue to
throw `:not-implemented-yet`.
Tests:
- happy path (with-temp Card in current DB) for both 'Card and :model/Card
- nil input returns nil
- unknown entity_id surfaces :unknown-card agent-error
- card belonging to a different database surfaces :cross-database-card
- non-Card models still throw :not-implemented-yet
Totals: mp-test 11\u219215 / 36\u219249 assertions; regressions clean (repair 50/98,
construct 20/63).
* entity_details: expose portable_entity_id for saved questions and models (repr-plan step 11.2)
The agent uses entity_details to discover saved questions and models it can
reference in a representations query via `source-card:`. Previously the
response carried the numeric id but not the portable entity_id, so the LLM
had nothing to put into `source-card:` \u2014 it would guess `card__<id>` or
the numeric id, both of which fail serdes resolution.
This adds `:portable_entity_id` to the card-details payload (both `question`
and `model` variants), analogous to `:portable_fk` on table details.
Tests: +2 (question and model), both use `with-temp Card` and assert the
payload carries the exact entity_id.
* agent-lib: source-card end-to-end (repr-plan step 11.3)
With the MP resolver handling `import-fk` for Card (step 11.1) and
`entity_details` exposing `portable_entity_id` (step 11.2) in place, this
commit wires `source-card:` into the representations pipeline end-to-end.
Representations schema:
- added `source-card :string` to the stage map. Invalid types (e.g. integer)
are rejected at validation with the standard `:agent-error?` relay.
Repair pipeline:
- new **Pass 5.5 `infer-source-card-field-types*`** stamps `base-type` /
`effective-type` onto `[field, opts, "<col>"]` refs in a stage whose
source is a `source-card:` card. Works by mini-resolving a bare
`{source-card: <eid>}` stage, calling `lib/returned-columns`, and
indexing the results by column name \u2014 parallel to Pass 5 for multi-stage
queries. Best-effort no-op when mp is nil or lookup fails, so the real
error surfaces at resolve/validate time.
- updated the top-level `repair` docstring to describe Pass 5.5 and the
multi-pass `mp`-dependency contract.
Prompt (`construct_notebook_query.md`):
- new "Querying a saved question or model" section under "Examples for
each MVP operation" with a filter-and-limit example, rules for column
references by output name (same shape as cross-stage refs), mutual
exclusivity with `source-table`, and the same-database constraint.
- "Multi-stage queries" updated to mention `source-card:` as an
alternative first-stage source.
- removed "source-card" from the Phase-1-not-yet-supported list
(which is now empty).
Tests (E2E):
- happy path: `source-card: <21-char NanoID>` resolves to numeric id 500;
field reference `[field, {}, "TOTAL"]` gets `base-type :type/Float`
stamped from the card's result-metadata.
- unknown entity_id (valid shape) surfaces `:agent-error?` with code
`:unknown-card`.
- schema rejects non-string `source-card` value (integer) with an agent-
error.
Totals: construct 20\u219223 / 63\u219271 assertions; regressions all green
(repair 50/98, mp 15/49, entity-details 17/71). Kondo 0/0 on touched files.
Dev-workflow note: Metabase entity_ids are 21-char NanoIDs
(`#"^[A-Za-z0-9_-]{21}$"`). The serdes import resolver's
`{:source-card (id :guard portable-id?)}` pattern only fires when the
id has that shape; malformed ids pass through untouched and blow up at
schema validation (`pos-int?`). Tests that exercise the `:unknown-card`
path must therefore use a valid-shape NanoID that simply doesn't exist,
not a short fake.
* agent-lib: harden source-card prompt + :unknown-card remediation message (repr-plan step 11 follow-up)
Real-world test revealed two cooperating problems:
1. The LLM confidently invents a `source-card:` entity id when it already
knows the card's numeric id from a prior tool call, rather than calling
`entity_details` first. This produces `:unknown-card` with a technically-
correct but un-actionable message ("No saved question or model found
with entity_id X").
2. When the tool rejects the invented id, the agent falls back to a native-
SQL workaround like `SELECT * FROM {{#175-orders-count}} AS orders_count`,
which defeats the point of the representations pipeline.
Fix:
- **Prompt** (`construct_notebook_query.md`): the "Querying a saved question
or model" section now opens with a mandatory three-step workflow (call
entity_details \u2192 copy portable_entity_id verbatim \u2192 reference fields by
their `name`). Explicitly bans inventing, constructing, abbreviating, or
pattern-matching entity_ids, and bans `SELECT ... {{#<id>-<slug>}}` as a
fallback. Added a corresponding one-liner in "Rules and common mistakes"
for repeated exposure.
- **Error message** for `:unknown-card`: now tells the LLM exactly how to
recover \u2014 call `entity_details` with `entity-type: question` / `model`
and the numeric id, then copy `portable_entity_id` verbatim. Matches the
prompt guidance so the agent can self-correct without further hints.
No test impact: existing tests assert on ex-data (`:error :unknown-card`,
`:agent-error? true`), not on the user-facing message text. Totals unchanged:
mp 15/49, construct 23/71, repair 50/98. Kondo clean.
* metabot: expose portable_entity_id on card/model results to unblock source-card (repr-plan step 11 follow-up)
Even with a strict prompt and a helpful `:unknown-card` error message, the
LLM kept inventing entity_ids when asked to reuse a saved question as a
`source-card:` source. Root cause: the tool responses the LLM consults
to identify cards (`search`, `read_resource` for `metabase://question/<id>`
and `metabase://model/<id>`) never actually showed the card's entity_id.
So the agent had a numeric id and a name, but no way to derive the 21-char
NanoID \u2014 and defaulted to fabrication or native-SQL `{{#id-slug}}`
workarounds.
This commit plumbs `portable_entity_id` through to those response shapes
so the agent always has it one copy-paste away from where it needs it.
- **`search` tool** (`metabase.metabot.tools.search`): new
`enrich-with-portable-entity-ids` post-processing step that, for
`question` and `model` results, looks up the Card's `entity_id` in bulk
(one `t2/select-pk->fn` keyed by id) and attaches it as
`:portable_entity_id`. Tables/dashboards/metrics/transforms are left
alone \u2014 `source-card:` only accepts cards. Bulk lookup keeps search
overhead flat regardless of how many cards come back.
- **XML rendering** (`metabase.metabot.tools.shared.llm-representations`
+ `resources/metabot/prompts/llm_representations.selmer`): threaded
`portable_entity_id` into
- `search-result->xml` (rendered as a `portable_entity_id="..."`
attribute on the result tag, emitted only when present so unrelated
result types stay unchanged)
- `question->xml` (attribute on `<metabase_question>`)
- `model->xml` (attribute on `<metabase-model>`)
`entity_details`' `card-details` already exposed `:portable_entity_id`
in its structured-output (step 11.2) \u2014 it just wasn't reaching the XML
the LLM actually reads.
- **Prompt** (`construct_notebook_query.md`): updated the mandatory
workflow under "Querying a saved question or model" to reflect that
both `search` and `read_resource` now carry `portable_entity_id`
directly, so the common path is to reuse it from the already-loaded
search response rather than calling `entity_details` again.
Tests:
- `enrich-with-portable-entity-ids-test` (search_test): verifies cards
(question and model variants) pick up `:portable_entity_id`, and that
dashboards do not.
- New `testing` blocks in `question->xml-test` and `model->xml-test`:
attribute is emitted when present, omitted when absent.
- `search-result->xml-test` extended with four blocks covering question,
model, card-without-eid, and table (which must never carry the
attribute).
Totals: search-test 7\u21928 / 35\u219245, llm-representations-test 19\u219220 /
220\u2192228. All regressions green (mp 15/49, entity-details 17/71, repair
50/98, construct 23/71). Kondo clean.
Dev caveat: Selmer templates are loaded once via
`metabase.metabot.agent.prompts/template-cache`. When iterating on
`llm_representations.selmer` in a running REPL, call
`(metabase.metabot.agent.prompts/clear-cache!)` to pick up edits \u2014
otherwise the old template shape lingers and the new attributes appear to
be silently dropped.
* repair: unwrap nested [field opts [field inner-opts target]] clauses
LLMs occasionally write a field clause wrapping another field clause, e.g.
["field" {"temporal-unit" "month"}
["field" {} ["Sample" "PUBLIC" "ACCOUNTS" "CANCELED_AT"]]]
reasoning as "apply granularity to this existing field." The intended MBQL
is a single field clause with the options merged. Without repair, the nested
shape caused FK resolution to fail loudly ("Portable field FK ... must have
at least 4 elements"), since the inner [field ...] clause was treated as an
FK path.
Add Pass 1.7 (normalize-shape stage) that collapses nests of any depth into
one flat [field <merged-opts> <target>] clause. Outer options win on key
conflicts (outer is what the LLM thought it was adding). Works for FK
targets and string-column (cross-stage) targets alike.
* metabot: eliminate metric/table/field hallucination surfaces (repr-plan)
Sweeping set of fixes driven by benchmark telemetry — LLM kept inventing
schemas, tables and fields because the payloads it saw did not carry the
identity data it needed. Moves the contract from 'LLM infers from
vibes' to 'LLM copies the portable path verbatim' and adds targeted
agent-facing errors for the remaining slips.
Benchmark result on NLQ Metrics - Categorical Filters: 96% score /
78% passing (up from systemic construct_notebook_query failures);
remaining 2 failures are judge-semantic, not tool-level.
Contract shape (M2, M3, V1, Important A)
- Remove top-level `database:` from the LLM-facing query contract; the
repair layer stamps it internally from source-table[0] / source-card,
so the LLM only has to anchor one FK path and cannot contradict itself.
- `<metric>` tags gain `base_table_fully_qualified_name`, wired through
`enrich-with-metric-base-tables` on search results and directly out of
`metric-details` for read_resource. LLM now knows which table to put
in `source-table:` when consuming a metric.
- `<field>` tags gain `fk_target_fully_qualified_name` whenever the
column carries an FK. Closes the 'write customer.email on the facts
table' class of hallucinations.
- `<table>`, `<metric>`, `<search-result>` tags carry `database_name`
consistently so cross-DB portable FKs never have to be guessed.
- `portable_entity_id` surfaced on card/model/metric details.
metric-details dual-shape fix
- `metric-details` is called with two shapes of `card`: a t2 row
(snake_case) from `get-card`, and a `lib.metadata/card` kebab-case
SnakeHatingMap forwarded by `convert-metric`. Earlier code read
`(:dataset-query card) -> :query -> :source-table` which was
broken twice over (wrong case, legacy MBQL-3 shape); and reading
`:table_id` on the kebab map threw deprecation errors that surfaced
to the LLM as empty `<resource> **Error:**` blocks. Now accepts both
`:table-id`/`:table_id` and `:entity-id`/`:entity_id` and pulls the
base table off `report_card.table_id` directly.
Targeted agent-facing errors (F, F.2, FK-hints)
- `find-table` in resolve/mp now distinguishes `:unknown-schema` (with
available schemas), `:unknown-table` wrong-schema variant (lists the
schemas where the name exists), and `:unknown-table` fuzzy variant
using token-level Jaccard with a same-schema boost (handles
`fct_cards -> int_brex_card_dim` via the shared `card` token).
- `find-field` now surfaces FK-reachable candidates when the column
name exists on an outbound FK target: 'No column "name" on table
X. It exists on FK-linked tables: [Analytics, schema, target, name].'
The LLM copies the path and the implicit-join path takes over.
- All agent-facing errors tagged `:agent-error? true` so the tool
wrapper logs them at debug instead of error-with-stacktrace.
uri-in-source-table fail-fast (M3)
- Detect `source-table: "metabase://..."` / metric URIs early and
return a targeted hint pointing at read_resource + the right
base-table path, instead of a generic validation failure.
Prompt updates (construct_notebook_query.md)
- 'Using a metric' rewritten: explicit 'do not invent schema/table
names; copy base_table_fully_qualified_name verbatim'.
- 'Implicit joins' tip rewritten around the new
`fk_target_fully_qualified_name` attribute with a concrete
customerio example.
Log noise (construct.clj)
- Split the tool-level catch by `:agent-error?`: expected agent
feedback logs at debug with no stacktrace; real errors keep
`log/error e`.
Tests
- New / updated: metric base-table enrichment, metric XML FQN, field
XML FK attribute, entity-details base-table regression,
construct representation error hints, repair unwrap coverage,
schema/table/field resolver error tiers + FK-candidate hints.
142 tests / 623 assertions green.
Files:
- resources/metabot/prompts/{llm_representations.selmer,tools/construct_notebook_query.md}
- src/metabase/agent_lib/representations{,/repair}.clj
- src/metabase/metabot/tools/{construct,entity_details,search,util}.clj
- src/metabase/metabot/tools/shared/llm_representations.clj
- src/metabase/models/serialization/resolve/mp.clj
- matching tests under test/
* Slim down LLM entity representations prompt
- Drop redundant attributes from rendered XML (database_name where
database_id/engine already present, portable_entity_id, FK target
FQN, base_table FQN in metric/search).
- Remove human-prose definition descriptions for measures/segments;
keep the formal <definition> only.
- Remove standalone <collection> rendering and the question fields
block to reduce prompt noise.
- Rename segment_id attribute to id for consistency with other tags.
* Pass resolver explicitly to import-mbql
Use the 2-arity `import-mbql` form instead of `binding` the dynamic
`*import-resolver*`. The dynamic var is intended for the serdes-import
middleware stack, where the resolver is threaded through many internal
calls. Here we have a single, local call site; passing the resolver
as an argument is simpler, easier to test, and avoids relying on
thread-local state.
* Split asset and metadata lookups in serdes mp resolver
The metadata-provider-backed resolver had two implicit responsibilities:
warehouse-schema lookups (databases, tables, fields) backed by the
`MetadataProvider`, and Metabase-content lookups (cards by entity_id)
that quietly went through `serdes/lookup-by-id` and therefore required
the application database.
Make that split explicit by introducing a small `ContentStore`
protocol with a default app-DB-backed implementation. `import-resolver`
now takes an optional `content-store` as its second arg; the 1-arity
form preserves today's behavior. This unlocks reusing the resolver in
contexts without an app DB (the serdes checker, in-memory fixtures,
snapshots) without further refactoring.
Add tests exercising a custom in-memory content store for the happy
path, the unknown-card branch, and the cross-database-card branch.
* Honor extended source-field variants in implicit-join repair
The implicit-join repair pass now leaves any field clause whose options
already carry one of the disambiguating keys alone:
* source-field (already supported)
* source-field-name (new) - LLM-authored implicit join from a
previous stage's output column
* source-field-join-alias (new) - LLM-authored implicit join where
the FK-bearing column lives on an
explicitly-joined table
* join-alias (already supported)
We do not auto-fill the two new variants - filling them in correctly
would require resolving the previous stage's returned-columns or an
explicit-join's FK shape, which is out of scope here. The resolver and
`lib.normalize` already pass these strings through unchanged, so the
remaining work is purely "don't second-guess what the LLM authored".
Tests: two new repair-pass leave-alone tests, plus an end-to-end
resolve-test that asserts both keys propagate through to pMBQL with the
expected types (string for the names, numeric id for any
co-existing portable `source-field`).
Prompt: a single rules entry under "Implicit joins" describing each
variant in one line; no extra examples.
* Migrate /v2/construct-query and /v2/query to representations YAML
Both endpoints now accept a flat `{\"query\": \"<yaml-string>\"}` body in the
canonical representations YAML format, the same format already used by the
`construct_notebook_query` / `document_construct_model_chart` /
`slackbot_query` agent tools (after step 14). The old sexp-style
`{source, operations}` envelope is gone.
Why
- The YAML is fully self-describing: the database is derived from the first
stage's `source-table:` / `source-card:`, all field references are
portable FKs, and there is no auxiliary `source_entity` /
`referenced_entities` envelope (consistent with repr-plan step 13).
- Single source of truth for the query format reference: the prompt
`resources/metabot/prompts/tools/construct_notebook_query.md` documents
the YAML for the LLM and now also for the HTTP endpoint.
- MCP-friendly: the input schema stays a flat `:string` at the root, so
inputSchema flattening for the MCP `construct_query` / `query` tools
Just Works without any `:and`/`:fn` wrappers.
Endpoint changes (`agent_api/api.clj`)
- `::program-request` / `agent-lib/program-schema` re-export gone; the
`metabase.agent-lib.core` alias is unused and removed from the require.
- New `::construct-query-request` = `{:query NonBlankString}`.
- `::query-request` multi-dispatch: `{continuation_token | query}` (tag
rename `:program` -> `:fresh`).
- New `evaluate-yaml-to-live-query` / `evaluate-yaml-for-execution` thin
wrappers around `metabot-construct/execute-representations-query` (the
pipeline already raises `:agent-error?` ex-data on input failures, which
the exception middleware surfaces as 400s).
Tests (`agent_api/api_test.clj` + `mcp/api_test.clj`)
- New `orders-yaml` / `orders-field-ref` helpers build a representations
YAML payload against the live application database, keeping per-test
bodies compact.
- All previously-program-shaped tests rewritten as YAML-shaped tests with
the same coverage: simple table query, limit handling, count
aggregation, filter clause, metric-via-aggregation reference,
single-page combined query, continuation-token pagination, per-page cap.
- 404-for-unknown-table replaced by 400-with-`:unknown-table`-ex-data
(the new agent-error path; the old 404 was a side effect of the pre-repr
source-entity fetch).
- New `construct-query-rejects-empty-query-test` covers schema-level
rejection of missing/blank `:query`.
Documentation (`reference.md`)
- The big sexp-program section (\~250 lines covering operators, references,
filter operators, aggregation operators, temporal helpers, four worked
examples) is replaced by a thin description of the wire protocol plus a
pointer to the prompt for the format reference. The reference no longer
duplicates information from the prompt; the prompt is now the single
source of truth.
- Error-response table reflects the actual error keys the repr pipeline
raises (`unknown-database`, `unknown-table`, `unknown-field`,
`unknown-card`, `missing-source-in-first-stage`, `ambiguous-fk` /
`no-fk-path`, `uri-in-source-table`).
Tests run (clojure-eval): 79 tests / 376 assertions across
`agent_api.api-test`, `mcp.api-test`, and the construct-representations
tool-test suite. All green; clj-kondo 0/0 on the four touched files.
The legacy sexp-pipeline scaffolding in `metabase.metabot.tools.construct`
(`execute-program`, `program-source->source-entity`,
`build-evaluation-context`, `source-entity->model-str`,
`resolve-source-database-id`) is now unused everywhere; deletion is its own
follow-up commit.
* Delete now-unused legacy sexp-pipeline scaffolding from metabot.tools.construct
After step 15 migrated the HTTP /v2/construct-query and /v2/query
endpoints to the representations YAML pipeline, the sexp-program bridge
inside `metabot.tools.construct` has no callers anywhere. Dropping it
removes ~120 lines of code and the last in-tree path that resolves a
database from a single `source_entity.id` rather than from the YAML.
Removed:
* `resolve-source-database-id` - source_entity -> db-id lookup
* `source-entity->model-str` - metabot-string -> agent-lib-string mapper
* `program-source->source-entity` - HTTP source -> sexp source-entity
* `source-metadata-for` - lib metadata for source entity
* `surrounding-tables-for` - visible-columns -> surrounding tables
* `available-measure-ids` - measures advertised by a source entity
* `build-evaluation-context` - assembles the EvaluationContext
* `execute-program` - sexp-in-array entry point
Required namespace aliases that became unused are dropped too:
* `metabase.agent-lib.core` (only `agent-lib/evaluate-program`)
* `metabase.lib.metadata` (only `source-metadata-for`)
Verification: a repo-wide grep for each removed symbol returns no hits
outside `construct.clj`. Tests run via clojure-eval (177 tests / 692
assertions across agent_api.api-test, mcp.api-test,
construct-representations-test, slackbot-query-test, document-test, the
three agent-lib repr test namespaces, and serialization resolve.mp-test)
all green; clj-kondo 0/0 on the touched file.
Note: the underlying agent-lib sexp pipeline (`metabase.agent-lib.core`,
`agent-lib.schema`, `agent-lib.eval/*`, `agent-lib.mbql-integration/*`,
`agent-lib.repair.*`, etc.) is still present and referenced by its own
internal tests \u2014 that deletion is repr-plan step 16.
* Delete agent-lib sexp pipeline (repr-plan step 16)
After steps 14 and 15 migrated every consumer (slackbot, document tools,
construct_notebook_query, the /v2/construct-query and /v2/query HTTP
endpoints, and their MCP equivalents) to the representations YAML pipeline,
the entire structured-program ("sexp-in-array") side of agent-lib has no
remaining callers anywhere in the tree. This commit deletes it.
Verified absence of external callers
- `grep -rn 'metabase.agent-lib' src/ test/ --include='*.clj*'` outside of
`agent_lib/` itself returns only the three repr requires in
`metabot/tools/construct.clj` and one prose mention in an api-test
docstring.
- Inside agent-lib, no non-representations namespace is required by
`representations*` (the parser, repair, and resolver depend only on
`metabase.lib.*`, `metabase.models.serialization.resolve*`, and stdlib
helpers).
Removed (78 files, 7581 lines)
- src/metabase/agent_lib/{core,syntax,refs,join_spec}.clj
- src/metabase/agent_lib/{schema,schema/top_level}.clj
- src/metabase/agent_lib/eval{,/{args,invoke,source,walker}}.clj
- src/metabase/agent_lib/mbql_integration{,/{common,field_resolution,fields,joins,orderables}}.clj
- src/metabase/agent_lib/repair{,/{normalize{,/{forms,top_level}},context{,/passes},stages}}.clj
- src/metabase/agent_lib/runtime{,/{bindings,fields,lookup,transforms}}.clj
- src/metabase/agent_lib/validate{,/{context,cross_checks,operators,program,semantic,walker}}.clj
- src/metabase/agent_lib/capabilities{,/{derived,synthetic,catalog{,/{aggregation,expressions,filtering,joins,ordering,sources,top_level}}}}.clj
- src/metabase/agent_lib/common/{coercions,context,errors,literals}.clj
- All matching tests, the agent-lib test_util, plus fuzz_test and guards_test.
Kept (the entire repr surface)
- src/metabase/agent_lib/representations.clj (parse + structural validation)
- src/metabase/agent_lib/representations/repair.clj (idempotent shape passes + implicit joins)
- src/metabase/agent_lib/representations/resolve.clj (portable -> numeric ID -> normalized pMBQL)
- The three matching test namespaces.
Module-graph adjustments (.clj-kondo/config/modules/config.edn)
- `agent-lib :api`: drop `metabase.agent-lib.core` (no longer exists). The
repr namespaces remain the public API.
- `agent-api :uses`: drop `agent-lib`. After step 15 `agent_api.api` no
longer requires anything from `metabase.agent-lib`; it goes through
`metabot.tools.construct` instead, which itself is the sole user of
`metabase.agent-lib.representations*`.
- `metabot :uses` still includes `agent-lib` (for the repr namespaces) -
unchanged.
- `lib :friends` still includes `agent-lib` because the resolver still
reaches into `metabase.lib.normalize` / `:metabase.lib.schema/query` -
unchanged.
Verification (clojure-eval against fresh REPL on :49319)
- 181 tests / 719 assertions across:
metabase.agent-lib.representations-test
metabase.agent-lib.representations.repair-test
metabase.agent-lib.representations.resolve-test
metabase.agent-api.api-test
metabase.mcp.api-test
metabase.metabot.tools.construct-representations-test
metabase.metabot.tools.slackbot-query-test
metabase.metabot.tools.document-test
metabase.models.serialization.resolve.mp-test
All green.
- clj-kondo on src/metabase/agent_lib, src/metabase/agent_api,
src/metabase/metabot/tools/construct.clj, src/metabase/mcp,
test/metabase/agent_lib, test/metabase/agent_api, test/metabase/mcp:
0 errors, 0 warnings.
The agent-lib module is now lean and representations-native, as called
for in repr-plan.md "Phase 3 - cleanup and polish" step 16. Steps 17-20
(rename llm-representations -> llm-shape, normalize `:query-content` to
post-repair YAML, prompt sweep, optional /v1 endpoint cleanup) remain.
* Rename llm-representations -> llm-shape (output-side XML formatters)
Mechanical rename per repr-plan step 17.
Why
----
There are two distinct "representations" in the metabot:
- **Input-side**: the canonical MBQL 5 YAML format that the LLM authors
when calling `construct_notebook_query` / `/v2/construct-query`. Owned
by `metabase.agent-lib.representations.*` and the `construct_notebook_query.md`
prompt.
- **Output-side**: the XML shape that we render entity payloads (tables,
fields, metrics, search results, query metadata, ...) into when handing
them back to the LLM as tool results / context. Owned by
`metabase.metabot.tools.shared.llm-representations` and the
`llm_representations.selmer` template.
Calling both "representations" was already confusing pre-PR; after this
PR's heavy work on the input-side format the symmetry became actively
misleading in code review. This commit renames the output-side surface to
`llm-shape` so the two never get conflated again.
What changed
------------
File renames (git-mv-detected):
src/metabase/metabot/tools/shared/llm_representations.clj
-> src/metabase/metabot/tools/shared/llm_shape.clj
test/metabase/metabot/tools/shared/llm_representations_test.clj
-> test/metabase/metabot/tools/shared/llm_shape_test.clj
resources/metabot/prompts/llm_representations.selmer
-> resources/metabot/prompts/llm_shape.selmer
Symbol updates (no behaviour change):
- ns: `metabase.metabot.tools.shared.llm-representations`
-> `metabase.metabot.tools.shared.llm-shape`
- alias: `llm-rep` (kept) - the human-readable mental model is still
"render to LLM-facing shape", we just decoupled the *name of the
shape* from the input-side representations name.
- cache key + loader fn names: `llm-representations` -> `llm-shape`
in `metabase.metabot.agent.prompts`.
- private `llm-template-name` constant points at the renamed selmer file.
- one stale prose comment in `agent/user_context.clj`.
Updated alias requires across consumers (no per-file logic change):
- src/metabase/metabot/tools/{construct,resources,sql,metadata,charts,search}.clj
- src/metabase/metabot/agent/{prompts,user_context}.clj
- the lone test that still imported the old alias
(test/metabase/metabot/agent/user_context_test.clj).
Verification
------------
- clj-kondo on src/metabase/metabot, src/metabase/agent_api, src/metabase/mcp
and matching test trees: 0 errors / 0 warnings.
- Targeted tests run via clj-nrepl-eval against a freshly-loaded REPL:
168 / 738 across construct-representations-test, slackbot-query-test,
document-test, agent.user-context-test, agent_api.api-test, mcp.api-test,
agent_lib.representations.{repair,resolve}-test - all green.
- `llm-shape-test` itself has 20 pre-existing failures unrelated to this
rename: I confirmed this by stashing the rename and running the old
`llm-representations-test` against the same HEAD - same 20 fails.
Bisecting against `master` shows the regression dates back to commits
`d47d35b7bec` ("Slim down LLM entity representations prompt") and / or
the test-restoration commit `87fe407f5f`. The expectations in those
restored tests assume an older (pre-slim) selmer template. They will be
refreshed in repr-plan step 19 ("Final prompt cleanup") rather than as
part of this rename, to keep this commit a pure mechanical rename.
* Emit post-repair representations YAML as :query-content (repr-plan step 18)
What changed
============
`structured->query-data` (in `metabase.metabot.tools.construct`) used to
JSON-encode the *legacy MBQL* form of a constructed query and put that JSON
in `:query-content`. That was a hold-over from the sexp pipeline: the LLM
authored the query in one shape (sexp or YAML) and saw it back in a third
(legacy MBQL JSON), with no overlap to the field paths / aggregation refs
it had just typed. On the next turn it had to re-derive everything.
This commit makes `:query-content` the **post-repair canonical
representations YAML** instead - i.e. the same string-keyed portable shape
the LLM submitted, but with all of the repair-pass normalizations applied:
* empty `{}` options stamped on every clause that was missing one;
* `lib/type` markers added to the query and to every stage;
* `source-field` auto-wired on implicit-join field clauses;
* `@agg-N` placeholder references resolved to UUID form;
* inline order-by aggregations rewritten to aggregation-refs;
* top-level `database:` rewritten to the metadata provider's actual name.
The LLM now sees exactly the normalized form of what it wrote, byte-for-byte
ready to be referenced on the next turn.
Implementation
==============
1. `execute-representations-query` is restructured so the repaired-and-
validated parsed form is held in a local before resolution. The previous
`->>` chain (parse / repair / validate / resolve) folded the repaired
value back into the resolver immediately and discarded it; we now keep it
so we can serialize it.
2. The repaired value is YAML-encoded with
`{:flow-style :block}` (multi-line, human-readable - matches what the
LLM authors and what `construct_notebook_query.md` shows in examples).
3. The serialized string is added to `:structured-output` as `:query-yaml`,
alongside the existing `:query` (resolved pMBQL) and `:query-id` keys.
4. `structured->query-data` consumes `:query-yaml` and puts it in
`:query-content`. The legacy-MBQL JSON path is gone (no behavioural
reason to keep two encodings; the wire shape stays a single XML attribute).
5. `metabase.util.json` is no longer used here; replaced with
`metabase.util.yaml` in the require block.
Test (`construct-representations-test/happy-path-test`)
========================================================
The existing happy-path test grew an extra `testing` block that:
* asserts `:query-yaml` is a string in the structured output;
* round-trips it through `yaml/parse-string ... :keywords false` (matching
`repr/parse-yaml`'s convention) and verifies the parsed shape is the
portable string-keyed form (`"lib/type" -> "mbql/query"`,
portable FK `["Sample", "PUBLIC", "ORDERS"]` for the source-table) -
i.e. *not* the resolved numeric-id pMBQL;
* verifies idempotency: feeding the emitted YAML back to
`execute-representations-query` yields a structured output whose
`:query-yaml` is byte-equal to the first one. This is the property
that lets the LLM use the previous turn's response unchanged.
Verification
============
- 24 tests / 94 assertions in `construct-representations-test` - all green.
- 168 tests / 742 assertions across the wider repr-touching suite
(slackbot-query-test, document-test, agent.user-context-test,
agent_api.api…
Update generated docs (master) Co-authored-by: github-actions <github-actions@github.com>
* fix table/field creation in serdes * tests * fix * fix * fix the test * fix * fix
* Enable cljfmt fixing comment alignment * Align bb cljfmt version * Revert cljfmt version bump for now * Update with latest master
* handle duplicate copies of gson on classpath fixes metabase#73736 The relevant error message for us: ``` java.lang.NoSuchMethodError: 'com.google.gson.stream.JsonWriter com.google.gson.stream.JsonWriter.value(float)' com.google.api.client.json.gson.GsonGenerator.writeNumber(GsonGenerator.java:105) com.google.cloud.bigquery.BigQueryRetryAlgorithm.getErrorDescFromResponse(BigQueryRetryAlgorithm.java:227) com.google.cloud.bigquery.BigQueryRetryAlgorithm.shouldRetryBasedOnBigQueryRetryConfig(BigQueryRetryAlgorithm.java:115) com.google.cloud.bigquery.BigQueryRetryHelper.run(...:108) driver.bigquery_cloud_sdk$execute_bigquery(bigquery_cloud_sdk.clj:751) ``` ```clojure user=> (enumeration-seq (.. (Thread/currentThread) getContextClassLoader (getResources "com/google/gson/stream/JsonWriter.class"))) (#object[java.net.URL "0x3bc8ea4f" "jar:file:/Users/dan/.m2/repository/com/google/code/gson/gson/2.12.1/gson-2.12.1.jar!/com/google/gson/stream/JsonWriter.class"] #object[java.net.URL "0x50073d1c" "jar:file:/Users/dan/.m2/repository/com/vertica/jdbc/vertica-jdbc/23.4.0-0/vertica-jdbc-23.4.0-0.jar!/com/google/gson/stream/JsonWriter.class"]) ``` - Vertica bundles: gson 2.8.9 (vendored, unshaded) - BigQuery demands: gson 2.12.1 (via com.google.code.gson/gson in deps tree — needs JsonWriter.value(float) which was added in 2.9.0) contents of vertica jar: 71 com/vertica/io 70 com/google/gson/internal/bind 61 com/vertica/utilities/conversion 188 gson class files bundled inside vertica-jdbc, vs ~600 Vertica classes. They just vendored the entire gson library unshaded into their fat JAR. Only gson — nothing else third-party. So mystery solved. We had two copies of gson and vertica could slip through. Our fix is to ensure that we only accept (or overwrite) when it comes from a gson lib ```clojure (def ^:private gson-conflict-handler "vertica-jdbc (and potentially other fat JARs) bundle their own copy of gson classes. When these overwrite the correct version from com.google.code.gson/gson, BigQuery's error-handling path crashes with NoSuchMethodError on JsonWriter.value(float), introduced in gson 2.9.0. This handler ensures the pinned gson version always wins regardless of JAR processing order. See metabase#73736." {"com/google/gson/.*" (fn [{:keys [lib path in]}] (if (= lib 'com.google.code.gson/gson) {:write {path {:stream in}}} nil))}) ``` * add activation conflict handler as well ```clojure uberjar-test=> (clojure.test/run-tests) Testing build.uberjar-test Clean Delete /Users/dan/projects/work/metabase/target/classes Delete /Users/dan/projects/work/metabase/target/uberjar/metabase.jar FAIL in (class-file-conflicts-test) (uberjar_test.clj:55) No unexpected class file conflicts in the EE uberjar Unexpected class file conflicts in packages: #{"javax/activation"} If benign, add to known-conflicting-prefixes with a comment. If dangerous, add a conflict handler in build.uberjar. expected: (empty? unexpected) actual: (not (empty? #{"javax/activation"})) Ran 1 tests containing 1 assertions. 1 failures, 0 errors. {:test 1, :pass 0, :fail 1, :error 0, :type :summary} ``` Here's the output when i removed the `activation-conflict-handler` from the uberjar-er. It hits conflicts on these files. It doesn't know where the conflicts come from. The api is aware of the lib trying to write and there's already a conflict. So the order isn't stable and we must match on the files being written, not the lib writing. Because lib A might write first so lib B tries to write the same class we think it's conflict on B. But then B is written first and now it appears a conflict in A. also an exclusion in databricks so it doesn't bring in another logger and we will no longer get this: ``` ❯ clj -J"$(llm-repl 6006)" -M:"$ALIASES":build:build-dev:llm-repl Downloading: vlaaad/reveal/maven-metadata.xml from clojars WARNING: Specified aliases are undeclared and are not being used: [:performance] SLF4J(W): Class path contains multiple SLF4J providers. SLF4J(W): Found provider [org.apache.logging.slf4j.SLF4JServiceProvider@122ea8dc] SLF4J(W): Found provider [org.slf4j.jul.JULServiceProvider@6cb417fc] SLF4J(W): See https://www.slf4j.org/codes.html#multiple_bindings for an explanation. SLF4J(I): Actual provider is of type [org.apache.logging.slf4j.SLF4JServiceProvider@122ea8dc] ``` * fixing it up from comments
…etabase#74572) * Fix NullPointerException in render-minibar with missing range stats render-minibar assumed the column fingerprint always supplied min and max values. When a minibar-enabled column lacks :type/Number fingerprint stats (e.g. an unsynced or computed column), min and max are nil and (< min 0) throws a NullPointerException that crashes the entire Slack/email notification delivery silently. Guard against nil min/max and fall back to rendering the plain formatted value, consistent with the existing non-numeric fallback. * Isolate per-card render failures in dashboard subscriptions A single card that fails to render (e.g. the minibar NPE in metabase#74007, or any future render error) could take down an entire dashboard subscription: the per-card try/catch in render-pulse-card-body swaps in a :render-error placeholder, but render methods build the body as lazy hiccup, so an exception thrown while building it escapes that catch and only fires later when the channel realizes the hiccup (email: hiccup/html; Slack: PNG rendering) — outside any per-card boundary, aborting the whole notification. Add explicit per-part isolation at the assembly boundary, where each part is already rendered and realized one at a time: - email :notification/dashboard reduce wraps each part's render+html - slack :notification/dashboard mapcat wraps each part->sections! On failure the part degrades to the existing error placeholder (new channel.render/error-rendered-part for email; an equivalent section block for Slack) and the remaining cards still deliver. This catches any part failure, lazy or not, and leaves the happy path unchanged. * Simplify min/max nil check in render-minibar
* tests: migrate jest to swc
* fix tests and circular dependency
* fix test
* fix typecheck
* review
* tweak config
* use correct swc plugin
* test: detect memory leak
* test: detect memory leak
* test: fix multiple warnings
* revert
* revert
* revert
* Update frontend/src/metabase/querying/filters/components/FilterPicker/FilterColumnPicker/FilterColumnPicker.unit.spec.tsx
Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
* test: use synchronous act() for synchronous dispatches/blur
Address review feedback: the act() callbacks only perform synchronous
work (store.dispatch, input.blur), so the synchronous act(() => {}) form
is sufficient. dispatchLocationChange is no longer async as a result.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Use unicode regex for bigquery alias (metabase#75123) * use unicode regex for bigquery alias * trigger ci * fix workspace e2e test Co-authored-by: Riley Thompson <riley@metabase.com>
…ase#75262) GDGT-2558 move alerts management from EE to OSS (metabase#75247) * GDGT-2558 move alerts management from EE to OSS * fix build error * fix: missing feature --------- Co-authored-by: Egor Grushin <egor@metabase.com> Co-authored-by: Nicola Mometto <nicola.mometto@metabase.com>
…abase#75240) Optimize temporal expressions the same as filters (metabase#74875) * optimize expressions * rename for accuracy * group into one middleware * include a between filter in test * rename filter to clause * improve check for day bucketing, add tests * fix kondo and a test * fix test and kondo * fix test for drivers Co-authored-by: Riley Thompson <riley@metabase.com>
…r values in tables/detail fields" (metabase#75298) [GDGT-2548] chore: use shared constant to set empty cell placeholder values in tables/detail fields (metabase#75223) replace uses of em-dash empty value placeholder in fields and table cells with shared constant Co-authored-by: YB <yeowboon@metabase.com>
metabase#75299) GDGT-304: Fix unthreadsafe number parsing in csv upload (metabase#75119) Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>
…ry failures" (metabase#75306) Alerts management: failing tab includes abandoned + query failures (metabase#75189) * Alerts management: failing tab includes abandoned + query failures The failing tab filtered on last_send (channel delivery) only, so alerts that never reached delivery — query errors, or runs abandoned on instance restart — showed as "not failing" even though nobody got the alert. last_check is the whole-run rollup (success/failed/abandoned), already a superset of last_send. Add a last_check_status filter and point the failing tab at it. The explicit "last send attempt" filter still filters delivery only, so check vs send stay distinct. * Failing tab: sort by last_check and explain abandoned runs the failing tab filters on last_check but still sorted by last_send, so abandoned and query-fail alerts (which have no last_send) sank to the bottom, the exact ones it's meant to surface. default the tab to sort by last_check. abandoned runs have no task_history message, so they showed as failing with a blank reason. synthesize an explanation instead. * small nitpick --------- Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com> Co-authored-by: Egor Grushin <egor@metabase.com>
…base#75307) Remove Our Analytics from library snippet header (metabase#75266) Co-authored-by: Nick Fitzpatrick <nick@metabase.com>
…base#75308) allow collection creation when publishing tables (metabase#75261) Co-authored-by: Nick Fitzpatrick <nick@metabase.com>
…se#75316) Add Prometheus metric for search result counts (metabase#75186) Co-authored-by: Chris Truter <crisptrutski@users.noreply.github.com>
…erdes export" (metabase#75280) GHY-3801 preserve result_metadata for native cards on serdes export (metabase#75259) * GHY-3801 preserve result_metadata for native cards on serdes export Native (non-model) cards can't have their result_metadata re-derived at import time the way MBQL cards can: query->expected-cols computes MBQL columns statically from synced metadata, but a native query is opaque SQL that can only be introspected by executing it. GHY-3339 dropped result_metadata from all non-model exports on the assumption that import re-derives it; that holds for MBQL but leaves native source cards with nil metadata, which breaks downstream GUI questions that join them ("Error adding alias info to join..."). Export the full result_metadata for native non-model cards (mirroring the pre-GHY-3339 transforms that import-result-metadata already inverts), while keeping the lean model export and the MBQL non-model skip. * Whitelist exported keys + assert column names (PR review) - export-result-metadata: switch the native branch from a blacklist (dissoc :fingerprint) to an explicit keep-keys whitelist, so internal :lib/* and other computed keys aren't serialized — the bloat GHY-3339 set out to remove. Keeps the structural type info native needs plus the display soft-keys and FK transforms. - e2e test: assert the imported native card's result_metadata column names match the source, not just that metadata is non-empty. Co-authored-by: Eric Normand <ericwnormand@gmail.com>
…iver" (metabase#75312) fix: pin commons-configuration2 2.15.1 in Databricks driver (metabase#75228) databricks-jdbc-thin 3.3.1 transitively pulls commons-configuration2 2.11.0 which has CVE-2026-45205 (Uncontrolled Recursion, Medium). Fixed in 2.15.0; 2.15.1 is the latest patch on that line. Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…base#75313) fix: pin netty 4.1.135.Final in Snowflake driver (metabase#75229) AWS SDK (transitive via snowflake-jdbc-thin) pulls io.netty 4.1.126.Final which has SNYK-JAVA-IONETTY-16438930 (Data Amplification, High) and SNYK-JAVA-IONETTY-16438322 (Resource Exhaustion, High). Both fixed in 4.1.133.Final. Pin netty-codec, netty-handler, and netty-transport-classes-epoll to 4.1.135.Final to ensure the full netty family is on a consistent version. Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…port.log" (metabase#75263) GHY-3802 capture extract-phase logs in serialization export.log (metabase#75255) * GHY-3802 capture extract-phase logs in serialization export.log The serialization export endpoint hoisted `extract/extract` out of the `for-ns` log-capture block so invalid-input errors could still set a non-200 HTTP status. But escape-analysis warnings (content referencing entities outside the requested collections) are emitted eagerly during `extract`, so they were logged outside the capture and never reached `export.log` — leaving customers with a valid archive and an empty log. Capture the eager extract logs into the same buffer that becomes `export.log`, and write that buffer after the storage appender has closed (and flushed). Add a regression test asserting the escape warning lands in `export.log`. * kick ci * Rename serialization-log-nses to serialization-logger-prefixes These are log4j2 logger-name prefixes, not loaded namespaces. Clarify the name and docstring so it's obvious each prefix captures every logger nested under it. Co-authored-by: Eric Normand <ericwnormand@gmail.com>
…t-tag" (metabase#75328) feat: add embedding-sdk-tag-latest workflow for npm dist-tag (metabase#75245) * feat: add embedding-sdk-tag-latest workflow for npm dist-tag Adds a standalone reusable workflow that sets the `latest` dist-tag on a published @metabase/embedding-sdk-react version using a classic npm token (NPM_RELEASE_TOKEN). Trusted publishing (OIDC) cannot set dist-tags, so this lives in a separate workflow. Wires the new workflow into release-embedding-sdk.yml as a `tag-latest` job (disabled via `if: false`). Remove that line when the release branch becomes gold. Also removes the old commented-out inline dist-tag step. * fix: quote GITHUB_STEP_SUMMARY in embedding-sdk-tag-latest workflow * Cleanup step by not explicitly create .npmrc and rely on setup-node instead * ci: use secrets: inherit for npm token in sdk tag-latest workflow * ci: use DEFAULT_RUNNER_KEY in sdk tag-latest workflow * fix: add concurrency guard, permissions, and shell injection prevention to tag-latest workflow - Add concurrency group `npm-dist-tag-latest` (cancel-in-progress: false) to prevent parallel runs from racing - Add `permissions: {}` for least-privilege (workflow doesn't use GITHUB_TOKEN) - Hoist version to job-level env var `VERSION` to prevent shell injection via inputs.version - Add verify step to confirm version exists on npm before tagging - Quote package spec in npm commands --------- Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…tabase#75331) Enable all admin users to activate trial up to pro (metabase#75056) * refactor: enable non-store admins to go through upsells * refactor: enable non-store admins to buy transforms * fix: restore assertions Co-authored-by: Gabriel Moreno <gabriel@metabase.com>
…erministic" (metabase#75329) Make check-card dependency response order deterministic (metabase#75300) `broken-cards-response` built `:bad_cards`/`:bad_transforms` from `(keys card)` and an `IN` query with no `:order-by`, so the response order was nondeterministic. This flaked check-card-hydrates-dashboard-and-document-test, which matches `:bad_cards` positionally via `=?`. Achieve the deterministic ordering by sorting `created_at` anchored by `id`. --------- Co-authored-by: Nemanja Glumac <31325167+nemanjaglumac@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Ben Grabow <ben.grabow@metabase.com>
) Fix zoom in drill for auto binned lat/lon (metabase#75063) * Fix zoom in drill for auto binned lat/lon * Test fix 🔧 Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com>
docs - custom viz (metabase#75153) Co-authored-by: Jeff Bruemmer <jeff.bruemmer@gmail.com>
Remove failed bigquery fix attempt (metabase#75344) remove failed fix Co-authored-by: Riley Thompson <riley@metabase.com>
Clean table urls (metabase#75079) * Add Urls.table() builder for clean /table/:slug links * Route /table/:slug to the QueryBuilder as an ad-hoc question * Keep /table/:slug URL while the table question is unedited * Point table links at Urls.table for clean /table/:slug hrefs * Scope clean /table URL to the table route; update e2e * Address PR review: restrict clean /table URLs to concrete tables - Urls.table now accepts only ConcreteTableId; callers that may hold a virtual id (Saved Questions virtual DB, card__<id>) fall back to the ad-hoc question / tableRowsQuery URL: TableBrowser, detail-view nav, search results, notebook data picker, and metric source-table links. - updateUrl's /table-route check is now basename-aware so the canonical URL behavior works under subpath deployments. - Drop the virtual-id (card__1) URL test that enshrined an unroutable URL. * Fix CI: keep ad-hoc URL for modelToUrl table links modelToUrl serves collection/search/library contexts where a user may only have indirect (published-table) access; the /table/:slug route needs direct table metadata and 403s for those users (broke the data-studio table-collection-permissions e2e). Revert that one call site to tableRowsQuery. Update the InfoText unit test for the search-result table link, which uses Urls.table directly. --------- Co-authored-by: Romeo Van Snick <romeo@romeovansnick.be> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…urrency columns" (metabase#75115) Fix XLSX exports dropping the currency symbol for non-currency columns (metabase#75026) Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>
…led" (metabase#75088) Allow editing MySQL tables when partial_revokes is enabled (metabase#74573) Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>
Added new `strong-enough` password type (metabase#75104) * Added new `strong-enough` password type * Revert password policy to default normal * Numbers only test added * Misc Co-authored-by: Benoit Vinay <ben@benoitvinay.com>
Relax schema in security center (metabase#75382) security center: relax semver schema Co-authored-by: Nicola Mometto <nicola.mometto@metabase.com>
sandbox custom react components (metabase#73941) * WIP: sandbox custom react components * improve typing of widget mount * simplify widget mount module * refactor widget mount * refactor widget mount # Conflicts: # enterprise/frontend/src/metabase-enterprise/custom_viz/widget-mount.unit.spec.ts * refactor viz def settings sanitizer * improve types * cleanup naming * adjust readme and docs * adjust docs * use single generic mount to render viz and settings * improve types * simplify render in defineConfig * extract use-plugin-mount * store test custom viz plugin sources * fix missing locale in custom viz fixture * check string setting widgets with allowlist * add custom viz security custom settings widget security test case * simplify widget mount stamping to plain property * use isObject helper, skip non-object values * add comment explaining rendering flow in usePluginMount * add performMount and props to explicit deps * remove obsolete archive with custom plugin * update test fixture of calendar heatmap * add react settings widget sandboxing e2e test * bump sdk version * revert .only test annotation * improve test name Co-authored-by: Jakub Chodorowicz <jakub.chodorowicz@metabase.com>
* Keep teal brand and warning colours in light.ts and dark.ts * Take v0.62.1 version for all other values
trevorgowing
force-pushed
the
sync/upstream-v0.62.1
branch
from
4dac0de to
bc73043
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upstream Sync — v0.62.1
Minor version bump from v0.61.6. Merge conflicts across ~230 files require manual resolution.
Teal files — keep Teal values
frontend/src/metabase/ui/colors/constants/themes/light.ts— keepbrand: "#135756"and thewarningcolour valuesfrontend/src/metabase/ui/colors/constants/themes/dark.ts— same as above; also take the newbackground-success-secondaryline from v0.62.1Everything else
Take the v0.62.1 (
>>>>>>> v0.62.1) side for all other conflicts. Files with no v0.62.1 version should be deleted.