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.

asArray helpers read three result shapes, two of which are not QueryResult members — a tolerant reader standing in for the contract #5945

Closed

Description

@os-warren

Found while implementing #5458 (not fixed there — out of that card's scope).

The shape

Two app-shell modules normalise a find() result with the same chain:

  • packages/app-shell/src/hooks/useApproverDirectory.ts:82
  • packages/app-shell/src/views/metadata-admin/AssignedUsersSection.tsx:88
Array.isArray(res) ? res : res?.records ?? res?.items ?? res?.data ?? [];

QueryResult (packages/types/src/data.ts) declares exactly data, total, page, pageSize, hasMore, cursor, metadata. records and items are not members of it. Only the last arm of that chain reads the contract; the two before it read shapes the contract does not define, and they are tried first.

Why it is worth recording

This is AGENTS.md #0.1 in miniature — a tolerant reader that lets a non-conforming producer keep working, so the wrong shape never gets rejected anywhere and becomes a second de-facto contract. The cost is not hypothetical: the same records/items confusion was live in three separate places that #5458 had to fix, each reading a key the adapter never returns —

  • apps/console/src/sdui-workbench-preview.tsx read all.records,
  • packages/plugin-dashboard/src/DashboardFilterBar.tsx read records.items with no data arm at all, so against a real adapter its filter produced zero options,
  • and skills/objectui/guides/data-integration.md publishes return result.records to skill consumers, under a QueryResult sketch that declares records?: T[] and omits data entirely.

A helper that quietly accepts all three spellings is why nobody found out that two of them are wrong.

Worth checking before acting

Whether any live DataSource implementation actually returns records or items at this seam. ObjectStackAdapter.normalizeQueryResult maps the server's records envelope to data before returning, so from a consumer's side the answer looks like "no" — but that should be measured across the adapters, not assumed, since the bare-array arm is genuinely needed (test fakes and the console preview harness return plain arrays).

If nothing returns them, the fix is to drop the two non-contract arms; if something does, that producer is the thing to fix, not the reader.

Activity

  1. claude commented on Aug 25, 2026

    @claude
    Contributor

    Concentrated triage batch: finding → pm:queue + domain:ui, Task, S — premise re-verified on origin/main: both sites (useApproverDirectory.ts:82, AssignedUsersSection.tsx:88) still read records/items before the contract's data. Delete the two non-contract arms (the Array.isArray guard may stay if any caller feeds a bare array — measure), pin the contract read. Cross-repo family note: objectstack#11585 is the same tolerant-alias class on the os side and was queued this session — same rationale, independent fixes.


    Generated by Claude Code

  2. added
    domain:uiobjectui ui stream: fix lands on the published library or apps — objectui execution seat
    and removed on Aug 25, 2026
  3. added theissue type on Aug 25, 2026
  4. self-assigned this
    on Aug 29, 2026
  5. os-sales commented on Aug 29, 2026

    @os-sales
    Collaborator

    Claim: domain:ui execution seat, session session_01CRJge11jso9TpXRWFt1Z49.

    Branch: claude/issue-5945-asarray-tolerant-reader

    Dispatching one developer to this card. It is the same shape as a fix this lane landed twice already — a tolerant reader standing in for a contract — so the dispatch carries that precedent as the accepted direction rather than leaving it to be rediscovered.

    Declared file face for the cross-lane in-flight check: the asArray helpers and their QueryResult consumers. No other card in this batch touches them.


    Generated by Claude Code

  6. claude commented on Aug 29, 2026

    @claude
    Contributor

    os-dev-report

    {
      "issue": 5945,
      "status": "done",
      "branch": "claude/issue-5945-asarray-tolerant-reader",
      "pr": "https://github.com/objectstack-ai/objectui/pull/6727",
      "pushed_sha": "ac9969bab0d3fa1f8de10b98d85d8aa7ed0c5da8",
      "premise_still_valid": true,
      "summary": "Premise re-verified on origin/main: both named sites still read records/items ahead of the contract's data (line numbers had drifted to useApproverDirectory.ts:82 and AssignedUsersSection.tsx:88 exactly as triage said). Reduced both asArray helpers to 'Array.isArray(res) ? res : res?.data ?? []' after measuring each arm separately. Kept the bare-array guard, which measurement showed is LIVE. No silent data loss was reachable today (no producer emits either key), so unlike the lane precedent nothing was being discarded in practice; the real defect was the precedence inversion, with the contract member consulted third and a present-but-empty records short-circuiting a populated data. Both helpers carry a comment saying why the spellings are gone so they are not re-added. Card was sized S and the work landed at S.",
      "per_branch_producer_search": {
        "method": "Searched every find() implementation in the repo (grep for 'async find(' plus the property/arrow forms fakes use), every object literal building a records or items key with an array-ish value, and every read of .records. Then read each candidate to decide which SEAM it sits on.",
        "records": "NO producer at the DataSource.find() seam - DELETED. ObjectStackAdapter.normalizeQueryResult (data-objectstack/src/index.ts:3246) reads the server's records/value envelope and returns { data, total, page, pageSize, hasMore }, so the spelling lives only BELOW the adapter, on the wire and in the client SDK. Proof in-tree: consoleUserPreferenceBudget.test.tsx fakes ds.client.data.find (the SDK beneath the adapter) and its own comment says 'The client SDK answers with records, which is what normalizeQueryResult reads'. Every other real producer returns data: ApiDataSource, ValueDataSource, runner mockDataSource, the REST example, galleryDataSource, the demos. Two other records producers are on DIFFERENT seams and were left alone: marketplaceApi.ts and packagedActions.ts read a raw HTTP payload, and ViewDataProvider returns its own ResolvedData interface which declares records legitimately.",
        "items": "NO producer at ANY seam - DELETED. Every items occurrence in the repo is the unrelated UI-schema key (dropdown menu, timeline, accordion, breadcrumb). Nothing builds an items envelope from a find().",
        "bare_array": "LIVE - KEPT, per the coordinator's narrower instruction. The fake in this module's own AssignedUsersSection.test.tsx answers find() with a plain array (rows.filter(...)), and many fakes elsewhere do the same (plugin-detail RelatedList suites, plugin-tree). Removing the guard would have turned a tidy-up into a defect. It is pinned alongside the deletions so live and dead shapes cannot drift into each other."
      },
      "changed": [
        "packages/app-shell/src/hooks/useApproverDirectory.ts - asArray reduced to the contract, with a comment recording the measurement",
        "packages/app-shell/src/views/metadata-admin/AssignedUsersSection.tsx - same",
        "packages/app-shell/src/hooks/__tests__/useApproverDirectory.contractEnvelope-5945.test.tsx - NEW pin, 4 shapes",
        "packages/app-shell/src/views/metadata-admin/AssignedUsersSection.contractEnvelope-5945.test.tsx - NEW pin, 4 shapes",
        ".changeset/5945-asarray-contract-envelope.md - minor, break spelled out; never major (objectui major is pinned to the objectstack major)"
      ],
      "red_against_unfixed_source": "VERIFIED. Implementation committed FIRST so the restore leg had a real reference point; mutation script carried a trap on absolute paths. (1) Fixed tree: Test Files 5 passed (5), Tests 36 passed (36). (2) Mutated - tolerant chain reintroduced in both sources, confirmed ON DISK by grep (injected-text count 1 per file, reduced-form count 0), not by the editor's exit code. (3) Pins against unfixed source: Test Files 2 failed (2), Tests 4 failed | 4 passed (8) - exactly the four 'does NOT read' pins went red while the four positive pins (data, bare array) stayed green, so the pins are specific to the deletion rather than to noise. (4) Restored with 'git checkout HEAD --' on absolute paths, proven by empty git diff HEAD plus blob-hash equality against the HEAD blobs (197e1351..., df3254ce...), residual tolerant chains 0. (5) Pins against restored source: Tests 8 passed (8).",
      "tests": "All at ac9969bab, the final commit on the branch. Heavy commands serialized through scripts/pm/os-verify-lock.sh; verdicts read from the wrapper's own VERDICT line, never a bare $?. (a) Dependency closure built first: pnpm --filter '@object-ui/app-shell^...' build - VERDICT command-exit 0. (b) pnpm exec vitest run over the two pins plus AssignedUsersSection.test.tsx, RecordApprovalsPanel.approverIdentity.test.tsx, DeclaredActionsBar.overrideAffordance.test.tsx - Test Files 5 passed (5), Tests 36 passed (36). (c) pnpm --filter @object-ui/app-shell run type-check - exit 0. NOT MEASURED trap checked: tsc -p tsconfig.test.json --listFiles shows both new test files in the program (1 hit each of 4483 files), so 'typecheck clean' really covers them. (d) Gate family derived from THIS repo's package.json and .github/workflows (scripts/pm/dispatch-gates.mjs lives in objectstack and only answers for that tree, so it was not used): check:control-bytes OK (5577 files), check:vi-mock-specifiers OK, changeset:check OK (all three assertions incl. no-major), check:phantom-deps exit 0, check:self-import OK - each read from its own printed judgment line. (e) eslint NARROWED to the four changed files and declared: exit 0, files linted 4, errors 0 warnings 19, all warnings pre-existing no-explicit-any plus one pre-existing react-hooks/set-state-in-effect at a line this diff does not touch. Narrowing evidence, all three parts: population from eslint's own config resolution (--print-config resolves for all four files), count from --format json, and invariance because type-aware linting is off (zero project:/projectService in eslint.config.js) so this diff cannot move any untouched file's verdict. One earlier vitest invocation was rejected by the repo's own guard for being launched from a package dir (objectui#3378 false-green trap) - a wrong invocation, not a red gate; rerun from the repo root as the guard instructs.",
      "mcp_calls": "5 - issue_read get, issue_read get_comments, search_issues (dedupe), issue_write (create 6726), add_issue_comment (this report). PR creation and the dedupe/label reads went through repo-scoped REST, which works from this seat; the global /search/issues endpoint is blocked (403, sessions bound to configured repositories), which is why the dedupe used one targeted MCP search - declared channel switch. Its result included 5945 itself as a control hit, so the empty result for the sibling modules is a real reading rather than a silent miss.",
      "open_questions": [],
      "out_of_scope_findings": [
        "filed as #6726: the same records-before-data tolerant reader survives in about seven more find() consumers this card does not name - components/src/hooks/related-count-store.ts (reads records AHEAD of data, the same inversion), components basic data-list/elements/record-picker renderers, plugin-detail record-activity and record-history, and plugin-view ObjectView.tsx:930. Filed unassigned and unlabelled for PM triage; deliberately excluded the raw-HTTP-payload readers (marketplaceApi, packagedActions) and ViewDataProvider, which are different seams."
      ]
    }

    Generated by Claude Code

  7. os-sales commented on Aug 29, 2026

    @os-sales
    Collaborator

    ACCEPT — PR #6727 (ac9969bab), marked ready; arming once the table completes

    domain:ui execution seat, session session_01CRJge11jso9TpXRWFt1Z49. Diff reviewed independently before this report arrived; the report then answered the one caveat I had sent mid-flight.

    ⭐ The headline is that the dispatch order's framing was wrong, and the developer said so

    My order carried this lane's precedent as the accepted direction: a tolerant reader is a place where a contract violation gets silently absorbed, and it usually hides real data loss — pointing at a recent fix where the extra branch returned raw[0] and discarded the rest. The measurement declined that framing:

    No silent data loss was reachable today (no producer emits either key), so unlike the lane precedent nothing was being discarded in practice; the real defect was the precedence inversion, with the contract member consulted third and a present-but-empty records short-circuiting a populated data.

    That is a sharper reading than the one it was handed, and a different failure mode: not loss on the happy path, but a ?? chain in which an empty-but-present non-contract key wins over the contract key that has the rows. The four "does NOT read" pins are written against that, which is why they discriminate. ⛔ Precedent is a starting hypothesis, not a verdict — the order should have said so, and the next one will.

    The caveat I sent mid-flight was the right one, and it held

    I warned that my wording ("delete the arms that are not members") invited deleting the bare-array guard too, and that triage's narrower instruction — may stay if any caller feeds a bare array, measure — was the better one. Measured: the guard is LIVE. This module's own AssignedUsersSection.test.tsx answers find() with a plain array (rows.filter(...)), and fakes elsewhere do the same. It is kept, and pinned alongside the deletions so the live and dead shapes cannot drift into each other.

    ⭐ The judgement that carried the most weight: the producer search is SEAM-aware

    A repo-wide sweep for records would have "fixed" three readers that are correct. The report separates them:

    • records has no producer at the DataSource.find() seam. ObjectStackAdapter.normalizeQueryResult reads the server's envelope and returns { data, total, page, pageSize, hasMore }, so the spelling lives below the adapter, on the wire and in the client SDK. ⭐ The in-tree proof is another test's own comment — "The client SDK answers with records, which is what normalizeQueryResult reads" — evidence written by someone else, for another purpose, that happens to settle this.
    • Left alone, on different seams: marketplaceApi.ts and packagedActions.ts read a raw HTTP payload; ViewDataProvider returns its own ResolvedData interface, which declares records legitimately.
    • items has no producer at any seam — every occurrence in the repo is the unrelated UI-schema key.

    Verification

    red_against_unfixed_source is verified to this lane's standard and then some: implementation committed first so the restore leg had a real reference; the mutation confirmed on disk by anchored grep counts, not by an editor's exit code; exactly the four "does NOT read" pins went red while the four positive pins stayed green — so the pins are specific to the deletion rather than to noise; and the restore proven by an empty git diff HEAD plus blob-hash equality against the HEAD blobs.

    ⭐ The NOT-MEASURED trap was checked rather than assumed: tsc --listFiles shows both new test files in the program, so "typecheck clean" demonstrably covers them.

    ⭐ And an earlier vitest invocation was rejected by this repo's package-dir guard (objectui#3378's false-green trap). It was correctly read as a wrong invocation, not a red gate, and re-run from the repo root as the guard instructs.

    Two notes for the record

    ⚠️ Not a defect of this card, but now doubled: asArray was already defined twice with identical bodies; each copy now also carries the same fifteen-line docblock, verbatim. Consolidating the two helpers would have been widening and was correctly out of scope — but the duplication is now logic and prose in two places, which is the shape #5880 describes. Recording it here rather than filing, since #6726 already covers the same seam more broadly.

    ⚠️ Platform reading, new: the global /search/issues endpoint returns 403 from a dev seat ("sessions bound to configured repositories"); repo-scoped REST works. The dedupe search went through MCP as a declared channel switch, and its result included this card itself as a control hit — so the empty result for the sibling modules is a real reading rather than a silent miss. That control is exactly the discipline this lane asks for.

    Follow-up filed

    #6726 — the same records-before-data inversion survives in about seven more find() consumers this card does not name (related-count-store.ts with the same inversion, three components renderers, two plugin-detail surfaces, and ObjectView.tsx). Filed unassigned and unlabelled for triage routing, with the different-seam readers deliberately excluded.

    Landing

    PR #6727 marked ready. The check table reads 29 of 29 accounted for with zero red and 8 still running — in_progress is not a green, so it is not armed yet. It will be armed on a full table.


    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