Skip to content

Commit b4bc2ab

Browse files
hotlongclaude
andcommitted
refactor(types): move the 5xx fault-log rule to @objectstack/types and land it on sendError
Re-homed under the domain seat's serial fence: `packages/rest/src/package-routes.ts` is held by the open PR #14499, so this no longer edits that file. The REST direct-mount doors are covered from the producer side instead. `sendError` (`@objectstack/types`) is the single writer for every nested-envelope error in the repo, and every catch in the package registrar ends there — so wiring the rule at that one exit covers those doors with no per-door call, and covers any door added later by construction. That also puts the helper in the same package as `resolveThrownHttpError`, on the same argument: a rule two doors must agree on cannot live inside one of them. `@objectstack/observability` and `packages/rest` are back to origin/main byte-for-byte, as is the system-context census page (its line rot was caused by the package-routes import this drops). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 parent bb4aa09 commit b4bc2ab

9 files changed

Lines changed: 127 additions & 78 deletions

File tree

.changeset/log-every-5xx-server-fault.md

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
11
---
2-
"@objectstack/observability": patch
2+
"@objectstack/types": patch
33
"@objectstack/runtime": patch
4-
"@objectstack/rest": patch
54
---
65

7-
fix(observability,runtime,rest): log every 5xx at `error` level instead of answering it silently (#14310)
6+
fix(types,runtime): log every 5xx at `error` level instead of answering it silently (#14310)
87

98
A 500 that leaves no server-side line is diagnosed from the browser or not at
109
all. Measured on `main`, through the real plugin and the real route handlers: a
@@ -27,21 +26,24 @@ reasons:
2726
so even a wired reporter never saw those.
2827

2928
**The rule now has one definition.** `logServerFault` (new, in
30-
`@objectstack/observability`, which owns the operator-facing `Logger` /
31-
`ErrorReporter` channel and is already a dependency of both consumers) emits
32-
exactly one `error`-level record carrying method, path, request id, the
33-
message and — where the door still holds the throw — the stack. It could not
34-
live in either consumer: `@objectstack/runtime` depends on `@objectstack/rest`,
35-
so an import could only ever point one way — the same argument that put
36-
`resolveThrownHttpError` in `@objectstack/types` rather than in one of its two
37-
doors.
38-
39-
Wired at each transport's own single exit, so a fault costs one line and never
40-
two: the dispatcher's thrown exit (`errorResponseBase`) and returned exit
41-
(`sendResultBase`), the AI-route mount that writes its own result, and the REST
42-
direct-mount package registrar's `sendThrownError` plus its two reported
43-
driver faults. `packages/rest`'s `/data` doors were already loud
44-
(`logUnexpectedRouteError`) and are untouched.
29+
`@objectstack/types`) emits exactly one `error`-level record carrying method,
30+
path, request id, the message and — where the door still holds the throw — the
31+
stack. It shares a home with `resolveThrownHttpError` for the same reason that
32+
rule was moved there in #8016: a rule two doors must agree on cannot live
33+
inside one of them, because `@objectstack/runtime` depends on
34+
`@objectstack/rest` and an import could only ever point one way.
35+
36+
Wired at each transport's single exit, so a fault costs one line and never two:
37+
38+
- `sendError` — the one writer for every nested-envelope error in the repo. The
39+
REST direct-mount registrars (the `/api/v1/packages` door that mounts first
40+
in production) become loud through it with no per-door call, so a door added
41+
later cannot forget one.
42+
- The dispatcher's thrown exit (`errorResponseBase`), its returned exit
43+
(`sendResultBase`) and the AI-route mount that writes its own result.
44+
45+
`packages/rest`'s `/data` doors were already loud via `logUnexpectedRouteError`
46+
and are untouched.
4547

4648
`error` level is load-bearing: the CLI's default is `warn` and `error` (40)
4749
outranks `warn` (30), so the record clears `--log-level`'s default without
@@ -51,7 +53,8 @@ deliberate instruction rather than the default this fixes.
5153
**4xx stays quiet**, decided once inside the helper rather than at each call
5254
site — client mistakes are already explained by the response, and logging them
5355
is how a `?state=draft` probe once printed 45 stack traces in one browsing
54-
session.
56+
session. The wire body is byte-identical at every door: this adds a side
57+
effect, never a field.
5558

5659
⚠️ Behaviour change worth knowing before upgrading: a deployment that answers
5760
a *declared* 5xx on a polled route — `501 NOT_IMPLEMENTED` from an uninstalled

content/docs/permissions/system-context.mdx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,9 +160,9 @@ The largest single consumer — **20 of the 109 sites**.
160160
| 49 | Action `requiredPermissions` bypassed | runtime | Get: engine self-invocation runs any action | `action-execution.ts:399` |
161161
| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:4716`, `:6079`, `:6327`, `:6758`, `:6951` |
162162
| 51 | The shared metadata-write verdict itself returns `allowed` | metadata-core | Get: the one function all of row 50's doors consult answers yes before any capability is examined | `meta-write-capability.ts:134` |
163-
| 52 | Anonymous-deny seam satisfied on the domain dispatchers and the package/federation routes | runtime, rest | Get: passes with no `userId` | `domains/actions.ts:411`, `domains/ai.ts:60`, `domains/automation.ts:989`, `domains/meta.ts:232`, `domains/security.ts:78`, `domains/packages.ts:276`, `external-datasource-routes.ts:302`, `package-routes.ts:98` |
163+
| 52 | Anonymous-deny seam satisfied on the domain dispatchers and the package/federation routes | runtime, rest | Get: passes with no `userId` | `domains/actions.ts:411`, `domains/ai.ts:60`, `domains/automation.ts:989`, `domains/meta.ts:232`, `domains/security.ts:78`, `domains/packages.ts:276`, `external-datasource-routes.ts:302`, `package-routes.ts:97` |
164164
| 53 | MCP principal check satisfied | runtime | Get: MCP surface reachable with no user | `domains/mcp.ts:61` |
165-
| 54 | Package REST route capability gate bypassed | rest | Get: package read/write over REST without `manage_metadata` / `studio.access` / `setup.access` | `package-routes.ts:103` |
165+
| 54 | Package REST route capability gate bypassed | rest | Get: package read/write over REST without `manage_metadata` / `studio.access` / `setup.access` | `package-routes.ts:102` |
166166
| 55 | Package domain capability gates bypassed | runtime | Get: package management and package-inventory reads without the capability | `domains/packages.ts:95`, `:128` |
167167
| 56 | Activation write / authoring refusals do not fire | runtime | Get: activation artifacts writable and authorable without the activation-authoring capability | `activation-gate.ts:139`, `:190` |
168168
| 57 | Automation run-state read, flow-authoring write and unrelated-screen read all pass | runtime | Get: run state, flow writes and screen reads with no grant | `domains/automation.ts:254`, `:545`, `:635` |

packages/observability/src/index.ts

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -42,20 +42,6 @@ export {
4242
ConsoleErrorReporter,
4343
} from './error-exporters.js';
4444

45-
// [#14310] The shared "a 5xx is never silent" rule, read by every transport
46-
// that turns a fault into an HTTP envelope (REST's direct-mount doors and the
47-
// runtime dispatcher's two exits).
48-
export {
49-
logServerFault,
50-
isServerFault,
51-
serverFaultLogMessage,
52-
serverFaultLogMeta,
53-
describeFaultRequest,
54-
SERVER_FAULT_LOG_PREFIX,
55-
type ServerFaultLogInput,
56-
type ServerFaultRequest,
57-
} from './server-fault-log.js';
58-
5945
// Loggers
6046
export {
6147
NoopLogger,

packages/rest/src/package-routes.ts

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import { IHttpServer, shouldDenyAnonymous, ANONYMOUS_DENY_STATUS, ANONYMOUS_DENY
77
// gate's cohort was ruled separately (#7033 / #7023) and pins write-only callers
88
// OUT. Same value it read before — no re-ruling by side effect.
99
import { OBJECT_SCHEMA_READ_ONLY_EXEMPT_CAPABILITIES } from '@objectstack/metadata-core';
10-
import { logServerFault } from '@objectstack/observability';
1110
import type { PackageService } from '@objectstack/service-package';
1211
// The declared envelope is written in ONE place for the whole platform (#3973),
1312
// and so (#8016) is the rule that reads an HTTP answer off a THROWN error.
@@ -266,16 +265,6 @@ function sendThrownError(res: any, error: unknown): void {
266265
...(declaredCode !== undefined ? { declaredCode } : {}),
267266
...(thrown.userMessage !== undefined ? { userMessage: thrown.userMessage } : {}),
268267
};
269-
// [#14310] This registrar's 5xx were silent, and it is the door that mounts
270-
// FIRST in the production stack — so for `/api/v1/packages` the silent
271-
// answer was the live one, which is how the fault this card was filed on
272-
// stayed invisible for a week. `logServerFault` owns the 5xx test, so the
273-
// coded 4xx refusals this exit exists to carry (#8016's `409
274-
// DESTRUCTIVE_CHANGE`, the `[tenant_scope_required]` 400) still cost no
275-
// line. It logs the UNSANITISED `error`: the withhold above is scoped to
276-
// what the CLIENT reads, and an operator losing the driver text to the same
277-
// rule would trade a leak for the blind spot #5437 already refused.
278-
logServerFault({ status: thrown.status, error, code: thrown.code });
279268
sendError(
280269
res,
281270
thrown.status,
@@ -647,14 +636,6 @@ export function registerPackageRoutes(
647636
// for a `PackageService` implementation that reports failure without
648637
// saying why, which is the one thing the old `error?: string` could not
649638
// distinguish from a driver dump.
650-
// [#14310] A REPORTED driver fault never passes through
651-
// {@link sendThrownError} — nothing was thrown — so it owes its own
652-
// line, or this 5xx stays as silent as the thrown ones were.
653-
logServerFault({
654-
status: 500,
655-
code: 'PACKAGE_PUBLISH_FAILED',
656-
message: result.driverFault?.message ?? `Failed to publish ${manifest.id}.`,
657-
});
658639
sendError(
659640
res,
660641
500,
@@ -1014,12 +995,6 @@ export function registerPackageRoutes(
1014995
// producer returns a bare flag with no message channel at all
1015996
// (`PackageDeleteResult`), which is what keeps that true — this route is
1016997
// a status-classification defect only, never a disclosure.
1017-
// [#14310] Same shape as the publish fault above: reported, not thrown.
1018-
logServerFault({
1019-
status: 500,
1020-
code: 'PACKAGE_DELETE_FAILED',
1021-
message: `Failed to delete ${packageId}${version ? `@${version}` : ''}.`,
1022-
});
1023998
sendError(
1024999
res,
10251000
500,

packages/runtime/src/dispatcher-plugin.ts

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
22

33
import { Plugin, PluginContext, IHttpServer, ANONYMOUS_DENY_BODY, ANONYMOUS_DENY_STATUS } from '@objectstack/core';
4-
import { looksLikeInternalErrorLeak, INTERNAL_ERROR_MESSAGE, resolveThrownHttpError, serverFaultProvenance, demotedDeclaredCode } from '@objectstack/types';
4+
import { looksLikeInternalErrorLeak, INTERNAL_ERROR_MESSAGE, resolveThrownHttpError, serverFaultProvenance, demotedDeclaredCode, logServerFault, describeFaultRequest } from '@objectstack/types';
55
import { DispatcherErrorCode } from '@objectstack/spec/api';
6-
import type { IAuthService, IMetadataService } from '@objectstack/spec/contracts';
6+
import type { IAuthService, IMetadataService, Logger } from '@objectstack/spec/contracts';
77
import type { CounterStore } from '@objectstack/plugin-auth/rate-limit-storage';
88
import { HttpDispatcher, HttpDispatcherResult, type HttpProtocolContext } from './http-dispatcher.js';
99
import { isServiceServeable } from './service-serveable.js';
@@ -20,11 +20,6 @@ import {
2020
} from './security/index.js';
2121
import { resolveSessionData, resolveSessionPrincipalId } from './security/resolve-session-principal.js';
2222
import { buildActorUser } from './security/actor-user.js';
23-
import {
24-
logServerFault,
25-
describeFaultRequest,
26-
type Logger,
27-
} from '@objectstack/observability';
2823
import {
2924
NoopMetricsRegistry,
3025
NoopErrorReporter,

packages/types/src/index.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@ export * from './response-envelope.js';
1818
// direct-mount REST registrar, which used to answer 500 INTERNAL_ERROR for a
1919
// coded 4xx the dispatcher mapped correctly.
2020
export * from './thrown-http-error.js';
21+
// [#14310] The sibling rule to the one above, for the same two doors: "is this
22+
// answer worth an operator's attention?". `resolveThrownHttpError` decides what
23+
// the CLIENT is told; this decides what the LOG says — 5xx always, 4xx never.
24+
export * from './server-fault-log.js';
2125
export * from './validation-failure.js';
2226
// [#6615] The one home for Postgres' `«sub-object» "x" of relation "y"` phrase,
2327
// whose missing-COLUMN spelling contains a legal missing-TABLE phrase as a

packages/types/src/response-envelope.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
*/
5252

5353
import type { ApiError, ErrorCode } from '@objectstack/spec/api';
54+
import { logServerFault } from './server-fault-log.js';
5455

5556
/**
5657
* The only thing an envelope builder needs from a response object.
@@ -207,5 +208,22 @@ export function sendError(
207208
message: string,
208209
extra?: Pick<ApiError, 'category' | 'httpStatus' | 'details' | 'requestId' | 'declaredCode' | 'userMessage'>,
209210
): void {
211+
// [#14310] A 5xx is never silent. This writer is the single exit for every
212+
// nested-envelope error in the repo, so the rule is applied ONCE here rather
213+
// than at each registrar's catch block — a per-door call is a thing a new
214+
// door can forget, and the `/api/v1/packages` 500 that motivated the card
215+
// went unlogged for a week through exactly such a door.
216+
//
217+
// ⛔ Not a second opinion about the answer: `logServerFault` reads the same
218+
// `status` this call is about to write, and its own 5xx gate keeps every
219+
// deliberate 4xx refusal — the coded `409 DESTRUCTIVE_CHANGE`, the
220+
// `403 FORBIDDEN` capability denials above it — as quiet as they were.
221+
//
222+
// The thrown value is not available here (callers resolve it into `message`
223+
// before arriving), so this line carries the message and code rather than a
224+
// stack. A door still holding the throw can call `logServerFault` itself for
225+
// the stack-bearing line; none does today, and the transports that DO hold
226+
// it log at their own exits instead.
227+
logServerFault({ status, code, message, ...(extra?.requestId ? { request: { requestId: extra.requestId } } : {}) });
210228
res.status(status).json({ success: false, error: { code, message, ...extra } });
211229
}

packages/observability/src/__tests__/server-fault-log.test.ts renamed to packages/types/src/server-fault-log.test.ts

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ import {
2222
serverFaultLogMeta,
2323
describeFaultRequest,
2424
SERVER_FAULT_LOG_PREFIX,
25-
} from '../server-fault-log.js';
25+
} from './server-fault-log.js';
26+
import { sendError } from './response-envelope.js';
2627

2728
const spyLogger = () => ({
2829
debug: vi.fn(),
@@ -119,6 +120,70 @@ describe('serverFaultLogMessage / serverFaultLogMeta', () => {
119120
});
120121
});
121122

123+
describe('sendError — the funnel every nested-envelope 5xx exits through', () => {
124+
/**
125+
* This is the half that makes the REST direct-mount registrars loud
126+
* without any per-door call: `packages/rest`'s package routes end every
127+
* catch in `sendError`, so wiring the rule HERE covers them (and any door
128+
* added later) by construction rather than by remembering.
129+
*
130+
* The sink is `console.error` because `sendError` takes no logger — it is
131+
* a pure envelope writer reached from ~50 sites that have no logger to
132+
* pass. Spying it is the only way to observe this seam, and it is done
133+
* ONLY here: the behavioural pins that matter (level, exact count, the
134+
* message and stack) assert against an INJECTED logger above and in
135+
* `packages/runtime/src/dispatcher-5xx-always-logged.test.ts`, where a
136+
* console spy would have been the weaker instrument.
137+
*/
138+
const makeRes = () => {
139+
const res: any = {
140+
statusCode: undefined as number | undefined,
141+
body: undefined as any,
142+
status(c: number) { res.statusCode = c; return res; },
143+
json(b: any) { res.body = b; return res; },
144+
};
145+
return res;
146+
};
147+
148+
it('logs a 5xx', () => {
149+
const spy = vi.spyOn(console, 'error').mockImplementation(() => { });
150+
try {
151+
sendError(makeRes(), 500, 'INTERNAL_ERROR', 'the driver fell over');
152+
const lines = spy.mock.calls.filter((c) => String(c[0]).startsWith(SERVER_FAULT_LOG_PREFIX));
153+
expect(lines).toHaveLength(1);
154+
expect(String(lines[0][0])).toContain('the driver fell over');
155+
} finally {
156+
spy.mockRestore();
157+
}
158+
});
159+
160+
it('stays quiet on a 4xx — the coded refusals this door exists to carry', () => {
161+
const spy = vi.spyOn(console, 'error').mockImplementation(() => { });
162+
try {
163+
sendError(makeRes(), 409, 'DESTRUCTIVE_CHANGE', 'that change drops a column');
164+
sendError(makeRes(), 403, 'FORBIDDEN', 'Managing packages requires `manage_metadata`.');
165+
expect(spy.mock.calls.filter((c) => String(c[0]).startsWith(SERVER_FAULT_LOG_PREFIX))).toHaveLength(0);
166+
} finally {
167+
spy.mockRestore();
168+
}
169+
});
170+
171+
it('leaves the wire body byte-identical — this change adds a side effect, not a field', () => {
172+
const spy = vi.spyOn(console, 'error').mockImplementation(() => { });
173+
try {
174+
const res = makeRes();
175+
sendError(res, 500, 'INTERNAL_ERROR', 'boom', { details: { a: 1 } });
176+
expect(res.statusCode).toBe(500);
177+
expect(res.body).toEqual({
178+
success: false,
179+
error: { code: 'INTERNAL_ERROR', message: 'boom', details: { a: 1 } },
180+
});
181+
} finally {
182+
spy.mockRestore();
183+
}
184+
});
185+
});
186+
122187
describe('describeFaultRequest', () => {
123188
it('reads the spellings adapters actually use', () => {
124189
expect(describeFaultRequest({ method: 'GET', path: '/a', requestId: 'r1' }))

packages/observability/src/server-fault-log.ts renamed to packages/types/src/server-fault-log.ts

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,15 +28,18 @@
2828
*
2929
* ## Why it lives here
3030
*
31-
* `@objectstack/rest` and `@objectstack/runtime` both emit 5xx envelopes and
32-
* both already depend on this package, which owns the operator-facing channel
33-
* (`Logger`, `LOG_LEVELS`, `ErrorReporter`). The rule cannot live in either
34-
* consumer: `runtime` depends on `rest`, so an import could only ever point
35-
* one way — the same argument that put `resolveThrownHttpError` in
36-
* `@objectstack/types` rather than in one of its two doors. Centralising it
37-
* is also what keeps the two doors serving `/api/v1/packages` from printing
38-
* two lines for one fault: each door logs at its own single exit, and the
39-
* predicate that decides "is this worth a line" has one definition.
31+
* Same argument, and the same package, as `resolveThrownHttpError` one file
32+
* over: a rule two doors must agree on cannot live inside one of them.
33+
* `@objectstack/runtime` depends on `@objectstack/rest`, so an import between
34+
* the two doors could only ever point one way — which is exactly why the
35+
* "what status does this throw mean" rule was moved here in #8016. "Is this
36+
* answer worth an operator's attention" is the same kind of rule, read by the
37+
* same two doors, so it gets the same home rather than a second one.
38+
*
39+
* Living beside {@link sendError} is what makes the REST side automatic: that
40+
* writer is the single exit for every nested-envelope 5xx, so the direct-mount
41+
* registrars need no per-door call and cannot forget one. Each transport logs
42+
* at its own single exit, so a fault costs one line and never two.
4043
*
4144
* ## `error` level, and why that clears the default
4245
*
@@ -56,7 +59,7 @@
5659
* whole rule: at or above 500.
5760
*/
5861

59-
import type { Logger } from './contracts.js';
62+
import type { Logger } from '@objectstack/spec/contracts';
6063

6164
/** The request coordinates an operator needs to find the failing call. */
6265
export interface ServerFaultRequest {

0 commit comments

Comments
 (0)