From ce146ab285c49a68d6aaca4562316195ee79181f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 15:44:57 +0000 Subject: [PATCH] fix(rest): relay a producer-declared 5xx on `/analytics/dataset/query` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `POST /api/v1/analytics/dataset/query` collapsed every producer-declared 5xx onto a hand-built `500 ANALYTICS_QUERY_FAILED`, where `POST /data/:object` relays the declared status and ADR-0112 code and withholds only the prose (#5582). Measured door-to-door on one error object, a declared `{ status: 503, code: 'SERVICE_UNAVAILABLE' }` answered `503 SERVICE_UNAVAILABLE` on `/data` and `500 ANALYTICS_QUERY_FAILED` here. `/data` is the reference and does not move. Its 5xx arm is lifted into `declaredServerFaultAnswer` and read by both doors, the way the 4xx arm already imports `classifiedRefusalAnswer` — a third local opinion at this boundary is how the two faces came to disagree. The sibling analytics face `/analytics/query` already relayed both halves, so this door was the only one of three overwriting a producer's declaration. The prose withhold is untouched (#5352/#5367/#5811) and `logError` still runs before the relay branch, so a declared status cannot buy a producer past the operator's log. An undeclared fault keeps `500 ANALYTICS_QUERY_FAILED` and #5667's tiering. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HbG3rGVLjZStHQxHDtzJdJ --- ...lytics-dataset-query-declared-5xx-relay.md | 60 ++++++ .../analytics-dataset-dimension-gate.test.ts | 11 +- .../analytics-filter-refusal-envelope.test.ts | 62 ++++++- ...lytics-read-scope-refusal-envelope.test.ts | 30 ++- packages/rest/src/error-response.ts | 72 +++++++- .../rest-hook-refusal-message-parity.test.ts | 171 ++++++++++++++---- packages/rest/src/rest-server.ts | 52 ++++++ 7 files changed, 397 insertions(+), 61 deletions(-) create mode 100644 .changeset/analytics-dataset-query-declared-5xx-relay.md diff --git a/.changeset/analytics-dataset-query-declared-5xx-relay.md b/.changeset/analytics-dataset-query-declared-5xx-relay.md new file mode 100644 index 0000000000..33ca8343c1 --- /dev/null +++ b/.changeset/analytics-dataset-query-declared-5xx-relay.md @@ -0,0 +1,60 @@ +--- +"@objectstack/rest": patch +--- + +fix(rest): `POST /analytics/dataset/query` relays a producer-declared 5xx instead of collapsing it to `500 ANALYTICS_QUERY_FAILED` (#11718) + +**Response-contract change on a shipped public route.** The door is +`POST /api/v1/analytics/dataset/query` (and its environment-scoped twin). An +error whose producer declared a 5xx `status` now reaches the client with **that +status and that ADR-0112 `code`**, where the route previously answered a +hand-built `500` `ANALYTICS_QUERY_FAILED` for every one of them. + +Measured door-to-door before the fix, one error object, both handlers driven in +process: + +| face | answer | +|---|---| +| `POST /api/v1/data/:object` | `503` — `{"error":"Internal server error","code":"SERVICE_UNAVAILABLE"}` | +| `POST /api/v1/analytics/dataset/query` | `500` — `{"code":"ANALYTICS_QUERY_FAILED","error":"Internal server error"}` | + +`/data` is the reference and does not move. Its relay is #5582's ruling — `502` +and `503` are `isExpectedDataStatus` lifecycle outcomes that proxies and retry +policies read differently from a `500`, so collapsing them destroys the +declaration — and that ruling never reached this route because the analytics +catch built its 5xx envelope by hand. The **sibling** analytics face +`/analytics/query` already relayed both halves through +`dispatcher-plugin.errorResponseBase`, so this door was the only one of three +overwriting a producer's declaration. + +The repair imports `/data`'s own arm rather than restating it: the branch is +lifted into `declaredServerFaultAnswer` in `error-response.ts` and read by both +doors, the same way the 4xx arm already imports `classifiedRefusalAnswer`. A +third local opinion at this boundary is how the two faces came to disagree. + +**Not a re-opening of #5352/#5367/#5811 — the prose is still withheld.** A +declared server fault's message is still replaced by the generic sentence, from +the same shared arm, and the full original text still reaches the operator: the +`logError` line runs *before* the relay branch and is unconditional, so a +producer cannot buy its way past the operator's log with a declared status. What +moves is the classification the producer declared and this route was +overwriting. + +**What callers see change:** + +- A declared `{ status: 503, code: 'SERVICE_UNAVAILABLE' }` → `503` + `SERVICE_UNAVAILABLE` (was `500` `ANALYTICS_QUERY_FAILED`). +- An unregistered declared code demotes exactly as `/data` demotes it — `503` + `{"code":"SERVICE_UNAVAILABLE","declaredCode":"WAREHOUSE_UNAVAILABLE"}` (#9232). +- `read-scope-sql`'s ten fail-closed RLS refusals answer `500` + `READ_SCOPE_COMPILE_FAILED` instead of `500` `ANALYTICS_QUERY_FAILED`. Their + 2026-08-06 ruling is untouched in substance — still a SERVER fault, still + `500`, still with the RLS policy content withheld from the body and intact in + the log — and the code they now carry is the one they declare and the one the + sibling `/analytics/query` face has always shipped to clients. + +**Unchanged:** an *undeclared* fault. No declared status means nothing to relay, +so it keeps `500` `ANALYTICS_QUERY_FAILED` and #5667's tiering, which leaves a +self-authored fault readable. A declared **4xx** is untouched — that band is +arms ① and ①b, and the half-envelope rule (a 4xx status with no code invents no +code) still stands. diff --git a/packages/rest/src/analytics-dataset-dimension-gate.test.ts b/packages/rest/src/analytics-dataset-dimension-gate.test.ts index 3d46dbda7b..62d305fa35 100644 --- a/packages/rest/src/analytics-dataset-dimension-gate.test.ts +++ b/packages/rest/src/analytics-dataset-dimension-gate.test.ts @@ -325,7 +325,9 @@ describe('[#5520] the 500 body no longer ships driver internals', () => { // messages name RLS policy fields — the 5xx branch withholds the text. So // what this half now guards is that #5520's `looksLikeInternalErrorLeak` // withhold and #5367's declared-server-fault withhold COMPOSE rather than - // fight: same 500 code, message withheld, log intact. + // fight: same 500, message withheld, log intact. [#11718] The CODE on the + // wire is now the producer's own `READ_SCOPE_COMPILE_FAILED`; the status + // and the withhold — this half's actual subject — do not move. const c = await post( buildRoute(async () => throwingAnalytics( @@ -338,7 +340,12 @@ describe('[#5520] the 500 body no longer ships driver internals', () => { { dataset, selection: { measures: ['account_count'], dimensions: ['industry'] } }, ); expect(c.statusCode).toBe(500); - expect(c.body.code).toBe('ANALYTICS_QUERY_FAILED'); + // [#11718] The declared code is RELAYED rather than overwritten with + // `ANALYTICS_QUERY_FAILED`. What this half guards is unchanged and is + // asserted on the next two lines: #5520's `looksLikeInternalErrorLeak` + // withhold and #5367's declared-server-fault withhold still COMPOSE rather + // than fight — same 500, message withheld, no policy field in the body. + expect(c.body.code).toBe('READ_SCOPE_COMPILE_FAILED'); expect(c.body.error).toBe(INTERNAL_ERROR_MESSAGE); expect(String(c.body.error)).not.toMatch(/owner_email/); }); diff --git a/packages/rest/src/analytics-filter-refusal-envelope.test.ts b/packages/rest/src/analytics-filter-refusal-envelope.test.ts index 13831cc417..df2c1f8a5a 100644 --- a/packages/rest/src/analytics-filter-refusal-envelope.test.ts +++ b/packages/rest/src/analytics-filter-refusal-envelope.test.ts @@ -284,13 +284,18 @@ describe('[#5352 → #5367] the message-sniffing fallback is GONE', () => { // So the same input is asserted the other way round — and the bare form, which // is what the list used to rescue, is asserted too. Between them they pin that // no message test survives anywhere in this catch. - it('read-scope-sql: the DECLARED 500 → 500 ANALYTICS_QUERY_FAILED, policy content withheld', async () => { + it('read-scope-sql: the DECLARED 500 → 500 READ_SCOPE_COMPILE_FAILED, policy content withheld', async () => { const message = '[read-scope-sql] unsupported operator "$regex" on "owner_email" (fail-closed).'; const err = Object.assign(new Error(message), { code: 'READ_SCOPE_COMPILE_FAILED', status: 500 }); const route = buildRoute(async () => ({ queryDataset: vi.fn().mockRejectedValue(err) })); const res = await post(route, { dataset, selection }); expect(res.statusCode).toBe(500); - expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED'); + // [#11718] The declared code is RELAYED now, not overwritten — see this + // file's sibling `analytics-read-scope-refusal-envelope.test.ts` header. + // The status this test is really about is untouched: still 500, still not + // the sniffed 400 the deleted message list used to produce. + expect(res.body.code).toBe('READ_SCOPE_COMPILE_FAILED'); + expect(res.body.code).not.toBe('DATASET_INVALID'); // The disclosure half: an RLS policy's field name must not come back. expect(String(res.body.error)).not.toMatch(/owner_email/); expect(String(res.body.error)).not.toMatch(/read-scope-sql/); @@ -380,19 +385,56 @@ describe('[#5352] reading the envelope did not turn every failure into a 400', ( expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED'); }); - it('a 5xx-status error is NOT passed through — an internal fault keeps the 500 envelope, message withheld', async () => { - // Deliberate asymmetry: the passthrough is 4xx-only, so a producer cannot - // re-label a server fault with a code of its own and slip past the - // `logError` line that makes it visible to operators. + it('[#11718 — INVERTED] a declared 5xx IS relayed, and is still logged and still withheld', async () => { + // ── This pin asserted the collapse. It is inverted, not deleted ───────── + // + // What it asserted, verbatim: "a 5xx-status error is NOT passed through — + // an internal fault keeps the 500 envelope, message withheld", reasoned as + // "the passthrough is 4xx-only, so a producer cannot re-label a server + // fault with a code of its own and slip past the `logError` line that makes + // it visible to operators." + // + // That REASON is answered rather than overruled, and answering it is what + // made the repair safe: `logError` runs BEFORE the relay branch and is + // unconditional, so every declared 5xx is still on the operator's line with + // its full original text. Asserted here, not assumed — the argument for the + // old behaviour is only retired if its concern is actually covered. + // + // What was NOT answerable was the collapse itself. `/data` relays a declared + // 5xx's status and code (#5582: `502`/`503` are `isExpectedDataStatus` + // lifecycle outcomes proxies and retry policies read differently from a + // `500`), and so does the sibling `/analytics/query`. Measured door-to-door + // in `rest-hook-refusal-message-parity.test.ts` §8f. const err = Object.assign(new Error('upstream analytics warehouse is unavailable'), { code: 'WAREHOUSE_UNAVAILABLE', status: 503, }); const route = buildRoute(async () => ({ queryDataset: vi.fn().mockRejectedValue(err) })); - const res = await post(route, { dataset, selection }); - - expect(res.statusCode).toBe(500); - expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED'); + const logSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + let res: any; + let logged: string; + try { + res = await post(route, { dataset, selection }); + // Read the calls BEFORE restoring: `mockRestore` resets the recorded + // calls as well as the implementation, so reading after it reports an + // empty log for a route that logged perfectly well. + logged = logSpy.mock.calls.map((args) => args.map(String).join(' ')).join('\n'); + } finally { + logSpy.mockRestore(); + } + + expect(res.statusCode).toBe(503); + expect(res.statusCode).not.toBe(500); + // [#9232] `WAREHOUSE_UNAVAILABLE` is not an ADR-0112 member, so it is + // DEMOTED to `declaredCode` beside the code the status derives — the same + // answer `/data` gives it, which is the whole point of importing that arm + // instead of hand-building a second envelope here. + expect(res.body.code).toBe('SERVICE_UNAVAILABLE'); + expect(res.body.declaredCode).toBe('WAREHOUSE_UNAVAILABLE'); + expect(res.body.code).not.toBe('ANALYTICS_QUERY_FAILED'); + // The operator still has the whole thing — the concern the old pin named. + expect(logged).toContain('Analytics dataset query error'); + expect(logged).toContain('upstream analytics warehouse is unavailable'); // [#5367] Second half of the asymmetry, added with the read-scope ruling: a // producer that DECLARES a server fault has declared that the detail is the // operator's, so the message is withheld here and kept in `logError`. This diff --git a/packages/rest/src/analytics-read-scope-refusal-envelope.test.ts b/packages/rest/src/analytics-read-scope-refusal-envelope.test.ts index ffbcfca25f..5596876ab3 100644 --- a/packages/rest/src/analytics-read-scope-refusal-envelope.test.ts +++ b/packages/rest/src/analytics-read-scope-refusal-envelope.test.ts @@ -2,8 +2,24 @@ /** * [#5367, maintainer ruling 2026-08-06] A read-scope lowering failure reaches the - * caller as `500 ANALYTICS_QUERY_FAILED` **with the RLS policy withheld**, and the - * full text reaches the operator's log. + * caller as a **500 with the RLS policy withheld**, and the full text reaches the + * operator's log. + * + * [#11718] The CODE in that sentence was `ANALYTICS_QUERY_FAILED` and is now + * `READ_SCOPE_COMPILE_FAILED` — the one these refusals declare. Nothing in the + * 2026-08-06 ruling moved: the family is still a SERVER fault, still `500`, still + * has its policy content withheld, and the `400 DATASET_INVALID` it was rescued + * from is still asserted against below. What changed is that + * `/analytics/dataset/query` stopped OVERWRITING the producer's declaration with + * a code of its own. That overwrite was never ruled — it was what ③'s hand-built + * envelope happened to emit — and it made this face the only one of three that + * did it: `/data` relays a declared 5xx's status and code (#5582), and the + * SIBLING analytics face `/analytics/query` has shipped + * `READ_SCOPE_COMPILE_FAILED` to clients all along + * (`analytics-query-read-scope-withhold.test.ts`, measured against a real + * `AnalyticsService`). So this file's answer now equals its sibling's for one + * refusal, where before the same fault was named two different things depending + * on which analytics door the caller used. * * ## What this closes * @@ -178,13 +194,19 @@ describe('[#5367] POST /analytics/dataset/query — a read-scope failure is a 50 ]; for (const c of CASES) { - it(`${c.name} → 500 ANALYTICS_QUERY_FAILED, body carries no policy detail`, async () => { + it(`${c.name} → 500 READ_SCOPE_COMPILE_FAILED, body carries no policy detail`, async () => { const route = buildRoute(async () => analyticsWithScope(c.scope)); const res = await post(route, { dataset, selection }); // Classification: a server fault, not the caller's mistake. expect(res.statusCode).toBe(500); - expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED'); + // [#11718] …named by its PRODUCER. `read-scope-sql` declares + // `READ_SCOPE_COMPILE_FAILED` / 500 and the route relays it instead of + // overwriting it with `ANALYTICS_QUERY_FAILED`. The status is untouched + // by that repair — this family declares 500 — so the 2026-08-06 ruling's + // own subject is asserted unchanged, immediately above. + expect(res.body.code).toBe('READ_SCOPE_COMPILE_FAILED'); + expect(res.body.code).not.toBe('ANALYTICS_QUERY_FAILED'); // …and specifically NOT the pre-ruling answer. expect(res.statusCode).not.toBe(400); expect(res.body.code).not.toBe('DATASET_INVALID'); diff --git a/packages/rest/src/error-response.ts b/packages/rest/src/error-response.ts index c71cd9f13c..3336302fc0 100644 --- a/packages/rest/src/error-response.ts +++ b/packages/rest/src/error-response.ts @@ -418,6 +418,58 @@ function thrownCodeFields(error: any, status: number): { code?: string; declared return { code: thrown.code, ...(demoted !== undefined ? { declaredCode: demoted } : {}) }; } +/** + * [#11718] The DECLARED-SERVER-FAULT relay, as one definition instead of a + * shape each door re-derives: a producer that declared a 5xx keeps its + * `status` and its ADR-0112 `code`, and loses only its prose. + * + * Answers `undefined` for everything else — no declared status, a declared + * 4xx, a status outside 400–599 — so a caller can ask it first and keep its + * own arm for the rest. + * + * ## Why this is a function and not a fourth copy + * + * #11718 measured one producer-declared `{ status: 503, code: + * 'SERVICE_UNAVAILABLE' }` through two doors and got two answers: + * + * ``` + * POST /api/v1/data/:object → 503 {"error":"Internal server error","code":"SERVICE_UNAVAILABLE"} + * POST /api/v1/analytics/dataset/query → 500 {"code":"ANALYTICS_QUERY_FAILED","error":"Internal server error"} + * ``` + * + * The analytics dataset face built its 5xx body by hand, so #5582's relay — + * argued on the ground that `502`/`503` are `isExpectedDataStatus` lifecycle + * outcomes that proxies and retry policies read differently from a `500` — + * simply never reached it. It is the same failure mode {@link + * classifiedRefusalAnswer} was extracted for one arm earlier: a third local + * opinion at this boundary is precisely how two faces come to disagree. So the + * arm is imported, not restated. + * + * ⚠️ Its gate is `declaredHttpStatus(...) >= 500`, NOT `declaresServerFault` + * alone. The two differ on a 5xx that declared no `code`, and the difference is + * load-bearing: a producer declaring `{ status: 503 }` and nothing else still + * gets its status relayed, carrying no code at all. Nothing is invented for the + * half that was not declared — see the paragraph in {@link mapDataError}'s arm + * this body was lifted from. Gating on `declaresServerFault` instead would + * silently keep collapsing that shape onto `500`, which is the very defect, + * one case narrower. + */ +export function declaredServerFaultAnswer( + error: any, +): { status: number; body: Record } | undefined { + const declaredStatus = declaredHttpStatus(error); + if (declaredStatus === undefined || declaredStatus < 500) return undefined; + return { + status: declaredStatus, + body: { + error: INTERNAL_ERROR_MESSAGE, + ...(declaresServerFault({ status: declaredStatus, code: error?.code }) + ? thrownCodeFields(error, declaredStatus) + : {}), + }, + }; +} + /** * [#8264] Postgres' missing-relation template, anchored on the QUOTED * identifier the driver always emits — never on the bare "does not exist" @@ -896,16 +948,16 @@ function classifyDataError(error: any, object?: string): { status: number; body: // still decides WHETHER a code rides — its non-empty-string half is the // same question `thrownCodeFields` asks internally, so the two agree by // construction and this arm's body-shape decision is unchanged. - if (declaredStatus >= 500) { - return { - status: declaredStatus, - body: { - error: INTERNAL_ERROR_MESSAGE, - ...(declaresServerFault({ status: declaredStatus, code: error?.code }) - ? thrownCodeFields(error, declaredStatus) - : {}), - }, - }; + // + // [#11718] The arm's BODY now lives in {@link declaredServerFaultAnswer}, + // unchanged — every paragraph above still describes it, and this call is + // the only reader of it that existed before. It was lifted out so the + // `/analytics/dataset/query` face could answer a declared 5xx with the + // same bytes rather than a second hand-built envelope; `/data`'s answer + // is the reference and does not move. + const declaredServerFault = declaredServerFaultAnswer(error); + if (declaredServerFault !== undefined) { + return declaredServerFault; } // [#5423] The 4xx arm is UNCHANGED by #5582: a 4xx message is addressed // TO the caller and is the remedy, so it keeps its wording, its diff --git a/packages/rest/src/rest-hook-refusal-message-parity.test.ts b/packages/rest/src/rest-hook-refusal-message-parity.test.ts index 4ed258779c..9bc5417e5f 100644 --- a/packages/rest/src/rest-hook-refusal-message-parity.test.ts +++ b/packages/rest/src/rest-hook-refusal-message-parity.test.ts @@ -508,6 +508,7 @@ describe('[#11588] the crash-with-a-declared-4xx divergence this card does NOT c // only the first: // ① a declared 4xx + `code` → `{ code, message }` // ③ everything else → `500 ANALYTICS_QUERY_FAILED` +// (③ has since split — see the #11718 block at the end of this header.) // Neither arm's STATUS moves here. ③ answering 500 for an undeclared hook // refusal (where `/data` answers 400) is a separate defect and is NOT touched. // @@ -534,6 +535,32 @@ describe('[#11588] the crash-with-a-declared-4xx divergence this card does NOT c // Predicted before running the #11684 leg, against `4ceae8ab0`: // §8a GREEN (① untouched) · §8b RED · §8c GREEN (③ untouched) // §8d GREEN (① untouched) · §8e RED +// +// ── [#11718] …and §8f's recorded bound is now CLOSED, so it is FLIPPED ────── +// +// §8f recorded that the two faces still disagreed in the 5xx band: one +// producer-declared `{ status: 503, code: 'SERVICE_UNAVAILABLE' }` answered +// `503 SERVICE_UNAVAILABLE` on `/data` and `500 ANALYTICS_QUERY_FAILED` here. +// #11718 asked which door was right and the repo had already answered at two +// of three doors — `/data` (#5582) and the SIBLING analytics face +// `/analytics/query` (via `dispatcher-plugin.errorResponseBase`) both relay the +// declared status AND the declared code. This face was the sole collapse. +// +// ③ now splits, exactly as ① did in #11684: +// ③a a DECLARED 5xx → relayed through `declaredServerFaultAnswer`, which is +// `/data`'s own arm imported rather than restated; prose still withheld +// ③b everything else → unchanged `500 ANALYTICS_QUERY_FAILED`, #5667 tiering +// +// §8f is FLIPPED rather than deleted, for §8b's reason: a pin that records a +// measured defect is the only evidence the defect existed. §8g is added beside +// it as the positive control on ③b, so §8f cannot pass for a route that stopped +// classifying anything. +// +// Predicted before running the #11718 leg, against `2b9a5222`: +// §8a GREEN (① untouched) · §8b GREEN (①b untouched) · §8c RED (status moves) +// §8d GREEN (① untouched) · §8e GREEN (client band — REGRESSION GUARD, it was +// green before this change too) · §8f RED · §8g GREEN (③b untouched — +// REGRESSION GUARD, new pin over unchanged behaviour) // --------------------------------------------------------------------------- const ANALYTICS_PATH = '/api/v1/analytics/dataset/query'; @@ -641,15 +668,27 @@ 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('§8c a declared 5xx still has its prose withheld', async () => { + it('§8c a declared 5xx still has its prose withheld — and now keeps its status', async () => { + // [#11718] The PROSE half is what this pin was written for and it is + // unchanged: a declared server fault's detail is the operator's, and + // `connect ECONNREFUSED 10.0.0.5:5432` must not reach the caller. + // + // The STATUS literal moved with #11718's repair. It was `500` here + // because ③ hand-built that envelope, not because anything ruled that a + // declared `503` becomes a `500` — §8f measured the disagreement that + // literal encoded, and now pins the agreement. Asserted alongside the + // withhold precisely so the two halves cannot be confused again: the + // message is withheld, the classification is relayed. const res = await analyticsRefusal( sandboxRefusal('connect ECONNREFUSED 10.0.0.5:5432', { code: 'SERVICE_UNAVAILABLE', status: 503, }), ); - expect(res.statusCode).toBe(500); + expect(res.statusCode).toBe(503); + expect(res.body.code).toBe('SERVICE_UNAVAILABLE'); expect(res.body.error).toBe(INTERNAL_ERROR_MESSAGE); + expect(String(JSON.stringify(res.body))).not.toContain('ECONNREFUSED'); }, 60_000); it('§8d ⭐ POSITIVE CONTROL — a non-sandboxed refusal is still verbatim', async () => { @@ -696,41 +735,103 @@ describe('[#11588] the analytics dataset face answers in the hook\'s words too', } }, 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. + it('§8f [#11718 — REPAIRED] the two faces agree in the 5xx band too', async () => { + // ── This pin used to assert the disagreement. It is flipped, not + // deleted, for the same reason §8b was ──────────────────────────── // - // 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. + // What it asserted, verbatim from #11718: "MEASURED AND NOT REPAIRED — + // the two faces still disagree in the 5xx band", `/data` answering + // `503 SERVICE_UNAVAILABLE` and this face `500 ANALYTICS_QUERY_FAILED` + // for ONE producer-declared refusal. That bound on §8e's parity claim + // was real and this pin is the evidence it was measured rather than + // missed, so the assertion is turned around and the reasoning kept. // - // 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' }, - }); + // ── WHICH DOOR WAS RIGHT, AND HOW IT WAS DECIDED ──────────────────── + // + // #11718 named three candidate answers — `503 SERVICE_UNAVAILABLE`, + // `503 ANALYTICS_QUERY_FAILED`, and "leave it at 500" — and recorded + // that #5352's text does not distinguish them, because it argues the + // MESSAGE, not the status. The repo had already committed, at two of + // its three analytics-and-data doors: + // + // - `/data` relays both halves (#5582's passthrough: keep the status, + // keep the code, drop the prose), argued on `502`/`503` being + // `isExpectedDataStatus` lifecycle outcomes that proxies and retry + // policies read differently from a `500`. + // - The SIBLING analytics face `/analytics/query` relays both halves + // too, through `dispatcher-plugin.errorResponseBase` — measured, and + // pinned end-to-end against a real `AnalyticsService` in + // `analytics-query-read-scope-withhold.test.ts`, which asserts a + // read-scope refusal arrives as `500` + `READ_SCOPE_COMPILE_FAILED`. + // + // So `/analytics/dataset/query` was the only one of three collapsing a + // producer's declaration, and it collapsed it only because this arm + // built its 5xx body by hand. The repair imports `/data`'s own arm + // ({@link declaredServerFaultAnswer}) rather than restating it. + // + // ⛔ NOT a re-opening of #5352/#5367/#5811. Those rule the PROSE and the + // prose is still withheld on both faces — asserted below, on both, for + // exactly that reason. + // + // ⚠️ Neither status nor code is asserted only as "they agree". Agreement + // alone is vacuous against this defect in one direction — the defect IS + // a 5xx with a code, so "both are 5xx with a code" was already true. The + // DECLARED values are named too, so a future collapse on either side + // reddens here whichever side moves. + const cases: Array<[string, any, number, string, string | undefined]> = [ + // A REGISTERED ADR-0112 code: relayed verbatim, both faces. + ['registered code', sandboxRefusal('boom', { code: 'SERVICE_UNAVAILABLE', status: 503 }), + 503, 'SERVICE_UNAVAILABLE', undefined], + // An UNREGISTERED spelling: #9232's demote channel, and the two + // faces must agree on the demote as well as on the relay — this is + // the half a hand-built envelope could never have got right. + ['unregistered code', sandboxRefusal('boom', { code: 'WAREHOUSE_UNAVAILABLE', status: 503 }), + 503, 'SERVICE_UNAVAILABLE', 'WAREHOUSE_UNAVAILABLE'], + // The `statusCode` spelling (#7525) in the 5xx band. + ['`statusCode` spelling', sandboxRefusal('boom', { code: 'SERVICE_UNAVAILABLE', statusCode: 502 }), + 502, 'SERVICE_UNAVAILABLE', undefined], + ]; - 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); + for (const [label, error, status, code, declaredCode] 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' }, + }); + + const where = `${label}: analytics ${analytics.statusCode} ${JSON.stringify(analytics.body)} ` + + `vs /data ${data.statusCode} ${JSON.stringify(data.body)}`; + + // The two faces agree… + expect(analytics.statusCode, where).toBe(data.statusCode); + expect(analytics.body.code, where).toBe(data.body.code); + expect(analytics.body.declaredCode, where).toBe(data.body.declaredCode); + // …on the values the PRODUCER declared, not merely with each other. + expect(analytics.statusCode, where).toBe(status); + expect(analytics.body.code, where).toBe(code); + expect(analytics.body.declaredCode, where).toBe(declaredCode); + // …and specifically NOT the pre-repair answer. + expect(analytics.statusCode, where).not.toBe(500); + expect(analytics.body.code, where).not.toBe('ANALYTICS_QUERY_FAILED'); + // Both still withhold the prose — that half never disagreed (§8c). + expect(analytics.body.error, where).toBe(INTERNAL_ERROR_MESSAGE); + expect(data.body.error, where).toBe(INTERNAL_ERROR_MESSAGE); + } + }, 60_000); + + it('§8g ⭐ POSITIVE CONTROL — an UNDECLARED fault still answers `500 ANALYTICS_QUERY_FAILED`', async () => { + // [#11718] The bound on §8f, and the reason the repair is a RELAY rather + // than a blanket. Without this, §8f could pass for a route that had + // stopped classifying anything at all. A fault nobody declared has no + // status and no code to relay, so ③ still owns it — `ANALYTICS_QUERY_FAILED` + // is this route's answer for exactly that class, and #5667's tiering + // keeps a self-authored fault readable. + const res = await analyticsRefusal( + new Error('[Analytics] no strategy can handle query for cube "pipeline"'), + ); + + expect(res.statusCode).toBe(500); + expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED'); + expect(String(res.body.error)).toMatch(/no strategy can handle query/); }, 60_000); }); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 62fd96cb29..39a50b2668 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -206,6 +206,7 @@ import { mapDataError, sandboxBusinessMessage, classifiedRefusalAnswer, + declaredServerFaultAnswer, sendThrownError, sendDeclaredFault, sendFieldVisibilityFault, @@ -9487,6 +9488,57 @@ export class RestServer { // the same two fields ① derives `envelopeStatus`/`envelopeCode` // from. logError('[REST] Analytics dataset query error:', error); + // ── [#11718] ③a A DECLARED 5xx is RELAYED, not collapsed ── + // Measured door-to-door: one producer-declared + // `{ status: 503, code: 'SERVICE_UNAVAILABLE' }` answered + // `503 SERVICE_UNAVAILABLE` on `POST /data/:object` and + // `500 ANALYTICS_QUERY_FAILED` here. #5582's argument is + // that `502`/`503` are `isExpectedDataStatus` LIFECYCLE + // outcomes — a proxy retries them and alerts differently — + // so collapsing them onto `500` destroys the declaration. + // That ruling landed one door over and never reached this + // one, because this arm built its 5xx body by hand. + // + // ⛔ This does NOT re-open #5352/#5367/#5811. Those rule the + // PROSE, and the prose is still withheld — byte-identical, + // `INTERNAL_ERROR_MESSAGE`, from the same shared arm — and + // the full text still reaches the operator through the + // `logError` line ABOVE this branch, which is deliberately + // placed first so a relayed status cannot buy a producer its + // way past the log. What moves is the CLASSIFICATION the + // producer declared and this route was overwriting. + // + // {@link declaredServerFaultAnswer} is `/data`'s own arm, + // imported for the reason {@link classifiedRefusalAnswer} + // above it is: a third local opinion at this boundary is how + // the two faces came to disagree in the first place. The + // SIBLING analytics face already agreed with `/data` — + // `/analytics/query` relays both halves through + // `dispatcher-plugin.errorResponseBase` (measured: a + // read-scope refusal reaches the client as `500` + // `READ_SCOPE_COMPILE_FAILED`, pinned in + // `analytics-query-read-scope-withhold.test.ts`) — so this + // face was the only one of three collapsing the declaration. + // + // ⚠️ Consequence, deliberate and named: a declared 5xx now + // carries the PRODUCER's code, so `read-scope-sql`'s ten + // fail-closed refusals answer `500 READ_SCOPE_COMPILE_FAILED` + // here instead of `500 ANALYTICS_QUERY_FAILED`. Their + // 2026-08-06 ruling is untouched in substance — a SERVER + // fault, `500`, policy content withheld — and the code they + // now carry is the one they declare and the one the sibling + // face has always shipped. `ANALYTICS_QUERY_FAILED` remains + // this route's answer for an UNDECLARED fault, below. + const declaredFault = declaredServerFaultAnswer(error); + if (declaredFault) { + return res.status(declaredFault.status).json(declaredFault.body); + } + // ── ③b The generic 500, for a fault nobody declared ────── + // `declaresServerFault` is kept in the withhold test rather + // than dropped as dead: it reads `error.status` alone and is + // not bounded above, so a nonsense `status: 700` with a code + // still reaches here (`declaredHttpStatus` requires < 600) + // and must keep the withhold it has had since #5367. const outward = declaresServerFault(error) || looksLikeInternalErrorLeak(msg) ? INTERNAL_ERROR_MESSAGE : clientMsg.slice(0, 500);