Skip to content

fix(metadata-protocol): withhold caught driver text from the batch verbs' response payloads (#8333) - #8436

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-8333-batch-verb-driver-text
Aug 13, 2026
Merged

fix(metadata-protocol): withhold caught driver text from the batch verbs' response payloads (#8333)#8436
os-zhuang merged 3 commits into
mainfrom
claude/issue-8333-batch-verb-driver-text

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#8333

Extends the rule #8136 installed for the uninstall cluster (P1–P5) to the eight batch-verb producers it enumerated and deliberately did not convert (P6–P13). Each of these reaches a client as data on a response rather than as a message, so no HTTP boundary's 5xx message withhold has ever been able to reach them.

The rule, unchanged: a caught error's sentence is quoted back to a caller only when that error declared itself a client-facing refusal — a 4xx status in the ADR-0112 envelope. Anything else gets a stable sentence, and the original goes to the server log.

Step 1 was the measurement, and it decided the outcome

#8333 is explicit that these eight must not be swept blind. Every site was driven for real against a failing sys_metadata before a line changed. Measured on origin/main:

#SiteDriver text reached the payload?Measured pre-fix valueAuthored refusals in that catchDeclared 4xx?
P6publishPackageDraftsfailed[].erroryesSQLITE_ERROR: no such table: sys_metadataNOT_OVERRIDABLE 403 · author-time INVALID_METADATA 422 + issues · METADATA_CONFLICT 409all
P7side effects failures[].erroryessame, appended to the prefixnone — infrastructure onlyn/a
P8materializeApplied.erroryessamenone via throw — see belown/a
P9seed-apply erroryessameseed-request ZodErrorno
P10duplicatePackagefailed[].erroryessameINVALID_METADATA 422yes
P11revertCommitfailed[].erroryessameVERSION_NOT_FOUND 404, ITEM_LOCKED 403, VERSION_NOT_RESTORABLE 409, WRITABLE_PACKAGE_REQUIRED 422yes
P12rollbackToPackageCommitfailed[].erroryessameCOMMIT_NOT_FOUND 404yes
P13migrateStoredMetadatarows[].reasonyessameINVALID_METADATA 422, ITEM_LOCKED 403yes

Two measured facts changed what this PR does:

  • P8's authored population never enters its catch. The real materializer (plugin-security) reports a refusal by RETURNING { success: false, error }; only driver faults throw. So converting the catch blanks no authoring feedback, and the returned string is pinned as untouched.
  • P9 was the one site the rule could not simply be applied to.applySeedBodies used SeedLoaderRequestSchema.parse(), so a malformed seed body arrived as a raw ZodError — authoring feedback that declared nothing. Per the card's step 2 the cure is at the producer, not the collector: it is now a safeParse raising a real 422 INVALID_METADATA. The author is strictly better off — that field carried a multi-line dump of zod internals and now carries the curated, path-pointing summary every other authoring surface produces.

These sites also wrote no log at all, so each conversion adds the console.warn that keeps the operator's half of the contract — the shape deletePackage's cleanup collector already uses.

The mandatory positive control

A real spec-validation failure on the publish path — a flow draft whose approval node carries broken CEL, the same worked example runtime-gate.test.ts uses — still reports, after the change:

failed[0].type = "flow"
failed[0].name = "leave_approval"
failed[0].error = "[invalid_metadata] flow/leave_approval failed author-time validation:
flows[0].nodes[1].config.approvers[0].value:
[approval-expression-invalid] expression approver does not parse as CEL…"
failed[0].code = "INVALID_METADATA"
failed[0].issues = [{ rule: "approval-expression-invalid", path: "flows[0].nodes[1].config.approvers[0].value", … }]

Which draft, which field, plus the machine-readable halves the Studio form highlights with. Green.

Reverse verification — direction predicted before running

Predicted with protocol.ts reverted: 16 red / 10 green. Measured: 17 red / 9 green.

⚠️ One missed prediction, kept rather than tidied. P7's log case was predicted green-in-both-directions because its console.warn predates this card — it came back RED, and rightly: that case asserts BOTH halves, and expectNothingLeaked runs first over a pre-fix payload that still carried the driver line. So it is evidence for the withhold, not a guard on the logging.

The [GUARD] sections earn their place under a different variant: with declaresClientRefusal forced to return false unconditionally, they go red — measured 8 red / 18 green. A second small miss recorded there too: 9 were predicted red and 8 came back, the survivor being "P8 leaves a RETURNED authored refusal alone" — correctly green, because that string never passes through the predicate. The variant run therefore doubles as proof of this PR's central measured claim.

Deliberately not touched

Verification

  • @objectstack/metadata-protocol — 81 files, 1196 tests green, on the merged tree.
  • Downstream consumer sweep, PREFIX form (--filter '...@objectstack/metadata-protocol' = packages that DEPEND ON it), against a rebuilt dist since packages/rest imports the built package: @objectstack/objectql 197 files / 3539 green, @objectstack/rest 111 files / 1827 green.
  • Two objectql fixtures pinned pre-[finding] metadata-protocol's batch verbs still put caught error text on client-facing payloads — the 8 producers option C did not reach #8333 behaviour and were re-judged individually rather than batch-re-spelled: the all-or-nothing publish double threw a code with no status, which the real producer never does (ITEM_LOCKED is 403) — declaration added, assertion unchanged; the throwing-materializer response-shape test keeps its shape assertion and takes the withheld string.
  • Gates: check:nul-bytes, check:engine-double-contract (195 pinned — the new doubles are pinned, not ledgered), check:error-code-casing, check:durability-log-level, check:filter-alias-parity, check:cross-package-test-inputs, check:changeset-gate-self-tests, check:objectui-changeset, check:query-options-erasure, check:type-check-coverage, check-changeset-no-major — all green. Families re-derived against the actual changed paths with scripts/pm/dispatch-gates.mjs.

Merged origin/main before pushing; #7748 (PR #8401) had landed and all measurements were re-run against the merged tree. No conflict with revertCommit.


Generated by Claude Code

…rbs' response payloads (#8333)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
…ts authoring-feedback bound (#8333)
Adds protocol.batch-verb-driver-text.test.ts: the eight P6-P13 reproductions,
the [GUARD] over-block bounds, the mandatory positive control (a real broken-CEL
approval flow still names the offending field on publish), P9's producer-side
declaration, and the operator half (the withheld line still reaches the log).
Two objectql fixtures pinned pre-#8333 behaviour and are re-judged individually:
the all-or-nothing publish double threw a `code` with no `status`, which the
real producer never does (ITEM_LOCKED is 403) - declaration added; the throwing
materializer's response-shape test keeps its shape assertion and takes the
withheld string, since a bare plugin throw is indistinguishable from driver text.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
@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 1:09pm

Request Review

@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.

@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

ACCEPT — PM review, domain:metadata seat

Green at d97fcc07: all 25 checks, each job's own conclusion verified individually. Path-fork check clean — 5 files, none under docs/adr/**, .claude/skills/** or skills/**.

I read the protocol.ts patch, not only the report. All eight producers route through clientFacingFailureText, each carries its measured justification at the site, and every one gained the console.warn these paths never wrote. The two objectql fixture diffs are +11/−1 and +12/−1 — consistent with "re-judged individually", not batch-re-spelled.

The card's central trap was disarmed by measurement, not by care

#8333's warning was that publishPackageDrafts' failed[].error carries authored refusals and driver text indistinguishably, so a blind sweep would blank the authoring feedback #4277 exists for. Two measured facts made the conversion safe rather than lucky:

  • P8's authored population never enters its catch. The real materializer reports refusals by returning{success: false, error}; only driver faults throw. And the rule is still applied uniformly — "a third-party materializer that throws a declared 4xx still keeps its sentence" — rather than special-cased around the one implementation that happens to exist.
  • P9 was the one site the rule could not be applied to, and it was fixed at the producer..parse().safeParse(), with seedRequestValidationError minting a real ADR-0112 envelope (422 / INVALID_METADATA / structured issues) through the same zodIssuesToMetadataIssues every other authoring surface uses. That satisfies declaresClientRefusal on its own merits, so the collector needed no special case — exactly the direction [finding] metadata-protocol interpolates raw driver text into client-facing messages — three downstream sanitizers each have a hole because of it (option C of #8086) #8136 set. ⛔ Loosening the rule at the collector would have turned a positive list into a guess and re-admitted the driver text.

The author is also strictly better off there: that field used to carry a stringified ZodError — a multi-line dump of zod internals — and now carries a curated, path-pointing summary.

One thing the card did not name, found by reading the code

P13's old fallback was String(e), which is itself a disclosure — String(err) renders as Error: SQLITE_ERROR: …. So that site had two leak paths, e?.message and the fallback, and both are closed. The card enumerated the first only.

The verification design is the strongest part

Two ablations testing different properties. The first predicted 16 red / 10 green and measured 17 — and the extra was kept rather than tidied, with the correct reading: P7's log case asserts both halves and the leak-check runs first, so it is evidence for the withhold rather than a guard on the logging. The second forces declaresClientRefusal to return false unconditionally, turning the guard sections red — and its lone survivor is "P8 leaves a RETURNED authored refusal alone", correctly green because that string never passes through the predicate. That run therefore doubles as proof of the PR's own central measured claim, which is a better argument than any assertion about P8 could have been.

The mandatory positive control is real, not synthesized: a flow draft with genuinely broken CEL still answers with the offending draft, the offending field path, code, and the structured issues the Studio form highlights with.

Also correct:code and issues left alone at P6 with the reason stated (a closed union under carryCatalogedErrorCode, filed as #8441); P7's prefix unchanged byte-for-byte because pins read it; check:engine-double-contract shows the new doubles pinned, not ledgered; merged after #7748 landed with measurements re-run, and no revertCommit conflict, so the adjacency warning held.

Residues, all filed and triaged:#8441 (bug+security — the sibling code limb still ships SQLITE_ERROR, measured on the fixed branch), #8442 (bug+securityseed-loader.ts interpolates driver text into errors[].message on the same 200, reproduced), #8443 (finding — a copy of P9's disclosure in packages/runtime, read from source and not reproduced; labelled finding rather than bug precisely because the report said so).

Marking ready and enqueueing. Fixes #8333 closes the card on merge.


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/xlteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[finding] metadata-protocol's batch verbs still put caught error text on client-facing payloads — the 8 producers option C did not reach

2 participants

@os-zhuang@claude