refactor: extract shared multi-provider JSON helpers to eliminate duplicate code#43329
Conversation
…licate code Move isValidProviderConfig, isValidModelConfig, and parseMultiProviderJson into a new copilot_sdk_multi_provider.cjs shared module. Both copilot_sdk_driver.cjs and the sample driver (.github/drivers/copilot_sdk_driver_sample_node.cjs) now import from this single source of truth, eliminating the drift risk when the shape of GH_AW_COPILOT_SDK_MULTI_PROVIDER_JSON changes. Closes #43328 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Score breakdown: Impact 15 (dedup reduces drift risk in multi-provider JSON helpers) + Urgency 10 + Quality 10 (no tests, draft state) Extracts shared multi-provider JSON helpers from copilot_sdk_driver.cjs and sample driver into a single module. +70/-66 lines. Currently DRAFT — no reviews yet. Low-priority refactor, suitable for batched review.
|
There was a problem hiding this comment.
Pull request overview
This PR refactors Copilot SDK multi-provider parsing/validation so both the production driver and the Node sample driver share a single implementation, reducing the risk of logic drift when the GH_AW_COPILOT_SDK_MULTI_PROVIDER_JSON shape evolves.
Changes:
- Added a shared CommonJS helper module for multi-provider JSON parsing and minimal shape validation.
- Updated the production Copilot SDK driver to import (and re-export)
parseMultiProviderJsonfrom the shared module. - Updated the Node sample driver to import the shared helpers and re-export them to preserve its existing
module.exportssurface.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/copilot_sdk_multi_provider.cjs | New shared parsing/validation helpers for multi-provider JSON env var. |
| actions/setup/js/copilot_sdk_driver.cjs | Removes duplicated helper implementations; imports/re-exports shared parser. |
| .github/drivers/copilot_sdk_driver_sample_node.cjs | Removes duplicated helper implementations; imports shared helpers and re-exports them. |
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: 2
- Review effort level: Low
| * Returns `null` when the env var is unset or contains invalid JSON. | ||
| * On success returns `{ model, providers, models }` where the shapes match the | ||
| * Copilot SDK `NamedProviderConfig` / `ProviderModelConfig` types. |
| function parseMultiProviderJson(value) { | ||
| if (!value) return null; | ||
| try { | ||
| const parsed = JSON.parse(value); | ||
| if (!parsed || typeof parsed !== "object") return null; |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43329 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting with suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Missing test file: The new
copilot_sdk_multi_provider.cjsmodule has no paired*.test.cjs, breaking the consistent testing pattern of this directory. As the single source of truth, it should be the most tested file in this PR. - Over-exported surface:
isValidProviderConfigandisValidModelConfigare internal predicates that don't need to be part of the public API. Exporting them (and re-exporting from the sample driver) unnecessarily widens the contract. - Fragile relative path: The cross-directory
../../actions/setup/js/import in the sample driver is invisible to future movers.
Positive Highlights
- ✅ Excellent motivation: eliminates a real drift risk between production and sample drivers
- ✅ Backward compatibility is well-handled — both drivers re-export
parseMultiProviderJsonso existing callers are unaffected - ✅ Good JSDoc on the shared function, including typed return value
- ✅ The
//@ts-check`` pragma is a nice touch for a.cjsfile - ✅ Clean, minimal PR scope — only touches what needs to change
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 45.1 AIC · ⌖ 5.96 AIC · ⊞ 6.6K
Comment /matt to run again
| isValidProviderConfig, | ||
| isValidModelConfig, | ||
| parseMultiProviderJson, | ||
| }; |
There was a problem hiding this comment.
[/tdd] No paired test file for the new shared module — this is the single source of truth for multi-provider validation, and it currently has zero test coverage.
💡 Suggested test file: copilot_sdk_multi_provider.test.cjs
Every other module in actions/setup/js/ has a paired *.test.cjs. A minimal suite should cover: null/empty/invalid-JSON inputs, missing or empty providers/models arrays, invalid provider shape (missing baseUrl), invalid model shape, and a valid round-trip that confirms the model string is trimmed.
@copilot please address this.
|
|
||
| module.exports = { | ||
| isValidProviderConfig, | ||
| isValidModelConfig, |
There was a problem hiding this comment.
[/codebase-design] isValidProviderConfig and isValidModelConfig are implementation details of parseMultiProviderJson but are exported publicly. Exporting them widens the module's surface unnecessarily and invites callers to rely on the internal validation predicates directly.
💡 Suggestion
Keep the two helpers private (unexported) unless there is a concrete caller outside this module that needs them. If they genuinely need to be public, document their intended use case. The sample driver already only imports all three — but its module.exports re-exports them too, compounding the surface widening.
@copilot please address this.
| "use strict"; | ||
|
|
||
| const fs = require("node:fs"); | ||
| const { isValidProviderConfig, isValidModelConfig, parseMultiProviderJson } = require("../../actions/setup/js/copilot_sdk_multi_provider.cjs"); |
There was a problem hiding this comment.
[/codebase-design] The relative path ../../actions/setup/js/copilot_sdk_multi_provider.cjs is fragile — if either file moves, the link silently breaks at runtime with no compile-time signal.
💡 Suggestion
Consider documenting why the relative path is used (e.g. in a comment) so future movers know this dependency exists. Alternatively, if the repo ever adopts a workspace/package structure, a named package import would be more robust. At minimum, a comment like // shared with actions/setup/js — do not move without updating this path keeps the coupling visible.
@copilot please address this.
There was a problem hiding this comment.
Request changes: 3 blocking issues in the new shared module
The refactor is sound — eliminating the duplicate is the right call. However, copilot_sdk_multi_provider.cjs introduces three correctness/maintainability issues that should be fixed before this is the authoritative source of truth for multi-provider config validation.
Blocking findings
- Empty strings pass
isValidProviderConfig/isValidModelConfig(high) —{ name: "", baseUrl: "" }passes validation and causes an opaque SDK error at connection time. Add.length > 0guards on every string field. - Silent
catch { return null; }(high) — all error context is lost;process.env.GH_AW_COPILOT_SDK_MULTI_PROVIDER_JSONmisconfiguration becomes invisible. Log the caught error to stderr before returningnull. model: ""returned when field is absent (medium) — the implicit""== 'not set' contract works today only because callers use|| undefined. Update the return type tostring | nulland returnnullexplicitly so future callers have an unambiguous signal.
Also note: existing review threads already flag the JSDoc accuracy on null semantics (line 35) and missing unit tests (line 48) — those should be addressed too.
🔎 Code quality review by PR Code Quality Reviewer · 87.3 AIC · ⌖ 6.69 AIC · ⊞ 5.4K
Comment /review to run again
| * @returns {boolean} | ||
| */ | ||
| function isValidProviderConfig(p) { | ||
| return p && typeof p.name === "string" && typeof p.type === "string" && typeof p.baseUrl === "string"; |
There was a problem hiding this comment.
Empty strings pass validation in isValidProviderConfig and isValidModelConfig: a provider with { name: "", type: "", baseUrl: "" } passes the shape check and reaches the Copilot SDK, causing opaque connection or URL errors far from the validation site.
💡 Suggested fix
function isValidProviderConfig(p) {
return (
p &&
typeof p.name === "string" && p.name.length > 0 &&
typeof p.type === "string" && p.type.length > 0 &&
typeof p.baseUrl === "string" && p.baseUrl.length > 0
);
}
function isValidModelConfig(m) {
return (
m &&
typeof m.id === "string" && m.id.length > 0 &&
typeof m.provider === "string" && m.provider.length > 0
);
}Non-empty checks are cheap and surface misconfigured GH_AW_COPILOT_SDK_MULTI_PROVIDER_JSON at the validation boundary rather than as a cryptic SDK runtime failure.
| if (!parsed.models.every(isValidModelConfig)) return null; | ||
| const model = typeof parsed.model === "string" ? parsed.model.trim() : ""; | ||
| return { model, providers: parsed.providers, models: parsed.models }; | ||
| } catch { |
There was a problem hiding this comment.
Silent catch destroys all diagnostic information: every error — malformed JSON, unexpected TypeError, future bugs in validators — collapses to null with no log output. This is now the single source of truth for multi-provider parsing; when it silently returns null, the caller emits only a generic 'invalid' error message and the root cause is invisible.
💡 Suggested fix
} catch (err) {
// Log to stderr so operator errors (malformed JSON, wrong shape) are diagnosable
process.stderr.write(
`[copilot-sdk-multi-provider] failed to parse GH_AW_COPILOT_SDK_MULTI_PROVIDER_JSON: ${err}\n`
);
return null;
}Alternatively, accept an optional logger callback so callers can control where the diagnostic goes — useful for test isolation.
| // Validate minimal shape: providers must have name/type/baseUrl, models must have id/provider | ||
| if (!parsed.providers.every(isValidProviderConfig)) return null; | ||
| if (!parsed.models.every(isValidModelConfig)) return null; | ||
| const model = typeof parsed.model === "string" ? parsed.model.trim() : ""; |
There was a problem hiding this comment.
model: "" when the field is absent creates an implicit, fragile contract: the function returns model: "" when parsed.model is missing or not a string. The only documented return type is { model: string, ... } with no indication that "" means 'not set'. The production caller handles this via process.env.COPILOT_MODEL || multiProviderConfig.model || undefined — correctly, but only because "" is falsy. A future caller that checks model !== undefined or model !== null will silently use the wrong value.
💡 Suggested fix
// Return null (or omit the field) when model is not set
const model = typeof parsed.model === "string" && parsed.model.trim() ? parsed.model.trim() : null;
return { model, providers: parsed.providers, models: parsed.models };And update the JSDoc return type:
* `@returns` {{ model: string | null, providers: ..., models: ... } | null}
This makes the absence of a model field explicit rather than encoding it as an empty string.
There was a problem hiding this comment.
Review: refactor — extract shared multi-provider JSON helpers
Good refactor — the duplication elimination is correct and the module boundary is sensible. Two issues need to be addressed before merge.
🔴 Blocking
-
No tests for the new shared module (
copilot_sdk_multi_provider.cjs)
This module is now the single source of truth for multi-provider config validation affecting both drivers. Every other siblingcopilot_sdk_*.cjshas a test file. A regression here silently breaks both the production and sample drivers. See inline comment on line 65. -
Inaccurate
@returns {boolean}annotation onisValidProviderConfig/isValidModelConfig
The//@ts-check`` directive is present, so TypeScript will surface this. Whenpis falsy the function returns `p` (e.g. `null`) not `false`. Fix with `!!p &&`. See inline comment on line 19.
🟡 Non-blocking
- Brittle cross-tree relative require in
.github/drivers/
../../actions/setup/js/copilot_sdk_multi_provider.cjswill silently break if either directory moves. Consider importing through the production driver re-export or document the structural assumption. See inline comment on line 5 of the sample driver.
What's good
- Identical logic correctly extracted with zero semantic change
- Re-exports from both drivers preserve backward compatibility for all existing callers
- JSDoc is thorough in the new shared module
- Header comment clearly describes consumers and purpose
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 71.9 AIC · ⌖ 5.73 AIC · ⊞ 4.8K
| * @returns {boolean} | ||
| */ | ||
| function isValidProviderConfig(p) { | ||
| return p && typeof p.name === "string" && typeof p.type === "string" && typeof p.baseUrl === "string"; |
There was a problem hiding this comment.
The @returns {boolean} JSDoc annotation is inaccurate: when p is falsy, p && ... short-circuits and returns p itself (e.g. null, undefined) rather than false.\n\nWith // @ts-check`` enabled, TypeScript will flag callers that depend on the strict boolean type. Though `.every()` coerces truthy/falsy correctly today, the imprecise annotation is a latent maintenance risk.\n\nConsider:\njs\nfunction isValidProviderConfig(p) {\n return !!p && typeof p.name === 'string' && typeof p.type === 'string' && typeof p.baseUrl === 'string';\n}\n\n\nSame applies to `isValidModelConfig`.\n\n@copilot please address this.
| isValidProviderConfig, | ||
| isValidModelConfig, | ||
| parseMultiProviderJson, | ||
| }; |
There was a problem hiding this comment.
No test file exists for the new copilot_sdk_multi_provider.cjs module. The repository has 362 test files, and every other copilot_sdk_*.cjs sibling has coverage (e.g. copilot_sdk_driver.test.cjs). The extracted logic is the single source of truth for multi-provider config validation, so regressions here would silently affect both drivers.\n\nPlease add a copilot_sdk_multi_provider.test.cjs covering at least:\n- parseMultiProviderJson(undefined) → null\n- Invalid JSON → null\n- Missing providers/models → null\n- Invalid provider shape → null\n- Valid input → correct { model, providers, models }\n\n@copilot please address this.
| "use strict"; | ||
|
|
||
| const fs = require("node:fs"); | ||
| const { isValidProviderConfig, isValidModelConfig, parseMultiProviderJson } = require("../../actions/setup/js/copilot_sdk_multi_provider.cjs"); |
There was a problem hiding this comment.
The require() path ../../actions/setup/js/copilot_sdk_multi_provider.cjs is a brittle relative path from a .github/drivers/ file into actions/setup/js/. This will silently break if either directory is moved or if the sample driver is copied elsewhere.\n\nThe production driver in actions/setup/js/ uses a safe local ./copilot_sdk_multi_provider.cjs reference. Consider:\n1. Embedding the sample driver's helpers via a thin wrapper re-export in the production driver (already done for parseMultiProviderJson), and importing from the production driver instead; or\n2. Adding a path-integrity test that verifies the relative path resolves correctly.\n\nAt minimum, add a comment explaining the expected repo layout so a future directory refactor doesn't silently break this.\n\n@copilot please address this.
isValidProviderConfig,isValidModelConfig, andparseMultiProviderJsonwere copy-pasted identically between the production driver and the Node sample driver, creating a drift risk whenever theGH_AW_COPILOT_SDK_MULTI_PROVIDER_JSONshape changes.Changes
actions/setup/js/copilot_sdk_multi_provider.cjs— single source of truth for multi-provider config parsing and shape validationcopilot_sdk_driver.cjs— removes the three inline implementations; imports from the shared module; re-exportsparseMultiProviderJsonunchanged socopilot_harness.cjscallers are unaffected.github/drivers/copilot_sdk_driver_sample_node.cjs— same removal; imports via../../actions/setup/js/copilot_sdk_multi_provider.cjs; re-exports all three helpers to preserve its existingmodule.exportscontractBoth drivers now resolve to the same function identity: