MCP-01: Minimal MCP prototype — taskdeck://boards via stdio#664
MCP-01: Minimal MCP prototype — taskdeck://boards via stdio#664Chris0Jeky merged 13 commits intomainfrom
Conversation
…y resolution Abstracts user resolution for stdio (local OS identity → first DB user) and future HTTP (JWT/API-key) scenarios. StdioUserContextProvider honours a McpServer:DefaultUserId config key or falls back to the first user in the DB.
Exposes active boards as a compact JSON MCP resource scoped to the authenticated
user. Shape: { boards: [{ id, name, columnCount, cardCount, isArchived, updatedAt }],
totalCount }. Read-only; no board mutations via this resource.
When launched with --mcp, the process runs as a single-session stdio MCP server (no HTTP, no JWT, no CORS, no SignalR). The web startup path is completely unchanged. Logs are redirected to stderr so stdout is reserved for the MCP JSON-RPC transport.
7 tests covering: StdioUserContextProvider configured/fallback/empty-DB paths, BoardResources compact JSON shape, archived-board exclusion, column and card count accuracy, and empty board list.
Copy to .mcp.json in the repo root (or merge into existing) to wire Claude Code to the Taskdeck MCP server running in stdio mode.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an MCP (Model Context Protocol) server implementation, allowing Taskdeck to function as an MCP server via stdio. Key changes include the addition of the BoardResources provider, a new IUserContextProvider interface with a StdioUserContextProvider implementation, and necessary configuration updates. The review identified several critical issues: the IUserContextProvider interface and its implementation currently rely on synchronous, blocking I/O which can cause deadlocks and thread pool starvation; the StdioUserContextProvider constructor performs inefficient database queries; and the ListBoards method in BoardResources exhibits an N+1 query pattern that will degrade performance as the number of boards increases.
| public interface IUserContextProvider | ||
| { | ||
| /// <summary> | ||
| /// Returns the current user's ID. | ||
| /// Throws <see cref="InvalidOperationException"/> if no user can be resolved. | ||
| /// </summary> | ||
| Guid GetCurrentUserId(); | ||
|
|
||
| /// <summary> | ||
| /// Attempts to resolve the current user's ID without throwing. | ||
| /// </summary> | ||
| bool TryGetCurrentUserId(out Guid userId); |
There was a problem hiding this comment.
The IUserContextProvider interface uses synchronous methods, which forces implementations (like StdioUserContextProvider) to use blocking calls for I/O. This can lead to deadlocks and thread pool starvation in ASP.NET Core. Since this is a new interface, it should be designed as asynchronous from the start.
public interface IUserContextProvider
{
/// <summary>
/// Returns the current user's ID.
/// </summary>
Task<Guid> GetCurrentUserIdAsync(CancellationToken cancellationToken = default);
/// <summary>
/// Attempts to resolve the current user's ID without throwing.
/// </summary>
Task<Guid?> GetUserIdAsync(CancellationToken cancellationToken = default);
}| } | ||
|
|
||
| // Fall back to the first user in the database (single-user local-first scenario). | ||
| var users = unitOfWork.Users.GetAllAsync().GetAwaiter().GetResult(); |
There was a problem hiding this comment.
This line has two significant issues:
- Sync-over-async: Using
.GetAwaiter().GetResult()in a constructor is a dangerous pattern in .NET that can cause deadlocks. Since this class is registered asScoped, this blocking I/O happens on every request scope. - Inefficiency:
GetAllAsync()followed byFirstOrDefault()may load the entire users table into memory before picking the first one.
Recommendation: Move this logic into the implementation of an asynchronous GetCurrentUserIdAsync method and use a targeted query like FirstOrDefaultAsync().
| MimeType = "application/json")] | ||
| public async Task<string> ListBoards() | ||
| { | ||
| var userId = _userContext.GetCurrentUserId(); |
There was a problem hiding this comment.
Once the IUserContextProvider is updated to be asynchronous (see the suggestion in that file), this call should be updated to use await.
var userId = await _userContext.GetCurrentUserIdAsync();| foreach (var boardDto in listResult.Value) | ||
| { | ||
| // Fetch board detail to get column and card counts. | ||
| // Acceptable for Phase 1: local SQLite, typically < 20 boards per user. | ||
| var detailResult = await _boardService.GetBoardDetailAsync(boardDto.Id, userId); | ||
| if (!detailResult.IsSuccess) | ||
| continue; // skip boards we can't read (race condition / deleted mid-request) | ||
|
|
||
| var detail = detailResult.Value; | ||
| var cardCount = detail.Columns.Sum(c => c.CardCount); | ||
|
|
||
| boardSummaries.Add(new | ||
| { | ||
| id = detail.Id, | ||
| name = detail.Name, | ||
| columnCount = detail.Columns.Count, | ||
| cardCount, | ||
| isArchived = detail.IsArchived, | ||
| updatedAt = detail.UpdatedAt | ||
| }); | ||
| } |
There was a problem hiding this comment.
This loop introduces an N+1 query pattern by calling GetBoardDetailAsync for every board returned by ListBoardsAsync. While the comment at line 57 notes this is acceptable for Phase 1, it will become a significant performance bottleneck as the number of boards grows.
Consider adding a specialized method to BoardService or IBoardRepository that can fetch board summaries including column and card counts in a single query (e.g., using a SQL JOIN or a GroupBy projection).
…lR DI failure Replaced AddApplicationServices() in MCP mode with targeted registrations for BoardService (no realtime notifier), AuthorizationService, and AuthorizationService. This prevents DI failure caused by CompositeBoardRealtimeNotifier requiring IHubContext<BoardsHub> which is unavailable when SignalR is not loaded.
Self-Review FindingsMCP path isolation (skips web middleware): yes — Board query scoping (user-scoped, no leaks): yes — Compact JSON format: Program.cs web path unchanged: yes — the DI registration leaks (critical fix applied): Fixed. Initial version called Security concerns: none identified. stdio mode inherits OS process identity; no network listener; no JWT bypass path. An attacker who can launch the process already has direct SQLite read access. Critical issues resolved: DI failure from SignalR dependency (fixed in post-commit e2f7f56). |
There was a problem hiding this comment.
Pull request overview
Implements Phase 1 of a minimal embedded MCP server (stdio transport) in the API process, exposing a single taskdeck://boards resource and a basic stdio user-identity mapping.
Changes:
- Add
--mcpstartup mode that runs an MCP stdio host instead of the web API pipeline. - Introduce MCP board resource (
taskdeck://boards) with compact JSON output and add a stdio user context provider. - Add example MCP client config and a new test suite validating resource shape and identity resolution.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| mcp.example.json | Adds a sample .mcp.json-style config for running the server via dotnet run ... -- --mcp. |
| backend/tests/Taskdeck.Api.Tests/McpBoardResourcesTests.cs | New tests covering board resource JSON shape/counts and stdio user context resolution. |
| backend/src/Taskdeck.Infrastructure/Mcp/StdioUserContextProvider.cs | Implements stdio user identity resolution via configured user ID or DB fallback. |
| backend/src/Taskdeck.Application/Interfaces/IUserContextProvider.cs | New application-layer abstraction for MCP user identity resolution. |
| backend/src/Taskdeck.Api/Taskdeck.Api.csproj | Adds ModelContextProtocol package dependency. |
| backend/src/Taskdeck.Api/Program.cs | Adds --mcp branching startup path configuring MCP stdio transport and resources. |
| backend/src/Taskdeck.Api/Mcp/BoardResources.cs | New MCP resource provider exposing taskdeck://boards with compact JSON output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .ConfigureServices((ctx, services) => | ||
| { | ||
| // Infrastructure (DbContext, Repositories, UoW) | ||
| services.AddInfrastructure(ctx.Configuration); | ||
|
|
||
| // Register only the Application services needed by MCP resources. | ||
| // We deliberately skip web-only services (SignalR notifiers, workers, | ||
| // LLM providers, rate limiting, etc.) to keep the MCP host minimal. | ||
| services.AddScoped<Taskdeck.Application.Services.BoardService>(sp => | ||
| new Taskdeck.Application.Services.BoardService( | ||
| sp.GetRequiredService<IUnitOfWork>(), | ||
| sp.GetService<Taskdeck.Application.Services.IAuthorizationService>())); | ||
| services.AddScoped<Taskdeck.Application.Services.AuthorizationService>(); | ||
| services.AddScoped<Taskdeck.Application.Services.IAuthorizationService>( | ||
| sp => sp.GetRequiredService<Taskdeck.Application.Services.AuthorizationService>()); | ||
|
|
||
| // Stdio identity: maps the OS process owner to the local default user. |
There was a problem hiding this comment.
In MCP stdio mode the host never runs EF Core migrations. Web mode does this in ConfigureTaskdeckPipeline and the CLI does it during startup, so --mcp can fail against a fresh/older DB (missing tables/columns) even before the user-resolution logic runs. Consider running dbContext.Database.Migrate() once after Build() (before RunAsync()), using a scoped TaskdeckDbContext like the other entrypoints.
| var configuredId = configuration["McpServer:DefaultUserId"]; | ||
| if (configuredId is not null && Guid.TryParse(configuredId, out var parsed)) | ||
| { | ||
| _userId = parsed; | ||
| logger.LogInformation("MCP stdio: using configured DefaultUserId {UserId}", _userId); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Guid.TryParse returns true for the all-zero GUID ("00000000-0000-0000-0000-000000000000"). With McpServer:DefaultUserId set to that value, this provider will treat it as resolved and GetCurrentUserId() will return Guid.Empty, which contradicts the interface contract and will later cause validation failures. Treat Guid.Empty as invalid (and fall back / throw) when reading the configured ID.
| var users = unitOfWork.Users.GetAllAsync().GetAwaiter().GetResult(); | ||
| var firstUser = users.FirstOrDefault(); |
There was a problem hiding this comment.
The constructor blocks on an async EF call (GetAllAsync().GetAwaiter().GetResult()) and loads all users just to pick the first. This can cause thread-pool starvation and unnecessary memory/IO, and it will run every time the scoped provider is created. Prefer a synchronous/async-safe query that only fetches one row (e.g., FirstOrDefaultAsync on the Users DbSet) or refactor to resolve the user ID in an async initialization step outside the constructor.
| var users = unitOfWork.Users.GetAllAsync().GetAwaiter().GetResult(); | |
| var firstUser = users.FirstOrDefault(); | |
| var firstUser = unitOfWork.Users.FirstOrDefaultAsync().GetAwaiter().GetResult(); |
| /// Resolves user identity for stdio MCP sessions. | ||
| /// In stdio mode the process runs under the OS user's identity, so no network | ||
| /// authentication is needed. We map to the configured default local user or, | ||
| /// if none is configured, to the first non-system user found in the database. |
There was a problem hiding this comment.
The XML comment says the fallback chooses the "first non-system user", but there’s no concept of a system user in User and the implementation selects the first user unconditionally. Either adjust the comment to match the behavior, or implement the intended filtering criterion (e.g., active users only, specific role, etc.).
| /// if none is configured, to the first non-system user found in the database. | |
| /// if none is configured, to the first user found in the database. |
| foreach (var boardDto in listResult.Value) | ||
| { | ||
| // Fetch board detail to get column and card counts. | ||
| // Acceptable for Phase 1: local SQLite, typically < 20 boards per user. | ||
| var detailResult = await _boardService.GetBoardDetailAsync(boardDto.Id, userId); | ||
| if (!detailResult.IsSuccess) | ||
| continue; // skip boards we can't read (race condition / deleted mid-request) | ||
|
|
||
| var detail = detailResult.Value; | ||
| var cardCount = detail.Columns.Sum(c => c.CardCount); | ||
|
|
||
| boardSummaries.Add(new | ||
| { | ||
| id = detail.Id, | ||
| name = detail.Name, | ||
| columnCount = detail.Columns.Count, | ||
| cardCount, | ||
| isArchived = detail.IsArchived, | ||
| updatedAt = detail.UpdatedAt | ||
| }); |
There was a problem hiding this comment.
ListBoardsAsync(... includeArchived: false) is documented to return only active boards, but if a board is archived between the list call and the subsequent detail fetch, this code will still emit it with isArchived: true. To keep the resource contract stable, re-check detail.IsArchived (or boardDto.IsArchived) and skip archived boards before adding them to the response.
| // Fetch board detail to get column and card counts. | ||
| // Acceptable for Phase 1: local SQLite, typically < 20 boards per user. | ||
| var detailResult = await _boardService.GetBoardDetailAsync(boardDto.Id, userId); | ||
| if (!detailResult.IsSuccess) | ||
| continue; // skip boards we can't read (race condition / deleted mid-request) | ||
|
|
||
| var detail = detailResult.Value; | ||
| var cardCount = detail.Columns.Sum(c => c.CardCount); | ||
|
|
||
| boardSummaries.Add(new | ||
| { | ||
| id = detail.Id, | ||
| name = detail.Name, | ||
| columnCount = detail.Columns.Count, | ||
| cardCount, | ||
| isArchived = detail.IsArchived, | ||
| updatedAt = detail.UpdatedAt |
There was a problem hiding this comment.
This implementation does an N+1 query pattern: list boards, then fetch full details (columns/cards) for each board to compute counts. Even on local SQLite this can become noticeably slow as board count grows. Consider adding a repository/service method that returns board summaries with columnCount and cardCount in one query/projection so the MCP resource can be served with O(1) DB round-trips.
| // Fetch board detail to get column and card counts. | |
| // Acceptable for Phase 1: local SQLite, typically < 20 boards per user. | |
| var detailResult = await _boardService.GetBoardDetailAsync(boardDto.Id, userId); | |
| if (!detailResult.IsSuccess) | |
| continue; // skip boards we can't read (race condition / deleted mid-request) | |
| var detail = detailResult.Value; | |
| var cardCount = detail.Columns.Sum(c => c.CardCount); | |
| boardSummaries.Add(new | |
| { | |
| id = detail.Id, | |
| name = detail.Name, | |
| columnCount = detail.Columns.Count, | |
| cardCount, | |
| isArchived = detail.IsArchived, | |
| updatedAt = detail.UpdatedAt | |
| // Use summary data directly from ListBoardsAsync to avoid N+1 detail queries. | |
| boardSummaries.Add(new | |
| { | |
| id = boardDto.Id, | |
| name = boardDto.Name, | |
| columnCount = boardDto.ColumnCount, | |
| cardCount = boardDto.CardCount, | |
| isArchived = boardDto.IsArchived, | |
| updatedAt = boardDto.UpdatedAt |
| var services = new ServiceCollection(); | ||
| services.AddLogging(); | ||
| services.AddDbContext<TaskdeckDbContext>(opts => opts.UseSqlite($"Data Source={_dbPath}")); | ||
| services.AddInfrastructure( | ||
| new ConfigurationBuilder() | ||
| .AddInMemoryCollection(new Dictionary<string, string?> | ||
| { | ||
| ["ConnectionStrings:DefaultConnection"] = $"Data Source={_dbPath}" | ||
| }) | ||
| .Build()); |
There was a problem hiding this comment.
The test service setup registers TaskdeckDbContext twice: once via AddDbContext and again inside services.AddInfrastructure(...) (which also calls AddDbContext). This duplication makes the test DI graph harder to reason about and can mask configuration differences (e.g., warning suppression configured by AddInfrastructure). Prefer relying on AddInfrastructure alone (since you already provide ConnectionStrings:DefaultConnection) and remove the extra AddDbContext call.
Convert interface to async (GetCurrentUserIdAsync, GetUserIdAsync) to eliminate sync-over-async blocking in the constructor. StdioUserContextProvider now lazily resolves the user ID on first async call, uses a targeted FirstOrDefaultAsync query instead of loading all users, rejects Guid.Empty as invalid, and fixes the misleading "non-system user" XML comment.
…dition Use await on GetCurrentUserIdAsync now that the interface is async. Add IsArchived re-check on detail result to guard against boards archived between the list and detail fetches. Clarify the N+1 detail-fetch comment to note BoardDto lacks column/card counts and this is Phase 1 acceptable.
The MCP stdio branch skipped database migrations, leaving the schema potentially out of date. Add Database.Migrate() using a scoped TaskdeckDbContext after Build() but before RunAsync(), mirroring the web mode behaviour in ConfigureTaskdeckPipeline.
Remove the explicit AddDbContext call that duplicated the registration already done by AddInfrastructure. Update all StdioUserContextProvider tests to use the async interface (GetCurrentUserIdAsync, GetUserIdAsync) and inject TaskdeckDbContext instead of IUnitOfWork. Add a new test verifying that Guid.Empty in config falls back to the database.
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Adversarial Review -- PR #664 (MCP-01 Minimal Prototype)
CI is all green. The earlier bot findings (sync-over-async, Guid.Empty, missing migrations, archived race condition) have all been addressed in follow-up commits. The remaining issues below are new or were not fully resolved.
1. Configured DefaultUserId is not validated against the database (Medium -- correctness)
File: backend/src/Taskdeck.Infrastructure/Mcp/StdioUserContextProvider.cs, lines 57-61
When McpServer:DefaultUserId is set in configuration, GetUserIdAsync returns it immediately without verifying the user exists in the database:
if (_configuredUserId.HasValue)
{
_resolvedUserId = _configuredUserId;
return _resolvedUserId;
}If the configured GUID is a typo, stale, or refers to a deleted user, the MCP server silently operates as that phantom user. BoardService.ListBoardsAsync will then return zero boards (authorization filters them out), with no error or diagnostic explaining why the user sees nothing. This is a silent misconfiguration failure.
Recommendation: After resolving the configured ID, verify it exists with a single _dbContext.Users.AnyAsync(u => u.Id == _configuredUserId) check. Throw or log a warning if the user is not found, so operators don't spend time debugging empty MCP responses.
2. Board resource tests do not exercise authorization scoping (Medium -- test gap)
File: backend/tests/Taskdeck.Api.Tests/McpBoardResourcesTests.cs, test constructor
The test DI setup registers BoardService without an IAuthorizationService:
services.AddScoped<BoardService>();This resolves the BoardService(IUnitOfWork) constructor, where _authorizationService is null. When _authorizationService is null, ListBoardsAsync(userId, ...) short-circuits and returns ALL boards regardless of the userId parameter:
// In BoardService.ListBoardsAsync:
if (_authorizationService is null)
{
var boards = await _unitOfWork.Boards.GetByIdsAsync(candidateBoardIds, ...);
return Result.Success(boards.Select(MapToDto)); // no user filtering
}This means the board resource tests are not actually testing user-scoped access. They pass because each test uses its own DB file with only one user, so there is no cross-user data to leak. But the production MCP path registers AuthorizationService (which is correct), and no test validates that this scoping works end-to-end through the MCP resource layer.
Recommendation: Add a test that creates boards owned by two different users, instantiates BoardResources with User A's context, and verifies User B's boards are excluded. Register AuthorizationService in the test DI to match the production MCP DI graph.
3. ListBoards throws InvalidOperationException on service failure -- MCP SDK error contract unclear (Low -- robustness)
File: backend/src/Taskdeck.Api/Mcp/BoardResources.cs, lines 53-54
if (!listResult.IsSuccess)
throw new InvalidOperationException($"MCP: failed to list boards: {listResult.ErrorMessage}");When the board service returns a failure Result, this throws an InvalidOperationException. The MCP SDK likely catches unhandled exceptions and converts them to JSON-RPC error responses, but the behavior is undocumented in the SDK XML docs. If the SDK does NOT catch this, an unhandled exception in a resource method could crash the stdio host process.
This is low severity because local SQLite failures are rare, and the MCP SDK likely does handle it. But it would be worth verifying (or adding a try-catch that returns a structured error JSON) to avoid a potential process crash on transient DB errors.
Summary
The PR is in good shape after the iterative fixes. Architecture boundaries are respected (IUserContextProvider in Application, StdioUserContextProvider in Infrastructure, BoardResources in Api). The MCP path correctly isolates from the web pipeline. Authorization is wired in production DI. The two medium findings (DefaultUserId validation and test authorization gap) are worth addressing before merge. The low finding is a hardening item that can be deferred.
When McpServer:DefaultUserId pointed to a GUID not present in the database, the MCP server silently operated as a phantom user and returned empty results. Now the configured ID is verified against the Users table; if the user does not exist a warning is logged and resolution falls back to the first local user (same as the no-config path).
Three new tests address adversarial review findings:
1. StdioUserContextProvider_WhenConfiguredIdDoesNotExistInDb_FallsBackToFirstUser
— verifies the new DB-existence check falls back correctly.
2. StdioUserContextProvider_WhenConfiguredIdDoesNotExistAndNoUsers_Throws
— verifies a clear error when no users exist at all.
3. BoardResources_ListBoards_OnlyReturnsCurrentUserBoards
— creates two users with separate boards and asserts each user
only sees their own boards through the MCP resource layer,
exercising the AuthorizationService ownership filter.
Add docs/MANUAL_VERIFICATION_CHECKLIST.md — a generated manual QA checklist (2026-04-01) covering PRs #665-#669 and recent merged PRs #568-#664. Provides sectioned, checklist-style verification steps, prerequisites, API and UI test cases (review cards, GDPR export/delete, metrics, GitHub OAuth, LLM tool-calling, inbox/notifications, accessibility, CI/dependency checks, etc.), known regressions to verify, and final automated smoke/test commands to run after manual verification.
Add multiple status updates and delivery notes: LLM tool-calling Phase 1 (#649) with CompleteWithToolsAsync, ToolCallingChatOrchestrator, read tool executors, SignalR ToolStatusEvent and 67 tests; MCP minimal prototype (#652/#664) with ModelContextProtocol v1.2.0, StdioUserContextProvider, BoardResources and --mcp startup flag plus integration tests; UX feedback wave 3 improvements to proposal card UX; GDPR data portability and account deletion features (DataExportService, AccountDeletionService, DataPortabilityController) with tests; Board metrics dashboard (BoardMetricsService, MetricsController, frontend route) and GitHub OAuth frontend integration. Also update direction/guardrails section to reflect tool-calling orchestrator, data portability, metrics, and MCP baseline.
Update roadmap and status docs to reflect recent Phase 1 deliveries and related status changes. IMPLEMENTATION_MASTERPLAN.md: mark LLM tool-calling Phase 1 (`#649`) and MCP server Phase 1 (`#652`/`#664`) as delivered, adjust dependency chains, add follow-up trackers, and annotate analytics/security/GitHub OAuth items as delivered where applicable (PRs referenced). STATUS.md: record LLM tool-calling and MCP Phase 1 completion (2026-04-01), add metrics to direction guardrails, close remaining UX items, and bump Phase 4 progress from 93% to 96%. Includes updated issue/PR references for traceability.
Bump TESTING_GUIDE.md to 2026-04-02 and update verified totals and re-verification dates (backend 2053, frontend 1496, combined 3549+). Add detailed coverage notes for recent PRs: LLM Tool-Calling (PR #669), MCP Server (PR #664), GDPR Data Portability/Account Deletion (PR #666), Board Metrics (PR #667), and GitHub OAuth frontend tests (PR #668), including tracking issues, test targets, and brief manual validation steps. Record addition of a developer-portal CI artifact step (PR #658) and expand planned quality expectations to include the new services and API/store coverage for OAuth and metrics.
Closes #652
Summary
Phase 1 of #648: minimal MCP server embedded in the API process.
ModelContextProtocolNuGet package (v1.2.0)IUserContextProviderinterface in Application layer +StdioUserContextProviderin Infrastructure for local stdio identity mappingBoardResourcesclass exposingtaskdeck://boardsresource (compact JSON: id, name, columnCount, cardCount, isArchived, updatedAt)--mcpstartup flag selects stdio host mode; web startup path unchangedmcp.example.jsonclient config for Claude Code / CursorUsage
Add to
.mcp.jsonin repo root:{ "mcpServers": { "taskdeck": { "command": "dotnet", "args": ["run", "--project", "backend/src/Taskdeck.Api/Taskdeck.Api.csproj", "--", "--mcp"] } } }Or use
mcp.example.jsonas a starting point. Then ask Claude: "What boards do I have?"Verification
dotnet build backend/Taskdeck.sln -c Release— clean (0 errors)dotnet test backend/Taskdeck.sln -c Release -m:1— all 1982 pass (7 new)