Skip to content

fix(plugin-email): use the shared read-decoration strip, not a blanket underscore sweep - #16239

Merged
os-zhuang merged 4 commits into
mainfrom
claude/issue-16152-email-strip-drift
Sep 6, 2026
Merged

fix(plugin-email): use the shared read-decoration strip, not a blanket underscore sweep#16239
os-zhuang merged 4 commits into
mainfrom
claude/issue-16152-email-strip-drift

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes #16152

plugin-email kept a module-local stripReadDecorations that dropped every key starting with _, drifting from the shared list in packages/spec/src/kernel/metadata-read-decorations.ts — the module whose header says the list lives in spec precisely so producer and consumers cannot drift. This PR deletes the private copy and calls the shared helper.

Located by content, not by name: the same identifier is exported from @objectstack/spec/kernel, so a grep finds both. ⛔ metadata-read-decorations.ts is not touched — the shared list is correct as it stands.

The schema fact, re-verified on this branch's merge base

The private copy justified its blanket sweep in its own docblock by claiming EmailTemplateDefinitionSchema "declares no underscore key". Re-measured rather than inherited from the card:

  • packages/spec/src/system/email-template.zod.ts:5 imports MetadataProtectionFields, and line 141 spreads it into the shape: ...MetadataProtectionFields,.
  • packages/spec/src/shared/strict-object.tsstrictObject is z.object(shape, { error }).strict(), so the shape really is closed.
  • MetadataProtectionFields (packages/spec/src/kernel/metadata-protection.zod.ts:81) declares exactly seven keys: _lock, _lockReason, _lockSource, _provenance, _packageId, _packageVersion, _lockDocsUrl.
  • Neither _diagnostics nor _draft appears anywhere in that chain (grep count 0 in both files).

⇒ The claim is false. Every ADR-0010 envelope key is declared and parses clean; the two read decorations are undeclared and genuinely must go. The copy was removing keys the schema was deliberately widened to accept, and keeping the two it had to remove — disagreeing with the shared list in both directions, exactly as the card states.

What the dropped provenance actually costs: measured

The card and the triage comment both left this open, and the triage seat's census stopped at the package boundary. Measured through to the write, and the answer is structural rather than a census result:

  1. readEffectiveTemplate has exactly one consumer in the repo — email-plugin.ts:1240, feeding upsertDeclaredEmailTemplate. Nothing else sees its return value.
  2. upsertDeclaredEmailTemplate (bootstrap-declared-email-templates.ts:187) parses the body and then projects it through mapTemplateToRow (:76), a closed column list: name, label, category, locale, subject, body_html, body_text, from_address, from_name, reply_to, active, is_system, description, variables_json. No underscore key is projected.
  3. sys_email_template (packages/platform-objects/src/audit/sys-email-template.object.ts) declares 19 columns and zero underscore columns.

The dropped provenance costs nothing on this path, and could not have cost anything. The stronger reading is not "nothing reads it" but "nothing can read it": the envelope cannot reach the written row whatever the strip does. This also answers the triage seat's pre-registered escalation clause — trigger (a) does not fire, and neither does (b): the _lock* family cannot make a written row lose protection, because sys_email_template never carried lock state; ADR-0010 protection lives on sys_metadata. p3 stands on both triggers.

Which repair, and why the other one is wrong here

Took the first branch — call the shared helper — matching the four consumers that already do (the dataset query in rest-server.ts:10502, the cold-boot flow bind in service-automation/plugin.ts:1906, saveMetaItem's verbatim persist, and the route-level seed apply in runtime/domains/packages.ts:1572).

The second branch — a separately named envelope strip beside it — would have been wrong for this path, and the measurement above is why. A second strip would encode the rule "this path needs the envelope gone". That rule is not true here: the envelope's absence from the row is already guaranteed by mapTemplateToRow's closed projection. Spelling it as a strip would put a second copy of that truth one layer up — which is the very drift this PR removes, re-created in a new place. There is deliberately no second pass, and the docblock says so.

Safety of the narrower list was checked rather than assumed: the only producers of underscore keys on a served item are _diagnostics (decorateMetadataItem, metadata-diagnostics.ts:159), _draft (draft/preview reads), and the ADR-0010 envelope (registry / overlay row). That is exactly METADATA_READ_DECORATIONS plus the declared envelope, with nothing left over.

A second, unplanned finding: the sweep was hiding a malformed fixture

Switching to the narrow list turned the pre-existing test restores a baseline served WITH read decorations on it red. The cause is not the repair — the fixture carried _provenance: { source: 'code' }, an object, where MetadataProvenanceSchema is z.enum(['package','org','env-forced']). It was never spec-legal; the blanket sweep deleted it before the parse could say so. Corrected to _provenance: 'package', the spelling every other fixture in the repo uses.

This is the silent-swallow failure demonstrated on the repo's own test data, and it is the concrete reason the blanket form is worse than a wrong list: it also swallows malformed values of the keys it sweeps.

Each pin's population

Ablation restored the pre-fix blanket copy. Mutation proved on disk — git hash-object delta d5293255… to 91191c53…, injected-marker count 1, surviving shared-import count 0 — and the restore proved by blob equality back to d5293255… plus an empty git diff HEAD, under a trap … EXIT INT TERM with an absolute path.

Pin Populates under ablation?
does not silently swallow an underscore key the schema never declared Yes — red. expected [] to have a length of 1: under the blanket sweep the undeclared key vanishes before the parse and the reset reports success.
the schema declares the ADR-0010 envelope and rejects the read decorations No — a pure spec assertion; it guards the falsified justification, not the plugin.
re-materializes a baseline served with the FULL protection envelope on it No — and this is the measurement, not a weak test: the envelope is unobservable at the row either way, because mapTemplateToRow drops it. It stands as a regression guard that the envelope does not break the parse.

Reported plainly because only one of the three is defect-discriminating: the other two are premise and regression guards, and calling them pins of the fix would overstate them.

Verification

Gate family derived mechanically on the final head b454fc09d via node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack; the Reconciliation line reads 56 families, and --commands on the final head is byte-identical to the list that was run. All 56 measured green, exit codes captured immediately after a single redirected command, never through a pipe.

Two came back exit 3 — PREREQUISITE NOT MET, read as unmeasured rather than as passes: check:dual-build-cjs-loads and check:type-check-debt both need the built closure. Ran turbo run build --filter='./packages/*' --filter='./packages/*/*' and re-ran both — green (103/66/619/1 against floors 90/58/520/1; 12 ledger entries re-measured, none above its recorded number).

One came back a genuine exit 1: check:test-source-alias rejected a dynamic import('@objectstack/spec/system') inside a test body — this package resolves that specifier through dist/, so the first call would transform the dependency's module graph inside a clocked window. Hoisted to a module-scope import, as the gate prescribes; re-run green.

Also ran the 4 artifact-roster gates the derivation flags as keeping their roster in a directory one of these paths is in, whose silence is not evidence either way: check-changeset-fixed, check:authz-resolver, check:error-code-casing, check:filter-alias-parity — all exit 0.

  • pnpm --filter @objectstack/plugin-email test30 files / 468 tests passed
  • pnpm --filter @objectstack/plugin-email typecheck — exit 0, and its check:test-typecheck leg confirms the test layer really is compiled, so the new test file is measured rather than merely excluded
  • Ratchet families re-run on the final head after the last commit: type-check-coverage, type-check-debt, test-source-alias, cross-package-test-inputs, dts-closure, nul-bytes — all exit 0

Heavy runs were serialized through scripts/pm/os-verify-lock.sh; the wall-clock figures in its VERDICT lines are shared-box readings, not idle-machine numbers.

Changeset

Included, patch. Judged rather than defaulted: @objectstack/plugin-email is private: false at 17.3.0, and the diff changes observable runtime behaviour of that published package — an undeclared underscore key now surfaces on the write's response instead of being swallowed. Not a skip-changeset case, which is for diffs that publish nothing.

Out of scope

packages/plugins/plugin-security/src/permission-set-projection.test.ts:1231 pins mergeRowPatchIntoBody stripping _packageId and _provenance from a body — the same drift as this card, in a third package, and its fixture carries the same malformed _provenance: { a: 1 } object shape. Deliberately not fixed here: different package, and it would pull in a separate test and gate surface. It could not be filed as an issue during this run — the MCP search_issues channel needed for the duplicate check returned API rate limit already exceeded (repo-scoped REST is 403 for this session), and filing without a duplicate check is not an option this seat takes. Handed to the PM in the structured report for filing.


Generated by Claude Code

…t `_` sweep

`readEffectiveTemplate` stripped read decorations with a module-local copy of
`stripReadDecorations` that dropped every key starting with `_`. The shared
list it drifted from (`spec/kernel/metadata-read-decorations.ts`) carries
exactly `['_diagnostics', '_draft']` and names the ADR-0010 protection envelope
as "Deliberately NOT" a member — envelope state the write path legitimately
carries, allowlisted by the closed schemas.

The copy justified the sweep on the claim that `EmailTemplateDefinitionSchema`
"declares no underscore key". `email-template.zod.ts` spreads
`MetadataProtectionFields` into its `strictObject`, so every envelope key is
declared and parses clean.

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

`_provenance: { source: 'code' }` is an object where
`MetadataProvenanceSchema` is `z.enum(['package','org','env-forced'])`. The
fixture was never spec-legal; the module-local blanket `_` sweep deleted it
before the parse could say so. With the shared strip it reaches the schema and
is rejected — the silent swallow, demonstrated on the repo's own fixture.

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

`check:test-source-alias` flagged the dynamic `import('@objectstack/spec/system')`
inside a test body: this package resolves that specifier through `dist/`, so the
first call transforms the dependency's whole module graph inside a clocked
window. Hoisted to a module-scope import, where vitest clocks nothing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
@github-actions github-actions Bot added the size/m label Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-email, touching 2 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/kernel/services.mdx (via EmailServicePlugin (symbol, a top-level class))
What this run could not see
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 4 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 1ebde49536b431dc891b4bbe1a144b638c6205aepackageMentionDocs.

Which tree this was computed on

This run read content/docs from b6b6c5c0bc4d40bb0f61092d0661c17b84a21545 — the merge of head b454fc09d3decaa82fc6812f6e732045c41ba385 into base 1ebde49536b431dc891b4bbe1a144b638c6205ae, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin b6b6c5c0bc4d40bb0f61092d0661c17b84a21545 && git checkout b6b6c5c0bc4d40bb0f61092d0661c17b84a21545
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 1ebde49536b431dc891b4bbe1a144b638c6205ae b454fc09d3decaa82fc6812f6e732045c41ba385 && git checkout -B drift-repro 1ebde49536b431dc891b4bbe1a144b638c6205ae && git merge --no-ff b454fc09d3decaa82fc6812f6e732045c41ba385

node scripts/docs-audit/affected-docs.mjs --json 1ebde49536b431dc891b4bbe1a144b638c6205ae

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 1ebde49536b431dc891b4bbe1a144b638c6205ae → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Copy link
Copy Markdown
Collaborator Author

PM verification — PASS

Measured at head b454fc09d against merge-base f5aec38a6, independently of the report.

Fence — held

Local merge-base diff: 3 files — email-plugin.ts, its test, one changeset. packages/spec/src/kernel/metadata-read-decorations.ts is untouched, which was the brief's hard line: the repair had to be this consumer converging on the shared list, ⛔ never the shared list being widened to suit one consumer.

The card's premise — falsified justification confirmed false, at the source

The deleted private copy justified its blanket _-sweep in its own docblock with "EmailTemplateDefinitionSchema … declares no underscore key". I read the schema rather than the claim:

  • packages/spec/src/system/email-template.zod.ts:5 imports MetadataProtectionFields; :141 spreads it into the strictObject. So all seven ADR-0010 envelope keys (_lock, _lockReason, _lockSource, _provenance, _packageId, _packageVersion, _lockDocsUrl) are declared and parse clean.
  • grep -c '_diagnostics|_draft' over that same schema returns 0 — those two are genuinely undeclared, so they genuinely must go.
  • packages/spec/src/kernel/metadata-read-decorations.ts:48METADATA_READ_DECORATIONS = ['_diagnostics', '_draft'].

⇒ The schema disagrees with the old docblock in both directions at once, exactly as the card filed. The blanket sweep was removing keys the schema had been deliberately widened to accept, under a reason that was never true.

⭐ The measurement that decided the repair, re-taken

The brief made step 1 a measurement, not a code change, because it decides which of two repairs is correct. I re-took it:

  • readEffectiveTemplate is private (email-plugin.ts:1293) with exactly one call site (:1240), into upsertDeclaredEmailTemplate.
  • That path projects through mapTemplateToRow (bootstrap-declared-email-templates.ts:76), which is a closed literal: every key spelled out — name, label, category, locale, subject, body_html, body_text, from_address, from_name, reply_to, active, is_system, description, variables_json — with no spread of the source object and no underscore key.

⇒ The dropped provenance could not have been read by anything, rather than merely not being read today. That is the stronger statement, and it is what makes branch 2 (a second named envelope strip) the wrong repair: it would encode the rule "this path needs the envelope gone", which is false — and a second copy of a truth mapTemplateToRow's projection already holds is this card's own drift re-created one layer up. ⛔ One function must not silently mean both rules; the seat did not let it.

Pin population — reported honestly, and the honesty is the right call

Only one of three pins is defect-discriminating (red under ablation with expected [] to have a length of 1). The other two do not populate, and the report says so rather than dressing them up:

  • the spec assertion guards the falsified justification, not the behaviour;
  • the full-envelope re-materialisation is unobservable at the row either way, because mapTemplateToRow drops it — which is the measurement above, not a weak test.

⭐ A seat that reports two of its three pins as non-populating is doing the thing that makes the third one trustworthy.

The side finding is an exposure, not a regression

Switching to the narrow list turned a pre-existing test red because its fixture carried _provenance: { source: 'code' } — an object where MetadataProvenanceSchema is z.enum(['package','org','env-forced']). That fixture was never spec-legal; the blanket sweep had been deleting it before the parse could object. Corrected to 'package'. ⇒ The red is the fix revealing an old defect, not causing a new one — the correct reading, and in scope.

Changeset

Judged, not defaulted: plugin-email is private: false at 17.3.0 and runtime behaviour changes ⇒ a patch changeset, ⛔ not a skip-changeset case.

PM half-state — mine, and now fixed

The seat reported the card carried assignee os-warren with no Claim: comment, and posted its own because the repo CLAUDE.md makes claim-first binding. ⛔ That was my omission, not its error — pm-dispatch/SKILL.md:525-536 makes the claim step 2 of my atomic pair, and os-dev.md:38 tells the seat the PM's claim already satisfies it. My omission is what forced the improvisation, and the ad-hoc shape is what stalled five other PRs on check-clause2-carriers. Card #16152 now carries a readable Claim: + Clause-②: no (5557676461).

Carried forward, not dropped

The seat could not file its out-of-scope finding — search_issues answered API rate limit already exceeded, and filing without a duplicate check is a forbidden shape. Correctly reported rather than silently skipped. It is on this seat's queue: plugin-security's permission-set-projection.test.ts:1231 pins the same drift in a third package, with the same malformed object-shaped _provenance.

Verdict: PASS. needs:contract-review applied.


Generated by Claude Code

Copy link
Copy Markdown
Contributor

Contract review (clause ②) — PASS · landing held on the pair declaration — PR #16239 at head b454fc09 (Fixes #16152)

Reviewed by the director seat at tier (claude-fable-5-1, session session_01TezFG8ZMrNH6n5VTNpPpdH), 2026-09-06 07:52Z; taken here because the domain:services seat's tier fuse is blown.

Clause ② answer: no surface move; conformance limb yes — a consumer converges on the shared decoration list. No exported symbol or signature moves on @objectstack/plugin-email's published .d.ts; the deleted function was module-local and readEffectiveTemplate is private. packages/spec/src/kernel/metadata-read-decorations.ts is untouched — the brief's hard line, held. The accept set of the DELETE /meta/email_template/:name reset path narrows in exactly the direction protocol 17's closed schemas already declare: an underscore key that is neither _diagnostics / _draft nor a declared ADR-0010 envelope key is no longer swallowed before the parse; it surfaces as a projector failure on the write's own response. Defect repair.

Content, re-read at the source rather than the prose: email-template.zod.ts:141 spreads MetadataProtectionFields into the strictObject, so the seven envelope keys are declared and the deleted docblock's justification was false in both directions. The "which repair" measurement holds — readEffectiveTemplate has one consumer, mapTemplateToRow is a closed column list with no underscore key, sys_email_template declares no underscore column — so the envelope cannot reach the row whatever the strip does, and a second envelope-stripping pass would re-create this card's drift one layer up. The malformed fixture (_provenance: { source: 'code' } against z.enum(['package','org','env-forced'])) is the silent-swallow failure demonstrated on the repo's own test data; corrected to 'package'. The plugin-security sibling with the same shape is correctly left for its own card, not absorbed.

Tests read (three new pins, populations stated honestly: one defect-discriminating, one premise guard against the schema, one regression guard). The check:test-source-alias red on a dynamic import('@objectstack/spec/system') was fixed as the gate prescribes (hoisted to module scope).

Changeset: @objectstack/plugin-email: patch — correct (published package, observable behaviour, no surface move). CI at b454fc09: 31 success · 6 skipped · 0 failing. Governed-merge audit on the 3 paths: 0 hits.

Landing — held on one thing

--pair 16239 exits 4: card #16152's claim has no Claim: / Clause-②: spelling. needs:contract-review comes off this PR now (card never carried it). On pair exit 0 the next director pass flips ready-for-review + auto-merge (squash).


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review September 6, 2026 08:36
@os-zhuang
os-zhuang enabled auto-merge September 6, 2026 08:37
@os-zhuang
os-zhuang added this pull request to the merge queue Sep 6, 2026
Merged via the queue into main with commit dfb7a0d Sep 6, 2026
42 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-16152-email-strip-drift branch September 6, 2026 09:16
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/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

plugin-email keeps a private, drifted copy of stripReadDecorations that also strips the ADR-0010 provenance envelope its own schema allowlists

3 participants