feat(mcp): MCP contract — REST↔MCP parity gate + capability tiers - #334
Conversation
…+ permission tiers Design artifact only (no implementation). Mirrors ADR-026's hand-authored+ CI-enforced approach for the MCP tool surface: a parity contract test, a read/write/destructive capability-tier model with server-side gating, and a sequenced write-tool build-out (closes #44). Open decisions flagged [DEFAULT — confirm] pending author review. Refs #44 #43 #99 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… gated off) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gistrar Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ema check Add the create_project tier to TOOL_TIERS (write) and extract registerProjectTools into project-tools.ts to keep tools.ts under the 400-line cap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… note Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Closes a gap in the parity gate (flagged by correctness + house-style review): INV-1 counted an op 'covered' even if its mapped tool name was misspelled or since-removed. INV-2b asserts every OP_TO_TOOL value is in declaredToolNames(), so a phantom mapping turns the build red — the drift ADR-044 exists to prevent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds MCP capability tiers, env-driven tier allowlisting, tier-gated tool registration, REST↔MCP contract mappings and parity tests, a new ChangesMCP capability tiers and contract enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant McpServer
participant ToolRegistrar
participant TOOL_TIERS
Client->>McpServer: start tools/list or tools/call
McpServer->>ToolRegistrar: register(name, config, handler)
ToolRegistrar->>TOOL_TIERS: lookup tool tier
TOOL_TIERS-->>ToolRegistrar: read / write / destructive or missing
alt tier missing
ToolRegistrar-->>McpServer: throw McpError
else tier allowed
ToolRegistrar->>McpServer: registerTool with merged annotations
else tier gated off
ToolRegistrar-->>ToolRegistrar: record declared name only
end
sequenceDiagram
participant CI
participant OpenAPI
participant ContractMap
participant McpServer
CI->>OpenAPI: load spec and extract operations
CI->>ContractMap: read OP_TO_TOOL, MCP_UNEXPOSED, MCP_NATIVE
CI->>McpServer: register tools and collect declared names
ContractMap-->>CI: parity inputs
CI-->>CI: assert coverage, no orphans, and tier presence
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/adr/045-mcp-capability-tiers.md (1)
124-126: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the
idempotentHintclaim.The capability helper currently stamps only
readOnlyHint/destructiveHint;idempotentHintis not part of the implementation contract insrc/mcp/capabilities.ts. Please narrow the ADR to the hints the code actually emits, or add the missing hint in code first.Suggested wording
- Tiers map to MCP tool annotations ... `idempotentHint` where applicable. + Tiers map to MCP tool annotations ... `readOnlyHint` / `destructiveHint`.🤖 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 `@docs/adr/045-mcp-capability-tiers.md` around lines 124 - 126, The ADR currently mentions an `idempotentHint` that is not emitted by the implementation. Update the wording in the capability tiers document to match `src/mcp/capabilities.ts`, referencing the `readOnlyHint` and `destructiveHint` behavior only, unless you first add `idempotentHint` support in the capability helper and its contract.Source: Linked repositories
🧹 Nitpick comments (3)
src/mcp/tool-registry.ts (1)
38-42: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTool-supplied annotations can silently override tier-derived safety hints.
{ ...tierAnnotations(tier), ...config.annotations }lets any tool's ownconfig.annotationsoverwritereadOnlyHint/destructiveHintcomputed from its tier. Since the whole point of the tier model is that clients can trust these hints as an authoritative safety signal, a future tool author passing custom annotations could unintentionally mask a destructive tool as read-only (or vice versa). Consider flipping the spread order so tier-derived hints always win, or explicitly disallowreadOnlyHint/destructiveHintinToolConfig.annotations.♻️ Proposed fix
server.registerTool( name, - { ...config, annotations: { ...tierAnnotations(tier), ...config.annotations } }, + { ...config, annotations: { ...config.annotations, ...tierAnnotations(tier) } }, handler );🤖 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/mcp/tool-registry.ts` around lines 38 - 42, The tool registration in registerTool currently allows config.annotations to override tier-derived safety hints, which can mask the intended readOnlyHint/destructiveHint from tierAnnotations(tier). Update the annotation merge in server.registerTool so the tier-based values are authoritative, either by making tierAnnotations(tier) win over config.annotations or by filtering readOnlyHint/destructiveHint out of ToolConfig.annotations before merging. Use registerTool, tierAnnotations, and ToolConfig.annotations as the key places to adjust.src/mcp/create-project-handler.ts (1)
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
ToolResult-shaped type instead of reusing the shared one.This locally redeclares the same
ToolError/ToolOk/ToolResultunion already defined for MCP tool responses (seesrc/mcp/tools.tsandtoolError()'s return typeToolErrorinhandlers.ts). Two independent declarations of the same shape will silently drift if the response contract changes.♻️ Suggested fix
-type ToolResult = - | { readonly isError: true; readonly content: { readonly type: 'text'; readonly text: string }[] } - | { readonly content: { readonly type: 'text'; readonly text: string }[] }; +import type { ToolResult } from './handlers.js';🤖 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/mcp/create-project-handler.ts` around lines 6 - 8, The local ToolResult union in create-project-handler duplicates the shared MCP tool response shape and should be removed in favor of the existing types. Update the handler to import and use the shared ToolError/ToolOk/ToolResult definitions from the MCP types module (and the ToolError return type used by toolError() in handlers.ts) so create-project-handler stays aligned with the single source of truth.src/mcp/create-project.integration.test.ts (1)
40-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnvalidated type assertion narrows OpenAPI operation shape.
opis cast to a shape assumingrequestBody.content['application/json'].schema.requiredexists; if the actual/projectsPOST operation lacks a JSON request body (or it's restructured), this throws a rawTypeErrorat.schema.requiredrather than failing with a clear assertion message. As per coding guidelines,**/*.{ts,tsx}should "avoid ... type assertions across module boundaries."Consider using
OperationObject.parse(rawOp)(already used invalidate-response.ts) or an optional-chain guard with an explicit assertion message instead of a raw cast.🤖 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/mcp/create-project.integration.test.ts` around lines 40 - 48, The `/projects` POST operation check in `create-project.integration.test.ts` uses an unsafe type assertion on `op`, which can cause a raw TypeError when the request body shape is missing or different. Update the test to validate the OpenAPI operation more safely, using the same `OperationObject.parse(rawOp)` approach used in `validate-response.ts` or an explicit optional-chain guard with a clear assertion message. Keep the loop that compares `required` against `CreateProjectBodySchema.shape`, but make the operation lookup and JSON schema access robust without cross-module type assertions.Source: Coding guidelines
🤖 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/superpowers/plans/2026-07-02-mcp-contract.md`:
- Around line 902-910: The roadmap table in the plan document is malformed and
triggering Markdownlint warnings, so it needs to be normalized for reliable
rendering. Rework the table around the affected roadmap block so each row has
consistent columns, and move the dense tier/ops notes out of the table if
needed. Use the existing roadmap rows for Wave/Domain/Tools/Ops burned down as
the anchor, and split the packed tool notes into bullets or separate lines where
they no longer break table formatting.
---
Outside diff comments:
In `@docs/adr/045-mcp-capability-tiers.md`:
- Around line 124-126: The ADR currently mentions an `idempotentHint` that is
not emitted by the implementation. Update the wording in the capability tiers
document to match `src/mcp/capabilities.ts`, referencing the `readOnlyHint` and
`destructiveHint` behavior only, unless you first add `idempotentHint` support
in the capability helper and its contract.
---
Nitpick comments:
In `@src/mcp/create-project-handler.ts`:
- Around line 6-8: The local ToolResult union in create-project-handler
duplicates the shared MCP tool response shape and should be removed in favor of
the existing types. Update the handler to import and use the shared
ToolError/ToolOk/ToolResult definitions from the MCP types module (and the
ToolError return type used by toolError() in handlers.ts) so
create-project-handler stays aligned with the single source of truth.
In `@src/mcp/create-project.integration.test.ts`:
- Around line 40-48: The `/projects` POST operation check in
`create-project.integration.test.ts` uses an unsafe type assertion on `op`,
which can cause a raw TypeError when the request body shape is missing or
different. Update the test to validate the OpenAPI operation more safely, using
the same `OperationObject.parse(rawOp)` approach used in `validate-response.ts`
or an explicit optional-chain guard with a clear assertion message. Keep the
loop that compares `required` against `CreateProjectBodySchema.shape`, but make
the operation lookup and JSON schema access robust without cross-module type
assertions.
In `@src/mcp/tool-registry.ts`:
- Around line 38-42: The tool registration in registerTool currently allows
config.annotations to override tier-derived safety hints, which can mask the
intended readOnlyHint/destructiveHint from tierAnnotations(tier). Update the
annotation merge in server.registerTool so the tier-based values are
authoritative, either by making tierAnnotations(tier) win over
config.annotations or by filtering readOnlyHint/destructiveHint out of
ToolConfig.annotations before merging. Use registerTool, tierAnnotations, and
ToolConfig.annotations as the key places to adjust.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 429ec42a-66e1-47d7-970e-829c9a4f4d41
📒 Files selected for processing (22)
.env.exampleAGENTS.mddocs/adr/044-mcp-contract-testing.mddocs/adr/045-mcp-capability-tiers.mddocs/superpowers/plans/2026-07-02-mcp-contract.mddocs/superpowers/specs/2026-07-02-mcp-contract-design.mdsrc/lib/env.test.tssrc/lib/env.tssrc/mcp/capabilities.test.tssrc/mcp/capabilities.tssrc/mcp/contract-map.tssrc/mcp/contract.integration.test.tssrc/mcp/create-project-handler.tssrc/mcp/create-project.integration.test.tssrc/mcp/onboarding-tools.tssrc/mcp/project-tools.tssrc/mcp/server.integration.test.tssrc/mcp/server.rate-limit.test.tssrc/mcp/server.tssrc/mcp/tool-registry.test.tssrc/mcp/tool-registry.tssrc/mcp/tools.ts
- tool-registry: tier-derived annotations now win over tool-supplied ones so readOnlyHint/destructiveHint stay authoritative (a tool can't mis-signal itself as safe) + regression test [CodeRabbit] - create-project-handler: reuse the exported ToolResult from handlers.ts instead of a third duplicate type; full shared-type sweep tracked in #331 [CodeRabbit] - design spec: drop the idempotentHint claim — tierAnnotations emits only readOnlyHint/destructiveHint [CodeRabbit] - plan doc: remove literal pipes inside the roadmap table code span (markdownlint MD056/MD038) [CodeRabbit] - add list_libraries read tool + map get /libraries -> it, so an MCP-only agent can discover the sourceLibraryIds that create_project requires [Codex P2] Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Review items addressed in commit 1120376. CodeRabbit
Codex (GPT-5.5 xhigh — second adversarial review, additional eyes)
|
#334 (merged) already claimed ADR-044 (mcp-contract-testing) and ADR-045 (mcp-capability-tiers), so the configurable-rate-limiting ADR moves to the next free number to avoid a duplicate. No code references the number. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#334 (merged) already claimed ADR-044 (mcp-contract-testing) and ADR-045 (mcp-capability-tiers), so the configurable-rate-limiting ADR moves to the next free number to avoid a duplicate. No code references the number. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…out (#335) * feat(rate-limit): env-seeded, runtime-mutable rate limits + demo opt-out Both express-rate-limit limiters (REST uploads, MCP) were hardcoded and could not be turned off. Make them configurable via env and read LIVE per request, so a future admin surface can retune or disable limiting at runtime with no restart. Secure by default (limiting ON); the web UI demo opts out via its own .env. - src/lib/env.ts: add DISABLE_RATE_LIMIT + RATE_LIMIT_{UPLOAD,MCP}_MAX + RATE_LIMIT_WINDOW_MS (Zod, secure defaults). config stays mutable (not frozen) so runtime changes are honoured. - limiters read config via per-request closures (skip/limit); only windowMs is fixed at construction (library limitation). REST keeps its test-mode skip; MCP intentionally does not (its rate-limit test exercises the live limiter, and integration suites raise the ceiling via the rateLimitMax option). - examples/web_ui_demo: .env.example ships DISABLE_RATE_LIMIT=true; the .sh/.ps1 launchers hand that file to the API via node --env-file-if-exists. Node's --env-file does not override already-exported vars, so the demo's PORT=3001 cannot clobber the API's PORT (verified on Node 26). - openapi.yaml: describe the affected routes' limits as configurable defaults. - docs/adr/044: record the decision, incl. why config is not frozen. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(rate-limit): demo opt-out survives a clean checkout; drop test type-assertion Review follow-up for PR #335. - examples/web_ui_demo launchers (.sh/.ps1): load the committed .env.example BEFORE .env via node --env-file-if-exists, so DISABLE_RATE_LIMIT=true reaches the API on a fresh clone where the gitignored .env does not exist yet (Codex [P2]). A user's real .env still overrides; exported PORT/DATABASE_URL/NODE_ENV still win over both files. - src/mcp/server.rate-limit.test.ts: replace `address() as { port }` cast with an isAddressInfo type guard (CodeRabbit nitpick; aligns with the no-assertions rule). - docs/adr/044: document the .env.example-first launcher loading. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(adr): renumber rate-limiting ADR 044 → 046 #334 (merged) already claimed ADR-044 (mcp-contract-testing) and ADR-045 (mcp-capability-tiers), so the configurable-rate-limiting ADR moves to the next free number to avoid a duplicate. No code references the number. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Why
openapi.yamlis CI-locked so it can't drift from the code (ADR-026), but the MCP tool surface had no such guarantee — adding a REST route wired up nothing on the MCP side, silently. And the MCP server was read-heavy (~22 read/parse/generate tools, zero write tools), so an AI agent could read SpecR but not do what a user can. This gives the agent surface the same no-drift guarantee as the API, plus a permission model so an agent can't perform admin/destructive actions by default.This is the foundation for the "first-class agent integration" ask: the demo chat (
examples/web_ui_demo) already derives its toolset from MCPtools/listat runtime, so every tool added here appears in the chat with zero per-tool wiring.What
src/mcp/contract.integration.test.ts) — the MCP analog of ADR-026's route↔spec test.contract-map.tsbinds every user-facing OpenAPI op to an MCP tool (OP_TO_TOOL), an explicit burn-down exemption (MCP_UNEXPOSED), or an MCP-native tool (MCP_NATIVE). Enforced invariants: INV-1 (every op mapped or exempt), INV-2/2b (no orphan tools, no phantom mappings), INV-3 (every tool tiered), INV-4 (write-tool schema parity), disjointness. A new REST route with no tool/exemption turns the build red.src/mcp/capabilities.ts,tool-registry.ts) — every tool declaresread/write/destructive(mapped to MCPreadOnlyHint/destructiveHint). A tier-gating registrar routes all registrations;MCP_ALLOWED_TIERS(defaultread,write) controls exposure — destructive/admin actions are gated off by default (gating by absence: not registered ⇒ not listed ⇒ not callable). Token-scoped tiers are deferred to feat(api): Phase 5f — authentication + multi-tenant (JWT, org isolation) #43 (auth); the hook point is in place.create_project(tierwrite), the canonical template over the shared service layer (validates with the existing Zod schema, never throws, returnstoolError).Generation-from-OpenAPI was rejected for the same reason ADR-026 rejected spec-from-code: it flattens the hand-tuned, LLM-facing tool descriptions. Design + plan:
docs/superpowers/{specs,plans}/2026-07-02-mcp-contract*.Remaining write-tool waves (projects/packages, paragraphs #44, spec lifecycle, merge, assignment, config CRUD) burn down
MCP_UNEXPOSEDin follow-up PRs, each following thecreate_projectrecipe.Testing
pnpm build— tsc)pnpm lint— eslint + tsc + prettier)pnpm test:integration— 781 passed / 0 failed; MCP surface 66/66; contract test 5/5)POST /mcp, confirmcreate_projectis callable under default tiers and no destructive tool is listedNotes for reviewers
get /healthis exempt; nothing mutating hidden in EXEMPT; no destructive op mis-tagged.MCP_UNEXPOSEDlist is the intended burn-down baseline, not a gap.🤖 Co-authored by Claude Opus 4.8. Refs #44, #43. (Does not close #44 — this is the foundation; write-tool waves follow.)
Summary by CodeRabbit
New Features
MCP_ALLOWED_TIERS(defaultread,write), preventing destructive tools from being exposed unless allowed.Documentation
Tests