Repository navigation
fix(start-plan): route to the live Anthropic gateway - #34
Conversation
The start-plan OpenAI gateway (POST /api/v1/zcode-plan/chat/completions) was retired server-side and returns a plain 404 since the rollout of the Anthropic-messages gateway (observed 2026-08-28). The live ZCode desktop client (3.10.1) now posts Anthropic-format messages to POST /api/v1/zcode-plan/anthropic/v1/messages with the start-plan JWT as a Bearer credential. - buildUpstreamURL: start-plan now returns the live Anthropic gateway path - handler.ts: start-plan no longer translates to OpenAI; both plans send Anthropic upstream (translateOpenAIToAnthropic covers client-format translation only) - upstream.test.ts: rewrite the 9 start-plan tests that asserted the dead OpenAI gateway to assert the live Anthropic gateway (URL, Bearer JWT, anthropic-version, start-plan system blocks, cache_control, default max_tokens, thinking passthrough in both client formats, and 403 -> 502 translation_failed wrapping for OpenAI clients)
… completes The Aliyun pe risk engine references a bare `print` global. The window polyfill defines a harmless no-op stub, but HOST_CRITICAL_GLOBALS listed `print`, so the Bun global-alias pass skipped it and guest scripts (which run in the HOST realm under Bun) hit `ReferenceError: print is not defined` inside the pe risk engine — breaking the fingerprint chain and degrading solve success (711 WINDOW-ERRORs in one day of serving). Bun's host global has no native `print`, so aliasing the stub shadows nothing critical. Remove `print` from HOST_CRITICAL_GLOBALS.
The start-plan gateway serves the -flash variant (the model used by claimed trial plans like the weekend package), but the pinned catalog only listed the 10 coding-plan models, so client-side discovery never listed glm-5.3-flash. Add it with the same 1M-context specs as glm-5.3.
…ct failures
Two resilience gaps observed against the live gateway (2026-08-29):
1. The gateway sometimes issues the captcha challenge as HTTP 400 with
{"code":3007,"msg":"captcha verify failed"} in the JSON body instead
of the captcha response header. The header-based challenge detection
never fired, so clients received a raw 502 instead of a retried
request. For non-OK, non-SSE responses, peek a clone of the body for
"code":3007 and treat it as a challenge so the existing header-based
retry block handles both shapes.
2. Transient connect failures (DNS blip, TLS reset — Bun's 'Unable to
connect') surfaced immediately as 502 upstream_unreachable. Retry the
connect up to 3 attempts with 500ms/1s backoff; a failed CONNECT never
reached upstream, so resending is side-effect-free.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b0a4e08a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const maxConnectAttempts = 3; | ||
| for (let attempt = 1; ; attempt++) { | ||
| try { | ||
| upstreamResp = await dispatch(upstreamReq, upstreamHeaderPairs); |
There was a problem hiding this comment.
Rebuild the Request before each fetch retry
For the normal unsigned and unrouted path—particularly every start-plan request—dispatch passes the same upstreamReq object to fetch on every attempt. Bun marks that request's body as used even when fetch rejects with Unable to connect, so attempts 2 and 3 fail immediately with Request body already used instead of reconnecting, after unnecessarily waiting through both backoffs. Construct a fresh Request from transformedBody for each attempt, as the captcha retry already does.
Useful? React with 👍 / 👎.
| } catch (err) { | ||
| if (attempt >= maxConnectAttempts) throw err; |
There was a problem hiding this comment.
Retry only failures known to precede request writes
When an explicit or enforced session selects sendOrderedUpstreamRequest, dispatch can reject because the socket closed before response headers even though ordered-transport.ts has already written the complete request body; this unconditional catch then submits the completion again, potentially consuming quota up to three times. It also retries client AbortErrors and delays cancellation by the backoffs. Restrict retries to failures positively identified as occurring before the request was written and stop immediately when clientReq.signal is aborted.
Useful? React with 👍 / 👎.
TriDefender
left a comment
There was a problem hiding this comment.
Request changes
The core routing fix is right and the evidence is solid — verified bun x tsc --noEmit clean and 565/565 tests pass locally on this branch. However, two issues block merge: the new connect-retry never actually reaches upstream on the path it targets, and the start-plan /v1/responses route is broken differently by the same URL change.
P1 — the connect-retry loop is dead code on start-plan (Request body already used)
The retry loop re-dispatches the same upstreamReq Request object, but sendUpstreamRequest calls fetch(req, { decompress, signal }) with no body in the init — the body flows from the Request object's own stream. After a connect-level failure that body is marked used, so attempt 2 throws before any network I/O. Verified empirically on both runtimes (raw-socket server that resets the 1st connection and serves HTTP on the 2nd; same Request object reused; init without body — faithful to sendUpstreamRequest):
- Bun: attempt 1 connect failure →
req.bodyUsed === true→ attempt 2 throwsRequest body already used→ attempt 3 same → 502. The body never reaches upstream. - Node 24 / undici: attempt 2 throws
Cannot construct a Request with a Request object that has already been used. Same outcome.
The retry only works where dispatch rebuilds the Request — i.e. when endpoint routing remaps the URL or client signing rewrites the header pairs (both branches construct new Request(..., body: transformedBody)). Start-plan hits neither escape hatch: the JWT credential contains two dots so parseSigningCredential returns undefined → signing skipped (finalPairs === pairs, same reference), and the exact-match routing table doesn't cover the start-plan URL. So the fix written for start-plan is ineffective exactly there — it adds ~1.5s of backoff and then replaces the real connect error with a misleading 502 upstream_unreachable: Request body already used.
Suggested fix: drop the sendUrl === req.url && finalPairs === pairs ? req : ... reuse branch in dispatch and always build a fresh Request per attempt (the routing/signing branch already uses and proves that construction), or rebuild upstreamReq inside the retry loop before re-dispatching.
Test note: a mock fetchImpl that throws-then-succeeds will not catch this — mocks don't enforce bodyUsed the way real fetch does. The regression test needs to assert freshness explicitly (e.g. expect(req.bodyUsed).toBe(false) inside the mock on every call), or use a real-socket integration test.
P2 — start-plan /v1/responses now posts an OpenAI body to the Anthropic endpoint
src/proxy/responses-handler.ts:148 still does upstreamFormat = startPlan ? "openai" : "anthropic" and builds an OpenAI Chat body for start-plan, while this PR makes buildUpstreamURL return the Anthropic messages URL for start-plan unconditionally. Net effect for start-plan Codex CLI / Agents SDK users: before this PR → 404 on the retired gateway; after this PR → an OpenAI-format body POSTed to the live Anthropic messages endpoint.
The fix is the same flip as handler.ts: the response side already has the full Anthropic→Chat→Responses branch keyed on upstreamFormat (~line 208, translateRequestOpenAIToAnthropic on the request side, anthropicSseToOpenaiSse / translateResponseAnthropicToOpenAI on the response side), so making upstreamFormat unconditional "anthropic" is a complete fix.
(Pre-existing, not this PR's regression, but becomes load-bearing once this route works: responses-handler has no captcha challenge handling at all — no loadCaptcha, no challenge retry — unlike handler.ts.)
P3 — no tests for the two new resilience behaviors
Neither the in-body 3007 detection nor the connect retry is covered by the 565 passing tests. The 3007 detection is easy to unit test (mock a 400 + {"code":3007} body, assert re-solve + one retry).
P4 — README routing diagram is now stale
README lines ~242–257 still describe start-plan as the OpenAI-compatible gateway with passthrough / TRANSLATE Anthropic→OpenAI semantics. Needs a sync with this change.
Nits
- The handler comment says "v2.4" while the project version is 2.6.0.
upstream.ts: the comment "The legacy OpenAI route`${STARTPLAN_ANTHROPIC_BASE}/chat/completions`" reads odd with the renamed variable.- The retry loop doesn't skip on client abort (
clientReq.signal.aborted) — aborted requests still get retried with backoff before 502ing to a disconnected client.
Verified as sound
- The routing change matches the evidence (3.10.1 posts Anthropic messages with Bearer JWT; old path 404s); the rewritten tests assert the new gateway shape thoroughly (URL, Bearer JWT, anthropic-version, start-plan system blocks, cache_control, max_tokens defaults, thinking passthrough in both client formats, 403 → 502 translation_failed wrap consistent with coding-plan).
- Decompress semantics stay correct:
translateModeis now true only for OpenAI clients; Anthropic clients on both plans are passthrough (decompress: false+stripAutoDecodedEncodingon Node) — anti-pattern #6 holds. - The in-body 3007 peek is well-placed (after the 401 early-return, before the retry block), uses
clone()per the project's observe+forward convention, excludes SSE, and the sentinel value is only used truthily. - The captcha
printfix is correct:printis inEXTRA_WINDOW_PROPSbut was skipped by the alias pass because ofHOST_CRITICAL_GLOBALS; neither the Bun nor the Node host defines a globalprint, so aliasing the stub shadows nothing. - The async bridge is unaffected (its own
/api/v1/off-peak/anthropic/v1/messagesendpoint).
…nconditional Anthropic upstream on /v1/responses - handler.ts: rebuild upstreamReq inside the connect-retry loop so every attempt fetches with a fresh body stream (a reused Request is marked bodyUsed after the first fetch, so attempts 2+ throw 'Request body already used' before any I/O — dead retry exactly on start-plan, which hits the plain pass-through path where dispatch does not rebuild the Request). Skip retries once the client signal is aborted. - responses-handler.ts: always translate Responses -> Chat -> Anthropic for both plans. Previously start-plan built an OpenAI Chat body while buildUpstreamURL now returns the live Anthropic messages endpoint, which would post an OpenAI-format body to an Anthropic endpoint. startPlan body transform (system blocks / cache_control) is preserved. Review: TriDefender request-changes on PR TriDefender#34 (P1, P2). Nits P3/P4 (tests for the new resilience behaviors, README routing sync) follow in a separate commit.
…only upstream - README routing diagram now shows both plans posting to the zcode.z.ai /api/v1/zcode-plan/anthropic/v1/messages gateway and the /v1/responses Anthropic-only chain (review P4). - handler.ts plan comment version bumped v2.4 -> v2.6 to match package.json (review nit).
…ct retry
Two regression tests for the resilience behaviors flagged in review P3:
- in-body 3007: a start-plan upstream 400 with {"code":3007} in the
JSON body (no captcha header) is treated as a captcha challenge and
retried with a fresh token (asserts the retry header carries tok-2,
not the consumed tok-1).
- connect retry freshness: the mock asserts req.bodyUsed === false on
every call, per the review's test note — a mock that merely
throws-then-succeeds does not catch the reused-Request bug, because
mocks do not enforce bodyUsed the way real fetch does.
Captcha module is injected via mock.module("./captcha.js") (same
technique as captcha-pool.test.ts).
… the PR branch per maintainer's own merge of 8b273aa
|
Thanks for the thorough review — the empirical verification (raw-socket reset test on both runtimes) was exactly the kind of evidence needed. All points are addressed in 3 new commits (673b498, 98590af, ad4af31 + merge 15d756e bringing in your master merge). P1 — dead connect-retry on start-plan: Fixed by rebuilding P2 — /v1/responses OpenAI body to Anthropic endpoint: P3: Both new behaviors now covered in P4 + nits: README routing diagram synced to the Anthropic-only upstream reality; Also merged your 8b273aa (master + PR #33 usage fixes) into the branch — no conflicts, full suite green after the merge. One note for future readers: the in-body 3007 challenge variant was observed live on 2026-08-29 (weekend plan, evening peak) — the gateway appears to switch between the header-based and body-based challenge forms under load, so both detections are worth keeping. |
TriDefender
left a comment
There was a problem hiding this comment.
Approve
Thanks for the quick turnaround — all points addressed and verified locally on head 15d756e0: bun x tsc --noEmit clean, 589/589 tests pass, and I re-ran the empirical raw-socket check against the fixed shape (fresh Request per attempt, no body in the fetch init): attempt 2 succeeds after a connection reset and the upstream receives the body intact. The P1 fix (rebuilding upstreamReq via buildUpstreamRequest after backoff, mirroring the proven captcha-retry pattern) plus the client-abort guard is correct, and the P2 flip in responses-handler (upstreamFormat = "anthropic" unconditional, startPlan preserved through transformRequestBody) closes the /v1/responses gap.
Two non-blocking follow-ups, for whoever picks up the next pass — neither blocks this merge:
1. The new freshness assertion is vacuous (test-quality)
In handler-resilience.test.ts, the mock asserts req.bodyUsed === false on every call. A pure mock fetchImpl never consumes the Request body, so bodyUsed stays false on every call — including with the original bug (the reuse bug only manifests inside real fetch's internals). The test documents intent but would not fail on a regression back to Request reuse. To actually pin it:
- assert object identity instead: record
firstReqon call 1 and assertexpect(req).not.toBe(firstReq)on call 2, or - have the throwing mock call consume the body first (
await req.text()beforethrow), makingbodyUsedobservable.
2. The retry catches all dispatch throws, including post-write failures (correctness edge)
The loop retries on any throw from dispatch, but a failure after the request body was fully written (ordered transport when clientIdentity is configured, or plain fetch where the server accepts the request and closes before response headers) also triggers a resend — the upstream may have already processed the LLM request, so the resend isn't strictly side-effect-free as the comment claims (duplicate token consumption possible). Low probability and the default config is unaffected (ordered transport off), but worth tightening later: have sendOrderedUpstreamRequest mark post-write errors (e.g. a flag attached to the thrown error) and skip the retry for those, narrowing the retry to genuine connect-stage failures.
…ge failures Two non-blocking follow-ups from the PR #34 review: 1. The freshness regression test asserted req.bodyUsed === false inside a mock fetchImpl — vacuous, because a mock never consumes the Request body, so it passed even under the original reuse bug. Pin freshness by object identity instead: record the first Request and assert the retried dispatch receives a different instance. 2. The connect-retry loop re-sent the request on ANY dispatch throw, including failures after the ordered transport already wrote the full request head+body to the wire (socket closed before response headers) — resending in that window could submit the completion twice and consume quota up to three times. sendOrderedUpstreamRequest now flags errors raised after the write completed (err.postWrite), and the retry loop skips flagged errors, narrowing retries to genuine connect-stage failures. Plain-fetch post-headers-write failures remain undetectable at this layer (unchanged, per review).
Two more bare 502 rows appeared (batch 452ms, stream 2.9s) with zero accompanying lines even after the gateway-retry logging. Static elimination leaves exactly two producers that reach the 502 catch without any line: the ladder's pre-flight abort throw (client hung up while the request was still being read/transformed) and a postWrite transport death (request fully on the wire, never retried by the PR TriDefender#34 review decision). Both now log their reason in the 502 catch of both the chat and /v1/responses paths, and the abort error string became a shared constant so the catch recognizes it without typing. Also replaces an em dash that had slipped into the previous skip log.
Summary
Fixes start-plan routing so the proxy works against the live gateway again.
POST /api/v1/zcode-plan/chat/completions) was retired server-side and returns a plain 404 since the rollout of the Anthropic-messages gateway (observed 2026-08-28). The live ZCode desktop client (3.10.1) now posts Anthropic-format messages toPOST /api/v1/zcode-plan/anthropic/v1/messageswith the start-plan JWT as a Bearer credential (confirmed from the live client's runtime config/logs).translateOpenAIToAnthropicnow covers client-format translation only.HTTP 400,{"code":3007,"msg":"captcha verify failed"}in the JSON body) — previously only the response-header challenge was detected, so clients received a raw 502 instead of a retried request.printstub under Bun:HOST_CRITICAL_GLOBALSlistedprint, so guest scripts running in the host realm hitReferenceError: print is not definedin the Aliyun pe risk engine, breaking the fingerprint chain and degrading solve success (711 WINDOW-ERRORs in one day of serving). Bun's host global has no nativeprint, so aliasing the stub shadows nothing.glm-5.3-flashin the pinned catalog (the model used by claimed trial plans like the weekend package).Evidence
Server-side retirement of the old start-plan OpenAI gateway observed 2026-08-28:
POST https://zcode.z.ai/api/v1/zcode-plan/chat/completions→404 page not found(plain text, same as a bogus path), while sibling/billing/preview→ 400 (path exists)runtimeModel.baseURL = https://zcode.z.ai/api/v1/zcode-plan/anthropic,apiFormat: anthropic-messages, providerbuiltin:zai-start-plan,hasActiveStartPlan: trueTesting
bun x tsc --noEmitcleanbun test: 565 pass / 0 failNote on overlap with #32
#32 also touches
src/provider/models.ts(adds glm-5.3-flash) plus translator/reasoning work. This PR is independent and focused on the start-plan routing fix; if #32 lands first, the models.ts hunk here will need a trivial rebase. The routing/captcha fixes here are orthogonal to #32.