fix(pipeline): expose RawRequest before llm middlewares#1687
Conversation
Greptile SummaryThis PR fixes a sequencing bug in the LLM pipeline where
Confidence Score: 5/5The change is a safe, targeted one-line reorder that correctly makes the raw inbound request available to middlewares earlier in the pipeline. The fix is minimal and the intent is clear. Inbound transformers that already set RawRequest themselves (e.g. openai/inbound.go) assign the same pointer that the pipeline would, so the overwrite is harmless. The new test verifies the core scenario. The only gaps are a test-code style duplication and a missing edge-case test for middleware-replacing-the-request, neither of which affects runtime correctness. No files require special attention; both changed files are straightforward. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Pipeline
participant Inbound as Inbound Transformer
participant MW1 as Middleware 1 (OnInboundLlmRequest)
participant MW2 as Middleware 2 (OnInboundLlmRequest)
participant Outbound as Outbound Transformer
Client->>Pipeline: Process(ctx, rawRequest)
Pipeline->>Inbound: TransformRequest(ctx, rawRequest)
Inbound-->>Pipeline: llmRequest
Note over Pipeline: ✅ llmRequest.RawRequest = rawRequest (BEFORE middlewares — this PR's fix)
Pipeline->>MW1: OnInboundLlmRequest(ctx, llmRequest)
Note over MW1: Can now read llmRequest.RawRequest.Headers
MW1-->>Pipeline: llmRequest (modified)
Pipeline->>MW2: OnInboundLlmRequest(ctx, llmRequest)
Note over MW2: Can now read llmRequest.RawRequest.Headers
MW2-->>Pipeline: llmRequest (modified)
Pipeline->>Outbound: TransformRequest(ctx, llmRequest)
Outbound-->>Pipeline: outboundHttpReq
Note over Pipeline: MergeInboundRequest + RawRequest middlewares + execute
Reviews (1): Last reviewed commit: "fix(pipeline): expose RawRequest before ..." | Re-trigger Greptile |
| // testOutboundWithHeaders is used when the pipeline needs a non-nil header map | ||
| // so inbound header merging can proceed without panicking. | ||
| type testOutboundWithHeaders struct{} | ||
|
|
||
| func (t *testOutboundWithHeaders) APIFormat() llm.APIFormat { | ||
| return "test/format" | ||
| } | ||
|
|
||
| func (t *testOutboundWithHeaders) TransformRequest(ctx context.Context, request *llm.Request) (*httpclient.Request, error) { | ||
| return &httpclient.Request{Headers: http.Header{}}, nil | ||
| } |
There was a problem hiding this comment.
testOutboundWithHeaders duplicates all five interface methods from testOutbound, differing only in the TransformRequest return value. Embedding testOutbound and overriding just that method removes the repetition entirely.
| // testOutboundWithHeaders is used when the pipeline needs a non-nil header map | |
| // so inbound header merging can proceed without panicking. | |
| type testOutboundWithHeaders struct{} | |
| func (t *testOutboundWithHeaders) APIFormat() llm.APIFormat { | |
| return "test/format" | |
| } | |
| func (t *testOutboundWithHeaders) TransformRequest(ctx context.Context, request *llm.Request) (*httpclient.Request, error) { | |
| return &httpclient.Request{Headers: http.Header{}}, nil | |
| } | |
| // testOutboundWithHeaders is used when the pipeline needs a non-nil header map | |
| // so inbound header merging can proceed without panicking. | |
| type testOutboundWithHeaders struct { | |
| testOutbound | |
| } | |
| func (t *testOutboundWithHeaders) TransformRequest(ctx context.Context, request *llm.Request) (*httpclient.Request, error) { | |
| return &httpclient.Request{Headers: http.Header{}}, nil | |
| } |
| func TestProcess_SetsRawRequestBeforeInboundLlmMiddlewares(t *testing.T) { | ||
| ctx := context.Background() | ||
| callOrder := []string{} | ||
|
|
||
| m1 := newTrackingMiddleware("M1", &callOrder) | ||
| m2 := newTrackingMiddleware("M2", &callOrder) | ||
|
|
||
| exec := &testExecutor{} | ||
| factory := NewFactory(exec) | ||
| p := factory.Pipeline( | ||
| &testInbound{}, | ||
| &testOutboundWithHeaders{}, | ||
| WithMiddlewares(m1, m2), | ||
| ) | ||
|
|
||
| request := &httpclient.Request{Headers: http.Header{"X-Test": []string{"1"}}} | ||
| result, err := p.Process(ctx, request) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, result) | ||
| require.True(t, m1.sawRawRequestOnInbound) | ||
| require.True(t, m2.sawRawRequestOnInbound) | ||
| } |
There was a problem hiding this comment.
The test uses trackingMiddleware.OnInboundLlmRequest, which passes the request through unchanged. It therefore doesn't cover the scenario where M1 returns a brand-new *llm.Request (without copying RawRequest), which would leave M2 seeing nil. Since the contract now relies on RawRequest surviving through the full middleware chain, a test case where a middleware wraps or replaces the request object would make the guarantee explicit and guard against regressions in future middleware implementations.
There was a problem hiding this comment.
Code Review
This pull request modifies the pipeline execution flow to ensure the original RawRequest is available to middlewares during the inbound request phase, supported by a new test case and test helper. The review feedback identifies a potential regression where RawRequest could be lost if a middleware returns a new request instance, suggesting a re-assignment after the middleware chain. Additionally, there is a recommendation to refactor the test helper testOutboundWithHeaders using struct embedding to reduce boilerplate code.
| llmRequest, err = p.applyBeforeRequestMiddlewares(ctx, llmRequest) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
While moving the RawRequest assignment before the middlewares allows them to access the original request, it introduces a potential regression: if any middleware returns a new llm.Request instance (rather than modifying the pointer in-place), the RawRequest field will be nil for the rest of the pipeline. This would break httpclient.MergeInboundRequest later in processRequest.
To ensure robustness and maintain the previous behavior's guarantee that RawRequest is present during execution, you should re-assign it after the middleware chain finishes.
| llmRequest, err = p.applyBeforeRequestMiddlewares(ctx, llmRequest) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Step 2: Apply before request middlewares | |
| llmRequest, err = p.applyBeforeRequestMiddlewares(ctx, llmRequest) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Re-ensure RawRequest is set even if a middleware returned a new request object. | |
| llmRequest.RawRequest = request |
| type testOutboundWithHeaders struct{} | ||
|
|
||
| func (t *testOutboundWithHeaders) APIFormat() llm.APIFormat { | ||
| return "test/format" | ||
| } | ||
|
|
||
| func (t *testOutboundWithHeaders) TransformRequest(ctx context.Context, request *llm.Request) (*httpclient.Request, error) { | ||
| return &httpclient.Request{Headers: http.Header{}}, nil | ||
| } |
There was a problem hiding this comment.
testOutboundWithHeaders is currently duplicating all methods of testOutbound. You can significantly reduce boilerplate by embedding testOutbound and only overriding the TransformRequest method. The other redundant methods for testOutboundWithHeaders (like TransformResponse, TransformStream, etc.) can then be removed from this file.
type testOutboundWithHeaders struct {
testOutbound
}
func (t *testOutboundWithHeaders) TransformRequest(ctx context.Context, request *llm.Request) (*httpclient.Request, error) {
return &httpclient.Request{Headers: http.Header{}}, nil
}
Summary
llm.Request.RawRequestbeforeOnInboundLlmRequestmiddlewares runWhy
Request-header-based LLM middlewares can depend on the original inbound request headers. Today
/responses-style inbounds can miss them becauseRawRequestis assigned afterOnInboundLlmRequestmiddlewares run.Validation
cd llm && go test ./pipeline -count=1