Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package-lock.json
*.tsbuildinfo
.idea/
.superpowers/
.worktrees/
.vscode/
Thumbs.db
Desktop.ini
Expand Down
18 changes: 14 additions & 4 deletions docs/mcp-tools.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Relay MCP Tool Contracts

Issue #26 implements the five safe capture/read handlers from this version `1` contract. Tool discovery exposes strict input schemas: malformed request shapes (including unknown, forbidden, missing, or out-of-range fields) receive SDK-native MCP `InvalidParams`. Schema-valid tool execution returns structured `{ schemaVersion: 1, data, warnings }`; execution errors use `VALIDATION_ERROR`, `NOT_FOUND`, `STORAGE_ERROR`, or `INTERNAL_ERROR` without stack traces, SQLite details, secrets, or local paths. Compact text is a compatibility supplement, never a parsing requirement.
Issue #21 adds five intent-specific mutation handlers to the version `1` contract alongside the issue #26 capture/read handlers. Tool discovery exposes strict input schemas: malformed request shapes (including unknown, forbidden, missing, or out-of-range fields) receive SDK-native MCP `InvalidParams` (`-32602`) before application execution. Schema-valid tool execution returns structured `{ schemaVersion: 1, data, warnings }`; execution errors use stable Relay codes without stack traces, SQLite details, secrets, or local paths. Compact text is a compatibility supplement, never a parsing requirement.

## `task_capture`

Expand All @@ -26,24 +26,34 @@ Input: required valid `sessionId` and `limit` from 1 through 100. Output: `{ ses

Input: task ID plus one or more editable task fields or explicit clear flags. MCP `null` is rejected; explicit `clear*` flags are the only clear operation, and a value cannot accompany its matching flag. Output: `{ task, change }`, including `NO_CHANGE` for an approved no-op. `sessionId`, provenance, status, and lifecycle timestamps are never editable.

`change` is `{ action: "EDITED" | "NO_CHANGE", fields }`. `fields` lists only persisted editable fields that changed, in stable order: `title`, `description`, `priority`, `workspace`, `sourceContext`.

The editable fields are `title`, `description`, `priority`, `workspace`, and `sourceContext`. Nullable fields are cleared only with `clearDescription`, `clearPriority`, `clearWorkspace`, or `clearSourceContext`; direct `null` and value-plus-clear requests are invalid params. A normalized value that is already persisted returns `change: { action: "NO_CHANGE", fields: [] }` without a persistence write.

## `task_triage`

Input: task ID and target `INBOX`, `ACTIVE`, or `BACKLOG`. Output: `{ task, change }`. `IN_PROGRESS`, `DONE`, and `ARCHIVED` have their own intent-specific tools.

`change` is `{ action: "TRIAGED" | "NO_CHANGE", from, to }`; `from` and `to` are the persisted source and result statuses.

## `task_start`

Input: task ID. Output: `{ task, change }`. It performs only the focused start lifecycle operation.
Input: task ID. Output: `{ task, change: { action: "STARTED" | "NO_CHANGE" } }`. It performs only the focused start lifecycle operation.

## `task_complete`

Input: task ID. Output: `{ task, change }`. It performs only the focused completion lifecycle operation.
Input: task ID. Output: `{ task, change: { action: "COMPLETED" | "NO_CHANGE" } }`. It performs only the focused completion lifecycle operation.

## `task_archive`

Input: task ID. Output: `{ task, change }`. It performs only the focused archive lifecycle operation.
Input: task ID. Output: `{ task, change: { action: "ARCHIVED" | "NO_CHANGE" } }`. It performs only the focused archive lifecycle operation.

All lifecycle mutations return `change.action = "NO_CHANGE"` when the focused operation leaves the persisted task unchanged, without changing timestamps or writing the repository. Invalid lifecycle transitions use `CONFLICT`; attempting a restricted mutation of an archived task uses `ARCHIVED_TASK`. Other schema-valid execution failures map to `VALIDATION_ERROR`, `NOT_FOUND`, `STORAGE_ERROR`, or generic `INTERNAL_ERROR`; internal messages are never exposed.

## Mutation safety and versioning

Invoke `task_edit`, `task_triage`, `task_start`, `task_complete`, and `task_archive` only after explicit user direction in the active conversation. Relay validates data and lifecycle legality but cannot authenticate conversational intent under the OS-user trust boundary, so it intentionally has no fake `confirmed`, `requestedBy`, or copied-user-text field.

The MCP SDK owns schema-invalid input handling (`InvalidParams`, `-32602`). Relay owns errors after a schema-valid request reaches the application and returns the structured versioned error envelope. Each mutation invokes one focused application operation; MCP does not read SQLite or implement lifecycle legality.

There is no `task_update`, `task_set_status`, generic CRUD mutation, or unrestricted lifecycle command. Tool names are not version-prefixed; a breaking change requires a new integer schema version and an explicit compatibility decision. Later issues implement every handler through shared application services; MCP never reads SQLite directly.
Original file line number Diff line number Diff line change
@@ -0,0 +1,198 @@
# Issue #21 MCP Mutation Review Fixes Implementation Plan

> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.

**Goal:** Bring PR #30 in line with the detailed review follow-up at issuecomment-5087087958 while keeping the scope limited to issue #21's five intent-specific MCP mutation tools.

**Architecture:** Keep MCP adapters responsible for strict input validation, one focused application-operation invocation, and result/error mapping. The application layer remains authoritative for lifecycle legality and returns atomic before/after mutation results so metadata does not require a second read.

**Tech Stack:** TypeScript, Zod, `@modelcontextprotocol/sdk`, Vitest, pnpm, MCP stdio transport.

## Global Constraints

- Do not add CLI work, skills, vendor integrations, auth, bulk mutation, restore/reopen, permanent deletion, or persistence redesign.
- Preserve public MCP output shapes and contract schema version.
- `task_edit`, `task_triage`, `task_start`, `task_complete`, and `task_archive` remain the only mutation tools.
- Do not weaken or delete tests to make the suite pass.
- Schema-invalid requests must remain SDK-native invalid-params failures; valid requests that fail during execution must use Relay structured errors.

---

### Task 1: Complete the mutation schema test matrix

**Files:**

- Modify: `tests/unit/interfaces/mcp/create-mcp-server.test.ts`
- Add a focused schema test under `tests/unit/interfaces/mcp/` only if the server test becomes unreadable.

**Required checks:**

- [x] `task_edit` rejects `{ taskId }` without an editable field.
- [x] `task_edit` rejects unknown fields and each forbidden/immutable field: `status`, `createdByType`, `createdByName`, `sessionId`, `createdAt`, `updatedAt`, `startedAt`, `completedAt`, `archivedAt`, `confirmed`, and `requestedBy`.
- [x] `task_edit` rejects direct `null` for nullable fields where clearing requires a clear flag.
- [x] `task_edit` rejects value-plus-clear conflicts for `description`, `priority`, `workspace`, and `sourceContext`.
- [x] `task_triage` accepts only `INBOX`, `ACTIVE`, and `BACKLOG`, and rejects `IN_PROGRESS`, `DONE`, and `ARCHIVED`.
- [x] `task_start`, `task_complete`, and `task_archive` reject unknown fields.
- [x] Schema-invalid calls are verified through actual `client.callTool(...)` behavior and remain SDK-native invalid-params responses.

### Task 2: Cover every editable field, clear operation, and stable ordering

**Files:**

- Modify: `tests/unit/interfaces/mcp/create-mcp-server.test.ts`

**Required checks:**

- [x] Test successful edits for `title`, `description`, `priority`, `workspace`, and `sourceContext`.
- [x] Test successful clears for `clearDescription`, `clearPriority`, `clearWorkspace`, and `clearSourceContext`.
- [x] For each mutation, assert the complete returned task, `change.action === 'EDITED'`, and only the persisted field in `change.fields`.
- [x] Fetch the task afterward with `task_get` and compare it with the mutation result.
- [x] Change `sourceContext`, `title`, and `priority` in request order and assert stable metadata order `['title', 'priority', 'sourceContext']`.

### Task 3: Complete approved no-op coverage

**Files:**

- Modify: `tests/unit/interfaces/mcp/create-mcp-server.test.ts`
- Inspect: domain lifecycle implementation and existing lifecycle tests before deciding archive behavior.

**Required checks:**

- [x] `task_edit` setting a normalized current value returns success with `NO_CHANGE` and `fields: []`.
- [x] `task_edit` clearing an already-null field returns success with `NO_CHANGE` and `fields: []`.
- [x] `task_triage` to the task's current `INBOX`, `ACTIVE`, or `BACKLOG` status returns `NO_CHANGE` with equal `from` and `to`.
- [x] Starting an `IN_PROGRESS` task and completing a `DONE` task return successful `NO_CHANGE` results.
- [x] Verify authoritative archive behavior; preserve the current same-target archived no-op if domain tests confirm it.
- [x] Assert timestamps remain unchanged and no repository update occurs for no-ops.

### Task 4: Complete structured execution-error mapping coverage

**Files:**

- Modify: `tests/unit/interfaces/mcp/create-mcp-server.test.ts`
- Modify fixtures under `tests/unit/application/tasks/` only when a controlled repository/application double is needed.

**Required checks:**

- [x] Add a schema-valid request producing `VALIDATION_ERROR`.
- [x] Add a missing-task mutation producing `NOT_FOUND`.
- [x] Add invalid lifecycle transitions producing `CONFLICT`, including more than one intent where useful.
- [x] Add archived edit, triage, start, and complete cases producing `ARCHIVED_TASK`; test archive as either restricted or no-op according to the authoritative domain contract.
- [x] Add a repository update failure producing `STORAGE_ERROR` without leaking SQL, paths, internal messages, stack, or causes.
- [x] Add an unexpected controlled exception producing generic `INTERNAL_ERROR` without exposing the original exception message.

### Task 5: Prove each handler invokes exactly one focused application operation

**Files:**

- Modify: `tests/unit/interfaces/mcp/` focused registration/handler tests.
- Modify: `src/interfaces/mcp/tools/*.ts` only if a wiring defect is found.

**Required checks:**

- [x] Use a narrow `TaskApplication` fake/spy to prove `task_edit` calls only edit.
- [x] Prove `task_triage` maps `INBOX` to move-to-inbox, `ACTIVE` to activate, and `BACKLOG` to move-to-backlog.
- [x] Prove `task_start`, `task_complete`, and `task_archive` each call only their focused operation.
- [x] Prove handlers do not access repositories, fetch separately before mutation, call multiple lifecycle methods, or implement lifecycle legality.

### Task 6: Extract one shared MCP output-envelope schema helper

**Files:**

- Create: `src/interfaces/mcp/schemas/mcp-output-schema.ts`
- Modify: `src/interfaces/mcp/schemas/read-tool-schemas.ts`
- Modify: `src/interfaces/mcp/schemas/mutation-tool-schemas.ts`
- Update import-only tests if required.

**Required implementation:**

- [x] Add `createMcpOutputSchema<T extends z.ZodType>(data: T)` that builds the strict `{ schemaVersion, data, warnings }` envelope using the existing contract version and warning schema.
- [x] Make read/capture and mutation schemas use this one helper.
- [x] Preserve public output shapes, contract version, read/capture behavior, and inferred types.

### Task 7: Simplify the duplicated application mutation API

**Files:**

- Modify: `src/application/tasks/task-application.ts`
- Modify: `src/application/tasks/use-cases/edit-task.ts`
- Modify: `src/application/tasks/use-cases/transition-task.ts`
- Modify direct callers and affected tests only.

**Required implementation:**

- [x] Choose one canonical mutation API returning `{ before: Task; task: Task }` for edit, move-to-inbox, activate, move-to-backlog, start, complete, and archive.
- [x] Update existing callers that need only the result to use `.task` at their boundary.
- [x] Preserve atomic before/result capture without duplicate repository reads.
- [x] Keep lifecycle rules in the domain/application layer and avoid unrelated refactoring.
- [x] Add or update tests proving existing HTTP/UI behavior remains unchanged and no mutation rules are duplicated.

### Task 8: Preserve existing tools and stdio protocol cleanliness

**Files:**

- Modify: `tests/integration/mcp-stdio.test.ts`
- Modify existing MCP unit tests only as needed.

**Required checks:**

- [x] Existing tools remain discoverable and unchanged: `task_capture`, `task_list`, `task_get`, `task_find_similar`, and `session_captures_list`.
- [x] Exactly the five approved mutation tools are present.
- [x] Generic tools such as `task_update`, `task_set_status`, and unrestricted mutation variants remain absent.
- [x] Built stdio integration performs capture, edit, triage or start, complete or archive, and final readback.
- [x] Assert stdout contains only MCP protocol output and no logs, stack traces, SQL details, or debug output.

### Task 9: Update documentation and PR evidence

**Files:**

- Modify: `docs/mcp-tools.md`
- Update PR #30 description only after final verification evidence is available.

**Required checks:**

- [x] Document exact input names (`taskId`, `target`, and clear flags), editable fields, direct-null rejection, clear behavior, deterministic field order, triage restrictions, result shapes, no-ops, archived/conflict behavior, explicit-user-direction precondition, absence of fake confirmation fields, and SDK-invalid-params versus Relay execution-error behavior.
- [x] Run the full verification list and record command-by-command evidence from the final local code state; repository-wide `pnpm verify` remains blocked before later stages by unrelated pre-existing formatter failures.
- [x] Inspect `git diff --stat main...HEAD` and `git diff main...HEAD` for accidental scope expansion.
- [x] Record local test counts, stdout cleanliness, absence of generic mutation tools, and unchanged read/capture tools.
- [ ] Update PR #30's description with final evidence after the verified local changes are intentionally committed and pushed.

## Final verification evidence

- `pnpm test`: PASS — 25 files, 371 tests.
- `pnpm test:coverage`: PASS — 88.76% statements, 81.34% branches, 88.99% functions, 90.92% lines.
- `pnpm typecheck`: PASS.
- Changed-file Prettier check: PASS.
- Changed-file ESLint check: PASS.
- `pnpm build`: PASS.
- `pnpm validate:assets`: PASS.
- `pnpm verify`: BLOCKED at `pnpm format:check` by 16 unrelated pre-existing files outside this follow-up's changed set; no unrelated formatting sweep was applied.
- `pnpm audit --audit-level high`: BLOCKED by the environment's denied npm registry network request; no audit result was produced.
- Remote PR #30 CI at existing head `2732e70f315ead1a9678369e031b290d37b75d2c`: `verify` and CodeRabbit SUCCESS. Local changes are not yet represented by that remote SHA, so the PR description and re-review request remain intentionally untouched.

## Verification commands

```bash
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 verify
```

## Human review checkpoints

- [ ] Every mutation schema rejects unknown, immutable, provenance, session, and timestamp fields.
- [ ] `task_triage` cannot reach `IN_PROGRESS`, `DONE`, or `ARCHIVED`.
- [ ] No fake conversational confirmation field exists.
- [ ] Every handler calls one focused application operation.
- [ ] Before/after metadata is captured without duplicate reads.
- [ ] Approved no-ops return success with deterministic metadata and no unnecessary writes.
- [ ] Archived restrictions match the domain lifecycle contract.
- [ ] Read/capture tools and envelopes have no unintended changes.
- [ ] MCP stdout is protocol-clean.
- [ ] `pnpm verify` passes on the final commit.
33 changes: 33 additions & 0 deletions docs/superpowers/plans/2026-07-27-issue-21-mcp-mutations.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# Issue #21 MCP Mutation Tools — Implementation Plan

**Goal:** Add the five user-directed, intent-specific MCP mutation tools while reusing the shared runtime, MCP envelopes, error mapping, DTO mapping, and task application service established by issue #26.

**Architecture:** The MCP interface receives strict Zod-validated input, invokes exactly one focused `TaskApplication` operation, and returns the existing versioned MCP success envelope with a full task DTO and deterministic change metadata. The adapter never reads SQLite or accepts generic status/provenance/session/timestamp mutation.

## Files and responsibilities

- `src/interfaces/contracts/task-contract.ts`: define mutation-specific input and output contract shapes, including clear directives and detailed change metadata.
- `src/interfaces/mcp/schemas/mutation-tool-schemas.ts`: re-export mutation schemas and wrap result schemas in the standard MCP output envelope.
- `src/interfaces/mcp/mapping/change-metadata.ts`: compare pre/post tasks in a stable field order and produce edit/triage/lifecycle `NO_CHANGE` metadata.
- `src/interfaces/mcp/tools/task-{edit,triage,start,complete,archive}.ts`: one focused handler per intent.
- `src/interfaces/mcp/tools/register-mutation-tools.ts`: compose those five registrations.
- `src/interfaces/mcp/create-mcp-server.ts`: add mutation registration next to the #26 read/capture registrations.
- `src/interfaces/mcp/mapping/mcp-errors.ts`: distinguish domain transition and archived-task errors with the stable #19 MCP codes.
- `tests/unit/interfaces/mcp/create-mcp-server.test.ts`: prove discovery, strict schemas, outputs, no-ops, lifecycle/error behavior, and absence of generic mutation capabilities.
- `tests/integration/mcp-stdio.test.ts`: prove mutation works in the built stdio process while stdout remains protocol-clean.
- `docs/mcp-tools.md`: document explicit-user-direction precondition and every mutation contract.

## Execution sequence

1. Add strict-schema and pure metadata tests; run them red.
2. Add shared mutation schemas, deterministic metadata, and stable conflict/archive error mapping.
3. Implement edit and restricted triage with success, clear, no-op, and error tests.
4. Implement start, complete, and archive as separate focused handlers; prove generic mutation tools are absent.
5. Extend built stdio coverage, update documentation, then run all required checks including `pnpm verify`.

## Review checklist

- Every mutation calls one focused `TaskApplication` method; none touches SQLite.
- No input accepts `confirmed`, authorization prose, provenance, session, status outside triage's three targets, or timestamps.
- All no-ops return `NO_CHANGE` successfully with deterministic metadata.
- Existing read/capture tools remain registered and unchanged.
2 changes: 1 addition & 1 deletion eslint.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import reactHooks from 'eslint-plugin-react-hooks';
/** @type {import('eslint').Linter.Config[]} */
const config = [
{
ignores: ['dist/**', 'coverage/**', 'node_modules/**'],
ignores: ['dist/**', 'coverage/**', 'node_modules/**', '.worktrees/**'],
},
{
files: ['**/*.ts', '**/*.tsx', '**/*.js'],
Expand Down
Loading
Loading