#1080: ✨ Implement build_sqon v2 - #1093
Conversation
* Per feedback from PR#1091, reviewed and aligned the capitalization of "unknown" in MCP messages and tests * Golden rule going forward: capitalize only when the word begins the emitted message * Updated one message which previously wasn't following the rule, and added extra context to tests checking for lowercase "unknown" (i.e. added the beginning of their messages too) to prevent future confusion
…ngSqon` errors * Fixed a bug where errors with `existingSqon` (catalogue mismatch, invalid SQON) were not caught and returned in the same batch as clause errors, resulting in the need for multiple re-submissions to fix all errors * Updated MCP Server `queryValidation.ts` to split structural SQON validation (validateSqon) from semantic SQON validation (validateSqonFields), allowing consumers in the `build_sqon` flow to validate that `existingSqon` is valid AND belongs to the correct catalogue * Updated docs to reference new validation flow within `build_sqon` tool * Updated MCP Server integration tests to include testing the `build_sqon` error case of invalid clauses + invalid existingSqon
* Fixed a bug where merging range filters (`gt`/`gte`/`lt`/`lte`) with date-string values silently produced `null` instead of a comparison * Added `compareBounds`, ordering two bounds numerically, or by parsed timestamp when both are date strings, or lexicographically when both are strings `Date.parse` rejects * `mergeIntoExisting` now returns `undefined` when the two bounds cannot be ordered at all (a boolean, an array, or one bound of each type), and `reduceSqon` keeps both clauses in that case rather than collapsing them into a corrupt one. Safe under either combination: under `and` applying both equals applying the stricter one alone, and under `or` applying either equals the looser one * Added eight `reduceSqon` test cases * Closed the corresponding `.dev/tech-debt.md` entry
* Added the `wildcard` operator with the `fieldNames` (plural) shape, plus `all` and `some-not-in`, which `modules/sqon` already supported but v1 withheld * Rejected `*` inside `in`-like values, which Arranger silently runs as a regex, directing callers to `wildcard` instead * Extracted `checkFieldOperator` so clause validation and the `existingSqon`/`execute_query` SQON walk share one implementation of the field-and-operator rules * Stopped advertising unrestricted operators as applying to "any field type", which contradicted the per-type operator lists the catalogue actually enforces * Summarized multi-field text clauses with display names joined by "or", matching their any-field-matches semantics
…zzy` out into v2.1 * Updated the user-facing surfaces (`docs/mcp-server.md`, `apps/mcp-server/README.md`, `CHANGELOG.md`) for the new operator coverage, the `fieldNames` shape and its any-field-matches semantics, and the asterisk rejection's asymmetry with `execute_query`'s raw `sqon` path * Stopped `docs/concepts.md` presenting `fuzzy` as implemented, closing that tech-debt entry, and closed the `*`-in-`in`-values open question as resolved * Added tech-debt entries for the `applicableTo` divergence between `modules/sqon` and catalogue introspection, and for `integration-tests/mcp-server`'s tsconfig never typechecking app sources cleanly
2745143 to
1d66e30
Compare
# Conflicts: # CHANGELOG.md # apps/mcp-server/src/mcp/buildSqonTool.ts # docs/mcp-server.md # modules/sqon/src/builder/reduce.ts
justincorrigible
left a comment
justincorrigible
left a comment
There was a problem hiding this comment.
ready to squash. took the liberty to re-sync with main after some changes I made earlier, and then left comments here to illustrate how that rippled into your branch.
will open a new PR tomorrow with the rest of the related work, so that we can land this v2 part of the build_sqon 👍
| 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.'), | ||
| }), |
There was a problem hiding this comment.
no change needed in this PR
a follow-up PR stacked on this one adds a check here: refuse, or ask the caller to confirm, an all clause carrying more than one value unless the field's isArray is true. Flagging it now so it's visible before that PR is up.
context:
all can silently match nothing on a field that only ever holds one value, because it requires the field to contain every listed value at once, which is only possible if the field can actually hold more than one value at a time. If it can't, all with two or more values will never match anything, and this returns an empty result the same way a normal "no matches" query would, not an error...
rejoice! here's now a way to tell those two cases apart. my added merge resolving the conflicts with main brought in a new field-metadata property, isArray, reported per field by catalogue introspection: true means it can hold more than one value, false means it's declared to hold exactly one, and null means nothing declared either way. this clause here doesn't check it yet.
| // 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 }; |
There was a problem hiding this comment.
note from merge conflict resolution
main's union-merge fix grafted onto this branch's dispatch
the pre-check just above (ending in its own continue) is main's fix for the same issue reduceSqon used to over-merge: a plain in clause matching an existing field is "unioned" (aka "or") into it before this branch's wildcard/scalar dispatch below runs, so both changes apply rather than one overwriting the other. Verified: 250/250 apps/mcp-server tests pass, including this branch's wildcard/all/resolveExistingSqon coverage and main's union-merge tests together.
| * `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 = ( |
There was a problem hiding this comment.
note from merge conflict resolution
both sides combined here, not one replacing the other
the op-selection logic, foldIntoOutput, and pivot-aware matching come from main's already-merged fix (in no longer merges under and; nothing merges under not). the date-aware bound comparison (compareBounds, above) and mergeIntoExisting returning undefined for un-orderable bounds are this branch's addition, carried forward unchanged.
Verified: 165/165 modules/sqon tests pass against this combination, including the property-based reduceSqon tests.
| - **`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. |
There was a problem hiding this comment.
no change needed in this PR
the follow-up PR stacked on this one updates this wording once all itself enforces that check.
"Use all directly" needs the same caveat this bullet already gives the in merge
this bullet is from main, kept as part of resolving this PR's conflict, not something you wrote. it points to your all operator as the fix for the in merge's ambiguity; but all only reliably matches when the field can hold more than one value, a fact reported by the isArray field metadata this same merge brought in (true multi-valued, false single-valued, null undeclared).
on a field that's false or null, all with more than one value can't match either, the identical failure this bullet is warning about for in, just on the operator it now recommends.
| 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. |
There was a problem hiding this comment.
no change needed in this PR
the follow-up PR stacked on this one updates this wording once all itself enforces that check.
"Use all directly" needs the same caveat this paragraph already gives the in merge (same as the CHANGELOG.md entry)
this paragraph is from main, kept as part of resolving this PR's conflict, not something you wrote. it points to your all operator as the fix for the in merge's ambiguity; but all only reliably matches when the field can hold more than one value, a fact reported by the isArray field metadata this same merge brought in (true multi-valued, false single-valued, null undeclared).
on a field that's false or null, all with more than one value can't match either, the identical failure this paragraph is warning about for in, just on the operator it now recommends.
2 participants
Summary
Implements v2 of the
build_sqonMCP tool, bringing it to full operator parity with what is currently available inmodules/sqon. Updates unit tests, integration tests, and documentation to match, and addresses a few tech-debt items which surfaced while planning this work.Also fixes a bug in the SQON module's
reduceSqonfunctionality where date-range bounds would get merged tonull, caused by an uncheckedas numbercast.Issues
Description of Changes
MCP Server
Implemented v2 of the
build_sqontool: thewildcardtext operator with thefieldNames(plural) clause shape, plusallandsome-not-in, whichmodules/sqonalready supported but v1 withheld.checkFieldOperatortoqueryValidation.ts, the shared field-and-operator check now used by both clause validations inexistingSqon/execute_query*insidein-likevalues, which Arranger silently runs as a regular expression rather than an exact match, directing callers to usewildcardinsteadexistingSqonerrors were not batched with clause errors, forcing multiple re-submissions to fix one callbuild_sqonMCP Tool #1091 feedbackSQON Module
reduceSqonwhich corrupted merged date-range bounds tonull, caused byMath.max/Math.minbehind an uncheckedas numbercast. Bounds now compare numerically, parsed by timestamp, or lexicographically; unorderable bounds are kept as two clauses rather than merged.Integration Tests
fieldNames, negated wildcard, thealloperator, and a multi-field wildcard through the aggregations path*rejection and forfuzzybeing refused (since it is not implemented)existingSqonreported in one responseDocs
docs/mcp-server.md,apps/mcp-server/README.md, andCHANGELOG.mdfor the new operator coverage, thefieldNamesshape, and the asterisk*rejection's deliberate asymmetry withexecute_query's rawsqonpathapplicableTodivergence betweenmodules/sqonand catalogue introspection, and forintegration-tests/mcp-server's tsconfig never typechecking app sources cleanlySpecial Instructions
Before running these changes, you should rebuild the
sqonmodule to ensure you have the latest dist:# from root npm run sqon:buildReadiness Checklist
.env.schemafile and documented in the README