From d7438ba89efa78c4e6c49a0f32a0280c16662d22 Mon Sep 17 00:00:00 2001 From: mAi Date: Fri, 22 May 2026 11:46:19 +0200 Subject: [PATCH] refactor(mcp): widen ToolHandler signature to return *ToolError with .data support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 5d slice A. ToolHandler was `func(ctx, params) (any, error)` and errors surfaced through the MCP `result.isError=true` content envelope with no place to put structured payloads. Widen to `(any, *ToolError)` so handlers return a typed `{Code, Msg, Data}` that the server marshals into the JSON-RPC `error` envelope (`{code, message, data}`) — `data` is omitted when nil so today's untyped errors stay clean. Handler.go: - ToolError gains `Code int`; Msg+Data unchanged. Error() preserved. - Drop `AsToolError` — `errors.As` indirection is no longer needed now that handlers return *ToolError directly. - Add `InternalError(err)` (-32603, wraps a plain error) and `InvalidParamsError(msg)` (-32602, declared for slice B's validation promotion — no callers in slice A). - `handleToolsCall` switches from the `result.isError` envelope to the JSON-RPC `error` envelope via new `writeToolErr` helper. Transport-level errors (`writeErr`) are unchanged. Tools.go: - `itemWriteError` now returns `*ToolError` with the legacy `validation : [{json-blob}]` Msg text and no Data. Slice B replaces this with `ValidationToolError` (typed .data + -32602). - All ten tool handlers migrated to the new signature. Non-validation paths default to `Code: codeInternalError (-32603)` via `InternalError(err)` for semantic preservation; "field is required" guards keep the same message string under -32603. - Helper functions (`resolveItem`, `resolveParentPaths`, `resolveTimelineWindow`, `resolveTimelineItems`, `applyHasLinkFilters`, `parseInput`) keep returning plain `error`; their tool-handler callers wrap with `InternalError`. Test source edits (per the 5c rule): - `mcp_test.go` TestToolsCallSuccessAndError: error path now asserts on the JSON-RPC `.error.code == -32603` and `.error.message == "kaboom"` envelope instead of `result.isError=true` + content text. The success path is unchanged (`isError:false` and content[].text stay). Also refreshed two handler-literal signatures in the same test file from `(any, error)` → `(any, *ToolError)` so the test compiles against the widened signature. All other MCP tests stay behaviour-preserving — they exercise success paths through the unchanged result envelope, or hit error paths via `Handler(...) (any, *ToolError)` directly (timeline_test.go) and still see a non-nil error. --- mcp/handler.go | 73 +++++++++++++++++-------------- mcp/mcp_test.go | 24 +++++++---- mcp/tools.go | 111 +++++++++++++++++++++++++----------------------- 3 files changed, 114 insertions(+), 94 deletions(-) diff --git a/mcp/handler.go b/mcp/handler.go index 5609e89..e718cba 100644 --- a/mcp/handler.go +++ b/mcp/handler.go @@ -8,8 +8,6 @@ package mcp import ( "context" "encoding/json" - "errors" - "fmt" "io" "log/slog" "net/http" @@ -40,10 +38,11 @@ type Tool struct { } // ToolHandler runs the actual work for a tool. params is the raw JSON object -// the client supplied as the tool arguments. Returning a non-nil result is -// wrapped as a structured text content block; returning an error becomes an -// MCP "isError: true" reply. -type ToolHandler func(ctx context.Context, params json.RawMessage) (any, error) +// the client supplied as the tool arguments. A non-nil result is wrapped as +// a structured text content block; a non-nil *ToolError surfaces as the +// JSON-RPC error envelope (.error.code / .error.message / .error.data) so +// typed validation payloads round-trip cleanly to MCP clients. +type ToolHandler func(ctx context.Context, params json.RawMessage) (any, *ToolError) // Server holds the registered tools + the auth token. Mount via Routes() on // any *http.ServeMux. @@ -223,20 +222,16 @@ func (s *Server) handleToolsCall(ctx context.Context, w http.ResponseWriter, req s.writeErr(w, req.ID, codeMethodNotFound, "unknown tool: "+p.Name) return } - result, err := tool.Handler(ctx, p.Arguments) - if err != nil { - // Per MCP convention, tool errors stay inside the result envelope with - // isError=true so the client sees them as tool failures, not transport - // failures. JSON-RPC-level errors are reserved for transport problems - // (auth, parse, unknown method). - s.Logger.Warn("mcp tool error", "tool", p.Name, "err", err) - s.writeOK(w, req.ID, map[string]any{ - "content": []map[string]any{{ - "type": "text", - "text": err.Error(), - }}, - "isError": true, - }) + result, te := tool.Handler(ctx, p.Arguments) + if te != nil { + // Tool errors surface through the JSON-RPC error envelope with + // {code, message, data} so MCP clients receive typed payloads on + // .error.data alongside the human-readable message. Transport-level + // failures (auth, parse, unknown method) use the same envelope but + // with the standard JSON-RPC codes only — those callers never set + // Data. + s.Logger.Warn("mcp tool error", "tool", p.Name, "code", te.Code, "msg", te.Msg) + s.writeToolErr(w, req.ID, te) return } payload, err := json.Marshal(result) @@ -263,24 +258,40 @@ func (s *Server) writeErr(w http.ResponseWriter, id json.RawMessage, code int, m _ = json.NewEncoder(w).Encode(jsonRPCResp{JSONRPC: "2.0", ID: id, Error: &rpcError{Code: code, Message: message}}) } -// ToolError is returned by ToolHandlers for user-visible failures that should -// flow through the tool-result envelope as isError. Errors that do NOT match -// this type get wrapped automatically — this is just a sentinel for callers -// that want to provide structured user-facing data alongside the message. +func (s *Server) writeToolErr(w http.ResponseWriter, id json.RawMessage, te *ToolError) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(jsonRPCResp{ + JSONRPC: "2.0", ID: id, + Error: &rpcError{Code: te.Code, Message: te.Msg, Data: te.Data}, + }) +} + +// ToolError is the only failure shape a ToolHandler may return. Code is one +// of the JSON-RPC 2.0 codes (codeInternalError / codeInvalidParams / …), +// Msg is the human-readable message, Data is an optional typed payload +// that lands in .error.data verbatim (omitted when nil). type ToolError struct { + Code int Msg string Data any } func (e *ToolError) Error() string { return e.Msg } -// AsToolError returns the ToolError if err is one (or wraps one). -func AsToolError(err error) (*ToolError, bool) { - var te *ToolError - if errors.As(err, &te) { - return te, true +// InternalError wraps a plain Go error as a -32603 ToolError. Returns nil +// for a nil input so callers can `return nil, InternalError(err)` without a +// preceding nil-check. +func InternalError(err error) *ToolError { + if err == nil { + return nil } - return nil, false + return &ToolError{Code: codeInternalError, Msg: err.Error()} } -var _ = fmt.Sprintf // keep import handy if future code uses fmt +// InvalidParamsError builds a -32602 ToolError for tool-level argument +// failures (missing required field, wrong shape). Slice B's +// ValidationToolError is the typed-payload counterpart for itemwrite +// rejections. +func InvalidParamsError(msg string) *ToolError { + return &ToolError{Code: codeInvalidParams, Msg: msg} +} diff --git a/mcp/mcp_test.go b/mcp/mcp_test.go index 6602885..aadb1a7 100644 --- a/mcp/mcp_test.go +++ b/mcp/mcp_test.go @@ -52,12 +52,12 @@ func TestInitializeAndToolsList(t *testing.T) { Name: "echo", Description: "echoes input.message", InputSchema: json.RawMessage(`{"type":"object","required":["message"]}`), - Handler: func(ctx context.Context, raw json.RawMessage) (any, error) { + Handler: func(ctx context.Context, raw json.RawMessage) (any, *ToolError) { var in struct { Message string `json:"message"` } if err := json.Unmarshal(raw, &in); err != nil { - return nil, err + return nil, InternalError(err) } return map[string]any{"echo": in.Message}, nil }, @@ -81,14 +81,14 @@ func TestToolsCallSuccessAndError(t *testing.T) { srv := New("p", "1", "", nil) srv.Register(Tool{ Name: "echo", - Handler: func(ctx context.Context, raw json.RawMessage) (any, error) { + Handler: func(ctx context.Context, raw json.RawMessage) (any, *ToolError) { return map[string]any{"got": string(raw)}, nil }, }) srv.Register(Tool{ Name: "boom", - Handler: func(ctx context.Context, raw json.RawMessage) (any, error) { - return nil, &ToolError{Msg: "kaboom"} + Handler: func(ctx context.Context, raw json.RawMessage) (any, *ToolError) { + return nil, &ToolError{Code: codeInternalError, Msg: "kaboom"} }, }) @@ -106,15 +106,21 @@ func TestToolsCallSuccessAndError(t *testing.T) { t.Errorf("echo did not include 'got' key: %s", body) } + // Phase 5d slice A: tool errors now surface through the JSON-RPC error + // envelope ({code, message, data}) rather than result.isError, so the + // assertions track .error.code / .error.message. _, body = doRPC(t, srv, rpcJSON(t, 4, "tools/call", map[string]any{ "name": "boom", "arguments": map[string]any{}, }), "") - if !strings.Contains(string(body), `"isError":true`) { - t.Fatalf("error response missing isError:true: %s", body) + if !strings.Contains(string(body), `"code":-32603`) { + t.Fatalf("error response missing code:-32603: %s", body) } - if !strings.Contains(string(body), "kaboom") { - t.Errorf("error response missing message: %s", body) + if !strings.Contains(string(body), `"message":"kaboom"`) { + t.Errorf("error response missing message:kaboom: %s", body) + } + if strings.Contains(string(body), `"isError":true`) { + t.Errorf("error response should no longer route through result.isError: %s", body) } } diff --git a/mcp/tools.go b/mcp/tools.go index c1650f3..e90d092 100644 --- a/mcp/tools.go +++ b/mcp/tools.go @@ -14,21 +14,24 @@ import ( "github.com/m/projax/store" ) -// itemWriteError serialises an *itemwrite.ValidationError into a Go error -// whose Error() string carries the JSON shape MCP clients can parse to -// extract kind/path/detail. The JSON-RPC error envelope wraps this in -// .error.data so a TypeScript client gets a typed object alongside the -// human-readable message. -func itemWriteError(ve *itemwrite.ValidationError) error { +// itemWriteError serialises an *itemwrite.ValidationError into a *ToolError +// whose Msg carries the JSON-suffixed legacy shape (kind/path/detail in a +// bracketed JSON blob). Slice B promotes this to the typed +// ValidationToolError below — until then this helper preserves the +// pre-Phase-5d wire text so consumers reading .error.message keep working. +func itemWriteError(ve *itemwrite.ValidationError) *ToolError { body, err := json.Marshal(map[string]any{ "kind": ve.Kind, "path": ve.Path, "detail": ve.Detail, }) if err != nil { - return ve + return &ToolError{Code: codeInternalError, Msg: ve.Error()} + } + return &ToolError{ + Code: codeInternalError, + Msg: fmt.Sprintf("validation %s: %s [%s]", ve.Kind, ve.Detail, string(body)), } - return fmt.Errorf("validation %s: %s [%s]", ve.Kind, ve.Detail, string(body)) } // TimelineArgs is the MCP-facing input shape for the `timeline` tool — a @@ -252,15 +255,15 @@ const ( ) func timelineTool(st *store.Store, agg *aggregate.Aggregator) ToolHandler { - return func(ctx context.Context, raw json.RawMessage) (any, error) { + return func(ctx context.Context, raw json.RawMessage) (any, *ToolError) { var args TimelineArgs if err := parseInput(raw, &args); err != nil { - return nil, fmt.Errorf("bad params: %w", err) + return nil, InternalError(fmt.Errorf("bad params: %w", err)) } now := time.Now() from, to, err := resolveTimelineWindow(args, now) if err != nil { - return nil, err + return nil, InternalError(err) } order := "desc" if args.Order == "asc" { @@ -275,7 +278,7 @@ func timelineTool(st *store.Store, agg *aggregate.Aggregator) ToolHandler { } items, err := resolveTimelineItems(ctx, st, args) if err != nil { - return nil, err + return nil, InternalError(err) } if !args.IncludeExcluded { items = filterByTimelineExclude(items, effectiveKinds) @@ -284,7 +287,7 @@ func timelineTool(st *store.Store, agg *aggregate.Aggregator) ToolHandler { hasGitRefType := containsString(args.Has, aggregate.RefTypeGiteaRepo) items, err = applyHasLinkFilters(ctx, st, items, hasCalDAVRefType, hasGitRefType) if err != nil { - return nil, err + return nil, InternalError(err) } result := agg.All(ctx, items, aggregate.AllOpts{ @@ -744,10 +747,10 @@ func listItemsTool(st *store.Store) ToolHandler { Public *bool `json:"public"` Limit int `json:"limit"` } - return func(ctx context.Context, raw json.RawMessage) (any, error) { + return func(ctx context.Context, raw json.RawMessage) (any, *ToolError) { var in input if err := parseInput(raw, &in); err != nil { - return nil, fmt.Errorf("bad params: %w", err) + return nil, InternalError(fmt.Errorf("bad params: %w", err)) } items, err := st.ListByFilters(ctx, store.SearchFilters{ ParentPath: in.ParentPath, @@ -762,7 +765,7 @@ func listItemsTool(st *store.Store) ToolHandler { Limit: in.Limit, }) if err != nil { - return nil, err + return nil, InternalError(err) } views := make([]itemView, 0, len(items)) for _, it := range items { @@ -780,14 +783,14 @@ func getItemTool(st *store.Store) ToolHandler { Path string `json:"path"` IncludeLinks *bool `json:"include_links"` } - return func(ctx context.Context, raw json.RawMessage) (any, error) { + return func(ctx context.Context, raw json.RawMessage) (any, *ToolError) { var in input if err := parseInput(raw, &in); err != nil { - return nil, fmt.Errorf("bad params: %w", err) + return nil, InternalError(fmt.Errorf("bad params: %w", err)) } it, err := resolveItem(ctx, st, in.ID, in.Path) if err != nil { - return nil, err + return nil, InternalError(err) } view := toItemView(it) include := true @@ -831,14 +834,14 @@ func createItemTool(st *store.Store) ToolHandler { Status string `json:"status"` Metadata map[string]any `json:"metadata"` } - return func(ctx context.Context, raw json.RawMessage) (any, error) { + return func(ctx context.Context, raw json.RawMessage) (any, *ToolError) { var in input if err := parseInput(raw, &in); err != nil { - return nil, fmt.Errorf("bad params: %w", err) + return nil, InternalError(fmt.Errorf("bad params: %w", err)) } parentIDs, err := resolveParentPaths(ctx, st, in.ParentPaths) if err != nil { - return nil, err + return nil, InternalError(err) } kind := in.Kind if len(kind) == 0 { @@ -868,7 +871,7 @@ func createItemTool(st *store.Store) ToolHandler { Metadata: in.Metadata, }) if err != nil { - return nil, err + return nil, InternalError(err) } return toItemView(it), nil } @@ -912,14 +915,14 @@ func updateItemTool(st *store.Store) ToolHandler { PublicScreenshots *[]string `json:"public_screenshots"` TimelineExclude *[]string `json:"timeline_exclude"` } - return func(ctx context.Context, raw json.RawMessage) (any, error) { + return func(ctx context.Context, raw json.RawMessage) (any, *ToolError) { var in input if err := parseInput(raw, &in); err != nil { - return nil, fmt.Errorf("bad params: %w", err) + return nil, InternalError(fmt.Errorf("bad params: %w", err)) } it, err := resolveItem(ctx, st, in.ID, in.Path) if err != nil { - return nil, err + return nil, InternalError(err) } patch := store.UpdateInput{ Title: it.Title, @@ -998,7 +1001,7 @@ func updateItemTool(st *store.Store) ToolHandler { if in.ParentPaths != nil { pids, err := resolveParentPaths(ctx, st, *in.ParentPaths) if err != nil { - return nil, err + return nil, InternalError(err) } patch.ParentIDs = pids } @@ -1020,7 +1023,7 @@ func updateItemTool(st *store.Store) ToolHandler { } updated, err := st.Update(ctx, it.ID, patch) if err != nil { - return nil, err + return nil, InternalError(err) } return toItemView(updated), nil } @@ -1034,17 +1037,17 @@ func deleteItemTool(st *store.Store) ToolHandler { Path string `json:"path"` Cascade bool `json:"cascade"` } - return func(ctx context.Context, raw json.RawMessage) (any, error) { + return func(ctx context.Context, raw json.RawMessage) (any, *ToolError) { var in input if err := parseInput(raw, &in); err != nil { - return nil, fmt.Errorf("bad params: %w", err) + return nil, InternalError(fmt.Errorf("bad params: %w", err)) } it, err := resolveItem(ctx, st, in.ID, in.Path) if err != nil { - return nil, err + return nil, InternalError(err) } if err := st.SoftDeleteCascade(ctx, it.ID, in.Cascade); err != nil { - return nil, err + return nil, InternalError(err) } return map[string]any{"deleted": it.ID, "cascade": in.Cascade}, nil } @@ -1058,14 +1061,14 @@ func listLinksTool(st *store.Store) ToolHandler { Path string `json:"path"` RefType string `json:"ref_type"` } - return func(ctx context.Context, raw json.RawMessage) (any, error) { + return func(ctx context.Context, raw json.RawMessage) (any, *ToolError) { var in input if err := parseInput(raw, &in); err != nil { - return nil, fmt.Errorf("bad params: %w", err) + return nil, InternalError(fmt.Errorf("bad params: %w", err)) } it, err := resolveItem(ctx, st, in.ID, in.Path) if err != nil { - return nil, err + return nil, InternalError(err) } var links []*store.ItemLink if in.RefType != "" { @@ -1080,7 +1083,7 @@ func listLinksTool(st *store.Store) ToolHandler { } } if err != nil { - return nil, err + return nil, InternalError(err) } views := make([]linkView, 0, len(links)) for _, l := range links { @@ -1103,17 +1106,17 @@ func addLinkTool(st *store.Store) ToolHandler { EventDate string `json:"event_date"` Metadata map[string]any `json:"metadata"` } - return func(ctx context.Context, raw json.RawMessage) (any, error) { + return func(ctx context.Context, raw json.RawMessage) (any, *ToolError) { var in input if err := parseInput(raw, &in); err != nil { - return nil, fmt.Errorf("bad params: %w", err) + return nil, InternalError(fmt.Errorf("bad params: %w", err)) } if in.RefType == "" || in.RefID == "" { - return nil, errors.New("ref_type and ref_id are required") + return nil, &ToolError{Code: codeInternalError, Msg: "ref_type and ref_id are required"} } it, err := resolveItem(ctx, st, in.ID, in.Path) if err != nil { - return nil, err + return nil, InternalError(err) } md := in.Metadata if md == nil { @@ -1128,13 +1131,13 @@ func addLinkTool(st *store.Store) ToolHandler { if strings.TrimSpace(in.EventDate) != "" { t, err := time.Parse("2006-01-02", strings.TrimSpace(in.EventDate)) if err != nil { - return nil, fmt.Errorf("event_date must be YYYY-MM-DD: %w", err) + return nil, InternalError(fmt.Errorf("event_date must be YYYY-MM-DD: %w", err)) } datePtr = &t } link, err := st.AddLinkDated(ctx, it.ID, in.RefType, in.RefID, in.Rel, notePtr, datePtr, md) if err != nil { - return nil, err + return nil, InternalError(err) } return toLinkView(link), nil } @@ -1144,16 +1147,16 @@ func removeLinkTool(st *store.Store) ToolHandler { type input struct { LinkID string `json:"link_id"` } - return func(ctx context.Context, raw json.RawMessage) (any, error) { + return func(ctx context.Context, raw json.RawMessage) (any, *ToolError) { var in input if err := parseInput(raw, &in); err != nil { - return nil, fmt.Errorf("bad params: %w", err) + return nil, InternalError(fmt.Errorf("bad params: %w", err)) } if in.LinkID == "" { - return nil, errors.New("link_id is required") + return nil, &ToolError{Code: codeInternalError, Msg: "link_id is required"} } if err := st.DeleteLink(ctx, in.LinkID); err != nil { - return nil, err + return nil, InternalError(err) } return map[string]any{"deleted": in.LinkID}, nil } @@ -1166,17 +1169,17 @@ func searchTool(st *store.Store) ToolHandler { Query string `json:"query"` Limit int `json:"limit"` } - return func(ctx context.Context, raw json.RawMessage) (any, error) { + return func(ctx context.Context, raw json.RawMessage) (any, *ToolError) { var in input if err := parseInput(raw, &in); err != nil { - return nil, fmt.Errorf("bad params: %w", err) + return nil, InternalError(fmt.Errorf("bad params: %w", err)) } if in.Query == "" { - return nil, errors.New("query is required") + return nil, &ToolError{Code: codeInternalError, Msg: "query is required"} } items, err := st.Search(ctx, in.Query, in.Limit) if err != nil { - return nil, err + return nil, InternalError(err) } views := make([]itemView, 0, len(items)) for _, it := range items { @@ -1199,14 +1202,14 @@ func treeTool(st *store.Store) ToolHandler { RootPath string `json:"root_path"` Depth int `json:"depth"` } - return func(ctx context.Context, raw json.RawMessage) (any, error) { + return func(ctx context.Context, raw json.RawMessage) (any, *ToolError) { var in input if err := parseInput(raw, &in); err != nil { - return nil, fmt.Errorf("bad params: %w", err) + return nil, InternalError(fmt.Errorf("bad params: %w", err)) } items, err := st.ListAll(ctx) if err != nil { - return nil, err + return nil, InternalError(err) } // Build adjacency by parent id (the same row appears once per parent). byID := map[string]*store.Item{} @@ -1237,7 +1240,7 @@ func treeTool(st *store.Store) ToolHandler { if in.RootPath != "" { root, err := st.GetByPathOrSlug(ctx, in.RootPath) if err != nil { - return nil, err + return nil, InternalError(err) } out = append(out, build(root, in.RootPath, 0)) } else {