Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell): PUT the object payload's fields as the spec's name-keyed map - #6491
Merged
Merged
Conversation
…keyed map
`ObjectSchema.fields` is a REQUIRED record keyed by field name. Both of
`MetadataService`'s object writers emitted an array, and `saveFields` ran
the conversion in the wrong direction outright: the server's own document
arrives with `fields` as a map, and `fields.map(toFieldPayload)` replaced
it with an array on every field save.
Measured on the installed `@objectstack/spec` 17.2.0: an array — empty or
not — is refused `invalid_type @ fields`; a map (including `{}`) parses;
omitting the key is refused the same way. And the route is not lenient
about it. `metadata-protocol`'s `saveMetaItem` resolves metadata type
`object` to that same `ObjectSchema`, `safeParse`s the whole item and
throws `422 INVALID_METADATA` before persisting, so the array was
refused rather than stripped or stored.
The conversion refuses what it cannot key, because the spec will not:
`fields: { undefined: … }` PARSES GREEN, so a blind conversion would have
traded a loud, harmless 422 for a silently corrupt stored document. A
missing or blank `name` throws; so does a duplicate name, which is a loss
the array shape did not have. The map is built with `Object.fromEntries`
so a field literally named `__proto__` — a spec-legal name — becomes a
key instead of invoking the prototype setter.
`saveObject` with no `existingFields` still omits the key rather than
writing `{}`: a PUT is an upsert, so `{}` would delete every field of an
object on a save that only meant to rename it. `saveFields`' list IS
authoritative, so an empty one does write `{}`.
The public `existingFields` parameter keeps its `FieldMetadataPayload[]`
type — the array is converted inside — so no call site changes.Contributor
✅ 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
marked this pull request as ready for review
August 26, 2026 05:27
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#6240
ObjectSchema.fieldsis a required record keyed by field name. Both ofMetadataService's object writers emitted an array, andsaveFieldsran the conversion in the wrong direction outright: the server's own document arrives withfieldsas a map, andfields.map(toFieldPayload)replaced it with an array on every field save. Only the sibling writerMetadataFieldsPageagreed with the spec.Step one — the card's unmeasured premise, measured
The card filed "whether the route is lenient about this today is unmeasured". It is not lenient, and there is no third outcome.
Against the installed
@objectstack/spec17.2.0 (ESM build,dist/data/index.mjs):fields: [{ name: 'n', type: 'text', label: 'N' }]invalid_type @ fields— "expected record, received array"fields: []invalid_type @ fields— the container's type, not its contentsfields: { n: { type: 'text', label: 'N' } }fields: {}invalid_type @ fields— it is requiredAnd against the framework's own write door (
objectstack@f5a7f9c):metadata-protocol'ssaveMetaItemresolves metadata typeobjectto that sameObjectSchema(spec/kernel/metadata-type-schemas.tsbindsobject: ObjectSchema;resolveOverlaySchemareads that registry),safeParses the whole item, and on failure throws422 INVALID_METADATAbefore persisting. So the array was refused — not stripped (nothing strips it) and not stored (the throw precedes the write). The "stored verbatim" outcome objectui#6238 measured applies to types whose schema is tolerant or unregistered, andobjectis neither.The user-visible claim, measured rather than argued: every designer object save and every designer field save that went through this service was a 422 that wrote nothing.
The three dispatch constraints
1 — the map key comes from a REQUIRED name, and a missing one fails loudly. Not a stylistic preference. Measured on 17.2.0:
The spec accepts a
{ undefined: … }document. A conversion that keyed blindly would therefore have traded a loud, immediate, harmless 422 for a silently corrupt stored document.toFieldsMapthrows instead, and the pins assert both the throw and that no request was issued — the half atoThrow()assertion cannot see.Two more failure modes the conversion would otherwise have introduced, both refused:
amount; a map cannot, so the later would silently swallow the earlier. That loss is created by this conversion, so this conversion refuses it.__proto__.map['__proto__'] = fieldinvokes the prototype setter rather than creating a key — and__proto__is a spec-legal field name (the record's key schema is/^[a-z_][a-z0-9_]*$/, measured green). The map is built withObject.fromEntries, which defines an own property.2 —
saveFields' unknown-server-key preservation survives, and gains a pin....existingObjectstill carries every key of the fetched document this service does not model, asserted with a positive control in the same output provingfieldsreally was replaced. It matters more now than before, not less: while the body was refused, nothing it preserved ever reached storage.3 — the public signature is SEVERED, and this PR is additive-only at the public surface.
saveObject(obj, existingFields?: FieldMetadataPayload[])keeps its array parameter byte-for-byte; the conversion happens inside. Checked against the built entry rather than by grepping a sourceexportkeyword:dist/index.d.tscarries 0export *chains and names neitherMetadataServicenor the payload types, but it does exportuseMetadataService(): MetadataService | null, so the class's method signatures are reachable from the entry — and the diff touches none of them. The reshapedObjectMetadataPayload.fieldstype is declaredexportin its source file but appears in no exported signature (grepon the builtMetadataService.d.ts: declaration only) and has no importer anywhere in the repo, so its shape growth is not reachable from the entry. Nothing downstream is asked to change.Which half each check covers
⭐ The parity gate here is declaration-only, so it cannot see this class at all.
scripts/check-designer-field-key-parity.mjscompares key names against the spec's accept sets.fieldsis inObjectSchema's accept set under either shape, so the gate was green for the entire time the array was on the wire — its own coverage note 4 says so. It is green before and after this change, and that is not evidence about the fix. The half it cannot see is covered by runtime assertions on the captured request bytes (JSON.parseof the PUT body), which is what every new pin reads.Ablations — the right cells red, the controls green
Each mutation was verified on disk (injected/removed
grep -con the anchored text, both directions), restored bygit checkout HEAD -- <path>under atrap … EXIT INT TERMwith absolute paths, and every restore verified by blob-hash equality against the pinned base blob plus an emptygit diff HEAD—RESTORE-CHECK OK blob=403bc0a95…after all four. No rebuild leg was needed or skipped: every test imports the subject relatively (from './MetadataService') and@object-ui/data-objectstackis aliased topackages/data-objectstack/srcby the rootvitest.config.mts, so nodist/participates. The oracle@objectstack/spec/datais an installed package and was never mutated.the instrumentschema cases; all 3 sibling pin files;saveAdvisories;retiredObjectEnabled__proto__cell, every shape cellObject.fromEntries__proto__toFieldsMap(fields ?? [])(the wipe)omits fields entirely … does NOT write {}C and D are what make those two cells non-degenerate: each sits precisely where the two readings disagree.
The landed witness, flipped
MetadataService.specKeyObjectPayload.test.tspinned['invalid_type @ fields']so this could not silently change. It has now changed on purpose, so the pin is replaced rather than deleted — the claim it was really making ("judge the whole body, not just its key names") still holds and is stronger green than red, with a falsification asserting the body carries the field rather than having been emptied. Ablation A reds it.Deliberately NOT done
saveObjectwith noexistingFieldsstill omits the key rather than writing{}.{}parses green and a PUT is an upsert, so defaulting would delete every field of an object on a save that only meant to rename it. The body stays refused — unchanged from before this card. Pinned as a positive control; ablation D is the proof it is load-bearing. The judgement about that parameter is recorded in finding(app-shell):saveObjectcalled withoutexistingFieldsstill PUTs a bodyObjectSchemarefuses —fieldsis REQUIRED, and{}is not a safe default #6490._diagnostics,_draft) go back out on a PUT ObjectSchema refuses by name #6480. It edits the same expression and stays open; this change neither fixes nor blocks it. It should get easier:fieldsis now built by a single named helper instead of an inline.map, so the read-decoration strip it needs has one obvious seam, and the object-level spread it is about is untouched.invalid_key @ fields.firstName) instead of the container level — a strictly better diagnosis, since the author is told which field. Restating the spec's key regex client-side would be the second contract this repo bans; pinned as an honest-limit case instead.Follow-ups filed (
finding, unassigned, ungraded)saveFieldsrebuilds every field entry from the designer model, so per-FIELD server keys are dropped on every field save #6488 —saveFieldsrebuilds each field entry from the designer model, so per-field server keys (expression,precision,system, …) are dropped. Pre-existing, but newly reachable: the drop only lands now that the body is no longer refused. The sibling writer already solves it withcarryOver.MetadataFieldsPagekeys itsfieldsmap by blind assignment — a nameless field becomes{ undefined: … }and a field named__proto__vanishes #6489 —MetadataFieldsPagehas both map-key hazards this PR closes inMetadataService({ undefined: … }, and__proto__silently dropped by assignment).saveObjectcalled withoutexistingFieldsstill PUTs a bodyObjectSchemarefuses —fieldsis REQUIRED, and{}is not a safe default #6490 — the severed public-signature judgement:fieldsis required, so a no-fieldssaveObjectbody is still refused, and{}is not a safe default.Verification
Run at
c9a2d916f, the final commit, each verdict quoted from the gate itself.type-checkcovers the edits rather than merely passing beside them:tsc -p tsconfig.test.json --listFilesreports all six edited/created files in the program (1 hit each), so "typecheck clean" is a statement about them.Lint was not narrowed — the full repo-wide
eslint . --no-inline-config --format jsonran to completion: 3814 files judged by eslint's own config, 90 errors and 11244 warnings, every one of them pre-existing style noise in packages this diff does not touch. The nineservices/files contribute 0 errors;MetadataService.ts's single warning is the pre-existingconst raw: anyinsaveFields, and the diff adds no: anyline (git diff | grep '^+.*: any'is empty). Type-aware linting is not enabled (noproject/projectServiceineslint.config.js), so this diff cannot move any untouched file's verdict.check:readme-exportsfails locally with 61 findings, all of the single form "type entry./dist/index.d.tsis not on disk — runpnpm buildfirst", in six packages this diff does not touch. That is a prerequisite-not-met, not a measurement: building@object-ui/app-shellcleared all 8 of its own findings and left the other six packages' untouched. Recorded as NOT MEASURED locally rather than as a red; CI builds first.Generated by Claude Code
Generated by Claude Code