Skip to content

feat(desktop): merge MCP command and args into one command-line field - #2918

Merged
Astro-Han merged 1 commit into
apache:mainfrom
GabrielDrapor:feat/mcp-command-line-field
Aug 17, 2026
Merged

feat(desktop): merge MCP command and args into one command-line field#2918
Astro-Han merged 1 commit into
apache:mainfrom
GabrielDrapor:feat/mcp-command-line-field

Conversation

@GabrielDrapor

Copy link
Copy Markdown
Contributor

Extracted from #2653 per the review recommendation to split it into independently reviewable units. This is the command-line slice, standalone and independent of the OAuth work.

What

Merges the MCP editor's separate command / args inputs into one command-line field, with shell-style tokenization (quotes, escapes) parsed in mcp-command-line.ts and validated by mcp-editor-validation.ts. The dialog shows one field that round-trips to the stored {command, args} shape.

Tests

apps/desktop/src/main/__tests__/mcp-command-line.test.ts covers tokenization (quoting, escaping, unterminated quotes) and round-tripping; editor-validation tests updated for the merged field.

Co-Authored-By: Claude noreply@anthropic.com

https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This slice looks very good overall. The split is clean and independently reviewable, the command-line field remains a tokenizer rather than a shell, the stored { command, args } contract is preserved, and the formatter/parser provide one clear round-trip authority. The focused tests and CI evidence also look solid. I did not find any P0–P3 issues.

The only thing missing for me is a small before/after screenshot of the stdio editor dialog. This changes the visible form from separate command and arguments fields to one command-line field, and changes the primary grid from equal columns to a 1:2 ratio. A screenshot at the normal dialog width should be enough to confirm that the command help text, environment field, and working-directory field remain visually balanced.

Once that screenshot is present, I expect this slice to be ready to approve. Thanks for extracting this into such a focused first step.

AI-assisted review disclosure: Codex reviewed the parsing, formatting, validation, renderer wiring, tests, and CI evidence. Astro-Han verified the architectural boundary, found no P0–P3 code issues, and made the final decision to request the remaining visual evidence.

@GabrielDrapor

Copy link
Copy Markdown
Contributor Author

Here is the before/after of the stdio editor dialog at the normal dialog width:

Before — separate 命令 field and 参数 textarea, id/command in equal columns:

before: separate command and args fields

After — one command-line field with the shell-quoting hint; the id shrinks to slug width and the command line takes the remaining 2/3, with 环境变量 and 工作目录 unchanged below:

after: single command-line field

@GabrielDrapor
GabrielDrapor force-pushed the feat/mcp-command-line-field branch from d9974a9 to a1dca7a Compare August 13, 2026 07:50
@GabrielDrapor

Copy link
Copy Markdown
Contributor Author

CI note: all jobs green except two known-flaky/inherited reds — windows_recovery died in npm ci with a registry socket reset (UND_ERR_SOCKET; same job passes on the sibling stack PRs), and e2e's single failure is quote-selection.spec.ts, which fails identically on current main (run 31675601561). A rerun should clear windows_recovery; I don't have rerun permission here. Screenshots for the review ask are in the comment above.

The Add/Edit MCP dialog's stdio form asked for the command and a
one-arg-per-line textarea separately. Users naturally type the whole
line ("uvx mcp-science timer") into the command box. The 命令 field now
accepts the full command line: quote-aware tokenization (no shell
semantics) maps it onto the mcp.json command/args shape, and editing an
existing server formats command+args back into one line losslessly.

Unbalanced quotes surface as an inline field error instead of a save
failure, and the primary-fields grid gives the endpoint column twice
the width of the server-id slug.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ac5rv6WUKWtMZgQb5QPN16
@GabrielDrapor
GabrielDrapor force-pushed the feat/mcp-command-line-field branch from a1dca7a to 563c786 Compare August 14, 2026 01:35
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for the split — the boundary with #2653 is clean (only the MCP editor's 7 files, no OAuth content), the storage contract is untouched (McpStdioServerConfig and the runtime consumption path are unchanged, mcp.json schema identical), old configs round-trip losslessly through formatCommandLine, and the hand-written tokenizer is deliberately non-shell (no expansion/globs/operators), matching the old "not parsed by a shell" semantics. CI is green.

Conclusion: PASS — no blocking findings (no P0/P1/P2).

All findings are P3 (nice-to-have, non-blocking):

  • F1configFromDraft's defensive throw is half-done (mcp-page.tsx:995-997 only checks !parsed.ok; a '""' input silently saves command: ''). The only caller validates first so it's unreachable — either add the !parsed.command check or drop the throw.
  • F2 — empty-string args behavior change is undocumented: the old UI filtered blank lines, the new UI can save args: [''] via "", which spawns with an empty arg. More correct, but worth a line in the description.
  • F3 — Windows trailing-backslash paths ("C:\Program Files\") report unbalanced-quote since \" is treated as an escape (standard shell behavior, but commandHelp could mention the \ rule).
  • F4 — the CSS 1fr/2fr grid also widens the remote form's URL field (visual only, no correctness impact).
  • F5 — test gap (deferrable): no formatCommandLine round-trip test with single/mixed quotes (logic verified correct), no dialog component-level test.
  • F6mcp-command-line.ts:2 mixes a Chinese word into an English comment.

F1/F2 are one-liners if you touch the file anyway; F5 is a fine follow-up.


AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on ollama-cloud/deepseek-v4-flash). The subagent extracted the tokenizer and ran 40+ edge cases locally (including property-style round-trip tests over 30 tricky tokens — all pass) and grepped the PR branch for residual old-structure references (zero). The P3 findings are static analysis; please weigh them with your own judgment.

中文摘要(AI 辅助审查)

结论:PASS,无 P0/P1/P2,全部 P3。与 #2653 边界干净(只动 MCP editor 7 个文件,无 OAuth 内容);存储契约零变化(McpStdioServerConfig 与运行时消费路径未动、mcp.json schema 不变);旧配置经 formatCommandLine 无损往返;手写 tokenizer 刻意非 shell 语义(无展开/通配/操作符),与旧"不经过 shell 解析"一致。解析器经 40+ 边界用例实测正确(含 30 个刁钻 token 的属性式往返测试全过),PR 分支内旧结构零残留。P3 项:F1 configFromDraft 防御性 throw 只防一半('""' 静默存 command:'',但唯一调用方先校验不可达);F2 空字符串参数行为变更未声明(旧 UI 过滤空行,新 UI 可经 "" 存 args:['']);F3 Windows 尾反斜杠路径报 unbalanced-quote(标准 shell 行为,文案可补充);F4 CSS 1fr/2fr 同时影响 remote 表单 URL 字段(纯视觉);F5 测试缺口可延后(无单引号/混合引号往返测试、无组件级测试);F6 注释混入中文。

@Astro-Han
Astro-Han merged commit 794e760 into apache:main Aug 17, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants