diff --git a/.changeset/automation-toggle-unknown-flow-404.md b/.changeset/automation-toggle-unknown-flow-404.md new file mode 100644 index 0000000000..d1c7d9eb81 --- /dev/null +++ b/.changeset/automation-toggle-unknown-flow-404.md @@ -0,0 +1,34 @@ +--- +"@objectstack/runtime": patch +--- + +fix(runtime): toggling an unknown automation flow answers 404, not 500 (#7535) + +`POST /api/v1/automation/:name/toggle` against a flow name the registry does not +hold answered **500 `INTERNAL_ERROR`**. It now answers **404 +`RESOURCE_NOT_FOUND`**, naming the flow it could not find. + +The class was the defect, not the wording. Clients and retry layers branch on +it: 5xx means "the server broke, try again", 4xx means "your request was wrong, +don't". A typo'd flow name presented as a transient server fault, so any +retry-on-5xx caller re-sent — repeatedly — a request that can never succeed. + +The cause is that `toggleFlow` on an unregistered name throws a plain +`Error("Flow '' not found")`. It carries no `.status`, so both dispatcher +error exits fell back to their 500 default. The fix is at the domain handler, +which now runs the **same existence probe `GET /automation/:name` already +uses** before touching the service — deciding which HTTP status a plain domain +error means is the serving boundary's job, and sharing one probe keeps the two +routes from disagreeing about which flows exist. + +This brings the missing-flow arm up to the standard the endpoint's *body* arm +already met (#3899), where `{"enable": false}` — one letter off — is a located +400 naming the offending key rather than a silent enable. The refusals compose +in that order: a malformed body is still rejected without the registry being +consulted at all. + +Unchanged: toggling a real flow in either direction, the documented bodyless +enable, the strict `{ enabled?: boolean }` validation, and any +`IAutomationService` implementation that omits the optional `getFlow` — it +cannot be asked whether a flow exists, so its toggle proceeds exactly as before +rather than inventing a 404. diff --git a/packages/runtime/src/domains/automation-toggle-unknown-flow.test.ts b/packages/runtime/src/domains/automation-toggle-unknown-flow.test.ts new file mode 100644 index 0000000000..3f1f1fdaf4 --- /dev/null +++ b/packages/runtime/src/domains/automation-toggle-unknown-flow.test.ts @@ -0,0 +1,163 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #7535 — `POST /automation/:name/toggle` against a flow that does not exist. + * + * It answered **500 INTERNAL_ERROR**. The engine's `toggleFlow` throws a plain + * `Error("Flow '' not found")` for an unregistered name; the error + * carries no `.status`, so both dispatcher error exits fell back to 500 — the + * server-fault bucket — for a mistake that is entirely the caller's. + * + * The consequence is not cosmetic. Clients and retry layers branch on the + * class: 5xx means "the server broke, try again", 4xx means "your request was + * wrong, don't". A typo'd flow name presented as a transient server fault, so + * a retry-on-5xx client re-sent a request that can never succeed. + * + * The bar is the neighbour on the same endpoint: `{ enabled?: boolean }` is + * strict, and `{"enable": false}` — one letter off — is a LOCATED 400 naming + * the offending key rather than a silent enable (#3899). The missing-flow arm + * now answers in kind: 404 in the house envelope, naming the flow. + */ + +import { describe, it, expect, vi } from 'vitest'; + +import { HttpDispatcher } from '../http-dispatcher.js'; +import { validationFailureDetails } from '../validation-failure.js'; + +/** + * An automation service holding exactly the flows it is given — the shape the + * real engine presents: `getFlow` resolves `null` for an unknown name and + * `toggleFlow` THROWS for one (`engine.ts`: `if (!this.flows.has(name)) throw`). + * Modelling the throw is the point: a fake that quietly succeeded would pass + * whether or not the handler ever checks. + */ +function makeDispatcher(flowNames: string[] = ['welcome_flow']) { + const flows = new Map( + flowNames.map((n) => [n, { name: n, trigger: { type: 'manual' } }]), + ); + const enabled = new Map(); + const spies = { + getFlow: vi.fn(async (name: string) => flows.get(name) ?? null), + toggleFlow: vi.fn(async (name: string, on: boolean) => { + if (!flows.has(name)) throw new Error(`Flow '${name}' not found`); + enabled.set(name, on); + }), + }; + const services: Record = { automation: spies }; + const resolve = (name: string) => services[name]; + const kernel: any = { + getService: resolve, + getServiceAsync: async (name: string) => resolve(name), + context: { getService: resolve }, + }; + return { dispatcher: new HttpDispatcher(kernel), spies, enabled }; +} + +const CTX = { request: {}, executionContext: { userId: 'user_1' } } as any; + +describe('#7535 — toggling a flow that does not exist is 404, not 500', () => { + it('answers 404 in the house error envelope, naming the unknown flow', async () => { + const { dispatcher, spies } = makeDispatcher(); + + const result = await dispatcher.handleAutomation( + '/definitely_not_a_flow/toggle', + 'POST', + { enabled: false }, + CTX, + ); + + expect(result.handled).toBe(true); + // The class, which is the whole defect: a caller mistake, not a fault. + expect(result.response?.status).toBe(404); + + const error = result.response?.body?.error; + expect(result.response?.body?.success).toBe(false); + // House envelope (`error-envelope.ts`): a SEMANTIC code, never the + // number, plus the status mirrored onto the body. + expect(error?.code).toBe('RESOURCE_NOT_FOUND'); + expect(error?.httpStatus).toBe(404); + // Named, the way the body rejection names the offending key (#3899) — + // a bare "not found" leaves the caller to guess which of path, flow or + // route the server could not resolve. + expect(error?.message).toContain('definitely_not_a_flow'); + + // Refused before the service was asked to mutate anything. + expect(spies.toggleFlow).not.toHaveBeenCalled(); + }); + + it('does not reach 500 by any other route — no retry-on-5xx client is provoked', async () => { + const { dispatcher } = makeDispatcher(); + for (const body of [{ enabled: true }, { enabled: false }, undefined]) { + const result = await dispatcher.handleAutomation('/nope/toggle', 'POST', body, CTX); + expect(result.response?.status, JSON.stringify(body ?? null)).toBe(404); + } + }); + + it('still toggles a real flow, in both directions', async () => { + const { dispatcher, spies, enabled } = makeDispatcher(); + + const off = await dispatcher.handleAutomation('/welcome_flow/toggle', 'POST', { enabled: false }, CTX); + expect(off.response?.status).toBe(200); + expect(off.response?.body?.data ?? off.response?.body).toMatchObject({ name: 'welcome_flow', enabled: false }); + expect(spies.toggleFlow).toHaveBeenLastCalledWith('welcome_flow', false); + expect(enabled.get('welcome_flow')).toBe(false); + + const on = await dispatcher.handleAutomation('/welcome_flow/toggle', 'POST', { enabled: true }, CTX); + expect(on.response?.status).toBe(200); + expect(spies.toggleFlow).toHaveBeenLastCalledWith('welcome_flow', true); + expect(enabled.get('welcome_flow')).toBe(true); + + // The documented legacy shape: a bodyless toggle enables. + const legacy = await dispatcher.handleAutomation('/welcome_flow/toggle', 'POST', undefined, CTX); + expect(legacy.response?.status).toBe(200); + expect(spies.toggleFlow).toHaveBeenLastCalledWith('welcome_flow', true); + }); + + it('leaves #3899 strict-body validation intact — a bad body is still a located 400', async () => { + const { dispatcher, spies } = makeDispatcher(); + + let thrown: any; + try { + await dispatcher.handleAutomation('/welcome_flow/toggle', 'POST', { enable: false }, CTX); + } catch (e) { + thrown = e; + } + expect(thrown, '{"enable": false} was accepted').toBeDefined(); + expect(validationFailureDetails(thrown)?.fields).toMatchObject([{ field: 'enable' }]); + expect(spies.toggleFlow).not.toHaveBeenCalled(); + }); + + it('checks the body BEFORE the registry — a malformed body never triggers a lookup', async () => { + // Order matters for more than tidiness: #3899's guarantee is that + // nothing reaches the service until the body is legal. An existence + // probe running first would consult the registry on a request the + // handler is about to refuse anyway. + const { dispatcher, spies } = makeDispatcher(); + + await expect( + dispatcher.handleAutomation('/definitely_not_a_flow/toggle', 'POST', { enabled: 'false' }, CTX), + ).rejects.toThrow(); + + expect(spies.getFlow).not.toHaveBeenCalled(); + expect(spies.toggleFlow).not.toHaveBeenCalled(); + }); + + it('an implementation without `getFlow` is unchanged — the probe is optional on the contract', async () => { + // `getFlow?` is optional on `IAutomationService`. A service that omits + // it cannot be asked whether a flow exists, so the toggle proceeds + // exactly as it did before rather than this inventing a 404. + const toggleFlow = vi.fn(async () => undefined); + const services: Record = { automation: { toggleFlow } }; + const resolve = (name: string) => services[name]; + const kernel: any = { + getService: resolve, + getServiceAsync: async (name: string) => resolve(name), + context: { getService: resolve }, + }; + const dispatcher = new HttpDispatcher(kernel); + + const result = await dispatcher.handleAutomation('/whatever/toggle', 'POST', { enabled: false }, CTX); + expect(result.response?.status).toBe(200); + expect(toggleFlow).toHaveBeenCalledWith('whatever', false); + }); +}); diff --git a/packages/runtime/src/domains/automation.ts b/packages/runtime/src/domains/automation.ts index 244abdc088..84ec853a64 100644 --- a/packages/runtime/src/domains/automation.ts +++ b/packages/runtime/src/domains/automation.ts @@ -122,7 +122,7 @@ export function createAutomationDomain(deps: DomainHandlerDeps): DomainRoute { * PUT /:name → updateFlow * DELETE /:name → deleteFlow (unregisterFlow) * POST /:name/trigger → execute (legacy: trigger/:name also supported) - * POST /:name/toggle → toggleFlow + * POST /:name/toggle → toggleFlow (unknown name → 404, #7535) * GET /:name/runs → listRuns (query: limit, cursor — validated, #7300; * status — validated AND honoured, #7359) * GET /:name/runs/:runId → getRun @@ -369,6 +369,40 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str ]); } const enabled = (toggleBody as { enabled?: boolean }).enabled ?? true; + // [#7535] The unknown-FLOW arm, brought up to the standard the + // body arm above already meets. `toggleFlow` on a name the + // registry does not hold throws a plain `Error` ("Flow '' + // not found", service-automation's engine) carrying no + // `.status`, so both dispatcher catches fell back to **500 + // INTERNAL_ERROR** for what is purely a caller mistake. That + // tells every client the opposite of the truth: 5xx reads as + // "the server broke, retry", so a typo'd flow name had + // retry-on-5xx callers hammering a request that can never + // succeed. 404 says "your request was wrong" — and names which + // flow was wrong, the way the body rejection names the key. + // + // Answered HERE rather than by teaching a generic catch to + // recognise that message: which HTTP status a plain domain + // error means is the serving boundary's decision (see + // ../validation-failure.ts), and this is the SAME existence + // probe `GET /:name` uses below, so the two routes cannot + // disagree about which flows exist. + // + // Deliberately AFTER the body checks: a malformed body is still + // refused without the registry being consulted at all, so + // #3899's "nothing reaches the service until the body is legal" + // holds unchanged. + // + // `getFlow` is optional on `IAutomationService`; an + // implementation that omits it cannot be asked whether the flow + // exists, so the toggle proceeds as before rather than this + // inventing an answer. + if (typeof automationService.getFlow === 'function') { + const existing = await automationService.getFlow(name); + if (!existing) { + return { handled: true, response: deps.error(`Flow '${name}' not found`, 404) }; + } + } await automationService.toggleFlow(name, enabled); return { handled: true, response: deps.success({ name, enabled }) }; }