Uh oh!
There was an error while loading. Please reload this page.
fix(metadata-protocol): stop interpolating raw driver text into client-facing messages (#8136) - #8334
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 1 package(s): 3 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also reference the affected code. These are read-only:
|
…oducer dispatch contract (#8136) `check:engine-double-contract` flagged the engine double in protocol.driver-text-disclosure.test.ts (line 195): its `update()` accepted call shapes `ObjectQL.update` refuses, which is how #4434 shipped a dead REST route with its suite green. The double's `delete()` was already pinned via `assertEngineDeleteDispatch`; only `update` was loose. Open the fake's `update` with `assertEngineUpdateDispatch(data, options)` from `@objectstack/metadata-core` — never `@objectstack/objectql`, which depends on this package, so that import would close a cycle turbo refuses. The file already imported the delete predicate from metadata-core, so this needs no package.json change and no lockfile churn. The shrink-only baseline ledger is untouched: this is the real pin, not an exemption. update doubles go 90 -> 91 pinned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
Uh oh!
There was an error while loading. Please reload this page.
Part of #8136
Option C of #8086, at the producer. This lands the measured half of the sweep — the uninstall/overlay-delete cluster, which is the cluster #8130 reproduced end to end — and records the full producer inventory below so the remainder is a scoped follow-up rather than an unknown. Deliberately a partial sweep, stated rather than silently narrowed; the remainder is filed as #8333.
STEP 0 — the premise, re-measured on today's
mainThe card is a day old and
protocol.tsmoved under four merges since it was written, so the premise was re-measured rather than assumed. Producers were located by reading the file, never by grepping the strings the cards quote — the producer #8086 originally named no longer exists and survives only as docblock prose.PM assumption 1 — confirmed by reading.
deletePackage's first database touch is at method-body level:There is no enclosing
try; the method's per-itemcatchwraps only thedeleteMetaItemloop below it. So a driver error propagates whole and undeclared, rather than being collected intofailed[].PM assumption 2 — the disclosure still reproduces. Measured, not inferred: see the reverse verification in STEP 3, where the unfixed producer on today's
mainputs the driver line both in the thrown message and inside thefailed[]array that rides on a 400.STEP 1 — the rule, stated once at the producer
A caught error's sentence is quoted back to a caller only when that error declared itself a client-facing refusal — a 4xx
statusin the ADR-0112 envelope. Anything undeclared (a bareErrorfrom a driver) or declared a server fault gets a stable sentence naming the operation that failed, and the original error rides oncauseso the operator's log still receives it whole.This is a positive list, not a negative heuristic. That is the whole point of option C: correctness that does not depend on having enumerated the world's SQL engines.
⛔ Not
!declaresServerFault(err). That looks equivalent and would reinstate the entire defect: a bareErrorfrom a driver declares nothing, so it fails that test and would be quoted — and a bare driverErroris precisely the measured case. The two predicates answer different halves and neither is the other's negation.⛔
looksLikeInternalErrorLeakis untouched, per the card's binding guard. The three downstream belts stay exactly as they are; what changed is that this particular producer no longer needs them.STEP 2 — the full producer inventory
Every site in
packages/metadata-protocol/src/protocol.tswhere caught error text reaches a client-facing sentence or payload. Log-only sites (console.warn) are correct as they are and are excluded — that is where the driver text is supposed to go.Fixed in this PR — the uninstall / overlay-delete cluster:
deletePackage, firstengine.find('sys_metadata')DELETE /api/v1/packages/:idbodymessagedeleteMetaItem, repository-path re-wrapdeleteMetaItem, legacy raw-engine re-wrapdeletePackagecleanups[].errorPACKAGE_DELETE_PARTIAL400, insidedetailsdeletePackagefailed[].errorP4 and P5 are the half no HTTP boundary can reach: they ride on a 400 inside
details, not the message, so #8130's 5xx withhold and #8016's mapping both pass over them. That asymmetry is the argument for fixing the producer rather than adding a fourth belt.Not fixed here — filed as #8333:
publishPackageDraftsfailed[].errormaterialize.failures[].errormaterializeApplied.error(feeds P7)errorduplicatePackagefailed[].errorrevertCommitfailed[].errorrevertToCommitfailed[].errormigrateStoredMetadatarows[].reasonTwo sites that look like members and are not: the copy path's
the flow does not canonicalizeandconversion refusedstrings interpolate authored schema-validation text, not driver text. They are left alone deliberately.STEP 3 — reverse verification, direction predicted BEFORE running
Predicted: reverting
protocol.tstoorigin/mainturns the withhold cases (sections 1, 2, 4) RED, while section 0 — which measures the shared predicate this card does not touch — and the two[GUARD]cases stay GREEN, because a declared 4xx refusal is quoted verbatim in both directions.Observed, exactly as predicted: 13 failed | 3 passed, the 3 green being section 0 and the two
[GUARD]cases.The raw driver text is visible in the failure output, which is the point — including the data-path leak that no boundary can reach:
Restored afterwards and proved byte-identical with
git hash-object(7f943bd7...before and after), not by an insertion count.The over-block bound was measured separately: with
declaresClientRefusalforced to returnfalseunconditionally — "withhold everything" — 2 failed | 14 passed, and the 2 are exactly the[GUARD]cases. That is what stops this fix being satisfied by a blanket replacement that would delete the self-correcting refusals #4277 exists for.STEP 3b — the operator half, and the second pin this change re-judged
Withholding text from the client is only half the contract; the other half is that the driver line still reaches the operator.
packages/rest/src/rest-5xx-message-sanitization.test.ts(#5437's pins) asserts exactly that, and two of its cases went red here. They were not a fixture nit and were not repaired blindly — the wire assertions in them still passed, and only theloggedText(...)half failed, which is the interesting direction.Measured with a throwaway probe at that seam rather than assumed:
So the driver line does still reach the log, on
cause; the helper simply reada.messageand never walked the chain. Disposition: the helper now traversescause, so the log guarantee keeps measuring, and the stale comment claiming "the only way this text can reach the log is if the protocol really did interpolate the driver error into the message" is corrected in place — that case's own prediction ("if the producer ever stops doing that, this goes red") came true, and it is re-pointed rather than deleted.Proved non-vacuous: with the producer's two
causeassignments stripped anddistrebuilt, those two cases go RED again.distrebuild is load-bearing and cost a false green:packages/restimports the built@objectstack/metadata-protocol, so the first strip-and-rerun came back green purely becausediststill held the fixed build. Judged before rebuilding, that would have been recorded as "the log guarantee is not pinned", which is the opposite of the truth.STEP 4 — the #8130 pin, inverted rather than repaired
#8130 left a deliberately-red-in-future case asserting that
protocol.deletePackagereally does let the driver line out. This is that day. Per the card's binding instruction the pin is inverted, not mended — making it green again would mean re-teaching the protocol to leak. Its end-to-end walk (realObjectQL, real protocol, real route) is kept; what changed is what it observes at the far end: a declared 503SERVICE_UNAVAILABLEinstead of an undeclared 500 whose prose the door had to withhold.sendThrownErrorguards every producer reaching that registrar, not just this one.Scope honoured
packages/metadata-protocol/src/**, client-facing message construction in error/catch paths only.save— publish, rollback and the 409 conflict denial never write a row #7748 is held behind this card).sys-metadata-repository.tsuntouched ([Decision]OS_METADATA_WRITABLEunlocks a write on a read-only package while Studio renders a "Read-only" badge — which one is telling the truth? #8146 in flight).metadata-diagnostics.tsuntouched ([security] GET /api/v1/meta/datasources still serves stored cleartext credentials — the metadata read path has no per-type redaction hook #8154's card).packages/restsource is touched:packages/rest/src/package-door-5xx-message-sanitization.test.ts— the card explicitly instructs this PR to invert its pin.packages/rest/src/rest-5xx-message-sanitization.test.ts— a disclosed expansion beyond the fenced region, not silent. Landing the mandated producer fix necessarily re-judges the pins that asserted the old producer behaviour, and leavingmainred was not an option. The edit is confined to a test helper and a stale comment; it preserves an existing guarantee rather than relaxing one. Flagged for the PM to confirm.looksLikeInternalErrorLeaknot widened.protocol.tshas a concurrent editor in #7654 (thegetMetaItemsread merge — disjoint region).mainwas merged twice, most recently atc797473d9, andprotocol.tswas untouched by those commits. Nothing was hand-resolved around another agent's work; the merge queue arbitrates the rest.Remainder
P6–P13 are filed as #8333 (unassigned, unlabelled for triage) rather than swept blind, for a measured reason: several of them carry authored refusals as well as driver text.
publishPackageDrafts'failed[].erroris the clearest — it ships alongsidecodeand structured spec-validationissues, and applying the 4xx rule there without first measuring which publish-path refusals actually declare astatusrisks blanking exactly the authoring feedback the[GUARD]section exists to protect. That measurement is a round of work in its own right, and doing it badly is worse than scheduling it.Verification
pnpm --filter @objectstack/metadata-protocol testprotocol.driver-text-disclosure.test.tspnpm --filter @objectstack/rest testpnpm --filter @objectstack/runtime testpnpm --filter @objectstack/types testpnpm check:cross-package-test-inputspnpm check:durability-log-levelpnpm check:filter-alias-paritypnpm check:nul-bytespnpm check:error-code-casingpnpm check:authz-resolverpnpm check:changeset-gate-self-testspnpm check:meta-type-normalizedpnpm check:objectui-changesetpnpm check:query-options-erasurenode scripts/check-changeset-no-major.mjspnpm check:type-check-debtThe consumer sweep direction is stated so it can be reviewed:
metadata-protocol's downstream consumers (the prefix...@objectstack/metadata-protocolform) are 30+ packages, which is the full farm and CI's job. The radius that actually asserts the changed strings was bounded by grep tometadata-protocol,restandtypes— all three run above, plusruntime, which owns the package-door parity pin.Five of the gate families above were not in the dispatch prompt's list — they came from re-deriving
scripts/pm/dispatch-gates.mjsagainst the actual changed paths, which is why that re-derivation step is worth its one command.Changeset included — user-visible response-message change. No new error code (
SERVICE_UNAVAILABLEis the standard catalog's own 503 code and this file already raises it for exactly this condition), no status change on the overlay-delete re-wraps, noerror.codeinvented.Generated by Claude Code