fix(validation): reject non-string inputs and out-of-range dimensions with 400 - #433
Merged
jpr5 merged 4 commits intoSep 13, 2026
Conversation
commit: |
Contributor
Author
|
Note on the latest commit: CI failed twice on the Node-20 leg in |
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
jpr5
force-pushed
the
fix/strict-input-validation-embeddings-search
branch
2 times, most recently
from
September 13, 2026 00:42
f7e713b to
23790fa
Compare
…de dimensions
The new 400s catch real 500s and those stay. But several guards were drawn
tighter than the wire formats these endpoints serve, turning working 200s into
400s — on a mock, a 400 where the provider returns 200 surfaces as the
consumer's bug, not ours.
Correctness:
- normalizeEmbeddingInput (renamed from normalizeStringArrayInput, which no
longer described it) accepts number[] and number[][]. Per
EmbeddingCreateParams.input, `input` is
`string | Array<string> | Array<number> | Array<Array<number>>`; the token
forms are what tiktoken chunkers emit. They are folded to a string key rather
than handed raw to createHash().update().
- normalizeTextInput accepts any array, joining as the previous
`Array.isArray(raw) ? raw.join(" ") : raw` did, and reads `.text` out of
ModerationCreateParams' multimodal parts. Only non-string, non-array values —
the ones that actually crashed matchesPattern() — still 400.
- MAX_EMBEDDING_DIMENSIONS is no longer an OpenAI model width. /v1/embeddings
also serves Azure and every OpenAI-compatible server routed through
COMPAT_SUFFIXES, where 4096-dimension models are ordinary. It is now the
ECMAScript array-length bound (2**32 - 1), where `new Array(n)` actually
throws RangeError, and a test pins the literal so it cannot drift.
- The dimensions check moves onto the deterministic-fallback branch, the only
path that reads it, so fixture-replay, chaos, strict and record/proxy requests
are no longer gated on it. `dimensions: null` means unset again.
- The `input: []` rejection is dropped. It returned 200 with `data: []` before
and crashed nothing, so there was no defect behind it.
- The new 400 bodies carry `param` and `code`. The OpenAI SDK's APIError reads
both straight off the error object, so omitting them hands consumers
`undefined` where the real API gives a value.
- /search echoes the query it was given again, matching on the normalized string
only; an array payload was being echoed back as a joined string.
- EmbeddingRequest.input / dimensions and the search/rerank body types are
`unknown`: they arrive as arbitrary JSON and are validated at runtime, so the
declared types must not claim a shape the parser cannot guarantee.
Also drops the npm-publish-verify-workflow.test.ts hunk — a weaker retry of the
flake already fixed by CopilotKit#436, which deleted elapsedMs outright — and the empty
CI-retrigger commit.
jpr5
force-pushed
the
fix/strict-input-validation-embeddings-search
branch
from
September 13, 2026 00:51
23790fa to
9958f27
Compare
…locate
MAX_EMBEDDING_DIMENSIONS was 2**32 - 1, the ECMAScript array-length bound —
where `new Array(n)` throws RangeError. But handleEmbeddings does not stop at
allocating: generateDeterministicEmbedding fills the array and the response is
JSON-serialized. Against a real out-of-process server, one request at exactly
the sanctioned maximum aborted the process:
POST /v1/embeddings {"input":"hi","dimensions":4294967295}
-> curl exit 52 (empty reply), HTTP 000
FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory
SERVER DEAD
No response was written and the mock died, taking every test sharing it with
it. The suite made this worse by pinning the value as accepted, so the false
invariant was certified rather than merely unguarded.
The cap is a serialization budget. Measured on this tree, a response body costs
19.58 bytes per dimension (4096 -> 80,456 B; 100,000 -> 1,958,539 B; 1,000,000
-> 19,583,034 B). 100,000 dimensions is a ~1.96 MB body built in 7 ms at 83 MB
RSS under a 1 GB heap, and still leaves >12x headroom over the widest width in
circulation (3072 for text-embedding-3-large, 4096/8192 on the
OpenAI-compatible servers this endpoint also serves).
The same request now returns 400 with the server still serving, and
dimensions: 100000 still returns 200 with a 100,000-element embedding.
`openai/error.js` reads `param`/`code` straight off `body.error`, so the new 400 envelopes are right to carry the keys — consumers see `undefined` otherwise. That justifies emitting the keys, not the values `"input"` and `"dimensions"`. Nothing sourced those. No recorded OpenAI 400 for these endpoints exists anywhere under fixtures/, and no drift canary probes a 400 here, so the value could never be caught drifting. Meanwhile the repo already answers this case the honest way four times over — server.ts:607, :633, :869 and :964 all emit `param: null, code: null` — and byteplus-video.ts:385-410 states the governing rule outright: the mock never authors a wire value it did not observe, and an honest absence beats a plausible invention. A consumer asserting `err.param === "input"` would have been asserting something aimock made up; a consumer asserting `err.param === null`, the value aimock itself emits elsewhere, would have broken on these two endpoints alone. Emit `param: null, code: null`, matching server.ts, and unpin the invented values in the suite, which was certifying them.
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
POST /v1/embeddings, /v1/moderations, /search and /v2/rerank trusted the JSON body shape after JSON.parse and crashed downstream with a generic 500 instead of a 400:
embeddings: numeric/object/mixed-arrayinputreachedcreateHash().update()(TypeError → 500);dimensions: -1threwRangeErrorinnew Array(), huge values attempted GB allocations (OOM).moderation/search/rerank: non-stringinput/querycrashedmatchesPattern()(text.toLowerCase()) and.slice(0, 80)logging (500).This PR adds strict 400 validation at the handler boundary:
src/helpers.ts: new sharedvalidateEmbeddingDimensions()(integer 1–3072, default 1536),normalizeStringArrayInput(),normalizeTextInput()+MAX_EMBEDDING_DIMENSIONSexport.src/embeddings.ts: 400 on non-string/mixed input, empty arrays, and invalid dimensions (non-integer, out-of-range, wrong type).src/moderation.ts,src/search.ts,src/rerank.ts: 400 on non-string input/query (arrays of strings still accepted and joined).{ error: { message, type: 'invalid_request_error' } }.Verification
src/__tests__/input-validation.test.ts: 25 tests (unit + HTTP integration per endpoint, boundary dimensions 1/3072, journal assertions) — all pass.embeddings(49),control-api(23),cohere-embed(10),gemini-embeddings(18) — 100 passed.pnpm typecheck,eslintandprettierclean on all touched files.