Skip to content

fix(mcp): accept canonical resource names in path params and keep error results schema-valid#6108

Merged
boojack merged 2 commits into
usememos:mainfrom
harpchad:fix/mcp-resource-names-and-error-results
Jul 23, 2026
Merged

fix(mcp): accept canonical resource names in path params and keep error results schema-valid#6108
boojack merged 2 commits into
usememos:mainfrom
harpchad:fix/mcp-resource-names-and-error-results

Conversation

@harpchad

Copy link
Copy Markdown
Contributor

Fixes #6107

Problem

The MCP tools substitute path parameters directly into the REST route, so memo_get_memo with memo: "memos/abc123" requests /api/v1/memos/memos/abc123 and 404s. But memos/abc123 is exactly what the API returns as name in every memo object — an MCP client that round-trips the name it received from memo_create_memo/memo_list_memos gets a 404 on every follow-up call, and for memo_update_memo the edit is silently lost.

The failure is then hidden by a second issue: on REST errors the tools return structuredContent: {"error": ...}, which doesn't conform to the tool's declared outputSchema (e.g. required state/content/visibility). Spec-strict clients (Claude Code, LiteLLM, the ChatGPT connector) reject the response with a schema-validation error, masking the underlying 404.

Changes

  • substitutePathParameters now routes values through trimResourceNamePrefix: when a path parameter's placeholder directly follows its collection segment (/api/v1/memos/{memo}) and the value carries that collection prefix (memos/abc123), the prefix is stripped. Bare IDs pass through unchanged; foreign prefixes and multi-segment values are left untouched. Works generically for {memo}, {attachment}, and any future path parameter.
  • newToolErrorResult no longer attaches structuredContent. Error results report through isError: true and text content, per the spec's structured-content guidancestructuredContent is only meaningful when it conforms to the declared output schema. As a bonus, clients now see the real error message (404 Not Found: memo not found) instead of a validation failure.

Testing

  • New table-driven test TestBuildAPIRequestAcceptsResourceNamesForPathParameters covering canonical names, bare IDs, nested routes, attachment names, and the two left-untouched cases; updated error-shape assertions in adapter_test.go and service_test.go.
  • go test ./server/router/mcp/... passes; go build ./... clean; golangci-lint run reports 0 issues.
  • Verified end-to-end against a locally built server over the streamable HTTP endpoint: create → get/update/comment/list-comments/delete all succeed using full memos/<id> names (update verified persisted), and a nonexistent-memo call returns isError: true with no structuredContent and the real 404 message.

harpchad added 2 commits July 23, 2026 07:33
The API returns names like "memos/abc123", but path substitution placed
the value into "/api/v1/memos/{memo}" verbatim, so round-tripping a
returned name produced "/api/v1/memos/memos/abc123" and a 404 on every
get/update/delete/comment call. Strip the collection prefix when the
placeholder directly follows its collection segment; bare IDs, foreign
prefixes, and multi-segment values pass through unchanged.
Error results carried structuredContent {"error": ...}, which cannot
conform to the success outputSchema each tool declares. Spec-strict
clients reject the result with a schema-validation error, masking the
real message. Report errors through isError and text content only.
@harpchad
harpchad requested a review from a team as a code owner July 23, 2026 12:34
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 385c2463-af37-4520-96b9-baf0c116af4a

📥 Commits

Reviewing files that changed from the base of the PR and between 41dd3d1 and 6db96ec.

📒 Files selected for processing (4)
  • server/router/mcp/adapter.go
  • server/router/mcp/adapter_test.go
  • server/router/mcp/result.go
  • server/router/mcp/service_test.go

Walkthrough

MCP path substitution now accepts canonical resource names by removing collection prefixes before URL-escaping path identifiers. Tests cover bare IDs, canonical names, nested routes, and reserved characters. Tool errors now return text content with IsError: true while omitting StructuredContent; related adapter and service tests were updated accordingly.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the main change: canonical resource-name handling and schema-valid tool errors.
Description check ✅ Passed The description is clearly about the same MCP path-parameter and error-result fixes.
Linked Issues check ✅ Passed The changes address #6107 by accepting canonical resource names and removing schema-invalid structuredContent from errors.
Out of Scope Changes check ✅ Passed The added tests and helper logic are directly tied to the linked MCP fixes and do not appear unrelated.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@boojack boojack left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@boojack
boojack merged commit 03e34bd into usememos:main Jul 23, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP: tools with {memo} path param reject canonical memos/{id} resource names (404); error results violate the declared outputSchema

2 participants