Fix Copilot BYOK target resolution for self-hosted provider URLs - #49464
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes Copilot BYOK routing by deriving the AWF API proxy target from literal provider URLs.
Changes:
- Adds
COPILOT_PROVIDER_BASE_URLas a final target fallback. - Covers literal and expression-backed URLs.
- Recompiles the Ollama workflow with the resolved target.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/engine_api_targets.go |
Adds BYOK target resolution. |
pkg/workflow/awf_helpers_test.go |
Tests resolution and AWF configuration. |
.github/workflows/daily-byok-ollama-test.lock.yml |
Includes the Ollama proxy target. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (59 additions across 3 files). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: Fix Copilot BYOK target resolution for self-hosted provider URLs
The changes are correct and well-structured. No blocking issues found.
What was reviewed:
GetCopilotAPITargetnow correctly falls back toCOPILOT_PROVIDER_BASE_URLwhen neitherengine.api-targetnorGITHUB_COPILOT_BASE_URLis configuredextractLiteralEngineEnvHostproperly guards against GitHub expression values (${{...}}) to avoid misrouting- New tests cover the new fallback path, the expression-skip path, and the
BuildAWFConfigJSONintegration - The lock file diff reflects the expected
targets.copilot.hostinjection
Minor observation (non-blocking): The log message at line 177 (No GITHUB_COPILOT_BASE_URL, deriving Copilot API target from literal COPILOT_PROVIDER_BASE_URL) fires even when COPILOT_PROVIDER_BASE_URL is also absent, adding noise in standard non-BYOK workflows. Consider emitting it only when the env var is actually present.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 32 AIC · ⊞ 5.3K
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics (3 tests)
Verdict
|
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review
Applied /diagnosing-bugs and /tdd — one observation on test coverage.
Summary
What the PR does well
- Root cause is clearly identified and addressed at the right layer (
GetCopilotAPITarget). - The expression-skipping guard in
extractLiteralEngineEnvHostprevents misrouting when the URL is a GitHub Actions secret reference — good defensive coding. - Three focused tests cover the new fallback path, the expression-skip path, and end-to-end AWF config JSON output.
- Lock file is recompiled and the embedded AWF config now includes the concrete Copilot target — the fix is visible in the generated artifact.
One gap
The new GetCopilotAPITarget fallback means GetCopilotAllowlistTargets now reaches the same BYOK host via two internal code paths (direct extractLiteralEngineEnvHost call + the new GetCopilotAPITarget fallback). The seen-map deduplication handles this correctly at runtime, but no test exercises that path, so a future refactor could silently introduce duplicates. See the inline comment for a suggested test case.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 42.1 AIC · ⊞ 7K
Comment /matt to run again
| // Final fallback: derive from a literal BYOK provider URL so AWF's api-proxy preserves | ||
| // non-default hosts and ports for self-hosted OpenAI-compatible backends such as Ollama. | ||
| awfHelpersLog.Print("No GITHUB_COPILOT_BASE_URL, deriving Copilot API target from literal COPILOT_PROVIDER_BASE_URL") | ||
| return extractLiteralEngineEnvHost(workflowData, constants.CopilotProviderBaseURL) |
There was a problem hiding this comment.
[/tdd] The new GetCopilotAPITarget fallback causes GetCopilotAllowlistTargets to encounter the BYOK host from two code paths; the deduplication logic is not exercised by the new tests.
Add a case to TestGetCopilotAllowlistTargets that sets only COPILOT_PROVIDER_BASE_URL and asserts the host appears exactly once in the result slice.
@copilot please address this.
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: No blocking issues found
The Copilot BYOK target fallback logic is narrow, safe, and well-tested.
Review notes
- The new fallback in
GetCopilotAPITargetonly activates when bothengine.api-targetandGITHUB_COPILOT_BASE_URLare unset, and it correctly reuses the existingextractAPITargetHosthost/port parsing so behavior is consistent with other BYOK env-var handling (OpenAI, Anthropic). - GitHub Actions expression values (
${{ ... }}) inCOPILOT_PROVIDER_BASE_URLare correctly skipped via astrings.Contains(rawValue, "${{")guard inextractLiteralEngineEnvHost, preventing an unresolved expression string from leaking into the generated AWF config as a bogus host. - Test coverage is solid: it adds cases for the literal fallback, the expression-skip path, and an integration test (
TestBuildAWFConfigJSONIncludesCopilotLiteralBYOKTarget) verifying the target actually appears in the generated JSON. - The regenerated
daily-byok-ollama-test.lock.ymldiff is minimal (2 lines) and matches the expectedtargets.copilot.hostaddition for both the main and threat-detection jobs. - Ran an independent second pass plus a
grumpy-codersub-agent pass; neither surfaced any correctness, concurrency, security, or maintainability issue within the changed lines.
🔎 Code quality review by PR Code Quality Reviewer · auto · 105.6 AIC · ⊞ 7.8K
Comment /review to run again
|
🎉 This pull request is included in a new release. Release: |
The Daily BYOK Ollama workflow was compiling without a concrete Copilot api-proxy target even though
COPILOT_PROVIDER_BASE_URLpointed at a local Ollama endpoint. In AWF, that left Copilot BYOK traffic misrouted and produced repeated503failures on/v1/models.Root cause
GetCopilotAPITargetonly considered:engine.api-targetGITHUB_COPILOT_BASE_URLCOPILOT_PROVIDER_BASE_URLhad no derived Copilot target, so the generated AWF config omittedapiProxy.targets.copilot.host.Target resolution update
COPILOT_PROVIDER_BASE_URLwhen no explicit Copilot target is set.Generated workflow impact
Focused coverage
COPILOT_PROVIDER_BASE_URLExample of the new fallback path: