diff --git a/.changeset/empty-capability-answers-501.md b/.changeset/empty-capability-answers-501.md new file mode 100644 index 0000000000..e80e9ac877 --- /dev/null +++ b/.changeset/empty-capability-answers-501.md @@ -0,0 +1,22 @@ +--- +'@objectstack/runtime': major +--- + +A dispatcher domain whose route is mounted but whose implementation is absent answers **501**, not a 404 that blames the route (#4093 follow-up). + +Two different facts were being answered with whatever each domain happened to reach for, and only `mcp` told them apart: + +- **The route is not there.** `/mcp` when the server is disabled for the environment; `/analytics` when the service is unserveable, because `dispatcher-plugin` gates the *mount* and never registers those paths (#4000). A path the server does not expose is a **404** from the host's own router. **Unchanged** — that half was already right. +- **The route is there; the implementation is not.** Every unconditionally-mounted domain. The request reached a handler that had nothing to delegate to. That is **501**, and it is what changes here. + +`/automation` and `/notifications` returned `{ handled: false }`, which looks neutral but lands on the dispatcher plugin's single exit: `404 ROUTE_NOT_FOUND` with the hint *"No handler matched this request. Check the API discovery endpoint for available routes."* Both halves were false — a handler did match, and discovery correctly does not list the route, so the hint pointed at a page that would never mention it. An operator reads that as a routing bug and goes looking for one that does not exist. + +`/ui` answered **503**, which claims the condition is temporary. An uninstalled MetadataPlugin does not become installed by retrying. + +`/ai` answered **404** for the same mounted-but-unimplemented case. + +The refusal now carries the same remedy sentence discovery reports for that slot (`serviceUnavailableMessage`, shared via `@objectstack/spec/system`), so the wall and the discovery entry cannot drift into naming different fixes — `POST /api/v1/automation` on a stack without the service answers `501 Install @objectstack/service-automation to enable`. `/ai` keeps a local message: its real provider ships outside this workspace as a Cloud/EE package, so the shared table — verified against workspace packages — records no entry and would otherwise describe it as "nothing ships". + +Deliberately unchanged: `analytics`'s route-mount gate (a genuinely absent path); `mcp`'s 404-vs-501 pair, which is the model; the `handled: false` at the END of each domain, which means "no sub-route matched" and is a true 404; `GET /ai/agents`'s empty-list 200, a deliberate courtesy for the console's per-navigation poll; and `/ai`'s `503 routes not yet initialized`, which is a different condition (service present, internal state unready) and may genuinely be transient. + +FROM → TO: requests to `/automation`, `/notifications`, `/ui/*` and `/ai/*` on a deployment lacking the backing service now get `501` (with the package to install) instead of `404`/`503`. Anything branching on the old status should branch on `status >= 400` or read the error code; the capability was equally unavailable before, so no working flow changes. Discovery is unaffected — it already reported these slots `unavailable` and advertised no route for them. diff --git a/content/docs/api/index.mdx b/content/docs/api/index.mdx index a65a83d6a9..91b9e79bdd 100644 --- a/content/docs/api/index.mdx +++ b/content/docs/api/index.mdx @@ -142,6 +142,19 @@ Served by the runtime dispatcher (`@objectstack/runtime`), not `@objectstack/res **Service Status Values**: `available` (fully operational), `registered` (route declared but handler unverified — may return 501), `degraded` (partial functionality), `unavailable` (not installed), `stub` (placeholder that throws errors) +### What an absent capability answers + +Discovery never advertises a route for a service it reports `unavailable`. If you call one anyway, the status tells you **which kind of absence** you hit: + +| You get | Meaning | Example | +| :--- | :--- | :--- | +| **404** | The route is not mounted. The server does not expose this path at all. | `/analytics/*` without an analytics service — the mount itself is gated; `/mcp` when the MCP server is disabled for the environment | +| **501** | The route is mounted; nothing implements it. The request reached a handler that had nothing to delegate to. | `/automation`, `/notifications`, `/ui/*`, `/ai/*`, `/auth/*`, `/i18n/*`, `/graphql` without their backing service | + +A 501 body names the package that would provide the capability — the same sentence `services..message` carries in discovery, so the wall and the discovery entry always agree. A 404 here means what 404 always means: check the path. + +Neither is retryable. Nothing answers **503** for a missing capability; that status is reserved for genuinely transient states (the kernel still booting, on `GET /ready`). + --- ## Error Handling diff --git a/packages/runtime/src/domain-handler-registry.test.ts b/packages/runtime/src/domain-handler-registry.test.ts index a4136063fe..47120dcd17 100644 --- a/packages/runtime/src/domain-handler-registry.test.ts +++ b/packages/runtime/src/domain-handler-registry.test.ts @@ -351,14 +351,18 @@ describe('HttpDispatcher extracted domains (PR-3: keys/storage/ui)', () => { expect(download).not.toHaveBeenCalled(); }); - it('/ui/view/:object serves the protocol getUiView result; 503 without a protocol service', async () => { + it('/ui/view/:object serves the protocol getUiView result; 501 without a protocol service', async () => { const getUiView = vi.fn().mockResolvedValue({ view: 'list-def' }); const ok = await makeDispatcher({ protocol: { getUiView } }).dispatch('GET', '/ui/view/account/list', undefined, {}, {} as any); expect(ok.response?.status).toBe(200); expect(getUiView).toHaveBeenCalledWith({ object: 'account', type: 'list' }); + // [#4093 follow-up] Was 503. The route is mounted and the + // implementation is absent — 501. 503 claimed the condition was + // temporary; an uninstalled MetadataPlugin does not install itself. const missing = await makeDispatcher().dispatch('GET', '/ui/view/account', undefined, {}, {} as any); - expect(missing.response?.status).toBe(503); + expect(missing.response?.status).toBe(501); + expect(missing.response?.body?.error?.message ?? '').toContain('MetadataPlugin'); }); }); @@ -702,9 +706,13 @@ describe('HttpDispatcher extracted domains (PR-7: auth/ai)', () => { expect(result.response?.body?.agents).toBeUndefined(); }); - it('/ai routes 404 (service missing) for non-agents paths', async () => { + // [#4093 follow-up] Was 404. `/ai/*` is mounted unconditionally, so a + // request with no AI service reached a handler that had nothing to + // delegate to — 501. (`GET /ai/agents` keeps its deliberate empty-list + // 200, asserted separately: the console polls it on every navigation.) + it('/ai routes 501 (service missing) for non-agents paths', async () => { const result = await makeDispatcher().dispatch('POST', '/ai/chat', { q: 'hi' }, {}, {} as any); - expect(result.response?.status).toBe(404); + expect(result.response?.status).toBe(501); }); it('/ai dispatches to a matching cached kernel route with params + user threading', async () => { diff --git a/packages/runtime/src/domains/ai.ts b/packages/runtime/src/domains/ai.ts index 49d50e65aa..d7b2f566d4 100644 --- a/packages/runtime/src/domains/ai.ts +++ b/packages/runtime/src/domains/ai.ts @@ -14,6 +14,7 @@ import { shouldDenyAnonymous, ANONYMOUS_DENY_STATUS, ANONYMOUS_DENY_CODE, ANONYMOUS_DENY_MESSAGE, } from '@objectstack/core'; import { isServiceServeable } from '../service-serveable.js'; +import { capabilityUnavailable } from './unavailable.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; @@ -66,7 +67,14 @@ export async function handleAIRequest(deps: DomainHandlerDeps, subPath: string, } // [#3842] Was a hand-rolled envelope with the status in `code`. It has // no header or shape of its own, so it is simply the shared exit now. - return { handled: true, response: deps.error('AI service is not configured', 404) }; + // 501, not 404: `/ai/*` IS mounted, so the request reached a handler + // with nothing behind it — see ./unavailable.ts. The message stays + // local rather than using the shared sentence: the real provider + // (`@objectstack/service-ai`) ships outside this workspace as a + // Cloud/EE package, so CORE_SERVICE_PROVIDER — verified against + // workspace packages by check:service-providers — records `null` for + // this slot and would describe it as "nothing ships", which is wrong. + return capabilityUnavailable(deps, 'ai', 'AI service is not configured'); } // The AI service exposes route definitions via buildAIRoutes. diff --git a/packages/runtime/src/domains/automation.ts b/packages/runtime/src/domains/automation.ts index 3406d46dc2..61f200469d 100644 --- a/packages/runtime/src/domains/automation.ts +++ b/packages/runtime/src/domains/automation.ts @@ -12,6 +12,7 @@ import { CoreServiceName } from '@objectstack/spec/system'; import type { IAutomationService } from '@objectstack/spec/contracts'; import { isServiceServeable } from '../service-serveable.js'; +import { capabilityUnavailable } from './unavailable.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; @@ -119,8 +120,12 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str // automation capability. This domain is the sharpest case for the rule: a // stub whose `execute` returns `{ success: true }` without running anything // answered 200, so a caller (or an agent) read "flow executed" off a flow - // that never ran. 404 handled by caller. - if (!isServiceServeable(automationService)) return { handled: false }; + // that never ran. + // + // 501, not the `handled: false` this used to return: `/automation` IS + // mounted, so the dispatcher's ROUTE_NOT_FOUND exit ("No handler matched + // this request") described neither half truthfully. See ./unavailable.ts. + if (!isServiceServeable(automationService)) return capabilityUnavailable(deps, 'automation'); const m = method.toUpperCase(); const parts = path.replace(/^\/+/, '').split('/').filter(Boolean); diff --git a/packages/runtime/src/domains/notifications.ts b/packages/runtime/src/domains/notifications.ts index 6359e0b50d..d6538056a6 100644 --- a/packages/runtime/src/domains/notifications.ts +++ b/packages/runtime/src/domains/notifications.ts @@ -22,6 +22,7 @@ import { CoreServiceName } from '@objectstack/spec/system'; import type { INotificationService } from '@objectstack/spec/contracts'; import { isServiceServeable } from '../service-serveable.js'; +import { capabilityUnavailable } from './unavailable.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; @@ -67,8 +68,13 @@ export async function handleNotificationRequest( // The dev stub implementing exactly `send`/`sendBatch` was never a bug on // its part: it followed the contract, and the contract was the incomplete // thing. + // + // 501 rather than the `handled: false` this used to return — `/notifications` + // is mounted, so that produced a ROUTE_NOT_FOUND whose hint ("check the API + // discovery endpoint") pointed at a page that correctly does not list the + // route. See ./unavailable.ts. The slot key is `notification`, singular. if (!service || !isServiceServeable(service) || typeof service.listInbox !== 'function') { - return { handled: false }; + return capabilityUnavailable(deps, 'notification'); } // Narrowed for the routes below: the entry probe established `listInbox`. const inbox = service as INotificationService & Required>; @@ -105,7 +111,7 @@ export async function handleNotificationRequest( // POST /notifications/read — mark specific notifications read. if (subPath === 'read' && m === 'POST') { - if (typeof inbox.markRead !== 'function') return { handled: false }; + if (typeof inbox.markRead !== 'function') return capabilityUnavailable(deps, 'notification'); const ids: string[] = Array.isArray(body?.ids) ? body.ids.map((x: unknown) => String(x)) : []; const result = await inbox.markRead(userId, ids); return { handled: true, response: deps.success(result) }; @@ -113,7 +119,7 @@ export async function handleNotificationRequest( // POST /notifications/read/all — mark all of the user's inbox read. if (subPath === 'read/all' && m === 'POST') { - if (typeof inbox.markAllRead !== 'function') return { handled: false }; + if (typeof inbox.markAllRead !== 'function') return capabilityUnavailable(deps, 'notification'); const result = await inbox.markAllRead(userId); return { handled: true, response: deps.success(result) }; } diff --git a/packages/runtime/src/domains/ui.ts b/packages/runtime/src/domains/ui.ts index dc0bbbcf74..88242fa122 100644 --- a/packages/runtime/src/domains/ui.ts +++ b/packages/runtime/src/domains/ui.ts @@ -10,6 +10,7 @@ import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; +import { capabilityUnavailable } from './unavailable.js'; export function createUiDomain(deps: DomainHandlerDeps): DomainRoute { return { @@ -44,7 +45,11 @@ export async function handleUiRequest( return { handled: true, response: deps.errorFromThrown(e, 500) }; } } else { - return { handled: true, response: deps.error('Protocol service not available', 503) }; + // 501, not the 503 this used to answer: 503 claims the condition + // is temporary, but an uninstalled MetadataPlugin does not become + // installed by retrying. The message now names that remedy, and is + // the same sentence discovery reports for the slot (#4146). + return capabilityUnavailable(deps, 'ui'); } } diff --git a/packages/runtime/src/domains/unavailable.ts b/packages/runtime/src/domains/unavailable.ts new file mode 100644 index 0000000000..33dbeb4f00 --- /dev/null +++ b/packages/runtime/src/domains/unavailable.ts @@ -0,0 +1,65 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { serviceUnavailableMessage } from '@objectstack/spec/system'; +import type { HttpDispatcherResult } from '../http-dispatcher.js'; +import type { DomainHandlerDeps } from '../domain-handler-registry.js'; + +/** + * The one answer a dispatcher domain gives when its route is mounted but + * nothing implements the capability behind it — **501**, carrying the same + * remedy sentence discovery reports for that slot. + * + * ## The distinction this encodes + * + * Two different facts were being answered with whatever each domain happened + * to reach for. They are not the same fact, and `mcp` is the one domain that + * already told them apart: + * + * - **The route is not there.** `/mcp` when the server is disabled for this + * environment; `/analytics` when the service is unserveable, because + * `dispatcher-plugin` gates the *mount* and never registers those paths + * (#4000). Asking for a path the server does not expose is a **404**, and + * the host's own router says so. Nothing here overrides that. + * + * - **The route is there; the implementation is not.** Every domain mounted + * unconditionally. The request reached a handler — it simply has nothing to + * delegate to. That is **501 Not Implemented**, and it is what this helper + * is for. + * + * ## What it replaces + * + * `return { handled: false }` looked like a neutral "not mine", but the + * dispatcher plugin's single exit turns it into `404 ROUTE_NOT_FOUND` with the + * hint *"No handler matched this request. Check the API discovery endpoint for + * available routes."* Both halves are false: a handler did match, and + * discovery — correctly — does not list the route, so the hint sends the + * caller to a page that will not mention it. An operator reads that as a + * routing bug and goes looking for one that does not exist. + * + * `/ui` said **503**, which claims the condition is temporary. An uninstalled + * MetadataPlugin does not become installed by retrying. + * + * ## Why the message comes from `spec` + * + * `serviceUnavailableMessage` is the same sentence `services..message` + * carries in discovery (#4093 follow-up), so the 501 body and the discovery + * entry cannot drift into naming different remedies — and a caller who hits + * the wall gets the fix without a second round trip. + * + * @param slot the `CoreServiceName` key, NOT the route segment — `/notifications` + * is served by the `notification` slot, and the remedy is looked up + * by slot. + */ +export function capabilityUnavailable( + deps: DomainHandlerDeps, + slot: string, + /** + * Overrides the shared sentence. Only for slots whose provider cannot be + * named by `CORE_SERVICE_PROVIDER` — it is verified against workspace + * packages, so a real provider that ships outside this repo (`ai`) has no + * entry there and would otherwise be described as "nothing ships". + */ + message?: string, +): HttpDispatcherResult { + return { handled: true, response: deps.error(message ?? serviceUnavailableMessage(slot), 501) }; +} diff --git a/packages/runtime/src/http-dispatcher.test.ts b/packages/runtime/src/http-dispatcher.test.ts index 8fa6cf2c4b..3ba4fec20a 100644 --- a/packages/runtime/src/http-dispatcher.test.ts +++ b/packages/runtime/src/http-dispatcher.test.ts @@ -855,10 +855,16 @@ describe('HttpDispatcher', () => { expect(service.listInbox).not.toHaveBeenCalled(); }); - it('is unhandled (→ 404) when no notification service is registered', async () => { + // [#4093 follow-up] Was `handled: false` (→ the dispatcher's + // ROUTE_NOT_FOUND 404). `/notifications` is mounted, so that 404's + // hint — "No handler matched this request" — was false, and it + // pointed at a discovery page that correctly omits the route. + it('answers 501 when no notification service is registered', async () => { const d = new HttpDispatcher(notifKernel(null)); const result = await d.handleNotification('', 'GET', undefined, {}, ctx('u1')); - expect(result.handled).toBe(false); + expect(result.handled).toBe(true); + expect(result.response?.status).toBe(501); + expect(result.response?.body?.error?.message ?? '').toContain('service-messaging'); }); }); @@ -1101,12 +1107,17 @@ describe('HttpDispatcher', () => { expect(result.response?.body?.data?.flows).toEqual(['f1']); }); - it('should return unhandled when automation service not registered', async () => { + // [#4093 follow-up] Was `handled: false` → 404; now 501 with the + // remedy, because the route is mounted and only the implementation + // is missing. + it('answers 501 when the automation service is not registered', async () => { (kernel as any).getService = vi.fn().mockResolvedValue(null); (kernel as any).services = new Map(); const result = await dispatcher.handleAutomation('', 'GET', {}, { request: {} }); - expect(result.handled).toBe(false); + expect(result.handled).toBe(true); + expect(result.response?.status).toBe(501); + expect(result.response?.body?.error?.message ?? '').toContain('service-automation'); }); }); @@ -2685,9 +2696,13 @@ describe('HttpDispatcher', () => { }); serveOnly('automation', stub); + // [#4093 follow-up] The answer is 501 now, not `handled: false`. + // What this test pins is unchanged and is the part that matters: + // the stub is NEVER CALLED, so nothing can read "flow executed" + // off a flow that never ran. for (const [path, method] of [['', 'GET'], ['', 'POST'], ['trigger/x', 'POST'], ['x/trigger', 'POST']] as const) { const result = await dispatcher.handleAutomation(path, method, { name: 'x' }, { request: {} }); - expect(result.handled, `${method} /automation/${path}`).toBe(false); + expect(result.response?.status, `${method} /automation/${path}`).toBe(501); } expect(stub.execute).not.toHaveBeenCalled(); expect(stub.trigger).not.toHaveBeenCalled(); @@ -2745,7 +2760,7 @@ describe('HttpDispatcher', () => { const result = await dispatcher.handleNotification('', 'GET', undefined, {}, { request: {}, executionContext: { userId: 'usr_1' }, } as any); - expect(result.handled).toBe(false); + expect(result.response?.status).toBe(501); expect(stub.listInbox).not.toHaveBeenCalled(); }); @@ -2753,13 +2768,13 @@ describe('HttpDispatcher', () => { // reads as a fault AND loses the empty-list courtesy the console's // per-navigation `GET /ai/agents` poll depends on. Treating it as an // empty slot restores both. - it('/ai — a stub slot 404s per route and keeps the /ai/agents empty list', async () => { + it('/ai — a stub slot 501s per route and keeps the /ai/agents empty list', async () => { const stub = stubbed({ chat: vi.fn(), listModels: vi.fn() }); serveOnly('ai', stub); const chat = await dispatcher.handleAI('/ai/chat', 'POST', { messages: [] }, {}, { request: {} }); expect(chat.handled).toBe(true); - expect(chat.response?.status).toBe(404); + expect(chat.response?.status).toBe(501); expect(stub.chat).not.toHaveBeenCalled(); const agents = await dispatcher.handleAI('/ai/agents', 'GET', undefined, {}, { request: {} }); @@ -2880,8 +2895,8 @@ describe('HttpDispatcher', () => { it('a ui-slot occupant buys no route: the old dev-boot shape stays un-advertised and un-served', async () => { // What plugin-dev used to register: a shapeless placeholder in the - // `ui` slot, no protocol anywhere. /ui could only 503 — discovery - // must say so instead of advertising it. + // `ui` slot, no protocol anywhere. /ui could only refuse — + // discovery must say so instead of advertising it. const placeholder = { _serviceName: 'ui', __serviceInfo: { status: 'stub', handlerReady: false, message: 'Dev placeholder' } }; serveMap({ ui: placeholder }); @@ -2891,10 +2906,10 @@ describe('HttpDispatcher', () => { const served = await dispatcher.handleUi('/view/account', {}, { request: {} }); expect(served.handled).toBe(true); - expect(served.response?.status).toBe(503); + expect(served.response?.status).toBe(501); }); - it('a wrong-shaped protocol (no getUiView) is not advertised — mirrors the domain 503', async () => { + it('a wrong-shaped protocol (no getUiView) is not advertised — mirrors the domain 501', async () => { serveMap({ protocol: { saveMetaItem: vi.fn() } }); const info = await dispatcher.getDiscoveryInfo('/api/v1'); diff --git a/packages/runtime/src/route-parity.integration.test.ts b/packages/runtime/src/route-parity.integration.test.ts index 243ee65b27..b48c9430c8 100644 --- a/packages/runtime/src/route-parity.integration.test.ts +++ b/packages/runtime/src/route-parity.integration.test.ts @@ -235,12 +235,19 @@ describe('Route parity: discovery is service-aware — no dead advertisement (#3 ).toBeFalsy(); }); - it('a request to the un-provisioned notifications route resolves to 404 (consistent with not advertising it)', async () => { - // Not advertised AND 404 → declared === enforced (neither over- nor - // under-promised). The failure mode #3369 forbids is the inverse: - // advertised in discovery yet 404 on the listener. + it('a request to the un-provisioned notifications route refuses (consistent with not advertising it)', async () => { + // Not advertised AND not served → declared === enforced (neither over- + // nor under-promised). The failure mode #3369 forbids is the inverse: + // advertised in discovery yet refused on the listener. + // + // [#4093 follow-up] The refusal is 501, not 404. The invariant this + // test protects is unchanged — 501 is just as much "not served" — and + // it expresses it better: the route IS mounted, so a 404 claiming "no + // handler matched" sent the operator hunting for a routing bug. The + // body now names the package that would provide the capability. const res = await fetch(`${baseUrl}/api/v1/notifications`, { headers: { 'x-test-user': 'admin1' } }); - expect(res.status).toBe(404); + expect(res.status).toBe(501); + expect(JSON.stringify(await res.json())).toContain('service-messaging'); }); }); diff --git a/scripts/check-route-envelope.mjs b/scripts/check-route-envelope.mjs index 90689102fd..c2553d07df 100644 --- a/scripts/check-route-envelope.mjs +++ b/scripts/check-route-envelope.mjs @@ -210,6 +210,12 @@ const DISPATCHER_DOMAINS = { // is audited in that package's own routes, not here. 'ui.ts': { handBuilt: 0 }, + // [#4093 follow-up] Not a domain — the shared 501 every mounted-but- + // unimplemented domain answers with. It exists precisely so that refusal is + // built in ONE place instead of once per domain, which is this check's own + // thesis; it answers through `deps.error` like any domain body. + 'unavailable.ts': { handBuilt: 0 }, + // Kind 1 — enveloped, hand-built only because the helper cannot say it. 'keys.ts': { handBuilt: 1,