Skip to content

Commit 9dd4c06

Browse files
committed
fix(frontend): surface pending tools in ServerDetail approval banner
selectQuarantinedTools dropped freshly-pending tools whenever no tool was 'changed', so an all-pending, non-quarantined server hid the Tool-Quarantine banner even though those tools are genuinely blocked by the backend (checkToolApprovals -> BlockedTools) and the Servers page counts them (pending_count + changed_count). Banner and count now agree. - selectQuarantinedTools: surface both 'pending' and 'changed' tools on a non-quarantined server; keep suppression while the server is quarantined. - ServerDetail: subtle dismissible hint explaining pending tools come from tool-level quarantine and can be auto-approved via skip_quarantine / quarantine_enabled. - Tests: reconcile MCP-2101 trust-model unit tests to the new behavior, add all-pending regression test, add view-level banner/buttons/badge/hint test. - Docs: update tool-quarantine.md Web UI section. Related: MCP-2917
1 parent c0e569a commit 9dd4c06

6 files changed

Lines changed: 187 additions & 44 deletions

File tree

docs/features/tool-quarantine.md

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -205,20 +205,22 @@ curl -H "X-API-Key: your-key" \
205205
1. Open the MCPProxy dashboard
206206
2. Click on a server in the server list
207207
3. Navigate to the **Tools** tab in the server detail view
208-
4. Review changed (and any residual pending) tools and click **Approve** or **Approve All**
208+
4. Review pending and changed tools and click **Approve** or **Approve All**
209209

210210
Each quarantined tool also offers a **Block** button (and the banner a **Block
211211
All**) next to Approve. Blocking rejects the tool: it leaves the quarantine list
212212
and is disabled in the tools list, so AI agents can neither see nor call it.
213213
Blocking is reversible — re-enable the tool later with its toggle in the tools
214214
list (or `mcpproxy tools enable <server:tool>`).
215215

216-
The server detail view's **Tool Quarantine** banner is shown only when a tool's
217-
status is `changed` (a rug-pull). Once a change has surfaced, any residual
218-
`pending` tools are listed alongside it so they can be cleared in the same pass.
219-
Freshly-`pending` baseline tools do **not** raise the banner on their own:
220-
approving the **server** (lifting the server-level Security Quarantine) promotes
221-
its baseline `pending` tools to `approved`. While the server-level **Security
216+
The server detail view's **Tool Quarantine** banner surfaces every `pending`
217+
(new, never-approved) or `changed` (rug-pull) tool while the server itself is
218+
**not** quarantined. Both are genuinely blocked by the backend until the
219+
operator acts, and the server list page counts them (`pending_count +
220+
changed_count`), so the banner and the count agree. The banner carries a short,
221+
dismissible hint noting that pending tools come from tool-level quarantine and
222+
can be auto-approved by setting `skip_quarantine: true` (per-server) or
223+
`quarantine_enabled: false` (global). While the server-level **Security
222224
Quarantine** banner is showing, the Tool-Quarantine banner is suppressed
223225
entirely — the operator approves the server first, and the two banners never
224226
appear at once.

frontend/src/utils/toolQuarantine.ts

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,28 +2,33 @@ import type { ToolApproval } from '@/types'
22

33
/**
44
* Selects the tools that warrant the per-server Tool-Quarantine banner / list
5-
* (Spec 032, parent MCP-2081, MCP-2101).
5+
* (Spec 032, parent MCP-2916, MCP-2917).
66
*
7-
* Trust model (confirmed on MCP-2081): when an operator approves a *server*
8-
* (lifting the server-level Security Quarantine), the backend promotes that
9-
* server's baseline `pending` tools to `approved`. A freshly-`pending` baseline
10-
* tool is therefore NOT a reason to nag the operator with a tool-level banner —
11-
* only a `changed` tool (a rug-pull) is.
7+
* On a live, NON-quarantined server a `pending` (new, never-approved) tool is
8+
* genuinely blocked by the backend (`checkToolApprovals` → `BlockedTools`) and
9+
* the Servers page already counts it (`pending_count + changed_count`). The
10+
* banner must therefore surface both `pending` and `changed` tools so the
11+
* operator can approve them; banner and count must agree. Pending tools come
12+
* from tool-level quarantine and can be auto-approved by setting
13+
* `skip_quarantine: true` (per-server) or `quarantine_enabled: false` (global).
1214
*
1315
* Rules:
1416
* - While the server is quarantined, suppress the tool banner entirely. The
1517
* server-level Security Quarantine banner already covers it and the operator
1618
* must approve the server first — never show two banners at once.
17-
* - Otherwise the banner keys off `status === 'changed'`. Only once a change
18-
* has surfaced do we also include any residual `pending` tools so the
19-
* operator can clear them in the same approval pass.
19+
* - Otherwise surface every tool that is `pending` (awaiting first approval) or
20+
* `changed` (a rug-pull), since both are blocked until the operator acts.
21+
*
22+
* Note: this intentionally reverses the MCP-2101 "don't nag on a pending
23+
* baseline" behavior for non-quarantined servers. That trust model assumed
24+
* approving the server would promote pending→approved, but a server can be
25+
* non-quarantined (e.g. `skip_quarantine`) while its tools stay pending and
26+
* blocked, leaving the operator no way to approve them.
2027
*/
2128
export function selectQuarantinedTools(
2229
toolApprovals: ToolApproval[],
2330
serverQuarantined: boolean,
2431
): ToolApproval[] {
2532
if (serverQuarantined) return []
26-
const hasChanged = toolApprovals.some((t) => t.status === 'changed')
27-
if (!hasChanged) return []
2833
return toolApprovals.filter((t) => t.status === 'changed' || t.status === 'pending')
2934
}

frontend/src/views/ServerDetail.vue

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,26 @@
340340
<div class="text-sm">
341341
{{ quarantinedTools.length }} tool(s) require approval before they can be used by AI agents.
342342
</div>
343+
<!-- MCP-2917: subtle, dismissible hint explaining where pending
344+
tools come from and how to opt out of tool-level approval. -->
345+
<div
346+
v-if="!quarantineHintDismissed"
347+
data-test="quarantine-hint"
348+
class="text-xs opacity-70 mt-1 flex items-start gap-1"
349+
>
350+
<span>
351+
Pending tools come from tool-level quarantine. To approve them automatically, set
352+
<code class="text-[11px]">skip_quarantine: true</code> for this server or
353+
<code class="text-[11px]">quarantine_enabled: false</code> globally.
354+
</span>
355+
<button
356+
type="button"
357+
data-test="quarantine-hint-dismiss"
358+
@click="quarantineHintDismissed = true"
359+
class="btn btn-ghost btn-xs px-1 -mt-0.5"
360+
aria-label="Dismiss hint"
361+
>✕</button>
362+
</div>
343363
</div>
344364
<div class="flex items-center gap-2">
345365
<button
@@ -1324,15 +1344,19 @@ const selectedToolSchema = ref<Tool | null>(null)
13241344
// Tool quarantine (Spec 032)
13251345
const toolApprovals = ref<ToolApproval[]>([])
13261346
const approvalLoading = ref(false)
1347+
// MCP-2917: the Tool-Quarantine banner carries a one-line hint about how to
1348+
// auto-approve pending tools; let the operator dismiss it for the session.
1349+
const quarantineHintDismissed = ref(false)
13271350
const toolToggleLoading = ref<Record<string, boolean>>({})
13281351
// Single in-flight flag for the bulk Enable All / Disable All buttons so
13291352
// they're mutually exclusive with each other and with any per-tool toggle.
13301353
const bulkToolToggleLoading = ref(false)
13311354
1332-
// MCP-2101 (Spec 032): the Tool-Quarantine banner / list keys off `changed`
1333-
// (rug-pull) tools, NOT freshly-`pending` baseline tools, and is suppressed
1334-
// entirely while the server-level Security Quarantine banner is showing.
1335-
// See selectQuarantinedTools for the trust-model rationale.
1355+
// MCP-2917 (Spec 032): the Tool-Quarantine banner / list surfaces every
1356+
// `pending` (awaiting first approval) or `changed` (rug-pull) tool while the
1357+
// server itself is NOT quarantined (both are blocked by the backend until the
1358+
// operator acts), and is suppressed entirely while the server-level Security
1359+
// Quarantine banner is showing. See selectQuarantinedTools for the rationale.
13361360
const quarantinedTools = computed(() => {
13371361
return selectQuarantinedTools(toolApprovals.value, server.value?.quarantined ?? false)
13381362
})

frontend/tests/unit/quarantine-block-view.spec.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ vi.mock('@/services/api', () => {
2525
],
2626
})
2727
),
28-
// selectQuarantinedTools only surfaces the list when ≥1 tool is "changed"
29-
// (a rug-pull review); a pending tool then rides along.
28+
// selectQuarantinedTools surfaces both "changed" (rug-pull) and "pending"
29+
// tools on a non-quarantined server (MCP-2917); a "changed" tool here.
3030
getToolApprovals: vi.fn(() =>
3131
ok({
3232
tools: [
Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
import { describe, it, expect, beforeEach, vi } from 'vitest'
2+
import { mount, flushPromises } from '@vue/test-utils'
3+
import { createPinia, setActivePinia } from 'pinia'
4+
import { createRouter, createWebHistory } from 'vue-router'
5+
6+
// MCP-2917: a non-quarantined server whose tools are all `pending` (new, never
7+
// approved) must still surface the Tool-Quarantine banner with the per-tool and
8+
// bulk Approve/Block buttons, the `pending` warning badge, and the dismissible
9+
// hint. Pre-fix the banner was hidden whenever no tool was `changed`, even
10+
// though those pending tools are genuinely blocked by the backend.
11+
12+
vi.mock('@/services/api', () => {
13+
const ok = (data: unknown = {}) => Promise.resolve({ success: true, data })
14+
return {
15+
default: {
16+
getServers: vi.fn(() =>
17+
ok({
18+
servers: [
19+
{
20+
name: 'github',
21+
protocol: 'stdio',
22+
enabled: true,
23+
connected: true,
24+
quarantined: false, // NOT quarantined — the key condition
25+
tool_count: 1,
26+
},
27+
],
28+
})
29+
),
30+
// All tools pending, none changed — the exact MCP-2917 bug scenario.
31+
getToolApprovals: vi.fn(() =>
32+
ok({
33+
tools: [
34+
{ tool_name: 'create_issue', status: 'pending', description: 'Create an issue' },
35+
],
36+
count: 1,
37+
})
38+
),
39+
getToolDiff: vi.fn(() => ok({})),
40+
getServerTools: vi.fn(() =>
41+
ok({ tools: [{ name: 'create_issue', description: 'Create an issue', enabled: false }] })
42+
),
43+
getSecurityOverview: vi.fn(() => ok({})),
44+
listScanners: vi.fn(() => ok({ scanners: [] })),
45+
getServerLogs: vi.fn(() => ok({ logs: [] })),
46+
discoverServerTools: vi.fn(() => ok({})),
47+
approveTools: vi.fn(() => ok({ approved: 1 })),
48+
blockTools: vi.fn(() => ok({ blocked: 1 })),
49+
},
50+
}
51+
})
52+
53+
describe('ServerDetail — pending-only Tool Quarantine banner (MCP-2917)', () => {
54+
beforeEach(() => {
55+
setActivePinia(createPinia())
56+
})
57+
58+
async function mountDetail() {
59+
const api = (await import('@/services/api')).default
60+
const ServerDetail = (await import('@/views/ServerDetail.vue')).default
61+
const router = createRouter({
62+
history: createWebHistory(),
63+
routes: [{ path: '/servers/:serverName', component: { template: '<div/>' } }],
64+
})
65+
await router.push('/servers/github?tab=tools')
66+
await router.isReady()
67+
const wrapper = mount(ServerDetail, {
68+
props: { serverName: 'github' },
69+
global: { plugins: [createPinia(), router] },
70+
})
71+
await flushPromises()
72+
return { wrapper, api }
73+
}
74+
75+
it('shows the banner with Approve/Block buttons for an all-pending, non-quarantined server', async () => {
76+
const { wrapper } = await mountDetail()
77+
expect(wrapper.find('[data-test="tool-quarantine-banner"]').exists()).toBe(true)
78+
expect(wrapper.find('[data-test="quarantine-approve-all"]').exists()).toBe(true)
79+
expect(wrapper.find('[data-test="quarantine-block-all"]').exists()).toBe(true)
80+
expect(wrapper.find('[data-test="quarantine-approve-create_issue"]').exists()).toBe(true)
81+
expect(wrapper.find('[data-test="quarantine-block-create_issue"]').exists()).toBe(true)
82+
})
83+
84+
it('renders the pending warning badge for the tool', async () => {
85+
const { wrapper } = await mountDetail()
86+
const list = wrapper.find('[data-test="tool-quarantine-list"]')
87+
expect(list.exists()).toBe(true)
88+
const badge = list.find('.badge-warning')
89+
expect(badge.exists()).toBe(true)
90+
expect(badge.text()).toContain('pending')
91+
})
92+
93+
it('shows the dismissible hint and lets the operator dismiss it', async () => {
94+
const { wrapper } = await mountDetail()
95+
expect(wrapper.find('[data-test="quarantine-hint"]').exists()).toBe(true)
96+
await wrapper.find('[data-test="quarantine-hint-dismiss"]').trigger('click')
97+
expect(wrapper.find('[data-test="quarantine-hint"]').exists()).toBe(false)
98+
})
99+
100+
it('Approve calls api.approveTools with the pending tool name', async () => {
101+
const { wrapper, api } = await mountDetail()
102+
await wrapper.find('[data-test="quarantine-approve-create_issue"]').trigger('click')
103+
await flushPromises()
104+
expect(api.approveTools).toHaveBeenCalledWith('github', ['create_issue'])
105+
})
106+
})

frontend/tests/unit/tool-quarantine-banner.spec.ts

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,19 @@ import { describe, it, expect } from 'vitest'
22
import { selectQuarantinedTools } from '@/utils/toolQuarantine'
33
import type { ToolApproval } from '@/types'
44

5-
// MCP-2101 (Spec 032, parent MCP-2081): the per-server Tool-Quarantine banner
6-
// must stop nagging for freshly-`pending` baseline tools and must never show
5+
// MCP-2917 (Spec 032, parent MCP-2916): the per-server Tool-Quarantine banner
6+
// must surface BOTH `pending` (new, never-approved) and `changed` (rug-pull)
7+
// tools whenever the server itself is NOT quarantined, and must never show
78
// alongside the server-level Security Quarantine banner.
89
//
9-
// Trust model (confirmed on MCP-2081): approving a *server* promotes its
10-
// baseline `pending` tools to `approved` on the backend. So a baseline
11-
// `pending` tool is NOT a reason to surface a tool-level banner — only a
12-
// `changed` tool (a rug-pull) is.
10+
// This intentionally reverses the MCP-2101 "don't nag on a pending baseline"
11+
// behavior for non-quarantined servers: on a live, non-quarantined server a
12+
// `pending` tool is genuinely BLOCKED by the backend (checkToolApprovals →
13+
// BlockedTools) and the Servers page already counts it (pending_count +
14+
// changed_count). The banner and the count must agree, so the operator gets
15+
// the Approve/Block buttons to clear pending tools. While the server is still
16+
// quarantined the server-level banner covers everything, so we suppress the
17+
// tool-level banner to avoid two banners at once.
1318

1419
function approval(over: Partial<ToolApproval>): ToolApproval {
1520
return {
@@ -22,27 +27,28 @@ function approval(over: Partial<ToolApproval>): ToolApproval {
2227
}
2328
}
2429

25-
describe('selectQuarantinedTools (MCP-2101)', () => {
30+
describe('selectQuarantinedTools (MCP-2917)', () => {
2631
it('suppresses the tool banner entirely while the server is quarantined', () => {
27-
// Even a rug-pull `changed` tool must not surface a SECOND banner while the
28-
// server-level Security Quarantine banner is up — operator approves the
29-
// server first, then the backend promotes baseline pending→approved.
32+
// The server-level Security Quarantine banner already covers it and the
33+
// operator must approve the server first — never show two banners at once.
3034
const tools = [
3135
approval({ tool_name: 'a', status: 'changed' }),
3236
approval({ tool_name: 'b', status: 'pending' }),
3337
]
3438
expect(selectQuarantinedTools(tools, true)).toEqual([])
3539
})
3640

37-
it('does NOT surface freshly-pending baseline tools (the core fix)', () => {
38-
// Not quarantined, no changed tool → baseline `pending` tools alone must
39-
// not raise the banner. Pre-fix this returned the pending tools.
40-
const tools = [
41-
approval({ tool_name: 'a', status: 'pending' }),
42-
approval({ tool_name: 'b', status: 'pending' }),
43-
approval({ tool_name: 'c', status: 'approved' }),
44-
]
45-
expect(selectQuarantinedTools(tools, false)).toEqual([])
41+
it('surfaces freshly-pending tools on a non-quarantined server (the core fix)', () => {
42+
// Regression for MCP-2917: an all-`pending`, not-quarantined server used to
43+
// hide the banner (the old `hasChanged` early-return) even though every
44+
// tool is genuinely blocked. Now they must surface for approval.
45+
const a = approval({ tool_name: 'a', status: 'pending' })
46+
const b = approval({ tool_name: 'b', status: 'pending' })
47+
const tools = [a, b, approval({ tool_name: 'c', status: 'approved' })]
48+
const result = selectQuarantinedTools(tools, false)
49+
expect(result).toContain(a)
50+
expect(result).toContain(b)
51+
expect(result).not.toContainEqual(approval({ tool_name: 'c', status: 'approved' }))
4652
})
4753

4854
it('surfaces a `changed` (rug-pull) tool when the server is not quarantined', () => {
@@ -51,7 +57,7 @@ describe('selectQuarantinedTools (MCP-2101)', () => {
5157
expect(selectQuarantinedTools(tools, false)).toEqual([changed])
5258
})
5359

54-
it('once a change has surfaced, also includes residual pending tools', () => {
60+
it('surfaces both pending and changed tools together when not quarantined', () => {
5561
const changed = approval({ tool_name: 'rugpull', status: 'changed' })
5662
const pending = approval({ tool_name: 'leftover', status: 'pending' })
5763
const tools = [changed, pending, approval({ tool_name: 'fine', status: 'approved' })]

0 commit comments

Comments
 (0)