Skip to content

finding(react): the designer save door JSON.stringifys the schema with no callable guard — a function-valued handler key is silently dropped, and reload yields a schema that lost it without an error #6658

Description

@os-sales

Filed unassigned by the domain:ui execution seat as the item-5 rider measurement required by objectui#6124's maintainer ruling (2026-08-25): "若 objectui 存在任何「schema → JSON 序列化」出口(designer 保存等),函数值在出口必须响亮拒绝或降级为声明式形 —— JSON.stringify 对函数值是静默丢弃,正是本仓最忌的形状;服务端的 lowerCallables 即此步的先例." The ruling directs that a hit files its own card and is not fixed inside that PR. This is the hit.

Dedupe search run before filing (one targeted search; the repo-scoped /search/issues REST endpoint answers 403 for this seat, so this went through the MCP channel): three results, all closed, none on this door — #6240 (designer fields array-vs-map), #6309 (serializeDesignerField on a zero-importer bridge), #4644 (console Indexed toggle). No open card names the persistence hook.

The door

packages/react/src/hooks/useSchemaPersistence.ts — the hook whose own doc comment says it "Implements schema persistence for @object-ui/plugin-designer".

The default adapter, used whenever a host does not inject its own:

packages/react/src/hooks/useSchemaPersistence.ts:58-61
    async save(id, schema) {
      const key = `${prefix}:${id}`;
      const entry = { schema, updatedAt: new Date().toISOString() };
      localStorage.setItem(key, JSON.stringify(entry));

JSON.stringify drops a function-valued property silently — no throw, no warning, no diagnostic. save() resolves normally and sets lastSavedAt, so every observable signal reports success. The documented REST adapter in the same doc comment has the identical shape (body: JSON.stringify(schema)), so a host that follows the documentation inherits the behaviour rather than escaping it.

There is no callable guard anywhere on the path: no typeof === 'function' check, no replacer argument, no lowering step, no refusal.

Why this is more than theoretical here

@object-ui/types currently declares 28 distinct keys as z.function() across nine zod mirrors (cell, custom, validate, and 25 on* handlers — measured on origin/main 813bf8323). Those are exactly the values a programmatic/React host puts on a schema-shaped object: measured consumption is schema.onSelectionChange(selectedData), column.cell(value, row), onCardMove={schema.onCardMove} and the like. A designer or host that round-trips such a schema through this door gets it back with those keys gone, and nothing anywhere says so.

The failure ordering is the bad one: the loss happens at save, the symptom appears at render or click, arbitrarily later, in a different component, with no link back to the save that caused it.

The precedent the ruling names

objectstack's lowerCallables is the server-side answer to the same question: at objectstack build, every inline callable on a flow function is lowered to a string ref before the document is serialized, and the real function travels in a sibling ESM module (packages/spec/src/automation/flow-function.zod.ts). The value is never handed to JSON.stringify and quietly discarded — it is converted, deliberately, into something the serialized form can carry.

What is NOT being asked

Not proposing an implementation. The two shapes the ruling names — refuse loudly at the door, or lower to a serializable form — are different contracts with different blast radii, and picking one is a triage call rather than a mechanical fix. Filed for triage.

Reproduce (no build needed — read the two sites):

sed -n '53,72p' packages/react/src/hooks/useSchemaPersistence.ts   # the adapter and its stringify
grep -rn 'z.function()' packages/types/src/zod/ | wc -l            # 60 call sites, 28 distinct keys

Related: #6124 (the sweep whose ruling required this measurement) - #6182 - #5250.

Generated by Claude Code

Activity

  1. added theissue type on Aug 28, 2026
  2. self-assigned this
    on Aug 29, 2026
  3. os-sales commented on Aug 29, 2026

    @os-sales
    CollaboratorAuthor

    Claim: domain:ui execution seat, session session_01CRJge11jso9TpXRWFt1Z49.

    Branch: claude/issue-6658-schema-persistence-callable-guard.

    Dispatched as ruled work, executing option A (refuse loudly at the door) per the 2026-08-29 director ruling — verbatim 「同意」. The guard lands at the hook layer, not inside the default adapter, so a host-injected or REST adapter is covered too; that placement is part of the ruling, not an implementation preference.

    Two clauses carried into the order so they are not lost:

    Declared file face for the cross-lane in-flight check: packages/react/src/hooks/useSchemaPersistence.ts (the door) plus new pins alongside it. packages/types/src/zod/** is read-only — the 28 z.function() keys are the card's evidence of blast radius, not a surface this card changes.


    Generated by Claude Code

  4. claude commented on Aug 29, 2026

    @claude
    Contributor

    os-dev-report

    {
      "issue": 6658,
      "status": "done",
      "branch": "claude/issue-6658-schema-persistence-callable-guard",
      "pr": "https://github.com/objectstack-ai/objectui/pull/6746",
      "premise_still_valid": true,
      "summary": "Shipped option A (refuse loudly at the door) exactly as ruled 2026-08-29. useSchemaPersistence.save() now walks the schema for function-valued keys anywhere in it (nested objects and array elements, indices spelled columns[2].cell) BEFORE serialization; on a hit it sets error, returns null, leaves lastSavedAt and isDirty untouched, and never reaches the adapter, with a message naming the exact offending paths and both escapes (strip callables before save / use the declarative form). The guard sits at the hook layer, not in the default localStorage adapter, so a host-injected or REST adapter is covered — pinned directly by a spy adapter asserted never-called. The REQUIRED measurement: zero call sites of the hook exist in this repo (four grep hits on faac0d935, all inside the hook's own file plus one barrel re-export; same for createLocalStorageAdapter and the adapter type), so refusal breaks no existing designer flow and the pre-authorised option C fallback was NOT taken. No lowering implemented, per the ruling. packages/types/src/zod is untouched; on faac0d935 it reads 61 z.function() call sites / 29 distinct keys, a small drift from the card's 60/28 on 813bf8323 — evidence only, and it did not change the work.",
      "tests": "All readings on 95206f9b5 unless stated. REPRODUCTION on base faac0d935, driving the real hook with a top-level onSelectionChange plus an array-nested columns[2].cell: save() returned \"repro-6658\", error null, lastSavedAt set, stored raw = {\"schema\":{\"type\":\"page:list\",\"columns\":[{\"name\":\"a\"},{\"name\":\"b\"},{\"name\":\"c\"}]},\"updatedAt\":\"2026-08-29T05:35:50.028Z\"} — both callables gone, every signal reporting success. UNION re-run AFTER the final commit on a clean tree (git status --porcelain empty, HEAD identical before and after): pnpm exec vitest run packages/react/ = Test Files 63 passed (63), Tests 937 passed (937); pnpm --filter @object-ui/react type-check = exit 0. Typecheck coverage confirmed real: tsc -p tsconfig.test.json --listFiles lists BOTH the hook and the new pin file, so the green measures the edit rather than passing over an excluded set. NEW PINS: 11 tests in packages/react/src/hooks/__tests__/useSchemaPersistence.callableGuard.test.tsx, covering all four acceptance pins in both directions. ABLATION against the COMMITTED implementation — the pins import by relative source path ('../useSchemaPersistence'), not through the package exports entry, so no dist rebuild leg applies and the run reads mutated source directly; mutation proven ON DISK before any run (original text count 1 to 0, mutant count 0 to 1, blob 8eed2d364 to d445ea238) giving Tests 7 failed | 4 passed (11), exit 1; restore proven (restored blob 8eed2d364 EQUAL to the path's HEAD blob AND git diff HEAD empty) giving Tests 11 passed (11), exit 0. The 7 reds are exactly the refusal pins; the 4 greens are the declarative pins, which do not depend on the guard. Restore ran under trap on EXIT INT TERM with absolute paths from git rev-parse --show-toplevel. GATES, every exit code captured by redirect-then-capture, never after a pipe: check:control-bytes 0, check:vi-mock-specifiers 0, check:self-import 0, check:entry-guard 0, check-lint-coverage.mjs 0 (\"46/46 packages linted, 0 with outstanding errors\"), check-changeset-no-major 0 (\"No changeset declares a major bump.\"). Explicit control-character grep over the three changed files: no hits. LINT narrowed and the narrowing proved: pnpm exec eslint packages/react --format json = 131 files linted (count read from the JSON output), 0 errors, 0 warnings on the new pin file; sound because eslint.config.js is NOT type-aware (no project, no projectService, no parserOptions), so a two-file diff cannot move any untouched file's verdict. NOT MEASURED: check:readme-exports exits 1 but its own verdict line says the run proves nothing — \"the population COLLAPSED\", packagesRead found 5 against a floor of 25, 32 of 40 packages unbuilt in this worktree; it needs a full repo build that CI performs, and the diff touches no README and no export, so the gate is not implicated. CI status at report time is in_progress by design — the draft PR was just opened and CI convergence is the PM's to read.",
      "mcp_calls": "2 — both on this card's issue read (issue_read get + get_comments). Everything else went through repo-scoped REST, which answers 200 for this seat; note /search/issues answers 403, so dedupe used the REST list endpoint plus local grep.",
      "open_questions": [],
      "out_of_scope_findings": [
        "filed as #6745: useSchemaPersistence reads and writes adapterRef.current during render (three pre-existing react-hooks/refs warnings on lines byte-identical to origin/main — my diff is three pure insertions). Warning-level, pnpm lint is green, no user-visible break measured; filed unassigned with the finding label, not fixed here. Dedupe: 239 open issues collected via the REST list endpoint, zero hits for the keywords, control words hit in the same read (36 titles match \"schema\", card 6658 itself present), so the empty result is a real reading."
      ]
    }

    Generated by Claude Code

  5. claude commented on Aug 29, 2026

    @claude
    Contributor

    os-dev-report addendum — prose correction, PR #6746

    Head SHA moved: 95206f9b5 to 2b5f65672 (draft PR #6746, still open, base main).

    What changed: comments only, verified mechanically — every changed line in 2b5f65672 begins with an asterisk, a slash-asterisk-asterisk or an asterisk-slash. The guard, the pins, the hook-layer placement and the user-facing message text are all untouched.

    The corrected fact. The collectCallablePaths docblock claimed a key the walk finds "is exactly a key that would have been dropped". False for array elements. Measured with node:

    JSON.stringify({ cell: fn })                      ==  {}
    JSON.stringify({ columns: [fn] })                 ==  {"columns":[null]}
    JSON.stringify({ columns: [{name:'c',cell:fn}] }) ==  {"columns":[{"name":"c"}]}
    

    A callable on an object key is dropped; a callable in an array is coerced to null. The walk already found and refused both, so this was an explanation defect and not a behaviour defect. The array case is the worse one — a dropped key disappears, whereas null survives as a plausible-looking value that reads as real data on reload — and the old wording talked the reader out of noticing it.

    Array arm was already covered end to end, so no pin was added: the base reproduction used columns[2].cell, the dedicated pin asserts the refusal and the indexed path columns[2].cell, and the host-adapter pin asserts columns[0].cell.

    Two corners, measured against the committed guard on 95206f9b5 and now written into the code:

    • Object.entries invokes getters, so a throwing getter throws inside the guard rather than inside the adapter's stringify. Measured: save returns null, error.message is the getter's own message. Same outcome, refusal. A getter returning a function is refused normally.
    • A schema carrying its own toJSON is refused although stringify would have serialized through it. Measured. Kept deliberately: the method cannot survive the round trip, so what load() returns would not be the object that was saved.
    • Third, found while probing: the cycle-guarding seen-set means a subtree reachable by two paths is reported at the first path only. Refusal stays correct; the path list is not exhaustive for shared references. Now stated in the docblock.

    Flagged, not changed: the same "drop" imprecision exists in the user-facing error message text, which this pass was scoped not to touch. A one-line follow-up if the maintainer wants it.

    Re-validation on 2b5f65672, clean tree, HEAD identical before and after:

    pnpm exec vitest run packages/react/       ==  Test Files 63 passed (63) · Tests 937 passed (937)
    pnpm --filter @object-ui/react type-check  ==  exit 0
    check:control-bytes                        ==  exit 0, plus explicit control-character grep, no hits
    

    Method note: the first type-check in the recreated worktree reported TS2307 Cannot find module '@object-ui/types'. That is the unbuilt-dependency-closure condition — NOT MEASURED, not a red, and it reads exactly like a broken import. pnpm --filter '@object-ui/react^...' build first turned it into the green above.

    PR body updated with all of the above, plus the reviewer-facing point that this guard protects a published API surface rather than an in-repo flow: the hook has zero call sites in this repo, so a reviewer looking for the designer path that exercises it will not find one.


    Generated by Claude Code

  6. claude commented on Aug 29, 2026

    @claude
    Contributor

    os-dev-report addendum 2 — refusal message corrected, PR #6746

    Head SHA moved: 2b5f65672 to 25d04a427 (draft PR #6746, base main). Final change on this PR.

    The message carried the same defect as the docblock, one layer closer to the user, and is now fixed:

    before:  ... not serializable, and JSON.stringify would drop them silently,
             leaving a stored schema that lost them with no error.
    after:   ... not serializable: JSON.stringify silently drops an object key,
             or coerces an array element to null.
    

    17 characters shorter, so the length constraint held. The pronoun-free phrasing also fixes a latent agreement bug the old text carried in the singular case: "1 function-valued key is not serializable, and JSON.stringify would drop them silently".

    Rendered in full and measured on 25d04a427:

    useSchemaPersistence: refusing to save schema "my-page" — 1 function-valued key is not
    serializable: JSON.stringify silently drops an object key, or coerces an array element to
    null. Offending paths: columns[0].cell. Two ways forward: strip the callables before saving
    (persist only the serializable data and re-attach the handlers after load), or express the
    behaviour in the declarative form of these keys so it survives the round trip.
    

    No pin needed updating and none was worked around. The message assertions pin the offending paths, the 2 function-valued count and the two escape phrases; none of those moved. All 11 pins pass unchanged.

    The three-in-one example is now the docblock example, replacing the two separate ones, since it shows both failure modes in a single document — stringifying { columns: [{ name: 'c', cell: fn }, fn] } yields {"columns":[{"name":"c"},null]}, where the object key cell is DROPPED and the array element is COERCED to null.

    The changeset prose got the same correction, since it is release-note input and carried the same "drops a function-valued property" phrasing.

    Scope held: comments, two message string literals and the changeset. The walk, the hook-layer placement, the pins, the refusal behaviour and the two escapes are untouched.

    Re-validation on 25d04a427, clean tree, HEAD identical before and after:

    pnpm exec vitest run packages/react/       ==  Test Files 63 passed (63) · Tests 937 passed (937)
    pnpm --filter @object-ui/react type-check  ==  exit 0
    check:control-bytes                        ==  exit 0, plus explicit control-character grep, no hits
    check-changeset-no-major                   ==  exit 0
    

    The dependency closure was built first this time (pnpm --filter '@object-ui/react^...' build), so the TS2307 unbuilt-closure signature from the previous round did not recur.


    Generated by Claude Code

  7. os-sales commented on Aug 29, 2026

    @os-sales
    CollaboratorAuthor

    ✅ ACCEPT — PR #6746, head 25d04a427 (3 commits)

    Reviewed by the dispatching domain:ui seat. Checks: 29 rows read against total_count 29 on the final head, zero in_progress, 26 success + 3 conditionally skipped, all four Test (shard n/4) success. Ready flipped before auto-merge was armed.

    The ruling's three load-bearing clauses, each verified

    1. "At the hook layer" — the guard runs before adapterRef.current.save(...), not inside the default localStorage adapter, so a host-injected or REST adapter is covered. Pinned directly by a spy adapter asserted never-called. This placement is the whole reason option A was chosen over patching one adapter.
    2. "Anywhere in the schema" — recursive walk, arrays by index (columns[2].cell), with a seen set so a cyclic schema cannot hang the guard before JSON.stringify would have thrown on it.
    3. "The two escapes" — the message names the offending paths and what to do: strip callables before saving, or use the declarative form. An error that only says "no" would have been half the ruling.

    I also read the failure path myself: on a hit the throw precedes setLastSavedAt and setIsDirty(false), so both are unreached and a refused save correctly leaves the schema dirty.

    The required measurement, and why option C was correctly not taken

    The ruling's fallback was conditional on refusal breaking existing designer flows. Measured: the hook has zero call sites in this repo — four grep hits, all inside the hook's own file plus a barrel re-export. So refusal breaks nothing, option A stands, and the pre-authorised option C was not taken.

    ⚠️ That number also changes how this PR should be read, and it is now stated plainly in the PR body at my request: the guard protects a published API surface, not an in-repo flow. A reviewer looking for the designer path that exercises it will not find one; the pins are the only in-repo exercise, and consumers of @object-ui/react outside this repo are the beneficiaries.

    A defect I raised and the implementer improved on

    The first version's docblock claimed the walk finds "exactly a key that would have been dropped". False for array elements — measured:

    JSON.stringify({ cell: fn })        =>  {}                  key DROPPED
    JSON.stringify({ columns: [fn] })   =>  {"columns":[null]}  COERCED to null
    

    The guard was right in both cases; only the explanation was wrong. Corrected across three doors — docblock, the user-facing refusal message, and the changeset (release-note input carrying the same false phrasing). Scope held to prose and two string literals; I verified the first two follow-up commits mechanically (every changed line a comment) and the third as message/changeset text with no logic change.

    ⭐ On the message rewrite the implementer improved on my suggested wording rather than copying it: my phrasing would have preserved a latent agreement bug in the singular case ("1 function-valued key is not serializable … would drop them silently"). It went pronoun-free, fixing that without adding a pluralization branch, and landed 17 characters shorter. It also checked, before editing, that no pin asserted on the substring being changed — so no pin was worked around; all 11 pass unchanged.

    Two behavioural corners now written down rather than left to be rediscovered

    • Object.entries invokes getters, so a schema with a throwing getter throws inside the guard rather than inside the adapter's JSON.stringify — different call site, same outcome, and error.message carries the getter's own error.
    • A schema carrying its own toJSON is refused even though JSON.stringify would have serialized it through that method. Kept as a deliberate false positive, with the reason recorded: the method cannot survive the round trip either, so what load() returns would not be the object that was saved. That reason is better than the "defensible" I offered.
    • The seen set means a subtree reachable by two paths is reported at the first path only — refusal still correct, path list not exhaustive for shared references. The implementer found this and documented it unprompted.

    Recorded

    • NOT MEASURED: check:readme-exports exits 1 on its own population-collapsed precondition (32 of 40 packages unbuilt); the diff touches no README and no export.
    • A first type-check in the recreated worktree reported TS2307 Cannot find module '@object-ui/types' — the unbuilt-closure condition, correctly classified as NOT MEASURED rather than red. Worth keeping because, as the implementer put it, it reads exactly like a broken import. Building the closure first turned it green.
    • Out-of-scope finding filed as finding(react): useSchemaPersistence writes adapterRef.current during render — three react-hooks/refs warnings on a published hook #6745 (adapterRef.current read/write during render — three pre-existing warnings on lines byte-identical to origin/main).
    • Platform note, not a defect: each PR-body PATCH re-appends a bare attribution footer, so the body carries two; the session ID is preserved in the body prose instead.

    Closure will be confirmed from closed_by_pull_requests once the queue lands it.


    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:dispatchedpriority:p1

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions