Skip to content

feat: MCP server management UI (#538)#1237

Open
bergeouss wants to merge 2 commits intonesquena:masterfrom
bergeouss:feat/538-mcp-server-management
Open

feat: MCP server management UI (#538)#1237
bergeouss wants to merge 2 commits intonesquena:masterfrom
bergeouss:feat/538-mcp-server-management

Conversation

@bergeouss
Copy link
Copy Markdown
Contributor

Add full MCP server management to the WebUI Settings > System panel.

Backend: GET/PUT/DELETE /api/mcp/servers endpoints with masked secrets. Frontend: server list with transport badges, add/edit/delete form. i18n: 21 keys in 7 locales. 21 tests. Closes #538

- Add GET /api/mcp/servers (list with masked secrets)
- Add PUT /api/mcp/servers/<name> (add/update stdio and http servers)
- Add DELETE /api/mcp/servers/<name> (remove server)
- MCP section in System settings with server list, add/delete form
- Auto-detect transport type (stdio vs http) from server config
- Mask sensitive values (API keys, tokens, passwords) in list response
- Uses showConfirmDialog for delete confirmation (no native confirm)
- i18n: 21 keys across 7 locales
- 21 tests (list, save, delete, mask_secrets, validation)
@nesquena-hermes
Copy link
Copy Markdown
Collaborator

Thanks for this PR! MCP server management in the Settings > System panel is a highly requested feature (#538), and the implementation scope looks right — REST endpoints with masked secrets on the backend, a management UI on the frontend, and i18n coverage across 7 locales.

A few things to confirm before merge:

Secret masking semantics
The GET /api/mcp/servers endpoint returns masked secrets. Confirming: is the masked value a fixed placeholder (e.g. "***") or a partial reveal (e.g. "sk-...xyz")? And does PUT /api/mcp/servers handle a masked-value submission as "leave secret unchanged" vs. treating it as a literal update? This is a common footgun where a round-trip edit accidentally overwrites a real secret with the masked placeholder.

Transport badge types
What transport types does the badge cover? At minimum stdio and http/sse — are there others? Confirming the badge handles unknown/future transport types gracefully (e.g. falls back to the raw string rather than erroring).

DELETE endpoint safety
Does DELETE /api/mcp/servers require any confirmation step on the frontend, or is deletion immediate? A mistaken delete of a configured server could silently break tool availability. Even a simple "are you sure?" modal would be a good guard.

21 tests: good coverage for a new API surface. Are the tests covering the masked-secret round-trip case (edit and re-submit with masked value should not overwrite the stored secret)?

Once the above are confirmed, this looks ready!

…#1237)

- Add _strip_masked_values() to skip masked placeholders in PUT endpoint,
  preserving the original stored secret values instead of overwriting them
- Fix transport badge to gracefully handle unknown/future transport types
  with a fallback that shows the raw string
- Add TestStripMaskedValues (5 tests) for the round-trip protection logic
- Addresses reviewer feedback on secret masking semantics and transport badge
@nesquena-hermes
Copy link
Copy Markdown
Collaborator

Thanks for the follow-up commit — the masked-value round-trip protection directly addresses the concern raised above. Confirming that PUT now treats a masked placeholder as "leave secret unchanged" rather than overwriting is exactly the right semantics.

If the other points (transport badge unknown-type fallback, DELETE confirmation) are also addressed or were already handled in the original implementation, this is ready to go. Looking good!

@bergeouss
Copy link
Copy Markdown
Contributor Author

Review Feedback Follow-up

Thanks for the confirmation on the masked-value round-trip fix!

Regarding the other two points you asked about — both were already handled in the original implementation:

  • Transport badge unknown-type fallback: static/panels.js line 3084 uses a ternary chain that defaults to mcp-unknown class (.mcp-unknown in style.css line 1715) when the transport type is neither http nor stdio.

    const transportClass=s.transport==='http'?'mcp-http':s.transport==='stdio'?'mcp-stdio':'mcp-unknown';
  • DELETE confirmation: deleteMcpServer() in static/panels.js line 3142 calls showConfirmDialog() before issuing the DELETE request, with danger: true and focusCancel: true flags.

    const _ok=await showConfirmDialog({title:t('mcp_delete_confirm_title'),message:t('mcp_delete_confirm_message',name),confirmLabel:t('delete_title'),danger:true,focusCancel:true});

Both are covered — ready for merge whenever you are. 🚀

🤖 AI-assisted via Hermes Agent

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.

feat(settings): MCP server management UI — add, remove, and inspect MCP servers without editing config.yaml

2 participants