test(spec): envelopeViolations — the conformance check BaseResponseSchema cannot express - #4090
Merged
Merged
Conversation
…hema cannot express Every conformance suite from the #3843 line leads with `BaseResponseSchema.safeParse(body)` under a comment calling it "the contract itself, imported — not a restatement of it". That overclaimed, and the gap is demonstrable: BaseResponseSchema.safeParse({ success: true }) // passes BaseResponseSchema.safeParse({ success: true, data: link, link }) // passes The schema declares no `data` (each response type adds its own via `.extend({ data })`) and a plain z.object strips unknown keys rather than rejecting them. So it catches the one drift it was added for — a missing or non-boolean `success`, the flag `unwrapResponse` keys on — and nothing else. The second body is exactly the duplicate-payload drift /share-links shipped until #4038/#4049 removed it, and safeParse passed it the whole time. What was actually catching that drift was the assertions each suite hand-wrote beside it, which works only for as long as whoever writes the next suite remembers to. `envelopeViolations(body)` returns every departure as readable reasons, empty when conformant: `success` must be a boolean; a success body must carry `data` (`undefined` only — null/[]/{}/0/'' are payloads, not absences); a failure body must carry `error` with a string code and message; and no top-level key outside success/data/error/meta, which is the general form of the duplicate-payload drift. It deliberately does NOT check the shape of `data` — that is each route's own payload schema, and conflating the two is what let SettingsNamespacePayload describe a whole body before #3843 and only `data` after it. Ten suites now assert it beside safeParse, and their comments say what each of the two actually proves. Verified the pairing is load-bearing: reintroducing the /share-links duplicate key fails the new assertion and still passes the old one. Verified the rules are not too strict by running every existing suite unchanged — spec 6951, rest 505, runtime 914, storage 220, settings 189, datasource 154, i18n 62, sharing 225, client 200, all green, nothing flagged. Placed in contract.zod.ts beside the schema it completes, alongside the other plain predicates spec/api already exports (standardErrorCodeForHttpStatus, readServiceSelfInfo). No new package. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 5 package(s): 116 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
`check:api-surface` is a ratchet on `@objectstack/spec`'s public exports, and the
new predicate is an addition to it:
./api
+ envelopeViolations (function)
0 breaking (removed/narrowed), 1 added.
One line, alphabetically beside the other predicates that module already exports.
Two things this caught that are worth writing down.
The gate lives in the TypeScript Type Check job, which runs TEN steps — I had
found four of them by grepping a window of lint.yml that cut off before the rest,
and ran only those. The full list is: spec tsc, check:docs, check:skill-refs,
check:react-blocks, a workspace build, examples typecheck, downstream-contract
typecheck, check:api-surface, check:skill-examples, and the two i18n checks. All
ten pass locally now.
And `gen:api-surface` reads `dist/index.d.ts`, so it fails outright under
`OS_SKIP_DTS=1` — the flag I had been using to keep local builds fast. Rebuilding
spec with declarations is a prerequisite for regenerating the snapshot, not an
optional speed-up.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z
Contributor
Author
|
TypeScript Type Check went red on The gate is a ratchet on Correctly classified — additive, nothing removed or narrowed. Regenerated with Two things worth recording, because both are the kind of trap that reads as "my change broke something" when it isn't. The job runs ten steps, not the four I ran. I found the gates by grepping a window of
All ten pass locally now. This is the same failure mode as #3972, where I missed
Nothing about the change itself moved — the predicate, the suites, and the comments are as reviewed. Generated by Claude Code |
os-zhuang
marked this pull request as ready for review
July 30, 2026 09:46
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
#4090 follow-up) (#4105) * test(runtime): the dispatcher envelope check uses the shared predicate (#4090 follow-up) `domain-handler-registry.test.ts` hand-rolled "no key beside the envelope's own may hold the payload" for #4038. #4090 extracted that rule into `envelopeViolations` so it stops having two definitions; this is the caller that motivated the extraction, now using it. Leaving the local copy would have recreated the exact failure this line has been closing — one rule, two places, and only one of them updated next time. It also was not the same rule: the local set allowed any body whose top-level keys were success/data/meta, so it passed a success body with NO `data` at all, and one carrying an `error` alongside `success: true`. The shared predicate rejects both. Verified the swap is a strict improvement rather than a refactor, by breaking the producer two ways and checking the suite catches each: { success: true, data: link, link } → caught (the old check caught this too) { success: true } → caught (the old check PASSED this) runtime 914 passed. All ten gates in the TypeScript Type Check job pass, plus the six `check:*` guards. Note for the next person touching spec: after restarting a branch onto a newer main, `check:api-surface` and `check:i18n-coverage` both fail against a stale `packages/spec/dist` — the first reports other people's exports as added/removed, the second rejects an example config for a chart type the fresh spec allows. Neither is a real failure; rebuilding spec clears both. They read as "my change broke something" and are not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z * chore: add the empty changeset this PR needs Check Changeset requires one on every PR; an empty-frontmatter changeset is the sanctioned form for a change that releases nothing, and the gate's own error message says so. Second time this session — #4009 was a comment-only PR that failed the same way. I fixed that one and did not draw the rule, which is that "this releases nothing" is a reason to write an EMPTY changeset, not a reason to skip the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes a gap in my own prior work on the #3843 line: the assertion those suites lead with proves less than their comments claim.
The overclaim
Every conformance suite opens with this, under a comment I wrote in #3972 and reused since:
It isn't the contract. Demonstrably:
BaseResponseSchemais{ success, error?, meta? }— it declares nodata(each response type adds its own via.extend({ data })), and a plainz.objectstrips unknown keys rather than rejecting them.So it catches the one drift it was added for — a missing or non-boolean
success, the flagunwrapResponsekeys on, which is what #3675 / #3689 / #3843 were about — and nothing else. That second body is precisely the duplicate-payload drift/share-linksshipped until #4038 / #4049 removed it, andsafeParsepassed it the entire time.What was actually catching it: assertions each suite hand-wrote alongside (
deadTopLevel,expectData, theENVELOPE_KEYSloop I wrote once indomain-handler-registry.test.ts). That works exactly as long as whoever writes the next suite remembers to.envelopeViolations(body)Returns every departure as readable reasons; empty means conformant.
successmust be a booleanunwrapResponsekeys on; without it a body is handed to callers rawdataundefinedonly —null,[],{},0,''are payloads, not absenceserrorwith stringcode+messagesuccess/data/error/metaIt deliberately does not check the shape of
data. That's each route's own payload schema, and conflating the two is what letSettingsNamespacePayloaddescribe a whole body before #3843 and onlydataafter it.Placed in
contract.zod.tsbeside the schema it completes, alongside the plain predicatesspec/apialready exports (standardErrorCodeForHttpStatus,readServiceSelfInfo). No new package — andBaseResponseSchemaitself is untouched, because addingdataor.strict()would hit every.extend()caller and every producer for no gain over fixing this in the assertions.Verification
Two directions, because either alone would be misleading.
The pairing is load-bearing. I reintroduced the exact
/share-linksdrift ({ success: true, data, link: data }). The new assertion fails withstray top-level keylink— the payload belongs underdata``; the oldsafeParseline still passes. That contrast is the argument for the change.The rules aren't too strict. Every existing suite runs unchanged and green — nothing legitimate is flagged:
@objectstack/spec@objectstack/rest@objectstack/runtime@objectstack/clienttsc --noEmit·check:docs·check:skill-refs·check:react-blockscheck:route-envelope·check:error-code-casing·check:console-sha·check:nul-bytes·check:role-word·check:org-identifierpnpm lintTwo local-artifact notes, both of which look like failures and aren't:
pnpm lintreports 61 errors — all inside.cache/objectui-96ee72e85439/, the gitignored console build artifact my earlier.objectui-shabump left in this worktree. Zero in real source; CI checks out fresh and never sees it.restrun showed 8 anonymous-deny 401 failures. That's the stale-dist signature I flagged in fix(plugin-sharing)!: the share-link routes emit the declared envelope, and the last ratchet retires (#3983) #4037's method note — I'd rebuilt onlyspec, not its dependents. A fullturbo run buildcleared it (505/505). Same for the recurringruntimedatasource-autoconnectfailure: gitignored.objectstack/data/memory-driver.jsonaccumulating rows across local runs, 914/914 once removed.Generated by Claude Code