feat(plugins): plugins, MCP, and skills management on the provider-plugin model - #344
feat(plugins): plugins, MCP, and skills management on the provider-plugin model#344lucas77778 wants to merge 27 commits into
Conversation
Codex has no plugin enable/disable RPC and no such CLI subcommand — enablement is the config value plugins."<id>".enabled, which config/value/write persists into config.toml. Install and uninstall ride plugin/install and plugin/uninstall, which share plugin/read's marketplace addressing rule. Toggling resolves its target through plugin/installed first, so a caller-supplied id never reaches the TOML key path and an uninstalled plugin fails here instead of leaving a stray config entry.
Bumps the wire protocol: two new requests plus an optional pendingAuthApps on plugin.updated, which carries the provider apps an install left unauthorized — codex reports them for most of its catalog and LinkCode runs no OAuth flow. The router had no case for the new kinds and its default arm is a silent Effect.void, so an unrouted request would hang the client with no error. Every mutation now ends in one shared readback: the providers blind-write, so re-listing is the only proof the change landed. An uninstalled plugin stays in the marketplace catalog with no installations, so both directions reply with the same shape.
The three plugin mutations share one reply kind, so they share one pending tag; setPluginEnabled now resolves with the mutation result rather than a bare plugin. Market gains an Install action and Plugins a confirmed Uninstall, both gated on the plugin's own managementCapabilities so claude cards stay read-only. Neither needs its own cache handling: the Plugins/Market split is derived from installations, so patching the one replaced entry moves it between tabs. An install that leaves apps unauthorized says so in a toast instead of reading as finished.
The remote curated catalog listed `metabase` twice among 2321 live entries. Ids are the normalized model's identity, so a duplicate reaches the client as two cards sharing one key; keep the installed copy.
The live remote catalog is 2300+ entries and plugin/read costs ~160ms each, so detailing the whole catalog blew the 30s discovery deadline and the Plugins page reported codex discovery as failed. Measured on 0.144.1: plugin/list 3.3s, plugin/installed 14ms, one plugin/read ~160ms. Market cards now carry no component list; their identity, description, category and keywords come from the catalog summary, which already has them.
# Conflicts: # packages/foundation/schema/src/wire/message.ts # packages/host/engine/src/session/lifecycle-service.ts
macOS `tmpdir()` is a symlink into /private/var and git reports the resolved form, so the recorded repoRoot never matched the fixture's raw mkdtemp path — two worktree-service tests failed on every macOS checkout while Linux CI, where /tmp is a real directory, stayed green.
# Conflicts: # .release-please-manifest.json # apps/daemon/drizzle/meta/_journal.json # apps/daemon/package.json # apps/daemon/src/__tests__/config.test.ts # apps/daemon/src/config.ts # apps/daemon/src/index.ts # apps/daemon/src/provider-store.ts # apps/desktop/CHANGELOG.md # apps/desktop/package.json # apps/mobile/src/app/host/[hostId]/session/[sessionId].tsx # apps/webview/src/shell/web-workbench-shell.tsx # packages/client/core/src/client.ts # packages/client/core/src/client/control-channel.ts # packages/client/sdk/src/client.ts # packages/client/sdk/src/operations.ts # packages/client/workbench/src/surface/use-workbench-sessions.ts # packages/foundation/schema/src/wire/index.ts # packages/foundation/schema/src/wire/message.ts # packages/foundation/schema/src/wire/session.ts # packages/host/engine/src/__tests__/engine-session-input.test.ts # packages/host/engine/src/__tests__/fixtures/history-adapter.ts # packages/host/engine/src/engine.ts # packages/host/engine/src/session/history-service.ts # packages/host/engine/src/session/lifecycle-service.ts # packages/host/engine/tests/integration/worktree-service.test.ts # packages/presentation/i18n/src/locales/en.ts # packages/presentation/i18n/src/locales/zh-cn.ts # pnpm-lock.yaml
Zerlight
left a comment
Zerlight
left a comment
There was a problem hiding this comment.
I found three high-priority safety issues and seven correctness or UX issues. The most serious problems are that custom MCP credentials bypass the existing secret vault, while Claude skill toggles target the wrong settings layer and can weaken settings-file permissions. Please address the inline comments before merging.
Verification: 136 focused tests passed. In the full suite, 2,312 tests passed and one unrelated timeout passed immediately when rerun in isolation. The wire-version change from 66/66 to 67/67 is consistent with the new required config.get.result.customMcpServers field.
|
|
||
| /** Persist custom MCP servers, preserving other fields; `0600` (env/headers may hold secrets). */ | ||
| export function saveCustomMcpServers(servers: CustomMcpServer[]): void { | ||
| writeConfigField('customMcpServers', servers); |
There was a problem hiding this comment.
[P1] Store custom MCP credentials in the vault
This writes each complete CustomMcpServer, including stdio env values and HTTP headers, directly to config.json. These fields commonly contain long-lived credentials but bypass the existing encrypted/keyring-backed secret vault. Mode 0600 does not protect backups or Windows storage semantics, and a pre-existing loose file is written before the later chmod. Please add an MCP vault namespace and persist only structure plus secret references here.
| } | ||
|
|
||
| private settingsFileFor(scope: StandaloneSkill['scope'], cwd: string | undefined): string { | ||
| if (scope === 'project' && cwd) return join(cwd, '.claude', 'settings.json'); |
There was a problem hiding this comment.
[P1] Write project skill overrides to settings.local.json
For a project skill this targets the shared .claude/settings.json, although Claude's /skills UI saves personal visibility choices to .claude/settings.local.json (official documentation); this adapter also reads the local file after the project file. With an opposite local override, the mutation dirties the shared config and then fails readback because the local value still wins. Without one, a personal UI action still modifies potentially tracked team configuration.
| const temporaryFile = `${file}.${process.pid}.${randomUUID()}.tmp`; | ||
| try { | ||
| await mkdir(dirname(file), { recursive: true }); | ||
| await writeFile(temporaryFile, `${JSON.stringify(value, null, 2)}\n`, 'utf8'); |
There was a problem hiding this comment.
[P1] Preserve settings-file permissions during atomic writes
This temporary file is created with Node's default 0666 mode masked by the process umask (0644 under the common 022 setting). The following rename replaces the original inode, so toggling any skill can downgrade an existing 0600 ~/.claude/settings.json and expose settings, environment values, or hooks to other local users. Create the temporary file with the original mode, or at least 0600, before renaming it.
| /** Client-minted, stable across edits (mirrors the Account.id precedent). */ | ||
| id: z.string().min(1), | ||
| enabled: z.boolean(), | ||
| server: McpServerSchema, |
There was a problem hiding this comment.
[P2] Reject blank custom MCP names before persistence
McpServerSchema accepts an empty name, and the form's min(1) check runs before trimming, so a whitespace-only name is persisted as an empty string. Starting an unsupported agent then creates a warning with an empty serverName, but McpWarningSchema rejects it; the transport drops the entire session.started frame, leaving the start promise pending after the daemon has already created a live session. Please enforce a trimmed non-empty name at both the persistence and form boundaries.
| this.providers.setAccounts(accounts), | ||
| ), | ||
| ).pipe( | ||
| Effect.andThen( |
There was a problem hiding this comment.
[P2] Validate the whole config.set request before committing fields
When one payload contains provider/account changes and a custom MCP patch, the earlier fields are persisted before applyPatch performs its conflict checks. A duplicate ID, duplicate name, reserved name, or storage failure therefore returns request.failed while the provider/account changes remain committed. Build and validate one next-state snapshot before writing, or make each wire request modify only one configuration resource.
| id: string, | ||
| opts: PluginDiscoveryOptions = {}, | ||
| ): Promise<PluginInstallOutcome> { | ||
| return this.withDiscoveryServer(async (server) => { |
There was a problem hiding this comment.
[P2] Do not apply the discovery timeout to installs
installPlugin (and uninstallPlugin) runs inside withDiscoveryServer, whose unconditional 30-second timer aborts and closes the app server. A valid network, Git, or npm-backed installation can exceed 30 seconds, causing the request to fail and potentially leaving partially applied provider state. Scope this deadline to discovery, or give mutations their own cancellation and readback lifecycle.
| </span> | ||
| } | ||
| > | ||
| {group.discoveryFailed ? ( |
There was a problem hiding this comment.
[P2] Keep successful plugin data visible on partial discovery failure
The engine catches plugin and standalone-skill discovery separately and preserves the successful side's values, but either failure marks the single provider status as failed. This branch checks that status first, so a skills/list failure replaces a non-empty, successfully discovered plugin catalog with an error row. Render the cards alongside a facet-specific warning, or carry separate statuses for plugins and skills.
| marketplaceLabel: plugin.marketplace?.displayName ?? plugin.marketplace?.name, | ||
| availability: plugin.availability, | ||
| installed: plugin.installations.length > 0, | ||
| canInstall: plugin.managementCapabilities.install, |
There was a problem hiding this comment.
[P2] Gate installation on plugin availability
canInstall mirrors only the management capability and ignores availability, despite the schema contract requiring clients to combine them. Codex maps DISABLED_BY_ADMIN to blocked while retaining install: true, so the Market tab exposes an actionable Install button that policy forbids. Combine the capability with availability here, and apply the same rule to enable/toggle affordances.
| const separator = row.pluginKey.indexOf(':'); | ||
| const provider = row.pluginKey.slice(0, separator); | ||
| if (provider !== 'claude-code' && provider !== 'codex') return; | ||
| const updated = await toggle.trigger({ |
There was a problem hiding this comment.
[P2] Do not drive per-skill state with a plugin-level toggle
A bundled-skill row is checked from the component's skill.enabled, but this handler calls the whole plugin's setPluginEnabled. For the valid state where a Codex plugin is enabled while one bundled skill is disabled, clicking the switch merely re-enables the already enabled plugin; readback leaves the skill disabled and the switch never changes. Either represent plugin enablement in this row or expose a real per-skill mutation.
| onSubmit: (draft: CustomMcpServerDraft) => void; | ||
| }): React.ReactNode { | ||
| const t = useTranslations('settings.plugins.mcp'); | ||
| const { control, register, handleSubmit } = useForm<McpForm>({ |
There was a problem hiding this comment.
[P2] Surface MCP form validation errors
The form installs a resolver but never reads formState.errors or field state, and no FieldError is rendered. An empty name, command, or URL therefore makes handleSubmit silently refuse to submit while the dialog stays open with no feedback. Bind each validation error to its field and expose an accessible message.
Builds the client-facing management surface for provider plugins, LinkCode-owned MCP servers, and skills on top of the provider-plugin model that landed with CODE-23/403/432 (which was backend-only: no wire contract, no UI,
managementCapabilitieshardcoded all-false).Supersedes the closed #261 / CODE-382~387 batch — that design invented a parallel "capability unit + connector" concept in the same paths. Design doc: Plugins / MCP / Skills Management.
What ships
Wire (62 → 64, one lockstep bump per batch)
plugin.list.get/resultcarryingPlugin[], standalone skills, andproviderStatus[]— so the UI can tell "this agent has no plugins" from "its CLI failed".plugin.set-enabled/plugin.updatedandskill.set-enabled/skill.updated, both replying with the re-read entity so clients patch one cache entry instead of re-running discovery.config.get/config.setgain custom MCP servers: masked reads (envKeys/headerKeys, never values) and per-key patch ops.session.startedgains optionalmcpWarnings.Provider management, honestly capability-gated
claude plugin enable|disable -s <scope>— itsmanagementCapabilitieswere factually wrong (all-false) and are corrected; codex stays read-only because it genuinely has no plugin-toggle surface.skillOverridesin settings.json (what the TUI/skillsdialog writes; read-modify-write preserving every other key and any finername-only/user-invocable-onlytier the user set), codex callsskills/config/write.Custom (BYO) MCP servers
~/.linkcode/config.json(0600, per-entry tolerant parse), injected into MCP-capable sessions'StartOptions.mcpServersalongside the existing simulator endpoint, withagent-unsupported/name-conflictwarnings instead of silent drops.buildCustomMcpPatchis the highest-value unit test in the batch.Settings page — one "Plugins & Skills" category on desktop and webview, four tabs: Plugins (installed only, provider-grouped, capability-gated switches), Market (uninstalled marketplace listings), MCP (custom servers + read-only plugin-provided ones), Skills (plugin + standalone, per-skill switches). Manual refresh only — discovery is a real CLI shell-out.
Verification
pnpm check:ci+pnpm testgreen on every commit (2178 tests post-merge).claude plugin disable/enableround-trips on the real CLI; a real skill toggle writesskillOverridesand restoring it removes the key;~/.claude/settings.jsondiffed byte-identical to a pre-test backup afterwards.Two real bugs were found by that live pass and are fixed here rather than left for later:
available[].sourcebecame a union (bare path string in 52 of 275 entries, object with its ownsourcediscriminator in the rest) and itsversionisnullfor 262 of 275; codex omitsversionentirely while the schema required the key present. One bad entry failed the whole array, so the page was empty everywhere. Regression fixtures now cover every observed shape.Fieldboth renderedname="secrets.0.value"(Field owns a single control), silently dropping the key field from the form.Deliberately out of scope
Managed/HQ connectors and OAuth (CODE-94/96/340 untouched); plugin install/uninstall/update — the Market tab is browse-only, and both providers do expose install surfaces (
claude plugin install, codexplugin/install) that a follow-up can light up; claude'sname-only/user-invocable-onlyskill tiers have no wire representation yet (preserved, not coarsened); retrofitting the same masking ontoaccounts, which still round-trips plaintext.The Market list renders a bounded 60 entries per provider with a visible "showing 60 of 273 — narrow it down with search" note rather than truncating silently; virtualizing it is a follow-up if browsing the full catalog matters.
Closes CODE-487, CODE-488, CODE-490, CODE-491, CODE-492, CODE-493, CODE-494, CODE-495, CODE-496, CODE-497, CODE-502, CODE-503, CODE-504, CODE-505.