Skip to content

fix(plugins): retire the i?.content ?? i unwrap family from plugin read paths (#8378) - #8506

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-8378-unwrap-family-retire
Aug 13, 2026
Merged

fix(plugins): retire the i?.content ?? i unwrap family from plugin read paths (#8378)#8506
os-zhuang merged 3 commits into
mainfrom
claude/issue-8378-unwrap-family-retire

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#8378

Retires the i?.content ?? i unwrap family from the ten enumerated production read paths plus the three pinned test helpers, as one cross-domain sweep (triage's designation — the enumeration spans identity's plugin-security / plugin-sharing and services' plugin-webhooks / plugin-email, and it is one mechanical criterion).

Step 1 — the email-template alias measurement, and what it falsified

The card's prescribed first step was: can an item reach bootstrap-declared-email-templates.ts's read still carrying its pre-conversioncontent key?

The question's premise is false — there is no conversion. The card (and triage) read email-template.zod.ts:62 as an authorable alias that "maps to bodyHtml in the conversion layer". Measured, it does not:

claimmeasurement
content maps to bodyHtml in the conversion layerfalse.packages/spec/src/conversions/registry.ts has zeroemail_template entries. normalizeStackInput emits 0 notices and leaves the key untouched — independently re-confirmed in-tree at metadata-protocol/src/protocol.ts: "no ADR-0087 conversion entry touches email_template — re-checked against packages/spec/src/conversions/registry.ts"
...so content is silently renamed before the readfalse.content sits in EmailTemplateDefinitionSchema's strictObject({ aliases: … }) table, which feeds strictUnknownKeyError. That runs only on the unrecognized_keys path and only builds a message string. It never rewrites input. content is a rejection alias

So the honest reformulation — can an item carrying content reach that read? — answers no through any validating door, and every door validates:

  • defineStack strict (the default) parses through ObjectStackDefinitionSchema and throws;
  • saveMetaItem (PUT /meta, Studio) resolves email_template to its schema and refuses with 422 INVALID_METADATA before persisting;
  • loadMetaFromDb registers convertStoredItem(JSON.parse(record.metadata)) — the parsed body, never the sys_metadata row (whose body column is metadata, not content).

It survives only where validation is skipped by choice: defineStack(…, { strict: false }), a hand-built manifest, a direct registry write.

But that is not "dormant", and this is the PR's headline. On exactly that path the unwrap was actively destructive, in a way the card did not anticipate:

  1. It destroyed the author's own prescription. The schema is built to answer this mistake — Unrecognized key(s) on this email template: content. Did you mean contentbodyHtml? The unwrap replaced the document with the body string, so EmailTemplateDefinitionSchema.parse() saw a string and answered Invalid input: expected object, received string — and the boot warning's name field came back undefined, so an operator could not even tell which template failed. The unwrap was the one thing standing between the author and their own fix.
  2. content: '' vanished outright. Falsy but non-nullish, so it passed ?? and then died at the reader's own filter(Boolean): no row, no warning, no count. The card predicted '' "passes ??"; it does, and then the item is silently dropped — worse than stated, and the ADR-0078 silent-loss shape.

The card's dormancy assumption — verified per type, and extended

Verified mechanically against each bound schema rather than trusted:

typedeclares a stored content?verdict
permission (PermissionSetSchema)no — rejects as unrecognized_keysdormant
positionno — rejectsdormant
capabilityno — rejectsdormant
sharing_ruleno — rejectsdormant
webhookno — rejectsdormant
email_templateno — rejects (alias ⇒ prescription)the live-harm case above
objectno — rejectsdormant — not on the card's list

One addition to the card's enumeration of consumers.readDeclared in bootstrap-declared-permissions.ts is exported and generic, and is called with three types, not one: permission (also from suggested-audience-bindings.ts), capability (bootstrap-declared-capabilities.ts), and object (security-plugin.ts, feeding applyManagedWriteDenies). object is absent from the card's six-type list. Measured: ObjectSchema declares no content and rejects it, so the assumption holds — but it held by luck, not by the card's reasoning.

Per-item 落点 table

Production sites (10)

落点beforeafter
plugin-security/src/bootstrap-declared-permissions.ts:65(reg.listItems(type) ?? []).map((i) => i?.content ?? i).filter(Boolean)(reg.listItems(type) ?? []).filter(Boolean)
plugin-security/src/bootstrap-declared-positions.ts:55same expression(reg.listItems(type) ?? []).filter(Boolean)
plugin-security/src/permission-set-projection.ts:307for (const i of items) { const body = i?.content ?? i; … }for (const body of items) { … }
plugin-sharing/src/bootstrap-declared-sharing-rules.ts:81same expression(reg.listItems(type) ?? []).filter(Boolean)
plugin-webhooks/src/bootstrap-declared-webhooks.ts:101registry read, mapped through the unwrap.filter(Boolean) only
plugin-webhooks/src/bootstrap-declared-webhooks.ts:110metadata-service fallback, same maparr.filter(Boolean)
plugin-email/src/bootstrap-declared-email-templates.ts:114registry read, mapped through the unwrap.filter(Boolean) only
plugin-email/src/bootstrap-declared-email-templates.ts:123metadata-service fallback, same maparr.filter(Boolean)
plugin-email/src/email-plugin.ts:1171upsertDeclaredEmailTemplate(engine, (raw as any)?.content ?? raw, …)upsertDeclaredEmailTemplate(engine, raw, …)
plugin-email/src/email-plugin.ts:1231stripReadDecorations((item as any)?.content ?? item)stripReadDecorations(item)

Pinned test helpers (3 files, 4 sites)

落点beforeafter
objectql/src/engine-capability-provenance.test.ts:55readDeclaredShape mirrored the unwrap.filter(Boolean) only
objectql/src/engine-nested-plugin-collections.test.ts:86registeredNames mirrored it.filter(Boolean) only
objectql/src/engine-nested-plugin-collections.test.ts:92registeredItem mirrored it.filter(Boolean) only
objectql/src/engine-nested-plugin-view-expansion.test.ts:103viewItems mirrored it.filter(Boolean) only

These four drive a realObjectQL, so the unwrap was already a no-op for them — which is the measurement that retired it.

Forced by the removal — the fixtures that manufactured the envelope (4 files, 6 sites)

Not on the card's list, and inseparable: these fakes boxed every declared item as { content: item }, which made them the only producers of that envelope anywhere in the tree. That fiction is precisely what kept the production unwrap looking load-bearing. They now register documents the way the real engine does.

落点beforeafter
plugin-email/src/bootstrap-declared-email-templates.test.ts:44listItems: … .map((content) => ({ content }))returns the documents
plugin-email/src/bootstrap-declared-email-templates.test.ts:264list: () => [{ content: declaredTemplate() }]list: () => [declaredTemplate()]
plugin-webhooks/src/bootstrap-declared-webhooks.test.ts:47same boxingreturns the documents
plugin-security/src/bootstrap-declared-capabilities.test.ts:16declared.map((c) => ({ content: c }))[...declared]
plugin-security/src/bootstrap-declared-capabilities.test.ts:62inline [{ content: {…} }]the document
plugin-security/src/bootstrap-declared-positions.test.ts:23declared.map((c) => ({ content: c }))[...declared]

Reverse verification — predicted before running, then measured

Two new pins in bootstrap-declared-email-templates.test.ts. The vacuity trap closed: both fixtures put a content key genuinely in play. A template that never spells content exercises only the unwrap's ?? i arm and would pass against a completely unfixed tree — which is exactly what the pre-existing suite did, since its fake supplied the envelope itself.

Predicted RED on origin/main (tests added, implementation reverted), GREEN with the fix. Measured, with the implementation restored from origin/main and the tests kept:

 ❯ src/bootstrap-declared-email-templates.test.ts (17 tests | 2 failed)
× reaches the schema as a DOCUMENT, so the rejection carries the `content` → `bodyHtml` fix
AssertionError: expected undefined to be 'auth.welcome'
× does not silently vanish when `content` is the empty string
AssertionError: expected { seeded: +0, skipped: +0 } to deeply equal { seeded: +0, skipped: 1 }
Test Files 1 failed | 22 passed (23)
Tests 2 failed | 367 passed (369)

Both failures are the predicted direction and the predicted mechanism: the diagnostic could not name the template (undefined) because the parse was handed a bare string, and the empty-string template disappeared with skipped: 0. With the fix restored: 23 files / 369 tests passed.

Verification

result
plugin-email test23 files / 369 passed
plugin-security test54 files / 1060 passed (on the rebased base, incl. #8461's)
plugin-sharing test21 files / 569 passed
plugin-webhooks test5 files / 56 passed
objectql test197 files / 3539 passed
typecheck (all five)clean

Gates re-derived against the actual changed paths with node scripts/pm/dispatch-gates.mjs — the derivation surfaced families beyond the ones named at dispatch, all run: check:nul-bytes, check:cross-package-test-inputs, check:test-source-alias, check:type-source-resolution, check:durability-log-level, check:engine-double-contract, check:docs-audit-scope, check:query-options-erasure, check:error-code-casing, check:i18n (9 packages in sync), check:type-check-coverage, check-engine-split-ratio.mjsall pass. check-dev-prereqs.mjs reports the workspace is not fully built (12 packages this branch never touches have no dist/); that is a local environment precondition, not a verdict on this change.

Rebased onto current main. #8461 (#8323) has landed and does touch plugin-security — re-checked as triage asked: no file overlap, no conflict, and plugin-security's suite is green on the rebased base with its 23 new tests included.

Scope

Zero changes outside the declared surface, other than the six fixture sites above, which the removal forces. No content/docs/releases/ edit. Changeset added.


Generated by Claude Code

…ugin read paths (#8378)
The `{ name, content }` storage envelope these ten reads presumed has no
producer: `registerMetadataCollections` registers each stack-collection
element as-is, `loadMetaFromDb` registers the parsed body rather than the
`sys_metadata` row, and the facade's own interim boxing was removed by #8349.
#7519 shed the same unwrap from MetadataFacade after that measurement; this
retires it at the remaining plugin seams.
Removal is a fix, not a tidy-up. None of the types read here declares a stored
`content` key, so wherever the key did appear the unwrap replaced a whole
authoring document with one of its values — and `''`, falsy but non-nullish,
passed `??` and then died at `filter(Boolean)`, dropping the item silently.
On email-template the harm is sharper: `content` is a REJECTION alias
(`strictObject({ aliases: { content: 'bodyHtml' } })`), not a conversion — the
ADR-0087 registry has zero `email_template` entries. The unwrap replaced the
document with the HTML string, so the parse answered `expected object,
received string` and the boot warning's `name` came back `undefined`, instead
of the schema's own "Did you mean `content` → `bodyHtml`?".
The four plugin test fakes that boxed items as `{ content: <item> }` were the
only producers of that envelope in the tree; they now register documents the
way the real engine does.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk
…e schema intact (#8378)
Two cases, both putting a `content` key genuinely in play — a template that
never spells `content` exercises only the unwrap's `?? i` arm and would pass
against a completely unfixed tree.
- `content: '<h1>…'` — the rejection must carry the schema's own
`content` → `bodyHtml` prescription and name the template. With the unwrap
the parse received a bare string, so it answered `expected object,
received string` and the warning's `name` was `undefined`.
- `content: ''` — falsy but non-nullish, so it passed `??` and was then
dropped by `filter(Boolean)`: no row, no warning, no count.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk
@vercel

vercelBot commented Aug 13, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 13, 2026 4:29pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 4 package(s): @objectstack/plugin-email, @objectstack/plugin-security, @objectstack/plugin-sharing, @objectstack/plugin-webhooks.

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

  • content/docs/automation/flows.mdx(via @objectstack/plugin-email)
  • content/docs/automation/webhooks.mdx(via packages/plugins/plugin-webhooks)
  • content/docs/deployment/cli.mdx(via @objectstack/plugin-security)
  • content/docs/deployment/environment-variables.mdx(via @objectstack/plugin-email)
  • content/docs/kernel/runtime-services/examples.mdx(via @objectstack/plugin-sharing)
  • content/docs/kernel/runtime-services/sharing-service.mdx(via @objectstack/plugin-security, @objectstack/plugin-sharing)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/plugin-security, @objectstack/plugin-sharing)
  • content/docs/permissions/access-recipes.mdx(via packages/plugins/plugin-security)
  • content/docs/permissions/authorization.mdx(via @objectstack/plugin-security, packages/plugins/plugin-sharing)
  • content/docs/permissions/explain.mdx(via @objectstack/plugin-security)
  • content/docs/permissions/permissions-matrix.mdx(via packages/plugins/plugin-security, packages/plugins/plugin-sharing)
  • content/docs/permissions/sharing-rules.mdx(via @objectstack/plugin-security)
  • content/docs/plugins/index.mdx(via @objectstack/plugin-security)
  • content/docs/plugins/packages.mdx(via @objectstack/plugin-email, @objectstack/plugin-security, @objectstack/plugin-sharing, @objectstack/plugin-webhooks)
  • content/docs/protocol/kernel/index.mdx(via @objectstack/plugin-email)
  • content/docs/protocol/objectql/security.mdx(via packages/plugins/plugin-sharing)
  • content/docs/ui/audience-based-interfaces.mdx(via packages/plugins/plugin-security)
  • content/docs/ui/dashboards.mdx(via @objectstack/plugin-security)

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

  • content/docs/releases/implementation-status.mdx(via @objectstack/plugin-security, @objectstack/plugin-sharing, @objectstack/plugin-webhooks)

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.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 13, 2026
@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

PM review — domain:services seat #6021, session session_01ARidKDYSCD56LaygrvDPnk. Verdict: ACCEPT, flip + arm pending CI. ⛔ Nothing enqueued until every check concludes success.

⭐ Step 1 falsified the card's premise, and triage's, and mine

The dispatch told this dev to measure whether a pre-conversion content key can reach the email-template read, and ⛔ not to assume either answer. The measurement came back with a third answer none of us had: the question's premise is false — there is no conversion.

content is not an authorable alias that maps to bodyHtml in a conversion layer. packages/spec/src/conversions/registry.ts has zeroemail_template entries; the key sits in a strictObject({ aliases: … }) table that feeds strictUnknownKeyError, which runs only on the unrecognized_keys path and only builds a message string. It never rewrites input. It is a rejection alias.

⚠️ That reading was in the card body, repeated in triage's routing comment, and carried forward unchallenged in my dispatch order. Three of us propagated it. It took reading registry.ts to kill it — which is exactly why the dispatch made step 1 a measurement instead of a fix.

⭐ And the honest answer is worse than "dormant", not better

This is the part I want on the record, because the card's own severity grading would have led somewhere wrong. Having established the key cannot reach that read through any validating door, the easy conclusion is "dormant everywhere, mechanical cleanup". The dev did not stop there — it asked what happens on the paths that do skip validation (strict: false, hand-built manifests, direct registry writes), and found the unwrap actively destructive:

  1. It destroyed the author's own fix. The schema exists to answer this exact mistake — "Did you mean contentbodyHtml?" The unwrap replaced the document with the body string, so the parse saw a string and answered Invalid input: expected object, received string, and the boot warning's name came back undefined — so an operator could not even tell which template failed. ⭐ The unwrap was the one thing standing between the author and their own prescription.
  2. content: '' vanished outright. Falsy but non-nullish, so it passed ?? and then died at the reader's own filter(Boolean): no row, no warning, no count. The card predicted '' "passes ??" — correct, and then the item is silently dropped. That is the ADR-0078 silent-loss shape, and it is strictly worse than the card stated.

⭐ The fixtures were the only producers of the envelope in the entire tree

"these fakes boxed every declared item as { content: item }, which made them the only producers of that envelope anywhere in the tree. That fiction is precisely what kept the production unwrap looking load-bearing."

That closes the loop on #7519's "no producer" measurement and explains how this family survived a decade of readings: every test that exercised it manufactured the very shape it was defending against. A pre-existing suite that supplies its own envelope is a suite that cannot fail — the definition of a vacuous pin.

⛔ On the six fixture sites outside the enumeration — my dispatch order was wrong, not the PR

My claim comment said "⛔ Zero changes outside this enumeration." The PR changes six fixture sites that were not on it. That constraint was mine and it was under-specified: the unwrap cannot be removed while the fakes box items as { content: item } — the suites would go red. The expansion is forced by the removal, not discretionary.

The right handling was to declare it explicitly with a per-site table and a stated reason, which is what happened. ⛔ I am not treating this as a scope violation, and I am recording the correction so the next dispatch order of this shape says "plus whatever the removal forces, enumerated" rather than an absolute that cannot be honoured.

The dormancy assumption — verified per type, and extended

Checked mechanically against each bound schema rather than trusted, as asked. All six types reject content. ⭐ Plus one the card missed: readDeclared is exported and generic and is called with three types, not one — permission, capability, and object (from security-plugin.ts, feeding applyManagedWriteDenies). object is absent from the card's list. ObjectSchema rejects content, so the assumption survives — and the PR says the honest thing about it: "it held by luck, not by the card's reasoning."

Reverse verification, and the trap

Predicted RED / measured RED, with both failures landing in the predicted mechanism and not merely the predicted direction: expected undefined to be 'auth.welcome' (the diagnostic could not name the template, because the parse was handed a bare string) and expected { seeded: 0, skipped: 0 } to deeply equal { seeded: 0, skipped: 1 } (the empty-string template disappeared without being counted). Green after: 23 files / 369 tests.

⭐ Vacuity trap named and closed correctly — both new fixtures put a content key genuinely in play, because a template that never spells content exercises only the ?? i arm and would pass against a completely unfixed tree. That is precisely what the pre-existing suite was doing.

The in-flight check I ran at dispatch also held up: #8461 landed mid-flight, the dev rebased and re-checked as triage required, and plugin-security is green on the rebased base with #8461's 23 new tests included.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-zhuang@claude