relay: round-trip tool metadata + surface upstream non-2xx as error (PR #323 review fix)#325
Open
FMXExpress wants to merge 1 commit into
Open
Conversation
Two PR #323 review fixes. P1: Tool-call metadata MUST round-trip through the envelope. A multi- turn relayed session that fires a tool call had assistant messages with ToolCalls and tool-result messages with ToolCallId/Name; both got dropped because BuildRelayRequestBody only emitted role+content, and the worker's DecodeMessages only read those two. The second turn then sent OpenAI/Anthropic/Gemini a request with no tool_call_id -> "missing tool_call_id" 400 / silent stall. Encoder now emits the OpenAI-shape fields (tool_calls array, tool_call_id, name) when non-empty; worker decoder reads them back. Same wire fields workers already see for response tool calls (HandleRelayRespond), so any worker that already forwards results forward this too. P2: Workers must encode any non-2xx upstream provider status as a relay error, not as a successful reply. Pre-fix, BuildResponseJSON only treated StatusCode=-1 (socket/TLS failure) as an error. A 429 or 5xx from upstream OpenAI/etc. fell into the else branch with the error JSON body as content, and the gateway's DecodeResponse mapped that to StatusCode := 200 -- bypassing Cfg.Fallbacks entirely and surfacing the provider error text to the agent as the assistant's reply. New ResponseIsError helper treats StatusCode -1 and anything outside [200..299] as error (StatusCode = 0 from older providers still treats as success to preserve compat). Tests: extended src/tests/relay_queue_tests.pas with - tool-call metadata round-trips through the envelope - worker encodes non-2xx upstream status as a relay error covering 429 / 500 / -1 / 200 / 0 paths. Same suite, runs via `make test-relay-queue`. All 20 cases green. BuildRelayWorkerResponseJSON is now exported from PasClaw.Cmd.Relay solely so the test program can drive the encoder directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TBcLtmpj7dqA5tyFbGnQon
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00d9ba60c8
ℹ️ 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".
Comment on lines
+198
to
+202
| TCObj.PutStr('id', Messages[i].ToolCalls[j].Id); | ||
| TCObj.PutStr('type', Messages[i].ToolCalls[j].Kind); | ||
| FnObj := TJsonObject.Create; | ||
| FnObj.PutStr('name', Messages[i].ToolCalls[j].Func.Name); | ||
| FnObj.PutStr('arguments', Messages[i].ToolCalls[j].Func.Arguments); |
There was a problem hiding this comment.
When the relay worker is backed by Gemini 3+ and returns a tool call, the TToolCall.ProviderSignature must be echoed on the next request so PasClaw.Providers.Gemini can emit thoughtSignature (see src/pkg/providers/PasClaw.Providers.Gemini.pas:531-539). This new request envelope serializes only id/type/function fields, and the matching decoder only restores those fields, so relayed Gemini tool-call continuations still drop the signature and the next Gemini request fails with the documented missing thought_signature 400.
Useful? React with 👍 / 👎.
5 tasks
khongten001
pushed a commit
to khongten001/PasClaw
that referenced
this pull request
Jul 13, 2026
PR FMXExpress#326 review P2: a browser worker page served from a different origin than the gateway -- the documented browser/WebLLM case -- got blocked by CORS before the bearer-token / worker-id checks ever ran. The query-string fallback fix in the previous commit was necessary (EventSource can't set headers) but not sufficient (CORS blocks all of it). Adds permissive CORS on the three /v1/relay/* endpoints: - EmitRelayCors: Access-Control-Allow-Origin: *, Allow-Methods: GET, POST, OPTIONS, Allow-Headers reflecting the browser's preflight request (or a sensible default), Max-Age: 600 (10-minute preflight cache). - HandleRelayOptionsPreflight: 204 + Allow-* headers for the CORS preflight request, before the bearer-token gate (per the CORS spec, preflights must not carry credentials). - OnCommandOther wired on TIdHTTPServer to receive OPTIONS; routes /v1/relay/* preflights to HandleRelayOptionsPreflight and 405s everything else. - HandleRelayPoll, HandleRelayRespond, HandleRelayStatus stamp CORS headers on the actual response. HandleRelayStatus's signature gained an ARequest parameter so it can read Access-Control-Request-Headers like the others. - EmitSSEResponseHeaders now copies AResp.CustomHeaders into the raw HTTP response line. The SSE handler bypasses Indy's response writer to fix the chunked-encoding bug; that bypass silently dropped any CORS headers EmitRelayCors stamped, so /v1/relay/poll's SSE response was missing them. Now passes through cleanly. - Auth-gate intact: GET /v1/relay/poll without a token still returns 401 + WWW-Authenticate. The CORS preflight doesn't require auth, but the subsequent actual request does. - docs/providers-relay.md: new "CORS" callout under the wire protocol section explaining the policy + why permissive is safe (token = auth boundary, no cookies). - webui browser snippet: comment updated to note CORS is handled server-side, no client setup required. Why permissive (Allow-Origin: *)? The relay endpoints are guarded by a bearer token (or the ?token= query fallback). Anyone reaching them already had to present credentials. Echoing the Origin header (vs hard-allowlisting) would require operators to configure a CORS allowlist, which buys nothing security-wise because the token IS the gate. Allow-Origin: * is safe here specifically because we don't use credentials=include -- cookies aren't involved, the token rides as a header or query param. Live smoke (port 18889, token gated): - OPTIONS /v1/relay/respond/req_x -> 204 + Allow-* headers - GET /v1/relay/poll w/ Origin -> 200 SSE + CORS headers - GET /v1/relay/status w/ Origin -> 200 JSON + CORS headers - POST /v1/relay/respond w/ Origin -> 200 + CORS headers - GET /v1/relay/poll w/o token -> 401 + WWW-Authenticate This branch (claude/webui-relay-fixes-v2) replaces the PR FMXExpress#326 stack: rebased the two FMXExpress#324 commits + the FMXExpress#325 tool-metadata fix + this CORS commit straight onto current main. Closes the merge conflict on PR FMXExpress#326 (stale base after PR FMXExpress#323 merged) and addresses the P1 tool-metadata review by carrying FMXExpress#325's fix into this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TBcLtmpj7dqA5tyFbGnQon
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 review fixes against PR #323. Stacked on
claude/cmd-relay-workerso the diff shows only the delta; auto-rebases to main once #323 merges.P1 — tool-call metadata must round-trip
A multi-turn relayed session that fires a tool call had its assistant
ToolCallsand tool-resultToolCallId/Namesilently dropped — both ends of the wire were stripping them. The next turn forwarded a request with notool_call_id, and OpenAI 400'd with "missing tool_call_id" (Anthropic / Gemini equivalent on their side). Fixed at both ends:BuildRelayRequestBody(gateway side,PasClaw.Providers.Relay.pas): emitstool_callsarray (id / type / function.name / function.arguments), top-leveltool_call_id, and top-levelnamewhen non-empty.DecodeMessages(worker side,PasClaw.Cmd.Relay.pas): reads them back into the correspondingTMessagefields, with a newDecodeToolCallshelper that mirrors the existing response-side decoder inHandleRelayRespond.P2 — surface upstream non-2xx as a relay error
BuildResponseJSONonly encodederrorwhenStatusCode = -1(pre-HTTP socket failure). A 429 / 5xx from upstream OpenAI/Anthropic/etc. fell into the success branch, the gateway saw no error field, mapped toStatusCode := 200, and the agent saw the provider's error JSON as the assistant's reply — bypassingCfg.Fallbacksentirely. NewResponseIsErrorhelper treats-1and any code outside [200..299] as an error;StatusCode = 0(older providers that don't populate the code) still treats as success for back-compat. RenamedBuildResponseJSON→BuildRelayWorkerResponseJSONand exposed it in the interface so the new test can drive it.Test plan
Both fixes covered by new cases in
src/tests/relay_queue_tests.pas:tool-call metadata round-trips through the envelope— assistant withToolCalls, tool result withToolCallId+Name; assertstool_callsarray,id, functionname+arguments,tool_call_id,role:"tool"all present.worker encodes non-2xx upstream status as a relay error— coversStatusCode = 429,500,-1(all →errorkey with descriptive prefix), and200/0(both → noerrorkey, success path).make test-relay-queue— 20/20 cases green.make allclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01TBcLtmpj7dqA5tyFbGnQon
Generated by Claude Code