m/projax
1
0
Fork 0
Code Issues 5 Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(mcp): widen ToolHandler signature to return *ToolError with .data support

Browse Source 
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 <kind>: <detail> [{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.
This commit is contained in:
mAi
2026-05-22 11:46:19 +02:00
parent 7ebd435044
commit d7438ba89e
3 changed files with 114 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files

73
mcp/handler.go
View File

@@ -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}
}

24
mcp/mcp_test.go
View File

@@ -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)
}
}

111
mcp/tools.go
View File

@@ -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 {