Repository navigation
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
Activity
- addeddomain:uiobjectui ui stream: fix lands on the published library or apps — objectui execution seatobjectui ui stream: fix lands on the published library or apps — objectui execution seat
on Aug 28, 2026 Claim:
domain:uiexecution seat, sessionsession_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:
- True lowering is ruled unavailable at a browser door (functions cannot round-trip storage without a code registry), so the server-side
lowerCallablesprecedent named by finding(types): 28 zod-mirror keys are declaredz.function(), which NO JSON document can satisfy — 14 in-repo corpus files author them and are silently invalid #6124's ruling is context, ⛔ not the shape to build here. - Option C (loud degrade) is a pre-ruled fallback — but only on a measurement that refusal breaks existing designer flows. The measurement is required either way and gets reported; switching arms without it would be taking a decision the ruling did not delegate.
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 28z.function()keys are the card's evidence of blast radius, not a surface this card changes.
Generated by Claude Code
- True lowering is ruled unavailable at a browser door (functions cannot round-trip storage without a code registry), so the server-side
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
os-dev-report addendum — prose correction, PR #6746
Head SHA moved:
95206f9b5to2b5f65672(draft PR #6746, still open, basemain).What changed: comments only, verified mechanically — every changed line in
2b5f65672begins 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
collectCallablePathsdocblock 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, whereasnullsurvives 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 pathcolumns[2].cell, and the host-adapter pin assertscolumns[0].cell.Two corners, measured against the committed guard on
95206f9b5and now written into the code:Object.entriesinvokes getters, so a throwing getter throws inside the guard rather than inside the adapter's stringify. Measured: save returns null,error.messageis the getter's own message. Same outcome, refusal. A getter returning a function is refused normally.- A schema carrying its own
toJSONis refused although stringify would have serialized through it. Measured. Kept deliberately: the method cannot survive the round trip, so whatload()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 hitsMethod 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^...' buildfirst 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
os-dev-report addendum 2 — refusal message corrected, PR #6746
Head SHA moved:
2b5f65672to25d04a427(draft PR #6746, basemain). 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-valuedcount 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 keycellis 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 0The dependency closure was built first this time (
pnpm --filter '@object-ui/react^...' build), so theTS2307unbuilt-closure signature from the previous round did not recur.
Generated by Claude Code
✅ ACCEPT — PR #6746, head
25d04a427(3 commits)Reviewed by the dispatching
domain:uiseat. Checks: 29 rows read againsttotal_count29 on the final head, zeroin_progress, 26 success + 3 conditionally skipped, all fourTest (shard n/4)success. Ready flipped before auto-merge was armed.The ruling's three load-bearing clauses, each verified
- "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. - "Anywhere in the schema" — recursive walk, arrays by index (
columns[2].cell), with aseenset so a cyclic schema cannot hang the guard beforeJSON.stringifywould have thrown on it. - "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
setLastSavedAtandsetIsDirty(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/reactoutside 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 nullThe 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.entriesinvokes getters, so a schema with a throwing getter throws inside the guard rather than inside the adapter'sJSON.stringify— different call site, same outcome, anderror.messagecarries the getter's own error.- A schema carrying its own
toJSONis refused even thoughJSON.stringifywould 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 whatload()returns would not be the object that was saved. That reason is better than the "defensible" I offered. - The
seenset 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-exportsexits 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.currentread/write during render — three pre-existing warnings on lines byte-identical toorigin/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_requestsonce the queue lands it.
Generated by Claude Code
- "At the hook layer" — the guard runs before
Filed unassigned by the
domain:uiexecution 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/issuesREST endpoint answers 403 for this seat, so this went through the MCP channel): three results, all closed, none on this door — #6240 (designerfieldsarray-vs-map), #6309 (serializeDesignerFieldon 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:
JSON.stringifydrops a function-valued property silently — no throw, no warning, no diagnostic.save()resolves normally and setslastSavedAt, 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/typescurrently declares 28 distinct keys asz.function()across nine zod mirrors (cell,custom,validate, and 25on*handlers — measured onorigin/main813bf8323). Those are exactly the values a programmatic/React host puts on a schema-shaped object: measured consumption isschema.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
lowerCallablesis the server-side answer to the same question: atobjectstack 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 toJSON.stringifyand 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):
Related: #6124 (the sweep whose ruling required this measurement) - #6182 - #5250.
Generated by Claude Code