Refactor MCP gateway converters into shared profiles#53503
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors MCP gateway converters into shared factory and profile modules while preserving engine-specific exports and behavior.
Changes:
- Adds reusable profile-to-conversion factory logic.
- Centralizes Copilot, Codex, Claude, and Gemini conversion rules.
- Converts engine scripts into thin wrappers and adds focused tests.
Show a summary per file
| File | Description |
|---|---|
convert_gateway_config_factory.cjs |
Builds and executes shared conversion options. |
convert_gateway_config_factory.test.cjs |
Tests factory behavior and error handling. |
convert_gateway_config_profiles.cjs |
Defines centralized engine conversion profiles. |
convert_gateway_config_profiles.test.cjs |
Verifies supported profile declarations. |
convert_gateway_config_copilot.cjs |
Uses the Copilot profile. |
convert_gateway_config_codex.cjs |
Uses the Codex profile. |
convert_gateway_config_claude.cjs |
Uses the Claude profile. |
convert_gateway_config_gemini.cjs |
Uses the Gemini profile. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 8/8 changed files
- Comments generated: 0
- Review effort level: Balanced
|
@copilot Please refresh this branch if needed, re-check for any unresolved review concerns, then run the
|
Refreshed against |
PR TriageCategory: refactor · Risk: medium · Score: 47/100 Breakdown: impact 25/50 · urgency 10/30 · quality 12/20 Recommended action: Consolidates per-engine MCP gateway config converters (claude/codex/copilot/gemini) into a shared profile factory (+324/-245, 8 files). CI green. Only a bot COMMENTED review so far — worth a careful pass since it touches all engine converters at once. Batching with related setup-JS refactor PRs.
|
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![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 /codebase-design and /tdd — requesting changes on a few actionable issues.
📋 Key Themes & Highlights
Issues
- Dead imports —
rewriteUrlimported but unused in bothcopilot.cjsandgemini.cjsafter the refactor. - Side effect in a data module —
core.info(...)called inside the codex profile'sgetUrlPrefix, coupling logging to a pure data object. - Eager vs lazy
outputPath—codexandclaudesnapshotRUNNER_TEMPat module-load time;copilotuses a lazy thunk. The inconsistency can cause subtle test isolation bugs. - Thin transform coverage —
profiles.test.cjschecks profile shape but doesn't test thetransformEntryfunctions that are the behaviorally meaningful part of this refactor.
Positive Highlights
- ✅ Clean extraction into a single profile table — auditing per-engine differences is now much easier.
- ✅ Factory correctly handles both
transformEntryandtransformServerforms without breaking existing callers. - ✅
setFailedOnErrordelegation to the factory is a nice isolation of thecore.setFailedcoupling. - ✅ Good test coverage in
factory.test.cjsincluding the error-reporting path.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 58.6 AIC · ⌖ 10.8 AIC · ⊞ 7.8K
Comment /matt to run again
| } | ||
| }); | ||
| } | ||
| const { rewriteUrl } = require("./convert_gateway_config_shared.cjs"); |
There was a problem hiding this comment.
[/codebase-design] rewriteUrl is imported here but never referenced in this file after the refactor — dead import that misleads readers.
💡 Fix
Remove line 6:
const { rewriteUrl } = require("./convert_gateway_config_shared.cjs");URL rewriting now happens inside profiles.cjs. The same dead import exists in convert_gateway_config_gemini.cjs.
@copilot please address this.
There was a problem hiding this comment.
Kept this import because rewriteUrl remains part of this entrypoint's pre-existing public module exports. Removing it would make module.exports.rewriteUrl undefined and break compatibility.
| function getGeminiHostDomain() { | ||
| return process.env.MCP_GATEWAY_HOST_DOMAIN || "localhost"; | ||
| } | ||
| const { rewriteUrl } = require("./convert_gateway_config_shared.cjs"); |
There was a problem hiding this comment.
[/codebase-design] Same dead import as in convert_gateway_config_copilot.cjs — rewriteUrl is imported but never called here after the refactor.
💡 Fix
Remove line 6:
const { rewriteUrl } = require("./convert_gateway_config_shared.cjs");@copilot please address this.
There was a problem hiding this comment.
Kept this import because rewriteUrl remains part of this entrypoint's pre-existing public module exports. Removing it would make module.exports.rewriteUrl undefined and break compatibility.
| outputPath: path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/mcp-servers.json"), | ||
| transformEntry: transformClaudeEntry, | ||
| serialize: servers => JSON.stringify({ mcpServers: servers }, null, 2), | ||
| }, |
There was a problem hiding this comment.
[/codebase-design] core.info(...) is called inside the codex profile's getUrlPrefix function — this embeds a side effect (logging) deep in a data definition, making the profile hard to test without a real core shim.
💡 Suggested fix
Move the log call into the factory or into the codex entrypoint main(), not inside a data-tier function:
// In convert_gateway_config_factory.cjs or convert_gateway_config_codex.cjs
getUrlPrefix: ({ domain, port }) => {
if (domain === "host.docker.internal") {
// Log happens at the call site, not buried in the profile object
return `(172.30.0.1/redacted)
}
return `(redacted)
},Or accept core as a dependency parameter on the factory so the profile stays pure.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 1a9e742: Codex URL resolution is now pure in the profile; the shared factory emits the preserved resolution log from the profile's message provider. Added factory coverage for this behavior.
| return toml; | ||
| }, | ||
| }, | ||
| claude: { |
There was a problem hiding this comment.
[/codebase-design] outputPath for codex and claude is evaluated eagerly at module-load time (path.join(process.env.RUNNER_TEMP || "/tmp", ...)), but copilot uses preRunOutputPath for lazy resolution at run time. This asymmetry means importing profiles.cjs in a test will snapshot the value of RUNNER_TEMP at import time, potentially diverging from the value set later in the test.
💡 Suggested fix
Make outputPath for codex and claude also a thunk (or use preRunOutputPath) to match the copilot pattern:
codex: {
...
preRunOutputPath: () => path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/config.toml"),
...
},
claude: {
...
preRunOutputPath: () => path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/mcp-servers.json"),
...
},This makes all profiles consistently lazy and avoids env-snapshot bugs in tests.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 1a9e742: Codex and Claude now resolve RUNNER_TEMP through preRunOutputPath at conversion time. Added coverage that changes RUNNER_TEMP after module import.
| expect(gatewayConversionProfiles.claude).toMatchObject({ format: "Claude", engine: "Claude" }); | ||
| expect(gatewayConversionProfiles.gemini).toMatchObject({ format: "Gemini", engine: "Gemini", contextOptions: { extraRequiredEnv: ["GITHUB_WORKSPACE"] } }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The profiles test checks that profiles exist and have the right shape, but no test exercises the transformEntry / transformServer functions exported from profiles.cjs (e.g., that transformClaudeEntry sets type: "http" and deletes tools, or that transformGeminiEntry removes type). These are the behaviorally interesting parts of the refactor and the most likely place for regressions.
💡 Suggested additions
it("transformClaudeEntry sets type:http and removes tools", () => {
const result = transformClaudeEntry({ url: "(host/redacted) tools: ["*"] }, "(host/redacted)
expect(result.type).toBe("http");
expect(result).not.toHaveProperty("tools");
});
it("transformGeminiEntry removes type field", () => {
const result = transformGeminiEntry({ url: "(host/redacted) type: "http" }, "(host/redacted)
expect(result).not.toHaveProperty("type");
});
it("transformCopilotEntry adds tools:['*'] when missing", () => {
const result = transformCopilotEntry({ url: "(host/redacted) }, "(host/redacted)
expect(result.tools).toEqual(["*"]);
});@copilot please address this.
There was a problem hiding this comment.
Addressed in 1a9e742: added direct coverage for Copilot defaults, Claude HTTP/tool transformation, Gemini type removal, and URL rewriting.
|
@copilot Please address the unresolved review feedback below, refresh the branch if needed, then run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Please review the unresolved feedback below, refresh the branch if needed, then run the
|
Branch is refreshed and already contains |
|
@copilot Please address the unresolved review feedback below, refresh the branch if needed, then run the
|
…e-remove-repeated-scaffolds Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Refreshed with current |
The engine-specific MCP gateway converter scripts repeated the same wrapper logic around shared conversion. This made per-engine differences harder to audit and increased drift risk.
Shared converter factory
convert_gateway_config_factory.cjsto build and runrunGatewayConversion(...)options from declarative profiles.main()scaffolding.Centralized engine profiles
convert_gateway_config_profiles.cjsfor Copilot, Codex, Claude, and Gemini rules.Thin engine entrypoints
convert_gateway_config_*.cjsscript to a profile-backed wrapper.transformCopilotEntry,toCodexTomlSection, andmain.Example profile shape:
run: https://github.com/github/gh-aw/actions/runs/32080816855> Generated by 👨🍳 PR Sous Chef · gpt54 · 10.1 AIC · ⌖ 7.87 AIC · ⊞ 8.8K · ◷
Run: https://github.com/github/gh-aw/actions/runs/32087856389> Generated by 👨🍳 PR Sous Chef · gpt54 · 10 AIC · ⌖ 8.08 AIC · ⊞ 8.8K · ◷