Emit authoritative Copilot custom-routing signal in lock metadata - #49991
Conversation
🔍 PR TriageCategory: feature · Risk: low · Total score: 15/100
Draft PR just opened (0 files changed yet) to add a default-Copilot-configuration signal to lock file metadata. Agent has not started substantive work. Recommended action:
|
Co-authored-by: salmanmkc <32169182+salmanmkc@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds authoritative Copilot custom-routing metadata to compiled lock files, resolving #49990.
Changes:
- Adds and emits
engine_base_url_customized. - Centralizes Copilot BYOK/custom-routing detection.
- Adds unit, compile, serialization, and regenerated workflow coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/lock_schema.go |
Adds the routing metadata field. |
pkg/workflow/lock_schema_test.go |
Tests serialization and omission. |
pkg/workflow/engine_api_targets.go |
Defines shared routing predicates. |
pkg/workflow/copilot_engine_execution.go |
Reuses shared BYOK detection. |
pkg/workflow/compiler_yaml_header.go |
Populates lock metadata. |
pkg/workflow/compiler_yaml_test.go |
Tests compiled metadata. |
pkg/workflow/awf_helpers_test.go |
Covers custom-routing cases. |
.github/workflows/smoke-copilot-aoai-entra.lock.yml |
Regenerates Entra BYOK metadata. |
.github/workflows/smoke-copilot-aoai-apikey.lock.yml |
Regenerates API-key BYOK metadata. |
.github/workflows/daily-byok-ollama-test.lock.yml |
Regenerates Ollama BYOK metadata. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 0
- Review effort level: Balanced
🧪 Test Quality Sentinel Report
📊 Metrics (10 tests)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: Request changes — the new "authoritative" custom-routing signal has a false-positive edge case
One correctness bug undermines the PR's core claim of an authoritative engine_base_url_customized signal: isCopilotBYOKMode treats a present-but-empty COPILOT_PROVIDER_BASE_URL env value as BYOK mode (key-presence check, not value check), so a blank/templated-empty override will incorrectly flag the workflow as customized. Also flagged a test-coverage gap: the new BYOK/custom-config predicate is only tested through the combined isCopilotCustomConfig wrapper, leaving the sandbox-gated branch of isCopilotBYOKMode unverified in isolation.
💡 Themes and highlights
- Correctness:
engineEnvHasKeychecks map key presence only; combined with the new metadata surfacing, this can misreport default configs as customized. - Predicate reuse: Good —
isCopilotBYOKModeis now shared between the execution path and the metadata predicate, preventing drift between runtime behavior and reported metadata. This part of the design is sound. - Test coverage: New tests thoroughly cover the OR'd
isCopilotCustomConfigoutcome and omitempty/serialization behavior ofEngineBaseURLCustomized, but do not isolate the BYOK sandbox-gating branch. - Struct field additions and lock schema wiring look mechanically correct and consistent with existing patterns (alphabetized struct tags, omitempty usage matching sibling bool
Strict).
🔎 Code quality review by PR Code Quality Reviewer · auto · 101 AIC · ⌖ 6.54 AIC · ⊞ 7.9K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/engine_api_targets.go:187
isCopilotBYOKMode treats a present-but-empty COPILOT_PROVIDER_BASE_URL as BYOK mode, which will falsely mark the new engine_base_url_customized signal as true for a default configuration.
<details>
<summary>💡 Details</summary>
engineEnvHasKey (pkg/workflow/engine_helpers.go) only checks map key presence via _, ok := workflowData.EngineConfig.Env[key] — it does not check that the value is non-empty. A workflow with engine.env: {COPILOT_PROVIDER_BASE_URL: ""} (e.g. an accidentall…
pkg/workflow/awf_helpers_test.go:37
New test coverage exercises isCopilotCustomConfig end-to-end but never isolates isCopilotBYOKMode, leaving the sandbox/firewall-gated providerOverrideBYOK branch under-tested on its own.
<details>
<summary>💡 Details</summary>
All new tests (TestIsCopilotCustomConfig, TestCompileWorkflowMetadataMarksCopilotCustomConfig) go through the combined isCopilotCustomConfig predicate, which OR's together isCopilotBYOKMode(...) and GetCopilotAPITarget(...) != "". There is no direct unit…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — two focused comments; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Missing direct test for
isCopilotBYOKMode: The predicate now drives both the runtime BYOK path and metadata emission. It is covered transitively throughTestIsCopilotCustomConfig, but a dedicated test table would pin the contract and give faster signal if the implementation changes. - Silent divergence between
engineEnvHasKeyandGetCopilotAPITargetforCOPILOT_PROVIDER_BASE_URL: Worth a brief comment to clarify intent for future readers.
Positive Highlights
- ✅ Clean single-source-of-truth:
isCopilotCustomConfigandisCopilotBYOKModecorrectly unify runtime and metadata paths, eliminating the previous drift risk. - ✅ Good
omitemptydiscipline —engine_base_url_customizedis absent from the lock file for default configs, keeping metadata small. - ✅ 7-case test table in
TestIsCopilotCustomConfigcovers all documented signal combinations. - ✅ Schema and
AgentMetadataInfoare updated consistently; no dangling references.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 70.9 AIC · ⌖ 13.6 AIC · ⊞ 7.1K
Comment /matt to run again
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (198 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot sous-chef triage: The branch is green and the latest fix is in; please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Implemented and pushed
Actions taken: empty Hand-off: CI must be re-triggered by a maintainer (close/reopen PR, workflow_dispatch, or maintainer push) before merge. |
|
@copilot sous-chef triage: There are still two unresolved review threads even though your latest follow-up says the fix is in.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. I addressed both open review comments and pushed
I can’t resolve review threads via the available agent APIs in this environment, but both threads are now replied to and ready to be resolved. |
|
@copilot sous-chef triage: Your latest follow-up says both open review comments were addressed on
|
Completed PR-finisher pass on current head I’m still blocked from programmatically resolving review threads from this agent environment (reply is supported, thread-resolution mutation is not exposed), so the two threads remain open even though they are substantively addressed:
Merge-readiness snapshot:
Handoff: please resolve the two addressed threads in the UI and re-trigger/reconcile required checks for |
PR Triage: #49991
Carry-over from a prior triage run (previously deferred). Emits Copilot custom-routing signal in lock metadata.
|
|
@copilot Your latest follow-up says both open review threads were addressed, but they are still unresolved. Please resolve the remaining threads directly after confirming the current head includes the fixes, then run the
|
Confirmed on current head PR-finisher merge-readiness snapshot:
Hand-off: please re-trigger required CI as a maintainer (close/reopen PR, workflow_dispatch, or maintainer push) and complete final mergeability/check verification in UI before merge. |
|
@copilot sous-chef triage: This PR still needs another pass before maintainers can investigate quickly. Please refresh the branch if needed, address remaining blockers, then run the
|
|
ty @pelikhan |
|
🎉 This pull request is included in a new release. Release: |
Lock metadata exposed agent/schema fields but not whether Copilot ran with default GitHub routing vs customized provider/base URL/api target. Downstream consumers had to infer from compiled step env, which misses
engine.api-targetand is brittle against compiler output changes.Metadata surface
engine_base_url_customizedto# gh-aw-metadata(LockMetadata).AgentMetadataInfo→GenerateLockMetadata→ header emission.Single-source routing predicate
isCopilotBYOKMode(...)isCopilotCustomConfig(...)isCopilotCustomConfigcombines:engine.model-providergateway +COPILOT_PROVIDER_BASE_URL)GetCopilotAPITarget, includingengine.api-targetandGITHUB_COPILOT_BASE_URL).Drift prevention
Coverage updates
branch-refresh requested by PR Sous Chef run: https://github.com/github/gh-aw/actions/runs/30830929419