Enable BYOK coverage for .NET E2E tests#2058
Draft
roji wants to merge 7 commits into
Draft
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96047547-c040-4728-b7f0-f43aa880aef5
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96047547-c040-4728-b7f0-f43aa880aef5
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96047547-c040-4728-b7f0-f43aa880aef5
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96047547-c040-4728-b7f0-f43aa880aef5
Requires a runtime package containing the singular-provider task model validation fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96047547-c040-4728-b7f0-f43aa880aef5
Requires a runtime package containing the lightweight-provider User-Agent fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96047547-c040-4728-b7f0-f43aa880aef5
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96047547-c040-4728-b7f0-f43aa880aef5
Contributor
Cross-SDK Consistency Review ✅This PR makes changes to test infrastructure and internal test harness components only — no public SDK API surfaces are modified. Changes reviewed:
No cross-SDK issues found. These changes do not introduce new public API methods, options, or behaviors that would need to be mirrored in Node.js, Python, Go, Java, or Rust SDKs. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Enables BYOK (provider-backed) coverage for several .NET E2E scenarios and improves fidelity of the Anthropic Messages adapter when tool results include nested media, while updating snapshots/tests to avoid double-consuming image budgets.
Changes:
- Re-enabled multiple .NET E2E tests across the provider matrix by removing CAPI-only traits and adjusting assertions.
- Updated Anthropic Messages adapter + tests to preserve image/document blocks nested inside tool results.
- Simplified blob-attachment transcript snapshots to validate image inclusion without requiring a second “view” turn.
Show a summary per file
| File | Description |
|---|---|
| test/snapshots/session_config/should_accept_blob_attachments.yaml | Updates blob-attachment transcript snapshot to a single-turn image acknowledgment. |
| test/snapshots/session/should_accept_blob_attachments.yaml | Updates blob-attachment transcript snapshot to avoid tool calls and second image budget usage. |
| test/harness/modelProtocolAdapters.test.ts | Adds coverage for nested media blocks inside Anthropic tool results. |
| test/harness/anthropicMessagesAdapter.ts | Extracts nested tool-result media into canonical image/file content parts. |
| dotnet/test/Harness/E2ETestBase.cs | Centralizes helper to detect image content parts in outbound requests. |
| dotnet/test/E2E/StreamingFidelityE2ETests.cs | Removes CAPI-only gating on non-streaming delta fidelity tests. |
| dotnet/test/E2E/SessionE2ETests.cs | Adjusts blob attachment test to assert response + outbound request contains image_url. |
| dotnet/test/E2E/SessionConfigE2ETests.cs | Re-enables several BYOK scenarios; updates blob attachment + User-Agent/client name coverage. |
| dotnet/test/E2E/RpcTasksAndHandlersE2ETests.cs | Removes CAPI-only gating for invalid task-agent model behavior. |
| dotnet/test/E2E/GitHubTelemetryForwardingE2ETests.cs | Updates comment clarifying GitHub telemetry remains intentionally CAPI-only. |
Review details
Comments suppressed due to low confidence (1)
dotnet/test/E2E/SessionE2ETests.cs:1
- Asserting an exact LLM response string (including punctuation) is typically brittle across provider backends and model versions, and can introduce flaky E2E failures unrelated to the behavior under test (blob/image propagation). Since the test’s primary goal appears to be validating that the outbound request contains the image content, consider loosening the content assertion (e.g., case-insensitive match on “Image received” and/or allow optional trailing punctuation) or removing it entirely and relying on the request-validation assertion.
/*---------------------------------------------------------------------------------------------
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Low
Comment on lines
+269
to
+276
| for (const part of content) { | ||
| if (part.type === "text" && typeof part.text === "string") { | ||
| text.push(part.text); | ||
| } else if (part.type === "image" || part.type === "document") { | ||
| const contentPart = anthropicMediaContentPart(part); | ||
| if (contentPart) media.push(contentPart); | ||
| } | ||
| } |
Comment on lines
199
to
+214
| } else if (block.type === "tool_result") { | ||
| flushUserContent(); | ||
| const toolResult = anthropicToolResultContent(block.content); | ||
| result.push({ | ||
| role: "tool", | ||
| tool_call_id: block.tool_use_id ?? "", | ||
| content: anthropicToolResultContent(block.content), | ||
| content: toolResult.text, | ||
| }); | ||
| toolResultMedia.push(...toolResult.media); | ||
| } | ||
| } | ||
|
|
||
| flushUserContent(); | ||
| if (toolResultMedia.length) { | ||
| result.push({ role: "user", content: toolResultMedia }); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
cd test/harness && npm test(58 passed)cd dotnet && dotnet format --verify-no-changes --no-restoreRuntime dependency
This PR remains draft because SDK main pins
@github/copilot1.0.73, which predates all required runtime fixes.2fa7d64c. Anthropic in 1.0.73 still emits deltas with SDK streaming disabled. No SDK workaround is applied and no unpublished runtime is pinned. The minimum supported package is the first published@github/copilotrelease containing runtime commitec6338or newer.ec6338.b93e17f760743601a916a55f9180db4aac7850ed— legacy BYOK subagent model validationf400d825707c678dc2d7216c038c818026eee850—ClientNamepropagation to BYOK user agentsRuntime PR #13354 is still open with head
f400d825707c678dc2d7216c038c818026eee850; no merge SHA or package version exists yet. The first usable published package must contain both commits, including the TypeScript factory changes, regeneratedLightweightClientCreatePlanInput.userAgent, and rebuilt native runtime binary. Partial cherry-picks are incompatible.The SDK package pin can be updated only after the first published
@github/copilotversion contains runtimeec6338or newer plus the complete runtime PR #13354 changes. Unpublished artifacts must not be pinned.