fix(control): validate injected error status/body and unquote multipart boundaries - #434
Merged
jpr5 merged 4 commits intoSep 13, 2026
Merged
Conversation
commit: |
jpr5
added a commit
that referenced
this pull request
Sep 13, 2026
## Why The `EXECUTED: the retry window is honoured, not multiplied by a longer deadline` negative control in `src/__tests__/npm-publish-verify-workflow.test.ts` asserted on **real elapsed wall clock**. It is red on `main` @ `252a4cc` (run 34597377003, legs `test (20)` and `test (26)`), which puts a red X on every open PR — including the three external-contributor PRs #433/#434/#435. Widening the window was already tried (`20e0d88`) and is not the fix. The step's body computes `ELAPSED` from `date +%s`, which is whole-second: if `START` is sampled just before a second boundary, the loop's own arithmetic crosses a 3s deadline after as little as ~2.0s of real time. That is sub-second phase, not runner load — it fails on an idle laptop. ## RED On `origin/main`, unloaded, 25 consecutive runs of just that test: **8 failed**. ``` RUN 6: RED → expected 2469 to be greater than or equal to 2800 RUN 7: RED → expected 2501 to be greater than or equal to 2800 RUN 8: RED → expected 2492 to be greater than or equal to 2800 RUN 9: RED → expected 2475 to be greater than or equal to 2800 RUN 16: RED → expected 2474 to be greater than or equal to 2800 RUN 17: RED → expected 2539 to be greater than or equal to 2800 RUN 21: RED → expected 2567 to be greater than or equal to 2800 RUN 22: RED → expected 2466 to be greater than or equal to 2800 ``` ## Fix `date` and `sleep` are external commands, so they can be stubbed on `PATH` exactly like the existing `npm` stub. The step's `run:` body is still executed **verbatim and unmodified** — only its clock is now the test's. - `sleep N` records `N` and advances a virtual clock file by `N`; `date +%s` reads it. - Timing becomes an **observable**: the exact attempt count and the exact backoff schedule the loop issued. No assertion in the file touches wall clock any more (`elapsedMs` is gone from `Observation`). - The observations now run at the **shipped** `300s`/`5s`/`30s` defaults instead of a shrunken 1s window, so what is tested is what CI runs. A five-minute window costs nothing on a virtual clock — the whole file got *faster*, ~30s → ~8s. - The default schedule is asserted exactly: attempts at `t+0,5,15,35,65,95,125,155,185,215,245,275,305`, sleeps `[5,10,20,30,30,30,30,30,30,30,30,30]`, failing at attempt 13. - "Honoured, not multiplied" is now a property that no runner speed can break: a shorter deadline stops the loop sooner, and overshoot past any deadline is `< deadline + MAX_DELAY` (one backoff step). - A body that stopped sleeping would spin against a clock that never advances, so the `npm` stub jumps the clock after 40 attempts — turning a hang into a legible failure. ## GREEN 20 consecutive green runs of the file: 12 standalone, then 8 more **concurrently with the full `npx vitest run`** (189 files / 5804 tests, exit 0) hammering the same machine. Zero failures. ## Mutation proof — it still bites Both mutations applied to the real `publish-release.yml` guard: | Mutation | Result | |---|---| | Retry loop deleted (`while :; do` → single pass, i.e. the pre-fix "ask once" body) | **RED — 5 of 10 tests fail** (`expected 1 to be 2`, `expected 1 to be 3`, `expected +0 to be 1` ×3) | | Deadline's `exit 1` → `exit 0` | **RED — 3 of 10 tests fail** (`expected +0 to be 1` ×3) | Workflow restored afterwards; this PR changes **one file**, the test. ## Other wall-clock assertions in the file Yes — three more, all fixed the same way: the `never appears FAILS` control (`elapsedMs >= 4000`), the `FAILS FAST` control (`elapsedMs < 10_000`), and the loose `attempts > N` bounds. All are now exact attempt counts and exact sleep schedules. ## Gates (raw exit codes) | Gate | Exit | |---|---| | `prettier --check` | 0 | | `npx eslint .` | 0 | | `pnpm typecheck` | 0 | | `npx vitest run` (full) | 0 | | `npx commitlint --from origin/main --to HEAD` | 0 | | `npx actionlint` | 1 — **pre-existing on `main`**, SC2086 in `changelog-radar.yml` / `test-drift.yml`; no workflow file is touched by this PR | 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01AvkmhXVLqSSEW6FvPQHSu5
…nquote boundaries Follow-up to the review of this PR. Four measured problems with the original guards, all reproduced against the real server before and after: - The 200-599 ceiling was invented. Node's writeHead accepts 100-999, and 600, 999 and a stringified "500" all worked end to end on main. Post-guard they returned 400 and the injection silently no-opped, so the request under test got a normal 200 — an error-injection test would have gone green and vacuous. The bound is now 200-999: 1xx is still excluded (it cannot terminate a response and hangs the request until the client times out, measured), and 600-999 is unassigned rather than invalid, which is exactly what a mock server exists to serve. - body.code: null was rejected, but aimock emits `code: response.error.code ?? null` itself, and so does OpenAI — posting aimock's own error envelope back to /__aimock/error returned 400. null now reads as absent, for status too. - Single-quote stripping had no basis in RFC 2045/2046: "'" is a legal token char and a legal bchar, so boundary='abc' is a bare token whose value includes the quotes. Stripping them broke a working request into a silent whisper-1 fallback. Dropped, along with the test that pinned it. - The RFC 2046 claim was not delivered: [^\s;]+ truncates before any unquoting, so boundary="a b c" yielded "a both before and after — the one case quoting is mandatory for. The quotes are consumed by the match now. - Validation moved to where all three sinks reach it (fixture-loader, which server.ts and llmock.ts already import and which already range-checked a fixture status): POST /__aimock/error, LLMock.nextRequestError — the path the docs use, where nextRequestError(99) returned 500 and (100) hung — and POST /__aimock/fixtures now share one predicate. Docs and CHANGELOG record the surviving behaviour changes to /__aimock/error, which is published surface that aimock-pytest calls raise_for_status() on.
Same rule as `status: null` and the null body fields: `{"status": 429,
"body": null}` is what a serializer emits for an unset optional, it worked
on main (every field defaulted), and the new guard answered 400 while the
injection silently no-opped. The shape is not malformed — only a non-object
non-null body is.
…Status The comment cited `http-status.ts`, a file that does not exist. The predicate it refers to, `isInjectableStatus`, lives in this same file.
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.
Summary
Two request-handling crashes/hangs, one PR (both are unvalidated-input → broken-server-state bugs on the control/multipart path):
1.
POST /__aimock/erroraccepted anystatus(server.ts)status: 99/0/1000/NaN/"500"was queued unchecked and reachedres.writeHead(status)on the next matched request →ERR_INVALID_HTTP_STATUS/RangeError(500 instead of the injected error).status: 100(and all 1xx) is worse: informational statuses can never terminate a response, so the next request hangs until the client times out (reproduced with a 5s-timeout test before the fix).body.message/type/codesilently broke error classification downstream, and a top-levelnull/array JSON body threwTypeErroron property access.statusmust be an integer in [200, 599] (explicitnullrejected — it previously slipped through??and silently became 500);bodymust be an object with string-onlymessage/type/code; top-level must be an object. Violations get 400 and queue nothing.2.
extractBoundarykept RFC 2046 quotes (transcription.ts)form-data, and Pythonrequestsemitboundary="…". The returned delimiter became--"…", never matched, andhandleTranscription(plus image-edit/variation and video handlers reusing the helper) silently fell back tomodel=whisper-1/ droppedstream=true.undefined.Verification
src/__tests__/control-error-multipart.test.ts: 16 tests — boundary unit cases (bare/quoted/single/trailing-params/empty), quoted-boundary HTTP routing + journal model assertion, error-status/body rejection matrix with nothing-queued assertions, valid 200/418/599 + default-500 queueing.control-api(23),multimedia(33),image-edits(10),multimedia-types(20) — 86 passed.pnpm typecheck,eslint,prettierclean.