Fix invalid JSON escapes in GitHub remote MCP gateway config generation#41864
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage — §28289524040
Fix targets
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (9 lines, threshold is 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Fixes malformed JSON emitted in MCP gateway config generation for GitHub remote mode by removing the problematic escaping on GitHub remote MCP env placeholders, and improves start_mcp_gateway.cjs JSON parse failure diagnostics by surfacing likely line/column/key context.
Changes:
- Emit
${...}placeholders (instead of backslash-prefixed forms) for GitHub remote MCPenvvalues so the generated JSON remains parseable. - Add
getJSONParseErrorContexttostart_mcp_gateway.cjsand a Vitest covering invalid-escape scenarios. - Update lock output and Go tests to match the corrected env placeholder format.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/mcp_renderer_github.go | Updates GitHub remote MCP env placeholder rendering to avoid invalid JSON escapes. |
| pkg/workflow/github_remote_mode_test.go | Updates assertions to match new env placeholder output in generated lock content. |
| pkg/workflow/github_remote_config_test.go | Updates expected remote config strings for env placeholders. |
| actions/setup/js/start_mcp_gateway.cjs | Adds targeted JSON parse error context extraction and logs it on parse failures. |
| actions/setup/js/start_mcp_gateway.test.cjs | Adds unit test coverage for the new parse-context helper. |
| .github/workflows/github-remote-mcp-auth-test.lock.yml | Regenerates lock output for corrected env placeholder rendering. |
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: 2
- Review effort level: Low
| "env": { | ||
| "GITHUB_HOST": "\\${GITHUB_SERVER_URL}", | ||
| "GITHUB_PERSONAL_ACCESS_TOKEN": "\\${GITHUB_MCP_SERVER_TOKEN}" | ||
| "GITHUB_HOST": "${GITHUB_SERVER_URL}", | ||
| "GITHUB_PERSONAL_ACCESS_TOKEN": "${GITHUB_MCP_SERVER_TOKEN}" |
| return null; | ||
| } | ||
|
|
||
| const safePos = Math.min(pos, Math.max(0, jsonText.length - 1)); |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — approving with improvement suggestions on the new diagnostic helper.
📋 Key Themes & Highlights
Key Themes
- Test spec completeness:
getJSONParseErrorContextis exported and documented but the single test only assertskeyandlineText, leavinglineandcolumnunverified; thenullreturn path is also untested. - Diagnostic precision:
keyMatchis position-agnostic (works for prettified JSON but could misattribute keys in minified config);safePosclipping at EOF is correct but unexplained.
Positive Highlights
- ✅ Surgical root-cause fix: a single
\removal inmcp_renderer_github.gocorrectly unblocks gateway startup without side effects. - ✅ Valuable diagnostic improvement: surfacing line/column/key context on
JSON.parsefailures is a meaningful observability win. - ✅ Good JSDoc coverage on the new helper, and the fix is validated across all affected test files.
- ✅ Lock-file hash rotation is expected and correct given the content change.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 55.7 AIC · ⌖ 7.59 AIC · ⊞ 6.6K
| const context = getJSONParseErrorContext(invalidConfig, parseErrorMessage); | ||
| expect(context).toBeTruthy(); | ||
| expect(context?.key).toBe("GITHUB_HOST"); | ||
| expect(context?.lineText).toContain(`"GITHUB_HOST"`); |
There was a problem hiding this comment.
[/tdd] The assertions only verify key and lineText — line and column are part of the returned contract but are unchecked, so regressions in position arithmetic would silently pass.
💡 Suggested additions
expect(context?.line).toBe(5); // "GITHUB_HOST" is on line 5 of invalidConfig
expect(context?.column).toBeGreaterThan(0);Pinning at least line turns this into a real specification and catches off-by-one regressions in getJSONParseErrorContext.
@copilot please address this.
| expect(context?.key).toBe("GITHUB_HOST"); | ||
| expect(context?.lineText).toContain(`"GITHUB_HOST"`); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] No test covers the null return path — when parseErrorMessage contains no "position N" substring the function silently returns null, but that branch is never exercised.
💡 Suggested test
it("returns null when error message has no position info", () => {
const context = getJSONParseErrorContext('{"x":1}', 'Unexpected token');
expect(context).toBeNull();
});This matters because different JS engines (or future Node.js versions) may not include "position N" in their error messages, and the calling code in main() already guards on if (parseContext) — so the null path is a real production path that should be tested.
@copilot please address this.
| const resolvedLineEnd = lineEnd === -1 ? jsonText.length : lineEnd; | ||
| const lineText = jsonText.slice(lineStart, resolvedLineEnd); | ||
| const column = safePos - lineStart + 1; | ||
| const keyMatch = lineText.match(/"([^"]+)"\s*:/); |
There was a problem hiding this comment.
[/diagnose] keyMatch scans all of lineText for the first "key": pattern, regardless of where in the line the error pos falls. For prettified JSON (one key per line) this works fine, but for minified or compacted JSON it could report a key that precedes the actual error position.
💡 Improvement suggestion
Consider searching only within the substring up to column so the match is anchored to the error site:
const keyMatch = lineText.slice(0, column).match(/"([^"]+)"\s*:\s*$/)
|| lineText.match(/"([^"]+)"\s*:/);The fallback preserves the existing behaviour when no key appears before the error column (e.g., the error is inside a key name itself).
@copilot please address this.
| return null; | ||
| } | ||
|
|
||
| const safePos = Math.min(pos, Math.max(0, jsonText.length - 1)); |
There was a problem hiding this comment.
[/diagnose] safePos clips pos to jsonText.length - 1 when the parse error position equals the string length (EOF). This shifts the reported column one character back, which could mislead debugging for truncated or unterminated-string errors. A brief comment explaining the intent would help future readers.
💡 Suggested clarification
// Clip to last valid index; pos === length (EOF) is safe to round down by one.
const safePos = Math.min(pos, Math.max(0, jsonText.length - 1));Alternatively, special-case pos >= length to report line: <last line>, column: <EOL+1> for a more accurate EOF diagnosis.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 2 Go test changes: Verdict
|
There was a problem hiding this comment.
Non-blocking observations on the diagnostic helper
The root fix (removing the \\${ invalid-JSON-escape from env values) is correct and the lock file + Go test updates are consistent. The notes below are all on the newly introduced getJSONParseErrorContext utility.
Findings (all non-blocking)
safePos off-by-one — Math.max(0, jsonText.length - 1) as the upper bound silently miscounts when V8 reports pos === jsonText.length (the typical "Unexpected end of JSON input" path). Fix: Math.min(pos, jsonText.length). See inline comment.
Misplaced JSDoc cast — /** @type {Error} */ err.message casts the string property, not the unknown err binding. TypeScript infers parseMessage : Error instead of string. The pre-existing pattern is copied into both new files. Fix: /** @type {Error} */ (err).message or an instanceof guard.
Thin test coverage — one test covers the happy path; the null-return path and the end-of-input (pos === length) edge case are unexercised.
🔎 Code quality review by PR Code Quality Reviewer · 131.3 AIC · ⌖ 9.26 AIC · ⊞ 5.2K
Comments that could not be inline-anchored
actions/setup/js/start_mcp_gateway.cjs:86
safePos is off-by-one for "Unexpected end of JSON input" errors — the most common parse failure when a heredoc is truncated or unterminated.
<details>
<summary>💡 Details and suggested fix</summary>
Math.max(0, jsonText.length - 1) caps safePos one short of the valid upper bound. JSON.parse can legally return pos === jsonText.length (e.g., V8: "Unexpected end of JSON input at position N"). When that happens the before slice misses the last character, so the reported column…
actions/setup/js/start_mcp_gateway.cjs:402
JSDoc cast is applied to .message (a string) rather than to err (the unknown catch binding), so TypeScript infers parseMessage as Error instead of string.
<details>
<summary>💡 Details and suggested fix</summary>
The intent of the JSDoc comment is to assert that the caught value is an Error instance so .message can be accessed without a TS error. The annotation /** @type {Error} */ err.message applies the cast to the result of .message, making parseMessage type…
actions/setup/js/start_mcp_gateway.test.cjs:223
Test coverage covers only one happy path, leaving the null-return and end-of-input paths untested.
<details>
<summary>💡 Suggested additional cases</summary>
Three cases worth adding:
-
Error message without "position N" —
getJSONParseErrorContext("{}", "Unexpected token")should returnnull(the guard exists but is untested). -
pos === jsonText.length— the "Unexpected end of JSON input" case. Currently triggers thesafePosoff-by-one noted above. A test makes any f…
actions/setup/js/start_mcp_gateway.cjs:415
The full config — now containing the live GITHUB_MCP_SERVER_TOKEN value — is dumped to the Actions log on any JSON parse failure.
<details>
<summary>💡 Why this PR introduces the risk and how to fix it</summary>
Before this PR, the heredoc contained "\\\". The leading backslash prevented shell expansion, so the literal placeholder string flowed through to mcpConfig and no real secret ever reached this variable.
After this PR, the heredoc delimiter is unquoted, so `${GITHUB_MCP_SERV…
|
🎉 This pull request is included in a new release. Release: |
Start MCP Gatewaycould fail before launch because GitHub remote MCP env placeholders were emitted with a leading backslash, producing invalid JSON escapes (for example\https://...). This change removes the bad escaping path and improves parse-failure diagnostics so malformed config points to the likely key/location.Gateway config rendering (root cause)
${...}placeholders instead of\${...}in JSON values.Failure diagnostics in gateway startup
start_mcp_gateway.cjsto report likely line/column and key when JSON parsing fails.Generated workflow + expectations