Skip to content

Commit 10f15c4

Browse files
committed
refactor(qa)!: replace the route-envelope package with a repo-wide check script (#3843 review)
Review feedback: a whole workspace package for one ~150-line pure function is over-engineering, and its name broke the repo's one naming rule. Both points stand. `@objectstack/route-envelope-conformance` was also the only package in 40+ whose directory name did not match its package name (the sole prior exception, objectstack-vscode, is a VS Code extension requirement). Replaced by `scripts/check-route-envelope.mjs` + `pnpm check:route-envelope`, wired into lint.yml beside the nine sibling `check:*` guards — which is what this repo's guard convention actually is. framework has zero custom ESLint plugins (objectui has the eslint-rules/ precedent, framework does not) and no root vitest for a rule's own tests, so the check-script form fits without inventing new infrastructure. Net: one workspace package, one build node, five devDependencies and one tsconfig removed; one script added. Going repo-wide is not just tidier — it is a stronger guard, and it proved that immediately by finding two modules #3843's hand survey missed: * plugin-sharing/share-link-routes.ts — the FIFTH drifting module. No body carries `success`, and one answers `{ ok: true }`, the private second word #3689 retired from storage. Filed #3983, pinned by the guard's ratchet. (#3675's own changeset had cited this file as an example of the good shape; that was true of its `error` object and nothing else.) * metadata/routes/hmr-routes.ts — declared EXEMPT with a reason (dev-only SSE endpoint, not on the SDK surface), not skipped. A per-package scan structurally cannot notice a module nobody thought to convert. Three states are declared deliberately — conformant / ratcheted / exempt — the honest classification ADR-0049 asks for; a module the scan finds but the table does not declare is an ERROR, never a default, since applying the 2/1/1 default to an unknown module would let a new one pass by coincidence. The scan now parses the TypeScript AST instead of matching regexes, which fixes two real bugs the three copied blocks had: 1. comments were stripped with String.replace, whose line-comment pattern also ate `//` inside string literals and truncated the rest of that line — response writes included. A guard that under-counts passes while drift ships. 2. `.json(` does not mean "write a response". hmr-routes.ts calls `c.req.json()` twice to READ a request body; a textual count reports that as two unenveloped responses. Comments and literals are not AST tokens and request-vs-response is a property of the callee, so both disappear and the 110-line tokenizer is gone. The script carries a --self-test pinning each case: the nine sibling guards have none, but both of these bugs survived a review of the regex version. The driven-body half stays in each module's suite, where it belongs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z
1 parent 5ccfc2e commit 10f15c4

25 files changed

Lines changed: 472 additions & 1076 deletions

.changeset/route-envelope-four-more-modules.md

Lines changed: 41 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -79,29 +79,46 @@ merge order: objectui's `packages` readers were already tolerant
7979

8080
## The guard is shared now, not copied
8181

82-
New private `@objectstack/route-envelope-conformance` (`packages/qa/route-envelope`)
83-
exports one pure `checkRouteEnvelope(source) => finding[]`. Its load-bearing
84-
assertion is structural rather than per-route: **count the `.json(` call sites**.
85-
When every body goes through the `sendOk` / `sendError` pair, that count is fixed
86-
at two and does not grow with the route list — so a *future* route that
87-
hand-rolls a body fails the guard, which is the coverage a driven test can never
88-
give.
82+
`scripts/check-route-envelope.mjs` + `pnpm check:route-envelope`, wired into
83+
`lint.yml` alongside the nine sibling `check:*` guards. Its load-bearing assertion
84+
is structural rather than per-route: **it counts the response write sites per
85+
module.** When every body goes through the `sendOk` / `sendError` pair that count
86+
is fixed at two and does not grow with the route list — so a *future* route that
87+
hand-rolls a body fails the guard. That is the coverage a driven-body test can
88+
never give, since it can only drive the routes that existed the day it was
89+
written.
8990

9091
This existed three times already as an open-coded regex block (storage error,
91-
storage success, i18n error); the third copy was the signal it wanted lifting
92-
(#3843 option 3, done before the conversions). All three now delegate to it, as
93-
do the four new per-module suites. The shared version also fixes a real bug in
94-
the copies: they stripped comments with `String.replace`, which also ate `//`
95-
inside string literals and silently truncated the rest of that line —
96-
`.json(` calls included. A guard that under-counts call sites passes while drift
97-
ships, which is the failure mode this issue is about. The shared scanner
98-
tokenizes properly and pins that case in its own suite.
99-
100-
**One ratchet, stated rather than hidden.** `i18n-service-plugin.ts` is declared
101-
at `jsonCallSites: 5, successBuilders: 4`. Its error half *is* consolidated
102-
(#3675), but each of its four read routes builds `{ success: true, data }`
103-
inline. Those bodies are correct — that is not envelope drift — but an
104-
unconsolidated builder is a weaker guard: a fifth read route could get the shape
105-
wrong and only a driven test would notice. The numbers pin today's structure
106-
exactly (a new inline body fails), and consolidating behind a `sendOk` drives
107-
them to the 2 / 1 the other five modules use.
92+
storage success, i18n error). Lifting it did more than deduplicate: a per-package
93+
scan **structurally cannot notice a module nobody thought to convert**, and going
94+
repo-wide found two the moment it ran — neither is in #3843's hand-written survey:
95+
96+
- `plugin-sharing/share-link-routes.ts` — the fifth drifting module. No body
97+
carries `success`, and one answers `{ ok: true }`, the private second word #3689
98+
retired from storage. Filed as #3983 and pinned by the guard; converting it is
99+
breaking for share-link consumers and needs its own sweep.
100+
- `metadata/routes/hmr-routes.ts` — declared **exempt** with a reason (dev-only
101+
SSE endpoint, not on the SDK surface), not skipped. Three states, deliberately —
102+
conformant / ratcheted / exempt — because that is the honest classification
103+
ADR-0049 asks for. A route module the scan finds but the table does not declare
104+
is an **error**, never a default: applying `2 / 1 / 1` to an unknown module would
105+
let a new one pass by coincidence.
106+
107+
It also drops the regex for the TypeScript AST, fixing two real bugs the copies
108+
had. They stripped comments with `String.replace`, whose line-comment pattern also
109+
ate `//` inside string literals and truncated the rest of that line — response
110+
writes included. And `.json(` does not mean "write a response": `hmr-routes.ts`
111+
calls `c.req.json()` twice to READ a request body, which a textual count reports as
112+
two unenveloped responses. Comments and literals are not AST tokens, and
113+
request-vs-response is a property of the callee, so both disappear. The script
114+
carries a `--self-test` pinning each case — the nine sibling guards have none, but
115+
both of these bugs survived a review of the regex version.
116+
117+
**The i18n ratchet, stated rather than hidden.** `i18n-service-plugin.ts` is
118+
declared at `responses: 5, ok: 4, err: 1` with a ratchet pointing at #3973. Its
119+
error half *is* consolidated (#3675), but each of its four read routes builds
120+
`{ success: true, data }` inline. Those bodies are correct — that is not envelope
121+
drift — but an unconsolidated builder is a weaker guard: a fifth read route could
122+
get the shape wrong and only a driven test would notice. The numbers pin today's
123+
structure exactly (a new inline body fails) and drop to the conformant `2 / 1 / 1`
124+
when #3973 lands.

.github/workflows/lint.yml

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,21 @@ jobs:
9999
- name: Single authz resolver guard
100100
run: pnpm check:authz-resolver
101101

102+
# #3843 response-envelope guard. The route ledgers audit which routes EXIST;
103+
# nothing audited what comes back, so six route modules emitted bodies
104+
# outside BaseResponseSchema while carrying green `sdk` rows. Counts the
105+
# response write sites per module via the TS AST: when every body goes
106+
# through the module's sendOk/sendError pair that count is fixed, so a NEW
107+
# route hand-rolling a body fails here — coverage a driven test cannot give.
108+
#
109+
# Repo-wide on purpose. The three predecessors were per-package, which is
110+
# structurally unable to notice a module nobody thought to convert; both
111+
# ratcheted modules in the script's table were found by this scan and are
112+
# absent from #3843's hand survey. An undeclared route module is an error,
113+
# never a default. Runs its own --self-test first.
114+
- name: Response-envelope guard
115+
run: pnpm check:route-envelope
116+
102117
# Release-notes drift guard: the platform is one version-locked train, so
103118
# every released @objectstack/spec major must have a curated, navigable
104119
# release page at content/docs/releases/v<major>.mdx. Catches the gap that

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
"check:role-word": "node scripts/check-role-word.mjs",
3535
"check:org-identifier": "node scripts/check-org-identifier.mjs",
3636
"check:authz-resolver": "node scripts/check-single-authz-resolver.mjs",
37+
"check:route-envelope": "node scripts/check-route-envelope.mjs --self-test && node scripts/check-route-envelope.mjs",
3738
"check:console-sha": "node scripts/check-console-sha.mjs",
3839
"check:release-notes": "node scripts/check-release-notes.mjs",
3940
"check:node-version": "node scripts/check-node-version.mjs"

packages/qa/route-envelope/LICENSE

Lines changed: 0 additions & 202 deletions
This file was deleted.

0 commit comments

Comments
 (0)