fix: move the swe-bench-pro seed agent to chat completions - #67
Open
shehabyasser-scale wants to merge 1 commit into
Open
fix: move the swe-bench-pro seed agent to chat completions#67shehabyasser-scale wants to merge 1 commit into
shehabyasser-scale wants to merge 1 commit into
Conversation
## Why Varun asked on #64 whether this is a gateway-sidecar bug we should fix there instead, and whether chat completions would be simpler. It is not a sidecar bug, and yes. The gateway is a deliberate provider-agnostic passthrough (`inference.py` "forward any inference endpoint ... to the upstream litellm proxy, which decides what it supports"). `previous_response_id` appears exactly once in vero, in `_RequestAttributor`, purely to stamp a `thread_id` on log records; that class is documented as unable to affect proxying. So the field is forwarded untouched to a proxy fronting `fireworks_ai/deepseek-v4-flash`, and Fireworks has no Responses API response store. We could give the sidecar one, but it would not help: to honour the pointer we would reconstruct the transcript and send it upstream in full anyway, because the provider cannot resolve it. Identical bytes on the wire, identical token cost. The only thing that changes is that the gateway becomes stateful, so a store bug corrupts conversations across every trial at once instead of in one agent. Chat completions is the better answer, and for a stronger reason than style: the only thing the Responses API gives us over it for these models is reasoning passthrough across turns, and we already discard it. The agent asks for `reasoning: {"effort": "high"}` every turn and then rebuilds a conversation of assistant text plus `function_call`/`function_call_output` pairs, with no reasoning items. We were paying for statefulness and getting a stateless conversation regardless. Chat completions has no field a proxy can accept and ignore, so the failure mode stops existing rather than being worked around. ## What changed - `_TOOL_SCHEMAS` holds the six schemas verbatim; `TOOLS` wraps them in the nested `{"type": "function", "function": {...}}` shape chat completions wants. - `_responses_create` -> `_completion_create` on `chat.completions`. Retry and backoff are untouched. - `instructions=` becomes a system message, `input` becomes `messages`, `max_output_tokens` becomes `max_tokens`, `reasoning: {effort}` becomes `reasoning_effort`. Usage reads prompt/completion tokens. - A turn is now ONE assistant message carrying both its text and its `tool_calls`, followed by `role: "tool"` results. Chat completions rejects a tool result whose `tool_call_id` was not declared by the assistant message before it, so unlike Responses-API items these cannot be split. The existing whole-turn trimming invariant already guaranteed this and now protects both directions. The gateway keeps forwarding the field, because rejecting it would be wrong: a gateway fronting real OpenAI honours it, and `_RequestAttributor` threads those chains. It now warns once per process instead, so a delegated conversation is one grep away rather than invisible. Non-null only, since SDKs serialise the key as null when unused. ## Tests Seed agent 5 -> 7 tests: the resend and whole-turn-trim tests are rewritten for the chat shape and assert the call/result pairing that chat completions enforces, plus new coverage for the nested tool schema and for `reasoning_effort` reaching only reasoning models (`deepseek-v4-flash` matches none, so target behaviour is unchanged). One new gateway test covers warn-once and that proxying is untouched. Seed agent 7 passed, `test_v05_harbor_inference` 18 passed, full vero suite 456 passed. The 2 failures there are `test_v05_docker_sandbox` and `test_v05_harbor_isolation_container`, both from a corrupted local Docker content store ("blob ... input/output error" on every `docker run`), not from this change. Not yet measured end to end: unit tests pin the wire shape, but only a real run confirms `deepseek-v4-flash` behaves the same through chat completions. Blocked on the same Docker corruption. Worth an A/B on the 66-case sample before this is trusted for headline numbers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Follow-up to #64, answering @varunursekar's question there. Stacked on
fix/swebp-agent-context, so the diff shown is only the migration. Re-target tomainwithgh pr edit --base mainif we would rather supersede #64 than stack.Is it a sidecar bug? No
The gateway is a deliberate provider-agnostic passthrough:
previous_response_idappears exactly once in vero, in_RequestAttributor,purely to stamp a
thread_idon log records. That class is documented as"best-effort by construction ... it can never affect proxying". The field is
forwarded untouched to a proxy fronting
fireworks_ai/deepseek-v4-flash, andFireworks has no Responses API response store.
We could give the sidecar one. It would not help. To honour the pointer we would
reconstruct the transcript and send it upstream in full anyway, because the
provider cannot resolve it: identical bytes on the wire, identical token cost.
The only thing that changes is that the gateway becomes stateful, so a store bug
corrupts conversations across every trial at once instead of in one agent.
Why chat completions, beyond style
The only thing the Responses API gives us over chat completions for these models
is reasoning passthrough across turns, and we already throw it away. The agent
asks for
reasoning: {"effort": "high"}on every turn, then rebuilds aconversation of assistant text plus
function_call/function_call_outputpairswith no reasoning items in it. We were paying for statefulness and getting a
stateless conversation regardless.
Chat completions has no field a proxy can accept and silently ignore, so the
failure mode stops existing rather than being worked around.
What changed
_TOOL_SCHEMASholds the six schemas verbatim;TOOLSwraps them in thenested
{"type": "function", "function": {...}}shape._responses_create->_completion_createonchat.completions. Retry andbackoff untouched.
instructions=becomes a system message,inputbecomesmessages,max_output_tokensbecomesmax_tokens,reasoning: {effort}becomesreasoning_effort; usage reads prompt/completion tokens.tool_calls, followed byrole: "tool"results. Chat completions rejects atool result whose
tool_call_idwas not declared by the assistant messagebefore it, so unlike Responses-API items these cannot be split. The existing
whole-turn trimming invariant already guaranteed this and now protects both
directions.
The gateway keeps forwarding the field, because rejecting it would be wrong: a
gateway fronting real OpenAI honours it, and
_RequestAttributorthreads thosechains. It now warns once per process instead, so a delegated conversation is one
grep away rather than invisible. Non-null only, since SDKs serialise the key as
null when unused. Verified against the last grid run: of 65,262 logged requests,
zero request bodies ever carried the field.
Tests
test_v05_harbor_inferenceNew coverage: the nested tool schema, and
reasoning_effortreaching onlyreasoning models (
deepseek-v4-flashmatches none, so target behaviour isunchanged). The resend and whole-turn-trim tests are rewritten for the chat shape
and now assert the call/result pairing chat completions enforces.
The 2 suite failures are
test_v05_docker_sandboxandtest_v05_harbor_isolation_container, both from a corrupted local Docker contentstore (
blob sha256:... input/output erroron everydocker run), not thischange.
Not yet measured
Unit tests pin the wire shape; only a real run confirms
deepseek-v4-flashbehaves the same through chat completions. Blocked on the same Docker corruption.
Worth an A/B on the 66-case sample before this is trusted for headline
numbers.
🤖 Generated with Claude Code
Greptile Summary
This PR migrates the swe-bench-pro seed agent from the OpenAI Responses API to Chat Completions, eliminating the
previous_response_iddelegation bug that caused 0.0000 scores on all 66 sampled cases. The gateway gains a one-shot process-level warning (not a rejection) whenprevious_response_idis still forwarded, so future regressions are onegrepaway rather than invisible.agent.py): tool schemas are split into_TOOL_SCHEMAS(flat, Responses-API shape) andTOOLS(nested{"type":"function","function":{...}}for Chat Completions);instructions/input/max_output_tokens/reasoning.effortbecome system message/messages/max_tokens/reasoning_effort; usage fields map toprompt_tokens/completion_tokens; each turn is one assistant message carrying both text andtool_callsfollowed byrole:"tool"results.inference.py): a closure-captured set latch (_stateful_warned) fires alogger.warningonce per app instance when a request body carries a non-nullprevious_response_id, without touching proxying behaviour.test_agent.py,test_v05_harbor_inference.py): helpers refactored for Chat Completions shape; two new test cases cover the nested tool schema andreasoning_effortgating; the resend/trim tests are rewritten to assert the call/resulttool_call_idpairing Chat Completions enforces.Confidence Score: 5/5
The migration is straightforward and well-guarded: the
if not calls: breakpath prevents emptytool_callsarrays from ever entering history, the call/result pairing invariant is preserved, and the gateway change is purely additive (warn-only, no proxying change).All Chat Completions field mappings are correct (messages, max_tokens, reasoning_effort, prompt_tokens, tool_calls shape). The early-break on empty calls ensures the assistant message always carries at least one tool call when written to history. The gateway latch is safe under asyncio's single-threaded model. Test coverage is expanded rather than reduced, covering the new wire shape, tool-schema nesting, reasoning_effort gating, and call/result pairing.
Files Needing Attention: No files require special attention. The only caveat is the live A/B validation against deepseek-v4-flash that the author flags as blocked on Docker corruption.
Important Files Changed
if not calls: breakguard prevents emptytool_callsarrays from ever reaching the history.previous_response_id; uses a closure-captured set to avoidnonlocal, fires only on non-null values, leaves proxying untouched.previous_response_id, stays quiet for null, and leaves all three requests proxied successfully.Sequence Diagram
sequenceDiagram participant Agent as SweBenchProAgent participant CC as chat.completions participant GW as VeRO Gateway participant LM as litellm/upstream Note over Agent: Turn 1 Agent->>CC: "create(messages=[system,user], tools=TOOLS)" CC->>GW: POST chat/completions GW->>LM: forward LM-->>GW: choices[0].message GW-->>CC: response CC-->>Agent: message with tool_calls Agent->>Agent: append turn_items to blocks Note over Agent: Turn 2 Agent->>CC: "create(messages=[system,user,block1,...])" CC->>GW: POST with full conversation GW->>LM: forward LM-->>GW: submit call GW-->>CC: response CC-->>Agent: submit - break Note over GW: previous_response_id guard GW->>GW: if non-null and not warned - log warning onceReviews (1): Last reviewed commit: "fix: move the swe-bench-pro seed agent t..." | Re-trigger Greptile