fix(desktop): preserve MCP URL query suffixes#6587
Conversation
Normalize MCP source URLs by trimming trailing slashes only from the path portion before query or fragment components. This keeps query parameters and fragments intact when they legitimately end with a slash. Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
|
Thanks for the PR, @VectorPeak! (re-run) Template looks good ✓ Problem: The Direction: Fixing URL normalization to respect URL structure (path vs. query vs. fragment) is clearly correct and aligned with qwen-code's MCP support. MCP servers that use query tokens, signed parameters, or fragment-based routing would be affected by the current behavior. Size: 10 production lines + 41 test lines. Minimal and focused. Not applicable for core module gate (changes are in Approach: The fix is well-targeted — split at the first Moving on to code review. 🔍 中文说明感谢贡献！（重新运行） 模板完整 ✓ 问题： 方向：修复 URL 归一化以区分 URL 结构（path / query / fragment）是完全正确的，与 qwen-code 的 MCP 支持一致。使用 query token、签名参数或 fragment routing 的 MCP server 会受到当前行为的影响。 规模：10 行生产代码 + 41 行测试。精简聚焦。不触及核心模块（改动在 方案：修复方案精准——在第一个 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe fix is correct and minimal. The approach — finding the first The logic handles all edge cases properly:
The function is called in two places ( Correction from prior run: the test file's VerificationVerified the normalization logic with a standalone node script comparing before/after behavior across 10 URL shapes: All 10 cases produce correct results: path trailing slashes still removed (backward compatible), query/fragment trailing slashes preserved (fixed), combined query+fragment handled correctly (fixed), non-parseable URLs still fall back gracefully (backward compatible). Maintainer @wenshao also verified locally on macOS with the full bun test suite (163 pass / 0 fail across 15 files) and an extended 22-URL adversarial A/B matrix (69 assertions, all pass). 中文说明代码审查修复正确且最小化。方案——用 逻辑处理所有边界情况都正确：无 query/fragment 走原有正则、不可解析 URL 同样正确降级、path 斜杠 + query 组合正确处理、query + fragment 组合也正确处理（ 函数在两个地方被调用（ 更正：测试文件的 验证使用独立 node 脚本对比修复前后行为，覆盖 10 种 URL 形态，所有场景均产生正确结果。维护者 @wenshao 也在 macOS 上完整验证了 bun 测试套件（15 个文件 163 通过 / 0 失败）和 22 种 URL 扩展对抗矩阵（69 处断言全通过）。 — Qwen Code · qwen3.7-max |
|
This is a clean, focused correctness fix. The The scope is minimal — one function, one test file, no drive-by changes. The test file correctly uses Approving. ✅ 中文说明这是一个干净、聚焦的正确性修复。 范围最小——一个函数、一个测试文件、无顺手改动。测试文件正确使用 通过。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Suggestions — commit
|
Clarify that MCP URL normalization removes trailing slashes only from the path portion while preserving query strings and fragments unchanged. Co-authored-by: qwen-code-ci-bot <253268222+qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
|
Thanks for the careful review. I updated the JSDoc to say that trailing slashes are removed from the path portion only, and that query strings and fragments are preserved unchanged. I also reran the focused Windows test: \cd packages/desktop && bun test packages/shared/src/sources/tests/server-builder-url.test.ts. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| export function normalizeMcpUrl(url: string): string { | ||
| return url.replace(/\/+$/, ''); | ||
| const suffixIndex = url.search(/[?#]/); | ||
| if (suffixIndex === -1) { |
There was a problem hiding this comment.
[Suggestion] Consider adding a brief comment explaining why the URL constructor is intentionally avoided here. A future maintainer might see the manual search(/[?#]/) + slice() parsing and refactor to new URL(), which would subtly change behavior (host lowercasing, percent-encoding normalization, throwing on unparseable inputs like 'not a url///').
| if (suffixIndex === -1) { | |
| // Intentionally avoids URL constructor: preserves original URL verbatim and handles non-parseable strings without throwing. | |
| const suffixIndex = url.search(/[?#]/); |
— qwen3.7-max via Qwen Code /review
| 'https://api.example.com/mcp?key=value/', | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[Suggestion] The tests cover ? and # independently but not together. Adding a combined case locks in the invariant that url.search(/[?#]/) correctly handles ? before # per RFC 3986 ordering, and prevents a regression if someone rewrites the split logic.
| expect(normalizeMcpUrl('https://api.example.com/mcp/?key=value/')).toBe( | |
| 'https://api.example.com/mcp?key=value/', | |
| ); | |
| }); | |
| test('preserves trailing slashes when both query and fragment are present', () => { | |
| expect(normalizeMcpUrl('https://api.example.com/mcp/?key=val#/route/')).toBe( | |
| 'https://api.example.com/mcp?key=val#/route/', | |
| ); |
— qwen3.7-max via Qwen Code /review
✅ Local verification (maintainer) — recommend mergeBuilt and ran this PR end-to-end on macOS, which also closes the PR's Environment: macOS 15 (Darwin 24.6, arm64) · Bun 1.3.14 (matches the author's env) · tsc 5.9.3 · Node 22. Verified against PR head What I ran
Does it actually fix the bug?Yes. I inlined the pre-PR normalizer (
Impact chain confirmedThe normalized URL flows straight into the live connection URL in two call sites, so the old corruption was reaching real endpoints — this is a genuine fix, not cosmetic:
One minor, non-blocking observationThe new implementation also trims the authority-terminating slash on host-only URLs that carry a suffix, e.g. VerdictMinimal, correct, well-scoped. Green on macOS here plus the author's Windows/Linux runs, no regressions, typecheck clean. LGTM — safe to merge. 🟢
🇨🇳 中文版本✅ 维护者本地验证 —— 建议合并在 macOS 上完整构建并运行了本 PR，同时补上了 PR 中 环境： macOS 15 (Darwin 24.6, arm64) · Bun 1.3.14（与作者环境一致）· tsc 5.9.3 · Node 22。在独立 worktree 中基于 PR head 执行的检查
（截图见上方英文部分） 是否真的修复了问题？是。我把 改动前 的归一化实现（ 影响链已确认归一化后的 URL 会直接进入两处真实连接 URL，所以旧的破坏是会实际影响到远程端点的 —— 这是真实修复，而非表面改动：
一点小的、不阻塞合并的观察新实现对带 suffix 的 纯 host URL 也会裁掉那个结尾斜杠，例如 结论改动最小、正确、范围清晰。macOS（本次）叠加作者的 Windows/Linux 运行结果全绿，无回归，类型检查干净。LGTM —— 可以安全合并。 🟢
|
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
3 participants
What this PR does
This PR narrows MCP source URL normalization so trailing slash cleanup only applies before the query string or fragment. Remote MCP URLs still have trailing slashes removed from the path portion, but query values and hash fragments are preserved exactly as configured.
Why it's needed
What Problem This Solves
The previous implementation normalized MCP URLs with a string-level trailing slash trim:
That treats the entire URL as one flat string. URLs are structured values: the path, query string, and fragment each carry different routing or authentication semantics. For a URL such as
https://api.example.com/mcp?key=/, the trailing slash belongs to the query parameter value, not the path. Removing it changes the configured endpoint semantics from?key=/to?key=.That distinction matters for remote MCP sources because the configured
mcp.urlis passed into the runtime connection path. Query values can carry tokens, tenant identifiers, signed request material, compatibility flags, or provider-specific routing parameters. Fragments can also be used by some integrations as client-side routing or endpoint metadata. A normalizer that trims the whole string can therefore corrupt a valid remote MCP URL before the client attempts to connect.Changes
This PR keeps the existing path cleanup behavior, but narrows where it applies. Instead of trimming trailing slashes from the entire URL string,
normalizeMcpUrl()now finds the first query or fragment boundary and trims only the path prefix before that boundary.export function normalizeMcpUrl(url: string): string { - return url.replace(/\/+$, ''); + const suffixIndex = url.search(/[?#]/); + if (suffixIndex === -1) { + return url.replace(/\/+$/, ''); + } + + return ( + url.slice(0, suffixIndex).replace(/\/+$, '') + url.slice(suffixIndex) + ); }The intentionally unchanged behavior is the existing path suffix normalization:
https://api.example.com/mcp/still becomeshttps://api.example.com/mcp. The changed behavior is limited to preservingsearchandhashcontent exactly as configured.Evidence
The failure can be reproduced with the old string-level normalizer:
After this change, only the path portion is trimmed:
The added focused test covers the key URL shapes:
It verifies path trailing slash cleanup, query values ending in
/, fragments ending in/, and the legacy fallback behavior for non-parseable strings.Possible call chain / impact
The same helper is also reused by MCP connection validation, so preserving URL structure benefits both runtime source setup and validation-time connection checks.
This PR does not change OAuth discovery, credential headers, stdio MCP sources, API sources, or the behavior that removes trailing slashes from the path itself. It only prevents query strings and fragments from being treated as part of the path cleanup target.
Reviewer Test Plan
How to verify
Run the focused Bun test for MCP URL normalization:
The test covers path trailing slash cleanup plus query and fragment values that legitimately end with
/.Evidence (Before & After)
N/A for screenshots; this is non-UI URL normalization behavior.
Before this change, string-level normalization produced:
After this change, only the path portion is trimmed:
Tested on
Environment (optional)
Windows: Bun 1.3.14 installed at
D:\ZXY\Dev\bun\bin. Linux: WSL Ubuntu 22.04 with Bun 1.3.14 installed at/mnt/d/ZXY/Dev/bun-linux/bin.Validation run:
Risk & Scope
Linked Issues
N/A
中文说明
这个 PR 做了什么
这个 PR 收窄 MCP source URL 的归一化逻辑：只在 query string 或 fragment 之前清理 path 的尾部斜杠。远程 MCP URL 的 path 仍会去掉尾斜杠，但 query 参数和 hash fragment 会按用户配置原样保留。
为什么需要它
之前的实现用整串字符串做尾部斜杠删除：
这会把 URL 当成一整段普通字符串处理。对于
https://api.example.com/mcp?key=/这样的 URL，最后的/属于 query 参数值，而不是 path。删除它会把配置语义从?key=/改成?key=，可能破坏依赖 query token、签名参数或 fragment routing 的远程 MCP server。这是一个很小的 URL 结构保真修复：只规范化 path 后缀，不改写
search或hash组件。Reviewer 测试计划
运行 focused Bun test：
该测试覆盖 path 尾斜杠清理，以及 query / fragment 里合法保留尾部
/的情况。证据（Before & After）
非 UI 改动，无截图。
修复前，字符串级归一化会产生：
修复后，只清理 path 部分：
测试环境
Windows：Bun 1.3.14，安装在
D:\ZXY\Dev\bun\bin。Linux：WSL Ubuntu 22.04，Bun 1.3.14，安装在/mnt/d/ZXY/Dev/bun-linux/bin。执行过的验证：
风险与范围
主要风险很低；本 PR 只影响远程 MCP source URL 的归一化，并保留原有 path 尾斜杠清理行为。不包含 macOS 本地验证、真实远程 MCP server 调用、OAuth discovery、credential headers、stdio MCP sources 或 API sources。预计无 breaking changes。
关联 Issue
N/A