Skip to content

Envelope drift is not just service-storage: four more route modules emit bare bodies, two of them the pre-#3675 { error: '<string>' } #3843

Description

@os-zhuang

Found while fixing #3689 (PR #3837). That issue scoped itself to storage-routes.ts, and #3675 covered service-i18n alongside it — but neither asked whether the same drift exists elsewhere. It does, and in two modules it is the older shape, the one #3675 already declared wrong.

The survey

Counting res.json(...) call sites against the two envelope halves, on main at 88ef03e:

Module res.json calls carry success: true bare { error: '<string>' }
packages/services/service-settings/src/settings-routes.ts 5 0 0
packages/services/service-datasource/src/admin-routes.ts 12 0 3
packages/rest/src/external-datasource-routes.ts 7 0 1
packages/rest/src/package-routes.ts 16 3 3
(fixed) service-storage/src/storage-routes.ts 2 (both helpers) 0

What each one actually emits

settings-routes.ts — the shape #3675's changeset described: nested error, no success flag.

res.status(status).json({ error: { code, message, ...extra } });   // :50
await res.json({ manifests });                                     // :65  — bare success
await res.json({ values: result });                                // :118 — bare success

service-datasource/admin-routes.tserror is a string, i.e. exactly the pre-#3675 shape, and the codes are lowercase snake (relevant to #3841):

res.status(503).json({ error: 'datasource_admin_unavailable' });   // :49
res.status(400).json({ error: 'datasource_admin_error', message }); // :52 — message a SIBLING of error
res.status(404).json({ error: 'not_found' });                      // :110
res.json({ datasources });  res.json({ drivers:  });  res.json({ tables });  // bare successes

A caller reading body.error.message gets undefined here and the real message from the dispatcher — the identical asymmetry #3675 opened on.

rest/external-datasource-routes.ts — same bare-string error, plus its own private success word, the ok that #3689 just retired from storage:

res.status(503).json({ error: 'external_service_unavailable' });        // :41
res.json({ ok: results.every((r: any) => r.ok), results });            // :100

rest/package-routes.ts — partially converted, which is arguably worse than untouched: 3 of 16 bodies carry success: true, so the same module answers two shapes depending on which route you hit.

Why the ledgers and the existing suites miss it

Same reason #3675 and #3689 give: the route ledgers audit which routes exist and whether the SDK can address them, not what comes back. And the two conformance suites that do check bodies (error-envelope.conformance.test.ts, success-envelope.conformance.test.ts) are per-package — they live in service-storage and service-i18n and scan only their own module's source. Nothing generalises them.

What needs deciding

  1. Convert all four, module by module, to the sendError / sendOk helper pair storage-routes.ts now uses, with a conformance suite per module. Correct and mechanical, but it is four PRs and the { error: '<string>' }{ error: { code, message } } move is breaking for each module's consumers, exactly as it was in fix(service-storage,service-i18n): emit the declared error envelope, not a bare { error } (#3675) #3687.
  2. Convert the two { error: '<string>' } modules only (admin-routes.ts, external-datasource-routes.ts) and leave the merely-missing-success ones. Cheaper; closes the dialect that actually breaks body.error.message readers, leaves a smaller declared ≠ actual gap behind.
  3. Lift the guard instead of the code — one shared conformance harness (a package the service packages can import, or a lint rule) that fails any route module emitting a body outside BaseResponseSchema, then fix what it reports. Front-loads the work but is the only option that stops the next module from drifting.

Recommendation: 3 then 1. Three passes of the same hand-rolled suite (storage, i18n, and now four more) is the signal that the harness wants to be shared rather than copied. The scan in success-envelope.conformance.test.ts is already module-agnostic — it strips comments, counts .json( call sites, and asserts each envelope half is built in exactly one place; generalising it is mostly a matter of parameterising the path.

Worth sequencing after #3841 and #3842, since converting a bare { error: 'not_found' } means choosing a code vocabulary, and that is #3841's call.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions