From 9dd4c0693d6a924d008f449ad7d3a04e1e23394d Mon Sep 17 00:00:00 2001
From: Algis Dumbris <a.dumbris@gmail.com>
Date: Fri, 19 Jun 2026 10:00:22 +0300
Subject: [PATCH] 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
---
 docs/features/tool-quarantine.md              |  16 +--
 frontend/src/utils/toolQuarantine.ts          |  27 +++--
 frontend/src/views/ServerDetail.vue           |  32 +++++-
 .../tests/unit/quarantine-block-view.spec.ts  |   4 +-
 .../quarantine-pending-banner-view.spec.ts    | 106 ++++++++++++++++++
 .../tests/unit/tool-quarantine-banner.spec.ts |  46 ++++----
 6 files changed, 187 insertions(+), 44 deletions(-)
 create mode 100644 frontend/tests/unit/quarantine-pending-banner-view.spec.ts

diff --git a/docs/features/tool-quarantine.md b/docs/features/tool-quarantine.md
index a8b8078e..f6b5329e 100644
--- a/docs/features/tool-quarantine.md
+++ b/docs/features/tool-quarantine.md
@@ -205,7 +205,7 @@ curl -H "X-API-Key: your-key" \
 1. Open the MCPProxy dashboard
 2. Click on a server in the server list
 3. Navigate to the **Tools** tab in the server detail view
-4. Review changed (and any residual pending) tools and click **Approve** or **Approve All**
+4. Review pending and changed tools and click **Approve** or **Approve All**
 
 Each quarantined tool also offers a **Block** button (and the banner a **Block
 All**) next to Approve. Blocking rejects the tool: it leaves the quarantine list
@@ -213,12 +213,14 @@ and is disabled in the tools list, so AI agents can neither see nor call it.
 Blocking is reversible — re-enable the tool later with its toggle in the tools
 list (or `mcpproxy tools enable <server:tool>`).
 
-The server detail view's **Tool Quarantine** banner is shown only when a tool's
-status is `changed` (a rug-pull). Once a change has surfaced, any residual
-`pending` tools are listed alongside it so they can be cleared in the same pass.
-Freshly-`pending` baseline tools do **not** raise the banner on their own:
-approving the **server** (lifting the server-level Security Quarantine) promotes
-its baseline `pending` tools to `approved`. While the server-level **Security
+The server detail view's **Tool Quarantine** banner surfaces every `pending`
+(new, never-approved) or `changed` (rug-pull) tool while the server itself is
+**not** quarantined. Both are genuinely blocked by the backend until the
+operator acts, and the server list page counts them (`pending_count +
+changed_count`), so the banner and the count agree. The banner carries a short,
+dismissible hint noting that pending tools come from tool-level quarantine and
+can be auto-approved by setting `skip_quarantine: true` (per-server) or
+`quarantine_enabled: false` (global). While the server-level **Security
 Quarantine** banner is showing, the Tool-Quarantine banner is suppressed
 entirely — the operator approves the server first, and the two banners never
 appear at once.
diff --git a/frontend/src/utils/toolQuarantine.ts b/frontend/src/utils/toolQuarantine.ts
index 42e0a39b..b371e6f1 100644
--- a/frontend/src/utils/toolQuarantine.ts
+++ b/frontend/src/utils/toolQuarantine.ts
@@ -2,28 +2,33 @@ import type { ToolApproval } from '@/types'
 
 /**
  * Selects the tools that warrant the per-server Tool-Quarantine banner / list
- * (Spec 032, parent MCP-2081, MCP-2101).
+ * (Spec 032, parent MCP-2916, MCP-2917).
  *
- * Trust model (confirmed on MCP-2081): when an operator approves a *server*
- * (lifting the server-level Security Quarantine), the backend promotes that
- * server's baseline `pending` tools to `approved`. A freshly-`pending` baseline
- * tool is therefore NOT a reason to nag the operator with a tool-level banner —
- * only a `changed` tool (a rug-pull) is.
+ * On a live, NON-quarantined server a `pending` (new, never-approved) tool is
+ * genuinely blocked by the backend (`checkToolApprovals` → `BlockedTools`) and
+ * the Servers page already counts it (`pending_count + changed_count`). The
+ * banner must therefore surface both `pending` and `changed` tools so the
+ * operator can approve them; banner and count must agree. Pending tools come
+ * from tool-level quarantine and can be auto-approved by setting
+ * `skip_quarantine: true` (per-server) or `quarantine_enabled: false` (global).
  *
  * Rules:
  *  - While the server is quarantined, suppress the tool banner entirely. The
  *    server-level Security Quarantine banner already covers it and the operator
  *    must approve the server first — never show two banners at once.
- *  - Otherwise the banner keys off `status === 'changed'`. Only once a change
- *    has surfaced do we also include any residual `pending` tools so the
- *    operator can clear them in the same approval pass.
+ *  - Otherwise surface every tool that is `pending` (awaiting first approval) or
+ *    `changed` (a rug-pull), since both are blocked until the operator acts.
+ *
+ * Note: this intentionally reverses the MCP-2101 "don't nag on a pending
+ * baseline" behavior for non-quarantined servers. That trust model assumed
+ * approving the server would promote pending→approved, but a server can be
+ * non-quarantined (e.g. `skip_quarantine`) while its tools stay pending and
+ * blocked, leaving the operator no way to approve them.
  */
 export function selectQuarantinedTools(
   toolApprovals: ToolApproval[],
   serverQuarantined: boolean,
 ): ToolApproval[] {
   if (serverQuarantined) return []
-  const hasChanged = toolApprovals.some((t) => t.status === 'changed')
-  if (!hasChanged) return []
   return toolApprovals.filter((t) => t.status === 'changed' || t.status === 'pending')
 }
diff --git a/frontend/src/views/ServerDetail.vue b/frontend/src/views/ServerDetail.vue
index e037a0bd..d04e7f33 100644
--- a/frontend/src/views/ServerDetail.vue
+++ b/frontend/src/views/ServerDetail.vue
@@ -340,6 +340,26 @@
                 <div class="text-sm">
                   {{ quarantinedTools.length }} tool(s) require approval before they can be used by AI agents.
                 </div>
+                <!-- MCP-2917: subtle, dismissible hint explaining where pending
+                     tools come from and how to opt out of tool-level approval. -->
+                <div
+                  v-if="!quarantineHintDismissed"
+                  data-test="quarantine-hint"
+                  class="text-xs opacity-70 mt-1 flex items-start gap-1"
+                >
+                  <span>
+                    Pending tools come from tool-level quarantine. To approve them automatically, set
+                    <code class="text-[11px]">skip_quarantine: true</code> for this server or
+                    <code class="text-[11px]">quarantine_enabled: false</code> globally.
+                  </span>
+                  <button
+                    type="button"
+                    data-test="quarantine-hint-dismiss"
+                    @click="quarantineHintDismissed = true"
+                    class="btn btn-ghost btn-xs px-1 -mt-0.5"
+                    aria-label="Dismiss hint"
+                  >✕</button>
+                </div>
               </div>
               <div class="flex items-center gap-2">
                 <button
@@ -1324,15 +1344,19 @@ const selectedToolSchema = ref<Tool | null>(null)
 // Tool quarantine (Spec 032)
 const toolApprovals = ref<ToolApproval[]>([])
 const approvalLoading = ref(false)
+// MCP-2917: the Tool-Quarantine banner carries a one-line hint about how to
+// auto-approve pending tools; let the operator dismiss it for the session.
+const quarantineHintDismissed = ref(false)
 const toolToggleLoading = ref<Record<string, boolean>>({})
 // Single in-flight flag for the bulk Enable All / Disable All buttons so
 // they're mutually exclusive with each other and with any per-tool toggle.
 const bulkToolToggleLoading = ref(false)
 
-// MCP-2101 (Spec 032): the Tool-Quarantine banner / list keys off `changed`
-// (rug-pull) tools, NOT freshly-`pending` baseline tools, and is suppressed
-// entirely while the server-level Security Quarantine banner is showing.
-// See selectQuarantinedTools for the trust-model rationale.
+// MCP-2917 (Spec 032): the Tool-Quarantine banner / list surfaces every
+// `pending` (awaiting first approval) or `changed` (rug-pull) tool while the
+// server itself is NOT quarantined (both are blocked by the backend until the
+// operator acts), and is suppressed entirely while the server-level Security
+// Quarantine banner is showing. See selectQuarantinedTools for the rationale.
 const quarantinedTools = computed(() => {
   return selectQuarantinedTools(toolApprovals.value, server.value?.quarantined ?? false)
 })
diff --git a/frontend/tests/unit/quarantine-block-view.spec.ts b/frontend/tests/unit/quarantine-block-view.spec.ts
index 6f5a3be4..ef16556c 100644
--- a/frontend/tests/unit/quarantine-block-view.spec.ts
+++ b/frontend/tests/unit/quarantine-block-view.spec.ts
@@ -25,8 +25,8 @@ vi.mock('@/services/api', () => {
           ],
         })
       ),
-      // selectQuarantinedTools only surfaces the list when ≥1 tool is "changed"
-      // (a rug-pull review); a pending tool then rides along.
+      // selectQuarantinedTools surfaces both "changed" (rug-pull) and "pending"
+      // tools on a non-quarantined server (MCP-2917); a "changed" tool here.
       getToolApprovals: vi.fn(() =>
         ok({
           tools: [
diff --git a/frontend/tests/unit/quarantine-pending-banner-view.spec.ts b/frontend/tests/unit/quarantine-pending-banner-view.spec.ts
new file mode 100644
index 00000000..77460c3c
--- /dev/null
+++ b/frontend/tests/unit/quarantine-pending-banner-view.spec.ts
@@ -0,0 +1,106 @@
+import { describe, it, expect, beforeEach, vi } from 'vitest'
+import { mount, flushPromises } from '@vue/test-utils'
+import { createPinia, setActivePinia } from 'pinia'
+import { createRouter, createWebHistory } from 'vue-router'
+
+// MCP-2917: a non-quarantined server whose tools are all `pending` (new, never
+// approved) must still surface the Tool-Quarantine banner with the per-tool and
+// bulk Approve/Block buttons, the `pending` warning badge, and the dismissible
+// hint. Pre-fix the banner was hidden whenever no tool was `changed`, even
+// though those pending tools are genuinely blocked by the backend.
+
+vi.mock('@/services/api', () => {
+  const ok = (data: unknown = {}) => Promise.resolve({ success: true, data })
+  return {
+    default: {
+      getServers: vi.fn(() =>
+        ok({
+          servers: [
+            {
+              name: 'github',
+              protocol: 'stdio',
+              enabled: true,
+              connected: true,
+              quarantined: false, // NOT quarantined — the key condition
+              tool_count: 1,
+            },
+          ],
+        })
+      ),
+      // All tools pending, none changed — the exact MCP-2917 bug scenario.
+      getToolApprovals: vi.fn(() =>
+        ok({
+          tools: [
+            { tool_name: 'create_issue', status: 'pending', description: 'Create an issue' },
+          ],
+          count: 1,
+        })
+      ),
+      getToolDiff: vi.fn(() => ok({})),
+      getServerTools: vi.fn(() =>
+        ok({ tools: [{ name: 'create_issue', description: 'Create an issue', enabled: false }] })
+      ),
+      getSecurityOverview: vi.fn(() => ok({})),
+      listScanners: vi.fn(() => ok({ scanners: [] })),
+      getServerLogs: vi.fn(() => ok({ logs: [] })),
+      discoverServerTools: vi.fn(() => ok({})),
+      approveTools: vi.fn(() => ok({ approved: 1 })),
+      blockTools: vi.fn(() => ok({ blocked: 1 })),
+    },
+  }
+})
+
+describe('ServerDetail — pending-only Tool Quarantine banner (MCP-2917)', () => {
+  beforeEach(() => {
+    setActivePinia(createPinia())
+  })
+
+  async function mountDetail() {
+    const api = (await import('@/services/api')).default
+    const ServerDetail = (await import('@/views/ServerDetail.vue')).default
+    const router = createRouter({
+      history: createWebHistory(),
+      routes: [{ path: '/servers/:serverName', component: { template: '<div/>' } }],
+    })
+    await router.push('/servers/github?tab=tools')
+    await router.isReady()
+    const wrapper = mount(ServerDetail, {
+      props: { serverName: 'github' },
+      global: { plugins: [createPinia(), router] },
+    })
+    await flushPromises()
+    return { wrapper, api }
+  }
+
+  it('shows the banner with Approve/Block buttons for an all-pending, non-quarantined server', async () => {
+    const { wrapper } = await mountDetail()
+    expect(wrapper.find('[data-test="tool-quarantine-banner"]').exists()).toBe(true)
+    expect(wrapper.find('[data-test="quarantine-approve-all"]').exists()).toBe(true)
+    expect(wrapper.find('[data-test="quarantine-block-all"]').exists()).toBe(true)
+    expect(wrapper.find('[data-test="quarantine-approve-create_issue"]').exists()).toBe(true)
+    expect(wrapper.find('[data-test="quarantine-block-create_issue"]').exists()).toBe(true)
+  })
+
+  it('renders the pending warning badge for the tool', async () => {
+    const { wrapper } = await mountDetail()
+    const list = wrapper.find('[data-test="tool-quarantine-list"]')
+    expect(list.exists()).toBe(true)
+    const badge = list.find('.badge-warning')
+    expect(badge.exists()).toBe(true)
+    expect(badge.text()).toContain('pending')
+  })
+
+  it('shows the dismissible hint and lets the operator dismiss it', async () => {
+    const { wrapper } = await mountDetail()
+    expect(wrapper.find('[data-test="quarantine-hint"]').exists()).toBe(true)
+    await wrapper.find('[data-test="quarantine-hint-dismiss"]').trigger('click')
+    expect(wrapper.find('[data-test="quarantine-hint"]').exists()).toBe(false)
+  })
+
+  it('Approve calls api.approveTools with the pending tool name', async () => {
+    const { wrapper, api } = await mountDetail()
+    await wrapper.find('[data-test="quarantine-approve-create_issue"]').trigger('click')
+    await flushPromises()
+    expect(api.approveTools).toHaveBeenCalledWith('github', ['create_issue'])
+  })
+})
diff --git a/frontend/tests/unit/tool-quarantine-banner.spec.ts b/frontend/tests/unit/tool-quarantine-banner.spec.ts
index c6da8335..89a6bc04 100644
--- a/frontend/tests/unit/tool-quarantine-banner.spec.ts
+++ b/frontend/tests/unit/tool-quarantine-banner.spec.ts
@@ -2,14 +2,19 @@ import { describe, it, expect } from 'vitest'
 import { selectQuarantinedTools } from '@/utils/toolQuarantine'
 import type { ToolApproval } from '@/types'
 
-// MCP-2101 (Spec 032, parent MCP-2081): the per-server Tool-Quarantine banner
-// must stop nagging for freshly-`pending` baseline tools and must never show
+// MCP-2917 (Spec 032, parent MCP-2916): the per-server Tool-Quarantine banner
+// must surface BOTH `pending` (new, never-approved) and `changed` (rug-pull)
+// tools whenever the server itself is NOT quarantined, and must never show
 // alongside the server-level Security Quarantine banner.
 //
-// Trust model (confirmed on MCP-2081): approving a *server* promotes its
-// baseline `pending` tools to `approved` on the backend. So a baseline
-// `pending` tool is NOT a reason to surface a tool-level banner — only a
-// `changed` tool (a rug-pull) is.
+// This intentionally reverses the MCP-2101 "don't nag on a pending baseline"
+// behavior for non-quarantined servers: on a live, non-quarantined server a
+// `pending` tool is genuinely BLOCKED by the backend (checkToolApprovals →
+// BlockedTools) and the Servers page already counts it (pending_count +
+// changed_count). The banner and the count must agree, so the operator gets
+// the Approve/Block buttons to clear pending tools. While the server is still
+// quarantined the server-level banner covers everything, so we suppress the
+// tool-level banner to avoid two banners at once.
 
 function approval(over: Partial<ToolApproval>): ToolApproval {
   return {
@@ -22,11 +27,10 @@ function approval(over: Partial<ToolApproval>): ToolApproval {
   }
 }
 
-describe('selectQuarantinedTools (MCP-2101)', () => {
+describe('selectQuarantinedTools (MCP-2917)', () => {
   it('suppresses the tool banner entirely while the server is quarantined', () => {
-    // Even a rug-pull `changed` tool must not surface a SECOND banner while the
-    // server-level Security Quarantine banner is up — operator approves the
-    // server first, then the backend promotes baseline pending→approved.
+    // The server-level Security Quarantine banner already covers it and the
+    // operator must approve the server first — never show two banners at once.
     const tools = [
       approval({ tool_name: 'a', status: 'changed' }),
       approval({ tool_name: 'b', status: 'pending' }),
@@ -34,15 +38,17 @@ describe('selectQuarantinedTools (MCP-2101)', () => {
     expect(selectQuarantinedTools(tools, true)).toEqual([])
   })
 
-  it('does NOT surface freshly-pending baseline tools (the core fix)', () => {
-    // Not quarantined, no changed tool → baseline `pending` tools alone must
-    // not raise the banner. Pre-fix this returned the pending tools.
-    const tools = [
-      approval({ tool_name: 'a', status: 'pending' }),
-      approval({ tool_name: 'b', status: 'pending' }),
-      approval({ tool_name: 'c', status: 'approved' }),
-    ]
-    expect(selectQuarantinedTools(tools, false)).toEqual([])
+  it('surfaces freshly-pending tools on a non-quarantined server (the core fix)', () => {
+    // Regression for MCP-2917: an all-`pending`, not-quarantined server used to
+    // hide the banner (the old `hasChanged` early-return) even though every
+    // tool is genuinely blocked. Now they must surface for approval.
+    const a = approval({ tool_name: 'a', status: 'pending' })
+    const b = approval({ tool_name: 'b', status: 'pending' })
+    const tools = [a, b, approval({ tool_name: 'c', status: 'approved' })]
+    const result = selectQuarantinedTools(tools, false)
+    expect(result).toContain(a)
+    expect(result).toContain(b)
+    expect(result).not.toContainEqual(approval({ tool_name: 'c', status: 'approved' }))
   })
 
   it('surfaces a `changed` (rug-pull) tool when the server is not quarantined', () => {
@@ -51,7 +57,7 @@ describe('selectQuarantinedTools (MCP-2101)', () => {
     expect(selectQuarantinedTools(tools, false)).toEqual([changed])
   })
 
-  it('once a change has surfaced, also includes residual pending tools', () => {
+  it('surfaces both pending and changed tools together when not quarantined', () => {
     const changed = approval({ tool_name: 'rugpull', status: 'changed' })
     const pending = approval({ tool_name: 'leftover', status: 'pending' })
     const tools = [changed, pending, approval({ tool_name: 'fine', status: 'approved' })]