master
73 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Fraser
|
afa79587c2 | Lint without building cljs (#83718) | ||
|
bryan
|
6cb8e9e904 |
Add API changelog tooling: semantic OpenAPI diff with severity ranking (#82688)
* Add API changelog tooling: semantic OpenAPI diff with severity ranking
Answering "has the API changelog been updated?" currently means reading a
10k-line JSON diff by hand, so it falls to whoever is asked in Slack. This adds
the mechanical part of that job as two mage tasks.
`./bin/mage openapi-diff OLD.json NEW.json [--severity breaking]`
resources/openapi/openapi.json is generated from the Malli endpoint schemas, so
the spec at any git ref is what the API was at that ref. The task parses two
specs into {METHOD /path -> shape} maps, inlines $refs (component schemas change
too), and compares params, request body, and response schemas.
Findings are classified BREAKING / ADDITIVE / DOC_ONLY and sorted breaking-first.
A change is breaking when it requires MORE from the caller or provides LESS to
the caller; anything else is just a change. That rule is directional and it
inverts between request and response, so severity is decided from both schemas
rather than from how a change happens to render:
request breaking: newly required, type/enum narrowed, schema closed, field removed
additive: new OPTIONAL field or param, type/enum widened, made nullable
response breaking: field removed, field may now be null
additive: new field returned
Adding an optional parameter is not breaking; returning extra data is not
breaking. A public diff of two Stripe specs found 663 of 679 changes additive,
which is why filtering matters more than summarizing.
`./bin/mage openapi-staleness`
The self-healing CI job runs only on PRs labelled `openapi-self-healing`, so the
committed spec drifts; it is currently ~38 API commits behind. A stale spec makes
the diff under-report, so this reports drift and names the commits rather than
letting a clean diff read as "no changes". The label gate is deliberate (#67854
introduced it to stage the rollout after #67294 removed the spec entirely over
merge conflicts), so it is left alone here.
Response coverage is partial: ~199 response entries declare a schema and can be
compared, ~1,700 are description-only 2XX/4XX/5XX stubs and cannot be.
Tests cover both directions of every request and response rule, $ref resolution
including the SCIM schemas whose property is literally named "$ref", a guard
against the failure mode in mozilla-ai/otari#1315 (breaking markers dropped at
render time), and a signal-to-noise case asserting additive bulk cannot drown
the breaking few.
* Generate specs from git refs, so the diff is not bound to the committed blob
`./bin/mage openapi-diff --refs <old-ref> <new-ref>` checks each ref out into a
throwaway worktree, runs that ref's own `generate-openapi-spec`, and diffs the
two results. Nothing is committed and no checkout is touched.
This removes the staleness problem rather than reporting it. The committed
`openapi.json` only updates on PRs labelled `openapi-self-healing`, so it lags
source badly: `--committed origin/release-x.63.x origin/master` reports 0
findings, because both refs carry the same stale blob. Generating from source
reports 670 -> 714 operations and 10 removed endpoints for the same pair.
It needs no support from the ref being diffed beyond `generate-openapi-spec`
already existing, so it works on historical refs. `--committed` keeps the fast
path and now prints a warning that its result under-reports, since an empty diff
from a stale spec is indistinguishable from an API that did not change.
Validated against the real v63 -> master delta. The top removals are the three
`GET .../execute` endpoints that the hand-written 0.64 changelog documents as
replaced by `POST .../execute/values`, and it independently surfaces a query
param rename (`pinned_state` -> `pinned-state`) that has no entry yet.
That run also shows a precision limit worth stating: it reports 239 breaking
findings, but 217 are the identical `additionalProperties: false` line from one
PR (#82447, closing mu/defn argument schemas). The classification is right --
closing a schema does require more of the caller -- but as changelog input that
is one entry, not 217. The skill now tells the drafter to collapse findings by
text and treat a repeated finding as a single systematic change.
* Group identical findings so a systematic change reads as one entry
`--grouped` collapses findings with identical text across endpoints and sorts by
blast radius, because the per-endpoint view is the wrong shape for drafting a
changelog.
On the real v63 -> master delta it turns 239 breaking findings into 113 distinct
changes. The largest spans 217 endpoints and is a single upstream PR (#82447,
closing mu/defn argument schemas): one changelog entry, reported once with its
endpoints, instead of 217 identical lines to scroll past.
Grouping is a separate render mode rather than a transformation of the default.
The per-endpoint view answers "what changed on this route?", the grouped view
answers "what distinct changes happened?", and collapsing the former into the
latter would lose the ability to check a specific endpoint.
The long tail is where the individually-interesting entries live: the same run
surfaces a `pinned_state` -> `pinned-state` query param rename that no changelog
entry covers yet.
* Fix review findings: two defects let breaking changes pass silently
Eric's review found 13 issues. The two that mattered both caused false
negatives - the tool reporting nothing where a caller would break.
The depth guard counted nesting levels, not `$ref` hops. `seen` already breaks
cycles, so the counter was doing a different job than its name claimed: past 12
levels both specs collapsed to the literal "<deep>" and compared equal, hiding
every change in 52 of 637 operations. It now advances only on a `$ref` hop, and
52 falls to 17.
`response-lines` compared only top-level properties and delegated anything deeper
to `(widening? new old)`. Swapping those arguments inverts value-set semantics -
which is why the nullable case read correctly - but not object-property
semantics, so a removed nested response field read as additive. It now recurses
the way `body-lines` does.
Chasing the second one surfaced a third of the same shape: `widening?` never
compared array `items`, so a narrowing inside an array was invisible. Found by a
test written for the SCIM `$ref`-named-property case.
Also fixed:
- `committed-spec!` built a shell command by interpolation, so a ref containing
`$`, a quote, or a space produced a wrong file. Values now ride as positional
arguments and the shell never parses them.
- The `--grouped` header counted severity-filtered-out changes, inflating the
count above the body printed beneath it.
- The removed and added endpoint lists were severity-gated, so `--severity
breaking` hid the added half of an endpoint move - at exactly the severity the
skill tells a drafter to start with. Both lists now print at every severity.
- The staleness check watched 66 of the 158 files that contain `defendpoint`,
missing `src/metabase/*/api/*.clj` and all of `enterprise/backend/src`, so it
could report `OK: spec is current` with dozens of newer endpoint files on disk.
It also compared `%cI` strings across a history carrying both `+08:00` and `Z`,
and listed commits from a narrower pathspec than the date came from.
- `DOC_ONLY` was unreachable: a description-only change produced no findings and
was dropped. Descriptions are what the skill reads to spot changes it cannot
explain, so a reworded docstring now surfaces as doc-only.
- A misspelled `--severity` fell through to nil and silently printed everything.
It now exits. `--committed` without `--refs` was accepted and ignored; it now
exits too.
- Each `--refs` run left two multi-megabyte specs in /tmp.
- `brief` truncated its non-map branch without the "..." its map branch adds.
- The first commit message claimed a test covered the SCIM `$ref`-named property.
No such test existed; one exists now, along with tests for the depth guard, the
nested response removal, and the doc-only path.
One correction to that commit message stands: `babashka.json` would have avoided
the `:discouraged-namespace` ratchet entirely, since `mage/doctor.clj` already
uses it unsuppressed. Keeping cheshire for consistency with the three other mage
namespaces that use it; the budget moves 68 -> 69 against current master.
The `de-slop` skill the review could not find does exist, at
~/.claude/skills/de-slop - it is a personal skill, not in the repo.
* Fix second review: four more directions where a finding was inverted or absent
All nine findings addressed. The reviewer supplied a failing test for each of the
first five; all five reproduced on the previous head, and all now pass.
Three of the four major findings were the same mistake in different places: a
schema shape that the output path handed to `widening?` with swapped arguments,
which inverts value-set semantics but not object-property semantics.
- A response array of objects went to the leaf branch, so a removed element field
read as additive and an added one read as breaking. 24 of the 199 comparable
response schemas in the committed spec are top-level arrays. Arrays now recurse
through `items` in the output direction.
- `schema-props` unwraps a nullable `oneOf`, so a nullable OBJECT reached the
object branch, where only keys and requiredness were compared and the type
change was invisible. The committed spec has 726 nullable object properties.
Both `body-lines` and `response-lines` now compare type sets there, with the
severity test pointing the correct way for each direction.
- `response-lines` discarded the required sets, so a response field the API may
now omit produced nothing - the same loss of guarantee as one that may now be
null, which was already breaking. 174 of 637 operations declare `required` in a
response schema.
The fourth major finding was a shortcut in `widening?`: any empty old schema was
treated as "no constraint", so a field going from Malli `:any` to a concrete type
read as additive. `operations` stored an absent body as `{}`, which made "no body"
and "any value" indistinguishable; an absent body is now nil and `{}` falls
through to the type and enum checks. After ref resolution the committed spec has
1,182 property schemas that are `{}`.
The depth limit is gone rather than re-tuned. `seen` already terminates every
path against a finite component set, so the limit only ever replaced deep nodes
with the same literal on both sides - which compares equal and hides the change.
The MBQL clause schemas form chains long enough to hit it: 17 operations carried
`<deep>`, and now none do while 18 terminate on genuine cycles via `<recursive>`.
A self-diff of the committed spec costs 2.9s instead of 1.4s and still reports 0
findings.
Also fixed:
- `format-epoch` shelled out to `date -r`, which only BSD and macOS read as epoch
seconds; on GNU coreutils it means "the mtime of this file" and printed nothing,
silently, because the call was `:quiet?`. Now `java.time`.
- `openapi-staleness` claimed the spec was "current" from endpoint files alone.
661 of 763 component schemas live outside `.api` namespaces, so a shared-schema
change moves the generated spec invisibly. The OK message now says what it
actually checked and points at `--refs`.
- A `--refs` run that could neither generate nor read the second ref leaked the
first spec: materialization happened in the `let` bindings, outside the `try`
whose `finally` removes the directory.
- The skill told an agent that two identical committed specs prove the API did not
change, and that the commit touching `openapi.json` is the PR that changed the
API. Both are wrong for the same reason - the committed spec only moves on
`openapi-self-healing` PRs. It now says to re-run without `--committed` before
reporting no changes, and to search endpoint source for attribution.
* Drop the ratchet bump: babashka.json instead of cheshire, and fix stale docs
The `:discouraged-namespace` increase was avoidable and the rationale I gave for
it was backwards. `babashka.json` is not in the discouraged list, and 6 mage
namespaces already use it unsuppressed against 5 on cheshire - so the
"consistency" argument pointed the other way. Swapping to it removes both the
68 -> 69 bump and the `:clj-kondo/ignore`, and this PR no longer touches
.clj-kondo/ratchets.edn at all. `read-str` keywordizes by default, so the two
spec reads pass `{:key-fn identity}`; this namespace is string-key throughout.
Verified unchanged behavior: self-diff still 0 findings, the real 0.64 change
still 1 breaking.
Three docstring counts were wrong. The `api-source-pathspecs` docstring claimed
238 `/api/ee/` paths (actual 106 - it looks like 106 paths plus 132 operations,
double-counted), 66 files matching `src/metabase/**/api.clj` (actual 65), and 158
files containing `defendpoint` (actual 160). The argument the docstring makes is
unaffected - the narrow pathspec still misses most endpoint files - but the
numbers are now the measured ones.
Also removed documentation that described code this branch already deleted:
- A comment in `resolve-refs` said "Only a $ref hop advances depth" while the
docstring 17 lines above correctly says there is no depth limit. There is no
depth counter left; the comment described the previous commit's version.
- Two test comments and a test name referenced a `<deep>` placeholder guard that
no longer exists, and cited a "52 of 637" measurement of the mechanism that was
removed. The tests themselves are valid - they assert deep and long-chain
changes are detected - so only the prose changed.
- SKILL.md quoted "`--committed` reports 0 findings" for v63 vs master as a fixed
figure. That was true when written and is not now: the blobs have since
diverged and it reports 1. The underlying point stands, so it is stated
qualitatively rather than as a number that drifts.
- SKILL.md said `--refs` "works on any ref". The code only attempts that and has
a documented fallback for refs that will not build with the current toolchain;
the text now says so.
* Fix third review: object unions and nullable arrays hid breaking changes
- A union of two or more object variants was compared by its first
variant only, so removing or editing any other variant produced no
finding. Such unions now compare whole: widening only when every old
variant survives unchanged.
- Nullable arrays (`oneOf [array, null]`) skipped the array branch, so a
change to their items was ranked additive. Both array branches now
unwrap the null first.
- The render test never rendered. It now asserts breaking operations
print under the breaking headings of both renderers, and a new test
covers the --severity filter.
* Rank changes to unmodeled schema keywords breaking, and compare bounds, allOf, and tuples
The old widening check returned "safe" for any keyword it did not
recognize, so each review round found another way for a breaking change
to read as additive: allOf, minimum/minLength, map-of value schemas,
prefixItems, nullable enums, closed nullable objects.
`compatible?` replaces it. It takes a direction (:request or :response),
compares unions variant by variant, compares bounds, pattern, and format,
and compares allOf and prefixItems by position. A change to any keyword
it does not model ranks breaking. A false breaking finding costs one
look; a missed one ships undocumented.
Documentation keys (description, title, examples, default) are stripped
before comparison, so a reworded field description is doc-only instead
of an additive schema change.
Also:
- When two summaries are identical, the finding names where the schemas
differ (e.g. `items.enum: +"measure"`), so distinct changes no longer
merge in the grouped view.
- Generation deletes the checked-out spec before running, so a generator
that logs and swallows an error no longer passes the committed spec off
as fresh. A timeout falls back to the committed spec instead of
throwing.
- An empty or non-spec input file exits 1 instead of reporting 0 BREAKING.
- openapi-staleness lists commits after the spec commit, not including it.
* Close the gaps an independent check found in the compatible? rewrite
- Only a nullable union unwraps to its object. `string | object` and
similar unions are compared whole, so a change to the non-object
variant is no longer hidden.
- Keywords beside an object's properties (allOf, a map-of
additionalProperties, minProperties) are compared.
- If the field walk finds nothing but the stripped schemas differ, the
whole schema is ranked instead of falling through to doc-only.
- Inclusive and exclusive numeric bounds compare as one bound.
- A response allOf may gain members (narrowing output); a request allOf
may lose them.
- An enum or const without a type widening to its type is additive.
- requestBody.required and request content types are compared, and a
response that declares a schema for the first time is reported as
additive.
- A missing input file says so.
* Match operations across path-variable renames; count only visible severities in the grouped header
- Operations and path params are keyed by position, not variable name.
Renaming `{id}` to `{card-id}` leaves the URL a caller sends unchanged,
so it is now a doc-only "path variable renamed" finding rather than a
removed endpoint plus an added one. Routes that would merge under
position-keying (`/api/database/{id}/schemas` and
`/api/database/{virtual-db}/schemas`) keep their names. Reports still
print the real spec paths.
- At --severity breaking, the grouped header no longer counts the
new-endpoint list, which is additive. The list still prints.
- The skill's attribution section no longer opens with the
openapi.json git log that it then says names the wrong author.
* Rank a changed or dropped request default as breaking
`default` was stripped with the documentation keys, so a default that
flipped (say a query param from false to true) ranked doc-only and
--severity breaking hid it. The schema still validates the same
requests, but a caller who omits the field now gets different
behavior.
Request defaults are now compared: changed or dropped is breaking,
added where there was none is additive. Response defaults are ignored.
* State one definition of breaking that covers every rule
A change is breaking when an existing caller, sending exactly what it
sent before, can now fail or get a different result. The old wording
(requires more / provides less) did not cover changed defaults, and it
presented two conservative rules - removed request fields and changes
to unmodeled keywords - as if they followed from it. Those are now
stated as what the tool does when it cannot tell.
* Skill: point to docs-write, and attribute with all six API source pathspecs
de-slop is a personal skill the repo does not ship; docs-write carries
the Metabase style guide. The attribution git log used three of the six
pathspecs in api-source-pathspecs and missed enterprise api/ dirs and
routes.clj files (35 tracked files, e.g. sso/api/saml.clj).
|
||
|
dpsutton
|
07534d6a33 |
Fix stale references in backend expert agents and module-conventions skill (#83658)
- backend-module-conventions: the retire-a-changeset precedent cited a nonexistent id (v59.2026-07-10T22:29:18). Point at the two real precedents and describe both placements they show (same file, same release vs. a later release retiring an older one). Drop the last em-dash. - modules-backend-expert: description path is .clj-kondo/config/modules/config.edn. - platform-backend-expert: run kondo hook tests through bin/test-agent (they are on the dev classpath) instead of raw clj -X:dev:test, which CLAUDE.md says not to use. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
bryan
|
6aa6b1560f |
Revamp backend expert subagents; add shared backend-module-conventions skill (#83426)
* Revamp backend expert subagents; add shared backend-module-conventions skill Claude routes on each agent's frontmatter description, and every session loads all of them. The eleven descriptions were ~29KB of <example> blocks; they are now ~440 chars each (~4.9KB total), shaped as "expert for X. Use when Y. Not for Z (use other-agent)" so neighbouring scopes stop overlapping. Shared rules move out of the agent bodies into one skill that every expert preloads via `skills:`: module layout (.core/.db/.models/.settings/.init/.rest), the `<module>.db` rule and `[:auto/param]` value binding, v60+ migration layout/IDs/backports, module-boundary checks, and a before-handing-back checklist. Each body was rewritten against the current tree: every namespace and path re-verified, stale facts removed (metabot now OSS, workspaces gone, search engine defaults, entity_id semantics, permission coalescing, model persistence swap), and a Return contract added. * De-slop backend expert agents and backend-module-conventions skill Split sentences over 25 words, make passive voice active, state the present instead of narrating history, and use one name per thing. Skill headers are now action titles. Every backticked namespace, path, and setting is unchanged. * Spell out the retire-via-DELETE pattern for shipped changesets Learned the hard way via #83425 / #83549: the "never delete a changeset" rule is right but didn't explain the recovery path, so the first instinct is to just remove the yaml entry. Add the one-sentence recipe: append a same-version-id DELETE-from-DATABASECHANGELOG changeset to the same file. * Clarify what's actually frozen on a shipped changeset Dan pushed back on "immutable": checksums auto-recompute, so cosmetic edits (comments, SQL whitespace, author, rollback block, preconditions) are fine. What's actually locked is the id and the semantic effect — change what the changeset DOES and ran instances diverge from fresh installs. * Prefer the simple rule over the precise one for shipped changesets Dan's correction was accurate but "cosmetic edits are fine" invites rationalization. "Don't edit; add a new one" is 99% correct, needs no mid-crisis branching, and the 1% waste (a true typo fix) costs less than one bad call where someone decides their SQL rewrite is cosmetic. |
||
|
Fraser
|
48b9e66904 | Use oxlint (#82015) | ||
|
Chris Truter
|
e902d2623c |
Name module cycles and fail when they merge (#82386)
The file .clj-kondo/config/modules/cycle-clusters.edn names each cyclic cluster of the module require graph by an anchor module. The cluster holding the anchor carries the name. Membership is computed, never recorded, so a named cluster can grow or shrink without an edit. The test metabase.core.module-cycles-test fails until every cluster holds exactly one anchor. A merge of two named clusters fails and shows the requires joining them. A split asks its author to name the new half, and an anchor that leaves every cycle has to be re-anchored or retired. The name-module-cycle skill suggests space-themed names. |
||
|
Stas Gavrylov
|
5b9d69ca66 | Improve Cypress flaky-test guidance (#82752) | ||
|
bryan
|
34ff3598e3 |
mcp-v2 integration branch (#81245)
* 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 (
|
||
|
Alexander Polyankin
|
38cda45f69 |
Type every :any and [:map-of :keyword ...] property in the API (#82196)
* Make defendpoint reject request schemas that reach an open map, a keyword-keyed map-of or :any Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Close the MBQL 5 query and column schemas and declare the keys the query processor adds Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Type the shared schemas requests reach and drop ms/Map and ms/JSONString Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Close every map schema a request can reach Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Regenerate the modules config Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Keep action parameters string-keyed and type lens params and permission graph ids Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Declare the sandbox coercion keys a column's options can carry Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Declare the alias-info and Mongo keys a column can carry and strip internal keys from legacy column metadata Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Carry the Mongo column keys in the serialize test fixture Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Declare the keys the frontend and query processor put on columns, parameters and card embeds - column metadata: :remapping, a typed :lib/options, the nest-breakouts option key - query parameters accept the rest of a stored filter declaration (sectionId, isMultiSelect, values source config etc.) - static-list values may be one-tuples; raw-value template tags may carry a nil widget-type - card embed attrs declare chart_href, child_target_id and host_data; explore filter refs accept the legacy field_ref the chart column has - notification card echoes a metric's query_description - serialization deep-sort orders mixed keyword/string keys - viz-settings column refs are normalized before matching; a Field hydrated with its table can be fingerprinted; broken segment definitions read back as nil - test fixtures: real fingerprints, snippet tag type, http channel details, comment content, no min_duration_ms on duration strategies Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Wire the defendpoint check, keep every map single-keyed, and apply the review findings - defendpoint now calls closed-schemas/check! on every request schema; the walker no longer lets :any leak through collections, treats map?/:some as :any, skips :=/:enum/:fn values and never skips the default entry - no map mixes keyword and string keys: ms/string-keyed-object gives an opaque JSON object string-keyed declared entries; ProseMirror attrs (documents, comments, explorations embeds), CSV upload parts, search weights and testing query specs use it and their readers use string keys; the serdes deep-sort comparator is back to master - the Metabot context is fully typed from the frontend types instead of an opaque default; a draft transform query without a database is accepted - permission graphs, settings PUT and SCIM PATCH reject a body whose map-of entries the decoder dropped; ad-hoc query endpoints drop a client-sent cache strategy; action parameter values keep collections out without nil-ing them; SCIM meta.resourceType is optional; sandbox user attributes are LoginAttributes; a metric filter clause carries its offset options; explore filter refs are the legacy field_ref the chart column has; result_metadata written by older releases and hasVariableTemplateTagTarget on parameters are declared; a bare map-of on a request path is guarded against scalars Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Close the endpoints tests define and let a column's target and name field be an id reference The check runs on every defendpoint, the ones test namespaces define included. A column's :target and :name-field are hydrated Fields on the backend but id references in the metadata the frontend hands over; the closed column schema nil'd the reference, which lost the search field of a model's primary key. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Hand an action's query only the parameter values, and fix two fixtures the closed schemas caught A query action's stored parameters carry the frontend's widget settings; the query gets the value keys. The Metabot viewing-context tests use ids no card can have, and the Mongo cache test spells the strategy key the schema does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Declare the table name BigQuery's compiled native form carries on a native stage Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Keep a BigQuery RECORD's sub-field sync metadata off its result column, and fix a binning fixture Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> |
||
|
Fraser
|
e92dfab50e | Refine TypeScript authoring and review skills (#82141) | ||
|
Stas Gavrylov
|
ace7aa5dd0 | Add Loki and Figma Desktop workflows to component skill (#81763) | ||
|
Chris Truter
|
30f9f25ba9 |
Refine kondo ratchet commands and prompts (#81776)
* Make ratchet shrinking explicit, and check it in one place The ratchet test fixture ran `fix!` on every non-CI run, so `project-tests ratchets` rewrote .clj-kondo/ratchets.edn in your working tree before asserting against it. A check that edits what it checks leaves a diff nobody asked for, in the file every PR conflicts over, and its only reachable failure was a bug in the fixer that had just run. Removing the auto-fix forces the dev/CI split to go too: a strict local test that no longer tightens would fail on every branch that removes an ignore. So over-budget is now the rule everywhere, and shrinking is only `./bin/mage kondo-ratchets-shrink`, which the shrink workflow runs on master. That left two implementations of nearly the same check. The babashka `check-report` gains the justification finding — the one assertion the JVM suite had that it lacked — so `./bin/mage kondo-ratchets` covers over-budget, config over-budget, unjustified ignores, normalization and unknown linters on its own. CI runs it instead of booting a JVM, and the five now-duplicated in-repo assertions leave the test namespace, which drops from 6.2s to 0.8s because it no longer walks the source tree. Task names follow: check-kondo-ratchets (which nothing called) becomes kondo-ratchets, fix-kondo-ratchets becomes kondo-ratchets-shrink, and the merge task becomes private -kondo-ratchets-merge, since only the git merge driver invokes it. bin/merge-kondo-ratchets keeps its name; installed merge drivers point at it. * Polish kondo ratchet guidance * Prime the ratchet shrink workflow Intentionally raise the :case-symbol-test budget from 1 to 2 without adding a suppression. The read-only check allows the unused budget; after this PR merges, the shrink workflow should restore it to 1 through a Tighten ratchets automation PR. * Make the ratchet suite check the project Keep the ratchet tooling unit tests in the backend suite, but make project-tests ratchets run the read-only repository check. Include it in the bare project-tests command so local project-level checks still validate the checked-in ratchet policy. * List project-test suites once * Address review findings on the ratchet check check-report threw NPE on a ratchets map without :comment-exempt, while tolerating a missing :ignore-counts or :config-counts. Default it, so the three policy keys behave alike. The run-project-ratchet-checks description and the step comment both still described the old wiring. The check now runs alongside the backend checks, which no longer include it, and "no JVM" is not quite true: resolving clj-kondo on a cold cache shells out to the JDK prepare-backend installs. * Warn about stale comment exemptions The only check that a :comment-exempt entry is still needed was a local-only test, which went vacuous when the dev and CI semantics were unified, so nothing named a stale exemption any more. The check now reports one the way it reports an :unlimited policy nobody uses: informational, never a failure, since only the shrink workflow removes them. * Say why the merge driver finds mage through its own path The script runs against whatever repository holds the conflict, which is not always the one it lives in -- the tests merge stages in a throwaway repo. A relative bin/mage looks plausible and fails there, so record the reason. * Stop wrapping two comments that did not need it One fits on a single line once it says less. The other breaks at its sentence boundary rather than mid-clause. * Break the ratchets header on its sentences Four sentences started mid-line and wrapped mid-clause, one of them trailing "in the PR." onto a line of its own. Same words, one sentence per line. * Keep comment exemptions until someone removes them fix! dropped a :comment-exempt entry as soon as every ignore of its linter carried a comment, or the last uncommented one was deleted -- so an exemption could disappear as a side effect of unrelated work, and getting it back is a change to defend in review. An exemption records a decision about a linter, not a count, which is the reason :unlimited policies are already kept at zero and only warned about. Treat the two alike: the check and the shrinker name a stale exemption, and a person deletes it. |
||
|
Stas Gavrylov
|
5885d30ad0 |
GDGT-2486 Add elevation, radius, and spacing scale variables (#80481)
* GDGT-2486 Add elevation, radius, and spacing scale variables
* GDGT-2486 Remap scale tokens in files merged from master
* Address review
Review fixes for the GDGT-2486 scale migration:
- Revert double-shifted tokens from the merge-remap pass: it re-applied
the old-to-new mapping to 22 files already remapped by the first pass,
shifting spacing values one extra step (e.g. xl 24px -> xxl 32px).
- Fix codemod escapes where old key names kept their name but changed px:
Modal default padding (lg->xl, 24px), MultiSelect radius (sm->xs, 6px),
MetricSearchInput radius-sm, ThemeEditor PreviewPanel radius-md,
ColumnInput theme.spacing.xs->xxs, ChartTypeSettings Space h props,
TableRowForCacheableItem icon placeholder w (md->lg, 16px),
ResizeHandle/ResizableBoxHandle grip h (xs->xxs, 4px).
- Pin Group/Stack default gap to "lg" (16px): Mantine's default is
"md", which is 12px in the new scale (was 16px in the old scale).
- Allowlist the runtime-defined --mantine-spacing-xxs/-xxl and
--mantine-radius-xxs CSS variables in find-never-defined-css-variables
(fixes fe-lint CI).
- Add theme.unit.spec.ts covering the spacing/radius/shadow scales,
dark-mode shadow set, whitelabel independence, and a guard asserting
every scale-based component default resolves to a defined key.
- Derive Card/Paper story argTypes options from the real scale.
- Augment MantineThemeSizesOverride so the custom scale tokens appear in
editor autocomplete for p/m/gap/radius/shadow props.
- Update the emotion-migrate and metabase-ui-component-from-figma skill
token tables to the new scale (spacing/radius values, shadow tokens,
snap tables).
* Update Loki Snapshots
* Address review: fix h/w style props missed by the scale codemod
The codemod did not remap height/width style props, so every bare h=/w=
scale-key value silently changed px with the new scale. Remap each to the
px-preserving key (xs 4px->xxs, md 16px->lg, lg 24px->xl, xl 32px->xxl):
- PaneHeader breadcrumb row h xl->xxl (32px, restores data studio
pane header height)
- BrowseDatabases logo boxes h/w xl->xxl (32px)
- DatabaseConnectionStringField rows h lg->xl (24px)
- DatabaseEngineList logo Center h/w lg->xl (24px)
- Metabot chat chevron flexes h/w md->lg (16px)
- MetricsFilterPill h lg->xl (24px)
- DatasetNotebook divider h xs->xxs (4px)
- DataReference NodeList icon w md->lg (16px)
- transforms-python ResizableBoxHandle grip h xs->xxs (4px)
Audited: no remaining bare h/w/maw/mah/miw/mih old-key props whose px
changed (verified line-identical against master for every occurrence).
* Address review: fix bdrs and responsive-object size tokens missed by the codemod
Two more escape classes from the scale codemod, found by a full sweep of
every touched-token form (string style props, responsive objects, CSS
vars, theme accessors) with each line compared against master:
- bdrs style prop (border-radius): remapped px-preservingly at 69 sites
in 58 files (xs 4px->xxs, sm 6px->xs, md 8px->sm). The codemod remapped
radius= but not its style-prop alias bdrs.
- Responsive object values (e.g. p={{ base: "md", sm: "xl" }}): remapped
the token VALUES at 9 sites in 6 files (base/sm/md breakpoint keys are
untouched; only their spacing values shifted: md 16->lg, xl 32->xxl,
xs 4->xxs).
Sweep result after fixes: zero remaining line-identical old-key token
references to spacing xs/md/lg/xl, radius xs/sm/md, or shadow xs/md in
any form. Intentional survivors unchanged: radius/shadow/bdrs "lg"
(disclosed broken-key-now-16px class), spacing/radius sm where the
px value did not change.
Verified: tsc, eslint (changed files), metabase/ui jest suite.
* Address review: fix remaining token escape forms (object props, braces props, style objects)
Final codemod escape classes from the exhaustive token sweep, all
remapped px-preservingly:
- Object-literal / default-parameter prop values (15 sites / 8 files):
ItemList picker-item default containerProps pb xs->xxs (4px, the
reported gap), FormFooter pt lg->xl, SlackSetup pt lg->xl,
EmbeddingSecretKeyWidget mb xs->xxs, CreateDashboardForm labelProps
mb xs->xxs x2, databases styles mb lg->xl, NotebookDataPicker
menuDropdownProps mt xl->xxl, Banner story py md->lg
- Braces-wrapped string props: CloudSMTPConnectionCard p lg->xl (24px)
and gap md->lg (16px), SettingsLdapForm mt xs->xxs,
DashboardLeaveConfirmationModal mb lg->xl
- style-object CSS properties: DatasetNotebook borderRadius xs->xxs
- Ternary token values inside responsive objects: SummarizeStep gap
md->lg (16px)
Sweep now returns zero real misses across every checked form: string
style props (incl. h/w/bdrs), braces props, responsive objects with
ternaries, object-literal/default-param props, style-object CSS props,
CSS var() references, theme.* accessors. Remaining grep hits are
untouched scales (lh, fz, size, breakpoints) or px-identical sm values.
* Address review: preserve Mantine spacing defaults exposed by Loki
* Update Loki Snapshots
* Address review: fix remaining scale regressions and visual guards
* Address review: make Loki update capture new references
* Update Loki Snapshots
* Address review: keep only targeted Loki references
* Address review: assign ticket component elevations
* Update Loki Snapshots
* Address review: tighten MonthPicker header spacing
* Address review: refine elevation visual stories
* Update Loki Snapshots
* Address review: keep renamed Menu Loki story selected
* Update Loki Snapshots
* Address review: preserve inherited Mantine defaults
* Address review: allow stock Mantine shadow tokens
* Address review: replace token audit test with ESLint rule
* Address review: drop unrelated whitespace-only dev/src changes
* Address review: use rem() in radius scale and rem docs
* Update Loki Snapshots
* GDGT-2486: Address PR comments
* GDGT-2486: Adjust negative tokens
* GDGT-2486: Address review findings
* Update Loki Snapshots
---------
Co-authored-by: Metabase Automation <github-automation@metabase.com>
|
||
|
Chris Truter
|
7f73a7464c |
Add merge tool for ratchet conflicts (#81341)
* Add a manual kondo ratchet conflict merger * Split kondo ratchet merge tests * Document the ratchet conflict merger * Simplify unlimited ratchet merge policy * Simplify ratchet merger for final policy schema * Resolve ratchet conflicts to the stricter policy Concurrent policy changes now always resolve: the smaller budget wins, a finite budget beats :unlimited, and a removed entry beats any budget. Comment exemptions merge on their own. Every stage is read through read-ratchets and shape-checked before a disabled stage decides the result. bin/merge-kondo-ratchets reads the index stages by number, so a file deleted on one side and changed on the other exits nonzero with nothing staged, and both-sides-add merges against an empty base. It runs from any directory in the repository and stages only .clj-kondo/ratchets.edn. mage.merge-kondo-ratchets-test drives the script against real conflicts in temporary repositories. * Point the clojure-write skill at the ratchet merge resolver * Reject malformed stages before merging ratchets The shape checks move out of read-ratchets into validate-policies, and merge-ratchets runs them on every stage, so a stage with :config-counts [] or :comment-exempt [] is an error rather than an empty set of policies that the merge would read as removals. * Cover unreadable stages in the merger tests An empty file, two forms, or a non-map stage now fails the reader instead of merging as no policies; the script leaves such a conflict for a human and stages nothing. * Reject a non-map ratchet stage before merging A nil stage passed every field check because dissoc, keys, and the defaults all accept nil, so a malformed stage could merge as an empty set of policies. * Reject non-keyword linter names The merge keys on (str linter) and renders the same way, so a string, nil or symbol name was rewritten as a keyword, collided with one, or produced a file that no longer read back -- and the script still exited 0 saying it had staged the merge. Also describe the two sides by git stage: "current" and "incoming" are inverted during a rebase. * Run each refused merge once in the tests leaves-unresolved? and output each ran the script, so every refusal ran it twice and the "changed nothing" assertion covered a different run from the one whose message was checked. Read the merged policies with the EDN reader while here. * Compare the conflicted file, not just its status A conflicted path reports UU, DU or UD however its contents are rewritten, so comparing git status alone did not show that a refused run left the file alone. * Tighten merge-tool docstrings and the ratchet conflict docs * Compare the index stages of a refused merge too Porcelain status reports a conflicted path as DU/UU whatever its stages hold, so it alone does not show that a refused run left the conflict alone. The stages carry each side's blob. * Spell out the unmerged status codes UU/DU/UD as bare shorthand assumes the reader knows git's porcelain codes for a conflicted path. * Clarify the unmerged-state snapshot comment |
||
|
Chris Truter
|
3c5d1111d7 | Run project checks in a separate job (#81318) | ||
|
Andrei Chernykh
|
94f0aec2e9 |
Replace @cypress/skip-test with a local helper (#80947)
* Replace @cypress/skip-test with a local e2e helper * Document --spec requirement in the e2e-test skill * Tidy skip-test helper comments and skill doc wording |
||
|
Andrei Chernykh
|
68746a6d43 |
Clean up leftover JS-era config, deps, and docs after the TypeScript migration (#80032)
* Clean up leftover JS-era config, deps, and docs after the TypeScript migration * Address review: keep .jsx in resolve.extensions, drop .js from loader tests, includes-based e2e validator filter * Keep @babel/node for a separate deps cleanup round * Keep js/jsx in rspack loader tests |
||
|
Fraser
|
522b0458e0 | Create FE Modularization skill (#80132) | ||
|
Kevin Kelleher
|
789d334847 |
Explorations (#73244)
* Fixed failing CI * Add query timestamp, refactor error marker * no-mbql-introspection pass * Order temporal-axis exploration variants by date, not aggregation When the row cap fires on a time series, aggregation-desc ordering keeps the highest-count buckets scattered across the full range: gaps in the x-axis, old spikes beating recent data. Temporal axes now order by the temporal breakout desc (aggregation desc as tiebreak) before the limit, so the most recent contiguous window survives. - time-facet: order by the temporal breakout (second breakout) desc - default: branch on dim type — temporal dims by recency, categorical dims keep aggregation desc - per-value-time-series: gains the same date-desc cap (had none, so a single fine-grained series could blow query-caching-max-kb) - all three also filter the NULL date bucket: it can't render on a time axis, and its position under ORDER BY DESC is warehouse-dependent (Postgres NULLs first, MySQL last), making cap survival non-deterministic otherwise - temporal-pattern-day/hour unchanged (bounded, no cap interaction) * Added debug info to interestingness calculations * even less mbql introspection * oops, delete .scratch * Sidebar UI fixes - error marker, loading shimmer * Interestingness >= 0.7 * Adjust prompts list animation * Fix e2e * Note that the LLM query planner is a POC, not used in production * Fix searching for explorations * Use llm-metabot-provider for everything * Log interestingless calculation as info for now * Log interestingless calculation back as debug * Fix FE lint * fix ai summary showing dirty while generating * Use llm-metabot-provider * collection breadcrumbs * add back null values for temporal variants * order by for temporal pattern variants * fix top-n-other * doh, fix temporal pattern queries * fix name in viz header, don't show be error in tooltip * sort numeric bins in heat map header, fix tests * UXW-4483 Add matching colors for category + breakout charts (#75912) * Fix CI * Fix units, fix loki * UXW-4522 Update last query run timestamp in background * Fix loki * Fix e2e * Improve Explorations viz types (#75958) * UXW-4483 Add matching colors for category + breakout charts * improve viz types first pass * add error boundary * add row chart support for dimension_value_colors * Fix typecheck * fix tests * better handling of segments with row fallback, fix null color bug --------- Co-authored-by: Denis Berezin <denis.berezin@metabase.com> * UXW-4402: Make explorations respect sandboxing/impersonation/db routing (#75572) * Make explorations respect sandboxing/impersonation/db routing * some self-review fixes * more cleanup * test fixes * doh * Fixing CI failures * capitalize 'By' consistently * Fixed CI failure * add loki snapshots * Review fix * Fixes after merge * Rework Explorations UX (#76300) * comment on exploration viz first pass * wip * wip * wip * UX changes first pass finished * code cleanup, fix, add tests * address review comments * Code cleanup --------- Co-authored-by: Nathan Voxland <nathan@metabase.com> * Refine explorations with metabot (#75326) * teach metabot about new exploration ux * refine explorations * address self-cr * pass over FE * CR response * fix test * Add :model/Exploration to api + eid-translation model-imports These modules reference :model/Exploration after merging the latest explorations base; declare the imports to satisfy model boundaries. --------- Co-authored-by: Kevin Kelleher <kevin.kelleher@metabase.com> * Harden explorations runner: - planner crash recovery - Bound timeline scoring - add metrics * UX tweaks * Additional logging of errors on ai summary, plus increased thinking budget * Improved timeline interestingness calculation * Handle sonnet 5 etc better * Make explorations groups a real thing (#76643) * Rename ExplorationThreadGroup model to ExplorationBlock Explorations organize their queries in a hierarchy: an Exploration has Threads (one per run), a Thread has Blocks, and a Block is one metrics-by-dimensions grouping that renders as a single sidebar heading. That persisted grouping entity was named ExplorationThreadGroup, and the word "group" was overloaded across the backend and frontend. Rename the entity to Block — the name the frontend already uses — so the larger data-model changes that follow start from one clear name. No behavior change. Renames :model/ExplorationThreadGroup -> :model/ExplorationBlock and table exploration_thread_group -> exploration_block. - Append a renameTable changeset rather than editing the createTable in place: the original is already applied to running dev DBs, so an in-place edit would fail checksum validation and force a DB rebuild. - Rename the model namespace/file; update all call sites, the require alias (:as block), and insert-blocks!. - Update model registrations (resolution, serdes models, copy, entity-id test) and re-sort the entries. - Refresh the model's docstrings to describe a "block". The planner's :thread-groups ctx key and the read-side group-tree are left untouched here; they belong to later read-side work. * Persist exploration pages with stable ids reconciled across reruns Within a Block, an Exploration's queries are organized into Pages: a Page is the bundle of queries for one (metric, dimension, query_type). Until now a Page existed only as a derived, content-addressed key computed at read time, so anything that wanted to point at a Page had to reference a string that changed whenever the grouping definition changed. This makes a Page a real persisted row whose surrogate id is its identity, so comments (and stars, coming next) can anchor to a Page and survive both a rerun — where queries are deleted and regenerated — and a future re-grouping. Introduce ExplorationPage: the persisted (block, card, dimension, query_type) page within a block. - New exploration_page table + model, FK to exploration_block (cascade) and report_card. Registered in resolution, serdes (excluded), copy, and the entity-id test. - Add exploration_query.page_id (NOT NULL, FK, cascade): the reconciler stamps it on every query, so it's required from the start. group_id stays for now as the block pointer the read-side and deep-link URLs use; dropping it waits for the read-side rework. - Reconcile in insert-plan-rows!, the single insert point for both initial Start and rerun: find-or-create a page per distinct key (stamping page_id), then GC pages left with no queries. reset-thread- for-rerun! is unchanged — pages survive a query delete (cascade is page->query), so find-or-create recovers the same page ids on rerun. - No DB unique index on the key: dimension_id is text (not portably indexable on MySQL) and planning is single-threaded, so uniqueness is enforced by find-or-create and matching is done in Clojure. - Migration changesets appended (the table/group_id changesets are already applied to the running dev db). Tests cover page-id stamping, rerun id-stability, orphan GC, and perms. * Return nested blocks→pages from the read API; drop query.group_id An Exploration Thread's structure is Blocks, each containing its Pages, each Page bundling the ids of its queries. Until now the backend instead returned a hand-rolled flat list of typed nodes (sidebar/singleton/page) linked by parent_group_id and keyed by auto:<...> strings, which the frontend then re-nested. Now that Blocks and Pages are real rows, return the nested shape the tables already encode: GET /:id returns :blocks, each with its :pages, built from ExplorationBlock + ExplorationPage rows and queries grouped by page_id. - New metabase.explorations.blocks (renamed from .groups): blocks-tree builds the nested shape; block headings + page names + score-sorting ported over; block-display-name made public for the LLM planner ctx. - Deep links go to a page by id: page-url → /question/research/<exp>/ page/<page-id>; the AI-summary and document-append builders pass the primary query's page_id. - Drop exploration_query.group_id (and its FK/index); a query reaches its block through its page now — build-row-context (used by the runner to finalize each row's MBQL) resolves the block via the page, which is what the group_id read there used to do. - API schemas: ExplorationQueryGroup → ExplorationBlockNode + ExplorationPageNode. - Migration changesets appended (the table is live on the dev db). - Rewrite the read-model + endpoint tests for the nested shape; every ExplorationQuery test fixture now hangs off a page (page_id NOT NULL). The create payload key stays :groups for now (separate input contract, touches the data-picker flow). FE consumption of :blocks lands next. * Consume nested blocks→pages on the frontend The frontend renders an Exploration Thread as sidebar headings — one per Block — with a navigable item per Page (a Page being the bundle of queries for one metric/dimension/variant). It used to receive a flat typed-node `groups` list and re-nest it itself; this switches it to the nested `blocks` array (block → pages) the read API now returns, and renames the navigable unit from "group" to "page" throughout. Users see the same sidebar and charts — this lines the frontend up with the backend's shape. - exploration.ts: add ExplorationBlockNode / ExplorationPageNode; thread `.groups` → `.blocks`; remove ExplorationQueryGroup / its display-type; the navigable id is now a page node id (kept as an opaque string). - Sidebar utils build the tree directly from blocks→pages (one heading per block, one child per page); score-sorting + empty-block filtering preserved. - The navigable unit is renamed group→page end to end: the route segment becomes /page/<id>, matching the backend's AI-summary deep links. The selection/url/comment-draft id stays a string via String(page.id), so the existing string-keyed plumbing is unchanged. - Visualization toolbar/header/comment-draft props renamed group→page; the mark-as-interesting handler is left a stub for the stars feature. - Specs, stories, and test-utils rewritten to the nested shape. The create-payload request key stays `groups` (separate input contract). * Retain comment-anchored pages across rerun GC; point comment URLs at /page/ A Page bundles the queries for one (metric, dimension, query_type), and a comment can be anchored to a Page — comment.child_target_id holds the page id. On a rerun an Exploration's queries are deleted and regenerated, and any Page left with no queries is garbage-collected. A Page a comment points at must survive that cleanup even when it ends up empty, or the comment would resolve to nothing. This makes the rerun GC comment-aware and moves comment deep-links onto the page route. - gc-orphan-pages! now retains an orphan page when some "exploration"- targeted comment's child_target_id equals the page id (compared as strings). Empty + no comment is still GC'd as before. Page ids are globally-unique PKs, so matching on child_target_id alone is unambiguous. - exploration-comment-url: /group/<child> → /page/<child>, matching the page route and blocks/page-url; stale "group ID" docstring fixed. - explorations imports :model/Comment for the retention lookup (comments doesn't depend on explorations, so no dependency cycle). - No backfill: the dev Postgres has zero exploration comments and zero auto:* child_target_id rows, so the optional migration is skipped. - Test: comment on a page → drop that page's selection → rerun → the page is retained and the comment still resolves; a no-comment empty page in the same rerun is still GC'd. * Generate short and long page names from metric, dimension, and variant A Page — the bundle of queries for one (metric, dimension, query_type) — is shown in the sidebar under its Block's heading, but is also referenced on its own elsewhere (comment deep-links, AI-summary chart titles, search). Page names are derived, not stored, so a Page needs both a short label that omits whatever its Block heading already shows (e.g. under a "Number of Orders" heading: "By Category over time") and a full, self-describing label for context-free places ("Number of Orders by Category over time"). This generates both from the Page's parts — the metric name, the dimension name, and a short per-variant qualifier — rather than by string-slicing a finished name. - variants/variant-qualifier: query_type → a short, param-free localized qualifier ("over time", "(Day of week)", ...) or nil for the default breakdown. Lives next to query-name so all variant naming is in one place. Unit-tested. - blocks: page-long-name and page-short-name generate from parts (metric + dimension + qualifier) using tru templates and the block's anchor; page-node emits :name and :long_name. - API ::ExplorationPageNode and the FE ExplorationPageNode type gain :long_name (FE display wiring left for separate work). - Same-dimension pages that differ only by variant (default vs top-n-other) are now distinctly named instead of colliding; a blocks-tree test that asserted the collision now asserts they differ. * Add ability to star pages, sidebar tabs UX (#76755) * add ability to star, sidebar tabs * - renamed "user interestingness" to "starred" - Let starred just be true|false, no null - Removed DELETE starred API, just PUT now --------- Co-authored-by: Nathan Voxland <nathan@metabase.com> * Fix CI: test compile error, migration boolean type, e2e formatting - documents/search_test.clj: qualify text->prose-mirror-ast with the documents.test-util/ alias in the exploration-thread exclusion test. The unresolved symbol broke backend test compilation, which cascaded to every app-db/driver test job and clj-kondo ("0 tests" everywhere). - 20260428_explorations.yaml: use ${boolean.type} instead of bare boolean for exploration_page.starred (required post-v49 so MySQL uses BIT(1)). - oxfmt formatting for two explorations frontend files (fe-lint). * Update e2e specs for reworked comment URL + explorations sidebar - comment helper: match the node comment link by path only. The `?new=true` marker was removed from comment URLs (base commit "respect query params in all urls"); the CommentsMenu unit test was updated but this e2e helper was missed. Verified the comment flow still works (opening a node's thread shows the new-comment editor). - explorations spec: align with the reworked sidebar — * `by <dim>` leaf label is now `By <dim>` * Scratchpad doc test: only Scratchpad is created synchronously; the AI Summary doc is created asynchronously, so stop asserting it (and the removed "Findings" grouping) at creation time. * AI-summary completion test: the AI Summary doc row now hangs under the thread heading ("Initial investigation"), not a "Findings" group. * remove the two "Add to document" tests — that control was removed. * Fix stylelint rule-empty-line-before in explorations CSS modules Add blank line before nested &:hover rules in ActionToolbar.module.css and ExplorationGroupVisualization.module.css. These surfaced in fe-lint's lint-css step once the oxfmt formatting failure was resolved. * - cleaned up migration file - Improve group renaming in code - preserve starts between re-runs like comments * Fixing CI failures --------- Co-authored-by: Kevin Kelleher <kevin.kelleher@metabase.com> Co-authored-by: Nathan Voxland <nathan@metabase.com> * Don't send empty messages to the LLM * address review comments from 76755 * UXW-4765 Hide charts (#77488) * UXW-4765 Hide charts * Review fixes * Fix the failing explorations unit tests and Loki stories (#77564) * Fix the failing explorations unit tests and Loki stories at the branch tip * Restore the original all-hidden copy and drop thread headings from it * Delete the outdated ExplorationGroupVisualization stories and their Loki references * Rephrase the hide toast to put the chart name first * UXW-4764 Remove interestingness badges on results (#77544) * UXW-4764 Remove interestingness badges on results * Drop e2e assertion for the removed sidebar interestingness markers * Colocate the interesting and error markers with their only consumers * Remove the interestingness marker from the timeline picker too * fix types, lints, formatting * Uxw 4779 update tab menu to match designs (#77543) * UXW-4779 Update sidebar tab menu to match designs * Show a pointer cursor across the whole SegmentedControl * Give the icon tabs img semantics and hide the decorative icon names * Let getExplorationSidebarTabsInfo own the tab icon config * Revert the SegmentedControl root pointer cursor, Mantine already puts it on the labels * Regenerate modules config after master merge The merge left 'explorations' and master's new 'entity-retrieval' out of sorted order, which metabase.core.modules-test asserts on. * Float the action toolbar pinned to the bottom of the chart canvas (#77667) * fix types * revert timelines in documents * Review fixes: stop requiring the QP's internal cache.impl from other modules Explorations code that saves and loads query-result snapshots was requiring metabase.query-processor.middleware.cache.impl directly — an internal namespace of the query-processor module that other modules aren't supposed to reach into. Since the module's :api set is generated from actual usage, those requires had put cache.impl on the module's public surface — something master never exposed. Modules talk to each other through a small public facade (metabase.query-processor.core) instead. The two helpers those callers need — do-with-serialization (write a QP result as a gzipped blob) and with-reducible-deserialized-results (read one back) — are now re-exported from the qp.core facade, and the explorations callers go through it. One consumer can't: queries/cached_result.clj is itself loaded while qp.core is loading, so requiring the facade from there would be a circular-load error at boot. It gets a proper home instead — a new small, documented public namespace, metabase.query-processor.result-serialization, re-exporting the same two helpers. With nothing outside the module requiring cache.impl anymore, the regenerated :api drops it, matching master. * Review fixes: explorations planner and AI-summary polish - Nested cardEmbeds: the AI-summary postprocessor wrapped only top-level cardEmbed nodes in resizeNode containers, while the card materializer walks the whole document — so a cardEmbed the LLM emitted inside e.g. a blockquote got a real card but rendered collapsed to zero height. The wrapper now walks the tree too (skipping already-wrapped nodes). - Awareness-tier sort validation: phase 2 validates that embedded categorical charts carry an explicit sort instruction, but the validation set was built only from top-tier breakouts — an awareness-tier categorical chart escaped validation entirely, and its prompt block (slim-block) never told the model a sort was required. Both tiers are validated now and slim-block emits the same per-chart sort directive as full-block. - Contextual-scoring gate order: safe-score+describe resolved its scoring context (a Card select and a SQL compile) before checking whether the row is scorable at all (chart config present, creator known, non-blank thread prompt) — wasted DB reads and a query compile for every unscorable row. The cheap gates run first now. Its docstring also claimed the LLM gate throws and is swallowed; it actually returns nil from a pre-flight cond. - Discovery-cache TTL: the top-K category discovery cache was a process-lifetime LRU, so a stale top-K result could be served forever on a long-lived instance; entries now also expire after 15 minutes. - Chart-name i18n: filtered-subset chart names built their multi-value summary as (str (count values) " values") — a hardcoded, unpluralized English fragment inside an otherwise-translated name. It uses trun now. - Docstring/comment corrections: effective-cardinality documented (and branched on) a :num-bins bucket that no caller can produce — branch and docs removed; run-one-plan-iteration! said it runs "the LLM planner" when the default is the mechanical one; a variants-test section comment claimed temporal variants add a not-null filter that no variant adds. Also repoints this module's result-blob serialization calls at the qp.core facade (the consumer half of the cache.impl cleanup in this stack's first commit). * Review fixes: explorations API permissions, documents, and search - Creating an exploration accepted arbitrary card ids (in blocks) and timeline ids without checking the caller could read them, so you could attach someone else's private metric or timeline and see its name/events later. Both are read-checked at create time now, and timeline hydration filters out timelines the viewer can't read. A duplicated timeline id used to hit a unique constraint and 500 the request; ids are deduped. - The restart endpoint re-selected the Exploration row after resetting the thread, but the reset never touches that row — the redundant select is dropped. - GET /api/document listed documents that belong to exploration threads, which search, recents, and collection items all deliberately hide. It applies the same filter now — which as a side effect removes an N+1 pattern where each listed doc could trigger several permission queries. - The document validator rejected {:type "doc" :content []} — the exact empty document the backend itself creates for new threads. An empty root doc is valid now. - Search: is_published was deliberately hidden from search responses on master (#66841) and this branch accidentally re-exposed it — hidden again in both search engines. Exploration descriptions are searchable in the legacy engine too, matching the new engine. - Data picker: metrics and dimension groups now use the same interestingness filter (a metric could previously reference dimension ids that appeared in no group), search matches the displayed "<group> - <dimension>" text, and a collection-ids query that ran twice per request runs once. - Consistency: the create event publishes after the transaction commits (observers could previously fire before the data was visible). * Review fixes: metabot / LLM infrastructure - Asking Claude for extended thinking while also forcing a tool call (tool_choice "required") produced a request Anthropic rejects with a 400. The guard that already handled this for structured output now covers the general tools path too. - The e2e guard that stops tests from hitting a real LLM provider threw a raw MalformedURLException on unparseable URLs; it now fails closed with its normal friendly message — and it was moved out of a catch-all that would have rerouted it, so it fails loudly. - The timeline tools handed the LLM broken XML (an attribute quote and a tag never closed) and interpolated user-written names/descriptions unescaped, letting content break the markup. Output is well-formed and escaped. - Hardening/docs: the retryable-error check bounds its exception cause-chain walk at 10 (a cyclic getCause could loop forever); the contextual-interestingness response schema now really forbids extra properties (its docstring claimed it already did); docstrings about thinking model buckets, return shapes, and inputs corrected. - Also repairs three test breaks that already existed at HEAD: an arity mismatch from the OpenRouter merge, a test asserting thinking blocks are dropped (they now stream as reasoning parts), and tests that didn't stub the LLM gate's newer usage/permission checks. * Review fixes: lib-metric, metrics transforms, startup sync, interestingness - group-by-source was re-exported for JVM Clojure but missing from the ClojureScript branch of lib-metric.core; added. - Metric/measure display-info inherited a default :long-display-name even where the method overrode :display-name, so the two could disagree; they're set together now. - A failure in the startup metric-dimension sync aborted server boot entirely; it's wrapped to log an error and continue. - The interestingness "leftovers" backfill loaded every unscored field for the database into memory in one select; it now streams one field at a time, and the namespace docstring mentions this second recompute trigger. - Added a unit test covering the dimensions :out transform (kebab-key renames + type-value keywordization). * Review fixes: enterprise perms/sync/deps, cached card query, test quality - Remote sync: an event payload that simply omitted a key used to satisfy a nil condition (e.g. "exploration_thread_id must be nil") — conditions now require the key to be present with the right value, checked with (find object k), which distinguishes a present-but-nil entry from an absent key. Vector values in removal conditions rendered as broken equality clauses; they render as IN. - Dependency graph: smartLink nodes with nil/placeholder ids added nil to the dependency set (the sibling cardEmbed branch already guarded this); guarded, with regression tests for both branches. - Test quality: the revisions exclusion test asserted the impl against its own private set, so it could never fail; it checks a hardcoded expected list now. Exploration bookmarks get real API coverage (create/list/delete/ordering/archived), and a mislabeled test string was fixed. * Review fixes: v4 instance-analytics views with explorations usage sources The AI-usage analytics views translate machine source names ("exploration", "contextual_interestingness", …) into human-readable labels with a SQL CASE. The explorations stack added its new sources to the v1 SQL files — but the views actually installed on instances are v3, and nothing reinstalled them, so users saw the raw machine names fall through the ELSE branch. Adds v4 of ai_usage_log and metabot_conversations (v3 plus the new CASE branches, with one consistent "Explorations" label — v1 said "Exploration" for the source but "Explorations" for the profile), a migration that reinstalls the views at v4, an updated pointer comment in usage.clj, and a parity test that fails whenever a known source/profile is missing a CASE branch in the latest views — exactly the drift that happened here. * Review fixes: frontend (minimal) and e2e Only the low-risk frontend fixes from the review pass, plus the e2e test fixes; the other frontend findings are deliberately left out to keep this branch's FE diff small. - getDimensionIcon existed byte-for-byte in two files; the duplicate module is deleted and its one importer now uses the copy in common/utils/columns.ts. - e2e: an assertion that searched for text inside another text node (and so always passed) is meaningful now; readSelectedText no longer breaks Cypress's retry behavior by asserting after a .then; and the seemingly pointless GET /api/metric/:id in seedMetrics got a comment explaining the side effect it exists for (it persists metric dimensions). * Remove the LLM query planner Explorations plans queries with the mechanical planner only now. Removed: the LLM planner namespace, its prompt templates (plan.selmer / repair.selmer) and prompt loader, the prompt-markdown renderers in the planner-context namespace, the explorations-query-planner setting (mechanical/llm/auto), and the :skip-no-llm exit for "planner requires an LLM but none is configured". pick-planner! now always returns the mechanical planner — kept as a seam so tests can stub it and a future planner has an obvious dispatch point. The planner-contract protocol and the shared context builder stay (they are planner-agnostic), and the query_plan_transcript column stays (every planner writes a transcript); its remark no longer describes an LLM exchange. * remove graph._dimension_value_colors (per bar colors) * Add row count to explorations API (#77740) Already available, we just need to persist it and thread it through the API. * Format fix * Exploration UX... um explorations. (#76901) * click thru * better comment links * chart avatars * avatars * allow subsubexplorations * disable explore further for Other group in top n charts and dimension anchored blocks * fix explore further for binned numeric columns * comment highlighting, remove avatars * comment label formatting * map and table highlighting * use built in viz click actions popover instead of custom * fix comment badge in dark mode * self review cleanup * fix and simplify new thread toast * fix be lints, add tests * fix tests * add fe, e2e tests * - POST /:id/explore-further now verifies the clicked page belongs to this exploration before copying its block, closing a hole where any page in the instance could be copied into any writable exploration. - key on the stable :explore-filters spec instead of a hash of the per-row-reconstructed :dataset_query (which minted fresh :lib/uuids and so never hit). build-row-context exposes :explore-filters on the row context for the cache to key on. Also reuse metabase.util/capitalize-first-char in the block title and thread name helpers instead of hand-rolled str/upper-case. * Code cleanup * Gate exploration derived metadata by viewer's permissions * fix lints * Code review improvements * Den/uxw 4741 explorations new sidebar (#77674) * Update tabs view, add filter button menu * Fix tests * Add sidebar items vertical lines + icons * Other sidebar changes - viewed state, sticky hierarchy * Show sub-explorations near it's parent item * Fix issues * Self review * Fixes after merge * Self review * Review fixes * Fix empty states * Review fixes --------- Co-authored-by: John Swanson <john.swanson@metabase.com> Co-authored-by: Kevin Kelleher <kevin.kelleher@metabase.com> Co-authored-by: Nathan Voxland <nathan@metabase.com> Co-authored-by: Denis Berezin <denis.berezin@metabase.com> * Explorations self-review cleanup (#77717) * Consolidate the explorations migration in place The 063 explorations migration had grown by accretion: tables were created and then altered by later changesets in the same unreleased file. Since the branch convention is to edit unmerged changesets in place, the final shapes are folded into the createTables: - exploration_query gains params, nullable dataset_query/entity_id, and page_id inline (the page_id FK and index stay separate — exploration_page is created later in the file, so they can't run earlier). - stored_result gains data_access_token inline; its backfill changeset was deleted outright — created and backfilled in the same release, it could only ever run against an empty table. - exploration_query_result gains chart_stats / metric_description / chart_description inline, which also retires the randomly-named changeset id v63.gr324asd. - idx_eqti_query_id deleted: the unique constraint on the same leading column already provides that index. The completed_at and analysis_started_at backfills are deleted outright: the explorations feature merges to master in one piece, so at upgrade time no instance can have pre-existing exploration threads to backfill. (They were also dangerous — they stamped columns the cancel endpoint and the completion handler gate on being NULL, permanently locking out any thread in flight during the migration.) Cosmetics: backfilled default docs are named "Scratchpad" to match runtime (the backfill said "Findings"); stale remarks corrected (chart_stats is EDN not JSON, the real query_type variant list, what collection_id actually defaults to). Existing dev databases will need a migration reset. * Set exploration thread started_at in the INSERT The thread was inserted without started_at and then immediately updated to set it, on the theory that the background worker must not see the thread before its dependent document/block/timeline rows exist. The whole create shares one transaction, so intra-transaction ordering is invisible to the worker (it reads on another connection; the commit is atomic) — the flag can simply be set in the INSERT, saving a write. * Guard exploration restart against in-flight work The restart endpoint could fire while the planner was still working on the thread, deleting its rows mid-plan and wedging the thread in a state no worker would ever pick up again. reset-thread-for-rerun! is now a CAS: it applies only to a terminal thread (completed_at set) with no query still mid-execution, and the endpoint returns 409 ("cancel it before restarting") otherwise. A successful reset leaves the thread exactly in the shape the planning worker's claim predicate picks up. The endpoint also published :event/exploration-update, which provably did nothing: restart changes no Exploration column, and the revision system skips unchanged objects. The publish is removed; the AI-summary doc reset still records a Document revision, which is what /mine's last-touched ordering composes. * Route mid-stream LLM errors through the provider-friendly rethrow LLM responses stream lazily; a connection timeout between SSE chunks escaped as a raw SocketTimeoutException because the provider error handling only wrapped request setup — the reduction happens later, in the agent loop. Every provider adapter's SSE reducible (claude, openai, openrouter, azure, bedrock) is now wrapped so consumption-time failures get the same translation as request-time errors. * Grant the read-only timelines scope under metabot-nlq The explorations metabot profile instructs the model to use the timeline tools, but those tools were granted only under :permission/metabot-other-tools while the profile is gated on NLQ — so for NLQ-only users they were silently filtered out of scope while the prompt kept referring to them. agent:timelines:* (whose only member is the read-only list/details scope) is now granted under both permissions. * Don't retry deterministically-failing fields in the interestingness leftovers pass The leftovers pass selects on dimension_interestingness IS NULL, so a field whose scoring deterministically throws was re-attempted on every sync forever. A per-process attempted-marker now skips fields that already failed in this process; a restart re-enables retry, so transient failures still converge. Process-local by design — no schema change, and no sentinel score leaking into product surfaces that read the column as a number. * Fail closed when a sandboxed field's user attributes are missing sandbox-token-for-table returned nil when the field was sandboxed but the user's sandbox-attribute lookup came up empty — and the downstream compatibility gate reads nil as "unrestricted". That's fail-open. It now returns a user-scoped "indeterminate" sentinel that matches nobody else's token (two indeterminate users don't match each other either): fail-closed. Also adds direct and end-to-end tests for the impersonation and routing token producers, which previously had none. * Require the (card, stored_result) pairing on the cached card-query path POST /api/card/:id/query with a stored_result_id read-checked the card but then served whatever snapshot id the client sent — nothing verified the snapshot actually belonged to that card. The pairing must now exist in stored_result_use (404 when it doesn't, so an unpaired probe can't confirm a snapshot exists). Both materialization paths already insert the pairing rows, and the table ships in the same unreleased release, so no legacy data lacks them. The viewer-lens check also denied a nil dataset_query only by accident (a schema exception swallowed by the catch — and in prod, where mu/defn logs instead of throwing, it would have failed open); it denies explicitly now. Plus: consistent t2/select-one forms in the endpoint, a stored_result_use docstring update, and the generated module config picking up the new StoredResultUse consumption. * Apply the personal-collection default when it resolves after first render The "default new explorations to my personal collection" logic captured personalCollectionId once in a useState initializer — if the selector resolved after the first render, the default never applied. It now reconciles when the id arrives, without overriding an explicit user choice. * Move the explorations migrations to v64 The v63 release branch has been cut, so the explorations feature will ship in v64 and its migrations belong in the 064 directory. Both files move (20260428_explorations.yaml and 20260519_exploration_bookmark.yaml) and every changeset ID is reprefixed v63. -> v64. No changeset content changes. Since we're already editing unreleased changesets in place on this branch, existing dev databases needed a migration reset anyway. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * v_metabot_conversations v4: restore include-deleted semantics lost in merge Master's #77542 (conversation branches) and this branch both created metabot_conversations/v4 in parallel: master's dropped the deleted_at IS NULL filters so soft-deleted (rewound/regenerated) messages are counted; ours added CASE branches for the new explorations usage sources and profile. The 'Merged from master' resolution kept only our copy, silently losing the deleted-message semantics — which is why deleted-messages-included-test and last-message-at-test fail: they assert master's behavior against our view. This drops the three filter lines from each dialect's v4 file so v4 is the union of both changes. Both installing changesets (our v63 and master's v64) are runOnChange: true and checksum the referenced file, so every instance reinstalls the corrected view on upgrade — no new version or migration needed. * Drop the redundant v_metabot_conversations v4 installer Master's #77542 (064/20260713_metabot_analytics_views_include_deleted.yaml) already installs metabot_conversations v4 via a runOnChange changeset against the same SQL files, which now carry both sides' changes: the explorations CASE branches and the include-deleted semantics. A second installer here was redundant — and would have collided with master's changeset IDs (v64.2026-07-13T00:00:00/01) had this file been mechanically renumbered to v64 like the other explorations migrations. The ai_usage_log v4 changeset stays: nothing on master installs that view. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Move the ai_usage_log v4 migration to 064 Same reasoning as the explorations tables: v63 is cut, this ships in v64. The changeset ID takes the next free slot for its date (v64.2026-07-13T00:00:02 — master's include-deleted file owns 00 and 01) rather than a mechanical v63->v64 rewrite that would have collided. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * Show a crossed-eye indicator with a tooltip on hidden pages in the exploration sidebar (#77907) * Explorations no AI Summary (#77878) * Extract AI Summary + document-in-exploration out of explorations Removes the LLM-generated "AI Summary" feature and the ability to display a document within an exploration. The feature-restoring (inverse) commit is kept on branch `explorations-ai-summary`; cherry-pick it to bring the functionality back. Backend: - Delete the explorations.ai-summary.* pipeline, composite, document-perms, prompt templates, and their tests - Drop exploration->Document creation, the /thread/:id/documents endpoints, the :documents hydrate, ai_summary_document_id, and the AI-summary reset - Revert the documents-module coupling: content-visibility gate, the document.exploration_thread_id FK/column, the view-log skip, and the GET /document list filter; delete documents/validate.clj - Drop the collection-items show_exploration_documents option and the exploration-doc exclusions in recents / search / serdes / remote-sync; drop the search exploration-thread-id attribute - Remove the ai_summary_document_id and document.exploration_thread_id migration changesets (edited in place) Frontend: - Delete the ExplorationDocument component and its tests - Remove the AI-summary sidebar node, completion toast, and the document API / urls / types; keep the shared comments sidesheet and page routing * Extract static card-embed in documents out of explorations Removes the static (cached-snapshot) card-embed capability from documents, so it can land on its own branch (`static-embed`) independently of the AI-Summary feature. The feature-restoring (inverse) commit lives on `static-embed`. Keeps the base `stored_result` model, the exploration query-result cache, and the cached-result permission helpers (`assert-can-view-cached-result!`) that the exploration `/query/:id` streaming endpoint uses. Also keeps the general `report_card.document_id` "card in a document" feature (it predates this work). Removed: - The cached-result serve path (`cached-dataset`, `apply-sort`, `deserialize-cached-result`, `allowed-chart-sorts`, `viewer-can-view-cached-result?`) and the `stored_result_id`/`sort` branch of `POST /api/card/:id/query` - `stored_result_use.card_id` (column + model arm; keeps `exploration_id`) - The documents serdes static-node filter (reverted to master's live-only rewrite) and the prose-mirror static docstrings - Frontend: the tiptap `cardEmbed` static rendering (the `stored_result_id` + `sort` + `chart_href` trio) across the CardEmbed node/menu, document hooks, redux, and `card.ts`; deleted the CardEmbedNode static unit spec * Fix pre-existing cljfmt/oxfmt formatting to unblock CI These 4 files (a stray blank line each) are unchanged from the base explorations branch — the format checks were failing on inherited, not extraction, changes. * more documents cleanup * Update Loki Snapshots * change references from entity (which was page or document) to just page --------- Co-authored-by: Kevin Kelleher <kevin.kelleher@metabase.com> Co-authored-by: Metabase Automation <github-automation@metabase.com> * Restore fix * Fix Metabot exploration tools streaming empty results to the client (#77990) Since |
||
|
Anderson
|
70a9109bf3 |
Track Dimensions tab views and dimension add/remove/reorder/edit events for metrics (#78721)
* Add Snowplow event tracking to the metric Dimensions tab * Update events' definitions to comply with new approach * Update analytics-events claude skill to account for new approach defined in #71631 |
||
|
Stas Gavrylov
|
477deaced9 |
Improve frontend TypeScript skill loading (#79009)
Update frontend skills to improve typescript-write referencing |
||
|
Kamil Mielnik
|
74ec111ffe |
Prevent claude from inserting // Unjustified type cast. FIXME comments (#78035)
|
||
|
Ben Grabow
|
ba83271710 |
Auto-fix modules config.edn (#77541)
* Auto-fix modules config.edn * Extract generic nrepl integration wrapper * Remind claude to use this * Demo the module config fixer with some tests |
||
|
Oleh K
|
3512caf5ee |
refactor: Migrate feedback, components, misc, upsell, illustration & admin color tokens (#76871)
* Migrate remaining color tokens to new names (GDGT-2535)
Migrate the remaining legacy design-system color tokens to their new
names per the GDGT-2517 migration guide:
- feedback: error/danger -> feedback-negative, success ->
feedback-positive, warning/gold -> feedback-warning
- misc/core: info -> core-info, white -> core-white, shadow ->
shadow-default, overlay -> modal-overlay, focus -> input-focus,
metabase-brand -> core-metabase_brand
- admin: admin-navbar* -> navbar-admin*
- upsell/illustration: per guide
- remove unused bronze/copper/silver
Covers all reference forms: CSS `var(--mb-color-*)`, `color()`/helper
calls, Mantine `c`/`bg`/`color`/`iconColor` props (string and ternary),
object-literal `color:`/`iconColor:` keys, `ColorName` return values,
arrays, and `Record<_, ColorName>` value maps. Tightens `Undo.toastColor`
from `string` to `ColorName` so future token removals fail at compile
time. Maps the `Alert` component's semantic `color` prop to feedback
tokens internally so its call sites stay unchanged.
Also sweeps up brand/filter/summarize consumer stragglers left over from
the earlier (merged) migration. SDK/whitelabel input keys, the
derive-theme alias definitions, and static-viz's local palette are left
on their existing names by design.
* Migrate missed success-secondary props in StepperWithCards
The .module.css was migrated to feedback-positive-selected but the two
inline c="success-secondary" props (Done check icon + label) were missed.
* Update Loki Snapshots
* Fix toastColor type widening in metabot toast-prop helpers
Undo.toastColor tightened to ColorName in this branch; the two get*ToastProps
helpers returned object literals where toastColor widened to string (missing
as const, unlike their iconColor/dismissIconColor siblings).
* Update TransformListPage sync-color spec to migrated feedback token names
utils.ts color map was migrated (success/warning/danger -> feedback-*) but
the spec assertions still expected the old names.
* Update SDK theme + treemap specs for feedback/misc token mirrors
- get-embedding-theme: SDK mapping now fans text-white->core-white and
focus->input-focus, so the expected override gains those keys.
- treemap option: getColor("white") became getColor("core-white"); add
core-white to the test's MOCK_COLORS palette.
* Update Loki Snapshots
* Update toast color-attr assertions to migrated feedback token names
* Cleanup
* Migrate remaining color tokens in Button and Table stories
* Add explainer comment for colors naming convention
---------
Co-authored-by: Metabase Automation <github-automation@metabase.com>
|
||
|
Oleh K
|
8f5eb11fba |
refactor: Migrate text and syntax tokens (#76508)
* GDGT-2533 Migrate text and syntax tokens
Sweep all consumer references from the legacy text/syntax tokens to their
new names (added in the foundation PR):
- text-tertiary -> text-disabled (and -inverse) — value shifts from solid
orion to orion-alpha (intended recolor)
- text-hover -> text-brand-hover (value unchanged)
- syntax-parameters(-active) -> text-syntax-parameter(-active)
- saturated-{blue,green,yellow,red,purple} -> core-{blue,green,yellow,red,
purple}-saturated (value unchanged)
`text-tertiary` and `text-hover` are also SDK public theme keys, so the
SDK_TO_MAIN_APP_COLORS_MAPPING right-hand sides gain the new internal names
as second targets and the public keys are left unchanged.
Per design review, the five saturated colors are renamed to
`core-{color}-saturated` palette primitives rather than `text-syntax-{role}`;
this PR adds those token definitions, removes the superseded text-syntax-*
ones, and keeps the legacy saturated-* definitions emitting until cleanup.
`text-secondary-opaque` is left untouched (open design question).
* Formatting
* Update Loki Snapshots
* Keep public SDK theme key text-tertiary in embedding examples/tests
text-tertiary is an embedder-facing SDK theme key (MetabaseColors), not an
internal token. The blanket sweep wrongly renamed it to the internal
text-disabled in three embedding spots that live outside the type-check
scope (e2e specs + the CLI scaffold snippet). Revert those to the public
text-tertiary; the SDK maps it internally via SDK_TO_MAIN_APP_COLORS_MAPPING.
---------
Co-authored-by: Metabase Automation <github-automation@metabase.com>
|
||
|
Nemanja Glumac
|
9a255e1059 |
Require an honest e2e-vs-unit-test breakdown in e2e-test-review reports (#76514)
Add a mandatory closing section to the e2e-test-review skill that classifies each it() block as load-bearing e2e, narrowable, or a unit/component-test candidate, grounded in which layers the test actually exercises. Wire it into the review process and final-check steps so every report includes it. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Oleh K
|
6dd944131e |
refactor: Migrate background-* tokens to background_page-* / background_surface-* (#76049)
* GDGT-2532 Migrate background-* tokens to background_page / background_surface
Sweep all consumer references from the legacy `background-*` semantic
tokens to their `background_page-*` / `background_surface-*` counterparts
(added in the foundation PR):
- Page: background-primary/secondary/tertiary (+ -inverse), -highlight
(-> -highlighted), -filter -> background_page-*
- Surface: background-disabled (+ -inverse), -hover, -selected, -brand
(-> -brand-subtle), -success, -warning (+ -secondary -> -strong),
-error (+ -secondary -> -subtle) -> background_surface-*
- Replaces: background-info -> background_page-tertiary;
background-success-secondary -> feedback-positive-selected
SDK public theme keys (background, background-secondary, -hover, -disabled,
-error, -light) are part of the embedder-facing contract and are left
unchanged; the SDK_TO_MAIN_APP_COLORS_MAPPING right-hand sides gain the new
internal names as second targets so SDK/whitelabel overrides reach consumers
that now read the new tokens. Alert.config.tsx's `background-${color}`
synthesis is replaced with an explicit lookup map.
Drive-by fixes (pre-existing, surfaced by the sweep):
- EmbedFrame.module.css: fix `-inverseer` typo (undefined var since #65855).
- Bare `var(--mb-color-background)` was never a real token (rendered
transparent at 8 sites); point each at the correct token by role and
remove the dead `--mb-color-background` definition in LogoBadge that was
silencing find-never-defined-css-variables.
* Use surface background tokens for Switch component
The Switch is a control (a surface on the page), so its track/thumb
backgrounds should use `background_surface-*` rather than `background_page-*`.
Disabled states now use the purpose-built `background_surface-disabled`;
the enabled off-track uses `background_surface-secondary`.
* Remove redundant comment
* Use text-secondary for RowChart axis text
The axis tick + title text was borrowing `background_page-tertiary-inverse`
(a background token) for its color. That token recolors as part of the
background migration, which shifted the axis text. Point it at the proper
`text-secondary` token instead — also consistent with the goal label.
* Update Loki Snapshots
* Formatting
* Update Loki Snapshots
* Update dark-mode body background assertion for recolored page-secondary
The body background uses `background_page-secondary`, which this migration
intentionally recolors in dark mode (color-mix off the page primary). Update
`assertDarkMode` from rgb(5, 14, 21) to rgb(5, 17, 25) (#051119) to match.
Light mode is unchanged.
* Assert dark-mode body background as color(srgb ...) computed value
The body background is a color-mix(in srgb, ...), which browsers serialize
in computed style as color(srgb ...). Assert the exact computed value
instead of an approximated rgb().
* Formatting
---------
Co-authored-by: Metabase Automation <github-automation@metabase.com>
|
||
|
Nemanja Glumac
|
dc80345e66 |
Drop the fix-flaky-test command (#76299)
|
||
|
Oleh K
|
18eeb58dc2 |
refactor: Sweep border-* tokens to border-neutral-* (#75611)
* Sweep `border-*` tokens to `border-neutral-*` (GDGT-2531) Renames all internal consumers of the legacy border semantic tokens to their `border-neutral-*` counterparts (added by the foundation PR): - `--mb-color-border` → `--mb-color-border-neutral` - `--mb-color-border-strong` → `--mb-color-border-neutral-strong` - `--mb-color-border-stronger` → `--mb-color-border-neutral-strongest` - `--mb-color-border-subtle` → `--mb-color-border-neutral-subtle` Sweep covers `var(--mb-color-…)` references in CSS modules and styled-components, `color()`/`alpha()`/`darken()`/`getColor()`/ `renderingContext.getColor()`/`theme.fn.themeColor()` helper calls, `theme.colors.border`/`theme.colors.border[0]` accesses, Mantine `c`/`bg`/`color` props, and a couple of single-site palette lookups. All four are pure renames — `border-neutral*` resolve to the same primitives as the legacy tokens in both light and dark modes. `border` is an SDK v1 theme input key (`MetabaseColors.border`), so `SDK_TO_MAIN_APP_COLORS_MAPPING.border` now maps to both `["border", "border-neutral"]`. Without it, an SDK consumer setting `theme.colors.border = "#hex"` would update only the legacy `--mb-color-border`; the new `--mb-color-border-neutral` that the swept code reads would stay at its default. The dual-map covers both SDK v1 paths — the global `colors` object via `getEmbeddingColorPalette` and the Mantine override via `getEmbeddingThemeOverride`. Unlike `brand`/`filter`/`summarize`, `border` isn't a whitelabel (`application-colors`) key, so no whitelabel-passthrough mirror is needed. Also flips the four `border` references in the emotion-migrate skill to their `border-neutral` equivalents, matching the per-sweep convention. The SDK contract surfaces (`embedding-color-palette.ts` left-side input key, `dynamic-css-vars-config.ts`, `dynamic-sdk-color-defaults.ts`, `MetabaseTheme.ts`, `apply-theme-preset.ts`) stay on the legacy `border` name. * Realign table in SKILL.md |
||
|
Nemanja Glumac
|
f35c54150c | Harden the flaky text fix command (#76005) | ||
|
Nemanja Glumac
|
c8350535e7 |
Design the autonomous e2e flake fixer (#75909)
Replace the human push-approval gate with a branch-existence dedup check: the branch name is derived solely from the Linear ref (automated-e2e-flake-fix-<TEAM_SLUG>-<ISSUE_NUMBER>), and an existing remote branch means a run is already in flight — otherwise it's a go, no approval. The command stays identity-agnostic, so a human locally and an automated trigger run the same path. Also: prefer Linear MCP with a REST/GraphQL fallback for headless runs, drop the poll cadence to 15 minutes, and lead the Linear audit-trail comment with total time spent and tokens used. * Add claude-e2e-flake-fix workflow for autonomous e2e flake fixing Runs /fix-flaky-test headlessly as the Metabase Automation user, triggered by workflow_dispatch or repository_dispatch (e.g. from ci-conductor) with a Linear issue ref. ci-conductor only dispatches; the agent runs here, in CI. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Nemanja Glumac
|
4c586296a9 |
Pass build_jar to stress-test workflow in /fix-flaky-test (#75891)
The e2e-stress-test-flake-fix workflow gained a build_jar input that builds the uberjar from the branch instead of downloading a prebuilt ancestor JAR. Teach the command to set it: build whenever any commit on the branch touches source (backend or frontend), keeping it on for later test-only attempts; skip the build only when every commit is test-only. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Oleh K
|
376db5fb43 |
refactor: Sweep brand/filter/summarize tokens to core-* (#75332)
* GDGT-2529: Add new color tokens alongside existing ones Foundation for the GDGT-2517 color token refactor. Adds new and renamed semantic tokens without changing any existing emission or call sites. After this PR, both old and new CSS variable names resolve to the same values; migration sweeps will move consumers to the new names, and the cleanup PR will remove the old. * GDGT-2529: Move kept tokens from Legacy to New section These 14 tokens are not being renamed (text-primary, text-brand, border-brand, tooltip-*, switch-off, etc.) and have no equivalent in the New section. Moving them into the New group prevents readers from assuming they're discontinued when they encounter the Legacy section's 'Prefer using new ones' comment. * Update Loki Snapshots * Rearrange and aort new colors keys alphabetically * Add explanation to core-brand/filter/summarize copy * Update Loki Snapshots * Split light/dark themes into `deprecatedColors` and `actualColors` Per review feedback on #75227 — replaces the `// Legacy colors` and `// New` section comments with named top-level constants so the intent shows up in the import graph rather than as inline prose. * Sweep `brand`/`filter`/`summarize` to `core-*` (GDGT-2530) Renames all consumer-side references to the legacy semantic tokens to their `core-` counterparts added by the foundation PR: - `--mb-color-brand` → `--mb-color-core-brand` (and `-hover`) - `--mb-color-filter` → `--mb-color-core-filter` - `--mb-color-summarize` → `--mb-color-core-summarize` Sweep covers: `var(--mb-color-…)` references in CSS/styled-components, `color()`/`alpha()`/`lighten()`/`darken()`/`getColor()` helper calls, Mantine `color`/`c`/`bg`/`iconColor`/`colorScheme`/`primaryColor` props, type unions, default destructure values, ternary/fallback expressions in JSX, static color-name lists, plus three doc files (emotion-migrate skill, custom-viz template README, mantine-styles contract test). Both old and new CSS variables continue to emit during the migration, and the SDK/whitelabel input keys (`theme.colors.brand`, `application-colors.brand`) are unchanged; the rename is internal. * Re-align emotion-migrate SKILL.md tables after the brand → core-brand edits * Fix two visual regressions from the brand → core-brand sweep - SDK v1 `theme.colors.brand` (and `filter`/`summarize`) now writes to both the legacy and the new core-* internal token via `SDK_TO_MAIN_APP_COLORS_MAPPING`, so consumers that have already migrated to `var(--mb-color-core-brand)` pick up SDK overrides the same way legacy consumers do. The v2 path was already covered by the mirror in `derive-theme.ts`; this PR patches the v1 path. - `<Alert color="core-brand">` lost its tinted background because `Alert.config.tsx` was synthesizing the background-color token name via `background-${props.color}`, which produced the non-existent `background-core-brand`. Added an explicit `core-brand → background-brand` case so the tint resolves correctly while the `background-*` family is still on the legacy namespace. * Revert sweep over CssVarsDeclarationPlugin test assertions The sweep accidentally flipped two `toContain("--mb-color-brand: ;")` assertions to `core-brand`, but the test's synthetic TS fixtures still define `type ColorKey = "brand" | …` — the plugin parses those literally and emits `--mb-color-brand`, so the assertions need to track the fixture, not the production rename. * Mirror `brand`/`filter`/`summarize` to `core-*` in embedding palette The SDK v1 path rebuilds the global `colors` map via `setGlobalEmbeddingColors` → `getEmbeddingColorPalette` on every theme or whitelabel change, which bypasses the mirror in `derive-theme.ts`. Whitelabel `application-colors` comes in via `appPalette` directly keyed by the legacy `brand`/`filter`/`summarize` names, so before this fix only the legacy CSS vars picked up the whitelabel override and the new `core-*` keys silently fell back to their built-in defaults. Mirror them at the end of the merge so consumers reading `var(--mb-color-core-brand)` get the right color regardless of where in the merge chain the brand value came from. * Try to fix missing core-* variables * Remove manual patch of core-* colors in getEmbeddingColorPalette --------- Co-authored-by: Metabase Automation <github-automation@metabase.com> |
||
|
Oleh K
|
5b36ecb251 |
Add Storybook showcase helpers and a Figma component skill (#75693)
* Add reusable Storybook showcase components Storybook-only presentational primitives for building component showcase stories: StoryShowcase (bordered panel + title), StorySection, StoryRow, and StoryJsx (monospace JSX with light, theme-aware syntax highlighting via tokenizeJsx). Live in metabase/ui/stories/showcase so they stay out of the metabase/ui barrel. * Add metabase-ui-component-from-figma skill Project skill codifying the workflow for restyling/implementing a metabase/ui component from a Figma spec: check out the issue, map usage/blast radius, build the Storybook showcase first, style from exact semantic tokens, iterate, then commit and migrate call sites. Registered in frontend/CLAUDE.md. * Handle closing tags and text children in StoryJsx tokenizer Track whether we're inside a tag vs. element content so text children render as plain text, and stop clearing the tag-name expectation on `/` so the name in a closing tag (e.g. `</Kbd>`) is recognized as a tag. Handles `<Kbd>⌘</Kbd>`. * Refine component-from-figma skill per review feedback Raise the abstraction (drop Chip-specific anecdotes and one-time setup rationale), split into update-existing vs create-new modes with the showcase/styling order flipped for new components, derive the showcase matrix from the component's full state space (theme excluded — it's global, not a prop), and add a scoped test-first step (custom-interactive components only, not thin Mantine wrappers). * Address skill review feedback: trim duplication, add story example Trim the Phase 1 mode-decision step and Phase 2 path reiteration, add an upfront Figma desktop MCP availability check, mention playwright-mcp as an alternative browser MCP, and embed an illustrative finalized matrix story. * Flag non-color token scale mismatches in component-from-figma skill Extend the discrepancy guardrail beyond colors: radius/spacing/elevation/type values that have no matching step in the codebase's scales (e.g. Figma radius md=12px vs --mantine-radius-md=8px) should be flagged to the user, not silently baked in as literals. * Fix typo |
||
|
Nicolò Maria Mezzopera
|
301179ef9b |
Add frontend skill patterns from FE PR review feedback (#72689)
* Add frontend skill patterns from Gadget PR review feedback Codify recurring review feedback from the last 2 months of Gadget frontend PRs into shared skill patterns so Claude can apply and enforce them while writing and reviewing TypeScript code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add error/loading, Mantine, i18n, security, bundle-size patterns Extend frontend skill patterns with themes surfaced by aggregating PR review feedback across all 29 frontend engineers over the last 3 months. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Address PR review feedback: restructure skills, remove duplication - Inline typescript-patterns into typescript-write, delete shared file - Merge testing-patterns into e2e-test-create, delete shared file - Move Mantine/i18n guidance to frontend/CLAUDE.md - Remove duplicate review checklist from typescript-review - Remove too-specific examples (custom viz, metabase-lib) - Remove Cypress line from react-redux-patterns - Add non-strict null comparison rule (egorgrushin) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Address second round of PR feedback and fix incongruences - Revert unstructured blob in e2e-test-create; surgically insert helpers into Phase 0, intercept guidance into Phase 3, Cypress.env into "What NOT to do" - Remove redundant "Before you submit" checklist from typescript-write - Fix frontend/CLAUDE.md tech stack line (CSS Modules > Emotion → Mantine > CSS Modules) - Add etc. to style props list, remove translation-branch-in-tests line - Deduplicate "do not derive loading from data" in react-redux-patterns - Add "check helpers first" preamble to Intercepts example Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Update Claude skills and guidelines * Address PR comments * Updates following PR comments * Address feedback * Fix path typo |
||
|
Nemanja Glumac
|
a866620984 |
Pilot: self-healing e2e tests (/fix-flaky-test command) (#75434)
Add /fix-flaky-test pilot command for self-healing e2e tests A bounded, human-supervised loop that diagnoses a quarantined flaky e2e test, proposes a fix on a branch, and verifies it with CI stress runs (burn_in=50, fail_fast, network throttling on/off). Pilot, not autonomous: a human approves before any push and the resulting PRs never auto-merge. |
||
|
Cam Saul
|
4eb1408f48 |
Remove legacy :druid driver / migrate to :druid-jdbc (#75143)
* Remove legacy `:druid` driver / migrate to `:druid-jdbc` * Add no-op migration rollback * Update e2e tests * Remove additional places mentioning removed Druid driver * Kondo * Update modules config |
||
|
Nathan Voxland
|
6f3b45b576 | Expand Clojure style guide with helpers and idioms (#75124) | ||
|
Timothy S. Dean
|
8a36e2964f |
Claude.md instructions for docstrings (#74246)
Adds guidelines for writing docstrings to the Clojure development skill. Docstrings should describe the caller's contract (arguments, return values, preconditions), not implementation details — implementation context belongs in inline comments at the relevant code site. Document deviations from expectations, not conformance to them. |
||
|
Cam Saul
|
6404c3c029 | Stop using 'pMBQL' and 'MLv2' in the codebase (again) (#73883) | ||
|
bryan
|
45d4213e77 | Add 10 backend expert subagents under .claude/agents/ (#73461) | ||
|
Nathan Voxland
|
35b9869301 | agent-improve: bot self-retrospectives (#73130) | ||
|
Nathan Voxland
|
60dffb06c9 | uxbot: UX testing agent (#73128) | ||
|
Nathan Voxland
|
95e7c618c6 | qabot: QA analysis agent (#73127) | ||
|
Nemanja Glumac
|
7d1310b7c5 | Improve e2e skills wrt test timing (#73563) | ||
|
Nathan Voxland
|
3475fb1506 | fixbot: Simple issue-fixing agent (#73129) | ||
|
Nathan Voxland
|
09f80ed3bc | reprobot: bug reproduction agent (#73132) | ||
|
Nathan Voxland
|
9e8194f56d | Bot infrastructure and orchestrator (#73126) | ||
|
Nemanja Glumac
|
fa51e6aa8e | Add Cypress test review skill (#73378) | ||
|
Romeo Van Snick
|
3b2fa353a2 |
Cleanup metabase/utils/analytics (#72981)
|