Preserve Tinfoil reasoning passthrough semantics#153
Conversation
Keep Factory session metadata out of the repository. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
WalkthroughAdds propagation of reasoning-related extra fields through the proxy and web handlers: a new Changes
Sequence Diagram(s)sequenceDiagram
participant Upstream as Upstream API
participant Proxy as tinfoil-proxy
participant Web as Rust Handler
participant Client as OpenAI Client
rect rgba(120,180,240,0.5)
Note over Upstream,Client: Reasoning propagation (streaming & non-streaming)
Upstream->>Proxy: Response with extra_fields (`reasoning`, `reasoning_content`)
activate Proxy
Proxy->>Proxy: extractExtraFieldString(extra_fields)
Proxy->>Proxy: populateReasoningFields(message, extra_fields)
Proxy->>Web: stream/emit delta with reasoning fields
deactivate Proxy
activate Web
Web->>Web: prefer delta.reasoning, fallback to delta.reasoning_content
Web->>Client: SSE/forwarded message with reasoning metadata
deactivate Web
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Keep chat completions lossless by forwarding `reasoning` and `reasoning_content` exactly as received, while letting the Responses API bridge consume either field name. Includes the rebuilt proxy binary. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
6db4c3b to
a71b077
Compare
Keep replayed assistant turns lossless by forwarding explicit empty content and include the rebuilt proxy binary. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Refresh the dev and prod PCR manifests after rebuilding the EIFs and appending their signed history entries. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
fe7591f to
dcbb9ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tinfoil-proxy/main.go (1)
910-921: Consider restoringToolCallsin the nil-message fallback path.When
response.Choices[i].Messageis nil, the fallback only restoresRole,Content, andRefusal. If the SDK's completion containsToolCallsorFunctionCall, they would be lost. While this is an edge case (the marshal/unmarshal path at lines 892-904 should normally preserve these), adding them to the fallback would make the recovery more robust.💡 Proposed fix to include tool calls in fallback
if response.Choices[i].Message == nil { response.Choices[i].Message = &ChatMessage{ Role: string(completion.Choices[i].Message.Role), Content: completion.Choices[i].Message.Content, Refusal: completion.Choices[i].Message.Refusal, } + // Restore tool calls if present + if len(completion.Choices[i].Message.ToolCalls) > 0 { + toolCallsJSON, _ := json.Marshal(completion.Choices[i].Message.ToolCalls) + json.Unmarshal(toolCallsJSON, &response.Choices[i].Message.ToolCalls) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinfoil-proxy/main.go` around lines 910 - 921, The nil-message fallback currently creates a ChatMessage with only Role, Content and Refusal, losing completion-side ToolCalls and FunctionCall; when building the fallback ChatMessage in the loop over response.Choices (comparing to completion.Choices) include any ToolCalls and FunctionCall from completion.Choices[i].Message into the new ChatMessage before calling populateReasoningFields so those fields are preserved (update the fallback construction where response.Choices[i].Message is set to &ChatMessage{...} to copy ToolCalls and FunctionCall from completion.Choices[i].Message).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tinfoil-proxy/main.go`:
- Line 19: The import for the OpenAI respjson package is incorrect; update the
import of the respjson package used by main.go from the wrong path that includes
"packages/" to the correct module path by replacing the imported path for the
respjson package so the code imports the package named respjson from
"github.com/openai/openai-go/v2/respjson" (i.e., remove the "packages/" segment)
so references to respjson in main.go compile.
---
Nitpick comments:
In `@tinfoil-proxy/main.go`:
- Around line 910-921: The nil-message fallback currently creates a ChatMessage
with only Role, Content and Refusal, losing completion-side ToolCalls and
FunctionCall; when building the fallback ChatMessage in the loop over
response.Choices (comparing to completion.Choices) include any ToolCalls and
FunctionCall from completion.Choices[i].Message into the new ChatMessage before
calling populateReasoningFields so those fields are preserved (update the
fallback construction where response.Choices[i].Message is set to
&ChatMessage{...} to copy ToolCalls and FunctionCall from
completion.Choices[i].Message).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2ddf7044-0eb2-40e8-88a1-61f749fdcc07
⛔ Files ignored due to path filters (3)
pcrDev.jsonis excluded by!pcrDev.jsonpcrProd.jsonis excluded by!pcrProd.jsontinfoil-proxy/dist/tinfoil-proxyis excluded by!**/dist/**
📒 Files selected for processing (3)
pcrDevHistory.jsonpcrProdHistory.jsontinfoil-proxy/main.go
✅ Files skipped from review due to trivial changes (2)
- pcrProdHistory.json
- pcrDevHistory.json
Summary
.factoryworkspace filesreasoningandreasoning_contentas distinct chat-completions fields in the Tinfoil proxy while still rebuilding the shipped binarysrc/web/openai.rsjust update-pcr-allValidation
tinfoil-proxy)Summary by CodeRabbit
Bug Fixes
Chores