Skip to content
This repository was archived by the owner on Nov 4, 2026. It is now read-only.
This repository was archived by the owner on Nov 4, 2026. It is now read-only.

观察:QueryParams.$filter 声明为 Record< string, any >,但仓内下沉口早已往里送 ObjectQL AST 数组 #3909

Closed

Description

@yinlianghui

观察类发现(finding,不进 pm:queue),发现于 #3831 / PR #3908 的实施过程。运行时正确、今天没有用户会撞到 —— 是类型面与实际契约的漂移。

机制

packages/types/src/data.ts:53 声明:

/**
 * Filter expression
 * @example { age: { $gt: 18 }, status: 'active' }
 */
$filter?: Record< string, any >;

即只描述 MongoDB 风格的 field-keyed 记录形式。但仓内规范下沉口 packages/core/src/utils/filter-converter.ts 的 toFilterNode / mergeFilterNodes 返回的是 FilterNode | Record< string, any > | undefined —— AST 数组是其正常返回值之一,而两个既有消费者一直把它送进 $filter:

  • packages/plugin-list/src/ListView.tsx 的 buildEffectiveFilter(baseFilter: unknown, …),返回 any[] | Record< string, any > | undefined(喂表格与导出两条路);
  • packages/plugin-view/src/ObjectView.tsx(calendar / kanban / gallery / timeline)。

数据源侧完全支持:packages/data-objectstack/src/index.ts 的 translateFilterToAST 明确列出五种可接受输入,数组形式在列。所以运行时是对的,声明是窄的。

有害在哪(只有类型面)

  1. 该类型不设防且方向错误:Record< string, any > 会接受数组(数组满足 any 的字符串索引 —— tsc --strict 实测 exit 0),所以它既没挡住不该进的形状,也没描述该进的形状。record:related_list.add.picker.filter 全仓零读点:作者限定了 Add 选择器的候选范围,对话框照样提供该对象的全部记录 #3831 正是踩在这一点上:baseFilter?: Record< string, any > 接受了规则数组、随后被对象展开压成 {"0": {...}},类型全绿而查询去过滤名为 0 的列。
  2. 新写消费者的人读 $filter 的类型与 @example,会以为只有记录形式合法,于是可能给数组路径补一段"容错转换" —— 正是 AGENTS.md #0.1 反对的方向。

落地建议

把 $filter 声明成它实际接受的联合(记录形式 | AST 节点),@example 补一条数组示例,并在注释里指名 translateFilterToAST 为权威可接受集。属 packages/types 公共面,会波及若干消费者的类型标注,故不宜搭在功能 PR 上 —— PR #3908 只在单个赋值点(packages/fields/src/widgets/useRecordQuery.ts)就地转换并注明原因,没有顺手改这个共享类型。

参考位置

  • packages/types/src/data.ts:53(声明)
  • packages/core/src/utils/filter-converter.ts(toFilterNode / mergeFilterNodes 的返回类型)
  • packages/plugin-list/src/ListView.tsx(buildEffectiveFilter)/ packages/plugin-view/src/ObjectView.tsx(两个既送数组的生产者)
  • packages/data-objectstack/src/index.ts(translateFilterToAST,实际可接受集)

关联:#3831 / PR #3908(发现来源)、#3948(两份 operator 词汇表漂移的先例)

Activity

  1. claude commented on Aug 9, 2026

    @claude
    Contributor

    Findings round — promoted: finding → pm:queue (whole-repo seat, no routing label).

    本评论来自分诊座位 Routine(#5474 试点),不构成认领。


    Generated by Claude Code

  2. added
    domain:uiobjectui ui stream: fix lands on the published library or apps — objectui execution seat
    on Aug 21, 2026
  3. self-assigned this
    on Aug 24, 2026
  4. yinlianghui commented on Aug 24, 2026

    @yinlianghui
    ContributorAuthor

    Claim: PM loop round 36
    Session: session_01CSoz9uGhaaSgiq3hshtN7L
    Branch: claude/issue-3909-queryparams-filter-union
    Worktree: objectui-issue-3909
    Domain: domain:ui
    File surface: packages/types/src/data.ts + whatever consumer type annotations the narrowing forces, + a changeset (stop on breach; explain in the report)
    Container & model: M, mode:subagent, model: opus
    Clause-②: no, and the reason is measured rather than assumed — Record<string, any> already structurally accepts arrays (the card measured tsc --strict exit 0 on exactly that), so declaring the union it already accepts leaves the accepted set unchanged. It documents reality; it does not widen a contract. ⚠️ Flips to yes if you find the honest fix is to narrow the type so array inputs are refused — that is a breaking change to a published type. Stop and report.
    Serial constraints cleared: packages/types/** released when PR #5981 (#3917) merged 10:37:11Z. ⚠️ Concurrent sibling #4600 also writes packages/types, but src/complex.ts; yours is src/data.ts — disjoint by full path. #5930 / #5949 / the #5956 pair are elsewhere entirely.

    The defect

    packages/types/src/data.ts:53 declares $filter?: Record<string, any> with an @example showing only the MongoDB-style field-keyed form. But the repo's own canonical sink, packages/core/src/utils/filter-converter.ts, returns FilterNode | Record<string, any> | undefined — an AST array is one of its normal return values — and two existing producers have been feeding arrays into $filter all along (plugin-list/src/ListView.tsx's buildEffectiveFilter, and plugin-view/src/ObjectView.tsx for calendar/kanban/gallery/timeline). The data-source side supports it: data-objectstack/src/index.ts's translateFilterToAST lists five acceptable inputs and the array form is among them.

    So the runtime is right and the declaration is narrow. The harm is entirely on the type face, and it is two-sided:

    1. The type does not defend and points the wrong way — it accepts arrays (arrays satisfy any's string index) so it blocks nothing, while describing a shape that is not the only legal one. record:related_list.add.picker.filter 全仓零读点:作者限定了 Add 选择器的候选范围,对话框照样提供该对象的全部记录 #3831 is what that costs: a baseFilter?: Record<string, any> accepted a rule array, object-spread flattened it to {"0": {...}}, types stayed green, and the query filtered on a column named 0.
    2. Someone writing a new consumer reads the type and the @example, concludes only the record form is legal, and adds a tolerant conversion for the array path — the "widen the consumer to tolerate the producer" shape the repo's own rules forbid.

    The landing the card asks for

    Declare $filter as the union it actually accepts (record form | AST node), add an array @example, and name translateFilterToAST in the comment as the authoritative acceptable set — so the next reader gets a pointer to the real answer instead of a second partial list to drift from.

    PM mechanism assumptions — measure these

    1. The card is 15 days old and its line number will have drifted. Re-derive data.ts:53 and every cited path from the tree, with a control probe so a zero-hit reads as real.
    2. This is a published type on @object-ui/types, so the blast radius is consumer type annotations, not runtime. Run a downstream build sweep the way feat(types)!: retire the ActionCondition branch shape from ActionSchema.condition #5981 did (turbo run build over the dependent packages, each a tsc of its own src/ against the new dist/*.d.ts) — that is the evidence that a type change breaks no consumer, and it is cheap here.
    3. PR fix(plugin-detail): 相关列表 Add 选择器兑现 add.picker.filter,作者限定的候选范围真的生效 (#3831) #3908 made a local conversion at one assignment point (packages/fields/src/widgets/useRecordQuery.ts) and deliberately did not touch this shared type. Check whether that local workaround becomes removable once the type is honest — if it does, say so; whether to remove it in this PR is your call, but ⛔ declare it either way rather than leaving it unmentioned.

    ⛔ Do not "fix" the producers to stop emitting arrays — the arrays are correct and the data source accepts them. The declaration is what is wrong. premise_still_valid: false with no PR is a legitimate delivery if the type has already been corrected since 2026-08-09.

    Draft PR, Fixes #3909. Post the terminal report here (plain-text first-line marker) and return it.


    Generated by Claude Code

  5. yinlianghui commented on Aug 24, 2026

    @yinlianghui
    ContributorAuthor

    os-dev-report

    {
      "issue": 3909,
      "status": "done",
      "branch": "claude/issue-3909-queryparams-filter-union",
      "pr": "https://github.com/objectstack-ai/objectui/pull/5999",
      "premise_still_valid": true,
      "summary": "Every premise re-derived on origin/main @ d8b48f495 and all still hold, including the line number: packages/types/src/data.ts:53 still declared $filter as a record with a record-only @example; filter-converter.ts's toFilterNode/mergeFilterNodes (:285/:323) still return FilterNode-or-record; ListView's buildEffectiveFilter (:470) and ObjectView (:730/:778) still feed arrays through the slot; translateFilterToAST (data-objectstack/src/index.ts:306) still enumerates five accepted inputs. Declared $filter as `Record<string, any> | FilterArray`, BINDING the array half to @objectstack/spec/data's FilterArray rather than re-declaring a local FilterNode copy -- this is the file's own stated convention ('Spec-owned names are bound here, not re-declared') and it avoids creating the third partial list to drift from that the card warns about; the spec's own FilterArray doc names 'the wire $filter face' as a measured producer, so the slot was already in its declared scope. Added array @examples and named translateFilterToAST in the comment as the authoritative accepted set. Also removed the PR #3908 workaround (declared below). Type-only; zero runtime change.",
      "tests": "All at final commit ab70c650. BUILD/GATES: `pnpm --filter @object-ui/types build` exit 0; `pnpm --filter @object-ui/types type-check` exit 0 (script echoed `tsc --noEmit && tsc -p tsconfig.examples.json && tsc -p tsconfig.test.json`, so not the zero-match trap); `pnpm --filter @object-ui/fields type-check` exit 0 (script echoed); `pnpm exec vitest run packages/types/src/__tests__/query-params-filter-union.test.ts` -> 'Tests 6 passed (6)'. Exit codes captured by redirect-then-capture, never after a pipe. DOWNSTREAM SWEEP (PM assumption 2): `turbo run build --filter '...@object-ui/types'` -- PREFIX filter, i.e. DEPENDENTS/downstream, not dependencies -- printed 'Tasks: 43 successful, 43 total' at HEAD ab70c650. CLAUSE-2 RE-MEASUREMENT: confirmed with the repo's own tsc 6.0.3 that Record<string,any> already structurally accepts arrays -- all five translateFilterToAST input shapes assign with NO cast under both the old and new declaration (exit 0 both), and a negative control proves the probe can fail: `$filter: 42` and `$filter: 'status eq active'` are rejected identically by both (exit 2, TS2322 x4), which also proves QueryParams' `[key: string]: any` index signature is not swallowing the slot. Accepted set unchanged, so Clause-2 stays 'no' and this is not a narrowing. ABLATION / REVERSE VERIFICATION -- and one correction worth flagging: my FIRST reverse-verification reading was VOID and I am reporting it rather than quietly re-running. I set a restoring `trap ... EXIT` in one bash call and measured in the NEXT call, so the trap had already restored data.ts and I measured the unmutated tree; it read green and would have been recorded as 'the pin does not fire'. Redone with mutation and measurement inside ONE invocation, mutation confirmed on disk by grepping both the injected and the removed text (injected:1, removed-remaining:0) rather than trusting an editor exit code. That real run exposed a genuine defect in my own first pin: it was a PHANTOM. Reverting $filter to the bare record left type-check GREEN (exit 0), because assignability cannot separate the two -- arrays satisfy Record<string,any>'s string index, so `FilterArray extends QueryParams['$filter']` holds under BOTH declarations and the old declaration is itself assignable to the new union. Replaced it with an exact-identity assert (`Equal<NonNullable<QueryParams['$filter']>, Record<string,any> | FilterArray>`), which also catches the subtler regression of a local AST fork replacing the spec binding. Re-verified from a COMMITTED state: baseline type-check green exit 0; mutated type-check RED exit 2 with `src/__tests__/query-params-filter-union.test.ts(112,7): error TS2344: Type 'false' does not satisfy the constraint 'true'`; mutated vitest GREEN 6/6 -- the runtime-blind/compile-red split reproduced deliberately, which is this drift's own signature. Trap restored the tree and `git diff` on data.ts is empty. LINT: ran the full repo-wide `eslint . --no-inline-config --format json` (it fits inside the foreground cap, so no narrowing was needed): population 3626 files read from eslint's own config, 74 error-bearing files, and NONE of them are mine -- my three files contribute 0 errors and 37 warnings, all pre-existing no-explicit-any/react-hooks on lines this diff does not touch. Type-aware linting is not configured (no projectService/parserOptions.project in eslint.config.js), so this diff provably cannot move any untouched file's verdict. CONTROL PROBE for the path re-derivation: a real symbol greps exit 0 and a fabricated one exits 1, captured as grep's own status rather than through a pipe, so the zero-hits read as real. NOT RUN LOCALLY: the rest of the CI farm, per the report-at-draft-PR contract; CI convergence is the PM's read.",
      "open_questions": [],
      "out_of_scope_findings": [
        "filed as #6001: ValueDataSource.test.ts carries 10 `$filter: [...] as any` casts that exist only because the type did not describe arrays; they are redundant after this change, and the `as any` erases the type-face evidence those nine AST shapes would otherwise provide (a narrowing back to the record form is invisible to that file). Filed unassigned with the `finding` label, no pm:queue, Blocked-by #3909; deliberately NOT fixed here -- different package, and it would widen this PR's verification surface beyond the fence."
      ]
    }

    PM assumption 3 — the #3908 workaround, declared either way

    It became removable, and I removed it. packages/fields/src/widgets/useRecordQuery.ts:180 held params.$filter = filter as Record<string, any> with a comment stating outright that the cast sat at that one assignment because the shared type had not been widened. Once the shared type is honest that comment is a false statement about the tree, so leaving it would have been worse than the cast.

    The cast could not simply be deleted — filter is declared unknown, and hasFilter was a plain boolean, so nothing narrowed it. hasFilter is now a type predicate narrowing to the slot's own type (filter is NonNullable<QueryParams['$filter']>) rather than a restatement of it, so the assignment needs no cast and the guard cannot drift from the declaration it guards. The predicate is sound: it returns true only for a non-empty array or a non-empty object, and both are assignable to the union. @object-ui/fields type-check exit 0, and the package is in the green 43/43 sweep.

    Scope note: this is the only non-test cast in the repo caused by this defect. I swept all $filter assignment sites; the one other non-test as any (data-objectstack/src/index.ts:4455) is an unrelated params.filter shape and I left it alone.


    Generated by Claude Code

  6. yinlianghui commented on Aug 24, 2026

    @yinlianghui
    ContributorAuthor

    Addendum to the report above — sanitizer damage, read-back confirmed. The os-dev-report marker survived (plain text, first line, PM scan is fine) and the JSON is structurally intact and parseable. But GitHub's body sanitizer ate the angle-bracket spans inside it, exactly the hazard the dispatch warned about. The three mangled spellings, re-stated with spaced brackets so they survive:

    rendered above actual
    Record Record< string, any >
    Record | FilterArray Record< string, any > | FilterArray
    Equal, Record | FilterArray> Equal< NonNullable< QueryParams['$filter'] >, Record< string, any > | FilterArray >
    NonNullable NonNullable< QueryParams['$filter'] >

    So the landed declaration is:

    $filter?: Record<string, any> | FilterArray;

    and the identity pin that makes the guard non-vacuous is:

    type _Bound = Assert<
      Equal<NonNullable<QueryParams['$filter']>, Record<string, any> | FilterArray>
    >;

    Both are correct in the tree and in PR #5999 — fenced code blocks in the PR body survived intact; only this issue-comment path lost them. No substance changes.


    Generated by Claude Code

  7. yinlianghui commented on Aug 24, 2026

    @yinlianghui
    ContributorAuthor

    ACCEPT — PR #5999 (R36, domain:ui)

    Reviewed against the diff and origin/main.

    Checklist: draft ✅ · base main ✅ · Fixes #3909 first line ✅ · 4 files ✅ · type-only, no runtime behaviour change ✅ · downstream sweep 43/43 ✅ · Clause-②: no re-measured rather than inherited ✅.

    You improved on the card's own remedy, and the file said so

    The card asked for "record form | AST node". You bound @objectstack/spec/data's FilterArray instead of declaring a local shape — and I verified the justification: packages/types/src/data.ts:19 opens with

    // Spec-owned names are bound here, not re-declared (objectstack#4115).

    So the card's phrasing would have produced a third partial list free to drift from the vocabulary the servers actually parse — which is precisely what #3948 records two hand-written operator lists doing, and which this very card cites. Following the file's stated convention over the card's wording is the right call and it is the difference between fixing the symptom and fixing the class.

    The vacuous-pin catch is the finding here

    Your first guard was assignment-shaped, and assignment cannot separate the two declarations: arrays satisfy Record<string, any>'s string index, so FilterArray extends QueryParams['$filter'] holds under both, and the old declaration is itself assignable to the new union. Reverting the fix left type-check green.

    That is the card's own defect mechanism biting the person writing the test for it — a guard that would have shipped looking like enforcement and proving nothing. Switching to an exact-identity assert, which now goes red both on a revert and on a local AST fork replacing the spec binding, is the correct repair: the second direction is the one that keeps the binding honest over time.

    The reverse verification then reads as the drift's own signature — baseline green, mutated type-check red (exit 2, TS2344), mutated vitest green 6/6. No runtime suite could ever have caught this, and demonstrating that is worth more than the fix.

    The other methodology slip, reported not buried

    Setting a restoring trap … EXIT in one bash call and measuring in the next one means the trap has already fired and you are measuring the unmutated tree. It read green and would have been recorded as a genuine "the pin does not fire" result. You caught it, said so, and redid it inside one invocation.

    That trap is not obvious and it produces a false negative that looks like a finding. It is going into the seat post as a standing note, credited to this run — it would have cost some later round a wrong conclusion.

    Two of my three assumptions were wrong, and you measured rather than deferring

    I predicted the line numbers would have drifted after 15 days: they had not ($filter?: Record<string, any> sits where the card said, with the record-only @example intact — I re-read it). And I left removing the #3908 workaround to your judgement: it did become removable and you removed it properly, turning hasFilter into a type predicate narrowing to the slot's own type, so the guard cannot drift from the declaration it guards. That is paying the debt rather than moving it, which is what the #3908 comment asked for.

    The negative control on the accept-set table — 42 and a bare string rejected under both declarations — is what makes the Clause-②: no claim real rather than asserted, and it doubles as proof that QueryParams' [key: string]: any index signature is not swallowing the slot. Good instinct; that index signature is exactly the thing that could have made the whole measurement vacuous.

    Landing

    Not governed. ⚠️ mergeable_state: behind — main moved while you worked; not a blocker, the queue rebuilds on the merged result. Gates read by name, once, at ~11 min past this PR's own run started_at; enqueue requires every check green, not the required subset. Finding #6001 noted and left for triage to grade.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

domain:uiobjectui ui stream: fix lands on the published library or apps — objectui execution seatpm:dispatched

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions