fix(spec,runtime,service-automation,client): GET /automation/:name/runs?status= filters instead of being dropped (#7359) - #7490
Merged
Conversation
…uns?status=` filters instead of being dropped (#7359) 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" `<select>` submits. 2. **`ListRunsRequestSchema` now references `ExecutionStatus`** instead of re-listing its eight members inline. I had written a comment claiming the boundary and the wire could not drift; that was not true as written, since the schema held a hand-copied duplicate. Made it true rather than softening the comment. Identical members, so no wire change — plus a spec test pinning that every `ExecutionStatus` member parses, which fails if the lists diverge. 3. **The typed client can send it** (`packages/client`). The issue names the SDK gap as the reason nothing had tripped over this. Leaving it out would make the enforced filter reachable only from raw HTTP, while the Runs view triage cited as the real consumer goes through this client. Additive optional param on both `automation.listRuns` and `automation.runs.list`. This is a fourth package beyond the three the dispatch listed — flagging it explicitly; it is additive and does not touch `domains/automation.ts`, so it carries no conflict risk with #7360. ## Constraints honoured - Descriptor routes (`GET /actions`, `GET /connectors`) NOT touched — #7360's diff in this file will not conflict; my change is confined to the runs branch. - `content/docs/releases/**` not touched. Changeset added instead. - Worktree-first (`../objectstack-7359`); no `git stash` at any point. ## Gates - `pnpm lint` — clean - `pnpm typecheck` — 126/126 tasks - `packages/spec` — 373 files, 9790 tests - `packages/runtime` — 121 files, 1939 tests - `packages/services/service-automation` — 73 files, 895 tests - `packages/client` — 21 files, 279 tests Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WPDMabeKZnjXeoG41wb4rK
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
size/l
documentation
Contributor
📓 Docs Drift CheckThis PR changes 4 package(s): 114 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 7 release-owned page(s) also reference the affected code. These are read-only:
|
…?status` filter The hand-written table enumerated this route's query parameters as `?limit`, `?cursor` — accurate until the previous commit made `?status` real. Flagged by the docs-drift check on #7490; the generated `references/api/automation-api.mdx` already documented `status` from the schema and needed no change, which also confirms the schema's switch to referencing `ExecutionStatus` was not a wire change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WPDMabeKZnjXeoG41wb4rK
os-help
marked this pull request as ready for review
August 11, 2026 02:09
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7359
Triage's option 1 (ENFORCE), as ruled by the PM.
The defect, re-measured on the branch point
Relocated by content (branch point
afdc6ea), not by triage's8a9c079line numbers. All three layers confirmed still defective:spec/src/api/automation-api.zod.tsListRunsRequestSchemadeclaresstatus: z.enum([…8 members]).optional(), "Filter by execution status"spec/src/contracts/automation-service.tslistRuns?(flowName, options?: { limit?, cursor? })— no slotruntime/src/domains/automation.ts{ limit, cursor }—statusnever leaves the HTTP layerSo
?status=failedwas dropped silently and the caller got 200 with every run of the flow, capped bylimit. A monitoring caller paging for failures reads the first 20 runs of any status and concludes those are the failures.automation-runs-query-validation.test.tscarried an explicit pin of that behaviour (?status=failedis 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 quietly.What changed
status?: ExecutionStatusadded to thelistRunsoption. One optional key, no breaking change.query-param.tshelper, as directed. That module had no enum gate, so this addsparseEnumParambeside the existing boolean/integer/string ones rather than hand-rolling a comparison. Refuses in the house shape:validationFailure→ 400VALIDATION_FAILED(ADR-0112) +details.fields[]. No new error vocabulary — ADR-0114's closed catalog already carriesinvalid_optionfor a non-member, andinvalid_typefor a value that was never a single string (repeated?status=a&status=b, structured?status[$ne]=x), the same mappingparseStringParammakes for that condition.AutomationEngine.listRuns.Which store each filter arm covers
Applied once, to the merged map, so it covers both: the in-memory ring buffer (the live half) and the durable rows from
store.listHistory(the half that survives a restart).Filtering after the merge rather than on each arm is load-bearing for a second reason found while writing it:
executionLogsholds more than one entry per run id (a run that pauses appendspaused, 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=pausedand lets the stale one survive — every approval/screen/wait run that had since completed would report itself as still paused. That is exactly the defectrun-history.test.ts's "latest entry wins" block pins forgetRun. My first draft filtered each arm and had this bug; 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 return fewer thanlimitmatches while older matching rows exist. This is the merge's pre-existing shape (durable was already capped atlimitbefore the sort-and-slice); pushing it down 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.Proving the tests can fail
Every new test mutated to red, then reverted:
statusfrom the options objectparseStringParaminstead ofparseEnumParamstatusentirelyretryingThe two one-armed mutations are the ones the dispatch warned about, and both are caught.
This also found a weak test of my own: the 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.Decisions the dispatch did not specify
?status=means no filter, not a 400 —parseIntegerParam's falsy gate, notparseBooleanParam's refusal. 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 precisely what "no filter" means. It is also what an "All statuses"<select>submits.ListRunsRequestSchemanow referencesExecutionStatusinstead of re-listing its members inline. I had written a comment claiming wire and boundary could not drift; that was not true while the schema held a hand-copied duplicate, so I made it true rather than softening the comment. Identical members ⇒ no wire change, plus a spec test that fails if the lists diverge.automation.listRunsandautomation.runs.list. This is a fourth package beyond the three the dispatch listed, flagged explicitly; it does not touchdomains/automation.ts, so it carries no conflict risk with finding: a repeated?paradigm=/?source=/?category=/?type=on the automation descriptor routes silently empties the designer palette (200, zero rows) #7360.Constraints honoured
GET /actions,GET /connectors) not touched — finding: a repeated?paradigm=/?source=/?category=/?type=on the automation descriptor routes silently empties the designer palette (200, zero rows) #7360's diff in this file will not conflict; changes are confined to the runs branch.content/docs/releases/**not touched — changeset added instead.git stashat any point.Gates
pnpm lintclean ·pnpm typecheck126/126 · spec 9790 · runtime 1939 · service-automation 895 · client 279 — all green.Generated by Claude Code