feat(cli): import Claude MCP servers#5095
Conversation
|
|
||
| describe('importConfigCommand', () => { | ||
| it('is a built-in command available in all execution modes', () => { | ||
| expect(importConfigCommand.name).toBe('import-config'); |
There was a problem hiding this comment.
[Suggestion] The action() method is never invoked in tests. The helper functions (parseImportConfigArgs, resolveImportSourceForScope, formatClaudeMcpImportResult) are tested in isolation, but the integration wiring — including the try/catch error path when getSettingScope throws — is uncovered.
| expect(importConfigCommand.name).toBe('import-config'); | |
| describe('importConfigCommand', () => { | |
| it('is a built-in command available in all execution modes', () => { | |
| expect(importConfigCommand.name).toBe('import-config'); | |
| expect(importConfigCommand.kind).toBe(CommandKind.BUILT_IN); | |
| expect(importConfigCommand.supportedModes).toEqual([ | |
| 'interactive', | |
| 'non_interactive', | |
| 'acp', | |
| ]); | |
| }); | |
| describe('action integration', () => { | |
| it('runs the import and formats the result', async () => { | |
| // mock context with settings that returns empty sources | |
| // and verify the returned MessageActionReturn | |
| }); | |
| it('catches thrown errors and returns an error message', async () => { | |
| // mock settings where workspace.path === user.path | |
| // verify getSettingScope throw is caught, not propagated | |
| }); | |
| }); |
— qwen3.7-max via Qwen Code /review
| }); | ||
|
|
||
| it('imports Claude Desktop MCP servers from the platform config path', () => { | ||
| const desktopPath = getClaudeDesktopConfigPath(homeDir, 'darwin'); |
There was a problem hiding this comment.
[Suggestion] Only the darwin platform is tested for getClaudeDesktopConfigPath. The win32 branch (with and without APPDATA env var) and the Linux/default branch (~/.config/Claude/...) have no test coverage. Consider adding:
| const desktopPath = getClaudeDesktopConfigPath(homeDir, 'darwin'); | |
| const desktopPath = getClaudeDesktopConfigPath(homeDir, 'darwin'); | |
| // Also test other platforms: | |
| // expect(getClaudeDesktopConfigPath(homeDir, 'win32', { APPDATA: 'C:\\Users\\u\\AppData\\Roaming' })) | |
| // .toBe(path.win32.join('C:\\Users\\u\\AppData\\Roaming', 'Claude', 'claude_desktop_config.json')); | |
| // expect(getClaudeDesktopConfigPath(homeDir, 'linux')) | |
| // .toBe(path.join(homeDir, '.config', 'Claude', 'claude_desktop_config.json')); |
— qwen3.7-max via Qwen Code /review
| }); | ||
| } | ||
|
|
||
| function loadClaudeDesktopMcpServers( |
There was a problem hiding this comment.
[Suggestion] loadClaudeDesktopMcpServers is a ~45-line copy of loadMcpServersFromSettingsFile. The only difference is how filePath is computed. Consider replacing the body with a one-liner:
| function loadClaudeDesktopMcpServers( | |
| function loadClaudeDesktopMcpServers( | |
| homeDir: string, | |
| platform: NodeJS.Platform, | |
| env: NodeJS.ProcessEnv, | |
| ): ClaudeMcpSourceResult { | |
| return loadMcpServersFromSettingsFile( | |
| getClaudeDesktopConfigPath(homeDir, platform, env), | |
| 'Claude Desktop', | |
| 'claude-desktop', | |
| ); | |
| } |
— qwen3.7-max via Qwen Code /review
| continue; | ||
| } | ||
|
|
||
| if (!isRecord(serverConfig)) { |
There was a problem hiding this comment.
[Suggestion] This isRecord(serverConfig) check is unreachable — copyMcpServers already filters non-record values with an error before storing them. The 'invalid' skip reason can never be produced. Consider removing the dead branch, or add a comment explaining it guards against a future code path.
| if (!isRecord(serverConfig)) { | |
| if (existingNames.has(name)) { | |
| skipped.push({ name, source: source.label, reason: 'already-exists' }); | |
| continue; | |
| } |
— qwen3.7-max via Qwen Code /review
| }, | ||
| completion: async (_context, partialArg) => { | ||
| const current = partialArg.trimStart(); | ||
| const candidates = [ |
There was a problem hiding this comment.
[Suggestion] The completion function matches candidate.startsWith(current) against the full partialArg. Once the user types "claude-code ", no candidate starts with that string, so --scope user/--scope project become unreachable. Consider completing on the last token only:
| const candidates = [ | |
| completion: async (_context, partialArg) => { | |
| const parts = partialArg.trimStart().split(/\s+/); | |
| const current = parts[parts.length - 1] ?? ''; | |
| const candidates = [ | |
| 'all', | |
| 'claude-code', | |
| 'claude-desktop', | |
| '--scope user', | |
| '--scope project', | |
| ]; | |
| return candidates.filter((candidate) => candidate.startsWith(current)); | |
| }, |
— qwen3.7-max via Qwen Code /review
Local end-to-end verification (merge reference)I built this PR and exercised Verdict: the feature works end-to-end exactly as designed — reads the right Claude paths, writes the right Qwen scope, preserves existing names, skips reserved names, handles malformed configs gracefully, and does not pollute the prototype. LGTM for this Phase-1 (MCP-only) scope. Two non-blocking polish notes below. Environment: Linux, Node 22.22.2, head 1. PR test plan — all green
2. Real-binary end-to-end matrixSeed:
The TUI rendered case 1 as a 3. Non-blocking notes
Neither affects correctness. Reads/writes, scope resolution ( Good to merge as Phase 1 of #4845. 中文版（Chinese version）本地端到端验证（合并参考）我构建了此 PR，并在真实 结论：该功能端到端完全按设计工作——读取正确的 Claude 路径、写入正确的 Qwen scope、保留已有名称、跳过保留名、优雅处理损坏配置、且不污染原型。在 Phase 1（仅 MCP）范围内建议合并。下面是两条不阻断的打磨建议。 环境：Linux、Node 22.22.2、head 1. PR 测试计划 —— 全绿
2. 真实二进制端到端矩阵预置：
TUI 中场景 1 渲染为 3. 不阻断的建议
两者都不影响正确性。读写、scope 解析（ 作为 #4845 的 Phase 1，可以合并。 |
326fe26 to
e1e3f0b
Compare
|
thanks, both points make sense. pushed a follow-up that removes the dead invalid-skip branch/output and makes the no-op-with-warnings case render as a warning. also rebased onto latest main and added the missing zh/zh-TW command description translations that were tripping CI. focused tests, typecheck, lint, check-i18n, and diff check are green locally; CI is rerunning. |
Follow-up verification of the polish commit (merge reference)This supersedes my earlier report, which tested Both of my earlier notes are addressed, verified live, and the feature still works end-to-end. LGTM. What the polish commit changed (diff-confirmed at
|
| Command | Result |
|---|---|
vitest run (3 files) |
32 passed (12 + 9 + 11; the importConfig file gained the new no-op-warning test) |
check-i18n --workspace packages/cli |
exit 0 (was the CI failure; now fixed) |
typecheck --workspace packages/cli |
exit 0 |
lint --workspace packages/cli |
exit 0 |
git diff --check |
clean |
Two process notes, neither a PR defect: the combined 3-file
vitestrun hit a one-off file-level failure with 0 assertion failures, green on immediate retry — the known coverage temp-file/worker race. Andtypecheckfails withTS6305if run beforepackages/corefinishes rebuilding after the rebase (stalecore/dist); it passes once the core build completes, matching your own note about rebuilding core first.
2. Real-binary re-test (TUI via tmux) — the two fixes, observed
Same seeded isolated $HOME (Claude Code + Desktop configs; valid + __proto__ + a non-object cc-broken).
| Check | Prior head 326fe26d |
New head e1e3f0b4 |
|---|---|---|
/import-config (run 1) |
△ Imported 5 …, skips __proto__, warns cc-broken |
same — △ Imported 5 … cc-everything, cc-fetch, cc-http, cc-global-git, desktop-weather |
| re-run (0 imported + already-exists + warning) | ✕ error icon |
✅ △ warning — No new Claude MCP servers imported … (Note 2 fixed) |
non-object cc-broken |
warning + a dead Skipped invalid code path |
✅ warning only; dead path/output gone (Note 1) |
| settings persisted | 5 servers written | ✅ 5 servers written, config intact |
__proto__ prototype pollution |
none | ✅ none (no own key, clean prototype) |
Reads/writes, scope resolution, name preservation, reserved-name skipping, malformed handling, and persistence all still behave correctly in the real binary; the only user-visible change is the re-run now reading as a warning instead of an error, which is the intended fix.
Verdict: the polish cleanly resolves both notes with a new test and the i18n fix; nothing regressed. Good to merge as Phase 1 of #4845.
中文版（Chinese version）
polish 提交的跟进验证（合并参考）
本评论取代我此前的报告（当时测的是 326fe26d）。分支已 rebase，并合入了 e1e3f0b4（"polish Claude MCP import feedback"）。我在新 head 上于 Linux（Node 22.22.2）重新构建并重跑了全部验证。
我此前提的两点都已处理、已实测确认，且功能端到端仍正常。建议合并。
polish 提交改了什么（已在 e1e3f0b4 的 diff 中确认）
- 建议 1 —— 移除死的
invalid分支：从SkippedClaudeMcpServer联合类型去掉'invalid'，删除importClaudeMcpServers()中不可达的if (!isRecord(serverConfig)) … reason:'invalid'块，以及 formatter 里的skippedInvalid过滤与Skipped invalid server(s)输出。 - 建议 2 —— "0 导入 + 警告"现在是 warning：
messageType简化为errors>0 || skipped>0 ? 'warning' : 'info'——去掉了'error'分支，因此"有跳过/警告但imported===0"的结果渲染为 warning。真正的失败仍通过命令 action 的try/catch报为 error。新增单元测试（keeps no-op imports with source warnings as warnings）锁定该行为。 - CI：为
en/zh/zh-TW补上Import MCP servers from Claude configs命令描述翻译，修复check-i18n。
1. PR 测试计划 —— 新 head 上全绿
| 命令 | 结果 |
|---|---|
vitest run（3 文件） |
32 通过（12 + 9 + 11；importConfig 文件新增了 no-op-warning 测试） |
check-i18n --workspace packages/cli |
exit 0（此前的 CI 失败点，现已修复） |
typecheck --workspace packages/cli |
exit 0 |
lint --workspace packages/cli |
exit 0 |
git diff --check |
干净 |
两点流程说明，均非 PR 缺陷：3 文件合并
vitest运行出现过一次文件级失败但0 个断言失败，立即重跑即绿——即已知的 coverage 临时文件/worker 竞态。另外，rebase 后若在packages/core重建完成前跑typecheck，会因core/dist过期报TS6305；core 构建完成后即通过，与你自己提到的"先重建 core"一致。
2. 真实二进制复测（tmux 驱动 TUI）—— 两处修复实测
同一套预置隔离 $HOME（Claude Code + Desktop 配置；合法项 + __proto__ + 非对象的 cc-broken）。
| 检查项 | 旧 head 326fe26d |
新 head e1e3f0b4 |
|---|---|---|
/import-config（第 1 次） |
△ Imported 5 …，跳过 __proto__，警告 cc-broken |
相同 —— △ Imported 5 … cc-everything, cc-fetch, cc-http, cc-global-git, desktop-weather |
| 重跑（0 导入 + already-exists + 警告） | ✕ 错误图标 |
✅ △ 警告 —— No new Claude MCP servers imported …（建议 2 已修） |
非对象 cc-broken |
警告 + 一段死的 Skipped invalid 代码路径 |
✅ 仅警告；死路径/输出已移除（建议 1） |
| 设置持久化 | 写入 5 个 server | ✅ 写入 5 个 server，配置完整 |
__proto__ 原型污染 |
无 | ✅ 无（无自有键、原型干净） |
读写、scope 解析、名称保留、保留名跳过、损坏处理、持久化在真实二进制中均仍然正确；唯一用户可见变化就是重跑现在显示为 warning 而非 error，这正是本次预期的修复。
结论：polish 干净地解决了两点，并配套新增测试与 i18n 修复，无回归。作为 #4845 的 Phase 1，可以合并。
|
@qwen-code /triage |
|
@qwen-code /triage |
|
Thanks for the PR, @tt-a1i! Template doesn't strictly match On direction: This is Phase 1 of #4845 (Claude config migration), scoped to MCP servers only — exactly the right starting point. Helping users bring their existing MCP configs over from Claude Code and Claude Desktop is a real friction-reducer. The phased approach in the parent issue makes sense, and this PR delivers the lowest-risk, highest-value slice. No direct Claude Code CHANGELOG entry for On approach: The scope feels right — two new modules ( Moving on to code review. 🔍 中文说明感谢贡献，@tt-a1i！ 模板与 方向： 这是 #4845（Claude 配置迁移）的第一阶段，仅涉及 MCP 服务器——正确的起点。帮助用户从 Claude Code 和 Claude Desktop 迁移已有的 MCP 配置是真实存在的摩擦点。父 issue 的分阶段方案合理，本 PR 交付了风险最低、价值最高的部分。Claude Code CHANGELOG 中没有直接对应的 方案： 范围合适——两个新模块（ 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code reviewIndependent proposal before reading the diff: I'd create a core import module in The PR's approach matches this closely. Two clean modules — No critical blockers. No AGENTS.md violations. The implementation is straightforward and maintainable. Real-scenario testingSet up a fake Test 1:
|
|
This PR does exactly what it says, and it does it well. The implementation is clean — read Claude configs, merge into Qwen settings, skip duplicates, report clearly — with no unnecessary abstraction or speculative scope. The test suite is thorough (32 tests covering happy paths, edge cases like My independent proposal matched the PR's approach almost exactly, which tells me the design space is natural and the author found the right shape. The code is the kind you'd want to find when debugging at 2 AM — each function does one thing, error messages are actionable, and the data flow is easy to trace. Ship it. ✅ 中文说明本 PR 完全兑现了承诺。实现简洁——读取 Claude 配置，合并到 Qwen 设置，去重，清晰报告——没有不必要的抽象或投机性扩展。测试套件全面（32 项测试覆盖正常路径、 我的独立提案与 PR 方案几乎一致，说明设计空间自然，作者找到了正确的形态。代码简洁易懂——每个函数职责单一，错误信息可操作，数据流易追踪。 可以合并。✅ — 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. ✅
* feat(cli): import Claude MCP servers * fix(cli): polish Claude MCP import feedback
3 participants
Summary
/import-configfor Phase 1 Claude config migration: MCP servers onlyRefs #4845
Testing
npm exec -- vitest run packages/cli/src/config/claudeMcpImport.test.ts packages/cli/src/ui/commands/importConfigCommand.test.ts packages/cli/src/services/BuiltinCommandLoader.test.tsnpm run typecheck --workspace packages/clinpm run lint --workspace packages/clinpm run check-i18n --workspace packages/cligit diff --checkAI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.