Implement user-directed MCP task mutation tools - #30
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds five intent-specific MCP task mutation tools with strict schemas, structured change metadata, before/after application results, error mapping, server registration, documentation, and unit/integration coverage. ChangesMCP mutation workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant McpServer
participant TaskApplication
participant ChangeMetadata
MCPClient->>McpServer: Invoke task mutation
McpServer->>TaskApplication: Call focused mutation
TaskApplication-->>McpServer: Return before and task
McpServer->>ChangeMetadata: Compute change descriptor
ChangeMetadata-->>McpServer: Return structured metadata
McpServer-->>MCPClient: Return versioned result or mapped error
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
7 tasks
Owner
Author
Luna follow-up implementation plan for review fixesAddress the review findings below in order. Keep the scope limited to PR #30 and issue #21. Do not introduce CLI work, skills, vendor integrations, auth, bulk mutation, restore/reopen, permanent deletion, or persistence redesign. GoalBring PR #30 fully in line with issue #21 by:
Task 1 — Complete the mutation schema test matrixFilesPrimarily:
Required testsAdd explicit tests for
Guidance
Acceptance criteria
Task 2 — Cover every editable field and clear operationFiles
Required testsProve successful editing of every allowed field:
Prove explicit clearing of every nullable editable field:
For each case verify:
Stable field ordering testAdd one test that changes multiple non-adjacent fields in an input order different from the contract order. Example input order: {
taskId,
sourceContext: 'new context',
title: 'new title',
priority: 'HIGH'
}Expected ['title', 'priority', 'sourceContext']Do not derive ordering from object insertion order or request order. Acceptance criteria
Task 3 — Complete no-op coverageFiles
Required testsAdd explicit successful no-op tests for:
Important archive clarificationThe current lifecycle implementation gives For every approved no-op verify:
Acceptance criteria
Task 4 — Complete error mapping coverageFiles
Required execution-error testsCover the following Relay structured errors after valid MCP schema validation:
Guidance
Acceptance criteria
Task 5 — Verify every tool calls one focused application operationFiles
Required testsUse a narrow
Verify handlers do not:
Acceptance criteria
Task 6 — Extract one shared MCP output-envelope schema helperCurrent problem
FilesSuggested shape:
ImplementationExtract one helper equivalent to: export function createMcpOutputSchema<T extends z.ZodType>(data: T) {
return z
.object({
schemaVersion: z.literal(CONTRACT_SCHEMA_VERSION),
data,
warnings: z.array(warningSchema),
})
.strict();
}Use the repository’s TypeScript/Zod conventions and preserve useful inferred types. Do not introduce a generic schema framework beyond this one shared helper. Constraints
Acceptance criteria
Task 7 — Simplify the duplicated application mutation APICurrent problem
edit(...)
editWithPrevious(...)
start(...)
startWithPrevious(...)This creates two application surfaces for each mutation and lets future adapters choose inconsistent methods. Required decisionPrefer one canonical mutation contract rather than permanent method pairs. Recommended optionMake focused mutation methods return a shared mutation result: interface TaskMutationResult {
readonly before: Task;
readonly task: Task;
}Apply this consistently to:
Then update existing internal callers deliberately. Where an existing caller only needs the resulting task, use Alternative allowed only with justificationKeep the current task-returning API and introduce a separate narrowly scoped mutation facade used by MCP. If this option is chosen:
Constraints
Likely files
Acceptance criteria
Task 8 — Preserve existing tools and protocol cleanlinessFiles
Required assertions
Acceptance criteria
Task 9 — Update documentation and PR evidenceFiles
Documentation checksEnsure
PR description updateReplace stale validation text claiming Use evidence from the final head commit only. Required implementation sequence
Do not weaken or delete tests merely to make the suite pass. Verification commandsRun all of the following from a clean working tree: pnpm test -- tests/unit/interfaces/mcp
pnpm test -- tests/integration/mcp-stdio.test.ts
pnpm format:check
pnpm lint
pnpm typecheck
pnpm test:coverage
pnpm build
pnpm validate:assets
pnpm verifyAlso inspect the final diff for accidental scope expansion: git diff --stat main...HEAD
git diff main...HEADFinal evidence required in the PRReport:
Human review checkpointsBefore marking the PR ready for re-review, manually verify:
Once all items are complete, leave a concise PR comment summarizing the fixes and request re-review. |
krishna916
marked this pull request as ready for review
July 27, 2026 07:42
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/interfaces/contracts/task-contract.ts (1)
163-187: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEncode change-metadata invariants in the result schemas.
The schemas currently accept contradictory payloads:
EDITEDwith an empty or duplicatefieldsarray,NO_CHANGEwith changed fields, and triageNO_CHANGE/TRIAGEDvalues inconsistent withfromandto. Add discriminated unions or refinements so the MCP contract enforces the invariants already produced byeditChangeandtriageChange.🤖 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 `@src/interfaces/contracts/task-contract.ts` around lines 163 - 187, Strengthen taskEditResultSchema and taskTriageResultSchema with discriminated unions or refinements that enforce change metadata: EDITED requires one or more unique fields, while NO_CHANGE requires an empty fields array; triage NO_CHANGE requires from and to to match, and TRIAGED requires them to differ. Preserve the existing taskDtoSchema and strict object contracts.src/interfaces/mcp/tools/task-start.ts (1)
18-26: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAll mutation handlers are
asyncbut callTaskApplicationsynchronously. The shared root cause is that each handler'stry/catch→toMcpErrormapping only holds while the application methods stay synchronous; if any becomes promise-returning, the rejection escapes thecatchand surfaces as an unmapped SDK error instead of a structured MCP error.
src/interfaces/mcp/tools/task-start.ts#L18-L26:awaittheapplication.start(...)result (harmless today, future-proof) or keep the handler synchronous.src/interfaces/mcp/tools/task-complete.ts#L20-L29: same change forapplication.complete(...).src/interfaces/mcp/tools/task-edit.ts#L20-L50: same change forapplication.edit(...).🤖 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 `@src/interfaces/mcp/tools/task-start.ts` around lines 18 - 26, The async mutation handlers must await application operations so promise rejections remain within their structured error mapping. In src/interfaces/mcp/tools/task-start.ts lines 18-26, await application.start; apply the same change to application.complete in src/interfaces/mcp/tools/task-complete.ts lines 20-29 and application.edit in src/interfaces/mcp/tools/task-edit.ts lines 20-50, preserving each handler’s existing toMcpError catch behavior.tests/unit/interfaces/mcp/mutation-tool-handlers.test.ts (2)
288-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
afterEditUpdatedAt.It's read after both the edit and the triage no-op, so the name understates what it covers.
🤖 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/mutation-tool-handlers.test.ts` around lines 288 - 289, Rename the afterEditUpdatedAt variable in the mutation-tool handler test to reflect that its value is read after both the edit and the triage no-op, and update the corresponding expectation reference.
40-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the hardened connect helper instead of a second, weaker copy.
create-mcp-server.test.tshas aconnectMcpthat guards against a failed connect (closing both sides, idempotentclose,allSettled). This copy leaks the server/transport ifclient.connectrejects, andclosewill reject rather than settle if either side throws — leaving a dangling server between tests. Extracting the existing helper into a shared test util removes the divergence.🤖 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/mutation-tool-handlers.test.ts` around lines 40 - 46, Replace the local connect helper in the mutation handler tests with the hardened connectMcp helper from create-mcp-server.test.ts, extracting it into a shared test utility if needed. Preserve failed-connect cleanup, idempotent close behavior, and allSettled-based shutdown, and update callers to use the shared helper.tests/unit/interfaces/mcp/create-mcp-server.test.ts (1)
76-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name promises "only", but
arrayContainingis a subset check.
toEqual(expect.arrayContaining([...]))passes as long as the five names are present; exclusivity rests entirely on the two hardcodednot.toContainchecks at Lines 104-105. A newly added generic mutation tool would not fail this test. Consider asserting the exact tool-name set (read + capture + health + the five) so any new tool must be explicitly acknowledged.💚 Proposed tightening
- expect([...byName.keys()]).toEqual( - expect.arrayContaining([ - 'task_edit', - 'task_triage', - 'task_start', - 'task_complete', - 'task_archive', - ]), - ); + expect([...byName.keys()].sort()).toEqual(EXPECTED_TOOL_NAMES);🤖 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/create-mcp-server.test.ts` around lines 76 - 105, Strengthen the tool-name assertion in the test “exposes only the five intent-specific user-directed mutation tools” by comparing the complete set of exposed names against the expected read, capture, health, and five intent-specific mutation tools. Replace the subset-only arrayContaining assertion and remove reliance on the separate task_update/task_set_status exclusions so any newly added tool requires explicit acknowledgment.src/interfaces/mcp/tools/task-edit.ts (1)
20-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the repeated clear/value ternaries into a helper.
The same four-line nested ternary is repeated for
description,priority,workspace, andsourceContext. A small helper keeps the payload construction readable and makes adding a new clearable field a one-liner.♻️ Proposed refactor
+function optionalField<T>( + value: T | undefined, + clear: boolean | undefined, +): Record<string, T | null> | Record<string, never> { + if (value !== undefined) return { value } as never; + return clear === true ? ({ value: null } as never) : ({} as never); +}Or inline, keyed by field name:
- ...(input.description === undefined - ? input.clearDescription === true - ? { description: null } - : {} - : { description: input.description }), + ...nullableEdit('description', input.description, input.clearDescription),🤖 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 `@src/interfaces/mcp/tools/task-edit.ts` around lines 20 - 43, Extract the repeated clear-or-value logic from the application.edit payload in the task-edit handler into a small reusable helper. Have it accept the field value and corresponding clear flag, returning the field update or an empty object, then use it for description, priority, workspace, and sourceContext while preserving the current undefined and null-clearing behavior.
🤖 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.
Nitpick comments:
In `@src/interfaces/contracts/task-contract.ts`:
- Around line 163-187: Strengthen taskEditResultSchema and
taskTriageResultSchema with discriminated unions or refinements that enforce
change metadata: EDITED requires one or more unique fields, while NO_CHANGE
requires an empty fields array; triage NO_CHANGE requires from and to to match,
and TRIAGED requires them to differ. Preserve the existing taskDtoSchema and
strict object contracts.
In `@src/interfaces/mcp/tools/task-edit.ts`:
- Around line 20-43: Extract the repeated clear-or-value logic from the
application.edit payload in the task-edit handler into a small reusable helper.
Have it accept the field value and corresponding clear flag, returning the field
update or an empty object, then use it for description, priority, workspace, and
sourceContext while preserving the current undefined and null-clearing behavior.
In `@src/interfaces/mcp/tools/task-start.ts`:
- Around line 18-26: The async mutation handlers must await application
operations so promise rejections remain within their structured error mapping.
In src/interfaces/mcp/tools/task-start.ts lines 18-26, await application.start;
apply the same change to application.complete in
src/interfaces/mcp/tools/task-complete.ts lines 20-29 and application.edit in
src/interfaces/mcp/tools/task-edit.ts lines 20-50, preserving each handler’s
existing toMcpError catch behavior.
In `@tests/unit/interfaces/mcp/create-mcp-server.test.ts`:
- Around line 76-105: Strengthen the tool-name assertion in the test “exposes
only the five intent-specific user-directed mutation tools” by comparing the
complete set of exposed names against the expected read, capture, health, and
five intent-specific mutation tools. Replace the subset-only arrayContaining
assertion and remove reliance on the separate task_update/task_set_status
exclusions so any newly added tool requires explicit acknowledgment.
In `@tests/unit/interfaces/mcp/mutation-tool-handlers.test.ts`:
- Around line 288-289: Rename the afterEditUpdatedAt variable in the
mutation-tool handler test to reflect that its value is read after both the edit
and the triage no-op, and update the corresponding expectation reference.
- Around line 40-46: Replace the local connect helper in the mutation handler
tests with the hardened connectMcp helper from create-mcp-server.test.ts,
extracting it into a shared test utility if needed. Preserve failed-connect
cleanup, idempotent close behavior, and allSettled-based shutdown, and update
callers to use the shared helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 36f8e85c-fbad-4375-bffe-e7cd714a4362
📒 Files selected for processing (26)
.gitignoredocs/mcp-tools.mddocs/superpowers/plans/2026-07-27-issue-21-mcp-mutations-review-fixes.mddocs/superpowers/plans/2026-07-27-issue-21-mcp-mutations.mdeslint.config.jssrc/application/tasks/task-application.tssrc/application/tasks/use-cases/edit-task.tssrc/application/tasks/use-cases/transition-task.tssrc/interfaces/contracts/task-contract.tssrc/interfaces/http/task-routes.tssrc/interfaces/mcp/create-mcp-server.tssrc/interfaces/mcp/mapping/change-metadata.tssrc/interfaces/mcp/mapping/mcp-errors.tssrc/interfaces/mcp/schemas/mcp-output-schema.tssrc/interfaces/mcp/schemas/mutation-tool-schemas.tssrc/interfaces/mcp/schemas/read-tool-schemas.tssrc/interfaces/mcp/tools/register-mutation-tools.tssrc/interfaces/mcp/tools/task-archive.tssrc/interfaces/mcp/tools/task-complete.tssrc/interfaces/mcp/tools/task-edit.tssrc/interfaces/mcp/tools/task-start.tssrc/interfaces/mcp/tools/task-triage.tstests/integration/mcp-stdio.test.tstests/unit/application/tasks/task-application.test.tstests/unit/interfaces/mcp/create-mcp-server.test.tstests/unit/interfaces/mcp/mutation-tool-handlers.test.ts
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
1 participant
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
Implements issue #21's five intent-specific MCP mutation tools:
task_edittask_triagetask_starttask_completetask_archiveThe handlers reuse the shared MCP envelopes, schemas, DTO mapping, error mapping, and focused task-application operations. Mutation results include deterministic change metadata and safe no-op handling. No generic mutation path or SQLite access is introduced.
Validation
tsc --build --noEmitpnpm verifyremains blocked by pre-existing formatting violations in unrelated files; all files in this change pass Prettier checks.Summary by CodeRabbit
NO_CHANGEconsistently without unintended persistence effects.