Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell): strip the framework's read decorations before saveFields PUTs - #6502
Conversation
…lds PUTs `saveFields` spreads the served object verbatim (`...existingObject`) to preserve every key this service does not model. That spread does not distinguish keys the author owns from keys the framework adds on the way out: `_diagnostics` and `_draft` are read decorations the framework stamps onto every served metadata document, and `ObjectSchema` refuses both BY NAME, so a decorated document went straight back out on the PUT. Strip them at the write site through the spec's own exported `stripReadDecorations`, so the list stays the spec's rather than a local copy that goes stale the next time a decoration is added. This is the strip-on-write shape `MetadataObjectsPage.handleObjectsChange` already uses for `group` — applied where the spread is, because simply not writing the key is not enough when the spread is verbatim. Bounded on purpose, not a lenient "drop whatever the schema refuses" pass (AGENTS.md #0.1): it removes exactly the two keys the framework adds at read time and never stores, so a genuinely unrecognized author key still fails loudly. Nothing is lost by dropping them even though a PUT is an upsert — `_diagnostics` is the read-path verdict, recomputed on every read, and `_draft` reflects the row's `state` column and the `mode` parameter, never the body. The ADR-0010 protection envelope IS write-path state and the spec deliberately keeps it out of the decoration list, so this strip does not touch it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-support-ai
commented
Aug 26, 2026
PM review: ACCEPT at The fence held exactly. Option 1 only — the strip sits at the write site in ⭐⭐ The severity correction is the most valuable thing in this report, and it needs to be visible to anyone who reads this card later. The card predicted a hard And you were right that the fix stands anyway. The three reasons are the correct ones: the body is off-spec by the contract regardless of who cleans up after it, objectui is backend-agnostic and cannot assume any particular server strips, and the spec's own doc comment names a second consumer class beyond the write path. What changes is the severity story, not the verdict — this is a contract-correctness fix, not a live-outage fix, and saying so plainly is worth more than a louder claim would have been. ⭐ Refusing to date the server strip was the right call. A shallow checkout makes ⭐⭐ The count correction sharpens the ban instead of loosening it, and that is the part a weaker report would have got backwards. The card said "a dozen places" read The silent-loss question was answered, not waved at. PUT is an upsert, so removing a key is not neutral by default, and you measured that the helper removes exactly the two decorations, adds nothing, changes no surviving value, does not mutate its input, and returns the same reference when there is nothing to strip. Pinning the ADR-0010 protection envelope — The floor check was not asked for and should have been. The installed spec is 17.2.0, but app-shell declares Reverse verification is clean and isolated. Predicted RED, observed RED, with the four failures being exactly the four strip assertions — and the five controls staying green, including the three that measure the spec rather than your code. That is what makes it a measurement of this change rather than of something ambient. Mutation proved on disk by blob hash before any reading, restore proved by hash equality rather than an exit code, trap on EXIT INT TERM with absolute paths, and the no-rebuild-in-path claim stated plainly with the vitest source alias cited.
File face recorded, and #6488 / #6489 are released to dispatch once this lands: Landing: queued once CI settles green on Generated by Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
Fixes#6480
saveFieldsfetches the current object and spreads it verbatim (...existingObject) so every key the service does not model survives a field save. That spread does not distinguish keys the author owns from keys the framework adds on the way out._diagnosticsand_draftare the second kind, andObjectSchemarefuses both by name, so a decorated served document went straight back out on the PUT.Fixed as option 1 from the card — a strip at the write site, through the spec's own exported helper. Options 2 (adapter-level) and 3 (generic "strip what the schema refuses") were fenced off by the dispatch order and are not implemented.
The four pre-fix measurements
1 ·
stripReadDecorationsis exported by the spec this repo actually installs. Read on disk, not from the card:node_modules/.pnpm/@objectstack+spec@17.2.0_.../@objectstack/spec— version 17.2.0, matching the card.Also checked, because the import is the first of its kind in this repo:
packages/app-shelldeclares"@objectstack/spec": "^17.0.0", and that floor is honest — the 17.0.0 tarball from the registry carriesstripReadDecorationsin bothdist/kernel/index.d.tsanddist/kernel/index.js. No floor bump needed.2 · The zero-hit claim re-run with a positive control. A zero from a search not shown to find anything is not a measurement, so the same command shape was run for a symbol known to be imported from the same entry point:
Repo-wide across all file types, excluding
node_modules: control 11, claimed symbol 0. The card's claim holds.3 · The
_diagnosticsreader count is smaller than the card says, and the card's suggestion still stands. The card says "a dozen places ... (ResourceEditPage,ResourceListPage,JsonSourceEditor,flow-problems.ts,preview-registry.ts)". Counted directly, live reads of the decoration off a served document are 6 lines across 3 files:packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsxpackages/app-shell/src/views/metadata-admin/ResourceListPage.tsxpackages/data-objectstack/src/metadata-client.tsOf the five surfaces the card names, only two perform the property read.
JsonSourceEditor.tsx,flow-problems.tsandpreview-registry.tsmention_diagnosticsonly in comments — they consume diagnostics handed down as props. The count reaches "a dozen" only if comments and the route path<Route path="metadata/_diagnostics">(a URL segment, an entirely different thing) are swept in.The correction does not weaken the option-2 ban — it sharpens it. Two of the six reads are in
metadata-client.ts, the adapter, and they are deliberate re-attachment code: the adapter explicitly carries_diagnosticsonto the envelope it returns. An adapter-level strip would delete the key at the exact site written to preserve it, blinding both live readers.4 · The strip introduces no silent loss, and this was proved rather than assumed. A PUT here is an upsert — this repo's own
toObjectPayloadcomment says so ("a PUT is an upsert, so writing{}here would delete every field") — so removing a key is never neutral by default. These two are the exception, by the spec's own declaration:_diagnosticsis the read-path validation verdict, spread on bydecorateMetadataItemand recomputed on every read._draftreflects the row'sstatecolumn and themodeparameter, "never in the body".Neither is author state, so neither can be lost by not echoing it. Measured on the helper: it removes exactly those two keys, adds none, and changes no surviving key's value. Critically, the ADR-0010 protection envelope (
_lock,_lockReason,_lockSource,_provenance,_packageId,_packageVersion,_lockDocsUrl) — which is write-path state the server merges back — is deliberately not a member ofMETADATA_READ_DECORATIONSand survives intact. That is pinned in the test.One correction to the card's severity
The card predicts "the hard
422 INVALID_METADATAthat blocks every subsequent save". The card was explicit that this was observational, not a reproduced 422 — and that caveat turns out to matter. The framework'ssaveMetaItemperforms the same strip server-side, before the schema gate:So against a server carrying that code the PUT does not 422 on these two keys. I could not date that server-side strip: the
objectstackcheckout available here is shallow (50 commits), sogit log -Sattributes it to the oldest commit in the shallow history rather than to its real one. Treat the date as not measured.The fix stands regardless, and not as defensive leniency:
ObjectSchemarefuses both keys by name (measured on the installed spec, below). A client sending them is sending an off-spec body, whether or not a particular server cleans up after it — AGENTS.md #0.cols键,列数被静默丢弃 —— 要 3/4 列的示例在 docs 站实际渲染成 2 列 #4001.Reverse verification
Predicted RED, observed RED. The guard call was removed, the mutation was proved on disk (removed-text occurrences 0, injected-text occurrences 1, working blob
9d6bf497!= HEAD blobd32a488a), then the pin ran:The five that stayed green under mutation are the controls that make this a measurement of my change and not of something ambient: the three
the instrumenttests (they measure the spec, not this code), the author-key preservation test, and the undecorated no-op control.No rebuild sits between the mutation and the assertion. The test imports the mutated file by relative specifier (
./MetadataService), and the only workspace package it imports by name is aliased to source —vitest.config.mts:272maps@object-ui/data-objectstacktopackages/data-objectstack/src. So no packagedist/is in the path and the assertion cannot be reading a stale build. The mutation script carried atrap ... EXIT INT TERMrestore with absolute paths, and the restore was proved by blob-hash equality plus an emptygit diff HEAD, not by an exit code.Verification, all on
66556008dvitest run packages/app-shell/src/services/ + the two plugin-designer pins@object-ui/app-shelltype-check(tsc --noEmit && tsc -p tsconfig.test.json)@object-ui/app-shelllintcheck:control-bytesOK (5371 tracked text files)check:designer-field-key-paritydesigner-field-key-parity: OKcheck:spec-symbolsspec symbol derivation: OKcheck:phantom-depsEvery in-scope import is declared by the package that publishes itcheck:changeset-presence/check:changeset-no-majorcheck:spec-floorsreports a prerequisite, not a failure —no-artifact: Build the workspace before running this gate. Its own header says it reads each package's published.d.ts, so an internal value import like this one is outside its reach either way; the floor question it exists to catch was answered directly under measurement 1.Neither changed file contributes a lint finding.
MetadataService.tscarries exactly one warning,no-explicit-anyat line 476 — the pre-existingconst raw: any = await client.meta.getItem(...), which sat at line 463 before my comment shifted it.Lint narrowing, declared: the repo-wide
pnpm lintwas not run;@object-ui/app-shell's own lint task was. The narrowing is provable rather than assumed —eslint.config.jsconfigures no type-aware linting (noprojectService, noparserOptions.project), so a file's verdict is a function of its own bytes plus config. I changed no config and no file outsideapp-shell, so no untouched file's verdict can move. CI runs the full farm regardless.File face
Three files, and no more — #6488 and #6489 are queued on this seam:
packages/app-shell/src/services/MetadataService.ts— the import, the strip insaveFields, and a third bullet in that method's doc comment.saveFieldsis the only function whose body changed.packages/app-shell/src/services/MetadataService.readDecorationStrip.test.ts— new file..changeset/6480-strip-read-decorations.md— new file,patch.Untouched, and worth stating for the two queued cards:
saveObjectbuilds its body fromtoObjectPayload, an explicit modelled shape with no verbatim spread, so it cannot carry a decoration and needed no change.Generated by Claude Code
Generated by Claude Code