agentHost: preserve BYOK state through tool continuations - #331749
agentHost: preserve BYOK state through tool continuations#331749sgent-epic wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Preserves BYOK provider state through Copilot SDK tool continuations, addressing #329283.
Changes:
- Tracks pending response IDs by session, vendor, and model.
- Recovers matching tool-output continuations without overriding explicit state.
- Adds unit and SDK integration coverage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
byokLmProxyService.ts |
Implements continuation recovery and scoped state tracking. |
byokLmProxyService.test.ts |
Tests recovery, scoping, explicit IDs, and cleanup. |
copilotByokResponses.integrationTest.ts |
Verifies SDK tool continuation behavior end-to-end. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e547f44 to
9886540
Compare
|
Hi Vritant Bhardwaj (@vritant24), I hope you're doing well. Is there any way this could be prioritized? It's a blocker for wider piloting of agents in VS Code at Epic. I'm personally really liking the VS Code environment over other options, and I'd like to enable other folks to give it a try as well. Thanks for your time! |
4ec345b to
67cd0c4
Compare
| } | ||
|
|
||
| private _updateToolContinuation(state: ByokLmProxyState, key: string, result: IByokLmChatResult): void { | ||
| if (!result.responseId) { |
There was a problem hiding this comment.
result.responseId is not always evidence that the downstream response is resumable.
- Explicit-ZDR Responses requests use
store: falseand deliberately removeprevious_response_id. - The Responses processor still emits the returned ID as
stateful_marker. - This proxy then caches that ID and replaces the SDK replay with only tool outputs.
- On the next request, the ZDR endpoint drops the injected marker, leaving orphaned tool outputs without the original history.
Suggested fix:
- Define
stateful_markeras “resumable provider state”: do not emit it for explicit-ZDR/non-stored responses. - With no marker,
IByokLmChatResult.responseIdremains absent and this proxy preserves the full stateless replay. - Do not infer this from the local
body.store: the SDK intentionally sendsstore: falseto this facade even when the downstream provider is stateful. - Add a ZDR Responses tool-continuation test verifying that no marker is cached and the full replay reaches the provider.
There was a problem hiding this comment.
Fixed in bf9576442cb. responseId is now explicitly an opt-in resumable-state contract. The component that knows whether a response is resumable owns whether to report it. The proxy does not infer this from store, and the regression verifies that absence preserves full replay without previous_response_id.
| // A session can disappear after receiving a tool call, so keep abandoned | ||
| // continuations from growing for the lifetime of the shared proxy. | ||
| state.delete(key); | ||
| state.set(key, { responseId: result.responseId, calls }); |
There was a problem hiding this comment.
This retains only one continuation per (sessionId, vendor, modelId), but root, background, and subagent trajectories share the same session-scoped provider token.
A normal nested flow can therefore lose the parent state even without an HTTP race:
parent tool call -> stores resp_parent
subagent response -> overwrites/deletes the same entry
parent tool result -> recovery misses -> strict stateful provider rejects it
Suggested fix:
- Retain multiple pending responses and match trailing tool outputs against the exact call-ID/kind set.
- Recover only when exactly one candidate in the same session/vendor/model scope matches.
- Remove only the matched predecessor after a successful continuation; unrelated terminal responses must not clear other entries.
- Keep memory bounded with a flat global limit of 256 pending responses, not unbounded arrays per scope, and store only IDs/kinds rather than history.
- Add an interleaving test with parent and subagent continuations using the same session/vendor/model.
There was a problem hiding this comment.
Fixed in 6a805a3423f. The proxy now keeps a flat globally bounded set of candidates, resumes only a unique exact call-ID/kind match within the same scope, and removes only the matched predecessor after a successful bridge call. Tests cover parent/subagent interleaving, ambiguous matches, and the 256-entry bound.
6a805a3 to
83216ae
Compare
83216ae to
e7008ba
Compare
|
Hi Vritant Bhardwaj (@vritant24), please let me know if there are any changes required. Thanks! |
|
Thanks for the update. The parent/subagent collision looks fixed now. I think two things remain:
The latest rebase doesn’t appear to change either path. |
|
Thanks, Vritant Bhardwaj (@vritant24). Both are addressed in e2cee5c. ZDR responses no longer emit markers, with direct ZDR/stored coverage. Explicit continuations now remove matching cached state after success while retaining it on errors. Full compile and focused tests pass. |
e2cee5c to
abd344d
Compare
Mohammad javad Dianat (dianatofficial)
left a comment
There was a problem hiding this comment.
Verified the diff. Changes align with project standards.
d509d36 to
7a302ea
Compare
|
Hi Vritant Bhardwaj (@vritant24) can we run the test suites again? Thanks! |
0d98e35 to
8922007
Compare
|
Hi Vritant Bhardwaj (@vritant24), can you please help me with the next steps? Apologies for the nagging, but it's a blocker for us. TIA! |
8922007 to
05a545b
Compare
05a545b to
8188e25
Compare
|
Hi Vritant Bhardwaj (@vritant24) is there anything else I need to do? Can you please approve the workflows to run? |
8188e25 to
7f3cc34
Compare
7f3cc34 to
769a7b6
Compare
Summary
previous_response_idand forwarding only the matching tool outputsWhy
The bundled Copilot SDK currently replays the full Responses history after a tool call and omits
previous_response_id. Extension-provided stateful models then receive nostateful_markerand reject the continuation.The integration test reproduces the SDK request shape directly. Before the proxy change, it failed because the second request replayed the message, reasoning, and tool call with no response ID. It now reaches the provider with the original response ID and only the tool output.
Fixes #329283
Testing
npm run compile-clientscripts\test.bat --run src\vs\platform\agentHost\test\node\byokLmProxyService.test.tsscripts\test-integration.bat --run src\vs\platform\agentHost\test\node\providerIntegration\copilotByokResponses.integrationTest.tsgit diff --check upstream/main...HEAD