feat(mcp): add spry mcp server, Agent Skills, and serve integration (#188, #189, #190)#198
feat(mcp): add spry mcp server, Agent Skills, and serve integration (#188, #189, #190)#198medz wants to merge 21 commits into
Conversation
Add a first-party AI ecosystem for Spry: - spry-docs skill (.claude/skills/spry-docs.md): authoritative Spry guidance for AI coding agents covering project structure, routing, middleware, error handling, configuration, build targets, and validation workflows. - spry mcp server (lib/src/mcp/): local MCP server over stdio exposing 8 inspection tools — get_project_info, get_config, list_routes, list_middleware, list_error_handlers, explain_route, get_openapi_status, and get_client_status. Accessible via 'spry mcp' CLI command. - spry-debugging skill (.claude/skills/spry-debugging.md): debugging playbooks with MCP-first philosophy covering 404s, route mismatches, middleware ordering, error chains, OpenAPI, client gen, and build errors. - Spry plugin (.spry-plugin/): bundles both skills and the MCP server config into a single Claude Code / Codex installable package. - Fix roux version in AGENTS.md (0.5.x → 1.0.x). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Replace .claude/skills/*.md with skills/<name>/SKILL.md directories per the Agent Skills specification (agentskills.io) - Add 34 new tests: MCP protocol (10), MCP tools/path matching (24) - Make path matching utilities public for testability - Update AGENTS.md and plugin docs to reflect new structure Total: 325 tests, zero analyzer issues.
Replace .spry-plugin/ with per-platform marketplace directories: - .claude-plugin/plugin.json — Claude Code plugin manifest - .claude-plugin/.mcp.json — MCP server configuration - .agents/plugins/ — Agent Skills marketplace (TBD) - .codex-plugin/ — Codex plugin configuration (TBD) Skills remain in skills/ following the agentskills.io specification.
Add .codex-plugin/plugin.json following the Codex plugin format with full interface metadata including display name, descriptions, category, capabilities, default prompts, and brand configuration.
Remove .claude-plugin/, .codex-plugin/, .agents/, and .mcp.json. Keep skills/ (agentskills.io spec), MCP server code, and tests.
Both skills referenced files that didn't exist: - spry-docs: routing.md, middleware.md, handlers.md, config.md - spry-debugging: route-debugging.md All reference files now exist with detailed content covering routing syntax, middleware patterns, handler APIs, config options, and advanced route debugging. Follows agentskills.io progressive disclosure pattern — SKILL.md < 500 lines, references on demand.
- Add JsonRpcError type matching JSON-RPC 2.0 error response spec - Add protocol version constants (jsonRpcVersion, latestProtocolVersion) - Add instructions field to initialize response for LLM hints - Handle notifications/initialized and notifications/cancelled - Return tool errors as isError content (not protocol errors) - Add mcp bool to defineSpryConfig and BuildConfig - Show MCP command hint in spry serve output when mcp is enabled - Add ping method support
When mcp is enabled in config, spry serve creates a dedicated Spry instance alongside the main dev server. The MCP instance handles JSON-RPC over HTTP using Spry's own server — no generator changes, no app.dart modifications. - Add McpConfig extension type (enable, port) to config API - serve.dart: scan, create Spry instance, serve on MCP port - Clean up MCP runtime on restart via Runtime.close()
pathMatches and extractParams now handle both: - Spry source syntax: [id], [id=\d+], [...slug] - roux normalized paths: :id, :id(regex), *slug, /**
✅ Deploy Preview for dart-spry canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR implements a local MCP server and protocol, adds an MCP tool surface and path/middleware introspection, wires a standalone ChangesMCP server and protocol
MCP integration into spry serve
Documentation and AI agent guidance
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a56686b18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| final nextBuildSession = await _buildAndStart( | ||
| config, | ||
| out: out, | ||
| err: err, | ||
| processRunner: processRunner, | ||
| processStarter: processStarter, | ||
| installBun: installBun, | ||
| ); |
There was a problem hiding this comment.
On the non-hotswap restart path, _tryBuild above has already completed _prepareServeBuild and produced nextBuildPlan; calling _buildAndStart here runs _prepareServeBuild a second time after the old process is closed. For normal spry serve restarts this doubles every rebuild and, if the second build fails or has side effects, leaves the dev server stopped even though the first rebuild succeeded.
Useful? React with 👍 / 👎.
| // roux catch-all: `*name` matches remaining segments. | ||
| if (seg.startsWith('*') && !seg.startsWith('**')) { | ||
| return true; |
There was a problem hiding this comment.
For a route such as routes/[...slug].dart, the scanner stores the normalized path as **:slug, not *slug. This branch explicitly excludes segments starting with **, and the earlier exact seg == '**' check also misses **:slug, so spry.explain_route reports no match for named catch-all routes that the actual router handles.
Useful? React with 👍 / 👎.
| bool pathIsPrefix(String prefix, String path) { | ||
| if (prefix == '/**' || prefix == '/*') return true; | ||
| final normalizedPrefix = prefix.endsWith('/') ? prefix : '$prefix/'; | ||
| final normalizedPath = path.endsWith('/') ? path : '$path/'; | ||
| return normalizedPath.startsWith(normalizedPrefix); |
There was a problem hiding this comment.
When scoped middleware or error handlers live under a dynamic directory, e.g. routes/users/[id]/_middleware.dart, the scanner records the scope as /users/:id. This literal startsWith check does not treat :id as a segment wildcard, so spry.explain_route omits middleware/error handlers that the runtime includes through the roux router for requests like /users/42.
Useful? React with 👍 / 👎.
| final mw = entry.middleware!; | ||
| if (pathIsPrefix(mw.path, path)) { | ||
| middleware.add({ |
There was a problem hiding this comment.
For method-specific middleware such as _middleware.post.dart, mw.method is POST, but this condition only checks the path. As a result, explaining GET for the same path includes middleware that the runtime does not run, because Spry.fetch filters middleware by the request method with findAll(..., method: method.value, includeAny: true).
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
lib/src/builder/config.dart (1)
431-441: ⚡ Quick winConsider using lenient Map pattern for consistency.
The strict
is Map<String, Object?>check (line 437) differs from the pattern used in_openApiConfig(line 268) and_clientConfig(line 291), which both useMap()pattern matching followed byMap<String, Object?>.from(value)conversion. While the current implementation works for JSON-decoded values, the lenient pattern is more robust and consistent.♻️ Align with existing pattern
McpConfig? _readMcpConfig(Map<String, Object?> source, String key) { if (!source.containsKey(key)) { return null; } final value = source[key]; if (value == null) return null; - if (value is Map<String, Object?>) return McpConfig.fromJson(value); - throw LoadConfigException( - 'Invalid `$key`: expected a JSON object, got ${_describeValue(value)}.', - ); + return switch (value) { + Map() => McpConfig.fromJson(Map<String, Object?>.from(value)), + _ => throw LoadConfigException( + 'Invalid `$key`: expected a JSON object, got ${_describeValue(value)}.', + ), + }; }🤖 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 `@lib/src/builder/config.dart` around lines 431 - 441, The `_readMcpConfig` function currently uses a strict `is Map<String, Object?>` check which is inconsistent with `_openApiConfig` and `_clientConfig`; update `_readMcpConfig` to use the lenient `Map()` pattern match and then convert with `Map<String, Object?>.from(value)` before calling `McpConfig.fromJson`, and keep the existing `LoadConfigException` using `_describeValue` for non-map values so behavior remains consistent and robust for JSON-decoded inputs.test/mcp_tools_test.dart (1)
59-73: ⚡ Quick winMissing test case for path prefix bug.
The
pathIsPrefixtests don't cover the edge case where a prefix is a string-prefix but not a path-component-prefix. Add a test to verify that/adminis not considered a prefix of/admins.🧪 Suggested test case
test('prefix must match on path boundaries', () { expect(pathIsPrefix('/admin', '/admins'), isFalse); expect(pathIsPrefix('/api', '/api-v2'), isFalse); });🤖 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 `@test/mcp_tools_test.dart` around lines 59 - 73, Add a unit test covering the edge case where a prefix is a string-prefix but not a path-component-prefix: in the existing pathIsPrefix tests (group 'pathIsPrefix' in test/mcp_tools_test.dart) add a test named like 'prefix must match on path boundaries' that asserts pathIsPrefix('/admin', '/admins') isFalse and pathIsPrefix('/api', '/api-v2') isFalse so the function pathIsPrefix is verified to require path-boundary matching rather than simple substring matching.lib/src/mcp/mcp_protocol.dart (2)
43-48: ⚡ Quick winUse null-aware syntax instead of manual null check.
As per coding guidelines, prefer null-aware collection elements over manual
ifconditions.♻️ Apply guideline-compliant syntax
Map<String, dynamic> toJson() => { 'jsonrpc': jsonrpc, 'id': id, 'method': method, - if (params != null) 'params': params, + 'params': ?params, };🤖 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 `@lib/src/mcp/mcp_protocol.dart` around lines 43 - 48, In toJson() replace the manual conditional entry "if (params != null) 'params': params," with a null-aware map spread using the ...? operator so the params key is only added when non-null; e.g. construct a small map for params and spread it with ...? (referencing the toJson method and the params symbol) to comply with null-aware collection element guidelines.Source: Coding guidelines
143-148: ⚡ Quick winUse null-aware syntax instead of manual null check.
As per coding guidelines, prefer null-aware collection elements over manual
ifconditions.♻️ Apply guideline-compliant syntax
Map<String, dynamic> toJson() => { 'jsonrpc': jsonrpc, 'id': id, - 'error': {'code': code, 'message': message, if (data != null) 'data': data}, + 'error': {'code': code, 'message': message, 'data': ?data}, };🤖 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 `@lib/src/mcp/mcp_protocol.dart` around lines 143 - 148, Replace the manual null check inside toJson() (the map entry "if (data != null) 'data': data") with a null-aware spread so the 'data' entry is only included when non-null; update the Map returned by toJson() (inside the toJson method in mcp_protocol.dart) to use a null-aware spread expression (for example, ...?(data != null ? {'data': data} : null)) referencing the existing data symbol so the map construction is concise and guideline-compliant.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 `@AGENTS.md`:
- Around line 122-123: Remove the duplicate `bin/src/mcp.dart` entry from the AI
Integration section so the CLI entry remains only in the CLI section; instead,
list only the MCP protocol/server/tools implementation files in AI Integration
(e.g., references to MCP server, protocol handlers, and tooling), and ensure
`bin/src/mcp.dart` is not repeated in AGENTS.md.
In `@bin/src/serve.dart`:
- Around line 203-214: Replace the misuse of JsonRpcError.methodNotFound for
HTTP method validation: detect non-POST in the request (the existing
event.request.method check) and return a 405 response with a plain HTTP-style
JSON body (e.g., {"error":"Method Not
Allowed","allowed":"POST","received":event.request.method.value}) instead of the
JSON-RPC error object; update the Response construction that currently calls
JsonRpcError.methodNotFound to encode this simple HTTP error JSON and keep the
405 status and content-type header, leaving JSON-RPC errors only for actual
JSON-RPC processing (not the entry-point method check).
In `@lib/src/mcp/config.dart`:
- Around line 27-28: The doc comment for effectivePort is incorrect: the
implementation returns port ?? (defaultPort + 1) (i.e., defaultPort plus one
when port is null). Update the documentation above effectivePort to state
"otherwise defaultPort + 1" (or "otherwise [defaultPort] + 1") to match the
code, and make the implementation's intent explicit by wrapping the fallback in
parentheses (use port ?? (defaultPort + 1)) for clarity while keeping the
function name effectivePort and the field port unchanged.
- Around line 37-43: _optionalInt currently returns null for unparsable numeric
strings; change it to mirror lib/src/builder/config.dart behavior by throwing a
LoadConfigException when a String cannot be parsed to an int instead of
returning null. Update the String() arm in _optionalInt to attempt
int.tryParse(value) and if that yields null throw LoadConfigException with a
clear message containing the offending value and expected type; keep the
existing null, int, num, and fallback arms unchanged and ensure the thrown
exception type is LoadConfigException so callers get consistent validation
errors.
In `@lib/src/mcp/mcp_server.dart`:
- Around line 78-86: The dispatch function currently throws ArgumentError for
unknown methods which becomes an internal error; change dispatch (in
JsonRpcRequest handling) to return a JsonRpcResponse.error with code -32601
(method not found) instead of throwing (update the switch default to produce
methodNotFound response), and update _handleMessage to catch JsonRpcError (the
type representing JSON-RPC errors) and write its contained JsonRpcResponse
directly to the client rather than converting it to -32603; reference dispatch,
_handleMessage, and JsonRpcError when making these edits.
In `@skills/spry-debugging/references/route-debugging.md`:
- Around line 52-54: The fenced code block containing spry.explain_route(method:
"GET", path: "/admin/users") is missing a language tag; update the
triple-backtick fence to include a language (e.g., ```text or ```bash) so the
block passes MD040 linting and renders consistently—locate the block with
spry.explain_route(...) and add the appropriate language tag to the opening
fence.
In `@skills/spry-docs/references/middleware.md`:
- Around line 41-47: The fenced code block that shows the directory structure
(the block starting with the lines "routes/" and containing "_middleware.dart
# applies to all routes") is missing a language identifier; update that opening
fence to use a plain text tag (e.g., change ``` to ```text) so the snippet is
rendered correctly, and apply the same change to any other similar
directory-structure fences in middleware.md if present.
In `@skills/spry-docs/references/routing.md`:
- Line 25: The table entry documenting routes/[...catchall].dart is using the
single-segment pattern "/:catchall" which is incorrect for multi-segment
matches; update the displayed route pattern to the Spry catch-all form (for
example "/**:catchall") so the example matches the intended examples like "/any"
and "/any/path/here" and ensure the routes/[...catchall].dart row shows the
corrected pattern.
In `@skills/spry-docs/SKILL.md`:
- Around line 17-30: The fenced code block in SKILL.md showing the project
structure is missing a language identifier; update the opening triple-backtick
(```) to include a language tag such as text (e.g., ```text) for the block that
begins with "my-app/" so the project tree renders and is accessible correctly.
---
Nitpick comments:
In `@lib/src/builder/config.dart`:
- Around line 431-441: The `_readMcpConfig` function currently uses a strict `is
Map<String, Object?>` check which is inconsistent with `_openApiConfig` and
`_clientConfig`; update `_readMcpConfig` to use the lenient `Map()` pattern
match and then convert with `Map<String, Object?>.from(value)` before calling
`McpConfig.fromJson`, and keep the existing `LoadConfigException` using
`_describeValue` for non-map values so behavior remains consistent and robust
for JSON-decoded inputs.
In `@lib/src/mcp/mcp_protocol.dart`:
- Around line 43-48: In toJson() replace the manual conditional entry "if
(params != null) 'params': params," with a null-aware map spread using the ...?
operator so the params key is only added when non-null; e.g. construct a small
map for params and spread it with ...? (referencing the toJson method and the
params symbol) to comply with null-aware collection element guidelines.
- Around line 143-148: Replace the manual null check inside toJson() (the map
entry "if (data != null) 'data': data") with a null-aware spread so the 'data'
entry is only included when non-null; update the Map returned by toJson()
(inside the toJson method in mcp_protocol.dart) to use a null-aware spread
expression (for example, ...?(data != null ? {'data': data} : null)) referencing
the existing data symbol so the map construction is concise and
guideline-compliant.
In `@test/mcp_tools_test.dart`:
- Around line 59-73: Add a unit test covering the edge case where a prefix is a
string-prefix but not a path-component-prefix: in the existing pathIsPrefix
tests (group 'pathIsPrefix' in test/mcp_tools_test.dart) add a test named like
'prefix must match on path boundaries' that asserts pathIsPrefix('/admin',
'/admins') isFalse and pathIsPrefix('/api', '/api-v2') isFalse so the function
pathIsPrefix is verified to require path-boundary matching rather than simple
substring matching.
🪄 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
Run ID: 5c6f1695-4eb6-4c42-88d7-3e17704c6e82
📒 Files selected for processing (19)
AGENTS.mdbin/spry.dartbin/src/mcp.dartbin/src/serve.dartlib/config.dartlib/src/builder/config.dartlib/src/mcp/config.dartlib/src/mcp/mcp_protocol.dartlib/src/mcp/mcp_server.dartlib/src/mcp/mcp_tools.dartskills/spry-debugging/SKILL.mdskills/spry-debugging/references/route-debugging.mdskills/spry-docs/SKILL.mdskills/spry-docs/references/config.mdskills/spry-docs/references/handlers.mdskills/spry-docs/references/middleware.mdskills/spry-docs/references/routing.mdtest/mcp_protocol_test.darttest/mcp_tools_test.dart
…ttern - Add text/markdown language tags to code fences in SKILL.md, middleware.md, and route-debugging.md (CodeRabbit review) - Fix catch-all route pattern from /:catchall to /**:catchall in routing.md to correctly represent multi-segment matching
…ection The CLI entry is already listed in the CLI section. Keep only the MCP implementation files (protocol/server/tools) in AI Integration.
Return a simple JSON error body for HTTP method validation instead of a JSON-RPC error. JSON-RPC errors are reserved for actual JSON-RPC processing, not the entry-point HTTP method check.
- Fix effectivePort doc to say 'defaultPort + 1' instead of 'defaultPort' - Fix _optionalInt to throw FormatException for unparsable strings instead of silently returning null, consistent with config validation
- dispatch() now throws JsonRpcError.methodNotFound instead of ArgumentError for unknown RPC methods - _handleMessage catches JsonRpcError directly and writes it - serve.dart _handleMcpEvent catches JsonRpcError instead of ArgumentError for consistent error propagation
Align with _openApiConfig and _clientConfig patterns by using Map() destructuring instead of strict is Map<String, Object?> check, converting with Map<String, Object?>.from() for consistency.
Replace manual 'if (x != null)' guards with Dart null-aware syntax (?params, ?data) in JsonRpcRequest.toJson and JsonRpcError.toJson.
Verify pathIsPrefix requires path-component boundaries, not just string-prefix matching. /admin is not a prefix of /admins.
Handle roux named catch-all patterns like **:slug produced by the scanner for routes/[...slug].dart. Both pathMatches and extractParams now recognize **:param segments. Codex review: P2
Treat :param and [param] segments as wildcards in middleware scope prefix matching. This ensures scoped middleware under /users/:id correctly matches requests like /users/42. Codex review: P2
Skip scoped/global middleware whose HTTP method restriction doesn't match the requested method, matching the runtime behavior of Spry.fetch which filters by method with findAll(..., method: ...). Codex review: P2
Extract _startSession from _buildAndStart. On dev server restart, call _startSession with the already-built plan from _tryBuild instead of running _prepareServeBuild a second time. Codex review: P2
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b67ea7887e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (config.mcp?.enable == true) { | ||
| await _startMcpInstance(config, out, session); |
There was a problem hiding this comment.
When config.mcp.enable is true and _startMcpInstance throws (for example, the MCP port is already in use), _startRunner has already spawned the dev server process but _startSession exits without calling session.close(). In that failure path spry serve reports an error while leaving the child runtime running in the background, so wrap the MCP startup in cleanup or bind MCP before starting the runner.
Useful? React with 👍 / 👎.
| @@ -126,10 +134,11 @@ Future<int> runServe( | |||
| continue; | |||
There was a problem hiding this comment.
When MCP is enabled with reload: hotswap on a hotswap-capable target, this branch updates the generated files and then continues without restarting the MCP runtime or replacing the ProjectState captured by _startMcpInstance. After adding/removing routes or changing config, MCP tools keep serving the old scanned entries/config until a full restart, which defeats the live spry serve integration for Cloudflare/Vercel/Netlify hotswap sessions.
Useful? React with 👍 / 👎.
| final routeMethod = route.method?.value; | ||
| if (routeMethod != null && routeMethod != method) continue; | ||
| if (pathMatches(route.path, path)) { |
There was a problem hiding this comment.
For HEAD requests, the runtime's matchHandler falls back to a GET route when there is no explicit HEAD handler, but this strict method filter skips GET routes whenever method == 'HEAD'. In projects with only routes/foo.get.dart, spry.explain_route reports no matching route for HEAD /foo even though Spry.fetch will invoke the GET handler.
Useful? React with 👍 / 👎.
| final err = entry.error!; | ||
| if (pathIsPrefix(err.path, path)) { |
There was a problem hiding this comment.
When a scoped error handler is method-specific, such as _error.post.dart, the runtime collects errors with errors.findAll(path, method: method.value, includeAny: true), so it will not run for a GET request. This path-only check still includes that handler in spry.explain_route, making the reported error chain disagree with the actual request pipeline for method-restricted error handlers.
Useful? React with 👍 / 👎.
1 participant
Resolves #188
Resolves #189
Resolves #190
Summary by CodeRabbit
New Features
spry mcpCLI command and in-process MCP HTTP endpoint when enabled.Documentation
Tests