From 2af301e4a21a80ae0f90ff07dcde275a0abb96b3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 01:26:00 +0000 Subject: [PATCH 1/2] fix(spec,runtime,service-automation,client): `GET /automation/:name/runs?status=` filters instead of being dropped (#7359) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #7359. Triage's option 1 (ENFORCE), as ruled by the PM. ## What was measured Relocated by content on the branch point (afdc6ea), not by triage's `8a9c079` line numbers. All three layers confirmed still defective: - `packages/spec/src/api/automation-api.zod.ts:274` — `ListRunsRequestSchema` declares `status: z.enum([...8 members]).optional()`, described as "Filter by execution status". - `packages/spec/src/contracts/automation-service.ts:416` — `listRuns?(flowName, options?: { limit?: number; cursor?: string })`. No slot. - `packages/runtime/src/domains/automation.ts:462` — the runs branch built `{ limit, cursor }` only. `status` never left the HTTP layer. So `?status=failed` was dropped silently and the caller got 200 + every run of the flow, capped by `limit`. A monitoring caller paging for failures read the first 20 runs of any status and concluded those were the failures. The existing test `automation-runs-query-validation.test.ts` carried an explicit pin of that behaviour ("`?status=failed` is ignored, not refused"), written by #7300 which deliberately declined to decide between honouring and retiring the key. That pin is SUPERSEDED here — replaced by cases asserting the opposite on the same input, not deleted silently. ## What changed, and why 1. **Contract** (`contracts/automation-service.ts`) — `status?: ExecutionStatus` added to the `listRuns` option. One optional key, no breaking change. 2. **Boundary** (`runtime/src/domains/automation.ts`, runs branch ONLY) — reads the parameter through the shared `query-param.ts` helper, as directed. That module had no enum gate, so this adds `parseEnumParam` alongside the existing boolean/integer/string ones rather than hand-rolling a comparison at the call site. It refuses in the house shape: `validationFailure` -> 400 `VALIDATION_FAILED` (ADR-0112) + `details.fields[]`. No new error vocabulary — ADR-0114's closed catalog already carries `invalid_option` ("not a member of the field's declared options") for a non-member, and `invalid_type` for a value that was never a single string (repeated `?status=a&status=b`, structured `?status[$ne]=x`), which is the mapping `parseStringParam` already makes for that same condition. 3. **Engine** (`service-automation/src/engine.ts`) — filters at the MERGE point. ## Which store each filter arm covers The dispatch called out that a one-armed filter is the same class of wrong answer as the bug. It is applied once, to the merged map, which covers both: - in-memory ring buffer (`this.executionLogs`) — the live half; - durable rows (`store.listHistory`) — the half that survives a restart. Filtering after the merge rather than on each arm is deliberate and load-bearing for a second reason I found while writing it: `executionLogs` holds MORE THAN ONE entry per run id (a run that pauses appends 'paused', then its terminal entry), and the merge is what collapses them to the freshest. Filtering the arms before that collapse drops the terminal entry for `?status=paused` and lets the stale 'paused' one survive — every approval/screen/wait run that had since completed would report itself as still paused. That is precisely the defect `run-history.test.ts`'s "latest entry wins" block pins for `getRun`, and I very nearly re-introduced it one method over; my first draft filtered each arm. There is now a test for it. KNOWN, DOCUMENTED LIMIT (not fixed, not silently narrowed): the durable arm's window is still `listHistory(flowName, limit)` — that store method has no status slot, so the filter is applied to the rows that come back rather than pushed down. A status filter can therefore return fewer than `limit` matches while older matching rows exist. This is the merge's pre-existing shape (durable was already capped at `limit` before the sort-and-slice); closing it properly is a store-contract change and belongs in its own card. What it never does is return a run of another status. Stated in code, changeset, and here rather than left for someone to discover. ## How I proved the tests can fail Every new test was mutated to red and reverted. Six mutations: | mutation | red | |-------------------------------------------------|----------------------------------------| | handler drops `status` from the options object | 20 runtime tests | | `parseStringParam` instead of `parseEnumParam` | 6 runtime (refusals + empty spelling) | | engine ignores `status` entirely | 4 engine tests | | engine filters the IN-MEMORY arm only | 3 engine (incl. the durable one) | | engine filters the DURABLE arm only | 3 engine (incl. the in-memory one) | | engine filter made non-optional | preservation test + 4 pre-existing | | schema re-lists members, drops `retrying` | the new spec drift pin | The one-armed mutations are the two that matter for the dispatch's warning, and both are caught. This also FOUND A WEAK TEST: my first "merged listing" case used a fixture whose runs were all `failed`, so it passed even with the filter removed entirely. It now seeds one status per store (a durable failure + a live success under one flow name) and asserts each answer comes from a different store and neither leaks the other's run. It fails under all three engine mutations. ## Decisions the dispatch did not specify 1. **Empty `?status=` means NO filter, not a 400.** `parseIntegerParam`'s falsy gate, not `parseBooleanParam`'s refusal. Reason: the prior answers differ in kind. `?read=` used to serve the wrong HALF of the inbox, so refusing it was a strict improvement; `?status=` already served every run, which is exactly what "no filter" means, so it had a defensible answer that must not become a new 400. It is also what an "All statuses" `` submits. `limit` and `cursor` are +untouched, including out-of-range values, which remain the service's business. diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 9eb9f543d0..fce948431b 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -83,6 +83,7 @@ import type { ApprovalStatus, ApprovalDecisionResult, } from '@objectstack/spec/contracts'; +import type { ExecutionStatus } from '@objectstack/spec/automation'; import { Logger, createLogger } from '@objectstack/core/logger'; import { RealtimeAPI } from './realtime-api'; @@ -3130,12 +3131,17 @@ export class ObjectStackClient { /** Alias for `automation.runs.list`. */ listRuns: async ( flowName: string, - opts?: { limit?: number; cursor?: string }, + opts?: { limit?: number; cursor?: string; status?: ExecutionStatus }, ): Promise => { const route = this.getRoute('automation'); const params = new URLSearchParams(); if (opts?.limit != null) params.set('limit', String(opts.limit)); if (opts?.cursor) params.set('cursor', opts.cursor); + // [#7359] The route's declared `status` filter, now that the boundary + // honours it instead of dropping it. Until this card the typed client + // could not send it at all — which is why nothing had tripped over the + // server-side gap. + if (opts?.status) params.set('status', opts.status); const qs = params.toString(); const res = await this.fetch( `${this.baseUrl}${route}/${encodeURIComponent(flowName)}/runs${qs ? `?${qs}` : ''}`, @@ -5261,14 +5267,16 @@ export class ScopedProjectClient { }); return this.parent._unwrap(res); }, - /** List recent runs for a flow. */ + /** List recent runs for a flow, optionally narrowed to one status. */ listRuns: async ( flowName: string, - opts?: { limit?: number; cursor?: string }, + opts?: { limit?: number; cursor?: string; status?: ExecutionStatus }, ): Promise => { const params = new URLSearchParams(); if (opts?.limit != null) params.set('limit', String(opts.limit)); if (opts?.cursor) params.set('cursor', opts.cursor); + // [#7359] — see the sibling `listRuns` alias above. + if (opts?.status) params.set('status', opts.status); const qs = params.toString(); const res = await this.parent._fetch( this.url(`/automation/${encodeURIComponent(flowName)}/runs${qs ? `?${qs}` : ''}`), diff --git a/packages/runtime/src/domains/automation-runs-query-validation.test.ts b/packages/runtime/src/domains/automation-runs-query-validation.test.ts index 7900297439..3f28460096 100644 --- a/packages/runtime/src/domains/automation-runs-query-validation.test.ts +++ b/packages/runtime/src/domains/automation-runs-query-validation.test.ts @@ -1,7 +1,18 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * #7300 — `GET /api/v1/automation/:name/runs`'s coerced query parameters. + * #7300 / #7359 — `GET /api/v1/automation/:name/runs`'s query parameters, at + * the boundary that reads them. + * + * #7300 (below) closed the two parameters this handler already forwarded but + * COERCED. #7359 closed the third, which is the same 200-with-the-wrong-answer + * arrived at from the opposite direction: `status` was declared by + * `ListRunsRequestSchema`, had no slot on `IAutomationService.listRuns`, and + * was never built into the handler's option object — so `?status=failed` was + * dropped here in silence and the caller was answered with EVERY run of the + * flow. #7300 deliberately pinned that ignore-the-key behaviour rather than + * decide it; #7359 took the enforce route, so that one pin is superseded here + * by cases asserting the opposite on the same input. * * The filed defect is character-for-character #6928's, one file over: * `{ limit: query.limit ? Number(query.limit) : undefined, cursor: query.cursor }`. @@ -132,6 +143,60 @@ describe("#7300 — the same probe on this route's other passed-through paramete }); }); +describe('#7359 — a `?status=` outside the declared set is refused, not silently widened', () => { + it.each([ + ['a typo', 'faild'], + ['right word, wrong case', 'FAILED'], + ['a status of a neighbouring vocabulary', 'success'], + ['empty-ish but not a member', ' '], + ])('refuses ?status=%s with 400 VALIDATION_FAILED', async (_label, raw) => { + // Once the filter is honoured there is no safe reading left for a value + // outside the set. `?status=faild` cannot mean "no filter" — the caller + // plainly asked to narrow — and serving the empty list is no better, + // because "no runs are `faild`" and "no runs failed" read identically to + // a caller who cannot see their own typo. Both are a monitoring surface + // answering "you have no failures" with confidence. + const { details, status, listRuns } = await refusalFor({ status: raw }); + + expect(details?.code).toBe('VALIDATION_FAILED'); + expect(status).toBe(400); + // ADR-0114's closed catalog already carries the constraint this + // violates — `invalid_option`, "not a member of the field's declared + // options". No new error vocabulary is minted for it. + expect(details?.fields).toEqual([ + { field: 'status', code: 'invalid_option', message: expect.stringContaining('`status`') }, + ]); + expect(listRuns).not.toHaveBeenCalled(); + }); + + it.each([ + ['repeated parameter', ['failed', 'completed']], + ['structured', { $ne: 'failed' }], + ['numeric', 7], + ])('refuses ?status=%s — it was never a single string (invalid_type)', async (_label, raw) => { + // The same mapping `parseStringParam` makes for the same condition: a + // repeated `?status=failed&status=completed` arrives as an ARRAY, and a + // filter is not a set on this wire. `String([...])` would have made it + // the single value `'failed,completed'`, matching nothing. + const { details, status, listRuns } = await refusalFor({ status: raw }); + + expect(details?.code).toBe('VALIDATION_FAILED'); + expect(status).toBe(400); + expect(details?.fields).toEqual([ + { field: 'status', code: 'invalid_type', message: expect.stringContaining('`status`') }, + ]); + expect(listRuns).not.toHaveBeenCalled(); + }); + + it('names the declared members in the message, and caps the echoed value', async () => { + const { message } = await refusalFor({ status: 'z'.repeat(500) }); + + expect(message).toContain('failed'); // the caller is told what IS accepted + expect(message).toContain('pending'); + expect(message.length).toBeLessThan(300); + }); +}); + describe('#7300 — every value that had a defensible answer keeps it', () => { async function listWith(query: Record | undefined) { const { dispatcher, listRuns } = makeDispatcher(); @@ -141,28 +206,28 @@ describe('#7300 — every value that had a defensible answer keeps it', () => { it.each([ // [label, query, the exact options object `listRuns` must receive] - ['?limit=20', { limit: '20' }, { limit: 20, cursor: undefined }], - ['?limit=1 (the low boundary)', { limit: '1' }, { limit: 1, cursor: undefined }], - ['?limit=100 (the declared high boundary)', { limit: '100' }, { limit: 100, cursor: undefined }], + ['?limit=20', { limit: '20' }, { limit: 20, cursor: undefined, status: undefined }], + ['?limit=1 (the low boundary)', { limit: '1' }, { limit: 1, cursor: undefined, status: undefined }], + ['?limit=100 (the declared high boundary)', { limit: '100' }, { limit: 100, cursor: undefined, status: undefined }], // Out of RANGE is not out of DOMAIN. `ListRunsRequestSchema` bounds // `limit` to 1..100 and the engine slices by whatever it is handed; // neither answer is this boundary's to change, so both still arrive. - ['?limit=1000 (over the declared range)', { limit: '1000' }, { limit: 1000, cursor: undefined }], - ['?limit=-5 (under it)', { limit: '-5' }, { limit: -5, cursor: undefined }], + ['?limit=1000 (over the declared range)', { limit: '1000' }, { limit: 1000, cursor: undefined, status: undefined }], + ['?limit=-5 (under it)', { limit: '-5' }, { limit: -5, cursor: undefined, status: undefined }], // Falsy spellings meant "no limit here" before this gate existed and // still do — they must not become a new 400. `'0'` is NOT one of them: // the string is truthy, so `query.limit ? Number(query.limit) : …` read // it as the number `0` and passed it on, and that is preserved too. - ['?limit= (empty)', { limit: '' }, { limit: undefined, cursor: undefined }], - ['?limit=0', { limit: '0' }, { limit: 0, cursor: undefined }], - ['limit: 0 (in-process number)', { limit: 0 }, { limit: undefined, cursor: undefined }], - ['limit: null', { limit: null }, { limit: undefined, cursor: undefined }], - ['no parameters at all', {}, { limit: undefined, cursor: undefined }], + ['?limit= (empty)', { limit: '' }, { limit: undefined, cursor: undefined, status: undefined }], + ['?limit=0', { limit: '0' }, { limit: 0, cursor: undefined, status: undefined }], + ['limit: 0 (in-process number)', { limit: 0 }, { limit: undefined, cursor: undefined, status: undefined }], + ['limit: null', { limit: null }, { limit: undefined, cursor: undefined, status: undefined }], + ['no parameters at all', {}, { limit: undefined, cursor: undefined, status: undefined }], // A cursor is opaque: every string passes through VERBATIM, including // the empty one, exactly as the raw passthrough did. - ['?cursor=n_007', { cursor: 'n_007' }, { limit: undefined, cursor: 'n_007' }], - ['?cursor= (empty)', { cursor: '' }, { limit: undefined, cursor: '' }], - ['both together', { limit: '5', cursor: 'n_007' }, { limit: 5, cursor: 'n_007' }], + ['?cursor=n_007', { cursor: 'n_007' }, { limit: undefined, cursor: 'n_007', status: undefined }], + ['?cursor= (empty)', { cursor: '' }, { limit: undefined, cursor: '', status: undefined }], + ['both together', { limit: '5', cursor: 'n_007' }, { limit: 5, cursor: 'n_007', status: undefined }], ])('%s answers 200 and reaches the service unchanged', async (_label, query, expected) => { const { result, listRuns } = await listWith(query); @@ -180,15 +245,52 @@ describe('#7300 — every value that had a defensible answer keeps it', () => { expect(listRuns).toHaveBeenCalledWith('welcome_flow', undefined); }); - it('leaves an unknown query key alone — `?status=failed` is ignored, not refused (#6361)', async () => { - // `ListRunsRequestSchema` declares a `status` filter that this handler - // has never read, so the key is silently ignored today. Refusing unknown - // keys — or starting to honour this one — is a separate decision with - // its own blast radius; this change takes neither. + // ── #7359 ──────────────────────────────────────────────────────────────── + // The block above used to end with a case asserting that `?status=failed` + // was IGNORED — #7300 deliberately preserved that, since choosing between + // honouring and retiring the declared key was a separate decision. #7359 + // took the enforce route, so that pin is superseded rather than deleted: + // the cases below assert the opposite behaviour on the same input. + + it('FORWARDS a declared `?status=` to the service instead of dropping it', async () => { + // The defect: `status` is declared by `ListRunsRequestSchema` but was + // never built into this handler's option object, so it never left the + // HTTP layer. The caller got 200 + every run of the flow — a caller + // paging for failures read the first `limit` runs of ANY status and + // concluded those were the failures. const { result, listRuns } = await listWith({ limit: '2', status: 'failed' }); expect(result.response?.status).toBe(200); - expect(listRuns).toHaveBeenCalledWith('welcome_flow', { limit: 2, cursor: undefined }); + expect(listRuns).toHaveBeenCalledWith('welcome_flow', { limit: 2, cursor: undefined, status: 'failed' }); + }); + + it.each( + ['pending', 'running', 'paused', 'completed', 'failed', 'cancelled', 'timed_out', 'retrying'], + )('forwards every declared ExecutionStatus member — ?status=%s', async (member) => { + // The gate reads its members from the spec's `ExecutionStatus` enum, the + // same one `ListRunsRequestSchema` is built from, so this pins that the + // wire's declared set and the boundary's accepted set are one set. A + // member added to the enum and refused here would fail this row. + const { result, listRuns } = await listWith({ status: member }); + + expect(result.response?.status).toBe(200); + expect(listRuns).toHaveBeenCalledWith('welcome_flow', { limit: undefined, cursor: undefined, status: member }); + }); + + it.each([ + ['absent', {}], + ['?status= (empty — an "All statuses" select)', { status: '' }], + ['status: null', { status: null }], + ])('%s still means NO filter — the unnarrowed listing is unchanged', async (_label, query) => { + // The preservation half. An absent filter must keep reaching the + // service as `undefined` (list everything), and the empty spelling must + // not become a new 400: unlike `?read=`, which used to serve the wrong + // HALF of the inbox, `?status=` already served exactly what "no filter" + // means, so it had a defensible answer to preserve. + const { result, listRuns } = await listWith(query); + + expect(result.response?.status).toBe(200); + expect(listRuns).toHaveBeenCalledWith('welcome_flow', expect.objectContaining({ status: undefined })); }); it('still refuses an anonymous caller with 401 before it ever looks at the query', async () => { diff --git a/packages/runtime/src/domains/automation.ts b/packages/runtime/src/domains/automation.ts index 529032a184..b6be25e567 100644 --- a/packages/runtime/src/domains/automation.ts +++ b/packages/runtime/src/domains/automation.ts @@ -16,7 +16,8 @@ import { CoreServiceName } from '@objectstack/spec/system'; import type { IAutomationService } from '@objectstack/spec/contracts'; import { isServiceServeable } from '../service-serveable.js'; import { validationFailure } from '../validation-failure.js'; -import { parseIntegerParam, parseStringParam } from '../query-param.js'; +import { ExecutionStatus } from '@objectstack/spec/automation'; +import { parseEnumParam, parseIntegerParam, parseStringParam } from '../query-param.js'; import { capabilityUnavailable } from './unavailable.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; @@ -120,7 +121,8 @@ export function createAutomationDomain(deps: DomainHandlerDeps): DomainRoute { * DELETE /:name → deleteFlow (unregisterFlow) * POST /:name/trigger → execute (legacy: trigger/:name also supported) * POST /:name/toggle → toggleFlow - * GET /:name/runs → listRuns (query: limit, cursor — validated, #7300) + * GET /:name/runs → listRuns (query: limit, cursor — validated, #7300; + * status — validated AND honoured, #7359) * GET /:name/runs/:runId → getRun * POST /:name/runs/:runId/resume → resume a paused run (screen input / ADR-0019) * GET /:name/runs/:runId/screen → the screen a paused run awaits @@ -459,8 +461,30 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str // service's declared business (`ListRunsRequestSchema` bounds it // 1..100); this gate only refuses values that were never whole // numbers. + // + // [#7359] `status` is the THIRD declared parameter, and until + // now the only one this handler never read. `ListRunsRequestSchema` + // has always declared it (`z.enum([...8 ExecutionStatus members]) + // .optional()`), but it had no slot on `IAutomationService.listRuns` + // and was never built into this object — so `?status=failed` was + // dropped here, silently, and the caller was answered 200 with + // EVERY run of the flow capped by `limit`. That is worse than an + // empty page: a monitoring caller paging for failures reads the + // first 20 runs of any status and concludes those are the + // failures. #7300 deliberately left the key ignored rather than + // decide between honouring and retiring it; this card takes the + // enforce route (ADR-0049), so the declared surface is true. + // + // The members come from the spec's own `ExecutionStatus` enum + // rather than a list copied into this file: the wire schema is + // built from that same enum, so a future member cannot be + // accepted by one and refused by the other. const options = query - ? { limit: parseIntegerParam('limit', query.limit), cursor: parseStringParam('cursor', query.cursor) } + ? { + limit: parseIntegerParam('limit', query.limit), + cursor: parseStringParam('cursor', query.cursor), + status: parseEnumParam('status', query.status, ExecutionStatus.options), + } : undefined; const runs = await automationService.listRuns(name, options); return { handled: true, response: deps.success({ runs, hasMore: false }) }; diff --git a/packages/runtime/src/query-param.ts b/packages/runtime/src/query-param.ts index 44de41cfd4..56bcacf868 100644 --- a/packages/runtime/src/query-param.ts +++ b/packages/runtime/src/query-param.ts @@ -23,6 +23,16 @@ * second hand-rolled refusal for the same condition drifting away from the * first. * + * #7359 added the near neighbour of a coercion — a declared filter the + * boundary never read at all, which is the same 200-with-the-wrong-answer with + * the narrowing dropped instead of invented: + * + * ?status=failed → (nothing) → EVERY run, as if all of them had failed + * + * Its gate is {@link parseEnumParam}, and it lives here for the reason the + * others do: the moment a closed-set filter is honoured, the value outside the + * set needs a refusal, and that refusal must be the same one every route makes. + * * The refusal is the house shape, not a new channel: `validationFailure` — the * duck-typed `{ code: 'VALIDATION_FAILED', fields[] }` that BOTH dispatcher * error exits map to `400` with `details.fields[]` (#3918). No new spec @@ -117,6 +127,59 @@ export function parseIntegerParam(param: string, raw: unknown): number | undefin return parsed; } +/** + * A CLOSED-SET parameter — a filter whose declared values are an enum on the + * wire (`?status=failed` on `GET /api/automation/:name/runs`, whose + * `ListRunsRequestSchema` bounds it to the eight `ExecutionStatus` members). + * + * Written for #7359, which is the third shape in this module's family and the + * one that fails widest. The other two are coercions that invent a value; this + * one is a filter the boundary never read at all. `status` was declared on the + * wire, absent from `IAutomationService.listRuns`'s options, and never built + * into the handler's option object — so `?status=failed` was dropped silently + * and the caller was answered `200` with **every** run of the flow. A + * monitoring caller paging for failures read the first 20 runs of any status + * and concluded those were the failures. + * + * Once such a parameter is honoured, a value outside the set has no safe + * reading left. `?status=faild` cannot mean "no filter" — the caller plainly + * asked to narrow — and it cannot mean the empty result either, because + * "no runs are `faild`" and "no runs failed" are the same sentence to a caller + * who cannot see the typo. So it is refused, in the house shape: the closed + * ADR-0114 catalog already carries `invalid_option` for exactly this + * constraint ("not a member of the field's declared options"). + * + * REFUSED: a non-empty string outside `allowed` (`invalid_option`); anything + * that was never a single string at all — a repeated `?status=a&status=b`, a + * structured `?status[$ne]=x`, a number (`invalid_type`, the same mapping + * {@link parseStringParam} makes for the same condition). + * + * ACCEPTED as "no filter": absent, `null`, and the EMPTY string. The empty + * spelling is the one judgement call here and it follows + * {@link parseIntegerParam}'s falsy gate rather than + * {@link parseBooleanParam}'s refusal, because the prior answers differ in + * kind. `?read=` used to serve the UNREAD half — a wrong answer, so refusing it + * strictly improved on it. `?status=` used to serve every run, which is + * precisely what "no filter" means, so it already had a defensible answer and + * this gate must not turn it into a new `400`. It is also the spelling an "All + * statuses" `