fix(driver-sql,driver-memory): an uncompilable filter throws instead of matching everything (#3948) - #4029
Merged
Merged
Conversation
…of matching everything (#3948) A filter neither driver could compile was **skipped**, not rejected: no predicate was emitted and the query returned every row. The caller asked to filter and silently received the unfiltered set. The reachable shape is a bare comparison triple. `['close_date','before',…]` arrives at a driver only when `isFilterAST()` refused it — the operator is outside `VALID_AST_OPERATORS`, so `parseFilterAST()` never converted it and the raw array became `where`. driver-sql's loop then saw three *strings*, matched neither `and` nor `or`, and `continue`d past all three. driver-memory was worse: it cast every string to a logic keyword, opened three empty groups and returned `{}` — a filter matching every record. Reachable from ordinary authoring, not just malformed input: `before`/`after` are canonical `VIEW_FILTER_OPERATORS` members that `VALID_AST_OPERATORS` does not accept. Eight of the nineteen canonical view operators are in that position, including `equals`; the rest were masked only because ObjectUI's adapter alias table happened to cover them. Both drivers now throw on an element that is neither a logical keyword nor a condition array. The nested and `$`-object paths already threw on the same input, so this makes the three paths agree rather than disagree. driver-memory also gains seven operators it silently ignored — `not_in`, `is_null`, `is_not_null`, `isnull`, `isnotnull`, `is_empty`, `is_not_empty`, all in `VALID_AST_OPERATORS`, all previously hitting `default: return null`. `is_null` narrowed nothing instead of matching null rows. Alias sets and semantics mirror driver-sql's `whereNull`/`whereNotNull` arms so both backends accept one vocabulary. Throwing without implementing these would have traded a silent bug for a hard regression on operators the spec blesses. Tests: driver-sql 487 pass, driver-memory 153 pass, objectql 1171 pass. Closes #3948 item 1 Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 13 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
os-zhuang
pushed a commit
that referenced
this pull request
Aug 1, 2026
…without the driver prefix (#4436) Completes the WIP commit: adds the remaining sql-driver throw sites and the regression tests for both backends. #4209/#4029/#3948 settled the POSTURE — a filter carrying an operator the driver cannot compile is refused instead of silently matching every row. What was missing is the refusal's IDENTITY on the wire. The driver threw a bare `Error`, so `mapDataError` fell through to its default branch and served a body whose only key was `error`: GET /api/v1/data/showcase_task?filter={"title":{"$bogusop":"x"}} → 400 {"error":"[sql-driver] Unsupported filter operator \"$bogusop\" …"} Two contract breaks in one body — no `error.code` at all on a route whose sibling rejections all speak the ADR-0112 catalogue, and the driver-internal `[sql-driver]` prefix on the wire, which is what the #3867 sanitiser exists to stop. Fixed at the throw site (PD #12), not by teaching the REST layer to guess: both drivers now refuse through an `unsupportedFilterError` helper that stamps `code = StandardErrorCode.enum.INVALID_FILTER` — the constant, so a catalogue rename breaks the compile — and `status = 400`. `INVALID_FILTER` is the same code `metadata-protocol` already emits when a filter fails to parse upstream (`malformedFilterArrayError` / `unusableFilterError`): one condition, one wire code, however the caller reached it. The `status` also puts the rejection on `isExpectedQueryRejection`, so a client mistake stops being logged as an unhandled server error. Applied to every filter-COMPILATION refusal in both backends, not only the one branch the issue names: unsupported operator ($-object, legacy triple), unrecognised logical keyword, unrecognised element type, and a `between` / `$between` operand that is not a two-element array. They are the same envelope defect on adjacent lines, and #3948 made the two drivers agree that an uncompilable filter is a refusal — so their refusal envelopes have to agree too, or the cross-driver parity this repo relies on is false where it matters. Tests: new `sql-driver-filter-refusal-envelope.test.ts` (8) and `memory-filter-refusal-envelope.test.ts` (5) pin `code`, `status`, the absence of the internal prefix, and that the actionable operator/field/vocabulary detail survives. Full suites green: driver-sql 623 passed, driver-memory 286 passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD
akarma-synetal
pushed a commit
to akarma-synetal/framework
that referenced
this pull request
Aug 2, 2026
…bjectstack-ai#4435, objectstack-ai#4436, objectstack-ai#4483) (objectstack-ai#4496) * fix(spec): the $search auto field set's lead ORDERS the set, it must not admit one (objectstack-ai#4483) `autoDefaultFields` filtered every field through three exclusions (`SEARCH_AUTO_EXCLUDED_FIELDS`, `hidden`, unsearchable type) and then prepended the display/name/title field on an EXISTENCE check alone — so the exclusions did not hold for whichever field happened to lead, and the module's own "system / audit / heavy fields never auto-included" invariant was false. Not a contrived shape: ADR-0079's `provisionPrimary(schema, { synthesize: false })` designates `nameField` at registration, and on a table whose only textual column IS the primary key (system tables, junction tables, append-only logs) it designates `id`. `$search` then expanded to `{ id: { $contains: term } }` — a substring scan over the primary key, returning a narrow and semantically wrong row set. It loosened a second layer too: `resolveSearchFieldResolution` is also the objectstack-ai#4254 REST ingress gate's arbiter for "would the engine actually scan this field", so with `id` in `allowed` a `$searchFields=id` override was ACCEPTED rather than refused. The lead's job is to put the primary title FIRST, never to admit it, so it is now chosen from the already-filtered set. An excluded / hidden / unsearchable display field simply does not lead and the set is unchanged; an eligible one still leads, so the ordering intent is intact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD * wip(drivers): give the uncompilable-filter refusal an ADR-0112 code and drop the driver prefix (objectstack-ai#4436) IN PROGRESS — code change complete, regression test not yet written and the real-boot curl repro not yet run. A filter carrying an operator the driver cannot compile is already REFUSED rather than silently matched (objectstack-ai#4209/objectstack-ai#4029), but the refusal had no wire identity: the thrown `Error` carried no `code`, so `mapDataError`'s default branch served `{"error": "[sql-driver] Unsupported filter operator …"}` — no `error.code` at all, breaking the ADR-0112 contract every sibling rejection on the same route already honours (`INVALID_FIELD`, `INVALID_FILTER`, `RECORD_NOT_FOUND`), and leaking the `[sql-driver]` internal prefix that the objectstack-ai#3867 sanitiser exists to keep off the wire. Both drivers now throw through a local `unsupportedFilterError` that stamps `code = StandardErrorCode.enum.INVALID_FILTER` (the same catalogued code `metadata-protocol` emits when a filter fails to parse upstream — one condition, one wire code however the caller reached it) and `status = 400`, which also puts the rejection on `isExpectedQueryRejection` so a client mistake stops being logged as an unhandled server error. The internal prefix is gone from the message; the actionable operator/field/vocabulary detail stays. Applied to every filter-COMPILATION refusal in both backends, not just the one branch the issue names — they are the same envelope defect on adjacent lines, and objectstack-ai#3948 made the two drivers agree that an uncompilable filter is a refusal, so their refusal envelopes have to agree too. TODO: regression tests (driver-sql, driver-memory, REST envelope) + boot repro. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD * fix(drivers): the uncompilable-filter refusal speaks INVALID_FILTER, without the driver prefix (objectstack-ai#4436) Completes the WIP commit: adds the remaining sql-driver throw sites and the regression tests for both backends. objectstack-ai#4209/objectstack-ai#4029/objectstack-ai#3948 settled the POSTURE — a filter carrying an operator the driver cannot compile is refused instead of silently matching every row. What was missing is the refusal's IDENTITY on the wire. The driver threw a bare `Error`, so `mapDataError` fell through to its default branch and served a body whose only key was `error`: GET /api/v1/data/showcase_task?filter={"title":{"$bogusop":"x"}} → 400 {"error":"[sql-driver] Unsupported filter operator \"$bogusop\" …"} Two contract breaks in one body — no `error.code` at all on a route whose sibling rejections all speak the ADR-0112 catalogue, and the driver-internal `[sql-driver]` prefix on the wire, which is what the objectstack-ai#3867 sanitiser exists to stop. Fixed at the throw site (PD objectstack-ai#12), not by teaching the REST layer to guess: both drivers now refuse through an `unsupportedFilterError` helper that stamps `code = StandardErrorCode.enum.INVALID_FILTER` — the constant, so a catalogue rename breaks the compile — and `status = 400`. `INVALID_FILTER` is the same code `metadata-protocol` already emits when a filter fails to parse upstream (`malformedFilterArrayError` / `unusableFilterError`): one condition, one wire code, however the caller reached it. The `status` also puts the rejection on `isExpectedQueryRejection`, so a client mistake stops being logged as an unhandled server error. Applied to every filter-COMPILATION refusal in both backends, not only the one branch the issue names: unsupported operator ($-object, legacy triple), unrecognised logical keyword, unrecognised element type, and a `between` / `$between` operand that is not a two-element array. They are the same envelope defect on adjacent lines, and objectstack-ai#3948 made the two drivers agree that an uncompilable filter is a refusal — so their refusal envelopes have to agree too, or the cross-driver parity this repo relies on is false where it matters. Tests: new `sql-driver-filter-refusal-envelope.test.ts` (8) and `memory-filter-refusal-envelope.test.ts` (5) pin `code`, `status`, the absence of the internal prefix, and that the actionable operator/field/vocabulary detail survives. Full suites green: driver-sql 623 passed, driver-memory 286 passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD * fix(metadata-protocol): PATCH/DELETE of a nonexistent record answer RECORD_NOT_FOUND, not 200 (objectstack-ai#4435) The READ path was already honest — `getData` on an unknown id answers `404 RECORD_NOT_FOUND`. Both single-record WRITE paths reported success for a record that does not exist: PATCH /data/showcase_task/definitely_not_a_row → 200 {"record":null} DELETE /data/showcase_task/definitely_not_a_row → 200 {"success":true} REST is a pass-through here (`res.json(await p.deleteData(...))`), so these are the protocol's answers and this is where they are fixed. What it cost: a client that PATCHed a concurrently deleted record was told the write landed, and had to null-check a SUCCESS payload to find out otherwise; `DELETE` said `success: true` for any string in the path, so a typo'd id, an already-deleted row and a real deletion were indistinguishable — including in bulk, where `deleteMany {"ids":["nonexistent_1"]}` answered `succeeded: 1`. It is the same silent-no-op shape the v17 train removed everywhere else this window (objectstack-ai#4240/objectstack-ai#4303/objectstack-ai#4315, objectstack-ai#4169, objectstack-ai#4190), one level up. - `updateData` asks existence BEFORE the write, via the same `findOne` + caller context `getData` uses. Deliberately not a post-check on the returned row: the engine returns the post-write READBACK, which is also `null` when the row still exists but the write moved it out of the caller's row scope (reassigning `owner_id` away from yourself under an owner-scoped policy) — reading that as "not found" would 404 a write that succeeded. - `deleteData` and `deleteManyData` read the driver's own answer. The contract (`IDataDriver.delete` — "True if deleted, false if not found") already carried it; the code discarded it and pushed a literal `success: true`. Read as `=== false` on purpose: that is the contract's positive not-found value, while a driver returning the deleted row or an off-contract `undefined` gives no such signal, and inventing a 404 from a falsy return would break deletes against third-party drivers instead of reporting honestly. `success` on the 200 now means what it says. - The 404 envelope is extracted as `recordNotFoundError` so the read and the two write paths cannot drift apart again. Note on the issue's second half: the spec's `DeleteDataResponseSchema` declares `success`, not `deleted`, so the existing key is correct as-is and nothing renames. Tests: new `protocol.record-not-found.test.ts` (12) covers PATCH/DELETE/ deleteMany, the read/write agreement on the same id, delete-twice, mixed batches, the `=== false` reading, and that the existence probe is asked with the caller's context. Three `protocol.dropped-fields.test.ts` fixtures stubbed `findOne → null` while PATCHing — under the new contract that IS a 404, so they now describe an engine that has the row (they are about the strip channel, not about missing records). Suites green: metadata-protocol 169, rest + objectql unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD * fix(runtime): a sandbox capability denial is a 500 crash, not a 400 rejection (objectstack-ai#4431) The `action-crash-vs-rejection` contract (objectstack-ai#3951) pins the table: a `SandboxError` WITH `innerMessage` is a body's deliberate throw → 400; a `SandboxError` with NO `innerMessage` — timeout, capability denial — is a crash → 500. Capability denials were answering 400: POST /api/v1/actions/showcase_task/rc1_crash_probe → 400 {"error":{"code":"VALIDATION_ERROR", "message":"SandboxError: capability 'api.read' not granted to action …"}} Why: the gate throws `SandboxError` synchronously INSIDE a QuickJS host function, which rejects the async IIFE inside the VM, so it returns through the `__error` side-channel — and the pump loop presumed everything arriving there was user code throwing on purpose, setting `innerMessage` unconditionally. The dispatcher's classifier then read that as a deliberate rejection. So every capability denial stayed invisible to gateway error rates, APM and alerting — exactly the blindness objectstack-ai#3951 was written to close — and the client also received the `SandboxError: ` debug prefix that belongs only in server logs. `SandboxError`'s own jsdoc already said `innerMessage` is undefined for the sandbox's internal errors; that only held for denials detected OUTSIDE evaluation (a timeout, which takes the separate `budgetError` path). In-VM host-call denials — `ctx.api.*`, `ctx.log`, `ctx.crypto`, `ctx.api.transaction` — were misclassified. Fix: the sandbox's own faults now carry a marker THROUGH the VM. `hostErrorToVm` stamps `__objectstackSandboxFault` on any `SandboxError` it marshals, and the synchronous gates throw the VM handle it builds rather than a raw host error — quickjs-emscripten passes a thrown handle through verbatim while its `newError` path copies only `name`/`message`, which is precisely how the identity was lost. The reject handler reports the marker on the additive `__errorInfo` channel, and the pump loop, seeing it, rethrows with neither the `<kind> '<name>' threw:` wrapper (nothing threw — the sandbox refused) nor an `innerMessage`. The existing classifier then does the rest: name is `SandboxError`, no inner/code/fields ⇒ unexpected fault ⇒ `errorFromThrown(err, 500)`, and the message reaching the client is the capability text with the debug prefix stripped. A marker rather than a match on the flattened `SandboxError: …` text, because the flattening is user-reachable: a body that CATCHES the denial and throws its own business error must keep its 400, and that case is pinned. No ADR or contract was changed — this makes the runtime deliver the contract objectstack-ai#3951 already specifies. Tests: new `sandbox/capability-denial-is-a-fault.test.ts` (7) covers all four in-VM gates, the absence of innerMessage/code/fields, the prefix, the caught-and-rethrown rejection, an ordinary deliberate throw, and that a record `ValidationError` crossing `ctx.api` keeps its `code`/`fields` (the marker must not turn every failed write into a 500). Verified failing on all four denial cases before the fix. Runtime suite green: 73 files / 1033 tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD * chore: add changeset for the v17 REST envelope defects (objectstack-ai#4431, objectstack-ai#4435, objectstack-ai#4436, objectstack-ai#4483) * fix(test): call syncSchema with its real (object, schema) signature (objectstack-ai#4436) The objectstack-ai#4436 refusal-envelope test passed a single merged object where the driver takes the object name as its own first argument, so the suite could not type-check. Matches the idiom in the sibling memory-driver tests. * fix(metadata-protocol): one probe per PATCH, and the existence gate is not an RLS gate (objectstack-ai#4435) Follow-up to 959b838, fixing two defects the first cut introduced. Both were caught by CI (`Test Core` on @objectstack/objectql, `Dogfood Regression Gate 1/2`), and the second is the more serious of the two. ## 1. The existence probe duplicated OCC's read `updateData` called `assertVersionMatch` (which reads the row for its `updated_at`) and then `assertRecordExists` (which reads the same row again). Two round-trips per PATCH — a performance regression no gate reports — and the `protocol-data.test.ts` OCC cases said so directly ("expected to be called once, but got 2 times"). The two gates want the same row, so they now share one read: `probeRecord` fetches it, `assertVersionOf` became a PURE comparison over an already-read row, and `assertVersionMatch` survives only for `deleteData`, which needs no existence probe at all — the driver's own return reports whether a row matched, so a plain DELETE stays at zero extra reads and only an OCC token buys one. ## 2. The probe must ask EXISTENCE, not the caller's visibility The first cut probed with the CALLER's context, reasoning that it should match `getData`. That quietly turned the existence gate into an authorization gate: a row the caller cannot read comes back `null`, so the PATCH answers 404. Two things break. It moves an RLS decision out of the write policy. Whether an unreadable row may be written by id is the objectstack-ai#1994 pre-image check's call, made inside `engine.update`. A probe in front of it adds a second, different rule — scope creep into the security model, out of a bug fix about missing records. And it disarms a revert-provable security proof. `@proof: rls-by-id-write` (`qa/dogfood/test/rls-fixture.dogfood.test.ts`, referenced by the `permission.rowLevelSecurity.using` liveness ledger entry) boots a fixture whose member can read nothing and has no write policy, and asserts the runner reports `rls-hole` — the RED half that proves the gate can go red at all. A caller-scoped probe 404s that PATCH and the proof goes green: if objectstack-ai#1994 were ever reverted, this probe would MASK it. Accidentally hardening one path is not worth permanently blinding the gate that watches the whole class. So the probe runs as system and answers existence only. Authorization stays exactly where it was, and the sole behaviour added is the 404 the issue asked for: an id that names no row at all. Tests: `protocol-data.test.ts`'s OCC block now asserts the new contract — one probe on every PATCH (the existence probe, no OCC comparison without a token), still exactly one when OCC IS requested (the anti-duplication pin), 404 before any OCC verdict for a missing id, and DELETE without a token issuing no probe. Its fixtures now supply a row, because under this contract a PATCH of an absent record is correctly a 404 and those cases are about OCC. Two cases added to `protocol.record-not-found.test.ts` pin the system-context probe and that an unreadable-but-existing row still reaches the engine for RLS to decide. Green: objectql protocol-data 117, metadata-protocol 170, dogfood shard 1/2 38 files / 235 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3948 item 1 — the prerequisite the rest of that issue is blocked on.
A filter neither driver could compile was skipped, not rejected. No predicate was emitted and the query returned every row: the caller asked to filter and silently received the unfiltered set.
The reachable shape
A bare comparison triple.
['close_date','before','2024-01-01']arrives at a driver only whenisFilterAST()refused it — the operator is outsideVALID_AST_OPERATORS, soparseFilterAST()never converted it andprotocol.tsassigned the raw array towhere(it is a conversion gate, not a validation gate).applyFilterssaw three strings, matched neitherandnoror, andcontinued past all three. No WHERE clause.item.toLowerCase() as 'and' | 'or'), opening three empty logic groups, producing no conditions, and returning{}— a filter matching every record.This is reachable from ordinary authoring, not just malformed input.
beforeandafterare canonicalVIEW_FILTER_OPERATORSmembers (ui/view.zod.ts:90) thatVALID_AST_OPERATORSdoes not accept. Eight of the nineteen canonical view operators are in that position — includingequals. The other six were masked only because ObjectUI's adapter alias table happened to cover them; the two it missed are how this surfaced (objectstack-ai/objectui#2974).What changed
Both drivers now throw on a filter element that is neither a logical keyword nor a condition array. The nested-AST and
$-object paths already threw on the same class of input, so this makes the three paths agree instead of disagree.driver-memory also gains seven operators it was silently ignoring
not_in,is_null,is_not_null,isnull,isnotnull,is_empty,is_not_empty— all members ofVALID_AST_OPERATORS, all previously falling todefault: return nulland being dropped by the caller.is_nullnarrowed nothing instead of matching null rows.This was necessary, not scope creep: throwing without implementing them would have traded a silent wrong-result bug for a hard regression on operators the spec blesses. Alias sets and semantics mirror driver-sql's
whereNull/whereNotNullarms so both backends accept one vocabulary.Verification
driver-sqldriver-memoryobjectql(query engine)Two new tests. The memory one iterates
VALID_AST_OPERATORSdirectly, so the next operator the spec adds fails here rather than being silently dropped; both pin that a bare triple throws instead of returning every row.One pre-existing issue found and deliberately not fixed
applyFilters' legacy array form is infix ([condA, 'or', condB]). The prefix spec-AST form['or', condA, condB]compiles toANDon this path — but through the protocol it never gets here, becauseisFilterAST()accepts it andparseFilterAST()converts it to{$or: […]}, which takes the object branch. Only a directdriver.find({where: ['or', …]})call hits the broken path. Verified unchanged by this PR (nextJoinhandling is untouched); noted in the test so nobody "fixes" the test by changing the semantics.Changeset included —
driver-memoryminor for the new operators,driver-sqlpatch, with the FROM → TO mapping for anyone whose query starts throwing.