Skip to content

Commit d6f3f2f

Browse files
os-helpclaude
andauthored
fix(rest): honour a hook refusal's declared statusCode at the /api/v1/data error boundary (#7525) (#7575)
A write refused by an engine lifecycle hook that declared an explicit status came back as an opaque `500 INTERNAL_ERROR` with no `code`, while the server log held the correctly-shaped refusal. Two cases, each reproduced 2x by QA: `PATCH` a record with a `lockRecord:true` approval pending (log: `RECORD_LOCKED`) and `POST sys_approval_delegation` with a `delegator_id` the caller does not own (log: `FORBIDDEN ... statusCode 403`). The seam is `mapDataError`'s explicit-status passthrough, which opened on `typeof error.status === 'number'` and nothing else. The hooks declare their status as `statusCode`, so the refusal never entered that branch at all: it fell past every structured branch, matched no message heuristic, and left through `UNCLASSIFIED_FAULT`. #5582 widened the same branch's RANGE (4xx -> 400-599) and could not have covered this — the status was lost one question earlier. Fixed at the boundary, not at the two hooks: `status` -> `statusCode` -> default is already what every other HTTP exit in this repo reads (`errorFromThrown`, `errorResponseBase`, `endpoint-executor`, `domains/actions`, plugin-hono-server), so one thrown error answered 403 through a dispatcher route and 500 through `/api/v1/data`. The gate becomes a named `declaredHttpStatus()` asking the same 400-599 band over both spellings. `declaresServerFault`'s own `status`-only read is unchanged (#5811 ruled a DISCLOSURE rule must not depend on a spelling); the one call site inside the passthrough is handed the status just resolved, so a `{ statusCode: 5xx, code }` producer cannot ship its status with the code dropped. Claude-Session: https://claude.ai/code/session_01FGZ4X1ezVSLg8E9NvJitAE Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent e124711 commit d6f3f2f

3 files changed

Lines changed: 703 additions & 5 deletions

File tree

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
---
2+
'@objectstack/rest': patch
3+
---
4+
5+
fix(rest): a hook refusal that declares its status as `statusCode` reaches the wire with that status, not `500 INTERNAL_ERROR` (#7525)
6+
7+
**Observable behaviour change — read this if you alert or retry on `/api/v1/data` statuses.**
8+
A write refused by an engine lifecycle hook that declared an explicit status used
9+
to answer `500 INTERNAL_ERROR` with no `code`. It now answers the status the hook
10+
declared, carrying the hook's ADR-0112 `code`. Two refusals QA reproduced 2× each
11+
move from `500` to `409 RECORD_LOCKED` and `403 FORBIDDEN`. Monitoring that counted
12+
these as server faults will see a 5xx disappear and a 4xx appear, and a client
13+
retrying on 5xx will stop retrying a request that can never succeed.
14+
15+
## What was wrong
16+
17+
`mapDataError` — the error exit for the ~11 CRUD data routes, which bypass
18+
`resolveErrorResponse` entirely — opened its explicit-status passthrough on
19+
`typeof error.status === 'number'` and nothing else. An engine lifecycle hook
20+
declares its status as `statusCode`:
21+
22+
```ts
23+
// plugin-approvals/src/lifecycle-hooks.ts
24+
err.code = 'RECORD_LOCKED'; err.statusCode = 409; // a pending lockRecord approval
25+
err.code = 'FORBIDDEN'; err.statusCode = 403; // a delegation row the caller does not own
26+
```
27+
28+
so the refusal never entered that branch at all. It fell past every structured
29+
branch, matched no message heuristic, and left through `UNCLASSIFIED_FAULT` as
30+
`500 INTERNAL_ERROR` — for a deliberate, well-understood business refusal, with
31+
the correctly-shaped original sitting in the server log. The console never hit
32+
the record-lock case (the affordance is disabled while a lock is live); a direct
33+
API caller — script, integration, second-party client — got an unactionable 500.
34+
35+
**#5582 is not the fix and could not have been.** It widened this same
36+
passthrough's *range* (4xx → 400-599) for producers that declared `status`. The
37+
loss here is one question earlier: *whether* a status was declared at all.
38+
39+
## The fix, and why it is at the boundary
40+
41+
`status` → `statusCode` → default is what **every other HTTP exit in this repo**
42+
already reads — `runtime`'s `HttpDispatcher.errorFromThrown` (#3867),
43+
`dispatcher-plugin.errorResponseBase`, `endpoint-executor`, `domains/actions`,
44+
`plugin-hono-server`'s user endpoints. `mapDataError` was the single exit that
45+
read one spelling, which is why one thrown error came back as `403` through a
46+
dispatcher route and as `500` through `/api/v1/data`. The gate is now a named
47+
`declaredHttpStatus(error)` helper asking the same 400-599 band over both
48+
spellings.
49+
50+
Teaching the two approvals hooks to spell it `status` would have fixed two
51+
producers and left the boundary answering 500 for the next one — including
52+
`runtime`'s own `action-execution.ts` (`{ statusCode: 503 | 501 | 400 }`) and
53+
`metadata-protocol` (`{ statusCode: 404 }`). The hooks are unchanged.
54+
55+
## What deliberately did NOT change
56+
57+
- ⛔ **`declaresServerFault`'s own read is still `status`-only.** #5811 ruled that
58+
a *disclosure* rule must not depend on a producer's spelling, and that is
59+
untouched. This is *status resolution*, a different question, and the one call
60+
site inside the passthrough hands the predicate the status this boundary just
61+
resolved — otherwise a `{ statusCode: 5xx, code }` producer would take the 5xx
62+
arm and then be told it declared no fault, dropping its code.
63+
- **The 5xx withhold is unconditional as before.** A `statusCode`-declared 5xx
64+
gets `INTERNAL_ERROR_MESSAGE` plus its code; no producer prose crosses the
65+
boundary, and the full text still reaches the operator.
66+
- **A hook that declares NO status is unchanged** — still judged by the
67+
classifiers, still the terminal sanitised `500 INTERNAL_ERROR`. Promoting a
68+
bare `code` to a 4xx would be consumer-side leniency; that belongs with #7463,
69+
not here.
70+
- **The structured branches keep their precedence.** `OBJECT_NOT_FOUND`,
71+
`DELETE_RESTRICTED`, `VALIDATION_FAILED` and the rest still sit above the
72+
passthrough and still win, `statusCode` or not.
73+
- **`resolveErrorResponse` still reads `status` only.** It delegates to
74+
`mapDataError` for everything it does not pass through, so both doors already
75+
give one wire answer without a second copy of the two-spelling read.
76+
77+
Coverage: `rest-hook-refusal-status-passthrough.test.ts` — 26 cases, including
78+
both reported requests walked in process on the real `PATCH /data/:object/:id`
79+
and `POST /data/:object` routes. Run against unmodified `main` the file is 15/26
80+
red; three further mutations cover the remaining 11, so no case is unfalsifiable.

0 commit comments

Comments
 (0)