34ff3598e3
* Enable the feature the unknown-parameter test needs to reach its check
`resolve-parameters` refuses any `parameters` at all without `dashboard-subscription-filters`, and
that refusal runs before the unknown-id check. The teaching-error test supplied a bad id without
enabling the feature, so whenever it ran without ambient token state the call failed on the feature
and never reached the check the test exists to pin — both assertions red, which is what CI hit.
Wrapped in `mt/with-premium-features`, matching the two sibling parameter tests, which already pin
their own token state rather than inheriting it.
Verified fails-when-broken: swapping the feature set to `#{}` fails both assertions.
Claude-Session: https://claude.ai/code/session_018PNCQxp9kCypqE3dLQS9va
* [mcp-v2 12] definitions: cover the write-scope readback degradation
Both tools echo the created/updated row, which carries its name and
definition — so a token holding only `agent:content:write` would learn
the content of anything it may touch. `v2.write/readback` degrades that
echo to an ack, and all four call sites here relied on it untested.
`readback-requires-read-scope-test` drives create and update for both
tools with the write scope alone, asserting the payload is exactly
`{id, note}` and the note names the missing read scope. It then reads the
row from the DB to show the write itself still landed, and repeats the
update with both scopes to show the full row comes back — so the
assertion cannot pass merely because the tool returned something thin.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AbCo3ybRitdVSQXYcG97QR
* [mcp-v2 12] definitions: reject a blank name
Both tools declare `name` as `[:string {:min 1}]`, which " " satisfies,
so a whitespace-only name was accepted and stored — laxer than the REST
endpoints whose checks these tools otherwise inherit, which take
`ms/NonBlankString`. The JSON schema the MCP manifest is built from
cannot express non-blank, so the check belongs in the handler, beside
the `str/blank?` check `revision_message` already gets.
`check-name!` runs in all four branches immediately after the method-args
check, before any id resolution — so a blank name on update reads as an
argument error rather than "not found".
`blank-name-test` was written first and watched fail: the create half
stored `{"id":1,"name":" ", ...}`, and the update half returned the
not-found message because the lookup preceded any name check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AbCo3ybRitdVSQXYcG97QR
* [mcp-v2 12] definitions: cover clearing a description
`clear` is the only way to unset a property — a null cannot carry that
meaning, since strict clients fill every unset property with null and the
registry strips nulls at the boundary. `dispatch-write`'s expansion is
unit-tested in `write_test`, but nothing checked that a cleared property
actually reaches the column through these two tools.
`clear-description-test` creates each entity with a description, clears
it, and asserts both that the column is null and that the key is absent
from the echo — the concise projection drops nils rather than echoing
null back.
It also pins where a non-clearable property is refused: `clear`'s schema
is `[:sequential [:enum "description"]]`, so that is rejected as an
invalid argument before `expand-clear` runs, making its own "can't be
cleared" message unreachable for these two tools.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AbCo3ybRitdVSQXYcG97QR
* MCP v2 get_parameter_values: reject date constraints chain filtering can't parse
`check-constraints!` promised to reject any constraint chain filtering would
silently drop, but covered only the three field-shaped drop paths. The fourth is
in the value: `chain-filter/add-filter` routes a string value on a temporal field
through `params.dates/date-string->filter` and catches a parse failure into
`nil`, dropping that filter and running the query unnarrowed.
Proven before fixing — a `_DATE_` constraint of "sometime last spring" against a
checkins dashboard was accepted and returned all 100 venues with
`has_more_values false`: a completely unfiltered list handed back as a filtered
one. That is the failure this tool exists to prevent, and a date string an LLM
would plausibly produce is the likeliest way to hit it.
The check runs `date-string->filter` itself, so it can't disagree with
`add-filter` about what parses — the same "probe with the real thing" property
the reachability check gets from `filterable-field-ids`. Temporal-ness is tested
as `effective_type` (falling back to `base_type`) against `:type/Temporal`,
matching `lib.types.isa/temporal?` on the column `instance->metadata` builds. It
is scoped exactly to `add-filter`'s branch — bare strings only, per field-id, so
a multi-field constraint is judged per field the way the fetch judges it. The
error names the grammar that does parse rather than only refusing.
Also fixes an adjacent misdiagnosis in the same fn: when the target parameter has
no field-ids at all, chain filtering falls back to `filter-values-from-field-refs`,
which ignores constraints outright. That was reported as "no join path to this
parameter's table", which sends the reader looking for a missing FK. It now names
the real cause.
parameters-test 32 tests / 89 assertions; :module parameters 151t / 385a;
fix-modules-config unchanged; kondo 0/0; cljfmt clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XJcw6ZamZBNhJ8SQxgJB13
* MCP v2 get_parameter_values: cover constraints and query applied together
The tool's most involved path had no test. `constraints-test` passes constraints
with no query and `query-search-test` passes a query with no constraints, so
nothing reached `chain-filter-search` with a non-empty constraint set — the one
route that also pins `chain-filter/*allow-implicit-uuid-field-remapping*` false.
The test asserts the two narrowings compose rather than merely co-exist: a
category that matches the query but is excluded by the constraint must be absent.
Searching "African" alone finds it; searching "African" under price=4 returns
nothing, which fails if the constraint is dropped once a query is present.
No behaviour change — the path was already correct. This closes the coverage hole
so it stays that way.
parameters-test 33 tests / 92 assertions; kondo 0/0; cljfmt clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XJcw6ZamZBNhJ8SQxgJB13
* MCP v2 get_parameter_values: blame the search when a query matches nothing
An empty result carried one sentence regardless of how it was reached: "its
source may be empty, filtered to nothing for you, or a free-text filter with no
value list." Every cause it offers is a property of the parameter, so after a
search that simply missed, the agent is told the filter may be broken when the
actual recovery is to search for something else — and none of the three listed
causes is even true.
`steering-line` now takes the caller's `query` and, when there was one, names it
and points at the recovery that fits: search for less, or drop `query` to list
everything. The generic sentence still covers the no-query case, where those
three causes really are the candidates.
Threading `query` into `values-content` is the only structural change; the
slicing and the other three branches are untouched.
Covered by no-match-for-query-blames-the-query-test, which asserts on both
targets that the line quotes the search and drops the causes that can't explain
it.
parameters-test 34 tests / 98 assertions; kondo 0/0; cljfmt clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XJcw6ZamZBNhJ8SQxgJB13
* Fix archived document hits being dropped from archived searches
validate-and-enrich-documents re-fetched document hits with a hardcoded
`:archived false`, so any document matched by an `archived: true` search
failed that lookup and was dropped by the `keep` — while `:total` still
counted it. The result was an empty `data` alongside a nonzero total that
no offset could ever reach.
The predicate now takes the archived state the search asked for:
`validate-and-enrich-documents` gains an `archived?` first argument and
selects `:archived (boolean archived?)`, with the call site in `search`
passing `(boolean archived)`.
Reachable only since the v2 search tool started exposing `archived`
through this pipeline; documents are archivable (`:archived true` in the
document search spec), so both `{archived: true, type: ["document"]}` and
an unscoped `{archived: true}` lost every document hit.
Test proven red before the fix (parameter added, hardcoded `false` kept):
archived hit dropped (expected [id], actual ()), and an active hit wrongly
retained for an archived search. The new test pins both directions.
Verified: metabot.tools.search-test + mcp.v2.tools.search-test +
mcp.v2.common-test 72 tests / 270 assertions / 0 failures; kondo 0/0;
cljfmt clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CF3ZXRaq6wy8sFR9L8DPKa
* Test that only a superuser's search reaches the engine for transforms
This pipeline dropped remove-unreadable-transforms and relies instead on
the transform search spec's `:visibility :superuser`, which the engine
applies when it narrows a context to its applicable models. That holds
only if the pipeline hands the engine the caller's real superuser status,
and nothing pinned that.
Captures the context passed to ranked-results and asserts `:models`
contains "transform" for an admin and never for a non-superuser, with
"dashboard" asserted present in both directions so neither can pass on an
empty set.
Proven to be a real guard by mutation: with `:is-superuser?` hardcoded to
true in the per-query context, the non-superuser assertion failed
(expected (not (contains? models "transform")), actual (not (not true)))
while the admin assertions stayed green.
Deliberately narrower than an end-to-end search: getting a transform to
appear in the index needs search.ingestion/*force-sync* plus an explicit
ingest! built from private search.engine/search.ingestion vars (see
metabase.transforms.search-test), which would couple this test to search
internals. An end-to-end attempt was discarded because its positive
control failed — the admin did not find their own transform — leaving the
non-superuser assertions vacuous. The visibility rule itself is covered by
metabase.search.filter-test; what is new here is the wiring.
Verified: metabot.tools.search-test + mcp.v2.tools.search-test +
mcp.v2.common-test 74 tests / 279 assertions / 0 failures; kondo 0/0;
cljfmt clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CF3ZXRaq6wy8sFR9L8DPKa
* Stop a caller-supplied collection-id escaping a metabot's confinement
An embedded metabot, and the nlq profile, may only search inside its own
collection. That is a containment boundary, not a default. The shared
search fn had been changed to let an explicit collection-id take
precedence over it:
collection-id (or collection-id (when confined? (:collection_id metabot)))
so a caller passing both would search wherever it asked, outside the bot's
collection. Inert today — no caller sets :collection-id, and the v1 tool
schemas are closed — but the v2 search tool fills that key straight from a
request filter, so the escape is one caller away.
Precedence inverted: the confined collection wins, and a caller-supplied
collection-id applies only when the metabot is unconfined.
Test proven red first: confined-collection-is-not-overridable-test failed
on exactly the escape case (expected the bot's collection, got the
caller's) while both controls passed. It pins all three cases — confined
with no collection-id, confined against an explicit one, and an
unconfined metabot still honouring an explicit one, so the fix cannot
over-restrict.
Trade-off: a confined caller asking for a sub-collection inside its
confinement now gets the whole confined subtree instead of the narrower
request. Intersecting would need a descendant check; no caller passes both
today, so it is not built.
Verified: search suites 75 tests / 282 assertions / 0 failures; the other
caller, metabot.agent.core-test, 23 tests / 140 assertions / 0 failures;
kondo 0/0; cljfmt clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CF3ZXRaq6wy8sFR9L8DPKa
* Build narrowing disclosures from the final type set
When `type` is omitted, each of created_by / collection_id / archived can
narrow the search to the types that support it, and each narrowing is
disclosed. Every disclosure built its "narrowed the search to ..." list as
`(set/difference effective-types (set bad))` — the full engine surface
minus only its OWN exclusions — so with two filters narrowing at once the
later message advertised types the earlier one had already removed.
With {created_by: "me", archived: true} and no type, the archived
disclosure claimed the search covered `collection` and `segment` when the
final type set excluded both. The namespace's stated contract is that
narrowing is disclosed; here it was disclosed inaccurately.
The accumulator now records {:excluded :label :because} per filter instead
of a prebuilt sentence, and the sentences are formatted after the fold
from the final narrowed set — one correct type list everywhere, each
message keeping its own reason for what it excluded.
Test proven red first: the archived disclosure failed with
expected: "action, dashboard, document, measure, metric, model, question"
actual: "action, collection, dashboard, ... question, segment"
The created_by disclosure passed, because that filter's exclusions are a
superset and its private subtraction coincided with the final set — which
is why no single-filter test could have caught this.
Single-filter wording is unchanged, so the existing
omitted-type-narrows-and-discloses-filter-incompatibility-test passes as
written.
`:label` rather than `:filter` for the key: as `:filter` it shadowed
clojure.core/filter in the formatting fn. Kondo did not flag it.
Verified: mcp.v2.tools.search-test + mcp.v2.common-test +
metabot.tools.search-test 76 tests / 288 assertions / 0 failures;
kondo 0/0; cljfmt clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CF3ZXRaq6wy8sFR9L8DPKa
* Steer an empty list page instead of returning a bare envelope
truncation-line only fires on arithmetic truncation — (offset + limit) <
total — so a page that came back empty carried no steering line at all.
An offset at or past the end produced
{"data":[],"returned":0,"total":37}
which reads as "nothing matches" when the truth is "nothing *here*", and
gave the caller no hint that paging back would help. The same silence hit
an empty first page whose every match was dropped after the count (a stale
index hit, a row gone unreadable).
list-content now emits an empty-page line whenever data is empty and total
is positive, splitting the two cases: a positive offset is told the offset
and total and steered back with a smaller `offset`; offset 0 is told the
matches are no longer readable or have been removed, and is NOT steered to
an offset, which could not help. A floored total stays "at least N".
Deliberately silent when total is 0 or unknown — that envelope already
says what happened, and inventing a line there would be noise.
truncation-line itself is untouched, so the parameters tool's own
steering-line wrapper (which handles its zero-returned case before ever
reaching truncation-line) is unaffected.
Test proven red first: the five positive assertions failed against the
bare JSON payload, while all three negative controls (total 0, unknown
total, non-empty page) passed before and after.
Verified: common + v2 search + v2 parameters + metabot search 107 tests /
377 assertions / 0 failures; kondo 0/0; cljfmt clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CF3ZXRaq6wy8sFR9L8DPKa
* Disclose the types a collection-scoped search silently drops
The namespace's contract is that the engine's silent narrowing is never
relied on. Two types slipped through it when `type` was omitted and
`collection_id` was set:
transform — its spec has `:collection-id false`, so
search-context->applicable-models drops the model outright from any
collection-scoped search. Always, on every edition.
table — the ::collection-hierarchy where-clause excludes tables when the
Library feature is absent, so OSS silently searched no tables.
Named explicitly, each was already a teaching error. With `type` omitted
both were dropped in silence, and the disclosure the caller did get named
them as in scope: "collection_id narrowed the search to ..., table,
transform — database, measure, segment don't live in collections."
Both are now narrowed and disclosed alongside the collectionless types,
each with its own reason, since they are excluded by different parts of
the engine and only one is edition-dependent.
The transform half was not in the original review note; it surfaced from
the pre-fix disclosure text while fixing the table half.
Tests proven red first: 6 assertions across the two affected deftests.
One pre-existing assertion legitimately changed — the collection_id
sub-test of omitted-type-narrows-and-discloses-filter-incompatibility-test
asserted exactly 1 disclosure and now sees 2. It is also now pinned to
#{:library}, so the count no longer depends on whichever token the suite
happens to run under.
Verified: v2 search + common + parameters + metabot search 108 tests /
386 assertions / 0 failures; kondo 0/0; cljfmt clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CF3ZXRaq6wy8sFR9L8DPKa
* Sign the MCP session capability payload so a client cannot self-assert it
`supports-mcp-ui?` is what `:required-extensions` gates on, and it read a
payload the CLIENT echoes back in its own session id. Unsigned, that is a
self-assertion: any caller could mint `<uuid>.{"v":1,"ui":true}` and claim
MCP Apps support it never advertised at `initialize` — which is what let a
narrow bearer reach `refresh_ui_credential` and mint a UI credential.
Session ids are now `<uuid>.<payload>.<signature>`. The signature covers
the uuid as well as the payload: every UI-capable session carries the
identical payload, so a payload-only signature would be a reusable token
that could be lifted onto any correlator. It is domain-separated from the
UI-credential HMAC so neither can be replayed as the other.
An unsigned or tampered id remains a VALID session. The id authenticates
nothing — it is a stateless correlator, and any well-formed one works for
its presenter, which `well-formed-but-never-issued-session-id-is-accepted-for-its-presenter-test`
pins. Only the claim inside it is disbelieved, so the gate fails closed
without breaking that property.
`supports-mcp-ui?` also stops returning true for a bare UUID. That branch
made the signature pointless — a caller could omit the payload entirely
and be believed anyway. Such ids predate capability hints, so `unknown`
is the honest reading.
Behavioural, and shared with v1: a client holding a session minted before
this change loses MCP Apps tools until it re-initializes. Bounded by
session lifetime, and it fails closed.
* fix(mcp): ignore database id for get_fields
Allow agents to carry database_id through the browse hierarchy without rejecting get_fields requests.\n\nRefs GHY-4396
* Stop stamping the MCP UI credential as an unrestricted session
The credential was stamped `::scope/unrestricted`, so the route allowlist
was the only thing between it and full session privilege. Those are two
different questions — which routes the credential may pass through, and
what privilege it arrives with — and collapsing them into one value meant
any way of reaching a route off the list (a routing change, a new alias,
a mistake in the set) served the request as the user.
It now carries `::scope/mcp-ui`, a second sentinel alongside
`::unrestricted`. It satisfies no endpoint's declared `:scope`, and
`ensure-scopes-checked` refuses it where none is declared, so the same
mistake fails closed. Being a keyword it can never be requested, granted,
or named on a consent screen. `:token-scopes-checked` is what lets the
allowlisted routes serve it, since they declare no scope of their own.
Deliberately NOT done by annotating those routes with the sentinel:
`/api/user/current`, `/api/session/properties` and the `/api/dataset`
routes live in `users-rest`, `session` and `query-processor`, and marking
them would push MCP vocabulary into three unrelated namespaces —
deepening the coupling GHY-4400 exists to remove, to buy locality in a
set currently frozen at eight entries.
The allowlist's docstring now says what it is: an inventory of what the
embedded app happens to call, because the credential is installed as
app-wide auth on the client, rather than a decision about what it should
reach.
* [mcp-v2 12] definitions: pin the table-move test's permission precondition
`table-move-permission-test` passed a solo namespace run but failed CI's
full-suite partition. `with-no-data-perms-for-all-users!` resets only the
All Users group, so the temp group's access to `checkins` was left at
whatever default a freshly created group inherits — ambient state that
differs between a namespace run and a full partition. When the analyst
could author on `checkins`, the retarget succeeded and the assertion
failed as "expected a tool error, got success".
Block the database for the group and grant `venues` back, so `checkins`
is denied by construction rather than by the absence of a grant, and
assert both halves of the precondition up front so a recurrence fails at
the setup with a clear message instead of as a confusing success.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AbCo3ybRitdVSQXYcG97QR
* Stop answering JSON-RPC notifications
A message carrying no `id` is a notification (JSON-RPC 2.0 §4.1) and must
not be answered. A known method sent without one — `{"jsonrpc": "2.0",
"method": "ping"}` — executed and then replied with `"id": null`: a
response invented for something the client is not listening on, in the
shape §5 reserves for a message that could not be parsed at all.
Notifications still execute. A notification is a request the client is
not waiting for, not one to ignore, so the dispatch is unchanged and only
the result is dropped. The malformed case keeps its null-id reply, which
is what §5 actually asks for; conflating the two is what produced the
bug.
Keyed on absence of the `id` key rather than a nil value, so a request
that explicitly sends `"id": null` is still treated as malformed rather
than silently swallowed.
* Pin the superuser flag instead of reading it off a shared test user
transform-visibility-is-superuser-only-test failed on the MySQL 9 EE and
MariaDB EE app-db shards, deterministically on both. The failure was the
POSITIVE control: the admin case saw models #{"dashboard"}, so transform
was absent for everyone and the non-superuser assertions were proving
nothing.
Cause: the test took the superuser flag from the ambient test user, and
`mt/with-test-user` resolves it through `request/with-current-user`, which
SELECTs the shared user row:
(t2/select-one [:model/User [:is_superuser :is-superuser?] ...] :id ...)
That makes the assertion depend on the state of a row other tests in the
same parallel shard also touch. This branch has already been bitten by the
same class of pollution once — recents-total-is-a-floor-test, fixed by
moving off the shared :crowberto user.
`api/*is-superuser?*` is now bound explicitly. That is the exact var
`search.filter/visible-to?` reads, and pinning it is what the assertion is
actually about: that this pipeline propagates the caller's superuser
status into the search context, not where the status came from. A sanity
assertion on the admin branch ("dashboard" must be present) keeps a future
empty/short model set from passing as a green positive control.
This isolates the variable rather than hiding it: if transform were being
dropped for some reason other than the superuser flag, the test would
still fail on those shards with the flag pinned true.
Still catches the regression it guards: with `:is-superuser?` hardcoded to
true in the per-query context, the non-superuser assertion fails with
expected (not (contains? models "transform")), actual (not (not true)).
Verified: metabot search + v2 search + v2 common 79 tests / 308 assertions
/ 0 failures; kondo 0/0; cljfmt clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CF3ZXRaq6wy8sFR9L8DPKa
* Run the transform visibility test on both search engines
The previous fix was wrong. transform-visibility-is-superuser-only-test
kept failing on the MySQL 9 EE and MariaDB EE shards after it, with the
positive control still red:
"a superuser's search reaches the engine with transforms in scope"
expected: (contains? models "transform")
actual: (not (contains? #{"dashboard"} "transform"))
The superuser flag was never the cause. Pinning it did isolate the
variable, which is how this became findable: with `:is-superuser?` bound
true the admin case still saw no transform, so the gate had to be
elsewhere.
Two engines gate transforms by different code. MySQL and MariaDB cannot
hold the appdb search index, so they fall back to in-place, whose
applicable-models has a rule the appdb path has no equivalent of:
(empty? enabled-types) (disj "transform")
`enabled-transform-source-types` comes from `transforms-enabled`, whose
getter falls back to a cloud-token check when the setting is unset — so
under `with-premium-features` it was false, the source-type set was empty,
and in-place dropped transform for everyone. H2 uses the appdb path, which
has no such gate, so the test passed locally and was meaningless on the
two shards that ran the other engine.
Both inputs are now pinned: `transforms-enabled` is set explicitly, and
the body runs under `with-appdb-search-and-legacy-search` so the shard's
engine choice can no longer decide whether this test means anything. The
assertion count doubling from 4 to 8 confirms both passes execute.
Diagnosis proven locally rather than by another CI round-trip: with
`transforms-enabled` flipped to false, exactly 1 of 8 assertions fails —
the superuser case, on the in-place pass only — reproducing the CI failure
byte for byte on H2.
Verified: metabot search + v2 search + v2 common 79 tests / 312 assertions
/ 0 failures; kondo 0/0; cljfmt clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CF3ZXRaq6wy8sFR9L8DPKa
* fix(mcp): include dashboard parameter wiring
* Cover the query_handle source, which no live test reached
Both handle tests were deferred behind the unregistered execute_sql tool, so
nothing exercised resolve-source's query_handle branch: a regression there —
notably resolving through the native-rejecting resolve-query-handle! instead of
resolve-query-handle-for-save!, which would make every execute_sql handle
unsaveable — passed the suite untouched.
Mint handles straight into the store instead, the way question_test already
does. The deferred tests stay as the ledger for the whole-tool path; these pin
the branch in the meantime.
Four cases: an MBQL handle creates a transform whose target follows the
handle's database, a native handle stores as a native transform without
re-demanding agent:sql:run (minting it already passed that gate — the
exemption is deliberate, and is the half of the contract the mutation above
would silently break), a handle swaps an existing transform's source, and an
unknown handle teaches the recovery rather than surfacing an internal error.
Verified by mutation: pointing the branch at resolve-query-handle! fails 2 of
the 4; disabling the branch outright fails all 4.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Lzc5ERUWqWRx9JbDxK5Lc
* Mint a cursor only for a projection with a provable unique key
`tiebreaker-specs` fell back to the full projected tuple whenever no source-table
PK was projected, and treated that tuple as a total order. For an unaggregated
stage it isn't: the tuple repeats, and the strictly-past-the-boundary keyset then
skips every row tied with the boundary.
A `fields`-narrowed projection of non-unique columns hit this. PRODUCTS holds 200
rows across 4 categories; paging a CATEGORY-only projection at row_limit 5 served
20 rows and then reported itself complete — no truncation flag, no cursor, 180
rows silently gone.
Split the two meanings the function conflated. `unique-key-specs` now returns nil
when neither uniqueness proof holds, so a missing proof is distinguishable from an
order that is already total, and both callers refuse instead of inventing one.
The second proof has to look back past the last stage rather than just testing it:
`next-page-query` carries an aggregated query's keyset in an appended stage, so
from page 2 on the last stage is that pass-through — unaggregated and PK-less.
Inspecting only the last stage would close the gap and kill every aggregated chain
at page 2 instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JL8iPKC7nSLarfcnYDkaCS
* fix(mcp): address v2 protocol review
Reject unsupported DELETE without dropping session ownership. Enforce JSON-RPC 2.0 envelopes and notification silence. Keep pre-dispatch tool failures on the JSON-RPC error channel.
* Carry the target's database through a source-database swap
The target was only recomputed when the call named one, so an update that
swapped the source onto another database left the stored target pointing at the
old one. The target records the database it writes and that database follows the
query, so the row — and the echo built from it — named a database the transform
does not write. Feeding that echo back then hit resolve-target's target/query
database check and was refused, dead-ending the read-modify-write the tool is
built around. Only the JSON blob was stale; before-update recomputes the
target_db_id column, so runs went to the right place.
Patch the database onto the existing target instead of rebuilding it through
resolve-target: a target this tool can't author (incremental) still gets its
database corrected rather than becoming uneditable by a source-only edit. The
caller's own `target` still wins, being later in the cond->.
The test drives it across a real second database via with-temp-copy-of-db and
was watched failing first: stale database in both echo and stored row, plus the
round-trip rejection as the third symptom.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Lzc5ERUWqWRx9JbDxK5Lc
* Supersede the previous page's keyset instead of stacking beside it
`lib/filter` appends — `add-filter-to-stage` skips only exact duplicates — so
every cursor page ANDed a fresh boundary predicate onto the stage while leaving
its predecessors in place. Paging ORDERS two rows at a time, the stored handle
grew 668 -> 2692 bytes over nine pages and kept going, one dead predicate per
page, in the payload and in the emitted WHERE alike. With the full-tuple key each
of those clauses is quadratic in the projection width.
Stamp each minted clause and drop stamped ones off the target stage before adding
the current page's. Dropping them cannot change results: every row the new
predicate admits lies strictly past a boundary row that itself satisfied the old
one, so the new predicate implies the old. Filters the caller wrote carry no stamp
and are left alone.
The stamp rides in the clause's options map, which no schema declares. It survives
serialization, the base64/JSON round trip through the handle store, and
rehydration today; should Lib ever strip undeclared option keys the predicates
would accumulate again exactly as before — same results, larger query — so this is
an optimization and not something correctness rests on.
Stored handle now flat at 716 bytes across nine pages.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JL8iPKC7nSLarfcnYDkaCS
* Finish the sentence that teaches the source shapes
`accepted-shapes` is appended to five source-shape teaching errors — it is what
an agent reads to work out how to retry — and it promised "either" and then
listed one dialect. The missing half was the name-based dialect, which
definition->query genuinely resolves via portable-query?, so the message
advertised only one of the two shapes the tool accepts. Restored in the wording
metric.clj already uses for the same pair.
Nothing held the sentence to the code, because only the numeric-id dialect had a
test. Cover the portable branch, and assert the sentence names both dialects so
the half-written form can't come back.
Verified by mutation: disabling the portable branch fails the new
portable-definition test and nothing else.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Lzc5ERUWqWRx9JbDxK5Lc
* Refuse tag ids that name no tag, instead of dropping them
update-transform-tags! filters unknown ids out of the list it is given and
writes what's left, so `tag_ids` — documented as "replaces the current list" —
replaced it with a shorter one and reported success. An agent constructing ids
from a stale read has no reason to look twice, and jobs select the transforms
they run by tag, so the tag that failed to attach is noticed when a schedule
doesn't fire. Checked in the tool rather than in the shared write, which REST
also calls: this is the same restatement check-target-free! makes of REST's
target-conflict 403.
Cover collection_id and tag_ids while here — neither had a test, which is why
the silent drop had gone unremarked. The transform-folder namespace rule is
pinned too, since that refusal comes from a layer below this tool.
Adds a :model/TransformTag reference, so the generated module config gains it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Lzc5ERUWqWRx9JbDxK5Lc
* Pin the cursor's reliance on the QP to re-type a stringified boundary
Every execute_query run carries `js-int-to-string?`, which renders integers past
2^53-1 as strings. The cursor boundary is read straight out of the served row, so
a key column holding such values mints a predicate that compares a numeric column
against a string literal — verified, it really does emit
`[">" [field type/BigInteger] "9007199254740995"]`.
That compiles correctly, on every driver, because the query processor re-types it:
`wrap-value-literals` gives the bare literal the column's effective type and
`auto-parse-filter-values` parses it back to a bigint, both above the driver layer
(preprocess.clj:106-107).
Nothing covered that, and it is invisible from the cursor code, which reads a row
value and hands it to `lib/>` with no indication that its type is the query
processor's problem. Add a fixture whose values straddle the boundary and a test
that pages across it, plus a comment naming the dependency, so a change to the
middleware order or to the boundary read fails loudly here rather than in a
warehouse.
No production behavior change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JL8iPKC7nSLarfcnYDkaCS
* Mint a handle from the question, not from the page it was read on
A cursor page's stored query embeds the keyset boundary it resumed from — that is
how paging works — and the handle returned alongside was minted straight from it.
So a caller who paged to page 3 and then saved got a card reading "orders WHERE
id > 4171": a scroll position persisted as a question, which is nobody's question.
Strip the minted keyset off the last stage before storing, in `mint-handle!` so
every mint site is covered. Only predicates this namespace stamped come off; a
filter the caller wrote is part of the question and stays. Paging is untouched —
the cursor carries its own boundary — and it is a no-op for a query that never
paged.
Re-running a cursor page's handle now serves the question from the top rather than
that page again. The tool description and the namespace docstring promised
"exactly the query that ran" and are corrected: a handle is what a later tool
saves or visualizes, so the page boundary was never the useful part of it.
One artifact stays: a query carrying its own :limit has it spent down across pages,
so a deep page's handle asks for the rows still owed rather than the count the
caller wrote. Recovering that needs state the handle store deliberately does not
keep; `without-page-boundary` says so.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JL8iPKC7nSLarfcnYDkaCS
* Register the :document projection where both its tools can see it
document.clj projects through :document but did not require the namespace
that registered it, so the registration was reachable only via api.clj's
alphabetical require order. Loading the document tool on its own — from a
test, or any future consumer — left the projection unregistered, and
projections/project throws a 500 that the error sanitizer collapses to a
bare "Internal error".
Move the registration to projections.clj, which both tools already require,
matching the :question block's rule for a projection two tools share. The
key vector goes with it; content.clj's select-keys pre-narrow is dropped
rather than duplicated, since project re-applies select-keys on every path
and ::document retains the full map anyway.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015AFrL3LpdWBaQizx5QWaF5
* Return dimensions the way the metric and measure endpoints do
The `dimensions` include documents itself as returning the same
dimensions/dimension_mappings pair GET /api/metric/:id and
GET /api/measure/:id return. Nothing compared the two, and it drifted three
ways:
- It skipped the wire encoding, so the section shipped the internal
kebab-case shape (display-name, effective-type, and an internal
:lib/source) where every other field in the tool, and the REST API, uses
snake_case REST property names.
- It kept orphaned dimensions, which both endpoints drop unless asked for
them. An agent grouping by one would query a column that no longer exists.
- compute-dimensions reconciled every computed pair for an uninitialized
metric, while seeding takes own-table and explicitly-joined columns only.
A metric nobody has opened in the UI reported every FK-reachable column,
and since this read never persists, it kept reporting them.
Encode through ->api-dimensions/->api-dimension-mappings, drop orphans, and
give compute-dimensions the same three-way metric branch sync-dimensions!
has, sharing one definition of the seed filter with it.
The parity test this needed also replaces the curated-metric fixture, which
was built from get_content's own output and so agreed with whatever shape
the tool emitted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015AFrL3LpdWBaQizx5QWaF5
* Don't record a document view on a get_content read
`documents/get-document` publishes `:event/document-read` unless told not
to, and that handler bumps view_count, stamps last_viewed_at, writes a
view_log row, and pushes the document onto the caller's recently-viewed
list. get_content declares readOnlyHint and idempotentHint, and the
dimensions include already goes out of its way to compute without
persisting; a document read was quietly doing four writes and filling a
user's recents with the agent's reads.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015AFrL3LpdWBaQizx5QWaF5
* Register the :metric projection once, where both its tools can see it
content.clj and metric.clj each registered :metric with identical key
vectors. register-projection! overwrites silently, so which one won was
decided by api.clj's require order, and the two would have drifted apart the
first time either was edited.
Deleting either copy on its own just moves the problem: whichever tool keeps
the registration leaves the other reading a projection registered by a
namespace it does not require. Register it in projections.clj instead, which
both tools already require — the same place :question and :document live,
and for the same reason.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015AFrL3LpdWBaQizx5QWaF5
* Drop the TODO comments above the two restored document tests
Both tests were deferred while get_content did not exist and came back with
this slice; only the notes telling a reader they were still disabled were
left behind.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015AFrL3LpdWBaQizx5QWaF5
* Stop documenting a per-type scope gate that isn't there
The type->spec docstring said each entry carried "the extra runtime :scope
the type needs on top of the tool's base agent:resource:read", and the ns
docstring said each item was scope-checked. No entry has a :scope key, the
handler never reads one, and agent:resource:read is not a scope that exists.
The behaviour is right — GHY-4225 folded the per-entity read scopes into
agent:content:read, and the tool gates on that once at the registry
boundary, the same way search does. Only the comments were wrong, and a
comment describing a security control that isn't implemented is one a reader
will trust. Say what actually happens, and say that the absence of a
per-type scope is deliberate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015AFrL3LpdWBaQizx5QWaF5
* Cover the read paths get_content had no tests for
Four gaps, all coverage rather than defects — the behaviour was already
correct in each case:
- entity_id resolution had no test at all, though it is half the id schema.
Covers collection, question, snippet and document by entity_id, plus the
two teaching errors: alerts are numeric-only, and a string that is neither
shape is rejected.
- the definition include was only exercised for question. measure, segment
and transform each build theirs behind a catch that returns nil, and the
section is omitted when it does, so a broken exporter was indistinguishable
from a type with nothing to export.
- reads-formerly-gated-types named transforms but tested only alert, snippet
and document, so no transform read ran under a real agent:content:read
token.
- the transform target table had no test.
On that last one: the permission gate's false branch turns out to be
unreachable. Reading a transform requires superuser or data analyst,
superusers read every table, and table-permission-for-user hands every data
analyst manage-table-metadata :yes unconditionally, which alone satisfies
can-read? on a Table. Rather than force it with redefs, the test covers the
reachable paths — present on a detailed read, absent on a concise one, absent
when the target has never been created — and records why the gate is dead
code kept as defence in depth.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015AFrL3LpdWBaQizx5QWaF5
* Say what the SQL kill switch actually gates, and stop telling callers to pass a cursor alone
Two docs that contradicted the code.
check-execute-sql-enabled! claimed "every v2 path that runs or stores raw SQL
consults this one gate." It does not: run_saved_question executes a stored native
card without it, as does execute_query over a :source-card naming one. The gate
covers the paths where the agent AUTHORS the SQL — execute_sql, and question_write
/ transform_write storing agent-authored native — which is what the setting's own
description scopes it to, and what the card's read check guards on the other side.
Say that instead, so nobody infers the stronger rule and treats those two paths as
holes.
The cursor argument told callers to "pass it alone" while the tool description
told them row_limit alongside keeps the page size — and the cursor test passes
both. The exclusivity it was reaching for is already stated in the same sentence
and covers only query | query_handle | cursor, so an agent obeying the arg would
have silently dropped back to 100-row pages.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JL8iPKC7nSLarfcnYDkaCS
* Move the v2 redaction queries into mcp/db.clj, guarding the id splice
`redaction.clj` was the one namespace this branch adds that ran its own
app-DB queries, against the policy that a module's queries live in its
`db.clj`. Both move across unchanged in behaviour: the `t2/hydrate` call
becomes `mcp.db/hydrate-notification`, and the batched tenant lookup
becomes `mcp.db/user-id->tenant-id`. `lookup-recipient-tenant-ids` keeps
only the part that is actually redaction logic — deriving the ids of the
recipients whose `:user` hydration came back nil.
The empty-collection case moves with the query rather than staying at the
call site: `[:in ()]` is not valid SQL, and that is a fact about the query,
not about recipients.
`user-id->tenant-id` refuses anything that is not a collection of
integers. The ids are spliced into an `[:in ...]` clause, where HoneySQL
binds a number as a parameter but renders a string, a raw form, or a
nested map as SQL — so a non-integer element arriving there is an
injection vector rather than a type error. It is refused before any SQL is
built, and the offending values ride `ex-data` rather than the message so
nothing caller-controlled reaches a log line.
The check is `int?`, not `Long`: `core_user.id` is a 32-bit column and
comes back from JDBC as an `Integer`, so a Long-only check would satisfy
any test written with literals and then refuse every real call. `int?`
still admits only boxed integer types, which is the whole of what the
guard needs. It throws rather than using `assert`, matching the mint-time
guard in `session.clj` — `*assert*` can be compiled out, and a guard that
disappears in some builds is not a guard.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FwF5gxSyq2T5FsnyhViozq
* update tools descriptions
* update mcp v2 skills (#81915)
update skills
* Name the right threat in the user-id splice guard
The guard's docstring said HoneySQL renders a string in an `[:in ...]`
clause as SQL. It does not — strings and numbers are both bound as
parameters. The shapes HoneySQL reads as syntax rather than as a value are
vectors (function calls, so `[:raw "1) OR 1=1 --"]` renders verbatim and
`[:inline x]` inlines a literal), maps (subqueries), and keywords and
symbols (identifiers). Those are the injection vectors; a string id is a
plain bug, refused all the same because a silent no-match is a worse answer
than a throw.
The check itself was already correct and is unchanged. Only the reason
given for it was wrong, which matters for a guard people will edit code
against — the docstring is what tells the next reader which shapes have to
stay refused.
Covers `[:inline ...]` and a bare symbol in the refusal test, the two real
shapes the first pass missed.
Deliberately not wrapping the ids in `[:lift ...]` on top of this. It is
unreachable behind an `int?` check, and it would trade the throw for
driver-dependent silence: `[:in [[:lift "1"]]]` binds the string and H2
coerces it, returning a match, where Postgres would not. For this lookup
that is the wrong direction — a recipient whose tenant silently reads as
nil slips past the tenant filter.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FwF5gxSyq2T5FsnyhViozq
* mcp-v2.02 bookmark_content (#81246)
* MCP v2 tool: bookmark_content
Adds the v2 bookmark_content tool — per-user favorites, keyed by (type, id).
Extraction: bookmarks/api.clj factors bookmark!/un-bookmark!/bookmarked? out
of the REST endpoints so REST and MCP share them. The POST endpoint now
creates via app-db/select-or-insert! (concurrency-safe: a racing duplicate
returns the existing row instead of tripping the unique constraint); its 400
on already-bookmarked is unchanged. REST tests extended, existing pass.
Unlike REST, both tool directions are idempotent (see ns docstring for why),
both read-check the item, and the name echo is gated on the read scope so a
write-only token can't use it as a name oracle.
Tests: 9 tool tests + bookmarks REST suite green.
Review question for this PR: one tool — is bookmark_content's contract right?
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BjQcjQx7NPGr3Rpf6QaG4b
Also lands (first consumer, per the common.clj slimming in the core PR):
- NEW v2/resolve.clj — entity-id / numeric-id resolution + the read-check
pairing with the not-found collapse (+ resolve_test, moved from common_test).
- NEW v2/write.clj — readback (the scope-gated write echo).
CI guard: this slice's mcp -> bookmarks edge put `bookmarks` into the
modules-that-trigger-driver-tests set (mage's module-graph ratchet, 44 -> 45).
Bookmark logic cannot affect driver behavior — the edge is a consumer edge —
so `bookmarks` joins driver-affecting-overrides rather than bumping the
ratchet. Full mage suite green (82 tests).
Review nit (round 1): fetch-item's flavor-mismatch teaching error printed
"is a null" for a card type outside question/model/metric; guard the nil case
with a type-agnostic message.
--- Round-2 /review-pr response ---
IMPORTANT (per-user-test could not fail on its named assertion) FIXED. The test called the tool
bare, so a tool-level error was discarded and the assertions passed regardless. It now routes
both calls through tool-result (throws on tool error) and has :lucky bookmark-then-un-bookmark
its own row, so the "one user's bookmark is invisible to another's un-bookmark" claim has a live
counterfactual. Proven fail-when-broken (stubbing un-bookmark! to a no-op turns the test red).
MINOR fixed: the id schema now requires a positive int (matching REST's ms/PositiveInt); the
degraded write-scope readback now surfaces :bookmarked (via a new backward-compatible ack-keys
arg on v2.write/readback, so a write-only caller still learns the resulting state).
MINOR deferred (pre-existing REST behavior, out of a tool-slice's scope, confirmed structurally):
archived content is bookmarkable but filtered from listings; the Trash collection is bookmarkable;
readback's :url key has no producer on this branch (lands with collection_write). Each is shared
with the existing REST bookmark endpoints; changing it means deciding REST semantics.
Fresh-JVM: bookmark-test 9 tests / 43 assertions / 0 failures; kondo 0/0; modules unchanged.
* Route resolve.clj through mcp.db; correct the db.clj docstring
Three fixes from review of the mcpv2-01-core merge resolution.
resolve.clj called t2/select-one directly, so the toucan2 kondo rule this
same change adds shipped already violated by a file it introduces -- and at
the highest-value site, where an agent-supplied id enters a query. Adds
mcp.db/select-one-by-id, which asserts the id is already an integer (entity
ids are resolved before they reach it, so a string there is a bug rather
than a lookup), and routes resolve.clj through it. The rule now flags only
v2/redaction.clj, which predates this branch.
bookmarks/db.clj's ns docstring still claimed every function was "a direct
Toucan 2 call with no additional logic" after the generic dispatch tier
moved in, which is the one part of that docstring a reader relies on.
insert-bookmark!'s unique-constraint rationale moves from its docstring to
an inline comment on the select-or-insert! call it explains.
Tests: bookmarks.api-test + mcp.v2.tools.bookmark-test + mcp.v2.resolve-test,
30 tests / 150 assertions, green.
* readback: refuse an empty read-scopes instead of skipping the gate
`(remove f [])` and `(remove f nil)` both yield `()`, so an empty or nil
`read-scopes` made `missing` empty and returned the full ungated row -- a
gate that did not run was indistinguishable from one that ran and passed.
Not reachable from `bookmark_content`, which passes a literal one-element
vector. It matters because this namespace is shared write-path machinery for
the `_write` tools still to land, and the mistakes a future caller actually
makes -- omitting the argument, or computing `read-scopes` from a per-type
map that misses -- both landed on silently ungated rather than an error.
There is no legitimate empty case, so it throws.
Adds write_test.clj, which the namespace did not have: full-row grant,
degradation (exact key set, so `:name` cannot leak), empty and nil
`read-scopes` refusal, the unrestricted sentinel and nil `token-scopes`,
note pluralization, and an ack-key absent from the row. Verified the
refusal test bites -- removing the guard fails it twice (`[]` and nil) and
leaves the other five green.
Also drops `readback`'s 3-arity, which had no callers, and the ns
docstring's reference to which slice lands next.
Tests: write-test + tools.bookmark-test + resolve-test + bookmarks.api-test,
36 tests / 161 assertions, green.
* Clean up comments in resolve.clj
Removed unnecessary comments for clarity.
* Drop the stray toucan-in-mcp kondo rule from this branch
The rule entered here through the mcpv2-01-core merge resolution rather than
from either parent, and it now lives on mcpv2-00b-shared-infra next to the
mcp/db.clj boundary it governs. Keeping a second copy here only produced seven
false positives on MCP test namespaces, whose toucan2.core requires are fixture
setup rather than the agent-id boundary the rule protects.
Every src/metabase/mcp namespace that touches toucan2.core is already exempt
(db.clj and models.*), so the rule guards nothing on this branch. It comes back
with the shared infra when the stack restacks.
* Fold the dead per-model inserts into one unknown-model failure
The generic (model, id, user-id) tier left three sibling functions dispatching
three different ways. insert-bookmark! reaches the bookmark models through a map
and select-or-insert!, so the five per-model insert fns it replaced had no
callers left anywhere; delete them.
An unrecognized model string failed differently in each of the three: a case
with no default threw "No matching clause", while insert-bookmark! destructured
a missing key into [nil nil] and failed further down inside Toucan with a nil
modelable. Route all three through one ex-info. Nothing reaches this today --
both callers validate against a schema first -- but this branch promotes
metabase.bookmarks.db to module API, so callers that validate nothing else can
now arrive.
Also guard the REST contract the insert swap sits under: a duplicate POST to
/api/bookmark/:model/:id still answers 400, which nothing covered before.
---------
Co-authored-by: Eric Normand <ericwnormand@gmail.com>
* mcp-v2.00b: land common.clj at its final size in 01 (#81975)
* Extract the v2 shared machinery into 01
The decomposition rule was "shared machinery lands with its first consumer."
The cost is that the first consumer becomes a dependency for every slice
after it, so a slice needing two functions inherits everything beneath the
slice that happened to add them.
`get_content` is the worked example: it calls `common/response-format` and
`common/select-fields`, both of which first appear in 08-search, so it sits
behind eight slices to reach two functions. Slices 03, 06 and 07 contribute
nothing it uses -- 03 adds zero defs to common.clj, 07 zero, 06 one. They
are in its path because 08 was stacked on them chronologically.
Brings the three shared namespaces to their final size here instead:
- common.clj 172 -> 342: the fields/response-format machinery (with its
private helpers) and the list envelope. `select-fields` needs
`projections/catalog`, which resolves from a runtime registry and whose
namespace already ships in 01; `LevenshteinDistance` is already on the
classpath via metabot.
- resolve.clj (118) and write.clj (85), which 02 created and 03 finalised.
Taken at their reviewed state, including routing the read through
`mcp.db/select-one-by-id` and refusing an empty `read-scopes` in
`readback` rather than silently skipping the gate.
- mcp/db.clj gains `select-one-by-id`, which resolve.clj needs.
Module config regenerated: `channel` (list envelope urls) and
`eid-translation` (id resolution) join the mcp module's :uses.
Slices 02, 03, 06, 07 and 08 still define this code and will conflict with
identical content when they merge; each needs the duplicate dropped. In
exchange 16 of the 21 slices stop depending on each other and can be
reviewed in any order.
Tests: v2 common/registry/api/redaction + transport, 75 tests /
445 assertions, green.
* Move the toucan-in-mcp kondo rule here, with the code it governs
The rule discourages toucan2.core outside metabase.mcp.db, and this branch is
what introduces resolve.clj routing its read through mcp.db/select-one-by-id.
Landing the rule with the slice that first followed it left a window where the
convention held in code but nothing enforced it.
Exemptions unchanged: metabase.mcp.db is the seam itself, and
metabase.mcp.models.* define the models it sits on top of. Warning rather than
error while v2/redaction.clj still violates it.
* Bring resolve_test and write_test along with their namespaces
This branch was shipping resolve.clj and write.clj with no tests -- including
readback's empty-read-scopes guard, which exists precisely because that
function had none and its fail-open path went unnoticed.
Tests: resolve-test + write-test + common-test, 17 tests / 51 assertions.
* Scope the toucan rule to source, and land the redaction fix with it
CI failed on six warnings, all in test namespaces. The pattern matched
namespace names beginning `metabase.mcp.`, which includes
`metabase.mcp.api-test` and its siblings. Tests call Toucan directly on
purpose -- fixtures, row assertions, counts -- and none of that is an
agent-supplied trust boundary, so the rule's reason for existing does not
apply to them. Excluded using the same `-test`/`test-util` convention the
file's own `test-namespaces` group uses rather than inventing another.
That left one real violation, v2/redaction.clj, and the fix for it already
existed on 02 (8791186195: the redaction queries moved into mcp/db.clj
behind an id-splice guard, with db_test.clj). Cherry-picked here, since a
rule that arrives already-violated teaches the next reader that violating it
is normal.
clj-kondo over src/metabase/mcp and test/metabase/mcp: 0 errors, 0 warnings.
Tests: db-test + v2 redaction/resolve/write/common, 25 tests / 82 assertions.
* Drop the toucan-in-mcp kondo rule; it belongs in the closing PR
Landing the rule at the bottom of the stack means every later slice that
calls Toucan directly fails the Clj-Kondo job when it merges into 01 --
warnings are fatal there. That is ~13 slices, each adding one new
tools/X.clj with a median of 2 t2/ call-sites.
The rule's value is stopping *new* violations, and during the merge window
nobody is writing new MCP namespaces; the code is already written. Enforcing
now converts clean merges into unscheduled refactors.
The redaction fix cherry-picked alongside it stays -- routing those queries
through mcp/db.clj behind an id-splice guard is worth having regardless of
whether a linter enforces the pattern.
Rule text and its exemptions are recorded on GHY-4416 for the closing PR,
which lands it after the burn-down with :level :error rather than :warning.
* Land common.clj's tests alongside the functions
The hoist moved the functions into 01 without their tests, leaving
select-fields, response-format, frontend-url and the list envelope with
no CI signal until slice 08 lands.
Cherry-picks the five deftests and the fields-test projection fixture
from mcpv2-08-tool-search; common_test.clj is now byte-identical to that
branch's version. 10 tests, 67 assertions, 0 failures.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0156QMhf6CLY92wtY3AfKepV
* Close three coverage blind spots in common_test
truncation-line had no direct test at all — it was only ever reached
through list-content, which never passes :param or :total-floor?. Both
are on the path parameters.clj takes on later slices, so the new
truncation-line-test pins the narrow-with sentence and the "at least N"
floor rendering, plus the two nil cases.
The unknown-path assertion in select-fields-test matched only
#"Unknown field path.*Nearest", so any three catalog entries would have
passed and the Levenshtein ranking — the sole use of the
LevenshteinDistance import — went unasserted. It now pins the leading
two suggestions ("nmae" -> name, collection); the third slot is left
open because collection_path and parameters.name are close enough that
pinning it would test the tie-break, not the ranking.
paths->tree's reduce across distinct top-level keys was unexercised —
the existing merge cases both walk one branch. Added the ordinary
fields: ["name","parameters.type"] shape.
Tests for the three bugs filed against this code (GHY-4420 the
total-floor? boundary, GHY-4421, GHY-4422) are deliberately left out:
they would assert today's buggy behavior, and each belongs with its
fix. The floor case here stays strictly truncated for that reason.
11 tests, 72 assertions, 0 failures.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0156QMhf6CLY92wtY3AfKepV
* Restore list-content's :empty-hint option ahead of its callers
The hoisted list-content computes its own empty-page sentence and takes
no :empty-hint. Six branches further up the stack (10-duplicate,
16-browse, 19-visualize, 20-oauth, 21-retire-v1, dashboard-write) carry
the design-C list-content that does, and the one caller is
list_databases in tools/browse.clj, which explains that browsing needs
query-builder or table-metadata permission on some database. Restacking
those onto this branch would hand that :empty-hint to a function that
ignores unknown map keys: no compile error, no failing test, the
sentence just disappears. Accepting the option here means the slices
land on a list-content that already honors it.
The two sentences are kept mutually exclusive by construction rather
than by coincidence of the current call site:
(or (empty-page-line opts) (when (= 0 total) empty-hint))
empty-page-line says "you paged past the end, N exist, page back";
:empty-hint says "there are genuinely zero results, and here is why".
Gating on (= 0 total) is not redundant with the or — without it, a
static hint could print over a paging situation and tell someone with
37 databases that none are visible to them.
The new list-content-empty-hint-test exists so that an incorrect
conflict resolution fails loudly instead of silently. It asserts the
verbatim browse hint text at total 0 (red if :empty-hint support is
dropped again), and that the hint does not displace the empty-page line
at total 37 / offset 100, nor the truncation line on a non-empty page
(red if the guard is "simplified" to a bare or). Both testing strings
name the browse.clj restack they guard.
12 tests, 82 assertions, 0 failures.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0156QMhf6CLY92wtY3AfKepV
* Gate list-content's :empty-hint on the offset, not just the total
`:empty-hint` is documented as the domain reason a genuinely empty result
set is empty, and the gate was meant to keep it away from a caller who
merely paged past the end. It only checked the total, so it did not.
`empty-page-line` requires `(pos? total)`, so at a total of 0 it declines
for every offset — including a nonzero one. The `or` then fell through to
the hint, and `list_databases` (which forwards the caller's raw offset
through `paged-list-content`) would answer "No databases are visible to
you" for someone who had actually paged past the end of an empty list.
The sentence stays true at a zero total, so this is a contract violation
rather than a false statement, but it is not what the comment claims.
Add the offset to the gate and pin it with a regression test. Verified
the test fails against the previous gate and passes against this one.
---------
Co-authored-by: Eric Normand <ericwnormand@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* mcp-v2.09a prep: extract the documents Markdown round-trip layer (#81432)
* MCP v2 prep: extract the documents Markdown round-trip layer
Part 9a of the MCP v2 decomposition of #77844. This lands the documents
Markdown layer that the document_write tool (a later slice, 9b) is the first
consumer of, split off so it reviews as a serialization extraction with no MCP
surface. v1 and v2 are untouched; this slice adds no tool.
- NEW metabase.documents.markdown (+ its 929-line test): the CommonMark <->
ProseMirror AST boundary — parse/serialize/splice, {% card %}/{% entity %}
token scanning, escaping, a nesting-depth bound. Ported verbatim (byte-
identical to the source), so its injection / permission-leak / stack-overflow
hardening is preserved intact.
- metabase.documents.models.document: gains the write orchestration
(create-document!, update-document!, get-document, clone-cards-in-document!
and their card-creation helpers). The unrelated explorations content-gating
block (can-read?/can-write? overrides, the content-visibility cache) is
deliberately NOT ported — it is a different feature's drift in the same file
and would change document permission semantics. Two further exploration-
Summary threads (a stored_result_use.card_id column from an unlanded
migration; serdes snapshot-embed hardening) are likewise excluded.
- metabase.documents.api.document: card-creation logic moves out of the REST
layer into the model (519 -> 358 lines); the endpoints become thin wrappers.
The unchanged REST document api-test is the check (88 tests / 518 assertions).
- metabase.documents.prose-mirror / documents.core: prose-mirror gains
smart-link-model->db-model; the facade exports the new parse/serialize/splice
+ model fns.
- metabase.comments.core / comments.models.comment: the comment facade gains
child-target-ids-for-document / comments-for-document / register-context-gate!
(notify-comment-id! kept — a grep confirmed zero callers, and dropping a
public re-export is a one-way door).
Fresh-JVM: markdown-test 285 assertions; REST document api-test 518; :module
comments 154; all 0 failures. kondo 0/0; cljfmt clean; modules-config regenerated
(documents module gains :model/Database). The one :module documents error
(collection_test Duplicate key) is a pre-existing deterministic bug in a file
this slice does not touch, reproducing identically in isolation.
* Address review findings in the documents markdown layer
- splice gets the same StackOverflowError -> 400 backstop parse has: the
re-parse of the edited region runs through flexmark, whose inline parser
can blow the stack on a long delimiter run in the replacement text.
- A codeBlock :language holding a backtick (which a tilde fence's info
string legitimately parses into) now re-serializes behind a tilde fence
instead of silently breaking the fence line, which let fenced
{% card %} text re-parse as a real embed; a newline in :language
collapses to a space.
- inline-lines collects per-line chunks and joins once: appending to the
accumulated string was quadratic in a paragraph's inline node count
(40k nodes: 1.9s -> 156ms, and it scaled 4x per doubling before).
- attr-pos-long / format-number range-check before (long n), so an
integral double past long range is a teaching error, not a raw
IllegalArgumentException.
- update-document!'s :cards schema matches the REST DocumentUpdateOptions
it backs (:int keys, ignored without :document) instead of narrowing it.
- comments-for-document keeps a deleted parent that still has replies,
content scrubbed to {}, matching GET /api/comment.
- create-card! back to private (no caller outside the namespace); stale
smart-link docstrings and a broken namespace link cleaned up.
- Tests for all of the above, plus previously uncovered branches:
container content-model violations (incl. via splice), splice span
bounds and the insertion/separator-only arms, token-grammar teaching
errors, and the entity-token code-mark/unknown-model guards.
* mcp-v2.03 collection_write (#81247)
* MCP v2 tool: bookmark_content
Adds the v2 bookmark_content tool — per-user favorites, keyed by (type, id).
Extraction: bookmarks/api.clj factors bookmark!/un-bookmark!/bookmarked? out
of the REST endpoints so REST and MCP share them. The POST endpoint now
creates via app-db/select-or-insert! (concurrency-safe: a racing duplicate
returns the existing row instead of tripping the unique constraint); its 400
on already-bookmarked is unchanged. REST tests extended, existing pass.
Unlike REST, both tool directions are idempotent (see ns docstring for why),
both read-check the item, and the name echo is gated on the read scope so a
write-only token can't use it as a name oracle.
Tests: 9 tool tests + bookmarks REST suite green.
Review question for this PR: one tool — is bookmark_content's contract right?
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BjQcjQx7NPGr3Rpf6QaG4b
Also lands (first consumer, per the common.clj slimming in the core PR):
- NEW v2/resolve.clj — entity-id / numeric-id resolution + the read-check
pairing with the not-found collapse (+ resolve_test, moved from common_test).
- NEW v2/write.clj — readback (the scope-gated write echo).
CI guard: this slice's mcp -> bookmarks edge put `bookmarks` into the
modules-that-trigger-driver-tests set (mage's module-graph ratchet, 44 -> 45).
Bookmark logic cannot affect driver behavior — the edge is a consumer edge —
so `bookmarks` joins driver-affecting-overrides rather than bumping the
ratchet. Full mage suite green (82 tests).
Review nit (round 1): fetch-item's flavor-mismatch teaching error printed
"is a null" for a card type outside question/model/metric; guard the nil case
with a type-agnostic message.
--- Round-2 /review-pr response ---
IMPORTANT (per-user-test could not fail on its named assertion) FIXED. The test called the tool
bare, so a tool-level error was discarded and the assertions passed regardless. It now routes
both calls through tool-result (throws on tool error) and has :lucky bookmark-then-un-bookmark
its own row, so the "one user's bookmark is invisible to another's un-bookmark" claim has a live
counterfactual. Proven fail-when-broken (stubbing un-bookmark! to a no-op turns the test red).
MINOR fixed: the id schema now requires a positive int (matching REST's ms/PositiveInt); the
degraded write-scope readback now surfaces :bookmarked (via a new backward-compatible ack-keys
arg on v2.write/readback, so a write-only caller still learns the resulting state).
MINOR deferred (pre-existing REST behavior, out of a tool-slice's scope, confirmed structurally):
archived content is bookmarkable but filtered from listings; the Trash collection is bookmarkable;
readback's :url key has no producer on this branch (lands with collection_write). Each is shared
with the existing REST bookmark endpoints; changing it means deciding REST semantics.
Fresh-JVM: bookmark-test 9 tests / 43 assertions / 0 failures; kondo 0/0; modules unchanged.
* MCP v2 tool: collection_write
Adds the v2 collection_write tool — create/update/move/archive collections.
Extraction: collections/update.clj lifts update-collection! out of
PUT /api/collection/:id — the four privates (move-collection!,
archive-collection!, move-or-archive-collection-if-needed!,
maybe-send-archived-notifications!) move verbatim (docstrings de-REST-ified,
notification alias renamed); the endpoint becomes a thin delegator through
the collections.core facade, mirroring the existing create-collection!
pattern. Verified move: bodies diff clean modulo whitespace/docstrings.
The foundation's REST-test changes for this file are all the BROWSE
extraction's repoints, so master's collections_rest api-test runs UNCHANGED
against this branch — that unchanged suite is the extraction check
(fresh-JVM: 202 tests, 941 assertions, green, incl. 23 tool tests).
Deferred to the content tool PR: echo-matches-concise-read-test (the write
echo vs get_content cross-check) — it requires tools/content.
Review question for this PR: one tool — is collection_write's contract right?
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BjQcjQx7NPGr3Rpf6QaG4b
Also lands (first consumer, per the common.clj slimming in the core PR):
- v2/resolve.clj grows collection resolution (resolve-collection-id{,-or-personal}).
- v2/write.clj grows `_write` method dispatch (dispatch-write + expand-clear).
- common.clj grows frontend-url. Tests moved alongside (resolve_test, write_test).
Review nits (round 1): fix the self-contradictory `name` field description
("Create only (editable on update)" -> "Required on create; editable on
update"); drop structuredContent from the write echo to match the stack's
text-only convention (no concrete programmatic consumer, no outputSchema).
CI kondo fix: the update-collection! extraction moved the only `tru` caller in
collections_rest/api.clj into collections/update.clj, leaving a dead
`[metabase.util.i18n :refer [tru]]` require — an unused-require warning, which
`mage kondo` (CI's check) exits nonzero on. Removed it.
--- Round-2 /review-pr response ---
IMPORTANT (archived:true + parent_id silently discarded the move) FIXED. The dispatch tested
archived first and returned, so a caller passing both got a trashed collection and no move, with
no signal — defeating the tool's "never believes an ignored field took effect" invariant. It is
now a teaching error naming the fix (move first then archive, or pass parent_id on a later
archived:false restore). Regression test added.
MINOR fixed: the four (is (tool-error ...)) assertions now match the specific expected message,
not any error text; the :name destructure was renamed (coll-name) so it no longer shadows
clojure.core/name.
Fresh-JVM: collection-test 24 tests / 65 assertions / 0 failures; kondo 0/0; modules unchanged.
--- Close-out M5 ---
MINOR (bare tool-error assertion): authority-level-update-test now asserts the specific "paid feature" message (parity with the create-side twin), so a wrong error text fails.
* Route resolve.clj through mcp.db; correct the db.clj docstring
Three fixes from review of the mcpv2-01-core merge resolution.
resolve.clj called t2/select-one directly, so the toucan2 kondo rule this
same change adds shipped already violated by a file it introduces -- and at
the highest-value site, where an agent-supplied id enters a query. Adds
mcp.db/select-one-by-id, which asserts the id is already an integer (entity
ids are resolved before they reach it, so a string there is a bug rather
than a lookup), and routes resolve.clj through it. The rule now flags only
v2/redaction.clj, which predates this branch.
bookmarks/db.clj's ns docstring still claimed every function was "a direct
Toucan 2 call with no additional logic" after the generic dispatch tier
moved in, which is the one part of that docstring a reader relies on.
insert-bookmark!'s unique-constraint rationale moves from its docstring to
an inline comment on the select-or-insert! call it explains.
Tests: bookmarks.api-test + mcp.v2.tools.bookmark-test + mcp.v2.resolve-test,
30 tests / 150 assertions, green.
* readback: refuse an empty read-scopes instead of skipping the gate
`(remove f [])` and `(remove f nil)` both yield `()`, so an empty or nil
`read-scopes` made `missing` empty and returned the full ungated row -- a
gate that did not run was indistinguishable from one that ran and passed.
Not reachable from `bookmark_content`, which passes a literal one-element
vector. It matters because this namespace is shared write-path machinery for
the `_write` tools still to land, and the mistakes a future caller actually
makes -- omitting the argument, or computing `read-scopes` from a per-type
map that misses -- both landed on silently ungated rather than an error.
There is no legitimate empty case, so it throws.
Adds write_test.clj, which the namespace did not have: full-row grant,
degradation (exact key set, so `:name` cannot leak), empty and nil
`read-scopes` refusal, the unrestricted sentinel and nil `token-scopes`,
note pluralization, and an ack-key absent from the row. Verified the
refusal test bites -- removing the guard fails it twice (`[]` and nil) and
leaves the other five green.
Also drops `readback`'s 3-arity, which had no callers, and the ns
docstring's reference to which slice lands next.
Tests: write-test + tools.bookmark-test + resolve-test + bookmarks.api-test,
36 tests / 161 assertions, green.
* Clean up comments in resolve.clj
Removed unnecessary comments for clarity.
* Drop the stray toucan-in-mcp kondo rule from this branch
The rule entered here through the mcpv2-01-core merge resolution rather than
from either parent, and it now lives on mcpv2-00b-shared-infra next to the
mcp/db.clj boundary it governs. Keeping a second copy here only produced seven
false positives on MCP test namespaces, whose toucan2.core requires are fixture
setup rather than the agent-id boundary the rule protects.
Every src/metabase/mcp namespace that touches toucan2.core is already exempt
(db.clj and models.*), so the rule guards nothing on this branch. It comes back
with the shared infra when the stack restacks.
* Route collections/update.clj's app-DB queries through collections/db.clj
The collections module keeps its app-DB queries in db.clj; update.clj still
reached for toucan2 directly. `update!` and the final re-read map onto the
existing `update-collection!` and `collection`; the archived-card sweep and the
move's parent lookup needed new fns, mirroring the query shapes
collections-rest/db.clj settled on.
`t2/hydrate` stays in update.clj — it hydrates the value api/write-check
returns rather than fetching one, matching how measures, segments, and
dashboards handle hydrate after the same refactor.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FZzZhTfVjp6ZGe9fBfK997
* Collapse parent_id's not-found and no-access answers
`resolve-collection-id` probed existence with no read check and let the
downstream write-check throw, so a token holding only agent:content:write could
tell a nonexistent collection id ("not found") from one it merely cannot write
("You don't have permissions"), and enumerate collection ids across the
permission boundary. POST /api/collection answers 403 either way. The `id`
argument already collapsed both through `resolve-and-read`; `parent_id` now
takes the same path, which keeps the bogus-id guard the probe was there for.
Two existing perms tests asserted the old behavior by pointing at another
user's personal collection; they now use a readable-but-not-writable collection,
so they still prove the write check fires without depending on the leak. The
unreadable-parent case is what the new test covers.
Also repairs this namespace's tool-result/tool-error helpers, stale since
c521e4c71c changed call-tool to return {:result}/{:error}. That commit updated
bookmark_test.clj but reached collection_test.clj through a merge with no
textual conflict, leaving every error assertion in the namespace failing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FZzZhTfVjp6ZGe9fBfK997
* Drop two collections-rest db helpers left without callers
`cards-in-collection` and `collection-location-columns` each had exactly one
caller, inside the two private fns this branch moved to
`metabase.collections.update`. The collections module cannot require
collections-rest, so update.clj carries its own copies; these are dead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FZzZhTfVjp6ZGe9fBfK997
* Refuse a parent_id move that would leave a collection in the trash
Archiving sets only archived/archived_directly/archive_operation_id, so a
trashed collection keeps its pre-trash :location. An update passing parent_id
without archived therefore skipped the archive branch, moved the collection
against that stale location, and echoed the new parent while the collection sat
in the trash — nothing appeared under the new parent.
`archived: true` + parent_id was already rejected; this is the same
contradiction from the other side, so it gets the same teaching error, pointing
at the `archived: false` + parent_id restore path that already works.
The check needs the collection's current state, so it can't join the pure
arg checks in check-method-args!. It takes the collection resolve-existing
already returned rather than fetching its own.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FZzZhTfVjp6ZGe9fBfK997
* Pin the write-echo degradation and the collection-id not-found collapse
Both properties were load-bearing and unasserted. The scope-gating test drove a
write-only token but checked only that :id was an int, which holds whether or
not the readback gate runs — dropping the gate, or widening its ack-keys to
leak :name, failed nothing. resolve-collection-id's main branch had no coverage
at all; the file tested only the nil/"root"/"trash" sentinels.
Each test was verified by mutation: removing the readback call reddens the
degradation test on both methods, and reverting the id collapse to
resolve-id-or-404 reddens the not-found test on the message-equality assertion
that is the actual security property.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FZzZhTfVjp6ZGe9fBfK997
* Close collection_write's namespace argument to the one namespace it documents
Any non-blank string was accepted and passed straight through to the create.
assert-valid-namespace only checks a collection against its parent, so a
root-level create with a typo passed: namespace "snippet" produced a collection
in a hierarchy nothing surfaces, and the echo confirmed the typo back — the one
thing the tool's contract says must never happen.
[:enum "snippets"] mirrors authority_level's [:enum "official"] two entries
below, and puts the allowed value in the tool schema where most clients will
reject a typo before it reaches us. transforms is left out deliberately: those
collections are created by the transforms machinery and carry its lifecycle.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FZzZhTfVjp6ZGe9fBfK997
---------
Co-authored-by: Eric Normand <ericwnormand@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [mcp-v2 09b] document_write tool
Second half of split slice 09 (DEC-0001). 09a landed the documents Markdown
round-trip layer; this adds the thin MCP tool that drives it.
Review question: does the write tool enforce document-level write permission
before it reaches the docstring-only-guarded model fns, and does it refuse to
launder a foreign card into a document the caller cannot write?
Surface:
- src/metabase/mcp/v2/tools/document.clj — the document_write handler.
create! runs api/create-check before documents/create-document!.
update! runs read-check (resolve-and-read-with) -> check-not-archived ->
api/write-check -> validate-collection-move-permissions, all before
documents/update-document!. Card-embed and smart-link resolution are
permission-checked (mi/can-read?); a foreign readable card is cloned, never
referenced; replace_all is bounded by max-replace-all-work.
- :document projection registered here (first mcp consumer); content/11 drops
its duplicate when it lands.
Inherited 09a-review finding (closed): the documents.core model fns enforce
permission by docstring only. 09b is their first direct caller, so the tool
carries the checks. permission-denied-write-leaves-no-db-trace-test asserts DB
state (no Document row, no cloned Card) on a refused write, not just :isError;
verified fails-when-broken by removing api/write-check.
Deferred tests (restore with slice-11/content): create-clones-card-test,
write-echo-and-read-name-the-body-alike-test.
Gates: document-test 80 assertions, :module mcp 5280 assertions, kondo 0,
cljfmt clean, modules config regenerated (mcp :uses comments+documents,
comments :api gains comments.core).
--- Correctness fix (round-1 review, critical) ---
apply-edit/replace-all spliced new_str into Markdown source as-is. splice re-parses the
edited region, so a replacement like "*" or a leading "#" reopened the block as a list or
heading and could drop a character (the "*" consumed as a list marker) — a text-only edit
silently corrupting structure and losing data. Reproduced: body "x x x x x" + edit x->*
replace_all yielded a bulletList with "* * * *" (4 not 5).
Fix: new public documents/escape-text (markdown.clj) escapes a raw string to its literal
form for any position — inline emphasis/link/code chars per line, plus a per-line leading
block-start escape. apply-edit escapes new_str before splicing; over-escaping a char not in
a special position is inert (CommonMark renders \# as #). splice itself is unchanged (its
markdown-source contract is load-bearing for heading/card-embed inserts — verified 09a
markdown-test stays green). New edit-replacement-is-literal-text-not-markdown-test locks it,
proven fails-when-broken.
* Resolve user smart links under the mention picker's visibility rule
`smart-link-readable?` let any non-sandboxed caller resolve any `:model/User` row, and the stored
label falls back to `:common_name` — the email address when a user has no name. A document author
could therefore enumerate names and emails across tenants, deactivated accounts and API-key
accounts included, by writing `{% entity model="user" %}` tokens and reading the labels back.
User rows are now selected the way `GET /api/comment/mentions` and `GET /api/user/recipients`
select them: a superuser sees everyone; anyone else sees active personal accounts in their own
tenant, narrowed by the `user-visibility` setting. Tests cover a deactivated user and
`user-visibility :none`. Modules config regenerated (mcp now uses users).
* Don't escape an edit that lands inside code
`apply-edit` escaped `new_str` unconditionally so a replacement like `*` or a leading `#` could not
reopen the block as a list or heading. That reasoning holds in prose and inverts inside code:
backslashes are literal in a code span or fenced block, so the escaping stored characters the caller
never wrote. `{old_str "ls", new_str "my_var"}` against "run `ls -la` now" stored
"run `my\_var -la` now" — verified by driving apply-edit before the fix. Snake_case identifiers in
code are the common case.
Escaping is now decided per match: `in-code-context?` counts unescaped backticks and fence lines
before the offset, and `replace-all` prices each occurrence against the serialization it is splicing,
so one match in prose and another in a code span each round-trip correctly.
Claude-Session: https://claude.ai/code/session_018PNCQxp9kCypqE3dLQS9va
* Enable the feature the user-visibility assertion depends on
The smart-link test narrowed `user-visibility` to `:none` and asserted that another user's name stops
resolving. But `user-visibility` is gated on `:email-restrict-recipients`, so in an OSS build the
setting reads `:all` whatever is stored: the narrowing never happened and the assertion pinned the
default instead of the rule. It passed locally and failed every OSS App DB CI job.
Wrapped in `mt/with-premium-features`, matching how the other `user-visibility` tests set it up.
Claude-Session: https://claude.ai/code/session_018PNCQxp9kCypqE3dLQS9va
* Fix document_write Markdown table handling
Reject GFM tables in full document bodies and edit replacements before persistence. Advertise the limitation in the tool description.
* fix(mcp): parse document edits as Markdown
* Budget document_write's edit work per call, not per edit
`max-replace-all-work` was checked once inside `replace-all`, so every entry
in `edits` got its own fresh allowance. `edits` is an unbounded list, which
makes that no bound at all: 100 edits each priced just under the ceiling buy
100x the work in one request.
The ceiling now covers the call. `apply-edit` takes and returns
`{:ast :spent}`, pricing each edit against the total the earlier ones have
already spent. It has to be spent down incrementally rather than totalled up
front — an edit's match count is only knowable against the text its
predecessors produced.
Plain edits are priced too. A single splice still re-serializes the whole
document, so `edit-work` counts splices (one for an edit, one per occurrence
for a `replace_all`) and both kinds draw on the same budget.
`replace-all` no longer serializes to price itself. It takes the
serialization and matches `apply-edit` already computed, which drops a full
redundant serialization per `replace_all` and makes its docstring's "one
serialization rather than one per match" true.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016fD5YPdm6TNR2WfU3k7MMB
* Follow collection_write's contract changes into document_write
Slice 03 changed two shared contracts under this branch. Both merged clean
and both were invisible to a targeted kondo run, which cannot resolve vars
across namespaces.
`v2.write/readback` gained an `ack-keys` parameter — caller-supplied keys
that survive when the response degrades for a token without the read scope.
`document_write` has none: nothing it echoes back is caller-supplied rather
than read-gated, so it passes nil, as collection_write does.
`registry/call-tool` now returns `{:error …}` when the registry refuses
before dispatch and `{:result mcp-content}` after the handler runs, so
`:isError` and `:content` moved under `:result`. The test helpers still read
them at the top level, which made every refusal look like a success and hid
both budget tests behind a vacuous pass. `error-text` reads either shape from
one place: a schema violation refused at the boundary carries `:error`, a
handler's teaching error carries `:result {:isError true}`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016fD5YPdm6TNR2WfU3k7MMB
* Record a recent view when a document is created
`command palette > should display collection names for documents in recents`
was failing on this branch (2 of 2 runs), passing 5/5 on master with a
byte-identical spec.
The documents extraction swapped `get-document` for `read-check` +
`hydrate-document` on the create path, so `:event/document-read` stopped
firing. That event's handler is the only thing that writes the `RecentViews`
row (documents/view_log.clj), and the command palette surfaces a just-created
document only through recents — the spec POSTs a document and never visits it.
Creating is arguably not reading, but recents rides on the same event, and
UXW-1786 expects a new document to be findable. Restored `get-document` and
dropped `post-document-does-not-record-view-test`, which pinned the new
behaviour; it and the e2e spec cannot both hold. The POST/PUT symmetry that
motivated it does not apply: PUT never recorded a view on master, POST always
did.
Added `post-document-records-a-recent-view-test` beside the GET/PUT view tests
so the expectation is asserted at the API level — the Cypress spec reports an
empty palette without saying why.
Decoupling recents from `:event/document-read` would let view counts stop
treating creation as consumption; that is a separate change.
* Make the session-signature tests capable of failing
The two signature assertions in `mcp-ui-capability-claim-must-be-signed-test`
passed for the wrong reason, so they proved nothing about signing.
`#"\\."` matches a literal backslash followed by any character, not an escaped
dot, so the split was a no-op: `uuid` received the whole session id and
`payload`/`sig` were nil. The reconstructed ids were malformed and rejected
before signature verification ever ran. Replacing `session-payload-signature`
with a constant left all six assertions green.
Fixing the split alone was not enough. The tampering case re-encoded the SAME
payload onto an already `ui true` id, so it rebuilt byte-identical and was
believed for the right reason. It now mints a `ui false` id and swaps in a
`ui true` payload under the original's signature, which is the escalation the
signing exists to stop.
Guards added so this cannot silently rot again: the splits assert they reached
a signature, the lifted signature asserts it differs from the original, and a
control asserts the unmodified id claims nothing.
Verified by mutation: with `session-payload-signature` stubbed to a constant
the test now fails 3 assertions, where it previously passed clean.
* Pin the transform search gate without writing a global setting
transform-visibility-is-superuser-only-test wrapped its body in
(mt/with-temporary-setting-values [transforms-enabled true]
(mt/with-premium-features #{:transforms-basic} ...))
which left `transforms-enabled` explicitly set for the rest of the JVM, and
took metabase.metabot.tools.transforms-test and metabase.revisions.api-test
down with it on every backend shard.
`with-temporary-setting-values` does not restore a setting to *unset*. It
captures `(setting/get setting-k)` on entry and writes that value back in its
`finally` (test/metabase/test/util.clj:552-609). `transforms-enabled` is unset
here, so the capture runs its getter's `cloud-enabled-transforms?` fallback
((and (is-hosted?) (has-feature? :transforms-basic))) and yields a computed
boolean rather than nil. Restoring that boolean persists a real Setting row,
so `explicit-transforms-enabled` stops returning nil and the fallback every
other transform test relies on is dead for the remainder of the run.
The capture also happened before any premium features were stubbed -- the
setting form was the outer one -- so it read ambient state, not
#{:transforms-basic}.
Downstream that lands on the permission check: `transform-readable?`
(src/metabase/transforms/models/transform.clj:35-42) gates on
`check-feature-enabled` *ahead* of the `api/*is-superuser?*` bypass, and that
gate begins with `(setting/get :transforms-enabled)`
(src/metabase/premium_features/token_check.clj:715-730). Once the setting is
pinned false, even admin callers are denied, which is why the revisions test
saw 403 on its superuser case and the metabot transform tools returned
"You don't have permissions to do that.".
Pin the gate at its actual seam instead: redefine
`transforms.gating/enabled-source-types`, which is the single place
search/impl.clj:364 reads it from. That covers what the test needs -- the
in-place engine drops transform for everyone when the enabled set is empty --
without touching persisted state, so there is nothing to restore and nothing
to leak.
Adding `:hosting` to the feature set would also have made the leaked value
harmless, but it only changes *which* value leaks; the setting would still
come back explicitly set. The underlying defect in
`do-with-temporary-setting-value!` (capturing the computed value rather than
the raw stored one) is left for a separate change -- it has ~229 call sites
and wants its own audit.
* Move the document comment queries into comments.db
Master added a clj-kondo hook requiring Toucan2 query calls to live in
`metabase[-enterprise].<module>.db`. This branch had added two `t2/select`
calls to `comments/models/comment.clj`. Neither side fails alone; merging the
new rule with the existing calls turned CI's Clj-Kondo job red with two
warnings.
`document-comments` and `document-child-target-counts` now live in
`comments/db.clj` alongside the module's other queries. The model functions
keep their logic — the deleted-comment scrub, `:creator` hydration, the
`select-keys` projection — and delegate only the query.
Full-tree kondo: 0 errors, 0 warnings; no other file violates the new rule.
:module comments 174 assertions, 0 failures / 0 errors. :module documents
1408 assertions with only the two pre-existing `collection-test` errors, which
predate this change.
* Move document_write's Toucan calls into mcp.db
#82099 added `:metabase/t2-query-namespace`, which confines application
database query calls to `metabase[-enterprise].<module>.db`. It arrived with
the master merge and made this file the only violator in the tree — three
warnings, and CI requires zero.
`select-by-ids` covers the two `:in` lookups and mirrors the existing
`select-one-by-id`, which already takes a dynamic model.
`select-users-where` takes an assembled HoneySQL clause, which is looser than
the "direct Toucan 2 call with no additional logic" the namespace documents.
The clause is built from the `user-visibility` setting, group membership and
the superuser flag — it decides which users the caller may resolve at all, so
it is a permission decision. Moving it into a db namespace would hide it and
pull those dependencies in. #066c63b88ee, the comments move on this branch,
set the precedent: the logic-bearing fn stays, only the query moves.
`select-by-ids` deliberately has no `{:pre [(seq ids)]}` despite
`select-one-by-id` guarding its argument. An AssertionError is not an
Exception, so it would bypass the `catch Exception` in `smart-link-rows` and
turn a logged warning into a propagated failure — a behavior change this
relocation should not smuggle in. Callers guard the empty case instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016fD5YPdm6TNR2WfU3k7MMB
* mcp-v2.04 prep: portable-dialect execution rework in the agent lib (#81248)
* MCP v2 tool: bookmark_content
Adds the v2 bookmark_content tool — per-user favorites, keyed by (type, id).
Extraction: bookmarks/api.clj factors bookmark!/un-bookmark!/bookmarked? out
of the REST endpoints so REST and MCP share them. The POST endpoint now
creates via app-db/select-or-insert! (concurrency-safe: a racing duplicate
returns the existing row instead of tripping the unique constraint); its 400
on already-bookmarked is unchanged. REST tests extended, existing pass.
Unlike REST, both tool directions are idempotent (see ns docstring for why),
both read-check the item, and the name echo is gated on the read scope so a
write-only token can't use it as a name oracle.
Tests: 9 tool tests + bookmarks REST suite green.
Review question for this PR: one tool — is bookmark_content's contract right?
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BjQcjQx7NPGr3Rpf6QaG4b
Also lands (first consumer, per the common.clj slimming in the core PR):
- NEW v2/resolve.clj — entity-id / numeric-id resolution + the read-check
pairing with the not-found collapse (+ resolve_test, moved from common_test).
- NEW v2/write.clj — readback (the scope-gated write echo).
CI guard: this slice's mcp -> bookmarks edge put `bookmarks` into the
modules-that-trigger-driver-tests set (mage's module-graph ratchet, 44 -> 45).
Bookmark logic cannot affect driver behavior — the edge is a consumer edge —
so `bookmarks` joins driver-affecting-overrides rather than bumping the
ratchet. Full mage suite green (82 tests).
Review nit (round 1): fetch-item's flavor-mismatch teaching error printed
"is a null" for a card type outside question/model/metric; guard the nil case
with a type-agnostic message.
--- Round-2 /review-pr response ---
IMPORTANT (per-user-test could not fail on its named assertion) FIXED. The test called the tool
bare, so a tool-level error was discarded and the assertions passed regardless. It now routes
both calls through tool-result (throws on tool error) and has :lucky bookmark-then-un-bookmark
its own row, so the "one user's bookmark is invisible to another's un-bookmark" claim has a live
counterfactual. Proven fail-when-broken (stubbing un-bookmark! to a no-op turns the test red).
MINOR fixed: the id schema now requires a positive int (matching REST's ms/PositiveInt); the
degraded write-scope readback now surfaces :bookmarked (via a new backward-compatible ack-keys
arg on v2.write/readback, so a write-only caller still learns the resulting state).
MINOR deferred (pre-existing REST behavior, out of a tool-slice's scope, confirmed structurally):
archived content is bookmarkable but filtered from listings; the Trash collection is bookmarkable;
readback's :url key has no producer on this branch (lands with collection_write). Each is shared
with the existing REST bookmark endpoints; changing it means deciding REST semantics.
Fresh-JVM: bookmark-test 9 tests / 43 assertions / 0 failures; kondo 0/0; modules unchanged.
* MCP v2 tool: collection_write
Adds the v2 collection_write tool — create/update/move/archive collections.
Extraction: collections/update.clj lifts update-collection! out of
PUT /api/collection/:id — the four privates (move-collection!,
archive-collection!, move-or-archive-collection-if-needed!,
maybe-send-archived-notifications!) move verbatim (docstrings de-REST-ified,
notification alias renamed); the endpoint becomes a thin delegator through
the collections.core facade, mirroring the existing create-collection!
pattern. Verified move: bodies diff clean modulo whitespace/docstrings.
The foundation's REST-test changes for this file are all the BROWSE
extraction's repoints, so master's collections_rest api-test runs UNCHANGED
against this branch — that unchanged suite is the extraction check
(fresh-JVM: 202 tests, 941 assertions, green, incl. 23 tool tests).
Deferred to the content tool PR: echo-matches-concise-read-test (the write
echo vs get_content cross-check) — it requires tools/content.
Review question for this PR: one tool — is collection_write's contract right?
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BjQcjQx7NPGr3Rpf6QaG4b
Also lands (first consumer, per the common.clj slimming in the core PR):
- v2/resolve.clj grows collection resolution (resolve-collection-id{,-or-personal}).
- v2/write.clj grows `_write` method dispatch (dispatch-write + expand-clear).
- common.clj grows frontend-url. Tests moved alongside (resolve_test, write_test).
Review nits (round 1): fix the self-contradictory `name` field description
("Create only (editable on update)" -> "Required on create; editable on
update"); drop structuredContent from the write echo to match the stack's
text-only convention (no concrete programmatic consumer, no outputSchema).
CI kondo fix: the update-collection! extraction moved the only `tru` caller in
collections_rest/api.clj into collections/update.clj, leaving a dead
`[metabase.util.i18n :refer [tru]]` require — an unused-require warning, which
`mage kondo` (CI's check) exits nonzero on. Removed it.
--- Round-2 /review-pr response ---
IMPORTANT (archived:true + parent_id silently discarded the move) FIXED. The dispatch tested
archived first and returned, so a caller passing both got a trashed collection and no move, with
no signal — defeating the tool's "never believes an ignored field took effect" invariant. It is
now a teaching error naming the fix (move first then archive, or pass parent_id on a later
archived:false restore). Regression test added.
MINOR fixed: the four (is (tool-error ...)) assertions now match the specific expected message,
not any error text; the :name destructure was renamed (coll-name) so it no longer shadows
clojure.core/name.
Fresh-JVM: collection-test 24 tests / 65 assertions / 0 failures; kondo 0/0; modules unchanged.
--- Close-out M5 ---
MINOR (bare tool-error assertion): authority-level-update-test now asserts the specific "paid feature" message (parity with the create-side twin), so a wrong error text fails.
* MCP v2 prep: portable-dialect execution rework in the agent lib
The shared machinery behind every portable-MBQL write path — metabot's
construct_notebook_query today, and the v2 MCP tools (metric_write,
question_write, execute_query) as they land on the stack:
- metabot/tools/construct: execute-representations-query reworked (new
signature; numeric-id source detection; recovery-hint wrapping around
resolution failures).
- NEW metabot/tools/recovery_hints: the standard 'what to call next'
sentences appended to dialect teaching errors.
- agent-lib representations{,.repair}: the corresponding repair/resolve
pipeline changes (resolve.clj needed nothing beyond master's).
Master's post-fork pMBQL→'MBQL 5' doc/message renames are re-applied on
top of the pulled files (wholesale pulls from the foundation would have
reverted them).
Behavior check: master's agent-lib suites (repair_test, resolve_test) and
metabot tool tests run UNCHANGED and green in a fresh JVM.
Review question for this PR: does construct_notebook_query (the existing
metabot consumer) behave identically where not deliberately extended?
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BjQcjQx7NPGr3Rpf6QaG4b
Review fixes (adversarial review round 1):
- F1: the agent-api /v2/construct-query endpoint and the slackbot query tool
called execute-representations-query without :recovery-hint, so both v1
surfaces relayed bare pipeline errors where slice 03 had baked guidance into
the throw sites. Wired recovery-hints/recovery-hint into both (as the ns
docstring already claims every v1 entry point does). Proven: a URI-in-
source-table error now carries the "use the portable FK" sentence on the
agent-api path.
- F4: node-table-fks never collected a numeric field ref's target table, so it
was never permission-checked; the no-fk-path error then named it from the
un-sandboxed metadata provider — a table-name oracle for any probed id once a
numeric binder lands. Fall back to the bare id for the numeric case (matching
the vector case's own-name behavior). Dark today (no binder on this slice),
fixed before it can go live.
Test fix: the F1 change gave slackbot's execute-representations-query call a
second (:recovery-hint) arg, but slackbot_query_test stubbed the fn with 1-arg
lambdas — the extra arg broke the stubs (nil results). Made the 4 stubs
variadic ([external-query & _]) so they accept the opts map; production change
unchanged. Suite green (53 assertions).
--- Round-2 /review-pr response ---
CRITICAL (numeric field refs bypass the pre-repair permission check) FIXED. A bare numeric field
id in a query node was never resolved to its table, so node-table-fks skipped it and query-check
never gated the field's table. node-table-fks now emits the source-field as a tagged numeric ref;
resolve-table-fk resolves it to the field's table via the permission-aware metadata provider and
checks that table. Two regression tests: one asserts the FK-target table is permission-checked,
one asserts a forbidden FK-target table blocks the call.
CRITICAL (export-source-field-portable writes an un-permission-checked FK path) REFUTED, and the
refutation is closed by the fix above. The exported source-field is the FK-BEARING column, which
by definition lives on the source table — the same table node-table-fks now collects and
query-check gates. Verified in a fresh JVM: the exported field resolves to a column on the source
table, not the FK-target; denying perms on the target raises 403 before export runs. The reviewer
conflated the FK-target table with the FK-bearing column.
IMPORTANT fixed: the recovery_hints ns docstring no longer asserts wiring that does not exist (it
now states the v2 query slice must pass it, and that until then it has no production caller); the
:no-fk-path message and the :unknown-table key collision are corrected. MINOR fixes applied.
Fresh-JVM: construct-representations-test + repair-test 228 tests / 616 assertions / 0 failures;
kondo 0/0; modules unchanged.
* Read-check the numeric source-card unconditionally
The numeric `source-card:` branch guarded its read check on
`api/*current-user-id*` being bound, so a caller with no current user got the
card's database id back without any permission check. Its portable sibling,
`tools.u/get-card-by-entity-id`, read-checks unconditionally — two branches of
one function disagreeing on whether permissions apply.
The guard was not there to avoid a crash: `read-check` -> `mi/can-read?`
resolves Card perms against the current-user permission set and denies when
there is no user, so removing the guard fails closed.
Not reachable in production yet — nothing binds `*numeric-ids-allowed?*` to
true until the v2 query slice lands — but this branch is where the dialect is
authored, so it is where the check belongs.
Adds the numeric `source-card` coverage the other numeric branches already
had: database-id resolution, that the card row reaches `read-check`, a denied
card surfacing 403, the same with `*current-user-id*` nil as the regression
guard, and an unknown id surfacing `:unknown-card`. Also covers two branches
that had none: `:unknown-table-id` for a numeric `source-table` that resolves
to nothing (kept distinct from the portable form's `:unknown-table`, since the
two want different recovery vocabulary), and `with-recovery-hint` passing an
error through untouched when there is no hint to add.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nng1SV562GGqn1Qf8dQmpM
* Route numeric source-card through the read-checked content store
The numeric-id dialect accepts a bare numeric `source-card`, but
`import-mbql` matched only `portable-id?`, which requires a string. A
numeric id therefore never reached the ContentStore -- and on agent
paths the store is the `read-checked` one, so that lookup is where the
permission check lives. The only remaining check was the explicit
`read-check` in `resolve-database-id-from-first-stage`, which by
construction sees `stages[0]` only, and which a stage carrying both
`source-table` and `source-card` skips entirely (the `cond` takes the
`source-table` branch).
Downstream, the repair pass's cross-stage column inference runs for any
stage with a `source-card` and reports the card's column names when a
referenced column is missing, so an unchecked card surfaced its columns
to a caller without access to it.
Fix at the resolution chokepoint rather than at the first stage:
`import-mbql` now has a numeric `source-card` branch, gated on
`*numeric-ids-allowed?*` so the portable-only surface is untouched.
Because that walk recurses, this covers every stage and every join
instead of just the one the database-id lookup happens to inspect. The
new `import-card-by-id` mirrors `import-card-by-entity-id`, including
its cross-database guard, and returns the id it was handed -- the
routing, not the translation, is the point.
Also drop the `(when api/*current-user-id* ...)` guard on the numeric
card's `read-check` in `construct.clj`. The entity_id branch checks
unconditionally and the numeric table branch fails closed; only this one
failed open for an unauthenticated caller.
Verified against a real appdb with an unprivileged user: both shapes now
raise 403 with no column names in the message or ex-data, where before
they resolved and reported them. The new tests fail on the unfixed tree
and pass here.
Test evidence: resolve.mp-test 41 tests / 136 assertions;
construct-representations + repair + resolve 249 tests / 707 assertions;
metabot tools 28 tests / 386 assertions; new
construct-numeric-permissions-test 4 tests / 11 assertions. All green.
* Require numeric ids to denote something the query may reference
Three findings from the slice-04 review, all the same bug: the numeric
dialect treats an LLM-authored integer as a resource reference and acts
on it directly. The portable dialect never had to state these rules --
it resolves *names* through a metadata provider that is already scoped
to the query's database and filtered to visible rows, so "in this
database" and "a column the warehouse exposes" fall out of resolution.
A bare id skips all of it.
`visible-table` / `visible-field` put the contract back: everything the
numeric dialect accepts is looked up through the query's own provider,
and the active/visibility rules are re-applied on top, because
`metadata-spec->honey-sql` deliberately drops its filter for by-id
lookups (`active-only?` is `(not (or id-set name-set))`). That default
is right for trusted callers asking for a specific row and wrong for an
agent-authored id.
1. Cross-database tables. `resolve-table-fk` returned a `pos-int?`
verbatim, so a join could pull a table from another database into a
query. `api/query-check` does not close this: it asks whether the
caller may query table N, never whether N belongs to this query's
database, so it passes for anyone with access to the other database.
Traced on the unfixed tree, the check ran against the foreign table
(id 9, db_id 2) inside a query targeting db 1 and passed; the query
failed later only because repair could not find the table in the
wrong provider. The refusal was incidental, not a boundary.
`resolve/mp.clj` has cross-database guards for cards, measures and
segments and none for tables, because the portable path never needed
one.
2. Sensitive / inactive / retired fields, and inactive or hidden tables,
were reachable by id and came back as result columns. The portable
dialect cannot name them at all -- they are absent from the by-name
fetch.
3. Table-existence oracle. The numeric branch of
`resolve-database-id-from-first-stage` distinguished "no such table"
from "table you cannot see" before any permission check had run,
which enumerates table ids instance-wide. It now read-checks and
reports both the same way. This lookup cannot go through the provider
-- it is what decides which provider to build -- so the check lives
there.
`assert-numeric-ids-are-referenceable!` raises the agent-facing errors
ahead of the permission sweep; `resolve-table-fk` independently refuses
the same ids, so the sweep fails closed even if a future caller skips
the assertion. Both new error keys already have recovery hints on both
surfaces. No-ops entirely on the portable-only surface.
Test evidence: 9 tests / 21 assertions in the new ns, all green, and 11
assertions across all three findings fail on the unfixed tree. Existing
suites unchanged: construct-representations + repair + resolve +
resolve.mp, 290 tests / 843 assertions, 0 failures.
Also corrects two wrong premises the tests caught: a query whose only
source is a foreign table is not a violation (the provider is simply
built for that database), and
`with-non-admin-groups-no-root-collection-perms` does not gate warehouse
tables, so the oracle test revokes data permissions instead.
* Read-check numeric metric, segment, and measure refs too
Follow-up to the numeric `source-card` fix, from review feedback: the
same gap existed for every other content reference, not just the source.
`import-mbql` guarded its content branches with `portable-id?`, which is
string-only. A numeric `["metric" {} 42]`, `["segment" {} 42]`, or
`["measure" {} 42]` therefore matched no branch at all -- it was never
resolved, so it never reached `import-fk`, so the read-checked content
store never saw it. Same shape as the `source-card` leak, across seven
more branches.
Rather than give each branch a numeric twin -- fourteen branches, and a
later one easy to forget -- the guard itself widens: `content-ref?` is
"a portable entity_id, or a bare id on the numeric-id surface", and
every content branch shares it. `import-fk` then dispatches numerics per
model, so `import-card-by-id` gains `import-measure-by-id` and
`import-segment-by-id` siblings, each carrying the same cross-database
guard as its portable counterpart. Metric refs resolve as Cards, which
is what they are.
Also from review:
- `:unknown-card-id` for a numeric card miss, matching `export-card-by-id`
and the `:unknown-table-id` precedent -- a numeric miss and a portable
miss want different recovery advice. Updates the one test that asserted
the old key.
- `resolve-database-id-from-first-stage`'s docstring said `:unknown-table`
where the code throws `:unknown-table-id`.
- Dropped a `^clojure.lang.ExceptionInfo` hint on `with-recovery-hint`
that propagates nowhere.
- `numeric-field-id-error!` no longer claims lib rejects bare numeric ids,
which stopped being true once the numeric surface landed.
The read-check on the numeric `source-table` lookup moved into
`metabot.db/readable-active-table-database-id`, keeping Toucan calls in
the db namespace where this branch put them.
Test evidence: 308 tests / 883 assertions across
construct-numeric-permissions, construct-representations, repair,
resolve, and resolve.mp -- all green.
* Correct a test comment about the PathSchema stack overflow
The comment called it a nested-quantifier backtracking problem. It is
not: `path-char-rx` excludes `/`, so segment boundaries are forced and
there is nothing to backtrack over — a 218-character failing input
matches in 0 ms. java.util.regex simply recurses once per quantifier
iteration on a *successful* match, so depth grows linearly with segment
count (~2000 segments overflows a default stack on its own). Atop the
pipeline's own frames the threshold drops far enough to hit it here.
Same conclusion either way — pre-existing, dev/test only, unrelated to
this branch — but the stated mechanism was wrong and would have sent the
next reader after a bug that isn't there.
* Say why the visibility lookups swallow every exception
Review noted the catch-all was undocumented. It is deliberate: this pass
decides only what a numeric id may name, so failing closed is the safe
direction and a provider outage is diagnosed elsewhere. Say so, rather
than leaving a bare `(catch Exception _ nil)` to read as an oversight.
* Drop a redundant do in a test helper
`try` already has an implicit body; kondo flagged it.
* Build the test fixtures with Lib instead of mt/mbql-query
`mt/mbql-query` is deprecated since 0.61.0, and clj-kondo exits 2 on
warnings — so the two call sites this ns added were failing the
Clj-Kondo CI job outright, not merely producing noise. Its own docstring
says to use Lib for new tests; done.
Same queries, same result columns: the victim card keeps its named
`SecretMargin` expression and its aggregation, which is what makes a
column-name leak unmistakable in the assertions.
Full-repo kondo now reports 0 errors, 0 warnings (was 2 warnings, exit 2).
* Close four numeric-id permission gaps found in review
All four are in the numeric-id handling this branch added.
**Two-element field refs escaped collection.** `node-table-fks` checked the
portable form in slots 2 *and* 1 but the numeric form in slot 2 only. Repair
inserts the options map later, so an LLM-authored `["field", 42]` still carries
its target in slot 1 when the collection pass runs — that shape reached
resolution unchecked. Both forms now read both slots.
**Field visibility ignored the parent chain.** A JSON column unfolds into child
fields carrying their own visibility, so an ordinary-looking child of a
`:sensitive` parent was referenceable by id — reaching inside a column the
warehouse marked unqueryable. `visible-field` now walks `:parent-id` and
requires every ancestor to be visible too, bounded so a corrupt chain cannot
spin.
**Unreadable and missing content were distinguishable.** A denial surfaced
`read-check`'s 403 while an absent id produced a 400 `:unknown-…`. Content ids
are sequential and easy to guess, so the status code alone told a caller whether
a hidden card, measure, or segment exists — the same oracle already closed for
the numeric table lookup. `maybe-read-check` now collapses a 403 to `nil`, which
every resolver already translates into its not-found error, so one change covers
all six store methods; non-403 exceptions still propagate, since those are real
errors rather than permission answers. `resolve-database-id-from-first-stage`
read-checks outside the store, so it got the same collapse.
**The table lookup denied users it should have allowed.** `mi/can-read?` for a
Table is polymorphic over the instance, and its published-collection branch reads
`:is_published` and `:collection_id`.
`readable-active-table-database-id` selected only `:id`/`:db_id`, so that branch
evaluated against absent keys and a user whose access came through a published
collection was told the table does not exist. It selects the whole row now. This
one was a fail-closed regression introduced by the previous commit's oracle fix.
Tests: four new cases, each verified to fail on the pre-fix tree. Existing tests
that asserted the 403 shape are updated to the collapsed one — their intent (the
check runs, fails closed, does not depend on a bound user, never leaks an
entity_id) is unchanged and still asserted.
319 tests / 935 assertions green across construct-numeric-permissions,
construct-representations, repair, resolve, resolve.mp, and content-store.
Full-repo kondo: 0 errors, 0 warnings.
* Close the query-check oracle and cover scoped content refs
Second review round. Both blocking findings are the same root cause as the
one the previous commit fixed — `mi/can-read?` is strictly weaker than
`mi/can-query?` for a Table, since only `can-read?` carries the
`manage-table-metadata :yes` disjunct — reached by two routes that fix did
not cover.
**The table oracle was still open one line later.** The previous commit
closed it at `readable-active-table-database-id`, which gates on
`can-read?`. `check-source-table-query-permissions!` runs immediately after
and gates on `api/query-check` → `can-query?`. So a metadata-only user
passed every earlier gate and got a bare 403 there, while a nonexistent id
got a 400 — confirming the table exists, is active, and is in this database.
`query-check-table!` now collapses a denial on a *numeric* reference into the
same `:unknown-table-id` an absent id produces. The portable path keeps its
403: its references are unguessable names, so there is nothing to enumerate
and the accurate status is worth more.
**Numeric measure and segment refs never reached a query-check at all.**
`node-table-fks` matched maps and `["field" …]` vectors, so `["measure" {} 44]`
and `["segment" {} 43]` fell through both branches — the tables they are
scoped to were never collected, and `assert-numeric-ids-are-referenceable!`
iterates the same collection, so it skipped them too. They do get
`api/read-check` through the store, but `can-read?` on both models delegates
to the Table's `can-read?` and lands on the same weaker gate. Adds a
`::scoped-content` tag mirroring the existing `::field-id` one, resolved to
the owning table through the read-checked store. Metric refs are Cards and
are deliberately not covered: a card pulls in whatever tables its own query
names, which this pre-flight sweep cannot see, and the QP re-checks them at
execution.
Also from the same review:
- `snippet-id` had been widened to `content-ref?`, but no resolver implements
`NativeQuerySnippet` — a numeric one fell through `import-fk`'s model tests
to `not-implemented!` and surfaced as a bare 501 with no `:agent-error?`,
never reaching the store. Narrowed back to `portable-id?`; `content-ref?`'s
docstring no longer claims snippets.
- `active-table-database-id` was dead: the pre-oracle-fix version its checked
sibling superseded within this branch, with the shorter name that reads as
the obvious choice. Deleted. The one test "stubbing" it was stubbing the
dead var and hitting the real app DB, passing only because id 999 happened
not to exist — retargeted to the function actually under test.
- `*numeric-ids-allowed?*`'s docstring described its readers in the future
tense; this PR is what added them. Rewritten to state the contract instead
of the rollout.
- `import-card-by-id`'s docstring still said a denial becomes "a 403", which
the previous commit's collapse made false.
- `visible-field`'s docstring now says the parent walk checks each ancestor's
visibility but not its `:table-id`, and why that is sound.
Tests: two new cases, both mutation-checked. The segment one deliberately puts
the segment on a different table than the stage's `source-table`, so it cannot
be satisfied by the source table's own check — the first version could, and
passed with the fix reverted.
321 tests / 943 assertions green. Full-repo kondo: 0 errors, 0 warnings.
* Keep the export fallback suppressed for a refused card
The 403 collapse in the previous commits removed a signal an internal
caller depended on. `llm-shape/export-query-for-llm` documents that "a
permission-refused export renders nothing at all rather than the
fallback", and implemented that by catching the 403. Once the content
store turned a denial into `nil`, that 403 stopped arriving, so a card
the caller may not read fell through to `query-edn-fallback` and printed
the raw query map.
No entity_id leaked — the export still refuses to emit one, so the
fallback showed `:source-card 1`, the numeric id the caller already
supplied. But the caller's contract changed, and the intent of that catch
was to render nothing.
The two requirements are not in conflict. Agent-facing, a denial and a
miss must be indistinguishable: same status, same error key, so a
guessable id cannot be used to probe for hidden content. Internally,
callers still need to tell them apart to decide whether falling back is
appropriate. So the collapse keeps the uniform error and records the
refusal out of band, in `*last-lookup-refused?*`, which says only that
*some* lookup was refused — never which row, so it cannot itself become
an oracle.
Caught by `llm-shape-test/transform-query->text-source-card-permission-test`,
which I had not run: it lives outside the affected-set I had been
checking. The second review round flagged this mechanism and I recorded
it as out of scope; it was not.
175 tests / 768 assertions green across llm-shape, content-store,
construct-numeric-permissions, construct-representations, and
resolve.mp. Full-repo kondo: 0 errors, 0 warnings.
* Collapse the read-check denial for numeric lookups only
The collapse was too broad. It lived in `maybe-read-check`, which every
content lookup passes through, so it applied to entity_id lookups as well
as numeric ones — and the two surfaces want opposite things.
Numeric ids are sequential and trivially guessable, so a distinguishable
denial is an existence oracle and the 403 has to become the same
not-found a missing id produces. A 21-character NanoID is not guessable,
so there is nothing to enumerate, and callers there depend on the accurate
status: `POST /api/agent/v2/construct-query` returns 403 for a metric
whose card the caller cannot read, and `llm-shape/export-query-for-llm`
suppresses its EDN fallback on one.
Collapsing both meant the entity_id path started answering 400
`:unknown-card` where it had answered 403 — caught by CI in
`agent-api.api-test/construct-query-permission-checks-test`, which asserts
the 403 for exactly that case.
`maybe-read-check` now takes `collapse-denial?`: true for the three
`-by-id` methods, false for the three `-by-entity-id` ones. The refusal is
still recorded in `*last-lookup-refused?*` either way, so callers that need
to know a denial happened without depending on the status keep working.
This is the second time the chokepoint's breadth cut the wrong way — it is
what made one edit cover all six store methods, and also what swept in a
surface that did not want the change. The split keeps the single
chokepoint and makes the difference explicit at it.
462 tests / 1698 assertions green across construct-numeric-permissions,
construct-representations, repair, resolve, resolve.mp, content-store,
llm-shape, and agent-api. Full-repo kondo: 0 errors, 0 warnings.
---------
Co-authored-by: Eric Normand <ericwnormand@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* mcp-v2.05 metric_write (#81249)
* MCP v2 tool: bookmark_content
Adds the v2 bookmark_content tool — per-user favorites, keyed by (type, id).
Extraction: bookmarks/api.clj factors bookmark!/un-bookmark!/bookmarked? out
of the REST endpoints so REST and MCP share them. The POST endpoint now
creates via app-db/select-or-insert! (concurrency-safe: a racing duplicate
returns the existing row instead of tripping the unique constraint); its 400
on already-bookmarked is unchanged. REST tests extended, existing pass.
Unlike REST, both tool directions are idempotent (see ns docstring for why),
both read-check the item, and the name echo is gated on the read scope so a
write-only token can't use it as a name oracle.
Tests: 9 tool tests + bookmarks REST suite green.
Review question for this PR: one tool — is bookmark_content's contract right?
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BjQcjQx7NPGr3Rpf6QaG4b
Also lands (first consumer, per the common.clj slimming in the core PR):
- NEW v2/resolve.clj — entity-id / numeric-id resolution + the read-check
pairing with the not-found collapse (+ resolve_test, moved from common_test).
- NEW v2/write.clj — readback (the scope-gated write echo).
CI guard: this slice's mcp -> bookmarks edge put `bookmarks` into the
modules-that-trigger-driver-tests set (mage's module-graph ratchet, 44 -> 45).
Bookmark logic cannot affect driver behavior — the edge is a consumer edge —
so `bookmarks` joins driver-affecting-overrides rather than bumping the
ratchet. Full mage suite green (82 tests).
Review nit (round 1): fetch-item's flavor-mismatch teaching error printed
"is a null" for a card type outside question/model/metric; guard the nil case
with a type-agnostic message.
--- Round-2 /review-pr response ---
IMPORTANT (per-user-test could not fail on its named assertion) FIXED. The test called the tool
bare, so a tool-level error was discarded and the assertions passed regardless. It now routes
both calls through tool-result (throws on tool error) and has :lucky bookmark-then-un-bookmark
its own row, so the "one user's bookmark is invisible to another's un-bookmark" claim has a live
counterfactual. Proven fail-when-broken (stubbing un-bookmark! to a no-op turns the test red).
MINOR fixed: the id schema now requires a positive int (matching REST's ms/PositiveInt); the
degraded write-scope readback now surfaces :bookmarked (via a new backward-compatible ack-keys
arg on v2.write/readback, so a write-only caller still learns the resulting state).
MINOR deferred (pre-existing REST behavior, out of a tool-slice's scope, confirmed structurally):
archived content is bookmarkable but filtered from listings; the Trash collection is bookmarkable;
readback's :url key has no producer on this branch (lands with collection_write). Each is shared
with the existing REST bookmark endpoints; changing it means deciding REST semantics.
Fresh-JVM: bookmark-test 9 tests / 43 assertions / 0 failures; kondo 0/0; modules unchanged.
* MCP v2 tool: collection_write
Adds the v2 collection_write tool — create/update/move/archive collections.
Extraction: collections/update.clj lifts update-collection! out of
PUT /api/collection/:id — the four privates (move-collection!,
archive-collection!, move-or-archive-collection-if-needed!,
maybe-send-archived-notifications!) move verbatim (docstrings de-REST-ified,
notification alias renamed); the endpoint becomes a thin delegator through
the collections.core facade, mirroring the existing create-collection!
pattern. Verified move: bodies diff clean modulo whitespace/docstrings.
The foundation's REST-test changes for this file are all the BROWSE
extraction's repoints, so master's collections_rest api-test runs UNCHANGED
against this branch — that unchanged suite is the extraction check
(fresh-JVM: 202 tests, 941 assertions, green, incl. 23 tool tests).
Deferred to the content tool PR: echo-matches-concise-read-test (the write
echo vs get_content cross-check) — it requires tools/content.
Review question for this PR: one tool — is collection_write's contract right?
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BjQcjQx7NPGr3Rpf6QaG4b
Also lands (first consumer, per the common.clj slimming in the core PR):
- v2/resolve.clj grows collection resolution (resolve-collection-id{,-or-personal}).
- v2/write.clj grows `_write` method dispatch (dispatch-write + expand-clear).
- common.clj grows frontend-url. Tests moved alongside (resolve_test, write_test).
Review nits (round 1): fix the self-contradictory `name` field description
("Create only (editable on update)" -> "Required on create; editable on
update"); drop structuredContent from the write echo to match the stack's
text-only convention (no concrete programmatic consumer, no outputSchema).
CI kondo fix: the update-collection! extraction moved the only `tru` caller in
collections_rest/api.clj into collections/update.clj, leaving a dead
`[metabase.util.i18n :refer [tru]]` require — an unused-require warning, which
`mage kondo` (CI's check) exits nonzero on. Removed it.
--- Round-2 /review-pr response ---
IMPORTANT (archived:true + parent_id silently discarded the move) FIXED. The dispatch tested
archived first and returned, so a caller passing both got a trashed collection and no move, with
no signal — defeating the tool's "never believes an ignored field took effect" invariant. It is
now a teaching error naming the fix (move first then archive, or pass parent_id on a later
archived:false restore). Regression test added.
MINOR fixed: the four (is (tool-error ...)) assertions now match the specific expected message,
not any error text; the :name destructure was renamed (coll-name) so it no longer shadows
clojure.core/name.
Fresh-JVM: collection-test 24 tests / 65 assertions / 0 failures; kondo 0/0; modules unchanged.
--- Close-out M5 ---
MINOR (bare tool-error assertion): authority-level-update-test now asserts the specific "paid feature" message (parity with the create-side twin), so a wrong error text fails.
* MCP v2 tool: metric_write
Adds the v2 metric_write tool — create/update metrics from an MBQL
definition (either dialect) or a query_handle.
Extraction: queries/card_write_checks.clj lifts the pre-write check stack
(check-card-can-be-saved!, check-no-save-cycle!, actual-collection-id,
check-allowed-to-create-card!, check-allowed-to-update-card!) out of
queries_rest/api/card.clj (−98; taken wholesale — zero master drift) so
REST and MCP run identical checks. queries/core.clj facade HAND-MERGED:
master's post-fork parameter-template-tag? export kept; foundation's
infer-metadata-with-model-overrides export skipped (no consumer here yet).
Projection ownership fix (the WORKLOG's load-order landmine, hit live):
the :metric key projection moves from content.clj's load-time side effect
into this tool — the type's owner — so metric_write works without
tools/content loaded. The content PR drops its copy.
Deferred tests (see WORKLOG ledger): the execute_query handle flow → query
PR; the get_content echo-parity and definition round-trip → content PR.
card_test's param-fields-hydrated-test is a post-fork master addition and
stays (foundation's deletion is fork lag, not intent).
Fresh-JVM: metric tool suite + agent-lib suites + full card REST suite
green (1155+ assertions across the adjudication runs).
Review question for this PR: one tool — is metric_write's contract right?
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BjQcjQx7NPGr3Rpf6QaG4b
Also lands (first consumer, per the common.clj slimming in the core PR):
- NEW v2/queries.clj — the representations-pipeline entry point, portable-dialect
resolution, and the full query-handle store (mint/encode/decode/resolve, the
save-path variant this tool consumes; the execute tools add the minting call
sites in the query PR). Handle tests move alongside as queries_test.
Review finding (round 1, both reviewers): check-metric-shape! rejected only a
PURE-native query (lib/native-only-query?); a native stage nested with an MBQL
stage slipped through to the generic can't-be-saved message (or an incidental
editable? rejection that would open if :native-permissions were ever populated
server-side). Tightened to the whole-tree query-guards/native-query? — the same
guard question_write uses — so any native stage is refused with the metric-
specific message. Regression test added.
--- Round-2 /review-pr response ---
CRITICAL (update! never refreshes result_metadata after a definition swap) REFUTED. Verified in a
fresh JVM: creating a metric then updating it to a different definition (count -> sum with a month
breakout) refreshes result_metadata from a 1-column to a 2-column set. queries/update-card!'s hook
recomputes result_metadata whenever dataset_query changes, so the extraction did not lose the
recompute step. Pinned with metric-write-lifecycle-test asserting the before/after metadata differ.
IMPORTANT fixed: destructiveHint is now true (archived:true trashes the metric; matches the sibling
collection_write tool); the tool no longer emits structuredContent against the stack's text-only
convention; an id on a create call is now rejected rather than silently ignored; collection_position
requires a positive int (matching the downstream contract).
MINOR: applicable fixes applied; the remaining two (a duplicated frontend-url, a kondo allowlist
churn) deferred as cosmetic.
Fresh-JVM: metric-test 25 tests / 79 assertions / 0 failures; kondo 0/0; modules unchanged.
* MCP v2 prep: portable-dialect execution rework in the agent lib
The shared machinery behind every portable-MBQL write path — metabot's
construct_notebook_query today, and the v2 MCP tools (metric_write,
question_write, execute_query) as they land on the stack:
- metabot/tools/construct: execute-representations-query reworked (new
signature; numeric-id source detection; recovery-hint wrapping around
resolution failures).
- NEW metabot/tools/recovery_hints: the standard 'what to call next'
sentences appended to dialect teaching errors.
- agent-lib representations{,.repair}: the corresponding repair/resolve
pipeline changes (resolve.clj needed nothing beyond master's).
Master's post-fork pMBQL→'MBQL 5' doc/message renames are re-applied on
top of the pulled files (wholesale pulls from the foundation would have
reverted them).
Behavior check: master's agent-lib suites (repair_test, resolve_test) and
metabot tool tests run UNCHANGED and green in a fresh JVM.
Review question for this PR: does construct_notebook_query (the existing
metabot consumer) behave identically where not deliberately extended?
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BjQcjQx7NPGr3Rpf6QaG4b
Review fixes (adversarial review round 1):
- F1: the agent-api /v2/construct-query endpoint and the slackbot query tool
called execute-representations-query without :recovery-hint, so both v1
surfaces relayed bare pipeline errors where slice 03 had baked guidance into
the throw sites. Wired recovery-hints/recovery-hint into both (as the ns
docstring already claims every v1 entry point does). Proven: a URI-in-
source-table error now carries the "use the portable FK" sentence on the
agent-api path.
- F4: node-table-fks never collected a numeric field ref's target table, so it
was never permission-checked; the no-fk-path error then named it from the
un-sandboxed metadata provider — a table-name oracle for any probed id once a
numeric binder lands. Fall back to the bare id for the numeric case (matching
the vector case's own-name behavior). Dark today (no binder on this slice),
fixed before it can go live.
Test fix: the F1 change gave slackbot's execute-representations-query call a
second (:recovery-hint) arg, but slackbot_query_test stubbed the fn with 1-arg
lambdas — the extra arg broke the stubs (nil results). Made the 4 stubs
variadic ([external-query & _]) so they accept the opts map; production change
unchanged. Suite green (53 assertions).
--- Round-2 /review-pr response ---
CRITICAL (numeric field refs bypass the pre-repair permission check) FIXED. A bare numeric field
id in a query node was never resolved to its table, so node-table-fks skipped it and query-check
never gated the field's table. node-table-fks now emits the source-field as a tagged numeric ref;
resolve-table-fk resolves it to the field's table via the permission-aware metadata provider and
checks that table. Two regression tests: one asserts the FK-target table is permission-checked,
one asserts a forbidden FK-target table blocks the call.
CRITICAL (export-source-field-portable writes an un-permission-checked FK path) REFUTED, and the
refutation is closed by the fix above. The exported source-field is the FK-BEARING column, which
by definition lives on the source table — the same table node-table-fks now collects and
query-check gates. Verified in a fresh JVM: the exported field resolves to a column on the source
table, not the FK-target; denying perms on the target raises 403 before export runs. The reviewer
conflated the FK-target table with the FK-bearing column.
IMPORTANT fixed: the recovery_hints ns docstring no longer asserts wiring that does not exist (it
now states the v2 query slice must pass it, and that until then it has no production caller); the
:no-fk-path message and the :unknown-table key collision are corrected. MINOR fixes applied.
Fresh-JVM: construct-representations-test + repair-test 228 tests / 616 assertions / 0 failures;
kondo 0/0; modules unchanged.
* metric_write update: hydrate moderation reviews like PUT /api/card/:id does
`update-card!` un-verifies a verified card whose query changes by reading `:moderation_reviews`
off the card-before. `update!` passed the bare write-check row, so a verified metric kept its
Verified badge after a definition swap through the tool, unlike through REST. The card-before is
now hydrated the way the REST handler hydrates it.
* Pin the create-side permission stack
`check-allowed-to-create-card!` is the only thing stopping a caller saving a metric into a collection
they cannot curate, or over a query they have no data permission to run — `create-card!` performs no
check of its own. Every create test in this namespace runs as an admin, for whom those checks are
vacuous, so deleting the line left the suite green.
Verified fails-when-broken: removing the call fails both branches of this test.
Claude-Session: https://claude.ai/code/session_018PNCQxp9kCypqE3dLQS9va
* Confine this branch's Toucan calls to db namespaces
New policy: t2 is called only from db.clj namespaces. The two source files
this branch adds each held a couple of direct Toucan calls; both move out.
metabase.queries.db takes the Dashboard collection_id lookup and the
:in_dashboards hydration out of card-write-checks. The local
dashboard-id->collection-id alias was pure indirection once the query moved,
so both sites now call the db fn directly.
metabase.mcp.v2.db takes the Card read-back and the moderation-review
hydration out of the metric tool. The comment explaining why the card-before
is hydrated stays at the call site, where the reasoning belongs.
Nothing outside this branch is touched: resolve.clj and redaction.clj came in
on mcpv2-01-core, and metric_test.clj's Toucan calls are read-back assertions
that deliberately bypass the production helpers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017tqiaJmaEXSfaWGUZ4LQLv
* Teach the agent about an empty metric definition
`definition: {}` came back as the sanitized "Internal error", which is exactly
what normalize-definition's docstring promises never to happen for a malformed
definition.
The arg schema is an open map, so `{}` validates. normalize-query's first cond
branch short-circuits an empty query to `{}` before its strict validation runs,
so nothing throws there either. `{}` is truthy, so create!'s "pass the metric's
query" fallback never fires. can-save? is the one that finally objects, by way
of a mu/defn input-schema violation the tool can only report as an internal
error.
Guard the normalized result instead. Both create! and update! reach it through
resolve-definition, so one check covers both.
Strict MCP clients are the ones that hit this: they fill every declared
property, so "no definition" arrives as `{}` rather than an absent key.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017tqiaJmaEXSfaWGUZ4LQLv
* Let a metric_write caller choose the display
The tool hard-coded :display :scalar on create and never wrote it on update,
with no display argument at all. But the shape gate deliberately accepts one
grouping, so `count by month` saved as a metric page rendering a single number,
unfixable through MCP. agent-api's create_metric and update_metric both took an
optional display, so this was a regression rather than a standing limitation.
Accept an optional display on both methods, defaulting to scalar. The accepted
values are enumerated so an LLM-invented display is an argument error instead of
junk on the card.
The enum is a local copy of agent-api's ::card-display rather than a require:
that namespace is the v1 REST surface this tool replaces and GHY-4250 removes,
and pinning v2 to it for seventeen strings buys a dependency we would have to
undo. No canonical list exists to share instead — four other namespaces already
keep their own.
Echo the written display back. It is a detailed-projection key, but the echo is
the only place a caller sees the scalar they never asked for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017tqiaJmaEXSfaWGUZ4LQLv
* Fold mcp.v2.db into mcp.db to satisfy the app-DB query linter
CI's `./bin/mage kondo` (full tree) flagged src/metabase/mcp/v2/db.clj:12:
"Application database query calls like `t2/select-one` must live in
metabase[-enterprise].<module>.db namespaces". The rule matches
`metabase.<module>.db`, and `metabase.mcp.v2.db` is a segment deeper, so
the module's db namespace already existed one level up as
`metabase.mcp.db` and this was a second one the rule cannot see.
Moved `hydrate-moderation-reviews` into metabase.mcp.db beside the
existing `hydrate-notification`, repointed metric.clj's two call sites
(`card` folds into the existing `select-one-by-id`), and deleted the
duplicate namespace.
Missed locally because the previous run linted only the files the merge
touched; this file came in with the branch and was never in that set.
Reproduced and fixed against the full-tree run CI uses: 0 errors,
0 warnings. Metric suite 30 tests, 95 assertions green. Modules config
unchanged.
---------
Co-authored-by: Eric Normand <ericwnormand@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Follow the base's contracts after the merge
Three changes the base made to shared namespaces that `question_write` had to
be adapted to; each merged textually clean and broke silently.
`v2.write/readback` dropped its 3-arity in favour of a required `ack-keys`, so
every write threw an ArityException. Passes nil, like collection_write.
`registry/call-tool` now returns `{:result …}`/`{:error …}` rather than raw MCP
content. The test namespace read `:isError`/`:content` off the top level, so its
error-path assertions failed and its success-path assertions passed vacuously. A
local `call-tool` unwraps `:result` and renders a pre-dispatch registry refusal
into the same content shape, which is what the assertions here are written
against.
The `:metabase/t2-query-namespace` rule confines app-db calls to a module's `.db`
namespace, and this file's three `t2/select-one` calls were the tree's only
violations. They go through `mcp.db/select-one-by-id`.
Verified: full `./bin/mage kondo` clean, and the twelve MCP v2 namespaces run
199 tests / 835 assertions green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0147UgqNGEjk9nV4jRnt9oS2
* Fix duplicate_content gate laundering and copy-vs-create semantics
`copy-question!` dissoc'd `:document_id` when building the copy. That column
is not placement -- it is the Card read gate. `mi/can-read? :model/Card`
conjoins `parent-document-permits?`, which short-circuits to `true` on a nil
`document_id`, so the copy was adjudicated by collection permissions alone
and disclosed, permanently and to every reader of the destination, material
the Document's content gate exists to withhold. `POST /api/card/:id/copy`
preserves the column and `create-card!`'s `data-keys` allowlist carries it,
so this tool was the only path that laundered the gate away.
Refuse a document-scoped source with a teaching error rather than carrying
the column: `copy-document!` re-parents such cards precisely because their
`collection_id` tracks the document rather than standing alone, so a copy
that kept `document_id` would claim membership in a document whose body
never references it. Neither outcome is the "standalone question in
collection-id" the caller asked for, so name the operation that works.
Also drop `check-allowed-to-create-card!` in favour of `api/create-check` +
`create-card!`, matching REST `/copy` and `clone-card!` (UXW-5037). Copying
is not authoring -- the query comes from a row the caller already passed a
read check on -- and the authoring stack refused a user who could read and
run a native card but lacked native query-building perms. Destination
create-permission is still enforced.
Tool description: drop the per-type create scopes GHY-4225 removed, stop
scoping the `uncopied` report to deep copies (`card->decision` discards
before it consults `deep-copy?`, so shallow copies drop cards too), and
state the document-scoped refusal.
`is_deep_copy` now rejects only an explicit `true` on a non-dashboard;
`false` is the documented default and the strict inputSchema forces clients
to send it. `readback` gets explicit ack-keys `[:type :uncopied_count]` so a
write-only token learns a copy was partial -- `:uncopied` itself must not
survive, since `redact` only redacts *unreadable* cards and an
archived-but-readable one keeps its real name.
Tests: pin the document-gated refusal through `mi/can-read?` as a user who
cannot read the parent document; pin the native-card copy; pin shallow-copy
`uncopied`; pin the degraded key set. Fix two existing tests -- the
"nothing was written" block counted the source collection the copy never
targets, and the destination-permission test asserted a bare `:isError`.
* Check run permission before inferring a card's result metadata
`question_write`'s update path ran the REST check stack before attaching
`:result_metadata`, so `check-update-result-metadata-data-perms` saw a patch
that did not contain the key and never fired. On an update carrying only
`column_metadata`, `check-allowed-to-modify-query` did not fire either — it is
gated on `:dataset_query` changing — so nothing checked data permissions at all
before inference ran over the stored query.
A caller with curate access to the card and no data permission on its query
could therefore annotate its columns, and the inference teaching error named the
query's real columns to them one guess at a time. The write succeeded, storing
the fully inferred metadata.
The create path never had this hole: `check-allowed-to-create-card!` runs the
run-permission check unconditionally before inference. This gives update the
same check, gated on the inference actually happening. It also covers what the
REST endpoint checks on a supplied `result_metadata`, because the overrides only
annotate columns the inference produced — they cannot name a table the query
does not.
`check-allowed-to-run-query!` is the pair `check-allowed-to-create-card!`
already inlined, extracted so both paths call the same thing.
Verified fails-when-broken: removing the call fails both new tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0147UgqNGEjk9nV4jRnt9oS2
* Derive protected-resource scopes from the resource path
The bare `/.well-known/oauth-protected-resource` endpoint advertised the v1
canonical resource with the v2 scope set: 6 scopes instead of 22. A client that
probes the bare path -- the behavior BOT-1617 added the endpoint to serve --
read 6, requested those 6, and received a token authorizing nothing on the
resource it had asked about. Empty `tools/list`, and no in-product way to widen
the grant afterwards. Worse, the wrong pairing became durable: the authorization
code grant copies the code's scope onto the access and refresh tokens without
re-consulting the client's registered ceiling, so recovery meant re-consenting
from scratch.
The cause was `protected-resource-metadata` taking the scope set as a separate
`scopes-fn` parameter. Three call sites passed a closure over their own path;
the fourth passed `mcp-resource-scopes` bare, which hit a 0-arity overload
hardcoding the v2 path. Both spellings type-check, and nothing at the call site
says "v2" -- the sibling accessor `mcp-canonical-path` returns the v1 path, so
the two read as interchangeable ways to name the MCP resource.
Rather than correct the fourth call site, drop the parameter: the function now
derives scopes from its own `resource-path`, so the resource and its scope set
cannot disagree. Slice 21 retires v1 and collapses this function; with the
parameter gone it cannot reintroduce the mismatch by overlooking an endpoint.
Delete the 0-arity overload, a delegating arity hiding a non-nil default. Its
only other caller was `default-grant-scopes`, where the union made it a provable
no-op -- `v2-scopes` is a subset of `all-scopes`, verified against a live
instance.
Also on the metadata/narrowing path:
- Delete the `:extra-scopes` plumbing. `registered-opt-in-scopes` read the key
off every registered tool, but `register-tool!`'s closed allowlist rejects it,
so the function provably returned the empty set. Since `register-tool!` runs
at namespace load from a top-level `deftool`, and those namespaces load from
the route table at boot, the first tool to use the documented key would have
taken down the entire v2 MCP surface rather than just that tool. This also
removes the only `mcp.core -> mcp.v2.registry` require.
- Narrow to the union of what every named resource accepts. RFC 8707 permits
several resource indicators and the token must work against each, but the
check asked only whether v2 was named *at all* and then applied that single
answer to the whole request. Naming a v1 alias alongside v2 discarded every
v1-only scope, order-independently -- contradicting the comment two lines
above it and silently shrinking a grant the user then approved.
- Canonicalize resource URIs via `.getAuthority` rather than `.getHost`.
`getHost` is nil for hosts `java.net.URI` considers non-conformant, notably
any containing an underscore -- routine for Docker Compose and internal k8s
service names, and accepted by Metabase's own `u/url?` as a Site URL. On such
an instance no indicator ever matched and narrowing was disabled instance-wide
and silently. Userinfo stripping, host case folding and default-port elision
are preserved.
- Normalize a bare-string `resources` argument. The endpoint schema admits one,
and `keep` over a String iterates characters, none of which canonicalize --
which would skip narrowing entirely. Latent today only because oidc-provider
vectorizes upstream; a security control should not depend on a coercion one
layer away.
- Answer RFC 6749 `invalid_scope` when narrowing removes every requested scope.
`narrow-scope-to-resource` returns nil both for "nothing was requested" and
"nothing survived", and the call site treated them alike -- rendering a consent
screen listing no permissions and minting a zero-scope token, a handshake that
looks successful and yields an empty tool list with no diagnostic.
Tests: assert, for every protected-resource endpoint including the bare one,
that `scopes_supported` equals the scope set for the `:resource` the response
itself advertises -- one invariant covering all four, keyed off the response so
the two cannot drift. The previous tests could not catch this: the bare-path
test omitted `scopes_supported` entirely and the other asserted only
`sequential?`, both of which stay green with the feature deleted. Verified by
reintroducing the defect and confirming the new assertions go red.
Deferred as follow-ups: BOT-2124 (resource indicators are not enforced as a
token audience), BOT-2125 (v2 sub-paths escape narrowing under prefix routing),
BOT-2126 (observability when narrowing declines to apply).
* Make the v2 UI tools' handle resolvable by the iframe
`visualize_query` and `render_drill_through` return `{query_handle}`, but the
iframe bundle that ships on this branch reads `structuredContent.query` and
short-circuits on `if (query)`. With `query` absent the pending tool result was
never stored, `deserializedCard` stayed null, `isReady` never became true, and
the loading spinner never cleared. Both tools were non-functional.
The slice dropped three coordinated pieces that make the handle resolvable.
Ported from `mcp-v2-foundation`:
- `GET /api/embed-mcp/queries/:handle`, returning `{:query :prompt}` behind
`check-session-header!` with an `ms/UUIDString` route param.
- `mcp-ui-parameterized-request-surface` + `on-mcp-ui-request-surface?`.
`mcp-ui-request-surface` matches `[method uri]` by exact string and cannot
express a path parameter, so without this the route 401s at the credential
middleware before the router ever sees it. The regex is UUID-pinned and
anchored with `re-matches`, keeping the entry as narrow as a literal one.
- `fetchQueryByHandle` plus a union `{query?, query_handle?, prompt?}` reader in
`useMcpApp`, guarding out-of-order resolutions with `pendingQueryHandleRef`.
One bundle serves v1 and v2, so both payload shapes stay supported.
Chose foundation's GET form over the POST variant on
`emb-2195-mcp-apps-are-stuck-on-loading-spinner-due-to-claude-sandbox`
(`79b792254cb`). The POST form needs no middleware change, but foundation's ships
the resolver, typed error messages, and the out-of-order guard already written.
`onAuthenticated` now receives the credential rather than reading it from state:
`setUiCredential` has not committed when the callback fires, so resolving from
the closure would use the previous (empty) credential and 401.
`queryError` is threaded to the route so a handle that cannot resolve renders an
error instead of spinning forever.
Docstring corrections. Three sites asserted the resolution endpoint existed in
the present tense; that is now true, and they name the route. The claim that the
store read is "keyed on `(mcp session, user)`" was false: `find-handle-row`
joins `core_session` and filters on `user_id` alone, and the looseness is
deliberate so a client rotating its MCP session keeps its handles. Verified live
against a running instance: cross-user resolution 404s, cross-session for the
same user 200s. The real native-SQL boundary is `check-mcp-ui-native-query!` on
`/api/dataset` via `+refuse-unscoped-native-sql`, which the file never named.
Fixed a dangling wikilink to a var that does not exist.
Dropped `:idempotentHint true` from `visualize_query`; the fresh-`query` path
inserts a handle row per call. `render_drill_through` keeps it.
Removed the `display` argument and output key. It was enum-validated and echoed
but no renderer read it, and `mcp_query_handle` has no column for it, so on the
documented preferred path (mint by `query`, re-visualize by `query_handle`) the
chart type was lost regardless. Rather than advertise a knob that does nothing,
it comes back end to end in GHY-4451.
Reverted the `all-scopes` rewrite. It was set-identical to the base
(`OLD-ONLY ()`, `NEW-ONLY ()`, 22 = 22) and deleted a correct sentence, while
the PR body claimed a scope-advertisement fix that never existed.
Tests. `visualize_test.clj` positively pinned the broken shape, asserting
`(= #{:query_handle} (set (keys body)))` — one side of a two-sided contract, and
green while the widget spun. It now asserts the absence of query data instead,
and two new tests walk the whole loop: mint a handle, call the tool, resolve the
returned handle over the credential-authenticated route. Deleting the endpoint
fails 9 tests; emptying the allowlist entry fails 6. The frontend spec covered
only v1's inline shape, so three tests now cover the handle path; two of them
fail against the previous hook.
Follow-ups filed: GHY-4450 (tools return success for handles the iframe will be
403'd on), GHY-4451 (restore `display` end to end), GHY-4452 (error-message,
schema and coverage cleanups), GHY-4453 (`prompt` dropped on the handle branch).
* Collapse an unreadable dashboard into the same not-found as a missing one
`resolve-dashboard!` read the row with no read check and threw the collapsed
not-found only when the row was absent. A dashboard that existed in a collection
the caller could not curate was refused later, by the write check, with a 403 —
distinguishable from the 404 a nonexistent id gave. A token scoped to one
collection could enumerate every dashboard id on the instance by watching one
against the other.
Resolving through `v2.resolve/resolve-and-read` gives the argument the read
check the rest of the module's id arguments have, so both cases read alike. The
write check still decides whether a card may be saved into the dashboard. This
also subsumes the hand-rolled existence check: `resolve-and-read` turns a
numeric id matching no row into the same not-found, which is what kept a bad id
from reaching the DB as a raw FK violation.
`update-move-into-dashboard-requires-destination-write-test` revoked every
permission on the destination, so under the read check it would now pass on
not-found and stop proving what it was written to prove — that the write
requirement on the move still runs. It now grants read and withholds write, so
the write check is again what refuses.
Verified fails-when-broken: restoring the unchecked read fails the new test.
Closes GHY-4443.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0147UgqNGEjk9nV4jRnt9oS2
* Clear the five kondo warnings failing CI on this stack
`./bin/mage kondo` over the whole repo exits 2 on five warnings, so the
backend-tests / Clj-Kondo job fails. Both files are byte-identical to this
slice's parent commit -- the warnings arrive with earlier slices in the stack
and fail the same way there. Fixing here so slice 20 can go green.
- `browse_test.clj`: the `deep` collection is created for its side effect only,
since the assertions look it up by name rather than through the binding.
Renamed `_deep`.
- `question_test.clj`: `check-semantic-type!` is flagged as destructive inside a
`^:parallel` test purely on the `!` suffix. It parses a keyword, throws on a
bad one and returns it -- no state touched -- so the suffix means "may throw",
not "mutates". Added to the `:parallel/safe` whitelist the linter's own
message points at.
* Gate a native query_handle like the other two query sources
The `query_handle` source stored whatever query the handle resolved to without
consulting either SQL gate, so a token holding only `agent:content:write` could
save a card containing raw SQL, on create and on update, with the
`mcp-execute-sql-enabled` kill switch turned off. A later `run_saved_question`
then executes it — `execute_sql` rebuilt without its scope or its switch, which
is what `check-execute-sql-enabled!` exists to prevent.
The exemption rested on the premise that minting a handle already spent those
gates. It doesn't. `construct_native_query` mints under `agent:sql:construct`
and never consults the kill switch, and `/api/embed-mcp/drills` mints from an
uninspected caller-supplied string. Nor does the handle stay with the credential
that minted it: resolution joins `core_session` on `user_id` alone, the MCP
session id being recorded for telemetry, so any credential of that user can
spend a handle minted by any other. There is no independent handle TTL either —
handles outlive the request by the backing session's lifetime.
Gating on the resolved query's shape, as the inline `query` source already does,
covers all of it without knowing which argument carried the SQL. MBQL handles
are unaffected.
The docstring asserted the premise this commit refutes, so it now records why a
handle is not proof the gates were spent.
`create-question-via-native-query-handle-test` pinned the vulnerable behavior as
intended and is inverted. The update path had no native-handle coverage at all;
it gets both halves, plus the kill switch on the handle route.
Verified fails-when-broken: removing the guard fails 9 assertions across the two
tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0147UgqNGEjk9nV4jRnt9oS2
* Pin the invariants two advertised-scope tests only appeared to check
Both tests passed regardless of their input, so they read as coverage of the
advertised scope sets without providing any. Rewritten to assert on the sources
rather than on values the construction already guarantees, and verified by
injecting the fault each is meant to catch.
- `advertised-scopes-are-distinct-test` counted duplicates in the *output* of
functions that build through a `sorted-set`, which is distinct by
construction whatever goes in. It now checks every endpoint path and the v2
surface literal, where a scope declared twice upstream is silently swallowed.
Its docstring also still described unioning "the default grant with the opt-in
scopes", which no longer exist.
- `default-grant-covers-everything-advertised-test` asserted that
`default-grant-scopes` covers the sets it is defined as the union of. It now
iterates `mcp-endpoint-paths`, which covers the per-path arities a client
actually meets -- including the v1 aliases, inside the ceiling today only
because `all-scopes` also feeds `supported-scopes`.
Add `mb-full-client-can-still-authorize-test`. Dropping `mb:full` from the
advertised sets is safe only because oidc-provider validates a requested scope
against the client's own registered `:scopes` and never against the provider's
`:scopes-supported`. Nothing pinned that, so un-advertising a scope would start
rejecting the first-party clients that legitimately hold it, silently.
Soften two docstrings that claimed more than the code delivers:
- `supported-scopes` implied explicit registration gates `mb:full`. Dynamic
registration is unauthenticated and passes a client-supplied `scope` through
unchecked, so omitting the scope here narrows who finds it, not who may ask.
- `narrow-scope-to-resource` read as though naming a resource bound the token to
it. It shapes the consent screen and the stored grant only; `resolve-access-token`
never reads `:resource`, and a client that omits the indicator is not narrowed
at all. Both gaps are inherited; BOT-2124 tracks closing them.
* alert_write: close scope, error-message, and delivery-notice gaps
Six fixes from review of the slice.
Scope: clearing `send_once` turns a single scheduled run into an
unbounded series, the same commitment a new schedule makes, so
`execute-scope-trigger` now gates it on `agent:query:run`. Only an
explicit `false` counts — nested nulls survive the boundary's stripping,
so `send_once: null` stays an omission.
Errors: `fetch-alert` now checks write alongside read, collapsing
unreadable, unwritable, and nonexistent into the one not-found that
GHY-4217 needs. That lets the blanket 403 catch in `update!` go, so
every other 403 — a disallowed email domain, a channel template the
caller can't write — reports its real cause instead of claiming the
alert does not exist. `can-write?` mirrors `can-update?` for the bodies
this tool sends: it never reassigns `creator_id` and refuses `card_id`.
Delivery: `notify-creator-of-delivery-change!` becomes
`notify-caller-of-delivery!` and runs on create too. The notification
API's own "you were added" mail removes the current user, so an
agent-created alert pointed at a third party told the token's owner
nothing.
Subscriptions: `:subscriptions` is a `:multi-row?` nested spec, so
writing one back deletes the rest. Passing `schedule` to an alert with
several is now refused, mirroring the multi-channel handler guard.
Validation: the extracted `update-notification!` validates
`::NotificationApiUpdateInput` rather than relying on callers, which the
one non-endpoint caller did not do.
Reverts the `util/cron.clj` `(or hour 0)` change. `check-schedule!`
requires an hour for weekly and monthly, so it was unreachable here,
while the shared util's other callers — pulse sends, v1 create_alert,
database sync schedules — would have had stored null-hour rows move from
hourly to midnight with no test pinning either behavior.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VyVfcPqrmdSnV4YPXDMcUK
* Check the update's new state before probing the warehouse
`update!` ran `check-target-move!` — which opens a warehouse connection to
resolve `target-table-exists?` — before any permission check on the database
that probe connects to.
The tool's own `api/write-check` covers the transform as stored. The caller's
`definition` then names its own `:database`, and `resolve-source` runs no
permission check of its own, so `source-db` becomes any database id the caller
writes down; `resolve-target` stamps it onto the target, and the probe follows
it. The first and only authorization touching the new database lives inside
`transforms/update-transform!`, on the line after.
So a data analyst holding transforms permission on the transform's own
database — enough to pass `write-check` — could ask "does table X exist in
database B?" for a database they hold nothing on, one call per guess, and
drive outbound connections there. Two further gates were skipped along with
the permission check: `check-database-feature` rejects the audit database, the
sample database, and any database with DB routing enabled, and
`throw-if-db-routing-enabled!`'s own docstring says to call it before any
driver operation.
Run the new state's gates first, which is the ordering the create path already
states in a comment ("Before the target check, which opens a warehouse
connection — a bad argument shouldn't pay for that"). REST's `PUT
/api/transform/:id` has the same order and never connects.
The regression test asserts the ordering directly rather than the outcome: it
redefines `driver/table-exists?` to record whether it ran, since the tool
converts a throwing probe into a generic "Internal error" that an
outcome-based assertion passes on. Watched failing on `probed` with the two
forms swapped, and only that test fails.
Verified in a clean JVM on this branch: transform-test 23 tests / 83
assertions; transforms.crud-test + transforms.models.transform-test 10 tests /
41 assertions; kondo 0/0; cljfmt no-op; fix-modules-config unchanged.
* Normalize a retyped model's display, and let a model be a list
Retyping a question to a model left its `display` alone, where
`PUT /api/card/:id` forces table. A model's display is not inert: the query
processor runs the pivot QP when it is `:pivot`, so a pivot question retyped
through this tool kept returning pivoted data — REST makes that unreachable.
A chart display also drives subscription rendering, dashcard sizing, and the
search display filter.
The earlier review dismissed this as cosmetic because "the model layer renders
as table regardless." It doesn't: nothing normalizes a model's display anywhere
in the render path. The invariant holds only because the UI's own convert action
sets table, and REST's rule catches the rest.
REST leaves an explicit `list` alone, and writing that carve-out showed the tool
could not express `list` at all — so an agent could neither build a list-view
model nor preserve one it was updating, even though the model editor offers
exactly table and list. `list` joins the display enum, and the carve-out honors
it.
Retype-to-model had no MCP coverage at all before this.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0147UgqNGEjk9nV4jRnt9oS2
* Close the three pre-merge findings from the stack review, and the OSS test failure
Adversarial review over slices 02..21 raised 21 findings against `subscription_write`;
three were called pre-merge, and the PR review found a fourth that reddens the OSS job.
The rest are deferred to a follow-up issue.
**`update!` did its work before anything authorized it.** `mi/can-read?` on a non-alert
Pulse is `(or superuser? creator? recipient?)`, so being on the recipient list alone
passed `fetch-subscription`'s gate — with no permission on the dashboard or its
collection. Everything between that gate and `update-pulse-with-perm-checks!`'s
`write-check` then ran for such a caller: `resolve-parameters` answers an unknown id by
printing the dashboard's complete parameter-id set, `classify` probes user existence and
active status, and the channel-topology errors name the channel count and type. The
write check's own 403 also escaped uncaught, so a recipient could tell subscriptions they
are a target of from ids they know nothing about — the existence oracle the sibling
`alert_write` closes with a named GHY-4217 comment one commit below on this stack.
Gating `fetch-subscription` on `write-check` closes all four: the disclosures never run,
and `resolve-and-read-with` collapses the 403 into the same not-found everyone else gets.
Purely additive — `update-pulse-with-perm-checks!` runs `(api/write-check
(retrieve-pulse id))`, the identical check on the identical object, so no caller who
previously succeeded is now refused.
**A schedule field the schedule type never reads was dropped silently.** `check-schedule!`
enforced only the missing-required direction, so `{schedule_type: "hourly", schedule_hour:
9}` reported success and delivered the dashboard 24 times a day with `schedule_hour` NULL.
`alert_write` refuses the same shapes, and its docstring claims the two tools are kept in
step. Adds `check-ignored-schedule-fields!` over this tool's vocabulary (no
`:schedule_minute`), matching the writer exactly: `schedule_hour` for all but hourly,
`schedule_day` for weekly and monthly, `schedule_frame` for monthly. A filled-in null
stays an omission, and monthly's more specific `mid`/`schedule_day` error still wins.
**Repointing a Slack subscription erased the channel's other stored details.** The email
branch carries `(:details existing)` forward with a comment explaining why; the slack
branch replaced the column outright, so an edit dropped `include_pdf`/`attachment_only`
set in the app and the `channel_id` the sender prefers over the display name. The next
send dropped the PDF, and a channel rename sent it nowhere. Now merges onto the stored
details, and `slack-details` carries the cache entry's id as `:channel_id` so a repoint
overwrites the stale one rather than leaving it to misroute.
**The two EE-only parameter tests could not pass on OSS.** Every `define-premium-feature`
getter is `(and config/ee-available? (has-feature? ...))`, which `with-premium-features`
cannot flip, so both failed on the feature refusal while reporting the feature as set.
Wrapped in `mt/when-ee-evailable`, the pattern `redaction_test` already uses for this.
Each fix is pinned by a test that fails when the fix is reverted, verified by reverting
each one in turn: the write-check test's byte-identical baseline comparison catches a 403
that merely contains "not found", and `update-requires-write-permission-test` is tightened
from `(some? (tool-error ...))` — a tautology once it returns — to the collapsed message,
which is why this defect shipped green.
Verification: 44 tests, 116 assertions on EE and 113 on OSS (the guarded bodies compile
away), both fresh-JVM; `:module mcp` 565 tests / 6096 assertions; kondo and cljfmt clean.
Before this change the OSS run was 2 failures and 1 error.
* fix: add_action must write a virtual_card
The frontend recognizes an action dashcard through
`visualization_settings.virtual_card.display` (`isActionDashCard` in
frontend/src/metabase/actions/utils.ts). `add_action` hand-built its
visualization settings and omitted the wrapper every other virtual op
gets from `virtual-card-settings`, so an MCP-created action dashcard
rendered as an empty card instead of a button, and neither the
ActionSidebar nor DashCardActionsPanel recognized it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPHtStCj5NibqNCV4e1dpp
* fix: add_parameter requires a name
`add_parameter` reused `parameter-fields` wholesale, where `:name` is
optional because `update_parameter` patches properties individually. A
nameless parameter gets no slug, which half-breaks embedding, public
links and URL parameter sync, and `dashboard->resolved-params` requires
a non-blank name — so the tool errored on read-back *after* the write
had committed, leaving the parameter behind.
`:name` now lives on each op rather than in the shared field list:
required on add, optional on update.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPHtStCj5NibqNCV4e1dpp
* fix: move_parameter refuses both index and dashcard_id
The op documents "pass exactly one of `index` or `dashcard_id`", but the
`cond` took the `dashcard_id` branch first, so a caller sending both got
the inline placement and no reorder with no error. `add_link` already
refuses the analogous both/neither case; this makes move_parameter
consistent with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPHtStCj5NibqNCV4e1dpp
* fix: move must not autoplace a dashcard against itself
`placement` took its siblings from the whole tab before the moved row
had been removed, so the card being moved counted as an obstacle to
itself and the search could never return the slot it already held. A
`move` carrying only a dashcard_id — which the schema allows, and which
reads as "leave it where it is" — always displaced the card: a
full-width size_y 4 card alone on a tab jumped from row 0 to row 4.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPHtStCj5NibqNCV4e1dpp
* Omit content_markdown when a document body has no Markdown form
get_content filled `content_markdown` with `prose-mirror/ast->text` output
when `documents/serialize` threw. That text is a flat space-join of text
nodes: no headings, no `{% card %}` embeds, no `:::` layout containers.
`content_markdown` is the key `document_write` accepts as a whole-body
rewrite, and both tool descriptions route a read-modify-write through it.
An agent reading a document with an unrenderable block, appending a line,
and writing it back replaced the stored AST with a single paragraph and
orphaned every anchored comment thread -- and the tool reported success.
`document_write`'s own `body-projection` faces the identical call and does
the opposite, with the rule in its docstring: "old_str is matched against
this exact text, so text that isn't the serialization is worse than none."
It emits `content_markdown_unavailable`. This mirrors it, including the
`Throwable` catch, so the two document paths agree.
Proven in a REPL against a body holding a heading, a `mysteryBlock` and a
card embed: `serialize` throws, `ast->text` yields "Q3 revenue regional
breakdown", and re-parsing that string returns 1 paragraph in place of 3
blocks -- card embed and heading gone.
Both tool descriptions corrected: `document_write` no longer tells the
agent to read with get_content and rewrite via content_markdown, and
get_content's round-trip promise now names the exception.
The existing fallback test asserted the degraded value was correct; it now
asserts the marker. Reverting the fix fails 5 assertions across the two
tests.
* Route the search tool's app-DB queries through mcp/db.clj
The Clj-Kondo check went red on the merge with three warnings, all in
tools/search.clj:
Application database query calls like `t2/select` must live in
metabase[-enterprise].<module>.db namespaces
The rule arrived with the base merge, and the module had already been routed
through metabase.mcp.db; search.clj was the last file still querying directly.
Its three reads move there as collections-for-read-check,
collection-id->name+location and snippets-by-archived-state, each a direct
Toucan 2 call with no added logic, per that namespace's own contract. The
column scoping the snippet query depends on travels with it: only
id/name/description plus collection_id for the read check, never :content,
which holds the SQL body. search.clj no longer requires toucan2.
snippet-rows-does-not-load-content-test stubs `t2/select` through with-redefs
to assert that column list. It still passes unchanged — with-redefs swaps the
var, and mcp.db resolves it at call time — so the guard survives the move
rather than needing to be rewritten around it.
Verified: targeted kondo 0/0 and a full `./bin/mage kondo` 0/0; cljfmt clean;
v2 search + v2 common + metabot search 81 tests / 329 assertions / 0 failures.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CF3ZXRaq6wy8sFR9L8DPKa
* Fix four correctness defects in the query slice found by adversarial review
Keyset paging and the SQL hint each had a way to return the wrong rows, or no
rows, without saying so. All four are proven in a REPL against the Sample
Database, with a regression test each.
1. `with-total-order` and `next-page-query` dropped the query's `:parameters`.
Both rebuild the query through `lib/prepare-for-serialization`, which strips
`:parameters` as runtime-only — but on an MBQL query they are a real filter
that the QP's parameter middleware ANDs on. `execute_query` runs every query
through `with-total-order` before executing, so whenever the rewrite fired
the QP ran the query unfiltered: a filter for quantity = 2 returned
quantities [2 3 2 6 5], with no error and no truncation flag, and the minted
handle described a different question than the one asked. `execute_sql`
already worked around the identical strip; the MBQL path did not.
2. A caller could forge the private `::keyset` marker. The marker means "this
namespace minted this predicate", and both `without-page-boundary` and
`supersede-previous-keyset` strip clauses carrying it. But a tool's `:query`
is an open [:map] and a namespaced option key rides through JSON decode and
serialization intact, so a caller could stamp its own filter and have it
silently dropped from the saved handle and from every later page — a narrow
visible first page over a wide underlying read. Caller-supplied queries are
now stripped where they enter. The cursor path deliberately keeps the marker:
superseding the previous page's predicate depends on it, and stripping there
stacks a dead filter per page instead.
3. `strip-sql-noise` threw StackOverflowError on an unbalanced quote. The
literal-blanking pattern `'(?:[^']|'')*'` is an alternation under a
quantifier, which java.util.regex compiles to one stack frame per character
consumed. A single unterminated quote made it backtrack across the whole
query. It runs on the `validate_only` path with no execution to gate it, and
StackOverflowError is an Error, so it escaped the handler that turns tool
failures into teaching errors and reached the transport uncaught. Replaced
with the equivalent unrolled loop `'[^']*(?:''[^']*)*'`, verified to produce
identical output on every literal shape and to handle 50k characters.
4. The cursor silently dropped every row whose key column was NULL. A keyset
predicate is all comparisons, and under three-valued logic every term is NULL
— never true — for a row whose key is NULL, so those rows were never served
and the response still reported truncated: false. Ordering ORDERS by DISCOUNT
served 1,915 of 18,760 rows and called the result complete. Nullable keys now
refuse to mint, joining the remap and lossy-value guards; MBQL cannot express
NULLS FIRST/LAST, so refusing is the only driver-independent answer.
The refusal keys on declared nullability AND an observed `fingerprint nil%`
above zero. Declared nullability alone is too blunt to gate on — in a typical
warehouse only primary keys are NOT NULL, so it would have cost the cursor
for nearly every non-PK-ordered query. The fingerprint is a sync-time sample,
so a column that gains its first NULL after the last sync stays cursorable
until the next one; that residual is the price of keeping paging usable.
`:module mcp` 658 tests / 6506 assertions green; kondo and cljfmt clean.
* Refuse a constraints value that names a column
`get_parameter_values` accepted `[:map-of :keyword :any]` for `constraints`, and
chain filtering compiles those values into MBQL. A value shaped like a clause was
therefore compiled as one: `{"_PRICE_": [["field", 49, nil]]}` became a reference
to that column, so the server evaluated `WHERE VENUES.PRICE = VENUES.CATEGORY_ID`
and returned the matching rows.
The caller needs only read access to a dashboard with two filters on one table.
It needs no query permission — `*param-values-query*` runs these lookups elevated
precisely so a caller who can only read a dashboard can still see its filter
values — and the reference reached columns marked `visibility_type: sensitive`,
which exist to be unreachable. A name-shaped reference works without a field id.
Reported by galdre on #81245 with a proof; his repro is the test.
`::constraint-value` now admits only scalars and sequences of scalars, so no
clause shape validates and nothing caller-supplied is ever compiled. Refusing at
the boundary rather than blacklisting known clause forms: a blacklist needs
updating every time MBQL grows a shape.
The ns docstring claimed no caller-supplied query material reaches the warehouse.
That is true again, but only because of this schema, so it now says so.
Verified by reproduction: before the fix both field-reference shapes returned
success where the test expects a refusal; after, all four attack shapes are
refused. The test keeps anti-vacuity controls — the unconstrained call returns
the full list and a literal constraint narrows it — so it cannot pass by failing
to reach the warehouse. Namespace: 86 assertions, 0 failures.
* Pin the constraints refusal against a caller who cannot query
The test I first wrote proved the schema rejects a clause-shaped constraint, but
ran under `with-fixtures`, which grants full data permissions. That misses the
point of the finding: the danger is not the shape, it is that a caller holding
NO query permission could read data through it.
Rebuilt on galdre's repro. The caller now has collection read and
`:perms/view-data :unrestricted` but `:perms/create-queries :no`, and the test
asserts up front that it cannot run a query against VENUES at all — so the
refusals below are about privilege, not validation tidiness. Adds his
sensitive-column case: a reference to a column marked `visibility_type:
sensitive`, which exists to be unreachable.
Verified by mutation: with the schema returned to `:any` this fails 3 assertions
(both field-reference shapes plus the sensitive column); the earlier weaker
version caught only 2. Anti-vacuity controls keep their teeth — the unconstrained
call returns all 100 values, a literal constraint narrows it, and a literal still
works after the fix, so the refusal is shape-based rather than blanket.
* Stop get_content handing a caller data-derived values it cannot reach
Two paths returned values computed from, or describing, rows and cards the
caller was denied. Both were reported in review of #81616.
`include: ["fields"]` returned stored column fingerprints verbatim. A
fingerprint is computed over the whole table: `:min` and `:max` are
individual cell values and `:distinct-count` counts every row. A caller
sandboxed to 10 of 100 rows received statistics over all 100, and a caller
with no data permission on the table at all received the same numbers.
Columns whose table is row-restricted now lose `:fingerprint`, via
`metabot.metadata-perms/row-restricted-table-ids` -- which covers
sandboxing, impersonation and database routing, and fails closed. Nothing
else in the payload is data-derived: names, types, semantic types and field
ids are schema, not rows.
`include: ["layout"]` returned `:visualization_settings` and
`:parameter_mappings` for dashcards whose card had just been redacted as
unreadable. `redact-dashcard` reduced the card to nothing, which is the
whole redaction for the projection, but `dashboard-layout` reads the same
dashcard directly -- so the caller still learned the card's author-written
title, the warehouse columns it renders with their labels, its click
targets, and the field ids its filters are wired to. The redaction now
strips those alongside the card. Layout keys (sizing, container styling)
describe the dashboard, not the card, and survive.
`:parameter_mappings` is emptied rather than removed: it is a required key
on a DashboardCard, and the parameters summary runs
`dashboard->resolved-params` over the same row, which fails its schema when
the key is absent.
The layout case is parity with REST rather than a regression --
`dashboards-rest/api.clj`'s `hide-unreadable-card` leaves the same settings
behind -- but this file already chose to be stricter than REST twice (the
link-entity strip in this very function, and the transform target-table
gate), so it follows that standard.
Tests: the fingerprint rule needs a live sandbox, so it lives in
`metabase-enterprise.mcp.v2.redaction-sandbox-test` alongside the other
sandbox-only redaction; it asserts an admin still sees fingerprints first,
so it cannot pass vacuously, and checks the table really is row-restricted
before trusting the result. The layout rule holds in OSS and is pinned in
`metabase.mcp.v2.redaction-test`. Reverting either fix fails its test.
`:module mcp` 5892 assertions, 0 failures.
* Cover archived reads and not-found parity beyond `question`
Two gaps Eric raised in review of #81616.
`get-content-reads-archived-content-test` pins the read-path behavior for
trashed content, which was projected but never exercised. Archived content
reads rather than 404ing -- an agent asked to restore something has to be
able to read it first -- and reports `archived: true` so it is not mistaken
for live. A live entity in the same test asserts the flag is not constant.
`get-content-not-found-parity-across-types-test` extends the existing oracle
check, which covered only `question`, to model, dashboard, document and
collection. Each type has its own fetch fn, so parity on one proves nothing
about the others; the test asserts the unreadable and nonexistent messages
differ only by the id.
Snippet is deliberately absent from that loop: snippets live in the
`snippets` collection namespace rather than under the restricted collection,
so `with-non-admin-groups-no-collection-perms` cannot make one unreadable.
Its parity needs a snippets-namespace setup of its own -- noted in the test.
Found while writing these, not fixed here: `:archived` is in the question,
collection and document projections but not the dashboard one, so a trashed
dashboard reads back with no archived flag. Filed to GHY-4471 rather than
widening this PR.
`content-test` 39 tests / 213 assertions, 0 failures.
* Name the scope get_parameter_values actually requires
scope-gate-test said the tool requires agent:resource:read. It declares
agent:content:read, and the test's own assertions say so — agent:sql:run is
refused, agent:content:read succeeds. agent:resource:read gates a resource
(catalog://metabase/fields), not this tool.
* fix(mcp): name budget-omitted tables from source rows
`budget-omitted` read `:id`/`:name` off the already-projected payload, but a
`fields` selection that excludes them leaves nothing to read: every omitted
entry serialized as `{"id": null, "name": null}`, so the caller could not tell
which tables were dropped and "request in a separate call" was unactionable.
`assemble-tables` no longer builds `:omitted` — its `:tables` is always a
prefix of `payloads`, which is index-aligned with `rows`, so `get-fields`
names the dropped tables from the source rows instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QkpS1yAdHikDjE1hQaShC
* fix(mcp): withhold fingerprints from row-restricted users
`get_fields`' detailed projection returned each field's `:fingerprint`, which
is computed at sync time across every row of the table. A user whose row
access is narrowed by sandboxing, connection impersonation, or database
routing got correctly narrowed `:values` but a whole-table fingerprint:
`:min`/`:max`/`:earliest`/`:latest` are literal values from rows they cannot
see, and `:global :distinct-count` is whole-table cardinality for every column
type. There is no safe subset, so the whole map is withheld, following
`metabot.tools.field-stats/field-statistics`.
The restriction lookup is batched over the request's tables and fails closed.
The `fields` projection path is covered by the same `detailed?` branch, so a
`fields: ["fields.fingerprint"]` selection cannot route around it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QkpS1yAdHikDjE1hQaShC
* fix(mcp): require query permission to inline field values
`get_fields` gated only on `mi/can-read?` on the table, but then inlined
cached FieldValues, whose own check bottoms out in `can-read?` as well. A
caller holding `manage-table-metadata` alone therefore got values that
`/api/field/:id/values` refuses with a 403 — REST gates `summary` on
`read-check` and `values` on `query-check`, and that split has to hold here
too: values are rows of the table, not metadata about it.
`attach-inline-values` now primes the table perms cache once per request and
restricts the batched fetch to tables the caller can query. Tables it can only
read keep their full field metadata, without `:values`. The gate is per table,
so a mixed batch resolves each one on its own.
`response_format` now documents that values and fingerprints are
permission-dependent and absent rather than empty when withheld.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QkpS1yAdHikDjE1hQaShC
* test(mcp): drive browse_data through the registry
Every browse_data test called the handler directly, so the scope gate, the
closed args schema, and top-level nil-stripping went unexercised — unlike
browse_collection, which already dispatches through `registry/call-tool`.
Covers scope denial and grant (including tools/list visibility), the closed
map's teaching errors for unknown keys, wrong types, out-of-range limits and
bad enum values, the strict-client contract that an all-nulls call equals the
minimal one, the interaction between stripping and contains?-based per-action
validation, and nested nils being caught by the schema rather than reaching
the handler.
No behavior changes: every mechanism already worked. Each assertion group was
verified non-vacuous by perturbing it and confirming it goes red.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QkpS1yAdHikDjE1hQaShC
* Build the sandbox test's card query with Lib
mt/mbql-query is deprecated since 0.61.0, and the :deprecated-var warning
fails the Clj-Kondo CI job.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015AFrL3LpdWBaQizx5QWaF5
* Move get_content's app-db queries into metabase.mcp.db
The module-boundary linter requires t2 calls to live in a <module>.db
namespace, and its warnings fail the Clj-Kondo CI job.
table-by-id deliberately omits select-one-by-id's integer precondition: a
nil table_id would raise an AssertionError, which escapes the catch
Exception that degrades one definition to nil, failing the whole batch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015AFrL3LpdWBaQizx5QWaF5
* test(mcp): waive the mbql-query deprecation in the sandbox test ns
The full-repo kondo run reported three `#'metabase.test.data/mbql-query is
deprecated` warnings and exited non-zero. `:deprecated-var` needs
whole-project analysis to resolve the potemkin import, so the per-file lints
the file was checked with reported 0/0.
Uses the ns attr-map waiver rather than inline ignores: `:deprecated-var` is
budgeted at 86 in `.clj-kondo/ratchets.edn`, and three more would trade a red
lint for a red `kondo-ratchet-test`. The attr-map is counted by neither ratchet
and is the settled convention for this deprecation in EE test namespaces.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QkpS1yAdHikDjE1hQaShC
* [mcp-v2 12] definitions: adapt to the base's shared contracts
The merge from mcpv2-01-core was textually clean but semantically broken
in three ways, none of which appeared as a conflict.
- `v2.write/readback` dropped its 3-arity for a single 4-arity taking
`ack-keys`. All four call sites passed three args, an ArityException at
runtime. Now pass `nil` explicitly, matching metric/collection/question.
- `registry/call-tool` now answers `{:result …}` / `{:error …}` rather
than bare MCP content. The test harness read the old shape, so every
refusal looked like a success and every result read nil — 101 failures.
`call-tool!` now normalizes both into the `{:isError true}` MCP shape,
as `content_test/call-content` already does, leaving the helpers alone.
- `metabase.mcp.db` arrived with the `:metabase/t2-query-namespace` rule
confining Toucan 2 calls to it. Both `t2/select-one :model/Table` calls
become `mcp.db/table-by-id`, whose contract already covers the
nil-tolerant column-value case these callers have.
Also: with the base's dynamic-require hook failing closed, `api-fn` trips
`:metabase/modules`. Added the sanctioned ignore and raised the budget to
24 — the dynamic resolve is deliberate, keeping `mcp` a consumer of the
segments/measures *models* rather than the *modules*.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AbCo3ybRitdVSQXYcG97QR
* kick ci
* Pass ack-keys to readback in alert_write
readback takes [token-scopes read-scopes row ack-keys]. The call site passed
three arguments, which fails to compile against the integration branch.
nil matches every other _write tool: ack-keys exempts caller-supplied keys from
the read gate on the degraded path, and alert_write returns only server-derived
state.
* Move alert_write's app-db queries into mcp.db
recipients/classify called t2/exists? and check-writable! called t2/select-one
directly, which the module-boundary linter rejects outside metabase.mcp.db.
notification-by-payload-type already existed for the select-one.
active-user-exists? is new, alongside the existing subscription-pulse-exists?.
* Unwrap the call-tool envelope in alert_test
registry/call-tool returns {:result ...}/{:error ...}. The helpers read :isError
off the bare response, so every expected-error assertion saw a nil :isError and
reported "got success".
Same shape as bookmark_test.clj on the integration branch.
* Revert slice-17 fixes that rode in on the merge
The merge resolution fixed two pre-existing defects in slice 17's files to get
this branch's lint green: `v2.write/readback` called with 3 args where it takes
4 (alert.clj, subscription.clj), and six `:metabase/t2-query-namespace`
warnings. Both belong on `mcpv2-17-query`, and carrying them here guarantees a
conflict when 17 fixes them itself.
Restores the five files to `origin/mcpv2-17-query` byte-for-byte and drops the
four `mcp.db` fns that existed only to serve them. The six browse fns 16 needs
stay.
Lint is now red on this branch with 17's own 2 errors and 6 warnings, in files
identical to 17's. It goes green when 17 does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QkpS1yAdHikDjE1hQaShC
* Give readback its fourth arg, move Toucan calls into mcp.db, and teach the tests call-tool's new shape
Three defects surfaced while merging 17 into 16. 16 patched them locally to get
its own CI green; they belong here.
readback takes [token-scopes read-scopes row ack-keys], but alert_write and
subscription_write passed three, so both threw an ArityException on every create
and update. Ten of the other eleven call sites pass nil for ack-keys; only
bookmark passes a value, because bookmarked is caller-supplied and has to
survive scope degradation. Neither of these returns anything the caller handed
in, so nil is right.
The t2-query-namespace hook wants Toucan query calls in the module's db
namespace. notification-by-payload-type and subscription-pulse-exists? already
existed and already fit; nullable-fields-with-fingerprints, active-user-exists?,
dashboard-parameters and delete-query-handles-created-before! are new. query.clj
and recipients.clj no longer need toucan2.core at all.
call-tool now returns {:error {:code :message}} on a pre-dispatch rejection and
{:result mcp-content} once the handler has run, rather than MCP content
directly. Six test namespaces still read the old shape, which cost 135 failures
in CI: :content came back nil and :isError never fired, so a tool error could
pass as success. The helpers take the whole outcome now and share dispatch-error?
and response-text, mirroring browse_test on 16 so the two agree. scope-gating-test
asserts the denial arrives as {:error {:message ...}}, pinning the behavior change
instead of papering over it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GJCEEY8W7fXmtMHUBS3Mc
* Fix subscription_write against the integration branch
readback takes four args; the call site passed three.
Two app-db queries moved into mcp.db, which the module-boundary linter requires:
subscription-pulse-exists? already existed, dashboard-parameters is new.
Test helpers unwrap the {:result ...}/{:error ...} envelope registry/call-tool
now returns, matching bookmark_test.clj.
* Drop the duplicate active-user-exists?
Slice 15 added one next to notification-by-payload-type; this branch already had
an identical definition. The merge kept both.
* Drop the duplicate active-user-exists?
Slice 15 added one next to notification-by-payload-type; this branch already had
an identical definition. The merge kept both.
* Unwrap the envelope in the surplus-schedule-field test
This case reads the response directly rather than through tool-error, so each
schedule type is asserted independently. It still read the pre-envelope shape,
so :isError was always nil and the refusal looked like a success.
* Fix dashboard_write against the integration branch
Four app-db queries moved into mcp.db, which the module-boundary linter requires:
dashboard.clj reuses the generic select-by-ids/select-one-by-id, subscription.clj
reuses subscription-pulse-exists? and a new dashboard-parameters.
Test helpers unwrap the {:result ...}/{:error ...} envelope registry/call-tool
now returns.
browse.clj still carries app-db calls here; slice 16 has already moved them and
merges first.
* Restore two mcp.db fns dropped in the integration merge
delete-query-handles-created-before! (called by the query-handle GC task) and
nullable-fields-with-fingerprints (called by v2/query.clj) are added by this
slice. Resolving db.clj wholesale toward the integration branch removed them,
so every namespace loading them failed with No such var.
* Stop import-mbql recursing forever on a numeric :source-card
content-ref? matches a portable entity_id or a bare numeric id. import-fk of a
numeric id returns a numeric id, so recurring on the whole match re-entered this
branch on its own output until the stack blew.
The :source-table branch escapes only because it rewrites the value to card__N
and stops matching. Recur on the rest of the map instead, and set :source-card
from the resolved value.
* Stop import-mbql recursing forever on a numeric :source-card
content-ref? matches a portable entity_id or a bare numeric id. import-fk of a
numeric id returns a numeric id, so recurring on the whole match re-entered this
branch on its own output until the stack blew.
The :source-table branch escapes only because it rewrites the value to card__N
and stops matching. Recur on the rest of the map instead, and set :source-card
from the resolved value.
* Stop import-mbql recursing forever on a numeric :source-card
content-ref? matches a portable entity_id or a bare numeric id. import-fk of a
numeric id returns a numeric id, so recurring on the whole match re-entered this
branch on its own output until the stack blew.
* Stop import-mbql recursing forever on a numeric :source-card
content-ref? matches a portable entity_id or a bare numeric id. import-fk of a
numeric id returns a numeric id, so recurring on the whole match re-entered this
branch on its own output until the stack blew.
* Stop import-mbql recursing forever on a numeric :source-card
content-ref? matches a portable entity_id or a bare numeric id. import-fk of a
numeric id returns a numeric id, so recurring on the whole match re-entered this
branch on its own output until the stack blew.
* Stop import-mbql recursing forever on a numeric :source-card
content-ref? matches a portable entity_id or a bare numeric id. import-fk of a
numeric id returns a numeric id, so recurring on the whole match re-entered this
branch on its own output until the stack blew.
* Pass ack-keys to readback in dashboard_write
readback takes [token-scopes read-scopes row ack-keys]; this call site passed
three. nil matches every other _write tool: dashboard_write returns
server-derived entity state, so nothing is exempt from the read gate.
* Unwrap the call-tool envelope in duplicate_test
registry/call-tool returns {:result ...}/{:error ...}; the helpers read :isError
off the bare response, so a refusal read as a success.
* Unwrap the call-tool envelope in visualize_test
registry/call-tool returns {:result ...}/{:error ...}. The helpers read :isError
and :structuredContent off the bare response, so every error assertion saw nil
and the suite reported 10 failures / 14 errors against working code.
Adds dispatch-error?/response-text matching the other v2 tool suites, and
routes the two direct (:isError (call! ...)) reads through it.
* Migrate legacy MCP OAuth clients and tokens onto the v2 scope set
Slice 21 repoints /api/metabase-mcp at the v2 tool surface, which gates every
tool on one of six coarse scopes. Every MCP client connected to a shipped
x.60-x.63 release holds a token carrying the 17 legacy per-entity agent scopes
instead. No legacy scope satisfies any v2 tool scope and registry/list-tools
filters silently, so those clients handshake successfully and then get HTTP 200
with an empty tools list - no error, nothing logged.
It does not self-heal: the refresh grant copies the original scope forward and
can only narrow, and rotation issues a fresh refresh token each time, so a
refreshing client stays broken indefinitely. Manual re-auth fails too, because
/oauth/authorize validates the requested scopes against the client's registered
snapshot before narrowing, returning a 400 invalid_request rendered raw in the
user's browser tab.
Two data-only custom migrations fix both halves:
- WidenDynamicOAuthClientScopesForMcpV2 unions the six v2 scopes into every
dynamically registered client's snapshot. This raises a ceiling, not a grant:
no token gains anything, and the user still consents per-authorization.
- RevokeLegacyMcpOAuthTokens stamps revoked_at on access AND refresh tokens
that satisfy no v2 tool scope and carry a scope outside the six. Both tables
must be revoked or the client refreshes instead of re-authenticating.
Afterwards a client's next request gets the existing invalid_token 401 with the
discovery challenge, re-authenticates against a snapshot that now validates, and
lands with the six scopes - the RFC 6750 path every MCP client already handles.
A 403 insufficient_scope step-up was rejected deliberately: step-up is currently
broken in both Claude Code and Codex, so it would leave clients stuck.
The scope strings are literals rather than a read of mcp.paths/v2-surface-scopes
so the migrations' behaviour is frozen against later edits to that vector. Both
are idempotent and safe to re-run.
Tests: a row-level migration test over the changelog, an end-to-end transport
test that characterizes the empty-tool-list bug and then pins the fixed
behaviour, and an authorize test proving the 400 becomes a consent page.
* Re-apply the browsable-database filter to FK neighbour tables
related_tables filtered only on mi/can-read?, which evaluates per-table perms
against :db_id and has no notion of a router destination: a destination reads as
affirmatively readable because the Database read check redirects to its router.
An FK from a still-router table into a table whose :db_id was reassigned carried
the destination's column names into the response by the neighbour path, which
the directly-requested path already refuses. browsable-db-ids is already computed
for that path; thread it through.
* Make the destination-permission tests reach the create-check
Both tests asserting the destination create-permission refusal wrapped the
destination in `with-non-admin-groups-no-collection-perms`, which revokes
*read* as well as curate. `destination-collection-id` resolves the argument
through `resolve-collection-id`, whose documented behaviour is to collapse
"doesn't exist" and "exists but not readable" into one not-found error so the
argument never reports the existence of a collection the caller cannot see --
so the call was refused there, before `api/create-check` ever ran, and both
tests failed on the tightened `#"(?i)permission"` assertion introduced with
the `check-allowed-to-create-card!` removal.
Grant read on the destination so it is readable but not curatable, which is
the state the create-check exists to refuse. Matches the sibling case in
`collection_test.clj`, which grants read for the same reason and notes that a
not-found collapse there would mean the destination check had stopped running.
* Move app-db queries out of dashboards/write and pulse/broken-subscriptions
The app-db linter requires t2/... calls to live in <module>.db namespaces.
Slice 18's write.clj and broken_subscriptions.clj ran queries inline, which
Kondo flagged once this branch merged with mcpv2-01-core (where both the
linter and the .db namespaces live).
Each moved call is a direct Toucan 2 call in the module's .db namespace with
no added logic; call sites keep identical semantics. Reused existing helpers
where they already covered the query (dashboard, update-dashboard!, card,
card-queries, dashcards-by-ids, insert-dashboard!, insert-dashboard-tabs!).
Also regenerates .clj-kondo/config/modules/config.edn, restoring pulse and
query-permissions to dashboards-rest :uses along with the model imports the
generator derives.
* Ship only the scope-widening half of the MCP v2 OAuth migration
Widening a dynamic client's registered scope snapshot grants nothing: no token
gains a scope, and the user consents to the six on the new screen. It is safe to
run on upgrade.
Revoking the legacy-shaped tokens is the half that forces the re-authentication,
and it logs every connected MCP client out once. That is a release decision, not
a migration detail, so RevokeLegacyMcpOAuthTokens stays written and tested but is
not wired into a changeset. The test pins that no token is revoked yet, so wiring
it up later is a visible change rather than a silent one.
* Revert "Ship only the scope-widening half of the MCP v2 OAuth migration"
This reverts commit 88fec4366f.
* Pass token-scopes to issue-ui-credential in callback_api_test
This slice retires the 2-arity that minted claimless, gate-exempt :legacy
credentials. Four call sites here still used it and threw ArityException.
* Drop the duplicate check-semantic-type! kondo entry
Resolving .clj-kondo/config.edn by unioning both sides of the conflict added an
entry the file already carried further down, which made the EDN unparseable:
Duplicate key: metabase.mcp.v2.tools.question/check-semantic-type!
A broken config takes down Clj-Kondo and kondo-ratchet-test together.
* Take the integration branch's browse_test
Resolving this file's conflict earlier kept a comment from this branch, and with
it the whole pre-slice-16 file. assemble-tables no longer returns :omitted, so 6
failures and 23 errors followed. This branch adds no test the integration branch
lacks.
* Drop opt-in-scopes with the plumbing it called
Slice 20 removed registered-opt-in-scopes from the v2 registry; keeping this
wrapper left a call to a function that no longer exists, and nothing else
referenced it. The v2.registry require goes with it.
* Fix the last v1-retirement fallout: arity, unused require, module config
visualize_test still called the 2-arity issue-ui-credential this slice retires.
usage_test required metabase.mcp.v2.api without using it, and marked a test
^:parallel that calls register-tool! — which mutates the shared registry.
Retiring v1 removes namespaces, so the module boundaries moved; regenerated
.clj-kondo/config/modules/config.edn.
* Point usage_test at v2's behaviour on the retired v1 alias
The bare /api/mcp alias now serves v2. Three assertions still described v1:
- a direct call-tool result read :isError off the bare response, but v2 returns
{:result ...}/{:error ...}
- an unknown tool is a registry rejection ({:error ...}) rather than a handler
result carrying :isError
- DELETE tore the session down and stamped ended_at on v1; v2 answers 405, so
nothing on that path ends the row
The DELETE case is a real gap, not just a test change: record-mcp-session-end!
is now callerless. Filed as GHY-4492 rather than wired up here, since choosing
where a v2 session ends is a design decision.
* Fix unregistered card-type schema keyword in check-card-can-be-saved!
`::queries.schema/card-type` was never registered; the schema is
`::card.type`. Malli threw `invalid-schema` at validation time, which
surfaced as an HTTP 500 where callers expected a 400/403.
The typo predates this merge, but nothing reached it: the REST card
endpoint still carried its own inline copy of this check, so the shared
fn had no live caller. Dropping that duplicate during the merge routed
the endpoint here and exposed it.
* GHY-4400 Give the MCP UI credential a purpose-built API (#82047)
* GHY-4400 Give the MCP UI credential a purpose-built API
`refresh_ui_credential` was a scope-escalation primitive. A token granting
only `agent:query:run` is refused `/api/user/current`, but the UI credential
minted from it was served the full profile — email, login and JWT attributes,
SSO source, group ids. Reproduced against the base commit; `/api/session/
properties` was worse, handing an admin's credential 400 settings keys against
140 for an anonymous caller, including SMTP, Slack, SAML, LDAP and embedding
secret configuration.
Both endpoints were reachable because `mcp-ui-request-surface` was an inventory
of what the embedded app happened to call rather than a decision about what the
credential should reach, and because the middleware stamped
`:token-scopes-checked true` unconditionally — which silently defeated the
`::scope/mcp-ui` stamp it sat next to. The comment claiming this "fails closed"
was wrong; it does now.
- Add `GET /api/embed-mcp/bootstrap`, returning the user and settings the
iframe needs. The user projection is a closed schema, so widening it is an
edit someone makes on purpose. Settings come back at non-admin visibility
whoever holds the credential.
- Replace the allowlist with `[method uri] -> required scope`. Query routes
and drills cost `agent:query:run` — the scope that already bought the iframe,
since the v2 shell resources declare it. `:token-scopes-checked` is set only
when the credential's signed claim covers the route, so a route added without
a scope decision is refused rather than served.
- Drop `/api/user/current` and `/api/session/properties` from the surface, and
seed the RTK caches from bootstrap instead, mirroring what the SSO path in
`embedding-sdk-ee/auth/auth.ts` already does.
The credential still reaches endpoints its minting bearer cannot: all seven
declare no `:scope`, so `ensure-scopes-checked` 403s a scoped bearer on each.
That delta is what the credential is for; what it is not is a place for routes
to accumulate. `request-surface-is-a-reviewed-allowlist-test` pins the set.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019r4TtdH2RQy1eGbQ5odnjr
* Record why the seeded caches are safe, and how they could stop being
Verified rather than assumed, against RTK 2.12.0: neither `initiate()` on a
freshly upserted entry issues a request. The SSO path in
`embedding-sdk-ee/auth/auth.ts` runs the same sequence, but its endpoints stay
reachable, so a spurious refetch there would succeed silently — it corroborates
nothing.
The subscription those calls register is also the one way this design breaks. An
invalidation of `current-user` or `session-properties` refetches, and both now
fail — the second one fails open, answering anonymously with public-only
settings rather than erroring.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019r4TtdH2RQy1eGbQ5odnjr
* Build the scope-gate test's query without a deprecated macro
`mt/mbql-query` has been deprecated since 0.61.0, and `mage kondo` exits
non-zero on warnings, so the one call reddened CI. Per-file linting cannot see
it: `:deprecated-var` needs the defining namespace's analysis cache, so the file
reads clean in isolation and only the whole-tree run reports it.
The test needs nothing from that macro beyond a valid non-native query, so
spelling out the map it expands to keeps the scope gate as the only thing
separating the two statuses.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019r4TtdH2RQy1eGbQ5odnjr
* Test the MCP iframe against the bootstrap projection, not a full user
Nothing on the frontend would have failed if the projection dropped a field the
render tree needs. The hook spec mocked the API module and seeded `{id: 1}`, so
it stayed green whatever the payload became, and the one MCP spec that mounts a
real `SdkQuestion` seeds a full `createMockUser()`.
- `createMockMcpAppsBootstrapUser` is an object literal typed as
`McpAppsBootstrapUser` rather than cast, so the build breaks if the type loses
a field.
- `createMockMcpAppsBootstrapSettings` picks the keys the backend's
`bootstrap-setting-visibilities` actually allows, derived from the setting
registry rather than guessed. The existing SDK state helpers re-expand
whatever they are given through `createMockSettings`, which would put the
admin-only keys back and make the exercise vacuous.
- `McpQuestionView.unit.spec.tsx` mounts the real question view with the store
seeded only from those fixtures, and asserts a chart actually renders.
What this does and does not catch, measured rather than assumed. Removing
`token-features` from the seeded settings turns the new spec red. Removing
`locale`, `permissions` or `is_superuser` from the user does NOT: every
consumer on the reachable path reads them optionally and defaults, so nothing
observable changes. Those three have compile-time coverage only. Runtime
coverage for them needs assertions on behavior they drive — a non-`en` locale
changing rendered text, a drill menu gated on `can_create_queries` — which is a
different test than this one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019r4TtdH2RQy1eGbQ5odnjr
* Stop asserting the UI credential reaches /api/user/current
v1's copy of the surface test still expected the pre-GHY-4400 behavior. It is
the twin of the v2 test already updated on this branch, and the only remaining
place asserting the old contract.
Its credential is minted through v1's `refresh_ui_credential`, so it is
`:legacy` — which makes this the one test pinning what a legacy credential can
still reach, coverage lost when `bootstrap-accepts-ui-credential-test` moved to
the scoped arity.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019r4TtdH2RQy1eGbQ5odnjr
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Sort the malli.util require in collections.db
Clj-Kondo and Cljfmt were both red on this file: kondo reported
`Unsorted namespace: malli.util`, cljfmt reported incorrect formatting. Same
defect from two angles — `c8d1ef7b47c`'s merge resolution placed the branch's
`[metabase.app-db.core]` above master's `[malli.util]`.
Full-tree kondo is clean afterwards, and no other file carries the same
problem.
This does not address the rest of that merge: `collections_rest/api.clj` still
has master's `/root/items` refactor reverted, which is what is failing ~164
backend assertions and 20+ e2e groups.
* Finish master's kebab-case rename on /root/items (GHY-4495)
Master's #79863 renamed the collection-items query params from snake_case to
kebab-case — `pinned_state` -> `pinned-state`, `include_library` ->
`include-library`, and the rest — updating the frontend and tests with it. Merge
`c8d1ef7b47c` applied that rename only PARTIALLY: `/root/items` kept the old
names while its own sibling `/root/items/metadata` had already been renamed, and
the test file was left similarly half-converted.
The endpoint schemas are `:closed true`, so an unrecognised param is ignored
rather than refused. Every filter silently stopped applying: `pinned-state=
is_not_pinned` returned pinned items, `include-available-models=true` returned
no `available_models`, and `collection-type` did not filter. The frontend was
already sending kebab-case, which is why this broke collection browsing broadly
rather than in one place.
Completing the rename is the whole fix:
- collections_rest/api.clj — the 17 occurrences in `/root/items`
- collections_rest/api_test.clj — 7 call sites the merge left snake_case
- collections/children.clj — docstrings still naming the old spelling
collections-rest: 186 tests, 950 assertions, 0 failures / 0 errors.
documents.api.collection-test: 0 failures. Full-tree kondo 0/0, cljfmt clean.
No remaining snake_case call sites anywhere in src/test/enterprise/e2e/frontend.
Note for GHY-4495: master's `children_query.clj` did NOT need restoring and
`collections/children.clj` did NOT need deleting. The branch's extraction (slice
16, for `browse_collection`) is fine as it stands; the failure was the rename,
not the refactor.
* Read ProseMirror node attrs by string key (GHY-4496)
Master changed ProseMirror node `attrs` from keyword keys to string keys and
made it a schema-enforced invariant: `::node.attrs` is a `string-keyed-object`
whose docstring states "every key, declared or not, is a string, so the map
never mixes keyword and string keys". The `mcpv2-01-core` merge (c8d1ef7b47)
took master's string-keyed `prose_mirror.clj` but kept the branch's
keyword-keyed consumers, so every read silently missed.
Silent, not loud: these produce wrong values rather than exceptions, and CI
was quiet about all of it.
- `update-cards-in-ast` never matched, so a card created inside a document kept
its negative placeholder id instead of being rewritten to the real one.
- `markdown/serialize` dropped a code block's language and rendered every
heading as h1, because `(:level attrs)` / `(:language attrs)` read nil off a
stored AST and fell through to their defaults.
- `changed_blocks` came back empty from document_write, since block identity is
keyed on `_id`.
- `get_content` reported `card_id` nil for every card embed, and comment
anchoring resolved no ancestors.
Every AST reaching this code from the app db is string-keyed: `:model/Document`
runs `prose-mirror/normalize-document` as an `:out` transform, and
`string-keyed-object` stringifies attrs keys on decode. Verified by round-tripping
a real Document row -- wrote `{:level 2}`, read back `{"level" 2}`.
Two sites are deliberately asymmetric: `markdown.clj`'s `segment->nodes` and
`container-node` read their own keyword-keyed parse structures and emit
string-keyed prose-mirror nodes. Converting both sides there would have broken
the parse path.
The test files hand hand-written ASTs straight to `parse`/`serialize`, bypassing
normalization, so their fixtures encoded a shape production never produces.
Converting them turned up two further defects in this commit that the source
sweep alone missed: the smartLink fallback serialized an empty label, and
`ast-id->type`/`changed-blocks` still read `:_id`.
Behaviour is unchanged wherever it was already correct -- the diff is 209
insertions against 209 deletions, all key style.
Verified: metabase.documents.markdown-test (352 assertions), plus
prose-mirror-test, search-test, mcp.v2.tools.content-test,
mcp.v2.tools.document-test, comments.render-test, search.ingestion-test --
864 assertions, 0 failures. `metabase.documents.api.document-test` improves
from 58 failures to 9; the remaining 9 (`copy-document-test`) fail identically
on the unpatched branch and are unrelated to attrs.
kondo 0 errors 0 warnings; cljfmt clean.
* Point McpQuestionView's mock import at __support__/state (GHY-4497)
The merge (c8d1ef7b47) left this spec importing `metabase/redux/store/mocks`,
a path master no longer has, failing `frontend-tests / fe-type-check` with
TS2307. Both `createMockEmbedState` and `createMockState` live under
`__support__/state` -- the barrel re-exports `./embed` and `./state` -- which is
where the rest of the tree imports them from (see
`embedding-sdk-bundle/test/server-mocks/sdk-init.ts`).
`bun run type-check-pure` goes from 66 errors to 65: this one, and nothing else
introduced. The other 65 are a missing ClojureScript build in a fresh worktree
(`cljs/*` TS2307) plus pre-existing @types/node mismatches (`Timeout` vs
`number`, `ImportMeta.dirname`), all present before this change and none in
files it touches.
The audit's second item for this ticket -- regenerating
`.clj-kondo/config/modules/config.edn` -- turned out to be a no-op:
`./bin/mage fix-modules-config` reports "config.edn already up to date", and
`metabase.core.modules-test` passes with 4492 assertions. GHY-4495 restored the
`metabase.pulse` `:api` entry the audit found dropped, so the drift was gone by
the time the ordering allowed this to run.
* Declare the MCP keyset marker in the clause options schema
MCP v2's keyset paging stamps a private marker on the filter clauses it mints,
so the next page can supersede its predecessor instead of stacking a dead
predicate beside it. Master's #82196 made `lib.schema.common/::options`
`:closed true` and enumerated every namespaced option key the platform stamps —
Mongo's `join-local`, SQL Server's `optimized-bucketing?`, the add-alias-info and
add-remaps keys. It could not have included this one: MCP v2 lives on this
branch.
So `lib/filter` began rejecting the stamped clause:
Invalid input: [nil #:metabase.mcp.v2.query{:keyset ["disallowed key, got: true"]}]
`next-page-cursor!` treats any failure to build a cursor as "this query cannot be
paged" — correct, since refusing to page is always safe — so the rejection
surfaced as cursors silently never being minted rather than as an error. That is
why it read as a guard refusal: `metabase.mcp.v2.query-test` failed 1 assertion
and errored 8 more with "Query handle not found", all downstream of a nil cursor.
Bisected: passes at `eea6bb9f914` (slice 17's own merge) and `a8024807cbc`,
fails at `c8d1ef7b47c` (the master merge). The merge changed none of
`mcp/v2/query.clj`, `mcp/v2/queries.clj` or `agent_api/query_guards.clj` — it
broke them through Lib.
Registering the key is the step #82196 performed for every other option-stamping
module, not a workaround. `{:optional true}`, so no existing query is affected.
`mcp/v2/query.clj`'s logic is unchanged: the GHY-4142 refusal contract — no
cursor unless gap-free paging is provable — is intact.
Also here:
- `internal-keys-fixture-covers-the-schemas-test` derives the declared key set
from the schema and requires a fixture entry for each, so it broke by design
until the fixture gained one. Adding it also gets the marker covered by the two
stripping tests, which is worth having.
- The `keyset-marker` docstring said the marker "rides in a clause's options map,
which no schema declares" and that undeclared keys would merely be stripped.
Both are now false — they are rejected — so it says what actually holds.
metabase.mcp.v2.query-test 62 assertions, metabase.lib.serialize-test 31, both
0 failures / 0 errors. Full-tree kondo 0/0. `:module query-processor`'s 8
failures are pre-existing, reproduced at a clean checkout of the same HEAD.
* GHY-4498 Accept a numeric id sent as a JSON string (#82294)
Accept a numeric id sent as a JSON string (GHY-4498)
Claude Desktop serializes every value of an `anyOf [integer, string]` param
as a string, so `run_saved_question` received `id: "16211"` and rejected it —
while the advertised schema's string branch says that value is valid. The
model cannot force the JSON type, so the numeric id was unreachable from
those clients and only the entity_id worked.
`normalize-id` coerces strings matching `^[1-9][0-9]*$` to integers, at three
chokepoints that between them cover every id param: `resolve-id-or-404` (the
read tools, and every collection_id/parent_id through `resolve-collection-id`),
`dispatch-write` (the `id` of all ten `_write` tools), and `get_content`'s
per-item fetch, which needs its own because alerts and subscriptions have no
entity_id column and reject a non-integer id before any resolution runs.
The `"root"`/`"trash"` sentinels are matched before coercion, and digit runs
too large for a long stay strings and fail validation like any other non-id.
Schemas are unchanged: the string branch already admitted these values.
The teaching error now names a retry that works — reading "pass a numeric id"
the obvious next move is to resend the same number, which fails identically.
Claude-Session: https://claude.ai/code/session_01EWoDYrBS4e4dAWaDeWLpVV
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Send alert_write's update only the fields the write schema accepts
Master's merge brought a new `notification/schema.clj` and closed every
notification schema, including the `hydrated-notification-schema` behind
`::NotificationApiUpdateInput`. `alert_write`'s update path was passing back the
whole hydrated notification from `get-notification`:
(notification.api/update-notification! id (assoc existing :payload payload))
An open map tolerated the read-only hydration keys. A closed one rejects them:
:creator ["disallowed key"], :id, :created_at, :updated_at
:payload {:id, :created_at, :updated_at, :disable_links, :card ["disallowed key"]}
`->mcp-error-content` sanitizes any non-caller-facing exception to a bare
"Internal error" and logs the original at a level the test runner suppresses, so
every alert_write update — including simply pausing an alert with
`{:active false}` — failed with no usable message. 7 failures / 24 errors.
The body is now built from the write shape rather than trimmed down from the read
shape, so a future hydration key cannot reintroduce this. `:subscriptions` and
`:handlers` still carry their existing rows when untouched, because the update
spec deletes whatever a body omits.
Bisected: passes at `a8024807cbc`, fails at `c8d1ef7b47c` (the master merge).
metabase.mcp.v2.tools.alert-test: 35 tests, 141 assertions, 0 failures / 0 errors.
kondo 0/0, cljfmt clean.
Not fixed here: `mcp.v2.tools.dashboard-test`'s 2 failures / 4 errors are a
DIFFERENT cause and unchanged by this. `metabase.dashboards.db/update-dashboard!`
types its `changes` as `(mut/select-keys ::dashboards.schema/dashboard.update
[:parameters])`, so any update touching `:description` or `:cache_ttl` is refused
— a dashboards-module bug, not a notification one.
* Say in bookmark_content's description that nothing reads a bookmark back
Nine of ten fleet workers, told to confirm a bookmark they had just written,
spent three or more calls establishing that no read path exists: grepping all
22 tool schemas for "bookmark" (it appears only inside `bookmark_content`),
checking `get_content`'s `include` enum, checking `search` for a `bookmarks:`
mode symmetric with its existing `recent:`, and reading the generated fields
catalog. Each concluded correctly that the write's own response is the only
report of bookmark state, but only after spending the calls to prove a
negative.
The absence is a real gap, not a defect to paper over: `search` and
`get_content` would each need the row hydrated with a per-user bookmark flag,
and `metabase.mcp.v2.projections` is by contract pure -- "no app-DB access and
no read of the current user" -- so a `bookmarked` field belongs in the fetch
layer, not the projection. That is a larger change than this one.
What is cheap now is telling the agent, so it stops paying to discover it.
* Say in search's `recent` description that it tracks UI views only
Five of ten fleet workers read content with `get_content`, then called
`search(recent: true)` expecting to see it, and got an empty list. Several
could not tell "these reads are not recorded" from "the recents index has not
caught up yet", and spent a call each establishing which.
Recents come from the view log, which the REST read paths write and the MCP
tools do not. That is the existing behaviour and this commit does not change
it -- an agent's tool calls are arguably not "views" in the sense the sidebar
means. It was simply never written down where the agent would look.
* Match master's response schema on the document copy endpoint
`POST /api/document/:id/copy` was 400ing on its own response:
"Invalid response" {:creator ["should be spelled :creator_id, received: #Instance :model/User ..."]}
The endpoint declared `:- ::documents.schema/document` and returns
`copy-document!`'s result, which is `get-document` — hydrated with `:creator`,
`:can_write`, `:can_delete`, `:can_restore`, `:is_remote_synced`. That was fine
while `::document` was open. The master merge brought its closed version, which
describes the app-DB row only ("every column of `:document`"), so a hydrated
document no longer satisfies it.
Master's own copy endpoint declares `:- [:map [:id ::documents.schema/document.id]]`
— an OPEN map that asserts an id is present and permits the rest. Its test asserts
on `:name`, `:collection_id` and the document AST, so master returns the whole
document too; the schema is permissive rather than descriptive. Adopting that line
verbatim leaves our declaration identical to master's, so there is nothing to
reconcile at the eventual merge.
Considered and rejected: adding a `::document.hydrated` schema. It worked, but put
new surface in a shared module to solve a branch-local problem, and diverged from
master where matching it costs nothing.
metabase.documents.api.document-test: 100 tests, 546 assertions, 0 failures /
0 errors — was 9 failures / 1 error. kondo 0/0, cljfmt clean.
* Let update-dashboard! take the schema its name promises
`dashboards.db/update-dashboard!` typed its `changes` as
`(mut/select-keys ::dashboards.schema/dashboard.update [:parameters])`, so every
attribute except `:parameters` was refused:
Invalid input: {:description ["disallowed key, got: nil"],
:cache_ttl ["disallowed key, got: nil"]}
`::dashboard.update` describes itself as "what an update (or insert) of a
Dashboard accepts", and `insert-dashboard!` directly above takes the whole thing.
Update accepting strictly fewer fields than insert was the anomaly; the narrowing
fit when the only caller wrote nothing else.
This branch added such a caller. `metabase.dashboards.write/update-dashboard!`
(new in `c0ddd5fcd4a`, GHY-4147 — the file does not exist on master) selects
`:name`, `:description`, `:archived`, `:collection_id`, `:cache_ttl`, `:width`,
`:position` and more before delegating here, so every dashboard_write touching an
attribute failed. Not merge damage: `dashboards/db.clj` is identical to master,
and the incompatibility is our new caller meeting master's narrower type.
mcp.v2.tools.dashboard-test: 2 failures / 4 errors -> 1 failure / 0 errors.
:module dashboards 114 assertions, 0 failures. :module dashboards-rest 1020
assertions with one pre-existing failure —
`dashboard-card-query-export-format-no-self-service-test`, an EE
download-permissions 403 — verified by running it with this change reverted.
kondo 0/0, cljfmt clean.
The remaining dashboard-test failure is GHY-4500, a separate bug: our copy of
`check-parameter-mapping-permissions` in `dashboards/write.clj` kept the narrow
`::parameter-mapping` schema that #82196 widened on the REST side, while still
assoc'ing `:dashcard-id`/`:card-id` onto every mapping.
* Declare the mapping keys the permission check is handed (GHY-4500)
Every `dashboard_write` that added or changed a parameter mapping —
`wire_parameter` by `target_tag`, `target_field` or raw `target`, and
`patch_dashcard` with `parameter_mappings` — returned "Internal error". Filters
could not be built through the MCP at all. `validate_only: true` succeeded on the
same ops, since it never reaches the check.
#82196 closed `::parameters.schema/parameter-mapping` to
`{parameter_id, target, card_id}`, and widened the REST copy of
`check-parameter-mapping-permissions` to also allow `:dashcard-id`/`:card-id`.
This branch has a second copy of that function in `dashboards/write.clj` — a file
that does not exist on master — which kept the narrow schema while lines 167/174/
183 still assoc those two keys onto every mapping before calling it:
Invalid input: [{:dashcard-id ["disallowed key, got: 177"],
:card-id ["disallowed key, got: 166"]}]
`mu/defn` throws `::invalid-input`, which carries no `:status-code`, so the MCP
sanitizer reported it as "Internal error" — see GHY-4502.
The schema now matches the REST copy verbatim, since this function is a fork of
it and the two drifting is what caused this.
Dev and test only: `mu.fn/instrument-ns?` skips prod, so schema checks do not run
there. That still covers every developer machine and CI, and the frontend was
never affected because it goes through the REST copy.
mcp.v2.tools.dashboard-test: 26 tests, 65 assertions, 0 failures / 0 errors — was
1 failure. :module dashboards green. kondo 0/0, cljfmt clean.
* Say in search's description that an empty result can be a cold index
A fleet worker searched five times for content that provably existed and got
`{"data":[],"returned":0,"total":0}` every time. Its instance's active search
index held one row where healthy peers held sixty-six -- indexing had not
finished when the worker started. Nothing in the response distinguished that
from a genuine no-match, so the worker concluded the content was absent and
abandoned search for manual browsing, turning a two-call job into about eleven.
The honest fix is for the envelope to carry the index's status, but nothing
readiness-related is exported from `metabase.search.core` today, so the tool
cannot ask. Until it can, this at least tells the agent that an empty result
has two causes and names the tools that do not depend on the index.
* Advertise the v2 scope set on every MCP path now that v1 is retired
`mcp-resource-scopes` branched on the path: `/api/metabase-mcp/v2` got the six
scopes the v2 tool registry gates on, and the other aliases got
`all-scopes` — twenty, including the fourteen per-entity agent-API scopes
(`agent:question:create`, `agent:sql:execute`, …) that v1's tools gated on.
That was right while those paths still served v1: narrowing them to the v2 set
would have told a client to request scopes no v1 tool accepted, leaving it with
an empty `tools/list`.
Retiring v1 (#81708) pointed every path at the same v2 handler and inverted the
bug. The canonical `/api/metabase-mcp` — the path the docs advertise, the one
`.well-known/oauth-protected-resource` names, and the one clients probe — served
v2 tools while advertising fourteen scopes nothing gates on, putting them on a
consent screen for capabilities that no longer exist. The old docstring
anticipated this and said the fn "collapses to the v2 answer" when v1 retires;
this is that collapse.
/api/metabase-mcp 20 scopes -> 6
/api/mcp 20 scopes -> 6
/api/metabase-mcp/v2 6 scopes -> 6 (unchanged)
`path` stays in the signature: RFC 9728 metadata is per-resource, and a future
surface may diverge again.
The tests changed because they pinned the branch itself — "a v1 alias keeps the
scopes v1's tools gate on", and an assertion literally computing
`(= (not= path "/api/metabase-mcp/v2") …)`. They now assert every path answers
the same set, which is the property that will keep holding.
No user-facing path changes: `docs/ai/mcp.md` and `llms-install.md` already
advertise `/api/metabase-mcp`, and `/api/metabase-mcp/v2` stays routed for
client configs written while the surface lived there.
Verified: 726 assertions across oauth-server.core-test, oauth-server.api-test,
mcp.v2.api-test and mcp.transport-test, 0 failures. kondo 0/0, cljfmt clean.
* Name the function when a server-side schema check fails (GHY-4502)
Any exception without a 4xx `:status-code` or an explicit MCP error code became
the two words "Internal error". The real exception was logged server-side, which
an agent cannot read and which on a dev box goes to somebody else's terminal.
That cost real time three times over on this branch alone — alert_write's
hydrated body, dashboard_write's parameter mappings, and the document copy
endpoint were all `mu/defn` schema failures reported as "Internal error", each
needing instrumentation past the sanitizer to see. The ticket records an agent
spending nine failing calls over two turns retrying variations, learning nothing,
because every answer was the same two words.
A `mu/defn` failure now names the function:
Server-side schema check failed in `check-parameter-mapping-permissions`:
[{:dashcard-id ["disallowed key, got: 177"]}]. This is a bug in Metabase, not
something to retry — report it.
The message says it is a bug rather than a caller mistake, so the agent reports
it instead of retrying variations of a call that cannot work.
Deliberately narrower than the ticket asked, on one point. The humanization is
included only for `::invalid-input`, where it describes the caller's OWN argument
echoed back. `me/humanize` embeds offending values ("disallowed key, got: 177"),
so for `::invalid-output` — which describes SERVER-produced data — echoing it
could hand the caller a row they have no right to read. Those name the function
and stop. `:value` is never touched in either case: it holds the whole argument
or return value.
Everything else still redacts: the existing SQL, JDBC and NPE cases in
`error-redaction-test` are unchanged.
The correlation-id half of the ticket is not done here.
mcp.v2.common-test: 13 tests, 91 assertions, 0 failures. :module mcp 3487
assertions with one pre-existing error in `query-test`
(`unknown-caller-key-never-reaches-the-qp-test`, a query-validation teaching
error) — verified by running it with this change reverted. kondo 0/0.
* Roll back the dashboard row when a create's second save fails (GHY-4501)
`dashboard_write` `create` with ops writes the dashboard row, then applies the
ops in a second save. When that second save threw, the row stayed. The tool's own
description tells the agent to "find it by name and finish it with update", so in
the field the agent read the error, called `create` again, and again — leaving a
pile of empty dashboards.
There was already a mitigation: the ops are compiled against a blank dashboard
before anything is written, so a bad OP cannot orphan a row
(`create-with-a-bad-op-writes-nothing-test`). It cannot catch a failure that is
not in the ops. The reported case was a `wire_parameter` create failing inside
`check-parameter-mapping-permissions` — a schema rejection the dry run never
reaches, because it does not perform the permission check.
The two writes now share a transaction, so anything throwing after the row exists
takes the row with it, whatever the reason.
Fixing GHY-4500 removed the reported trigger, but not the window: any post-create
failure had the same effect. The new test forces one directly rather than relying
on a bug to produce it, so the guarantee is pinned independently of whatever
happens to throw next.
mcp.v2.tools.dashboard-test: 27 tests, 67 assertions, 0 failures / 0 errors.
mcp.v2.dashboard-ops-test green. kondo 0/0, cljfmt clean.
* Retire the /api/metabase-mcp/v2 alias everywhere
`/api/metabase-mcp/v2` is where the v2 surface lived while it was behind a flag.
The switchover repointed the canonical path and `/api/mcp` at it, so the alias
had nothing left to do.
Removing it from `endpoint-paths` alone was not safe: the path kept serving
traffic while dropping out of everything derived from that set. In particular
`narrow-scope-to-resources` would no longer match an RFC 8707 indicator naming
it, and that path fails OPEN — an unmatched indicator narrows nothing, so a
client naming `/v2` would have kept `mb:full` where it previously lost it.
Carried through all five places:
- `mcp/paths.clj` — out of `endpoint-paths`; the docstring now names the three
things derived from the set and says what a drifted path costs.
- `mcp/core.clj` — `mcp-v2-path` deleted. It still read `mcp.paths/v2-path`,
which was already gone, so `metabase.mcp.core` would not load at all.
- `api_routes/routes.clj` — the `/v2` sub-route dropped; `/metabase-mcp` is now
a single handler.
- `oauth_server/api/metadata.clj` — the `/v2` RFC 9728 document removed. It
advertised `:resource` as the `/v2` URL, telling clients to send an indicator
nothing would recognise.
- tests — repointed at the canonical path, as the migration comments said the
switchover would ("The final switchover PR repoints this at metabase-mcp").
`oauth_server/core_test` needed real edits rather than a find-and-replace:
`/v2` was one of three paths in the union tests, so swapping the string would
have left them comparing a path against itself and quietly asserting nothing.
mcp.v2.api-test 134 assertions, transport-test 218, oauth core+api 338 — all 0
failures / 0 errors. :module mcp 3485 assertions, one pre-existing error in
query-test. Full-tree kondo 0/0. No reference to the retired path survives
outside one historical note in the paths docstring.
* Apply cljfmt to mcp.v2.tools.search
Whitespace only; the Cljfmt CI job was red on this file.
* Fix MCP Apps dropping bound template-tag values when rendering a handle
`execute_sql` mints a query_handle whose payload is deliberately wider than a dataset
query: the values bound to {{tag}} placeholders ride alongside it under `parameters`,
because `prepare-for-serialization` strips `:parameters` from the query as runtime-only.
`getMcpDeserializedCard` parsed that whole payload into `dataset_query`, so the bound
values ended up nested where nothing reads them, and the run assembled its parameters
from the card instead. Visualizing a handle re-ran the SQL with every {{tag}} unbound.
Split the payload instead: the dataset query becomes the card, and the bound values
become `initialSqlParameters`, keyed by template-tag name. That is the form the SDK
matches against the question's tag-derived parameters (by slug). Putting them on the
card would not work — they carry no `id`, and a card-level `parameters` array
short-circuits the very tag-derived list they need to be matched against.
* Restore the response schema on GET /collection/root/items
The merge from master kept the branch's endpoint body, which predates master adding
`:- ::ItemsResponse`, along with two suppressions that the branch no longer needs: the
kebab-case one went stale when the query params were renamed, and the response-schema
one put the tree one over budget on two ratchets.
* Name the required scope in the scope-denial error
The refusal read "Insufficient scope to call tool: bookmark_content" — the tool
the caller already named, and nothing it could act on. A fleet worker holding a
read-only token recorded that this contradicts the server's own `initialize`
instructions, which promise "a failed call always names its fix", and that it
had no way to learn which scope to request.
Both values are in hand at the branch: it fires on
`(not (mcp.scope/matches? token-scopes (:scope tool)))`, so the required scope
and the held set are bound right there. The message now carries them:
Insufficient scope to call tool: bookmark_content.
Requires agent:content:write; your token holds agent:content:read.
`:scope` may be a set of alternatives ([[mcp.scope/matches?]] accepts either),
which reads as "Requires one of a, b"; `token-scopes` may carry the
`::api.scope/unrestricted` keyword, which is not a scope a caller can request,
so only strings are listed back.
The tests changed because twelve of them pinned the old string exactly. They
now match an anchored prefix, so the tool name each one cares about is still
asserted while the scope detail is free to change; `registry-test` asserts the
full new shape, including the held-scope clause.
Verified: `:module mcp`, 3525 assertions, 0 failures. The one remaining error
(`query-test/unknown-caller-key-never-reaches-the-qp-test`) reproduces
identically on the unpatched tree — it is the closed-options-schema breakage
from the master merge, unrelated to this change.
* Return collection_path from get_content, as search already does
`get_content` returned a bare `collection_id`, so an agent asked where an item
lives had to spend a second call resolving the collection. Two fleet workers hit
this on the same task; one made the extra call and then noted it was redundant,
because `search`'s own rows had carried `collection_path` all along.
The path walk moves to `v2.resolve` so both tools share one implementation
rather than forking the permission handling: ancestors the caller cannot read
are omitted rather than failing the path, matching
[[collection/effective-ancestors]]'s breadcrumb semantics (A > B > C with B
unreadable reads "A/C"). `search` keeps its own batched version — it walks whole
result pages and its two lookups are per page, not per row.
`collection_path` joins `question-enrichment-keys`, the set of projection keys
computed at read time rather than selected as Card columns, so the column-select
paths (`list_models`, browse rows) drop it as they already do for
`query_summary` and `template_tags`.
Verified against a live instance: card 20 returns
`"collection_path":"Examples"`, matching what `search` reports for the same
card, and a root item returns nil rather than an empty string.
* Move the GHY-4313 unknown-key test onto the path the whitelist protects
Master closed `:metabase.lib.schema/query` in #82196, so a fresh `query` carrying an
unknown top-level key is now rejected upstream rather than reaching the QP input
whitelist. The test asserting the key is dropped ran on that path, so it errored on the
rejection before exercising the guard it exists to prove.
The guard is `select-keys` against `query-passthrough-keys`, not the schema, and the
distinction matters: the closed schema covers only the fresh path. A handle's stored
query is checked shallowly (non-empty stages, positive last-stage limit) and the
`/drills` callback stores a caller-supplied query verbatim, so an unknown key really
does reach the execution boundary there. Move the test to the handle path, where the
whitelist is load-bearing, and add a second test recording the fresh path's rejection.
Verified by mutation: replacing the `select-keys` with the raw query turns the handle
test red on `(not (contains? captured :evil))`.
* Test the MCP App parameter fix against the payload execute_sql actually mints
The spec asserted against a legacy `dataset_query` shape that was invented, not observed.
The real serialized query is MBQL 5 — `stages` rather than `dataset_query`, and
`template-tags` as an array rather than a map — captured from `prepare-for-serialization`
in the REPL. The extraction happens to be shape-agnostic, since it reads only
`parameters[].target`, so the tests passed against a fiction and proved nothing about
what the embed receives.
Rebuild them on the real payload and add the case that decides it: drive
`getCardUiParameters` and `getParameterValuesForQuestion` — the helpers
`loadQuestionSdk` uses — and assert the tag-derived parameter resolves to slug `cat` and
takes the bound value. Splitting the payload cleanly is worthless if the halves don't
rejoin there.
The value arrives array-wrapped because `normalizeParameterValue` wraps every string- and
number-typed parameter on the way to the request; that is the shape a filter widget
produces too.
Verified by mutation: making the parameter extraction return {} turns both value-carrying
tests red.
* Cover the MCP App parameter chain through to the request payload
Extends the binding test to the full path `loadQuestionSdk` walks: build the Question
from the card, apply template-tag parameters, derive values from initialSqlParameters,
set them, then assemble request parameters as `runQuestionQuery` does. Asserts the QP
receives the bound value against the target the tag declares.
This is the level the bug lived at, so it is the level worth guarding: the earlier tests
only prove the payload splits, and a clean split is worthless if the halves do not
rejoin.
* Correct the keyset-marker stripping claim in resolve-input
The comment said caller-supplied queries are stripped in "the `:query` branch above and
the `/drills` callback". `strip-caller-keyset-markers` has exactly one call site, the
`:query` branch — `POST /api/embed-mcp/drills` stores its caller-supplied query verbatim.
Documenting a guard that isn't there is worse than documenting the gap, so say what the
code does and why the gap is tolerated: reaching it needs the scoped UI credential, and
the effect is confined to the caller's own narrowed-question reporting.
* Fix Duplicate key crash in document collection-items tests
`#{normal-card-id dash-id in-document-card-id}` is a runtime set, and
`PersistentHashSet/createWithCheck` throws on duplicates rather than deduping. Card and
Dashboard ids come from separate sequences, so in a fresh app DB where this namespace is
the first to create both, each gets id 1 and the literal blows up before any assertion
runs. It survived full-suite runs, where thousands of preceding rows keep the sequences
apart, and failed only under `:only`.
Match on (id, model) pairs instead. The vectors differ by model string, so no duplicate is
possible, and it also closes the correctness hole the bare id set had — an unrelated
Dashboard sharing a Card's id would have been pulled into the filtered set.
Applied to the latent third instance in the trash-items test too, which spans Document,
Card, and Dashboard sequences and was one id alignment from the same crash.
metabase.documents.api.collection-test: 11 tests, 66 assertions, 0 failures (was 64 pass,
2 errors).
* GHY-4510 Withhold query_summary when a source card is unreadable
get_content on a card the caller can read rendered `query_summary` with
`lib/describe-query` over `application-database-metadata-provider`, which applies no
per-user read check. For a wrapper whose query sources a restricted card, that resolved
and printed the restricted card's display name — the one read surface that leaked it,
while the direct read 403s, the run path refuses via `check-card-read-perms`, and search
filters it out.
Gate the summary on the caller being able to read every card the query reads, at any
depth (`all-source-card-ids-recursive`, so a transitively nested source counts too).
Fails closed: a missing source card, or a query that can't be walked, withholds the
summary rather than rendering from metadata we cannot account for.
question, model, and metric all resolve through `fetch-card`, so one gate covers the
three types the report named.
The test drives the report's attack — restricted card in a revoked collection, readable
wrapper sourcing it — and asserts the name appears nowhere in the response, not merely
that `query_summary` is nil. Verified failing first: `query_summary` came back as the
restricted card's name verbatim.
metabase.mcp.v2.tools.content-test: 41 tests, 224 assertions, 0 failures.
* Placeholder the table and field ids in execute_query's examples
The description stated `ORDERS = table 5` and `CREATED_AT = field 42` as fact.
On a stock Sample Database ORDERS is table 2, and table 5 is FEEDBACK — a real
table — so an agent copying the example queries the wrong table and gets a
plausible answer with no error anywhere in the trace.
A fleet worker did exactly that: `execute_query {stages:[{source-table: 5,
limit: 5}]}` returned FEEDBACK's columns (ID, ACCOUNT_ID, EMAIL,
DATE_RECEIVED, RATING, ...). Nineteen of twenty-five workers on one case
flagged the discrepancy and looked the id up instead; the ones who did not
would have had no signal.
Correcting the numbers would not fix it. The ids are per-instance, and the
fleet saw 5 resolve to FEEDBACK on most instances and REVIEWS on another from
the same seed data — so any literal id in this docstring is wrong somewhere.
`<TABLE_ID>` and `<FIELD_ID>` cannot be pasted.
The same description already said "never invent or guess ids — discover ids
first", which its own examples contradicted.
* Decouple the MCP App parameter test from getParameterValuesForQuestion
CI type-checks the PR merged with master, and master changed that helper's signature from
{card, metadata, queryParams} to {card, queryParams, buildQuestion}. The spec compiled on
the branch and failed on the merge — a semantic conflict, clean to merge and broken to
type-check.
Call `getParameterValuesByIdFromQueryParams` against `question.parameters()` instead. That
is precisely what the helper delegates to on both sides — the old form passed
`getCardUiParameters(card, metadata)`, the new one passes `buildQuestion(card).parameters()`,
and `Question.parameters()` is `getCardUiParameters` over the same card — so the test
asserts the same semantics while no longer depending on a signature in flux. Both
primitives it now uses are byte-identical on master.
8 tests green; type-check, eslint, and oxfmt clean.
* GHY-4511 Make a question's visualization_settings readable (#82355)
* GHY-4511 Make a question's visualization_settings readable
A chart's settings could not be read back: not in concise, not in detailed,
not through `fields`, and `question_write` did not echo them either. An agent
could write a goal line or a series binding and had no way to check it.
- `get_content` gains a `settings` include for question/model, returning the
stored `visualization_settings` verbatim ({} when nothing is stored).
- `visualization_settings` joins the question detailed projection, which also
makes it a valid `fields` path. It stays one whole-blob path: its own keys
contain literal dots, so nothing deeper could ever be addressed.
- `question_write` echoes `visualization_settings` in its response.
- A `fields` path with no stored value now answers null instead of dropping
out, so an unset `cache_ttl` no longer looks like an unreadable field. A
path through a missing parent still drops — a null-valued shell would claim
a parent that isn't there.
- The visualization-settings skill's "To confirm" section now describes what
ships.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TUQL4KAPYRe7DJbwRF4dTy
* Name the include after the property it returns
Review: `include: ["settings"]` returned `visualization_settings`, so the
section name and the property name had to be translated by anyone reading
either. `visualization_settings` is also the name every other surface uses —
the REST API, question_write's args, the card model, the skill — so it is the
one a model reaches for unprompted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TUQL4KAPYRe7DJbwRF4dTy
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* GHY-4512 Read a link card's entity back behind a read check (#82357)
`add_link` with `entity` saved correctly all along. `get_content`'s `layout` include stripped `:entity` from every link card on the way out, so an entity link read back as `link: {}` — indistinguishable from an empty card, and impossible to round-trip through `patch_dashcard`. The strip was there to stop the `name`/`description` cached beside the reference at write time from reaching a caller no read check stands behind.
`dashboard-layout` now hydrates `:dashcard/linkcard-info`, the same batched hydration the REST dashboard endpoint runs, and returns the bare `{model, id}` reference — the shape `patch_dashcard` takes back verbatim — when the caller can read the target, and `{restricted: true}` when they cannot. The cached name and description still never leave the server.
Claude-Session: https://claude.ai/code/session_019aHyUCgLuS6QzkcRcZDcdd
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* GHY-4516 Let collection_write create transform folders (#82356)
browse_collection browses the transforms namespace and transform_write
files a transform into a collection_id, but collection_write's namespace
enum was "snippets" only — so the folders those two tools talk about
could not be made through MCP at all.
Update needed no change: namespace is already create-only, a move stays
inside the row's own namespace, and parent_id "root" lands at the root of
the transforms tree rather than the content root. Tests pin that.
Claude-Session: https://claude.ai/code/session_015dqtq17qCpVgMeRvu9uPWt
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* GHY-4517 Honor `name` on measure clauses (#82358)
The measures middleware substitutes a measure's underlying aggregation for
the `:measure` clause, carrying `:lib/uuid` and `:display-name` across but
dropping `:name`. A caller-supplied `{"name": "revenue"}` passed validation
and was then silently discarded, so the output column came back as the
underlying aggregation's name (`sum`, `count`) and a next-stage
`["field", {}, "revenue"]` failed with a raw database error.
The metrics middleware already preserves `:name`; a regression test now
covers that too. Also documents in the query-dialect skill what a metric's
or measure's output column is called.
Claude-Session: https://claude.ai/code/session_01Egi1Dqi8XGihQzQ5BmQYat
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Keep the MCP Apps iframe working when an endpoint declares a scope (#82361)
* Let enforce-scope defer to the MCP Apps UI route gate
An MCP Apps UI credential is not an OAuth token, and an endpoint's declared `:scope` is not
the vocabulary that confines it. `metabase.mcp.ui-surface/request-surface` is — a handful of
routes, each priced in the MCP scope the minting session had to hold — and it is strictly
narrower than any endpoint scope could be.
Those two mechanisms currently refuse each other. The credential carries `::mcp-ui`, which
satisfies no declared `:scope` by design, so declaring one on any route of that surface 403s
the iframe while every other caller is unaffected: an ordinary session carries
`::unrestricted` and passes. It merges clean and fails silently — the iframe stops rendering.
`ensure-scopes-checked` already honours `:token-scopes-checked` as "an authorization decision
already happened upstream". `enforce-scope` did not, and that asymmetry is the bug. The MCP UI
stamp is set only when the surface's own scope gate passes, so honouring it defers to a
route-specific decision rather than waiving one.
Conditioned on `::mcp-ui`, not on the stamp alone: `enforce-scope` sets the stamp itself on
success, so trusting it unconditionally would make every per-endpoint `:scope` a no-op
underneath a namespace-level `enforce-scope`. The test asserts that non-leak directly.
* Guard every MCP UI surface route against the endpoint scope layer
Drives each route on `request-surface` with a real UI credential through the real stack and
asserts the response is not a scope rejection — neither `unsupported_scope` from
`enforce-scope` nor `scope_not_permitted` from `ensure-scopes-checked`.
Asserting on the error codes rather than on a status or a scope name is what makes this
independent of whichever scope a future branch declares, and of what the handler then does
with a deliberately thin probe body: a 400 is a pass, because the request reached the handler.
A coverage assertion pins the probe set to the surface, so adding a route without a probe
fails rather than silently skipping the check.
Verified by mutation, both directions: declaring a `:scope` on POST /api/dataset turns this
red on exactly that route with the carve-out removed, and green with it in place.
* drop v2 ping tool and update llm-install (#82362)
* update llm-install
* drop v2 ping
* Drop the require comments and redundant keep-me around test_echo
* GHY-4518 Summarize a native question by its SQL, and name its database (#82363)
`query_summary` delegates to `lib/describe-query`, whose native-stage display
name is the placeholder "Native query" — enough in the UI, where the SQL renders
beside it, and nothing at all to a caller holding only the row. An audit of an
all-native dashboard had to guess each card's purpose from its description.
Native cards now summarize as the head of their own query text: whitespace
collapsed to one line, capped at 300 characters. The full query stays behind
`include: ["definition"]`, so the summary can stay a summary. A non-string
native query — Mongo stores a map — keeps the generic Lib description rather
than rendering a map.
Both branches stay behind the `source-cards-readable?` gate from GHY-4510: a
native query names a Card in a `{{#123}}` template tag as readily as an MBQL
stage names one in `:source-card`, and `all-source-card-ids` covers both.
The row also carries `database_name` now, read off the metadata provider it
already builds. Resolving `database_id` to a name previously cost a second
`search` call. It is flat rather than nested to match the vocabulary `search`
rows already use, and it joins `question-enrichment-keys` so the column-select
paths drop it instead of selecting a column that does not exist.
Claude-Session: https://claude.ai/code/session_01GXJcdTASE4jxhmE4fzRNkw
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* GHY-4519 Answer a date parameter with its range, not hundreds of dates (#82366)
`get_parameter_values` returned every distinct date in the column for a
`date/*` parameter — 721 on one dashboard, 100 per page — when what a caller
needs is the span and the grammar it can filter with.
A column-backed date parameter now answers:
{"kind": "date", "min": "2016-04-30", "max": "2020-04-19",
"distinct_dates": 18760,
"accepts": ["YYYY-MM-DD", "YYYY-MM-DD~YYYY-MM-DD", "past30days", "thisyear"]}
The range is aggregated rather than folded out of the fetched values, because
listing stops at `result-limit` and values come back ascending: the list's last
value is the 1000th-earliest, not the column's max. On orders.created_at that
gap is fourteen months — the list ends at 2019-02-22 against a real max of
2020-04-19. `chain-filter-range` builds the same source table, joins and
constraint filters the values query builds, aggregated to one row, so
constraints narrow the range exactly as they narrow a list.
Excluded: a date parameter with a static-list or card source, whose dates are a
curated set to pick from rather than a span to write a value inside.
`query` on a date parameter is now a teaching error. A range is two dates and a
grammar, so there is no list to search, and applying it silently would hand back
the whole span as though it had been narrowed.
Claude-Session: https://claude.ai/code/session_01GR7MMvsg2kH4dE24YyX1kS
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Address review nits in useMcpApp
- Drop the unjustified cast and FIXME on window.metabaseConfig; the global
Window.metabaseConfig declaration already types instanceUrl.
- Remove the redundant void operator on the async IIFE.
- Remove the redundant JSDoc on McpAppState.queryError.
* Address review: drop the unreachable v1 payload path, trim comments
Review feedback from sanex3339 on #81245.
Two of the comments were questions — whether the iframe can still receive a v1 response, and
whether the v1 field needs describing, given FE and BE ship from one instance. The answer is
that it cannot, and more strongly than the questions assume: v1 was deleted from this branch
in 84c334c6d2, and both v2 UI tools put `{query_handle}` in `structuredContent` and nothing
else (their output schemas declare only that key). So the inline `query` field, the branch
that read it, and the comments explaining both are dead. Removed.
That removal surfaced why the dead path survived: 8 tests drove the v1 inline shape, so the
suite exercised a payload the backend cannot produce. The fixtures now carry handles — the
real shape — and resolve through the mocked callback API. One test asserted the dead path
outright ("still renders v1's inline query") and is gone with it.
Also replaced the `// Unjustified type cast. FIXME` on `structuredContent` with a runtime
narrowing that checks each field instead of asserting the whole shape, and shortened the
comments flagged across the four other files. Kept the two that carry a hazard rather than a
restatement: the RTK subscription warning in `useMcpUserAndSettingsFetch`, and why
`McpQuestionView`'s spec must not use `mockSettings`.
20 suites, 68 tests green; type-check, eslint, oxfmt clean.
---------
Co-authored-by: Eric Normand <ericwnormand@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Aleksandr Lesnenko <alxnddr@gmail.com>
Co-authored-by: Aleksandr Lesnenko <alxnddr@users.noreply.github.com>
6.4 KiB
6.4 KiB
name, description
| name | description |
|---|---|
| clojure-write | Guide Clojure and ClojureScript development using REPL-driven workflow, coding conventions, and best practices. Use when writing, developing, or refactoring Clojure/ClojureScript code. |
Clojure Development Skill
Tool Preference
When clojure-mcp tools are available (e.g., clojure_eval, clojure_edit), always use them
instead of shell commands like ./bin/mage -repl. The MCP tools provide:
- Direct REPL integration without shell escaping issues
- Better error messages and feedback
- Structural Clojure editing that prevents syntax errors
Only fall back to ./bin/mage commands when clojure-mcp is not available.
@./../_shared/development-workflow.md @./../_shared/clojure-style-guide.md @./../_shared/clojure-commands.md
REPL-Driven Development Workflow
- Start with small, fundamental functions:
- Identify the core features or functionalities required for your task.
- Break each feature down into the smallest, most basic functions that can be developed and tested independently.
- Write and test in the REPL:
- Write the code for each small function directly in the REPL (Read-Eval-Print Loop).
- Test it thoroughly with a variety of inputs, including typical use cases and relevant edge cases, to ensure it behaves as expected.
- Integrate into source code:
- Once a function works correctly in the REPL, move it from the REPL environment into your source code files (e.g., within appropriate namespaces).
- Gradually increase complexity:
- Build upon tested, basic functions to create more complex functions or components.
- Compose smaller functions together, testing each new composition in the REPL to verify correctness step by step.
- Ensure dependency testing:
- Make sure every function is fully tested in the REPL before it is depended upon by other functions.
- This ensures that each layer of your application is reliable before you build on it.
- Use the REPL fully:
- Use the REPL as your primary tool to experiment with different approaches, iterate quickly, and get immediate feedback on your code.
- Follow functional programming principles:
- Keep functions small, focused, and composable.
- Use Clojure's functional programming features—like immutability, higher-order functions, and the standard library—to write concise, effective code.
How to Evaluate Code
Bottom-up Dev Loop
- Write code into a file.
- Evaluate the file's namespace and make sure it loads correctly with:
./bin/mage -repl --namespace metabase.app-db.connection
- Call functions in the namespace with test inputs, and observe that the outputs are correct
Feel free to copy these REPL session trials into actual test cases using
deftestandis. - Once you know these functions are good, return to 1, and compose them into the task that you need to build.
Writing Docstrings
A docstring is a contract for the caller, not a diary for the implementer. It states what the function does, what it takes, returns, throws, and the preconditions/invariants the caller must respect. Those guarantees and requirements belong there — they are exactly what the caller needs surfaced in the IDE.
When you find implementation context in a docstring, the default is to relocate it, not delete it — move it to an inline comment at the point in the body where it is actually relevant. That context is often genuinely valuable; it is just in the wrong place (the caller should not have to read it; the implementer standing at that line should). Delete outright only when it is blather: self-congratulation, restating the obvious, or documenting a property that is the expected default.
On that last case — narrating properties like "portable across all supported appdbs" earns no sentence. If it were not portable, that is either a bug, or it means callers must handle each case themselves — and in that case it is the absence of the property that must be documented. Document deviations from expectation, not conformance to it.
Heuristic — a sentence earns its place in the docstring only if it passes both tests:
-
Rewrite test (necessary, not sufficient): if rewriting the body to a different implementation with an identical contract could make the sentence false, it describes the implementation, not the contract. Relocate it to an inline comment at the point in the body where it applies.
-
Ownership test: if the sentence states a fact about code this function does not own — a callee's behavior, a library's guarantee — it survives any rewrite trivially, which is exactly why the rewrite test alone is not enough. If this body relies on that fact, it is implementation context: relocate it to the call site. If nothing here relies on it, it is blather about someone else's code: delete it, and let the callee's own docstring answer for it.
Say each fact once. Duplicated prose is a stale comment in advance: when the fact changes, one copy gets updated and the rest quietly rot. This holds even within code you own — a namespace docstring that re-summarizes its own multimethods, three sibling functions carrying the same paragraph about retry behavior. Give the fact one home, and have the other places name it rather than restate it.
Multi-line docstrings are not banned — a genuinely non-obvious constraint the code had to deal with can be worth explaining. But be prudent; the failure mode is far too much detail. When tempted to write a multi-paragraph explanatory docstring, check with the user first. And prefer a test to prose: if a future reader thinks "that's a silly way to do it" and changes it, a test should fail and tell them why. If that breakage keeps happening, that is the signal a comment was warranted.
Critical Rules for Editing
- Be careful with parentheses counts when editing Clojure code
- After EVERY change to Clojure code, verify readability with
-check-readable - End all files with a newline
- When editing tabular code, where the columns line up, try to keep them aligned
- Spaces on a line with nothing after it is not allowed
- After changing module boundaries (adding/removing/renaming a
srcnamespace, a cross-modulerequireor:model/Xreference, or a new module), run./bin/mage fix-modules-configto regenerate.clj-kondo/config/modules/config.ednand keepmetabase.core.modules-testgreen. No-op when nothing drifted; see "Module Boundaries" in the projectCLAUDE.md.