Implement MCP capture and read tools - #29
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughRelay now exposes five versioned MCP stdio task tools backed by ChangesMCP task tools
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant McpServer
participant TaskApplication
MCPClient->>McpServer: task_capture
McpServer->>TaskApplication: findSimilar(title, workspace, limit)
TaskApplication-->>McpServer: similar candidates
McpServer->>TaskApplication: create(task with AGENT provenance)
TaskApplication-->>McpServer: created task
McpServer-->>MCPClient: structured result with CREATED and warnings
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review verdict: Changes required before mergeThe implementation direction is good: the PR keeps MCP as an adapter over However, the PR does not yet satisfy issue #26. The main blockers are:
Please implement the following steps in order. Do not expand scope beyond these corrections. Step 1 — Register the real MCP input schemasProblemAll five tools currently register this schema: const rawToolInputSchema = z.object({}).passthrough();The handlers later call the strict contract schemas manually, so runtime validation rejects bad input, but MCP discovery tells clients that every object is accepted. This creates two different contracts:
Agents and MCP clients rely on the advertised schema for tool discovery and argument generation. The advertised schema must therefore be authoritative and match runtime validation. Required changesIn
In
inputSchema: taskListInputSchema
inputSchema: taskGetInputSchema
inputSchema: findSimilarInputSchema
inputSchema: sessionCapturesInputSchemaIn
inputSchema: agentCaptureInputSchema
TestsAdd a tool-discovery assertion that inspects the listed schemas and proves at minimum:
Do not merely test that invalid input is rejected after invocation. Test that discovery exposes the correct contract. Step 2 — Add focused test helpers before adding casesThe current MCP test file repeats server/client setup. The missing matrix will make it unwieldy. In async function createConnectedMcpTestServer(taskApplication: TaskApplication) {
// create server, linked transports and client
// connect both
// return { server, client, close }
}Also add a controllable fake or spy
Do not mock SQLite in MCP unit tests. MCP tests must use an injected application fake or the existing in-memory application fixture. Ensure every test closes the client/server transports in Step 3 — Prove
|
Decision resolved: implement Option 1The contract conflict has been resolved in favour of strict advertised schemas with SDK-native MCP Issue #26 has been updated with the authoritative behavior and test requirements. Issue #19 also has a contract-clarification comment documenting the distinction between protocol validation errors and Relay tool-execution errors. Terra implementation instructionPlease update this PR using the normal MCP SDK v1.29.0 Follow these steps:
Scope guardDo not add custom protocol dispatch, mutation tools, CLI behavior, skills, packaging, authentication, or unrelated refactoring in this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
tests/unit/interfaces/mcp/mcp-tool-contracts.test.ts (2)
420-451: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis matrix is fully subsumed by the per-tool matrix below.
Lines 453-503 run the identical six error/code pairs for
task_get(plus the other four tools) with the same envelope and leak assertions. Dropping this block removes ~30 duplicated lines without losing coverage. Extracting theerrorsarray to a module-level constant would also avoid re-declaring it inside the loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/interfaces/mcp/mcp-tool-contracts.test.ts` around lines 420 - 451, Remove the duplicated schema-valid execution error matrix beginning with the task_get test, since the per-tool matrix below already covers the same six error/code mappings and leak assertions. Preserve the broader per-tool coverage, and optionally hoist the shared errors array to a module-level constant to avoid redeclaring it inside the loop.
100-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert captured input after the call rather than inside the mock.
The tool handler wraps execution in
try/catch(src/interfaces/mcp/tools/task-capture.tslines 17-46), so an assertion thrown here is swallowed and re-surfaces as anINTERNAL_ERRORenvelope. The test still fails, but on a downstream schema-parse assertion with a misleading message. Recording the argument and asserting outside keeps failures readable.♻️ Proposed refactor
+ let captureInput: unknown; const application = taskApplication({ findSimilar: vi.fn(() => { calls.push('findSimilar'); return [task({ id: 'existing' })]; }), create: vi.fn((input) => { calls.push('create'); - expect(input).toMatchObject({ - creator: { type: 'AGENT', name: 'Codex' }, - sessionId: 'session-a', - sourceContext: 'issue-26', - }); - expect(input).not.toHaveProperty('status'); + captureInput = input; return task({ sourceContext: 'issue-26' }); }), });Then assert after
callTool:expect(captureInput).toMatchObject({ creator: { type: 'AGENT', name: 'Codex' }, sessionId: 'session-a', sourceContext: 'issue-26', }); expect(captureInput).not.toHaveProperty('status');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/interfaces/mcp/mcp-tool-contracts.test.ts` around lines 100 - 109, Update the create mock in the relevant test to record its input in a capture variable instead of asserting inside the mock. After callTool completes, assert the captured input matches the expected creator, sessionId, and sourceContext and does not contain status, preserving the existing task return behavior.tests/unit/interfaces/mcp/logger.test.ts (1)
5-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the stdio spies and prefer
mockClearovermockReset.Two issues with this setup:
- The spies are installed during collection and never restored, so
process.stdout.write/process.stderr.writestay stubbed for the rest of the worker's lifetime, potentially swallowing other output.mockReset()drops the() => trueimplementation, so any test added after the first would get a spy returningundefinedfromwrite.♻️ Proposed fix
-import { afterEach, describe, expect, it, vi } from 'vitest'; +import { afterAll, afterEach, describe, expect, it, vi } from 'vitest'; import { mcpLogger } from '../../../../src/interfaces/mcp/logger.js'; describe('mcpLogger', () => { const stderr = vi.spyOn(process.stderr, 'write').mockImplementation(() => true); const stdout = vi.spyOn(process.stdout, 'write').mockImplementation(() => true); afterEach(() => { - stderr.mockReset(); - stdout.mockReset(); + stderr.mockClear(); + stdout.mockClear(); + }); + + afterAll(() => { + stderr.mockRestore(); + stdout.mockRestore(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/interfaces/mcp/logger.test.ts` around lines 5 - 11, Update the stdio spy cleanup around stderr and stdout to call mockClear() after each test, preserving their mock implementations, and restore both spies so process.stdout.write and process.stderr.write are returned to their originals rather than remaining stubbed for the worker lifetime.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/mcp-tools.md`:
- Line 3: Update the Issue `#26` tool contract documentation to remove CONFLICT
and ARCHIVED_TASK from the documented execution error-code list. Keep
VALIDATION_ERROR, NOT_FOUND, STORAGE_ERROR, and INTERNAL_ERROR unchanged, and
preserve the surrounding input-schema and structured-response requirements.
In `@README.md`:
- Around line 40-47: Update the README’s outdated MCP statements: remove or
revise the claim that production MCP task handlers are downstream work, change
the mcp/ directory description to reflect shipped task behavior, and revise the
MVP scope statement to acknowledge production MCP task tools. Clarify the
tool-count wording near the MCP tool list so it identifies five task tools plus
the separate relay_health tool.
In `@src/application/tasks/use-cases/list-tasks.ts`:
- Around line 22-24: Update the workspace handling in the task list use case to
validate the input before calling trim(). Reject any non-string, non-null
workspace value with InvalidTaskRequestError, while preserving undefined, null,
and trimmed string normalization and the existing repository.list flow.
In `@tests/integration/mcp-stdio.test.ts`:
- Around line 11-13: Update the MCP integration setup around beforeAll so the
pnpm build:node operation has sufficient time to complete during Vitest setup.
Either configure a larger test.setupTimeout in vitest.config.ts or provide an
explicit timeout to this beforeAll callback, preserving the existing build
command and setup behavior.
- Around line 86-90: The session capture test should not assert task IDs in
UUID-derived order. Update the assertions around sessionA.structuredContent to
verify the expected count and that both first and second task IDs are present,
without requiring a specific ordering; alternatively, configure a deterministic
monotonic ID generator for this test.
In `@tests/unit/interfaces/mcp/create-mcp-server.test.ts`:
- Around line 36-54: Ensure every MCP test that connects a Client and server
closes both in all outcomes. Update the tests around createMcpServer,
client.connect, and server.connect (including the additional referenced cases)
to use try/finally or a shared idempotent close helper, invoking client.close()
and server.close() even when assertions or setup fail.
---
Nitpick comments:
In `@tests/unit/interfaces/mcp/logger.test.ts`:
- Around line 5-11: Update the stdio spy cleanup around stderr and stdout to
call mockClear() after each test, preserving their mock implementations, and
restore both spies so process.stdout.write and process.stderr.write are returned
to their originals rather than remaining stubbed for the worker lifetime.
In `@tests/unit/interfaces/mcp/mcp-tool-contracts.test.ts`:
- Around line 420-451: Remove the duplicated schema-valid execution error matrix
beginning with the task_get test, since the per-tool matrix below already covers
the same six error/code mappings and leak assertions. Preserve the broader
per-tool coverage, and optionally hoist the shared errors array to a
module-level constant to avoid redeclaring it inside the loop.
- Around line 100-109: Update the create mock in the relevant test to record its
input in a capture variable instead of asserting inside the mock. After callTool
completes, assert the captured input matches the expected creator, sessionId,
and sourceContext and does not contain status, preserving the existing task
return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3937ae9d-48b8-43dd-b94b-3b1a472b818a
📒 Files selected for processing (24)
README.mddocs/mcp-tools.mddocs/superpowers/plans/2026-07-26-issue-26-mcp-tools.mdsrc/application/tasks/task-repository.tssrc/application/tasks/use-cases/list-tasks.tssrc/database/tasks/sqlite-task-repository.tssrc/interfaces/contracts/task-contract.tssrc/interfaces/mcp/create-mcp-server.tssrc/interfaces/mcp/main.tssrc/interfaces/mcp/mapping/mcp-errors.tssrc/interfaces/mcp/mapping/mcp-result.tssrc/interfaces/mcp/mapping/task-mcp-dto.tssrc/interfaces/mcp/run-mcp-server.tssrc/interfaces/mcp/schemas/read-tool-schemas.tssrc/interfaces/mcp/tools/register-read-tools.tssrc/interfaces/mcp/tools/task-capture.tstests/integration/mcp-stdio.test.tstests/integration/task-repository.test.tstests/unit/application/tasks/task-application.test.tstests/unit/application/tasks/task-test-fixtures.tstests/unit/interfaces/mcp/create-mcp-server.test.tstests/unit/interfaces/mcp/logger.test.tstests/unit/interfaces/mcp/mcp-tool-contracts.test.tstests/unit/interfaces/mcp/run-mcp-server.test.ts
1 participant
Summary
Issue #26 acceptance coverage
Validation
No known unverified issue #26 acceptance criteria remain.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes