Skip to content

fix(metadata-protocol): the groupssections fold reaches the stored row (#7134) - #7468

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-7134-groups-fold-save-path
Aug 11, 2026
Merged

fix(metadata-protocol): the groupssections fold reaches the stored row (#7134)#7468
os-zhuang merged 2 commits into
mainfrom
claude/issue-7134-groups-fold-save-path

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes #7134

The gap, re-verified on origin/main before implementing

The card's premise holds, and both halves were re-measured at my base SHA 08363a09f rather than taken from the body (every line number in the card and its triage comment predates three landings on protocol.ts, so everything below is anchored on function names):

  • The producer fold is live. foldFormGroupsIntoSections is attached to FormViewSchema as a .overwrite() check in packages/spec/src/ui/view.zod.ts. Its own docstring already names this gap under "What this does NOT reach": "a sys_metadata row saved through saveMetaItem is persisted VERBATIM (the save validates and then discards parsed.data on purpose)".
  • Nothing under packages/metadata-protocol/src/ folds groups. Confirmed by grep. The only groups handling in packages/spec/src/conversions/registry.ts walks sections and groups as parallel keys for the visibleOnvisibleWhen rename — it does not fold one onto the other.

Measured directly through ViewMetadataSchema.safeParse at that SHA: a form authored with groups parses to one carrying sections, graftNormalizedOperators returns the authored body unchanged, and the row therefore keeps groups.

The card's key claim, re-measured rather than assumed

The card said graftNormalizedOperators cannot express this and needs a sibling, not a parameter. Confirmed. That function iterates Object.entries(authored) and only ever writes into a key the authored object already has — so it can neither drop groups nor introduce sections. A key move is outside what its walk can say.

What lands

graftFoldedFormSections — a sibling walk that replays exactly one normalization. At each structural position it treats the fold as having happened iff the author wrote groups, the parse did not keep it, and the parse produced sections in its place — the exact post-condition of the producer's fold. Consequences of detecting it that way rather than from a key list:

  • one walk covers every depth a form lives at: the flattened runtime overlay (top level), config on a ViewItem, and form / formViews.* on a container — each verified by its own test — and a form slot added later is covered with no edit here;
  • a groups key the schema keeps (a different vocabulary — app nav groups, a passthrough record) fails the middle test and is left alone.

What moves is the authored array, verbatim. parsed.data's sections carry schema defaults (collapsible, collapsed, columns); persisting those would be the wholesale swap this design exists to avoid. When the author wrote both keys, sections wins and groups is dropped — the producer's own precedence rule, empty array included.

Both grafts now run on every save, fold first, so the operator walk meets sections lined up with the parsed tree. Form sections carry no operator today (visibleWhen is a CEL string), so that ordering is structural hygiene rather than a measured fix, and the comment at the call site says exactly that.

Guardrails honoured

⚠️ Rows persisted before this change still carry groups. They heal on the author's next save, exactly as #4542's flow rows do. Nothing is backfilled at read — a deliberate scope line, stated in the changeset.

File surface, file by file

File Change
packages/metadata-protocol/src/protocol.ts New exported graftFoldedFormSections beside graftNormalizedOperators; saveMetaItem's post-validation graft composes the two (fold first).
packages/metadata-protocol/src/index.ts Export the new helper alongside its sibling.
packages/metadata-protocol/src/protocol.graft-folded-form-sections.test.ts New. 16 cases: the storage-row path through the REAL saveMetaItem (stub engine, row read back), plus helper-level structural pins.
packages/rest/src/public-form-routes.stored-row.test.ts New. 8 cases: the acceptance surface — the real public-form route handlers, fed the body a real saveMetaItem persisted. No production change in this package.
.changeset/groups-fold-reaches-stored-rows.md New. @objectstack/metadata-protocol patch.

Acceptance surface — pinned on the STORAGE-ROW path, not the code-authored one

The code-authored path was already fixed by #7128, so the pins drive the real saveMetaItem against a stub repository and read the persisted sys_metadata row — a helper-only pin would have stayed green if the call site were dropped. The REST tests then hand that genuine stored body to the real route handlers. The reader is stubbed (as in the existing public-form-routes.test.ts): the ADR-0087 stored-row conversion chain is a different seam and this card did not touch it, so stubbing it keeps the pin pointed at the save.

Of the three degradations #6926 listed, two clear and one does not:

Reverse verification

Directions declared before running. Probe: revert the call-site composition to graftNormalizedOperators(request.item, parsed.data), keeping the helper so the helper-level pins stay compilable; dist rebuilt before every read.

  • Predicted red, went red (10/10) — 6 save-path cases (flat overlay / config / container + formViews / authored-array-not-defaulted / both-keys precedence / fold-and-round-trip-keys coexist) and 4 REST cases (GET schema, POST submit, stored-row spelling, empty-groups folds to empty sections).
  • Green in BOTH directions, labelled GUARD in the test files (7) — round-trip keys survive; a canonical-sections form stores byte-identical; a LIST overlay is untouched; the operator graft still fires; and on the REST side the published schema, the submit whitelist, and the empty-declaration refusal (finding: a section-less public form publishes EVERY field of its target object to anonymous callers #6601/finding: a section-less public form accepts ANY non-anchor field from an anonymous visitor (the write-side twin of #6601) #6920) for a form that never carried the alias. Plus the BOUNDARY case above, green both ways by construction.
  • Also green both ways, by construction — the 6 helper-level structural pins, which the call-site-only revert does not reach. Reported as such, not counted as guards.
  • Missed predictions: 2, both mine, both fixed. The first pass had two cases labelled GUARD that went red: each bundled a fix-dependent sections assertion into an otherwise-guard-shaped test. Split — the guards now assert route/round-trip behaviour only, and the fix-dependent halves moved into the evidence blocks under their own names. The second pass had zero missed predictions, which is the run reported above.
  • Left unmeasured — nothing in the declared set.

Gates run locally

Gate Result
check:meta-type-normalized ✅ OK (18 files)
check:route-envelope ✅ 8 route modules, 7 conformant, 1 exempt
check:nul-bytes ✅ 6842 files scanned
@objectstack/metadata-protocol suite ✅ 70 files, 1025 tests
@objectstack/rest suite ✅ 81 files, 1323 tests
@objectstack/objectql suite ✅ 176 files, 3127 tests
@objectstack/rest typecheck ✅ clean

The rest suite and typecheck first reported failures that were unbuilt dependency dist in this fresh container (@objectstack/objectql, @objectstack/service-analytics, @objectstack/service-package — file-load errors with zero test failures, not regressions); the results above are after building those. @objectstack/metadata-protocol has no typecheck script (type-check-coverage ledger).

Per this card's dispatch I am not running the full lint farm locally — CI runs it once.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LBDWUuXmTrHe1o13eik3bT


Generated by Claude Code

…ored row (#7134)

PR #7128 folded `FormViewSchema.groups` onto canonical `sections` at the
producer, making the declared alias true for every consumer of a PARSED form.
It did not reach a form authored in Studio. `saveMetaItem` parses through that
same schema — so it already computed the folded body — and then discards
`parsed.data` on purpose, to keep the Studio-only round-trip keys (`isPinned`,
`isDefault`, `sortOrder`) a wholesale swap would strip. The authored spelling
was persisted verbatim, so the row reached `sections`-reading consumers spelled
`groups`, and two of the three public-form routes degraded: `GET /forms/:slug`
published an empty field schema (#6601) and `POST /forms/:slug/submit` refused
the submit on an empty `allowedFields` (#6920).

`graftFoldedFormSections` grafts that one normalization back on, a sibling of
`graftNormalizedOperators` rather than a parameter on it: that walk copies a
changed SCALAR at a key both sides carry, and `groups` → `sections` is a key
move its per-key loop cannot express. Detection is the producer fold's own
post-condition — the author wrote `groups`, the parse dropped it, the parse
produced `sections` — so one walk covers the flattened overlay, `config` on a
ViewItem, and `form`/`formViews.*` on a container with no list to maintain.

Per Prime Directive #12 `packages/rest` is untouched: no `?? match.form?.groups`
in the consumer. `packages/spec` is untouched too — this narrows what is stored,
never what is accepted.

Rows persisted before this change still carry `groups` and heal on the author's
next save, as #4542's flow rows do. Nothing is backfilled at read.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LBDWUuXmTrHe1o13eik3bT
@vercel

vercel Bot commented Aug 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 11, 2026 12:51am

Request Review

@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Aug 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol.

3 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx (via @objectstack/metadata-protocol)
  • content/docs/kernel/services-checklist.mdx (via @objectstack/metadata-protocol)
  • content/docs/protocol/kernel/http-protocol.mdx (via @objectstack/metadata-protocol)

1 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/v9.mdx (via @objectstack/metadata-protocol)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

…type errors (#7134)

Two CI gates on the previous head, both in the test file added by this PR:

`check:engine-double-contract` — the stub engine's `delete()` / `update()` did
not route through the producer's own dispatch predicates, so the double was
free to accept calls `ObjectQL` itself refuses. A fake looser than the real
implementation is a defect generator (#4434 shipped a dead REST route with a
green suite that way), which is the exact failure mode this card is about.
Both verbs now open with `assertEngineDeleteDispatch` / `assertEngineUpdateDispatch`
from `@objectstack/metadata-core` — already a dependency of this package, and
the right side of the edge: importing them from `@objectstack/objectql` would
invert a dependency turbo refuses.

`check:type-check-debt --re-measure` — `@objectstack/rest`'s TEST_DEBT records
155 and the branch measured 157. Both new errors came from ONE cause: the
extensionless `./rest-server` import does not resolve under
`moduleResolution: nodenext`, so `RestServer` became `any` and the callback over
it reported TS7006. Adding the `.js` extension takes the package back to exactly
155 — the ledger is untouched, which is what a shrink-only ratchet requires.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LBDWUuXmTrHe1o13eik3bT

Copy link
Copy Markdown
Contributor Author

ACCEPT — metadata seat, PM review.

Green on all 25 checks at 305d86f7, including the two that were red on the previous head. The patch round fixed both at the right layer:

  • check:engine-double-contract — the double now opens delete() / update() with assertEngineDeleteDispatch / assertEngineUpdateDispatch, imported from @objectstack/metadata-core rather than @objectstack/objectql. That import direction is the correct call: the other one inverts an edge turbo refuses.
  • check:type-check-debt --re-measure — 157 → 155 by adding the .js extension to the ./rest-server import (extensionless does not resolve under moduleResolution: nodenext, so RestServer degraded to any and the callback over it reported TS7006). The ledger entry was not raised. One broken extension reading as two type errors is a better diagnosis than the one I sent, and the shrink-only ratchet is intact.

What I checked at source rather than from the report:

⚠️ One claim in the body does not hold, and it is worth correcting rather than merging silently. The BOUNDARY case is described as an assertion that "goes red the day #7467 lands, so whoever fixes it is told to revisit." It will not. The fixture declares no publicPicker at all, so once #7487 makes the key declarable the route still answers 403 for this form — the assertion stays green and nobody is signalled.

The test itself is correct and its stated reasoning is correct in both worlds; what is wrong is only the belief that it functions as a tripwire. Not blocking: the fix is sound, and the revisit it was meant to trigger is already in flight as #7487. But a tripwire that cannot fire is worse than none, because it buys false confidence — flagging here so the record is accurate and whoever lands #7487 knows this pin will not tell them anything.

Merging SQUASH.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 11, 2026 01:21
@os-zhuang
os-zhuang added this pull request to the merge queue Aug 11, 2026
Merged via the queue into main with commit 769511c Aug 11, 2026
26 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-7134-groups-fold-save-path branch August 11, 2026 01:32
os-zhuang pushed a commit that referenced this pull request Aug 11, 2026
…ow answers (#7467)

PR #7468 landed the stored-row fold with a 'BOUNDARY: STILL 403' case
annotated to be revisited the day publicPicker became declarable. This is
the revisit: the same real saveMetaItem persists a groups-authored form
whose field carries a spec-valid publicPicker, and the same real route
handler answers 200 with projected data. The picker-less 403 half stays
pinned in public-form-lookup-picker.test.ts.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnMk7vfrt2zW7xvwLS3JDf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A Studio-saved form authored with groups still degrades on the REST public-form routes — the producer fold does not reach stored rows

2 participants