You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "Astro-Han (via GitHub)" <gi...@apache.org> on 2026/09/22 18:01:21 UTC
[PR] refactor(mcp): unify connection configuration and OAuth ownership [maka]
Astro-Han opened a new pull request, #5603:
URL: https://github.com/apache/maka/pull/5603
## Summary
MCP configuration, catalog installation, connection status and page state previously had overlapping responsibilities. A failed connection could roll back a saved installation, while the MCP page bypassed the existing Module Hub service boundary. This refactor makes configured connections the primary surface and `mcp.json` the configuration authority.
- Remove the catalog install/cancel/rollback transaction across IPC, preload, UI and tests. Templates now prefill the same form as manual creation; duplicate IDs cannot overwrite a connection, and connection failures leave saved configuration available to repair.
- Share the validated configuration mutation between Desktop and TUI: validate the complete replacement, retire credentials under the file lock, then publish. Keep each client's Host synchronization and commit-unknown reconciliation at its existing boundary.
- Move MCP page operations into the Module Hub service/controller seam. Use one aligned editor, expose login/cancel/logout, and derive pending authorization from the main-process OAuth owner so reopening the page still allows cancellation.
- Bind static OAuth registrations and stored credentials to their issuer and configuration fingerprint across restarts. Validate both success and error callback issuers through the SDK, including discovery without authorization metadata, and avoid reflecting remote errors.
- Give transformed MCP tool names a separate hashed namespace, eliminating deterministic collisions with unchanged names. Runtime Host remains the execution authority; the SDK remains the OAuth and transport implementation.
The replaced paths are deleted in this PR. Existing credential generations/CAS, transport handling and client-specific publication semantics remain because they protect real lifecycle boundaries. The complete diff removes 90 lines net, including regression tests. Refs #2921 for the overlapping editor/inspector UX work; this PR also changes configuration and OAuth ownership.
## Migration
- Existing static OAuth clients must specify `oauth.issuer`. Old static credentials without the new registration binding require login again. Dynamically registered clients do not require static client configuration.
- Ordinary short tool names remain unchanged. Overlong, sanitized or separator-ambiguous names move to the hashed namespace. An interrupted task containing an old affected name can fail the automatic continuation tool-catalog safety check after upgrade. History is retained and a new turn can proceed. This is an intentional fail-closed upgrade limitation: names remain a function of identity, rather than changing when unrelated tools enter or leave the catalog.
- JSON import retains its explicit same-ID update behavior. Templates and manual create use insert-only semantics.
## Verification
- OAuth issuer, credential retirement, configuration and callback regressions exercised real local HTTP/loopback fixtures; the final static-registration restart/OAuth/controller selection passed **62 tests**.
- Desktop/TUI configuration mutation and commit-unknown selection: **42 passed**. Final Module Hub controller selection: **4 passed**, including host switching, stale reads, saved configuration after connection failure, duplicate creation, and cancel after reopening.
- Runtime projection and related OAuth/native-capability regression selection: **103 passed**. Independently reproduced the documented old-name continuation limitation through the real Runtime Host continuation fixture.
- Affected workspace builds, Desktop typechecks, `npm run format`, `npm run lint`, and `git diff --check` passed.
- Storybook build and browser DOM/geometry/play/console/accessibility checks passed for connections, templates, editor, narrow editor, login-required and connection-failure states in light/dark themes. Duplicate-create and OAuth form interactions were also checked through DOM events.
- Removed tests for the deleted install rollback mechanism; retained create non-overwrite, durability, credential lifecycle and Host-boundary protections.
- No full-repository suite, packaged Electron acceptance, or external-account OAuth login was run. CI has not been awaited.
The editor comparison uses the same story and 1280×900 viewport, with BEFORE from main `6cb8c5808` and AFTER from this branch.
## AI use
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: Codex implemented the refactor, regression tests, documentation and browser validation; an independent Codex review checked OAuth, configuration mutation and tool identity contracts.
## Checklist
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(mcp): unify connection configuration and OAuth ownership [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han merged PR #5603:
URL: https://github.com/apache/maka/pull/5603
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(mcp): unify connection configuration and OAuth ownership [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on code in PR #5603:
URL: https://github.com/apache/maka/pull/5603#discussion_r4082314051
##########
apps/desktop/src/main/mcp-ipc-main.ts:
##########
@@ -381,23 +317,30 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): void {
changed(deps);
}
});
+ // Another process (the TUI, another window) replacing mcp.json is followed
+ // the way a change made here is. A login in flight needs nothing: the
+ // manager binds each round to the URL it started against. This stays off
+ // the mutation lane, where a slow connect would hold up login claims. Changes
+ // apply one at a time, so the last sync is of the last file read.
+ let following = Promise.resolve();
+ return deps.store.subscribeChanges((error) => {
+ if (error) {
+ console.error('[mcp] stopped following mcp.json changes:', error);
+ return;
+ }
+ following = following
+ .then(async () => {
+ await deps.ensureReady();
+ await deps.manager.sync(await deps.store.get());
Review Comment:
**P2 — trigger ② (cross-process convergence): a late follower can revoke the new configuration's completed login.** Both managers start on static client A; Desktop changes the same server to client B and signs in, while the TUI/provider is still behind a slow sync or action. When that follower reaches `manager.sync(latest)`, `syncNow()` sees the old→new registration change and calls `forgetAuthorization(oldConfig)` again. The old config is only used for the stdio shortcut; `coordinator.erase()` reads the *current* record and tombstones it without checking `serverUrl` / `clientConfigHash`. CAS does not prevent this: the new login's record is the erase's legitimate read basis.
Reproduced on this SHA with two real managers sharing OAuth storage and the existing HTTP OAuth fixture: change only `clientId`, complete authorization + PKCE on A (`connected`, authenticated, token stored), then sync B. The token disappears, generation advances 1→2, and A's reconnect fails with `requires interactive authorization`. The new automatic followers make this an ordinary convergence path; each follower's local queue does not serialize credential ownership across processes.
Smallest fix: make config-driven retirement conditional on the *previous* URL/static-client binding inside the credential lane/CAS transition, so following an already-retired configuration cannot erase its successor's tokens or pending round. Keep explicit logout unconditional, and add the two-manager login→late-follow regression.
<details>
<summary>中文</summary>
两个进程先持有旧静态客户端配置。A 改成新 client 并完成登录，B 稍后跟随时会再次执行凭据退休；`forgetAuthorization(oldConfig)` 没有核对记录归属，而是直接把当下的新登录记录清空。CAS 防不住，因为读取的新记录正是本次清理的合法 basis。用真实 OAuth fixture 已复现：A 登录成功 → B 同步 → token 消失、generation 1→2 → A 重连要求重新登录。
最小修复：配置变更触发的退休在 credential lane/CAS 内核对旧 URL 与静态客户端绑定，只清理仍属于旧配置的凭据；显式退出登录继续无条件清理。补充双 manager 的“新登录完成后，另一端迟到同步”测试。
</details>
_Automated review by Codex._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(mcp): unify connection configuration and OAuth ownership [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #5603:
URL: https://github.com/apache/maka/pull/5603#discussion_r4083094654
##########
apps/desktop/src/main/mcp-ipc-main.ts:
##########
@@ -381,23 +317,30 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): void {
changed(deps);
}
});
+ // Another process (the TUI, another window) replacing mcp.json is followed
+ // the way a change made here is. A login in flight needs nothing: the
+ // manager binds each round to the URL it started against. This stays off
+ // the mutation lane, where a slow connect would hold up login claims. Changes
+ // apply one at a time, so the last sync is of the last file read.
+ let following = Promise.resolve();
+ return deps.store.subscribeChanges((error) => {
+ if (error) {
+ console.error('[mcp] stopped following mcp.json changes:', error);
+ return;
+ }
+ following = following
+ .then(async () => {
+ await deps.ensureReady();
+ await deps.manager.sync(await deps.store.get());
Review Comment:
Fixed in c133c68a6. A config-change retirement now passes the successor config into the credential lane, and `erase()` skips a record already bound to that successor's URL and static client. Removal, logout and the pre-write erase stay unconditional. I didn't retire "only what belongs to the old config": it would leave orphans bound elsewhere after a removal, which the existing removal test rejects. The regression test ('following a URL change late keeps the login already made for the new URL') uses one late manager plus a record already bound to the new URL, instead of a second full login. It exercises the same check; the URL change and the clientId-only change share that code path.
<details>
<summary>中文</summary>
已在 c133c68a6 修复。配置变更触发的凭据退休现在会把后继配置一起传进 credential lane；`erase()` 在 lane 内读到记录后，如果它已经绑定到后继配置的 URL 和静态客户端，就跳过不清。删除服务器、显式退出登录、写入前的预清理仍然无条件清理。没有采用"只清属于旧配置的凭据"：那样删除服务器时，绑定在别处的孤儿记录会留下来，现有的删除测试不接受这种结果。回归测试（'following a URL change late keeps the login already made for the new URL'）用一个迟到的 manager，加一条已绑定到新 URL 的记录，代替第二次完整登录。它验证的是同一处判断；改 URL 和只改 clientId 走的是同一条代码路径。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org