feat(responses): preserve tool semantics across Chat conversions - #2116
feat(responses): preserve tool semantics across Chat conversions#2116Zacks-Zhang wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds Responses-specific tool models and conversion paths, including Responses-to-Chat adaptation, streamed tool-call restoration, persisted ChangesResponses interoperability
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ResponsesRequest
participant PersistentOutboundTransformer
participant RequestService
participant ResponsesHistory
participant ResponsesChatToolAdapter
participant ChatUpstream
participant ResponsesStreamRestorer
ResponsesRequest->>PersistentOutboundTransformer: TransformRequest
PersistentOutboundTransformer->>RequestService: LoadCompletedResponseExchange
RequestService-->>ResponsesHistory: scoped request and response bodies
ResponsesHistory->>ResponsesChatToolAdapter: hydrated message history
ResponsesChatToolAdapter->>ChatUpstream: converted tools and messages
ChatUpstream-->>ResponsesStreamRestorer: streamed Chat tool-call chunks
ResponsesStreamRestorer-->>ResponsesRequest: restored Responses events
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryAdds scoped Confidence Score: 5/5The PR appears safe to merge. The previously reported unbounded history expansion is now constrained by cumulative byte and depth limits, and oversized stored bodies are rejected through bounded storage reads or database-side filtering before full materialization; no blocking failure remains.
|
| Filename | Overview |
|---|---|
| internal/server/orchestrator/responses_history.go | Expands scoped previous-response chains into Chat history while enforcing cycle, depth, and cumulative byte limits. |
| internal/server/biz/request.go | Loads completed stored exchanges within a caller-provided budget and applies database-side size filtering before body materialization. |
| internal/server/biz/data_storage.go | Adds bounded reads across database, object-store, and filesystem-backed storage, including advertised-size and observed-size enforcement. |
| llm/transformer/openai/responses/outbound_convert.go | Extends Responses-to-Chat conversion to retain tool identities, ownership, arguments, and lifecycle semantics. |
| llm/transformer/openai/responses/inbound_stream.go | Restores streaming Responses tool events and finish-state conversion from Chat-compatible upstream output. |
Sequence Diagram
sequenceDiagram
participant Client
participant Orchestrator
participant RequestService
participant Storage
participant ChatUpstream
Client->>Orchestrator: Responses request with previous_response_id
loop Up to 1024 responses and 32 MiB
Orchestrator->>RequestService: Load exchange with remaining byte budget
RequestService->>Storage: Bounded request/response body read
Storage-->>RequestService: Stored exchange
RequestService-->>Orchestrator: Scoped exchange
Orchestrator->>Orchestrator: Convert exchange into Chat history
end
Orchestrator->>ChatUpstream: Current request plus expanded history
ChatUpstream-->>Client: Converted Responses result
Reviews (6): Last reviewed commit: "feat(responses): 合并连续工具调用消息" | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
llm/transformer/openai/responses/request_extensions.go (1)
268-297: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMirror the guards from
mergeRawOnlyToolsin the input-item merge.
mergeRawOnlyToolsrejects negativeRepresentedToolCount(Line 323) and re-checksstructuredIndex > len(structuredTools)after advancing (Line 347). The new input path does neither, so a negative count from a persisted/round-trippedRawInputItemsfragment inflatestotal, drivesstructuredIndexnegative, andstructuredItems[structuredIndex]panics.🛡️ Proposed fix
representedCount := 0 for _, fragment := range requestExt.RawInputItems { + if fragment.RepresentedToolCount < 0 { + return nil, false + } representedCount += fragment.RepresentedToolCount } @@ if fragment, ok := rawByIndex[i]; ok { items = append(items, cloneRaw(fragment.Raw)) structuredIndex += fragment.RepresentedToolCount + if structuredIndex > len(structuredItems) { + return nil, false + } continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/responses/request_extensions.go` around lines 268 - 297, Update the input-item merge logic around RawInputItems to reject fragments with negative RepresentedToolCount, matching mergeRawOnlyTools. After advancing structuredIndex for a raw fragment, validate it is not greater than len(structuredItems), and return nil, false when invalid before any indexing occurs.llm/transformer/openai/responses/inbound_stream.go (1)
1109-1121: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClosing tool-call items iterates a map, so parallel-call event order is nondeterministic.
for idx, tc := range s.toolCallshas randomized iteration order in Go. When two or more tool-call items are still open at finish (the new parallel case captured intestdata/tool-2.stream.jsonlLines 142-145), thefunction_call_arguments.done/output_item.donepairs can be emitted foroutput_index3 before 2. That both breaks the strict event-by-event golden comparison ininbound_stream_test.go(Lines 100-111) and produces out-of-order SSE for clients that rely on ascendingoutput_index. Sorting the indexes makes it deterministic.🔒 Proposed deterministic ordering
- // Close any open tool call items - for idx, tc := range s.toolCalls { + // Close any open tool call items in ascending index order so parallel + // calls emit terminal events deterministically. + indexes := make([]int, 0, len(s.toolCalls)) + for idx := range s.toolCalls { + indexes = append(indexes, idx) + } + sort.Ints(indexes) + + for _, idx := range indexes { + tc := s.toolCalls[idx] if !s.toolCallItemStarted[idx] { continue }Requires importing
sort.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/responses/inbound_stream.go` around lines 1109 - 1121, Update the tool-call closing loop around s.toolCalls to collect and sort its indexes in ascending order before emitting function_call_arguments.done and output_item.done events. Preserve the existing filtering and item-ID fallback logic, and add the required sort import so parallel tool calls consistently close by ascending output_index.
🧹 Nitpick comments (6)
internal/server/orchestrator/outbound.go (1)
387-396: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrevious-response history is re-fetched from storage on every candidate retry.
llmRequest.PreviousResponseIDon the original request is untouched (only the hydrated clone has it cleared), so if this candidate fails and the orchestrator retries with another Chat-format candidate,hydratePreviousResponsesForChatre-walks the fullprevious_response_idchain (DB queries + external storage reads, up tomaxPreviousResponseChainDepth) again for identical data. Consider caching the hydrated messages/request onp.stateafter the first successful hydration and reusing it on subsequent attempts for the same originalPreviousResponseID.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/orchestrator/outbound.go` around lines 387 - 396, The retry path around hydratePreviousResponsesForChat should cache the successfully hydrated request or messages in p.state, keyed by the original llmRequest.PreviousResponseID, and reuse that cached result for subsequent Chat-format candidate attempts. Ensure the cache is populated only after successful hydration and preserves the existing error behavior and request transformation.llm/transformer/openai/responses/inbound.go (1)
875-919: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated function-like-tool conversion.
The "namespace" sub-tool branch (function/generic-client-like path) and the "default" case's generic-client-like path both marshal
tool.Parametersand build an almost identicalllm.Tool{Type:"function", Function: llm.Function{...}}literal, differing only inNamespace/ResponsesOriginhandling. A small helper (e.g.buildFunctionLikeTool(tool Tool, namespace string, forceOrigin bool) (llm.Tool, error)) would remove the duplication and reduce the size of this already-large switch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/responses/inbound.go` around lines 875 - 919, Extract the duplicated function-like tool conversion from the namespace sub-tool branch and the default generic-client-like branch into a shared helper near the surrounding conversion logic. Have the helper marshal parameters and build the common llm.Tool structure while accepting namespace and ResponsesOrigin behavior as inputs; update both callers to use it and preserve their current origin semantics and error context.llm/transformer/openai/outbound_convert.go (1)
81-117: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNil request returns
(nil, nil, nil).The only current caller (
OutboundTransformer.TransformRequest) rejects nil earlier, so this is latent, but a future caller gets a nil*Requestand nil*responsesChatToolAdapterwith no error and will panic onoaiReq.ReasoningEffort/toolAdapter.mappings(). Returning an error here is cheap insurance.♻️ Suggested change
if r == nil { - return nil, nil, nil + return nil, nil, fmt.Errorf("chat completion request is nil") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/outbound_convert.go` around lines 81 - 117, Update requestFromLLMWithResponsesToolAdapter to return a non-nil error when the input request r is nil, rather than returning nil request, nil adapter, and nil error. Preserve the existing conversion flow for non-nil requests so callers cannot proceed with missing Request or responsesChatToolAdapter values.llm/transformer/openai/responses_chat_tools.go (1)
472-488: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSignature omits
ParametersJsonSchema.Two same-named function tools differing only in
Function.ParametersJsonSchema(seellm/tools.go:54-64) hash to the same signature, so the second is treated as a duplicate and dropped without a conflict warning. Worth including the field if that mutually-exclusive variant can reach this path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/responses_chat_tools.go` around lines 472 - 488, The functionDefinitionSignature function omits Function.ParametersJsonSchema when building the normalized signature. Include this mutually exclusive field in the marshaled signature so tools differing by ParametersJsonSchema produce distinct signatures, while preserving the existing Parameters handling and duplicate detection flow.llm/transformer/openai/responses/inbound_stream.go (1)
897-917: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer
toolCallItemID[toolCallIndex]for the delta'sitem_id.The fallback to
s.currentItemIDresolves to whichever item was opened last, so with parallel tool calls a delta for an earlier index can be attributed to the wrong item whenstate.IDis still empty.closeCurrentOutputItem(Line 1114) already prefers the newtoolCallItemIDmap; using it here keepsadded/delta/doneitem IDs consistent.♻️ Proposed change
- itemID := s.toolCalls[toolCallIndex].ID + itemID := s.toolCallItemID[toolCallIndex] + if itemID == "" { + itemID = s.toolCalls[toolCallIndex].ID + } if itemID == "" { itemID = s.currentItemID }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/responses/inbound_stream.go` around lines 897 - 917, Update handleToolSearchCallDelta to resolve itemID from s.toolCallItemID[toolCallIndex] before falling back to s.currentItemID, matching closeCurrentOutputItem’s lookup and keeping tool-search added, delta, and done events consistent for parallel calls.llm/transformer/openai/responses_chat_integration_test.go (1)
716-846: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree stream helpers duplicate the same pipeline.
simulateResponsesChatStream,simulateResponsesChatCustomChoices, andsimulateResponsesChatCustomStreamall repeat inbound transform → chat outbound transform →TransformStream→ event collection, differing only in the request body and how provider chunks are built. Extracting a single core (request body +[]map[string]anychoices) and letting the custom variants build choices on top would cut ~80 lines and keep future protocol changes in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/responses_chat_integration_test.go` around lines 716 - 846, Extract the shared inbound/outbound transformation and response-event collection from simulateResponsesChatStream, simulateResponsesChatCustomChoices, and simulateResponsesChatCustomStream into one helper accepting the request body and provider choices. Update each existing helper to construct only its request-specific body and choices, then delegate to the shared helper while preserving custom tool-name resolution and terminal events.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/server/biz/request.go`:
- Around line 51-81: Update loadStoredResponseExchangeBody to return an
invalid_request_error when databaseBody is nil for a missing stored request
body, matching the existing os.ErrNotExist behavior for external storage;
preserve the empty JSON response for absent response bodies by distinguishing
bodyName before returning.
In `@llm/tools.go`:
- Around line 44-46: The CacheControl documentation in Tool contradicts its
json:"cache_control,omitempty" tag. Either change the tag to json:"-" to enforce
non-serialization, or remove/correct the non-serialization comment so it
accurately describes the existing conditional JSON serialization, matching
ToolCall.CacheControl behavior.
In `@llm/transformer/openai/responses_chat_tools.go`:
- Around line 700-732: Update restoreResponsesChatMessage’s custom-tool unwrap
logic so it only replaces the raw arguments when JSON unmarshalling succeeds and
the input field is present; otherwise retain the original
call.Function.Arguments and emit the same missing-input warning used by the
streaming path. Preserve the existing wrapped-input behavior for valid objects
containing input.
In `@llm/transformer/openai/responses/inbound_stream.go`:
- Around line 320-349: Update enqueueTerminalResponse to handle the
"content_filter" finish reason as an incomplete response, selecting
StreamEventTypeResponseIncomplete and setting IncompleteDetails.Reason to
"content_filter". Preserve the existing "length" mapping to "max_output_tokens"
and the default completed behavior for other finish reasons.
- Around line 863-875: Update handleCustomToolCallDelta and
handleToolSearchCallDelta to ensure state.ResponseCustomToolCall and
state.ResponseToolSearchCall are initialized before appending delta input.
Lazily create each missing sub-struct for its corresponding tool-call state,
then preserve the existing accumulation and metadata handling.
---
Outside diff comments:
In `@llm/transformer/openai/responses/inbound_stream.go`:
- Around line 1109-1121: Update the tool-call closing loop around s.toolCalls to
collect and sort its indexes in ascending order before emitting
function_call_arguments.done and output_item.done events. Preserve the existing
filtering and item-ID fallback logic, and add the required sort import so
parallel tool calls consistently close by ascending output_index.
In `@llm/transformer/openai/responses/request_extensions.go`:
- Around line 268-297: Update the input-item merge logic around RawInputItems to
reject fragments with negative RepresentedToolCount, matching mergeRawOnlyTools.
After advancing structuredIndex for a raw fragment, validate it is not greater
than len(structuredItems), and return nil, false when invalid before any
indexing occurs.
---
Nitpick comments:
In `@internal/server/orchestrator/outbound.go`:
- Around line 387-396: The retry path around hydratePreviousResponsesForChat
should cache the successfully hydrated request or messages in p.state, keyed by
the original llmRequest.PreviousResponseID, and reuse that cached result for
subsequent Chat-format candidate attempts. Ensure the cache is populated only
after successful hydration and preserves the existing error behavior and request
transformation.
In `@llm/transformer/openai/outbound_convert.go`:
- Around line 81-117: Update requestFromLLMWithResponsesToolAdapter to return a
non-nil error when the input request r is nil, rather than returning nil
request, nil adapter, and nil error. Preserve the existing conversion flow for
non-nil requests so callers cannot proceed with missing Request or
responsesChatToolAdapter values.
In `@llm/transformer/openai/responses_chat_integration_test.go`:
- Around line 716-846: Extract the shared inbound/outbound transformation and
response-event collection from simulateResponsesChatStream,
simulateResponsesChatCustomChoices, and simulateResponsesChatCustomStream into
one helper accepting the request body and provider choices. Update each existing
helper to construct only its request-specific body and choices, then delegate to
the shared helper while preserving custom tool-name resolution and terminal
events.
In `@llm/transformer/openai/responses_chat_tools.go`:
- Around line 472-488: The functionDefinitionSignature function omits
Function.ParametersJsonSchema when building the normalized signature. Include
this mutually exclusive field in the marshaled signature so tools differing by
ParametersJsonSchema produce distinct signatures, while preserving the existing
Parameters handling and duplicate detection flow.
In `@llm/transformer/openai/responses/inbound_stream.go`:
- Around line 897-917: Update handleToolSearchCallDelta to resolve itemID from
s.toolCallItemID[toolCallIndex] before falling back to s.currentItemID, matching
closeCurrentOutputItem’s lookup and keeping tool-search added, delta, and done
events consistent for parallel calls.
In `@llm/transformer/openai/responses/inbound.go`:
- Around line 875-919: Extract the duplicated function-like tool conversion from
the namespace sub-tool branch and the default generic-client-like branch into a
shared helper near the surrounding conversion logic. Have the helper marshal
parameters and build the common llm.Tool structure while accepting namespace and
ResponsesOrigin behavior as inputs; update both callers to use it and preserve
their current origin semantics and error context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d7a87d4-e1ee-45e8-a847-90ab54a3534f
📒 Files selected for processing (29)
docs/en/api-reference/openai-api.mddocs/zh/api-reference/openai-api.mdinternal/ent/internal/schema.gointernal/ent/migrate/schema.gointernal/ent/schema/request.gointernal/server/biz/request.gointernal/server/orchestrator/outbound.gointernal/server/orchestrator/outbound_test.gointernal/server/orchestrator/responses_history.gointernal/server/orchestrator/responses_history_test.gollm/constants.gollm/tools.gollm/transformer/openai/outbound.gollm/transformer/openai/outbound_convert.gollm/transformer/openai/outbound_convert_test.gollm/transformer/openai/outbound_test.gollm/transformer/openai/responses/inbound.gollm/transformer/openai/responses/inbound_stream.gollm/transformer/openai/responses/inbound_stream_test.gollm/transformer/openai/responses/inbound_test.gollm/transformer/openai/responses/model.gollm/transformer/openai/responses/outbound.gollm/transformer/openai/responses/outbound_convert.gollm/transformer/openai/responses/outbound_convert_test.gollm/transformer/openai/responses/outbound_test.gollm/transformer/openai/responses/request_extensions.gollm/transformer/openai/responses/testdata/tool-2.stream.jsonlllm/transformer/openai/responses_chat_integration_test.gollm/transformer/openai/responses_chat_tools.go
1b19d1e to
e3ecfa4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/server/orchestrator/responses_history.go (1)
98-173: 🚀 Performance & Scalability | 🔵 TrivialDeep
previous_response_idchains cause many sequential blocking round-trips.Each hop does one storage fetch (
LoadCompletedResponseExchange) plus two fullTransformRequestJSON parses. WithmaxPreviousResponseChainDepth = 1024, a worst-case (or adversarial) chain triggers up to 1024 sequential DB round-trips and ~2048 transform passes synchronously on the request path before the Chat outbound call is even made.Consider adding chain-depth metrics/logging (e.g., warn when depth exceeds some threshold) so pathological chains are observable, and/or evaluate lowering the default depth cap or batch-loading exchanges to reduce request-thread latency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/orchestrator/responses_history.go` around lines 98 - 173, Add observability to the previous_response_id traversal loop by recording chain depth and emitting a warning or metric when it crosses a defined pathological-chain threshold, while retaining the existing maxPreviousResponseChainDepth validation and error behavior. Use the existing logging/metrics facilities and anchor the change around the depth loop and maxPreviousResponseChainDepth rather than altering response transformation semantics.llm/transformer/openai/responses/inbound_stream_test.go (1)
93-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a per-case assertion hook over matching on the fixture filename.
Adding an optional
extraAssert func(*testing.T, []StreamEvent)field to the table struct keeps the case-specific check next to its case and avoids string coupling tollm-tool-2.stream.jsonl.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/responses/inbound_stream_test.go` around lines 93 - 95, Update the table-driven test cases and execution loop in the inbound stream test to support an optional extraAssert func(*testing.T, []StreamEvent) hook. Move the parallel function-call lifecycle assertion into the relevant test case’s extraAssert field, invoke the hook when present, and remove the inputStreamFile filename comparison.llm/transformer/openai/responses_chat_integration_test.go (1)
716-846: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree near-identical stream simulation helpers could share one core.
simulateResponsesChatStream,simulateResponsesChatCustomChoices, andsimulateResponsesChatCustomStreamrepeat the same request-transform → provider-events → stream-collect pipeline. Extracting the common tail (build provider events from choices, transform, collect events) would leave each helper as a thin wrapper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/responses_chat_integration_test.go` around lines 716 - 846, Extract the duplicated request-transform, provider-event construction, stream transformation, and event-collection logic from simulateResponsesChatStream, simulateResponsesChatCustomChoices, and simulateResponsesChatCustomStream into one shared helper. Have the existing helpers only prepare their request-specific choices or fragments and delegate to the common pipeline, preserving custom tool-name setup and finish-event behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/server/orchestrator/responses_history.go`:
- Around line 107-118: Update loadPreviousResponsesHistory around
LoadCompletedResponseExchange to enforce the 32 MB limit for each persisted
exchange before its request and response bodies are fully loaded or decoded. Add
an early size check or per-exchange cap in the loader path, reject oversized
exchanges with transformer.ErrInvalidRequest, and preserve the existing
cumulative nextPreviousResponseHistoryBytes validation.
---
Nitpick comments:
In `@internal/server/orchestrator/responses_history.go`:
- Around line 98-173: Add observability to the previous_response_id traversal
loop by recording chain depth and emitting a warning or metric when it crosses a
defined pathological-chain threshold, while retaining the existing
maxPreviousResponseChainDepth validation and error behavior. Use the existing
logging/metrics facilities and anchor the change around the depth loop and
maxPreviousResponseChainDepth rather than altering response transformation
semantics.
In `@llm/transformer/openai/responses_chat_integration_test.go`:
- Around line 716-846: Extract the duplicated request-transform, provider-event
construction, stream transformation, and event-collection logic from
simulateResponsesChatStream, simulateResponsesChatCustomChoices, and
simulateResponsesChatCustomStream into one shared helper. Have the existing
helpers only prepare their request-specific choices or fragments and delegate to
the common pipeline, preserving custom tool-name setup and finish-event
behavior.
In `@llm/transformer/openai/responses/inbound_stream_test.go`:
- Around line 93-95: Update the table-driven test cases and execution loop in
the inbound stream test to support an optional extraAssert func(*testing.T,
[]StreamEvent) hook. Move the parallel function-call lifecycle assertion into
the relevant test case’s extraAssert field, invoke the hook when present, and
remove the inputStreamFile filename comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bdedbcea-b4b4-44c2-a8cf-5bd9ea1074f6
📒 Files selected for processing (29)
docs/en/api-reference/openai-api.mddocs/zh/api-reference/openai-api.mdinternal/ent/internal/schema.gointernal/ent/migrate/schema.gointernal/ent/schema/request.gointernal/server/biz/request.gointernal/server/orchestrator/outbound.gointernal/server/orchestrator/outbound_test.gointernal/server/orchestrator/responses_history.gointernal/server/orchestrator/responses_history_test.gollm/constants.gollm/tools.gollm/transformer/openai/outbound.gollm/transformer/openai/outbound_convert.gollm/transformer/openai/outbound_convert_test.gollm/transformer/openai/outbound_test.gollm/transformer/openai/responses/inbound.gollm/transformer/openai/responses/inbound_stream.gollm/transformer/openai/responses/inbound_stream_test.gollm/transformer/openai/responses/inbound_test.gollm/transformer/openai/responses/model.gollm/transformer/openai/responses/outbound.gollm/transformer/openai/responses/outbound_convert.gollm/transformer/openai/responses/outbound_convert_test.gollm/transformer/openai/responses/outbound_test.gollm/transformer/openai/responses/request_extensions.gollm/transformer/openai/responses/testdata/tool-2.stream.jsonlllm/transformer/openai/responses_chat_integration_test.gollm/transformer/openai/responses_chat_tools.go
🚧 Files skipped from review as they are similar to previous changes (5)
- llm/constants.go
- docs/en/api-reference/openai-api.md
- docs/zh/api-reference/openai-api.md
- llm/transformer/openai/responses/outbound_test.go
- llm/transformer/openai/responses/outbound_convert_test.go
- 保真转换 function、custom、namespace、tool_search 与 additional_tools - 恢复流式及非流式工具调用、错误和结束状态语义 - 展开 previous_response_id 历史并限制累计体积，增加模拟测试与文档
e3ecfa4 to
eb836b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
llm/transformer/openai/responses/request_extensions.go (1)
122-139: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign
representedRawToolCountwith namespace subtool conversion.
representedRawToolCountonly countssubTool.Type == "function"insidenamespacetools, whileconvertToolsToLLMalso emits structuredfunctiontools for generic client execution subtools (isGenericClientFunctionLike(subTool)). A namespace containing a non-"function"client-execution subtool will generate more structured tools than the raw fragment reports, so the merge loop may reject raw replay viastructuredIndex != len(structuredTools)or skip/duplicate structured tools.🐛 Proposed fix
func representedRawToolCount(tool Tool) int { if tool.Type == "tool_search" { return 1 } if tool.Type != "namespace" { return 0 } count := 0 for _, subTool := range tool.Tools { - if subTool.Type == "function" { + if subTool.Type == "function" || isGenericClientFunctionLike(subTool) { count++ } } return count }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/responses/request_extensions.go` around lines 122 - 139, Update representedRawToolCount to count every namespace subtool that convertToolsToLLM converts into a structured function tool, including subtools recognized by isGenericClientFunctionLike, rather than only subtools with Type "function". Keep tool_search and non-namespace handling unchanged so the raw count matches the structured output used by the merge loop.llm/transformer/openai/responses/outbound_convert.go (1)
657-724: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winHandle
tool_search_calloutput inconvertOutputToMessage.
TransformResponsepassesresp.OutputthroughconvertOutputToMessage, so anytool_search_callitem in the Responses output will pass the switch with no tool-call handling and be omitted from the assistantllm.ToolCalls. Add atool_search_callcase that creates aResponseToolSearchCall-backed tool call, or document/store the item for tool results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/responses/outbound_convert.go` around lines 657 - 724, Update convertOutputToMessage to handle outputItem.Type == "tool_search_call" instead of dropping it; append an llm.ToolCall backed by ResponseToolSearchCall and populate its available call ID, name, and input fields consistently with custom tool calls. Preserve existing handling for function_call and custom_tool_call items.llm/transformer/openai/responses/inbound_stream.go (2)
898-913: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDelta events don't use the new per-index
toolCallItemIDmap.initToolCallnow records the emitted item ID ins.toolCallItemID[idx]andcloseCurrentOutputItemprefers it, but both delta handlers still resolveitem_idfromtc.IDwiths.currentItemIDas fallback. With parallel tool calls,currentItemIDpoints at the most recently initialized item, so deltas for an earlier index without a call ID are attributed to the wrong item while its terminal events use the right one.
llm/transformer/openai/responses/inbound_stream.go#L898-L913: resolveitemIDfroms.toolCallItemID[toolCallIndex]first inhandleCustomToolCallDelta, keepingtc.ID/currentItemIDas fallbacks.llm/transformer/openai/responses/inbound_stream.go#L918-L938: apply the sametoolCallItemID-first resolution inhandleToolSearchCallDelta.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/responses/inbound_stream.go` around lines 898 - 913, The delta handlers resolve item IDs incorrectly for parallel tool calls. In llm/transformer/openai/responses/inbound_stream.go lines 898-913, update handleCustomToolCallDelta to prefer s.toolCallItemID[toolCallIndex], then fall back to tc.ID and s.currentItemID; apply the same resolution order in handleToolSearchCallDelta at lines 918-938.
1129-1141: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose tool-call items in deterministic index order.
initToolCallno longer closes the previous tool-call item, so multiple entries can be open here and this loop iteratess.toolCallsin Go's randomized map order. The emittedarguments.done/output_item.donepairs for parallel calls can therefore come out in a different order per run — nondeterministic for clients, and it makes the positional fixture comparison againsttestdata/tool-2.stream.jsonlflaky.🛠️ Proposed fix
- for idx, tc := range s.toolCalls { + indices := make([]int, 0, len(s.toolCalls)) + for idx := range s.toolCalls { + indices = append(indices, idx) + } + sort.Ints(indices) + + for _, idx := range indices { + tc := s.toolCalls[idx] if !s.toolCallItemStarted[idx] { continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/responses/inbound_stream.go` around lines 1129 - 1141, Update the tool-call closing loop in the stream finalization logic to process open entries in deterministic index order rather than ranging directly over the randomized s.toolCalls map. Collect and sort the tool-call indices, then preserve the existing itemID fallback and closing-event behavior for each sorted index.
🧹 Nitpick comments (3)
internal/server/orchestrator/responses_history_test.go (1)
246-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test allocates a full 32 MiB body on every run.
requestSizeis derived frommaxPreviousResponseHistoryBytes, so the suite allocates and JSON-unmarshals a ~32 MiB padded body just to exercise the cap. Making the limit an injectable packagevar(overridden witht.Cleanuprestore) would keep the same boundary coverage at negligible cost.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/orchestrator/responses_history_test.go` around lines 246 - 268, Make maxPreviousResponseHistoryBytes an injectable package-level var, then update TestLoadPreviousResponsesHistory_RejectsCumulativeHistoryOverByteLimit to temporarily override it with a small test value and restore the original via t.Cleanup. Preserve the existing boundary assertion and error expectations while avoiding allocation and unmarshalling of a full 32 MiB body.llm/transformer/openai/responses_chat_tools.go (1)
559-647: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePending tool-call fragments are dropped when a stream ends without a
finish_reason.Fragments buffered in
r.pendingare only flushed in thechoice.FinishReason != nilbranch. If an upstream stream terminates without a finish reason (truncated/aborted upstream), buffered names and argument fragments are silently discarded rather than surfaced as a partial call. A terminal flush hook (or emitting on stream close) would make the degradation observable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/responses_chat_tools.go` around lines 559 - 647, The restore flow only emits buffered entries from r.pending when choice.FinishReason is present, so terminal streams without a finish reason lose partial tool calls. Add a stream-close or terminal flush path around responsesChatToolStreamRestorer.restore that drains pending calls for the affected choices, applies the same mapping/plain classification, appends the partial calls to the response, and removes them from r.pending.llm/transformer/openai/responses_chat_integration_test.go (1)
754-884: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider collapsing the three near-identical stream simulators.
simulateResponsesChatStream,simulateResponsesChatCustomChoices, andsimulateResponsesChatCustomStreamrepeat the same pipeline (build Responses request → chat outbound → wrap provider chunks → collect Responses events). A single helper taking the request body plus a[]map[string]anychoice builder would let the other two become thin wrappers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llm/transformer/openai/responses_chat_integration_test.go` around lines 754 - 884, The three helpers duplicate the request transformation, provider stream construction, response transformation, and event collection pipeline. Consolidate that shared logic into one helper parameterized by the request body and generated choice chunks, then reduce simulateResponsesChatStream, simulateResponsesChatCustomChoices, and simulateResponsesChatCustomStream to thin wrappers that prepare their inputs while preserving their current request and chunk behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@llm/transformer/openai/responses/inbound.go`:
- Around line 887-947: Update representedRawToolCount to count namespace
subtools using the same promotion predicate as the conversion logic: include
subtools whose type is "function" or satisfy isGenericClientFunctionLike, not
only those with type "function". Keep fragment counts aligned with the
structured tools produced by the namespace conversion path.
---
Outside diff comments:
In `@llm/transformer/openai/responses/inbound_stream.go`:
- Around line 898-913: The delta handlers resolve item IDs incorrectly for
parallel tool calls. In llm/transformer/openai/responses/inbound_stream.go lines
898-913, update handleCustomToolCallDelta to prefer
s.toolCallItemID[toolCallIndex], then fall back to tc.ID and s.currentItemID;
apply the same resolution order in handleToolSearchCallDelta at lines 918-938.
- Around line 1129-1141: Update the tool-call closing loop in the stream
finalization logic to process open entries in deterministic index order rather
than ranging directly over the randomized s.toolCalls map. Collect and sort the
tool-call indices, then preserve the existing itemID fallback and closing-event
behavior for each sorted index.
In `@llm/transformer/openai/responses/outbound_convert.go`:
- Around line 657-724: Update convertOutputToMessage to handle outputItem.Type
== "tool_search_call" instead of dropping it; append an llm.ToolCall backed by
ResponseToolSearchCall and populate its available call ID, name, and input
fields consistently with custom tool calls. Preserve existing handling for
function_call and custom_tool_call items.
In `@llm/transformer/openai/responses/request_extensions.go`:
- Around line 122-139: Update representedRawToolCount to count every namespace
subtool that convertToolsToLLM converts into a structured function tool,
including subtools recognized by isGenericClientFunctionLike, rather than only
subtools with Type "function". Keep tool_search and non-namespace handling
unchanged so the raw count matches the structured output used by the merge loop.
---
Nitpick comments:
In `@internal/server/orchestrator/responses_history_test.go`:
- Around line 246-268: Make maxPreviousResponseHistoryBytes an injectable
package-level var, then update
TestLoadPreviousResponsesHistory_RejectsCumulativeHistoryOverByteLimit to
temporarily override it with a small test value and restore the original via
t.Cleanup. Preserve the existing boundary assertion and error expectations while
avoiding allocation and unmarshalling of a full 32 MiB body.
In `@llm/transformer/openai/responses_chat_integration_test.go`:
- Around line 754-884: The three helpers duplicate the request transformation,
provider stream construction, response transformation, and event collection
pipeline. Consolidate that shared logic into one helper parameterized by the
request body and generated choice chunks, then reduce
simulateResponsesChatStream, simulateResponsesChatCustomChoices, and
simulateResponsesChatCustomStream to thin wrappers that prepare their inputs
while preserving their current request and chunk behavior.
In `@llm/transformer/openai/responses_chat_tools.go`:
- Around line 559-647: The restore flow only emits buffered entries from
r.pending when choice.FinishReason is present, so terminal streams without a
finish reason lose partial tool calls. Add a stream-close or terminal flush path
around responsesChatToolStreamRestorer.restore that drains pending calls for the
affected choices, applies the same mapping/plain classification, appends the
partial calls to the response, and removes them from r.pending.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 288aa3bc-3d83-45d4-8cce-5f1a2b8ce6fe
📒 Files selected for processing (29)
docs/en/api-reference/openai-api.mddocs/zh/api-reference/openai-api.mdinternal/ent/internal/schema.gointernal/ent/migrate/schema.gointernal/ent/schema/request.gointernal/server/biz/request.gointernal/server/orchestrator/outbound.gointernal/server/orchestrator/outbound_test.gointernal/server/orchestrator/responses_history.gointernal/server/orchestrator/responses_history_test.gollm/constants.gollm/tools.gollm/transformer/openai/outbound.gollm/transformer/openai/outbound_convert.gollm/transformer/openai/outbound_convert_test.gollm/transformer/openai/outbound_test.gollm/transformer/openai/responses/inbound.gollm/transformer/openai/responses/inbound_stream.gollm/transformer/openai/responses/inbound_stream_test.gollm/transformer/openai/responses/inbound_test.gollm/transformer/openai/responses/model.gollm/transformer/openai/responses/outbound.gollm/transformer/openai/responses/outbound_convert.gollm/transformer/openai/responses/outbound_convert_test.gollm/transformer/openai/responses/outbound_test.gollm/transformer/openai/responses/request_extensions.gollm/transformer/openai/responses/testdata/tool-2.stream.jsonlllm/transformer/openai/responses_chat_integration_test.gollm/transformer/openai/responses_chat_tools.go
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/en/api-reference/openai-api.md
- internal/server/orchestrator/outbound_test.go
- docs/zh/api-reference/openai-api.md
- llm/transformer/openai/responses/outbound_convert_test.go
| converted := llm.Tool{ | ||
| Type: "function", | ||
| Function: llm.Function{ | ||
| Name: namespaceFunctionName(tool.Name, subTool.Name), | ||
| Namespace: tool.Name, | ||
| Description: subTool.Description, | ||
| Parameters: params, | ||
| Strict: subTool.Strict, | ||
| }, | ||
| }) | ||
| } | ||
| if subTool.Type != "function" { | ||
| converted.ResponsesOrigin = "raw_tool" | ||
| } | ||
| result = append(result, converted) | ||
| } | ||
|
|
||
| default: | ||
| // Skip unsupported tool types | ||
| continue | ||
| if isGenericClientFunctionLike(tool) { | ||
| params, err := json.Marshal(tool.Parameters) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal %s tool parameters: %w", tool.Type, err) | ||
| } | ||
| result = append(result, llm.Tool{ | ||
| Type: "function", | ||
| Function: llm.Function{ | ||
| Name: tool.Name, Description: tool.Description, | ||
| Parameters: params, Strict: tool.Strict, | ||
| }, | ||
| ResponsesOrigin: "raw_tool", | ||
| }) | ||
| continue | ||
| } | ||
| result = append(result, opaqueResponsesTool(tool, "raw_tool")) | ||
| } | ||
| } | ||
|
|
||
| return result, nil | ||
| } | ||
|
|
||
| // isGenericClientFunctionLike reports whether an unknown client tool can use function semantics. | ||
| func isGenericClientFunctionLike(tool Tool) bool { | ||
| return isFunctionLike(tool) && tool.Execution == "client" | ||
| } | ||
|
|
||
| // isFunctionLike reports whether a Responses declaration has a function schema. | ||
| func isFunctionLike(tool Tool) bool { | ||
| return tool.Name != "" && tool.Parameters != nil | ||
| } | ||
|
|
||
| // opaqueResponsesTool preserves an unsupported declaration for same-protocol replay. | ||
| func opaqueResponsesTool(tool Tool, origin string) llm.Tool { | ||
| return llm.Tool{ | ||
| Type: llm.ToolTypeResponsesOpaqueTool, | ||
| ResponseOpaqueTool: &llm.ResponseOpaqueTool{ | ||
| SourceType: tool.Type, Name: tool.Name, Execution: tool.Execution, | ||
| Description: tool.Description, | ||
| }, | ||
| ResponsesOrigin: origin, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Namespace/default tool promotion logic verified against tests — no issues found here.
isGenericClientFunctionLike/isFunctionLike/opaqueResponsesTool correctly gate promotion vs. opaque preservation and match all new test expectations (DoesNotInferUnknownNamespaceExecutionOwner, PreservesUnsupportedFutureToolForExplicitRejection, PromotesFutureClientFunctionLikeTools). Note: the predicate used here (subTool.Type == "function" || isGenericClientFunctionLike(subTool)) to decide which namespace subtools become structured function tools is not mirrored by representedRawToolCount in request_extensions.go, which only counts subTool.Type == "function" — see the corresponding comment on that file for the resulting fragment-count mismatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@llm/transformer/openai/responses/inbound.go` around lines 887 - 947, Update
representedRawToolCount to count namespace subtools using the same promotion
predicate as the conversion logic: include subtools whose type is "function" or
satisfy isGenericClientFunctionLike, not only those with type "function". Keep
fragment counts aligned with the structured tools produced by the namespace
conversion path.
- 保真转换 tool_search_call 流式及非流式生命周期 - 稳定并行工具调用标识与关闭顺序 - 限制历史响应读取内存并补充边界回归测试
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/server/biz/data_storage_limited_test.go (1)
111-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
repeatingReader.Readreturning(0, nil)at exhaustion violates theio.Readercontract.The current test only survives because
io.LimitReader(reader, 5)hits its own EOF at exactlyremaining. Any future tweak whereremaining < readLimitmakesio.ReadAllspin forever. Returnio.EOFinstead.♻️ Proposed fix
func (r *repeatingReader) Read(p []byte) (int, error) { if r.remaining == 0 { - return 0, nil + return 0, io.EOF }Add the
ioimport:import ( "errors" + "io" "path/filepath" "testing"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/biz/data_storage_limited_test.go` around lines 111 - 121, Update repeatingReader.Read so it returns 0, io.EOF when remaining is zero, adding the io import as needed; preserve the existing byte-generation and decrement behavior while data remains.internal/server/orchestrator/responses_history_test.go (1)
34-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord
maxBytesinpreviousResponseLoadCallto assert the shrinking budget.
loadPreviousResponsesHistorypassesmaxPreviousResponseHistoryBytes - historyByteson each hop, but the fake discards it, so the cumulative-budget tests can only observe the response IDs, not that the remaining budget actually decreases per link. Capturing it makes the accounting regression-proof.♻️ Proposed change
l.calls = append(l.calls, previousResponseLoadCall{ responseID: responseID, projectID: projectID, apiKeyID: apiKeyID, + maxBytes: maxBytes, })Extend the struct accordingly:
type previousResponseLoadCall struct { responseID string projectID int apiKeyID *int maxBytes int64 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/orchestrator/responses_history_test.go` around lines 34 - 54, Extend previousResponseLoadCall with a maxBytes int64 field, and update mapPreviousResponseLoader.LoadCompletedResponseExchange to record the received maxBytes when appending each call. Preserve the existing response, project, and API key tracking so cumulative-budget tests can assert the shrinking budget per hop.internal/server/biz/request.go (1)
1373-1402: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEmpty
bodyRowsconflates "oversized" with "row disappeared", and the two size expressions needn't escape the closure.The budget was already validated against
metadatajust above, so a zero-row result here almost certainly means the request row was updated or deleted between the two queries — yet it surfaces asErrStoredResponseExchangeTooLarge, which the orchestrator turns into a misleadingprevious_response_id history exceeds N bytes400. Consider re-checking existence (or returningnil, nilso the caller reports "not found"). AlsorequestSizeExpression/responseSizeExpressionare only read inside theModifyclosure, so they can be plain locals.♻️ Proposed cleanup
- requestSizeExpression := "" - responseSizeExpression := "" var bodyRows []struct { RequestBody objects.JSONRawMessage `json:"request_body"` ResponseBody objects.JSONRawMessage `json:"response_body"` } err = client.Request.Query(). Where(request.IDEQ(metadata.ID)). Limit(1). Select(request.FieldRequestBody, request.FieldResponseBody). Modify(func(selector *entsql.Selector) { - requestSizeExpression = storedResponseBodySizeExpression(dialectName, selector.C(request.FieldRequestBody)) - responseSizeExpression = storedResponseBodySizeExpression(dialectName, selector.C(request.FieldResponseBody)) + requestSizeExpression := storedResponseBodySizeExpression(dialectName, selector.C(request.FieldRequestBody)) + responseSizeExpression := storedResponseBodySizeExpression(dialectName, selector.C(request.FieldResponseBody)) selector.Where(entsql.P(func(builder *entsql.Builder) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/biz/request.go` around lines 1373 - 1402, Update the body lookup around requestSizeExpression, responseSizeExpression, and bodyRows so empty results are distinguished from an oversized stored exchange: re-check whether the request identified by metadata.ID still exists (or return nil, nil so the caller emits its not-found response), and only return ErrStoredResponseExchangeTooLarge when the row exists but fails the size predicate. Simplify requestSizeExpression and responseSizeExpression to closure-local values since they are only consumed inside Modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/server/biz/request.go`:
- Around line 62-75: Update storedResponseBodySizeExpression to measure the raw
stored bytes for Postgres and MySQL JSON columns, rather than casting them to
text or CHAR and measuring normalized JSON. Preserve the existing SQLite
behavior and ensure the returned dialect-specific expressions match the original
JSONRawMessage byte length used by budgeting.
In `@llm/transformer/openai/responses/inbound_stream_test.go`:
- Around line 233-250: Strengthen the assertions in the deltaEvents validation
around addedItemIDs so output indexes 0, 1, and 2 are each present exactly once
before comparing item IDs. Keep the existing item-ID checks, but count or
otherwise validate each event’s OutputIndex to reject cases where all deltas use
the same index.
---
Nitpick comments:
In `@internal/server/biz/data_storage_limited_test.go`:
- Around line 111-121: Update repeatingReader.Read so it returns 0, io.EOF when
remaining is zero, adding the io import as needed; preserve the existing
byte-generation and decrement behavior while data remains.
In `@internal/server/biz/request.go`:
- Around line 1373-1402: Update the body lookup around requestSizeExpression,
responseSizeExpression, and bodyRows so empty results are distinguished from an
oversized stored exchange: re-check whether the request identified by
metadata.ID still exists (or return nil, nil so the caller emits its not-found
response), and only return ErrStoredResponseExchangeTooLarge when the row exists
but fails the size predicate. Simplify requestSizeExpression and
responseSizeExpression to closure-local values since they are only consumed
inside Modify.
In `@internal/server/orchestrator/responses_history_test.go`:
- Around line 34-54: Extend previousResponseLoadCall with a maxBytes int64
field, and update mapPreviousResponseLoader.LoadCompletedResponseExchange to
record the received maxBytes when appending each call. Preserve the existing
response, project, and API key tracking so cumulative-budget tests can assert
the shrinking budget per hop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d9685865-319f-479d-8d86-86b7146cb703
📒 Files selected for processing (13)
internal/server/biz/data_storage.gointernal/server/biz/data_storage_limited_test.gointernal/server/biz/request.gointernal/server/orchestrator/responses_history.gointernal/server/orchestrator/responses_history_test.gollm/transformer/openai/responses/inbound_stream.gollm/transformer/openai/responses/inbound_stream_test.gollm/transformer/openai/responses/outbound_convert.gollm/transformer/openai/responses/outbound_convert_test.gollm/transformer/openai/responses/outbound_stream.gollm/transformer/openai/responses/outbound_stream_test.gollm/transformer/openai/responses/outbound_test.gollm/transformer/openai/responses/request_extensions_roundtrip_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/server/orchestrator/responses_history.go
- llm/transformer/openai/responses/outbound_test.go
- llm/transformer/openai/responses/inbound_stream.go
- 按数据库持久化后的 JSON 文本字节验证历史读取限额 - 明确 PostgreSQL 与 MySQL 的 Scan 字节预算语义 - 覆盖并行工具增量的完整输出索引集合
- 合并 function、custom 与 tool_search 调用为单个 assistant turn - 保留 namespace 映射和含冒号的 call ID - 补充输出边界及实际并行历史回归测试
1 participant
Summary
Validation
go test ./internal/server/orchestrator ./internal/server/bizcd llm && go test ./transformer/openai/...git diff --checkSupersedes #2106 after the branch history was rebuilt.
Fixes #2105
Summary by CodeRabbit
previous_response_id.