Skip to content

Commit 22df871

Browse files
os-helpclaude
andauthored
fix(runtime): answer 404, not 500, when toggling an unknown automation flow (#7535) (#7558)
`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. Cause: `toggleFlow` on an unregistered name throws a plain `Error("Flow '<name>' not found")` (service-automation's engine). It carries no `.status`, so both dispatcher error exits — `errorFromThrown` and the plugin's `errorResponseBase` — fell back to their 500 default. Fixed at the domain handler rather than in a generic catch, on purpose. Which HTTP status a plain domain error means is the serving boundary's decision (the rule `validation-failure.ts` already states for `ValidationError` → 400), and teaching a shared catch to recognise one engine's message string would make every domain's not-found depend on that prose. The handler instead runs the **same existence probe `GET /automation/:name` already uses**, so the two routes cannot disagree 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 this inventing a 404. Six tests in `automation-toggle-unknown-flow.test.ts`, each proved to fail against a mutated source (status 404→500; guard deleted; message stops naming the flow; `!existing` inverted; optional-method guard dropped; probe moved ahead of the body checks). Full `@objectstack/runtime` suite green: 123 files, 1982 tests. Claude-Session: https://claude.ai/code/session_015heAKHUUrVM5GGFgvhf317 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2c1988c commit 22df871

3 files changed

Lines changed: 232 additions & 1 deletion

File tree

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
---
2+
"@objectstack/runtime": patch
3+
---
4+
5+
fix(runtime): toggling an unknown automation flow answers 404, not 500 (#7535)
6+
7+
`POST /api/v1/automation/:name/toggle` against a flow name the registry does not
8+
hold answered **500 `INTERNAL_ERROR`**. It now answers **404
9+
`RESOURCE_NOT_FOUND`**, naming the flow it could not find.
10+
11+
The class was the defect, not the wording. Clients and retry layers branch on
12+
it: 5xx means "the server broke, try again", 4xx means "your request was wrong,
13+
don't". A typo'd flow name presented as a transient server fault, so any
14+
retry-on-5xx caller re-sent — repeatedly — a request that can never succeed.
15+
16+
The cause is that `toggleFlow` on an unregistered name throws a plain
17+
`Error("Flow '<name>' not found")`. It carries no `.status`, so both dispatcher
18+
error exits fell back to their 500 default. The fix is at the domain handler,
19+
which now runs the **same existence probe `GET /automation/:name` already
20+
uses** before touching the service — deciding which HTTP status a plain domain
21+
error means is the serving boundary's job, and sharing one probe keeps the two
22+
routes from disagreeing about which flows exist.
23+
24+
This brings the missing-flow arm up to the standard the endpoint's *body* arm
25+
already met (#3899), where `{"enable": false}` — one letter off — is a located
26+
400 naming the offending key rather than a silent enable. The refusals compose
27+
in that order: a malformed body is still rejected without the registry being
28+
consulted at all.
29+
30+
Unchanged: toggling a real flow in either direction, the documented bodyless
31+
enable, the strict `{ enabled?: boolean }` validation, and any
32+
`IAutomationService` implementation that omits the optional `getFlow` — it
33+
cannot be asked whether a flow exists, so its toggle proceeds exactly as before
34+
rather than inventing a 404.
Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #7535 — `POST /automation/:name/toggle` against a flow that does not exist.
5+
*
6+
* It answered **500 INTERNAL_ERROR**. The engine's `toggleFlow` throws a plain
7+
* `Error("Flow '<name>' not found")` for an unregistered name; the error
8+
* carries no `.status`, so both dispatcher error exits fell back to 500 — the
9+
* server-fault bucket — for a mistake that is entirely the caller's.
10+
*
11+
* The consequence is not cosmetic. Clients and retry layers branch on the
12+
* class: 5xx means "the server broke, try again", 4xx means "your request was
13+
* wrong, don't". A typo'd flow name presented as a transient server fault, so
14+
* a retry-on-5xx client re-sent a request that can never succeed.
15+
*
16+
* The bar is the neighbour on the same endpoint: `{ enabled?: boolean }` is
17+
* strict, and `{"enable": false}` — one letter off — is a LOCATED 400 naming
18+
* the offending key rather than a silent enable (#3899). The missing-flow arm
19+
* now answers in kind: 404 in the house envelope, naming the flow.
20+
*/
21+
22+
import { describe, it, expect, vi } from 'vitest';
23+
24+
import { HttpDispatcher } from '../http-dispatcher.js';
25+
import { validationFailureDetails } from '../validation-failure.js';
26+
27+
/**
28+
* An automation service holding exactly the flows it is given — the shape the
29+
* real engine presents: `getFlow` resolves `null` for an unknown name and
30+
* `toggleFlow` THROWS for one (`engine.ts`: `if (!this.flows.has(name)) throw`).
31+
* Modelling the throw is the point: a fake that quietly succeeded would pass
32+
* whether or not the handler ever checks.
33+
*/
34+
function makeDispatcher(flowNames: string[] = ['welcome_flow']) {
35+
const flows = new Map<string, { name: string; trigger: { type: string } }>(
36+
flowNames.map((n) => [n, { name: n, trigger: { type: 'manual' } }]),
37+
);
38+
const enabled = new Map<string, boolean>();
39+
const spies = {
40+
getFlow: vi.fn(async (name: string) => flows.get(name) ?? null),
41+
toggleFlow: vi.fn(async (name: string, on: boolean) => {
42+
if (!flows.has(name)) throw new Error(`Flow '${name}' not found`);
43+
enabled.set(name, on);
44+
}),
45+
};
46+
const services: Record<string, unknown> = { automation: spies };
47+
const resolve = (name: string) => services[name];
48+
const kernel: any = {
49+
getService: resolve,
50+
getServiceAsync: async (name: string) => resolve(name),
51+
context: { getService: resolve },
52+
};
53+
return { dispatcher: new HttpDispatcher(kernel), spies, enabled };
54+
}
55+
56+
const CTX = { request: {}, executionContext: { userId: 'user_1' } } as any;
57+
58+
describe('#7535 — toggling a flow that does not exist is 404, not 500', () => {
59+
it('answers 404 in the house error envelope, naming the unknown flow', async () => {
60+
const { dispatcher, spies } = makeDispatcher();
61+
62+
const result = await dispatcher.handleAutomation(
63+
'/definitely_not_a_flow/toggle',
64+
'POST',
65+
{ enabled: false },
66+
CTX,
67+
);
68+
69+
expect(result.handled).toBe(true);
70+
// The class, which is the whole defect: a caller mistake, not a fault.
71+
expect(result.response?.status).toBe(404);
72+
73+
const error = result.response?.body?.error;
74+
expect(result.response?.body?.success).toBe(false);
75+
// House envelope (`error-envelope.ts`): a SEMANTIC code, never the
76+
// number, plus the status mirrored onto the body.
77+
expect(error?.code).toBe('RESOURCE_NOT_FOUND');
78+
expect(error?.httpStatus).toBe(404);
79+
// Named, the way the body rejection names the offending key (#3899) —
80+
// a bare "not found" leaves the caller to guess which of path, flow or
81+
// route the server could not resolve.
82+
expect(error?.message).toContain('definitely_not_a_flow');
83+
84+
// Refused before the service was asked to mutate anything.
85+
expect(spies.toggleFlow).not.toHaveBeenCalled();
86+
});
87+
88+
it('does not reach 500 by any other route — no retry-on-5xx client is provoked', async () => {
89+
const { dispatcher } = makeDispatcher();
90+
for (const body of [{ enabled: true }, { enabled: false }, undefined]) {
91+
const result = await dispatcher.handleAutomation('/nope/toggle', 'POST', body, CTX);
92+
expect(result.response?.status, JSON.stringify(body ?? null)).toBe(404);
93+
}
94+
});
95+
96+
it('still toggles a real flow, in both directions', async () => {
97+
const { dispatcher, spies, enabled } = makeDispatcher();
98+
99+
const off = await dispatcher.handleAutomation('/welcome_flow/toggle', 'POST', { enabled: false }, CTX);
100+
expect(off.response?.status).toBe(200);
101+
expect(off.response?.body?.data ?? off.response?.body).toMatchObject({ name: 'welcome_flow', enabled: false });
102+
expect(spies.toggleFlow).toHaveBeenLastCalledWith('welcome_flow', false);
103+
expect(enabled.get('welcome_flow')).toBe(false);
104+
105+
const on = await dispatcher.handleAutomation('/welcome_flow/toggle', 'POST', { enabled: true }, CTX);
106+
expect(on.response?.status).toBe(200);
107+
expect(spies.toggleFlow).toHaveBeenLastCalledWith('welcome_flow', true);
108+
expect(enabled.get('welcome_flow')).toBe(true);
109+
110+
// The documented legacy shape: a bodyless toggle enables.
111+
const legacy = await dispatcher.handleAutomation('/welcome_flow/toggle', 'POST', undefined, CTX);
112+
expect(legacy.response?.status).toBe(200);
113+
expect(spies.toggleFlow).toHaveBeenLastCalledWith('welcome_flow', true);
114+
});
115+
116+
it('leaves #3899 strict-body validation intact — a bad body is still a located 400', async () => {
117+
const { dispatcher, spies } = makeDispatcher();
118+
119+
let thrown: any;
120+
try {
121+
await dispatcher.handleAutomation('/welcome_flow/toggle', 'POST', { enable: false }, CTX);
122+
} catch (e) {
123+
thrown = e;
124+
}
125+
expect(thrown, '{"enable": false} was accepted').toBeDefined();
126+
expect(validationFailureDetails(thrown)?.fields).toMatchObject([{ field: 'enable' }]);
127+
expect(spies.toggleFlow).not.toHaveBeenCalled();
128+
});
129+
130+
it('checks the body BEFORE the registry — a malformed body never triggers a lookup', async () => {
131+
// Order matters for more than tidiness: #3899's guarantee is that
132+
// nothing reaches the service until the body is legal. An existence
133+
// probe running first would consult the registry on a request the
134+
// handler is about to refuse anyway.
135+
const { dispatcher, spies } = makeDispatcher();
136+
137+
await expect(
138+
dispatcher.handleAutomation('/definitely_not_a_flow/toggle', 'POST', { enabled: 'false' }, CTX),
139+
).rejects.toThrow();
140+
141+
expect(spies.getFlow).not.toHaveBeenCalled();
142+
expect(spies.toggleFlow).not.toHaveBeenCalled();
143+
});
144+
145+
it('an implementation without `getFlow` is unchanged — the probe is optional on the contract', async () => {
146+
// `getFlow?` is optional on `IAutomationService`. A service that omits
147+
// it cannot be asked whether a flow exists, so the toggle proceeds
148+
// exactly as it did before rather than this inventing a 404.
149+
const toggleFlow = vi.fn(async () => undefined);
150+
const services: Record<string, unknown> = { automation: { toggleFlow } };
151+
const resolve = (name: string) => services[name];
152+
const kernel: any = {
153+
getService: resolve,
154+
getServiceAsync: async (name: string) => resolve(name),
155+
context: { getService: resolve },
156+
};
157+
const dispatcher = new HttpDispatcher(kernel);
158+
159+
const result = await dispatcher.handleAutomation('/whatever/toggle', 'POST', { enabled: false }, CTX);
160+
expect(result.response?.status).toBe(200);
161+
expect(toggleFlow).toHaveBeenCalledWith('whatever', false);
162+
});
163+
});

packages/runtime/src/domains/automation.ts

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ export function createAutomationDomain(deps: DomainHandlerDeps): DomainRoute {
122122
* PUT /:name → updateFlow
123123
* DELETE /:name → deleteFlow (unregisterFlow)
124124
* POST /:name/trigger → execute (legacy: trigger/:name also supported)
125-
* POST /:name/toggle → toggleFlow
125+
* POST /:name/toggle → toggleFlow (unknown name → 404, #7535)
126126
* GET /:name/runs → listRuns (query: limit, cursor — validated, #7300;
127127
* status — validated AND honoured, #7359)
128128
* GET /:name/runs/:runId → getRun
@@ -369,6 +369,40 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str
369369
]);
370370
}
371371
const enabled = (toggleBody as { enabled?: boolean }).enabled ?? true;
372+
// [#7535] The unknown-FLOW arm, brought up to the standard the
373+
// body arm above already meets. `toggleFlow` on a name the
374+
// registry does not hold throws a plain `Error` ("Flow '<name>'
375+
// not found", service-automation's engine) carrying no
376+
// `.status`, so both dispatcher catches fell back to **500
377+
// INTERNAL_ERROR** for what is purely a caller mistake. That
378+
// tells every client the opposite of the truth: 5xx reads as
379+
// "the server broke, retry", so a typo'd flow name had
380+
// retry-on-5xx callers hammering a request that can never
381+
// succeed. 404 says "your request was wrong" — and names which
382+
// flow was wrong, the way the body rejection names the key.
383+
//
384+
// Answered HERE rather than by teaching a generic catch to
385+
// recognise that message: which HTTP status a plain domain
386+
// error means is the serving boundary's decision (see
387+
// ../validation-failure.ts), and this is the SAME existence
388+
// probe `GET /:name` uses below, so the two routes cannot
389+
// disagree about which flows exist.
390+
//
391+
// Deliberately AFTER the body checks: a malformed body is still
392+
// refused without the registry being consulted at all, so
393+
// #3899's "nothing reaches the service until the body is legal"
394+
// holds unchanged.
395+
//
396+
// `getFlow` is optional on `IAutomationService`; an
397+
// implementation that omits it cannot be asked whether the flow
398+
// exists, so the toggle proceeds as before rather than this
399+
// inventing an answer.
400+
if (typeof automationService.getFlow === 'function') {
401+
const existing = await automationService.getFlow(name);
402+
if (!existing) {
403+
return { handled: true, response: deps.error(`Flow '${name}' not found`, 404) };
404+
}
405+
}
372406
await automationService.toggleFlow(name, enabled);
373407
return { handled: true, response: deps.success({ name, enabled }) };
374408
}

0 commit comments

Comments
 (0)