Skip to content

fix(webui): surface tool-level quarantine on the Servers list#519

Merged
Dumbris merged 1 commit into
smart-mcp-proxy:mainfrom
HaloCollar:fix/server-card-tool-quarantine-banner
May 24, 2026
Merged

fix(webui): surface tool-level quarantine on the Servers list#519
Dumbris merged 1 commit into
smart-mcp-proxy:mainfrom
HaloCollar:fix/server-card-tool-quarantine-banner

Conversation

@electrolobzik
Copy link
Copy Markdown
Contributor

Problem

The Servers grid in the Web UI renders the big orange Server is quarantined banner + Approve button only when server.quarantined === true. Servers that are trusted at the server level but ship tools blocked by Spec 032 tool-level quarantine (pending / changed) get only a tiny stat-desc line under the tool count — easy to miss, and shadowed entirely by the X disabled line whenever any tool is also disabled, because both lines share a v-if / v-else-if chain at ServerCard.vue:32–46.

The detail page does escalate tool-level quarantine to its own yellow alert with an Approve-All button (ServerDetail.vue:325). So users who hit the daemon's quarantine pending approval error find clear remediation on Details, but nothing actionable on the list view — and they report the list as broken or inconsistent.

A common path into this state is setup.sh-style provisioning that registers servers with --no-quarantine but doesn't catch every tool in the post-add auto-approve pass (OAuth servers like Atlassian, lazy-spawn servers, or anything exceeding the approval-poll timeout). Those servers end up quarantined=false + quarantine.pending_count > 0 — exactly the gap this PR closes.

Change

  • Adds a second alert-warning banner in ServerCard.vue for tool-level quarantine, with a Review link to /servers/<name>?tab=tools. Mutually exclusive with the server-level banner via v-else-if — when the server itself is quarantined the existing banner already implies the tools won't run.
  • Splits the stat-desc v-if / v-else-if chain so X disabled and N pending approval can render side-by-side instead of one masking the other. The <token-size> hint stays the "nothing wrong" fallback.
  • Adds a toolQuarantineSummary computed that picks one of:
    • All N tools pending security approval (every tool pending)
    • N of M tools pending security approval (partial)
    • N tools changed since approval — re-review needed (rug-pull guard)
    • mixed pending, changed form when both apply

No backend change. The quarantine.{pending_count,changed_count,blocked_count} block is already populated by enrichServersWithQuarantineStats in both REST list (internal/httpapi/server.go:1140) and SSE paths (internal/runtime/event_bus.go:270). The v-memo deps on Servers.vue:165–175 already include both pending_count and changed_count, so the new banner re-renders reactively.

Tests

Extends frontend/src/components/__tests__/ServerCard.test.ts with five new cases:

  • partial: 3 of 10 tools pending + Review link present + no server-level banner
  • full: All 4 tools pending
  • changed-only (rug pull): 2 tools changed since approval
  • precedence: server-level banner wins when server.quarantined === true
  • mixed disabled+pending: both stat-desc lines render together

Test plan

  • CI runs frontend tests with the new cases (could not exercise locally — sandboxed environment couldn't reach @guolao/vue-monaco-editor to reinstall node modules).
  • Manual: add a server with --no-quarantine and tools that aren't auto-approved → list now shows yellow banner + Review link.
  • Manual: quarantine a server manually → only the existing Server is quarantined banner shows (no double warning).
  • Manual: server with some tools disabled and some pending → both stat-desc lines render.

The Servers grid only renders the big orange "Server is quarantined"
banner + Approve button when `server.quarantined === true`. Servers
that are trusted at the server level but ship tools blocked by Spec 032
tool-level quarantine (pending / changed) had only a tiny `stat-desc`
under the tool count — easy to miss, and shadowed entirely by the
`X disabled` line when any tool was also disabled, because both lines
shared a `v-if / v-else-if` chain.

The asymmetry is confusing: the detail page DOES escalate tool-level
quarantine to a yellow alert panel with an Approve-All button, so users
who hit the "tool requires approval" error from the daemon find
clear remediation on Details but nothing on the list view. They report
the list as broken or inconsistent.

This commit:

* Adds a second `alert-warning` banner (`v-else-if="quarantineToolCount > 0"`)
  with a Review link that routes to `/servers/<name>?tab=tools`. Server
  and tool quarantine remain mutually exclusive on the card — the
  server-level banner already implies the tools won't run, so we don't
  stack two warnings.
* Splits the stat-desc `v-if / v-else-if` chain so `X disabled` and
  `N pending approval` can render side-by-side. Token-size hint stays
  the "nothing wrong" fallback.
* Computes a context-aware summary string:
  - "All N tools pending security approval" when every tool is pending
  - "N of M tools pending security approval" when partial
  - "N tools changed since approval — re-review needed" for rug-pull
  - mixed pending+changed when both apply

No backend change: the `quarantine.pending_count` / `changed_count` /
`blocked_count` block is already populated by
`enrichServersWithQuarantineStats` (REST + SSE).

Extends the existing ServerCard test with five new cases covering the
partial / full / changed / mixed-with-server-level / mixed-with-disabled
scenarios.
@codecov-commenter
Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Member

@Dumbris Dumbris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, Roman — really nicely scoped fix. 🙏

A few things I appreciated:

  • Correct precedence: routing the tool-level banner through v-else-if after server.quarantined means a fully-quarantined server shows only the existing banner — no double warning. The dedicated test for this makes the intent unambiguous.
  • Unbundling the stat-desc chain so X disabled and N pending approval can coexist is exactly right — the old v-else-if masking was the subtle part of the bug, and you caught it.
  • quarantineToolCount summing pending_count + changed_count means the rug-pull (changed-only) path correctly surfaces the banner, and toolQuarantineSummary covers all four states (all/partial/changed/mixed) with clean pluralization.
  • Zero backend change, and you verified the quarantine.{pending,changed}_count block + v-memo deps were already wired for reactivity. Solid homework.

Five well-targeted test cases. CI frontend job is green, so the local sandbox limitation is fully covered.

One tiny non-blocking nit for a future pass: the compact stat-desc still reads {{ quarantineToolCount }} pending approval while the count includes changed tools, so a changed-only server reads "2 pending approval" in the small line (the banner itself says "changed" correctly). Not worth holding the merge.

Approving — thank you!

@Dumbris Dumbris merged commit 1cb6d98 into smart-mcp-proxy:main May 24, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants