Files
metabase/enterprise/backend/test/metabase_enterprise/curated_search/reconcile_test.clj
Chris Truter 0bc758804b Add search_curated tool for NLQ (#75237)
* Add semantic_layer_index table and the semantic-layer-search OSS module

The semantic layer is a hand-curated library of saved search prompts, each
mapped to the entities that answer it (canonical = one directly-answering
entity, sources = a set to combine), plus agent-facing usage_instructions.

- migration: single createTable for semantic_layer_index (search_prompt,
  usage_instructions, entities JSON, type, verified, entity_id, timestamps)
- :model/SemanticLayerIndex with validation (non-empty entities; canonical =>
  exactly one) and serdes spec (polymorphic entity refs export as portable
  Card entity_ids / Table paths, mirroring export-viz-link-card)
- write hooks only nudge the enterprise background sync (request-sync!
  defenterprise shim, no-op in OSS) - no embedding or pgvector work on the
  write path
- superuser CRUD API at /api/semantic-layer-search
- module config: new semantic-layer-search module entry

* Add the semantic_layer_search Metabot tool and wire it into the nlq profile

The tool matches the user's request by vector similarity against the curated
semantic layer instead of ranking the whole instance:

- over-fetches raw per-prompt hits, dedupes to distinct entity sets (several
  prompts map to one entity), and returns a small best-first list
- hydrates matched entity refs into the enriched search-result shape via a new
  public tools.search/entity-refs->search-results (portable_entity_id,
  database names, fully-qualified names, metric base tables), so the agent can
  build a query without an extra read_resource round-trip
- surfaces raw cosine similarity and flags low-confidence matches (< 0.45)
  with a leading <note> + confidence="weak", so a miss reads as a miss
- the nlq profile uses it in place of nlq-search-tool

* Add the enterprise pgvector mirror with asynchronous reconciliation

Replaces the hackathon design (embedding-service + pgvector writes inside the
appdb write path via Toucan hooks, best-effort and silently drifting) with a
self-healing background sync:

- reconcile! diffs the whole authoritative appdb table against the mirror by
  content hash every run: stale rows are re-embedded (batched), orphans
  deleted, unchanged rows cost nothing. Any missed write - pgvector downtime,
  a crashed node, an import that bypassed the hooks - is repaired on the next
  run with no operator action.
- index-table/ensure-tables! owns the pgvector DDL: vectors table + HNSW
  index + a single-row meta table recording the embedding model identity and
  schema version. A model/dims change (or version bump) drops and recreates
  the vectors table; the next reconcile re-embeds everything. That
  drop-and-rebuild is the whole model-change story - the source table is
  small and always available.
- task.sync runs reconcile! on a 30s Quartz schedule (cluster-singleton,
  DisallowConcurrentExecution); appdb write hooks trigger-now! the job so
  curator edits are searchable within seconds, with trigger coalescing as
  free debounce.
- core/search serves the tool's similarity query (cosine distance + flat
  canonical/verified boosts, regular-search-shaped score breakdown); [] when
  pgvector is unconfigured or the mirror table does not exist yet.

End-to-end test drives CRUD API -> reconcile -> pgvector -> tool against an
isolated mirror with the 4-dim mock embedding model.

* Wire SemanticLayerIndex into serialization export/import

- exported-models + extract/model-set (with a :no-semantic-layer-index
  opt-out, like sibling models) + ingest legal-top-level-paths
  (semantic_layer_index/, matching the model's storage-path label)
- round-trip coverage: in-memory ref transformation and a real on-disk
  export/import (the two layers that have silently dropped models before),
  excused from the shared baseline fixture it covers itself

* Simplify the semantic layer to a single entity per entry

Drop the sources/canonical distinction: every entry now maps one saved prompt
to exactly ONE entity. Simplifications that follow:

- appdb table: `entities` JSON array + `type` column become a single
  `entity` JSON object; the canonical-arity validation hooks disappear
- pgvector mirror: `canonical` column gone; scoring is similarity (1.0) +
  verified boost (0.1) only
- tool: dedupe key is the single entity, matches lose the kind attribute,
  flatten-data loses match_group, and total_count now trivially equals the
  per-entity row count
- API: no type field/filter

Also folds in the roborev branch-review findings (job 1915):

- serdes entity-ref vocabulary now covers ALL the entity models the tool
  hydrates (question was missing -> export threw; card was hydrated nowhere),
  with an export-viz-link-card-style fallback for unmapped model strings
  instead of aborting the export, and round-trip coverage for every card
  flavor (card/model/metric/question) plus table
- the sync job uses task/defjob (log context + tracing span) instead of a
  raw deftype re-implementing half of it

The remaining two findings (PUT canonical revalidation, total_count vs
flattened rows) are moot now that canonical/sources is gone.

* Address Copilot PR feedback: card->question normalization, XML escaping, finite embeddings

- entity refs with :model "card" now hydrate (normalized to the agent-facing
  "question" type via a new public tools.search/ref-model->entity-type, used
  for both hydration grouping and the tool's lookup key)
- curator-entered saved_search_prompt / usage_instructions are XML-escaped in
  the tool output
- format-embedding rejects NaN/Infinity (they satisfy number? but produce SQL
  literals pgvector chokes on)

* Fix nlq-profile tests for the renamed tool + roborev follow-up nits

CI: the pre-existing profile/tool tests asserted the nlq profile exposes a
tool named "search"; it is now "semantic_layer_search". embedding_next
deliberately keeps the general nlq-search tool (switching embedding customers
to a curated-only layer that starts empty was not in scope), so the
matches-nlq invariant test now asserts the tool sets are identical modulo
that one substitution.

roborev (jobs 1916/1917):
- reuse llm-shape/escape-xml (now public) instead of a second, narrower copy
- format-embedding error message covers non-numbers too ("invalid value")
- API EntityRef :model is now an enum of the known entity types, so garbage
  refs are rejected at the boundary; the serdes :else passthrough (for rows
  predating a model string's removal) gets a test
- migration header notes the in-place changeset edit for anyone who ran the
  earlier branch revision

* Relax the entity-ref response schema to tolerate retired model strings

The write-side enum stays (garbage refs still 400 on POST/PUT), but GET
responses accept any :model string: rows can predate a model string's
retirement — serdes already tolerates those — and under dev/test response
validation a single legacy row would otherwise 400 the whole list endpoint.
(roborev job 1918)

* Drop the in-place-edit note from the migration header

The earlier revision never ran anywhere that survives (and the table name is
new on this branch anyway), so there is no checksum-mismatch audience to warn.

* Fix CI: migration 062->063, ASCII minus, mage driver-test override

* Lint with-isolated-mirror as fn, not let (single-symbol binding)

* Address Copilot round 2: rank by blended score in SQL, PUT patch semantics, smaller nits

- search: the SQL LIMIT now orders by distance minus the verified boost
  (equivalent to the blended score), so a verified row whose boost lifts it
  into the top N is no longer cut by the raw-distance limit; covered by a
  ranking test that fails against the old ordering
- PUT: presence of a key (contains?), not nil-ness, decides what changes, so
  an explicit null clears usage_instructions; entity/verified reject null in
  the schema since the columns are non-nullable
- EntityRef write schema: id must be a positive int
- entity-refs->search-results: drop the per-card (set verified) rebuild;
  select-fn-set already returns a set
- tool instructions prose names the match/note elements without literal
  angle-bracket tags, keeping the instructions element well-formed

* Drop the unused HNSW index from the mirror table

The blended ORDER BY (distance minus the verified boost) can't be served by
an HNSW index, so since the ranking fix the index was built and maintained
but never used on the read path. An exact scan is intentional and
microseconds at curated-table scale; documented at the ranking expression
and in the index-table namespace docstring. (roborev job 1923)

* Tighten docstrings: contracts first, one sentence per line

- mirror/search shim now documents its return shape (the caller contract the
  metabot tool builds on)
- upsert-batch! claimed to return the rows; it returns the count
- format-embedding states the contract (throws on non-finite elements); the
  pgvector-chokes rationale moves to an inline comment at the check
- stale plural in the module docstring (entries map to ONE entity now)
- multi-sentence paragraphs reflowed to one sentence per line

* Bump mirror schema-version so existing tables shed the orphaned HNSW index

Dropping the index creation left tables built under version 1 carrying a
maintained-but-unused index until some other rebuild trigger. The version
bump forces the one-time drop-and-rebuild on first sync. (roborev job 1924)

* Test that a schema-version mismatch alone triggers the mirror rebuild

The version bump is the rebuild trigger the previous commit leans on; cover
it directly (stale-write the meta row, assert :rebuilt + emptied vectors
table) so dropping schema_version from the meta comparison can't regress
silently. (roborev job 1926)

* Assert the mirror rebuild self-terminates

After the rebuild cases, a follow-up ensure-tables! must return :ok — i.e.
write-meta! healed the meta row. Without this, breaking schema_version
handling in write-meta! would silently rebuild on every sync and the tests
would still pass. (roborev job 1927)

* Rename semantic-layer-search -> curated-search; tool 'search_curated', augment not replace

Drops the overloaded 'semantic layer' term (it collides with the BI metrics
layer and with 'semantic search' = embedding IR). New vocabulary throughout:

- appdb table semantic_layer_index -> curated_search_entries
  (model :model/CuratedSearchEntry); pgvector mirror -> curated_search_index
  (now the only thing called an 'index')
- modules curated-search / enterprise/curated-search; API /api/curated-search;
  serdes model CuratedSearchEntry, dir curated_search_entries
- tool var curated-search-tool, :tool-name "search_curated" (verb-first like
  the other tools; distinct from the general 'search' so the two can coexist)

Behavior: the nlq profile now offers BOTH the general search and the curated
search tool, rather than replacing one with the other. The curated tool is
gated on the :semantic-search feature (via a :feature-semantic-search
capability derived in profiles/feature-capabilities) and its description tells
the agent to prefer it, using general search as fallback/complement.

Also tightened two over-long inline comment blocks in EE core.

* Fix alphabetical ordering after the curated-search rename

The in-place sed rename left three sorted lists with curated-search/
CuratedSearchEntry in the old 's' slot:
- api_routes route-map (fails keep-routes-sorted-test) + its keep-me
- models/resolution model->namespace map
Move them to their 'c' slots. (roborev job 2041)

* Filter curated-search hydration by current-user read permissions

entity-refs->search-results fetched Tables/Cards via direct t2/select with no
permission check, so an admin-curated entry pointing at a restricted entity
could leak its name/description/db/portable_entity_id to a user who can't
access it. Filter both fetches with mi/can-read? (mirrors the transform
filter already in this pipeline). (Copilot PR review)

* Address claude-code branch review: permission-filter before take-n, locale-safe XML

- build-matches: hydrate (and thus permission-filter) the whole deduped
  candidate set, then take n — taking n first let unreadable top hits shrink
  the result below n while readable matches sat just past the cut. The
  preceding permission fix made this reachable.
- match->xml: use String/format with Locale/ROOT so %.3f always renders a '.'
  decimal separator (clojure.core/format uses the default locale; a
  comma-decimal site locale produced score="0,950").
Both covered by new regression tests. (roborev job 2042)

* Address claude-fable-5 branch review

- core_test e2e: bind a superuser around the tool calls — the permission
  filter (mi/can-read?) needs *current-user-id*, so the test was returning []
  and would fail wherever it actually runs.
- semantic-search.yml: add the curated_search EE test dir to each job's :only,
  so the pgvector-gated tests (reconcile, rebuild, e2e) actually run in CI
  instead of silently no-opping — which is why the broken e2e went unnoticed.
- dedupe-by-entity: normalize the model so a "card" and a "question" entry
  for the same Card collapse to one (both hydrate to one record).
- ordering: move CuratedSearchEntry / curated-search to their alphabetical
  slots in exported-models, mage driver-affecting-overrides, and the kondo
  lint-as map (+ column alignment).
(roborev job 2048)

* Address claude-fable-5 review: schedule sync independent of feature flag, harden hydration + prompt

- sync task: schedule the Quartz job whenever pgvector is configured (boot-fixed),
  not gated on the semantic-search feature. The job body already self-gates, so
  this keeps the periodic safety net + the write-path trigger's target job alive
  when a license is enabled after startup (otherwise the mirror silently never
  syncs while the tool is offered). New pgvector-configured? predicate.
- card-refs->results: emit one record per ref so a card registered under two
  type strings no longer collapses (last-type-wins) and drops a match.
- API: search_prompt is NonBlankString on POST/PUT — reject blank prompts
  (meaningless embedding + degraded serdes slug).
All covered by new tests. (roborev job 2110)
2026-06-20 00:07:54 +02:00

120 lines
6.7 KiB
Clojure

(ns metabase-enterprise.curated-search.reconcile-test
(:require
[clojure.test :refer :all]
[metabase-enterprise.curated-search.index-table :as index-table]
[metabase-enterprise.curated-search.reconcile :as reconcile]
[metabase-enterprise.semantic-search.db.datasource :as semantic.db.datasource]
[metabase-enterprise.semantic-search.test-util :as semantic.tu]
[metabase.test :as mt]
[next.jdbc :as jdbc]
[next.jdbc.result-set :as jdbc.rs]
[toucan2.core :as t2]))
(set! *warn-on-reflection* true)
(deftest content-hash-test
(let [row {:search_prompt "p" :usage_instructions "u" :entity {:model "table" :id 1}
:verified false}]
(testing "equal rows hash equal; nil and absent usage_instructions hash like empty"
(is (= (reconcile/content-hash row) (reconcile/content-hash row)))
(is (= (reconcile/content-hash (assoc row :usage_instructions nil))
(reconcile/content-hash (assoc row :usage_instructions "")))))
(testing "every mirror-relevant field changes the hash"
(let [h (reconcile/content-hash row)]
(doseq [variant [(assoc row :search_prompt "p2")
(assoc row :usage_instructions "u2")
(assoc row :entity {:model "table" :id 2})
(assoc row :verified true)]]
(is (not= h (reconcile/content-hash variant)) (pr-str variant)))))))
(deftest format-embedding-rejects-invalid-values-test
(testing "non-numbers, NaN and infinities are rejected before they reach a raw SQL literal"
(doseq [bad [Double/NaN Double/POSITIVE_INFINITY Double/NEGATIVE_INFINITY "0.1"]]
(is (thrown-with-msg? Exception #"invalid value"
(index-table/format-embedding [0.1 bad 0.3]))
(pr-str bad)))
(is (string? (index-table/format-embedding [0.1 -0.2 0.3])))))
(defmacro ^:private with-isolated-mirror
"Run `body` with isolated pgvector tables bound and dropped afterwards, binding `ds-sym` to the
pgvector datasource. Skips the body entirely when no pgvector store is configured."
[[ds-sym] & body]
`(when semantic.db.datasource/db-url
(let [suffix# (System/nanoTime)
~ds-sym (semantic.db.datasource/ensure-initialized-data-source!)]
(binding [index-table/*vectors-table* (str "curated_search_index_test_" suffix#)
index-table/*meta-table* (str "curated_search_index_meta_test_" suffix#)]
(try
~@body
(finally
(jdbc/execute! ~ds-sym [(str "DROP TABLE IF EXISTS "
index-table/*vectors-table* ", "
index-table/*meta-table*)])))))))
(defn- mirror-rows [ds]
(jdbc/execute! ds
[(format "SELECT index_id, search_prompt, verified, content_hash FROM \"%s\" ORDER BY index_id"
index-table/*vectors-table*)]
{:builder-fn jdbc.rs/as-unqualified-lower-maps}))
(deftest ^:sequential reconcile-lifecycle-test
(with-isolated-mirror [ds]
(let [model semantic.tu/mock-embedding-model]
(mt/with-temp [:model/CuratedSearchEntry {id-1 :id}
{:search_prompt "monthly revenue"
:entity {:model "table" :id 1}}
:model/CuratedSearchEntry {id-2 :id}
{:search_prompt "orders and customers" :verified true
:entity {:model "table" :id 2}}]
(let [appdb-total (t2/count :model/CuratedSearchEntry)]
(testing "first run mirrors every appdb row"
(is (=? {:upserted appdb-total :deleted 0 :unchanged 0}
(reconcile/reconcile! ds model)))
(is (=? {id-1 {:search_prompt "monthly revenue" :verified false}
id-2 {:search_prompt "orders and customers" :verified true}}
(-> (into {} (map (juxt :index_id identity)) (mirror-rows ds))
(select-keys [id-1 id-2])))))
(testing "an unchanged second run embeds and writes nothing"
(is (=? {:upserted 0 :deleted 0 :unchanged appdb-total}
(reconcile/reconcile! ds model))))
(testing "an updated row is re-embedded; the rest stay unchanged"
(t2/update! :model/CuratedSearchEntry id-1 {:verified true})
(is (=? {:upserted 1 :deleted 0 :unchanged (dec appdb-total)}
(reconcile/reconcile! ds model)))
(is (=? {:verified true}
(->> (mirror-rows ds) (filter #(= id-1 (:index_id %))) first))))
(testing "a write that bypassed the hooks entirely is still repaired (self-healing)"
;; Tamper with the mirror behind the reconciler's back.
(jdbc/execute! ds [(format "UPDATE \"%s\" SET content_hash = 'bogus' WHERE index_id = %d"
index-table/*vectors-table* id-2)])
(is (=? {:upserted 1 :deleted 0}
(reconcile/reconcile! ds model))))))
(testing "rows deleted from the appdb are removed from the mirror as orphans"
;; The with-temp rows above are gone now.
(is (=? {:upserted 0 :deleted 2}
(reconcile/reconcile! ds model)))))))
(deftest ^:sequential rebuild-on-model-change-test
(with-isolated-mirror [ds]
(let [model semantic.tu/mock-embedding-model]
(mt/with-temp [:model/CuratedSearchEntry _
{:search_prompt "monthly revenue"
:entity {:model "table" :id 1}}]
(let [appdb-total (t2/count :model/CuratedSearchEntry)]
(testing "populate the mirror under the original model"
(is (=? {:upserted appdb-total} (reconcile/reconcile! ds model))))
(testing "a model-identity change drops the vectors table and the next run re-embeds everything"
(let [new-model (assoc model :model-name "model-v2")]
(is (= :rebuilt (index-table/ensure-tables! ds new-model)))
(is (= [] (mirror-rows ds)))
(is (=? {:upserted appdb-total :deleted 0 :unchanged 0}
(reconcile/reconcile! ds new-model)))
(testing "a schema-version mismatch alone also triggers the rebuild"
;; stale-write the meta row, as if the table were built by an older code version
(jdbc/execute! ds [(format "UPDATE \"%s\" SET schema_version = schema_version - 1"
index-table/*meta-table*)])
(is (= :rebuilt (index-table/ensure-tables! ds new-model)))
(is (= [] (mirror-rows ds))))
(testing "the rebuild heals the meta row, so it doesn't recur on the next sync"
(is (= :ok (index-table/ensure-tables! ds new-model)))))))))))