[WRONG BRANCH] fix(providers): un-obfuscate GitHub Copilot streamed tool-call arguments - #990
Conversation
|
This pull request currently targets @Simon-Opopeee Please retarget this PR to Its title has been prefixed with This pull request is being kept as a draft automatically. Once every issue above is resolved, it will be marked ready for review again. |
📝 WalkthroughWalkthroughThe change adds a GitHub Copilot SSE rewrite that removes obfuscation metadata, suppresses ciphertext argument deltas, and emits plaintext arguments before matching completion events. The rewrite applies only to GitHub Copilot streaming responses. ChangesGitHub Copilot obfuscation handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubCopilot
participant SSEPayloadRewrite
participant ResponsesClient
GitHubCopilot->>SSEPayloadRewrite: Send obfuscated function-call delta
SSEPayloadRewrite->>ResponsesClient: Send empty delta without obfuscation
GitHubCopilot->>SSEPayloadRewrite: Send done event with plaintext arguments
SSEPayloadRewrite->>ResponsesClient: Send plaintext arguments delta
SSEPayloadRewrite->>ResponsesClient: Send matching done event
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/server/responses/core.ts`:
- Around line 1772-1774: Extract the github-copilot provider check and
createGithubCopilotObfuscationRewrite call into a separate rewrite flag
variable, then include that flag in the needsClientRewrite conditional
expression on Line 1765 so the rewrite is considered when determining whether
clientBody should use rewrittenBody instead of nativeBody. Use the same rewrite
flag variable when building payloadRewrites to ensure win32EagerRewrite and
selectEagerPath apply the rewrite consistently. Add a focused regression test
covering the Windows native-passthrough relay path and eager relay selection
path to verify the Copilot rewrite is applied end-to-end.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ab306201-5f12-413f-acdb-b0b75289d10d
📒 Files selected for processing (3)
src/server/responses/core.tssrc/server/sse-payload-rewrite.tstests/sse-payload-rewrite.test.ts
| route.providerName === "github-copilot" | ||
| ? createGithubCopilotObfuscationRewrite() | ||
| : undefined, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include the Copilot rewrite in needsClientRewrite.
On Line 1765, needsClientRewrite excludes the condition added on Lines 1772-1774. On Windows, clientBody then returns nativeBody and bypasses rewrittenBody. If selectEagerPath selects the eager relay, it also omits rewritePayload because win32EagerRewrite uses the same false gate. The client receives the original ciphertext deltas.
Define one Copilot rewrite flag. Include it in needsClientRewrite. Use the same flag when building payloadRewrites. Add a focused regression test for the Windows and eager relay paths.
Proposed fix
const repairConfig = route.provider.responsesItemIdRepair;
- const needsClientRewrite = imageGenCallAliases.size > 0 || hasResponsesItemIdRepair(repairConfig);
+ const needsGithubCopilotObfuscationRewrite = route.providerName === "github-copilot";
+ const needsClientRewrite = imageGenCallAliases.size > 0
+ || hasResponsesItemIdRepair(repairConfig)
+ || needsGithubCopilotObfuscationRewrite;
// Compose opt-in payload rewrites into one parse/stringify pass (image-gen restore first).
const payloadRewrites = [
createImageGenCallRestoreRewrite(imageGenCallAliases),
hasResponsesItemIdRepair(repairConfig)
? createResponsesItemIdPayloadRewrite(repairConfig!, translatorBudget)
: undefined,
- route.providerName === "github-copilot"
+ needsGithubCopilotObfuscationRewrite
? createGithubCopilotObfuscationRewrite()
: undefined,
].filter((rewrite): rewrite is NonNullable<typeof rewrite> => rewrite !== undefined);Based on the PR objective, the rewrite must apply to both native-passthrough relay paths. As per path instructions, “A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| route.providerName === "github-copilot" | |
| ? createGithubCopilotObfuscationRewrite() | |
| : undefined, | |
| needsGithubCopilotObfuscationRewrite | |
| ? createGithubCopilotObfuscationRewrite() | |
| : undefined, |
🤖 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 `@src/server/responses/core.ts` around lines 1772 - 1774, Extract the
github-copilot provider check and createGithubCopilotObfuscationRewrite call
into a separate rewrite flag variable, then include that flag in the
needsClientRewrite conditional expression on Line 1765 so the rewrite is
considered when determining whether clientBody should use rewrittenBody instead
of nativeBody. Use the same rewrite flag variable when building payloadRewrites
to ensure win32EagerRewrite and selectEagerPath apply the rewrite consistently.
Add a focused regression test covering the Windows native-passthrough relay path
and eager relay selection path to verify the Copilot rewrite is applied
end-to-end.
Source: Path instructions
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0602e6fc66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| route.providerName === "github-copilot" | ||
| ? createGithubCopilotObfuscationRewrite() | ||
| : undefined, |
There was a problem hiding this comment.
For a GitHub Copilot Responses stream with neither image-generation aliases nor item-ID repair—the normal tool-call case—the new rewrite is added to payloadRewrites while needsClientRewrite remains false. On Windows this prevents win32EagerRewrite and later selects the unchanged nativeBody; configured eager relay also receives no rewritePayload, as does Darwin with streamMode: "eager-relay". The ciphertext therefore still reaches Codex and the tool turn stalls on those paths. Include the Copilot condition in needsClientRewrite and cover the platform gates with a regression test.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| }; | ||
| // The relay writes `data: <payload>` then the block delimiter, so this | ||
| // payload becomes two consecutive valid SSE events (delta, then done). | ||
| return `${JSON.stringify(deltaEvent)}\n\ndata: ${JSON.stringify(parsed)}`; |
There was a problem hiding this comment.
When the upstream done frame includes event: response.function_call_arguments.done, as the repository's own src/bridge.ts:18-20 framing does, embedding a blank line inside the replacement payload leaves that event field attached to the synthetic delta and emits the original done data as an unnamed message event. Clients that dispatch by SSE event name therefore receive neither event under its correct name and can still fail to commit the tool call. Emit two complete blocks with their respective event: fields, and test a named upstream frame.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| type: "response.function_call_arguments.delta", | ||
| item_id: parsed.item_id, | ||
| output_index: parsed.output_index, | ||
| sequence_number: parsed.sequence_number, |
There was a problem hiding this comment.
The inserted delta copies the done event's sequence_number, so the emitted stream contains two consecutive recognized events with the same number—for example, the new test produces both the plaintext delta and done event at sequence 5. Other generated Responses streams increment the sequence for every event in src/bridge.ts:278, while src/server/ws-bridge.ts:247 forwards rewritten payloads verbatim; clients that order or deduplicate by this field can therefore discard either the plaintext arguments or the completion event. Track an insertion offset and renumber the done and subsequent events so the sequence remains unique and monotonic.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| if (hadObfuscation) delete parsed.obfuscation; | ||
| const type = parsed.type; | ||
| if (type === "response.function_call_arguments.delta" && hadObfuscation) { | ||
| if (typeof parsed.item_id === "string") obfuscatedCallIds.add(parsed.item_id); |
There was a problem hiding this comment.
If a malformed, truncated, or long-running Copilot stream sends obfuscated deltas for distinct item_id values without matching done events, every ID remains in obfuscatedCallIds until the entire stream is collected. The relay releases its transient translator-budget charge after each event, so this cumulative set is not covered by the existing byte limit and an upstream can grow the proxy heap indefinitely even while client backpressure is working. Charge and cap the tracked IDs, removing them on all item terminal paths, and surface overflow through the existing typed stream-failure representation.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
GitHub re-encrypts item_id per SSE event, so delta and done never share an id; tracking by output_index keeps the plaintext delta injection working.
GitHub encrypts reasoning summaries with its own scheme (re-encrypted per event); Responses clients cannot decrypt them and stay stuck on thinking. Replace the ciphertext with a canonical empty plaintext summary in output_item events and the terminal completed payload.
…n values GitHub re-encrypts the response id and every item id per SSE event, so the terminal completed payload disagrees with the streamed events; clients reconcile by id and fail to finalize the turn. Pin each entity to its first-seen id across created, output_item, and completed events.
|
Closing: wrong base (main). Will reopen a single clean PR against dev. |
What
GitHub Copilot's
/responseswire is not compatible with Responses clients for thevscode-chatintegration. Agentic turns on Responses-only Copilot models (gpt-5.6-luna,gpt-5.6-sol,gpt-5.6-terra,gpt-5.4,gpt-5.4-mini,gpt-5.5,gpt-5.3-codex) stall in Codex clients: the app stays stuck on "thinking" and tool calls never complete. Fixes #989.Root cause (three layers)
delta+obfuscationfield); the.doneevent carries the plaintextarguments.encrypted_contentwith no plaintext summary; clients cannot decrypt them.completedpayload disagrees with the streamed events (call_idis the only stable field).Fix
New
createGithubCopilotObfuscationRewrite()insrc/server/sse-payload-rewrite.ts, wired into the passthroughpayloadRewritescomposition whenroute.providerName === "github-copilot"(applies to all Copilot models on both relay paths):argumentsare re-emitted as one delta immediately before.done, keeping the stream protocol-conformant.encrypted_contentare replaced with a canonical empty plaintext summary (inoutput_itemevents and the terminalcompletedpayload).created,output_item, andcompletedevents.obfuscationmetadata is stripped from every event; clean events pass through byte-identical.Tests
Added to
tests/sse-payload-rewrite.test.ts: ciphertext deltas → empty deltas + plaintext delta before.done; reasoning neutralization; response/item id pinning; clean events pass through unchanged.bun test tests/sse-payload-rewrite.test.tsandbun x tsc --noEmitpass.