fix: prevent malformed OpenAI Responses continuations - #24725
Conversation
… continuity 400s > Worked on by Mux on Mike's behalf.
Bumps coder/fantasy to 5afd6320c353, which: - skips provider-executed web_search item references during manual replay so OpenAI does not see a ws_* without its required rs_*. - validates that every local function_call has a matching function_call_output before returning the Responses input. > Worked on by Mux on Mike's behalf.
> Worked on by Mux on Mike's behalf.
> Worked on by Mux on Mike's behalf.
> Worked on by Mux on Mike's behalf.
|
/coder-agents-review |
There was a problem hiding this comment.
Well-structured three-layer defense against malformed OpenAI Responses API continuations. The chain mode guard (assistantHasUnresolvedLocalToolCalls + shouldActivateChainMode) is correctly conservative, the error classifier sanitizes sensitive IDs, and the test infrastructure validation catches real prompt shape errors. Clean commit structure, thorough integration coverage across four distinct scenarios, and the assertNoLeak helper is a nice touch for preventing ID leakage.
5 P3, 6 Nit. No blockers.
The main theme: ValidateResponsesAPIInput is a genuinely useful validator, but only one of four integration tests wires it in. The chain-mode-blocked test is the one most likely to produce a structurally invalid prompt (synthetic tool results injected into a full replay), and it is the one without validation. The other tests (NoStaleWebSearchReplay, ChainModeStillFiresForProviderExecutedOnly) would also benefit, or better yet, wire the validator into the fake server's handleResponses so all Responses-path tests get it automatically.
"A genuine gem has no hidden flaws." (Bisky, inspecting the test suite)
coderd/x/chatd/chattest/openai.go:53
Nit [DEREM-11] Comment says // For responses API but after the UnmarshalJSON below, Prompt now holds items from either "prompt" (legacy) or "input" (Responses API). Suggest: // Prompt holds provider items from either "prompt" or "input" (Responses API). (Gon)
🤖
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
10 of 11 findings addressed in 52f567e. Thorough fixes across the board.
Review is blocked on one remaining item:
DEREM-11 (Nit): Prompt field comment at chattest/openai.go:53 still reads // For responses API after the UnmarshalJSON change that populates it from either "prompt" or "input". This was folded into the R1 review body because the line was outside the diff. No code change or author response. Please either update the comment or acknowledge this is intentionally skipped.
Once DEREM-11 is addressed or acknowledged, the full panel will review the new commit.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 11 R1 findings addressed cleanly. The fixes are thorough: ValidateResponsesAPIInput wired into the fake server for all tests, doc comments on key functions, exported shared helper, split prefix/stale-ID checks, specific diagnostic message. Four of six panel reviewers found no new issues. CI green.
1 P3, 1 P4, 1 Nit new. DEREM-16 (Netero) downgraded from P3 to Nit because the test catches drift on mismatch, so the duplication is survivable.
The remaining P3 is a test coverage gap on the parse-error safety fallback. The return true on line 3307 (and 3330) is the safety net that blocks chain mode when content is unparseable. It has doc comments explaining the rationale, but no test exercises the path. If someone flips it to return false, the suite stays green while the safety net disappears.
"If a future change flips the error return to false, the suite stays green while the safety net disappears." (Bisky)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 14 findings from R1 and R3 addressed. R4 delta is test-only (863dcc8): parse-error safety fallback tests, mixed call-type test, and assertDirectionalMessage replacing the duplicated constant. Netero clean across all 8 sections. No new findings from the panel or first pass.
This is a well-executed fix with defense-in-depth: chain mode guard prevents sending malformed continuations, injectMissingToolResults handles synthetic tool results on the full-replay path, and responsesAPIDiagnostic sanitizes the 400 if both layers miss. Test coverage is thorough across unit, prompt, and integration boundaries.
"I tried to build a case against this change and could not." (Pariston, R3)
🤖 This review was automatically generated with Coder Agents.
This updates the pin from the in-flight PR head to the merged commit on coder_2_33.
Summary
previous_response_idchain mode when the prior assistant response has unresolved local tool calls, so the next request can include paired tool outputs instead of sending an incomplete continuation.Tests
go test ./providers/openai -run 'TestResponsesToPrompt_(ReasoningWithStore|ReasoningWithWebSearchCombined|WebSearchRequiresReasoningReference|ReasoningWithFunctionCallCombined|WebSearchProviderExecutedToolResults)|TestPrepareParams_(SkipsProviderExecutedToolReferences|ValidatesFunctionCallOutputPairing)|TestValidateResponsesInput_WebSearchReferenceRequiresReasoning' -count=1go test ./providers/openai -count=1GOWORK=off go test ./coderd/x/chatd/chattest -run TestValidateResponsesAPIInput -count=1GOWORK=off go test ./coderd/x/chatd -run 'TestOpenAIResponses(NoStaleWebSearchReplay|FullReplayPairsReasoningAndWebSearch|ChainModeSkipsWhenLocalCallPending|ChainModeStillFiresForProviderExecutedOnly)$|TestResolveChainMode_' -count=1GOWORK=off go test ./coderd/x/chatd/chatprompt -run 'TestInjectMissingToolResults_' -count=1GOWORK=off go test ./coderd/x/chatd/chaterror -run TestClassify_OpenAIResponsesAPIDiagnostics -count=1GOWORK=off go test ./coderd/x/chatd/... -count=1git diff --checkgit commitpre-commit hook