Repository navigation
convertFiltersToAST has no branch for $and / $or / $not, so spec-legal FilterCondition combinators never reach the wire #6948
Description
Split out of #4664 (PR #6946). Pre-existing on every consumer of the shared
filter sink; that card newly makes it reachable by an author following the
spec's own type, which is why it is being written down now.
What happens
convertFiltersToAST (packages/core/src/utils/filter-converter.ts) — the
repo's ONE lowering from the MongoDB-style filter object to the ObjectQL AST,
reached through toFilterNode / mergeFilterNodes — has no branch for the
logical combinators. Measured against packages/core/dist at
40c479af2, composing each condition with a parent scope:
$or -> ["and",["task_version","=","tv-1"],["$or","=",[{"status":"open"},{"status":"blocked"}]]]
$and -> ["and",["task_version","=","tv-1"],["$and","=",[{"status":"open"},{"is_active":true}]]]
$not -> THROWS FilterOperatorError: Unknown filter operator 'status' for field '$not'.
plain -> ["and",["task_version","=","tv-1"],["status","!=","archived"]] (correct)
Two distinct failures, neither of them the right answer:
$and/$orfall through to the simple-equality branch (their value is
an array, so the operator loop is skipped) and produce a leaf naming a field
literally called$and/$or. That is a well-formed AST node carrying a
nonsense field, so the server refuses it (400 INVALID_FILTER) — loud, but
the diagnostic points at the author's field list rather than at the missing
lowering.$notenters the operator loop with its OWN nested object's keys read as
operators, so the thrown message names a nonsense operator
(Unknown filter operator 'status' for field '$not') and lists the supported
operators — none of which is what the author needs to hear.
Why it matters now
Field.relatedListFilter (@objectstack/spec 17.1.0, objectstack#8704 /
PR #8955) is typed FilterConditionSchema, and FilterCondition explicitly
declares all three:
$and?: FilterCondition[];
$or?: FilterCondition[];
$not?: FilterCondition; // NULL-safe per #5146So the spec tells a metadata author these are legal in the very key #4664 wired
up, while this repo's lowering cannot carry any of them to the wire. The same
gap applies to every other producer that reaches this sink — the component-level
record:related_list.filter (objectstack#7118), ListView.filter /
ViewTab.filter via buildEffectiveFilter, and plugin-view's ObjectView —
so this is a sink-level bug, not a related-list one.
Also worth deciding as part of the fix: $not is NULL-safe by ruling
(objectstack#5146, maintainer 2026-08-04 — NOT (…) OR col IS NULL, with the
guard on each LEAF inside the negation, not hoisted). Whatever lowering is
chosen has to preserve that or hand the negation to the server intact.
Why it was not fixed in #4664
Changing the shared sink changes what every one of those callers puts on the
wire, so it needs its own card, its own fixture set and its own reverse
verification. Working around it renderer-side — accepting the combinators in
one consumer and lowering them there — is precisely the second de-facto
contract AGENTS.md #0.1 forbids, so #4664 carried the value verbatim to the
existing sink and named this boundary in its PR body instead.
Where
packages/core/src/utils/filter-converter.ts—convertFiltersToAST
(the missing combinator branch),toFilterNode,mergeFilterNodes
Filed unassigned by the #4664 execution seat. No dedupe hit: a targeted issue
search for the combinator lowering returned only objectui#4744 (a different
function, ListView.convertFilterGroupToAST, closed), with a control query in
the same session returning #4664 itself.
Generated by Claude Code
Activity
os-zhuang commentedon Sep 4, 2026
分诊路由(本评论来自 objectstack 中央分诊席,objectstack#6015)· R+153 ·
date -u实测 2026-09-04T22:32:02Z 一轮domain:ui·bug·pm:queue·priority:p2。落点packages/core/src/utils/filter-converter.ts⇒domain:ui。bug判据:@objectstack/spec的FilterCondition明确声明三个组合子($and/$or/$not,其中$not按 objectstack#5146 是 NULL-safe),而本仓唯一的 lowering 一个分支都没有 ⇒ 声明 ≠ 交付,且是在 #4664 刚刚接上的那个键上。⇒ 不是「缺特性」,是「平台告诉作者合法、然后自己送不上线」。p2 判据:⛔ 不是 p1 —— 两种失效都是响亮的(
$and/$or⇒ 服务端400 INVALID_FILTER;$not⇒ 抛FilterOperatorError),没有静默错数据;而且没有测量今天是否真有作者写了组合子。⛔ 不是 p3 —— 它是接收器级缺陷,波及每一个到达该 sink 的生产者(Field.relatedListFilter、record:related_list.filter、ListView.filter/ViewTab.filter经buildEffectiveFilter、plugin-view的 ObjectView),而 #4664 刚把它变成作者按 spec 的类型就能走到的路径。⭐ 提级条件:测到任何作者态元数据里写了组合子,重估 p1。⭐ 两种失效的诊断信息都指错了地方,这是本卡最该被修的地方,不只是让它工作:
$and/$or的值是数组 ⇒ 跳过操作符循环 ⇒ 落到简单相等分支,产出一个字段名字面为$and/$or的叶子 ⇒ 服务端拒绝时指着作者的字段清单,而真正的原因是缺 lowering;$not进操作符循环、把自己嵌套对象的键当操作符读 ⇒ 抛出的消息说Unknown filter operator 'status' for field '$not'并列出支持的操作符 —— 没有一个是作者需要听的。$not的 NULL-safe 语义是已裁的(objectstack#5146,维护者 2026-08-04:NOT (…) OR col IS NULL,守卫加在否定内部的每个叶子上,不上提)。⇒ 要么 lowering 自己保住它,要么把否定原样交给服务端。⛔ 不得在本仓重新发明一套否定语义。⭐ 卡面对「为什么不在 #4664 里顺手修」的论证本席采纳并复核:在一个消费者里接受组合子并就地 lowering,正是 AGENTS.md #0.1 禁止的第二套事实契约;改共享 sink 会改变每一个调用者送上线的东西 ⇒ 需要自己的卡、自己的夹具集、自己的反向验证。⇒ 这也是本席不合卡的理由。
Generated by Claude Code
os-justin commentedon Sep 7, 2026
Claim: session
session_01YBWFb5YgMU5dw8p2VKj16S· branchclaude/issue-6948-filter-ast-combinatorsPM dispatch (
domain:uiseat). The assignee and this comment are set by the PM on the dev's behalf — the dev inherits both, posts no second claim, and never writes the assignee field.packages/core's filter/adapter surface moved today (PR #8437, objectui#7379, is in the merge queue touchingValueDataSource.ts). Different file, but cut from currentmainand re-read.Two things a sibling card measured today that bear directly on this one:
ValueDataSource's$-dialect matcher waves through every operator it does not recognise —default: breakadds no constraint, so$nin/$startsWith/$null/$existsall return every row (measured). IfconvertFiltersToASTis upstream of a matcher with that shape, a combinator that "never reaches the wire" and one that reaches it and is ignored are indistinguishable from a green test. Establish which failure mode you are actually fixing.ListView, which has its own ladder and a live producer. If your report makes a zero-producer claim, say which consumers the zero covers.The pin must assert what reaches the wire (the constructed AST, or the driver's received filter), not that a branch exists. And it needs a control that fires: an assertion that a combinator is present passes on a builder that emits everything.
⛔ If
$and/$or/$notturn out to be deliberately unsupported at this layer rather than missing, that is a contract question — report and stop.Generated by Claude Code
os-justin commentedon Sep 8, 2026
os-dev-report
{
"issue": 6948,
"status": "needs_decision",
"branch": "claude/issue-6948-filter-ast-combinators",
"pr": "#8456",
"premise_still_valid": true,
"summary": "The defect is real and the sink is the right place, but the card's account of HOW it fails is falsified, and that changes what was fixed. WHICH OF THE CARD'S NUMBERS STILL HOLD: re-derived on the branch base ca39427 against @objectstack/spec 17.3.0 (card measured packages/core/dist at 40c479a, spec 17.1.0), all four emissions reproduce byte for byte, including the $not throw naming 'Unknown filter operator status for field $not'. The file, the three named functions and both mechanisms hold; filter-converter.ts has not moved on main since 2a9513d. FALSIFIED: the card's claim that the $and/$or leaf is refused by the server with 400 INVALID_FILTER. Measured, isFilterAST(['$or','=',[...]]) is true and parseFilterAST returns {$or:[...]} — the exact FilterCondition the author wrote, because a [field,'=',value] node lowers to {[field]: value} and $or is a legal FilterCondition key. This repo already knew: data-objectstack/src/filter-entry-translation.test.ts pinned that round trip with the word 'verified'. WHICH FAILURE MODE WAS FIXED: neither of the brief's two. It is a third — reaches the wire and is interpreted correctly there, and is mis-evaluated silently to ZERO ROWS by every AST evaluator inside this repo, because the leaf is a well-formed COMPARISON node and ValueDataSource's matchesComparisonNode reads $or as a field name. Evidence: selectedIds(['$or','=',[...]]) is [] pre-fix and ['open-active','blocked-idle'] post-fix on the same fixture. THE FIX: $and/$or lower to ['and'|'or', ...children] (the spelling FILTER_ARRAY_LOGIC_KEYWORDS declares), children lowered recursively; the wire FilterCondition is UNCHANGED and pinned both ways, so what moves is executability plus the fact that operators inside a combinator branch now meet the same guard every other operator meets. Boolean identities (objectstack#5322) decided at the branch boundary: empty $and drops out (a childless ['and'] measures to isFilterAST false / parseFilterAST undefined = EVERY ROW, the one direction that must not happen); empty $or keeps its pre-existing leaf (measured to answer zero rows at both the wire and the matcher, and FALSE has no AST spelling); a {} disjunct absorbs its $or. $not IS FENCED AND ESCALATED, see open_questions. REACHABILITY, stronger than the card stated: $and has live in-repo producers today, not only Field.relatedListFilter — core/src/utils/merge-filters.ts mergeFilters feeds DashboardRenderer.tsx:851,858 and DatasetReportRenderer.tsx:1429,1447; ObjectMetricWidget.tsx:413 and DrillDownDrawer.tsx:106 build {$and:[...]} directly; FilterConditionField.tsx:209,213 emits both. The zero-evaluator claim covers exactly one evaluator: ValueDataSource's matchesASTFilter is the only in-repo reader of node[0] as a field; every other consumer forwards the node to a DataSource. ASSIGNEE: inherited from the PM dispatch, never written by me; the newest Claim: comment names this session and this branch.",
"tests": "All runs through the shared container lock, verdict lines quoted, at df25f4b. (1) vitest packages/core/ packages/data-objectstack/ packages/fields/ packages/plugin-dashboard/ packages/plugin-report/ -> 'VERDICT command-exit 0', 429 files, 6775 tests passed. (2) vitest the two pins + packages/plugin-list/ plugin-grid/ plugin-detail/ plugin-form/ plugin-view/ -> 'VERDICT command-exit 0', 442 files, 4360 passed 1 skipped. (3) vitest packages/components/ packages/react/ -> 'VERDICT command-exit 0', 313 files, 3100 tests passed. 1184 test files total, covering every direct consumer of the sink and every in-repo producer of $and/$or enumerated above. DECLARED NARROWING: app-shell, console, site, the examples and the remaining plugins go to CI; their $and references (ObjectFieldInspector, datasetFilterCondition) are metadata-authoring conversions that never reach convertFiltersToAST. type-check: 'pnpm --filter @object-ui/core --filter @object-ui/data-objectstack type-check' exit 0, and BOTH edited test files proven in-program with tsc --listFiles (new pin 1 hit, lit control filter-converter.ts 1 hit, dark control 0 hits; data-objectstack pin 1 hit, lit control src/index.ts 1 hit). This caught a real error vitest transpiles away: the first build failed TS2345 on 'children.push(lowered)' narrowing to any[] | FilterNode. lint: same filter, exit 0, 0 errors (433 pre-existing warnings). check-control-bytes.mjs OK on 6668 tracked files, plus a direct grep -naP over the four changed paths, exit 1 = no control bytes. check-changeset-presence.mjs verdict: 'OK 2 source file(s) of 2 released package(s) changed, and this change declares 1 changeset(s)'. check-changeset-no-major.mjs exit 0. check-governed-queue-guard.mjs --test on the four paths: 'NOT GOVERNED', so the ordinary route applies. THE PIN, both directions on one fixture: inclusion half asserts the exact ids ['open-active','blocked-idle']; exclusion half asserts done-active / null-status / no-status-key absent; a third case asserts the answer is neither ALL_IDS nor []; the lit control asserts selectedIds(undefined) equals ALL_IDS, so a converter that emits nothing usable fails; PRE_FIX_OR_NODE is pinned selecting [], so a converter emitting an unreadable node fails. WOULD A WORSE-THAN-THE-BUG IMPLEMENTATION PASS? No, and the ablation demonstrates why the exclusion half alone was not enough — see below. ABLATION of the READ SITE (the combinator dispatch block in convertFiltersToAST), from the committed implementation, trap on EXIT INT TERM with absolute paths: HEAD blob 5f99be95293258673b88816bdffa2004d8deec6f vs disk blob aec6ab2b7d1cc723422658edfc677bbe12fca336 after mutation (differ), anchor count 1 -> 0, injected marker count 1 at line 234. 19 NAMED RED ROWS across both packages: 'lowers $or to an AST group node', 'lowers $and the same way', 'nests, so a combinator inside a combinator survives as a group', 'AND-combines a combinator with the sibling fields of its own object', 'survives mergeFilterNodes, so a parent scope still NARROWS it', 'emits a node the spec accepts AS A GROUP', 'INCLUDES both branches of the union — not empty', 'so the answer is neither of the two failure shapes', '$and intersects, both directions', 'a parent scope still narrows a union it wraps', 'nested combinators evaluate as written', 'drops $and: [] — the TRUE identity constrains nothing', 'lets a {} disjunct absorb its $or', 'refuses $not nested inside a combinator too', 'refuses a combinator whose value is not an array', 'refuses a non-object member', 'runs the unknown-operator guard on combinator children', and both routes of 'lowers a top-level Mongo logical node to an AST group'. TWO ROWS STAYED GREEN UNDER ABLATION AND THAT IS THE FINDING: 'EXCLUDES the rows outside the union' stayed green because the ablated converter excludes EVERYTHING, and 'the pre-fix node selected NOTHING' stayed green because it is a control on a literal node. That is the brief's hazard reproduced live — an emptiness assertion passes on a builder that emits nothing — and it is why the inclusion half exists. RESTORE BY STATE, not by exit code: git diff HEAD empty and git hash-object equal to git rev-parse HEAD:path, both printed by the restore leg. NO REBUILD LEG IS OWED and it is proven rather than asserted: the root vitest.config.mts line 423 aliases @object-ui/core to packages/core/src, and the data-objectstack pin went RED under a packages/core SOURCE edit with no build in between, which is only possible if there is no dist hop.",
"mcp_calls": "4 — search_issues (1, refused: GraphQL pool rate-limited), create_pull_request (1), pull_request_read (1, PR body read back in full, intact), add_issue_comment (1, this comment). The REST channel is CLOSED for this session: a repo-scoped GET returned 403 'GitHub access is not enabled for this session', with and without an Authorization header, while /rate_limit returned 200. The issue body and timeline were therefore read through the zero-quota public-repo payload channel, not through REST or MCP.",
"open_questions": [
{
"question": "How should this sink carry $not to the wire? The escalation fence in my dispatch fires here: $not is not missing, it is UNREPRESENTABLE at this layer. Measured — FILTER_ARRAY_LOGIC_KEYWORDS is ['and','or'] and VALID_AST_OPERATORS.has('not') is false (53 members), so the AST has no negation keyword; and a De Morgan rewrite cannot be completed either, because startswith, endswith, between and icontains have no negated counterpart in VALID_AST_OPERATORS. A partial rewrite would also silently drop the NULL-safe rule of objectstack#5146, whose guard belongs on each LEAF inside the negation. What I SHIPPED is diagnostic-only and decides nothing: $not threw before this card (naming the author's own nested field as a bogus operator) and throws now with an accurate message, same INVALID_FILTER / 400 envelope. It is one arm, trivially revertible whichever way this is decided.",
"options": [
"A — Add a negation to the ObjectQL AST vocabulary upstream in @objectstack/spec (a 'not' keyword in FILTER_ARRAY_LOGIC_KEYWORDS, handled by parseFilterAST and isFilterAST), then lower $not to it here. Cost: an objectstack card, a spec release, and every AST evaluator (this repo's ValueDataSource included) needs a matching arm. Benefit: the only option under which a spec-declared operator becomes actually deliverable, and the NULL-safe semantics stay where the ruling put them — in the backends.",
"B — Let this sink hand a FilterCondition OBJECT to the wire when a $not is present, instead of an AST. Cost: measured to be worse, not neutral. mergeFilterNodes composes with a parent scope as ['and', parentScope, child], and an object in AST child position makes isFilterAST false for the whole filter; and ValueDataSource routes a non-array $filter to matchesFilter, whose switch has no $not arm and a 'default: break', so the negation would be silently dropped and the list would WIDEN. That is the one failure direction the sink exists to prevent.",
"C — Emit the accidental leaf ['$not','=',{...}], which measurably round-trips: parseFilterAST(['$not','=',{status:'x'}]) is {$not:{status:'x'}}. Cost: correct at the wire, wrong at every in-repo evaluator (field named $not, zero rows) — i.e. exactly the defect this card just fixed for $and/$or, re-introduced for $not. It would also enshrine an accident as a contract.",
"D — Keep the refusal as shipped (status quo of this PR) and file the AST-negation work as a separate objectstack card. Cost: a spec-declared operator stays undeliverable in this renderer, but LOUDLY, with a message that tells the author what to write instead."
],
"recommendation": "D now, A next. D is what the PR already does and it decides nothing; A is the only real fix and it is not this repo's to make. B and C should both be ruled OUT explicitly, because each looks like the cheap answer and each is measurably a regression — B widens the result set, C reproduces the bug being fixed. If the maintainer prefers, the $not arm can be dropped from this PR entirely and the pre-existing misleading throw left in place; I kept it because improving a diagnostic without changing the verdict is not a contract decision."
},
{
"question": "The existing pin in data-objectstack/src/filter-entry-translation.test.ts asserted the OLD emission ('leaves a top-level Mongo logical node alone') with the note 'verified, and the reason this is NOT rewritten here'. I flipped it. Was that mine to flip?",
"options": [
"A — Accept the flip. Its stated concern was the WIRE, and that half is preserved and now asserted rather than narrated: I added a companion case proving both spellings lower to the same FilterCondition. What the note did not cover is the evaluator one door in, which is the whole defect.",
"B — Revert the flip and lower only inside a new function, leaving translateFilterToAST's output byte-identical. Cost: two lowerings for one shape, i.e. the second de-facto contract AGENTS.md #0.1 forbids."
],
"recommendation": "A, and the PR body states the flip explicitly so a reviewer meets it rather than discovers it. Flagging it because a green pin with the word 'verified' on it is exactly the kind of thing a dev should not quietly overturn."
}
],
"out_of_scope_findings": [
"COULD NOT FILE (search_issues refused: 'API rate limit already exceeded for user ID 323634890'), reporting in full per the dispatch — PM to file. ValueDataSource's OBJECT-dialect matcher, matchesFilter (packages/core/src/adapters/ValueDataSource.ts, the switch beginning at line 231 on the base and still present on origin/main), implements only $gt $gte $lt $lte $ne $in $contains $icontains and ends in 'default: break'. Every other declared operator — $eq $nin $between $notContains $startsWith $endsWith $null $exists $and $or $not — therefore adds NO CONSTRAINT and the filter returns every row, silently. That is the opposite posture to its AST sibling matchesASTFilter, which refuses an unreadable node and EXCLUDES the row (objectui#7349, and the refuseFilterNode docblock argues the case at length). One adapter, two answers, selected by whether $filter arrives as an array or an object. Not in this card's scope and untouched by this PR; my change routes at most one extra shape here (an $or absorbed to TRUE with no sibling key), measured to give the same rows as before. Suggested labels: bug, domain:ui.",
"OBSERVATION, no card filed, PM may want a finding label. packages/fields/src/widgets/FilterConditionField.tsx's isMatchAllCriteria treats { $or: [] } as match-all (TRUE), pinned by FilterConditionField.matchAll.test.ts. objectstack#5322 rules { $or: [] } to be FALSE (zero rows), which is what this PR's lowering preserves. The divergence is in the SAFE direction for that surface — it is a sharing-rule warning guard, so over-warning costs a false alarm while under-warning would ship a permissive share — and the file's docblock frames it that way. Recording it so the two readings of the same shape are not later mistaken for a contradiction that needs 'fixing'."
]
}
Generated by Claude Code