feat(mcp): v1.1.0 MCP Platform Hardening + stdio support#71
Open
TinDang97 wants to merge 51 commits into
Open
Conversation
Single plan closes MCPI-01: patches _build_stream_config to append mcp__<key>__* wildcard patterns for each remote server into allowed_tools.
…s (MCPI-01) Remote MCP servers were loaded into the SDK session but their tools were never added to allowed_tools, so Claude could see them but not invoke them. Generate mcp__<key>__* patterns for each remote server and append to additional_tools in configure_sdk_for_space.
4 plans across 2 waves covering MCPI-02 through MCPI-06: - 31-01 (wave 1): DB migration + McpTransportType model/schema - 31-02 (wave 2): SSRF extraction to infrastructure/ssrf.py + _load_remote_mcp_servers hardening - 31-03 (wave 2): Server cap enforcement in repository + router - 31-04 (wave 1): Startup encryption key enforcement in main.py lifespan
- Add fail-fast check in main.py lifespan after jwt_provider_validated - Raises RuntimeError for production + empty ENCRYPTION_KEY - Raises RuntimeError for production + invalid Fernet-format key - Non-production environments are unaffected (dev key fallback preserved) - Error messages include Fernet key generation command for operators - Add 7 unit tests covering all enforcement scenarios
…EQUIREMENTS - Create 31-04-SUMMARY.md with task details and test coverage - Update STATE.md: 31-04 complete, 1/5 plans done, decisions recorded - Mark MCPI-06 complete in REQUIREMENTS.md
- Add McpTransportType StrEnum (SSE/HTTP) to workspace_mcp_server model - Add transport_type mapped column with server_default='sse' - Export McpTransportType from __all__ - Create migration 091 (down_revision=090_add_tags_and_usage_to_skills) - Add transport_type field to WorkspaceMcpServerCreate (default SSE) - Add transport_type field to WorkspaceMcpServerResponse (default SSE) - Pass transport_type from request body to WorkspaceMcpServer constructor - Add schema-level tests: test_transport_type_http_stored, test_transport_type_defaults_sse
- Add count_active_by_workspace() to WorkspaceMcpServerRepository (func.count WHERE is_deleted=False) - Add MCP_SERVER_CAP = 10 constant in workspace_mcp_servers.py - Cap check runs after set_rls_context, before WorkspaceMcpServer construction - 11th registration returns HTTP 422 with human-readable remediation message - Add 7 tests: boundary, message content, method existence, empty count, excludes-deleted count
…TTP transport (MCPI-02/03/05) - Extract SSRF validation to infrastructure/ssrf.py (shared utility) - Update _mcp_server_schemas.py to import validate_mcp_url from ssrf.py - Harden _load_remote_mcp_servers: skip failed servers (MCPI-03), re-validate URL at connect time (MCPI-05), branch on transport_type for HTTP/SSE configs (MCPI-02) - Add 5 unit tests: failed_server_skipped, ssrf_blocked_at_connect, http_transport_config, sse_transport_config, failed_does_not_block_others
… — SUMMARY, STATE, REQUIREMENTS
Three plans: DB migration + callback storage (wave 1), auto-refresh logic + frontend expiry badge in parallel (wave 2). All no new deps.
…torage - test_exchange_oauth_code_returns_tuple: assert 3-tuple (access, refresh, expires_in) - test_exchange_oauth_code_no_refresh_token: assert (tok, None, None) when fields absent - test_mcp_oauth_callback_stores_refresh_token: assert callback persists encrypted refresh + expiry - test_list_response_includes_token_expires_at: assert token_expires_at field in response schema
…n storage - Add 092_add_oauth_refresh_token.py: refresh_token_encrypted String(1024) nullable and token_expires_at DateTime(tz=True) nullable to workspace_mcp_servers - Add refresh_token_encrypted and token_expires_at Mapped fields to WorkspaceMcpServer - down_revision = "091_add_mcp_transport_type"
- _exchange_oauth_code returns tuple[str, str | None, int | None] | None - mcp_oauth_callback unpacks tuple; stores refresh_token_encrypted (Fernet) and token_expires_at (UTC + timedelta) when provider returns them - encrypt_api_key moved to module-level import for testability - WorkspaceMcpServerResponse schema gains token_expires_at: datetime | None - Fix existing callback tests to use tuple return + corrected patch target
- 32-01-SUMMARY.md: migration 092, ORM columns, router expansion, test stubs - STATE.md: advance to plan 01 complete, wave 2 ready (32-02, 32-03)
- test_expired_token_no_refresh_skips_server - test_expired_token_refresh_succeeds - test_expired_token_refresh_fails_skips All marked xfail(strict=False) until implementation lands.
- Add token_expires_at: string | null to MCPServer TS interface - Add ExpiryBadge component to mcp-server-card.tsx (expired/expires-in-Xh states) - Render ExpiryBadge after StatusBadge, guarded by auth_type === 'oauth2' - Update test fixtures in mcp-server-card.test.tsx and MCPServersStore.test.ts
- Add 32-03-SUMMARY.md with MCPO-03 requirement completion - Update STATE.md: advance to plan 03 complete, add decisions - Fix pyright ignore comment placement in workspace_mcp_servers.py (Rule 3)
…O-02) - Add datetime import and _OAUTH_EXPIRY_BUFFER (60s) to stream utils - Extend lazy import block to include McpAuthType - Insert MCPO-02 expiry check in _load_remote_mcp_servers: skip expired servers without refresh token, call _refresh_oauth_token when available - Handles naive datetime from SQLite tests via replace(tzinfo=UTC) guard - Log mcp_oauth_token_expired_no_refresh / mcp_oauth_refresh_failed_skip - Remove xfail markers from MCPO-02 tests (all three pass)
3 plans in 2 waves: DB schema + PATCH endpoint (wave 1), backend can_use_tool wiring + frontend admin toggle (wave 2 parallel).
…MCP_TOOL - Add 093_add_mcp_approval_mode.py: VARCHAR(16) NOT NULL server_default 'auto_approve' + CHECK constraint - Add McpApprovalMode StrEnum (AUTO_APPROVE/REQUIRE_APPROVAL) to workspace_mcp_server.py - Add approval_mode mapped_column to WorkspaceMcpServer ORM model - Add ActionType.REMOTE_MCP_TOOL to approval.py + DEFAULT_REQUIRE_ACTIONS set
- Add McpApprovalModeUpdate schema + approval_mode field to WorkspaceMcpServerResponse
- Add PATCH /{workspace_id}/mcp-servers/{server_id}/approval-mode endpoint (admin-only)
- Add test_update_approval_mode_success, _invalid, _unauthorized (3 new tests)
- Fix existing schema tests to set mock_server.approval_mode = "auto_approve"
- workspace_mcp_servers.py stays at exactly 700 lines
… endpoint, REMOTE_MCP_TOOL - Create 33-01-SUMMARY.md with full execution record - Update STATE.md: plan 01 complete, 7/11 plans done, 3 new decisions - Update ROADMAP.md: phase 33 in progress (1/3 summaries) - Mark MCPA-02 requirement complete in REQUIREMENTS.md
…tore action + API method
- MCPServer interface: approval_mode optional field ('auto_approve' | 'require_approval')
- MCPServersStore.updateApprovalMode: calls API and updates observable servers array
- mcpServersApi.updateApprovalMode: PATCH /workspaces/{id}/mcp-servers/{id}/approval-mode
- Test: updateApprovalMode calls API with correct args and updates observable
…CPA-01/04) - Extend create_can_use_tool_callback with workspace_id and remote_server_approval_map params - Add _handle_remote_mcp_approval: creates DB record, emits SSE event, blocks on wait_for_approval - mcp__key__tool with require_approval triggers full approval flow; auto_approve returns Allow immediately - Non-MCP tools and unknown server keys pass through unchanged (safe defaults) - Wire _build_stream_config: build remote_server_approval_map from ORM after _load_remote_mcp_servers - Add 5 unit tests: auto_approve, require+approved, require+rejected, non-MCP passthrough, no-map default
- Create 33-02-SUMMARY.md with task results and deviations - Update STATE.md: progress, decisions, session record - Update ROADMAP.md: phase 33 plan progress (2/3 summaries) - Mark REQUIREMENTS.md: MCPA-01, MCPA-04 complete
…CPA-02) - MCPServerCard: Switch toggle for 'Require approval for tool calls' - Switch checked when approval_mode='require_approval', unchecked for auto_approve - onUpdateApprovalMode prop (optional) triggers parent handler - mcp-servers-settings-page: handleUpdateApprovalMode wired to mcpStore.updateApprovalMode - Tests: Switch state (checked/unchecked), toggle calls onUpdateApprovalMode, page wiring
… store action, API method - 33-03-SUMMARY.md: full execution record, 24 tests, 7 files modified - STATE.md: progress 82%, decisions recorded, session updated - ROADMAP.md: phase 33 marked Complete (3/3 summaries) - REQUIREMENTS.md: MCPA-03 marked complete
…board tab 2 plans in 2 waves: - 34-01 (wave 1): TDD plan — extend AuditLogHook to detect mcp__remote_*__* tools, write ai.mcp_tool_call rows; new mcp_usage.py router (GET /ai/mcp-usage); migration 094 partial index; 14 unit tests (MCPOB-01 + MCPOB-02 backend) - 34-02 (wave 2): Add MCP Tools tab to CostDashboardPage with lazy useQuery + horizontal BarChart; getMcpToolUsage() in aiApi (MCPOB-02 frontend)
- 8 unit tests for AuditLogHook._create_audit_callback() remote MCP branch - Tests cover: audit row written, non-remote/plain tools excluded, non-fatal, underscore parsing, None duration, 64-char SHA-256 hash, UUID resource_id - RED phase: 5/8 fail (feature not yet implemented)
- 6 unit tests for aggregated MCP tool usage endpoint - Tests cover: empty result, invocation counts, multiple tools/servers, date filtering, auth requirement, sorted response - RED phase: all 6 fail (module not yet created)
- Add hashlib import at module level
- Extend _create_audit_callback() to detect mcp__remote_{uuid}__* tool names
- Write ai.mcp_tool_call audit rows with server_key, bare_tool, SHA-256
input hash, and duration_ms in payload JSONB
- Use elif guard so non-MCP tools still use the existing generic audit path
- Non-fatal: session_factory errors are logged and do not interrupt stream
- All 8 new tests GREEN, all 42 existing hooks tests still pass
- Create backend/src/pilot_space/api/v1/routers/mcp_usage.py with GET /ai/mcp-usage endpoint returning per-server+per-tool invocation counts - Register mcp_usage_router in ai.py (prefix /ai) and routers/__init__.py - Add migration 094_add_mcp_audit_index: partial index on audit_log (workspace_id, created_at) WHERE action='ai.mcp_tool_call' - Fix test_mcp_usage_requires_auth assertion for route method check - All 14 new tests GREEN; alembic heads: 094_add_mcp_audit_index (single)
…gration 094 - Create 34-01-SUMMARY.md (14 tests GREEN, all quality gates pass) - Update STATE.md: plan 10/13 complete, stopped_at 34-01, next 34-02 - Update ROADMAP.md: phase 34 in-progress (1/2 summaries) - Mark requirements MCPOB-01 and MCPOB-02 complete in REQUIREMENTS.md
- Add McpToolUsageEntry, McpServerSummary, McpToolUsageResponse types to ai.ts - Add getMcpToolUsage() method calling GET /ai/mcp-usage with X-Workspace-Id header - Add by_mcp tab to CostDashboardPage with lazy useQuery (enabled only when tab active) - Render horizontal BarChart of invocation counts (not dollar values) per tool - Empty state: "No MCP tool invocations for this period" with date range suggestion
- 34-02-SUMMARY.md: MCP Tools tab added to AI Cost Dashboard - STATE.md: advance to plan 34-02 complete, add decisions - ROADMAP.md: phase 34 marked Complete (2/2 summaries)
Two plans: backend data layer (migrations 095/096, ORM, repository, GET /mcp-catalog router, schema extensions) and frontend UI (API client, MCPCatalogStore, catalog card, Catalog tab in MCPServersSettingsPage).
…C-01/02/03/04) - Migration 095: mcp_catalog_entries table with Context7 + GitHub seeds - Migration 096: catalog_entry_id FK + installed_catalog_version on workspace_mcp_servers - McpCatalogEntry ORM model + repository - GET /api/v1/mcp-catalog endpoint - Schema extensions for catalog install tracking
…ions - Add McpCatalogEntry interface and mcpCatalogApi.list() for GET /mcp-catalog - Add MCPCatalogStore with loadCatalog(), isLoading, entries, error state - Export hasUpdate() and isInstalled() utility functions - Extend MCPServer with catalog_entry_id and installed_catalog_version fields - Extend MCPServerRegisterRequest with catalog_entry_id, installed_catalog_version, transport_type - Add MCPCatalogStore to AIStore as ai.mcpCatalog - All 12 MCPCatalogStore tests pass
…eeds - Create 35-01-SUMMARY.md with full execution results - Update STATE.md: progress 80%, decisions, session record - Update ROADMAP.md: phase 35 in-progress (1/2 plans complete) - Mark MCPC-01/02/03/04 requirements complete
- Add MCPCatalogCard (plain) with name, description, transport/auth badges, Official badge - Add Install button: disabled with "Installed" label when isInstalled=true - Add amber "Update Available" badge when hasUpdate=true - Add MCPCatalogTabContent (observer) with filter chips (All/HTTP/SSE) and card grid - Modify MCPServersSettingsPage to use shadcn Tabs (Registered Servers + Catalog) - Add handleInstallFromCatalog pre-filling registerServer from catalog entry fields - Success toast + switch to registered tab after successful catalog install - All 16 component tests pass
…s and one-click install - SUMMARY.md created for 35-02 (MCPCatalogStore + catalog card + tab content + tabs integration) - STATE.md updated: plan 13 complete, stopped at checkpoint:human-verify - ROADMAP.md Phase 35 marked Complete (2/2 plans with summaries)
…ntial-thinking) Extends the MCP server UI to support local stdio processes alongside existing remote SSE/HTTP servers. The form gains a transport type selector that conditionally shows either a URL field (remote) or command+args fields (stdio, e.g. npx -y @anthropic-ai/sequential-thinking). Cards display a "Stdio" badge and suppress the Authorize/Refresh Status buttons for stdio servers since they have no URL to probe. The catalog filter adds a "Stdio" chip, and the catalog install handler decodes the pipe-delimited url_template format used for stdio catalog entries. MCPServer and MCPServerRegisterRequest types, plus McpCatalogEntry, are updated to carry the new transport_type/stdio_command/stdio_args fields.
…ial-thinking) - Add McpTransportType.STDIO to enum; make url nullable for stdio servers - Add stdio_command and stdio_args columns to WorkspaceMcpServer model - Alembic migration 097: ALTER TYPE, add columns, make url nullable, seed sequential-thinking catalog entry - Update WorkspaceMcpServerCreate schema: url optional, stdio_command/stdio_args fields, model-level transport validation - Update WorkspaceMcpServerResponse to expose stdio_command, stdio_args, and optional url - Router passes stdio_command and stdio_args (JSON-encoded) when constructing WorkspaceMcpServer - _load_remote_mcp_servers: skip SSRF check for stdio, enforce ALLOWED_STDIO_COMMANDS allow-list, build McpServerConfig with command/args/env - Fix pre-existing test mock objects missing catalog_entry_id/installed_catalog_version (+ new stdio fields)
…king Sequential Thinking runs locally via npx with no API key needed. Added McpAuthType.NONE, updated migration 097 to add 'none' to mcp_auth_type enum, fixed seed to use auth_type='none', and updated frontend types to support the new auth type.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis PR implements v1.1.0 "MCP Platform Hardening" across the entire stack. It introduces transport-type flexibility (SSE/HTTP/stdio), OAuth refresh token support with expiry tracking, remote MCP tool approval workflows with database-backed requests, per-workspace server capacity enforcement, production encryption key validation, MCP tool call observability/analytics, and a globally seeded catalog with version tracking and one-click installation. Changes span planning documentation, database migrations (091–097), backend infrastructure refactoring, and new frontend UI for catalog browsing and approval configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend API
participant Database
participant OAuth Provider
rect rgba(100, 150, 200, 0.5)
note over User, OAuth Provider: OAuth Refresh Token Flow (MCPO-02)
User->>Frontend: Chat sends remote MCP tool invocation
Frontend->>Backend API: POST /chat with tool call
Backend API->>Database: Load workspace MCP servers
Database-->>Backend API: Server with expired token
Backend API->>OAuth Provider: POST /token (refresh_token grant)
OAuth Provider-->>Backend API: New access_token, refresh_token, expires_in
Backend API->>Database: Update server (encrypt & store new tokens, set expiry)
Database-->>Backend API: Updated
Backend API->>Backend API: Proceed with remote MCP invocation
Backend API-->>Frontend: Tool result
end
sequenceDiagram
participant User
participant Frontend
participant Backend API
participant Database
participant Approval Queue
participant WebSocket
rect rgba(150, 100, 200, 0.5)
note over User, WebSocket: Remote MCP Tool Approval Flow (MCPDD-003)
User->>Frontend: Request remote MCP tool call
Frontend->>Backend API: POST /chat with tool invocation
Backend API->>Database: Check server approval_mode
alt Server requires_approval
Backend API->>Database: Create approval request (ActionType.REMOTE_MCP_TOOL)
Backend API->>Approval Queue: Emit approval_request SSE event
Backend API->>WebSocket: Send event to user
User->>Frontend: User approves/denies inline
Frontend->>Backend API: POST /approve
Backend API->>Database: Update approval decision
Backend API->>Approval Queue: Continue tool execution
else auto_approve
Backend API->>Backend API: Proceed immediately
end
Backend API-->>Frontend: Tool result
end
sequenceDiagram
participant User
participant Frontend
participant Backend API
participant Database
participant MCP Catalog
rect rgba(200, 150, 100, 0.5)
note over User, MCP Catalog: Catalog Discovery & One-Click Install (MCPC-01)
User->>Frontend: Navigate to MCP Servers Settings
Frontend->>Backend API: GET /api/v1/mcp-catalog
Backend API->>Database: Query mcp_catalog_entries
Database-->>Backend API: [Context7, GitHub, Sequential Thinking...]
Backend API-->>Frontend: Catalog list (name, icon, transport, auth, is_official)
Frontend->>Frontend: Render catalog cards with filter chips
User->>Frontend: Filter by transport type (HTTP)
Frontend->>Frontend: Update visible entries
User->>Frontend: Click Install on GitHub entry
Frontend->>Backend API: POST /mcp-servers (url, auth_type, catalog_entry_id, installed_catalog_version)
Backend API->>Database: Store server + FK to catalog entry
Database-->>Backend API: Created server
Backend API-->>Frontend: Success response
Frontend->>Frontend: Switch to Registered Servers tab, show toast
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Review Summary by Qodofeat(mcp): v1.1.0 MCP Platform Hardening — stdio support, OAuth refresh, approval modes, catalog, observability
WalkthroughsDescription**Critical bug fix:** • Remote MCP tools were loaded but never added to allowed_tools — fixed with mcp__<key>__* wildcard patterns enabling SDK tool invocation. **Infrastructure hardening:** • Health-check gating — failed servers silently skipped at session load • DNS re-validation at connect time (SSRF TOCTOU fix) with new infrastructure/ssrf.py validation module • Per-workspace 10-server cap with HTTP 422 enforcement • Production ENCRYPTION_KEY enforcement — fail-fast startup with key generation guidance • Stdio transport support with security allow-list (npx, node, python3, python, uvx) **OAuth refresh flow:** • Refresh token storage (migration 092) with encrypted column and expiry tracking • Auto-refresh before session load with 60-second expiry buffer (MCPO-02) • Updated _exchange_oauth_code() to return 3-tuple (access_token, refresh_token, expires_in) **Remote MCP tool approval (DD-003):** • Per-server approval mode (auto_approve / require_approval) with PATCH endpoint • Extended can_use_tool callback with _handle_remote_mcp_approval() async flow • Approval mode map injection to pilotspace_agent for efficient permission checks **Observability:** • Audit trail for remote MCP tool invocations (ai.mcp_tool_call action) with SHA-256 input hashing • MCP tool usage analytics endpoint (GET /mcp-usage) with server/tool aggregation and date filtering • Partial index on audit_log(workspace_id, created_at) for dashboard query acceleration **MCP server catalog:** • Global catalog table (mcp_catalog_entries) with official seeds: Context7, GitHub, Sequential Thinking • Read-only catalog API endpoint with pagination and soft-delete filtering • Frontend MobX store with version drift detection (hasUpdate(), isInstalled()) • Catalog tracking columns (catalog_entry_id, installed_catalog_version) for update detection **Database & migrations:** • 7 new migrations (091–097): transport types, OAuth tokens, approval modes, audit index, catalog table, catalog FK, stdio support • New enums: McpTransportType (SSE/HTTP/STDIO), McpApprovalMode (AUTO_APPROVE/REQUIRE_APPROVAL), extended McpAuthType (added NONE) • Nullable url column for stdio servers; new columns: transport_type, stdio_command, stdio_args, refresh_token_encrypted, token_expires_at, approval_mode **Frontend:** • MCPCatalogStore MobX observable with load/error state management • MCPCatalogTabContent component with filter chips (All/HTTP/SSE/Stdio) • Extended MCPServersStore with updateApprovalMode() PATCH method • MCP usage analytics API client with date range filtering • Comprehensive test coverage for catalog store, approval flow, and component rendering **Test coverage:** • 620+ new lines in workspace MCP server tests (transport, cap, OAuth, approval) • 354-line catalog router endpoint tests • 268-line remote MCP tool audit logging tests • 244-line remote MCP approval flow tests • 233-line MCP usage analytics endpoint tests • 204-line MCP server loading hardening tests • 158-line production encryption key startup validation tests • Frontend catalog store and component test suites Diagramflowchart LR
A["Remote MCP Tools"] -->|"wildcard patterns"| B["allowed_tools"]
C["OAuth Provider"] -->|"refresh token"| D["Token Storage"]
D -->|"auto-refresh"| E["Session Load"]
F["MCP Server"] -->|"health check"| G["Skip Failed"]
H["URL"] -->|"DNS re-validate"| I["SSRF Block"]
J["Stdio Command"] -->|"allow-list"| K["Execute"]
L["Tool Invocation"] -->|"audit trail"| M["Dashboard"]
N["Catalog Entry"] -->|"install"| O["Workspace Server"]
P["Approval Mode"] -->|"per-server"| Q["User Decision"]
File Changes1. backend/tests/api/test_workspace_mcp_servers.py
|
Code Review by Qodo
1. Stdio request URL invalid
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+77
to
+85
| if (form.transportMode === 'stdio') { | ||
| data = { | ||
| display_name: form.displayName.trim(), | ||
| url: '', | ||
| auth_type: 'none', | ||
| transport_type: 'stdio', | ||
| stdio_command: form.stdioCommand.trim(), | ||
| stdio_args: form.stdioArgs.trim().split(/\s+/).filter(Boolean), | ||
| }; |
There was a problem hiding this comment.
1. Stdio request url invalid 🐞 Bug ✓ Correctness
The stdio registration path sends url: '', but the backend validates any non-None URL via SSRF/HTTPS checks, so stdio server registration will fail validation and block the feature.
Agent Prompt
### Issue description
Stdio MCP server registration currently sends `url: ''`, which is treated as a provided URL and fails backend SSRF/HTTPS validation. This blocks stdio server registration.
### Issue Context
Backend expects `url` omitted/None for `transport_type='stdio'`.
### Fix Focus Areas
- frontend/src/features/settings/components/mcp-server-form.tsx[74-85]
### Implementation notes
- In the stdio branch, remove the `url` field entirely (or set it to `undefined` / `null` depending on API client serialization).
- Ensure remote branch continues to send a non-empty `url`.
- Consider adding a small frontend test for stdio registration payload (no url field).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+719
to
+722
| import json as _json | ||
|
|
||
| _args: list[str] = _json.loads(server.stdio_args) if server.stdio_args else [] | ||
| _env: dict[str, str] = {} |
There was a problem hiding this comment.
2. Stdio args decode can crash 🐞 Bug ⛯ Reliability
_load_remote_mcp_servers does json.loads(server.stdio_args) without handling JSON decode errors, so one malformed row can raise and prevent agent session setup.
Agent Prompt
### Issue description
Agent session load can crash if `WorkspaceMcpServer.stdio_args` contains malformed JSON because `_load_remote_mcp_servers` calls `json.loads` without handling decode errors.
### Issue Context
This code runs on the agent’s session initialization path; it should skip bad servers rather than crashing the whole session.
### Fix Focus Areas
- backend/src/pilot_space/ai/agents/pilotspace_stream_utils.py[719-722]
### Implementation notes
- Wrap `json.loads` in try/except (JSONDecodeError, TypeError).
- On failure, log a warning with server_id/workspace_id and `continue` (silent skip consistent with other guard behavior).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+55
to
+108
| @router.get("", response_model=McpToolUsageResponse, summary="Get MCP tool usage") | ||
| async def get_mcp_tool_usage( | ||
| workspace_id: WorkspaceId, | ||
| current_user_id: CurrentUserId, | ||
| session: DbSession, | ||
| start_date: Annotated[date | None, Query(description="Period start date")] = None, | ||
| end_date: Annotated[date | None, Query(description="Period end date")] = None, | ||
| ) -> McpToolUsageResponse: | ||
| """Get per-server and per-tool MCP invocation counts for the workspace. | ||
|
|
||
| Queries the immutable audit_log table for rows where | ||
| action='ai.mcp_tool_call', aggregating by server_key and tool_name. | ||
|
|
||
| Default period: last 30 days. | ||
|
|
||
| Args: | ||
| workspace_id: Workspace UUID from request context header. | ||
| current_user_id: Current authenticated user ID. | ||
| session: Database session. | ||
| start_date: Optional period start date (inclusive). | ||
| end_date: Optional period end date (inclusive). | ||
|
|
||
| Returns: | ||
| Aggregated MCP tool usage with by_server and by_tool breakdowns. | ||
| """ | ||
| if not end_date: | ||
| end_date = datetime.now(UTC).date() | ||
| if not start_date: | ||
| start_date = end_date - timedelta(days=30) | ||
|
|
||
| start_dt = datetime(start_date.year, start_date.month, start_date.day, tzinfo=UTC) | ||
| end_dt = datetime(end_date.year, end_date.month, end_date.day, 23, 59, 59, tzinfo=UTC) | ||
|
|
||
| # GROUP BY server_key + tool_name using portable JSONB extraction. | ||
| # func.json_extract_path_text works with both PostgreSQL JSONB and SQLite JSON. | ||
| server_key_col = func.json_extract_path_text(AuditLog.payload, "server_key") | ||
| tool_name_col = func.json_extract_path_text(AuditLog.payload, "tool_name") | ||
|
|
||
| stmt = ( | ||
| select( | ||
| server_key_col.label("server_key"), | ||
| tool_name_col.label("tool_name"), | ||
| func.count(AuditLog.id).label("invocation_count"), | ||
| ) | ||
| .where( | ||
| (AuditLog.workspace_id == workspace_id) | ||
| & (AuditLog.action == "ai.mcp_tool_call") | ||
| & (AuditLog.created_at >= start_dt) | ||
| & (AuditLog.created_at <= end_dt) | ||
| ) | ||
| .group_by(server_key_col, tool_name_col) | ||
| .order_by(func.count(AuditLog.id).desc()) | ||
| ) | ||
| rows = (await session.execute(stmt)).all() |
There was a problem hiding this comment.
3. Mcp usage lacks rls setup 🐞 Bug ⛨ Security
The new /mcp-usage endpoint queries workspace-scoped tables using workspace_id from headers but never calls set_rls_context (required for RLS filtering) or verifies workspace access, risking incorrect results or cross-workspace exposure depending on DB/RLS configuration.
Agent Prompt
### Issue description
`GET /api/v1/mcp-usage` does not call `set_rls_context` and does not verify the requesting user’s access to the workspace identified by `X-Workspace-ID`. This can cause empty results (if RLS expects context) or data isolation issues (if the underlying tables aren’t protected as expected).
### Issue Context
Most workspace-scoped endpoints set RLS context early in the request.
### Fix Focus Areas
- backend/src/pilot_space/api/v1/routers/mcp_usage.py[55-108]
- backend/src/pilot_space/infrastructure/database/rls.py[18-27]
### Implementation notes
- Call `await set_rls_context(session, current_user_id, workspace_id)` at the start of the handler.
- Add an explicit workspace membership check (or admin/owner requirement) consistent with other workspace analytics endpoints, if applicable.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (18)
backend/src/pilot_space/main.py-141-160 (1)
141-160:⚠️ Potential issue | 🟡 MinorLog statement runs unconditionally despite validation being production-only.
Line 160 logs
"encryption_key_validated"outside theif settings.is_production:block. In non-production environments, this log entry will appear even though the encryption key validation was skipped, which could be misleading.🔧 Proposed fix
try: get_encryption_service() except EncryptionError as exc: raise RuntimeError( f"ENCRYPTION_KEY is invalid: {exc}. " 'Generate a valid key with: python -c "from cryptography.fernet import Fernet; ' 'print(Fernet.generate_key().decode())"' ) from exc - logger.info("encryption_key_validated") + logger.info("encryption_key_validated")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/pilot_space/main.py` around lines 141 - 160, The log "encryption_key_validated" is emitted unconditionally even when the production-only validation guarded by settings.is_production is skipped; update the code so logger.info("encryption_key_validated") is only called when the production validation runs and succeeds (i.e., move or wrap the logger.info inside the if settings.is_production: block after get_encryption_service() succeeds), keeping the existing exception handling for EncryptionError and referencing the same get_encryption_service and EncryptionError symbols..planning/ROADMAP.md-172-174 (1)
172-174:⚠️ Potential issue | 🟡 MinorProgress table rows have misaligned columns.
Rows 172–174 are missing the "Milestone" column and have inconsistent formatting compared to the table header and rows 169–171:
-| 33. Remote MCP Approval Framework | 3/3 | Complete | 2026-03-19 | - | -| 34. MCP Observability | 2/2 | Complete | 2026-03-19 | - | -| 35. MCP Server Catalog | 2/2 | Complete | 2026-03-19 | - | +| 33. Remote MCP Approval Framework | v1.1.0 | 3/3 | Complete | 2026-03-19 | +| 34. MCP Observability | v1.1.0 | 2/2 | Complete | 2026-03-19 | +| 35. MCP Server Catalog | v1.1.0 | 2/2 | Complete | 2026-03-19 |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/ROADMAP.md around lines 172 - 174, The three table rows for "33. Remote MCP Approval Framework", "34. MCP Observability", and "35. MCP Server Catalog" are missing the Milestone column and have inconsistent pipe spacing; update each row so it contains the same number of pipe-separated columns as the header/previous rows (i.e., include the Milestone cell between the item name and progress, and normalize spacing), for example by inserting the correct Milestone value in the rows shown ("| 33. Remote MCP Approval Framework | <Milestone> | 3/3 | Complete | 2026-03-19 | - |") and doing the same for entries 34 and 35 so formatting matches rows above.backend/tests/unit/ai/sdk/test_question_adapter.py-806-842 (1)
806-842:⚠️ Potential issue | 🟡 MinorAssert the rejected path still performs the approval handshake.
This test only checks the final deny message. It will still pass if the callback short-circuits directly to a
"rejected"denial without creating the approval row or enqueuing the SSE event, which is the main behavior added here. Mirror the interaction assertions from the approved-path test.Suggested test tightening
assert isinstance(result, PermissionResultDeny) + mock_create.assert_awaited_once() + mock_wait.assert_awaited_once_with(approval_id) + assert not queue.empty() assert "rejected" in result.message🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/ai/sdk/test_question_adapter.py` around lines 806 - 842, The test must assert that the approval handshake was performed before returning the denied result: after calling callback(f"mcp__remote_{server_id}__delete_file", ...), add assertions that ApprovalService.create_approval_request (mock_create) was awaited with the expected parameters (including the path and any actor/context), that wait_for_approval (mock_wait) was called with the returned approval_id, and that build_approval_sse_event (mock_sse) was invoked to enqueue the SSE; also verify the fake DB session (fake_session) was used where appropriate (e.g., had its add/commit or enqueue method called) so the test mirrors the approved-path interaction assertions rather than only checking the final PermissionResultDeny..planning/PROJECT.md-164-168 (1)
164-168:⚠️ Potential issue | 🟡 MinorUpdate the milestone status rows before merge.
This PR is the implementation of these items, but the table still leaves them as
— Pending, and the footer still says March 19, 2026. The planning doc will be stale as soon as this branch lands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/PROJECT.md around lines 164 - 168, The planning table still marks the implemented items as "— Pending" and the footer date remains "March 19, 2026"; update the three rows for "SSE + HTTP transport for remote MCP", "MCP catalog as installable registry", and "DD-003 approval extended to remote MCP tools" to their correct post-merge status (e.g., "Completed" or the appropriate milestone label) and revise the footer date to the actual merge/completion date to avoid leaving the planning doc stale. Ensure you edit the exact table cells containing those three row titles and change the footer line with the old date string.frontend/src/features/settings/components/mcp-catalog-card.tsx-72-74 (1)
72-74:⚠️ Potential issue | 🟡 MinorHandle
auth_type="none"explicitly.
McpCatalogEntry.auth_typeis a three-way union, but this ternary renders every non-bearer entry asOAuth2. No-auth catalog entries will be mislabeled in the card UI.💡 Suggested fix
<Badge variant="outline" className="text-xs"> - {entry.auth_type === 'bearer' ? 'Bearer' : 'OAuth2'} + {entry.auth_type === 'bearer' + ? 'Bearer' + : entry.auth_type === 'oauth2' + ? 'OAuth2' + : 'No Auth'} </Badge>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/settings/components/mcp-catalog-card.tsx` around lines 72 - 74, The Badge currently uses a two-way ternary that treats every non-'bearer' auth as OAuth2; update the rendering logic for entry.auth_type (the McpCatalogEntry union) to explicitly handle 'bearer', 'oauth2', and 'none' instead of a simple ternary: inside the mcp-catalog-card Badge rendering, replace the ternary with a conditional/switch that returns 'Bearer' for 'bearer', 'OAuth2' for 'oauth2', and a clear 'None' or 'No auth' label for 'none' so no-auth entries are not mislabeled.frontend/src/features/settings/pages/mcp-servers-settings-page.tsx-140-141 (1)
140-141:⚠️ Potential issue | 🟡 MinorMake the success toast auth-aware.
This copy is only correct for bearer installs. The seeded GitHub catalog entry is OAuth2, and
auth_type="none"is also valid, so users get the wrong next step after install.📝 Suggested fix
- toast.success('Server installed — add your auth token to activate it.'); + toast.success( + entry.auth_type === 'oauth2' + ? 'Server installed — authorize the connection to activate it.' + : entry.auth_type === 'bearer' + ? 'Server installed — add your auth token to activate it.' + : 'Server installed.' + );.planning/REQUIREMENTS.md-12-23 (1)
12-23:⚠️ Potential issue | 🟡 MinorVerify requirement completion status matches PR scope.
Several requirements are marked as pending (
[ ]):
- MCPI-01 (Line 12): Remote MCP tools allowed_tools patterns
- MCPO-01 (Line 21): OAuth refresh token storage
- MCPO-03 (Line 23): Token expiry tracking
However, the PR objectives indicate these are addressed in this PR ("OAuth refresh: migration 092 to store refresh tokens and expiry", "Token expiry badge in server settings UI"). Consider updating the checkboxes if these are now complete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/REQUIREMENTS.md around lines 12 - 23, Update the status checkboxes in .planning/REQUIREMENTS.md to reflect the actual changes in this PR: mark MCPI-01 as done if remote MCP tools are now added to allowed_tools with wildcard patterns, mark MCPO-01 as done because migration 092 adds the refresh_token column and storage, and mark MCPO-03 as done if token expiry (expires_at) is tracked and surfaced in the server settings UI badge; reference the requirement IDs (MCPI-01, MCPO-01, MCPO-03), the migration number (092), and the UI badge change when making the edits so reviewers can trace the updates to PR changes..planning/phases/32/32-03-SUMMARY.md-57-59 (1)
57-59:⚠️ Potential issue | 🟡 MinorTimestamp inconsistency: Completed time is before Started time.
Line 58 shows
Started: 2026-03-20T02:20:00Zbut Line 59 showsCompleted: 2026-03-19T19:23:02Z. The completion timestamp is approximately 7 hours before the start time and on a different date.📝 Suggested fix
- **Started:** 2026-03-20T02:20:00Z -- **Completed:** 2026-03-19T19:23:02Z +- **Completed:** 2026-03-20T02:28:00Z🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/phases/32/32-03-SUMMARY.md around lines 57 - 59, The "Started" and "Completed" timestamps are inconsistent (Completed is before Started); update the Completed value so it is after Started and matches the declared Duration, or adjust Started instead—specifically fix the "Started:" and "Completed:" fields so that Completed is 8 minutes after Started (e.g., set Completed to 2026-03-20T02:28:00Z if keeping Started at 2026-03-20T02:20:00Z) and ensure the "Duration:" reflects the corrected interval..planning/phases/32/32-03-PLAN.md-120-120 (1)
120-120:⚠️ Potential issue | 🟡 MinorFix markdown code block lint violations (MD046/MD040).
Convert the indented code examples to fenced blocks and ensure every fenced block declares a language tag so markdownlint passes consistently.
Also applies to: 146-146, 212-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/phases/32/32-03-PLAN.md at line 120, Convert all indented code examples to fenced code blocks and ensure every fenced block declares a language tag (e.g., replace indented blocks with ```python and close with ```), and update any existing fences that lack a language identifier to include the proper language to satisfy MD046/MD040; look for occurrences of triple-backtick markers (``` and ```python) in the file and update them accordingly, including the other occurrences noted in the comment..planning/STATE.md-64-64 (1)
64-64:⚠️ Potential issue | 🟡 MinorMinor grammar fix: "requires quoted" → "requires a quoted".
The static analysis tool flagged this. Consider updating for readability.
📝 Proposed fix
-- [31-02]: cast("McpServerConfig", {...}) used for TypedDict union assignment — ruff TC006 requires quoted type arg +- [31-02]: cast("McpServerConfig", {...}) used for TypedDict union assignment — ruff TC006 requires a quoted type arg🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/STATE.md at line 64, Update the grammar in the STATE.md entry describing ruff TC006 by changing "requires quoted" to "requires a quoted" so the sentence reads that ruff TC006 "requires a quoted type arg" (referencing the cast("McpServerConfig", {...}) note and ruff TC006) to improve readability.frontend/src/features/settings/components/mcp-server-form.tsx-77-85 (1)
77-85:⚠️ Potential issue | 🟡 MinorConsider sending
url: undefinedinstead of empty string for stdio servers.The backend
WorkspaceMcpServerCreateschema expectsurlto be nullable for stdio transport. Sending an empty string''may fail validation or cause issues. From the relevant snippet inMCPServersStore.ts,urlis typed asstring | undefined.♻️ Proposed fix
if (form.transportMode === 'stdio') { data = { display_name: form.displayName.trim(), - url: '', + url: undefined, auth_type: 'none', transport_type: 'stdio', stdio_command: form.stdioCommand.trim(), stdio_args: form.stdioArgs.trim().split(/\s+/).filter(Boolean), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/settings/components/mcp-server-form.tsx` around lines 77 - 85, When building the payload for stdio servers in the MCP server form (inside the block checking form.transportMode === 'stdio' where you set data), set url to undefined instead of an empty string so it matches the WorkspaceMcpServerCreate/MCPServersStore typing (string | undefined); update the data object creation that currently sets url: '' to url: undefined and ensure any downstream serialization will omit or send null/undefined as the backend expects.backend/tests/unit/api/test_mcp_catalog_router.py-21-42 (1)
21-42:⚠️ Potential issue | 🟡 MinorDuplicate
MagicMockimport inside_create_test_app.
MagicMockis already imported at module level (line 10), so the inner import on line 23 is redundant.♻️ Proposed fix
def _create_test_app(): """Create a minimal FastAPI app with mcp_catalog router and dependency overrides.""" - from unittest.mock import MagicMock - from fastapi import FastAPI🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/api/test_mcp_catalog_router.py` around lines 21 - 42, The inner import of MagicMock inside the _create_test_app function is redundant because MagicMock is already imported at the module level; remove the local "from unittest.mock import MagicMock" line and keep using the module-level MagicMock when constructing mock_user in _create_test_app so only one import exists and the function continues to work unchanged.backend/tests/unit/routers/test_mcp_usage.py-51-56 (1)
51-56:⚠️ Potential issue | 🟡 MinorUnused parameter
server_idin helper function.The
server_idparameter is passed but never used—row.idis always set toSERVER_UUIDconstant. Either use the parameter or remove it.♻️ Proposed fix
-def _make_server_row(server_id: str, display_name: str) -> MagicMock: +def _make_server_row(server_id: str, display_name: str) -> MagicMock: """Build a mock workspace_mcp_servers query result row.""" row = MagicMock() - row.id = SERVER_UUID + row.id = server_id if isinstance(server_id, type(SERVER_UUID)) else SERVER_UUID row.display_name = display_name return rowOr simply remove the parameter if always using the constant:
-def _make_server_row(server_id: str, display_name: str) -> MagicMock: +def _make_server_row(display_name: str) -> MagicMock: """Build a mock workspace_mcp_servers query result row.""" row = MagicMock() row.id = SERVER_UUID row.display_name = display_name return row🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/routers/test_mcp_usage.py` around lines 51 - 56, The helper _make_server_row has an unused parameter server_id while row.id is always assigned SERVER_UUID; either remove the server_id parameter from _make_server_row signature and all its call sites, or change the assignment to use the parameter (set row.id = server_id) so the passed value is honored; update any tests that call _make_server_row accordingly and ensure SERVER_UUID remains available where needed.backend/src/pilot_space/ai/agents/pilotspace_stream_utils.py-719-721 (1)
719-721:⚠️ Potential issue | 🟡 MinorUnhandled
JSONDecodeErrorcould break server loading loop.If
server.stdio_argscontains invalid JSON,json.loads()will raiseJSONDecodeError, which is not caught. This would propagate and potentially break the entire remote server loading for the workspace. Apply the same defensive pattern used for token decryption.🛡️ Proposed fix
import json as _json - _args: list[str] = _json.loads(server.stdio_args) if server.stdio_args else [] + try: + _args: list[str] = _json.loads(server.stdio_args) if server.stdio_args else [] + except _json.JSONDecodeError: + logger.warning( + "mcp_stdio_args_invalid_json", + server_id=str(server.id), + workspace_id=str(workspace_id), + ) + continue _env: dict[str, str] = {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/pilot_space/ai/agents/pilotspace_stream_utils.py` around lines 719 - 721, The JSON decode of server.stdio_args is unprotected: wrap the _json.loads(server.stdio_args) used to set _args in a try/except that catches json.JSONDecodeError (and optionally Exception), defaulting _args to an empty list on failure and emitting a warning/info log with the server identifier and the raw stdio_args; mirror the defensive pattern used in the token decryption code path so failures don’t propagate and break the remote server loading loop (refer to server.stdio_args, _args, and _json.loads in pilotspace_stream_utils.py).backend/src/pilot_space/api/v1/routers/mcp_usage.py-88-91 (1)
88-91:⚠️ Potential issue | 🟡 MinorMisleading comment:
json_extract_path_textis PostgreSQL-specific.The comment states this works with "both PostgreSQL JSONB and SQLite JSON", but
json_extract_path_textis a PostgreSQL-specific function. SQLite usesjson_extract()with different syntax. If SQLite compatibility is needed for testing, consider using SQLAlchemy's dialect-aware JSON path operators or update the comment to clarify this is PostgreSQL-only.Suggested comment fix
- # GROUP BY server_key + tool_name using portable JSONB extraction. - # func.json_extract_path_text works with both PostgreSQL JSONB and SQLite JSON. + # GROUP BY server_key + tool_name using PostgreSQL JSONB path extraction. + # Note: This is PostgreSQL-specific; tests should use PostgreSQL or mock this layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/pilot_space/api/v1/routers/mcp_usage.py` around lines 88 - 91, The comment above the JSON extraction is misleading: func.json_extract_path_text (used to build server_key_col and tool_name_col from AuditLog.payload) is PostgreSQL-specific; update the comment to state this is PostgreSQL/JSONB only and not SQLite, or replace the implementation with a dialect-aware approach (e.g., use SQLAlchemy’s dialect-specific JSON operators or conditional logic that uses func.json_extract for SQLite and func.json_extract_path_text for PostgreSQL) so the code and comment accurately reflect supported databases..planning/phases/34/34-02-PLAN.md-146-158 (1)
146-158:⚠️ Potential issue | 🟡 MinorUse fenced code blocks instead of indented blocks in these instruction snippets.
Lines 146-158 and 176-179 use indented code block style, which triggers MD046. Convert them to fenced blocks (for example,
```typescript) to keep markdownlint clean.Also applies to: 176-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/phases/34/34-02-PLAN.md around lines 146 - 158, Replace the indented Markdown code blocks at the two spots where you added the new interfaces and the getMcpToolUsage snippet with fenced code blocks (use ```typescript ... ```), i.e. wrap the three new exported interfaces (McpToolUsageEntry, McpServerSummary, McpToolUsageResponse) in a ```typescript fenced block and likewise wrap the aiApi addition (getMcpToolUsage) in a ```typescript fenced block so MD046 no longer triggers; refer to the newly added interface names and the aiApi.getMcpToolUsage method to locate the exact snippets to update..planning/phases/31/31-02-SUMMARY.md-65-70 (1)
65-70:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced verification block.
Line 65 starts a fenced code block without a language, which triggers MD040. Use
```text(or```bashif you want shell semantics).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/phases/31/31-02-SUMMARY.md around lines 65 - 70, The fenced code block showing test output is missing a language tag (triggers MD040); update the opening fence from ``` to a tagged fence such as ```text (or ```bash for shell semantics) so the block becomes a language-identified fenced code block containing the pyright/ruff/pytest output..planning/phases/32/32-01-SUMMARY.md-74-79 (1)
74-79:⚠️ Potential issue | 🟡 MinorAdd a language to this fenced block.
Line 74 opens a fenced code block without a language specifier (MD040). Use
```text(or a more specific language if appropriate).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/phases/32/32-01-SUMMARY.md around lines 74 - 79, The fenced code block that begins with ``` and contains the test summary lines (e.g. "11 passed, 6 xfailed, 4 xpassed, 7 warnings") should include a language specifier to satisfy MD040; update the opening fence from ``` to ```text (or another appropriate language) so the block reads ```text and leave the rest of the block unchanged.
🧹 Nitpick comments (22)
frontend/src/stores/ai/index.ts (1)
20-21: Minor pattern inconsistency in type re-exports.The
McpCatalogEntryandMcpCatalogListResponsetypes are re-exported directly from@/services/api/mcp-catalog, whereas other store-related types (e.g.,MCPServer,MCPServerStatuson lines 14-19) are re-exported from their respective store files. For consistency, consider re-exporting these types throughMCPCatalogStore.tsfirst.This is a minor organizational nit and doesn't affect functionality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/stores/ai/index.ts` around lines 20 - 21, Re-export the types McpCatalogEntry and McpCatalogListResponse through the MCPCatalogStore module instead of directly from the API module for consistency with other store-related types; update the barrel index to export these types from MCPCatalogStore (so keep the export names McpCatalogEntry and McpCatalogListResponse but import/forward them from MCPCatalogStore) and ensure MCPCatalogStore exports those type definitions (add type re-exports in MCPCatalogStore if missing).backend/tests/ai/agents/test_remote_mcp_loading.py (1)
245-249: Assert that_refresh_oauth_token()was actually awaited.These cases only check the final inclusion/exclusion. The success path still passes if expired servers are loaded without ever calling the refresh helper, so the behavior under test is not fully pinned down.
🧪 Tighten the test contract
- with patch( + refresh_mock = AsyncMock(return_value=True) + with patch( "pilot_space.api.v1.routers.workspace_mcp_servers._refresh_oauth_token", - new=AsyncMock(return_value=True), + new=refresh_mock, ): result = await _load_remote_mcp_servers(workspace.id, db_session) + refresh_mock.assert_awaited_once()Mirror the same pattern in the failure case and assert the mock was awaited there too.
Also applies to: 294-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/ai/agents/test_remote_mcp_loading.py` around lines 245 - 249, The test patches "pilot_space.api.v1.routers.workspace_mcp_servers._refresh_oauth_token" with an AsyncMock but never asserts it was awaited; update the two test cases that call _load_remote_mcp_servers (the success and the failure paths) to capture the AsyncMock instance from the patch and call its assertion (e.g., mock.assert_awaited_once() or mock.assert_awaited()) after awaiting _load_remote_mcp_servers to ensure _refresh_oauth_token was actually awaited; reference the existing AsyncMock patch of _refresh_oauth_token and the _load_remote_mcp_servers invocation when adding the assertions.frontend/src/features/settings/components/__tests__/mcp-catalog-card.test.tsx (1)
66-77: Add coverage for the otherauth_typelabels.
McpCatalogEntry.auth_typeis a three-way union, but this suite only exercises the bearer branch. Explicitnoneandoauth2cases would protect the badge mapping from regressing again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/settings/components/__tests__/mcp-catalog-card.test.tsx` around lines 66 - 77, Add two more unit tests in this file that mirror the existing auth_type test: render MCPCatalogCard with entry created by makeCatalogEntry({ auth_type: 'none' }) and assert the expected badge text for the "none" case, and render with makeCatalogEntry({ auth_type: 'oauth2' }) and assert the expected badge text for the "oauth2" case; use the same render pattern and screen.getByText assertions as in the existing test so McpCatalogEntry.auth_type's three-way mapping (handled by MCPCatalogCard) is covered and protected from regressions.frontend/src/features/settings/components/__tests__/mcp-catalog-tab-content.test.tsx (1)
1-12: Consider adding test for loading skeleton state.The behavioral contract (line 10) mentions "Shows loading skeleton when catalogStore.isLoading=true" but there's no corresponding test verifying this behavior.
💡 Suggested test for loading state
it('shows loading skeleton when catalogStore.isLoading is true', () => { const catalogStore = makeMockCatalogStore({ isLoading: true }); vi.mocked(useStore).mockReturnValue({ ai: { mcpCatalog: catalogStore }, } as unknown as ReturnType<typeof useStore>); render( <MCPCatalogTabContent workspaceId="ws-1" installedServers={emptyInstalledServers} onInstall={mockOnInstall} /> ); // Verify skeleton is rendered (adjust selector based on actual component) expect(screen.getByRole('status')).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/settings/components/__tests__/mcp-catalog-tab-content.test.tsx` around lines 1 - 12, Add a unit test in MCPCatalogTabContent tests that verifies the loading skeleton is shown when catalogStore.isLoading=true: create a mock store with makeMockCatalogStore({ isLoading: true }), mock useStore to return { ai: { mcpCatalog: catalogStore } }, render MCPCatalogTabContent (passing workspaceId, installedServers, onInstall), and assert the loading skeleton is present (e.g., expect(screen.getByRole('status')).toBeInTheDocument() or use the actual skeleton selector used by the component).frontend/src/features/settings/components/mcp-catalog-tab-content.tsx (1)
115-122: Avoid duplicateisInstalled(...)lookups per catalog card.
isInstalled(entry, installedServers)is executed twice for every row. Compute once and reuse for clearer intent and lower per-render work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/settings/components/mcp-catalog-tab-content.tsx` around lines 115 - 122, Compute and reuse the result of isInstalled(entry, installedServers) instead of calling it twice: create a local boolean (e.g., installed) by calling isInstalled(entry, installedServers) once, then pass installed to isInstalled prop and use it in the hasUpdate expression (hasUpdate = installed ? installedServers.some(s => s.catalog_entry_id === entry.id && hasUpdate(entry, s)) : false); update references to entry, installedServers, isInstalled, and hasUpdate accordingly so the per-row work is done once.backend/src/pilot_space/api/v1/routers/mcp_catalog.py (1)
34-35: Narrowtransport_type/auth_typeto enum orLiteraltypes in response schema.Using
strhere weakens API contract guarantees for clients. Prefer enum/Literal-backed fields to keep OpenAPI and frontend typing aligned with the finite domain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/pilot_space/api/v1/routers/mcp_catalog.py` around lines 34 - 35, The response schema currently types transport_type and auth_type as plain str (symbols: transport_type, auth_type in the response model within mcp_catalog.py); narrow these to explicit enums or typing.Literal unions representing the finite allowed values (or a Pydantic Enum) so OpenAPI and generated frontend types are accurate; update the response dataclass/Pydantic model (the model/class that declares transport_type and auth_type) to use the new Enum/Literal types, adjust any import and serialization usage to ensure FastAPI/OpenAPI emits the enum values, and run tests/schema generation to verify the updated contract.backend/src/pilot_space/api/v1/routers/_mcp_server_oauth_helpers.py (2)
19-22: Consider typingrepoparameter more specifically.Using
Anyloses type safety. IfWorkspaceMcpServerRepositoryis available, use it for better IDE support and type checking.♻️ Proposed improvement
+from pilot_space.infrastructure.database.repositories.workspace_mcp_server_repository import ( + WorkspaceMcpServerRepository, +) + async def _refresh_oauth_token( server: WorkspaceMcpServer, - repo: Any, + repo: WorkspaceMcpServerRepository, ) -> bool:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/pilot_space/api/v1/routers/_mcp_server_oauth_helpers.py` around lines 19 - 22, The parameter repo in _refresh_oauth_token is currently typed as Any; replace it with the concrete repository type (e.g., WorkspaceMcpServerRepository) to restore type safety and improve IDE support—update the function signature of _refresh_oauth_token to accept WorkspaceMcpServerRepository, add the necessary import for WorkspaceMcpServerRepository at the top of the module, and adjust any call sites that pass a repo to ensure they provide that repository type (or a compatible interface) so type checks pass.
46-54: Consider supportingclient_secretfor OAuth refresh token requests.OAuth providers that require confidential client authentication need both
client_idandclient_secretin refresh token requests. The model currently stores onlyoauth_client_id; to support this, you'd need to:
- Add an
oauth_client_secretfield toWorkspaceMcpServer(with Fernet encryption at rest, likeauth_token_encrypted)- Update schemas to accept it during server registration
- Include it in the refresh token request
This ensures compatibility with OAuth providers using the confidential client profile.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/pilot_space/api/v1/routers/_mcp_server_oauth_helpers.py` around lines 46 - 54, The refresh token POST to server.oauth_token_url only includes client_id, but some OAuth providers require confidential client authentication; add an oauth_client_secret field to the WorkspaceMcpServer model (encrypted at rest like auth_token_encrypted), update the server registration/validation schemas to accept oauth_client_secret, and modify the refresh logic that calls client.post (the data payload built in the function referencing server.oauth_client_id) to include server.oauth_client_secret when present so confidential clients can authenticate during refresh_token requests.backend/tests/unit/routers/test_mcp_usage.py (1)
185-200: Test name is misleading—it verifies router registration, not authentication.The test
test_get_mcp_usage_requires_authonly checks that a GET route exists on the router, not that unauthenticated requests are rejected. Consider renaming totest_get_mcp_usage_router_registeredor adding an actual auth test via HTTP client.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/unit/routers/test_mcp_usage.py` around lines 185 - 200, The test named test_get_mcp_usage_requires_auth is misleading because it only imports pilot_space.api.v1.routers.mcp_usage and asserts router.routes contains a GET; rename the test to test_get_mcp_usage_router_registered (or similar) to reflect that it verifies route registration, or instead implement a real auth test: use the test client to call the GET endpoint on router (resolve path from router.routes) with no workspace header and assert 401/422, using the existing router import and test client fixture. Ensure references to test_get_mcp_usage_requires_auth and router in the file are updated accordingly.frontend/src/features/settings/components/mcp-server-form.tsx (1)
84-84: Arguments split on whitespace doesn't handle quoted strings.Splitting on
/\s+/will break arguments containing spaces (e.g.,--message "hello world"becomes["--message", "\"hello", "world\""]). For MVP this may be acceptable, but consider documenting the limitation or using a shell-like parser.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/settings/components/mcp-server-form.tsx` at line 84, The current split of form.stdioArgs using form.stdioArgs.trim().split(/\s+/).filter(Boolean) will break quoted arguments; replace this naive split with a proper shell-like parser (e.g., use a utility such as shell-quote or parseArgsStringToArgv) when constructing stdio_args so quoted strings remain a single argument, or alternatively add a clear comment/validation in the mcp-server-form.tsx UI explaining the limitation; update the code that sets stdio_args to call the chosen parser on form.stdioArgs instead of .split(/\s+/).backend/src/pilot_space/infrastructure/database/models/mcp_catalog_entry.py (1)
59-68: Minor: Doc string doesn't mentionstdiotransport type.The doc attribute says
"MCP transport protocol: 'sse' or 'http'"butMcpTransportTypealso includesSTDIO. While catalog entries might only support remote transports, the doc should be accurate or explicitly state that catalog entries only support SSE/HTTP.📝 Suggested doc fix
nullable=False, - doc="MCP transport protocol: 'sse' or 'http'", + doc="MCP transport protocol: 'sse', 'http', or 'stdio'", )Or if only remote transports are valid for catalog:
nullable=False, - doc="MCP transport protocol: 'sse' or 'http'", + doc="MCP transport protocol for remote servers: 'sse' or 'http' (stdio not applicable for catalog entries)", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/pilot_space/infrastructure/database/models/mcp_catalog_entry.py` around lines 59 - 68, Doc string for the mapped column transport_type incorrectly lists only 'sse' and 'http' even though McpTransportType includes 'stdio'; update the doc on the transport_type mapped_column (symbol: transport_type in mcp_catalog_entry.py) to either include 'stdio' or explicitly note that catalog entries only support remote transports (SSE/HTTP) so the doc matches the enum semantics; change the doc string on the mapped_column declaration for transport_type accordingly.backend/tests/api/test_workspace_mcp_servers.py (4)
686-708: Missingtoken_expires_atattribute on mock_server.The
WorkspaceMcpServerResponseschema includestoken_expires_at: datetime | None = None, but the mock server object doesn't set this attribute. WhileMagicMockwill auto-create it when accessed, explicitly setting it improves test clarity and consistency with other tests in this file (e.g.,test_list_response_includes_token_expires_atat line 1072).🔧 Suggested fix
mock_server.approval_mode = "auto_approve" mock_server.catalog_entry_id = None mock_server.installed_catalog_version = None mock_server.stdio_command = None mock_server.stdio_args = None + mock_server.token_expires_at = None resp = WorkspaceMcpServerResponse.model_validate(mock_server)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/api/test_workspace_mcp_servers.py` around lines 686 - 708, Add the missing token_expires_at attribute to the mock_server used in the WorkspaceMcpServerResponse test: set mock_server.token_expires_at = None (or a concrete datetime if preferred) so the mock matches the schema field in WorkspaceMcpServerResponse and aligns with other tests like test_list_response_includes_token_expires_at; update the block where mock_server is constructed (the MagicMock with id, workspace_id, display_name, etc.) to include token_expires_at.
1106-1128: Same missingtoken_expires_atattribute issue.For consistency, add
mock_server.token_expires_at = Noneto this mock setup as noted in the earlier review comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/api/test_workspace_mcp_servers.py` around lines 1106 - 1128, The mock_server in the test is missing the token_expires_at attribute which causes schema validation inconsistency; update the mock setup for mock_server (used with WorkspaceMcpServerResponse.model_validate) to include mock_server.token_expires_at = None so the mocked object matches the expected response shape before asserting resp.transport_type == McpTransportType.SSE.
716-772: Unit tests validate constants and message format, not endpoint behavior.These tests verify the
MCP_SERVER_CAPconstant value and manually simulate the guard logic, which is useful for contract validation. Consider adding an integration test that actually callsPOST /mcp-serverswith 10 existing servers to verify the HTTP 422 response end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/api/test_workspace_mcp_servers.py` around lines 716 - 772, Current unit tests only assert the MCP_SERVER_CAP constant and simulate the guard logic; add an integration test that actually exercises the router by issuing a POST to the POST /mcp-servers endpoint with 10 existing MCP servers and asserting FastAPI returns HTTP 422. Create a new test (e.g., test_post_mcp_server_at_cap_returns_422) that uses the app/test client or existing fixtures to prepopulate 10 MCP servers, calls POST /mcp-servers, and asserts response.status_code == 422 and that the response.json()["detail"] contains str(MCP_SERVER_CAP), "maximum", and "Delete an existing server" so the end-to-end behavior of the guard and message formatting is validated.
1180-1181: Dead code:set_approvalfunction is defined but never used.This function appears to be leftover from refactoring. The test works because the endpoint directly mutates
mock_server.approval_mode. Remove the unused function.🧹 Remove dead code
mock_repo = AsyncMock() mock_repo.get_by_workspace_and_id = AsyncMock(return_value=mock_server) mock_repo.update = AsyncMock() - def set_approval(v: str) -> None: - mock_server.approval_mode = v - mock_current_user = MagicMock()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/api/test_workspace_mcp_servers.py` around lines 1180 - 1181, Remove the dead helper function set_approval from the test file; locate the function definition "def set_approval(v: str) -> None: mock_server.approval_mode = v" and delete it, since tests mutate mock_server.approval_mode directly and nothing calls set_approval; run the tests to confirm no references remain and commit the removal.backend/src/pilot_space/ai/agents/pilotspace_stream_utils.py (1)
762-764: Cross-layer import: AI layer imports from API router layer.Importing
_refresh_oauth_tokenfrompilot_space.api.v1.routers.workspace_mcp_serversinto the AI agents layer inverts the typical dependency direction (API depends on domain/infrastructure, not vice versa). This could lead to circular import issues as the codebase grows.Consider extracting
_refresh_oauth_tokento a shared module likepilot_space.ai.infrastructure.oauth_helpersorpilot_space.infrastructure.oauththat both layers can import from.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/pilot_space/ai/agents/pilotspace_stream_utils.py` around lines 762 - 764, The AI layer currently imports the private router function _refresh_oauth_token from pilot_space.api.v1.routers.workspace_mcp_servers causing a cross-layer dependency; extract _refresh_oauth_token into a shared infrastructure module (e.g., pilot_space.infrastructure.oauth or pilot_space.ai.infrastructure.oauth_helpers), move its implementation there, update import usages in pilotspace_stream_utils.py and the router to import the new shared function, and ensure the new module only depends on lower-level infra (HTTP/token storage) to avoid circular imports and preserve the original function name and behavior.backend/src/pilot_space/ai/sdk/question_adapter.py (1)
581-588: Potential key collision:tool_inputcould overwrite metadata keys.Spreading
**tool_inputafter settingtool_name,server, andserver_idmeans user-controlled input cannot overwrite them. However, if the order is changed or additional metadata keys are added, this could become an issue. Consider using a nested structure for clarity.🔧 More explicit structure
- action_data: dict[str, Any] = { - "tool_name": bare_tool, - "server": display_name, - "server_id": str(server_id), - **tool_input, - } + action_data: dict[str, Any] = { + "tool_name": bare_tool, + "server": display_name, + "server_id": str(server_id), + "input": tool_input, + }This keeps tool_input separate and avoids any collision risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/pilot_space/ai/sdk/question_adapter.py` around lines 581 - 588, The action_data dict currently merges metadata and user-controlled tool_input (variables: reason, action_data, bare_tool, display_name, server_id, tool_input) which risks key collisions; change action_data to keep metadata keys first and put the user payload under a nested key (e.g., "tool_input" or "input") so metadata like "tool_name", "server", "server_id" cannot be overwritten—update the dict construction in the block that builds action_data to something like {"tool_name": bare_tool, "server": display_name, "server_id": str(server_id), "tool_input": tool_input} and adjust any downstream code that reads action_data to reference the nested key..planning/phases/33/33-01-SUMMARY.md (1)
124-127: Add language identifier to fenced code block.The code block showing test results should have a language identifier for proper rendering.
Suggested fix
## Test Results -``` +```text 14 passed, 6 xfailed, 4 xpassed, 8 warnings ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/phases/33/33-01-SUMMARY.md around lines 124 - 127, The fenced code block containing the test results line "14 passed, 6 xfailed, 4 xpassed, 8 warnings" needs a language identifier for proper rendering; replace the opening fence ``` with ```text (so the block becomes ```text ... ```), ensuring the single-line test results remain unchanged.backend/src/pilot_space/api/v1/routers/workspace_mcp_servers.py (2)
699-699: Reconsider exporting private helper_refresh_oauth_tokenin__all__.The underscore prefix conventionally indicates internal/private usage. If this function is intended for external use (e.g., by
pilotspace_stream_utils.pyfor auto-refresh), consider renaming it without the underscore prefix to clarify its public API status.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/pilot_space/api/v1/routers/workspace_mcp_servers.py` at line 699, The symbol `_refresh_oauth_token` is exported in __all__ but its leading underscore implies it should be private; either remove `_refresh_oauth_token` from the __all__ list to keep it private, or make it an explicit public API by renaming the function to `refresh_oauth_token` and updating all imports (for example in pilotspace_stream_utils.py) and the __all__ entry accordingly; ensure you also update any references to the old name (e.g., call sites and tests) so nothing breaks.
144-144: Moveimport jsonto module-level imports.The
import json as _jsonis placed inside the function body. While this works, it's unconventional and adds minor overhead on each call. Consider moving it to the module-level imports for consistency with the rest of the codebase.Suggested refactor
from __future__ import annotations +import json import secrets import urllib.parseThen replace
_json.dumpswithjson.dumpsat line 177.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/pilot_space/api/v1/routers/workspace_mcp_servers.py` at line 144, The local import "import json as _json" should be moved to the module-level imports to avoid re-importing on each call; remove the in-function import, add "import json" at the top of the module, and update all usages (e.g., replace _json.dumps) to use json.dumps (check occurrences in workspace_mcp_servers.py such as the dump call currently using _json.dumps).backend/src/pilot_space/api/v1/routers/mcp_usage.py (1)
55-62: Consider whethercurrent_user_idis needed.The
current_user_iddependency is declared but not used in the function body. If it's solely for authentication gating, the presence ofWorkspaceId(which typically requires auth) may be sufficient. Otherwise, consider adding an audit log entry or removing the unused parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/pilot_space/api/v1/routers/mcp_usage.py` around lines 55 - 62, The handler get_mcp_tool_usage declares a dependency parameter current_user_id that isn't used; either remove current_user_id from the function signature (and eliminate the CurrentUserId import) if WorkspaceId already enforces auth, or if you need the user for auditing add a concise audit/log line referencing current_user_id (e.g., log the ID when the request is processed) and keep the parameter; update any tests or callers accordingly so there are no unused-parameter warnings.backend/src/pilot_space/infrastructure/database/models/workspace_mcp_server.py (1)
172-176: UseMcpApprovalModeend-to-end forapproval_mode.The enum is introduced above, but the mapped field is still
Mapped[str]over a plainString(16). That loses the type contract for every caller and defers bad values until the database check constraint fires. Consider mapping/coercing this field asMcpApprovalModeat the model boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/pilot_space/infrastructure/database/models/workspace_mcp_server.py` around lines 172 - 176, The approval_mode column is currently declared as Mapped[str] with String(16) which loses the enum type; update the model (e.g., the WorkspaceMcpServer model and its approval_mode attribute) to use Mapped[McpApprovalMode] and declare the column with an Enum type that references McpApprovalMode (e.g., mapped_column(Enum(McpApprovalMode), nullable=False, default=McpApprovalMode.AUTO_APPROVE, doc=...)); ensure the Enum type is imported from sqlalchemy and McpApprovalMode is used for default and any type coercion at the model boundary so callers get the enum type end-to-end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbe26d81-41b4-41e4-ad10-f47cb41a2742
📒 Files selected for processing (85)
.planning/PROJECT.md.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/30/30-01-PLAN.md.planning/phases/31/31-01-PLAN.md.planning/phases/31/31-02-PLAN.md.planning/phases/31/31-02-SUMMARY.md.planning/phases/31/31-03-PLAN.md.planning/phases/31/31-03-SUMMARY.md.planning/phases/31/31-04-PLAN.md.planning/phases/31/31-04-SUMMARY.md.planning/phases/32/32-01-PLAN.md.planning/phases/32/32-01-SUMMARY.md.planning/phases/32/32-02-PLAN.md.planning/phases/32/32-03-PLAN.md.planning/phases/32/32-03-SUMMARY.md.planning/phases/33/33-01-PLAN.md.planning/phases/33/33-01-SUMMARY.md.planning/phases/33/33-02-PLAN.md.planning/phases/33/33-02-SUMMARY.md.planning/phases/33/33-03-PLAN.md.planning/phases/33/33-03-SUMMARY.md.planning/phases/34/34-01-PLAN.md.planning/phases/34/34-01-SUMMARY.md.planning/phases/34/34-02-PLAN.md.planning/phases/34/34-02-SUMMARY.md.planning/phases/35/35-01-PLAN.md.planning/phases/35/35-01-SUMMARY.md.planning/phases/35/35-02-PLAN.md.planning/phases/35/35-02-SUMMARY.mdbackend/alembic/versions/091_add_mcp_transport_type.pybackend/alembic/versions/092_add_oauth_refresh_token.pybackend/alembic/versions/093_add_mcp_approval_mode.pybackend/alembic/versions/094_add_mcp_audit_index.pybackend/alembic/versions/095_add_mcp_catalog_entries.pybackend/alembic/versions/096_add_catalog_fk_to_mcp_servers.pybackend/alembic/versions/097_add_stdio_mcp_support.pybackend/src/pilot_space/ai/agents/pilotspace_agent.pybackend/src/pilot_space/ai/agents/pilotspace_stream_utils.pybackend/src/pilot_space/ai/infrastructure/approval.pybackend/src/pilot_space/ai/sdk/hooks_lifecycle.pybackend/src/pilot_space/ai/sdk/question_adapter.pybackend/src/pilot_space/api/v1/routers/__init__.pybackend/src/pilot_space/api/v1/routers/_mcp_server_oauth_helpers.pybackend/src/pilot_space/api/v1/routers/_mcp_server_schemas.pybackend/src/pilot_space/api/v1/routers/ai.pybackend/src/pilot_space/api/v1/routers/mcp_catalog.pybackend/src/pilot_space/api/v1/routers/mcp_usage.pybackend/src/pilot_space/api/v1/routers/workspace_mcp_servers.pybackend/src/pilot_space/infrastructure/database/models/mcp_catalog_entry.pybackend/src/pilot_space/infrastructure/database/models/workspace_mcp_server.pybackend/src/pilot_space/infrastructure/database/repositories/mcp_catalog_repository.pybackend/src/pilot_space/infrastructure/database/repositories/workspace_mcp_server_repository.pybackend/src/pilot_space/infrastructure/ssrf.pybackend/src/pilot_space/main.pybackend/tests/ai/agents/test_remote_mcp_loading.pybackend/tests/api/test_workspace_mcp_servers.pybackend/tests/unit/ai/agents/test_build_stream_config_allowed_tools.pybackend/tests/unit/ai/agents/test_pilotspace_stream_utils.pybackend/tests/unit/ai/sdk/test_hooks_lifecycle_mcp.pybackend/tests/unit/ai/sdk/test_question_adapter.pybackend/tests/unit/api/test_mcp_catalog_router.pybackend/tests/unit/routers/test_mcp_usage.pybackend/tests/unit/test_startup_encryption_enforcement.pydocs/MCP_VERIFICATION_GUIDE.mdfrontend/src/features/costs/pages/cost-dashboard-page.tsxfrontend/src/features/settings/components/__tests__/mcp-catalog-card.test.tsxfrontend/src/features/settings/components/__tests__/mcp-catalog-tab-content.test.tsxfrontend/src/features/settings/components/__tests__/mcp-server-card.test.tsxfrontend/src/features/settings/components/mcp-catalog-card.tsxfrontend/src/features/settings/components/mcp-catalog-tab-content.tsxfrontend/src/features/settings/components/mcp-server-card.tsxfrontend/src/features/settings/components/mcp-server-form.tsxfrontend/src/features/settings/pages/__tests__/mcp-servers-settings-page.test.tsxfrontend/src/features/settings/pages/mcp-servers-settings-page.tsxfrontend/src/services/api/ai.tsfrontend/src/services/api/mcp-catalog.tsfrontend/src/services/api/mcp-servers.tsfrontend/src/stores/ai/AIStore.tsfrontend/src/stores/ai/MCPCatalogStore.tsfrontend/src/stores/ai/MCPServersStore.tsfrontend/src/stores/ai/__tests__/MCPCatalogStore.test.tsfrontend/src/stores/ai/__tests__/MCPServersStore.test.tsfrontend/src/stores/ai/index.ts
Comment on lines
+112
to
+124
| <behavior> | ||
| - McpApprovalModeUpdate schema: Pydantic BaseModel with approval_mode: Literal["auto_approve", "require_approval"] | ||
| - WorkspaceMcpServerResponse gains approval_mode: str = "auto_approve" field | ||
| - PATCH /{server_id}/approval-mode: requires ADMIN/OWNER role, loads server, sets approval_mode, commits, returns updated WorkspaceMcpServerResponse | ||
| - test_update_approval_mode_success: PATCH with require_approval returns 200 + approval_mode == "require_approval" | ||
| - test_update_approval_mode_invalid: PATCH with "bad_value" returns 422 | ||
| - test_update_approval_mode_unauthorized: PATCH as MEMBER returns 403 | ||
| </behavior> | ||
| <action> | ||
| 1. Check current line count of workspace_mcp_servers.py (known ~670 lines). If adding the PATCH endpoint would exceed 695 lines (pre-commit 700-line limit), extract the handler to a helper or keep action body tight. The endpoint itself is ~20 lines so it should fit. | ||
| 2. Add McpApprovalModeUpdate to _mcp_server_schemas.py, and add approval_mode: str = "auto_approve" to WorkspaceMcpServerResponse. | ||
| 3. Add PATCH endpoint to workspace_mcp_servers.py. Import McpApprovalModeUpdate from _mcp_server_schemas. Follow the exact same role-check pattern used by the existing update endpoint (check for OWNER or ADMIN role, raise 403 otherwise). Use SQLAlchemy update statement or load-then-set pattern matching the existing codebase style. | ||
| 4. Write tests extending test_workspace_mcp_servers.py. Mock the DB session and repository following the existing test patterns in that file. Write the three test cases listed in the behavior block above. |
There was a problem hiding this comment.
Add RFC 7807 assertions for the new PATCH error paths.
The invalid-value and unauthorized cases are defined only as 422/403. If the endpoint follows FastAPI defaults, those responses will still be application/json, so this plan can pass while the backend contract is wrong. Please make the implementation/tests assert the application/problem+json content type and problem body shape for those paths.
As per coding guidelines, "Backend: All error responses must use Content-Type: application/problem+json (RFC 7807) format, not application/json".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/phases/33/33-01-PLAN.md around lines 112 - 124, Ensure the new
PATCH /{server_id}/approval-mode returns RFC 7807 problem responses
(Content-Type: application/problem+json and body with type, title, status,
detail) for error paths: the validation error for McpApprovalModeUpdate
(invalid-value -> 422) and the role-denied path in workspace_mcp_servers.py
(unauthorized -> 403). Update the handler (where you add McpApprovalModeUpdate
and set approval_mode on WorkspaceMcpServerResponse) to raise or return error
responses formatted as RFC7807 (include the required fields and correct status)
instead of default application/json, and adjust any helper/exception
construction used by the OWNER/ADMIN role check to produce
application/problem+json. Finally, extend tests
test_update_approval_mode_invalid and test_update_approval_mode_unauthorized to
assert response.headers["content-type"] == "application/problem+json" and that
the JSON body contains at least type, title, status, and detail fields (keep
test names: test_update_approval_mode_success,
test_update_approval_mode_invalid, test_update_approval_mode_unauthorized).
Comment on lines
+28
to
+31
| - "Admin can open MCP settings, click 'Catalog' tab, and see entry cards with name, description, transport type, auth type, and an Official badge for is_official entries" | ||
| - "Clicking 'Install' on a catalog entry calls mcpStore.registerServer() with url, auth_type, transport_type, oauth fields, catalog_entry_id, and installed_catalog_version pre-filled" | ||
| - "An installed server whose installed_catalog_version differs from catalog entry's catalog_version shows an amber 'Update Available' badge on its card in the Registered Servers tab" | ||
| - "The Catalog tab shows filter chips for transport type (All, HTTP, SSE) that narrow displayed cards" |
There was a problem hiding this comment.
Resolve where “Update Available” is supposed to appear.
The must-haves/success criteria place the badge in the Registered Servers tab, but Task 2 later says the correct implementation is Catalog-tab only and explicitly avoids mcp-server-card.tsx. Those directions produce different UI and tests, so the acceptance criteria need to be aligned before implementation.
Also applies to: 272-275, 338-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/phases/35/35-02-PLAN.md around lines 28 - 31, The acceptance
criteria conflict about where the "Update Available" badge should appear
(Registered Servers tab vs Catalog tab/Task 2 and explicit avoidance of
mcp-server-card.tsx); pick one canonical location and update the plan text and
any referenced tasks to match: either (A) state that "Update Available" appears
on the Registered Servers tab and update Task 2 to remove the Catalog-only
restriction and allow mcp-server-card.tsx changes, or (B) state that the badge
appears only in the Catalog tab and update the must-haves/acceptance criteria to
remove the Registered Servers reference; ensure all occurrences of the badge
text ("Update Available"), the tabs ("Registered Servers", "Catalog tab"), and
the Task 2 note about mcp-server-card.tsx are consistent (also update the other
affected sections mentioned around lines 272-275 and 338-339) so the plan and
tasks are aligned.
Comment on lines
+124
to
+140
| From MCPServersStore.ts (MCPServer type — MUST extend with new fields from 35-01): | ||
| ```typescript | ||
| export interface MCPServer { | ||
| id: string; | ||
| workspace_id: string; | ||
| display_name: string; | ||
| url: string; | ||
| auth_type: 'bearer' | 'oauth2'; | ||
| last_status: 'connected' | 'failed' | 'unknown' | null; | ||
| last_status_checked_at: string | null; | ||
| token_expires_at: string | null; | ||
| approval_mode?: 'auto_approve' | 'require_approval'; | ||
| created_at: string; | ||
| // Add these two (nullable, from 35-01 WorkspaceMcpServerResponse): | ||
| catalog_entry_id?: string | null; | ||
| installed_catalog_version?: string | null; | ||
| } |
There was a problem hiding this comment.
Keep the MCPServer contract nullable for stdio URLs.
This snippet still requires url: string, but backend WorkspaceMcpServerResponse now returns null for stdio servers. If the frontend follows this plan literally, loading a registered stdio server will require unsafe casts or break store hydration. Update the plan contract to model url as nullable and call out the stdio fields alongside it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.planning/phases/35/35-02-PLAN.md around lines 124 - 140, The MCPServer
interface must allow null URLs because WorkspaceMcpServerResponse returns null
for stdio servers; update the MCPServer type (symbol: MCPServer) to make the url
field nullable (url: string | null) and explicitly document or add nullable
stdio-related fields alongside it (e.g., any stdio-specific flags that the
backend returns) so store hydration and deserialization from
WorkspaceMcpServerResponse handle stdio entries safely; ensure any code using
MCPServer.url (e.g., store hydration and UI components) checks for null before
using the value.
Comment on lines
+44
to
+52
| "transport_type", | ||
| sa.VARCHAR(8), | ||
| nullable=False, | ||
| ), | ||
| sa.Column( | ||
| "auth_type", | ||
| sa.VARCHAR(8), | ||
| nullable=False, | ||
| ), |
There was a problem hiding this comment.
transport_type and auth_type need DB constraints (or enums).
Using plain VARCHAR here permits invalid values in a global catalog table. Please enforce allowed values with existing enums (preferred) or explicit CHECK constraints.
🛡️ Suggested constraint-based patch
sa.Column(
"transport_type",
sa.VARCHAR(8),
nullable=False,
),
sa.Column(
"auth_type",
sa.VARCHAR(8),
nullable=False,
),
+ sa.CheckConstraint(
+ "transport_type IN ('sse', 'http', 'stdio')",
+ name="ck_mcp_catalog_entries_transport_type_valid",
+ ),
+ sa.CheckConstraint(
+ "auth_type IN ('none', 'bearer', 'oauth2')",
+ name="ck_mcp_catalog_entries_auth_type_valid",
+ ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "transport_type", | |
| sa.VARCHAR(8), | |
| nullable=False, | |
| ), | |
| sa.Column( | |
| "auth_type", | |
| sa.VARCHAR(8), | |
| nullable=False, | |
| ), | |
| "transport_type", | |
| sa.VARCHAR(8), | |
| nullable=False, | |
| ), | |
| sa.Column( | |
| "auth_type", | |
| sa.VARCHAR(8), | |
| nullable=False, | |
| ), | |
| sa.CheckConstraint( | |
| "transport_type IN ('sse', 'http', 'stdio')", | |
| name="ck_mcp_catalog_entries_transport_type_valid", | |
| ), | |
| sa.CheckConstraint( | |
| "auth_type IN ('none', 'bearer', 'oauth2')", | |
| name="ck_mcp_catalog_entries_auth_type_valid", | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/alembic/versions/095_add_mcp_catalog_entries.py` around lines 44 -
52, The migration currently defines the "transport_type" and "auth_type" columns
as sa.VARCHAR(8) which allows invalid values; update the migration to enforce
allowed values by replacing those sa.VARCHAR(8) columns with enum-backed columns
(sa.Enum) referencing the existing enums used elsewhere (e.g., TransportType and
AuthType SQLAlchemy/DB enums) or, if enums do not exist in this repo, add
explicit CHECK constraints listing allowed literals for "transport_type" and
"auth_type"; ensure you use the same enum names (or constraint names) as other
migrations and include the enum/constraint creation in upgrade() and drop it in
downgrade() so the migration is reversible.
Comment on lines
+96
to
+101
| def downgrade() -> None: | ||
| """Remove stdio support columns (enum value cannot be removed from PostgreSQL).""" | ||
| op.alter_column("workspace_mcp_servers", "url", nullable=False) | ||
| op.drop_column("workspace_mcp_servers", "stdio_args") | ||
| op.drop_column("workspace_mcp_servers", "stdio_command") | ||
| # Note: PostgreSQL does not support removing enum values; 'stdio' remains in the enum. |
There was a problem hiding this comment.
Downgrade will fail if stdio servers exist with NULL urls.
Setting url back to NOT NULL (line 98) will fail if any workspace_mcp_servers rows have url=NULL (i.e., stdio servers). Consider either:
- Deleting stdio servers in the downgrade, or
- Setting a placeholder URL before altering the column.
🛡️ Proposed fix to handle existing stdio rows
def downgrade() -> None:
"""Remove stdio support columns (enum value cannot be removed from PostgreSQL)."""
+ # Set placeholder URL for any stdio servers before making column non-nullable
+ op.execute(
+ text(
+ "UPDATE workspace_mcp_servers SET url = 'removed://stdio-server' "
+ "WHERE url IS NULL"
+ )
+ )
op.alter_column("workspace_mcp_servers", "url", nullable=False)
op.drop_column("workspace_mcp_servers", "stdio_args")
op.drop_column("workspace_mcp_servers", "stdio_command")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/alembic/versions/097_add_stdio_mcp_support.py` around lines 96 - 101,
The downgrade() currently sets workspace_mcp_servers.url back to NOT NULL which
will fail if any stdio rows have url=NULL; update downgrade() to first handle
existing stdio rows by either (a) deleting rows that represent stdio servers
(identify by stdio_command IS NOT NULL or server_type == 'stdio') or (b)
updating those rows to set a non-null placeholder value for the url before
calling op.alter_column; then proceed to drop the stdio_args and stdio_command
columns and alter the url column. Reference the downgrade() function and the
workspace_mcp_servers table and stdio_command/stdio_args/url columns when making
the change.
Comment on lines
+13
to
+45
| def _generate_remote_tool_patterns(remote_servers: dict[str, object]) -> list[str]: | ||
| """Mirrors the production expression added to _build_stream_config.""" | ||
| return [f"mcp__{key}__*" for key in remote_servers] | ||
|
|
||
|
|
||
| def test_no_remote_servers_produces_no_patterns() -> None: | ||
| patterns = _generate_remote_tool_patterns({}) | ||
| assert patterns == [] | ||
| combined = list(ALL_TOOL_NAMES) + patterns | ||
| assert combined == list(ALL_TOOL_NAMES) | ||
|
|
||
|
|
||
| def test_single_remote_server_produces_one_wildcard() -> None: | ||
| remote_servers = {"remote_abc-123-def": object()} | ||
| patterns = _generate_remote_tool_patterns(remote_servers) | ||
| assert patterns == ["mcp__remote_abc-123-def__*"] | ||
|
|
||
|
|
||
| def test_multiple_remote_servers_produce_one_wildcard_each() -> None: | ||
| remote_servers = {"remote_aaa": object(), "remote_bbb": object()} | ||
| patterns = _generate_remote_tool_patterns(remote_servers) | ||
| assert "mcp__remote_aaa__*" in patterns | ||
| assert "mcp__remote_bbb__*" in patterns | ||
| assert len(patterns) == 2 | ||
|
|
||
|
|
||
| def test_all_tool_names_preserved_after_merge() -> None: | ||
| remote_servers = {"remote_xyz": object()} | ||
| patterns = _generate_remote_tool_patterns(remote_servers) | ||
| combined = list(ALL_TOOL_NAMES) + patterns | ||
| for name in ALL_TOOL_NAMES: | ||
| assert name in combined | ||
| assert "mcp__remote_xyz__*" in combined |
There was a problem hiding this comment.
These tests don’t validate the production integration path.
Right now the suite verifies a local helper, not the actual PilotSpaceAgent._build_stream_config wiring that passes additional_tools to configure_sdk_for_space. A regression in production could still pass this file.
Please add at least one test that exercises _build_stream_config and asserts additional_tools includes mcp__<key>__* when remote servers are present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/unit/ai/agents/test_build_stream_config_allowed_tools.py`
around lines 13 - 45, Add a unit test that calls
PilotSpaceAgent._build_stream_config (or constructs a PilotSpaceAgent and
triggers the path that calls _build_stream_config) with a non-empty remote
servers dict and then captures the args passed to configure_sdk_for_space
(mocking/stubbing it) to assert that the returned/forwarded additional_tools
contains the expected "mcp__<key>__*" wildcard(s); specifically reference
PilotSpaceAgent._build_stream_config, configure_sdk_for_space and the
additional_tools parameter and assert it includes "mcp__remote_xyz__*" (or the
key you inject) to ensure the production wiring is tested end-to-end.
|
|
||
| This mirrors exactly what the lifespan does after jwt_provider_validated. | ||
| """ | ||
| if settings.is_production: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify method definition and current call-site usage.
rg -nP 'def\s+is_production\s*\(' backend/src/pilot_space/config.py
rg -nP '\bif\s+settings\.is_production\b' backend/tests/unit/test_startup_encryption_enforcement.py
# Expected:
# 1) config.py shows is_production defined as a method.
# 2) this test file shows a non-invoked usage at the if-condition.Repository: pilotspace/pilot-space
Length of output: 140
Call is_production(); condition checks the method object instead of invoking it.
Line 47 references the bound method object (settings.is_production) without calling it. In Python, method objects are always truthy, so this condition executes regardless of the actual production status, causing production encryption enforcement to run in all environments.
Fix
- if settings.is_production:
+ if settings.is_production():📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if settings.is_production: | |
| if settings.is_production(): |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/unit/test_startup_encryption_enforcement.py` at line 47, The
condition in the test uses the bound method object instead of invoking it:
change the conditional that references settings.is_production to call the method
(settings.is_production()) so the check uses the returned boolean; update any
related occurrences in test_startup_encryption_enforcement.py (look for the if
settings.is_production line) to invoke the method rather than checking the
method object.
Comment on lines
+85
to
+97
| <button | ||
| key={value} | ||
| type="button" | ||
| onClick={() => setActiveFilter(value)} | ||
| className={`rounded-full px-3 py-1 text-xs font-medium transition-colors ${ | ||
| activeFilter === value | ||
| ? 'bg-primary text-primary-foreground' | ||
| : 'bg-muted text-muted-foreground hover:bg-muted/80' | ||
| }`} | ||
| > | ||
| {label} | ||
| </button> | ||
| ))} |
There was a problem hiding this comment.
Expose filter chip selection state to assistive tech.
The selected chip is currently conveyed only through styling. Add aria-pressed (and ideally focus-visible styles) so screen readers and keyboard users can identify the active filter state.
♿ Suggested patch
{FILTER_CHIPS.map(({ label, value }) => (
<button
key={value}
type="button"
onClick={() => setActiveFilter(value)}
+ aria-pressed={activeFilter === value}
className={`rounded-full px-3 py-1 text-xs font-medium transition-colors ${
activeFilter === value
- ? 'bg-primary text-primary-foreground'
- : 'bg-muted text-muted-foreground hover:bg-muted/80'
+ ? 'bg-primary text-primary-foreground'
+ : 'bg-muted text-muted-foreground hover:bg-muted/80'
}`}
>As per coding guidelines "Frontend: Implement WCAG 2.2 AA accessibility baseline; never use color as sole meaning carrier; prioritize keyboard-first interaction".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button | |
| key={value} | |
| type="button" | |
| onClick={() => setActiveFilter(value)} | |
| className={`rounded-full px-3 py-1 text-xs font-medium transition-colors ${ | |
| activeFilter === value | |
| ? 'bg-primary text-primary-foreground' | |
| : 'bg-muted text-muted-foreground hover:bg-muted/80' | |
| }`} | |
| > | |
| {label} | |
| </button> | |
| ))} | |
| <button | |
| key={value} | |
| type="button" | |
| onClick={() => setActiveFilter(value)} | |
| aria-pressed={activeFilter === value} | |
| className={`rounded-full px-3 py-1 text-xs font-medium transition-colors ${ | |
| activeFilter === value | |
| ? 'bg-primary text-primary-foreground' | |
| : 'bg-muted text-muted-foreground hover:bg-muted/80' | |
| }`} | |
| > | |
| {label} | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/settings/components/mcp-catalog-tab-content.tsx` around
lines 85 - 97, The filter buttons rendered in mcp-catalog-tab-content.tsx convey
selection only via color; update the button element (the map that uses
key={value}, onClick={() => setActiveFilter(value)}, activeFilter and value) to
add aria-pressed={activeFilter === value} and include
focus-visible/focus-outline utility classes in the className so keyboard users
see focus (e.g., add a focus-visible ring or outline style and ensure focus
styles are not color-only); keep the existing onClick and state logic unchanged.
Comment on lines
+153
to
+157
| <p className="text-xs text-muted-foreground truncate max-w-[200px]"> | ||
| {server.transport_type === 'stdio' | ||
| ? `${server.stdio_command ?? ''} ${server.stdio_args ? (JSON.parse(server.stdio_args) as string[]).join(' ') : ''}`.trim() | ||
| : (server.url ?? '')} | ||
| </p> |
There was a problem hiding this comment.
JSON.parse on stdio_args can throw if malformed.
If server.stdio_args contains invalid JSON, this will throw an unhandled exception and crash the component. Consider wrapping in try-catch or validating the format.
🛡️ Proposed fix with safe parsing
<p className="text-xs text-muted-foreground truncate max-w-[200px]">
{server.transport_type === 'stdio'
- ? `${server.stdio_command ?? ''} ${server.stdio_args ? (JSON.parse(server.stdio_args) as string[]).join(' ') : ''}`.trim()
+ ? `${server.stdio_command ?? ''} ${server.stdio_args ? (() => { try { return (JSON.parse(server.stdio_args) as string[]).join(' '); } catch { return server.stdio_args; } })() : ''}`.trim()
: (server.url ?? '')}
</p>Or extract to a helper function for clarity:
function formatStdioArgs(argsJson: string | null | undefined): string {
if (!argsJson) return '';
try {
return (JSON.parse(argsJson) as string[]).join(' ');
} catch {
return argsJson; // Fallback to raw string
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <p className="text-xs text-muted-foreground truncate max-w-[200px]"> | |
| {server.transport_type === 'stdio' | |
| ? `${server.stdio_command ?? ''} ${server.stdio_args ? (JSON.parse(server.stdio_args) as string[]).join(' ') : ''}`.trim() | |
| : (server.url ?? '')} | |
| </p> | |
| <p className="text-xs text-muted-foreground truncate max-w-[200px]"> | |
| {server.transport_type === 'stdio' | |
| ? `${server.stdio_command ?? ''} ${server.stdio_args ? (() => { try { return (JSON.parse(server.stdio_args) as string[]).join(' '); } catch { return server.stdio_args; } })() : ''}`.trim() | |
| : (server.url ?? '')} | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/settings/components/mcp-server-card.tsx` around lines
153 - 157, The JSX rendering in mcp-server-card.tsx uses
JSON.parse(server.stdio_args) which can throw on malformed JSON; update the
rendering to safely parse stdio_args—either create a helper like
formatStdioArgs(argsJson) that returns '' for null/undefined, tries to
JSON.parse and join string[] in a try-catch, and falls back to the raw argsJson
on parse failure, or inline the same try-catch logic before using the value;
replace the direct JSON.parse usage in the expression that builds the stdio
string (referencing server.stdio_command and server.stdio_args) with the safe
result.
Comment on lines
+117
to
+139
| await mcpStore.registerServer(workspaceId, { | ||
| display_name: entry.name, | ||
| url: '', | ||
| auth_type: entry.auth_type, | ||
| transport_type: 'stdio', | ||
| stdio_command: command, | ||
| stdio_args: args, | ||
| catalog_entry_id: entry.id, | ||
| installed_catalog_version: entry.catalog_version, | ||
| }); | ||
| } else { | ||
| await mcpStore.registerServer(workspaceId, { | ||
| display_name: entry.name, | ||
| url: entry.url_template, | ||
| auth_type: entry.auth_type, | ||
| transport_type: entry.transport_type, | ||
| oauth_auth_url: entry.oauth_auth_url ?? undefined, | ||
| oauth_token_url: entry.oauth_token_url ?? undefined, | ||
| oauth_scopes: entry.oauth_scopes ?? undefined, | ||
| catalog_entry_id: entry.id, | ||
| installed_catalog_version: entry.catalog_version, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Refresh the server list after a catalog install.
The manual form flow already calls mcpStore.loadServers(workspaceId) on success, but this path switches tabs immediately after registerServer(). Without the same reload, the Registered tab can stay stale right after a successful install.
🔄 Suggested fix
} else {
await mcpStore.registerServer(workspaceId, {
display_name: entry.name,
url: entry.url_template,
auth_type: entry.auth_type,
transport_type: entry.transport_type,
oauth_auth_url: entry.oauth_auth_url ?? undefined,
oauth_token_url: entry.oauth_token_url ?? undefined,
oauth_scopes: entry.oauth_scopes ?? undefined,
catalog_entry_id: entry.id,
installed_catalog_version: entry.catalog_version,
});
}
+ await mcpStore.loadServers(workspaceId);
toast.success('Server installed — add your auth token to activate it.');
setActiveTab('registered');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/settings/pages/mcp-servers-settings-page.tsx` around
lines 117 - 139, After each successful catalog install path that calls
mcpStore.registerServer(workspaceId, ...), immediately refresh the server list
by awaiting mcpStore.loadServers(workspaceId) before the code that switches
tabs; update both the stdio branch (where stdio_command/stdio_args are used) and
the transport/oauth branch so the Registered tab is reloaded and not stale after
install.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Complete MCP platform hardening milestone (v1.1.0) — 6 phases, 19 requirements, 51 commits, 85 files changed (+11,454 / -321 lines).
Critical bug fix:
allowed_tools— Claude could see them but couldn't invoke them. Fixed withmcp__<key>__*wildcard patterns.Infrastructure hardening:
ENCRYPTION_KEYenforcement — fail-fast startupOAuth refresh flow:
DD-003 approval for remote MCP tools:
can_use_toolcallback extended formcp__remote_*toolsInlineApprovalCardObservability:
ai.mcp_tool_callin audit_log)MCP server catalog:
Stdio transport support:
npx,node,python3,python,uvxMcpAuthType.NONEfor servers needing no authenticationMigrations (091–097)
transport_typecolumn +mcp_transport_typeenumrefresh_token_encrypted+token_expires_atapproval_modecolumnmcp_catalog_entriestable + Context7/GitHub seedscatalog_entry_idFK +installed_catalog_versionstdio_command,stdio_args,auth_type=none, Sequential Thinking seedTest plan
cd backend && uv run pytest tests/unit/ai/ -q— all agent tests passcd frontend && pnpm type-check && pnpm lint— zero errorsalembic upgrade head— 091 through 097 apply cleanlySELECT name, transport_type, auth_type FROM mcp_catalog_entries— 3 entriesENCRYPTION_KEY→ RuntimeError with key generation commanddocs/MCP_VERIFICATION_GUIDE.md(11 test flows)Summary by CodeRabbit
New Features
Improvements