fix(rest): classify record-share and analytics refusals at the shared /data door

.changeset/olive-donkeys-repeat.md

@@ -0,0 +1,45 @@
+---
+'@objectstack/rest': patch
+---
+
+fix(rest): classify a refusal by what the producer declared, not by how its message starts
+
+The three record-share routes and `POST /api/v1/analytics/dataset/query` each
+built their error answer by hand and shared no branch with the door every
+`/data` face reports through, so one refusal got a different wire answer
+depending on which route caught it. Both now ask that door first, through a
+single new seam (`classifiedRefusalAnswer`), for a refusal the **producer
+classified** — a declared 4xx `status`/`statusCode` **plus** a `code`, or a
+sandboxed hook body's business `throw`. Everything else is untouched.
+
+**Per route, old answer → new answer.** Check your error handling if you branch
+on any of these.
+
+`GET /api/v1/data/:object/:id/shares`, `POST` the same path, and
+`DELETE /api/v1/data/:object/:id/shares/:shareId`:
+
+| the thrown refusal | was | is now |
+| :--- | :--- | :--- |
+| `{ code: 'RECORD_LOCKED', status: 409 }` (any code outside the five prefixes) | `500` `SHARES_LIST_FAILED` / `SHARE_GRANT_FAILED` / `SHARE_REVOKE_FAILED` | `409` `RECORD_LOCKED` |
+| `{ code: 'FORBIDDEN', status: 403 }` — `plugin-sharing`'s own write gate | `500` `SHARE_*_FAILED` | `403` `FORBIDDEN` |
+| the same declared as `statusCode` rather than `status` | `500` `SHARE_*_FAILED` | the declared status + code |
+| a sandboxed hook refusal, no status declared | `500` `SHARE_*_FAILED`, message = the QuickJS wrapper `hook '<name>' threw: Error: <text>` | `400` `VALIDATION_ERROR`, message = the hook's own sentence |
+| a sandboxed hook body that CRASHED | `500` `SHARE_*_FAILED`, message = the wrapper around `TypeError: …` | `500` `SHARE_*_FAILED`, message = `Internal server error` |
+| `VALIDATION_FAILED:` / `PERMISSION_DENIED:` / `NOT_FOUND:` / `CONFLICT:` / `SHARING_NOT_ENABLED:` prefixed messages | 400 / 403 / 404 / 409 / 422 with the prefix stripped | **unchanged** |
+| anything else | `500` `SHARE_*_FAILED` with its own message | **unchanged** |
+
+`POST /api/v1/analytics/dataset/query`:
+
+| the thrown refusal | was | is now |
+| :--- | :--- | :--- |
+| a sandboxed hook refusal, no status and no code declared | `500` `{ code: 'ANALYTICS_QUERY_FAILED', error: <text> }` | `400` `{ message: <text> }` — the same status `POST /api/v1/data/:object` answers for the identical throw, and no code, because the producer declared none |
+| a declared 4xx + code spelled `statusCode` rather than `status` | `500` `ANALYTICS_QUERY_FAILED` | the declared status + code |
+| a declared 4xx + code spelled `status` | the declared status + code | **unchanged** |
+| a declared 5xx, a crashed hook body, a driver fault, anything unclassified | `500` `ANALYTICS_QUERY_FAILED` | **unchanged** |
+
+The nested `{ success: false, error: { code, message } }` envelope the sharing
+family answers is unchanged — only the status and code inside it move. The
+`VALIDATION_ERROR` on the sandbox row is the catalog's declared floor for a
+required `code` the producer did not name (`standardErrorCodeForHttpStatus`);
+the flat `/data` body omits `code` there instead, because its `code` is
+optional and ADR-0112 invents nothing.

packages/rest/src/error-response.ts

@@ -1605,6 +1605,88 @@ function resolveErrorResponse(error: any, object?: string): { status: number; bo
  return mapDataError(error, object);
 }
 
+/**
+ * [#11683 / #11684] The wire answer the `/data` door gives for a refusal the
+ * PRODUCER classified — or `undefined` when it classified nothing and the
+ * catching route's own fault terminal is the honest answer.
+ *
+ * ## Why this exists as an export rather than as a rule each route re-states
+ *
+ * Two route families build their error body by hand and share no branch with
+ * either door in this file: `/analytics/dataset/query` and the three
+ * record-share routes, both in `rest-server.ts`. Both re-derived
+ * classification locally — analytics from an in-line `error.status` +
+ * `error.code` read, the share family from `message.startsWith(CODE)` over
+ * five literal prefixes — and both landed a *different* answer from `/data`
+ * for one refusal. That is the door-disagreement shape #7525/#8016/#11588 keep
+ * producing whenever a boundary open-codes a read this file already owns;
+ * {@link sandboxBusinessMessage} was named for exactly that reason one card
+ * earlier, and this is its status-side counterpart. A route that asks this
+ * cannot drift, because there is nothing left at the route to drift.
+ *
+ * ## The two limbs, and why each is a limb
+ *
+ * A refusal is *classified* when the producer said which condition it is. This
+ * repo has already ruled on two ways of saying so, and this function is their
+ * union — not a third rule:
+ *
+ * 1. **A declared ADR-0112 envelope** — a `status`/`statusCode`
+ * ({@link declaredHttpStatus}, both spellings, #7525) in the 4xx band
+ * *and* a non-empty string `code`. **Both halves, deliberately**, which is
+ * #5352's standing ruling on the analytics arm this sits beside: a 4xx
+ * with no code would force a hand-built envelope to invent one, and a
+ * producer shipping half an envelope has a bug that should be found rather
+ * than papered over here. This function does not reopen that.
+ * 2. **A sandboxed body's business `throw`** — {@link sandboxBusinessMessage}
+ * reads non-`undefined`, i.e. the QuickJS body REPORTED something rather
+ * than CRASHED (#7543). A missing `code` is *not* half an envelope here:
+ * the producer is a metadata-app author writing `throw new Error('…')`,
+ * and `classifyDataError`'s unwrap door has answered that with `400` plus
+ * the verbatim sentence since it existed — pinned end to end by
+ * `hook-error-format.dogfood.test.ts` and by
+ * `rest-hook-refusal-message-parity.test.ts` §3. Nothing is invented for
+ * the code that was not declared either: `thrownCodeFields` answers `{}`,
+ * which is ADR-0112's own rule.
+ *
+ * ## What it deliberately refuses to answer
+ *
+ * A **5xx**, declared or resolved. A server fault is not a refusal addressed
+ * to the caller, so it belongs to the catching route's own terminal — which is
+ * where the analytics `500 ANALYTICS_QUERY_FAILED` envelope and the share
+ * family's `SHARE_*_FAILED` codes keep living, message-withholding
+ * ({@link declaresServerFault}, #5811) and all. Both bands are checked: the
+ * declared one before resolution, so a declared 5xx never reaches
+ * {@link resolveErrorResponse}'s heuristics at all, and the resolved one
+ * after, so an error that looked classified but resolves to
+ * {@link UNCLASSIFIED_FAULT} or {@link DATA_STORE_FAULT} is handed back rather
+ * than dressed up as a refusal.
+ *
+ * ## What it does NOT decide
+ *
+ * The DIALECT. It returns the classification — `{ status, body }`, the same
+ * flat shape {@link handleRouteError} would send — and the caller re-dresses
+ * it in whatever envelope that route publishes. The record-share family
+ * answers the NESTED ADR-0112 D5 envelope (#8111) and must keep doing so; the
+ * analytics face answers its own flat `{ code, message }`. Deciding the wire
+ * POSITION here would have moved one of them, and vocabulary and position are
+ * two separate decisions — ADR-0112's #9232 amendment says so in as many
+ * words.
+ */
+export function classifiedRefusalAnswer(
+ error: any,
+): { status: number; body: Record<string, unknown> } | undefined {
+ const declared = declaredHttpStatus(error);
+ // A declared server fault is not a refusal, whatever else it carries.
+ if (declared !== undefined && declared >= 500) return undefined;
+ const declaresEnvelope =
+ declared !== undefined
+ && typeof error?.code === 'string'
+ && error.code.length > 0;
+ if (!declaresEnvelope && sandboxBusinessMessage(error) === undefined) return undefined;
+ const resolved = resolveErrorResponse(error);
+ return resolved.status < 500 ? resolved : undefined;
+}
+
 /**
  * Whether a mapped data-error status represents an *expected* client/lifecycle
  * outcome (and therefore shouldn't be logged as "[REST] Unhandled error").

packages/rest/src/rest-hook-refusal-message-parity.test.ts

@@ -512,6 +512,28 @@ describe('[#11588] the crash-with-a-declared-4xx divergence this card does NOT c
 // refusal (where `/data` answers 400) is a separate defect and is NOT touched.
 //
 // Predicted before running: §8a RED, §8b RED, §8c GREEN, §8d GREEN.
+//
+// ── [#11684] …and that "separate defect" is now closed, so §8b is INVERTED ──
+//
+// The sentence above ("neither arm's STATUS moves") was #11588's fence, not a
+// verdict: it recorded that one hook body, one `throw`, produced `400` on
+// `/data` and `500` here, and left the disagreement standing. #11684 asked
+// which door was right; the answer was already in the repo rather than open,
+// and §8b's own comment now carries the evidence. A new arm ①b sits between ①
+// and ③ and asks `classifiedRefusalAnswer` — the `/data` door's own
+// classification — so:
+// ① unchanged — a declared 4xx + `code`; #5352's both-halves rule intact
+// ①b a sandboxed body's business `throw`, and the `statusCode` spelling
+// ③ unchanged — a declared 5xx, a CRASHED body, anything unclassified
+//
+// §8b is INVERTED rather than deleted: it recorded a measured defect, and a
+// pin that records one is the only evidence the defect existed. §8e is added
+// beside it — the door-to-door status pin the card's measurement table was,
+// written as an assertion instead of a paragraph.
+//
+// Predicted before running the #11684 leg, against `4ceae8ab0`:
+// §8a GREEN (① untouched) · §8b RED · §8c GREEN (③ untouched)
+// §8d GREEN (① untouched) · §8e RED
 // ---------------------------------------------------------------------------
 
 const ANALYTICS_PATH = '/api/v1/analytics/dataset/query';
@@ -560,16 +582,62 @@ describe('[#11588] the analytics dataset face answers in the hook\'s words too',
  expect(JSON.stringify(res.body)).not.toMatch(WRAPPER_RE);
  }, 60_000);
 
- it('§8b an UNDECLARED refusal reaches the 500 arm unwrapped — the status is NOT moved', async () => {
- // The sub-case the card's repro did not exercise. `/data` answers 400
- // with this sentence and analytics answers 500 with it; that status
- // disagreement is a separate defect and is deliberately left standing —
- // asserted here so the next reader sees it was measured, not missed.
+ it('§8b [#11684 — INVERTED] an UNDECLARED refusal answers 400, the same as `/data`', async () => {
+ // ── This pin used to assert the 500. It is inverted, not deleted ────
+ //
+ // What it asserted, verbatim from #11588: "an UNDECLARED refusal
+ // reaches the 500 arm unwrapped — the status is NOT moved", with the
+ // comment "that status disagreement is a separate defect and is
+ // deliberately left standing". The defect was real and this pin is the
+ // evidence it was measured rather than missed; deleting it would
+ // destroy that record, so the assertion is turned around and the
+ // reasoning kept.
+ //
+ // ── WHICH READING WON, AND WHY ──────────────────────────────────────
+ //
+ // #11684 named two candidate readings and asked which one the repo had
+ // already committed to. It had committed to the `/data` door's, and
+ // the two readings turn out not to compete — they govern different
+ // questions:
+ //
+ // - **ADR-0112's "the producer names the condition"** is a rule about
+ // the `code`, not the status. Read D1–D9 and its five amendments:
+ // every one of them rules on the code vocabulary, its closure, and
+ // the `declaredCode` demote channel. The phrase this reading is
+ // built on is not in the ADR at all — it is `error-response.ts`'s
+ // own prose, and there it applies to a DECLARED 5xx that carries no
+ // code ("a half-declaration is honoured for the half that was
+ // declared and nothing is invented for the half that was not"). It
+ // is not contradicted here: this arm invents no code either.
+ //
+ // - **The `/data` door's reading** is the one that rules the STATUS,
+ // and it is not a preference — it is a structural branch with two
+ // end-to-end pins behind it. `classifyDataError`'s sandbox unwrap
+ // door answers `declaredHttpStatus(error) ?? 400` with the verbatim
+ // `.innerMessage` for a body that REPORTED, and the sanitised 500
+ // for a body that CRASHED (`isScriptFaultMessage`, #7543). The
+ // reporting half is pinned end to end by
+ // `hook-error-format.dogfood.test.ts` ("DELETE blocked by a
+ // sandboxed hook returns ONLY the business message", 400) and in
+ // process by §3 above.
+ //
+ // So "an undeclared throw is unclassified" was never the repo's rule
+ // for THIS class. A sandboxed body that reports has classified itself
+ // structurally — the sandbox boundary is what makes `.innerMessage`
+ // exist at all — and only a body that CRASHES is unclassified. That
+ // one still answers 500 here, in §8c's neighbour arm and in §2 of
+ // `rest-share-refusal-classification.test.ts`. No live pin was found
+ // on the other side of the question, so the fork clause did not fire.
+ //
+ // No code, deliberately: the producer declared none and nothing is
+ // invented for it, which is `thrownCodeFields`' answer on `/data` and
+ // ADR-0112's rule. `ANALYTICS_QUERY_FAILED` is ③'s code and ③ is not
+ // where this refusal belongs any more.
  const res = await analyticsRefusal(sandboxRefusal('month-end close is in progress'));
 
- expect(res.statusCode).toBe(500);
- expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED');
- expect(res.body.error).toBe('month-end close is in progress');
+ expect(res.statusCode).toBe(400);
+ expect(res.body.message).toBe('month-end close is in progress');
+ expect(res.body.code).toBeUndefined();
  expect(JSON.stringify(res.body)).not.toMatch(WRAPPER_RE);
  }, 60_000);
 
@@ -595,4 +663,74 @@ describe('[#11588] the analytics dataset face answers in the hook\'s words too',
  expect(res.statusCode).toBe(409);
  expect(res.body.message).toBe(text);
  }, 60_000);
+
+ it('§8e [#11684] the two faces answer one hook `throw` with ONE status', async () => {
+ // The card's measurement table, as an assertion. Both handlers are the
+ // REAL ones, driven in process — the analytics face through its
+ // service provider, the `/data` face through `createData` — because a
+ // hand-rolled stand-in would only reproduce whichever assumption wrote
+ // it. Neither status is named: the claim is that they AGREE, so a
+ // future move on either side reddens here even if someone also updates
+ // the literal in §8b.
+ // ⚠️ The CLIENT band only, and that bound is a finding rather than a
+ // convenience — see §8f immediately below, which pins what it excludes.
+ const cases: Array<[string, any]> = [
+ ['undeclared refusal', sandboxRefusal('month-end close is in progress')],
+ ['declared 409 + code', sandboxRefusal('locked', { code: 'RECORD_LOCKED', status: 409 })],
+ ['`statusCode` spelling', sandboxRefusal('locked', { code: 'RECORD_LOCKED', statusCode: 409 })],
+ ['a CRASHED body (#7543)', sandboxRefusal('TypeError: x is not a function')],
+ ];
+
+ for (const [label, error] of cases) {
+ const analytics = await analyticsRefusal(error);
+ const rest = setup({ createData: vi.fn().mockRejectedValue(error) });
+ const data = await call(rest, 'POST', DATA_COLLECTION, {
+ params: { object: 'crm_account' }, body: { name: 'x' },
+ });
+
+ expect(
+ analytics.statusCode,
+ `${label}: analytics ${analytics.statusCode} ${JSON.stringify(analytics.body)} `
+ + `vs /data ${data.statusCode} ${JSON.stringify(data.body)}`,
+ ).toBe(data.statusCode);
+ }
+ }, 60_000);
+
+ it('§8f MEASURED AND NOT REPAIRED — the two faces still disagree in the 5xx band', async () => {
+ // Found by §8e's first draft, which included this case and reddened
+ // WITH the fix in place. Recorded rather than quietly dropped: a bound
+ // on a parity claim that nobody can see is how the next reader
+ // concludes the two faces agree everywhere.
+ //
+ // A producer declaring `503` + a code is answered `503
+ // SERVICE_UNAVAILABLE` on `/data` (the #5582 passthrough: keep the
+ // status, keep the code, drop the prose) and `500
+ // ANALYTICS_QUERY_FAILED` here — 502/503 are `isExpectedDataStatus`
+ // lifecycle outcomes that proxies and retry policies read differently
+ // from a 500, so this is the same class of loss #5582 closed one door
+ // over.
+ //
+ // NOT repaired by #11684, deliberately. ③'s "a declared 5xx keeps
+ // going through the `ANALYTICS_QUERY_FAILED` envelope" is #5352's
+ // ruling, re-argued by #5367 and #5811 and load-bearing for the
+ // read-scope refusals — moving it is a contract call on a shipped
+ // route that neither folded card asked for. `classifiedRefusalAnswer`
+ // hands a declared 5xx straight back for exactly this reason. Filed as
+ // its own card.
+ const error = sandboxRefusal('boom', { code: 'SERVICE_UNAVAILABLE', status: 503 });
+
+ const analytics = await analyticsRefusal(error);
+ const rest = setup({ createData: vi.fn().mockRejectedValue(error) });
+ const data = await call(rest, 'POST', DATA_COLLECTION, {
+ params: { object: 'crm_account' }, body: { name: 'x' },
+ });
+
+ expect(data.statusCode).toBe(503);
+ expect(data.body.code).toBe('SERVICE_UNAVAILABLE');
+ expect(analytics.statusCode).toBe(500);
+ expect(analytics.body.code).toBe('ANALYTICS_QUERY_FAILED');
+ // Both still withhold the prose — that half never disagreed (§8c).
+ expect(analytics.body.error).toBe(INTERNAL_ERROR_MESSAGE);
+ expect(data.body.error).toBe(INTERNAL_ERROR_MESSAGE);
+ }, 60_000);
 });