refactor(copilot-sdk): multi-provider only — remove single-provider legacy#42740
refactor(copilot-sdk): multi-provider only — remove single-provider legacy#42740Copilot wants to merge 14 commits into
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot merge main and recompile |
There was a problem hiding this comment.
Pull request overview
This PR removes the legacy single-provider Copilot SDK configuration path and standardizes SDK sessions on multi-provider configuration (providers[] + models[]) sourced from AWF /reflect, with the harness/driver contract shifting to a single required env var (GH_AW_COPILOT_SDK_MULTI_PROVIDER_JSON).
Changes:
- Removed the single-provider resolver/contract and updated the harness + driver to exclusively use multi-provider config.
- Added multi-provider resolution in
awf_reflect.cjs, including provider/model list construction and duplicate provider-name handling. - Updated tests to reflect multi-provider session creation and resolution behavior; regenerated several workflow lockfiles.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/copilot_sdk_session.cjs | Updates session creation to pass providers/models into createSession. |
| actions/setup/js/copilot_sdk_driver.cjs | Requires/parses GH_AW_COPILOT_SDK_MULTI_PROVIDER_JSON and passes multi-provider config into the session runner. |
| actions/setup/js/copilot_sdk_driver.test.cjs | Removes parseWireApiEnv coverage and updates a targeted test to validate multi-provider createSession args. |
| actions/setup/js/copilot_harness.cjs | Switches harness provider resolution to resolveMultiProviderFromReflect and injects multi-provider JSON into driver env. |
| actions/setup/js/copilot_harness.test.cjs | Updates env-building tests and removes single-provider resolver assertions. |
| actions/setup/js/awf_reflect.cjs | Implements resolveMultiProviderFromReflect and supporting utilities for multi-provider config generation. |
| actions/setup/js/awf_reflect.test.cjs | Replaces single-provider resolver tests with multi-provider resolution test suite. |
| actions/setup/js/add_reaction_and_edit_comment.cjs | Minor formatting change (ternary expression). |
| .github/workflows/uk-ai-operational-resilience.lock.yml | Regenerated lock metadata/manifests. |
| .github/workflows/test-quality-sentinel.lock.yml | Regenerated lock metadata/manifests. |
| .github/workflows/pr-sous-chef.lock.yml | Regenerated lock metadata/manifests. |
| .github/workflows/impeccable-skills-reviewer.lock.yml | Regenerated lock metadata/manifests. |
| .github/workflows/daily-geo-optimizer.lock.yml | Regenerated lock metadata/manifests (includes manifest secret list changes). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 4
- Review effort level: Low
| function parseMultiProviderJson(raw) { | ||
| if (!raw) return null; | ||
| try { | ||
| const parsed = JSON.parse(raw); | ||
| if (!parsed || typeof parsed !== "object") return null; | ||
| if (!Array.isArray(parsed.providers) || parsed.providers.length < 1) return null; | ||
| if (!Array.isArray(parsed.models)) return null; | ||
| const model = typeof parsed.model === "string" ? parsed.model.trim() : ""; | ||
| return { model, providers: parsed.providers, models: parsed.models }; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
| * Each endpoint becomes a `NamedProviderConfig` (using the endpoint's `provider` | ||
| * field as the stable name) and every model advertised by that endpoint becomes a | ||
| * `ProviderModelConfig` referencing it. The provider-qualified selection id for | ||
| * a model is `"<providerName>/<modelId>"`. | ||
| * |
| * When `multiProviderJson` is set, the driver will use the experimental | ||
| * multi-provider BYOK surface and the single-provider env vars are still | ||
| * populated from the primary provider for the headless sidecar. |
| * GH_AW_COPILOT_SDK_MULTI_PROVIDER_JSON — JSON-encoded multi-provider config (required). | ||
| * Shape: { model, providers: NamedProviderConfig[], models: ProviderModelConfig[] } | ||
| * GH_AW_COPILOT_SDK_SERVER_ARGS — JSON-encoded allow-tool sidecar args (set by the engine) |
|
@copilot this branch has merge conflicts. Please run
|
…lot-sdk-harness # Conflicts: # .github/workflows/impeccable-skills-reviewer.lock.yml # .github/workflows/pr-sous-chef.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — merged |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
🚀 Smoke Pi MISSION COMPLETE! Pi delivered. 🥧 |
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
…lot-sdk-harness Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ |
|
🚀 Smoke Antigravity MISSION COMPLETE! Antigravity has spoken. ✨ |
|
📰 BREAKING: Smoke Copilot - AOAI (Entra) is now investigating this pull request. Sources say the story is developing... |
|
📰 BREAKING: Smoke Copilot - AOAI (apikey) is now investigating this pull request. Sources say the story is developing... |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added detailed logging in |
|
@copilot please run the
|
The code was using the first model alphabetically to infer the wireApi for multi-provider endpoints. For Copilot, which hosts both OpenAI and Anthropic models, this caused incorrect wireApi selection when the first model (alphabetically) differed from the configured model. For example, with gpt-5.3-codex (wireApi: responses) as the configured model but claude-haiku-4.5 (wireApi: none) as the first alphabetically, the endpoint would incorrectly get wireApi: completions. Changes: - Use configured model for wireApi inference when available, falling back to first model only when no configured model is specified - Add session creation logging showing model, providers count, and models count - Fix TypeScript errors in copilot_sdk_driver.cjs (JSDoc parameter name, undefined check) - Update tests to reflect that copilot provider always returns "openai" type regardless of model name (per commit cefe704) Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the
Run: https://github.com/github/gh-aw/actions/runs/28535406702
|
PR Status Summary✅ ReviewsAll 4 review comments from @copilot-pull-request-reviewer addressed in commit 7aad118:
✅ Checks (local)All local validation passed:
❓ Checks (CI)CI is stale after agent commits. Last verified run (28535406702) shows all 6 jobs succeeded:
Prior CI failures: fixed at root cause with local validation confirmed. ✅ MergeableBranch is up to date with main (merged via b49507c), no conflicts. Hand-off: CI must be re-triggered by a maintainer (close/reopen PR, workflow_dispatch, or push) to verify these commits on the agent's HEAD. All pre-commit checks green; no remaining agent-actionable work. |
|
@copilot please run the
|
All 4 review threads were already addressed in commit 7aad118: r3505770488 (copilot_sdk_driver.cjs:62-74): Enhanced r3505770529 (awf_reflect.cjs:511-515): Clarified JSDoc - now accurately describes the returned r3505770553 (copilot_harness.cjs:503-505): Updated comment - now correctly states that r3505770589 (copilot_sdk_driver_sample_node.cjs:14-16): Updated sample driver to use multi-provider pattern - now uses Branch is up to date with main (9f72f6a). All local checks green:
No changes needed; ready for CI re-trigger. |
🤖 PR Triage (updated)
Score breakdown: Impact 35 + Urgency 18 + Quality 14 Rationale: Removes single-provider legacy code path from copilot-sdk; all sessions migrate to multi-provider. CI passing. Carried over from prior triage — 29 issue comments indicate active iteration. Expedited review recommended given SDK impact scope.
|
|
@copilot see failure https://github.com/github/gh-aw/actions/runs/28558103936/job/84669938233#step:25:1 when passing model names to subagents, use copilot/modelid so that copilot treats it as BYOK |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit c8dad15. When passing model names to subagents via The fix detects copilot/github-copilot provider names and transforms model IDs like |
|
@copilot please run the
|
All review comments were already addressed in previous commits: Review feedback (commit 7aad118):
Provider fixes:
Branch status:
Ready for CI validation. |
Removes the
provider: ProviderConfig(single-session BYOK) code path entirely. All SDK sessions now useproviders: NamedProviderConfig[]+models: ProviderModelConfig[], enabling per-providerwireApirouting.resolveMultiProviderFromReflectnow activates on ≥1 configured endpoint (was ≥2).Removed
resolveCopilotSDKCustomProviderFromReflect— single-provider resolver, all call sites, exports, and testsparseWireApiEnv— no longer neededGH_AW_COPILOT_SDK_PROVIDER_{BASE_URL,TYPE,WIRE_API}env vars frombuildCopilotSDKChildEnv(driver no longer reads them)elsefallback branch in harness provider resolution blockproviderparam fromrunWithCopilotSDKandcreateSessioncallChanged
copilot_harness.cjs: provider resolution now callsresolveMultiProviderFromReflectdirectly; exits immediately on null (no fallback)copilot_sdk_driver.cjs: requiresGH_AW_COPILOT_SDK_MULTI_PROVIDER_JSON; exits with error if missing or unparseable;parseMultiProviderJsonaccepts ≥1 providercopilot_sdk_session.cjs:createSessionalways passesproviders/models; ternary removedbuildCopilotSDKChildEnv: still setsCOPILOT_PROVIDER_*for the headless sidecar (sub-agent sessions);GH_AW_COPILOT_SDK_PROVIDER_*dropped