refactor: extract shared multi-provider JSON helpers to eliminate duplicate code #43329
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| // @ts-check | ||
|
|
||
| /** | ||
| * Copilot SDK Multi-Provider Helpers | ||
| * | ||
| * Shared parsing and validation utilities for the GH_AW_COPILOT_SDK_MULTI_PROVIDER_JSON | ||
| * environment variable. Consumed by both the production SDK driver | ||
| * (copilot_sdk_driver.cjs) and any custom driver (e.g. the Node sample driver | ||
| * under .github/drivers/) so that the validation logic stays in one place. | ||
| */ | ||
|
|
||
| "use strict"; | ||
|
|
||
| /** | ||
| * @param {any} p | ||
| * @returns {boolean} | ||
| */ | ||
| function isValidProviderConfig(p) { | ||
| return p && typeof p.name === "string" && typeof p.type === "string" && typeof p.baseUrl === "string"; | ||
|
Contributor
There was a problem hiding this comment. Empty strings pass validation in 💡 Suggested fixfunction 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
Contributor
There was a problem hiding this comment. The |
||
| } | ||
|
|
||
| /** | ||
| * @param {any} m | ||
| * @returns {boolean} | ||
| */ | ||
| function isValidModelConfig(m) { | ||
| return m && typeof m.id === "string" && typeof m.provider === "string"; | ||
| } | ||
|
|
||
| /** | ||
| * Parse the GH_AW_COPILOT_SDK_MULTI_PROVIDER_JSON env var. | ||
| * | ||
| * 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. | ||
|
Comment on lines
+33
to
+35
|
||
| * | ||
| * @param {string | undefined} value | ||
| * @returns {{ | ||
| * model: string, | ||
| * providers: import("@github/copilot-sdk").NamedProviderConfig[], | ||
| * models: import("@github/copilot-sdk").ProviderModelConfig[], | ||
| * } | null} | ||
| */ | ||
| function parseMultiProviderJson(value) { | ||
| if (!value) return null; | ||
| try { | ||
| const parsed = JSON.parse(value); | ||
| if (!parsed || typeof parsed !== "object") return null; | ||
|
Comment on lines
+44
to
+48
|
||
| if (!Array.isArray(parsed.providers) || parsed.providers.length < 1) return null; | ||
| if (!Array.isArray(parsed.models) || parsed.models.length < 1) return null; | ||
| // 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() : ""; | ||
|
Contributor
There was a problem hiding this comment.
💡 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: This makes the absence of a model field explicit rather than encoding it as an empty string. |
||
| return { model, providers: parsed.providers, models: parsed.models }; | ||
| } catch { | ||
|
Contributor
There was a problem hiding this comment. Silent 💡 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. |
||
| return null; | ||
| } | ||
| } | ||
|
|
||
| module.exports = { | ||
| isValidProviderConfig, | ||
| isValidModelConfig, | ||
|
Contributor
There was a problem hiding this comment. [/codebase-design] 💡 SuggestionKeep 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 @copilot please address this. |
||
| parseMultiProviderJson, | ||
| }; | ||
|
Contributor
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:
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/codebase-design] The relative path
../../actions/setup/js/copilot_sdk_multi_provider.cjsis 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 pathkeeps the coupling visible.@copilot please address this.