feat(mcp): add MCP Server Support #53

Merged

sarman merged 4 commits from feature/mcp-server-support into master 2026-05-23 22:15:11 +00:00

Conversation 0 Commits 4 Files Changed 27 +3027 -19

sarman commented 2026-05-23 21:25:05 +00:00

Owner

Copy Link

Summary

Adds full Model Context Protocol (MCP) server management, enabling the AI assistant to discover and call tools from external MCP servers during triage conversations.

What was built

Backend (Rust)

• rmcp = "1.7.0" dependency with stdio + Streamable HTTP transports
• Migration 018: mcp_servers, mcp_tools, mcp_resources tables with CHECK constraints
• src/mcp/ module: models, store, client, adapter, discovery, commands, transport/{stdio,http}
• AppState.mcp_connections: Arc<TokioMutex<HashMap<...>>> — live server connections
• .setup() hook auto-discovers enabled servers at application startup
• 8 new Tauri commands registered in invoke_handler
• execute_mcp_tool_call: PII scan + mandatory write_audit_event before every external tool call
• auth_value encrypted at rest (AES-256-GCM); scrubbed from every frontend response
• stdio transport rejects relative paths; uses Command::new() directly (no shell)

Frontend

• MCPServers.tsx settings page at /settings/mcp
• Server list with status badges (connected/pending/error), Discover Now button, enable/disable toggle
• Add/Edit modal: transport type (stdio/http), auth type (none/api_key/bearer/oauth2)
• tauriCommands.ts: McpServer, McpTool, McpServerStatus types + 8 typed command wrappers
• App.tsx: Plug icon, /settings/mcp route, sidebar nav entry

Wiki

• docs/wiki/MCP-Servers.md (new)
• docs/wiki/Database.md, IPC-Commands.md, Security-Model.md updated

Test plan

• 185/185 Rust tests pass (15 new TDD tests: 5 migration + 5 store + 5 adapter)
• 94/94 Vitest tests pass
• cargo clippy -- -D warnings: zero warnings
• npx tsc --noEmit: zero errors
• Security review: 9/9 requirements verified
• Manual: add HTTP MCP server → Discover Now → tools appear
• Manual: add stdio MCP server → tools appear, process spawned
• Manual: AI calls MCP tool → audit log entry written
• Manual: disable server → tools absent from next chat session
• Manual: relative path in stdio command → rejected with error

## Summary Adds full Model Context Protocol (MCP) server management, enabling the AI assistant to discover and call tools from external MCP servers during triage conversations. ### What was built **Backend (Rust)** - `rmcp = "1.7.0"` dependency with stdio + Streamable HTTP transports - Migration 018: `mcp_servers`, `mcp_tools`, `mcp_resources` tables with CHECK constraints - `src/mcp/` module: models, store, client, adapter, discovery, commands, transport/{stdio,http} - `AppState.mcp_connections: Arc<TokioMutex<HashMap<...>>>` — live server connections - `.setup()` hook auto-discovers enabled servers at application startup - 8 new Tauri commands registered in `invoke_handler` - `execute_mcp_tool_call`: PII scan + mandatory `write_audit_event` before every external tool call - `auth_value` encrypted at rest (AES-256-GCM); scrubbed from every frontend response - stdio transport rejects relative paths; uses `Command::new()` directly (no shell) **Frontend** - `MCPServers.tsx` settings page at `/settings/mcp` - Server list with status badges (connected/pending/error), Discover Now button, enable/disable toggle - Add/Edit modal: transport type (stdio/http), auth type (none/api_key/bearer/oauth2) - `tauriCommands.ts`: `McpServer`, `McpTool`, `McpServerStatus` types + 8 typed command wrappers - `App.tsx`: Plug icon, `/settings/mcp` route, sidebar nav entry **Wiki** - `docs/wiki/MCP-Servers.md` (new) - `docs/wiki/Database.md`, `IPC-Commands.md`, `Security-Model.md` updated ## Test plan - [x] 185/185 Rust tests pass (15 new TDD tests: 5 migration + 5 store + 5 adapter) - [x] 94/94 Vitest tests pass - [x] `cargo clippy -- -D warnings`: zero warnings - [x] `npx tsc --noEmit`: zero errors - [x] Security review: 9/9 requirements verified - [ ] Manual: add HTTP MCP server → Discover Now → tools appear - [ ] Manual: add stdio MCP server → tools appear, process spawned - [ ] Manual: AI calls MCP tool → audit log entry written - [ ] Manual: disable server → tools absent from next chat session - [ ] Manual: relative path in stdio command → rejected with error

sarman self-assigned this 2026-05-23 21:25:05 +00:00

sarman added 1 commit 2026-05-23 21:25:06 +00:00

feat(mcp): add MCP Server Support with TDD implementation ... 3588399dfd

Adds full Model Context Protocol (MCP) server management, enabling the
AI assistant to discover and call tools from external MCP servers during
triage conversations.

Backend (Rust):
- rmcp 1.7.0 dependency (client + stdio + Streamable HTTP transports)
- Migration 018: mcp_servers, mcp_tools, mcp_resources tables with
  CHECK constraints for transport_type, auth_type, discovery_status
- src/mcp/ module: models, store, client, adapter, discovery, commands,
  transport/{stdio,http}
- AppState gains mcp_connections: Arc<TokioMutex<HashMap<...>>>
- .setup() hook auto-discovers enabled servers at startup
- 8 new Tauri commands wired into invoke_handler
- execute_mcp_tool_call: PII scan + mandatory audit_log before execution
- Auth values encrypted at rest via integrations::auth::encrypt_token();
  scrubbed before any frontend response

Frontend:
- MCPServers.tsx settings page (/settings/mcp) with server list,
  status badges, Discover Now, Add/Edit modal, enable/disable toggle
- tauriCommands.ts: McpServer, McpTool, McpServerStatus types + 8 cmds
- App.tsx: Plug icon, /settings/mcp route, sidebar nav entry

Tests (TDD): 15 new tests, all green
- 5 migration tests (written before migration, red → green)
- 5 store CRUD + encryption tests
- 5 adapter sanitization + conversion tests

Verification: 185/185 Rust, 94/94 Vitest, clippy -D warnings: 0

sarman added 1 commit 2026-05-23 21:48:30 +00:00

style(mcp): apply rustfmt formatting a779756e48

sarman reviewed 2026-05-23 21:49:18 +00:00

sarman left a comment

Author

Owner

Copy Link

Automated PR Review could not be completed - LLM analysis failed or produced no output.

Automated PR Review could not be completed - LLM analysis failed or produced no output.

sarman added 1 commit 2026-05-23 21:54:30 +00:00

fix(ci): use qwen3-coder-next model for PR review 8e1145ec7a

sarman reviewed 2026-05-23 22:01:15 +00:00

sarman left a comment

Author

Owner

Copy Link

Automated PR Review (qwen3-coder-next via liteLLM):\n\nHere's a detailed security, correctness, and best practices review of the provided code changes for the MCP Server Support feature:

---

✅ 1. Summary

This PR adds robust support for the Model Context Protocol (MCP), enabling the app to integrate external tools via stdio or HTTP transports. The implementation is well-structured, with strong separation of concerns (backend Rust modules, frontend React components, and documentation). Key strengths:

• Security-first design: encrypted auth_value, PII scanning, mandatory audit logging
• Correct transport handling (absolute path enforcement for stdio, no shell invocation)
• Full CRUD + discovery via a clean IPC layer
• Comprehensive tests and documentation

Overall assessment: The PR demonstrates high code quality, adherence to security best practices, and solid engineering judgment. Only minor improvements are suggested.

---

⚠️ 2. Bugs & Correctness Issues

🚫 Critical: Missing auth_type Field in Schema & Models

• In docs/wiki/Database.md, mcp_servers table definition does not include auth_type, yet:
  • The UI (MCPServers.tsx) assumes auth_type is configurable.
  • MCP_SERVER_SUPPORT.md and docs describe 4 auth types: none, api_key, bearer, oauth2.
  • commands.rs and store.rs likely use auth_type.

Problem: Without auth_type, auth handling becomes ambiguous or broken.

🔧 Fix:

• Add auth_type TEXT NOT NULL DEFAULT 'none' CHECK(auth_type IN ('none','api_key','bearer','oauth2')) to mcp_servers.
• Update models.rs, store.rs, and frontend types accordingly.

💡 Note: If auth_type is derived from url or transport_config, that’s fragile and undocumented—explicit auth_type is safer.

---

🐞 Stdio Transport: Missing sh -c Bypass Is Not Guaranteed

"stdio transport rejects relative paths; never uses sh -c" is claimed in acceptance criteria.

In mcp/transport/stdio.rs, rmcp::transport::TokioChildProcess::new() is used, which does not invoke a shell by default—good!
However, the documentation in MCP-Servers.md says:

command — must be an absolute path. Relative paths are rejected to prevent path traversal attacks.

But:

• The current Rust code only checks for relative paths at the API/validation layer (e.g., in store.rs or commands.rs).
• If rmcp internally constructs the command string via shell_expand() (unlikely, but not documented), that could reintroduce risk.

🔍 Recommendation:

• Explicitly verify in stdio.rs that command is absolute and canonicalized before passing to rmcp.
• Add unit test: test_rejects_relative_path().
• Document in stdio.rs that no shell invocation occurs.

---

🚨 Potential Auth Leak in Error Paths

In mcp/adapter.rs → execute_mcp_tool_call:

• Audit events & PII scans happen, but no mention of sanitizing auth tokens from error logs or telemetry.

If rmcp or your stdio/HTTP client logs errors (e.g., "Auth failed: Bearer abc..."), the auth_value could leak.

🔧 Fix:

• Ensure all trace!(), warn!(), log::error!() calls strip auth headers.
• Sanitize responses before logging or forwarding to frontend.
• Add integration test: test_error_does_not_leak_auth.

---

🔐 3. Security Issues

✅ Good Practices Already Implemented

• ✅ auth_value stored encrypted with AES-256-GCM (same system as other secrets).
• ✅ Auth scrubbed in list_mcp_servers → auth_value: null.
• ✅ Mandatory write_audit_event() before tool calls.
• ✅ PII scanning (non-blocking) on tool args.
• ✅ No shell (sh -c) for stdio; absolute path enforced.

⚠️ Security Gaps to Address

1. OAuth2 Token Exchange: No CSRF Protection

The initiate_mcp_oauth() opens a WebView and handles the redirect.
But:

• No state nonce or state parameter is mentioned (critical for PKCE/CSRF mitigation).
• No timeout on auth flow (could hang).

🔧 Fix:

• Use PKCE (RFC 7636) for OAuth2.
• Generate & store a cryptographically random state + code_verifier in session storage.
• Validate state on redirect before exchanging the code.
• Add timeout (e.g., 2 min) and fail gracefully.

2. Missing Rate Limiting / Circuit Breaking for External Calls

• If an MCP server misbehaves (slow, DDoS-prone), it could stall the entire app.
• No timeout defined for discover_mcp_server or execute_mcp_tool_call.

🔧 Fix:

• Add configurable timeouts per server (e.g., discovery_timeout_ms, call_timeout_ms in transport_config).
• Wrap rmcp calls with tokio::time::timeout().
• Emit warning if discovery takes > X seconds.

3. Cascade Delete Without Confirmation

• Deleting a server triggers ON DELETE CASCADE on tools/resources.
• No audit event logs which tools were deleted (unless done in code — but no indication).

🔧 Fix:

• Add write_audit_event("mcp_server_deleted", ...) that includes deleted tool/resource IDs.
• Consider soft-delete (e.g., deleted_at) if tools are reused.

---

🌟 4. Best Practices

✅ Excellent

• Full migration with idempotency checks (per test list).
• cargo clippy -- -D warnings enforced.
• Clear module boundaries (mcp/ submodules).
• Frontend ↔ backend contract documented (IPC-Commands.md, docs/).
• Schema constraints (CHECK) on enums (transport_type, discovery_status).

📝 Suggestions for Improvement

1. Missing API Versioning / Schema Evolution Plan

• transport_config is a TEXT (JSON), but no versioning or deprecation strategy.
• What happens when the schema of transport_config changes (e.g., args becomes env + args)?

🔧 Fix:

• Add transport_config_version INTEGER NOT NULL DEFAULT 1.
• Migrate on change.
• Or use serde_json::Value + #[serde(default)] + version checks.

2. Frontend: auth_value Type Mismatch Risk

In src/lib/tauriCommands.ts, the request types (CreateMcpServerRequest) include auth_value?: string.

But if auth_type = "none", should users be allowed to provide auth_value? Currently, the frontend might let them, and the backend might store an empty string or ignore it.

🔧 Fix:

• In MCPServers.tsx, disable auth_value input when auth_type = "none".
• In backend, auth_value = NULL when auth_type = "none".

3. Discovery Status Not Resettable

• If a server transitions to error, there's no way to reset it to pending except delete/re-add.

🔧 Fix:

• Allow update_mcp_server to set discovery_status = 'pending' (e.g., for manual retry).

4. id Field: UUID vs. User-Friendly

• Server IDs are TEXT PRIMARY KEY. Are they auto-generated UUIDs? Or user-specified?

🔧 Fix:

• Confirm IDs are UUIDv7 or similar (not user-facing strings like "server-1" that encourage hardcoding).
• If human-readable IDs, enforce ^[a-z0-9-]+$.

---

✅ Final Verdict

Category	Assessment
Correctness	✅ Very good; only auth_type missing from DB schema
Security	✅ Strong baseline; needs PKCE & timeout safeguards
Best Practices	✅ Excellent; minor UX/evolution gaps remain

✅ Approve with Recommendations

This PR is production-ready with the following changes before merge:

1. Add auth_type column to migration 018 and models.
2. Add PKCE & state parameter to OAuth2 flow.
3. Add timeouts for all MCP remote calls.
4. Audit log tool deletions on server delete.

Once addressed, this becomes a gold-standard feature PR: secure, well-documented, and thoroughly tested.

Let me know if you'd like PR comment-ready language for these points!\n\n---\nautomated code review

Automated PR Review (qwen3-coder-next via liteLLM):\n\nHere's a detailed security, correctness, and best practices review of the provided code changes for the **MCP Server Support** feature: --- ## ✅ **1. Summary** This PR adds robust support for the **Model Context Protocol (MCP)**, enabling the app to integrate external tools via `stdio` or `HTTP` transports. The implementation is **well-structured**, with strong separation of concerns (backend Rust modules, frontend React components, and documentation). Key strengths: - Security-first design: encrypted `auth_value`, PII scanning, mandatory audit logging - Correct transport handling (absolute path enforcement for `stdio`, no shell invocation) - Full CRUD + discovery via a clean IPC layer - Comprehensive tests and documentation **Overall assessment**: The PR demonstrates **high code quality**, adherence to security best practices, and solid engineering judgment. Only minor improvements are suggested. --- ## ⚠️ **2. Bugs & Correctness Issues** ### 🚫 **Critical: Missing `auth_type` Field in Schema & Models** - In `docs/wiki/Database.md`, `mcp_servers` table definition **does not include `auth_type`**, yet: - The UI (`MCPServers.tsx`) assumes `auth_type` is configurable. - `MCP_SERVER_SUPPORT.md` and docs describe 4 auth types: `none`, `api_key`, `bearer`, `oauth2`. - `commands.rs` and `store.rs` likely use `auth_type`. **Problem**: Without `auth_type`, auth handling becomes ambiguous or broken. #### 🔧 Fix: - Add `auth_type TEXT NOT NULL DEFAULT 'none' CHECK(auth_type IN ('none','api_key','bearer','oauth2'))` to `mcp_servers`. - Update `models.rs`, `store.rs`, and frontend types accordingly. > 💡 *Note:* If `auth_type` is derived from `url` or `transport_config`, that’s fragile and undocumented—explicit `auth_type` is safer. --- ### 🐞 **Stdio Transport: Missing `sh -c` Bypass Is Not Guaranteed** > *"stdio transport rejects relative paths; never uses `sh -c`"* is claimed in acceptance criteria. In `mcp/transport/stdio.rs`, `rmcp::transport::TokioChildProcess::new()` is used, which **does not invoke a shell** by default—good! However, the *documentation in `MCP-Servers.md` says*: > > `command` — **must be an absolute path**. Relative paths are rejected to prevent path traversal attacks. But: - The current Rust code *only checks for relative paths* at the API/validation layer (e.g., in `store.rs` or `commands.rs`). - If `rmcp` internally constructs the command string via `shell_expand()` (unlikely, but not documented), that could reintroduce risk. #### 🔍 Recommendation: - Explicitly verify in `stdio.rs` that `command` is absolute and canonicalized *before* passing to `rmcp`. - Add unit test: `test_rejects_relative_path()`. - Document in `stdio.rs` that no shell invocation occurs. --- ### 🚨 **Potential Auth Leak in Error Paths** In `mcp/adapter.rs` → `execute_mcp_tool_call`: - Audit events & PII scans happen, but **no mention of sanitizing auth tokens from error logs or telemetry**. If `rmcp` or your stdio/HTTP client logs errors (e.g., `"Auth failed: Bearer abc..."`), the `auth_value` could leak. #### 🔧 Fix: - Ensure all `trace!()`, `warn!()`, `log::error!()` calls strip auth headers. - Sanitize responses *before* logging or forwarding to frontend. - Add integration test: `test_error_does_not_leak_auth`. --- ## 🔐 **3. Security Issues** ### ✅ **Good Practices Already Implemented** - ✅ `auth_value` stored **encrypted with AES-256-GCM** (same system as other secrets). - ✅ Auth scrubbed in `list_mcp_servers` → `auth_value: null`. - ✅ Mandatory `write_audit_event()` before tool calls. - ✅ PII scanning (non-blocking) on tool args. - ✅ No shell (`sh -c`) for stdio; absolute path enforced. ### ⚠️ **Security Gaps to Address** #### 1. **OAuth2 Token Exchange: No CSRF Protection** The `initiate_mcp_oauth()` opens a WebView and handles the redirect. But: - No state nonce or `state` parameter is mentioned (critical for PKCE/CSRF mitigation). - No timeout on auth flow (could hang). #### 🔧 Fix: - Use **PKCE** (RFC 7636) for OAuth2. - Generate & store a cryptographically random `state` + `code_verifier` in session storage. - Validate `state` on redirect before exchanging the code. - Add timeout (e.g., 2 min) and fail gracefully. #### 2. **Missing Rate Limiting / Circuit Breaking for External Calls** - If an MCP server misbehaves (slow, DDoS-prone), it could stall the entire app. - No timeout defined for `discover_mcp_server` or `execute_mcp_tool_call`. #### 🔧 Fix: - Add configurable timeouts per server (e.g., `discovery_timeout_ms`, `call_timeout_ms` in `transport_config`). - Wrap `rmcp` calls with `tokio::time::timeout()`. - Emit warning if discovery takes > X seconds. #### 3. **Cascade Delete Without Confirmation** - Deleting a server triggers `ON DELETE CASCADE` on tools/resources. - No audit event logs *which tools were deleted* (unless done in code — but no indication). #### 🔧 Fix: - Add `write_audit_event("mcp_server_deleted", ...)` that includes deleted tool/resource IDs. - Consider soft-delete (e.g., `deleted_at`) if tools are reused. --- ## 🌟 **4. Best Practices** ### ✅ **Excellent** - Full migration with idempotency checks (per test list). - `cargo clippy -- -D warnings` enforced. - Clear module boundaries (`mcp/` submodules). - Frontend ↔ backend contract documented (`IPC-Commands.md`, `docs/`). - Schema constraints (`CHECK`) on enums (`transport_type`, `discovery_status`). ### 📝 **Suggestions for Improvement** #### 1. **Missing API Versioning / Schema Evolution Plan** - `transport_config` is a `TEXT` (JSON), but no versioning or deprecation strategy. - What happens when the schema of `transport_config` changes (e.g., `args` becomes `env` + `args`)? #### 🔧 Fix: - Add `transport_config_version INTEGER NOT NULL DEFAULT 1`. - Migrate on change. - Or use `serde_json::Value` + `#[serde(default)]` + version checks. #### 2. **Frontend: `auth_value` Type Mismatch Risk** In `src/lib/tauriCommands.ts`, the request types (`CreateMcpServerRequest`) include `auth_value?: string`. But if `auth_type = "none"`, should users be allowed to provide `auth_value`? Currently, the frontend might let them, and the backend might store an empty string or ignore it. #### 🔧 Fix: - In `MCPServers.tsx`, disable `auth_value` input when `auth_type = "none"`. - In backend, `auth_value = NULL` when `auth_type = "none"`. #### 3. **Discovery Status Not Resettable** - If a server transitions to `error`, there's no way to reset it to `pending` except delete/re-add. #### 🔧 Fix: - Allow `update_mcp_server` to set `discovery_status = 'pending'` (e.g., for manual retry). #### 4. **`id` Field: UUID vs. User-Friendly** - Server IDs are `TEXT PRIMARY KEY`. Are they auto-generated UUIDs? Or user-specified? #### 🔧 Fix: - Confirm IDs are UUIDv7 or similar (not user-facing strings like `"server-1"` that encourage hardcoding). - If human-readable IDs, enforce `^[a-z0-9-]+$`. --- ## ✅ **Final Verdict** | Category | Assessment | |----------|------------| | **Correctness** | ✅ Very good; only `auth_type` missing from DB schema | | **Security** | ✅ Strong baseline; needs PKCE & timeout safeguards | | **Best Practices** | ✅ Excellent; minor UX/evolution gaps remain | ### ✅ **Approve with Recommendations** **This PR is production-ready *with* the following changes before merge:** 1. **Add `auth_type` column** to migration 018 and models. 2. **Add PKCE & `state` parameter** to OAuth2 flow. 3. **Add timeouts** for all MCP remote calls. 4. **Audit log tool deletions** on server delete. Once addressed, this becomes a **gold-standard feature PR**: secure, well-documented, and thoroughly tested. Let me know if you'd like PR comment-ready language for these points!\n\n---\n*automated code review*

sarman added 1 commit 2026-05-23 22:08:27 +00:00

fix(mcp): add timeouts, delete audit log, OAuth state nonce; improve PR review prompt ... 28cfcf1ef4

- call_tool: 30s hard timeout via tokio::time::timeout
- discover_server: 60s hard timeout wrapping full connect+discover sequence
- delete_mcp_server: write_audit_event before cascade delete, capturing
  server name, tool count, and resource count
- initiate_mcp_oauth: append cryptographically random state nonce for CSRF
- pr-review.yml: rewrite prompt to require line-quoted evidence for every
  finding and eliminate hallucinated false positives

sarman reviewed 2026-05-23 22:13:17 +00:00

sarman left a comment

Author

Owner

Copy Link

Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary
This PR adds MCP (Model Context Protocol) server support, including database migrations, backend services, frontend UI, and documentation. The implementation covers server management (CRUD + discovery), two transports (stdio, HTTP), four auth types (none, api_key, bearer, oauth2), encrypted credential storage, audit logging, PII scanning, and integration with the AI tooling pipeline. All automated tests pass, and zero clippy/TSLint errors are reported.

Findings

• [WARNING] MCP_SERVER_SUPPORT.md:line 71 — The documentation states that auth_value is “Always null in responses (scrubbed server-side)” for list_mcp_servers, but the IPC-Commands.md example shows auth_value?: string in McpServer interface. This inconsistency could mislead implementers or users about whether auth_value might ever be present in responses.
  → Suggested fix: Align the field to auth_value: null (i.e., remove it entirely) from the McpServer interface in IPC-Commands.md, or explicitly clarify that the field is omitted (not null) in all responses.

• [WARNING] MCP_SERVER_SUPPORT.md:line 237 — The “Adding an MCP Server (UI)” guide states: “For HTTP: typically {}” for transport_config. However, for HTTP servers, connection configuration (url) lives on the top-level url field, not in transport_config. This phrasing may confuse users into unnecessarily populating transport_config with irrelevant fields like "url": "...".
  → Suggested fix: Clarify that for HTTP transport, transport_config is always {} (empty JSON), and url is set separately at the server root level.

• [SUGGESTION] MCP_SERVER_SUPPORT.md:line 269 — The line \n\n## Instructions\n at the end of the diff’s MCP_SERVER_SUPPORT.md file appears to be an accidental leftover from the PR review prompt template (not part of the intended documentation).
  → Suggested fix: Remove the line and everything below it in that file, as it is not relevant to the feature documentation.

• [SUGGESTION] docs/wiki/MCP-Servers.md:line 135 — The “Authentication Types” table includes oauth2, but the table header (line 135) and preceding description list only api_key, bearer, and none. Though oauth2 is explained later, its omission from the summary table reduces discoverability.
  → Suggested fix: Update the table header to include oauth2, or add a footnote clarifying its inclusion.

Verdict: APPROVE WITH COMMENTS\n\n---\nautomated code review

Automated PR Review (qwen3-coder-next via liteLLM):\n\n**Summary** This PR adds MCP (Model Context Protocol) server support, including database migrations, backend services, frontend UI, and documentation. The implementation covers server management (CRUD + discovery), two transports (stdio, HTTP), four auth types (none, api_key, bearer, oauth2), encrypted credential storage, audit logging, PII scanning, and integration with the AI tooling pipeline. All automated tests pass, and zero clippy/TSLint errors are reported. **Findings** - [WARNING] MCP_SERVER_SUPPORT.md:line 71 — The documentation states that `auth_value` is “Always null in responses (scrubbed server-side)” for `list_mcp_servers`, but the IPC-Commands.md example shows `auth_value?: string` in `McpServer` interface. This inconsistency could mislead implementers or users about whether `auth_value` might ever be present in responses. → *Suggested fix*: Align the field to `auth_value: null` (i.e., remove it entirely) from the `McpServer` interface in IPC-Commands.md, or explicitly clarify that the field is omitted (not null) in all responses. - [WARNING] MCP_SERVER_SUPPORT.md:line 237 — The “Adding an MCP Server (UI)” guide states: *“For HTTP: typically `{}`”* for `transport_config`. However, for HTTP servers, connection configuration (`url`) lives on the top-level `url` field, not in `transport_config`. This phrasing may confuse users into unnecessarily populating `transport_config` with irrelevant fields like `"url": "..."`. → *Suggested fix*: Clarify that for HTTP transport, `transport_config` is always `{}` (empty JSON), and `url` is set separately at the server root level. - [SUGGESTION] MCP_SERVER_SUPPORT.md:line 269 — The line `\n\n## Instructions\n` at the end of the diff’s `MCP_SERVER_SUPPORT.md` file appears to be an accidental leftover from the PR review prompt template (not part of the intended documentation). → *Suggested fix*: Remove the line and everything below it in that file, as it is not relevant to the feature documentation. - [SUGGESTION] docs/wiki/MCP-Servers.md:line 135 — The *“Authentication Types”* table includes `oauth2`, but the table header (line 135) and preceding description list only `api_key`, `bearer`, and `none`. Though `oauth2` is explained later, its omission from the summary table reduces discoverability. → *Suggested fix*: Update the table header to include `oauth2`, or add a footnote clarifying its inclusion. **Verdict**: APPROVE WITH COMMENTS\n\n---\n*automated code review*

sarman merged commit ea7f484ce6 into master 2026-05-23 22:15:11 +00:00

sarman referenced this issue from a commit 2026-05-23 22:15:12 +00:00

Merge pull request 'feat(mcp): add MCP Server Support' (#53) from feature/mcp-server-support into master

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

Compat/Breaking

Breaking change that won't be backward compatible

  

Kind/Bug

Something is not working

  

Kind/Documentation

Documentation changes

  

Kind/Enhancement

Improve existing functionality

  

Kind/Feature

New functionality

  

Kind/Security

This is security issue

  

Kind/Testing

Issue or pull request related to testing

  

Priority

Critical

The priority is critical

  

Priority

High

The priority is high

  

Priority

Low

The priority is low

  

Priority

Medium

The priority is medium

  

Reviewed

Confirmed

Issue has been confirmed

  

Reviewed

Duplicate

This issue or pull request already exists

  

Reviewed

Invalid

Invalid issue

  

Reviewed

Won't Fix

This issue won't be fixed

  

Status

Abandoned

Somebody has started to work on this but abandoned work

  

Status

Blocked

Something is blocking this issue or pull request

  

Status

Need More Info

Feedback is required to reproduce issue or to continue work

No Label

Compat/Breaking

Kind/Bug

Kind/Documentation

Kind/Enhancement

Kind/Feature

Kind/Security

Kind/Testing

Priority

Critical

Priority

High

Priority

Low

Priority

Medium

Reviewed

Confirmed

Reviewed

Duplicate

Reviewed

Invalid

Reviewed

Won't Fix

Status

Abandoned

Status

Blocked

Status

Need More Info

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

sarman

1 Participants

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: sarman/tftsr-devops_investigation#53

No description provided.