Skip to content

Commit 95ef5c0

Browse files
hotlongclaude
andauthored
fix(runtime): PUT /automation/:name answers 400 VALIDATION_FAILED, not 500, matching POST (#8123) (#8177)
PUT /:name made the identical registerFlow call POST / makes, in the same file, with no classification around it — so the two doors disagreed about the class of an identical malformed-flow refusal after #8055 fixed POST alone. PUT now routes through the same module-local, route-agnostic flowDefinitionRefusal helper POST uses. Adds a cross-door parity suite that drives both routes with the same four bad bodies from #8055/#8123 and asserts the two responses agree on status, code, and details.fields[] shape — not just that PUT looks right in isolation, which is exactly the kind of pin that let the two drift apart the first time. Claude-Session: https://claude.ai/code/session_01B3Kurx8qufrDzNjk4rag7V Co-authored-by: Claude <noreply@anthropic.com>
1 parent 30536e3 commit 95ef5c0

3 files changed

Lines changed: 338 additions & 1 deletion

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
'@objectstack/runtime': patch
3+
---
4+
5+
`PUT /api/v1/automation/:name` answers 400 `VALIDATION_FAILED`, not 500, for a malformed flow definition — matching `POST /api/v1/automation` (#8123)
6+
7+
#8055 reclassified `POST /`'s `registerFlow` refusal from 500 `INTERNAL_ERROR` to 400 `VALIDATION_FAILED`. `PUT /:name` makes the identical `registerFlow` call in the same file and was left uncaught, so the two doors disagreed about the class of an identical refusal: publishing a flow through update got a different answer than publishing through create, for the same broken definition.
8+
9+
`PUT /:name` now routes its `registerFlow` call through the same `flowDefinitionRefusal` helper `POST /` uses, so both doors answer the same `code`, `status`, and `details.fields[]` shape for the same malformed body — including the #4277 self-correcting undeclared-config-key message, which survives verbatim.
10+
11+
**Which bodies are refused is unchanged** on both routes — a definition that registered before still registers, and every one that was refused is still refused, with the engine's own message intact.
Lines changed: 314 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,314 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #8123 — `POST /api/v1/automation` and `PUT /api/v1/automation/:name` must
5+
* answer the SAME class for the SAME malformed flow definition.
6+
*
7+
* #8055 fixed the class on `POST /` alone: a refusal thrown by
8+
* `automationService.registerFlow` used to escape as a 500 `INTERNAL_ERROR`,
9+
* and is now caught and reclassified through the module-local, route-agnostic
10+
* `flowDefinitionRefusal` helper into a 400 `VALIDATION_FAILED` with an
11+
* ADR-0114 `details.fields[]`. `PUT /:name` makes the identical
12+
* `registerFlow` call in the same file and, until this card, had no
13+
* classification around it at all — so the two doors disagreed about the
14+
* class of an identical refusal, the exact drift #7535's fix on the sibling
15+
* `/toggle` route was shaped to avoid ("the two routes cannot disagree").
16+
*
17+
* ## Why a per-route pin is not the bar
18+
*
19+
* A suite that only asserts `PUT` in isolation (as `automation-register-
20+
* error-class.test.ts` does for `POST`) would have stayed green through the
21+
* exact bug this card fixes: `PUT` was individually "consistent" with its own
22+
* (wrong) 500 the whole time. The pin that actually closes the drift has to
23+
* DRIVE BOTH DOORS with the same body and COMPARE the two responses, so that
24+
* reverting either door's classification — not just PUT's — reddens this
25+
* file. See the reverse-verification note in the PR description for the
26+
* measured failure text.
27+
*
28+
* ## The fake
29+
*
30+
* Same fake as `automation-register-error-class.test.ts` (#8055): three of
31+
* the four cases run the REAL `FlowSchema.parse` / `validateControlFlow` from
32+
* `@objectstack/spec/automation` — the very calls
33+
* `AutomationEngine.canonicalizeStoredFlow` makes — and the fourth
34+
* (#4277's undeclared config key) is reproduced from the engine's own
35+
* construction, because that check lives in `@objectstack/service-automation`
36+
* which `@objectstack/runtime` does not depend on. Duplicated here rather
37+
* than imported from that file: each domain test file in this package
38+
* constructs its own fake dispatcher, and importing test fixtures across
39+
* suites is not the existing convention.
40+
*/
41+
42+
import { describe, it, expect } from 'vitest';
43+
import { FlowSchema, validateControlFlow } from '@objectstack/spec/automation';
44+
45+
import { HttpDispatcher } from '../http-dispatcher.js';
46+
47+
/** Config keys the fake's `notify` descriptor declares (the #4277 legal set). */
48+
const NOTIFY_DECLARED_CONFIG_KEYS = ['message', 'recipients', 'channel'];
49+
50+
/**
51+
* The #4277 refusal, reproduced from `service-automation/src/engine.ts`
52+
* (`validateNodeConfigKeys` + `collectUndeclaredConfigKeys`) — see
53+
* `automation-register-error-class.test.ts` for the full derivation note.
54+
*/
55+
function undeclaredConfigKeyRefusal(flowName: string, nodeId: string, nodeType: string, key: string): Error {
56+
const violation =
57+
`node '${nodeId}' (${nodeType}): unknown config key \`${key}\` at config.${key}` +
58+
` It is not declared by this node type's configSchema, so nothing reads it.` +
59+
` Declared here: ${NOTIFY_DECLARED_CONFIG_KEYS.join(', ')}.`;
60+
return new Error(
61+
`Flow '${flowName}' rejected: 1 undeclared config key(s) (#4277).\n` +
62+
` - ${violation}\n` +
63+
`An undeclared key is never read, so it can only be a typo or dead config — fix the ` +
64+
`flow's metadata (rename or remove the key). If an executor genuinely reads this key, ` +
65+
`declare it on the node type's descriptor configSchema instead; read-but-undeclared ` +
66+
`keys are exactly the drift the #4045 reconciliation closed.`,
67+
);
68+
}
69+
70+
/**
71+
* A fresh dispatcher per call, backed by an automation service that refuses
72+
* the same definitions the real engine refuses, in the same order
73+
* `AutomationEngine.registerFlow` runs them: schema parse → control-flow
74+
* regions → #4277 undeclared config keys. One instance per call so a POST
75+
* probe and a PUT probe never share call-count state.
76+
*/
77+
function makeDispatcher() {
78+
const registered = new Map<string, unknown>();
79+
80+
const spies = {
81+
registerFlow: (name: string, definition: unknown) => {
82+
// Recorded before any gate runs — "the engine was consulted" must
83+
// hold whether the call ends in a refusal or a registration.
84+
calls.push(name);
85+
const parsed = FlowSchema.parse(definition) as { nodes?: Array<Record<string, any>> };
86+
validateControlFlow(parsed as any);
87+
for (const node of parsed.nodes ?? []) {
88+
if (node.type !== 'notify') continue;
89+
for (const key of Object.keys(node.config ?? {})) {
90+
if (!NOTIFY_DECLARED_CONFIG_KEYS.includes(key)) {
91+
throw undeclaredConfigKeyRefusal(String((definition as any)?.name), node.id, node.type, key);
92+
}
93+
}
94+
}
95+
registered.set(name, parsed);
96+
},
97+
getFlow: async (name: string) => registered.get(name) ?? null,
98+
};
99+
const calls: string[] = [];
100+
const services: Record<string, unknown> = { automation: spies };
101+
const resolve = (name: string) => services[name];
102+
const kernel: any = {
103+
getService: resolve,
104+
getServiceAsync: async (name: string) => resolve(name),
105+
context: { getService: resolve },
106+
};
107+
return { dispatcher: new HttpDispatcher(kernel), registered, calls };
108+
}
109+
110+
const CTX = { request: {}, executionContext: { userId: 'user_1' } } as any;
111+
112+
/** A definition that is legal at every gate the fake runs. */
113+
const WELL_FORMED = {
114+
name: 'welcome_flow',
115+
label: 'Welcome',
116+
type: 'autolaunched',
117+
nodes: [{ id: 'n', type: 'notify', label: 'Notify', config: { message: 'hi' } }],
118+
edges: [],
119+
};
120+
121+
/** The four bodies from #8055 / #8123, each one letter away from `WELL_FORMED`. */
122+
const BAD_BODIES = {
123+
/** 1 — a node with no `label` (`FlowSchema.parse`). */
124+
missingNodeLabel: {
125+
...WELL_FORMED,
126+
nodes: [{ id: 'n', type: 'notify', config: { message: 'hi' } }],
127+
},
128+
/** 2 — a node key the schema does not declare (`unrecognized_keys`). */
129+
unknownNodeKey: {
130+
...WELL_FORMED,
131+
nodes: [{ id: 'n', type: 'notify', label: 'Notify', next: 'other' }],
132+
},
133+
/** 3 — a `try_catch` whose `try` region is an array, not a region object. */
134+
malformedRegion: {
135+
...WELL_FORMED,
136+
nodes: [{
137+
id: 'g', type: 'try_catch', label: 'Guard',
138+
config: { try: [], catch: { nodes: [], edges: [] } },
139+
}],
140+
},
141+
/** 4 — a config key the node type's descriptor does not declare (#4277). */
142+
undeclaredConfigKey: {
143+
...WELL_FORMED,
144+
nodes: [{
145+
id: 'n', type: 'notify', label: 'Notify',
146+
config: { message: 'hi', totallyBogusKey: 'oops' },
147+
}],
148+
},
149+
} as const;
150+
151+
function postFlow(dispatcher: HttpDispatcher, body: unknown) {
152+
return dispatcher.handleAutomation('', 'POST', body, CTX);
153+
}
154+
155+
function putFlow(dispatcher: HttpDispatcher, name: string, body: unknown) {
156+
return dispatcher.handleAutomation(`/${name}`, 'PUT', body, CTX);
157+
}
158+
159+
/**
160+
* The whole house envelope for a caller-input refusal — ADR-0112's `code` AND
161+
* `status`, plus an ADR-0114 `fields[]` whose entries have the declared
162+
* shape. Applied to EACH door independently (so a shared regression, e.g.
163+
* both doors going back to 500, is still caught) as well as by direct
164+
* comparison below (so a ONE-SIDED regression is caught too).
165+
*/
166+
function assertValidationEnvelope(res: any, label: string) {
167+
expect(res?.status, `${label}: HTTP status`).toBe(400);
168+
expect(res?.body?.success, label).toBe(false);
169+
expect(res?.body?.error?.code, `${label}: error.code`).toBe('VALIDATION_FAILED');
170+
expect(res?.body?.error?.httpStatus, label).toBe(400);
171+
172+
const fields = res?.body?.error?.details?.fields;
173+
expect(Array.isArray(fields), `${label}: details.fields must be an array`).toBe(true);
174+
expect(fields.length, `${label}: details.fields must not be empty`).toBeGreaterThan(0);
175+
for (const f of fields) {
176+
expect(typeof f.field, `${label}: fields[].field`).toBe('string');
177+
expect(typeof f.code, `${label}: fields[].code`).toBe('string');
178+
expect(typeof f.message, `${label}: fields[].message`).toBe('string');
179+
}
180+
expect(res?.body?.error?.message, label).not.toBe('Internal server error');
181+
return fields;
182+
}
183+
184+
describe('#8123 — POST and PUT agree on the class of an identical flow refusal', () => {
185+
it.each(Object.entries(BAD_BODIES))(
186+
'case "%s": POST and PUT answer the SAME envelope for the identical body',
187+
async (label, body) => {
188+
const post = makeDispatcher();
189+
const postResult: any = await postFlow(post.dispatcher, body);
190+
191+
const put = makeDispatcher();
192+
const putResult: any = await putFlow(put.dispatcher, (body as any).name, body);
193+
194+
// Each door independently: the whole envelope, not just "not 500".
195+
const postFields = assertValidationEnvelope(postResult.response, `POST ${label}`);
196+
const putFields = assertValidationEnvelope(putResult.response, `PUT ${label}`);
197+
198+
// THE PIN: the two doors compared directly, for the SAME body.
199+
// A per-route assertion above would already have caught #8123
200+
// (PUT still 500) — this comparison is what keeps them from
201+
// drifting apart again in either direction, on any future change
202+
// to either branch.
203+
expect(putResult.response.status, `${label}: status parity`).toBe(postResult.response.status);
204+
expect(putResult.response.body.error.code, `${label}: code parity`).toBe(postResult.response.body.error.code);
205+
expect(putFields, `${label}: fields parity`).toEqual(postFields);
206+
expect(putResult.response.body.error.message, `${label}: message parity`)
207+
.toBe(postResult.response.body.error.message);
208+
209+
// The engine was still ASKED on both doors — a classification of
210+
// its verdict, not a new pre-check that changed which bodies
211+
// reach it.
212+
expect(post.calls, `${label}: POST must still consult the engine`).toContain((body as any).name);
213+
expect(put.calls, `${label}: PUT must still consult the engine`).toContain((body as any).name);
214+
},
215+
);
216+
217+
it('case "undeclaredConfigKey" (#4277): the self-correcting message survives verbatim on PUT', async () => {
218+
const { dispatcher } = makeDispatcher();
219+
const body = BAD_BODIES.undeclaredConfigKey;
220+
const result: any = await putFlow(dispatcher, body.name, body);
221+
222+
const message: string = result.response.body.error.message;
223+
expect(message).toContain('#4277');
224+
expect(message).toContain('unknown config key `totallyBogusKey`');
225+
expect(message).toContain('at config.totallyBogusKey');
226+
expect(message).toContain("not declared by this node type's configSchema");
227+
expect(message).toContain(`Declared here: ${NOTIFY_DECLARED_CONFIG_KEYS.join(', ')}.`);
228+
expect(message).toContain("node 'n' (notify)");
229+
// …and the same substance reaches the field entry, not a stub.
230+
const fields = result.response.body.error.details.fields;
231+
expect(fields).toHaveLength(1);
232+
expect(fields[0]).toEqual({ field: '(body)', code: 'invalid_value', message });
233+
});
234+
235+
it('case "missingNodeLabel": the raw Zod issue array does not reach the wire on PUT either', async () => {
236+
const { dispatcher } = makeDispatcher();
237+
const body = BAD_BODIES.missingNodeLabel;
238+
const result: any = await putFlow(dispatcher, body.name, body);
239+
240+
expect(Object.keys(result.response.body.error.details)).toEqual(['fields']);
241+
expect(result.response.body.error.details.issues).toBeUndefined();
242+
const wire = JSON.stringify(result.response.body);
243+
expect(wire).not.toContain('"expected"');
244+
expect(wire).not.toContain('"received"');
245+
expect(wire).not.toContain('"path"');
246+
});
247+
248+
it('the PUT-only { definition } wrapper still reaches the SAME classification as the bare form', async () => {
249+
// [#8123] Noted on the issue: PUT has its own `body.definition ?? body`
250+
// unwrap, so the definition it forwards to `registerFlow` is not
251+
// always the request body verbatim. Both dialects must classify the
252+
// same way.
253+
const bareRun = makeDispatcher();
254+
const body = BAD_BODIES.undeclaredConfigKey;
255+
const bare: any = await putFlow(bareRun.dispatcher, body.name, body);
256+
257+
const wrappedRun = makeDispatcher();
258+
const wrapped: any = await wrappedRun.dispatcher.handleAutomation(`/${body.name}`, 'PUT', { definition: body }, CTX);
259+
260+
expect(wrapped.response.status).toBe(bare.response.status);
261+
expect(wrapped.response.body.error.code).toBe(bare.response.body.error.code);
262+
expect(wrapped.response.body.error.message).toBe(bare.response.body.error.message);
263+
assertValidationEnvelope(wrapped.response, 'PUT { definition } wrapper');
264+
});
265+
});
266+
267+
// ---------------------------------------------------------------------------
268+
// Contrast controls — neither door may have WIDENED or NARROWED which bodies
269+
// are refused; only the class and envelope on PUT may have changed.
270+
// ---------------------------------------------------------------------------
271+
272+
describe('#8123 — what must not change', () => {
273+
it('a well-formed body still registers, 200, on BOTH doors', async () => {
274+
const post = makeDispatcher();
275+
const postResult: any = await postFlow(post.dispatcher, WELL_FORMED);
276+
expect(postResult.response?.status).toBe(200);
277+
expect(postResult.response?.body?.success).toBe(true);
278+
expect(post.registered.has('welcome_flow')).toBe(true);
279+
280+
const put = makeDispatcher();
281+
const putResult: any = await putFlow(put.dispatcher, WELL_FORMED.name, WELL_FORMED);
282+
expect(putResult.response?.status).toBe(200);
283+
expect(putResult.response?.body?.success).toBe(true);
284+
expect(put.registered.has('welcome_flow')).toBe(true);
285+
});
286+
287+
it('every bad body is still refused on PUT — never 200 — only the class changed from 500', async () => {
288+
for (const [label, body] of Object.entries(BAD_BODIES)) {
289+
const { dispatcher } = makeDispatcher();
290+
const result: any = await putFlow(dispatcher, (body as any).name, body);
291+
expect(result.response?.status, label).not.toBe(200);
292+
expect(result.response?.status, label).toBe(400);
293+
expect(result.response?.body?.error?.code, label).toBe('VALIDATION_FAILED');
294+
}
295+
});
296+
297+
it('an engine error on PUT that DECLARES its own class keeps it (same seam as POST)', async () => {
298+
// A service whose `registerFlow` declares its own `.status` — the
299+
// producer's escape hatch `flowDefinitionRefusal` already honours.
300+
const throwing = {
301+
registerFlow: () => {
302+
throw Object.assign(new Error('flow store unreachable'), { status: 503 });
303+
},
304+
};
305+
const services: Record<string, unknown> = { automation: throwing };
306+
const resolve = (name: string) => services[name];
307+
const kernel: any = { getService: resolve, getServiceAsync: async (name: string) => resolve(name), context: { getService: resolve } };
308+
const d = new HttpDispatcher(kernel);
309+
310+
const result: any = await putFlow(d, 'welcome_flow', WELL_FORMED);
311+
expect(result.response?.status).toBe(503);
312+
expect(result.response?.body?.error?.message).toBe('flow store unreachable');
313+
});
314+
});

packages/runtime/src/domains/automation.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -877,7 +877,19 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str
877877
{ field: '(body)', code: 'invalid_type', message: 'expected a flow definition object' },
878878
]);
879879
}
880-
automationService.registerFlow(name, definition);
880+
// [#8123] Same class as POST /: the engine's verdict on the
881+
// definition is served as a 400, not a 500 — reusing the
882+
// same route-agnostic `flowDefinitionRefusal` helper POST
883+
// uses above, so the two doors cannot disagree about the
884+
// class of an identical refusal (#8055 wired POST only).
885+
try {
886+
automationService.registerFlow(name, definition);
887+
} catch (e) {
888+
return {
889+
handled: true,
890+
response: deps.errorFromThrown(flowDefinitionRefusal(e), VALIDATION_FAILED_STATUS),
891+
};
892+
}
881893
return { handled: true, response: deps.success(definition) };
882894
}
883895
}

0 commit comments

Comments
 (0)