diff --git a/.dev/docs/atlas/roadmap/mcp-integration-readiness.md b/.dev/docs/atlas/roadmap/mcp-integration-readiness.md index eca57000f..153b88d9c 100644 --- a/.dev/docs/atlas/roadmap/mcp-integration-readiness.md +++ b/.dev/docs/atlas/roadmap/mcp-integration-readiness.md @@ -8,7 +8,7 @@ Six targeted improvements to make Arranger a well-behaved upstream for an MCP se #### SQON generation via `build_sqon` tool [done] -_Shipped 2026-08-10 (#1080), version 1: the scalar operators (`in`, `not-in`, `gt`, `gte`, `lt`, `lte`, `between`) with one `and`/`or` combination per call. Text operators and mixed AND/OR nesting are deliberately deferred; see § Phasing in `.dev/docs/build-sqon-tool.md` for the v2/v3 shape. The design and implementation records are `.dev/docs/build-sqon-tool.md` and `.dev/docs/build-sqon-implementation.md`._ +_Shipped 2026-08-10 (#1080), version 1: the scalar operators (`in`, `not-in`, `gt`, `gte`, `lt`, `lte`, `between`) with one `and`/`or` combination per call. **Version 2 shipped 2026-08-25**, adding `wildcard` text search via `fieldNames` plus `some-not-in` and `all`, which brings the tool to full parity with `modules/sqon` except for the unimplemented `fuzzy`. Still deferred: mixed AND/OR nesting (v3) and `fuzzy` (v2.1, blocked on three things listed in § Phasing). The design and implementation records are `.dev/docs/build-sqon-tool.md` and `.dev/docs/build-sqon-implementation.md`._ _Priority when open: high. Somewhat urgent: MCP SQON generation was hit-or-miss in practice._ @@ -20,11 +20,11 @@ This is also more token-efficient than the alternatives: embedding SQON document **Scope, as built:** -1. **`build_sqon` tool** in `apps/mcp-server/src/mcp/buildSqonTool.ts`, registered from `mcp/tools.ts`: accepts a `catalogueId`, one `combination` (`and`/`or`), a list of `clauses` (`fieldName`, `operator`, `value`, optional `negate`), and an optional `existingSqon`; validates every clause against that catalogue's introspection; folds them with `addFilterClause`; returns the SQON with a plain-English `summary`, a submitted-versus-final clause count, and a note when the two differ. One call carries the whole batch: the originally-scoped one-clause-per-call shape was rejected during design, because a per-clause call turns an N-condition query into N round trips and makes a rejected clause N/2 calls of wasted work on average (see § Why one call builds a whole batch in `.dev/docs/build-sqon-tool.md`). +1. **`build_sqon` tool** in `apps/mcp-server/src/mcp/buildSqonTool.ts`, registered from `mcp/tools.ts`: accepts a `catalogueId`, one `combination` (`and`/`or`), a list of `clauses` (`fieldName` or, for `wildcard`, `fieldNames`; plus `operator`, `value`, optional `negate`), and an optional `existingSqon`; validates every clause against that catalogue's introspection; folds them with `addFilterClause`; returns the SQON with a plain-English `summary`, a submitted-versus-final clause count, and a note when the two differ. One call carries the whole batch: the originally-scoped one-clause-per-call shape was rejected during design, because a per-clause call turns an N-condition query into N round trips and makes a rejected clause N/2 calls of wasted work on average (see § Why one call builds a whole batch in `.dev/docs/build-sqon-tool.md`). 2. **Agent-optimized tool description** [done]: the operator descriptions are generated at module load from `getSqonFieldOperatorDetails()`, so they cannot drift from `modules/sqon`, and are attached per schema branch rather than once for all operators, which keeps the emitted JSON Schema roughly a third smaller than repeating the full list on every branch. The batch-level guidance sits on the `clauses` array, once. -3. **Versioned changelog** [not done]: the tool interface is not versioned and carries no machine-readable changelog. Version 1's operator coverage is described in the tool's own input schema, which is authoritative but not versioned. Revisit if and when a v2 changes the input shape rather than only extending it. +3. **Versioned changelog** [not done]: the tool interface is not versioned and carries no machine-readable changelog. Operator coverage is described in the tool's own input schema, which is authoritative but not versioned. v2 only extended the input shape (a fourth and fifth union branch, and one more operator on an existing branch), so this stayed unnecessary; revisit if a later version changes an existing branch rather than adding one. -**Prerequisite (resolved):** the `build_sqon` tool's operator coverage is bounded by `SqonBuilder`'s. This was previously blocked on absorbing full operator coverage into `modules/sqon`; that absorption is now complete (see [Deprecate `sqon-builder`](#deprecate-sqon-builder)) and `modules/sqon`'s `SqonBuilder` already covers `all`, `between`, `gte`, `in`, `lte`, `not-in`, `some-not-in`, and `wildcard`. `build_sqon` can ship with full operator coverage from the start; no partial-coverage phasing is needed. +**Prerequisite (resolved):** the `build_sqon` tool's operator coverage is bounded by `SqonBuilder`'s. This was previously blocked on absorbing full operator coverage into `modules/sqon`; that absorption is now complete (see [Deprecate `sqon-builder`](#deprecate-sqon-builder)) and `modules/sqon`'s `SqonBuilder` already covers `all`, `between`, `gte`, `in`, `lte`, `not-in`, `some-not-in`, and `wildcard`. In the event v1 did phase anyway, for reasons of MCP-surface design rather than builder coverage: `fieldNames` is a second clause shape, not just another operator, and it was cleaner to ship the single-field shape first. v2 closed the gap on 2026-08-25. `fuzzy` is the one operator still outstanding, and it is outstanding in `modules/sqon` too. **Considered and deferred: TOON as output format.** [TOON (Token-Oriented Object Notation)](https://toonformat.dev) was evaluated as an optional compact output format, both for MCP responses (field listings, search results) and as a potential evolution of the SQON surface syntax itself. The MCP response case has genuine merit: TOON's tabular collapse applies well to uniform arrays like field listings. The SQON syntax case is weaker: SQON's recursive tree structure limits the tabular gains, and the `build_sqon` tool already removes the LLM from the synthesis loop, which was the main pain point. Revisit as an enhancement once the `execute_query` MCP implementation is available and real token budgets can be measured empirically. @@ -73,7 +73,7 @@ _Priority: medium. Recurring gap logged as a session open thread many times over **Scope:** - A dedicated `docs/reference/` page (or an extension of `mcp-server.md`) covering the full MCP tool surface: what each tool does, its input/output shape, and the elicitation-confirmation flow in `execute_query`. -- ~~To check once `build_sqon` ships: `docs/concepts.md`'s `fieldName`/`fieldNames` definition~~ done 2026-08-10: the definition now covers both positions, the `content` object and a flat clause-building argument, and the vocabulary table entry matches. +- ~~To check once `build_sqon` ships: `docs/concepts.md`'s `fieldName`/`fieldNames` definition~~ closed 2026-08-25, deliberately without adding the MCP usage. `concepts.md` is general Arranger vocabulary rather than an MCP page, so neither the prose definition nor the vocabulary table mentions `build_sqon`; the tool's flat `fieldName`/`fieldNames` arguments are documented in `docs/mcp-server.md` instead, and they carry the same names precisely so no second definition is needed. Both entries did change on that date to stop describing `fuzzy` as an existing operator. _Coordinate with whichever MCP work lands next; `execute_query` shipped in #1077 and `build_sqon` in #1080, both now named in `docs/mcp-server.md`._ diff --git a/.dev/docs/build-sqon-implementation.md b/.dev/docs/build-sqon-implementation.md index f514bfdbb..ebbfd9aa9 100644 --- a/.dev/docs/build-sqon-implementation.md +++ b/.dev/docs/build-sqon-implementation.md @@ -4,7 +4,7 @@ Step-by-step build order for the `build_sqon` MCP tool, with worked code and a c **Companion to [`build-sqon-tool.md`](build-sqon-tool.md).** That document is the design: why the tool exists, what it does, which drafts were rejected, and the rationale for each resolved choice. This one is the build: file layout, code, checks, tests, and the text surfaces that change when it ships. Where the two disagree, the disagreement is called out inline and the reason given. -**Scope:** v1 only, as phased in the design document. Scalar operators (`in`, `not-in`, `gt`, `gte`, `lt`, `lte`, `between`), one combinator per call, no text search. +**Scope:** v1, as phased in the design document. Scalar operators (`in`, `not-in`, `gt`, `gte`, `lt`, `lte`, `between`), one combinator per call, no text search. **v2 shipped 2026-08-25** and is recorded in § v2 near the end of this document rather than by rewriting the steps above, so the v1 build order stays readable as what it was. Every behavioural claim below was verified against the built `@overture-stack/sqon`, the installed MCP SDK, and current `modules/graphql-router`, `apps/search-server`, and `apps/mcp-server` source, as of `12053878`. See [§ Verified behaviour this plan depends on](#verified-behaviour-this-plan-depends-on). @@ -616,10 +616,12 @@ Remove this once `buildAggregations` is fixed, and leave the docstring pointing ## Step 8: the handler and registration -Order matters. Resolve the catalogue first so nothing else runs against a down catalogue; validate clauses before folding so errors carry clause indices; validate the built SQON after folding so `existing_sqon` problems surface too. +Order matters. Resolve the catalogue first so nothing else runs against a down catalogue; validate **both** inputs, `existingSqon` and the clauses, before folding either, so one response carries every problem with the call; keep a post-fold `validateSqon` as a failsafe for a fold that mangles valid inputs. + +**Corrected 2026-08-12, after review of #1091.** This step originally validated `existingSqon` in two places, neither of them part of the clause batch: a structural check that returned before `validateClauses` ran, and the catalogue check on the folded SQON, reachable only once every clause had passed. Both broke the design document's single-round-trip guarantee for a call carrying an invalid clause alongside an unusable `existingSqon`: the clauses were reported, the model fixed them, and only the resubmission revealed the `existingSqon` problem. `resolveExistingSqon` below returns errors instead of a result, so they merge into the clause list. The catalogue check can move ahead of the fold because a fold never invents a field name or rewrites a leaf's operator, so the two inputs together account for every leaf of the output. ```typescript -async ({ catalogueId, clauses, combination, existing_sqon: rawExistingSqon }) => { +async ({ catalogueId, clauses, combination, existingSqon: rawExistingSqon }) => { try { const resolution = await resolveCatalogue(client, config, catalogueId); if ('error' in resolution) { @@ -628,39 +630,28 @@ async ({ catalogueId, clauses, combination, existing_sqon: rawExistingSqon }) => const { fields, operators } = resolution.introspection; const context: CatalogueQueryContext = { fields, operators }; - let existingSqon: SqonNode | undefined; - if (rawExistingSqon !== undefined) { - const parsed = SqonSchema.safeParse(rawExistingSqon); - if (!parsed.success) { - const issues = parsed.error.issues.map( - (issue) => `- at ${issue.path.join('.') || 'root'}: ${issue.message}`, - ); - return errorResult( - `existing_sqon is not a valid SQON. Pass the "sqon" value from an earlier build_sqon response unchanged, or omit existing_sqon to start a new query.\n${issues.join('\n')}`, - ); - } - existingSqon = normalizeSqonNode(parsed.data); - } - - const clauseErrors = validateClauses(clauses, context); - if (clauseErrors.length > 0) { + // `existingSqon` errors lead, since a base query built for another catalogue has to be + // dropped before the clause fixes are worth making. + const existing = resolveExistingSqon(rawExistingSqon, context); + const errors = [...existing.errors, ...validateClauses(clauses, context)]; + if (errors.length > 0) { return errorResult( - `No SQON was built. Fix every clause listed, then resubmit the whole batch:\n${clauseErrors.join('\n')}`, + composeValidationError({ catalogueId, catalogueMismatch: existing.catalogueMismatch, errors }), ); } - const sqon = normalizeRoot(foldClauses({ clauses, combination, existingSqon })); + const sqon = normalizeRoot(foldClauses({ clauses, combination, existingSqon: existing.sqon })); - // Catches what the input schema cannot: fields referenced by existing_sqon that this - // catalogue does not have, and any structural surprise from the fold itself. + // A failsafe, not a user-facing check: both inputs were already validated against the + // catalogue, so a failure here is a defect in the fold rather than a fixable request. const validation = validateSqon(sqon, context); if (!validation.valid) { return errorResult( - `The clauses were valid individually, but the resulting SQON is not:\n- ${validation.errors.join('\n- ')}\nIf existing_sqon came from a different catalogue, rebuild the query for "${catalogueId}" instead of extending it.`, + `build_sqon combined valid inputs into an invalid SQON, so nothing was returned:\n- ${validation.errors.join('\n- ')}\nThis is a defect in the tool, not in the request: resubmitting the same inputs will not help. Tell the user what happened rather than retrying.`, ); } - const submittedCount = clauses.length + (existingSqon ? countFilterClauses(existingSqon) : 0); + const submittedCount = clauses.length + (existing.sqon ? countFilterClauses(existing.sqon) : 0); const filterCount = countFilterClauses(sqon); const notes = filterCount < submittedCount @@ -684,7 +675,41 @@ async ({ catalogueId, clauses, combination, existing_sqon: rawExistingSqon }) => Copy `errorResult` and `successResult` from [`executeQueryTool.ts`](../../apps/mcp-server/src/mcp/executeQueryTool.ts). An `isError: true` result skips output-schema validation in the SDK, so an error result correctly needs no `structuredContent`; a success result must always carry it once `outputSchema` is declared. -Do not route a bad `existing_sqon` through `describeExecutionError`. It maps every `ZodError` to "Arranger returned a response that did not match the expected introspection schema … indicates an Arranger version mismatch," which is wrong and unactionable for caller-supplied input. Parsing with `SqonSchema.safeParse` instead of letting `SqonBuilder.from()` throw keeps that path controlled. `SqonSchema` is already exported, so this needs no new export from `modules/sqon`. +Do not route a bad `existingSqon` through `describeExecutionError`. It maps every `ZodError` to "Arranger returned a response that did not match the expected introspection schema … indicates an Arranger version mismatch," which is wrong and unactionable for caller-supplied input. Parsing with `SqonSchema.safeParse` instead of letting `SqonBuilder.from()` throw keeps that path controlled. `SqonSchema` is already exported, so this needs no new export from `modules/sqon`. + +The handler leans on two helpers of its own, `resolveExistingSqon` and `composeValidationError`, plus one new export from `queryValidation.ts` that the first of them calls. All three are described below. + +`resolveExistingSqon` returns `{ sqon?, errors, catalogueMismatch }` rather than an early result, which is what lets its errors travel with the clause errors; a structural failure short-circuits the catalogue walk, since there is no tree to walk, but is still returned rather than thrown. `sqon` is absent whenever `errors` is non-empty, and the handler must not fold a resolution that carries errors: + +```typescript +const resolveExistingSqon = (raw: unknown, context: CatalogueQueryContext): ExistingSqonResolution => { + if (raw === undefined) { + return { catalogueMismatch: false, errors: [] }; + } + + const parsed = SqonSchema.safeParse(raw); + if (!parsed.success) { + const issues = parsed.error.issues.map((issue) => ` - at ${issue.path.join('.') || 'root'}: ${issue.message}`); + return { + catalogueMismatch: false, + errors: [ + `existingSqon is not a valid SQON. Pass the "sqon" value from an earlier build_sqon response unchanged, or omit existingSqon to start a new query.\n${issues.join('\n')}`, + ], + }; + } + + const sqon = normalizeSqonNode(parsed.data); + // Normalized first, so an operator alias in an existing SQON is checked in its canonical form + // rather than rejected as an operator the catalogue does not advertise. + const errors = validateSqonFields(sqon, context, { subject: 'existingSqon' }); + + return errors.length > 0 ? { catalogueMismatch: true, errors } : { catalogueMismatch: false, sqon, errors }; +}; +``` + +`validateSqonFields` is a new export from `arranger/queryValidation.ts`: the semantic half of `validateSqon`, taking an already-parsed `SqonNode` and returning a plain error list, with a `subject` option naming the input under validation (`existingSqon` here, defaulting to `SQON` so `execute_query`'s messages are unchanged). `validateSqon` now delegates its own walk to it, so there is one implementation of the field-and-operator rules, not two. + +`composeValidationError` assembles the one message: the `No SQON was built. Fix everything listed, then resubmit the whole batch:` header, the errors in list order, and the `If existingSqon came from a different catalogue, drop it and rebuild the query for "".` line. That last line is appended only for the catalogue-mismatch case, which is why `resolveExistingSqon` returns a `catalogueMismatch` flag rather than the caller inferring it from a non-empty `errors`: the advice is misdirection both when only the clauses are at fault and when `existingSqon` is not a SQON at all, since that message already says to pass the previous `sqon` back unchanged or omit it. Registration takes `config` as well as `client` from `deps`: @@ -726,16 +751,18 @@ npm run test -w apps/mcp-server There is no `executeQueryTool.test.ts`, so the pattern to follow is: keep the handler thin, export the logic, and test the exports. -| Target | Cases | -| -------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `describeOperators` | for each branch group, every operator in that group gets a line, and no operator outside it appears; across all three, every member of `BUILD_SQON_OPERATORS` is described exactly once and no alias or out-of-scope operator (`all`, `some-not-in`, `wildcard`, `fuzzy`) appears anywhere. This is what catches both a new operator landing in `modules/sqon` without a decision here, and a branch enum value left undescribed | -| `clauseSchema` | accepts `in` with an array and with a bare scalar; rejects `gt` with an array, `between` with one or three values, `in` with `[]`, any text operator, and any alias operator (`>=`, `=`) | -| `validateClauses` | unknown field; operator invalid for the field type; `negate` with `not-in`; quoted bound on a numeric field; accepted quoted bound on a `date` field; descending `between`; two invalid clauses in one batch both reported with correct indices; valid batch returns `[]` | -| `resolveCatalogue` | id outside `config.catalogues` returns an error and makes **no** client call; 404 returns the not-on-server message; a `{status:'failed', error:{code,message}}` body returns Arranger's own code and message and never reaches `catalogueIntrospectionSchema`; a healthy body parses | -| `summarizeSqon` | each of the ten operators; a `not` wrapper; a nested group parenthesized; a bare leaf; `{op:'and',content:[]}`; a single-child `and` renders without parentheses; display name preferred over field name; unknown field falls back to the field name | -| `countFilterClauses` | leaf, flat group, nested group, empty group, single-child group | -| `foldClauses` | three clauses under `and` and under `or`; single clause returns a bare leaf; negated single clause returns a root `not`; `gt 50` then `gt 70` under `and` reduces to one `gt 70`; folding onto an `existing_sqon` | -| `normalizeRoot` | a leaf is wrapped in `{op:'and',content:[leaf]}`; a group is returned unchanged; a root `not` is returned unchanged; wrapping is idempotent | +| Target | Cases | +| ----------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `describeOperators` | for each branch group, every operator in that group gets a line, and no operator outside it appears; across all three, every member of `BUILD_SQON_OPERATORS` is described exactly once and no alias or out-of-scope operator (`all`, `some-not-in`, `wildcard`, `fuzzy`) appears anywhere. This is what catches both a new operator landing in `modules/sqon` without a decision here, and a branch enum value left undescribed | +| `clauseSchema` | accepts `in` with an array and with a bare scalar; rejects `gt` with an array, `between` with one or three values, `in` with `[]`, any text operator, and any alias operator (`>=`, `=`) | +| `validateClauses` | unknown field; operator invalid for the field type; `negate` with `not-in`; quoted bound on a numeric field; accepted quoted bound on a `date` field; descending `between`; two invalid clauses in one batch both reported with correct indices; valid batch returns `[]` | +| `resolveCatalogue` | id outside `config.catalogues` returns an error and makes **no** client call; 404 returns the not-on-server message; a `{status:'failed', error:{code,message}}` body returns Arranger's own code and message and never reaches `catalogueIntrospectionSchema`; a healthy body parses | +| `summarizeSqon` | each of the ten operators; a `not` wrapper; a nested group parenthesized; a bare leaf; `{op:'and',content:[]}`; a single-child `and` renders without parentheses; display name preferred over field name; unknown field falls back to the field name | +| `countFilterClauses` | leaf, flat group, nested group, empty group, single-child group | +| `foldClauses` | three clauses under `and` and under `or`; single clause returns a bare leaf; negated single clause returns a root `not`; `gt 50` then `gt 70` under `and` reduces to one `gt 70`; folding onto an `existingSqon` | +| `validateSqonFields` | default subject reads `SQON ...`, matching what `validateSqon` reported before the split; a given subject reaches both the unknown-field and the invalid-operator message; one error per invalid leaf across nested combinations; a valid SQON returns `[]` | +| `existingSqon` batching | an `existingSqon` from another catalogue reported alongside an invalid clause in one response, `existingSqon` first; a structurally invalid `existingSqon` reported alongside an invalid clause rather than instead of it; an `existingSqon` operator that does not fit the field it names; the rebuild advice withheld when only the clauses are at fault; an operator alias in `existingSqon` still accepted | +| `normalizeRoot` | a leaf is wrapped in `{op:'and',content:[leaf]}`; a group is returned unchanged; a root `not` is returned unchanged; wrapping is idempotent | `resolveCatalogue`'s tests need a stub `ArrangerClient`. The workspace test script already runs with `--experimental-test-module-mocks`, and `arranger/validation.test.ts` is the existing precedent for faking the client. @@ -834,15 +861,19 @@ The tool-name and `catalogueId` corrections from the previous revision are appli 1. **The alias-normalization claim is wrong.** § Things to know about `reduceSqon` says normalization "runs inside `SqonBuilder.from()`/`addFilterClause` regardless of which spelling comes in, so the built SQON is identical either way." `addFilterClause` does not normalize: it dispatches on the literal operator string through a switch with no default, so `{operator: '>='}` returns `undefined`. Only `SqonBuilder.from()` normalizes, via `normalizeSqonNode`. The canonical-only decision is right, and stronger than the document argues: an alias would not produce an equivalent SQON, it would drop the clause. 2. **The v3 `reduceSqon` question is answered.** § Phasing marks "nesting an `and` branch under a new `or` should not get flattened away" as needing a test. It does not get flattened: `reduceSqon` only flattens an inner group when `inner.op === output.op`, verified. -3. **v1 needs no `fieldName`/`fieldNames` mutual exclusion.** The Zod sample carries a `.refine()` for it, and a paragraph on why `discriminatedUnion` cannot express it. With no text operators in v1 there is no `fieldNames` yet, so the whole problem is v2's. When it arrives, use value checks (`clause.fieldName !== undefined`), not `'fieldName' in clause`: Zod 3 keeps an explicitly-present `undefined` key, so `in` gives the wrong answer. -4. **`TextOperatorSchema` must not ship.** The sample input schema includes `zod.enum(['wildcard', 'fuzzy'])`. `fuzzy` has no implementation, and a `fuzzy` clause with `fieldNames` silently becomes a `wildcard` clause. +3. **No `fieldName`/`fieldNames` mutual exclusion is needed at all** (updated 2026-08-25, once v2 arrived). The Zod sample carries a `.refine()` for it, and a paragraph on why `discriminatedUnion` cannot express it. This revision predicted the problem would land in v2 and that value checks (`clause.fieldName !== undefined`) would be needed rather than `'fieldName' in clause`. Neither turned out to be true: because the shipped schema discriminates on `operator`, the `wildcard` branch simply has no `fieldName` key and every other branch has no `fieldNames` key, so the split is structural and no refinement exists. The Zod 3 caveat still holds for any key-presence test written elsewhere, which is why `foldClauses` dispatches on `clause.operator` rather than on `'fieldNames' in clause`. +4. **`TextOperatorSchema` must not ship.** The sample input schema includes `zod.enum(['wildcard', 'fuzzy'])`. `wildcard` shipped alone in v2; `fuzzy` stays out until it exists, because `addFilterClause`'s text branch ignores `operator` and builds a `wildcard` clause from a `fuzzy` request with no error. 5. **Step 5 of Implementation guidance is now incomplete.** It ends at "build the `summary` string from the final SQON, and return `{ sqon, summary }`." The output also needs the root normalization of step 7 above, and the reduction note when `filterCount` is lower than what was submitted. --- -## Open question +## Open question, resolved 2026-08-25 + +graphql-router's `opSwitch` gives `in`-like values magic meanings: a value containing `*` becomes a regex query, a `set_id:` prefix becomes a set lookup, and `__missing__` becomes a missing-field filter. v1 passed all three straight through, so a model reaching for substring search would put `*TP53*` in an `in` value and it would quietly work as a regex. + +**Resolved: `validateClause` rejects it and points at `wildcard`.** With v2 shipping a real text operator there is a correct way to express the intent, and offering two spellings for substring search reintroduces the ambiguity this tool exists to remove. More importantly the regex behaviour is invisible: the model asked for an exact match, got a pattern match, and nothing in the result says so. -graphql-router's `opSwitch` gives `in`-like values magic meanings: a value containing `*` becomes a regex query, a `set_id:` prefix becomes a set lookup, and `__missing__` becomes a missing-field filter. `build_sqon` passes all three straight through. Since v1 has no `wildcard`, a model reaching for substring search will put `*TP53*` in an `in` value and it will quietly work as a regex. Decide whether that is a documented feature of the tool, something to reject in `validateClause`, or something to leave undocumented and working. +The check covers `in`, `not-in`, `some-not-in`, and `all`, and inspects every value rather than only the first, unlike `opSwitch`, which tests `value[0]`. `set_id:` and `__missing__` are untouched, since neither contains an asterisk. `execute_query`'s raw `sqon` parameter is also untouched, so the regex path stays reachable for a client that wants it. That asymmetry is deliberate and documented in `docs/mcp-server.md`: a keyword value that genuinely contains an asterisk is reachable through `execute_query` but not through `build_sqon`. --- @@ -860,8 +891,9 @@ graphql-router's `opSwitch` gives `in`-like values magic meanings: a value conta - [x] `resolveCatalogue`: `status: 'failed'` short-circuited on the raw response, before Zod parsing - [x] `mcp/buildSqonTool.ts`: `foldClauses`, with the `undefined` guard - [x] `mcp/buildSqonTool.ts`: `normalizeRoot`, applied on output only, with the removal condition documented -- [x] `mcp/buildSqonTool.ts`: handler, in the order catalogue, existing_sqon, clauses, fold, `validateSqon` -- [x] `existing_sqon` parsed with `SqonSchema.safeParse`, not via a thrown `ZodError` +- [x] `mcp/buildSqonTool.ts`: handler, in the order catalogue, then `existingSqon` and clauses validated together into one error list, fold, `validateSqon` as a failsafe (reordered 2026-08-12; the original order let clause errors mask an `existingSqon` mismatch until a second call) +- [x] `existingSqon` parsed with `SqonSchema.safeParse`, not via a thrown `ZodError` +- [x] `arranger/queryValidation.ts`: `validateSqonFields` split out of `validateSqon` so an already-parsed node can be checked against the catalogue, with a `subject` naming the input in each message - [x] `registerBuildSqonTool` wired into `mcp/tools.ts`, taking `config` as well as `client` - [x] No elicitation in this tool - [ ] Decide whether `get_catalogue_fields` gets the same allowlist check in this pass @@ -871,6 +903,8 @@ graphql-router's `opSwitch` gives `in`-like values magic meanings: a value conta - [x] `mcp/buildSqonTool.test.ts`: description generation, schema accept/reject, fold shapes, `normalizeRoot` - [x] `mcp/buildSqonTool.test.ts`: `resolveCatalogue` across all four cases, with a stubbed client - [x] `arranger/clauseValidation.test.ts`: every validation branch, plus a multi-error batch +- [x] `arranger/queryValidation.test.ts`: `validateSqonFields`, including the default subject matching the pre-split wording +- [x] `mcp/buildSqonTool.test.ts`: an unusable `existingSqon` and an invalid clause reported in one response, in that order, for both the structural and the wrong-catalogue case - [x] `arranger/sqonSummary.test.ts`: every operator, negation, nesting, empty SQON, display names - [x] Fold shapes asserted through the registered handler rather than by exporting `foldClauses`/`normalizeRoot`; the bare-leaf-then-wrap behaviour is covered by the single-clause and empty-`existingSqon` cases - [x] `npm run test -w apps/mcp-server` passes from the monorepo root (189 tests) @@ -902,4 +936,52 @@ graphql-router's `opSwitch` gives `in`-like values magic meanings: a value conta **Still open** -- [ ] `*`, `set_id:`, and `__missing__` in `in` values: documented, rejected, or left alone +- [x] `*`, `set_id:`, and `__missing__` in `in` values: rejected for `*`, untouched for the other two (see § Open question, resolved) + +--- + +## v2, shipped 2026-08-25 + +`wildcard` text search plus `some-not-in` and `all`. No `modules/sqon` change was needed, which is what splitting `fuzzy` out into v2.1 bought. See the design document's § Phasing for why the split happened and what blocks v2.1. + +**What changed, by file** + +| File | Change | +| ------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `mcp/buildSqonTool.ts` | `some-not-in` into the in-like group; new `all` and `wildcard` union branches; `describeOperators` no longer names field types for an unrestricted operator; shape-dispatched fold; missing-asterisk note | +| `arranger/clauseValidation.ts` | clause input becomes a union; per-entry `fieldNames` validation reported as one message per clause; asterisk rejection for term-matched operators | +| `arranger/queryValidation.ts` | `checkFieldOperator` extracted as the shared lookup-and-verdict, returning a typed reason so each caller keeps its own wording | +| `arranger/sqonSummary.ts` | `fieldNames` rendered as display names joined with "or", matching the any-field-matches semantics | + +**Four things worth knowing before touching this again** + +- **The `all` branch is load-bearing, not defensive.** Measured: `addFilterClause({fieldName:'t', operator:'all', value:'x'})` builds `{"op":"all","content":{"fieldName":"t","value":"x"}}` without complaint, and `SqonSchema.safeParse` then rejects it. Without an array-only branch the tool would build an invalid SQON and only discover it at the post-fold failsafe. +- **A text clause reports one message, however many of its fields are bad.** One clause is one condition, so `validateFieldNames` joins every failing field into a single `clauses[i]: ` message. Splitting them would read as several broken clauses. +- **The extracted check stops at the verdict, deliberately.** `validateFilterClause` emits whole sentences led by a subject (`existingSqon references unknown field ...`), while `validateClause` returns lowercase fragments completing a `clauses[i]: ` prefix. Pushing the wording into `checkFieldOperator` would have changed `execute_query`'s existing error text and broken the default-subject assertions in `queryValidation.test.ts`. +- **The fold dispatches on `operator`, not on key presence.** `addFilterClause` is overloaded, so a union-typed argument does not compile; each branch passes its own object literal, still checked against its own overload. Dispatching on `'fieldNames' in clause` would be wrong for the Zod 3 reason in correction 3 above. + +**Numbers** + +| Measurement | before v2 | after v2 | +| ------------------------------------ | --------- | -------- | +| Input schema characters | 3805 | 5799 | +| Internal `$ref`s | 0 | 0 | +| `apps/mcp-server` unit tests | 199 | 249 | +| `integration-tests/mcp-server` tests | 69 | 80 | + +The unit-test baseline is 199 rather than the 189 recorded for v1 above, because `#1091`'s `existingSqon` batching work added tests between the two. + +The schema grew by two union branches plus the wildcard value description, which has to explain that `*` is required for a substring search. Tightening `describeOperators` to stop repeating a field-type pointer on every unrestricted operator recovered 310 of those characters. + +**Checklist** + +- [x] `wildcard` branch with `fieldNames`, `all` branch with an array-only value, `some-not-in` on the in-like branch +- [x] `fuzzy` withheld, with all three blockers recorded in the design document +- [x] `describeOperators` claims no field types for an operator `modules/sqon` does not restrict +- [x] `checkFieldOperator` extracted; `execute_query`'s error text unchanged +- [x] Asterisk rejected in term-matched values; `set_id:`/`__missing__` and the raw-`sqon` path untouched +- [x] Summary joins `fieldNames` display names with "or" +- [x] Missing-asterisk `notes` entry +- [x] `npm run test -w apps/mcp-server` (249), `npm run test:dev` (868 across five workspaces), `tsc --noEmit`, `prettier --check` +- [x] `integration-tests/mcp-server` run against real Arranger and Elasticsearch (80), including substring matching, any-field-matches, negation, and the aggregations path +- [x] `docs/mcp-server.md`, `apps/mcp-server/README.md`, `CHANGELOG.md`, `docs/concepts.md`, both `.dev` design documents, roadmap, tech-debt, session file diff --git a/.dev/docs/build-sqon-tool.md b/.dev/docs/build-sqon-tool.md index e5c9ddece..1e20ccdf0 100644 --- a/.dev/docs/build-sqon-tool.md +++ b/.dev/docs/build-sqon-tool.md @@ -149,7 +149,7 @@ build_sqon(input: { existingSqon?: SqonNode, clauses: Array<{ fieldName?: string, // for scalar operators - fieldNames?: string[], // for text operators (wildcard, fuzzy); mutually exclusive with fieldName + fieldNames?: string[], // for wildcard text search; mutually exclusive with fieldName operator: ScalarOperator | TextOperator, value: SqonScalar | SqonScalar[] | string, negate?: boolean, @@ -190,6 +190,8 @@ Batching wins even when something goes wrong. Rejecting a batch and fixing it is **What batching requires the tool to do in return:** check every clause before responding, and report every invalid one in the same error message, not just the first. If the tool stopped at the first bad clause, the LLM would fix it, resubmit, and only then discover a second problem, costing the exact round trip batching was meant to remove. +**This obligation covers the whole input, not only `clauses`** (corrected 2026-08-12, after review of #1091). The shipped tool originally validated `existingSqon` in two places that both sat outside the clause batch: a structural check that returned before `validateClauses` ran, and a catalogue check on the folded SQON that ran only after `validateClauses` had passed. A call carrying both an invalid clause and an `existingSqon` this catalogue cannot run therefore reported only the clauses, and the mismatch surfaced on the next call, which is the round trip this section exists to prevent. `existingSqon` is now validated against the catalogue independently of the fold and its errors join the same list, so one response carries every problem with the call. See [Error handling](#error-handling). + ### Why `build_sqon` needs a `catalogueId` **The choice:** whether `build_sqon` takes a `catalogueId` at all, or builds purely from what's in the `clauses` array. @@ -204,23 +206,29 @@ Batching wins even when something goes wrong. Rejecting a batch and fixing it is ### Operator reference -| Operator | v1 scope | sqon ready | Shape | Field property | Value type | ES/OS translation | -| ------------- | -------- | ---------- | ------ | -------------- | --------------------------------- | ------------------------------------- | -| `in` | yes | yes | Scalar | `fieldName` | `(string \| number \| boolean)[]` | `terms` query | -| `not-in` | yes | yes | Scalar | `fieldName` | `(string \| number \| boolean)[]` | `bool.must_not.terms` | -| `gt` | yes | yes | Scalar | `fieldName` | `number` | `range.gt` | -| `gte` | yes | yes | Scalar | `fieldName` | `number` | `range.gte` | -| `lt` | yes | yes | Scalar | `fieldName` | `number` | `range.lt` | -| `lte` | yes | yes | Scalar | `fieldName` | `number` | `range.lte` | -| `between` | yes | yes | Scalar | `fieldName` | `[number, number]` | `range.gte` + `range.lte` | -| `some-not-in` | no | yes | Scalar | `fieldName` | `(string \| number \| boolean)[]` | nested `bool.must_not` per value | -| `all` | no | yes | Scalar | `fieldName` | `(string \| number \| boolean)[]` | `bool.must` per value (all required) | -| `wildcard` | no | yes | Text | `fieldNames` | `string` | `multi_match` with wildcard | -| `fuzzy` | no | **no** | Text | `fieldNames` | `string` | `multi_match` with `fuzziness:"AUTO"` | - -- `some-not-in` and `all` already work in `modules/sqon`, but are out of scope for v1. Add them alongside v2, or as a separate v1.x. -- `wildcard` already works too. It waits for v2 only because v2 is what introduces the `fieldNames` shape. -- `fuzzy` has no implementation yet. See the fuzzy operator roadmap item. Do not add it here until that is done. +| Operator | tool scope | sqon ready | Shape | Field property | Value type | ES/OS translation | +| ------------- | ---------- | ---------- | ------ | -------------- | --------------------------------- | ------------------------------------- | +| `in` | v1 | yes | Scalar | `fieldName` | `(string \| number \| boolean)[]` | `terms` query | +| `not-in` | v1 | yes | Scalar | `fieldName` | `(string \| number \| boolean)[]` | `bool.must_not.terms` | +| `gt` | v1 | yes | Scalar | `fieldName` | `number` | `range.gt` | +| `gte` | v1 | yes | Scalar | `fieldName` | `number` | `range.gte` | +| `lt` | v1 | yes | Scalar | `fieldName` | `number` | `range.lt` | +| `lte` | v1 | yes | Scalar | `fieldName` | `number` | `range.lte` | +| `between` | v1 | yes | Scalar | `fieldName` | `[number, number]` | `range.gte` + `range.lte` | +| `some-not-in` | v2 | yes | Scalar | `fieldName` | `(string \| number \| boolean)[]` | nested `bool.must_not` per value | +| `all` | v2 | yes | Scalar | `fieldName` | `(string \| number \| boolean)[]` | `bool.must` per value (all required) | +| `wildcard` | v2 | yes | Text | `fieldNames` | `string` | one `wildcard` query per field, OR'd | +| `fuzzy` | v2.1 | **no** | Text | `fieldNames` | `string` | `multi_match` with `fuzziness:"AUTO"` | + +- `some-not-in`, `all`, and `wildcard` shipped with v2 (2026-08-25). `wildcard` waited only because v2 is what introduces the `fieldNames` shape. +- **`wildcard`'s ES translation is not a `multi_match`**, as an earlier revision of this table claimed. `getWildcardFilter` in `graphql-router` emits one ES `wildcard` query per field name, with `case_insensitive: true`, grouped by nesting level and combined under a `should`. So a clause matches when any one of its fields matches, and the value is compared against the whole field value: a value carrying no `*` finds an exact term rather than a substring. `build_sqon` returns a `notes` entry when that happens, since the difference is invisible in the result. +- `fuzzy` has no implementation yet. See the fuzzy operator roadmap item and § v2.1 below. Do not add it here until that is done. + +### Why the operator description cannot name field types + +`getSqonFieldOperatorDetails()` reports `applicableTo: 'all'` for `in`, `not-in`, `some-not-in`, `all`, and `wildcard`. A catalogue disagrees: `getValidFieldOperators` in `buildCatalogueIntrospection.ts` gives range-typed fields `['in','not-in','gt','gte','lt','lte','between']`, enum-like fields `['in','not-in','some-not-in','all','filter']`, and everything else `['in','not-in','filter']`. So `wildcard` is withheld from numeric and date fields, and `all` and `some-not-in` from those plus text fields. + +The catalogue is the authority, because `validateClauses` enforces it. Rendering `applicableTo: 'all'` as "any field type" would therefore advertise a clause the tool then rejects. `describeOperators` says nothing about field types for such an operator instead, and the `clauses` array description names `get_catalogue_fields` as the authority once. Copying graphql-router's type classification into `apps/mcp-server` was rejected: the repo already carries tech debt for duplicated transforms, and one more copy to drift is worse than a pointer. The `applicableTo` inaccuracy in `modules/sqon` is tracked separately, since correcting it changes the published `get_sqon_schema` contract. The tool's operator descriptions are generated from `getSqonFieldOperatorDetails()` at server startup, not hand-written. Adding an operator to `modules/sqon` updates the tool's description automatically. @@ -239,7 +247,7 @@ If any clause is invalid (wrong operator for its shape, wrong value type, double ``` clauses[1]: invalid operator "gt" for a text-search item (fieldNames provided). -Text-search operators: wildcard, fuzzy. Use fieldName (singular) for a scalar +Text-search operators: wildcard. Use fieldName (singular) for a scalar operator instead. clauses[3]: "not-in" already means "not equal to." Combining it with negate: true @@ -249,6 +257,17 @@ the value instead of excluding it. Nothing is applied until every clause passes. Partial success is not a state this tool has to handle. +**`existingSqon` is checked in the same pass, and reported in the same message.** Both ways it can be unusable, a value that is not a SQON at all and a SQON naming fields the target catalogue does not have, produce entries in the same list as the clause errors, ahead of them: + +``` +No SQON was built. Fix everything listed, then resubmit the whole batch: +existingSqon references unknown field "file.size". Use get_catalogue_fields to list valid fields. +clauses[0]: operator "gt" is not valid for field "donor.sex" (type "keyword"). ... +If existingSqon came from a different catalogue, drop it and rebuild the query for "participants". +``` + +`existingSqon` leads because a base query built for another catalogue has to be dropped before the clause fixes are worth making. The rebuild advice is appended only for the catalogue-mismatch case: an `existingSqon` that is not a SQON at all carries its own remedy in its own message, and asking which catalogue it came from would point at the wrong thing. The catalogue check runs on `existingSqon` directly rather than on the folded result, which it can do because a fold never invents a field name or rewrites a leaf's operator: every leaf of the output comes from either `existingSqon` or a clause, so validating the two inputs separately catches everything validating the output would, one round trip earlier. The post-fold `validateSqon` call remains as a failsafe for a fold that mangles valid inputs, and says so: reaching it is a defect in the tool, not a fixable request. + --- ## Phasing @@ -259,10 +278,25 @@ Nothing is applied until every clause passes. Partial success is not a state thi - Each clause: `fieldName`, `operator`, `value`, optional `negate` - One `combination` (`and` or `or`) for the whole batch, not mixed -### v2: text search +### v2: text search and the remaining set-membership operators (shipped 2026-08-25) + +**Split from what this section originally planned.** It read "add `fieldNames` as an alternative to `fieldName`, for `wildcard` and `fuzzy`," then noted that this "needs the fuzzy operator to exist in `modules/sqon` first." Those two sentences contradict each other: they block the half that was ready behind the half that has no implementation and an unresolved design question. `wildcard` shipped on its own instead, and `fuzzy` became v2.1 below. + +- `fieldNames` (plural) added as a fourth clause shape, for `wildcard` +- `some-not-in` and `all` added, closing the gap between what `modules/sqon` implements and what the tool exposes. `all` needs its own union branch: `AllFilterSchema` requires an array, and `addFilterClause` builds an `all` clause from a bare scalar without complaint while `SqonSchema` then rejects the result +- An asterisk inside an `in`, `not-in`, `some-not-in`, or `all` value is now rejected and redirected to `wildcard`, which resolves the open question this document previously carried about `*` in in-like values + +**No mutual-exclusion refinement was needed**, contrary to what § Implementation guidance predicted. The shipped input schema is a `discriminatedUnion` on `operator`, so the `wildcard` branch simply has no `fieldName` key and every other branch has no `fieldNames` key. The split is structural, and a clause sending the wrong one for its operator fails to match any branch. -- Add `fieldNames` as an alternative to `fieldName`, for `wildcard` and `fuzzy` -- Needs the fuzzy operator to exist in `modules/sqon` first; `wildcard` is ready now +### v2.1: fuzzy text search + +Blocked on three things, not one. Worth listing, because the first is the only one this document previously named: + +1. **The operator does not exist in `modules/sqon`.** No `FuzzyFilterSchema`, no `SqonBuilder.fuzzy()`, and `opSwitch` in `graphql-router` throws `unknown op` for it. +2. **An unresolved design question.** Whether `fuzzy` should tolerate leading-term fuzziness only (`operator: "AND"`) or any-term matching (`operator: "OR"`). See the fuzzy operator roadmap item. +3. **`addFilterClause`'s text branch ignores `operator` entirely** (`filter.ts`: `'fieldNames' in params ? SqonBuilder.wildcard(...) : buildScalarClause(...)`). Measured: `addFilterClause({fieldNames: ['a','b'], operator: 'fuzzy', value: 'jon'})` returns a **`wildcard`** clause with no error. Once `fuzzy` exists, both text operators share the plural shape, so no dispatch in `apps/mcp-server` can separate them and the fix has to happen in `modules/sqon`. This also means the `undefined` guard in `foldClauses` was never "the tripwire for v2's text operators" that this document and the implementation plan both called it: that guard only fires when a text operator arrives with a singular `fieldName` and falls off the scalar switch, which the plural shape never does. Corrected in both documents and in the code comment. + +Nothing in v2 depends on any of this, which is the point of the split. ### v3: nested combinations (AND and OR mixed in one query) @@ -289,12 +323,14 @@ SQON already supports this structurally: a combination node's children can be le - `gt`/`gte` keeps the larger value under `and`/`not`, the smaller under `or`. - `lt`/`lte` is the mirror image. - `between` never merges. + - `wildcard` never merges either, so two text searches on the same fields stay as two clauses. + - Range bounds compare numerically, by parsed timestamp when both are date strings, or lexicographically when both are strings that do not parse as dates. Two bounds with no ordering between them (a boolean, an array, or one of each type) are kept as separate clauses. Fixed 2026-08-25: date bounds previously went through `Math.max`/`Math.min`, yielding `NaN` and serializing to `null`, so an ordinary date narrowing produced a filter with no bound. - Example: two clauses for "age > 50" and "age > 70" under `and` come back as one `gt: 70` clause, not two. Only the `summary` string shows this happened. - **A group with one item gets unwrapped**, and an empty group gets dropped, unless it carries a `pivot`. - **`not` groups never get flattened into a parent group.** - **`pivot`**, an optional field on every node, blocks the two rules above. It already exists in the schema. No tool sets it yet; v3 needs to decide whether `build_sqon`/`combine_sqons` ever should. - **Only `and`/`or`/`not` exist as combinators.** There is no `xor`. -- **Symbol aliases exist** (`=`, `>=`, and similar) and get normalized before validation, but **only by `SqonBuilder.from()`, not by `addFilterClause`** (corrected 2026-08-10, measured). `addFilterClause` dispatches on the literal operator string through a switch with no default, so `{operator: '>='}` returns `undefined`: an alias does not build an equivalent clause, it drops the clause entirely. **The choice for `build_sqon`'s input schema, resolved:** canonical operator names only (`in`, `not-in`, `gt`, ...); the `operator` enum does not list `=`, `>=`, or any other alias. **The alternative considered:** also listing aliases as valid enum values, on the theory that a model biased by training data toward symbol operators would otherwise get rejected and need a retry. **Why canonical-only was chosen instead:** offering two spellings for the same operator reintroduces the exact ambiguity this tool exists to remove. The correction above makes the case stronger than it originally read here: an alias reaching the fold would silently produce a SQON missing that condition, not an equivalent one, so the enum is load-bearing rather than merely tidy. `foldClauses` keeps an `undefined` guard behind it as a failsafe for v2. Aliases stay relevant only for the raw-SQON paths (`execute_query`'s `sqon` parameter, `SqonSchema.parse()` called directly), which are different consumers with different constraints; `existingSqon` is normalized on the way in for the same reason, since it arrives through `SqonBuilder.from()`. +- **Symbol aliases exist** (`=`, `>=`, and similar) and get normalized before validation, but **only by `SqonBuilder.from()`, not by `addFilterClause`** (corrected 2026-08-10, measured). `addFilterClause` dispatches on the literal operator string through a switch with no default, so `{operator: '>='}` returns `undefined`: an alias does not build an equivalent clause, it drops the clause entirely. **The choice for `build_sqon`'s input schema, resolved:** canonical operator names only (`in`, `not-in`, `gt`, ...); the `operator` enum does not list `=`, `>=`, or any other alias. **The alternative considered:** also listing aliases as valid enum values, on the theory that a model biased by training data toward symbol operators would otherwise get rejected and need a retry. **Why canonical-only was chosen instead:** offering two spellings for the same operator reintroduces the exact ambiguity this tool exists to remove. The correction above makes the case stronger than it originally read here: an alias reaching the fold would silently produce a SQON missing that condition, not an equivalent one, so the enum is load-bearing rather than merely tidy. `foldClauses` keeps an `undefined` guard behind it, but **not as a text-operator tripwire**, which is what an earlier revision claimed: the guard fires only when an operator falls off the scalar switch, and `addFilterClause`'s text branch never reaches that switch. See § v2.1 for what actually needs fixing there. Aliases stay relevant only for the raw-SQON paths (`execute_query`'s `sqon` parameter, `SqonSchema.parse()` called directly), which are different consumers with different constraints; `existingSqon` is normalized on the way in for the same reason, since it arrives through `SqonBuilder.from()`. - **Extra properties on a node are silently kept, not rejected**, because every SQON schema uses Zod's `.passthrough()`. A typo in a required key (like `field` for `fieldName`) fails validation; a typo in an extra key does not. --- @@ -353,7 +389,9 @@ Note the schemas are factory functions rather than shared constants. Reusing one ## Progress to date -**v1 shipped 2026-08-10 (#1080).** This document remains the design record: read it for why the tool has the shape it does. For what was built, and the step-by-step plan it was built from, see `.dev/docs/build-sqon-implementation.md`, which also carries the measured behaviour table this document's corrections came from. v2 (text operators) and v3 (mixed combinators) are still open, and § Phasing above is still the plan for them. +**v1 shipped 2026-08-10 (#1080). v2 shipped 2026-08-25**, covering `wildcard` text search plus `some-not-in` and `all`. This document remains the design record: read it for why the tool has the shape it does. For what was built, and the step-by-step plan it was built from, see `.dev/docs/build-sqon-implementation.md`, which also carries the measured behaviour table this document's corrections came from. v2.1 (fuzzy) and v3 (mixed combinators) are still open, and § Phasing above is still the plan for them. + +v2 needed no change in `modules/sqon`, which is what the split described above bought. One `modules/sqon` fix did land immediately before it, separately: `reduceSqon` corrupted a merged date range bound to `null`, which `build_sqon`'s post-fold failsafe reported as a tool defect and which `graphql-router`'s network search path passed to remote nodes with no error at all. | Component | Location | | ---------------------------------------------- | -------------------------------------------------- | @@ -368,5 +406,6 @@ Note the schemas are factory functions rather than shared constants. Reusing one | MCP tool registration pattern | `apps/mcp-server/src/mcp/tools.ts` | | `build_sqon` tool, schemas, fold, and handler | `apps/mcp-server/src/mcp/buildSqonTool.ts` | | per-clause validation against a catalogue | `apps/mcp-server/src/arranger/clauseValidation.ts` | +| shared field-and-operator check | `apps/mcp-server/src/arranger/queryValidation.ts` | | plain-English summary and leaf counter | `apps/mcp-server/src/arranger/sqonSummary.ts` | | user-facing documentation | `docs/mcp-server.md`, `apps/mcp-server/README.md` | diff --git a/.dev/roadmap.md b/.dev/roadmap.md index 8db74fcf7..c4beeedf9 100644 --- a/.dev/roadmap.md +++ b/.dev/roadmap.md @@ -244,7 +244,7 @@ Consumers that build on Arranger (portal frontends, and internally `modules/char ### MCP integration readiness -_Priority: mixed per sub-item. One of six shipped (`build_sqon`, 2026-08-10); five open._ +_Priority: mixed per sub-item. One of six shipped (`build_sqon`, v1 2026-08-10, v2 2026-08-25); five open._ Six improvements making Arranger a well-behaved upstream for an MCP server layer. Open: schema cache invalidation signal (ETag/schema hash, `high`), SQON documentation in schema descriptions, field descriptions in the generated schema, making invisible query defaults SDL-visible (research), and the accumulated `/docs` gap for the MCP surface. @@ -318,7 +318,9 @@ This is a genuine prerequisite, not just adjacent work: access denial events (se _Priority: medium. Distinct from the `wildcard` operator already implemented._ -A `fuzzy` op doing Levenshtein matching via ES/OS `multi_match` with `fuzziness: "AUTO"`, same `fieldNames` shape as `wildcard`. Note `docs/concepts.md` already advertises this operator, so the published contract is fixed rather than free (see tech-debt). +A `fuzzy` op doing Levenshtein matching via ES/OS `multi_match` with `fuzziness: "AUTO"`, same `fieldNames` shape as `wildcard`. + +Now also blocking `build_sqon` v2.1, which is otherwise ready: the tool shipped `wildcard` on 2026-08-25 and `fuzzy` is the one operator it cannot offer. A second `modules/sqon` fix rides with this one: `addFilterClause`'s text branch dispatches on `'fieldNames' in params` and ignores `operator` entirely, so it already returns a `wildcard` clause for a `fuzzy` request with no error (measured). Once both text operators share the plural shape, no caller can distinguish them and the dispatch has to read `operator`. [Detail: implementation notes, schema shape, and the AND-versus-OR design question](docs/atlas/roadmap/sqon-operators.md#fuzzy-edit-distance-sqon-operator) diff --git a/.dev/sessions/2026-08-11T175652.md b/.dev/sessions/2026-08-11T175652.md new file mode 100644 index 000000000..83f6dc1b4 --- /dev/null +++ b/.dev/sessions/2026-08-11T175652.md @@ -0,0 +1,14 @@ +Standardized the case of "unknown" across the MCP server's field-validation error messages, after a flag that some tests assert `Unknown` and others `unknown`. + +The audit found five message strings and two emission contexts. `clauseValidation.ts` returns sentence fragments that `validateClauses` prefixes with `clauses[i]: `; `queryValidation.ts` returns whole sentences that `executeQueryTool` emits as bullets under `Query validation failed:`. Every test assertion faithfully mirrored its source string, so no test was wrong. Only one source string was: `Unknown field` in `clauseValidation.ts` was the sole capitalized fragment in a file whose other four messages start lowercase or with a quoted operator name, and it contradicted `.dev/docs/build-sqon-implementation.md`, which specifies lowercase there for exactly this reason. + +The rule adopted is that a message capitalizes its first word only when that word begins the emitted string. It already held for four of the five. The alternative considered and rejected was forcing `Unknown` uppercase everywhere: two `clauseValidation` messages begin with a quoted operator (`"between" takes [min, max]`, `"not-in" already means`) and can never be capitalized, so that rule cannot hold inside the one file it was meant to fix, and it would also require rewording `SQON references unknown field`, which is what distinguishes a bad field in the `sqon` from a bad field in `fields` when `execute_query` lists both errors together. + +- `apps/mcp-server/src/arranger/clauseValidation.ts`: `Unknown field` to `unknown field`, plus a `@remarks` line on `validateClause` recording that its messages are fragments completing the `clauses[i]: ` prefix, so the drift does not recur silently. `queryValidation.ts` is unchanged; it was already consistent. +- `apps/mcp-server/src/arranger/clauseValidation.test.ts`, `apps/mcp-server/src/mcp/buildSqonTool.test.ts`, `integration-tests/mcp-server/test/buildSqon.ts`: assertions follow the source. Every assertion that starts at a message's first word now includes the `clauses[0]: ` prefix, so the expected case is self-evident at the assertion rather than something a reader has to reconstruct from the emission context. That covers the unknown-field and invalid-operator assertions; the ones matching mid-message substrings (`double negative`, `needs a number`) cannot anchor to the prefix and are left alone. + +- `apps/mcp-server/src/arranger/queryValidation.test.ts`, `apps/mcp-server/src/mcp/buildSqonTool.test.ts`, `integration-tests/mcp-server/test/buildSqon.ts`, `integration-tests/mcp-server/test/executeQuery.ts`: the same self-evidence principle applied to the whole-sentence family. The five assertions that matched from the word `unknown` onward now anchor to `SQON references unknown field`, so the lowercase is visibly mid-sentence rather than something a reader reconstructs from the emission context. No message changed for this. + +`npm run test -w apps/mcp-server`: 189 passing, prettier clean. The integration suite was not run: it needs Elasticsearch on 9200, and the four changed assertions there are string-literal edits mirroring unit-tested behaviour. + +Incidental: `queryValidation.test.ts` is now prettier-clean. The two over-long assert lines flagged as pre-existing prettier dirt in the previous session were exactly the two rewritten here, and the rewrite matches prettier's own output. diff --git a/.dev/sessions/2026-08-12T122554.md b/.dev/sessions/2026-08-12T122554.md new file mode 100644 index 000000000..90e84da79 --- /dev/null +++ b/.dev/sessions/2026-08-12T122554.md @@ -0,0 +1,28 @@ +Closed a batching gap in `build_sqon` found in review of #1091: a call carrying both an invalid clause and an `existingSqon` the target catalogue cannot run reported only the clauses, so the mismatch surfaced on a second call. That is the round trip the design document's batching argument exists to remove, so it was a correctness gap against the design, not just a rough edge. + +The handler had three sequential early returns, and two of them sat outside the clause batch: the structural `SqonSchema.safeParse` of `existingSqon` returned before `validateClauses` ran at all, and the catalogue check on `existingSqon` fields lived in the post-fold `validateSqon`, reachable only once every clause had passed. Both are now folded into one error list. The fix rests on a property of the fold worth recording: `addFilterClause`/`reduceSqon` never invent a field name and never rewrite a leaf's operator (merging two `gt` clauses keeps one `gt` with the stricter bound; `negate` adds a `not` group, not a new leaf op), so every leaf of the output comes from either `existingSqon` or a clause. Validating the two inputs separately therefore catches everything validating the output caught, one round trip earlier. + +Decisions: + +- **Both failure modes of `existingSqon` batch, not just the catalogue mismatch.** Fixing only the reported case would have left the identical two-round-trip complaint reachable through the structural branch. A structural failure short-circuits the catalogue walk (there is no tree to walk) but is still returned as an error rather than an early result, so clause validation runs regardless. +- **`existingSqon` errors lead the list**: a base query built for another catalogue has to be dropped before the clause fixes are worth making. +- **The rebuild advice is gated on the catalogue-mismatch case specifically**, not on "`existingSqon` failed somehow". The first cut gated it on a non-empty `existingSqon` error list, which read fine until the composed messages were printed and inspected: a value that is not a SQON at all was being told to consider which catalogue it came from, when its own message already says to pass the previous `sqon` back unchanged or omit it. `resolveExistingSqon` now returns a `catalogueMismatch` flag for the caller to gate on. Worth noting as a method point: the substring assertions all passed before this was caught, and printing the three real messages side by side is what surfaced it. +- **The post-fold `validateSqon` stays, retargeted as a failsafe.** With both inputs pre-validated it is now unreachable through any known input, so its message says the opposite of what it used to: this is a defect in the tool, resubmitting will not help, do not retry. Kept rather than deleted because v2 (text operators) and v3 (mixed combinators) add folds it cannot yet see, and it is one tree walk. +- **`validateSqonFields` split out of `validateSqon`** rather than calling `validateSqon` on the already-parsed node. The caller holds a parsed, normalized `SqonNode`, so a result union whose structural branch cannot be reached was the wrong shape, and the second parse was wasted. Its `subject` option names the input under validation and defaults to `SQON`, which keeps every `execute_query` message byte-identical: no churn in `executeQueryTool.ts` or its assertions. + +Files: + +- `apps/mcp-server/src/arranger/queryValidation.ts`: `validateSqonFields` exported, taking a parsed node plus an optional `subject`; `validateSqon` delegates its walk to it, so the field-and-operator rules have one implementation. +- `apps/mcp-server/src/mcp/buildSqonTool.ts`: `resolveExistingSqon` returns `{ sqon?, errors }`; `composeValidationError` assembles the one message; the handler merges both error lists, folds only on a clean resolution, and reports the post-fold check as a tool defect. The docstrings carry the fold property above and the rule that a resolution with errors must never be folded. +- `apps/mcp-server/src/mcp/buildSqonTool.test.ts`: five new cases (the regression itself, the structural pairing, an `existingSqon` operator wrong for its field type, the ordering contract, and the rebuild advice withheld when only clauses fail), plus an assertion on the structural pairing that the advice stays withheld there too; the wrong-catalogue case updated to the batched message. The operator-alias case gained a comment noting it now also covers normalizing before the catalogue check. +- `apps/mcp-server/src/arranger/queryValidation.test.ts`: `validateSqonFields` covered directly, including that the default subject reproduces the pre-split wording. +- `integration-tests/mcp-server/test/buildSqon.ts`: test 11's assertion follows the new message; new test 20 asserts both problems in one response, in order, over a real transport. +- `.dev/docs/build-sqon-tool.md`: § Error handling documents the shared batch and why the check moved ahead of the fold; § Why one call builds a whole batch records that its obligation covers the whole input, not only `clauses`. +- `.dev/docs/build-sqon-implementation.md`: step 8's order, worked code, and helpers updated; test table and checklist extended. +- `docs/mcp-server.md`: one sentence, that `existingSqon` is validated in the same pass. + +Tests: `npm run test:dev` green (sqon 119, types 25, graphql-router 405, search-server 36, mcp-server 199, import 3). The MCP integration suite ran this time, against Elasticsearch on 9200: 69 passing, including the new case. Prettier and eslint clean on every changed file. The one eslint warning in `integration-tests/mcp-server/test/index.test.ts` (`err: any`) and the type errors from `tsc -p integration-tests/mcp-server` both pre-date this work and are unrelated: that tsconfig typechecks `apps/mcp-server` sources under different settings and already failed on `executeQueryTool.ts` before any change here. + +No `/docs` gap added: the user-facing behaviour change is error message wording, and `docs/mcp-server.md` was updated in the same pass. Nothing new for tech-debt; this closes a review finding rather than deferring one. + +Open thread, unchanged: `get_sqon_schema`'s cheat-sheet decision, and the two `[ ]` items in the implementation plan's checklist (`get_catalogue_fields` allowlist, and `*`/`set_id:`/`__missing__` in `in` values). diff --git a/.dev/sessions/2026-08-25T170000.md b/.dev/sessions/2026-08-25T170000.md new file mode 100644 index 000000000..7955de394 --- /dev/null +++ b/.dev/sessions/2026-08-25T170000.md @@ -0,0 +1,23 @@ +Planned and shipped `build_sqon` v2 in `apps/mcp-server`, with a `modules/sqon` bug fix ahead of it. Three commits, all tests green. + +**v2 as the spec defined it was not buildable, and the spec said so without noticing.** `.dev/docs/build-sqon-tool.md` § Phasing defined v2 as `fieldNames` for "`wildcard` and `fuzzy`", then noted in the next line that this "needs the fuzzy operator to exist in `modules/sqon` first". Those two sentences contradict each other: they block the half that was ready behind the half that has no implementation, no ES translation (`opSwitch` throws `unknown op`), and an unresolved AND-versus-OR design question. Split into v2 (`wildcard`, shipped) and v2.1 (`fuzzy`, blocked). The split is what kept v2 a pure MCP-layer change. + +- **Scope, as decided by the developer from three options each:** ship `wildcard` alone rather than waiting on or building `fuzzy`; add `some-not-in` and `all` in the same release, which the design document had left as "alongside v2, or as a separate v1.x"; and resolve the standing open question about `*` inside `in`-like values by rejecting it and redirecting to `wildcard`. +- **v2.1 has three blockers, not one.** Beyond the missing operator and the AND/OR question, `addFilterClause`'s text branch dispatches on `'fieldNames' in params` and never reads `operator`, so it already returns a `wildcard` clause for a `fuzzy` request with no error (measured against `dist`). Once both text operators share the plural shape, nothing downstream can tell them apart, so this is a required `modules/sqon` fix rather than a cosmetic one. Recorded in the roadmap's fuzzy item and § v2.1 of the design document. +- **A consequence of that same dispatch: the "tripwire for v2's text operators" never existed.** Both `.dev` documents and a code comment described `foldClauses`'s `undefined` guard that way. The guard fires only when an operator falls off the scalar switch, which the plural shape never reaches. Corrected in all three places. v2 sidesteps it by dispatching on `operator` at the call site. +- **The operator description had to stop naming field types.** `getSqonFieldOperatorDetails()` reports `applicableTo: 'all'` for `in`, `not-in`, `some-not-in`, `all`, and `wildcard`; `getValidFieldOperators` withholds `wildcard` from numeric and date fields and `all`/`some-not-in` from those plus text. The catalogue is what gets enforced, so "any field type" would have advertised clauses the tool rejects. `describeOperators` now says nothing about field types for an unrestricted operator and lets the `clauses` array description name `get_catalogue_fields` as the authority, once. Copying graphql-router's classification into `apps/mcp-server` was rejected as one more duplicated transform to drift; new tech-debt entry written for the real fix, which changes a published introspection contract. +- **No mutual-exclusion refinement was needed**, contrary to what the implementation plan predicted for v2. The shipped schema discriminates on `operator`, so the `wildcard` branch has no `fieldName` key and the others have no `fieldNames` key. The `all` branch is load-bearing rather than defensive: `addFilterClause` builds an `all` clause from a bare scalar without complaint and `SqonSchema` then rejects it. +- **A live v1 bug surfaced during planning and was fixed first, as its own commit.** `reduceSqon` merged same-field range bounds through `Math.max`/`Math.min` behind an `as number` cast; two date strings gave `NaN`, which serializes to `null`. Ordinary multi-turn date narrowing produced a bound-less filter. Two consumers, not one: `build_sqon`'s post-fold failsafe caught it and misreported it as a tool defect not worth retrying, while `convertToSqon` (`network/resolvers/index.ts:94`, via `SqonBuilder.from`) had no equivalent check, so a corrupted range query reached remote nodes and returned wrong or empty results silently. Bounds now compare numerically, by parsed timestamp, or lexicographically, and unorderable bounds are kept as two clauses. Closed the corresponding tech-debt entry, which had proposed exactly this fix. +- **`checkFieldOperator` extracted** from `validateFilterClause` so clause validation and the `existingSqon`/`execute_query` SQON walk share one implementation. It returns a typed reason rather than a message, because the two callers phrase the same finding differently on purpose: whole sentences led by a subject versus lowercase fragments completing a `clauses[i]: ` prefix. Pushing wording into the shared helper would have changed `execute_query`'s existing error text. + +**Cross-session review.** A peer session reviewed MCP-server tech debt and roadmap items against this plan. Three of its five findings were genuine gaps here and are folded in above (the tripwire claim, the date-merge bug, and the shared-check extraction); one was already covered and gained a useful addition on the fuzzy side. Its blast-radius claim that no consumer of `reduceSqon`'s range merging existed outside `apps/mcp-server` was wrong and was corrected: `SqonBuilder.from` is the reduce-carrying entry point, which is how the graphql-router network path is exposed. Its most useful contribution was catching that the fix first agreed with the developer (decline to merge) diverged from what the tech-debt entry proposed and quietly changed `build_sqon`'s `filterCount` behaviour, which is why the entry's own proposal shipped instead. + +**Verification.** `npm run test:dev` 868 pass across five workspaces; `apps/mcp-server` 199 to 249 tests; `integration-tests/mcp-server` 69 to 80, run against real Arranger and Elasticsearch, covering substring matching, any-field-matches across `fieldNames`, negated wildcard, and the aggregations path. `tsc --noEmit -p apps/mcp-server` and `prettier --check` clean. Emitted input schema grew 3805 to 5799 characters with zero `$ref`s. + +**Open threads.** + +- `get_sqon_schema`'s cheat sheet decision is still unmade, the last unchecked box in the v1 implementation plan. +- v3 (mixed AND/OR via a `combine_sqons` tool) is untouched and still the plan in § Phasing. +- `docs/concepts.md` mentions `build_sqon` in neither the prose definition nor the vocabulary table, per the developer's call that the page is general Arranger vocabulary rather than MCP-specific. The v1 implementation plan and the atlas entry had both called for adding the flat-tool-argument usage there; that is now closed as deliberately-not-doing, with `docs/mcp-server.md` carrying it instead. Not an open thread, recorded so the next reader does not re-open it as a gap. +- The `modules/sqon` range-merge fix got no CHANGELOG entry, per the developer's call that it fixes unreleased MCP-server behaviour. Worth revisiting if the graphql-router network-path exposure is judged separately releasable. +- New tech-debt entry: `integration-tests/mcp-server`'s tsconfig has never typechecked `apps/mcp-server` sources cleanly (seven errors, six of them pre-existing and unrelated to this work). diff --git a/.dev/tech-debt.md b/.dev/tech-debt.md index 704f4b4e5..19b311431 100644 --- a/.dev/tech-debt.md +++ b/.dev/tech-debt.md @@ -62,15 +62,6 @@ context: `modules/sqon/README.md` carries a "No stable release yet" section (mar **Fix:** Add an explicit max-depth check before or during parsing (a `zod.lazy` guard that tracks recursion depth and fails cleanly past a configurable limit, or a cheap pre-check walking the raw object once), so oversized nesting becomes a normal `{success:false}` validation failure instead of an engine-level exception. **Standalone:** yes. -### Merging range filters (`gt`/`gte`/`lt`/`lte`) with date-string values silently produces `null` instead of a comparison - -**File:** `modules/sqon/src/builder/reduce.ts:66-75` (`mergeIntoExisting`) -**Severity:** high -**Kind:** bug (correctness) -**Issue:** When two range filters on the same field are merged under `and`/`or`, the code does `Math.max(a, b)`/`Math.min(a, b)` after an `as number` cast, with no runtime check that the values are actually numeric. `gt`/`gte`/`lt`/`lte` explicitly support `'date'` fields (`operators/constants.ts:93`, `RANGE_APPLICABLE_TYPES`) and `SqonScalarValueSchema` permits string values for these ops, the ordinary shape for an ISO date filter. `Math.max`/`Math.min` on a date string coerces via `Number(...)`, which is `NaN` for a non-numeric string, and `NaN` serializes to `null`. Confirmed directly: merging `gt('donor.date_of_diagnosis','2020-01-01')` with `gt('donor.date_of_diagnosis','2021-06-15')` under `.and()` produces `{"op":"gt","content":{"fieldName":"donor.date_of_diagnosis","value":null}}`, silently corrupting an ordinary date-range-narrowing operation into a `null`-valued filter. `builder/index.test.ts`'s `reduceSqon` suite (lines 368-398) only exercises numeric values for these four ops; no test uses a date-typed (string) value. -**Fix:** In `mergeIntoExisting`, detect non-numeric scalar values and compare via string ordering (correct for ISO 8601 dates) or `Date.parse`, falling back to numeric comparison only when both values are genuinely numbers. Add a date-value test case to the existing `reduceSqon` suite. -**Standalone:** yes. - ### `removeFilter` can leave a schema-invalid or semantically-empty filter instead of removing it, contradicting its own documented contract **File:** `modules/sqon/src/builder/index.ts:210-217` (`stripValues`), consumed at lines 225-226 and 244-250 @@ -618,14 +609,23 @@ Compounding, separately tracked: even when `enableAdmin` is truthy, `router.ts` **Standalone:** no; needs a decision on the supported and intended Node versions before any file changes **Correction (2026-08-17):** `DEVELOPMENT.md:11` states the identical "v22 or higher" claim as `README.md:21` but isn't in this entry's file list; fix it in the same pass or it'll still disagree once the other three are resolved. -### `docs/concepts.md` documents a `fuzzy` SQON operator that does not exist +### `modules/sqon` reports operator field-type applicability that no catalogue agrees with -**File:** `docs/concepts.md:57,92` -**Severity:** high (a reader following this doc constructs an invalid SQON that fails schema validation) -**Kind:** stale documentation -**Issue:** Both lines present `fuzzy` as an existing, implemented operator on equal footing with `wildcard` ("Text-search operators (`wildcard`, `fuzzy`)..."). It isn't: `modules/sqon`'s leaf-node schema union has no `fuzzy` branch (`InLikeFilterSchema`/`AllFilterSchema`/`RangeLikeFilterSchema`/`BetweenFilterSchema`/`WildcardFilterSchema` only), and a filter with `op: "fuzzy"` fails validation outright. `CHANGELOG.md` (the entry that renamed `filter` to `wildcard`) explicitly says fuzzy/edit-distance matching "does not exist yet." `docs/reference/04-sqon-in-detail.md:224` gets this right (uses "fuzzy" only to name the not-yet-built concept being contrasted against); `concepts.md` is the only page with the incorrect claim. See also the roadmap's "Fuzzy (edit-distance) SQON operator" Features item, this is the real, planned-but-unbuilt op the doc is prematurely describing as shipped. -**Fix:** Remove `fuzzy` from both `concepts.md` lines, or rephrase as "wildcard (and a planned future `fuzzy` operator, not yet implemented)." -**Standalone:** yes; two-line docs fix. +**File:** `modules/sqon/src/operators/index.ts` (`getSqonFieldOperatorDetails`), against `modules/graphql-router/src/introspection/buildCatalogueIntrospection.ts:11-21` (`getValidFieldOperators`) +**Severity:** medium (a consumer trusting the module-level metadata advertises operators the catalogue rejects) +**Kind:** bug (correctness), duplicated source of truth +**Issue:** `getSqonFieldOperatorDetails()` reports `applicableTo: 'all'` for `in`, `not-in`, `some-not-in`, `all`, and `wildcard`, meaning every field type. `getValidFieldOperators` disagrees for three of the five: range-typed fields get `['in','not-in','gt','gte','lt','lte','between']`, enum-like fields get `['in','not-in','some-not-in','all','filter']`, and every other type gets `['in','not-in','filter']`. So `wildcard` is withheld from numeric and date fields, and `all` and `some-not-in` from those plus text fields. The catalogue is what actually gets enforced, since `apps/mcp-server`'s clause and SQON validation both check the introspected per-type lists. `build_sqon` hit this while building v2 and worked around it by having `describeOperators` say nothing about field types for an `applicableTo: 'all'` operator, rather than rendering it as "any field type" and advertising a clause the tool then rejects. `buildCatalogueIntrospection.ts` carries a comment acknowledging its own type sets were copied verbatim from `apps/search-server`, and names consolidation with `modules/sqon` as separate debt: this is that item, now with a concrete consumer. +**Fix:** Give `modules/sqon` the authoritative per-type mapping and have `getValidFieldOperators` derive from it rather than restating it. Note this changes the published `get_sqon_schema`/`arranger://introspection/sqon` payload, since `applicableTo` is part of it, so it is not a silent internal fix. Once done, `describeOperators` in `apps/mcp-server/src/mcp/buildSqonTool.ts` can name field types again and its workaround comment should be removed. +**Standalone:** no; changes a published introspection contract and touches two packages. Read the roadmap's SQON operator items first. + +### `integration-tests/mcp-server`'s tsconfig has never typechecked `apps/mcp-server` sources cleanly + +**File:** `integration-tests/mcp-server/tsconfig.json`, against `apps/mcp-server/src/mcp/buildSqonTool.ts` and `apps/mcp-server/src/mcp/executeQueryTool.ts` +**Severity:** low (no runtime effect; the tests pass and the app's own typecheck is clean) +**Kind:** build configuration +**Issue:** `npx tsc --noEmit -p integration-tests/mcp-server` reports errors in `apps/mcp-server` sources that `npx tsc --noEmit -p apps/mcp-server` does not, because the two projects resolve different compiler options over the same files. Confirmed 2026-08-25: it reports a `catalogIntrospectionSchema` optional-property mismatch and a `clauses` argument mismatch in `buildSqonTool.ts`, plus four in `executeQueryTool.ts` (`SqonValidationResult.errors`, a `fields` record, and two `ArrangerSort` arrays). All of them are the same shape of complaint, an inferred-from-Zod type with optional properties assigned to a type requiring them, so the likely cause is a single differing option rather than seven separate defects. Nobody typechecks that project directly today (its `test` script runs `tsx`), so the errors are invisible in normal use and were only noticed while verifying that a change had introduced none of its own. +**Fix:** Diff the two tsconfigs, align the option that differs, and either fix the resulting handful of genuine type errors or stop including app sources in that project's program. Worth doing before anything starts running `tsc` over it in CI, because the noise makes a real regression unfindable. +**Standalone:** yes. ### `hits`'s `score` field is declared in the schema and documented as always populated, but the resolver never assigns it diff --git a/CHANGELOG.md b/CHANGELOG.md index 7944b94d2..58436bb18 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,8 +66,8 @@ See [docs/reference/08-Migration/v3.1.md](docs/reference/08-Migration/v3.1.md) f - **New `apps/mcp-server`**: A Model Context Protocol server that exposes Arranger catalogues as LLM-queryable resources and tools. Separate Docker image: `ghcr.io/overture-stack/arranger-mcp-server`. Implements the MCP Streamable HTTP transport. - Resources: server introspection, SQON schema, per-catalogue fields. - Tools: `list_catalogues`, `get_sqon_schema`, `get_catalogue_fields`, `build_sqon`, `execute_query`. -- **`build_sqon` tool**: builds a validated SQON from plain `fieldName`/`operator`/`value` clauses, so a model selects conditions instead of writing query JSON. Every clause is checked against the catalogue's own field types and valid operators before anything is built, and one error is reported per invalid clause rather than stopping at the first, so a whole batch can be corrected in one resubmission. Returns the SQON alongside a plain-English `summary` built from the catalogue's display names (for reading back to the user before the query runs), and reports when equivalent clauses merged during the build so a lower filter count than was submitted is explained rather than silent. Optionally extends the SQON from an earlier call via `existingSqon`, for narrowing a query that already ran. Version 1 covers the scalar operators (`in`, `not-in`, `gt`, `gte`, `lt`, `lte`, `between`) with one `and`/`or` per call; text-search operators and mixed AND/OR nesting still require a hand-written `sqon` passed to `execute_query`. The server instructions, `execute_query`'s description, and the `query_arranger` prompt now all route SQON construction through this tool. See [docs/mcp-server.md](docs/mcp-server.md) for the full tool surface. -- **`build_sqon` merges two same-field `in` clauses by combining their value lists**: `status in ['active']` submitted alongside `status in ['pending']` builds `status in ['active', 'pending']`, meaning "either", and `notes` reports the merge so the lower filter count is explained rather than silent. "Either" is the correct reading on a single-valued field, where no document could satisfy both clauses at once. **The merge is not yet conditional on the field's `isArray`**, so on a field that can hold several values at once (`isArray: true`, or `null` where nothing declared it) the competing reading, "every one of these must be present", is equally legitimate and the merge picks "either" regardless. Version 1 cannot express the other reading: read `isArray` from `get_catalogue_fields` and pass a hand-written `all` SQON to `execute_query` when you need it. Tracked in the repo's tech-debt notes. +- **`build_sqon` tool**: builds a validated SQON from plain `fieldName`/`operator`/`value` clauses, so a model selects conditions instead of writing query JSON. Every clause is checked against the catalogue's own field types and valid operators before anything is built, and one error is reported per invalid clause rather than stopping at the first, so a whole batch can be corrected in one resubmission. Returns the SQON alongside a plain-English `summary` built from the catalogue's display names (for reading back to the user before the query runs), and reports when equivalent clauses merged during the build so a lower filter count than was submitted is explained rather than silent. Optionally extends the SQON from an earlier call via `existingSqon`, for narrowing a query that already ran. Covers every operator `modules/sqon` implements: the single-field operators (`in`, `not-in`, `some-not-in`, `all`, `gt`, `gte`, `lt`, `lte`, `between`) via `fieldName`, and `wildcard` text search across several fields at once via `fieldNames`. One `and`/`or` applies per call; mixed AND/OR nesting still requires a hand-written `sqon` passed to `execute_query`, as does the planned `fuzzy` operator. An asterisk inside an `in`-like value is rejected and redirected to `wildcard`, since Arranger would otherwise run it as a regular expression rather than matching it literally. The server instructions, `execute_query`'s description, and the `query_arranger` prompt now all route SQON construction through this tool. See [docs/mcp-server.md](docs/mcp-server.md) for the full tool surface. +- **`build_sqon` merges two same-field `in` clauses by combining their value lists**: `status in ['active']` submitted alongside `status in ['pending']` builds `status in ['active', 'pending']`, meaning "either", and `notes` reports the merge so the lower filter count is explained rather than silent. "Either" is the correct reading on a single-valued field, where no document could satisfy both clauses at once. **The merge is not yet conditional on the field's `isArray`**, so on a field that can hold several values at once (`isArray: true`, or `null` where nothing declared it) the competing reading, "every one of these must be present", is equally legitimate and the merge picks "either" regardless. Use the `all` operator directly when you need that reading. Tracked in the repo's tech-debt notes. ### Charts (`@overture-stack/arranger-charts`) @@ -126,7 +126,7 @@ The charts module was introduced in this release cycle as a new package. - **Fixed: merging two same-field filters under `not` could invert the query's meaning**: `not`'s children are each individually negated (`not[A, B]` means `¬A ∧ ¬B`), so combining two same-field clauses correctly under `not` requires flipping the operator itself; none of `reduceSqon`'s merge rules did that; they merged as if `not` behaved like `and`. Concretely, `not[not-in a:['2','3'], not-in a:['1']]` (matches nothing: the two clauses require `a` to be both in `{2,3}` and equal to `1`, which cannot hold) reduced to `not[not-in a:['1','2','3']]`, which matches `a` being `1`, `2`, or `3`. Found by differential testing against real query results, not by inspection. `reduceSqon` no longer merges same-field filters under `not` at all, for any operator; two clauses on the same field under `not` are now always kept separate, which costs a missed normalization but never an incorrect one. -- **Fixed: merging two same-field `in` filters under `and` widened the match set instead of narrowing it**: `AND(in a:['1','2'], in a:['1'])` requires a document to satisfy both clauses, their *intersection*, but was merged by unioning the value lists to `in a:['1','2']`, matching more documents than either clause alone. `reduceSqon` does not compute intersections, so `in` clauses under `and` are no longer merged at all; two separate `in` clauses under `and` already compile to two `terms` clauses under Elasticsearch's `bool.must`, which correctly evaluates as their intersection without any pre-computation needed. `or` is unaffected, union is the correct merge there. Confirmed this doesn't affect `arranger-components`' facet selection UI, which has its own independent same-field merge logic and never calls `reduceSqon` for it. +- **Fixed: merging two same-field `in` filters under `and` widened the match set instead of narrowing it**: `AND(in a:['1','2'], in a:['1'])` requires a document to satisfy both clauses, their _intersection_, but was merged by unioning the value lists to `in a:['1','2']`, matching more documents than either clause alone. `reduceSqon` does not compute intersections, so `in` clauses under `and` are no longer merged at all; two separate `in` clauses under `and` already compile to two `terms` clauses under Elasticsearch's `bool.must`, which correctly evaluates as their intersection without any pre-computation needed. `or` is unaffected, union is the correct merge there. Confirmed this doesn't affect `arranger-components`' facet selection UI, which has its own independent same-field merge logic and never calls `reduceSqon` for it. - **Fixed: same-field filter matching ignored `pivot` in three places**: a `pivot` scopes a nested-field condition to one matched sub-document, so treating two differently-scoped leaves (or one pivoted and one not) as the same filter silently reassigns a condition to the wrong nested scope, or drops its scoping entirely. - `reduceSqon`'s merge check now also compares `pivot`; two same-field leaves with different pivots are left as separate clauses instead of merged. diff --git a/apps/mcp-server/README.md b/apps/mcp-server/README.md index e5020dc71..b15e96244 100644 --- a/apps/mcp-server/README.md +++ b/apps/mcp-server/README.md @@ -16,7 +16,7 @@ The server registers five tools that cover the full query lifecycle: | `build_sqon` | Builds a validated SQON from plain field, operator, and value clauses, with a plain-English summary. Builds only; it executes nothing. | | `execute_query` | Builds, confirms, and executes a SQON-filtered query against a catalogue and returns the matching records. | -The intended call order is `list_catalogues` → `get_catalogue_fields` → `build_sqon` → `execute_query`, which is what `SERVER_INSTRUCTIONS` and the `query_arranger` prompt both describe. `build_sqon` covers the scalar operators (`in`, `not-in`, `gt`, `gte`, `lt`, `lte`, `between`) with one `and`/`or` per call; text operators and mixed combinators are not in version 1, so those still need a hand-written `sqon` passed to `execute_query`. +The intended call order is `list_catalogues` → `get_catalogue_fields` → `build_sqon` → `execute_query`, which is what `SERVER_INSTRUCTIONS` and the `query_arranger` prompt both describe. `build_sqon` covers every operator `modules/sqon` implements: the single-field operators (`in`, `not-in`, `some-not-in`, `all`, `gt`, `gte`, `lt`, `lte`, `between`) with `fieldName`, and `wildcard` text search across several fields with `fieldNames`. Mixed combinators and the planned `fuzzy` operator are not supported, so those still need a hand-written `sqon` passed to `execute_query`. ## Folder Structure diff --git a/apps/mcp-server/src/arranger/clauseValidation.test.ts b/apps/mcp-server/src/arranger/clauseValidation.test.ts index 7b002318b..294b88cab 100644 --- a/apps/mcp-server/src/arranger/clauseValidation.test.ts +++ b/apps/mcp-server/src/arranger/clauseValidation.test.ts @@ -10,14 +10,18 @@ const context: CatalogueQueryContext = { 'donor.sex': { type: 'keyword' }, 'donor.age_at_diagnosis': { type: 'long' }, 'donor.enrolled_on': { type: 'date' }, - 'donor.notes': { type: 'text' }, + 'donor.notes': { type: 'geo_point' }, + 'file.name': { type: 'text' }, }, // `keyword` mirrors the live introspection response, which still advertises the legacy `filter` - // alias; `long` uses the `>=` alias to prove the introspected list is normalized too. + // alias; `long` uses the `>=` alias to prove the introspected list is normalized too. `text` + // mirrors the fallback bucket, which gets the text operator but not the set-membership ones, and + // `geo_point` is deliberately absent so one field exercises the no-rules-for-this-type path. operators: { keyword: ['in', 'not-in', 'some-not-in', 'all', 'filter'], long: ['in', 'not-in', '>=', 'gt', 'lt', 'lte', 'between'], date: ['gt', 'gte', 'lt', 'lte', 'between'], + text: ['in', 'not-in', 'filter'], }, }; @@ -92,14 +96,14 @@ suite('validateClauses', () => { test('rejects a field the catalogue does not have, and points at get_catalogue_fields', () => { const errors = validate({ fieldName: 'not.a.field', operator: 'in', value: ['A'] }); assert.equal(errors.length, 1); - assert.ok(errors[0].includes('Unknown field "not.a.field"')); + assert.ok(errors[0].includes('clauses[0]: unknown field "not.a.field"')); assert.ok(errors[0].includes('get_catalogue_fields')); }); test('reports only the unknown field when the operator is also wrong for it', () => { const errors = validate({ fieldName: 'not.a.field', operator: 'gt', value: 'not-a-number' }); assert.equal(errors.length, 1); - assert.ok(errors[0].includes('Unknown field')); + assert.ok(errors[0].includes('clauses[0]: unknown field')); }); }); @@ -107,7 +111,9 @@ suite('validateClauses', () => { test('rejects an operator the field type does not accept, and lists the ones it does', () => { const errors = validate({ fieldName: 'donor.sex', operator: 'gt', value: 40 }); assert.equal(errors.length, 1); - assert.ok(errors[0].includes('operator "gt" is not valid for field "donor.sex" (type "keyword")')); + assert.ok( + errors[0].includes('clauses[0]: operator "gt" is not valid for field "donor.sex" (type "keyword")'), + ); assert.ok(errors[0].includes('in, not-in, some-not-in, all, wildcard')); }); @@ -186,4 +192,104 @@ suite('validateClauses', () => { assert.deepEqual(validateClauses([], context), []); }); }); + + suite('text-search clauses', () => { + test('accepts a wildcard across fields whose types advertise it', () => { + assert.deepEqual(validate({ fieldNames: ['study', 'file.name'], operator: 'wildcard', value: '*A*' }), []); + }); + + test('accepts a wildcard the catalogue advertises only under its legacy "filter" name', () => { + assert.deepEqual(validate({ fieldNames: ['study'], operator: 'wildcard', value: '*A*' }), []); + }); + + test('rejects a wildcard on a field type the catalogue withholds it from', () => { + const errors = validate({ fieldNames: ['donor.age_at_diagnosis'], operator: 'wildcard', value: '*4*' }); + assert.equal(errors.length, 1); + assert.ok(errors[0].includes('operator "wildcard" is not valid for field "donor.age_at_diagnosis"')); + assert.ok(errors[0].includes('(type "long")')); + }); + + test('names an unknown field within fieldNames', () => { + const errors = validate({ fieldNames: ['study', 'not.a.field'], operator: 'wildcard', value: '*A*' }); + assert.equal(errors.length, 1); + assert.ok(errors[0].includes('unknown field "not.a.field"')); + assert.ok(!errors[0].includes('"study"'), 'a valid field should not be named as a problem'); + }); + + // One clause is one condition, however many fields it spans, so every bad field is reported + // in that clause's single message rather than as several broken clauses. + test('reports every invalid field in one clause as one message', () => { + const errors = validate({ + fieldNames: ['not.a.field', 'donor.age_at_diagnosis', 'study'], + operator: 'wildcard', + value: '*A*', + }); + assert.equal(errors.length, 1); + assert.ok(errors[0].startsWith('clauses[0]: ')); + assert.ok(errors[0].includes('unknown field "not.a.field"')); + assert.ok(errors[0].includes('operator "wildcard" is not valid for field "donor.age_at_diagnosis"')); + }); + + test('accepts negate on a wildcard, the only way to express "does not contain"', () => { + assert.deepEqual(validate({ fieldNames: ['study'], operator: 'wildcard', value: '*A*', negate: true }), []); + }); + }); + + suite('asterisks in term-matched values', () => { + test('rejects an asterisk in an in-like value, pointing at the wildcard operator', () => { + const errors = validate({ fieldName: 'study', operator: 'in', value: ['*TP53*'] }); + assert.equal(errors.length, 1); + assert.ok(errors[0].includes('value "*TP53*" contains "*"')); + assert.ok(errors[0].includes('regular expression')); + assert.ok(errors[0].includes('"wildcard"')); + }); + + test('rejects an asterisk in a bare scalar value, not only in an array', () => { + assert.equal(validate({ fieldName: 'study', operator: 'in', value: '*A*' }).length, 1); + }); + + test('inspects every value rather than only the first', () => { + const errors = validate({ fieldName: 'study', operator: 'in', value: ['A', 'B*'] }); + assert.equal(errors.length, 1); + assert.ok(errors[0].includes('value "B*"')); + }); + + test('covers every term-matched operator', () => { + for (const operator of ['in', 'not-in', 'some-not-in', 'all']) { + assert.equal( + validate({ fieldName: 'study', operator, value: ['*A*'] }).length, + 1, + `${operator} should reject an asterisked value`, + ); + } + }); + + test('leaves a set reference and the missing-field sentinel alone', () => { + assert.deepEqual(validate({ fieldName: 'study', operator: 'in', value: ['set_id:abc'] }), []); + assert.deepEqual(validate({ fieldName: 'study', operator: 'in', value: ['__missing__'] }), []); + }); + + test('leaves an asterisk in a wildcard value alone, which is where it belongs', () => { + assert.deepEqual(validate({ fieldNames: ['study'], operator: 'wildcard', value: '*A*' }), []); + }); + + test('leaves a non-string value alone', () => { + assert.deepEqual(validate({ fieldName: 'donor.age_at_diagnosis', operator: 'in', value: [40] }), []); + }); + }); + + suite('set-membership operators', () => { + test('accepts "all" and "some-not-in" on a keyword field', () => { + assert.deepEqual(validate({ fieldName: 'study', operator: 'all', value: ['A', 'B'] }), []); + assert.deepEqual(validate({ fieldName: 'study', operator: 'some-not-in', value: ['A'] }), []); + }); + + test('rejects them on a text field, which the catalogue withholds them from', () => { + for (const operator of ['all', 'some-not-in']) { + const errors = validate({ fieldName: 'file.name', operator, value: ['A'] }); + assert.equal(errors.length, 1, `${operator} should be rejected on a text field`); + assert.ok(errors[0].includes(`operator "${operator}" is not valid for field "file.name"`)); + } + }); + }); }); diff --git a/apps/mcp-server/src/arranger/clauseValidation.ts b/apps/mcp-server/src/arranger/clauseValidation.ts index 295bf80d1..df24a3381 100644 --- a/apps/mcp-server/src/arranger/clauseValidation.ts +++ b/apps/mcp-server/src/arranger/clauseValidation.ts @@ -1,17 +1,31 @@ import { normalizeSqonOp, type SqonAcceptedOp } from '@overture-stack/sqon'; -import type { CatalogueQueryContext } from './queryValidation.js'; +import { checkFieldOperator, type CatalogueQueryContext } from './queryValidation.js'; /** - * A `build_sqon` clause, after Zod parsing and before it is folded into a SQON. + * A `build_sqon` clause naming one field, after Zod parsing and before it is folded into a SQON. */ -export type SqonClauseInput = { +export type SqonScalarClauseInput = { fieldName: string; operator: string; value: unknown; negate?: boolean; }; +/** + * A `build_sqon` text-search clause, which names several fields at once and matches a document when + * any one of them matches. `fieldNames` (plural) is what distinguishes it from a scalar clause, in + * the tool input and in the SQON leaf it becomes. + */ +export type SqonTextClauseInput = { + fieldNames: string[]; + operator: string; + value: unknown; + negate?: boolean; +}; + +export type SqonClauseInput = SqonScalarClauseInput | SqonTextClauseInput; + /** * Operators that already express exclusion, so `negate: true` on them is a double negative. */ @@ -22,43 +36,107 @@ const SELF_NEGATING_OPERATORS = new Set(['not-in', 'some-not-in']); */ const RANGE_OPERATORS = new Set(['gt', 'gte', 'lt', 'lte', 'between']); +/** + * Operators whose values Arranger matches as whole terms, and where an asterisk therefore changes + * how the query runs rather than what it matches. + */ +const TERM_MATCHING_OPERATORS = new Set(['in', 'not-in', 'some-not-in', 'all']); + +const isTextClause = (clause: SqonClauseInput): clause is SqonTextClauseInput => 'fieldNames' in clause; + +/** + * Validates the fields a text-search clause names, reporting every one that fails rather than the + * first. A text clause is one condition spanning several fields, so a single message listing each + * failure keeps the batch readable: one broken clause reads as one broken clause, however many of + * its fields are at fault. + * @param fieldNames - The fields the clause searches across. + * @param operator - The operator as submitted, used in the message so it matches what was written. + * @param canonicalOperator - The same operator, normalized, for checking against the catalogue. + * @param context - Catalogue fields and per-type operator rules from introspection. + * @returns One message covering every invalid field, or undefined when all of them are valid. + */ +const validateFieldNames = ( + fieldNames: string[], + operator: string, + canonicalOperator: string, + context: CatalogueQueryContext, +): string | undefined => { + const problems = fieldNames.flatMap((fieldName) => { + const problem = checkFieldOperator(fieldName, canonicalOperator, context); + if (problem === undefined) { + return []; + } + + return [ + problem.kind === 'unknown-field' + ? `unknown field "${fieldName}"` + : `operator "${operator}" is not valid for field "${fieldName}" (type "${problem.fieldType}"), which accepts: ${problem.validOperators.join(', ')}`, + ]; + }); + + if (problems.length === 0) { + return undefined; + } + + return `${problems.join('; ')}. Use get_catalogue_fields to list valid field names and the operators each field type accepts; do not guess.`; +}; + /** * Validates one clause against the catalogue, returning the first problem found or `undefined` * when the clause is valid. Ordered cheapest-first, and stops at the first failure: a clause with * two problems is fixed by re-reading the same field metadata either way, and two messages for one * clause reads as two broken clauses. + * @remarks Every message returned here is a sentence fragment completing the `clauses[i]: ` prefix + * added by `validateClauses`, so none of them capitalize their first word. * @param clause - `build_sqon` clause to be validated * @param context - the relevant catalogue's fields, including their types and valid SQON operators * @returns The first validation error found, or undefined if the clause is valid. */ const validateClause = (clause: SqonClauseInput, context: CatalogueQueryContext): string | undefined => { - const { fieldName, negate, operator, value } = clause; + const { negate, operator, value } = clause; if (negate === true && SELF_NEGATING_OPERATORS.has(operator)) { return `"${operator}" already means "not equal to", so combining it with negate: true is a double negative. Drop negate, or switch to "in" if you meant to include these values rather than exclude them.`; } - const field = context.fields[fieldName]; - if (!field) { - return `Unknown field "${fieldName}". Use get_catalogue_fields to list valid field names; do not guess.`; - } - // Normalizing both sides matches validateFilterClause in queryValidation.ts. The input enum is // canonical-only, so the clause operator is already canonical; the introspected list is not, // because graphql-router still advertises the legacy `filter` name (tracked tech-debt). const canonicalOperator = normalizeSqonOp(operator as SqonAcceptedOp); - const validOperators = context.operators[field.type]?.map((op) => normalizeSqonOp(op as SqonAcceptedOp)); - if (validOperators && !validOperators.includes(canonicalOperator)) { - return `operator "${operator}" is not valid for field "${fieldName}" (type "${field.type}"). Valid operators for this field: ${[...new Set(validOperators)].join(', ')}.`; + + // An asterisk in a term-matched value is not matched literally: graphql-router routes such a + // value to a regex query instead of a terms query, so the model gets substring behaviour it did + // not ask for and cannot see. `wildcard` is the operator that expresses this deliberately. + if (TERM_MATCHING_OPERATORS.has(canonicalOperator)) { + const values = Array.isArray(value) ? value : [value]; + const asterisked = values.find((entry) => typeof entry === 'string' && entry.includes('*')); + if (asterisked !== undefined) { + return `value "${String(asterisked)}" contains "*", which Arranger runs as a regular expression rather than matching the value literally. For substring search use the "wildcard" operator with fieldNames; for an exact match, drop the "*".`; + } + } + + if (isTextClause(clause)) { + return validateFieldNames(clause.fieldNames, operator, canonicalOperator, context); } + const { fieldName } = clause; + const problem = checkFieldOperator(fieldName, canonicalOperator, context); + if (problem?.kind === 'unknown-field') { + return `unknown field "${fieldName}". Use get_catalogue_fields to list valid field names; do not guess.`; + } + if (problem?.kind === 'invalid-operator') { + return `operator "${operator}" is not valid for field "${fieldName}" (type "${problem.fieldType}"). Valid operators for this field: ${problem.validOperators.join(', ')}.`; + } + + const fieldType = context.fields[fieldName]?.type; + // A range bound on a non-date field is numeric. A quoted number passes both the input schema // and the SQON schema, then gets compared lexicographically by ES/OS, where "9" > "70" is true. // That returns the wrong documents silently, which is worse than an error. - if (RANGE_OPERATORS.has(canonicalOperator) && field.type !== 'date') { + if (RANGE_OPERATORS.has(canonicalOperator) && fieldType !== 'date') { const bounds = Array.isArray(value) ? value : [value]; if (bounds.some((bound) => typeof bound !== 'number')) { - return `operator "${operator}" on field "${fieldName}" (type "${field.type}") needs a number, not a quoted string. Quote a bound only for a date field.`; + return `operator "${operator}" on field "${fieldName}" (type "${fieldType}") needs a number, not a quoted string. Quote a bound only for a date field.`; } } diff --git a/apps/mcp-server/src/arranger/queryValidation.test.ts b/apps/mcp-server/src/arranger/queryValidation.test.ts index 13247074b..68aa492c4 100644 --- a/apps/mcp-server/src/arranger/queryValidation.test.ts +++ b/apps/mcp-server/src/arranger/queryValidation.test.ts @@ -1,11 +1,15 @@ import assert from 'node:assert/strict'; import { suite, test } from 'node:test'; +import { SqonSchema } from '@overture-stack/sqon'; + import { + checkFieldOperator, validateAggregationFields, validateHitsFields, validateSortFields, validateSqon, + validateSqonFields, type CatalogueQueryContext, } from '#arranger/queryValidation.js'; @@ -28,6 +32,43 @@ const context: CatalogueQueryContext = { }, }; +// The shared half of the field-and-operator rules, used by both `validateSqonFields` (for +// `execute_query`'s SQON walk and `build_sqon`'s `existingSqon`) and `validateClauses` (for +// `build_sqon`'s clauses). It reports why a pairing is invalid and leaves the wording to the +// caller, because the two phrase the same finding differently on purpose. +suite('checkFieldOperator', () => { + test('returns undefined for a field and operator the catalogue accepts', () => { + assert.equal(checkFieldOperator('donor.sex', 'in', context), undefined); + }); + + test('reports a field the catalogue does not have', () => { + assert.deepEqual(checkFieldOperator('not.a.field', 'in', context), { kind: 'unknown-field' }); + }); + + test("reports an operator the field's type does not accept, with the type and the alternatives", () => { + assert.deepEqual(checkFieldOperator('donor.sex', 'gt', context), { + fieldType: 'keyword', + kind: 'invalid-operator', + validOperators: ['in', 'not-in', 'some-not-in', 'all', 'wildcard'], + }); + }); + + test('normalizes the catalogue\'s legacy "filter" name before comparing', () => { + assert.equal(checkFieldOperator('donor.sex', 'wildcard', context), undefined); + }); + + test('deduplicates the operators it offers as alternatives', () => { + const problem = checkFieldOperator('donor.sex', 'gt', context); + assert.ok(problem?.kind === 'invalid-operator'); + assert.equal(new Set(problem.validOperators).size, problem.validOperators.length); + }); + + test('accepts any operator when the catalogue lists none for the field type', () => { + const noRules: CatalogueQueryContext = { fields: { a: { type: 'geo_point' } }, operators: {} }; + assert.equal(checkFieldOperator('a', 'gt', noRules), undefined); + }); +}); + suite('validateSqon', () => { test('accepts an empty root SQON', () => { const result = validateSqon({ op: 'and', content: [] }, context); @@ -50,7 +91,10 @@ suite('validateSqon', () => { const sqon = { op: 'in', content: { fieldName: 'not.a.field', value: ['x'] } }; const result = validateSqon(sqon, context); assert.equal(result.valid, false); - assert.ok(!result.valid && result.errors.some((error) => error.includes('unknown field "not.a.field"'))); + assert.ok( + !result.valid && + result.errors.some((error) => error.includes('SQON references unknown field "not.a.field"')), + ); }); test('rejects an operator that is not valid for the field type', () => { @@ -102,7 +146,9 @@ suite('validateSqon', () => { const sqon = { op: 'wildcard', content: { fieldNames: ['donor.sex', 'bad.field'], value: 'blood' } }; const result = validateSqon(sqon, context); assert.equal(result.valid, false); - assert.ok(!result.valid && result.errors.some((error) => error.includes('unknown field "bad.field"'))); + assert.ok( + !result.valid && result.errors.some((error) => error.includes('SQON references unknown field "bad.field"')), + ); }); test('lists valid operators by canonical name in operator errors', () => { @@ -118,6 +164,45 @@ suite('validateSqon', () => { }); }); +suite('validateSqonFields', () => { + test('returns no errors for a SQON whose every leaf fits the catalogue', () => { + const sqon = { op: 'in', content: { fieldName: 'donor.sex', value: ['Female'] } }; + assert.deepEqual(validateSqonFields(SqonSchema.parse(sqon), context), []); + }); + + test('names the SQON generically by default, matching what validateSqon reports', () => { + const sqon = { op: 'in', content: { fieldName: 'not.a.field', value: ['x'] } }; + assert.deepEqual(validateSqonFields(SqonSchema.parse(sqon), context), [ + 'SQON references unknown field "not.a.field". Use get_catalogue_fields to list valid fields.', + ]); + }); + + test('names the specific input under validation when given a subject', () => { + const sqon = { op: 'in', content: { fieldName: 'not.a.field', value: ['x'] } }; + assert.deepEqual(validateSqonFields(SqonSchema.parse(sqon), context, { subject: 'existingSqon' }), [ + 'existingSqon references unknown field "not.a.field". Use get_catalogue_fields to list valid fields.', + ]); + }); + + test('applies the subject to operator errors as well as unknown fields', () => { + const sqon = { op: 'gt', content: { fieldName: 'donor.sex', value: 5 } }; + const errors = validateSqonFields(SqonSchema.parse(sqon), context, { subject: 'existingSqon' }); + assert.equal(errors.length, 1); + assert.ok(errors[0].startsWith('existingSqon operator "gt" is not valid for field "donor.sex"')); + }); + + test('reports one error per invalid leaf across nested combinations', () => { + const sqon = { + op: 'and', + content: [ + { op: 'in', content: { fieldName: 'not.a.field', value: ['x'] } }, + { op: 'or', content: [{ op: 'between', content: { fieldName: 'donor.sex', value: [1, 2] } }] }, + ], + }; + assert.equal(validateSqonFields(SqonSchema.parse(sqon), context).length, 2); + }); +}); + suite('validateHitsFields', () => { test('accepts known leaf fields', () => { assert.deepEqual(validateHitsFields(['id', 'donor.sex'], context), []); diff --git a/apps/mcp-server/src/arranger/queryValidation.ts b/apps/mcp-server/src/arranger/queryValidation.ts index 5a0b642d1..6086d53ef 100644 --- a/apps/mcp-server/src/arranger/queryValidation.ts +++ b/apps/mcp-server/src/arranger/queryValidation.ts @@ -21,6 +21,47 @@ export type CatalogueQueryContext = { export type SqonValidationResult = { valid: true; sqon: SqonNode } | { valid: false; errors: string[] }; +/** + * Why one field cannot carry one operator, with the detail each reason needs to be explained. + * Deliberately not a message: `build_sqon` reports clause problems as lowercase fragments + * completing a `clauses[i]: ` prefix, while `execute_query` reports whole sentences led by the name + * of the input being checked, so the two phrase the same finding differently on purpose. + */ +export type FieldOperatorProblem = + | { kind: 'unknown-field' } + | { kind: 'invalid-operator'; fieldType: string; validOperators: string[] }; + +/** + * Checks one field name against one canonical operator, using the catalogue's own field list and + * per-type operator rules. Shared by `execute_query`'s SQON walk and `build_sqon`'s clause + * validation, which both need exactly this pair of checks in exactly this order and previously + * carried their own copy. + * @param fieldName - Dot-notation field name, as catalogue introspection keys it. + * @param canonicalOp - The operator, already normalized: callers hold it in canonical form for + * their own messages, and normalizing twice would hide an alias that reached this far. + * @param context - Catalogue fields and per-type operator rules from introspection. + * @returns The reason the pairing is invalid, or `undefined` when it is valid. A field type the + * catalogue advertises no operators for is treated as valid, matching the behaviour both callers + * had before: absent rules are not the same as rules that exclude this operator. + */ +export const checkFieldOperator = ( + fieldName: string, + canonicalOp: string, + context: CatalogueQueryContext, +): FieldOperatorProblem | undefined => { + const field = context.fields[fieldName]; + if (!field) { + return { kind: 'unknown-field' }; + } + + const validOperators = context.operators[field.type]?.map((op) => normalizeSqonOp(op as SqonAcceptedOp)); + if (validOperators && !validOperators.includes(canonicalOp as ReturnType)) { + return { fieldType: field.type, kind: 'invalid-operator', validOperators: [...new Set(validOperators)] }; + } + + return undefined; +}; + /** Field types that represent containers rather than queryable leaf values. */ const CONTAINER_FIELD_TYPES = new Set(['nested', 'object']); @@ -33,33 +74,72 @@ const isSqonGroup = (node: SqonNode): node is SqonNode & { content: SqonNode[] } * Operator aliases (e.g. `>=` → `gte`, `filter` → `wildcard`) are normalized to their canonical * form before checking. The catalogue's introspected operator lists are normalized the same way, * so a catalogue that still advertises the legacy `filter` operator accepts `wildcard` clauses. - * Errors are appended to the provided accumulator. + * @param leaf - The filter clause to validate: a non-combination node, whose `content` carries + * `fieldNames` for a `wildcard` clause and `fieldName` for every other operator. + * @param context - Catalogue fields and per-type operator rules from introspection. + * @param errors - Accumulator the messages are appended to. Every field named by the clause is + * checked, so one clause can contribute more than one message. + * @param subject - How the SQON being validated is named in every message, so a caller validating + * one specific input (e.g., `existingSqon`) can point at it rather than at "SQON" in general. */ -const validateFilterClause = (leaf: SqonNode, context: CatalogueQueryContext, errors: string[]): void => { +const validateFilterClause = ( + leaf: SqonNode, + context: CatalogueQueryContext, + errors: string[], + subject: string, +): void => { const canonicalOp = normalizeSqonOp(leaf.op as SqonAcceptedOp); const content = leaf.content as { fieldName?: string; fieldNames?: string[] }; const fieldNames = canonicalOp === 'wildcard' ? (content.fieldNames ?? []) : [content.fieldName ?? '']; for (const fieldName of fieldNames) { - const field = context.fields[fieldName]; - if (!field) { - errors.push(`SQON references unknown field "${fieldName}". Use get_catalogue_fields to list valid fields.`); + const problem = checkFieldOperator(fieldName, canonicalOp, context); + if (problem === undefined) { continue; } - const validOperators = context.operators[field.type]?.map((op) => normalizeSqonOp(op as SqonAcceptedOp)); - if (validOperators && !validOperators.includes(canonicalOp)) { - errors.push( - `SQON operator "${canonicalOp}" is not valid for field "${fieldName}" (type "${field.type}"). Valid operators: ${[...new Set(validOperators)].join(', ')}.`, - ); - } + errors.push( + problem.kind === 'unknown-field' + ? `${subject} references unknown field "${fieldName}". Use get_catalogue_fields to list valid fields.` + : `${subject} operator "${canonicalOp}" is not valid for field "${fieldName}" (type "${problem.fieldType}"). Valid operators: ${problem.validOperators.join(', ')}.`, + ); } }; +/** + * Validates an already-parsed SQON semantically: every leaf's field name(s) and operator against + * the catalogue's fields and per-type operator rules. Structural validity is the caller's problem, + * which is what separates this from `validateSqon`: a caller holding a `SqonNode` it has already + * parsed (and possibly normalized) gets a plain error list rather than a result union whose + * structural branch cannot be reached. + * @param sqon - A parsed SQON node. + * @param context - Catalogue fields and operator rules from introspection. + * @param options.subject - How the SQON is named in each message. Defaults to `SQON`; pass the + * name of the specific input being checked when the caller validates more than one thing. + * @returns One message per invalid leaf; empty when every leaf is valid. + */ +export const validateSqonFields = ( + sqon: SqonNode, + context: CatalogueQueryContext, + { subject = 'SQON' }: { subject?: string } = {}, +): string[] => { + const errors: string[] = []; + const visit = (node: SqonNode): void => { + if (isSqonGroup(node)) { + node.content.forEach(visit); + } else { + validateFilterClause(node, context, errors, subject); + } + }; + visit(sqon); + return errors; +}; + /** * Validates a raw SQON value: first structurally against the shared SQON schema from * `@overture-stack/sqon`, then semantically against the catalogue's fields and per-type - * operator rules from introspection. + * operator rules from introspection (via `validateSqonFields`, which callers holding an + * already-parsed node can use directly). * @param rawSqon - The unparsed SQON value provided by the caller. * @param context - Catalogue fields and operator rules from introspection. * @returns The parsed SQON on success, or the list of validation errors. @@ -91,15 +171,7 @@ export const validateSqon = (rawSqon: unknown, context: CatalogueQueryContext): }; } - const errors: string[] = []; - const visit = (node: SqonNode): void => { - if (isSqonGroup(node)) { - node.content.forEach(visit); - } else { - validateFilterClause(node, context, errors); - } - }; - visit(parsed.data); + const errors = validateSqonFields(parsed.data, context); return errors.length > 0 ? { valid: false, errors } : { valid: true, sqon: parsed.data }; }; diff --git a/apps/mcp-server/src/arranger/sqonSummary.test.ts b/apps/mcp-server/src/arranger/sqonSummary.test.ts index 7c90453b3..243d3c67a 100644 --- a/apps/mcp-server/src/arranger/sqonSummary.test.ts +++ b/apps/mcp-server/src/arranger/sqonSummary.test.ts @@ -51,10 +51,18 @@ suite('summarizeSqon', () => { assert.equal(summary, 'Age at Diagnosis is between 40 and 60'); }); + // Joined with "or", not commas: a wildcard clause matches when any one of its fields matches, + // so a comma list would read as though every field had to match. test('describes "wildcard" as a pattern match, naming every field it searches', () => { const wildcard = { op: 'wildcard', content: { fieldNames: ['study', 'donor.sex'], value: '*A*' } }; const summary = summarizeSqon(wildcard as unknown as SqonNode, fields); - assert.equal(summary, 'study, donor.sex matches "*A*"'); + assert.equal(summary, 'Study or Biological Sex matches "*A*"'); + }); + + test('falls back to the raw field name for a wildcard field the catalogue does not describe', () => { + const wildcard = { op: 'wildcard', content: { fieldNames: ['study', 'not.a.field'], value: '*A*' } }; + const summary = summarizeSqon(wildcard as unknown as SqonNode, fields); + assert.equal(summary, 'Study or not.a.field matches "*A*"'); }); test('falls back to a literal rendering for an operator it does not know', () => { diff --git a/apps/mcp-server/src/arranger/sqonSummary.ts b/apps/mcp-server/src/arranger/sqonSummary.ts index 7f9be234f..b1b620823 100644 --- a/apps/mcp-server/src/arranger/sqonSummary.ts +++ b/apps/mcp-server/src/arranger/sqonSummary.ts @@ -7,10 +7,20 @@ const formatValue = (value: unknown): string => (typeof value === 'string' ? `"$ const formatValues = (value: unknown): string => (Array.isArray(value) ? value : [value]).map(formatValue).join(' or '); +const describeField = (fieldName: string, fields: SummaryFields): string => fields[fieldName]?.displayName || fieldName; + +/** + * Labels the fields a text-search clause spans. Joined with "or" rather than commas because that + * is what the clause means: Arranger matches the value against each field independently and the + * document qualifies when any one of them matches. + */ +const describeFields = (fieldNames: string[], fields: SummaryFields): string => + fieldNames.map((fieldName) => describeField(fieldName, fields)).join(' or '); + const describeLeaf = (leaf: SqonNode, fields: SummaryFields): string => { const content = leaf.content as { fieldName?: string; fieldNames?: string[]; value: unknown }; const { fieldName, fieldNames, value } = content; - const label = (fieldName && fields[fieldName]?.displayName) || fieldName || (fieldNames ?? []).join(', '); + const label = fieldName !== undefined ? describeField(fieldName, fields) : describeFields(fieldNames ?? [], fields); switch (leaf.op) { case 'in': diff --git a/apps/mcp-server/src/mcp/buildSqonTool.test.ts b/apps/mcp-server/src/mcp/buildSqonTool.test.ts index 829807fad..b1cafb75f 100644 --- a/apps/mcp-server/src/mcp/buildSqonTool.test.ts +++ b/apps/mcp-server/src/mcp/buildSqonTool.test.ts @@ -112,13 +112,23 @@ const expectError = async (input: Record, client: ArrangerClien const inClause = (fieldName: string, value: unknown) => ({ fieldName, operator: 'in', value }); suite('BUILD_SQON_OPERATORS', () => { - test('offers only the single-field scalar operators v1 supports', () => { - assert.deepEqual([...BUILD_SQON_OPERATORS], ['in', 'not-in', 'gt', 'gte', 'lt', 'lte', 'between']); + test('offers every operator modules/sqon implements except the text operator it cannot build', () => { + assert.deepEqual( + [...BUILD_SQON_OPERATORS], + ['in', 'not-in', 'some-not-in', 'gt', 'gte', 'lt', 'lte', 'between', 'all', 'wildcard'], + ); + }); + + // `fuzzy` has no implementation in modules/sqon, and addFilterClause's text branch ignores + // `operator` and builds a wildcard clause regardless, so offering it would silently run a + // different query than the one asked for. + test('excludes the text operator that has no implementation', () => { + assert.ok(!BUILD_SQON_OPERATORS.includes('fuzzy' as never)); }); - test('excludes the operators v1 deliberately withholds', () => { - for (const withheld of ['all', 'some-not-in', 'wildcard']) { - assert.ok(!BUILD_SQON_OPERATORS.includes(withheld as never), `${withheld} should not be offered`); + test('excludes every operator alias', () => { + for (const alias of ['=', '==', '>=', '<=', '>', '<', '!=', 'filter']) { + assert.ok(!BUILD_SQON_OPERATORS.includes(alias as never), `${alias} should not be offered`); } }); @@ -153,8 +163,25 @@ suite('describeOperators', () => { assert.ok(!description.includes('"between"')); }); - test('renders an operator applicable to every field type in plain English', () => { - assert.ok(describeOperators(['in']).includes('applies to any field type')); + // modules/sqon reports these operators as applying to every field type, but a catalogue + // withholds them from some types, and `validateClauses` enforces the catalogue. Claiming "any + // field type" here would advertise a clause the tool then rejects, so the description says + // nothing about field types and lets the `clauses` array description name the catalogue instead. + test('claims no field types for an operator modules/sqon does not restrict', () => { + for (const operator of ['in', 'wildcard', 'all', 'some-not-in']) { + const description = describeOperators([operator]); + assert.ok(!description.includes('any field type'), `${operator} should not claim any field type`); + assert.ok(!description.includes('applies to'), `${operator} should not name field types at all`); + assert.ok(description.includes('value is'), `${operator} should still name its value type`); + } + }); + + test('names the field types for an operator modules/sqon does restrict', () => { + for (const operator of ['gt', 'between']) { + const description = describeOperators([operator]); + assert.ok(description.includes('applies to '), `${operator} should name its field types`); + assert.ok(description.includes('date'), `${operator} should list date among them`); + } }); test('names the applicable field types for a type-restricted operator', () => { @@ -215,9 +242,45 @@ suite('build_sqon input schema', () => { assert.equal(parse(oneClause({ fieldName: 'a', operator: '=', value: 'A' })).success, false); }); - test('rejects the text operators, which v1 does not support', () => { + test('accepts a wildcard clause naming its fields with fieldNames', () => { + assert.equal(parse(oneClause({ fieldNames: ['a', 'b'], operator: 'wildcard', value: '*A*' })).success, true); + assert.equal(parse(oneClause({ fieldNames: ['a'], operator: 'wildcard', value: '*A*' })).success, true); + }); + + // The union discriminates on `operator`, so each branch carries only the field property its own + // operator takes. That enforces the fieldName/fieldNames split structurally, with no refinement. + test('rejects a wildcard clause that names its field with the singular fieldName', () => { assert.equal(parse(oneClause({ fieldName: 'a', operator: 'wildcard', value: '*A*' })).success, false); - assert.equal(parse(oneClause({ fieldName: 'a', operator: 'fuzzy', value: '*A*' })).success, false); + }); + + test('rejects a scalar clause that names its fields with the plural fieldNames', () => { + assert.equal(parse(oneClause({ fieldNames: ['a'], operator: 'in', value: 'A' })).success, false); + }); + + test('rejects an empty fieldNames array, and an empty name within it', () => { + assert.equal(parse(oneClause({ fieldNames: [], operator: 'wildcard', value: '*A*' })).success, false); + assert.equal(parse(oneClause({ fieldNames: [''], operator: 'wildcard', value: '*A*' })).success, false); + }); + + test('rejects a non-string or empty wildcard value', () => { + assert.equal(parse(oneClause({ fieldNames: ['a'], operator: 'wildcard', value: 40 })).success, false); + assert.equal(parse(oneClause({ fieldNames: ['a'], operator: 'wildcard', value: '' })).success, false); + }); + + test('rejects "fuzzy", which has no implementation to build', () => { + assert.equal(parse(oneClause({ fieldNames: ['a'], operator: 'fuzzy', value: 'jon' })).success, false); + assert.equal(parse(oneClause({ fieldName: 'a', operator: 'fuzzy', value: 'jon' })).success, false); + }); + + test('requires an array value for "all", which cannot take a bare scalar', () => { + assert.equal(parse(oneClause({ fieldName: 'a', operator: 'all', value: ['A', 'B'] })).success, true); + assert.equal(parse(oneClause({ fieldName: 'a', operator: 'all', value: 'A' })).success, false); + assert.equal(parse(oneClause({ fieldName: 'a', operator: 'all', value: [] })).success, false); + }); + + test('accepts "some-not-in" on the in-like branch', () => { + assert.equal(parse(oneClause({ fieldName: 'a', operator: 'some-not-in', value: ['A'] })).success, true); + assert.equal(parse(oneClause({ fieldName: 'a', operator: 'some-not-in', value: 'A' })).success, true); }); test('requires exactly two bounds for "between"', () => { @@ -546,6 +609,8 @@ suite('build_sqon existingSqon', () => { assert.equal(output.clauseCount, 1); }); + // The alias is normalized before the catalogue check, not just before the fold: an existing SQON + // spelling `gte` as `>=` must not be reported as an operator this catalogue does not accept. test('normalizes an operator alias in an existing SQON rather than rejecting it', async () => { const { output } = await buildSqon({ catalogueId: 'participants', @@ -580,10 +645,307 @@ suite('build_sqon existingSqon', () => { clauses: [inClause('study', ['A'])], existingSqon: { op: 'in', content: { fieldName: 'file.size', value: ['A'] } }, }); - assert.ok(message.includes('valid individually, but the resulting SQON is not')); - assert.ok(message.includes('unknown field "file.size"')); + assert.ok(message.startsWith('No SQON was built.')); + assert.ok(message.includes('existingSqon references unknown field "file.size"')); + assert.ok(message.includes('rebuild the query for "participants"')); + }); + + test('rejects an existing SQON whose operator does not fit the field it names in this catalogue', async () => { + const message = await expectError({ + catalogueId: 'participants', + combination: 'and', + clauses: [inClause('study', ['A'])], + existingSqon: { op: 'gt', content: { fieldName: 'donor.sex', value: 40 } }, + }); + assert.ok(message.includes('existingSqon operator "gt" is not valid for field "donor.sex"')); + }); + + // The regression this batching exists for: before it, validateClauses returned first and the + // existingSqon mismatch only surfaced on a second call, after the clauses had been fixed. + test('reports an unusable existingSqon and an invalid clause in the same response', async () => { + const message = await expectError({ + catalogueId: 'participants', + combination: 'and', + clauses: [{ fieldName: 'donor.sex', operator: 'gt', value: 40 }], + existingSqon: { op: 'in', content: { fieldName: 'file.size', value: ['A'] } }, + }); + assert.ok(message.includes('existingSqon references unknown field "file.size"')); + assert.ok(message.includes('clauses[0]: ')); assert.ok(message.includes('rebuild the query for "participants"')); }); + + test('reports a structurally invalid existingSqon alongside an invalid clause, not instead of it', async () => { + const message = await expectError({ + catalogueId: 'participants', + combination: 'and', + clauses: [inClause('not.a.field', ['A'])], + existingSqon: { op: 'in', value: ['A'] }, + }); + assert.ok(message.includes('existingSqon is not a valid SQON')); + assert.ok(message.includes('clauses[0]: unknown field "not.a.field"')); + // The rebuild advice speaks to a SQON built for another catalogue. A value that is not a SQON + // at all already carries its own remedy, so pointing at catalogues would be misdirection. + assert.ok(!message.includes('rebuild the query')); + }); + + test('lists existingSqon before the clauses, since a base query from another catalogue has to go first', async () => { + const message = await expectError({ + catalogueId: 'participants', + combination: 'and', + clauses: [{ fieldName: 'donor.sex', operator: 'gt', value: 40 }], + existingSqon: { op: 'in', content: { fieldName: 'file.size', value: ['A'] } }, + }); + assert.ok(message.indexOf('existingSqon references') < message.indexOf('clauses[0]: ')); + }); + + test('does not offer the rebuild advice when only the clauses are at fault', async () => { + const message = await expectError({ + catalogueId: 'participants', + combination: 'and', + clauses: [{ fieldName: 'donor.sex', operator: 'gt', value: 40 }], + existingSqon: wrappedRoot, + }); + assert.ok(message.includes('clauses[0]: ')); + assert.ok(!message.includes('rebuild the query')); + }); +}); + +suite('build_sqon text search', () => { + const wildcard = (fieldNames: string[], value: string) => ({ fieldNames, operator: 'wildcard', value }); + + test('builds a wildcard clause carrying every field it searches', async () => { + const { output } = await buildSqon({ + catalogueId: 'participants', + combination: 'and', + clauses: [wildcard(['study', 'donor.sex'], '*A*')], + }); + assert.deepEqual(output.sqon, { + op: 'and', + content: [{ op: 'wildcard', content: { fieldNames: ['study', 'donor.sex'], value: '*A*' } }], + }); + }); + + test('negates a wildcard clause, which is how "does not contain" is expressed', async () => { + const { output } = await buildSqon({ + catalogueId: 'participants', + combination: 'and', + clauses: [{ ...wildcard(['study'], '*A*'), negate: true }], + }); + assert.deepEqual(output.sqon, { + op: 'not', + content: [{ op: 'wildcard', content: { fieldNames: ['study'], value: '*A*' } }], + }); + }); + + test('folds a wildcard clause alongside scalar clauses in one group', async () => { + const { output } = await buildSqon({ + catalogueId: 'participants', + combination: 'and', + clauses: [inClause('donor.sex', ['Male']), wildcard(['study'], '*A*')], + }); + assert.deepEqual(output.sqon, { + op: 'and', + content: [ + { op: 'in', content: { fieldName: 'donor.sex', value: ['Male'] } }, + { op: 'wildcard', content: { fieldNames: ['study'], value: '*A*' } }, + ], + }); + }); + + // reduceSqon has no merge rule for wildcard, so two text searches on the same fields stay + // separate rather than being collapsed the way two `in` clauses would be. + test('keeps two wildcard clauses on the same fields separate', async () => { + const { output } = await buildSqon({ + catalogueId: 'participants', + combination: 'and', + clauses: [wildcard(['study'], '*A*'), wildcard(['study'], '*B*')], + }); + assert.deepEqual(output.sqon, { + op: 'and', + content: [ + { op: 'wildcard', content: { fieldNames: ['study'], value: '*A*' } }, + { op: 'wildcard', content: { fieldNames: ['study'], value: '*B*' } }, + ], + }); + }); + + test('summarizes a wildcard clause with display names joined by "or"', async () => { + const { output } = await buildSqon({ + catalogueId: 'participants', + combination: 'and', + clauses: [wildcard(['study', 'donor.sex'], '*A*')], + }); + assert.equal(output.summary, 'Study or Biological Sex matches "*A*"'); + }); + + test('rejects a wildcard on a field type the catalogue withholds it from', async () => { + const message = await expectError({ + catalogueId: 'participants', + combination: 'and', + clauses: [wildcard(['donor.age_at_diagnosis'], '*4*')], + }); + assert.ok(message.includes('operator "wildcard" is not valid for field "donor.age_at_diagnosis"')); + assert.ok(message.includes('(type "long")')); + }); + + test('reports every invalid field in one wildcard clause, as one clause error', async () => { + const message = await expectError({ + catalogueId: 'participants', + combination: 'and', + clauses: [wildcard(['study', 'not.a.field', 'donor.age_at_diagnosis'], '*A*')], + }); + assert.ok(message.includes('unknown field "not.a.field"')); + assert.ok(message.includes('operator "wildcard" is not valid for field "donor.age_at_diagnosis"')); + assert.equal(message.split('clauses[').length - 1, 1, 'one clause should report one error'); + }); + + test('notes that a wildcard value without "*" matches the whole field, not a substring', async () => { + const { output } = await buildSqon({ + catalogueId: 'participants', + combination: 'and', + clauses: [wildcard(['study'], 'A')], + }); + const notes = output.notes as string[]; + assert.ok(notes.some((note) => note.includes('contain no "*"'))); + }); + + test('adds no such note when the value carries a wildcard character', async () => { + const withStar = await buildSqon({ + catalogueId: 'participants', + combination: 'and', + clauses: [wildcard(['study'], '*A*')], + }); + assert.equal(withStar.output.notes, undefined); + + const withQuestionMark = await buildSqon({ + catalogueId: 'participants', + combination: 'and', + clauses: [wildcard(['study'], 'A?')], + }); + assert.equal(withQuestionMark.output.notes, undefined); + }); + + test('accepts a wildcard clause inside existingSqon and extends it', async () => { + const { output } = await buildSqon({ + catalogueId: 'participants', + combination: 'and', + existingSqon: { op: 'wildcard', content: { fieldNames: ['study'], value: '*A*' } }, + clauses: [inClause('donor.sex', ['Male'])], + }); + assert.deepEqual(output.sqon, { + op: 'and', + content: [ + { op: 'wildcard', content: { fieldNames: ['study'], value: '*A*' } }, + { op: 'in', content: { fieldName: 'donor.sex', value: ['Male'] } }, + ], + }); + }); +}); + +suite('build_sqon set-membership operators', () => { + test('builds an "all" clause requiring every value', async () => { + const { output } = await buildSqon({ + catalogueId: 'participants', + combination: 'and', + clauses: [{ fieldName: 'study', operator: 'all', value: ['A', 'B'] }], + }); + assert.deepEqual(output.sqon, { + op: 'and', + content: [{ op: 'all', content: { fieldName: 'study', value: ['A', 'B'] } }], + }); + assert.equal(output.summary, 'Study includes all of "A" or "B"'); + }); + + test('builds a "some-not-in" clause', async () => { + const { output } = await buildSqon({ + catalogueId: 'participants', + combination: 'and', + clauses: [{ fieldName: 'study', operator: 'some-not-in', value: ['A'] }], + }); + assert.deepEqual(output.sqon, { + op: 'and', + content: [{ op: 'some-not-in', content: { fieldName: 'study', value: ['A'] } }], + }); + }); + + test('rejects negate on "some-not-in", which is already negative', async () => { + const message = await expectError({ + catalogueId: 'participants', + combination: 'and', + clauses: [{ fieldName: 'study', operator: 'some-not-in', value: ['A'], negate: true }], + }); + assert.ok(message.includes('double negative')); + }); + + test('rejects "all" and "some-not-in" on a field type the catalogue withholds them from', async () => { + for (const operator of ['all', 'some-not-in']) { + const message = await expectError({ + catalogueId: 'participants', + combination: 'and', + clauses: [{ fieldName: 'donor.age_at_diagnosis', operator, value: [40] }], + }); + assert.ok( + message.includes(`operator "${operator}" is not valid for field "donor.age_at_diagnosis"`), + `${operator} should be rejected on a long field`, + ); + } + }); +}); + +suite('build_sqon asterisk in a term-matched value', () => { + test('rejects an asterisk in an in-like value and points at the wildcard operator', async () => { + const message = await expectError({ + catalogueId: 'participants', + combination: 'and', + clauses: [inClause('study', ['*TP53*'])], + }); + assert.ok(message.includes('contains "*"')); + assert.ok(message.includes('regular expression')); + assert.ok(message.includes('"wildcard"')); + }); + + test('checks every value, not only the first', async () => { + const message = await expectError({ + catalogueId: 'participants', + combination: 'and', + clauses: [inClause('study', ['A', 'B*'])], + }); + assert.ok(message.includes('value "B*"')); + }); + + test('applies to every term-matched operator', async () => { + for (const operator of ['in', 'not-in', 'some-not-in', 'all']) { + const message = await expectError({ + catalogueId: 'participants', + combination: 'and', + clauses: [{ fieldName: 'study', operator, value: ['*A*'] }], + }); + assert.ok(message.includes('contains "*"'), `${operator} should reject an asterisked value`); + } + }); + + // A set reference and a missing-field sentinel are the other two magic in-like values, and + // neither contains an asterisk, so neither is caught by this check. + test('leaves set references and the missing-field sentinel alone', async () => { + const { output } = await buildSqon({ + catalogueId: 'participants', + combination: 'and', + clauses: [inClause('study', ['set_id:abc', '__missing__'])], + }); + assert.deepEqual(output.sqon, { + op: 'and', + content: [{ op: 'in', content: { fieldName: 'study', value: ['set_id:abc', '__missing__'] } }], + }); + }); + + test('leaves an asterisk in a wildcard value alone, which is where it belongs', async () => { + const { output } = await buildSqon({ + catalogueId: 'participants', + combination: 'and', + clauses: [{ fieldNames: ['study'], operator: 'wildcard', value: '*TP53*' }], + }); + assert.equal(output.filterCount, 1); + }); }); suite('build_sqon clause validation', () => { @@ -607,7 +969,7 @@ suite('build_sqon clause validation', () => { { fieldName: 'donor.age_at_diagnosis', operator: 'gt', value: '40' }, ], }); - assert.ok(message.includes('clauses[0]: Unknown field "not.a.field"')); + assert.ok(message.includes('clauses[0]: unknown field "not.a.field"')); assert.ok(message.includes('clauses[2]: ')); assert.ok(!message.includes('clauses[1]: ')); }); diff --git a/apps/mcp-server/src/mcp/buildSqonTool.ts b/apps/mcp-server/src/mcp/buildSqonTool.ts index 94a99983a..7e686e9fc 100644 --- a/apps/mcp-server/src/mcp/buildSqonTool.ts +++ b/apps/mcp-server/src/mcp/buildSqonTool.ts @@ -10,48 +10,79 @@ import { type SqonNode, type SqonScalarOrArray, SqonSchema, + type TextFilter, } from '@overture-stack/sqon'; import { z as zod } from 'zod'; import { validateClauses } from '#arranger/clauseValidation.js'; import { type ArrangerClient, ArrangerRequestError } from '#arranger/client.js'; -import { validateSqon, type CatalogueQueryContext } from '#arranger/queryValidation.js'; +import { validateSqon, validateSqonFields, type CatalogueQueryContext } from '#arranger/queryValidation.js'; import { countFilterClauses, summarizeSqon } from '#arranger/sqonSummary.js'; import { catalogueIntrospectionSchema, type ArrangerCatalogueIntrospection } from '#arranger/types.js'; import { type McpServerDeps } from '#server.js'; import { type ArrangerMcpConfig } from '#utils/config.js'; /** - * v1 operators, grouped by the shape of their input values, to match the branching of the + * Operators grouped by the shape of their input values, to match the branching of the * discriminated union of the clause schema: * - in-like operators take a scalar or an array * - range operators take one bound * - between takes exactly two + * - all takes an array, never a bare scalar + * - wildcard takes one search string, and names its fields with `fieldNames` (plural) + * + * Canonical names only, no aliases (`=`, `>=`, `filter`): `addFilterClause` dispatches scalar + * operators on the literal operator string and returns `undefined` for an alias, so accepting one + * would silently drop the clause rather than build an equivalent SQON. + * + * `fuzzy` is deliberately absent. It has no implementation in `modules/sqon`, and the text branch + * of `addFilterClause` ignores `operator` entirely, so a `fuzzy` clause there builds a `wildcard` + * clause with no error: listing it would offer an operator that silently runs a different query. */ -const IN_LIKE_OPERATORS = ['in', 'not-in'] as const; +const IN_LIKE_OPERATORS = ['in', 'not-in', 'some-not-in'] as const; const RANGE_OPERATORS = ['gt', 'gte', 'lt', 'lte'] as const; const BETWEEN_OPERATOR = 'between' as const; +const ALL_OPERATOR = 'all' as const; +const WILDCARD_OPERATOR = 'wildcard' as const; /** - * Accepted operators for v1 of `build_sqon` (single-field scalar operators only). - * Canonical names only, no aliases. Used by tests to assert each accepted operator is described - * exactly once, and no rejected one is described at all. + * Every operator `build_sqon` accepts, derived from the per-shape groups rather than restated, so + * the aggregate cannot drift from what the schema actually takes. Used by tests to assert each + * accepted operator is described exactly once, and no rejected one is described at all. */ -export const BUILD_SQON_OPERATORS = [...IN_LIKE_OPERATORS, ...RANGE_OPERATORS, BETWEEN_OPERATOR] as const; +export const BUILD_SQON_OPERATORS = [ + ...IN_LIKE_OPERATORS, + ...RANGE_OPERATORS, + BETWEEN_OPERATOR, + ALL_OPERATOR, + WILDCARD_OPERATOR, +] as const; /** * Generates a description for the `operator` input of the `build_sqon` tool. Generated per-branch, * rather than hard-coded once to include all operator, in order to reduce context bloat. + * + * An `applicableTo` of `all` is rendered by saying nothing about field types, rather than by + * claiming "any field type", which would be wrong. `modules/sqon` reports the field types an + * operator generically applies to, while a catalogue advertises its own per-type operator lists, + * and the two disagree: `wildcard`, `all`, and `some-not-in` are all `all` here but are withheld + * from range-typed fields (and, for the latter two, from text fields) by catalogue introspection, + * which is what `validateClauses` enforces. Staying silent keeps this text honest without copying + * that classification into this package (tracked tech-debt), and the `clauses` array description + * already names the catalogue as the authority on which operators a field accepts, once, rather + * than repeating it on every unrestricted operator here. + * * @param operators - The operators this union branch accepts. - * @returns A lead sentence followed by one line per operator, naming its field types and value type. + * @returns A lead sentence followed by one line per operator, naming its value type and, where + * `modules/sqon` restricts it, its field types. */ export const describeOperators = (operators: readonly string[]): string => { const operatorsSet = new Set(operators); const operatorDescriptions = getSqonFieldOperatorDetails() .filter(({ op }) => operatorsSet.has(op)) .map(({ applicableTo, op, valueType }) => { - const fieldTypes = applicableTo === 'all' ? 'any field type' : applicableTo.join(', '); - return `- "${op}": applies to ${fieldTypes}; value is ${valueType}`; + const fieldTypes = applicableTo === 'all' ? '' : `applies to ${applicableTo.join(', ')}; `; + return `- "${op}": ${fieldTypes}value is ${valueType}`; }); return [ @@ -82,6 +113,19 @@ const clauseBase = () => ({ ), }); +const textClauseBase = () => ({ + fieldNames: zod + .array(zod.string().min(1)) + .min(1) + .describe( + 'Dot-notation field names from get_catalogue_fields, searched together: a document matches when any one of them matches. Use fieldNames (plural) only for text search; every other operator takes fieldName (singular).', + ), + negate: zod + .boolean() + .optional() + .describe('Wrap this one clause in a "not". This is how to express "does not contain".'), +}); + const clauseSchema = () => zod.discriminatedUnion('operator', [ zod.object({ @@ -102,6 +146,24 @@ const clauseSchema = () => .length(2) .describe('Exactly two bounds, [min, max], ascending and inclusive at both ends.'), }), + zod.object({ + ...clauseBase(), + operator: zod.literal(ALL_OPERATOR).describe(describeOperators([ALL_OPERATOR])), + value: zod + .array(scalarValue()) + .min(1) + .describe('Every value the field must contain. An array even for one value, never a bare scalar.'), + }), + zod.object({ + ...textClauseBase(), + operator: zod.literal(WILDCARD_OPERATOR).describe(describeOperators([WILDCARD_OPERATOR])), + value: zod + .string() + .min(1) + .describe( + 'The search string, matched case-insensitively against the whole field value. Include "*" for substring search: "*TP53*" finds a value containing TP53, while "TP53" matches only a value that is exactly TP53. "?" matches one character.', + ), + }), ]); const inputSchema = { @@ -229,6 +291,93 @@ type BuildSqonClause = zod.infer>; /** Wraps a single value in an array; passes through arrays unchanged. Not exported by `@overture-stack/sqon`. */ const asArray = (value: T | T[]): T[] => (Array.isArray(value) ? value : [value]); +/** + * An `existingSqon` input after validation: the parsed, normalized node when it is usable, plus + * every problem found with it. The two are independent by design, so the caller can collect these + * errors alongside the clause errors and report the whole batch at once. + * + * `sqon` is absent whenever `errors` is non-empty, and a caller must never fold an `existingSqon` + * that came back with errors: for a structural failure there is nothing to fold, and folding past a + * catalogue mismatch would build a SQON the target catalogue cannot run. + * + * `catalogueMismatch` separates the two failures, because they take different advice. A SQON naming + * fields this catalogue does not have is usually one built for another catalogue, and the fix is to + * rebuild. A value that is not a SQON at all carries its own remedy in its message, and telling the + * caller to consider which catalogue it came from would point at the wrong thing. + */ +type ExistingSqonResolution = { sqon?: SqonNode; errors: string[]; catalogueMismatch: boolean }; + +/** + * Validates the optional `existingSqon` input against the shared SQON schema and then against the + * catalogue, without stopping the caller from validating the new clauses too. + * + * The catalogue check runs here, before the fold, rather than on the folded result: a fold never + * invents a field name or rewrites a leaf's operator, so every leaf of the output comes from either + * this input or a clause, and checking both inputs separately catches the same problems one + * round-trip earlier. Checking the folded SQON instead meant a call carrying both an invalid clause + * and an `existingSqon` from another catalogue only ever reported the clauses, hiding the mismatch + * behind a resubmission. + * + * Structural failure short-circuits the catalogue check, since there is no tree to walk, but it is + * still returned as an error rather than thrown, so the caller can report it next to clause errors. + * + * @param raw - The unvalidated `existingSqon` argument, or undefined when the caller omitted it. + * @param context - The target catalogue's fields and per-type operator rules from introspection. + * + * @returns The normalized SQON to fold onto, or the reasons it cannot be used. + */ +const resolveExistingSqon = (raw: unknown, context: CatalogueQueryContext): ExistingSqonResolution => { + if (raw === undefined) { + return { catalogueMismatch: false, errors: [] }; + } + + const parsed = SqonSchema.safeParse(raw); + if (!parsed.success) { + const issues = parsed.error.issues.map((issue) => ` - at ${issue.path.join('.') || 'root'}: ${issue.message}`); + return { + catalogueMismatch: false, + errors: [ + `existingSqon is not a valid SQON. Pass the "sqon" value from an earlier build_sqon response unchanged, or omit existingSqon to start a new query.\n${issues.join('\n')}`, + ], + }; + } + + const sqon = normalizeSqonNode(parsed.data); + // Normalized first, so an operator alias in an existing SQON is checked in its canonical form + // rather than rejected as an operator the catalogue does not advertise. + const errors = validateSqonFields(sqon, context, { subject: 'existingSqon' }); + + return errors.length > 0 ? { catalogueMismatch: true, errors } : { catalogueMismatch: false, sqon, errors }; +}; + +/** + * Composes the single error result for a call whose inputs did not validate, listing every problem + * found across `existingSqon` and the clauses so the whole batch can be fixed in one resubmission. + * + * @param errors - Every validation message, `existingSqon` first. + * @param catalogueId - The catalogue the call targeted, named in the rebuild advice. + * @param catalogueMismatch - Whether `existingSqon` named fields this catalogue does not have, the + * one failure the rebuild advice speaks to. Withheld otherwise, since it is misdirection when the + * clauses alone are at fault, or when `existingSqon` is not a SQON at all. + * + * @returns The message body for the error result. + */ +const composeValidationError = ({ + catalogueId, + catalogueMismatch, + errors, +}: { + catalogueId: string; + catalogueMismatch: boolean; + errors: string[]; +}): string => { + const rebuildAdvice = catalogueMismatch + ? `\nIf existingSqon came from a different catalogue, drop it and rebuild the query for "${catalogueId}".` + : ''; + + return `No SQON was built. Fix everything listed, then resubmit the whole batch:\n${errors.join('\n')}${rebuildAdvice}`; +}; + /** * If `sqon` already carries a plain (non-negated) "in" leaf on `fieldName`, with no `pivot`, * returns a new sqon with that leaf's value unioned with `value`. Returns `undefined` when there @@ -285,8 +434,8 @@ const mergeIntoExistingInClause = ( * Folds every clause into one SQON, making one `addFilterClause` call per clause, except a plain * "in" clause that matches a field already present, which is merged into the existing leaf's * value instead (see `mergeIntoExistingInClause`). Internal to the handler: the model only ever - * sees the single `build_sqon` call. `reduceSqon` still runs inside each `addFilterClause` fold, - * so every other equivalent-clause merge (`not-in`, `all`, range bounds) still happens as before. + * sees the single `build_sqon` call. `reduceSqon` still runs inside each fold, so every other + * equivalent-clause merge (`not-in`, `all`, range bounds) still happens as before. * * @param input.clauses - The list of clauses provided to the `build_sqon` tool * @param input.combination - The combination operator provided to the `build_sqon` tool @@ -315,20 +464,36 @@ const foldClauses = ({ } } - const params: ScalarFilter = { - combination, - existing: sqon, - fieldName: clause.fieldName, - negate: clause.negate ?? false, - operator: clause.operator, - value: clause.value, - }; - - // `addFilterClause` dispatches scalar operators through a switch with no default, and - // returns undefined for anything outside it: a text operator, an operator alias, or an - // operator added to modules/sqon but not to buildScalarClause. - const next = addFilterClause(params); - // for v1 of build_sqon, this is unreachable. This guard exists as a failsafe for v2 + const shared = { combination, existing: sqon, negate: clause.negate ?? false }; + + // Two calls rather than one call on a union-typed object: `addFilterClause` is overloaded, + // and an overloaded signature will not accept `ScalarFilter | TextFilter`. Each argument is + // still checked against its own overload, so a signature change in modules/sqon breaks this + // at compile time. + // + // Dispatched on `operator`, the input union's own discriminator, rather than on whether a + // `fieldNames` key is present: an explicitly-present `undefined` key makes a key-presence + // test answer wrongly, and the operator is what actually decides the shape. + // + // `addFilterClause` dispatches scalar operators through a switch with no default and returns + // undefined for anything outside it: an operator alias, or an operator added to modules/sqon + // but not to buildScalarClause. It cannot catch a bad text operator, because its text branch + // ignores `operator` and builds a wildcard clause regardless, which is one of the reasons + // `fuzzy` stays out of the enum above. + const next = + clause.operator === WILDCARD_OPERATOR + ? addFilterClause({ + ...shared, + fieldNames: clause.fieldNames, + operator: clause.operator, + value: clause.value, + } satisfies TextFilter) + : addFilterClause({ + ...shared, + fieldName: clause.fieldName, + operator: clause.operator, + value: clause.value, + } satisfies ScalarFilter); if (next === undefined) { throw new Error(`clauses[${index}]: operator "${clause.operator}" produced no filter clause.`); } @@ -378,6 +543,7 @@ export const registerBuildSqonTool = (server: McpServer, { client, config }: Mcp 'Before calling this tool you MUST call get_catalogue_fields for the catalogue, to get valid field names and the operators each field type accepts. ' + 'State your understanding of the query in plain English and confirm it with the user before calling: this tool does not ask them to confirm. ' + 'Submit every condition as one call with multiple clauses, not one call per condition. ' + + 'For substring or text search across one or more fields, use the "wildcard" operator with fieldNames (plural) rather than putting "*" in a value. ' + 'This tool only builds a filter. Pass the returned "sqon" to execute_query, unchanged, to run it.' + 'For an unfiltered query (i.e. "show me all data"), skip this tool and pass {"op":"and","content":[]} to execute_query.', inputSchema, @@ -392,53 +558,66 @@ export const registerBuildSqonTool = (server: McpServer, { client, config }: Mcp const { fields, operators } = resolution.introspection; const context: CatalogueQueryContext = { fields, operators }; - let existingSqon: SqonNode | undefined; - if (rawExistingSqon !== undefined) { - const parsed = SqonSchema.safeParse(rawExistingSqon); - if (!parsed.success) { - const issues = parsed.error.issues.map( - (issue) => `- at ${issue.path.join('.') || 'root'}: ${issue.message}`, - ); - return errorResult( - `existingSqon is not a valid SQON. Pass the "sqon" value from an earlier build_sqon response unchanged, or omit existingSqon to start a new query.\n${issues.join('\n')}`, - ); - } - existingSqon = normalizeSqonNode(parsed.data); - } - - const clauseErrors = validateClauses(clauses, context); - if (clauseErrors.length > 0) { + // Both inputs are validated before either is acted on, and their errors are reported + // together: an invalid clause and an unusable existingSqon in the same call are one + // resubmission to fix, not two. `existingSqon` errors lead, since a base query built for + // another catalogue has to be dropped before the clause fixes are worth making. + const existing = resolveExistingSqon(rawExistingSqon, context); + const errors = [...existing.errors, ...validateClauses(clauses, context)]; + if (errors.length > 0) { return errorResult( - `No SQON was built. Fix every clause listed, then resubmit the whole batch:\n${clauseErrors.join('\n')}`, + composeValidationError({ + catalogueId, + catalogueMismatch: existing.catalogueMismatch, + errors, + }), ); } - const sqon = normalizeRoot(foldClauses({ clauses, combination, existingSqon })); + const sqon = normalizeRoot(foldClauses({ clauses, combination, existingSqon: existing.sqon })); - // Catches what the input schema cannot: fields referenced by existing_sqon that this - // catalogue does not have, and any structural surprise from the fold itself. + // A failsafe, not a user-facing check: every field name and operator in this SQON was + // already validated on the way in, as either a clause or part of existingSqon, so a + // failure here means the fold itself produced something the catalogue cannot run. + // Reaching it is a defect in this tool rather than a fixable input, which is why the + // message says not to retry. Kept because v2 and v3 add folds this cannot yet see. const validation = validateSqon(sqon, context); if (!validation.valid) { return errorResult( - `The clauses were valid individually, but the resulting SQON is not:\n- ${validation.errors.join('\n- ')}\nIf existingSqon came from a different catalogue, rebuild the query for "${catalogueId}" instead of extending it.`, + `build_sqon combined valid inputs into an invalid SQON, so nothing was returned:\n- ${validation.errors.join('\n- ')}\nThis is a defect in the tool, not in the request: resubmitting the same inputs will not help. Tell the user what happened rather than retrying.`, ); } - const submittedCount = clauses.length + (existingSqon ? countFilterClauses(existingSqon) : 0); + const submittedCount = clauses.length + (existing.sqon ? countFilterClauses(existing.sqon) : 0); const filterCount = countFilterClauses(sqon); - const notes = - filterCount < submittedCount - ? [ - `${submittedCount} filter clauses reduced to ${filterCount}: clauses on the same field and operator were merged into one. The summary describes the merged query.`, - ] - : undefined; + const notes: string[] = []; + + if (filterCount < submittedCount) { + notes.push( + `${submittedCount} filter clauses reduced to ${filterCount}: clauses on the same field and operator were merged into one. The summary describes the merged query.`, + ); + } + + // A wildcard value carrying no wildcard character is matched against the whole field + // value, so it finds an exact term rather than a substring. That is a legitimate query, + // which is why this is a note rather than a rejection, but it is rarely what a text + // search was reaching for and the difference is invisible in the result. + const exactTermSearches = clauses.filter( + (clause) => clause.operator === WILDCARD_OPERATOR && !/[*?]/.test(clause.value), + ); + if (exactTermSearches.length > 0) { + const values = exactTermSearches.map((clause) => `"${clause.value}"`).join(', '); + notes.push( + `Wildcard ${exactTermSearches.length === 1 ? 'value' : 'values'} ${values} contain no "*", so ${exactTermSearches.length === 1 ? 'it matches' : 'they match'} only a field equal to the whole string, not one containing it. Rebuild with "*" around the term if a substring search was meant.`, + ); + } return successResult({ sqon, summary: summarizeSqon(sqon, fields), clauseCount: submittedCount, filterCount, - ...(notes ? { notes } : {}), + ...(notes.length > 0 ? { notes } : {}), }); } catch (error) { return errorResult( diff --git a/docs/concepts.md b/docs/concepts.md index 007aaecd2..9cf9ed85c 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -54,9 +54,9 @@ A **filter clause** is a single field-level condition: } ``` -Within a filter clause, the `content` object identifies which field the condition applies to. For most operators this is a single `fieldName` property (a string): the name of one index field. Text-search operators (`wildcard`, `fuzzy`) instead use `fieldNames` (a string array), because they match a single value against multiple fields simultaneously. +Within a filter clause, the `content` object identifies which field the condition applies to. For most operators this is a single `fieldName` property (a string): the name of one index field. The `wildcard` text-search operator instead uses `fieldNames` (a string array), because it matches a single value against multiple fields simultaneously. A planned `fuzzy` operator will take the same `fieldNames` shape; it is not implemented yet. -`fieldName` and `fieldNames` are property names within a filter clause's `content` object, not references to a field as a catalogue-configuration concept. Do not abbreviate either to `field` in code, comments, parameters, or documentation: `field` is ambiguous, while `fieldName` and `fieldNames` are unambiguous names for specific properties in the SQON schema. +`fieldName` and `fieldNames` are names for properties of the SQON schema, not references to a field as a catalogue-configuration concept. Do not abbreviate either to `field` in code, comments, parameters, or documentation: `field` is ambiguous, while `fieldName` and `fieldNames` are unambiguous. A SQON wraps one or more filter clauses under a combinator: @@ -89,5 +89,5 @@ The word **filter** is used two ways: as a verb ("users filter the dataset") and | **filter clause** | One field-level condition within a SQON (a single `{op, content}` leaf node). | | **filter** | (verb) To narrow a dataset by selecting facet options. (noun) A SQON, or informally a single filter clause. | | **`fieldName`** | The string property in a filter clause's `content` that names the single index field the condition applies to. Used by most operators. Never abbreviate to `field`. | -| **`fieldNames`** | The string-array property used instead of `fieldName` by multi-field text operators (`wildcard`, `fuzzy`). Matches one value against all listed fields simultaneously. Never abbreviate to `fields` or `field`. | +| **`fieldNames`** | The string-array property used instead of `fieldName` by the multi-field `wildcard` text operator (and by the planned, not-yet-implemented `fuzzy`). Matches one value against all listed fields simultaneously. Never abbreviate to `fields` or `field`. | | **settings** | Elasticsearch's own term for index-level configuration (the ES `settings` API). Use "configuration" for Arranger-level concepts; keep "settings" when mirroring ES language. | diff --git a/docs/mcp-server.md b/docs/mcp-server.md index ee8aa2bd0..c8894789f 100644 --- a/docs/mcp-server.md +++ b/docs/mcp-server.md @@ -37,14 +37,18 @@ The server returns a short set of usage instructions that most clients fold into - `list_catalogues`: returns the catalogues registered on this Arranger instance - `get_sqon_schema`: returns the SQON JSON Schema and operator metadata - `get_catalogue_fields`: returns field metadata for one catalogue (input: `catalogueId`) -- `build_sqon`: builds a validated SQON from plain field, operator, and value inputs, so a model never has to write SQON itself (input: `{ catalogueId, combination: 'and' | 'or', clauses: [{ fieldName, operator, value, negate? }], existingSqon? }`) +- `build_sqon`: builds a validated SQON from plain field, operator, and value inputs, so a model never has to write SQON itself (input: `{ catalogueId, combination: 'and' | 'or', clauses: [{ fieldName | fieldNames, operator, value, negate? }], existingSqon? }`) - `execute_query`: builds, confirms, and executes a SQON-filtered query against one catalogue (input: `{ catalogueId, sqon, queryType = 'hits', fields [], first = 20, offset = 0, sort, aggregationFields = [], includeMissing = true, aggregationsFilterThemselves = false }`) -`build_sqon` returns `{ sqon, summary, clauseCount, filterCount, notes? }` and executes nothing: pass its `sqon` to `execute_query` unchanged. Every clause is validated against the catalogue before a SQON is built, and one error is reported per invalid clause so a whole batch can be corrected in a single resubmission. `summary` is a plain-English rendering of the built SQON, using the catalogue's display names, meant to be read back to the user for confirmation. `clauseCount` and `filterCount` differ when equivalent clauses on the same field merged during the build (two lower bounds on one field collapse to the stricter one, for example); `notes` explains the difference when they do. +`build_sqon` returns `{ sqon, summary, clauseCount, filterCount, notes? }` and executes nothing. Pass the resulting `sqon` to `execute_query` unchanged. Every clause is validated against the catalogue before a SQON is built, and one error is reported per invalid clause so that a whole batch can be corrected in a single resubmission. `summary` is a plain-English rendering of the built SQON, using the catalogue's display names, meant to be read back to the user for confirmation. `clauseCount` and `filterCount` differ when equivalent clauses on the same field merged during the build (two lower bounds on one field collapse to the stricter one, for example); `notes` explains the difference when they do. -Two `in` clauses on the same field also merge, by combining their value lists: `status in ['active']` together with `status in ['pending']` becomes `status in ['active', 'pending']`, meaning "either". That is the correct reading on a single-valued field, where no document could satisfy both clauses at once. It is not conditional on the field's `isArray` yet, so on a field that can hold several values at once (`isArray: true`, or `null` where nothing declared it) the other reading, "every one of these must be present", is equally legitimate and the merge silently picks "either" regardless. `build_sqon` cannot express "every one of these" in v1: check `isArray` through `get_catalogue_fields`, and when you need that reading, hand-write a SQON using `all` and pass it to `execute_query` directly. +Most clauses name one field with `fieldName` and take a value operator: `in`, `not-in`, `some-not-in`, `all`, `gt`, `gte`, `lt`, `lte`, `between`. A `wildcard` clause is the exception: it names several fields with `fieldNames` (plural) and matches when any one of them matches. Include `*` for a substring search, since `"TP53"` matches only a value that is exactly TP53 while `"*TP53*"` matches one containing it; `negate: true` expresses "does not contain". Which operators a field accepts is decided by the catalogue, not this tool, so read `operators` from `get_catalogue_fields`. -Version 1 accepts the scalar operators (`in`, `not-in`, `gt`, `gte`, `lt`, `lte`, `between`) and one `combination` for the whole call. Text-search operators and mixed AND/OR nesting are not yet supported: a query needing either still requires a hand-written `sqon` passed straight to `execute_query`. An unfiltered query needs no `build_sqon` call at all; pass `{"op":"and","content":[]}` to `execute_query` directly. +An asterisk inside an `in`, `not-in`, `some-not-in`, or `all` value is rejected, because Arranger runs such a value as a regular expression rather than matching it literally: use `wildcard` instead. `execute_query`'s raw `sqon` parameter still accepts it, so an asterisk-bearing keyword value is reachable there but not through `build_sqon`. + +Two `in` clauses on the same field also merge, by combining their value lists: `status in ['active']` together with `status in ['pending']` becomes `status in ['active', 'pending']`, meaning "either". That is the correct reading on a single-valued field, where no document could satisfy both clauses at once. It is not conditional on the field's `isArray` yet, so on a field that can hold several values at once (`isArray: true`, or `null` where nothing declared it) the other reading, "every one of these must be present", is equally legitimate and the merge silently picks "either" regardless. Use the `all` operator directly when you need that reading. + +One `combination` applies to the whole call. Mixed AND/OR nesting and the planned `fuzzy` operator are not yet supported: a query needing either still requires a hand-written `sqon`. An unfiltered query needs no `build_sqon` call at all; pass `{"op":"and","content":[]}` to `execute_query` directly. **Resources** (readable data by URI): @@ -74,7 +78,7 @@ For **LM Studio** and other model hosts, follow the client's documentation to ad A model connected over MCP should not construct SQON at all: `build_sqon` does it, from field, operator, and value inputs the model selects out of `get_catalogue_fields`. That is the whole point of the tool, so the rules below are enforced rather than merely documented, and a mistake is reported per clause instead of surfacing as an Arranger query error. -The rest of this section is for a client constructing SQON directly, without the MCP server: a script, a pipeline, or the two cases `build_sqon` does not yet cover (text-search operators, and mixing AND and OR in one query). Use the [introspection API](./reference/05-introspection.md) to derive field names, types, and valid operators at runtime rather than hard-coding them. This keeps the client current when a catalogue mapping changes. +The rest of this section is for a client constructing SQON directly, without the MCP server: a script, a pipeline, or the cases `build_sqon` does not yet cover (mixing AND and OR in one query, and the planned `fuzzy` operator). Use the [introspection API](./reference/05-introspection.md) to derive field names, types, and valid operators at runtime rather than hard-coding them. This keeps the client current when a catalogue mapping changes. Safe defaults for programmatic SQON construction: @@ -92,6 +96,6 @@ For a detailed walkthrough of the SQON format and how to compose queries, see [B ## What's coming -- **`build_sqon` text operators and mixed combinators**: version 1 covers scalar operators and one `and`/`or` per call; `wildcard` clauses and mixing AND and OR in one query are still to come +- **`build_sqon` mixed combinators and fuzzy search**: mixing AND and OR in one query, and the `fuzzy` (edit-distance) operator, are still to come - **Authentication**: the MCP server currently requires no auth; support is planned - **Chat interface**: a conversational front-end for non-technical users to search catalogues in plain language diff --git a/integration-tests/mcp-server/test/buildSqon.ts b/integration-tests/mcp-server/test/buildSqon.ts index cf91a42e3..03fd28b20 100644 --- a/integration-tests/mcp-server/test/buildSqon.ts +++ b/integration-tests/mcp-server/test/buildSqon.ts @@ -222,7 +222,7 @@ export default ({ getClient }: BuildSqonEnv) => { ); assert.match(text, /No SQON was built/); - assert.match(text, /clauses\[0\]: Unknown field "not_a_field"/); + assert.match(text, /clauses\[0\]: unknown field "not_a_field"/); assert.match(text, /clauses\[2\]: /); assert.ok(!text.includes('clauses[1]: '), 'expected the valid clause not to be reported'); }); @@ -236,7 +236,7 @@ export default ({ getClient }: BuildSqonEnv) => { }), ); - assert.match(text, /operator "gt" is not valid for field "vital_status"/); + assert.match(text, /clauses\[0\]: operator "gt" is not valid for field "vital_status"/); }); test("11.rejects an existing SQON built against another catalogue's fields", async () => { @@ -252,7 +252,7 @@ export default ({ getClient }: BuildSqonEnv) => { }), ); - assert.match(text, /unknown field "vital_status"/); + assert.match(text, /existingSqon references unknown field "vital_status"/); assert.match(text, /rebuild the query for "catalogue-b"/); }); @@ -286,7 +286,7 @@ export default ({ getClient }: BuildSqonEnv) => { assert.match(text, /'gte'/, 'expected the error to name the canonical operator the alias maps to'); }); - test('14.rejects the text operators, which this version of the tool does not build', async () => { + test('14.rejects a wildcard clause that names its field with the singular fieldName', async () => { const text = getErrorText( await callBuildSqon(getClient(), { catalogueId: 'catalogue-a', @@ -298,6 +298,18 @@ export default ({ getClient }: BuildSqonEnv) => { assert.match(text, /Invalid arguments for tool build_sqon/); }); + test('14a.rejects "fuzzy", which has no implementation to build', async () => { + const text = getErrorText( + await callBuildSqon(getClient(), { + catalogueId: 'catalogue-a', + combination: 'and', + clauses: [{ fieldNames: ['vital_status'], operator: 'fuzzy', value: 'ali' }], + }), + ); + + assert.match(text, /Invalid arguments for tool build_sqon/); + }); + test('15.rejects a batch with no clauses', async () => { const text = getErrorText( await callBuildSqon(getClient(), { catalogueId: 'catalogue-a', combination: 'and', clauses: [] }), @@ -404,4 +416,223 @@ export default ({ getClient }: BuildSqonEnv) => { ['b-001'], ); }); + + test('20.reports an unusable existingSqon and an invalid clause in the same response', async () => { + const text = getErrorText( + await callBuildSqon(getClient(), { + catalogueId: 'catalogue-b', + combination: 'and', + // Invalid for two independent reasons: the clause names a field of catalogue-a, and so + // does the existing SQON. + clauses: [{ fieldName: 'vital_status', operator: 'in', value: ['Alive'] }], + existingSqon: { + op: 'and', + content: [{ op: 'in', content: { fieldName: 'age_at_diagnosis', value: [40] } }], + }, + }), + ); + + assert.match(text, /existingSqon references unknown field "age_at_diagnosis"/); + assert.match(text, /clauses\[0\]: unknown field "vital_status"/); + assert.ok( + text.indexOf('existingSqon references') < text.indexOf('clauses[0]: '), + 'expected the existingSqon problem to be listed before the clause problems', + ); + }); + + test('21.builds a wildcard clause spanning several fields and summarizes them with "or"', async () => { + const built = getStructured( + await callBuildSqon(getClient(), { + catalogueId: 'catalogue-a', + combination: 'and', + clauses: [{ fieldNames: ['analysis_id', 'vital_status'], operator: 'wildcard', value: '*a-00*' }], + }), + ); + + assert.deepEqual(built.sqon, { + op: 'and', + content: [{ op: 'wildcard', content: { fieldNames: ['analysis_id', 'vital_status'], value: '*a-00*' } }], + }); + assert.match(built.summary, / or /, 'expected the summary to join the searched fields with "or"'); + }); + + test('22.a built wildcard SQON runs through execute_query and matches on a substring', async () => { + const built = getStructured( + await callBuildSqon(getClient(), { + catalogueId: 'catalogue-a', + combination: 'and', + clauses: [{ fieldNames: ['vital_status'], operator: 'wildcard', value: '*ceas*' }], + }), + ); + + const result = await getClient().callTool({ + name: 'execute_query', + arguments: { catalogueId: 'catalogue-a', sqon: built.sqon, fields: ['analysis_id'] }, + }); + assert.ok(!result.isError, `execute_query rejected a built wildcard SQON: ${JSON.stringify(result)}`); + const structured = result.structuredContent as ExecuteQueryStructured; + + // Matched case-insensitively against the whole value, so "*ceas*" finds both "Deceased" rows. + assert.equal(structured.total, 2); + assert.deepEqual((structured.hits ?? []).map((hit) => hit.analysis_id).sort(), ['a-002', 'a-005']); + }); + + test('23.a wildcard clause matches when any one of its fields matches', async () => { + const built = getStructured( + await callBuildSqon(getClient(), { + catalogueId: 'catalogue-a', + combination: 'and', + // Only analysis_id can match this pattern; vital_status never does. A clause spanning + // both must still return the analysis_id matches rather than requiring both to match. + clauses: [{ fieldNames: ['analysis_id', 'vital_status'], operator: 'wildcard', value: '*-004' }], + }), + ); + + const result = await getClient().callTool({ + name: 'execute_query', + arguments: { catalogueId: 'catalogue-a', sqon: built.sqon, fields: ['analysis_id'] }, + }); + assert.ok(!result.isError, `execute_query rejected a built wildcard SQON: ${JSON.stringify(result)}`); + const structured = result.structuredContent as ExecuteQueryStructured; + + assert.equal(structured.total, 1); + assert.deepEqual( + (structured.hits ?? []).map((hit) => hit.analysis_id), + ['a-004'], + ); + }); + + test('24.a negated wildcard excludes the matching documents', async () => { + const built = getStructured( + await callBuildSqon(getClient(), { + catalogueId: 'catalogue-a', + combination: 'and', + clauses: [{ fieldNames: ['vital_status'], operator: 'wildcard', value: '*ceas*', negate: true }], + }), + ); + + const result = await getClient().callTool({ + name: 'execute_query', + arguments: { catalogueId: 'catalogue-a', sqon: built.sqon, fields: ['analysis_id'] }, + }); + assert.ok(!result.isError, `execute_query rejected a negated wildcard SQON: ${JSON.stringify(result)}`); + const structured = result.structuredContent as ExecuteQueryStructured; + + assert.equal(structured.total, 3); + assert.deepEqual((structured.hits ?? []).map((hit) => hit.analysis_id).sort(), ['a-001', 'a-003', 'a-004']); + }); + + test('25.notes that a wildcard value carrying no "*" matches the whole field value', async () => { + const built = getStructured( + await callBuildSqon(getClient(), { + catalogueId: 'catalogue-a', + combination: 'and', + clauses: [{ fieldNames: ['vital_status'], operator: 'wildcard', value: 'Alive' }], + }), + ); + + assert.ok(built.notes, 'expected a note about the missing wildcard character'); + assert.ok( + built.notes.some((note) => note.includes('contain no "*"')), + `expected a missing-asterisk note, got: ${JSON.stringify(built.notes)}`, + ); + }); + + test('26.rejects a wildcard on a field type the catalogue withholds it from', async () => { + const text = getErrorText( + await callBuildSqon(getClient(), { + catalogueId: 'catalogue-a', + combination: 'and', + clauses: [{ fieldNames: ['age_at_diagnosis'], operator: 'wildcard', value: '*4*' }], + }), + ); + + assert.match(text, /operator "wildcard" is not valid for field "age_at_diagnosis"/); + }); + + test('27.rejects an asterisk in an in-like value, directing the caller to wildcard', async () => { + const text = getErrorText( + await callBuildSqon(getClient(), { + catalogueId: 'catalogue-a', + combination: 'and', + clauses: [{ fieldName: 'vital_status', operator: 'in', value: ['*ceas*'] }], + }), + ); + + assert.match(text, /contains "\*"/); + assert.match(text, /"wildcard"/); + }); + + test('28.builds "all" and "some-not-in" clauses on a keyword field', async () => { + const all = getStructured( + await callBuildSqon(getClient(), { + catalogueId: 'catalogue-a', + combination: 'and', + clauses: [{ fieldName: 'vital_status', operator: 'all', value: ['Alive'] }], + }), + ); + assert.deepEqual(all.sqon, { + op: 'and', + content: [{ op: 'all', content: { fieldName: 'vital_status', value: ['Alive'] } }], + }); + + const someNotIn = getStructured( + await callBuildSqon(getClient(), { + catalogueId: 'catalogue-a', + combination: 'and', + clauses: [{ fieldName: 'vital_status', operator: 'some-not-in', value: ['Alive'] }], + }), + ); + assert.deepEqual(someNotIn.sqon, { + op: 'and', + content: [{ op: 'some-not-in', content: { fieldName: 'vital_status', value: ['Alive'] } }], + }); + }); + + test('29.a built "all" SQON runs unchanged through execute_query', async () => { + const built = getStructured( + await callBuildSqon(getClient(), { + catalogueId: 'catalogue-a', + combination: 'and', + clauses: [{ fieldName: 'vital_status', operator: 'all', value: ['Deceased'] }], + }), + ); + + const result = await getClient().callTool({ + name: 'execute_query', + arguments: { catalogueId: 'catalogue-a', sqon: built.sqon, fields: ['analysis_id'] }, + }); + assert.ok(!result.isError, `execute_query rejected a built "all" SQON: ${JSON.stringify(result)}`); + const structured = result.structuredContent as ExecuteQueryStructured; + + assert.equal(structured.total, 2); + assert.deepEqual((structured.hits ?? []).map((hit) => hit.analysis_id).sort(), ['a-002', 'a-005']); + }); + + test('30.a multi-field wildcard SQON runs as an aggregations query', async () => { + const built = getStructured( + await callBuildSqon(getClient(), { + catalogueId: 'catalogue-a', + combination: 'and', + clauses: [{ fieldNames: ['analysis_id', 'vital_status'], operator: 'wildcard', value: '*a-00*' }], + }), + ); + + const result = await getClient().callTool({ + name: 'execute_query', + arguments: { + catalogueId: 'catalogue-a', + sqon: built.sqon, + queryType: 'aggregations', + aggregationFields: ['vital_status'], + }, + }); + assert.ok(!result.isError, `execute_query rejected a built wildcard SQON: ${JSON.stringify(result)}`); + const structured = result.structuredContent as ExecuteQueryStructured; + + const vitalStatus = structured.aggregations?.vital_status; + assert.ok(vitalStatus?.buckets, 'expected buckets for vital_status'); + const docCountsByKey = Object.fromEntries(vitalStatus.buckets.map((bucket) => [bucket.key, bucket.doc_count])); + assert.deepEqual(docCountsByKey, { Alive: 2, Deceased: 2, Unknown: 1 }); + }); }; diff --git a/integration-tests/mcp-server/test/executeQuery.ts b/integration-tests/mcp-server/test/executeQuery.ts index a07ee081f..4f4518ba2 100644 --- a/integration-tests/mcp-server/test/executeQuery.ts +++ b/integration-tests/mcp-server/test/executeQuery.ts @@ -247,7 +247,7 @@ export default ({ getClient, getServerUrl }: ExecuteQueryEnv) => { }); const text = getErrorText(result); - assert.match(text, /unknown field "vital_status"/); + assert.match(text, /SQON references unknown field "vital_status"/); assert.match(text, /get_catalogue_fields/); }); diff --git a/modules/sqon/src/builder/index.test.ts b/modules/sqon/src/builder/index.test.ts index 9bda46403..571a86386 100644 --- a/modules/sqon/src/builder/index.test.ts +++ b/modules/sqon/src/builder/index.test.ts @@ -485,6 +485,74 @@ suite('SQON builder', () => { assert.deepEqual(result, { op: 'lte', content: { fieldName: 'age', value: 65 } }); }); + // Date bounds are the ordinary shape of a range filter on a `date` field, and used to be + // coerced through Math.max/Math.min, which yields NaN and serializes to null: an ordinary + // date narrowing silently became a bound-less filter that fails schema validation. + test('keeps the later gt date under and', () => { + const result = SqonBuilder.gt('diagnosed', '2020-01-01') + .and(SqonBuilder.gt('diagnosed', '2021-06-15').toValue()) + .toValue(); + assert.deepEqual(result, { op: 'gt', content: { fieldName: 'diagnosed', value: '2021-06-15' } }); + }); + + test('keeps the earlier gt date under or', () => { + const result = SqonBuilder.or([ + SqonBuilder.gt('diagnosed', '2021-06-15').toValue(), + SqonBuilder.gt('diagnosed', '2020-01-01').toValue(), + ]).toValue(); + assert.deepEqual(result, { op: 'gt', content: { fieldName: 'diagnosed', value: '2020-01-01' } }); + }); + + test('keeps the earlier lte date under and', () => { + const result = SqonBuilder.lte('diagnosed', '2021-01-01') + .and(SqonBuilder.lte('diagnosed', '2020-01-01').toValue()) + .toValue(); + assert.deepEqual(result, { op: 'lte', content: { fieldName: 'diagnosed', value: '2020-01-01' } }); + }); + + test('keeps the later lte date under or', () => { + const result = SqonBuilder.or([ + SqonBuilder.lte('diagnosed', '2020-01-01').toValue(), + SqonBuilder.lte('diagnosed', '2021-01-01').toValue(), + ]).toValue(); + assert.deepEqual(result, { op: 'lte', content: { fieldName: 'diagnosed', value: '2021-01-01' } }); + }); + + test('merges date bounds a client supplied directly, not only builder-composed ones', () => { + const result = SqonBuilder.from({ + op: 'and', + content: [ + { op: 'lte', content: { fieldName: 'diagnosed', value: '2021-01-01' } }, + { op: 'lte', content: { fieldName: 'diagnosed', value: '2020-01-01' } }, + ], + }).toValue(); + assert.deepEqual(result, { op: 'lte', content: { fieldName: 'diagnosed', value: '2020-01-01' } }); + }); + + test('orders non-ISO date strings by parsed timestamp, not lexicographically', () => { + // Lexicographically '01/02/2021' sorts before '12/31/2020'; by date it is later. + const result = SqonBuilder.gt('diagnosed', '12/31/2020') + .and(SqonBuilder.gt('diagnosed', '01/02/2021').toValue()) + .toValue(); + assert.deepEqual(result, { op: 'gt', content: { fieldName: 'diagnosed', value: '01/02/2021' } }); + }); + + test('orders unparseable strings lexicographically rather than declining to merge', () => { + const result = SqonBuilder.gt('label', 'alpha').and(SqonBuilder.gt('label', 'beta').toValue()).toValue(); + assert.deepEqual(result, { op: 'gt', content: { fieldName: 'label', value: 'beta' } }); + }); + + test('keeps both range filters when the two bounds cannot be ordered against each other', () => { + const result = SqonBuilder.gt('age', 30).and(SqonBuilder.gt('age', '2020-01-01').toValue()).toValue(); + assert.deepEqual(result, { + op: 'and', + content: [ + { op: 'gt', content: { fieldName: 'age', value: 30 } }, + { op: 'gt', content: { fieldName: 'age', value: '2020-01-01' } }, + ], + }); + }); + test('unwraps a single-item and-combination to the item itself', () => { const inner = SqonBuilder.in('status', ['active']).toValue(); const result = SqonBuilder.and([inner]).toValue(); diff --git a/modules/sqon/src/builder/reduce.ts b/modules/sqon/src/builder/reduce.ts index 1c18a9577..7276137f3 100644 --- a/modules/sqon/src/builder/reduce.ts +++ b/modules/sqon/src/builder/reduce.ts @@ -1,4 +1,4 @@ -import type { SqonFieldFilter, SqonScalar } from '#builder/utils.js'; +import type { SqonFieldFilter, SqonScalar, SqonScalarOrArray } from '#builder/utils.js'; import type { SqonCombination, SqonNode } from '#schema/index.js'; import { asArray, isFieldFilter, isGroupNode } from '#builder/utils.js'; @@ -66,23 +66,76 @@ const deduplicateValues = (node: SqonNode): SqonNode => { return { ...node, content: { ...node.content, value: [...new Set(node.content.value)] } } as unknown as SqonNode; }; -/** Returns a new node that merges `incoming` into `existing` per the applicable reduction rule. */ -const mergeIntoExisting = (existing: SqonFieldFilter, incoming: SqonFieldFilter, combinationOp: string): SqonNode => { +/** + * Orders two range bounds: negative when `a` sorts before `b`, positive when it sorts after, `0` + * when they are equivalent, and `undefined` when the two cannot be ordered at all. + * + * Two numbers compare numerically. Two strings are the ordinary shape of a date bound, since + * `gt`/`gte`/`lt`/`lte` apply to `date` fields as well as numeric ones: they compare by parsed + * timestamp when both parse as dates, and lexicographically otherwise, which is also correct for + * an ISO 8601 string that `Date.parse` happens to reject. + * + * Anything else has no meaningful ordering here: a boolean, an array (which the range schemas + * permit even though a bound is conceptually scalar), or one bound of each type. Those return + * `undefined` so the caller keeps both clauses instead of merging them. Coercing them through + * `Math.max`/`Math.min` produced `NaN`, which serializes to `null` and silently replaced a real + * bound with an empty one. + */ +const compareBounds = (a: SqonScalarOrArray, b: SqonScalarOrArray): number | undefined => { + if (typeof a === 'number' && typeof b === 'number') { + return a - b; + } + + if (typeof a === 'string' && typeof b === 'string') { + const timeA = Date.parse(a); + const timeB = Date.parse(b); + if (!Number.isNaN(timeA) && !Number.isNaN(timeB)) { + return timeA - timeB; + } + return a < b ? -1 : a > b ? 1 : 0; + } + + return undefined; +}; + +/** + * Returns a new node that merges `incoming` into `existing` per the applicable reduction rule, or + * `undefined` when the rule cannot be applied because the two range bounds are not orderable. Only + * the range rules can decline; the value-merge rules concatenate and always apply. + */ +const mergeIntoExisting = ( + existing: SqonFieldFilter, + incoming: SqonFieldFilter, + combinationOp: string, +): SqonNode | undefined => { if (MERGE_VALUES_UNDER_OR_OPS.has(incoming.op) || MERGE_VALUES_UNDER_AND_OPS.has(incoming.op)) { - const merged = [...asArray(existing.content.value as SqonScalar[]), ...asArray(incoming.content.value as SqonScalar[])]; + const merged = [ + ...asArray(existing.content.value as SqonScalar[]), + ...asArray(incoming.content.value as SqonScalar[]), + ]; return { ...existing, content: { ...existing.content, value: merged } } as unknown as SqonNode; } - const a = existing.content.value as number; - const b = incoming.content.value as number; - const stricterIsGreater = combinationOp === 'and'; - - if (KEEP_MAX_UNDER_AND_OPS.has(incoming.op)) { - return { ...existing, content: { ...existing.content, value: stricterIsGreater ? Math.max(a, b) : Math.min(a, b) } } as unknown as SqonNode; + const a = existing.content.value; + const b = incoming.content.value; + const comparison = compareBounds(a, b); + if (comparison === undefined) { + return undefined; } - // KEEP_MIN_UNDER_AND_OPS - return { ...existing, content: { ...existing.content, value: stricterIsGreater ? Math.min(a, b) : Math.max(a, b) } } as unknown as SqonNode; + // Under `and` the stricter bound wins; under `or` the looser one does. `not` never reaches here: + // `shouldReduceOp` excludes range ops from `not` combinations before `mergeIntoExisting` is + // called. Which of the two is stricter flips with the operator: a greater floor is stricter for + // `gt`/`gte`, a lesser ceiling is stricter for `lt`/`lte`. + const stricterIsGreater = combinationOp === 'and'; + const keepGreater = KEEP_MAX_UNDER_AND_OPS.has(incoming.op) ? stricterIsGreater : !stricterIsGreater; + const greater = comparison >= 0 ? a : b; + const lesser = comparison >= 0 ? b : a; + + return { + ...existing, + content: { ...existing.content, value: keepGreater ? greater : lesser }, + } as unknown as SqonNode; }; /** @@ -104,9 +157,18 @@ const foldIntoOutput = (output: SqonCombination, reduced: SqonNode): void => { if (matchIdx >= 0) { const existing = output.content[matchIdx] as SqonFieldFilter; - // mergeIntoExisting doesn't dedupe its own result, so do it here. - output.content[matchIdx] = deduplicateValues(mergeIntoExisting(existing, reduced, output.op)); - return; + const merged = mergeIntoExisting(existing, reduced, output.op); + + // `undefined` means the two range bounds are not orderable (a boolean, an array, or + // a number against a non-parseable string), so both clauses are kept rather than + // collapsed into a corrupt one. That is safe under either combination: under `and` + // applying both is equivalent to applying the stricter one alone, and under `or` + // applying either is equivalent to the looser one. + if (merged !== undefined) { + // mergeIntoExisting doesn't dedupe its own result, so do it here. + output.content[matchIdx] = deduplicateValues(merged); + return; + } } } @@ -141,6 +203,11 @@ const foldIntoOutput = (output: SqonCombination, reduced: SqonNode): void => { * wins under `or` in both cases); `between` is kept as-is. See the `MERGE_VALUES_*`/`KEEP_*` sets * above for the per-op reasoning. * + * The four range ops compare date-string bounds as well as numeric ones, since they apply to + * `date` fields. Two bounds that cannot be ordered against each other (a boolean, an array, or + * one bound of each type) are left as two separate clauses rather than merged, which preserves + * the meaning under every combination type. + * * **Combination-node rules:** * - Empty inner combination: removed. * - Single-item `and`/`or` (unpivoted): unwrapped to its sole child.