Skip to content

Support per-model Copilot SDK wireApi via provider/model registry metadata#42497

Merged
pelikhan merged 5 commits into
mainfrom
copilot/implement-multiple-provider-configuration
Jun 30, 2026
Merged

Support per-model Copilot SDK wireApi via provider/model registry metadata#42497
pelikhan merged 5 commits into
mainfrom
copilot/implement-multiple-provider-configuration

Conversation

Copilot AI commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Copilot SDK v1.0.4 introduced multi-provider BYOK registry semantics where transport behavior (wireApi) can vary by model. This PR updates the harness/provider resolution path and model metadata pipeline so provider + wire settings are selected per model instead of inferred from provider type alone.

  • SDK provider resolution now includes per-model wire API

    • resolveCopilotSDKCustomProviderFromReflect now resolves:
      • provider type
      • provider baseUrl
      • model-scoped wireApi (responses / completions, when applicable)
    • resolution order for wireApi:
      1. models.json model metadata (wire_api / wireApi)
      2. provider-specific fallback (completions for OpenAI/Azure-compatible models)
      3. omitted for Anthropic
  • Harness → driver propagation of wire settings

    • harness now exports GH_AW_COPILOT_SDK_PROVIDER_WIRE_API alongside base URL/type.
    • SDK driver reads that value and injects it into ProviderConfig when creating the session.
  • Model catalog now carries routing metadata

    • make refresh-models-json preserves/normalizes wire_api for model entries.
    • mirrored catalogs were updated:
      • pkg/cli/data/models.json
      • actions/setup/js/models.json
    • Copilot model entries now encode routing metadata used by runtime resolution.
  • Model inventory workflow guidance updated

    • daily-model-inventory.md now explicitly instructs validation/preservation of provider_type + wire_api for Copilot models.
    • lockfile recompiled accordingly.
  • Targeted coverage updates

    • resolver tests now assert wireApi behavior (defaulting + explicit responses support).
    • harness/driver tests now assert propagation into createSession provider config.
// resolved session provider config (SDK driver)
provider: {
  type: "openai",
  baseUrl: "http://api-proxy:10002",
  wireApi: "responses" // model-specific
}

Copilot AI and others added 3 commits June 30, 2026 15:02
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 AI changed the title Support per-model wireApi in Copilot SDK harness provider config Support per-model Copilot SDK wireApi via provider/model registry metadata Jun 30, 2026
Copilot AI requested a review from pelikhan June 30, 2026 15:16
@pelikhan pelikhan marked this pull request as ready for review June 30, 2026 15:18
Copilot AI review requested due to automatic review settings June 30, 2026 15:18
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #42497 does not have the 'implementation' label and has only 28 new lines of code in business logic directories (threshold: 100).

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Test Quality Sentinel completed test quality analysis.

TQS analysis for PR #42497 already completed in this workflow run: comment posted (78/100, Acceptable) and APPROVE review submitted. Both safe-output limits were already consumed by the prior agent session.

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

PR Code Quality Reviewer completed the code quality review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the Copilot SDK BYOK provider-resolution pipeline so wireApi transport selection is resolved per model (via registry metadata) and propagated from the harness into the SDK driver, aligning runtime behavior with Copilot SDK v1.0.4 multi-provider registry semantics.

Changes:

  • Extended model catalogs and make refresh-models-json to preserve/normalize model routing metadata (provider_type, wire_api).
  • Updated AWF reflect-based provider resolution to compute model-scoped wireApi and updated harness/driver env wiring to pass it into SDK ProviderConfig.
  • Added/updated targeted JS tests to assert wireApi defaulting and explicit responses support; updated daily model inventory workflow guidance and lockfile.
Show a summary per file
File Description
pkg/cli/data/models.json Adds wire_api routing metadata to Copilot model entries.
actions/setup/js/models.json Mirrors wire_api metadata updates for the JS setup action.
Makefile Updates refresh-models-json jq transform to preserve provider_type and wire_api/wireApi.
actions/setup/js/copilot_sdk_driver.cjs Reads GH_AW_COPILOT_SDK_PROVIDER_WIRE_API and injects wireApi into the SDK provider config.
actions/setup/js/copilot_sdk_driver.test.cjs Asserts wireApi is forwarded into createSession provider config.
actions/setup/js/copilot_harness.cjs Captures resolved wireApi and exports it to the driver via env.
actions/setup/js/copilot_harness.test.cjs Updates expectations for resolved provider config to include wireApi.
actions/setup/js/awf_reflect.cjs Adds catalog lookup helper and introduces inferWireApiForModel for model-scoped wire API resolution.
actions/setup/js/awf_reflect.test.cjs Adds assertions for wireApi defaulting and explicit wire_api: responses from catalog metadata.
actions/setup/js/apply_safe_outputs_replay.cjs Refactors handler-config construction into a one-liner (no functional change).
.github/workflows/daily-model-inventory.md Documents validation/preservation rules for provider_type + wire_api in mirrored catalogs.
.github/workflows/daily-model-inventory.lock.yml Recompiled lockfile metadata after workflow markdown update.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 12/12 changed files
  • Comments generated: 3
  • Review effort level: Low

Comment on lines +445 to +458
const model = String(modelName || "").trim();
if (!model) return undefined;
const catalogEntry = getCatalogModelEntry(modelsJson, model);
const rawWireApi = catalogEntry && typeof catalogEntry.wire_api === "string" ? catalogEntry.wire_api : catalogEntry && typeof catalogEntry.wireApi === "string" ? catalogEntry.wireApi : "";
const normalizedWireApi = String(rawWireApi || "")
.toLowerCase()
.trim();
if (normalizedWireApi === "responses" || normalizedWireApi === "completions") {
return /** @type {"responses" | "completions"} */ normalizedWireApi;
}
if (providerType === "anthropic") {
return undefined;
}
return "completions";
Comment thread actions/setup/js/awf_reflect.cjs
provider: { type: "openai", baseUrl: "http://api-proxy:10002", wireApi: "responses" },
});
});

@github-actions github-actions Bot mentioned this pull request Jun 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

⚠️ Test Quality Score: 78/100 — Acceptable

Analyzed 7 test(s) across 3 JavaScript test files: 7 design, 0 implementation, 0 guideline violation(s).

📊 Metrics & Test Classification (7 tests analyzed)
Metric Value
New/modified tests analyzed 7
✅ Design tests (behavioral contracts) 7 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 2 (29%)
Duplicate test clusters 0
Test inflation detected No
🚨 Coding-guideline violations 0
Test File Classification Issues Detected
"resolves provider baseUrl and model from port when models_url is absent" actions/setup/js/awf_reflect.test.cjs:403 ✅ Design
"derives baseUrl from models_url origin when available" actions/setup/js/awf_reflect.test.cjs:439 ✅ Design
"uses openai type via model name heuristic on copilot endpoint when gpt model is selected" actions/setup/js/awf_reflect.test.cjs:466 ✅ Design
"uses modelsJson catalog for provider_type lookup" actions/setup/js/awf_reflect.test.cjs:484 ✅ Design
"uses wire_api from modelsJson when provided" ⭐ NEW actions/setup/js/awf_reflect.test.cjs:491 ✅ Design
"derives SDK custom provider and model from reflect data" actions/setup/js/copilot_harness.test.cjs:1715 ✅ Design
runWithCopilotSDK wireApi propagation actions/setup/js/copilot_sdk_driver.test.cjs:776 ✅ Design

Go: 0 (*_test.go); JavaScript: 7 (*.test.cjs). No other languages detected.

Test inflation ratios (additions: test / production):

Test File Test +lines Prod +lines Ratio Flag
awf_reflect.test.cjs 19 72 0.26:1
copilot_harness.test.cjs 1 4 0.25:1
copilot_sdk_driver.test.cjs 2 6 0.33:1

Score Breakdown

Component Value
Design tests (7/7 × 40) 40.0
Edge-case coverage (2/7 × 30) 8.6
Duplicate penalty (0 clusters) 20.0
Inflation bonus (none > 2:1) 10.0
Total 78.6 → 78

The score is depressed by limited explicit edge-case/error-path coverage among the changed tests. Tests 1–4 and 6 are straightforward assertion updates verifying the new wireApi field appears in happy-path outputs. The two edge-case credits go to: the new "uses wire_api from modelsJson when provided" test (verifies modelsJson wire_api overrides the default "completions" value), and the copilot_sdk_driver propagation test (uses result.exitCode === 0 plus createSession call assertion to verify end-to-end wire propagation via the external SDK).

Verdict

Check passed. 0% implementation tests (threshold: 30%). No guideline violations. All 7 analyzed tests verify observable behavior via toEqual() or result.exitCode assertions. The vi.fn() mock in copilot_sdk_driver.test.cjs targets the external Copilot SDK (acceptable external-I/O mocking) and is accompanied by a behavioral output assertion.

References: §28455342203

🧪 Test quality analysis by Test Quality Sentinel · 86.5 AIC · ⌖ 16.1 AIC · ⊞ 6.7K ·
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: 78/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%).

@github-actions

Copy link
Copy Markdown
Contributor

Skills-Based Review Summary 🧠

Applied /tdd and /codebase-designrequesting changes on 7 issues (5 test gaps, 2 code-quality).

📋 Issue breakdown

/tdd — Test Coverage Gaps

# Location Issue
1 awf_reflect.test.cjs:491 No direct unit tests for inferWireApiForModel()'s 3 resolution paths (null catalog, unknown value, anthropic fallback)
2 awf_reflect.test.cjs:504 getCatalogModelEntry() has no direct tests (case-insensitive match, cross-provider, null entry)
3 awf_reflect.cjs:452 Edge case untested: Anthropic model with explicit wire_api in catalog — does catalog win?
4 copilot_harness.cjs:751 No new test verifying GH_AW_COPILOT_SDK_PROVIDER_WIRE_API is exported with the correct resolved value
5 copilot_sdk_driver.cjs:109 main() env-var parsing path for GH_AW_COPILOT_SDK_PROVIDER_WIRE_API not covered by any test

/codebase-design — Code Quality

# Location Issue
6 awf_reflect.cjs:448 Redundant catalogEntry && guard + dead camelCase .wireApi fallback (Makefile normalises to snake_case)
7 apply_safe_outputs_replay.cjs:111 Unrelated formatting regression: 5-line readable block collapsed to a hard-to-scan 1-liner

@copilot please address the review comments above.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 97 AIC · ⌖ 11.4 AIC · ⊞ 6.5K ·
Comment /matt to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /tdd and /codebase-design — requesting changes on test coverage gaps and two code-quality issues.

📋 Key Themes & Highlights

Key Themes

  • Test coverage gaps (5 issues): The three resolution paths of inferWireApiForModel() and the getCatalogModelEntry() helper lack direct unit tests; the harness env-var propagation and the driver env-var parsing path are also untested. These gaps leave silent regression risk on a critical routing path.
  • Dead/redundant code (2 issues): A camelCase wireApi fallback in inferWireApiForModel() is unreachable after the Makefile normalises the catalog to snake_case. A redundant catalogEntry && guard is left after the extraction of getCatalogModelEntry(). And an unrelated formatting collapse in apply_safe_outputs_replay.cjs hurt readability.

Positive Highlights

  • getCatalogModelEntry() is a clean, well-documented DRY extraction — the refactor of inferProviderTypeForModel is noticeably simpler
  • ✅ Consistent end-to-end propagation: harness → env var → driver → createSession() is well-structured and matches the existing PROVIDER_TYPE / PROVIDER_BASE_URL pattern
  • ✅ JSDoc types on inferWireApiForModel() return type ("completions" | "responses" | undefined) are precise and helpful
  • ✅ Makefile jq correctly normalises both camelCase and snake_case wire_api from the upstream source — good defensive handling
  • daily-model-inventory.md guidance update is clear and actionable

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 97 AIC · ⌖ 11.4 AIC · ⊞ 6.5K
Comment /matt to run again

Comment thread actions/setup/js/awf_reflect.cjs Outdated
const model = String(modelName || "").trim();
if (!model) return undefined;
const catalogEntry = getCatalogModelEntry(modelsJson, model);
const rawWireApi = catalogEntry && typeof catalogEntry.wire_api === "string" ? catalogEntry.wire_api : catalogEntry && typeof catalogEntry.wireApi === "string" ? catalogEntry.wireApi : "";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] The rawWireApi assignment has two redundant catalogEntry && guards and a camelCase wireApi fallback that appears to be unreachable dead code.

Since getCatalogModelEntry() already validates its return as either null or a non-null non-array object, the second catalogEntry && is always truthy when the first succeeds. Additionally, models.json is always written with snake_case wire_api by the Makefile jq normalization, so the .wireApi camelCase branch is never reached at runtime.

💡 Suggested simplification
// Simplified — getCatalogModelEntry already validates the returned object
const rawWireApi = catalogEntry
  ? (typeof catalogEntry.wire_api === "string" ? catalogEntry.wire_api : "")
  : "";

If the camelCase .wireApi fallback is intentional (e.g. for injected catalog objects that bypass the Makefile pipeline), add a comment explaining why — otherwise remove it to avoid misleading readers.

@copilot please address this.

const normalizedWireApi = String(rawWireApi || "")
.toLowerCase()
.trim();
if (normalizedWireApi === "responses" || normalizedWireApi === "completions") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The resolution order between catalog wire_api and the Anthropic undefined fallback is not covered by a test — an Anthropic model with an explicit wire_api: "responses" in models.json would return "responses" here (catalog wins), which may or may not be intentional.

The PR description only documents the case where Anthropic is "omitted", but the code's branch ordering allows a catalog entry to override that.

💡 Suggested test to pin the decision
it("catalog wire_api wins over anthropic undefined fallback", () => {
  const reflectData = {
    endpoints: [{ provider: "anthropic", port: 10001, configured: true, models: ["claude-future"] }],
  };
  const modelsJson = {
    providers: {
      "github-copilot": { models: { "claude-future": { provider_type: "anthropic", wire_api: "responses", cost: {} } } },
    },
  };
  expect(resolveCopilotSDKCustomProviderFromReflect({ reflectData, model: "claude-future", modelsJson })).toEqual({
    model: "claude-future",
    // Assert whether wireApi: "responses" should be present or absent for Anthropic
    provider: { type: "anthropic", baseUrl: "(apiproxy/redacted) },
  });
});

If the catalog should never override the Anthropic fallback, swap the branch order or add an early guard.

@copilot please address this.

});
});

it("uses wire_api from modelsJson when provided", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Only one new test was added for inferWireApiForModel() behavior, and it's exercised indirectly through resolveCopilotSDKCustomProviderFromReflect. The three resolution paths and their edge cases lack direct unit tests.

Missing coverage:

  • wire_api value that isn't "responses" or "completions" (e.g. "grpc") → should silently fall through to the heuristic default
  • modelsJson is null → non-Anthropic model should still return "completions"
  • Anthropic provider with no catalog entry → should return undefined
💡 Suggested test structure (export `inferWireApiForModel` or test via the resolver)
describe("inferWireApiForModel (via resolver)", () => {
  it("returns completions for openai model with no catalog entry", () => {
    const result = resolveCopilotSDKCustomProviderFromReflect({
      reflectData: { endpoints: [{ provider: "copilot", port: 10002, configured: true, models: ["new-model"] }] },
      model: "new-model",
      modelsJson: null,
    });
    expect(result.provider.wireApi).toBe("completions");
  });

  it("ignores unrecognized wire_api value and falls back to heuristic", () => {
    const modelsJson = {
      providers: { "github-copilot": { models: { "gpt-x": { provider_type: "openai", wire_api: "grpc", cost: {} } } } },
    };
    const result = resolveCopilotSDKCustomProviderFromReflect({
      reflectData: { endpoints: [{ provider: "copilot", port: 10002, configured: true, models: ["gpt-x"] }] },
      model: "gpt-x",
      modelsJson,
    });
    expect(result.provider.wireApi).toBe("completions"); // falls through to heuristic
  });
});

@copilot please address this.

model: "gpt-5.5",
provider: { type: "openai", baseUrl: "http://api-proxy:10002", wireApi: "responses" },
});
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] getCatalogModelEntry() is a new, independently useful helper with several non-trivial behaviors, but it has no direct unit tests — only indirect coverage through higher-level resolver tests.

Edge cases worth pinning:

  • Model name matched case-insensitively ("GPT-5.5" matches "gpt-5.5")
  • Model name found in a secondary provider (not just the first one iterated)
  • modelsJson with a model entry that is null or an array → should return null
  • modelsJson is undefined or null → should return null gracefully

Direct tests here would also guard against accidental breakage if getCatalogModelEntry is later refactored.

@copilot please address this.

COPILOT_CONNECTION_TOKEN: copilotConnectionToken,
GH_AW_COPILOT_SDK_PROVIDER_BASE_URL: providerBaseUrl,
GH_AW_COPILOT_SDK_PROVIDER_TYPE: providerType,
GH_AW_COPILOT_SDK_PROVIDER_WIRE_API: providerWireApi,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] GH_AW_COPILOT_SDK_PROVIDER_WIRE_API is now injected into the child env, but copilot_harness.test.cjs has only one updated assertion (an existing test's expected value) — there is no new test verifying that the env var is actually exported with the resolved wireApi value.

A missing assertion here means a silent regression if the propagation from customProvider.provider.wireApi to sdkChildEnv is broken.

💡 Suggested assertion to add to an existing SDK mode harness test
// In a test that mocks resolveCopilotSDKCustomProviderFromReflect to return wireApi: "responses":
expect(spawnedChildEnv).toMatchObject({
  GH_AW_COPILOT_SDK_PROVIDER_WIRE_API: "responses",
});
// And verify that wireApi: "" is emitted (not omitted) when not set:
expect(spawnedChildEnv).toMatchObject({
  GH_AW_COPILOT_SDK_PROVIDER_WIRE_API: "",
});

@copilot please address this.

Comment thread actions/setup/js/copilot_sdk_driver.cjs Outdated
const rawWireApi = String(process.env.GH_AW_COPILOT_SDK_PROVIDER_WIRE_API || "")
.toLowerCase()
.trim();
const wireApi = rawWireApi === "responses" || rawWireApi === "completions" ? rawWireApi : undefined;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The main() function's GH_AW_COPILOT_SDK_PROVIDER_WIRE_API env-var reading and wireApi construction (lines 106–111) are not covered by any test. The driver tests exercise runWithCopilotSDK() directly with a pre-built provider object, so the env-var parsing path is skipped entirely.

A misbehaving env var (e.g. mixed-case "Responses" or a stray " responses ") would silently produce undefined — the .toLowerCase().trim() normalization is correct but untested.

💡 Test approaches
  • Add a small integration test that sets process.env.GH_AW_COPILOT_SDK_PROVIDER_WIRE_API and calls main() (stubbing SDK deps), asserting the provider passed to createSession includes wireApi: "responses".
  • Or extract the parsing into a named helper (e.g. parseWireApiEnv(raw)) and unit-test that helper directly.

@copilot please address this.

.filter(item => item.type && typeof item.type === "string")
.map(item => [item.type.replace(/-/g, "_"), {}])
);
const config = Object.fromEntries(validatedOutput.items.filter(item => item.type && typeof item.type === "string").map(item => [item.type.replace(/-/g, "_"), {}]));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] This change collapses a previously readable multi-line expression into a single long line with no functional difference — it appears to be an unintentional auto-formatter artifact unrelated to the PR's purpose.

The original 5-line form was more scannable; this 1-liner is harder to review and debug.

💡 Restore the prior formatting
const config = Object.fromEntries(
  validatedOutput.items
    .filter(item => item.type && typeof item.type === "string")
    .map(item => [item.type.replace(/-/g, "_"), {}])
);

@copilot please address this.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔎 Code quality review by PR Code Quality Reviewer · 93.3 AIC · ⌖ 7.79 AIC · ⊞ 1.6K
Comment /review to run again

.toLowerCase()
.trim();
if (normalizedWireApi === "responses" || normalizedWireApi === "completions") {
return /** @type {"responses" | "completions"} */ normalizedWireApi;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anthropic guard is bypassed when catalog has an explicit wire_api value — if any catalog entry sets wire_api: "completions" for an Anthropic model, the function returns "completions" and never reaches the providerType === "anthropic" check on line 455, violating the JSDoc contract and potentially injecting a wireApi field the Anthropic SDK is supposed to ignore (or could reject).

💡 Suggested fix

Move the Anthropic guard before the catalog lookup so it is unconditional:

function inferWireApiForModel(providerType, modelName, modelsJson) {
  // Anthropic SDK ignores wireApi; never emit it for Anthropic providers.
  if (providerType === "anthropic") return undefined;

  const model = String(modelName || "").trim();
  if (!model) return undefined;
  const catalogEntry = getCatalogModelEntry(modelsJson, model);
  const rawWireApi =
    catalogEntry && typeof catalogEntry.wire_api === "string"
      ? catalogEntry.wire_api
      : catalogEntry && typeof catalogEntry.wireApi === "string"
      ? catalogEntry.wireApi
      : "";
  const normalizedWireApi = String(rawWireApi || "").toLowerCase().trim();
  if (normalizedWireApi === "responses" || normalizedWireApi === "completions") {
    return normalizedWireApi;
  }
  return "completions";
}

This ensures no catalog misconfiguration can override the Anthropic constraint.

Comment thread actions/setup/js/awf_reflect.cjs Outdated

const providerType = inferProviderTypeForModel(String(endpoint.provider || ""), model, options?.modelsJson ?? null);
logger(`sdk-mode: custom provider resolved from awf-reflect (provider=${String(endpoint.provider || "unknown")} type=${providerType} baseUrl=${baseUrl} model=${model})`);
const wireApi = inferWireApiForModel(providerType, model, options?.modelsJson ?? null);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double catalog scan for the same model on the hot pathinferProviderTypeForModel (line 527) and inferWireApiForModel (line 528) each call getCatalogModelEntry internally, iterating over all providers and models twice for the same model string.

💡 Suggested refactor

Expose getCatalogModelEntry (or its result) as an explicit parameter or look it up once at the call site:

const catalogEntry = getCatalogModelEntry(options?.modelsJson ?? null, model);
const providerType = inferProviderTypeForModel(String(endpoint.provider || ""), model, catalogEntry);
const wireApi = inferWireApiForModel(providerType, model, catalogEntry);

This requires updating both inferProviderTypeForModel and inferWireApiForModel to accept a pre-fetched entry instead of raw modelsJson. For a small catalog the overhead is negligible today, but the duplicate scan grows linearly with catalog size and runs on every session creation.

};
expect(resolveCopilotSDKCustomProviderFromReflect({ reflectData, model: "gpt-5.5", modelsJson })).toEqual({
model: "gpt-5.5",
provider: { type: "openai", baseUrl: "http://api-proxy:10002", wireApi: "responses" },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test: Anthropic model with explicit catalog wire_api should not produce wireApi in provider config — the existing test suite verifies the happy path (wire_api: "responses"wireApi: "responses") but has no case where an Anthropic catalog entry has an explicit wire_api. This gap means the bug in inferWireApiForModel (catalog check before Anthropic guard) is untestable with the current suite.

💡 Suggested test to add
it("omits wireApi for Anthropic models even when catalog has wire_api", () => {
  const reflectData = {
    endpoints: [{ provider: "anthropic", port: 10002, configured: true, models: ["claude-opus-5"] }],
  };
  const modelsJson = {
    providers: {
      "github-copilot": {
        models: { "claude-opus-5": { provider_type: "anthropic", wire_api: "completions", cost: {} } },
      },
    },
  };
  const result = resolveCopilotSDKCustomProviderFromReflect({
    reflectData,
    model: "claude-opus-5",
    modelsJson,
  });
  expect(result?.provider.wireApi).toBeUndefined();
});

@github-actions

Copy link
Copy Markdown
Contributor

Review summary

The overall approach — model-scoped wireApi via registry metadata propagated from harness → driver — is well-structured. One correctness issue needs to be addressed before merge.

🔴 Blocking: provider-agnostic catalog lookup with duplicate model names

getCatalogModelEntry scans all providers and returns the first match. 9 models (gpt-5-mini, gpt-5.2, gpt-5.4, gpt-5.5, gpt-4.1, gpt-5.4-nano, gpt-5.3-codex, gpt-5.2-codex, gpt-5.4-mini) exist in both github-copilot (with wire_api) and openai (without wire_api). Correct resolution today depends on github-copilot appearing before openai in the JSON insertion order — a fragile implicit coupling to models.dev source ordering. If that order ever changes, those 9 models silently fall back to "completions" regardless of catalog intent. See inline comment on awf_reflect.cjs:364.

🟡 Non-blocking suggestions

  • Env-var parsing untested (copilot_sdk_driver.cjs:106): The new GH_AW_COPILOT_SDK_PROVIDER_WIRE_API parsing in main() is not covered; see inline comment.
  • Nested ternary (awf_reflect.cjs:448): the dual wire_api/wireApi lookup on a single line is hard to read; see inline comment.

References: §28455342224

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 111.7 AIC · ⌖ 7.23 AIC · ⊞ 4.9K ·

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 111.7 AIC · ⌖ 7.23 AIC · ⊞ 4.9K

Comment thread actions/setup/js/awf_reflect.cjs Outdated
if (!providers || typeof providers !== "object" || Array.isArray(providers)) {
return null;
}
for (const providerData of Object.values(providers)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: first-match-wins across providers for duplicate model names.

getCatalogModelEntry iterates Object.values(providers) and returns the first matching model entry, regardless of which provider contains it. There are currently 9 models (gpt-5-mini, gpt-5.2, gpt-5.4, gpt-5.5, gpt-4.1, gpt-5.4-nano, gpt-5.3-codex, gpt-5.2-codex, gpt-5.4-mini) present in both github-copilot (with wire_api) and openai (without wire_api).

Correctness currently depends on the JSON insertion order (github-copilot appearing before openai) being preserved by the upstream source and the jq transform. If models.dev or the jq pipeline ever returns providers in a different order, those 9 models would silently resolve with no wire_api entry, falling back to "completions" even for models that require "responses".

Suggested fix: Accept an optional providerName parameter so the lookup is deterministic regardless of insertion order:

function getCatalogModelEntry(modelsJson, modelName, providerName = null) {
  // ... existing guards ...
  const providers = modelsJson.providers;
  for (const [name, providerData] of Object.entries(providers)) {
    if (providerName && name !== providerName) continue;
    // ... existing inner loop ...
  }
  return null;
}

Then call getCatalogModelEntry(modelsJson, model, "github-copilot") in inferWireApiForModel.

@copilot please address this.

Comment thread actions/setup/js/awf_reflect.cjs Outdated
const model = String(modelName || "").trim();
if (!model) return undefined;
const catalogEntry = getCatalogModelEntry(modelsJson, model);
const rawWireApi = catalogEntry && typeof catalogEntry.wire_api === "string" ? catalogEntry.wire_api : catalogEntry && typeof catalogEntry.wireApi === "string" ? catalogEntry.wireApi : "";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainability: complex nested ternary is hard to read.

The single-line nested ternary for rawWireApi is difficult to scan and reason about:

const rawWireApi = catalogEntry && typeof catalogEntry.wire_api === "string" ? catalogEntry.wire_api : catalogEntry && typeof catalogEntry.wireApi === "string" ? catalogEntry.wireApi : "";

Consider extracting the dual-key lookup into a small helper or using a clearer fallback chain:

const rawWireApi = (catalogEntry && (catalogEntry.wire_api ?? catalogEntry.wireApi)) || "";

Or, if strict string-type checking is required:

const rawWireApi =
  typeof catalogEntry?.wire_api === "string" ? catalogEntry.wire_api
  : typeof catalogEntry?.wireApi === "string" ? catalogEntry.wireApi
  : "";

@copilot please address this.

Comment thread actions/setup/js/copilot_sdk_driver.cjs Outdated
/** @type {"openai" | "azure" | "anthropic"} */
const providerType = rawProviderType === "anthropic" || rawProviderType === "azure" ? rawProviderType : "openai";
log(`provider type: ${providerType}`);
const rawWireApi = String(process.env.GH_AW_COPILOT_SDK_PROVIDER_WIRE_API || "")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing unit test for env-var parsing path.

The new GH_AW_COPILOT_SDK_PROVIDER_WIRE_API env-var reading and validation (lines 106–111) is not covered by a unit test. The existing test in copilot_sdk_driver.test.cjs validates wireApi passthrough by calling runWithCopilotSDK directly with a provider object — but it bypasses the env-var parsing in main().

Concretely, the following cases are untested end-to-end:

  • GH_AW_COPILOT_SDK_PROVIDER_WIRE_API=responseswireApi: "responses" in ProviderConfig
  • GH_AW_COPILOT_SDK_PROVIDER_WIRE_API= (empty/unset) → no wireApi in ProviderConfig
  • Invalid value (e.g., "chat") → should be silently ignored (no wireApi)

Consider adding a test that invokes the env-var code path, or at minimum exporting a parseProviderFromEnv() helper that can be unit-tested directly.

@copilot please address this.

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot run pr-finisher skill

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@copilot run pr-finisher skill

Addressed the actionable review follow-ups in 96327a3. Local validation passed (make fmt, make lint, make test-unit, make test, targeted JS tests, and make agent-report-progress). CI on this head is still unverified because agent pushes do not re-trigger workflows.

@pelikhan pelikhan merged commit d82b666 into main Jun 30, 2026
37 checks passed
@pelikhan pelikhan deleted the copilot/implement-multiple-provider-configuration branch June 30, 2026 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants