Skip to content

fix(filter): refuse a where on a virtual formula field at both doors, instead of answering 200 with zero rows (#8296) - #8369

Merged
os-zhuang merged 5 commits into
mainfrom
claude/issue-8296-filter-unmaterializable-verdict
Aug 13, 2026
Merged

fix(filter): refuse a where on a virtual formula field at both doors, instead of answering 200 with zero rows (#8296)#8369
os-zhuang merged 5 commits into
mainfrom
claude/issue-8296-filter-unmaterializable-verdict

Conversation

@os-zhuang

@os-zhuangos-zhuang commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Fixes#8296

formula is the one field type no driver materialises a column for. Three query
axes can name a field; until this PR only two of them said so.

axisverdict for a formula field
SORT400 INVALID_SORT — ingress (#6994) and engine (#7095)
SEARCH400 INVALID_FIELD — refused by name (#6674)
FILTERaccepted — 200, 0 rows, no error

Dispatched under the standing maintainer ruling of 2026-08-12 (covering #7529 /
#7893 / #8010 / #7912): a declaration the platform cannot honour is refused at
the latest checkpoint that can see the whole picture, naming the offending key
path, and never answered 200.

Premise re-measured on current main

The card's line numbers were stale, so this was re-measured against cb43296ef
by function name, on a real ObjectQL with the real protocol on top — is_open
a formula over the stored status column, subtask_total a summary:

INGRESS where { is_open: true } -> 0 rows, NO ERROR
INGRESS where { is_open: false } -> 0 rows, NO ERROR
ENGINE find { is_open: true } -> [] , NO ERROR
ENGINE findOne { is_open: true } -> null
ENGINE count { is_open: true } -> 0
CONTROL where { status: 'open' } -> 4 rows
CONTROL where { subtask_total: 5 } -> 1 row (`summary` HAS a column)
CONTROL where { no_such_field: 1 } -> 400 INVALID_FIELD
READ the formula still hydrates -> 5 rows, value present

Both directions are wrong and the false one is the dangerous one: the same
predicate against a stored boolean returns every row, so a filter meaning
"not yet done" silently became "no records at all" — a changed row SET under a
200, which no amount of inspecting the response can reveal. The formula READS
correctly in that very same response, so the field is visibly populated and
simultaneously unfilterable.

Both doors, because the engine door is author-reachable

The card left "ingress-only, or the engine door too?" open. Measured: the
engine door is required.
plugin-reports' executeReport forwards a saved
report's filter verbatim —

this.engine.find(report.object_name, { where: q.filter, fields: q.fields, orderBy: q.orderBy, limit })

— which passes through no REST ingress at all, exactly as #7095 measured for
query.orderBy one axis over. An ingress-only fix would have left that half
open.

  • IngressassertFilterFieldsExist (packages/metadata-protocol) grows a
    second verdict after unknown. Covers everything reaching findData: the
    list route, POST /data/:object/query, the export route and the RPC
    dispatcher, in every filter spelling (where / filter / filters /
    $filter, the array sugar, and nested $and / $or), naming the caller's own
    wire spelling in param.
  • EngineassertFilterIsMaterializable
    (packages/objectql/src/filter-comparand-shape.ts) runs inside
    lowerWhereFilterArray, the ONE seam every caller-supplied where passes
    through, so find, findOne, count, aggregate, update and delete all
    answer alike and a new verb cannot miss the gate by omission. It judges the
    CALLER's where only — a middleware-injected RLS / sharing / tenant predicate
    is the platform's own and is never refused.

Both answer 400 INVALID_FIELD with field / fields / object (and param
at ingress), and both prescribe the remedy the sort and search axes already
share, with only the verb changed to name this axis:

Denormalise the value onto 'showcase_task' (a stored field, written when the
source changes) and filter that.

One vocabulary, and the two types that must NOT be caught

Both doors judge the field with the same @objectstack/spec/data predicate the
search axis uses (isVirtualSearchField / SEARCH_VIRTUAL_TYPES) rather than a
locally minted type list, so a gate and the drivers cannot disagree about which
types have a column. summary and autonumber still filter — both get real
stored columns; the set is exactly formula, and both are pinned as controls on
both doors. Reading, projecting and computing a formula field are untouched.

INVALID_FIELD rather than INVALID_FILTER, and no new code minted: this
verdict is about the NAME's type, which is what the ingress door already answers
with INVALID_FIELD on its neighbouring unknown verdict and what the SEARCH
axis answers for this very field class. INVALID_FILTER is objectql's
VALUE-shape envelope (#5869 / #7047) — a different fact. (The triage comment
suggested the INVALID_FILTER family; its operative constraint — do not mint a
new top-level code without checking the ADR-0114 catalog — is honoured. Happy to
flip the constant if the reviewer prefers.)

Blast radius — measured on source AND tests, with one exception

The card's second open question was the migration risk: a filter refusal turns
today's silent-zero surfaces into loud 4xx.

App metadata: clean, and that half of the sweep held. Every formula field
declared in shipped app metadata was enumerated — crm_contact.full_name,
crm_opportunity.expected_revenue / days_to_close, crm_lead.is_closed,
showcase_project.budget_remaining, showcase_field_zoo.f_formula — and each
occurrence checked: they appear only as view COLUMNS, form fields, an FLS
permission entry, translations and a record-level CEL predicate. No example
app, seed, view filter, saved report, flow or dashboard in this repo filters on
a formula field.
(The one where hit, case.is_closed in an analytics unit
test, is a mocked executeAggregate with no registry and no formula field, and
its key is dotted — a shape neither door judges.)

One TEST does filter one, and it is updated in this PR.
examples/app-todo/test/derived-flag-removal.test.ts registers a formula-shaped
object of its own (derived_task, invented by that file — not app metadata, not
in defineStack) to record why #7226 removed two inert flags rather than
deriving them. It pinned exactly the behaviour this PR abolishes: filtering a
formula answering 0 rows with no error. Updated here —

The read/projection half of that file (a formula still COMPUTES both flags
correctly) and its stored-column CONTROL assertions are untouched and still
pass. Nothing under examples/app-todo/src/** or its objectstack.config.ts
was touched — that app declares no formula field at all.

#7226's decision stands, and its reasoning is stronger. A formula field
still materialises no column and still cannot carry a predicate, so the eight
app filters that named those flags still could not have worked; removal in
favour of stored columns is still the only repair. Only the failure mode
changed, from an invisible zero to a named 400 — and that test's own docblock
had named the missing exception as the safe design ("an exception would have
been safe, because someone would have seen it"). This PR supplies it.

The reusable lesson. The first sweep enumerated formula fields declared
outside tests. That reads as "empty in-tree" but excluded exactly the
population that broke. A sweep for a BEHAVIOUR change has to cover test files:
tests are where the old answer is pinned, so a behaviour change lands on the pin
before it lands anywhere else.

Verification

Reverse verification, direction predicted before running: reverting the three
source files to origin/main and rebuilding turned the new pins red and left
every control green — 21 failed / 150 passed, and the failures are exactly the
refusal pins (11 ingress spellings, both message pins, all 6 engine verbs, the
engine message pin, the cross-door agreement pin). The pre-existing unknown
verdict, the stored/summary/autonumber controls, the blast-radius pins and
the registry-less pin stayed green throughout.

Suites, after merging main and rebuilding: objectql 196 files / 3522 tests,
metadata-protocol 79 / 1163, rest 110 / 1817, runtime 150 / 2306 — all
passing. pnpm lint clean, objectql typecheck clean, all 53 check:* gates
from the ESLint job green, plus scripts/check-engine-split-ratio.mjs (surfaced
by re-deriving gates from the actual changed paths, not named in the dispatch
list), and packages/speccheck:generated reports all 13 artifacts up to date
after the merge.

After the test/docblock/changeset correction above, the whole workspace test
suite was re-run locally (all 76 packages, partitioned into six balanced shards,
dogfood excluded as in CI), with examples/app-todo at 4 files / 106 tests
passing.


Generated by Claude Code

…8296)
The FILTER axis was the last of the three query axes with no
unmaterializable verdict: a `where` on a `formula` field cleared
`assertFilterFieldsExist` because the field IS known, reached a driver
that materialises no column for it, and answered 200 with zero rows in
BOTH directions — while SORT (#6994/#7095) and SEARCH (#6674) refuse the
same field by name.
Ingress: `assertFilterFieldsExist` grows a second verdict, judged by the
same `@objectstack/spec/data` predicate the search axis uses
(`isVirtualSearchField`), so gate and drivers cannot disagree about which
types have a column. `summary`/`autonumber` keep filtering — both have
real stored columns.
Engine: `assertFilterIsMaterializable` closes the door the REST ingress
cannot reach — a saved report forwards `query.filter` straight into
`engine.find` — at `lowerWhereFilterArray`, the one seam every
caller-supplied `where` passes through (find/findOne/count/aggregate/
update/delete).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
@vercel

vercelBot commented Aug 13, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 13, 2026 10:40am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/metadata-protocol, @objectstack/objectql.

16 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via @objectstack/metadata-protocol, @objectstack/objectql)
  • content/docs/data-modeling/formulas.mdx(via packages/objectql)
  • content/docs/deployment/migration-from-objectql.mdx(via @objectstack/objectql)
  • content/docs/deployment/vercel.mdx(via @objectstack/objectql)
  • content/docs/kernel/contracts/data-engine.mdx(via @objectstack/objectql)
  • content/docs/kernel/runtime-services/examples.mdx(via packages/objectql)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/metadata-protocol, @objectstack/objectql)
  • content/docs/kernel/services.mdx(via @objectstack/objectql)
  • content/docs/permissions/authentication.mdx(via @objectstack/objectql)
  • content/docs/permissions/system-context.mdx(via packages/objectql)
  • content/docs/plugins/index.mdx(via @objectstack/objectql)
  • content/docs/plugins/packages.mdx(via @objectstack/objectql)
  • content/docs/protocol/kernel/http-protocol.mdx(via @objectstack/metadata-protocol)
  • content/docs/protocol/kernel/index.mdx(via @objectstack/objectql)
  • content/docs/protocol/objectql/query-syntax.mdx(via packages/objectql)
  • content/docs/protocol/objectql/state-machine.mdx(via @objectstack/objectql)

2 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/objectql)
  • content/docs/releases/v9.mdx(via @objectstack/metadata-protocol)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

PM ruling on the red Test Core (2/3) — this is a patch round, NOT the blast-radius STOP

I read the assertion, not the turbo tail. Failure surface is exactly one test, and every other check on 5e9aa78 is green (ESLint, TypeScript, Build Core, Test Core 1/3 and 3/3, all three Dogfood shards, Temporal Conformance, Check Changeset).

FAIL examples/app-todo/test/derived-flag-removal.test.ts
> REVERSE — why the derive route was rejected, measured
> ...and is UNFILTERABLE: 0 rows, no error — which is why deriving was refused
Serialized Error: { status: 400, code: 'INVALID_FIELD', field: 'is_completed',
fields: [ 'is_completed' ], object: 'derived_task' }
❯ assertFilterIsMaterializable packages/objectql/src/filter-comparand-shape.ts:390:15
❯ lowerWhereFilterArray packages/objectql/src/engine.ts:640:5
❯ _ObjectQL.find packages/objectql/src/engine.ts:7316:13
❯ test/derived-flag-removal.test.ts:286:21

Why the STOP condition does not fire

The dispatch bar was: if the blast radius reaches shipped app fixtures or examples, stop — that is a migration question for the maintainer; do not "fix" the fixtures to make your gate pass. The purpose of that rule is to stop a dev quietly mutating an app's declared metadata — an object field, a view filter, seed data — so the platform's new refusal does not fire, hiding a real migration cost by editing the victim.

Measured against that purpose, nothing of the kind is happening here:

  • derived_task is a test-local object literal (const DERIVED = {...}) invented by that test to demonstrate the defect. It is not in TodoApp / defineStack, not an object, view, dashboard, report, flow or seed row.
  • The app's own metadata declares no formula field at allc11b69905 (fix(example-todo): remove the inert is_completed/is_overdue flags and repair every filter that read them #8295) removed both.
  • The sibling test NOTHING in the whole app stack references either field — keys or valuespassed, on a recursive walk of the real stack. That is the assertion that would have caught genuine app blast radius, and it is green.

So the only red assertion is one that deliberately constructs a formula-filtering object in order to document that filtering formulas is broken. It goes red because the defect it documents was fixed. Updating it is not fixing a fixture to pass a gate; it is updating a test whose subject matter is the defect.

I am stating this explicitly because it sits on the boundary of a rule I wrote, and I would rather be overruled cheaply than reinterpret my own STOP quietly: @maintainer, if you read "reaches shipped examples" literally rather than by purpose, say so and I will hold #8369 and re-file this as a migration card instead.

This failure is the strongest positive control on the PR

derived-flag-removal.test.ts was written for #7226 by a different card, in a different package, by someone not working on this one. It asserts the pre-#8296 behaviour directly — and its docblock names the missing exception as the safe design that did not exist:

"That asymmetry is the whole argument: an exception would have been safe, because someone would have seen it."

#8296 supplies precisely that exception. An independently-authored test in examples/ going red, through ObjectQL.find on an ordinary app call path with no protocol in the picture, proves the engine door is reachable from real application code — a reverse verification this PR did not have to construct. Please cite it in the PR body.

What must change — and what must not

Must change (three things):

  1. The two it(...) bodies in REVERSE — why the derive route was rejected, measured that filter on is_completed / is_overdue and expect []. Same measurement, new verdict: await expect(...).rejects.toMatchObject({ status: 400, code: 'INVALID_FIELD', field: 'is_completed', object: 'derived_task' }).
  2. The prose that is now factually inverted — the file docblock's "Why removed rather than derived as formulas" section, and the REVERSE describe block's docblock. Both currently say the failure is silent ("0 rows with no error", "returns cleanly rather than throwing", "a wrong answer traded for an invisible one"). examples/app-todo: is_completed and is_overdue are readonly flags that nothing ever maintains — permanently false, and one of them is read by a hook #7226's decision still stands and its reasoning gets stronger, not weaker — a formula still cannot be filtered, so those eight filters still could not have worked; the failure is now a named 400 instead of an invisible zero. Write it that way, cross-referencing The FILTER axis has no unmaterializable verdict: a where on a virtual formula field returns 0 rows silently, while sort and search refuse the same field with a 400 #8296.
  3. The changeset contains a false claim and must be corrected regardless of everything above: "Every shipped example app in this repo was swept: none filters on one, so nothing in-tree needed changing." CI just falsified it. The sweep covered app source and missed test files — which is where current behaviour gets pinned, and therefore where a behaviour change lands first. Restate it accurately and name this test.

Must not change: the first it in that block ("a formula field COMPUTES both flags correctly") — it passed, it only reads, and it is the half proving reads are untouched. Nor the stored-column CONTROLs. Nor anything in examples/app-todo/src/** or objectstack.config.ts. If you find yourself editing app metadata, stop and report — that is the STOP condition, and it has not fired yet.


Generated by Claude Code

…velope (#8296)
`derived-flag-removal.test.ts` registers a test-local formula-shaped object
(`derived_task`, invented by that file) to record why #7226 removed two inert
flags rather than deriving them, and it pinned the exact behaviour #8296
abolishes: filtering a formula answering 0 rows with no error. Its three
filtering assertions now assert the rejection envelope (status 400,
INVALID_FIELD, field, object) instead of an empty array, and the `it` title no
longer claims "0 rows, no error".
#7226's decision is unchanged and its reasoning is stronger: a formula field
still materialises no column and still cannot carry a predicate, so the eight
app filters that named those flags still could not have worked. Only the
failure mode changed, from an invisible zero to a named 400 -- which is the
exception this very docblock had named as the safe design. Both docblocks are
rewritten to state that.
The read/projection half (a formula COMPUTES both flags correctly) and the
stored-column CONTROL assertions are untouched; nothing under
examples/app-todo/src/ or objectstack.config.ts is touched, and that app
declares no formula field at all.
The changeset's blast-radius sentence is corrected in the same commit: the
original sweep covered app source and missed test files, which is where current
behaviour is pinned and therefore where a behaviour change lands first. No app
metadata filters a formula field -- that half held.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

ACCEPT — PM review, domain:metadata seat

Green at b386caff: all 26 checks, each job's own conclusion verified individually rather than by a roll-up. The substance of this review is already on record in the three-question rulings and the red-CI diagnosis, so this receipt is short.

Verified before accepting:

  • ⚠️The STOP condition was not tripped. Nothing under examples/app-todo/src/** or its objectstack.config.ts was touched; that app declares no formula field at all. The only red assertion was in a test that constructs its own formula-shaped object to document the defect being fixed, and updating it is not fixing a fixture to pass a gate.
  • All four surfaces are now consistent: the test's rejection assertions, both docblocks in derived-flag-removal.test.ts, the changeset, and the PR body's blast-radius section.
  • Path fork check: 6 changed files, all accounted for, none under docs/adr/**, .claude/skills/** or skills/** — so an AI seat may mark this ready and enqueue it.
  • Reverse verification has its direction predicted before running, and the 21 red / 150 green split names exactly which pins move and which controls hold.

Two things this PR did that are worth other seats copying.

First, the sweep correction. The original claim — "no example app, seed, view filter, saved report, flow or dashboard filters on a formula field" — was true and still misleading, because the enumeration covered formula fields declared outside tests and that excluded exactly the population that broke. The rewritten section states the app-metadata half held, names the one test that didn't, and draws the general lesson: a sweep for a behaviour change has to cover test files, because tests are where the old answer is pinned, so a behaviour change lands on the pin before it lands anywhere else. That is now a lane review bar.

Second, the red test was the strongest evidence on the PR rather than an obstacle to it. derived-flag-removal.test.ts was written for #7226 by a different card in a different package, and its docblock had named the missing exception as the safe design — "an exception would have been safe, because someone would have seen it." This PR supplies it. An independently-authored test in examples/ going red through ObjectQL.find, with no protocol in the call path, proves the engine door is reachable from ordinary application code — a reverse verification nobody had to construct.

#7226's decision stands and its reasoning is stronger: a formula still materialises no column and still cannot carry a predicate, so those eight filters still could not have worked. Only the failure mode changed, from an invisible zero to a named 400.

Marking ready and enqueueing. Fixes #8296 closes the card on merge.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 13, 2026 10:58
@os-zhuang
os-zhuang added this pull request to the merge queueAug 13, 2026
Merged via the queue into main with commit edff010Aug 13, 2026
27 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-8296-filter-unmaterializable-verdict branch August 13, 2026 11:14
hotlong pushed a commit that referenced this pull request Aug 13, 2026
… relay)
Merge commit first, regeneration as its own commit per the sanctioned
sequence; api-surface and export-origins regenerated after a fresh spec
build, spec-changes/upgrade-guide/docs from the merged registry. Both
sides verified present: the #8057 retirement (two [RETIRED] marks, the
prescription const, tombstones + engine refusal) and main's #8296/#8369
virtual-formula where-refusal doors.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Euoy6wyfzgiWtgCg4s6JK2
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-zhuang@claude