Skip to content

Four live adapter.find calls pass an unprefixed query option — and no-query-params-under-options gates only the sibling half of the class #5458

Closed

Description

@os-zhuang

Moved from objectstack-ai/objectstack#10470 by the triage seat (file-at-destination rule: every named site and the rule that should gate them live in this repo). Original filed 2026-08-21 by os-zhuang from the objectstack-ai/objectstack#10288 dev seat. Body reproduced verbatim below with bare issue refs qualified.


Census taken at objectui c40f3b8ca21ddc19e05682f1719fcf16d4ce7fba (objectstack pins 9a3daf8d37ad973a621e5edd276fe32467f90684; re-confirm at the pin before acting).

The class

QueryParams (packages/types/src/data.ts) declares only $-prefixed keys, and ObjectStackAdapter.convertQueryParams (packages/data-objectstack/src/index.ts) builds its outgoing options by copying exactly those. Any other key reaches no branch and is dropped — no throw, no warning. QueryParams also carries an index signature, so the type system accepts both spellings equally.

The consequence is an unbounded read, not a truncated one: the platform's GET list route has no default page size (objectstack packages/client/src/index.ts, pinned in its client.test.ts), so an absent top returns the entire match set. A dropped cap is therefore invisible until the object is large.

The four live sites

file call dropped key
apps/console/src/sdui-workbench-preview.tsx:65 find(objectName, { top: 200 }) top
packages/app-shell/src/views/ObjectView.tsx:1561 find(…, { limit: 0 }) limit
packages/app-shell/src/views/metadata-admin/AssignedUsersSection.tsx:109 find('sys_permission_set', { $filter: {…}, limit: 1 }) limit
content/docs/guide/react-pages.md:129 find(…, { filters: [['status','=','open']] }) filters

ObjectView.tsx:1561's limit: 0 is worth reading twice: $top: 0 is honoured end to end as "no records" (objectstack-ai/objectstack#6485 pinned it), so if that call means what it says, dropping the key inverts it from no rows to every row.

AssignedUsersSection.tsx:109 is a one-line-away neighbour of three correct calls ($top: 500, $top: 200, $top: 1000 on L124/131/136) — the shape a reviewer's eye slides over.

sdui-workbench-preview.tsx:66 also reads all.records off the result, with no .data arm. find() resolves to a QueryResult (data, never records), so that preview lists nothing regardless of the cap. skills/objectui/guides/data-integration.md:268 publishes the same return result.records; to skill consumers.

Why a rule, and which one

eslint-rules/no-query-params-under-options.js already gates the sibling shape — find(obj, { options: { $top: 100 } }) — and its own header makes the argument for mechanising this family: the mistake type-checks, it publishes, the symptom looks like a data problem rather than a code problem, "so a review catches it once and then misses the next one." It cites two live instances by two authors (object-timeline, objectui#4009 / objectstack-ai/objectstack#7137; object-kanban, objectui#4025).

The rule bans a $-prefixed key under options. It does not look at a bare key at the top level, which is the other half of the same class — and the half with four live sites. Extending it (or adding a sibling rule) is the cheap move: the AST walk already lands on the params object literal of a find/findOne call, the fix is a second predicate, and the contract lives in the same repo as the rule.

Suggested shape, narrow on purpose: flag a static key that is a known query-option name without its $ (top, skip, filter, filters, select, orderby, sort, expand, search, count, limit, offset) in the second argument of an adapter.find / dataSource.find / …findOne call. Not "any unprefixed key" — adapters legitimately take adapter-specific params, which is why the index signature exists.

An objectstack-side gate was considered for objectstack-ai/objectstack#10288 and rejected: QueryParams is objectui's contract, so a checker there would have to hard-code another repo's key list and become a second source of truth for it. The rule belongs next to the type it enforces.

Related

Activity

  1. added theissue type on Aug 21, 2026
  2. added
    domain:devxobjectui devx stream: fix lands on .github/, scripts/ or release pipeline — devx lane cross-repo
    on Aug 21, 2026
  3. self-assigned this
    on Aug 24, 2026
  4. os-warren commented on Aug 24, 2026

    @os-warren
    Collaborator

    Claim: PM loop round 3
    Session: session_01PQ3NihCHE9LUtHoGxo6A9f
    Branch: claude/issue-5458-unprefixed-query-options
    Worktree: objectui-issue-5458
    Domain: domain:devx (@ objectui, seat #5748)
    File surface: eslint-rules/no-query-params-under-options.js + its test; apps/console/src/sdui-workbench-preview.tsx; packages/app-shell/src/views/ObjectView.tsx; packages/app-shell/src/views/metadata-admin/AssignedUsersSection.tsx. (stop on breach; explain in the report)
    Container & model: M, mode:subagent, model: opus — the rule's scope is a judgement call. ⚠️ Tier is this seat's explicit decision; scripts/pm/dispatch-gates.mjs does not exist in objectui.
    Clause-②: no — and the boundary is load-bearing here, so read the constraint below. A lint rule that refuses more shapes tightens; it does not widen an acceptance set. What would be Clause-② is changing convertQueryParams to start honouring unprefixed keys, and that is forbidden below.
    Serial constraints cleared: PR #5925 is open against packages/app-shell/ — src/preview/DraftChangesPanel.tsx, comment-only, disjoint from the two src/views/** files here; read but ⛔ do not touch. Other open PRs by head: #5922 (layout docs), #5843 (governed docs/adr), #5774 (.github/workflows/ + ci-cd-pipeline.md), #5400 (release), #4958 (dependabot) — none intersect. Concurrent in this batch: #3840, which touches apps/console/src/__tests__/public-block-binding-reach.test.tsx only — disjoint from your apps/console file, but ⛔ do not edit that probe.

    ⚠️ The census has changed since the card was written — 3 live sites, not 4

    Re-measured on origin/main @ 2074a59. The card's census was taken at c40f3b8ca, and the fourth site is already fixed. Use this table, ⛔ not the card's:

    site card said measured now
    apps/console/src/sdui-workbench-preview.tsx :65, find(…, { top: 200 }) live, moved to :86
    packages/app-shell/src/views/ObjectView.tsx :1561, find(…, { limit: 0 }) live, moved to :1687
    .../metadata-admin/AssignedUsersSection.tsx :109, find(…, { …, limit: 1 }) live, still :109
    content/docs/guide/react-pages.md :129, find(…, { filters: [[…]] }) ALREADY FIXED — :231 now reads $filter: ['status', '=', 'open'], and :243 documents the rule in prose

    That doc page is now an ally, not a target: it already tells readers "An unprefixed filters: or top: is not a query option". ⛔ Do not re-fix it, and do not count it in your before/after. Every line number above is a lead — re-derive from the tree, ⛔ do not trust mine either.

    Why this matters more than a lint nit

    The failure is an unbounded read, not a truncated one. The platform's GET list route has no default page size, so a dropped cap returns the entire match set — invisible until the object is large. And ObjectView.tsx's limit: 0 inverts: $top: 0 is honoured end-to-end as no records, so dropping the key turns "no rows" into "every row". Establish what that call is actually meant to do before you rewrite it — ⛔ a mechanical limit → $top there could be the wrong fix if the intent was never "no rows".

    AssignedUsersSection.tsx:109 sits one line from three correct calls ($top: 500 / 200 / 1000) — the shape a reviewer's eye slides over, which is the argument for the rule.

    ⛔ Hard constraint — fix the call sites, not the adapter

    QueryParams declares only $-prefixed keys and convertQueryParams copies exactly those; the index signature is deliberate, because adapters legitimately take adapter-specific params. ⛔ Do not make the adapter start accepting unprefixed keys — that widens the acceptance set, which is Clause-② work and a maintainer-floor decision, not this card's. If you conclude the adapter is the right place, stop and report rather than doing it.

    The rule — narrow on purpose

    Extend no-query-params-under-options (or add a sibling; justify whichever you pick). The AST walk already lands on the params object of a find/findOne call, so this is a second predicate: flag a static key that is a known query-option name missing its $ — top, skip, filter, filters, select, orderby, sort, expand, search, count, limit, offset — in the second argument. ⛔ Not "any unprefixed key" — that would flag the legitimate adapter-specific params the index signature exists for, and a rule that cries wolf gets disabled.

    Requirements on the rule:

    1. It must fail on all three live sites before your fix and pass after. Show both runs. A rule that only passes after the call sites are already fixed proves nothing about the rule.
    2. It must not flag the correct neighbours ($top: 500 / 200 / 1000 in the same file) — assert that explicitly, it is the false-positive control.
    3. The existing sibling half ({ options: { $top: 100 } }) must keep failing. ⛔ No regression in what is already gated.
    4. The rule must be wired into the lint config that CI actually runs. An unregistered rule file is a no-op that looks like a fix — verify by making it fire, not by reading config.

    One more thing the card noticed, and it is a real second defect

    sdui-workbench-preview.tsx reads all.records off the result, but find() resolves to a QueryResult (data, never records) — so that preview lists nothing regardless of the cap. Fix it while you are in the file. ⚠️ skills/objectui/guides/data-integration.md:268 publishes the same return result.records; to skill consumers — that is a published skills surface. ⛔ Do not edit skills/** on this card; if it is still wrong on today's tree, report it and I will file it, since published skill packages are evaluated whole-package and carry their own line budget.

    Premise-first

    The body is a lead, not a specification. If a site is already fixed — one of four already was — report the measurement instead of manufacturing a diff. Read the card on GitHub yourself and check the body for sanitizer truncation; note it was moved verbatim from objectstack#10470, so bare refs there were rewritten.


    Generated by Claude Code

  5. os-warren commented on Aug 24, 2026

    @os-warren
    Collaborator
    {
      "issue": 5458,
      "status": "done",
      "branch": "claude/issue-5458-unprefixed-query-options",
      "pr": "https://github.com/objectstack-ai/objectui/pull/5946",
      "premise_still_valid": true,
      "summary": "Read the card on GitHub; body is COMPLETE, no sanitizer truncation (ends on its full '## Related' list, no mid-sentence cut) — the only sanitizer effect is HTML-entity escaping of quotes/apostrophes from the verbatim move out of objectstack#10470. Your corrected census held on today's tree (origin/main had moved 2074a59 -> ed35c23; branched from ed35c23): sdui-workbench-preview.tsx live at :86, ObjectView.tsx live at :1687, AssignedUsersSection.tsx live at :109, and content/docs/guide/react-pages.md ALREADY FIXED (:231 uses $filter, :243 documents the rule) — not touched, not counted. Fixed the three live sites plus a FOURTH the card never named, found by the new rule on its first repo-wide run: plugin-dashboard DashboardFilterBar.tsx:264 passed 'fields' AND 'top' in one literal, so a dashboard filter's option list read every row and every column of its source while its own comment described it as capped at 200; the same call read records.items (not a QueryResult member) so against a real adapter it produced zero options. ObjectView intent (ruling 4): the call is unambiguously count-only — comment says 'Fetch record count for footer display', the handler reads result.total first, the value feeds the footer's recordCount — so $top:0 is the correct spelling of what limit:0 meant, and I verified the pinned @objectstack/client@17.2.0 serializes it ('if (normalizedOptions.top != null)', so top=0 reaches the wire, and an ABSENT top is what is unbounded). The non-mechanical half: the row-counting fallbacks are REMOVED, not repointed — after asking for zero rows an empty data means 'you asked for none', not 'the object is empty', so keeping them would report a confident 0 for any adapter without a total; recordCount now falls to undefined and the footer line is omitted (render already guards on typeof === 'number'). No observable change for the ObjectStack adapter, whose normalizeQueryResult always yields a numeric total. Adapter NOT touched (ruling 1). New sibling rule object-ui/no-unprefixed-query-params rather than a second predicate on no-query-params-under-options: the sibling's $-key-under-options signature is context-free, while every name on this list is an ordinary key away from a finder call, so the two need different anchors — and one eslint-disable must not silence both halves. TWO PREMISE CORRECTIONS: (1) the card's claim that sdui-workbench-preview 'lists nothing regardless of the cap' is not what the file does — its fake adapter is 'find: async () => store.slice()', a bare array, so the Array.isArray arm wins and the .records arm was never reached there; it was a latent wrong read in a teaching surface, not a live blank list (the live blank list is DashboardFilterBar, which has no .data arm at all). (2) requirement (a) is UNMEETABLE for one of the three sites and I did not manufacture a pass: sdui-workbench-preview's find() sits inside a template literal holding runtime page metadata, so ESLint sees one TemplateLiteral and never a CallExpression — no AST rule can reach it. Measured, not assumed: on the pre-fix tree the repo-wide run reported the other sites and ZERO findings in that file while its { top: 200 } was live. Fixed by hand, boundary documented in the rule docblock, gap filed as #5944 with a suggested mechanism.",
      "tests": "All runs at final commit 3a683ea (tree clean; `git rev-parse --short HEAD` = 3a683ea). Exit codes captured before any pipe. (a) BEFORE/AFTER, real repo eslint.config.js, same three files: BEFORE 'x 186 problems (2 errors, 184 warnings)' with ObjectView.tsx 1687:47 and AssignedUsersSection.tsx 109:90 both object-ui/no-unprefixed-query-params; AFTER exit 0, 'x 184 problems (0 errors, 184 warnings)', 0 new-rule hits. (b) false-positive control: same BEFORE run, the correct neighbours $top:500 (L124), $top:200 (L131), $top:1000 (L151/171) produced zero reports — only L109 fired; pinned as valid cases in the test. (c) sibling regression: no-query-params-under-options unchanged and re-run in the new test file on both instances it was written for, still erroring. (d) wiring verified by making it fire (the runs above use the repo config), plus scripts/__tests__/lint-workflow.test.ts green — it imports the RESOLVED flat config; no rule name added to any of its three scanned prose surfaces. POPULATION CENSUS (load-bearing: error-level repo-wide rule): rule enabled alone over the repo, 'files linted: 3615 | files with hits: 0', CENSUS_EXIT=0; the pre-fix run of that same scan is what surfaced DashboardFilterBar.tsx. VITEST from repo root, each with VITEST_EXIT=0: eslint-rules/ 8 files/174 tests; + scripts/__tests__/lint-workflow.test.ts 9 files/183 tests; AssignedUsersSection.test.tsx + packages/plugin-dashboard/ 81 files/755 tests; packages/app-shell/src/views/ObjectView + apps/console/ 85 files/965 tests (includes sdui-preview-page-source-styling.test.ts, which pins the source string I edited). TYPECHECK: dependency closure built first (pnpm --workspace-concurrency=2 --filter '<pkg>^...' build, BUILD_EXIT=0), then 'pnpm --filter @object-ui/app-shell --filter @object-ui/plugin-dashboard --filter @object-ui/console type-check', TYPECHECK_EXIT=0 — script name is 'type-check' with a hyphen; the hyphenless spelling matches zero scripts and exits 0 having run nothing, so I confirmed all three package names are echoed ('packages/plugin-dashboard type-check: Done', 'packages/app-shell type-check: Done', 'apps/console type-check: Done'). NODE GATES, each by name with its own printed verdict: check-changeset-presence exit=0 '5 source file(s) of 3 released package(s) changed, and this change declares 1 changeset(s)'; check-changeset-no-major exit=0 'No changeset declares a major bump'; check-control-bytes exit=0 'OK (scanned 4931 tracked text file(s); skipped 85 binary)'; check-lint-coverage exit=0 'lint coverage: 46/46 packages linted, 0 with outstanding errors'. Also self-scanned every changed file with grep -naP for control bytes: clean. REVERSE VERIFICATION of the records.items->data fix: direction predicted first (only the new test reds). NO REBUILD NEEDED AND NONE CLAIMED — the test imports '../DashboardFilterBar' by relative path, so vitest transforms source and no dist is in the resolution path. MUTATION CONFIRMED ON DISK before running, not via an editor exit code: injected 'records?.items' count 1, removed 'records?.data' count 0, plus git diff --stat showing 1 file/1 insertion/1 deletion. Result: 1 failed, 13 passed — the failure is exactly the new 'QueryResult envelope' case timing out on findByRole('option'), i.e. the dropdown opening empty. Restore leg ran from a trap on EXIT/INT/TERM and was verified byte-identical (git status and git diff both empty afterwards). FIXTURE TRIAGE: DashboardFilterBar.options.test.tsx pinned the DEAD spelling (expect.objectContaining({ top: 200 })) — it passed while asserting the defect; retargeted to the exact params object { $select: ['industry'], $top: 200 } so a partial matcher can no longer hide a bare key. LOCAL SCOPE DECLARED: I did not run repo-wide `pnpm lint` (all 55+ rules); I ran the full repo config on every changed file (exit 0) plus the new rule alone over all 3615 files (0 hits). The narrowing is a measurement, not a gap: the population came from eslint's own config resolution rather than my guess, the 3615 count is read from --format json, and type-aware linting is NOT enabled in eslint.config.js (no projectService, no parserOptions.project, no recommendedTypeChecked), so this diff cannot move the verdict on any untouched file. CI runs the full farm regardless.",
      "open_questions": [
        {
          "question": "I added one entry beyond the ruling's enumerated key list: `fields` -> `$select`. Keep it or drop it?",
          "options": [
            "A. Keep. It is the same KIND of entry as the listed `limit`->`$top`, `offset`->`$skip`, `filters`->`$filter` and `sort`->`$orderby` (alias spellings, not literal missing-`$` names), and unlike the original 12 it has a live instance behind it: DashboardFilterBar passed `fields` and `top` in one literal. Re-ran the repo-wide census with it included: 3615 files, 0 hits, so it adds no false positives.",
            "B. Drop it back to the ruling's 12. The rule then gates the `top` half of that call and stays silent on the `fields` half two lines above it — the exact one-line-away blindness the card was written about."
          ],
          "recommendation": "A. The ruling's binding content is 'narrow and closed, not any unprefixed key', and this keeps it closed while being evidence-backed rather than speculative — the rule's own docblock invites exactly this ('add one when a real instance appears, with the instance cited') and cites the site. Flagging it because the card's list was explicit and this is your call, not mine."
        },
        {
          "question": "One of the three live sites has no mechanical gate and cannot have one from an AST rule. Accept as-is, or close the gap?",
          "options": [
            "A. Accept. The harness's fake adapter ignores its params entirely, so the hazard there is pedagogical, not an unbounded read.",
            "B. Close it with a sibling of apps/console/src/__tests__/sdui-preview-page-source-styling.test.ts, which already Vite-glob-enumerates the preview harnesses and extracts their `source` strings — parse each with @object-ui/sdui-parser and reuse the rule's key map. Filed as #5944."
          ],
          "recommendation": "B eventually, but NOT on this card — it needs a new test file outside the dispatched file surface, and a text scan over these key names is not the naive alternative (it would match the rule's own docblock and the react-pages prose). Filed rather than done."
        }
      ],
      "out_of_scope_findings": [
        "filed as #5944: no-unprefixed-query-params cannot see find() calls inside page-source template literals, so one of the four sites has no mechanical gate (label: finding, unassigned)",
        "filed as #5945: asArray helpers in useApproverDirectory.ts:82 and AssignedUsersSection.tsx:88 read `res?.records ?? res?.items ?? res?.data` — two non-contract shapes tried BEFORE the real QueryResult member, the tolerant reader that let three separate .records/.items misreads survive (label: finding, unassigned)",
        "NOT FILED, yours per the dispatch — skills/objectui/guides/data-integration.md is worse than the card recorded. Beyond `return result.records` (now at :275, not :268), the SAME snippet drops three query options (`filter:`, `sort:`, `limit: 20`), and the root cause is upstream at :78 where the published `QueryResult` sketch declares `records?: T[]` and `pageNumber?`, omitting `data` entirely — so the guide's example is consistent with a type declaration that is itself wrong. Real QueryResult is { data, total, page, pageSize, hasMore, cursor, metadata }. Published skill surface, whole-package line budget, untouched."
      ]
    }

    Generated by Claude Code

  6. os-warren commented on Aug 24, 2026

    @os-warren
    Collaborator

    os-dev-report

    Supersedes the previous comment (5390168300): the GitHub sanitizer ate its <!-- os-dev-report --> marker on the way into storage — the stored body begins with a bare code fence, so a marker scan would not have seen it. Same report, marker as literal text. The sanitizer also swallowed a short angle-bracket fragment inside the tests string there (--filter 'PKG^...' had been written with angle brackets); rewritten below.

    {
      "issue": 5458,
      "status": "done",
      "branch": "claude/issue-5458-unprefixed-query-options",
      "pr": "https://github.com/objectstack-ai/objectui/pull/5946",
      "premise_still_valid": true,
      "summary": "Read the card on GitHub; body is COMPLETE, no sanitizer truncation (ends on its full '## Related' list, no mid-sentence cut) — the only sanitizer effect is HTML-entity escaping of quotes/apostrophes from the verbatim move out of objectstack#10470. Your corrected census held on today's tree (origin/main had moved 2074a59 to ed35c23; branched from ed35c23): sdui-workbench-preview.tsx live at :86, ObjectView.tsx live at :1687, AssignedUsersSection.tsx live at :109, and content/docs/guide/react-pages.md ALREADY FIXED (:231 uses $filter, :243 documents the rule) — not touched, not counted. Fixed the three live sites plus a FOURTH the card never named, found by the new rule on its first repo-wide run: plugin-dashboard DashboardFilterBar.tsx:264 passed 'fields' AND 'top' in one literal, so a dashboard filter's option list read every row and every column of its source while its own comment described it as capped at 200; the same call read records.items (not a QueryResult member) so against a real adapter it produced zero options. ObjectView intent (ruling 4): the call is unambiguously count-only — comment says 'Fetch record count for footer display', the handler reads result.total first, the value feeds the footer's recordCount — so $top:0 is the correct spelling of what limit:0 meant, and I verified the pinned @objectstack/client@17.2.0 serializes it ('if (normalizedOptions.top != null)', so top=0 reaches the wire, and an ABSENT top is what is unbounded). The non-mechanical half: the row-counting fallbacks are REMOVED, not repointed — after asking for zero rows an empty data means 'you asked for none', not 'the object is empty', so keeping them would report a confident 0 for any adapter without a total; recordCount now falls to undefined and the footer line is omitted (render already guards on typeof === 'number'). No observable change for the ObjectStack adapter, whose normalizeQueryResult always yields a numeric total. Adapter NOT touched (ruling 1). New sibling rule object-ui/no-unprefixed-query-params rather than a second predicate on no-query-params-under-options: the sibling's $-key-under-options signature is context-free, while every name on this list is an ordinary key away from a finder call, so the two need different anchors — and one eslint-disable must not silence both halves. TWO PREMISE CORRECTIONS: (1) the card's claim that sdui-workbench-preview 'lists nothing regardless of the cap' is not what the file does — its fake adapter is 'find: async () => store.slice()', a bare array, so the Array.isArray arm wins and the .records arm was never reached there; it was a latent wrong read in a teaching surface, not a live blank list (the live blank list is DashboardFilterBar, which has no .data arm at all). (2) requirement (a) is UNMEETABLE for one of the three sites and I did not manufacture a pass: sdui-workbench-preview's find() sits inside a template literal holding runtime page metadata, so ESLint sees one TemplateLiteral and never a CallExpression — no AST rule can reach it. Measured, not assumed: on the pre-fix tree the repo-wide run reported the other sites and ZERO findings in that file while its { top: 200 } was live. Fixed by hand, boundary documented in the rule docblock, gap filed as #5944 with a suggested mechanism.",
      "tests": "All runs at final commit 3a683ea (tree clean; `git rev-parse --short HEAD` = 3a683ea). Exit codes captured before any pipe. (a) BEFORE/AFTER, real repo eslint.config.js, same three files: BEFORE 'x 186 problems (2 errors, 184 warnings)' with ObjectView.tsx 1687:47 and AssignedUsersSection.tsx 109:90 both object-ui/no-unprefixed-query-params; AFTER exit 0, 'x 184 problems (0 errors, 184 warnings)', 0 new-rule hits. (b) false-positive control: same BEFORE run, the correct neighbours $top:500 (L124), $top:200 (L131), $top:1000 (L151/171) produced zero reports — only L109 fired; pinned as valid cases in the test. (c) sibling regression: no-query-params-under-options unchanged and re-run in the new test file on both instances it was written for, still erroring. (d) wiring verified by making it fire (the runs above use the repo config), plus scripts/__tests__/lint-workflow.test.ts green — it imports the RESOLVED flat config; no rule name added to any of its three scanned prose surfaces. POPULATION CENSUS (load-bearing: error-level repo-wide rule): rule enabled alone over the repo, 'files linted: 3615 | files with hits: 0', CENSUS_EXIT=0; the pre-fix run of that same scan is what surfaced DashboardFilterBar.tsx. VITEST from repo root, each with VITEST_EXIT=0: eslint-rules/ 8 files/174 tests; + scripts/__tests__/lint-workflow.test.ts 9 files/183 tests; AssignedUsersSection.test.tsx + packages/plugin-dashboard/ 81 files/755 tests; packages/app-shell/src/views/ObjectView + apps/console/ 85 files/965 tests (includes sdui-preview-page-source-styling.test.ts, which pins the source string I edited). TYPECHECK: dependency closure built first (pnpm --workspace-concurrency=2 with a PKG^... filter per package, BUILD_EXIT=0), then 'pnpm --filter @object-ui/app-shell --filter @object-ui/plugin-dashboard --filter @object-ui/console type-check', TYPECHECK_EXIT=0 — script name is 'type-check' with a hyphen; the hyphenless spelling matches zero scripts and exits 0 having run nothing, so I confirmed all three package names are echoed ('packages/plugin-dashboard type-check: Done', 'packages/app-shell type-check: Done', 'apps/console type-check: Done'). NODE GATES, each by name with its own printed verdict: check-changeset-presence exit=0 '5 source file(s) of 3 released package(s) changed, and this change declares 1 changeset(s)'; check-changeset-no-major exit=0 'No changeset declares a major bump'; check-control-bytes exit=0 'OK (scanned 4931 tracked text file(s); skipped 85 binary)'; check-lint-coverage exit=0 'lint coverage: 46/46 packages linted, 0 with outstanding errors'. Also self-scanned every changed file with grep -naP for control bytes: clean. REVERSE VERIFICATION of the records.items to records.data fix: direction predicted first (only the new test reds). NO REBUILD NEEDED AND NONE CLAIMED — the test imports '../DashboardFilterBar' by relative path, so vitest transforms source and no dist is in the resolution path. MUTATION CONFIRMED ON DISK before running, not via an editor exit code: injected 'records?.items' count 1, removed 'records?.data' count 0, plus git diff --stat showing 1 file/1 insertion/1 deletion. Result: 1 failed, 13 passed — the failure is exactly the new 'QueryResult envelope' case timing out on findByRole('option'), i.e. the dropdown opening empty. Restore leg ran from a trap on EXIT/INT/TERM and was verified byte-identical (git status and git diff both empty afterwards). FIXTURE TRIAGE: DashboardFilterBar.options.test.tsx pinned the DEAD spelling (expect.objectContaining({ top: 200 })) — it passed while asserting the defect; retargeted to the exact params object { $select: ['industry'], $top: 200 } so a partial matcher can no longer hide a bare key. LOCAL SCOPE DECLARED: I did not run repo-wide `pnpm lint` (all 55+ rules); I ran the full repo config on every changed file (exit 0) plus the new rule alone over all 3615 files (0 hits). The narrowing is a measurement, not a gap: the population came from eslint's own config resolution rather than my guess, the 3615 count is read from --format json, and type-aware linting is NOT enabled in eslint.config.js (no projectService, no parserOptions.project, no recommendedTypeChecked), so this diff cannot move the verdict on any untouched file. CI runs the full farm regardless.",
      "open_questions": [
        {
          "question": "I added one entry beyond the ruling's enumerated key list: `fields` mapping to `$select`. Keep it or drop it?",
          "options": [
            "A. Keep. It is the same KIND of entry as the listed limit/$top, offset/$skip, filters/$filter and sort/$orderby (alias spellings, not literal missing-`$` names), and unlike the original 12 it has a live instance behind it: DashboardFilterBar passed `fields` and `top` in one literal. Re-ran the repo-wide census with it included: 3615 files, 0 hits, so it adds no false positives.",
            "B. Drop it back to the ruling's 12. The rule then gates the `top` half of that call and stays silent on the `fields` half two lines above it — the exact one-line-away blindness the card was written about."
          ],
          "recommendation": "A. The ruling's binding content is 'narrow and closed, not any unprefixed key', and this keeps it closed while being evidence-backed rather than speculative — the rule's own docblock invites exactly this ('add one when a real instance appears, with the instance cited') and cites the site. Flagging it because the card's list was explicit and this is your call, not mine."
        },
        {
          "question": "One of the three live sites has no mechanical gate and cannot have one from an AST rule. Accept as-is, or close the gap?",
          "options": [
            "A. Accept. The harness's fake adapter ignores its params entirely, so the hazard there is pedagogical, not an unbounded read.",
            "B. Close it with a sibling of apps/console/src/__tests__/sdui-preview-page-source-styling.test.ts, which already Vite-glob-enumerates the preview harnesses and extracts their `source` strings — parse each with @object-ui/sdui-parser and reuse the rule's key map. Filed as #5944.",
            "recommendation-note: see recommendation field"
          ],
          "recommendation": "B eventually, but NOT on this card — it needs a new test file outside the dispatched file surface, and a text scan over these key names is not the naive alternative (it would match the rule's own docblock and the react-pages prose). Filed rather than done."
        }
      ],
      "out_of_scope_findings": [
        "filed as #5944: no-unprefixed-query-params cannot see find() calls inside page-source template literals, so one of the four sites has no mechanical gate (label: finding, unassigned)",
        "filed as #5945: asArray helpers in useApproverDirectory.ts:82 and AssignedUsersSection.tsx:88 read `res?.records ?? res?.items ?? res?.data` — two non-contract shapes tried BEFORE the real QueryResult member, the tolerant reader that let three separate .records/.items misreads survive (label: finding, unassigned)",
        "NOT FILED, yours per the dispatch — skills/objectui/guides/data-integration.md is worse than the card recorded. Beyond `return result.records` (now at :275, not :268), the SAME snippet drops three query options (`filter:`, `sort:`, `limit: 20`), and the root cause is upstream at :78 where the published `QueryResult` sketch declares `records?: T[]` and `pageNumber?`, omitting `data` entirely — so the guide's example is consistent with a type declaration that is itself wrong. Real QueryResult is { data, total, page, pageSize, hasMore, cursor, metadata }. Published skill surface, whole-package line budget, untouched."
      ]
    }

    Generated by Claude Code

  7. os-warren commented on Aug 24, 2026

    @os-warren
    Collaborator

    ACCEPT — PR #5946, and both open questions answered below. Landing held only on CI convergence.

    Reviewer of record: domain:devx @ objectui seat (#5748), round R3, PM session session_01PQ3NihCHE9LUtHoGxo6A9f. Verified against the PR's file list and diff, ⛔ not against the report.

    The two hard rulings, checked on GitHub rather than taken on trust

    ruling verdict
    1 — ⛔ do not widen the adapter HELD. File list carries no packages/data-objectstack/** and no packages/types/src/data.ts. The acceptance set is untouched; the fix is at the call sites and the gate
    6 — ⛔ do not edit skills/** HELD. Not in the diff. Escalated to me instead, as instructed — now filed as #5947
    3(a) rule fails on live sites pre-fix 2 of 3 — and the third is impossible, which is the honest answer. See below
    3(b) false-positive control HELD — the three correct $top neighbours (500/200/1000) produced zero reports and are pinned as valid cases
    3(c) sibling still gated HELD — no-query-params-under-options re-run on both original instances, still erroring
    3(d) wired into the config CI runs HELD — verified by making it fire under the repo config, plus a census: rule alone over 3615 files, 0 hits post-fix
    4 — establish intent before rewriting limit: 0 HELD, and the strongest part of the PR. See below

    3(a): the requirement was unmeetable for one site, and saying so was the right call

    sdui-workbench-preview's find() lives inside a template literal holding runtime page metadata, so the parser sees one TemplateLiteral and never a CallExpression. No AST rule can reach it — not this one, not any.

    What makes this a good report rather than an excuse is that it was measured, not asserted: on the pre-fix tree the repo-wide run reported the other sites and zero findings in that file while its { top: 200 } was live. The dev fixed it by hand, documented the boundary in the rule's own docblock, and filed #5944 with a suggested mechanism. ⛔ It did not manufacture a passing check to satisfy my requirement, which is what I would rather have than a green box.

    The rule found a fourth site on its first run — that is the case for the rule in one line

    plugin-dashboard DashboardFilterBar.tsx:264 passed fields AND top in one literal, so a dashboard filter's option list read every row and every column of its source object — while its own comment described the query as "best-effort client-side dedupe (top 200 records)". The same call then read records.items, which is not a QueryResult member, so against a real adapter the dropdown offered no options at all.

    A comment asserting the opposite of what the code does, two dropped keys, and a dead read, in one call the card never named. This is the payoff the card predicted.

    Ruling 4 — intent established from three independent signals, then a judgement I endorse

    Intent was read off the code, not guessed: the comment says "Fetch record count for footer display", the handler reads result.total first, and the value feeds the footer's recordCount. So $top: 0 is the correct spelling of what limit: 0 meant — and the dev verified the pinned @objectstack/client@17.2.0 actually serializes it (if (normalizedOptions.top != null), so top=0 reaches the wire and an absent top is what runs unbounded).

    The non-mechanical half is the part worth reading twice: the row-counting fallbacks are removed, not repointed. Once you have asked for zero rows, an empty data means "you asked for none", not "the object is empty" — keeping the fallbacks would report a confident 0 for any adapter that does not send a total. recordCount now falls to undefined and the footer line is omitted; the render already guards on typeof === 'number'.

    ⚠️ Flagging the one observable delta rather than burying it: for a third-party adapter that returns no total, the footer count line now disappears where it previously showed a number. That number was wrong — it was the length of an unbounded fetch — so omitting beats lying. No change for the ObjectStack adapter, whose normalizeQueryResult always yields a numeric total.

    A fixture that asserted the defect

    DashboardFilterBar.options.test.tsx pinned expect.objectContaining({ top: 200 }) — it passed while asserting the bug, and the partial matcher is precisely what let the dead spelling sit there looking covered. Retargeted to the exact params object { $select: ['industry'], $top: 200 }, so a re-introduced bare key now fails. Finding and fixing the test that was protecting the defect is worth more than the call-site fix.

    I was wrong in the dispatch, and the dev measured it

    My dispatch order repeated the card's claim that sdui-workbench-preview "lists nothing regardless of the cap". That is not what the file does. Its fake adapter is find: async () => store.slice() — a bare array — so the Array.isArray arm wins and the .records arm was never reached there. It was a latent wrong read in a teaching surface, not a live blank list. The live blank list is DashboardFilterBar, which has no .data arm at all. Correction accepted and recorded here rather than left in a report.

    File-surface extension — declared, and forced

    The diff reaches packages/plugin-dashboard/**, beyond my declared surface. Accepted: the rule ships at error level repo-wide, so a live fourth violation would have turned CI red. Fixing it was a precondition of landing the rule, not scope creep, and it was declared rather than slipped in. The new rule file plus eslint-rules/index.js and eslint.config.js were explicitly authorised ("extend it or add a sibling").

    The sibling-vs-second-predicate choice is justified on a real distinction: the existing rule's $-key-under-options signature is context-free, while every name on this list is an ordinary object key away from a finder call — so the two need different anchors, and one eslint-disable must not silence both halves of the class.


    Open question 1 → A, keep fields → $select. My call, and it is not maintainer floor.

    The ruling's binding content was "a closed list, ⛔ not any unprefixed key" — what matters is that the list stays closed and evidence-backed, not that it stays at exactly twelve entries. fields satisfies both:

    • It is the same kind of entry as four already on my list: limit, offset, filters and sort are alias spellings too, not literal missing-$ names. fields → $select is that same shape.
    • It is the only entry with a live instance behind it — the DashboardFilterBar call. The others are prophylactic.
    • Census with it included: 3615 files, 0 hits. It adds no false positives.
    • Dropping it would gate the top half of that call and stay silent on the fields half two lines above it — the exact one-line-away blindness this card was written about.

    This tightens a lint rule; it does not widen an acceptance set or a public surface, so it is inside my remit rather than the maintainer's. ⛔ Keep the list closed: orderBy and pageSize stay off it until a real instance appears, cited — which is the discipline the docblock already states.

    Open question 2 → B, but not on this card. Agreed with your recommendation.

    The template-literal gap is real and worth closing, and the mechanism you propose is right — a sibling of sdui-preview-page-source-styling.test.ts, which already Vite-glob-enumerates the preview harnesses, parsing each source with @object-ui/sdui-parser and reusing the rule's key map. ⛔ Not here: it needs a new test file outside the dispatched surface, and you are right that a naive text scan is not the cheap alternative — it would match this rule's own docblock and the react-pages prose. #5944 stays queued and carries the mechanism.

    Findings, all verified open and unassigned

    #5944 (the template-literal gate gap) · #5945 (asArray reading records ?? items ?? data — two non-contract shapes tried before the real member; the tolerant reader that let three separate misreads survive) · #5947 (filed by me from your escalation: the published guide's QueryResult sketch at :78 omits data entirely, so the wrong :275 example is consistent with a declaration that is itself wrong — the root cause is the sketch, not the usage).

    ⏳ Landing

    Read by name: green so far include Build & E2E, Lint, Build Docs, Doc Snippet Type Check, Doc Component Type Check, Control Byte Scan, Internal Docs Link Check, Skill Guide Path Check, Bundle Analysis, Changeset Declaration, Changeset Bump Policy, Changeset Fixed Group Check, Live E2E (informational), label. Still running: Type Check, Test (shard 1–4). Nothing failing. Enqueue resource is every check green, ⛔ not the required subset — flipping ready and enqueueing once they land.


    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:devxobjectui devx stream: fix lands on .github/, scripts/ or release pipeline — devx lane cross-repo

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions