feat(tui): manage MCP servers from /mcp - #4062
Conversation
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
jackwener
left a comment
There was a problem hiding this comment.
I found two issues on exact head 44e156c618a925c81a0d316b1069ceffc5dbc603.
[P1] Serialize mcp.json mutations across Desktop and TUI processes
The new TUI is a second supported writer for the same default workspace file that Desktop already manages. Each surface creates its own McpConfigStore, but FileMcpConfigStore.serial() only orders calls made through that one JavaScript object. transform() still performs an unlocked read, computes a replacement document, and atomically renames it over mcp.json. The TUI's per-server revision check runs inside that same unlocked transform, so it cannot detect a write that another process commits after the read and before the rename.
I reproduced this through two real createMcpConfigStore() instances pointed at one workspace. Two concurrent transforms added different server IDs. Both promises returned success with their own server, while the durable file contained only one. The result was deterministic in 100/100 runs. In the full product path, both Desktop and TUI can therefore synchronize and report their own mutation as applied even though the later rename silently discards the other user's config. This can also lose the only stored copy of command arguments, headers, or environment values.
Please make the durable mutation linearizable across processes—for example, hold one cross-process workspace lock across read, validation, credential retirement, and rename, or route both surfaces through one mutation authority. Add a regression using two independent store/controller instances which proves unrelated concurrent additions both survive and same-server credential retirement stays ordered.
[P2] Keep the selected server visible before acting on it
The list changes selected on Up/Down, but it never adjusts top; PageUp/PageDown/Home/End change top without updating the selected server. In a six-row viewport with eight servers, I pressed Down five times. The screen still showed only s0 and s1 with no visible cursor, then Space executed { kind: "set_enabled", serverId: "s5", enabled: false }. The same invisible target is used by test and reconnect.
Please couple selection and scrolling so the selected server's rendered row range is always visible, and make paging/home/end move selection consistently. A long-list interaction regression should assert both the visible cursor and the server ID sent to execute().
The underlying feature need is valid: before this patch, the local TUI can display MCP state but cannot manage it. Reusing the existing config, manager, and capability publication owners is otherwise the right boundary; the blocking problem is that adding a second process-local action lane does not make the shared durable writer single-authority.
Local verification passed for the affected Core, Storage, MCP, Runtime, Runtime Host, and CLI builds, the full CLI suite (610/610), and the focused Core/Storage/controller/overlay tests. windows_recovery is green. The first hosted test attempt failed only in the unchanged prompt-rail Desktop E2E; I requested an exact-head rerun, which is still in progress. The branch currently merges cleanly with main (e8028fc9f…, merge tree 03902b2b8a08187af3ed305d60a7390e0f4332f9). There were no existing comments, reviews, inline comments, or review threads to duplicate.
Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
Generated-by: Codex
Generated-by: Codex
jackwener
left a comment
There was a problem hiding this comment.
The two findings from 44e156c618a925c81a0d316b1069ceffc5dbc603 are closed on exact head 1f9b2a12cbfa63ba1d4f7eb49acadb2f3cfd687f, but the new cross-process lock leaves one P2 recovery issue.
[P2] Recover an MCP config lock after its writer process crashes
FileMcpConfigStore.transform() now correctly holds withFileUpdateLock() across the authoritative read, asynchronous credential retirement, and durable rename. This closes the lost-update P1, but withFileUpdateLock() represents ownership only by creating the directory mcp.json.lock. The directory is removed in finally; it has no owner identity or lease that the next process can prove dead.
I exercised the production store in a child process, stopped it with SIGKILL after the transform acquired the lock, and then opened a new real store for the same workspace. The lock directory remained. The next upsert() waited 10,002 ms and failed with File update is locked by another process; every later Desktop or TUI mutation follows the same path until someone manually removes the hidden lock directory. TUI maps this to a generic persistence failure, so restarting the app does not explain or repair the condition.
This is a reasonable crash/restart path and leaves existing config readable, so I classify it as P2 rather than data loss. Please use the existing process-lifetime/native file-lock primitive (or another owner-identified lease) so OS process death releases ownership and a later writer can safely remove stale compatibility residue. Add a child-process crash regression that acquires the MCP transaction, kills the writer, and proves the next store can commit.
The original P1 is otherwise closed: two independent stores and controllers now preserve unrelated concurrent additions, and credential retirement runs inside the shared config transaction. The original selection P2 is also closed: rendered row ranges drive top, paging/home/end move selection, and the new long-list test proves the visible s5 row is the ID sent to execute().
The affected Core, Storage, MCP, Runtime, Runtime Host, Eval, CLI, UI, and Desktop main builds passed. The four focused Storage/CLI/Desktop files passed 54/54 tests. The full Storage run passed 980 tests with one unrelated Node 25 ExperimentalWarning-on-stderr child-process failure and 16 platform skips. windows_recovery is green; exact-head test is still running. The branch merges cleanly with current main (a956b1ae04aa7421a749931006a6df8fe564fc60, merge tree a9f3ac700e45b0733f0fa6799842a75092a7a562). There are no review threads.
Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
Generated-by: Codex
Summary
Add the management slice for the TUI's local
/mcpview.McpClientManagersynchronization, and Host capability refresh through one TUI-owned action lane. Stale edits are rejected while unrelated concurrent config changes are preserved.Refs #3838
Review focus
McpConfigStoreremains the durable config authority,McpClientManagerremains the connection and discovery authority, and the existing capability-provider service remains the Host publication authority. The new management layer only orders one user action across those owners and reports whether a committed mutation has synchronized and published yet.The mutation path validates endpoint transitions before credential retirement, fences stale edit/import snapshots, and lets an admitted action drain before shutdown. A persistence failure cannot be reported as live, while a later manager or Host publication failure is reported as a committed partial outcome instead of rolling durable config back.
Verification
npm --workspace maka-agent test— 610 passednpm --workspace @maka/runtime test— 3,064 passed, 7 skippednpm run typechecknpm run lintnpm run format:checkgit diff --check origin/main..HEADThe full workspace run initially encountered a stale fixed-path Runtime test database under
/tmp; after preserving that directory aside, the affected test and the complete Runtime suite passed. No product code was changed for that environment issue.Not run locally: an interactive Windows TUI session. Repository CI remains the cross-platform check.
AI use
Select exactly one:
Tool(s) and scope: Codex traced the existing MCP config, credential, manager, and publication boundaries; implemented the TUI management flows; reviewed concurrency and simplification risks; and ran the verification above.
Checklist
Does this PR entail a change in behavior?