feat(desktop): rework MCP editor dialog and inspector UX - #2921
feat(desktop): rework MCP editor dialog and inspector UX#2921GabrielDrapor wants to merge 3 commits into
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Codex automated review
I reviewed exact head efd243baf7b5d995a4d4c4203cec8ffba362d8bf as the final renderer/editor/inspector slice of the MCP OAuth stack. The hidden OAuth round-trip and dedicated login/logout UI close important gaps from the preceding activation PR, but the inspector still allows conflicting operations against the same server; see the inline P2 finding.
The cumulative diff is large because this is a stacked head, but the top commit is a coherent renderer slice and should not be mechanically split. The failing e2e check is the unrelated slash-command-menu timeout; the remaining checks are green, so I am not attributing it to this MCP change. No low-value production code or focused test block stood out for deletion.
Disclosure: This is an automated review performed by Codex using delegated adversarial review passes and a final evidence check. It has not been independently verified by Astro-Han or another human reviewer, does not constitute human approval, and does not represent the final judgment of a human reviewer.
efd243b to
e29ac92
Compare
|
Fixed in the updated head. Inspector operations now serialize per server through a claim/release lock ( There is no renderer component-test infrastructure in the desktop package, so the browser-callback race regression is at the lock's contract level ( |
a94d807 to
b2249c7
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The editor/inspector work is generally well factored, and centralizing OAuth transitions behind one coordinator is the right seam. Two security invariants are not yet enforced by the actual authority, though: logout terminality is only process-local, and endpoint binding is optional for existing records. The branch also conflicts with current main's newer MCP rediscovery and IPC boundaries, so those semantics must be preserved during rebase.
The simplest durable model is one persisted per-server generation/tombstone: logout atomically advances it, every flow carries and verifies it, and a credential record without a trusted endpoint binding is rejected. That replaces several process-local assumptions with one storage-level authority.
Review performed with three Codex reviewer agents and DeepSeek V4 Flash as advisory tools; I verified the findings against the latest head and current main.
中文评论
editor/inspector 的拆分总体合理，把 OAuth transition 收口到单一 coordinator 也是正确 seam。但两个安全不变量尚未由真实权威保证：logout terminality 仅限单进程，已有记录的 endpoint binding 又是可选的。该分支还与当前 main 更新后的 MCP rediscovery 和 IPC 边界冲突，rebase 时必须保留这些语义。
最简单的持久模型是每个 server 一个持久化 generation/tombstone：logout 原子推进 generation，所有 flow 携带并校验它，缺少可信 endpoint binding 的凭据记录直接拒绝。这样能用一个 storage-level authority 替代多项进程内假设。
本次审查使用了三位 Codex reviewer agents 与 DeepSeek V4 Flash 作为辅助工具；我已依据最新 head 和当前 main 复核问题。
b2249c7 to
aa2781d
Compare
📝 WalkthroughProblem solvedThis PR adds a secure MCP configuration and OAuth path. It:
Source of truthThe PR extends the existing MCP configuration store, credential store, MCP manager, runtime-host IPC, and preload bridge. It does not create a parallel renderer-owned configuration or credential authority. The main process remains the authority for secrets and OAuth state. The renderer receives redacted configuration and typed status results. The shared MCP packages remain the source of truth for transport security, OAuth state, credential transitions, and secret scrubbing. The renderer uses those contracts through IPC. Scope and complexityThe solution is coherent because the UI changes depend on the validation, OAuth, credential, and IPC contracts. The credential coordinator, generations, tombstones, endpoint binding, callback validation, and per-server operation locks address concrete race and security cases. This added complexity is necessary for logout finality, stale-write rejection, secret isolation, and concurrent server actions. The change is larger than the renderer UX slice. Its supporting MCP and storage foundation is required for the stated security behavior. Simplification opportunitiesNo safe deletion or simplification is evident from the supplied diff summary. The tests cover distinct behavior, including:
Removing these tests or merging the authorities would weaken regression coverage or security guarantees. Validation and risksValidation includes unit, contract, integration, transport-security, OAuth, storage, IPC, secret-handling, and renderer operation-lock tests. The tests cover forged callbacks, occupied ports, timeouts, aborts, issuer and endpoint validation, cleartext HTTP restrictions, cross-origin redirects, credential cleanup, stale writes, duplicate IDs, concurrent operations, tool discovery, and secret withholding. Concrete risks include OAuth flow failures, incorrect callback or issuer handling, stale credential writes, secret leakage during restoration or transport errors, endpoint changes, redirect handling, and inconsistent authentication status. Required-check status remains unverified because direct check results were not provided. Complexity deltaAdded
Removed or reduced
The PR increases absolute maintenance complexity because it adds OAuth, credential coordination, security policy, and public APIs. The increase is justified by the security and concurrency requirements. The expanded test coverage supports this conclusion. Review-relevant risks
The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughMCP support now includes OAuth authorization, persistent credential coordination, transport security, renderer secret protection, atomic server insertion, scoped IPC methods, per-server operation locking, and editor validation. ChangesMCP platform and storage
Desktop integration
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟡 Moderate · up to The PR can clear credentials for servers that remain configured when a restore fails, and can reject bulk updates involving an unchanged signed-in server, causing failed saves and unnecessary reauthentication. Owner follow-up is needed before the change is fully merge-ready. Sequence Diagram(s)sequenceDiagram
participant Renderer
participant DesktopIPC
participant McpClientManager
participant OAuthProvider
participant CredentialStorage
Renderer->>DesktopIPC: login(serverId)
DesktopIPC->>OAuthProvider: start authorization
OAuthProvider->>CredentialStorage: persist PKCE state
OAuthProvider-->>Renderer: open authorization URL
Renderer->>OAuthProvider: browser callback
OAuthProvider->>McpClientManager: finish authorization
McpClientManager->>CredentialStorage: persist OAuth tokens
McpClientManager-->>DesktopIPC: authenticated status
DesktopIPC-->>Renderer: needs-auth or connected status
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Both invariants are now enforced by a storage-level authority, implemented at the engine slice (#2653) exactly along the model you proposed — one persisted per-server generation/tombstone: P1 — logout terminality is cross-process. P2 — endpoint binding is mandatory. A record carrying credential material with no Rebase. The stack is rebased onto current |
|
/agentic_review |
Code Review by Qodo
1. Repeated query secrets disappear
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
aa2781d to
ea56530
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
apps/desktop/src/main/__tests__/mcp-ipc-main.test.ts (1)
44-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new
mcp:loginandmcp:logouthandlers.The fixtures stub
oauth.loginandoauth.logout, but no test invokes those handlers. Two new behaviors inmcp-ipc-main.tsstay unverified:
mcp:loginderivescallbackPortfromconfig.oauth?.callbackPortfor non-stdio servers only.- Both handlers run
changed(deps)infinally, so a failed login still emits and publishes.Both are observable through the existing
callsfixture. Do you want me to generate the two tests?apps/desktop/src/main/__tests__/mcp-oauth-controller.test.ts (1)
428-450: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover the RFC 9207
isspass-through.
mcp-oauth-controller.tslines 187-190 state that forwardingisstofinishAuthorizationis required for the SDK's authorization-server mix-up defense. The fixture never setsisson the redirect, and no test asserts that the controller forwards it. A regression that truncates the payload to a barecodewould pass this suite.Add an
issueIssoption tocreateOAuthFixtureand assert the captured callback payload. The existing hung-token test already captures the payload path, so the assertion is cheap.apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts (1)
17-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWiden the raw-invoke guard to any quote style.
The negative assertion only matches single quotes. A future
ipcRenderer.invoke("mcp:add", …)or backtick form passes the check. The positive assertions on Line 27 do not catch it either, because both call forms can coexist. Disposition: optional.♻️ Proposed change
- const rawMcpInvokes = preloadSource.match(/ipcRenderer\.invoke\(\s*'mcp:/gu) ?? []; + const rawMcpInvokes = preloadSource.match(/ipcRenderer\.invoke\(\s*['"`]mcp:/gu) ?? [];apps/desktop/src/renderer/mcp-page.tsx (2)
800-804: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe status label appears twice in the header.
StatusDotalready receiveslabel={state.label}on Line 802, and Line 803 prints the same string as visible text. Line 886 prints it a third time in the metadata list. The row renderer at Line 569-571 deliberately avoids this duplication. Assistive technology announces the state twice in the header alone. Consider dropping the metadatastatusLabelrow, since the header already states it. Disposition: optional.
998-1000: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelete the redundant outer condition.
editingis true only whenprops.state.mode === 'manual', so!editing || props.state.mode === 'manual'is always true. The inner{!editing && …}already gates the mode switch, and the transport switch is already gated byprops.state.mode === 'manual'. Remove the outer guard.♻️ Proposed change
- {(!editing || props.state.mode === 'manual') && ( - <div className="maka-mcp-editor-controls"> + <div className="maka-mcp-editor-controls">Close the element without the trailing
)}.As per path instructions: "Flag concrete cases where code can be deleted or simplified."
Source: Path instructions
apps/desktop/src/main/__tests__/mcp-editor-validation.test.ts (1)
109-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis loop re-tests
isLoopbackHostat a second layer.The loopback host set —
127.0.0.1,localhost,[::1],dev.localhost— is decided byisLoopbackHostinpackages/core/src/mcp.ts, not byvalidateMcpEditorDraft. The behavior this file owns is that the renderer routes remote URLs throughisNonLoopbackCleartextHttpand maps the result tourl: 'insecure-url'. One rejected host and one accepted host prove that. The remaining cases duplicate core coverage and must be updated in two places if the loopback rule changes.Confirm that
packages/corecovers the host enumeration; if it does, trim the loop here. Disposition: optional.#!/bin/bash # Description: Check for existing loopback-host coverage in the core package. rg -n -C 3 'isLoopbackHost|dev\.localhost|::1' --glob '**/__tests__/**' --glob '*.test.ts'As per path instructions: "Flag tests that duplicate existing coverage, assert implementation details, or do not protect observable behavior."
Source: Path instructions
packages/storage/src/__tests__/mcp-config-store.test.ts (1)
175-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the IPv6 loopback case to the allow list.
isLoopbackHostinpackages/core/src/mcp.tshas a dedicated'[::1]'branch. That branch depends onURL.hostnamekeeping the brackets. The loop at lines 181-185 does not exercise it. One entry closes the gap.Disposition: optional.
💚 Proposed addition
for (const url of [ 'http://127.0.0.1:8080/mcp', + 'http://[::1]:8080/mcp', 'http://localhost:3000/mcp', 'https://example.com/mcp', ]) {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4099ada3-cc79-4e1a-b2fb-765a86473c29
⛔ Files ignored due to path filters (3)
.maka-shots/after-dialog.pngis excluded by!**/*.png.maka-shots/before-dialog.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
apps/desktop/package.jsonapps/desktop/src/main/__tests__/mcp-editor-draft.test.tsapps/desktop/src/main/__tests__/mcp-editor-validation.test.tsapps/desktop/src/main/__tests__/mcp-ipc-main.test.tsapps/desktop/src/main/__tests__/mcp-oauth-controller.test.tsapps/desktop/src/main/__tests__/mcp-preload-scope.test.tsapps/desktop/src/main/__tests__/mcp-secret-guard.test.tsapps/desktop/src/main/__tests__/mcp-server-ops.test.tsapps/desktop/src/main/mcp-ipc-main.tsapps/desktop/src/main/mcp-oauth-controller.tsapps/desktop/src/main/mcp-oauth-storage.tsapps/desktop/src/main/mcp-secret-guard.tsapps/desktop/src/main/runtime-host-boot.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/locales/mcp-copy.tsapps/desktop/src/renderer/mcp-editor-draft.tsapps/desktop/src/renderer/mcp-editor-validation.tsapps/desktop/src/renderer/mcp-page.tsxapps/desktop/src/renderer/mcp-server-ops.tsapps/desktop/src/renderer/styles/module-pages/mcp.cssdocs/astryx-surface-file-inventory.mdpackages/core/package.jsonpackages/core/src/mcp-secrets.tspackages/core/src/mcp.tspackages/core/src/redaction.tspackages/mcp/src/__fixtures__/stdio-server.tspackages/mcp/src/__tests__/manager.test.tspackages/mcp/src/__tests__/oauth.test.tspackages/mcp/src/__tests__/transport-security.test.tspackages/mcp/src/credential-coordinator.tspackages/mcp/src/index.tspackages/mcp/src/oauth.tspackages/mcp/src/transport-security.tspackages/storage/src/__tests__/mcp-config-store.test.tspackages/storage/src/mcp-config-store.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review.
ea56530 to
e76c6bc
Compare
|
CodeRabbit findings addressed in the updated head:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/storage/src/__tests__/mcp-config-store.test.ts (1)
184-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining
isLoopbackHostbranches.
isLoopbackHostclaims four forms:localhost,*.localhost, IPv6[::1], and127.0.0.0/8. This test exercises onlylocalhostand127.0.0.1. The IPv6 and.localhostbranches carry the same security weight and currently have no test in this file. Add them to the existing loop.Disposition: optional.
♻️ Proposed addition
for (const url of [ 'http://127.0.0.1:8080/mcp', + 'http://127.1.2.3:8080/mcp', 'http://localhost:3000/mcp', + 'http://dev.localhost:3000/mcp', + 'http://[::1]:3000/mcp', 'https://example.com/mcp', ]) {apps/desktop/src/main/__tests__/mcp-ipc-main.test.ts (1)
151-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe masked-arg assertion can pass vacuously.
Line 216 reads
!seenScratch.args?.some(...). Ifargswere ever absent, the optional chain yieldsundefinedand the negation istrue, so the assertion passes without checking anything. The fixture setsargs, so this is not a current failure — it is a test that would stop protecting the redaction behavior after a fixture change. Assert the redacted array shape instead.Disposition: optional.
♻️ Proposed change
- assert.ok(!seenScratch.args?.some((arg: string) => arg.includes('sk-ant-api03-abcdef123456'))); + assert.ok(seenScratch.args); + assert.ok(!seenScratch.args.some((arg: string) => arg.includes('sk-ant-api03-abcdef123456')));apps/desktop/src/renderer/mcp-page.tsx (1)
998-998: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe outer guard is unreachable in one direction.
editingis true only whenstate.mode === 'manual'andeditingIdis set. So!editing || props.state.mode === 'manual'can only be false when the mode is'json'andeditingis true — a state the editor never constructs. The two inner guards on lines 1000 and 1017 already decide what renders.Disposition: optional.
♻️ Proposed simplification
- {(!editing || props.state.mode === 'manual') && ( + {(!editing || props.state.mode === 'manual') && ( <div className="maka-mcp-editor-controls">Replace the condition with
props.state.mode === 'manual' || !editing, or drop the wrapper and let the inner guards render nothing when both are false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a9320dd2-36eb-4601-9855-1eb486849c5c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
apps/desktop/package.jsonapps/desktop/src/main/__tests__/mcp-ipc-main.test.tsapps/desktop/src/main/__tests__/mcp-oauth-controller.test.tsapps/desktop/src/main/__tests__/mcp-secret-guard.test.tsapps/desktop/src/main/mcp-oauth-controller.tsapps/desktop/src/main/mcp-secret-guard.tsapps/desktop/src/main/runtime-host-boot.tsapps/desktop/src/renderer/mcp-page.tsxdocs/astryx-surface-file-inventory.mdpackages/core/src/__tests__/mcp-secrets.test.tspackages/core/src/mcp-secrets.tspackages/core/src/mcp.tspackages/mcp/src/__tests__/credential-coordinator.test.tspackages/mcp/src/__tests__/oauth.test.tspackages/mcp/src/credential-coordinator.tspackages/mcp/src/index.tspackages/storage/src/__tests__/mcp-config-store.test.tspackages/storage/src/mcp-config-store.tsscripts/generate-astryx-surface-inventory.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/astryx-surface-file-inventory.md
Included review availability: Your plan provides up to 3 included reviews per hour; 0 remain after this review.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The inspector/editor rework is visually and structurally strong, and the per-server operation controller is the right local seam. The current head is not approval-ready yet because several existing actionable threads remain open: repeated query-secret round trips, OAuth client binding to the authorization-server issuer, credential URL validation, and the OAuth controller deadline/resume boundaries. I am not duplicating those inline findings.\n\nThe existing per-server operation-lock thread is also only partially closed. Inspector actions now participate, but Marketplace → Manage can still open and save the same server while login owns the lock; saveDraft() then calls mcp.upsert() without claiming the server. The smallest UI fix is to route every edit/save entry through the same per-server claim. The stronger first-principles boundary is for the main process to reject config mutation while its OAuth controller owns an active round, because renderer state is not the authority.\n\nOne additional P3 focus-restoration issue is inline below. Current checks are green, but merge state remains blocked by the unresolved review state.\n\nReviewed with Codex using three independent reviewer agents and OpenCode Go DeepSeek V4 Flash (high); I verified the exact head, existing threads, Astryx component contracts, renderer/main-process operation boundaries, and live CI.\n\n
中文
\n\nInspector/editor 重做在视觉和结构上都不错，per-server operation controller 也是正确的局部扩展点。但当前 head 还不能批准，因为已有多个可执行线程仍未解决：重复 query secret 的 round-trip、OAuth client 与 authorization-server issuer 绑定、credential URL 校验，以及 OAuth controller 的 deadline/resume 边界。我不重复发布这些行内问题。\n\n现有 per-server operation-lock 线程也只部分关闭。Inspector action 已纳入锁，但 Marketplace → Manage 仍能在 login 持锁时打开并保存同一 server；随后saveDraft() 会在未 claim server 的情况下调用 mcp.upsert()。最小 UI 修复是让所有 edit/save 入口复用同一个 per-server claim。更符合第一性原理的边界，是主进程在 OAuth controller 持有 active round 时拒绝 config mutation，因为 renderer state 不是最终权威。\n\n下面另有一个新的 P3 焦点恢复问题。当前检查全绿，但 unresolved review 仍使 merge state blocked。\n\n本次由 Codex 配合三个独立 reviewer agent，以及 OpenCode Go DeepSeek V4 Flash（high）审查；我核验了精确 head、已有线程、Astryx 组件契约、renderer/main-process operation 边界和实时 CI。\n\n
|
This PR explicitly reworks the MCP editor and inspector UX. Could you please add screenshots before merge? At minimum, please show the add or edit dialog with live validation and advanced settings, plus the server inspector with its connection, tools, and stderr presentation. A before/after pair or one annotated composite is fine. Thanks! Posted by Codex on behalf of Astro-Han. |
e76c6bc to
1e101be
Compare
|
Everything on this slice is in the updated head: Per-server lock routing (synthesis) — OAuth client × issuer (Qodo) — implemented at the engine (#2653): discovery moving to a different authorization server drops the dynamically registered client and tokens; provider-level regression. Repeated query secrets (Qodo) — fixed in the guard slice (#2919): per-occurrence, order-preserving masking/restoring. URL credential validation (Qodo) — Focus restoration (inline P3) — a failed removal disarms |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/mcp/src/index.ts (1)
1224-1239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
abandonAuthorizationsilently does nothing when the storage view has noupdate.Line 1229 guards on
storage.updateand has no fallback. TodayflowStoragealways suppliesupdate, so the branch is total. If that ever changes, the round stays advertised bypendingAuthorizationand the boot resume rebinds a listener for a dead round — the exact failure the doc comment at Lines 1218-1223 exists to prevent.Prefer deleting the conditional over adding a fallback path:
McpOAuthStorage.updateis optional on the interface, but the value returned byflowStorageis not.Disposition: optional.
♻️ Proposed change: make the coordinator view's `update` non-optional at this call site
const storage = this.flowStorage(serverId); - if (storage.update) { - await storage.update(serverId, (basis) => { - const next = { ...basis }; - delete next.codeVerifier; - delete next.pendingRedirectUrl; - delete next.pendingServerUrl; - delete next.pendingState; - return next; - }); - } + // flowStorage always provides update; a missing one is a programming + // error, not a case to skip past in silence. + await this.requireCoordinator().transition(serverId, { epoch: this.requireCoordinator().epoch(serverId) }, (basis) => { + const next = { ...basis }; + delete next.codeVerifier; + delete next.pendingRedirectUrl; + delete next.pendingServerUrl; + delete next.pendingState; + return next; + });If you prefer the smaller edit, keep
storage.updateand narrow the return type offlowStoragesoupdateis required.apps/desktop/src/main/__tests__/mcp-secret-guard.test.ts (1)
25-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA plain edit that supplies a new header or env value has no coverage.
Every masking test round-trips the sentinel, and the rejection tests cover moved or repointed sentinels. The everyday path is missing: the renderer replaces a masked value with a real new value, and
restoreMcpConfigSecretsmust keep that new value instead of restoring the old one.Line 294-301 asserts this for
oauth.clientSecretonly, throughrestoreMcpServerSecret. Headers, env values, args, and query values have no equivalent assertion, so a regression that always prefers the previous value would pass this suite.Disposition: follow-up. Add one assertion per position kind, or one combined case.
💚 Proposed test: a supplied value wins over the stored one
+ it('keeps a supplied value instead of restoring the previous one', () => { + const previous: McpConfigFile = { + version: 1, + mcpServers: { + api: { + url: 'https://api.example.com/mcp', + headers: { Authorization: 'Bearer old-token' }, + }, + local: { command: 'npx', env: { PGPASSWORD: 'old-pg' } }, + }, + }; + const incoming = structuredClone(redactMcpConfigSecrets(previous)); + const api = incoming.mcpServers.api; + const local = incoming.mcpServers.local; + assert.ok(api && 'url' in api && api.headers); + assert.ok(local && 'command' in local && local.env); + api.headers.Authorization = 'Bearer rotated-token'; + local.env.PGPASSWORD = 'new-pg'; + + const restored = restoreMcpConfigSecrets(incoming, previous); + const restoredApi = restored.mcpServers.api; + const restoredLocal = restored.mcpServers.local; + assert.ok(restoredApi && 'url' in restoredApi); + assert.ok(restoredLocal && 'command' in restoredLocal); + assert.equal(restoredApi.headers?.Authorization, 'Bearer rotated-token'); + assert.equal(restoredLocal.env?.PGPASSWORD, 'new-pg'); + });Also applies to: 84-130
apps/desktop/src/renderer/mcp-page.tsx (1)
120-124: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
createMcpServerOpsruns on every render.
useRef(createMcpServerOps(...))evaluates its argument on each render and discards every result after the first. Construction has no side effects, so behavior is correct, but the allocation is wasted. Initialize lazily.♻️ Proposed change
- const serverOpsRef = useRef( - createMcpServerOps((ops) => { - setServerOps(new Map(ops)); - }), - ); + const serverOpsRef = useRef<ReturnType<typeof createMcpServerOps> | null>(null); + serverOpsRef.current ??= createMcpServerOps((ops) => { + setServerOps(new Map(ops)); + });Every call site already uses
serverOpsRef.current, so only the type changes.apps/desktop/src/main/mcp-ipc-main.ts (1)
85-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
McpConfigStore.insertpath.
@maka/storageis private, and no in-repository production code callsinsert; only its dedicated tests do. Remove the method and its dedicated tests. Keep duplicate detection inmcp:add'stransform, because secret restoration must use the same snapshot that the write commits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d4b9a4bb-6019-4a4a-9aee-9a31be28c8af
📒 Files selected for processing (20)
apps/desktop/src/main/__tests__/mcp-editor-validation.test.tsapps/desktop/src/main/__tests__/mcp-ipc-main.test.tsapps/desktop/src/main/__tests__/mcp-oauth-controller.test.tsapps/desktop/src/main/__tests__/mcp-secret-guard.test.tsapps/desktop/src/main/__tests__/mcp-server-ops.test.tsapps/desktop/src/main/mcp-ipc-main.tsapps/desktop/src/main/mcp-oauth-controller.tsapps/desktop/src/main/mcp-secret-guard.tsapps/desktop/src/renderer/locales/mcp-copy.tsapps/desktop/src/renderer/mcp-editor-validation.tsapps/desktop/src/renderer/mcp-page.tsxapps/desktop/src/renderer/mcp-server-ops.tspackages/core/src/mcp-secrets.tspackages/mcp/src/__tests__/credential-coordinator.test.tspackages/mcp/src/__tests__/oauth.test.tspackages/mcp/src/credential-coordinator.tspackages/mcp/src/index.tspackages/mcp/src/oauth.tspackages/storage/src/__tests__/mcp-config-store.test.tspackages/storage/src/mcp-config-store.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 1 remains after this review.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the editor and inspector consolidation. One current-head P2 remains inline: the advertised live validation suppresses every first-edit error except duplicate ID, including invalid or insecure URLs and embedded credentials.
This top layer should be restacked only after #2919, #2653, and #2920 land; inherited OAuth/IPC blockers belong in those owners and are not duplicated here. The failing astryx_surface check reports a stale inventory and should be regenerated after rebase.
This PR materially changes the MCP editor and auth inspector UX. The existing images show the earlier command/args consolidation and still use the old radio controls; they do not demonstrate this PR's segmented controls or inspector states. Please provide real before/after editor screenshots plus at least representative needs-auth and authenticated inspector screenshots. The existing AI disclosure is sufficient because the commits identify Claude co-authorship and link the contributing session.
Reviewed with Codex as an AI-assisted code review. I verified the exact-head diff, editor validation path, dependency ownership, CI, visual evidence, and provenance; no external model output was used.
中文说明
当前仍有一个 P2：所谓 live validation 在第一次编辑时只显示 duplicate ID，invalid/insecure URL、embedded credentials、unbalanced quote 等都被吞掉。继承的 OAuth/IPC 问题应回到下层 owner，不在这里重复。本 PR 明显改变 editor 和 auth inspector UX，但现有图片展示的是更早的 command/args 合并，未覆盖 segmented controls 或 inspector；请补真实 before/after 以及 needs-auth、authenticated 状态截图。AI 说明完整。
1e101be to
8b462e0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/main/mcp-ipc-main.ts (1)
62-67: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe active-login diff still compares sentinels against real secrets.
config.mcpServers[serverId]arrives from the renderer with each secret replaced by a marker, becausemcp:getConfigreturnsredactMcpConfigSecrets(...).serverholds the real on-disk value. For any secret-bearing server the two JSON strings never match, so an unchanged server is treated as changed andassertNoActiveLoginfires. A bulk write that touches nothing on the logging-in server is refused.Compare after restoring markers, for example diff
restoreMcpConfigSecrets(config, currentConfig).mcpServers[serverId]againstserver.Disposition: fix-now — the handler does not implement the invariant its own comment states.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 07eef9de-0959-4bcd-af58-ad892251245b
⛔ Files ignored due to path filters (3)
.maka-shots/after-dialog.pngis excluded by!**/*.png.maka-shots/before-dialog.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (39)
apps/desktop/package.jsonapps/desktop/src/main/__tests__/mcp-editor-draft.test.tsapps/desktop/src/main/__tests__/mcp-editor-validation.test.tsapps/desktop/src/main/__tests__/mcp-ipc-main.test.tsapps/desktop/src/main/__tests__/mcp-oauth-controller.test.tsapps/desktop/src/main/__tests__/mcp-preload-scope.test.tsapps/desktop/src/main/__tests__/mcp-secret-guard.test.tsapps/desktop/src/main/__tests__/mcp-server-ops.test.tsapps/desktop/src/main/mcp-ipc-main.tsapps/desktop/src/main/mcp-oauth-controller.tsapps/desktop/src/main/mcp-oauth-storage.tsapps/desktop/src/main/mcp-secret-guard.tsapps/desktop/src/main/runtime-host-boot.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/locales/mcp-copy.tsapps/desktop/src/renderer/mcp-editor-draft.tsapps/desktop/src/renderer/mcp-editor-validation.tsapps/desktop/src/renderer/mcp-page.tsxapps/desktop/src/renderer/mcp-server-ops.tsapps/desktop/src/renderer/styles/module-pages/mcp.cssdocs/astryx-surface-file-inventory.mdpackages/core/package.jsonpackages/core/src/__tests__/mcp-secrets.test.tspackages/core/src/mcp-secrets.tspackages/core/src/mcp.tspackages/core/src/redaction.tspackages/mcp/src/__fixtures__/stdio-server.tspackages/mcp/src/__tests__/credential-coordinator.test.tspackages/mcp/src/__tests__/manager.test.tspackages/mcp/src/__tests__/oauth.test.tspackages/mcp/src/__tests__/transport-security.test.tspackages/mcp/src/credential-coordinator.tspackages/mcp/src/index.tspackages/mcp/src/oauth.tspackages/mcp/src/transport-security.tspackages/storage/src/__tests__/mcp-config-store.test.tspackages/storage/src/mcp-config-store.tsscripts/generate-astryx-surface-inventory.mjs
🚧 Files skipped from review as they are similar to previous changes (32)
- apps/desktop/package.json
- packages/core/src/tests/mcp-secrets.test.ts
- packages/core/package.json
- packages/core/src/redaction.ts
- apps/desktop/src/renderer/mcp-server-ops.ts
- packages/mcp/src/tests/transport-security.test.ts
- packages/storage/src/tests/mcp-config-store.test.ts
- apps/desktop/src/main/tests/mcp-server-ops.test.ts
- apps/desktop/src/main/mcp-oauth-storage.ts
- scripts/generate-astryx-surface-inventory.mjs
- apps/desktop/src/renderer/styles/module-pages/mcp.css
- apps/desktop/src/preload/bridge-contract.d.ts
- apps/desktop/src/preload/preload.ts
- packages/mcp/src/tests/credential-coordinator.test.ts
- apps/desktop/src/main/runtime-host-boot.ts
- packages/mcp/src/transport-security.ts
- apps/desktop/src/renderer/mcp-editor-draft.ts
- packages/core/src/mcp.ts
- apps/desktop/src/main/mcp-secret-guard.ts
- apps/desktop/src/main/tests/mcp-ipc-main.test.ts
- apps/desktop/src/main/tests/mcp-oauth-controller.test.ts
- packages/mcp/src/fixtures/stdio-server.ts
- apps/desktop/src/renderer/mcp-editor-validation.ts
- apps/desktop/src/renderer/locales/mcp-copy.ts
- apps/desktop/src/main/mcp-oauth-controller.ts
- docs/astryx-surface-file-inventory.md
- packages/mcp/src/tests/manager.test.ts
- packages/mcp/src/credential-coordinator.ts
- packages/storage/src/mcp-config-store.ts
- apps/desktop/src/renderer/mcp-page.tsx
- packages/mcp/src/oauth.ts
- packages/mcp/src/index.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 0 remain after this review.
| // Credentials first here too: a bulk edit can REMOVE servers, and | ||
| // persisting the config deletion before the credential erase would — | ||
| // across a restart — orphan tokens a same-id re-add could inherit. | ||
| const removedIds = Object.keys(currentConfig.mcpServers).filter( | ||
| (serverId) => !Object.hasOwn(config.mcpServers, serverId), | ||
| ); | ||
| for (const serverId of removedIds) { | ||
| await deps.manager.forgetServerCredentials(serverId); | ||
| } | ||
| const next = await deps.store.transform((current) => | ||
| restoreMcpConfigSecrets(config, current), | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
A rejected restore still erases credentials for removed servers.
forgetServerCredentials runs at Line 75, before the store.transform at Line 77. restoreMcpConfigSecrets throws McpSecretRestoreError for any unrestorable marker anywhere in the payload. When it throws, the config write never lands: the "removed" servers stay configured, but their credentials are already gone. The user must log in again for servers the failed write never removed.
mcp:remove documents the opposite trade-off correctly, because there the config write cannot be rejected by restore. Here the rejecting input can belong to a different server.
Disposition: follow-up — the ordering is deliberate, but the failure path deserves a bounded fix, such as validating the restore before erasing.
|
P2 — live validation on the first edit — the first-edit gate now surfaces every non-presence error immediately (invalid/insecure URL, embedded credentials, unbalanced quote); only Also regenerated Screenshots (current head, real dev-app session): Before (current
The after shot also demonstrates this round's fix: an insecure URL errors live on the FIRST edit ("非本机地址需使用 HTTPS"), advanced settings default expanded. Inspector states (real OAuth rounds against a local fixture):
|
8b462e0 to
89670c5
Compare
|
Restacked onto latest main. The editor rework now composes with upstream's protocol work: the transport picker keeps upstream's |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the current editor/inspector pass. I re-reviewed exact head 89670c52ab299775fe5f06bc3ac6804947c99381: the prior first-edit validation, same-server operation serialization, and failed-removal focus issues are fixed. The actual editor live-validation and needs-auth/authenticated inspector screenshots satisfy the UI gate, and the Claude disclosure/provenance is complete. I resolved nine superseded threads.
One current-head P2 remains inline below: the active-login guard compares masked renderer sentinels with real stored secrets before restoration, so an unchanged secret-bearing server looks modified when a bulk edit targets a different server.
The remaining credential-first-before-validation thread (PRRT_kwDOSpfFGs6aeeCP) is the same owner invariant as #2920's canonical transaction blocker (PRRT_kwDOSpfFGs6aekEy), so I am keeping it open but not duplicating it. The stack should first fix #2920 with one serialized authoritative config/credential transaction, then incorporate this semantic active-login comparison, restack #2921, and run exact-head CI. The current workflows have not run.
AI-assisted review disclosure: OpenAI Codex performed the exact-head editor, inspector, concurrency, secret-restoration, thread, screenshot, provenance, and stack analysis; I verified the focused build evidence, reproduction, severity, deduplication, and live GitHub state before posting.
中文说明
旧的首次编辑校验、同 server 操作串行化和删除失败后的 focus 问题都已修复，9 个过时线程已关闭；真实 editor/inspector 截图与 AI provenance 也合规。
当前仍有一个 P2：active-login guard 在 secret restoration 前直接比较 renderer 的 sentinel 与 store 的真实 secret，只要 server 含被遮罩 secret，即使它语义上没改，编辑另一个 server 也会被误拒。另一个 credential-first-before-validation 线程与 #2920 的事务 blocker 属于同一 authority invariant，保留但不重复。请先在 #2920 用单一串行事务修好，再纳入本 PR 的语义比较、restack 并跑完整 CI。
| const incoming = Object.hasOwn(config.mcpServers, serverId) | ||
| ? config.mcpServers[serverId] | ||
| : undefined; | ||
| if (JSON.stringify(incoming) !== JSON.stringify(server)) assertNoActiveLogin(serverId); |
There was a problem hiding this comment.
[P2] Compare the restored semantic config before rejecting an active login
config came from the renderer with secret values replaced by sentinels, while server contains the real stored values. For any secret-bearing active-login server, these JSON strings differ even when that server was not edited, so importing or bulk-editing another server is incorrectly rejected—contradicting the per-server operation isolation this PR adds. Fold this into #2920's single serialized config transaction: restore and normalize sentinels against the transaction's authoritative snapshot first, then compute semantic changed/removed servers and apply the active-login gate before credential erasure and the conditional write. Add an active login on secret-bearing A + edit-only-B regression.
中文说明
renderer 配置含 secret sentinel，store 配置含真实 secret；直接 JSON 比较会把未修改的 A 误判为变化，导致 active login 期间编辑 B 也被拒。请纳入 #2920 的统一串行事务：先基于 authoritative snapshot restore/normalize，再做语义变化判定与 active-login gate，并补 A 登录、仅编辑 B 的回归。
89670c5 to
da54536
Compare
|
Restacked onto the fixed #2920 (head Regression added (in the #2920 tier, inherited here): with a login active on secret-bearing server A, a bulk edit that only touches server B succeeds — while a bulk edit that actually removes A is still refused with the login-in-progress error. Desktop suite at this head: 996 green; biome clean across the stack. |
da54536 to
fcc278f
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — and first a correction to how this PR reads from the outside, because it changes the review. Reviewed exact head fcc278ff03ed58eb1d2d1afdd71ee19f6ac091d5.
GitHub reports 33 files and +6415, but this is a linear child of #2920, which is a linear child of #2653, and the file lists are exact supersets. This PR's own commit is 9 files, +507/−160. The other ~5,900 lines are the two parents showing through. Anyone sizing their review off the headline number is going to over- or under-invest; the commit tab is the right way in, as the body says. For the same reason these three cannot merge in any order but #2653 → #2920 → #2921: this one does not compile without either parent, since mcp.add/mcp.login/mcp.logout arrive on the bridge contract in #2920 and isNonLoopbackCleartextHttp and the needs-auth/authenticated status vocabulary arrive in #2653.
At +507 the rework is proportionate to the problem — two banded radio blocks eating a third of the dialog height, endpoint problems surfacing only as a post-save toast, and an inspector with no vocabulary for the new OAuth states. The thing I checked hardest is the one that would have been a real finding and is not: the new renderer validator does not diverge from the store. Both call the same isNonLoopbackCleartextHttp/isLoopbackHost from @maka/core, and we executed both sides across http://[::1]:3000, http://[0:0:0:0:0:0:0:1], http://dev.localhost, http://127.1, http://LOCALHOST, http://0.0.0.0 and https://user:pass@example.com — they agree on every case, including bracket retention for IPv6. Duplicate-id is likewise advisory-live only, with the authoritative check still inside the store's serialized lane and the renderer consuming the {status:'exists'} envelope rather than racing it. That is the right shape. Also clean: no logging added anywhere in the renderer diff, toasts name fields and never values, getConfig still returns redacted secrets with sentinels restored in main, and untrusted server-supplied tool names and descriptions land in text nodes and a title attribute — no markdown renderer, no dangerouslySetInnerHTML, no command construction.
Two P2s and four P3s inline. Not approving while P1/P2 findings are open.
Two smaller notes I am not filing separately. The useRef(createMcpServerOps(...)) at line 123 re-evaluates its factory on every render — allocating a Map and three closures that are immediately discarded — and wants lazy init; harmless, but free to fix. And the commit message credits behaviour that is not in the commit: "editing a remote server carries its oauth block through the draft untouched" describes mcp-page-model.ts, whose diff against main is empty. That matters only because a reviewer trusting the message would believe it is under review here.
On packaging, see the inline notes and this summary: the natural split is to move the login/logout/needs-auth renderer half into #2920 where its IPC lives, keep validation and the upsert→add persistence change as its own revert unit, leave the actual layout rework as the small change the title describes, and take the inventory-generator line out entirely. If only one is taken, take the first — it removes this PR's compile-time dependency on #2920 and lets this stand as the pure UX change it is named for.
Review disclosure: this review was prepared with Claude Code, which read this PR's own commit at this head, executed both URL validators against a shared matrix of hostile hostnames, and ran reference searches for every claim of dead or unreferenced code. I checked each finding against the source before keeping it. Evidence grade is stated per finding. The human contributor reviewed this before posting.
| isDisabled={props.state.draft.transport === 'sse'} | ||
| width="100%" | ||
| /> | ||
| <TextArea label={props.copy.editor.headers} description={props.copy.editor.headersHelp} value={props.state.draft.headers} onChange={(value) => updateDraft('headers', value)} placeholder={'Authorization=Bearer …\nX-Workspace=…'} rows={3} /> |
There was a problem hiding this comment.
[P2] Mirror the store's OAuth/Authorization rule here, or the dialog invites an edit it cannot undo. The store's normalizeServer throws headers must not include Authorization when oauth is configured whenever a remote server carries an oauth block, and the draft carries that block through opaquely — correctly, and pre-existing — while the dialog exposes no OAuth field at all. So the placeholder on this field literally reads Authorization=Bearer … for a server where that header is rejected. Concretely: JSON-import {"mcpServers":{"notion":{"url":"https://mcp.notion.com/mcp","oauth":{"clientId":"abc"}}}}, open 编辑, type Authorization=Bearer t as the placeholder suggests, press 保存并连接 — and you get the store's raw untranslated English message in a toast, no field-level error, and no control anywhere in the dialog that can clear the invisible oauth block. The only exits are hand-editing mcp.json or deleting the server. The new validateMcpEditorDraft options bag already grew existingIds for exactly this kind of mirroring; add hasOAuth: Boolean(draft.oauth), give McpEditorErrors a headers field with an oauth-authorization-conflict code and copy, and cover it in mcp-editor-validation.test.ts. The store's assertSafeKey id rules — __proto__, over 128 characters, control characters — fall through to the same opaque toast; far less reachable, but the same gap.
| if (editing) { | ||
| next = await window.maka.mcp.upsert(serverId, serverConfig); | ||
| } else { | ||
| const result = await window.maka.mcp.add(serverId, serverConfig); |
There was a problem hiding this comment.
[P2] Extend the story bridges, since they are the only deterministic coverage this surface has. withScopedMakaBridge assigns target.maka = bridge wholesale rather than merging over a default, and all four MCP story bridges declare getConfig/listStatuses/setConfig/upsert/install/remove/cancelInstall/test/subscribeChanges and nothing else. This line changes the add path from upsert, which they have, to add, which they do not — so opening the editor story, filling a valid id and command line, and pressing 保存并连接 now throws window.maka.mcp.add is not a function, gets swallowed by the existing catch, and shows a generic save-failure toast. The story silently stops demonstrating the thing it exists to demonstrate. Worse for the new work: no story sets state: 'needs-auth' or authenticated: true, so the 登录 button, the warning Banner, the warning-tone row label and 退出登录 — the entire inspector vocabulary this PR adds — have no deterministic visual coverage at all, and apps/desktop/e2e/ has no MCP spec, so Storybook is the only lever available. Add add/login/logout to the four bridges and a story each for needs-auth and authenticated.
| const editing = Boolean(editor.editingId); | ||
| // Every edit/save entry point — the inspector's Edit, but also | ||
| // Marketplace → Manage — routes through this claim: a save must not | ||
| // race a login round (or any other operation) that owns the server. |
There was a problem hiding this comment.
[P3] The lock covers six of seven mutation entry points, and the localized copy you added for it is unreachable from the seventh. This comment says every edit/save entry point routes through the claim, but importJson calls window.maka.mcp.setConfig with no claim — it is untouched by this commit. Nothing corrupts, because main is the real authority and assertNoActiveLogin runs inside the exclusive lane; what is lost is the message. With a server parked on a browser OAuth callback, opening 通过 JSON 导入 and importing an unrelated server gets main's veto rendered as the raw English MCP server "notion" has a login in progress — wait for it to finish before changing its configuration, instead of the copy.errors.serverBusy string this PR added for exactly that situation. Claim every id in imported.mcpServers in importJson, or narrow the comment so it stops asserting an invariant the code does not hold.
| /> | ||
| <TextArea label={props.copy.editor.headers} description={props.copy.editor.headersHelp} value={props.state.draft.headers} onChange={(value) => updateDraft('headers', value)} placeholder={'Authorization=Bearer …\nX-Workspace=…'} /> | ||
| </> | ||
| <TextInput statusVariant="detached" hasAutoFocus={editing} label={props.copy.editor.url} value={props.state.draft.url} onChange={(value) => updateDraft('url', value)} isRequired placeholder="https://example.com/mcp" status={props.errors.url ? { type: 'error', message: props.errors.url === 'required' ? props.copy.editor.required : props.errors.url === 'insecure-url' ? props.copy.editor.insecureUrl : props.errors.url === 'url-credentials' ? props.copy.errors.urlCredentials : props.copy.editor.invalidUrl } : undefined} /> |
There was a problem hiding this comment.
[P3] Two small things on this line. url-credentials reaches into props.copy.errors.urlCredentials — the toast-title namespace — while its two siblings correctly read props.copy.editor.insecureUrl and props.copy.editor.duplicateId; move the string to editor.urlCredentials so field errors and toast titles stay separable. And the status prop is a four-branch nested ternary on a single ~500-character line, which is where that namespace slip hid; extracting urlStatusMessage(code, copy) next to the copy module makes the next one visible. While you are here, the className="maka-mcp-advanced" three lines down has no matching rule — a search at this head finds it only at its own definition, alongside the distinct .maka-mcp-advanced-fields that mcp.css does define. Remove it.
| */ | ||
| const MAKA_UI_ASTRYX_REEXPORTS = new Set([ | ||
| 'Badge', | ||
| 'Selector', |
There was a problem hiding this comment.
[P3] Take this line out of a UX PR. Adding Selector to MAKA_UI_ASTRYX_REEXPORTS regenerates 12 rows in docs/astryx-surface-file-inventory.md for files this PR does not otherwise touch — agent-graph-panel.tsx, bot-chat-detail.tsx, four settings pages, settings-surface.tsx, subagent-settings-page.tsx, usage-settings-page.tsx, provider-connection-detail.tsx and runtime-host-profiles-section.tsx. It is a correct one-line change and it is a separate intent: a revert of the MCP editor rework should not also revert an inventory-generator fix, and vice versa. It is a one-line mechanical PR on its own.
| @@ -0,0 +1,33 @@ | |||
| // Per-server operation lock for the MCP page: one mutation per server at a | |||
There was a problem hiding this comment.
[P3] This module does not earn its file, and its tests point at the wrong thing. createMcpServerOps is 33 lines wrapping Map.has/set/delete behind three methods plus a state mirror, with exactly one call site — a search at this head finds only the import and the useRef in mcp-page.tsx. That alone would be fine. What makes it worth mentioning is the inversion in its 55-line test file: all four cases assert the claim/release/actionFor API shape, so any correct refactor — inlining the map into a ref, or moving to useSyncExternalStore — would have to rewrite all four while changing no behaviour. Meanwhile the behaviour that actually matters, that a running login disables 测试 / 编辑 / 删除 and the Switch for that server and leaves the others alone, has no test anywhere, because there is no story or E2E for the inspector. If you are looking for something to cut, cut this; if you are looking for something to add, add that assertion instead.
Remote servers that answer 401 now surface as 需要登录 instead of a connection error. Background connects run the SDK's OAuth client against stored tokens: silent refresh works (the provider always defines a redirectUrl — leaving it undefined routes the SDK into the non-interactive token path before it ever reads the refresh token), and a connect that would need the user refuses before dynamic registration and maps to the new needs-auth state. Interactive rounds live in startAuthorization / finishAuthorization: discovery reuses the state the background 401 round persisted (including a custom resource_metadata URL from WWW-Authenticate), a challenge probe — GET, then an initialize POST at the SDK's current protocol version, pressing on past parameterless challenges — carries the 401's scope into the authorization request, dynamic client registration (static clientId/clientSecret config as fallback), PKCE, and a persisted verifier + state so the exchange survives restarts. Credentials follow the endpoint they were issued for, and deletion is terminal. The stored record carries the server URL it was minted against and every read path drops a mismatched record (an offline mcp.json edit cannot replay a token against a new endpoint). All credential state transitions flow through one coordinator: a per-server operation lane carries every read, write and delete; removing a server, changing its URL, or logging out bumps a credential epoch in-flight flows are pinned to — so a stale flow can neither read old material past a queued delete, write a refresh result over a cleared record, nor delete what a newer flow just stored. Every write stamps a monotonically increasing version validated against the basis it read, and where the backing store exposes compare-and-set, an external edit trips the check instead of being silently overwritten. A failed delete holds the server in error rather than connecting anyway, and a 401 after connect (recognized through the scrub boundary, which preserves the transport's status code) marks the server needs-auth and bumps the tool-snapshot revision so stale capabilities drop out of the Runtime Host. Authorization stays bound to where it came from. The authorization URL a server supplies is checked against the provenance of the configured endpoint — transport security is not network authority, so remotely-supplied loopback cleartext http is refused unless the user themselves configured a loopback origin. The OAuth callback travels as a typed payload that preserves the `iss` parameter for the SDK's RFC 9207 issuer mix-up check. And a configured Authorization header and OAuth are mutually exclusive on the wire: once OAuth owns a connection's authorization — configured, or evidenced by stored credentials — the static header is dropped rather than raced against the bearer token. Secrets stay contained on every path out. Where they live is enumerated once, in @maka/core/mcp-secrets — the same location plan the desktop IPC guard masks by — so the scrubber and the boundary cannot drift. Requests: one scoped fetch carries every remote and OAuth request; configured resource headers ride only the endpoint's own origin, any redirect hop that crosses an origin sheds Authorization/Cookie for the rest of the chain, and no hop may downgrade to non-loopback cleartext http. Messages and payloads: errors, status strings, stderr tails, tool-call results, structured content and tool descriptors — object keys included, since a server can smuggle a credential through a property name — are all scrubbed of the config's credential values and of everything harvested from OAuth storage traffic (access, refresh and id tokens, registered client secrets, the PKCE verifier, and the in-flight authorization code during its exchange). A value long enough to be unambiguous is substituted in place; a message containing a credential too short to splice out is withheld wholesale, because the boundary allows no third option. Tested end to end against a real authorization-server fixture, including silent refresh, revoked-session recovery, replay refusal, logout-during-refresh finality in both interleavings, external-writer CAS refusal, forged- and genuine-issuer callbacks, Authorization exclusion under stored credentials, reflected-secret scrubbing across token endpoint / resource error / tool error / success payload / metadata / object-key / authorization-code paths, short-secret withholding, bearer stripping across cross-origin redirects, cleartext-downgrade refusal, and challenge scope propagation for GET, bare-GET and strict POST-only servers. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj
login() binds an ephemeral 127.0.0.1 callback listener
(oauth.callbackPort pins it for statically registered clients) and
opens the system browser per RFC 8252 — refusing any authorization URL
that is not https or loopback http, since the URL comes from remote
OAuth metadata and a cleartext login off the machine would hand the
code exchange to the network. The listener verifies the OAuth state
round-trip before reading either the code or an error parameter, so a
forged access_denied on loopback cannot abort a real login — and it
settles with a typed payload that carries the code together with the
`iss` parameter, feeding the SDK's RFC 9207 issuer mix-up check
instead of dropping it on the floor. When a round does fail, only an
allowlisted RFC 6749 error code crosses toward the renderer; the
server-controlled error_description stays in the browser tab. A login
round interrupted by an app restart resumes at boot, from persisted
state alone: the resume claims its in-progress guard before any await,
rebinds the listener before and independent of the connect/publish
chain, treats a since-occupied port as nothing-to-resume rather than a
failure, and awaits readiness only for the final token exchange. One
deadline covers the complete round — discovery, the browser wait, and
the token exchange — so a remote endpoint that accepts a connection
and never answers cannot hold the in-progress guard or the loopback
listener; the listener closes on every exit. The deadline's abort
signal travels INTO the round — the manager aborts its requests and
fences its late storage writes on it — so a first round completing
after its timeout can neither exchange its code nor overwrite the
state of a newer round.
Tokens live in the shared CredentialStore (credentials.json, 0600) —
never in mcp.json — and the store exposes compare-and-set over the
stored record, so the runtime's credential coordinator refuses to
clobber a record something else edited underneath it.
The renderer reaches all of this through the same scoped Runtime Host
seam as every other MCP method: the handlers live on ScopedIpcMain,
whose first argument is the host ref, and a source-level contract test
pins every mcp: channel to invokeActiveRuntimeHost so a raw invoke
cannot sneak back in.
Removal is transactional in the safe direction: mcp:remove and
cancelled installs drop the server's stored credentials FIRST and
abort — config intact, retryable — if that fails, so a same-id re-add
can never inherit an orphaned token. The store gains an atomic insert
for the dialog's add path — a taken id answers as a typed
{ status: 'exists' } envelope (own-property checked) rather than as
prose fished out of a flattened IPC error string.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj
The dialog spent a third of its height on two banded radio sections before any content. Add-method and transport are mode switches, so they now take small SegmentedControls in one quiet toolbar row — the same control the 市场/已安装 switch uses. The server id shrinks to a slug-width field, the command line / URL keeps the full dialog width, and the stdio/remote extras fold into a 高级设置 Collapsible that defaults open. Validation stays on the field primitive (detached status messages, DESIGN.md §9): a colliding id surfaces live as it is typed and again from the store's atomic reject via the typed add envelope, the URL field mirrors the store's shared https-for-non-loopback rule instead of deferring it to an opaque save toast, and editing a remote server carries its oauth block through the draft untouched, so a JSON-imported static client survives the dialog. The inspector aligns with the Skill inspector archetype: the endpoint states itself once in the facts list, 删除 leaves the workaday action row for its own seat below the facts — composed through the published destructive Button variant, not an inline-ink recreation (DESIGN.md §9) — needs-auth leads with a 登录 primary action plus banner, the tool counter drops one step through the Text role system (type supporting, size xsm, tabular figures — DESIGN.md §9 counters, no literal font axes in product CSS), and every busy button — login, logout, test, delete — takes the Astryx isLoading contract whole, with no hand-swapped labels or disable-plus-spinner recreations (DESIGN.md §10). Inspector operations serialize per server: one mutation at a time, owned by a claim/release lock (inlined in mcp-page.tsx) the buttons derive their disabled state from. A login round parked on the browser callback locks out test, edit, toggle and delete for that server — none of them may race the callback against a reconnected, changed or absent server — while other servers stay fully operable. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj
fcc278f to
d6cdd67
Compare
|
Round addressed at head
Desktop suite 999 green (storybook tsconfig included); biome clean. |
2 participants
Final slice of the #2653 split: the renderer-only editor/inspector rework.
Stacked on #2920 (linear chain from the fork: #2918 → #2919 → #2653 → #2920 → this): until those merge the diff shows their commits too — review the
feat(desktop): rework MCP editor dialog and inspector UXcommit.What
Reworks the MCP editor dialog and server inspector: the add dialog validates live (duplicate ids answer as the typed
existsenvelope beside the input, at control height), advanced settings default expanded, needs-auth surfaces a 登录 action and authenticated connections a 退出登录 action, and the inspector presents connection state, transport, tool list and stderr tail consistently. CSS confined tomodule-pages/mcp.css; copy inlocales/mcp-copy.ts.Renderer-only: no main-process or engine changes in this commit.
Co-Authored-By: Claude noreply@anthropic.com
https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj