Skip to content

Commit fc71b84

Browse files
os-zhuangclaude
andauthored
fix(service-package): classify a publish driver fault as 5xx and stop returning driver text as caller data (#8131) (#8277)
* fix(service-package): classify a publish driver fault as 5xx and stop returning driver text (#8131) `POST /api/v1/packages/publish` answered `400 PACKAGE_PUBLISH_FAILED` with the raw driver message whenever the `INSERT INTO sys_packages` statement failed. Reproduced on a real SQLite engine before changing anything: 400 {"code":"PACKAGE_PUBLISH_FAILED","message":"no such table: sys_packages"} 400 {"code":"PACKAGE_PUBLISH_FAILED", "message":"NOT NULL constraint failed: sys_packages.tenant_ref"} Two independent defects: a server fault labelled a client error, and a driver dump handed back as caller-visible data. Fixed at the producer. `publish` no longer returns `(error as Error).message`; it returns a discriminated `driverFault` carrying a stable, non-interpolated sentence, and re-throws a refusal that declares its own status so the door's existing mapping answers it with that status and code. The door maps a returned driver fault to 500. The producer half is load-bearing, and measured to be: the 5xx withhold lives in `sendThrownError`, which a RETURNED failure never reaches at any status, and `looksLikeInternalErrorLeak('no such table: sys_packages')` is false — so reclassifying alone would have left the driver line on the wire. The discriminant is the STATUS channel only. Accepting a string `code` as a declaration was tried and reverted: every SQL driver populates it (ERR_SQLITE_ERROR, 42P01, ER_NO_SUCH_TABLE), so it re-threw genuine driver faults into a 500 whose message the heuristic does not withhold. Caller-facing 4xx is untouched, per the card's binding scope guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk * docs(changeset): state the ADR-0087 disposition without a ledger-style FROM/TO block (#8131) The ledger's subject is metadata; this change touches a runtime TS interface only. The consumer guidance stays — it is now prose plus the compiler error, which is the channel that actually reaches a caller of this service. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk * test(rest): invert the leak-predicate pins that #8132 turned red, and re-prove the fix without them (#8131) CI went red on three cases in this PR's own new suite, all reading `expected true to be false`. Cause: #8132 (f598aa8) landed on main after this branch point and taught `looksLikeInternalErrorLeak` the bare-SQLite and Postgres phrasings, so `no such table: sys_packages` is now recognised. The pins asserted it was NOT. That is the outcome those cases were written to signal, and their own instruction — and #8136's — was "delete or invert; do not repair to green". Inverted, not silently flipped: the prose that claimed a gap is rewritten, because the gap is closed. What this does NOT change is why the producer fix exists. That argument had two halves and only the second is retired: - structural (still true, and now the sole reason): the withhold lives in `sendThrownError`; a RETURNED failure reaches `sendError`, which consults no predicate at any status; - semantic (retired by #8132): the phrasing used to trip nothing. Re-measured against the WIDENED predicate, main's producer with only the status corrected to 500 still answers `500 {"code":"PACKAGE_PUBLISH_FAILED","message":"no such table: sys_packages"}` — the driver line on the wire while a predicate that recognises it perfectly is never asked. A new case pins exactly that, so nobody concludes #8132 made this fix redundant. Docblocks, the producer comment and the changeset are corrected to match; no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent a7e94e9 commit fc71b84

6 files changed

Lines changed: 1044 additions & 6 deletions

File tree

.changeset/lucky-schools-smash.md

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
---
2+
'@objectstack/service-package': minor
3+
'@objectstack/rest': patch
4+
---
5+
6+
Package publish: a driver fault is answered as a server error, and its driver text no longer reaches the caller
7+
8+
`POST /api/v1/packages/publish` answered **`400 PACKAGE_PUBLISH_FAILED`** when the
9+
`INSERT INTO sys_packages` statement itself failed, carrying the driver's own message as
10+
the caller-facing text. Measured on a real SQLite engine, that was literally:
11+
12+
```
13+
400 {"success":false,"error":{"code":"PACKAGE_PUBLISH_FAILED",
14+
"message":"no such table: sys_packages"}}
15+
400 {"success":false,"error":{"code":"PACKAGE_PUBLISH_FAILED",
16+
"message":"NOT NULL constraint failed: sys_packages.tenant_ref"}}
17+
```
18+
19+
Two defects in one line. The **status** was a client error for a fault the client had no
20+
part in — the mirror of the mislabelling fixed for the throw path, and it hid a real
21+
server fault from every dashboard that buckets by status. The **message** was raw driver
22+
text: a constraint dump naming physical tables and columns.
23+
24+
Fixed at the producer, which is the only place that closes it. A 5xx message withhold
25+
already exists at this door, but it is applied when an error is *thrown*, and this
26+
failure was *returned* — so it never met the withhold at any status. Reclassifying alone
27+
would have moved the driver line from a 400 to a 500 and left it on the wire; that is
28+
measured, and it stays true against the widened leak predicate that now recognises this
29+
phrasing, because nothing on the returned path ever consults one.
30+
31+
Now the driver's text goes to the log and nowhere else — it was already logged, so nothing
32+
an operator sees changes — and the caller gets a stable sentence that names what happened
33+
without quoting the driver.
34+
35+
**Caller-facing 4xx messages are unchanged.** A missing manifest, an invalid manifest, and
36+
any coded refusal thrown from below `publish` all keep their own status, code and
37+
self-correcting message — a `409 DESTRUCTIVE_CHANGE` is still a 409.
38+
39+
**BREAKING — the `PackageService.publish` return shape.** A bare `error` string could not
40+
say which side was at fault, so the door had one status for both and picked the wrong one.
41+
`publish` now reports a broken write as `{ success: false, driverFault: { message } }`;
42+
the `error` field is removed. If you only *call* `publish`, read
43+
`result.driverFault?.message` where you read `result.error`. If you *implement*
44+
`PackageService`, report a broken write through `driverFault` with a message safe to show
45+
a caller, and **throw** — rather than return — a refusal that carries its own `status`, so
46+
the door answers it with that status and code.
47+
48+
<!-- adr-0087: not-required (no-migration-prescription) This change retires no authorable key and adds none. `PackageService` is a runtime TypeScript service interface in `packages/services/service-package`; it has no Zod schema, no `packages/spec` declaration, no metadata type and no stored representation. `packages/spec` is untouched by this PR. Nothing exists for `objectstack migrate meta` to rewrite, because nothing an author writes and nothing persisted in `sys_metadata` or `sys_packages` changes shape — the wire envelope is unchanged too (still ADR-0112 `{ success, error: { code, message } }`), and only the STATUS a driver fault selects and the TEXT the producer puts in it move. Nor is there a FROM/TO rule a ledger entry could state: the ledger's subject is metadata, and the only readers affected here are TypeScript callers of one in-process service — measured as three in-repo consumers (`@objectstack/rest`, `@objectstack/runtime`, `@objectstack/cli`), of which exactly one, `packages/rest/src/package-routes.ts`, reads the changed field. The channel that reaches an affected reader is strictly more precise than any ledger line: the compiler itself. Reading the removed field is a hard type error at the call site — verified by reinstating it, which fails as `error TS2339: Property 'error' does not exist on type 'PackagePublishResult'` — so no consumer can carry this change silently, and this changeset's CHANGELOG text carries the one-line repair for the reader who hits it. -->

packages/rest/src/package-envelope.conformance.test.ts

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -236,11 +236,44 @@ describe('packages envelope (#3843) — error bodies', () => {
236236
run: () => drive(mount({}), 'POST', `${PKGS}/publish`, { body: { manifest: {}, metadata: {} } }),
237237
},
238238
{
239-
name: 'a publish the service refuses',
240-
status: 400,
239+
// [#8131] Was `a publish the service refuses`, driving
240+
// `{ success: false, error: 'version already published' }` against a
241+
// 400. That fixture pinned the exact limb #8131 removed — a bare
242+
// `error` string, which could not say whether the caller or the write
243+
// was at fault, so the door answered one status for both and picked the
244+
// client's. Replaced rather than re-spelled: the two outcomes it
245+
// conflated are now separate cases, here and below.
246+
name: 'a publish whose WRITE broke — a driver fault, so a 5xx',
247+
status: 500,
241248
code: 'PACKAGE_PUBLISH_FAILED',
242249
run: () => drive(
243-
mount({ publish: async () => ({ success: false, error: 'version already published' }) }),
250+
mount({
251+
publish: async () => ({
252+
success: false,
253+
driverFault: { message: 'The package registry could not store this package.' },
254+
}),
255+
}),
256+
'POST',
257+
`${PKGS}/publish`,
258+
{ body: { manifest: MANIFEST, metadata: {} } },
259+
),
260+
},
261+
{
262+
// The other half of what the old fixture named: a genuine REFUSAL still
263+
// answers 4xx, with the producer's own code — it is thrown, not
264+
// returned, and #8016's mapping reads it off the throw.
265+
name: 'a publish the service REFUSES keeps its own 4xx and code',
266+
status: 409,
267+
code: 'RESOURCE_CONFLICT',
268+
run: () => drive(
269+
mount({
270+
publish: async () => {
271+
throw Object.assign(new Error('com.acme.crm@1.0.0 is already published.'), {
272+
status: 409,
273+
code: 'RESOURCE_CONFLICT',
274+
});
275+
},
276+
}),
244277
'POST',
245278
`${PKGS}/publish`,
246279
{ body: { manifest: MANIFEST, metadata: {} } },

0 commit comments

Comments
 (0)