From 06e19fdb6cac47ff62e3df977665e9885f3813a4 Mon Sep 17 00:00:00 2001 From: Tiberiu Socaci Date: Fri, 16 Oct 2026 22:33:09 +0300 Subject: [PATCH] Preserve unavailable MCP grants on unrelated admin saves Signed-off-by: Tiberiu Socaci --- CHANGELOG.md | 3 + FEATURES.md | 4 ++ TEST-PLAN.md | 7 ++ public/app.js | 12 +++- test/channel-workdir-ui.test.js | 3 +- test/mcp-legacy-selection-browser.test.js | 84 +++++++++++++++++++++++ 6 files changed, 111 insertions(+), 2 deletions(-) create mode 100644 test/mcp-legacy-selection-browser.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 87cc6669..baac8fbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,9 @@ product overview. ## Unreleased +- Preserve existing unavailable Cloud MCP selections when saving unrelated admin settings. + Removing a selection refreshes the channel picker and prevents later saves from restoring it. + - Show enabled optional providers in the admin channel Runtime picker and preserve their saved engine/model selection when changing unrelated channel settings. diff --git a/FEATURES.md b/FEATURES.md index ed1a9966..f3808e91 100644 --- a/FEATURES.md +++ b/FEATURES.md @@ -182,6 +182,10 @@ operator definitions before saving, including stale cached discoveries. Previously stored selections remain removable; unrelated saves preserve them and runtime admission still drops unusable optional connectors with a notice while allowing the turn to continue. +- The shared admin MCP picker round-trips the exact saved reference for an unavailable selection, + even when discovery redacts its transport metadata. Channel saves refresh the picker from the + committed record, so removing an unavailable grant disables re-addition and later unrelated saves + cannot restore it. New selections still require current backend admission. ## Drive sync status for unlinked channels diff --git a/TEST-PLAN.md b/TEST-PLAN.md index 719201da..e43d9bdc 100644 --- a/TEST-PLAN.md +++ b/TEST-PLAN.md @@ -447,6 +447,13 @@ must run applicable gates before stable promotion; no live pass is implied by au ## Claude Cloud MCP admission (2026-10-08) +- [x] Browser regression: with `CG_BROWSER_MODULE` pointing to Playwright, run + `node --test test/mcp-legacy-selection-browser.test.js`. The actual admin form/router preserves + an existing credentialed MCP's exact saved reference when discovery returns a redacted unavailable + row and an unrelated setting is saved, with both Claude and Codex channel pins. Removal refreshes + the current picker, disables re-addition and survives another save; a forged new invalid selection + still returns 400 without changing grants. The test fails before the fix on the missing `match` + in the native form payload. This isolated browser regression does not complete the live gates below. - [x] Regression: `node --test test/claude-mcp-selection-admission.test.js test/mcp-discovery.test.js test/claude-selected-mcp.test.js test/channel-settings-modal.test.js test/access-grants.test.js test/folders-settings.test.js test/mcp-config.test.js diff --git a/public/app.js b/public/app.js index 3853a58f..c54e4909 100644 --- a/public/app.js +++ b/public/app.js @@ -1715,7 +1715,14 @@ function selectedMcpEntries(box, engine) { captureMcpSelection(box); const selected = mcpBoxState(box)[engine]; const available = catalogWithSavedEntries(box, engine); - return selected.map((id) => available.find((entry) => mcpEntryId(entry) === id)).filter(Boolean); + return selected.map((id) => { + const entry = available.find((candidate) => mcpEntryId(candidate) === id); + // Unavailable catalog rows redact transport references. Round-trip an existing grant + // verbatim rather than serializing that display-only stub and silently dropping it. + // New selections still use the catalog and every save is revalidated by the backend. + const original = box._mcpOriginals?.[engine]?.find((candidate) => mcpEntryId(candidate) === id); + return entry?.selectable === false && original ? original : entry; + }).filter(Boolean); } function wireGrantTierNavigation(select, currentTier) { @@ -2407,6 +2414,9 @@ function renderChannelDetail(ch) { // The server response is the validated, committed record. Reconcile the cached channel from // that whole record so a later SPA re-render cannot resurrect stale MCP/skill selections. ch.meta = reconcileChannelMeta(ch.meta, result.meta); + initializeMcpBox(mcpsBox, { claude: ch.meta.allowedMcps || [], codex: ch.meta.allowedCodexMcps || [] }); + if (!mcpsBox.dataset.mcpLoading) paintAllMcpBoxes(mcpsBox); + updateChecksCount(mcpsBox, mcpsCount); void refreshVpn(); skillsPicker?.update({ selected: ch.meta.skills || [] }); const acceptedGuests = channelGuestAcceptedIds( diff --git a/test/channel-workdir-ui.test.js b/test/channel-workdir-ui.test.js index 8205cd95..bddfae1b 100644 --- a/test/channel-workdir-ui.test.js +++ b/test/channel-workdir-ui.test.js @@ -56,11 +56,12 @@ function fixture({ workDir = "/home/operator/project", saveError } = {}) { const calls = []; const context = { card, ch, meta: ch.meta, Event, detailDirty: false, SELF_SAVING_CONTROLS: ".channel-env-card, .ch-vpn-controls", - engineSelect: { value: "codex" }, usersBox: { dataset: { ready: "" } }, mcpsBox: {}, skillsPicker: null, + engineSelect: { value: "codex" }, usersBox: { dataset: { ready: "" } }, mcpsBox: { dataset: {} }, mcpsCount: {}, skillsPicker: null, makeToolboxKeyInput: control(".ch-make-toolbox-key"), makeToolboxUrlInput: control(".ch-make-toolbox-url"), makeToolboxState: new Control(), clearMakeToolbox: false, tokenValue: () => "", selectedMcpEntries: () => [], checkedValues: () => [], + initializeMcpBox() {}, paintAllMcpBoxes() {}, updateChecksCount() {}, explicitCheckedValues: () => [], channelGuestSavePatch: () => ({}), channelGuestAcceptedIds: () => null, reconcileChannelMeta, attachReveal() {}, revealSecret() {}, paintModePill() {}, renderConvList() {}, refreshVpn() {}, async refreshConversationRows() { return true; }, setTimeout() {}, diff --git a/test/mcp-legacy-selection-browser.test.js b/test/mcp-legacy-selection-browser.test.js new file mode 100644 index 00000000..6c05d639 --- /dev/null +++ b/test/mcp-legacy-selection-browser.test.js @@ -0,0 +1,84 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import express from "express"; +import path from "node:path"; +import { writeFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { ensureTestEnv } from "./helpers.js"; + +ensureTestEnv(); + +test("unrelated admin saves retain unavailable MCP selections and removals cannot restore them", { skip: !process.env.CG_BROWSER_MODULE }, async (t) => { + const { chromium } = await import(process.env.CG_BROWSER_MODULE); + const { createAdminRouter } = await import("../src/web/routes/admin.js"); + const { saveSettings } = await import("../src/config/settings.js"); + const { upsertChannelEntry, defaultChannelMeta, saveChannelMeta, getChannelMeta } = await import("../src/config/store.js"); + const { catalogFromClaudeList } = await import("../src/gateway/mcp-discovery.js"); + const original = { name: "credentialed", namespace: "mcp__credentialed", match: { serverName: "credentialed" } }; + writeFileSync(path.join(process.env.CLAUDE_CONFIG_DIR, ".claude.json"), JSON.stringify({ mcpServers: { + credentialed: { command: "node", args: ["fixture-echo.mjs"], env: { TOKEN: "test-private-mcp-value" } }, + } })); + const catalog = await catalogFromClaudeList("credentialed: node fixture-echo.mjs - ✓ Connected"); + assert.equal(catalog[0].selectable, false); + assert.equal(catalog[0].match, undefined, "unavailable catalog intentionally redacts transport references"); + saveSettings({ engineEnabled: { claude: true, codex: true }, engineFallback: false }); + const app = express(); + app.use(express.json()); + app.get("/api/mcp/available", (req, res) => res.json({ servers: req.query.engine === "claude" ? catalog : [] })); + app.get("/api/health", (_req, res) => res.json({ slack: { connected: false }, engines: {} })); + app.use("/api", createAdminRouter({ slack: { getClient: () => null, snapshot: () => ({ connected: false }) } })); + const publicDir = fileURLToPath(new URL("../public", import.meta.url)); + app.use(express.static(publicDir, { dotfiles: "allow" })); + app.get("/conversations/channel/:id", (_req, res) => res.sendFile(path.join(publicDir, "index.html"), { dotfiles: "allow" })); + const server = await new Promise((resolve) => { const s = app.listen(0, "127.0.0.1", () => resolve(s)); }); + t.after(() => new Promise((resolve) => { server.closeAllConnections(); server.close(resolve); })); + const browser = await chromium.launch({ headless: true }); + t.after(() => browser.close()); + const page = await browser.newPage(); + page.setDefaultTimeout(10000); + const errors = []; + page.on("pageerror", (error) => errors.push(error.message)); + for (const engine of ["claude", "codex"]) { + const id = `C_MCP_LEGACY_${engine.toUpperCase()}`; + const entry = await upsertChannelEntry(id, { name: `mcp-legacy-${engine}`, type: "channel", isDM: false }); + await saveChannelMeta(entry.slug, { ...defaultChannelMeta({ channelId: id, name: entry.name, type: "channel", isDM: false }), engine, allowedMcps: [original], allowedCodexMcps: [], allowNetwork: false }); + await page.goto(`http://127.0.0.1:${server.address().port}/conversations/channel/${id}`); + await page.locator('[data-pane="mcps"].subtab').click(); + const selected = page.locator('.ch-mcps input[data-mcp-engine="claude"][value="credentialed"]'); + await selected.waitFor(); + assert.equal(await selected.isChecked(), true); + assert.equal(await selected.isDisabled(), false, "legacy selection remains removable"); + assert.ok(!(await page.locator(".ch-mcps").innerText()).includes("test-private-mcp-value")); + const save = async () => { + const pending = page.waitForResponse((r) => r.url().endsWith(`/channels/${id}/meta`) && r.request().method() === "PUT"); + await page.locator(".save-channel").click(); + const response = await pending; + assert.equal(response.status(), 200, await response.text()); + await page.locator(".detail-savebar .msg").filter({ hasText: /^Saved$/ }).waitFor(); + return response.request().postDataJSON(); + }; + await page.locator('[data-pane="runtime"].subtab').click(); + await page.locator(".ch-memory").setChecked(!(await page.locator(".ch-memory").isChecked())); + const body = await save(); + assert.deepEqual(body.allowedMcps, [original], `${engine} unrelated save round-trips the exact saved reference`); + assert.deepEqual((await getChannelMeta(entry.slug)).allowedMcps, [original]); + assert.equal((await getChannelMeta(entry.slug)).engine, engine); + assert.deepEqual((await getChannelMeta(entry.slug)).allowedCodexMcps, []); + await page.locator('[data-pane="mcps"].subtab').click(); + await selected.uncheck(); + await save(); + assert.deepEqual((await getChannelMeta(entry.slug)).allowedMcps, []); + assert.equal(await selected.isDisabled(), true, "committed removal refreshes the original selection and disables unavailable re-add"); + await page.locator('[data-pane="runtime"].subtab').click(); + await page.locator(".ch-memory").setChecked(!(await page.locator(".ch-memory").isChecked())); + assert.deepEqual((await save()).allowedMcps, [], "later saves cannot resurrect removed originals"); + const rejected = await page.evaluate(async ({ id, original }) => { + const r = await fetch(`/api/channels/${id}/meta`, { method: "PUT", headers: { "content-type": "application/json", "X-CG-Request": "1" }, body: JSON.stringify({ allowedMcps: [original] }) }); + return { status: r.status, body: await r.json() }; + }, { id, original }); + assert.equal(rejected.status, 400, "backend still rejects a newly added unavailable reference"); + assert.ok(!JSON.stringify(rejected).includes("test-private-mcp-value")); + assert.deepEqual((await getChannelMeta(entry.slug)).allowedMcps, []); + } + assert.deepEqual(errors, []); +});