refactor(mcp): unify connection configuration and OAuth ownership - #5603
Merged
Merged
Conversation
github-actions
Bot
added
effort/XXL
Astro-Han
force-pushed
the
fix/mcp-auth-integrity
branch
4 times, most recently
from
September 23, 2026 08:57
c6e2042 to
43fa2ae
Compare
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Use one validated configuration transaction for Desktop and TUI. Remove catalog installation and rollback state, route MCP through Module Hub services, and share the aligned editor between templates and manual creation. Keep login ownership in main and expose its pending state to reopened pages. Generated-by: Codex
Put connection type in the form as a standard selector and move JSON import to a secondary header action. Remove both radio rows and their custom alignment styles. Generated-by: Codex
Keep both transport options visible as a compact form value using the library's SegmentedControl, with a visible field label and accessible group name. Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
…dations - Connection rows lead with the service mark (brand mark by URL host, otherwise a local/remote glyph on the hub's shared plate) and end with one status column built from the status vocabulary: connecting and authorizing read as active, login as attention. - Recommendations use the Skills catalog row: one Plus button writes the official endpoint and opens the new row, which lands on its login step. A recommendation whose host is already in mcp.json leaves the list; that is derived from the config, not a second install state. - Monochrome marks become masks in the plate's ink and the hardcoded white logo tile is gone, so dark mode no longer shows bright squares. The vendored SVGs are untouched. - Search only appears from eight connections; the header meta counts rows that need attention instead of raw errors. - The inspector follows the scheduled-task inspector: state banner, switch, details, tools, collapsible error output, then actions with delete last. - The editor defaults to a remote URL, drops the subtitle and header icon, and moves the JSON/manual switch into the footer. - Stories cover loading, recommendation add, local/validation/advanced add, invalid JSON, remove confirm, search no-match, login pending and the narrow inspector dialog. Generated-by: Claude Code
…dots The inspector now follows Astryx's incident-console order, which the scheduled-task inspector already uses: status line, name, state actions, facts, tools, then edit/delete. Banners, the repeated brand mark and the tool chip wall are gone; login and failure reasons read as one secondary line under the name. Status dots only mark exceptions: yellow for login required, red for failures, a grey pulse while connecting. Connected and disabled rows are plain text. Generated-by: Claude Code
The MCP, Skills and scheduled-task pages opened the selected row in a side panel inside the 900px centred column. The panel took its width from that column, so every open squeezed the rows and the whole page jumped with no transition. ModulePage now shows the selected row's detail in one Astryx Dialog (DialogHeader with the item's name and state, content, and a footer with 删除 on the left and the primary actions on the right). Only one modal is ever on screen: the editor and any toast.confirm take the detail's place, and the detail comes back when they are cancelled. The detail copy was rewritten so each row reads on its own. The MCP "现代 · <date>" protocol row is removed: it was the negotiated spec revision, which users cannot act on. Pinning a version stays in 编辑 → 高级设置 for compatibility problems. The Skills 上下文 and 运行状态 rows are removed too, since the subtitle and switch already say the same thing. Generated-by: Claude Code
Closing a detail dialog left focus on the page body, and after the editor or a delete confirm handed the detail back, closing it lost focus again. Astryx returns focus to whatever was focused when a dialog opened; for a detail handed back by another dialog that is a control of the dialog that has just closed. ModulePage now records the row that opened the detail in a layout effect, before Astryx moves focus, focuses the detail's title when it is handed back, and focuses the row when the detail closes. The scheduled-task detail story asserts the edit round trip and the return to the row, and fails without the fix. Generated-by: Claude Code
The controller cleared its busy state before re-reading the config, so for the length of that read a recommendation that had just been added was still listed and clickable. A second click wrote again and opened the editor with "name already exists" although the first add had succeeded. The operation now stays held until the refreshed config is in state. Generated-by: Claude Code
The add editor held only the active mode's data, so switching between 手动填写 and 粘贴 JSON built a fresh empty state and discarded what had been typed on the other side. The editor state now carries the manual draft and the JSON source together, and the switch only changes the mode. The add story types on both sides, switches back and forth, and asserts both survive. Generated-by: Claude Code
The editor shows environment variables and headers as one KEY=value per line. A stored value holding a line break, such as a PEM key, was split on that break when the editor was opened and saved, so an untouched field was saved as two variables. A value holding a line break, or starting with a quote, is now shown as a JSON string and read back as one. Every stored value round-trips unchanged. A typed value in quotes is read the way a .env file reads it: KEY="abc" saves abc. Generated-by: Claude Code
Desktop, the TUI and the headless capability provider all read and write the same mcp.json, but none of them noticed another process's write, and Desktop saved the whole server it had loaded. A save or toggle in Desktop could silently undo an edit made in the TUI, and a process kept running servers the file no longer described until it restarted. - Storage: McpConfigStore.subscribeChanges watches the directory (atomic replace swaps the inode) and settles a temp-write-plus-rename burst into one notification. - Desktop: mcp:upsert is replaced by mcp:update, which carries the redacted server the renderer last showed and answers `stale` when the file no longer matches, and mcp:setEnabled, which flips only `enabled` on what is on disk. Main follows external writes through the same lane as its own mutations, then syncs and notifies the renderer. - Renderer: the editor compares the live config with what it opened and shows a notice when the server was changed (save replaces it) or deleted (save is disabled). Its own save is excluded so the notice never flashes. - TUI: follows external writes through its action lane; an open import preview survives because its commit already re-checks each server. - Capability provider: re-reads and syncs on each change, one at a time. A login in flight needs no cancellation on an external change: the manager already binds each authorization round to the URL it started against. Generated-by: Claude Code
…lane Review of the previous commit found two gaps in following mcp.json. Each process read the file first and only later started watching it, so a write in between went unseen until the next one. Desktop's gap spans from boot until the Runtime Host client registers. Watching now also notifies once when it begins, so every consumer re-reads after its first read. The TUI waits for its startup instead of dropping a change that arrives during it; it waits outside its action lane so nothing queues behind a slow startup. Desktop followed a change inside the mutation lane it shares with OAuth login claims, and manager.sync waits for connections, so a slow server could hold up a login on another one for up to a minute. Following does not need the lane: Desktop's own writes already sync outside it, and each write is followed by a notification, so the last sync reads the last file. Generated-by: Claude Code
…start Desktop read mcp.json once per change notification and synced whatever each read returned, so a slow read could finish after a newer one and leave the manager on the older file with nothing left to correct it. Changes now apply one at a time, still off the mutation lane that login claims use. The TUI dropped every notification once startup had failed, and skipped a notification for a file it already held even when syncing that file had failed. Fixing a broken mcp.json now brings it back without a restart, as Desktop already did, and a failed sync is retried on the next notification. Editing a connection saves under the id it was opened with. Trimming the typed id instead turned an imported id with surrounding spaces into a server that does not exist, and the save came back stale without a word. Generated-by: Claude Code
Astro-Han
force-pushed
the
fix/mcp-auth-integrity
branch
from
September 23, 2026 12:01
955e1ad to
50c80e1
Compare
Sun-GLiang
reviewed
Sep 23, 2026
Sun-GLiang
left a comment
Sun-GLiang
left a comment
Contributor
There was a problem hiding this comment.
Automated review by Codex of 50c80e103d904f59d88f15d4f409c180510a562d.
Standards / redundancy / minimality: no confirmed P0–P2 findings. Removing the old catalog and inspector implementations reduces duplication; the shared detail dialog migration is coherent, though the PR spans several concerns.
Spec / completeness / correctness: one confirmed P2, detailed inline: a delayed configuration follower can erase OAuth credentials already issued for the successor configuration. No confirmed P0/P1.
Validation: 167 focused existing tests passed against bundles built from the reviewed source. An additional two-manager test using the PR's HTTP OAuth fixture reproduced the P2 through a real authorization/PKCE exchange and a failing subsequent reconnect. These are fixture tests, not live-account OAuth or packaged Electron validation. This automated review does not replace the independent human review required by CONTRIBUTING.md.
Astro-Han
marked this pull request as ready for review
September 23, 2026 12:47
jackwener
approved these changes
Sep 23, 2026
jackwener
left a comment
jackwener
left a comment
Member
There was a problem hiding this comment.
I reviewed 50c80e103d904f59d88f15d4f409c180510a562d.
No P0–P2.
mcp.json is the single authority. Static OAuth clients require oauth.issuer and bind tokens to that issuer and a client-config hash. Ambiguous MCP tool names hash under mcp_h__ so they cannot collide with an unmodified mcp__ name. Desktop, TUI and the capability provider watch the file and apply changes in order; a save with a stale basis writes nothing, and the enable switch flips only that field. The catalog is gone; recommendations are four official endpoints. Module details are a dialog instead of a width-stealing side panel. I did not run a packaged Electron OAuth login.
Required check test is green on this head.
简体中文
我审查了 50c80e103d904f59d88f15d4f409c180510a562d。没有 P0–P2。
mcp.json 是唯一权威。静态 OAuth 必须带 issuer 并绑定凭据。工具名哈希避免碰撞。Desktop / TUI 跟文件走，过期保存不写，开关只改 enabled。旧目录删了。模块详情改对话框。没有跑打包 Electron 的 OAuth。本 head 上 test 绿。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
… late Another process can apply the same mcp.json change (new URL or static client), retire the old credentials and sign in before this process's follower reaches the change. The follower then retired the old config again, and erase() tombstoned whatever record it read -- the successor's fresh tokens -- so the next reconnect demanded a new login. CAS cannot catch it: the new record is the erase's legitimate basis. A config-change retirement now passes the successor config into the credential lane, and erase() leaves a record already bound to that successor's URL and static client alone. Removal, logout and the pre-write erase stay unconditional, so tombstones, stdio conversions and unbound legacy records retire as before. Generated-by: Claude Code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The old MCP catalog mixed usable connections with entries that required separate setup or had no verified endpoint. This PR makes configured connections in
mcp.jsonthe single authority. The MCP page lists configured connections with a status column, then up to four directly usable recommendations: Notion, Linear, Feishu, and the official MCP docs. One click on a recommendation writes its official endpoint and opens the new connection, which lands on its login step; a recommendation whose host is already configured leaves the list. Add MCP opens a blank custom editor (remote URL by default); JSON import remains available there. The old catalog and its brand SVGs are removed.Selecting a row on the MCP, Skills or scheduled-task page opens a detail dialog instead of a side panel. The side panel took its width from the 900px page column, so every open squeezed the rows. The dialog is laid out top to bottom: name and state, the enable switch with state actions (log in / test / log out), facts, tools or run history, error output, then delete on the left and edit / use on the right. Only one modal is shown at a time: the editor and delete confirmations replace the detail, and it comes back when they are cancelled. The MCP "negotiated protocol" row is removed because users cannot act on it; pinning a protocol version is still available under Edit → Advanced. Status dots mark only exceptions: login required, failed, and a grey pulse while connecting. Connected and disabled rows are plain text.
The editor keeps what was typed when switching between filling in and pasting JSON, and environment or header values holding a line break (such as a PEM key) now survive opening and saving unchanged. An added recommendation stays locked until the refreshed configuration lands, so it cannot be added twice.
Desktop, the TUI and the headless capability provider now follow each other's writes to
mcp.json: each watches the file and reconnects to what it now describes, without a restart, including after the file was broken and then fixed. A Desktop save no longer overwrites a change made in the TUI. If the server changed while its editor was open, the editor says so and saving replaces that change on purpose. If it was deleted, saving is disabled. The enable switch flips only that field on the current file.Desktop and TUI share validated configuration writes and credential retirement. OAuth credentials are bound to the issuer and static client configuration, while transformed tool names retain unambiguous identities. A failed connection remains editable.
Recommendations are derived from
mcp.json, not a second installation state. Endpoint references: Notion, Linear, Feishu, and MCP docs. Refs #2921 for the editor UX.Recommendation marks come from the MIT-licensed Lobe Icons and Allogo repositories (or the existing Feishu mark), with pinned source revisions and third-party notices.
Migration
Static OAuth clients need
oauth.issuerand may need to sign in again. A turn interrupted while using a renamed MCP tool may need to be restarted.Verification
Focused MCP and Module Hub tests, typechecks, format, lint and the Storybook build passed locally. Cross-process tests write
mcp.jsonfrom a second store and check that Desktop main and the TUI pick the change up; they also check that a Desktop edit based on an older copy writes nothing, that a toggle keeps the other process's change, and that unsubscribing stops following. Further tests check that a change made before a process starts following, or while the TUI is still starting, is still picked up; that Desktop applies changes in the order it read them; that the TUI recovers on the next change after a failed start or sync; that a process following a URL or client change late keeps the login another process already made for the new configuration; and that following never holds up a login behind a slow connection. The capability provider's follower has no test of its own: the CLI entry has no harness, so only the shared watcher it calls is tested. The tests use two stores in one process, not two processes. All 23 MCP stories (loading, empty, configured, recommendation add, custom/local/advanced add, validation, JSON import and invalid JSON, detail, remove confirm, search no-match, edited elsewhere, editing an imported id with surrounding spaces, login required/pending, connection failed, narrow page and narrow detail) plus the Skills and scheduled-task stories ran theirplayassertions in light and dark with no console errors, broken images or horizontal overflow. The public endpoints responded, but account-authorized OAuth and packaged Electron remain unverified.Same viewport and theme,
mainon the left and this PR on the right: the MCP page, then the MCP, Skills and scheduled-task details.AI use
Tool(s) and scope: Claude Code redesigned the MCP page, moved module page details into a dialog, and updated the stories; Codex implemented the MCP refactor and browser validation; an independent Codex review checked OAuth, configuration mutation, and tool identity contracts.
Checklist
Does this PR entail a change in behavior?