diff --git a/.changeset/i18n-consolidate-success-builder.md b/.changeset/i18n-consolidate-success-builder.md new file mode 100644 index 0000000000..3c6823c2d0 --- /dev/null +++ b/.changeset/i18n-consolidate-success-builder.md @@ -0,0 +1,27 @@ +--- +--- + +Consolidate `service-i18n`'s four inline success builders behind a `sendOk`, and +retire the route-envelope ratchet that pinned them (#3973 option 1). Deliberately +empty frontmatter: **no wire shape changes**, so there is nothing for a consumer to +read in a CHANGELOG. + +#3636 put the declared `{ success: true, data }` envelope on all three i18n read +routes, but built it inline in each — four call sites for one envelope half, while +the error half had already been consolidated behind `sendError` (#3675). Those +bodies were never wrong; the problem was the guard. + +`scripts/check-route-envelope.mjs` counts response write sites per module, so a +consolidated module sits at a fixed two however many routes it grows. This one sat +at five with a declared ratchet, because a fifth read route could have hand-rolled +a fourth-dialect body and only a driven test would have caught it — and a driven +test only covers the routes that existed when it was written. The four builders now +collapse into one, and the module declares the same `2 / 1 / 1` as the other five. + +Guard: **6 conformant / 1 ratcheted / 1 exempt**, down from 5 / 2 / 1. The remaining +ratchet is `share-link-routes.ts` (#3983), which needs its own consumer sweep. + +Also corrects a claim #3843 left in six suite headers: it said the repo-wide scan +"found two immediately (#3973, #3983)". #3973 was not one of them — i18n's +unconsolidated builder was already known — the two it actually surfaced were +`share-link-routes.ts` and the dev-only `hmr-routes.ts`. diff --git a/packages/rest/src/external-datasource-envelope.conformance.test.ts b/packages/rest/src/external-datasource-envelope.conformance.test.ts index cfd78cb9fc..cd222f62a6 100644 --- a/packages/rest/src/external-datasource-envelope.conformance.test.ts +++ b/packages/rest/src/external-datasource-envelope.conformance.test.ts @@ -25,7 +25,8 @@ * `pnpm check:route-envelope` in CI. It sits outside any package on purpose: the * three predecessors of that scan were per-package, which structurally cannot * notice a route module nobody thought to convert, and two such modules turned up - * the moment it went repo-wide (#3973, #3983). + * the moment it went repo-wide: `share-link-routes.ts` (#3983) and the dev-only + * `hmr-routes.ts`, neither of them in #3843's hand-written survey. * * What stays here is the half that has to live next to the routes it drives: * every branch driven, every body parsed against the real spec schemas. diff --git a/packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts b/packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts index 0dc007de68..cd75bf5aea 100644 --- a/packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts +++ b/packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts @@ -27,7 +27,8 @@ * `pnpm check:route-envelope` in CI. It sits outside any package on purpose: the * three predecessors of that scan were per-package, which structurally cannot * notice a route module nobody thought to convert, and two such modules turned up - * the moment it went repo-wide (#3973, #3983). + * the moment it went repo-wide: `share-link-routes.ts` (#3983) and the dev-only + * `hmr-routes.ts`, neither of them in #3843's hand-written survey. * * What stays here is the half that has to live next to the routes it drives: * every branch driven, every body parsed against the real spec schemas. diff --git a/packages/services/service-i18n/src/error-envelope.conformance.test.ts b/packages/services/service-i18n/src/error-envelope.conformance.test.ts index c00d81ba5c..c2df1d13eb 100644 --- a/packages/services/service-i18n/src/error-envelope.conformance.test.ts +++ b/packages/services/service-i18n/src/error-envelope.conformance.test.ts @@ -22,7 +22,8 @@ * * That move mattered more than deduplication. A per-package scan structurally * cannot notice a module nobody thought to convert, and going repo-wide found two - * immediately (#3973, #3983). It also dropped the regex: the old block stripped + * immediately — `share-link-routes.ts` (#3983) and the dev-only `hmr-routes.ts`, + * neither of them in #3843's hand-written survey. It also dropped the regex: the old block stripped * comments with `String.replace`, which ate `//` inside string literals and * truncated the rest of that line — response writes included — and counted * `c.req.json()` (a request READ) as an unenveloped response. The AST has neither diff --git a/packages/services/service-i18n/src/i18n-service-plugin.ts b/packages/services/service-i18n/src/i18n-service-plugin.ts index 2d077b15b9..aa0b6dacb7 100644 --- a/packages/services/service-i18n/src/i18n-service-plugin.ts +++ b/packages/services/service-i18n/src/i18n-service-plugin.ts @@ -30,6 +30,27 @@ function sendError(res: IHttpResponse, status: number, code: string, message: st res.status(status).json({ success: false, error: { code, message } }); } +/** + * Emit a success body in the DECLARED envelope — `BaseResponseSchema` + * (`packages/spec/src/api/contract.zod.ts`), i.e. `{ success: true, data }`. + * + * #3636 put the right shape on all three read routes, but built it inline in each + * one — four call sites for one envelope half, while the error half had already + * been consolidated behind `sendError` (#3675). Those bodies were never wrong; + * this is about the GUARD, not the wire. + * + * `scripts/check-route-envelope.mjs` counts response write sites per module, so a + * consolidated module sits at a fixed two no matter how many routes it grows. This + * one sat at five with a declared ratchet (#3973) precisely because a fifth read + * route could have hand-rolled a fourth-dialect body and only a driven test would + * have caught it — and a driven test only covers the routes that existed when it + * was written. Collapsing the four inline builders into this one retires that + * ratchet: the module now declares the same `2 / 1 / 1` as the other five. + */ +function sendOk(res: IHttpResponse, data: unknown): void { + res.json({ success: true, data }); +} + /** * Configuration options for the I18nServicePlugin. */ @@ -187,7 +208,7 @@ export class I18nServicePlugin implements Plugin { // copy is what let them answer in different shapes (#3833's lesson, // one route over). const locales = toLocaleDescriptors(i18n.getLocales(), i18n.getDefaultLocale?.() ?? 'en'); - res.json({ success: true, data: { locales } }); + sendOk(res, { locales }); } catch (error: any) { sendError(res, 500, 'INTERNAL', error?.message ?? 'Internal error'); } @@ -202,7 +223,7 @@ export class I18nServicePlugin implements Plugin { return; } const translations = i18n.getTranslations(locale); - res.json({ success: true, data: { locale, translations } }); + sendOk(res, { locale, translations }); } catch (error: any) { sendError(res, 500, 'INTERNAL', error?.message ?? 'Internal error'); } @@ -223,7 +244,7 @@ export class I18nServicePlugin implements Plugin { if (hasGetFieldLabels) { const labels = (i18n as II18nService & { getFieldLabels(obj: string, loc: string): Record }) .getFieldLabels(objectName, locale); - res.json({ success: true, data: { object: objectName, locale, labels } }); + sendOk(res, { object: objectName, locale, labels }); } else { // Fallback: read field labels out of the locale's translation data. // That data is NESTED (`objects..fields..label`) — the @@ -234,7 +255,7 @@ export class I18nServicePlugin implements Plugin { // drift out of shape again the way it did after that fix (#3833). const data = i18n.getTranslations(locale) as TranslationData | undefined; const labels = resolveObjectFieldLabels(data, objectName); - res.json({ success: true, data: { object: objectName, locale, labels } }); + sendOk(res, { object: objectName, locale, labels }); } } catch (error: any) { sendError(res, 500, 'INTERNAL', error?.message ?? 'Internal error'); diff --git a/packages/services/service-settings/src/envelope.conformance.test.ts b/packages/services/service-settings/src/envelope.conformance.test.ts index bbf5e1f20e..22f6a93eab 100644 --- a/packages/services/service-settings/src/envelope.conformance.test.ts +++ b/packages/services/service-settings/src/envelope.conformance.test.ts @@ -24,7 +24,8 @@ * `pnpm check:route-envelope` in CI. It sits outside any package on purpose: the * three predecessors of that scan were per-package, which structurally cannot * notice a route module nobody thought to convert, and two such modules turned up - * the moment it went repo-wide (#3973, #3983). + * the moment it went repo-wide: `share-link-routes.ts` (#3983) and the dev-only + * `hmr-routes.ts`, neither of them in #3843's hand-written survey. * * What stays here is the half that has to live next to the routes it drives: * every branch driven, every body parsed against the real spec schemas. diff --git a/packages/services/service-storage/src/error-envelope.conformance.test.ts b/packages/services/service-storage/src/error-envelope.conformance.test.ts index fb814a01d0..dba6db01d7 100644 --- a/packages/services/service-storage/src/error-envelope.conformance.test.ts +++ b/packages/services/service-storage/src/error-envelope.conformance.test.ts @@ -26,7 +26,8 @@ * * That move mattered more than deduplication. A per-package scan structurally * cannot notice a module nobody thought to convert, and going repo-wide found two - * immediately (#3973, #3983). It also dropped the regex: the old block stripped + * immediately — `share-link-routes.ts` (#3983) and the dev-only `hmr-routes.ts`, + * neither of them in #3843's hand-written survey. It also dropped the regex: the old block stripped * comments with `String.replace`, which ate `//` inside string literals and * truncated the rest of that line — response writes included — and counted * `c.req.json()` (a request READ) as an unenveloped response. The AST has neither diff --git a/packages/services/service-storage/src/success-envelope.conformance.test.ts b/packages/services/service-storage/src/success-envelope.conformance.test.ts index deb64f6d8b..699e127fad 100644 --- a/packages/services/service-storage/src/success-envelope.conformance.test.ts +++ b/packages/services/service-storage/src/success-envelope.conformance.test.ts @@ -43,7 +43,8 @@ * * That move mattered more than deduplication. A per-package scan structurally * cannot notice a module nobody thought to convert, and going repo-wide found two - * immediately (#3973, #3983). It also dropped the regex: the old block stripped + * immediately — `share-link-routes.ts` (#3983) and the dev-only `hmr-routes.ts`, + * neither of them in #3843's hand-written survey. It also dropped the regex: the old block stripped * comments with `String.replace`, which ate `//` inside string literals and * truncated the rest of that line — response writes included — and counted * `c.req.json()` (a request READ) as an unenveloped response. The AST has neither diff --git a/scripts/check-route-envelope.mjs b/scripts/check-route-envelope.mjs index 9db9f022ad..f783f7fe24 100644 --- a/scripts/check-route-envelope.mjs +++ b/scripts/check-route-envelope.mjs @@ -30,9 +30,10 @@ * #3675 / #3689 shipped this as a regex block copied into each converted * package. Three copies was the signal it wanted lifting (#3843 option 3), and * lifting it to a repo-wide scan buys the thing per-package copies structurally - * cannot: **a module nobody thought to convert still gets audited.** Both - * modules in the RATCHET table below were found that way — neither is in - * #3843's hand-written survey. + * cannot: **a module nobody thought to convert still gets audited.** Two modules + * in the table below were found exactly that way, neither of them in #3843's + * hand-written survey — `share-link-routes.ts` (ratcheted, #3983) and + * `hmr-routes.ts` (exempt). * * A module discovered by the scan but absent from the table is an ERROR, not a * default: silently applying `2 / 1 / 1` to an unknown module would let a new @@ -97,6 +98,10 @@ const MODULES = { 'packages/services/service-datasource/src/admin-routes.ts': { responses: 2, ok: 1, err: 1 }, 'packages/rest/src/external-datasource-routes.ts': { responses: 2, ok: 1, err: 1 }, 'packages/rest/src/package-routes.ts': { responses: 2, ok: 1, err: 1 }, + // Consolidated by #3973: #3636 put the right envelope on its three read routes + // but built it inline in four places, so this module carried a ratchet at 5 / 4 / 1 + // until those collapsed behind a `sendOk`. + 'packages/services/service-i18n/src/i18n-service-plugin.ts': { responses: 2, ok: 1, err: 1 }, // ── Exempt ────────────────────────────────────────────────────────────── @@ -114,17 +119,7 @@ const MODULES = { exempt: 'dev-only SSE endpoint (/api/v1/dev/*), not on the SDK surface', }, - // ── Ratchets: real, tracked, NOT blessed ──────────────────────────────── - - // The error half IS consolidated behind `sendError` (#3675). The success half - // is not: each of four read routes builds `{ success: true, data }` inline. - // Those bodies are CORRECT — `packages/runtime/src/i18n-success-envelope - // .conformance.test.ts` drives them — so this is not envelope drift. It is an - // unconsolidated builder, i.e. a weaker guard: a fifth read route could get - // the shape wrong and only a driven test would notice. - 'packages/services/service-i18n/src/i18n-service-plugin.ts': { - responses: 5, ok: 4, err: 1, ratchet: '#3973', - }, + // ── Ratchet: real, tracked, NOT blessed ───────────────────────────────── // Found BY THIS SCAN, absent from #3843's survey — the fifth drifting module. // `sendError` already nests `{ code, message }` (that is why #3675's changeset