feat(app): MCP management GUI in Settings → Integrations - #1505
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR implements full-stack MCP server management: raw global configuration reads and atomic edits, HTTP/OpenAPI endpoints, client synchronization, Settings UI add/edit/toggle/delete flows, lossless command parsing, localization, and automated tests. ChangesMCP Server Management
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsMcp
participant GlobalSync
participant GlobalAPI
participant ConfigService
User->>SettingsMcp: add/edit/toggle/delete MCP server
SettingsMcp->>GlobalSync: editMcp(set/remove/enable)
GlobalSync->>GlobalAPI: POST /global/config/mcp
GlobalAPI->>ConfigService: editGlobalMcp(input)
ConfigService-->>GlobalAPI: changed and missing
GlobalAPI-->>GlobalSync: edit response
GlobalSync->>GlobalAPI: GET /global/config/mcp
GlobalAPI->>ConfigService: getGlobalMcpRaw()
ConfigService-->>GlobalAPI: raw MCP entries
GlobalAPI-->>GlobalSync: mcpRaw
GlobalSync-->>SettingsMcp: updated server list
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Astro-Han
added
enhancement
github-actions
Bot
added
ui
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/components/dialog-mcp-form.tsx, packages/app/src/components/settings-integrations-parts.tsx, packages/app/src/components/settings-mcp.tsx, packages/app/src/context/global-sync.tsx, packages/app/src/i18n/en.ts, packages/app/src/i18n/zh.ts, packages/app/src/pages/settings/integrations.tsx)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
Non-technical users can add, edit, enable/disable, and delete global MCP servers from Settings → Integrations without hand-editing pawwork.json (issue #1485). Engine: add Config.editGlobalMcp, an atomic add/edit/rename/delete path for the mcp subtree. The merge-based updateGlobal can only add or override keys (mergeDeep and patchJsonc never remove), so real deletion and rename need a dedicated write. It seeds scattered global sources into the primary file first (so a delete is not shadowed by a copy in a sibling loaded file), applies removals + sets in one atomic write on the primary file (a rename is set(new)+remove(old), never observable half-applied), strips any residual shadow from sibling loaded files, and returns the removal names not found in any loaded global file so the route can 404. Exposed over POST /global/config/mcp (global.config.editMcp) with the SDK regenerated. Frontend: SettingsMcp renders an inline list in the Integrations page whose presence is driven purely by the fresh global config (so a deleted server disappears at once, no stale child-store dependency); add/edit/delete happen in a focused DialogMcpForm reusing the shared Dialog/TextField/segmented chrome. Management scope is global-only per the v1 decision. Verification: engine unit tests (71 pass across config + routes), app typecheck + lint clean, real-backend E2E add→edit→delete green, and a mcp-form snap grid reviewed in light and dark.
Config.Mcp is a zod-bridge value/type whose inferred type does not survive the test-project boundary, so cross-file references from the edit handler and its test failed typecheck (TS2694). Point both at ConfigMCP.Info(.zod) directly.
Astro-Han
force-pushed
the
feat/1485-mcp-management-gui
branch
from
July 9, 2026 17:16
1254a09 to
82fffa1
Compare
The no-dead-tokens guard bans standalone weight utilities; the active tab's emphasis comes from bg-surface-interactive-base + text-fg-strong, which is the design-system idiom (weight is baked into the typography token).
…ence, a11y
Three findings from code review of the MCP management GUI:
- P1: a local MCP `command` is an argv array passed straight to the stdio
transport (no shell), but the form flattened it with `join(" ")` and rebuilt
it with `split(/\s+/)`, so a single argv containing spaces (e.g. a path like
"/Users/me/My MCP/server.js") was silently chopped into two on edit, breaking
a previously-working server. Replace the lossy split/join with a minimal,
quote-aware, reversible split/join (mcp-command.ts) and lock the contract with
round-trip tests. Whitespace separates argv; single/double quotes protect
inner spaces; no shell semantics are introduced.
- P2: editGlobalMcp scanned every sibling global config file unconditionally,
even on a pure add that removes nothing, and parsed each with a throwing
jsonc parser after the primary file had already been written — a corrupt
unrelated sibling would surface as "save failed" while the write had in fact
succeeded. Only removals can be shadowed by a sibling, so skip the scan when
there is nothing to remove, and tolerate a malformed sibling (it is skipped by
the loader anyway) instead of failing the edit, matching PawWork's broken-file
resilience.
- P3: the env/header row remove button used the add button's aria-label, so
screen-reader users heard "Add variable/header" on a delete control. Give
KvRows a dedicated removeLabel with its own i18n keys.
…& oauth resilience
Two reviewers (codex gpt-5.6-sol, ChatGPT) independently converged on the first
two; the rest are codex-only. All verified against the code.
- Command round-trip was still lossy for an argv containing BOTH quote
characters (e.g. a `node -e` / `sh -c` payload like `console.log("it's ok")`).
Make split/join total via backslash escaping inside double quotes, so every
argv round-trips; extend the round-trip tests with mixed-quote and backslash
cases.
- editGlobalMcp's broken-sibling tolerance did not cover the first write: before
the primary config file exists, updateGlobal({}) seeds from every loaded
source, and a corrupt legacy source threw before the sibling try/catch, failing
all MCP edits. Make the PawWork seed skip a source it cannot parse/normalize
(the loader skips it too); plain opencode still fails fast. Add a first-write
test with a broken legacy source.
- Editing a remote MCP dropped an explicit `oauth: false` (which disables OAuth
auto-detection) because buildConfig used a truthy check; preserve it with
`oauth !== undefined`.
- The enable/disable Switch had no accessible name, so a screen reader could not
tell which server it toggled; add an aria-label with the server name (new
settings.mcp.toggle key, en/zh).
Rejected: a "legacy { enabled: false } MCP" cast finding — the MCP schema is a
strict Local|Remote union (type + command/url required), so that shape cannot
reach config.mcp.
…h a11y
Round-2 review (codex gpt-5.6-sol max, fresh eyes) surfaced four issues in the
MCP management GUI; this fixes all of them.
Secret leak on toggle/edit (P1): the list rendered and wrote back from
`config.mcp`, which the engine returns with `{env:...}` / `{file:...}` already
expanded, so toggling a server persisted the resolved secret to disk. Add
`GET /global/config/mcp` (Config.getGlobalMcpRaw) returning the literal,
unexpanded mcp subtree merged across loaded global files (PawWork-tolerant of a
broken sibling). The UI now renders from a new `mcpRaw` store slice and a toggle
uses a new field-level `enable` op on editGlobalMcp that patches only the
`enabled` key in place, leaving the rest of the entry byte-identical.
Enabled-only overrides (P1): a legacy `{ enabled: false }` entry is valid config
but has no type, so the old list force-cast it and a toggle 400'd / an edit
opened a blank form. Raw entries are now typed as a union; such entries can be
toggled but the edit affordance is hidden (only local/remote configs are
editable).
Windows path corruption (P1): a backslash inside double quotes unconditionally
consumed the next char, so a pasted `"C:\Program Files\app.exe"` lost its
separators. Only `\"` and `\\` are escapes now; any other backslash stays
literal. Added direct-input tests.
Switch accessible name (P2): the `aria-label` landed on the outer role="group",
never the inner role="switch" input. Use the shared Switch's `hideLabel` +
children so the name reaches the input (verified in E2E via getByRole).
Verification: opencode config+server unit tests (738 pass), app unit tests
(1989 pass), app + opencode typecheck and lint clean, real-backend E2E
add→edit→toggle→delete green. SDK regenerated.
…ip gaps
Round-3 review (codex gpt-5.6-sol max, fresh eyes) found three remaining P1s in
the raw-config edit path; this closes all of them.
Sibling shadow on overwrite: editGlobalMcp only stripped removed names from
sibling loaded files (e.g. a hand-created pawwork.json next to pawwork.jsonc).
A `set` writes a full entry to the primary, but the loader deep-merges sources,
so a stale sibling copy re-merged its old sub-fields back onto the new entry — a
deleted Authorization header kept being sent. The sibling scan now also strips
every `set` name (an `enable` override is still left alone, since it is meant to
shadow-merge onto a sibling's full entry).
Relative {file:...} rebasing: getGlobalMcpRaw returned literal file text, so a
relative reference captured from a legacy config dir resolved against the wrong
base once an edit wrote the entry into the PawWork Home primary. It now rebases
relative {file:...} to absolute against each source file's own dir, exactly as
the seed migration does. {env:...} stays literal (no secret resolution).
Schema-invalid entries: getGlobalMcpRaw only skipped JSONC syntax errors, so a
structurally-valid but schema-invalid file (e.g. `{ mcp: { x: null } }`) merged a
null entry that crashed the Integrations page on `"type" in config`. It now
validates each file against the schema and skips it whole in PawWork (mirroring
the loader), and asEditable guards non-object entries defensively.
Verification: opencode config+server unit tests (741 pass, +3 new), app unit
tests (1989 pass), full typecheck + lint clean, real-backend E2E
add→edit→toggle→delete green. No schema change, so the SDK is unchanged.
… allow override delete
Round-4 review (codex gpt-5.6-sol max, fresh eyes) found two P1s and one P2 left
in the raw-config edit path; this closes them.
Sibling revival (P1): the sibling scan only rejected bad JSONC, not schema-
invalid files. A sibling the loader drops whole (e.g. `{ "model": "x", "mcp": {
"srv": null } }`) would have `srv` stripped when a same-named server was added,
turning the file valid and silently activating its other config (model, plugins)
on the next load. The scan now validates each sibling against the schema and
leaves an invalid one untouched, mirroring the loader (PawWork skips it, plain
opencode fails fast).
Nested placeholder corruption (P1): the `{file:...}` rebase used a `[^}]+`
capture that truncates at the inner brace of `{file:{env:HOME}/token}`, so a
no-op save could rewrite the path to garbage. The rewriter now leaves any
`{file:...}` whose path contains a nested placeholder literal — the loader
expands the inner `{env:...}` and resolves the real path at load time. This also
fixes the same latent bug on the seed migration path.
Legacy override delete (P2): a `{ enabled }` override has no editable form, so
the edit button (which hosts delete) was hidden and the entry could only be
toggled, never removed. Such rows now get an inline two-click delete confirm in
the row itself.
Verification: opencode config+server unit tests (743 pass, +2 new), app unit
tests (1989 pass incl. i18n parity), full typecheck + lint clean, real-backend
E2E add→edit→toggle→delete green. No schema change, so the SDK is unchanged.
…CP edits
- editGlobalMcp sibling scan now substitutes {env:}/{file:} placeholders
before parse+normalize+schema, matching the loader: a sibling the loader
skips (missing env, bad file ref) is left untouched, so stripping a key can
never make it loadable and silently activate its model/plugins.
- updateGlobal and editGlobalMcp primary validation normalize before schema,
so a config still carrying a deprecated key (theme/keybinds/tui) — which the
loader accepts — no longer blocks global settings or MCP edits.
- joinCommand prefers single quotes for backslash/quote args (Windows paths),
so a stored C:\path reads back as 'C:\path' instead of doubling every
backslash; round-trip contract preserved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/server/routes/instance/httpapi/handlers/global.ts`:
- Around line 94-98: The EditMcpConfigInput schema in global.ts currently allows
omitted fields but not explicit nulls, which conflicts with the published POST
/global/config/mcp contract. Update the request parsing in the handler around
config.editGlobalMcp and EditMcpConfigPayload so null values for set, remove,
and enable are either normalized to undefined before validation or the schema is
changed to reject nullable inputs consistently. Keep the fix centered on
EditMcpConfigInput and the global config MCP route so parseJsonBody() accepts
the documented payload shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 54572ad9-6c17-442d-9c55-acc55cd25a17
⛔ Files ignored due to path filters (2)
packages/sdk/js/src/v2/gen/sdk.gen.tsis excluded by!**/gen/**packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (19)
packages/app/e2e/settings/settings-mcp.spec.tspackages/app/e2e/snap/mcp-form.snap.tspackages/app/src/components/dialog-mcp-form.tsxpackages/app/src/components/mcp-command.test.tspackages/app/src/components/mcp-command.tspackages/app/src/components/settings-integrations-parts.tsxpackages/app/src/components/settings-mcp.tsxpackages/app/src/context/global-sync.test.tspackages/app/src/context/global-sync.tsxpackages/app/src/context/global-sync/bootstrap.tspackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/settings/integrations.tsxpackages/opencode/src/config/config.tspackages/opencode/src/server/routes/instance/httpapi/groups/global.tspackages/opencode/src/server/routes/instance/httpapi/handlers/global.tspackages/opencode/test/config/pawwork-global-config.test.tspackages/opencode/test/server/config-routes.test.tspackages/sdk/openapi.json
…dator EditMcpConfigPayload used Schema.optional for set/remove/enable, which admits undefined and serializes to nullable fields in the OpenAPI/SDK contract — but the handler's zod validator rejects an explicit null with a 400. Switch to Schema.optionalKey (the same idiom already used for the upgrade payload) so the published contract is "key may be omitted" and agrees with runtime validation. Regenerated openapi.json and the SDK types (set?/remove?/enable? lose | null).
13 tasks
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
1 participant
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
Adds a GUI to Settings → Integrations for managing global MCP servers: add, edit, enable/disable, rename, and delete — no more hand-editing
pawwork.json.Config.editGlobalMcp— an atomic add/edit/rename/delete path for themcpsubtree. The merge-basedupdateGlobalcan only add or override keys (mergeDeepandpatchJsoncnever remove), so real deletion and rename need a dedicated write. It seeds scattered global sources into the primary file first (so a delete is not shadowed by a copy in a sibling loaded file), applies removals + sets in one atomic write on the primary file (a rename isset(new)+remove(old), never observable half-applied), strips any residual shadow from sibling loaded files, and returns the removal names not found in any loaded global file so the route can 404. Exposed overPOST /global/config/mcp(global.config.editMcp); SDK regenerated.SettingsMcprenders an inline list whose presence is driven purely by the fresh global config, so a deleted server disappears at once with no stale child-store dependency. Add/edit/delete happen in a focusedDialogMcpFormreusing the shared Dialog / TextField / segmented chrome (local vs remote, env / header key-value rows, inline delete confirm).Why
Issue #1485: a non-technical user has to hand-edit
pawwork.jsonto add an MCP server, gets the schema wrong, and the desktop app becomes unusable. PR-A (#1500) made config load degrade gracefully; this PR-B removes the need to touch the file at all for the common case. Management scope is global-only by design for v1 — project-scoped MCP servers remain in project config and are not surfaced in this list.Related Issue
#1485 (PR-B; follows PR-A #1500).
Human Review Status
PendingReview Focus
Config.editGlobalMcpcorrectness: the seed-before-delete step (no revival), the single atomic write for rename, sibling-file shadow stripping, and themissing[]404 semantics.Risk Notes
editGlobalMcpwrites and deletes keys in the global config file(s). It preserves JSONC comments/sibling keys (covered by a test) and only touches themcpsubtree.docs/design/preview/screenshots/which is a local-only path per repo convention (git-excluded), so it is not committed. See Screenshots below for how to regenerate it.How To Verify
Screenshots or Recordings
The
DialogMcpFormadd/edit surface, light and dark, reusing the shared Dialog/TextField/segmented chrome (warm neutrals, single orange accent, hairline borders). Regenerate withbun run snap mcp-form→docs/design/preview/screenshots/mcp-form.png. The PNG is a local-only artifact (that path is git-excluded by repo convention), and the same capture is produced by thee2e-artifactsCI job.Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit