Deduplicate gateway configuration conversion adapters - #50701
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #50701 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Centralizes gateway configuration conversion while preserving engine-specific adapters.
Changes:
- Adds a shared conversion runner.
- Refactors four engine converters to use callbacks.
- Adds runner-level pipeline coverage.
Show a summary per file
| File | Description |
|---|---|
convert_gateway_config_shared.cjs |
Adds shared runner. |
convert_gateway_config_shared.test.cjs |
Tests shared pipeline. |
convert_gateway_config_claude.cjs |
Adapts Claude conversion. |
convert_gateway_config_codex.cjs |
Adapts Codex TOML conversion. |
convert_gateway_config_copilot.cjs |
Adapts Copilot conversion. |
convert_gateway_config_gemini.cjs |
Adapts Gemini conversion. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| getUrlPrefix: ({ domain, port }) => { | ||
| if (domain === "host.docker.internal") { | ||
| core.info("Resolving host.docker.internal to gateway IP: 172.30.0.1"); | ||
| return `http://172.30.0.1:${port}`; |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics (24 tests)
|
There was a problem hiding this comment.
Review: Deduplicate gateway configuration conversion adapters
The refactoring is clean and correct. The shared runGatewayConversion runner faithfully preserves all engine-specific behaviours (Codex host resolution, Copilot HOME error handling, Gemini host-domain selection) and the security-sensitive 0o600 write path. Tests are well-structured.
One minor log-ordering change (non-blocking): In the Codex adapter, getUrlPrefix now emits core.info("Resolving host.docker.internal...") after the standard header lines (Converting..., Input:, Target domain:) because runGatewayConversion calls getUrlPrefix at line 188 — after lines 184-186. In the original code the resolver log came before Converting.... This is cosmetically different from the original but functionally harmless.
No blocking issues found. ✅> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 29.7 AIC · ⌖ 9.24 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting only; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Dual-callback coupling:
getTargetDomain+getUrlPrefixmust be kept in sync by callers; a singleresolveTargetcallback (or deriving domain from URL) would be safer. - Double invocation in Gemini:
getGeminiHostDomain()is called twice insidemain()callbacks — capture it once before callingrunGatewayConversion. - Test branch coverage: The new
runGatewayConversiontest is a solid happy-path spec, but theoutputPath-as-function and default-urlPrefixbranches are untested. logServerStatsimplicit parameter contract: the meaning of its two arguments (total vs included) is easy to mistake when adding a new engine.
Positive Highlights
- ✅ Clean extraction of the shared pipeline — net -43 lines of duplicated boilerplate.
- ✅ Security-critical
0600file permission is preserved centrally and tested. - ✅ Engine-specific behaviour (Codex host resolution, Copilot HOME guard, Gemini workspace path) survives via callbacks — good interface design.
- ✅ Test names read as specifications and the Arrange/Act/Assert structure is clear.
| function runGatewayConversion(options) { | ||
| const context = loadGatewayContext(options.contextOptions); | ||
| const targetDomain = options.getTargetDomain ? options.getTargetDomain(context) : context.domain; | ||
|
|
There was a problem hiding this comment.
[/codebase-design] getTargetDomain and getUrlPrefix are separate callbacks but callers must keep them in sync manually — a drift between the two would produce a misleading log line without affecting actual output.
💡 Suggestion
Consider merging into a single resolveTarget callback returning { targetDomain, urlPrefix }, or auto-derive targetDomain from the resolved urlPrefix (e.g. parse the hostname). Gemini already calls getGeminiHostDomain() twice for this reason, which is a code smell.
@copilot please address this.
| expect(fs.statSync(outputPath).mode & 0o777).toBe(0o600); | ||
| expect(mockCore.info).toHaveBeenCalledWith("Converting gateway configuration to Test format..."); | ||
| expect(mockCore.info).toHaveBeenCalledWith("Target domain: target.internal:80"); | ||
| expect(mockCore.info).toHaveBeenCalledWith("Servers: 1 included, 1 filtered (CLI-mounted)"); |
There was a problem hiding this comment.
[/tdd] The single test exercises the happy path but skips error scenarios — specifically outputPath as a function and getUrlPrefix absent (default fallback). These are meaningful branches in runGatewayConversion that a regression could silently break.
💡 Suggested additional tests
it("resolves outputPath when given as a function", () => {
const output = runGatewayConversion({
...
outputPath: (ctx) => path.join(dir, "dynamic.json"),
});
expect(fs.existsSync(path.join(dir, "dynamic.json"))).toBe(true);
});
it("uses default urlPrefix when getUrlPrefix is not provided", () => {
// MCP_GATEWAY_DOMAIN and PORT produce a urlPrefix; transformServer receives it
let capturedUrlPrefix;
runGatewayConversion({
...
transformServer: (_n, e, urlPrefix) => { capturedUrlPrefix = urlPrefix; return e; },
serialize: s => JSON.stringify(s),
});
expect(capturedUrlPrefix).toBe("(gateway.internal/redacted)
});@copilot please address this.
| function getGeminiHostDomain() { | ||
| return process.env.MCP_GATEWAY_HOST_DOMAIN || "localhost"; | ||
| } | ||
|
|
There was a problem hiding this comment.
[/codebase-design] getGeminiHostDomain() is called twice — once in getTargetDomain and once in getUrlPrefix — so the function body runs twice and the results must agree. If the env var changes between calls (unlikely in practice but possible in tests), the log and the URL could diverge.
💡 Suggestion
Capture the domain once outside the option callbacks:
const hostDomain = getGeminiHostDomain();
return runGatewayConversion({
...
getTargetDomain: () => hostDomain,
getUrlPrefix: ({ port }) => `(redacted)
});This also eliminates the redundant exported getGeminiHostDomain helper.
@copilot please address this.
| logCLIFilters(context.cliServers); | ||
| const servers = filterAndTransformServers(context.servers, context.cliServers, (name, entry) => options.transformServer(name, entry, urlPrefix, context)); | ||
| const output = options.serialize(servers, context, urlPrefix); | ||
|
|
There was a problem hiding this comment.
[/codebase-design] logServerStats is called with context.servers (the unfiltered original map) but Object.keys(servers).length (post-filter count), relying on the caller to pass the right pair. This implicit contract is invisible in the JSDoc and easy to misuse when adding a new engine.
💡 Suggestion
Compute the stat inside runGatewayConversion from the quantities already in scope — no extra parameter surface needed:
logServerStats(context.servers, Object.keys(servers).length);
// already correct — but document the contract in the JSDoc of logServerStatsAlternatively, make logServerStats accept { total, included } as a named-pair to surface the intent.
@copilot please address this.
|
@copilot please review the latest PR feedback, refresh this branch if it is behind
Run: https://github.com/github/gh-aw/actions/runs/31055742237
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…on via main() Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Added |
|
@copilot thanks for the follow-up. CI appears complete now. Please run the
Run: https://github.com/github/gh-aw/actions/runs/31057198185
|
… add missing branch coverage Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all three remaining notes:
All 73 tests pass. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. The |
Claude, Codex, Copilot, and Gemini converters repeated the same gateway context loading, CLI-server filtering, secure write, logging, and reporting pipeline. Only URL handling, entry transforms, output paths, and serialization differ by engine.
Shared conversion runner
runGatewayConversionto centralize the common pipeline.0600output handling and conversion reporting.Engine-specific adapters
Coverage