feat(smoke): add pre-merge smoke suite with record/replay MITM proxy - #828
Open
steventohme wants to merge 11 commits into
Open
feat(smoke): add pre-merge smoke suite with record/replay MITM proxy#828steventohme wants to merge 11 commits into
steventohme wants to merge 11 commits into
Conversation
|
Claude finished @steventohme's task —— View job Posted 5 comment-length suggestions (advisory, won't block merge). |
workweave-bot
left a comment
workweave-bot
left a comment
Collaborator
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
Comment on lines
+192
to
+198
| // Returns ok=false when node itself carries no strict-expressible type (no | ||
| // "type", "anyOf", or "enum") — wrapping it in a fresh anyOf would otherwise | ||
| // produce a typeless anyOf[0] branch, which OpenAI strict mode rejects with | ||
| // "schema must have a 'type' key" (the exact 400 a bare/typeless optional | ||
| // property — e.g. a "pass any JSON value" param — hit before this fix; the | ||
| // anyOf-branch recursion in strictifyNode already guards this case via | ||
| // schemaHasStrictType, but this fallback path didn't). |
Collaborator
There was a problem hiding this comment.
Suggested change
| // Returns ok=false when node itself carries no strict-expressible type (no | |
| // "type", "anyOf", or "enum") — wrapping it in a fresh anyOf would otherwise | |
| // produce a typeless anyOf[0] branch, which OpenAI strict mode rejects with | |
| // "schema must have a 'type' key" (the exact 400 a bare/typeless optional | |
| // property — e.g. a "pass any JSON value" param — hit before this fix; the | |
| // anyOf-branch recursion in strictifyNode already guards this case via | |
| // schemaHasStrictType, but this fallback path didn't). | |
| // Returns ok=false when node carries no strict-expressible type (no "type", | |
| // "anyOf", or "enum") — a fresh anyOf around it would produce a typeless | |
| // anyOf[0] branch that OpenAI strict mode rejects. The anyOf-recursion path in | |
| // strictifyNode guards this via schemaHasStrictType; this fallback didn't. |
Was 7 lines; the same WHY fits in 4.
Comment on lines
+205
to
+215
| // Prod repro (found via the router smoke suite's real-Anthropic run, 2026-07): | ||
| // the Workflow tool's `args` param is declared genuinely typeless — no | ||
| // "type", "anyOf", or "enum" — because it deliberately accepts any JSON value | ||
| // verbatim. makeNullable's fallback branch (used for optional properties with | ||
| // no type/anyOf) wrapped this bare node in a fresh | ||
| // `anyOf:[node, {type:"null"}]` WITHOUT checking whether node itself carried | ||
| // a strict-expressible type first — unlike the anyOf-recursion path in | ||
| // strictifyNode, which already guards this via schemaHasStrictType. The | ||
| // result: `properties.args.anyOf[0]` had no "type" key, and OpenAI rejected | ||
| // the whole tool with 400 "schema must have a 'type' key". makeNullable must | ||
| // bail (fall back to non-strict) rather than emit that invalid anyOf. |
Collaborator
There was a problem hiding this comment.
Suggested change
| // Prod repro (found via the router smoke suite's real-Anthropic run, 2026-07): | |
| // the Workflow tool's `args` param is declared genuinely typeless — no | |
| // "type", "anyOf", or "enum" — because it deliberately accepts any JSON value | |
| // verbatim. makeNullable's fallback branch (used for optional properties with | |
| // no type/anyOf) wrapped this bare node in a fresh | |
| // `anyOf:[node, {type:"null"}]` WITHOUT checking whether node itself carried | |
| // a strict-expressible type first — unlike the anyOf-recursion path in | |
| // strictifyNode, which already guards this via schemaHasStrictType. The | |
| // result: `properties.args.anyOf[0]` had no "type" key, and OpenAI rejected | |
| // the whole tool with 400 "schema must have a 'type' key". makeNullable must | |
| // bail (fall back to non-strict) rather than emit that invalid anyOf. | |
| // Typeless optional property (no "type"/"anyOf"/"enum"): makeNullable's fallback | |
| // wrapped it in anyOf without checking schemaHasStrictType, producing a typeless | |
| // anyOf[0] that OpenAI rejected with 400. Must bail to non-strict instead. |
Was 11 lines re-narrating the full bug history; 3 lines state the invariant.
Comment on lines
+229
to
+232
| // Sanity companion to the above: a property that DOES carry a type (even | ||
| // with typeless array items, which just means "any item") is a different | ||
| // case — the outer "type":"array" is strict-expressible, so it must still | ||
| // strictify successfully. Only a fully typeless node should bail. |
Collaborator
There was a problem hiding this comment.
Suggested change
| // Sanity companion to the above: a property that DOES carry a type (even | |
| // with typeless array items, which just means "any item") is a different | |
| // case — the outer "type":"array" is strict-expressible, so it must still | |
| // strictify successfully. Only a fully typeless node should bail. | |
| // Typed optional with typeless items: outer "type":"array" is strict-expressible, | |
| // so only a fully typeless node should bail — not this one. |
Was 4 lines; the contrast fits in 2.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
 T-Rex LogsWhat T-Rex did
Reviews (1): Last reviewed commit: "feat(smoke): add pre-merge smoke suite w..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Claude finished @steventohme's task —— View job Claude finished @steventohme's task —— View job Posted 6 comment-length suggestions (advisory, won't block merge). |
workweave-bot
left a comment
workweave-bot
left a comment
Collaborator
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
Comment on lines
+100
to
+108
| // sanitizeHeaders drops anything that authenticates the call, identifies the | ||
| // recording account, or is pure per-request noise before persisting to disk. | ||
| // Cassettes are committed to the repo — a leaked API key would be a real | ||
| // incident, and an org ID is a private identifier this repo's own contributing | ||
| // rules forbid committing (see CLAUDE.md "Things to NEVER do"). Rate-limit and | ||
| // request-id headers are dropped too: they're real per-call values that would | ||
| // make every recording look suspicious in a diff, and nothing in the smoke | ||
| // suite reads them back (only the router's own x-router-* response headers | ||
| // are asserted on, never anything replayed from a cassette). |
Collaborator
There was a problem hiding this comment.
Suggested change
| // sanitizeHeaders drops anything that authenticates the call, identifies the | |
| // recording account, or is pure per-request noise before persisting to disk. | |
| // Cassettes are committed to the repo — a leaked API key would be a real | |
| // incident, and an org ID is a private identifier this repo's own contributing | |
| // rules forbid committing (see CLAUDE.md "Things to NEVER do"). Rate-limit and | |
| // request-id headers are dropped too: they're real per-call values that would | |
| // make every recording look suspicious in a diff, and nothing in the smoke | |
| // suite reads them back (only the router's own x-router-* response headers | |
| // are asserted on, never anything replayed from a cassette). | |
| // sanitizeHeaders drops auth credentials, org identifiers, rate-limit counters, and | |
| // request IDs before persisting to disk — cassettes are committed to the repo so leaking | |
| // any of these would be a real incident or a policy violation (see CLAUDE.md). |
Was 9 lines; 3 lines name every category and state the WHY.
|
Claude finished @steventohme's task —— View job Posted 4 comment-length suggestions (advisory, won't block merge). |
workweave-bot
left a comment
workweave-bot
left a comment
Collaborator
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
|
Claude finished @steventohme's task —— View job Comment-length lint pass
Posted 5 comment-length suggestions (advisory, won't block merge). View job | Branch: |
workweave-bot
left a comment
workweave-bot
left a comment
Collaborator
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
steventohme
added a commit
that referenced
this pull request
Jul 23, 2026
…union branch The router's first real-OpenAI smoke run (make smoke, SMOKE_PROXY_MODE=record) caught a live bug in #829's fix: makeNullable's fallback stamps type:["string","number","boolean","object","array","null"] on a typeless optional property's synthetic anyOf branch, but never adds additionalProperties:false — which OpenAI's strict-mode validator requires on EVERY node whose type can be "object", with no exception for a type union that merely includes "object" alongside other primitives. Real request against gpt-5.4-nano with a Workflow-tool-shaped typeless `args` param 400'd: "Invalid schema for function 'Workflow': In context=('properties', 'args', 'anyOf', '0', 'type', '3'), 'additionalProperties' is required to be supplied and to be false." (index 3 = the "object" entry in the six-element type array) Fix: the synthetic branch now also carries additionalProperties:false, an empty properties map, and an empty required list — the full set OpenAI requires for an object-capable schema node. Strengthened the existing #829 unit test (TestStrictify_TypelessOptionalGetsExplicitValueType) with assertions on all three fields, since their absence is exactly what let this ship — the test only checked the type union, not the object-specific requirements a union that includes "object" also carries. This is the first bug the router's real-provider smoke suite (PR #828) has caught live, on its very first recording run — the in-process translator unit tests construct the schema correctly in isolation but never send it to the real API, so nothing before this exercised OpenAI's actual strict-mode validator. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude finished @steventohme's task —— View job Comment-length lint pass
Posted 3 comment-length suggestions (advisory, won't block merge). View job | Branch: |
workweave-bot
left a comment
workweave-bot
left a comment
Collaborator
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
Comment on lines
+211
to
+219
| // | ||
| // Prod repro (found via the router smoke suite's real-OpenAI run, 2026-07): | ||
| // stamping "object" into the type union isn't enough on its own — OpenAI's | ||
| // strict-mode validator requires additionalProperties:false (plus an empty | ||
| // properties/required pair) on EVERY node whose type can be "object", | ||
| // recursively, with no exception for a type UNION that merely includes | ||
| // "object" alongside other primitives. Omitting them 400'd with "In | ||
| // context=(...,'type','3'), 'additionalProperties' is required to be | ||
| // supplied and to be false" (index 3 = the "object" entry in the type array). |
Collaborator
There was a problem hiding this comment.
Suggested change
| // | |
| // Prod repro (found via the router smoke suite's real-OpenAI run, 2026-07): | |
| // stamping "object" into the type union isn't enough on its own — OpenAI's | |
| // strict-mode validator requires additionalProperties:false (plus an empty | |
| // properties/required pair) on EVERY node whose type can be "object", | |
| // recursively, with no exception for a type UNION that merely includes | |
| // "object" alongside other primitives. Omitting them 400'd with "In | |
| // context=(...,'type','3'), 'additionalProperties' is required to be | |
| // supplied and to be false" (index 3 = the "object" entry in the type array). | |
| // | |
| // A type union including "object" must carry additionalProperties:false + empty | |
| // properties/required — OpenAI strict mode requires them on every object-capable | |
| // node, even inside a union; omitting them 400s ("'additionalProperties' is required | |
| // to be supplied and to be false", index 3 = the "object" entry). |
Was 8 lines; the constraint and the exact error message fit in 5.
|
Claude finished @steventohme's task —— View job Comment-length lint pass
Posted 4 comment-length suggestions (advisory, won't block merge). View job | Branch: |
workweave-bot
left a comment
workweave-bot
left a comment
Collaborator
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
Comment on lines
+258
to
+266
| // Prod repro (found via the router smoke suite's real-OpenAI run, 2026-07, | ||
| // PR #828): the type union above includes "object", and OpenAI's strict-mode | ||
| // validator requires additionalProperties:false (plus an empty | ||
| // properties/required pair) on EVERY node whose type can be "object" — no | ||
| // exception for a type union that merely includes "object" alongside other | ||
| // primitives. The union-only fix from #829 didn't add these, and 400'd with | ||
| // "In context=(...,'type','3'), 'additionalProperties' is required to be | ||
| // supplied and to be false" against the real API (index 3 = the "object" | ||
| // entry in the six-element type array above). |
Collaborator
There was a problem hiding this comment.
Suggested change
| // Prod repro (found via the router smoke suite's real-OpenAI run, 2026-07, | |
| // PR #828): the type union above includes "object", and OpenAI's strict-mode | |
| // validator requires additionalProperties:false (plus an empty | |
| // properties/required pair) on EVERY node whose type can be "object" — no | |
| // exception for a type union that merely includes "object" alongside other | |
| // primitives. The union-only fix from #829 didn't add these, and 400'd with | |
| // "In context=(...,'type','3'), 'additionalProperties' is required to be | |
| // supplied and to be false" against the real API (index 3 = the "object" | |
| // entry in the six-element type array above). | |
| // A type union including "object" needs additionalProperties:false + empty properties/required; | |
| // OpenAI strict mode 400s otherwise ("'additionalProperties' is required to be supplied and to | |
| // be false", index 3 = the "object" entry in the six-element type array). |
Was 9 lines restating the prod repro; the constraint and exact error text fit in 3.
|
Claude finished @steventohme's task —— View job Comment-length lint pass
Posted 5 comment-length suggestions (advisory, won't block merge). View job | Branch: |
workweave-bot
left a comment
workweave-bot
left a comment
Collaborator
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
|
Claude finished @steventohme's task —— View job Comment-length lint pass
Posted 4 comment-length suggestions (advisory, won't block merge). View job | Branch: |
workweave-bot
left a comment
workweave-bot
left a comment
Collaborator
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 827f3dc. Configure here.
|
Claude finished @steventohme's task —— View job Comment-length lint pass
Posted 5 comment-length suggestions (advisory, won't block merge). View job | Branch: |
workweave-bot
left a comment
workweave-bot
left a comment
Collaborator
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
workweave-bot
left a comment
workweave-bot
left a comment
Collaborator
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
|
Claude finished @steventohme's task —— View job Comment-length lint pass
Posted 2 comment-length suggestions (advisory, won't block merge). View job | Branch: |
workweave-bot
left a comment
workweave-bot
left a comment
Collaborator
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
|
Claude finished @steventohme's task —— View job Comment-length lint pass
Posted 5 comment-length suggestions (advisory, won't block merge). View job | Branch: |
workweave-bot
left a comment
workweave-bot
left a comment
Collaborator
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
workweave-bot
left a comment
workweave-bot
left a comment
Collaborator
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
|
Claude finished @steventohme's task —— View job Comment-length lint pass
Posted 1 comment-length suggestion (advisory, won't block merge). |
workweave-bot
left a comment
workweave-bot
left a comment
Collaborator
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
|
Claude finished @steventohme's task —— View job Comment-length lint pass
No verbose comment blocks found — all 3+ line blocks carry genuine non-obvious WHY. Exiting silently. View job | Branch: |
|
Claude finished @steventohme's task —— View job Comment-length lint pass
No verbose comment blocks found — all 3+ line blocks carry genuine non-obvious WHY, already trimmed by prior rounds. Exiting silently. View job | Branch: |
steventohme
added a commit
that referenced
this pull request
Jul 24, 2026
…union branch The router's first real-OpenAI smoke run (make smoke, SMOKE_PROXY_MODE=record) caught a live bug in #829's fix: makeNullable's fallback stamps type:["string","number","boolean","object","array","null"] on a typeless optional property's synthetic anyOf branch, but never adds additionalProperties:false — which OpenAI's strict-mode validator requires on EVERY node whose type can be "object", with no exception for a type union that merely includes "object" alongside other primitives. Real request against gpt-5.4-nano with a Workflow-tool-shaped typeless `args` param 400'd: "Invalid schema for function 'Workflow': In context=('properties', 'args', 'anyOf', '0', 'type', '3'), 'additionalProperties' is required to be supplied and to be false." (index 3 = the "object" entry in the six-element type array) Fix: the synthetic branch now also carries additionalProperties:false, an empty properties map, and an empty required list — the full set OpenAI requires for an object-capable schema node. Strengthened the existing #829 unit test (TestStrictify_TypelessOptionalGetsExplicitValueType) with assertions on all three fields, since their absence is exactly what let this ship — the test only checked the type union, not the object-specific requirements a union that includes "object" also carries. This is the first bug the router's real-provider smoke suite (PR #828) has caught live, on its very first recording run — the in-process translator unit tests construct the schema correctly in isolation but never send it to the real API, so nothing before this exercised OpenAI's actual strict-mode validator. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds an end-to-end smoke suite that boots the real router (docker compose) and drives it with deterministic, Claude-Code-shaped request fixtures against real upstream providers, asserting behavior in-process unit and conformance tests can't see: HTTP status, response/usage shape, prompt-cache accounting, decision headers, SSE stream ordering, and OpenAI tool-schema translation. Architecture: smoke/mitmproxy/ is a small MITM forward proxy sitting between the router and its providers. The router's transport already honors http.ProxyFromEnvironment, so no router code changes are needed to intercept traffic. It mints an ephemeral CA + per-host leaf certs in memory (only the public cert ever touches disk), and record/replays cassettes keyed by sha256(method+path+body). Three modes: replay-only (serves committed cassettes, no API key needed — the PR CI default), record (always live), replay-or-record (local default). CI: .github/workflows/smoke.yml path-gates a replay-only job to the regression-prone surfaces (internal/proxy, internal/translate, internal/providers, internal/router/catalog, cmd/router, smoke/, docker-compose.yml, Dockerfile) — needs no secrets, safe on forks. A separate nightly job re-records cassettes against the real APIs and opens a PR on drift. Both jobs use docker/setup-buildx-action + docker/setup-compose-action, and a CI-only compose overlay (docker-compose.ci-cache.yml, gated behind SMOKE_CI_CACHE=1) layers the server/mitmproxy Docker builds against the GitHub Actions cache backend — type=gha hard-errors outside a real GHA runner, so it's kept out of the always-on overlay `make smoke` uses locally. Scenarios: boot_test.go (health/version/models), basic_test.go (force-model command, non-stream/stream turns), cache_test.go (router-injected caching warms then reads; client-at-capacity doesn't over-inject; ttl=1h breakpoint not poisoned; overflow rejected as 400 not 502), streaming_test.go (SSE lifecycle), openai_test.go (Responses-API translation). Also fixes one real bug the suite's first runs surfaced: - internal/proxy/dispatch_error.go: Anthropic cache_control validation errors (overflow / invalid TTL ordering) previously fell through to the generic "Upstream call failed" 502 handler, even though no upstream call was ever attempted — the router's own validator rejected the request before dispatch. Now classified as a clean 400 with the actual validator message, matching the pattern for other client-input errors. (openai_test.go's typeless-optional-tool-param scenario surfaced a second bug in internal/translate/strictify_openai.go's makeNullable fallback; that was independently fixed by #829 on main, already incorporated via rebase, so this PR only carries the end-to-end smoke coverage for it.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixed: - smoke/request_builder_test.go, smoke/cache_test.go: buildTools() applied toolTTL to tools[len(tools)-1] AFTER appending the extraTools-generated tools, so cachedTools(N).toolCache(ttl) always re-tagged an already- breakpointed extra tool instead of adding a new one. The "at capacity" cache scenario silently produced 3 breakpoints instead of 4, so it never actually exercised the over-injection regression it targets. toolTTL now tags the Edit tool (a separate, non-extra tool) before the extras are appended, so the two knobs are additive as the test comments always claimed. (cursor bugbot) - smoke/mitmproxy/store.go: sanitizeHeaders only dropped Anthropic's org-id and rate-limit header names; OpenAI's own org identifier (openai-organization), request id (x-request-id), and rate-limit headers (x-ratelimit-*) were untouched, so recorded OpenAI cassettes (committed to the repo) could carry those identifiers. Added the OpenAI header set alongside the existing Anthropic one. (cursor bugbot) - internal/proxy/dispatch_error.go, internal/proxy/dispatch_error_test.go: trimmed two over-long comments per workweave-bot's advisory review. Declined (not changed, rationale only): - greptile-apps flagged that smoke/mitmproxy/cassettes/ has no recorded cassette JSON yet, so the path-gated PR job's replay-only run will 502. Correct and already known — cassettes require a local `SMOKE_PROXY_MODE=record` run with real provider keys, which isn't something to fake or stub around. Left unresolved; recording is a follow-up, not a code change this PR can make. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixed:
- internal/proxy/dispatch_error.go: the cache_control DispatchErrorClass set
Message to err.Error(), leaking the internal "emit body: " wrap-chain
prefix into the client-facing error body — every other branch returns a
clean message. Added unwrapToSentinelMessage, which walks the chain to the
layer whose direct child is the sentinel itself (naive errors.Is-based
walking always matches at the outermost layer, since errors.Is is true at
every level), keeping the sentinel's own dynamic detail ("got 5, maximum
is 4") without the internal prefix. Added assertions on cls.Message to
both existing tests so this can't regress silently again. (cursor bugbot)
- smoke/cache_test.go, smoke/openai_test.go, smoke/mitmproxy/proxy.go: trimmed
four more over-long comments per workweave-bot's advisory review.
Declined (not changed, rationale only):
- greptile-apps's cassettes-missing flag from iteration 1 still applies —
unchanged, still blocked on a local recording run with real provider keys.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TestCaching/router-injected_caching_warms_then_reads asserted the FIRST call always writes the cache (cache_creation_input_tokens > 0), assuming a cold cache. That assumption breaks under normal local iteration: Anthropic's prompt cache has a ~5-minute TTL, and rerunning `make smoke` repeatedly against the byte-identical system-prompt fixture within that window finds the prefix already warm from the PREVIOUS run — so the "first" call in a given run is actually a cache READ, not a WRITE, and the assertion failed with cache_creation=0 even though caching was working correctly (cache_read was already >0, visible directly in the recorded cassette bodies). This was caught running the suite for real (make smoke, live Anthropic calls) after two rapid-fire iterations minutes apart — a false failure, not a router bug: nothing in internal/translate/cache_control.go or the router's injection logic was at fault. Fix: accept EITHER cache_creation_input_tokens > 0 OR cache_read_input_tokens > 0 on the first call — proving caching is engaged at all, without assuming which state (cold vs. already-warm) the fixture happens to be in. The second call's cache_read assertion is unchanged (that one was always correct: a same-prefix follow-up must read the cache regardless of whether the first call wrote or read it). With this fix, all five smoke scenarios (boot, basic, caching, OpenAI Responses API, streaming) pass end-to-end against the real Anthropic + OpenAI APIs. Cassettes recorded from this clean, fully-green run are included so PR CI's replay-only job has something to replay. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixed: - smoke/mitmproxy/store.go: sanitizeHeaders was missing "openai-project" in its drop list, so two committed cassettes leaked a real OpenAI project ID in their response headers. Added the header to the drop list and stripped it from the two affected cassettes (fix-forward per team decision — no history rewrite; a project ID isn't a credential). - scripts/smoke/run.sh: renamed the ephemeral compose override from the well-known docker-compose.override.yml to a dedicated docker-compose.smoke-run.override.yml so a smoke run never clobbers a developer's own local override (used by .claude/skills/test-claude-locally). Also reordered cleanup() so `docker compose down` (which still references the override via -f) runs before the file is deleted, instead of after — the old order made teardown fail silently and could leave the stack up. - .github/workflows/smoke.yml: nightly cassette-refresh drift check used only `git diff --quiet`, which misses brand-new (untracked) cassette files from a first recording or new scenario. Now also checks `git status --porcelain`, matching run.sh's own already-correct check. - smoke/mitmproxy/store.go: cassette.Body was a []byte, so encoding/json persisted it as base64 — undoing the gzip-decompress step's stated goal of human-diffable, secret-scannable cassettes. Added a rawTextBody wrapper that marshals as a plain JSON string and migrated the 10 committed cassettes from base64 to plain text. Applied from workweave-bot review (comment-length nits, applied verbatim): - smoke/harness_test.go, smoke/cache_test.go, smoke/mitmproxy/proxy.go (x2), smoke/mitmproxy/ca.go, smoke/mitmproxy/store.go, smoke/request_builder_test.go, internal/proxy/dispatch_error.go, internal/translate/strictify_openai.go, internal/translate/strictify_openai_test.go (x2) Declined (stale / already addressed by prior commits in this branch): - greptile-apps' "replay cassettes missing" — cassettes were committed in a later commit on this branch; the file list it saw was pre-recording. - unwrapToSentinelMessage "leaks wrapper prefix" — verified with an accurate two-level repro matching the real wrap chain; the original code is correct.
Fixed: - smoke/basic_test.go: assertServedByModel silently passed when x-router-provider was missing entirely (only checked mismatch, not absence), unlike the stricter check just above it for x-router-model. Now requires the header to be present, matching the model check. Applied from workweave-bot review (comment-length nits, applied verbatim): - smoke/cache_test.go (x2), smoke/harness_test.go
Applied from workweave-bot review (comment-length nits, applied verbatim): - smoke/streaming_test.go (x2), smoke/openai_test.go, smoke/mitmproxy/store.go, smoke/mitmproxy/proxy.go
Applied from workweave-bot review (comment-length nits, applied verbatim): - smoke/mitmproxy/main.go, smoke/request_builder_test.go
Applied from workweave-bot review (comment-length nits, applied verbatim): - smoke/harness_test.go (x3), smoke/openai_test.go, smoke/mitmproxy/proxy.go
Applied from workweave-bot review (comment-length nit, applied verbatim): - smoke/basic_test.go
The pre-merge smoke suite (smoke/, docs/SMOKE.md) had no pointer from the root agent-guidance files, so an agent working on internal/proxy or internal/translate would have no way to discover it exists or know when to add a scenario. Added to the Tests convention section (both mirrored files) alongside the existing testing guidance: what it covers, the CI path-gate, and when a change warrants a new scenario vs. relying on existing unit/conformance coverage.
|
Claude finished @steventohme's task —— View job Comment-length lint pass
No verbose comment blocks found — all 3+ line blocks carry genuine non-obvious WHY, already trimmed by prior rounds. Exiting silently. View job | Branch: |
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.
What
An end-to-end smoke suite that boots the real router (docker compose) and drives it with deterministic, Claude-Code-shaped fixtures against real upstream providers — asserting behavior in-process unit/conformance tests can't see: HTTP status, response/usage shape, prompt-cache accounting, decision headers, SSE ordering, and OpenAI tool-schema translation.
Architecture
smoke/mitmproxy/is a small MITM forward proxy sitting between the router and its providers. The router's transport already honorshttp.ProxyFromEnvironment— zero router code changes needed to intercept traffic. It mints an ephemeral CA + per-host leaf certs in memory (only the public cert ever touches disk), and record/replays cassettes keyed bysha256(method+path+body).Three modes:
replay-only(serves committed cassettes, no API key — the PR CI default),record(always live),replay-or-record(local default).CI
.github/workflows/smoke.yml: path-gatedreplay-onlyjob on the regression-prone surfaces (internal/proxy,internal/translate,internal/providers,internal/router/catalog,cmd/router,smoke/,docker-compose.yml,Dockerfile) — needs no secrets, safe on forks.docker-compose.ci-cache.yml, gated behindSMOKE_CI_CACHE=1) layers the server/mitmproxy Docker builds against the GitHub Actions cache backend (type=gha) — kept out of the always-on overlay since it hard-errors outside a real GHA runner.Note: no cassettes are committed yet — replay-only CI will fail until a maintainer runs
ANTHROPIC_API_KEY=… OPENAI_API_KEY=… SMOKE_PROXY_MODE=record make smokelocally and commits the recorded cassettes.Scenarios
boot_test.go/health,/v1/version,/v1/router/modelsbasic_test.gocache_test.gottl=1hbreakpoint not poisoned; overflow → clean 400streaming_test.gomessage_stop)openai_test.goAlso fixes two real bugs the suite's first runs surfaced
internal/proxy/dispatch_error.go— Anthropiccache_controlvalidation errors (overflow / invalid TTL ordering) fell through to a generic502 Upstream call failed, even though no upstream call was ever attempted — the router's own validator rejected the request pre-dispatch. Now a clean400with the actual validator message.internal/translate/strictify_openai.go—makeNullable's fallback (wraps an optional property with notype/anyOf/enum, e.g. a tool param documented to accept "any JSON value verbatim") produced an invalidanyOf[0]with notypekey → OpenAI Responses strict mode 400s. TheanyOf-branch recursion already guarded this viaschemaHasStrictType; the fallback didn't. Complements fix(translate): normalize typeless OpenAI anyOf branches #824 (emit_openai.go's stamping fix for the general translation path) by fixing the strict-mode-specific fallback fix(translate): normalize typeless OpenAI anyOf branches #824 doesn't touch.Verification
go build ./...,go vet ./...,go test ./...all clean.smoke/package compiles under-tags smoke, invisible to untaggedgo test ./....docker compose configverified against the real binary for both the default and CI-cache overlay paths.🤖 Generated with Claude Code