From 1e94fcbd9d4686bd518284fc90c845ba97ba28a2 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 1 May 2026 18:32:27 -0400 Subject: [PATCH] feat(mcp): structured MCP error envelopes with closed taxonomy (TASK-973) (#357) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(mcp): structured MCP error envelopes with closed taxonomy (TASK-973) Replaces raw stderr / status-text passthrough with a closed-set ErrorCode taxonomy + structured ErrorEnvelope. Agents can now branch on `error.code` instead of parsing free-form text: { "error": { "code": "no_workspace", "message": "No workspace context. Pass `workspace` explicitly, ...", "hint": "Available workspaces: docapp, pad-web", "available_workspaces": [{"slug": "docapp", "default": true}, ...] } } Taxonomy (8 codes): - no_workspace, unknown_workspace — populate available_workspaces - auth_required, permission_denied - item_not_found, validation_failed, conflict - server_error (catch-all) Implementation: - internal/mcp/errors.go (new): ErrorCode constants, ErrorEnvelope + ErrorPayload + WorkspaceHint types, NewErrorResult constructor, classifyExecError + classifyHTTPStatus dispatchers, regex pattern matchers for stderr classification, WorkspaceLister interface for hint enrichment. - internal/mcp/dispatch.go: ExecDispatcher.Dispatch routes failures through classifyExecError (with itself as the WorkspaceLister). Adds ListWorkspaces method that shells out to `pad workspace list --format json`. Adds RootArgs field so the listing inherits root flags (--url etc.). - internal/mcp/dispatch_http.go: packageHTTPResponse routes 4xx/5xx through classifyHTTPStatus. Lookup is intentionally nil here — TASK-977 (PLAN-943) owns the privacy-preserving available_workspaces filtering by OAuth allow-list. - cmd/pad/mcp.go: pre-flatten rootFlags into RootArgs at dispatcher construction. NewErrorResult emits BOTH structured content (for Claude Desktop, Cursor) AND a JSON text body (for older clients). Both decode to the same envelope so wire-level shape stays uniform. Tests: - TestNewErrorResult_Envelope: round-trip the envelope through structured + text surfaces. - TestClassifyExecError: 11 cases covering every taxonomy code via stderr patterns. - TestClassifyExecError_LookupFailureStillReturnsEnvelope: lookup failures degrade to empty available_workspaces, never drop the whole envelope. - TestClassifyHTTPStatus: 10 cases covering each HTTP status mapping including the workspace-vs-item 404 fork. - TestParseWorkspaceListJSON: happy path, empty / null / malformed, entry-without-slug skipping. - TestExtractUnknownWorkspaceSlug: regex helper round-trip. Out of scope (per task description): - HTTPHandlerDispatcher available_workspaces filtering by OAuth allow-list → TASK-977 (PLAN-943). - item_not_found "recent items" hint enrichment → also TASK-977. - Per-code docs page on getpad.dev/mcp/local → TASK-976. Parent: TASK-973 → PLAN-969. * fix(cli): add JSON output to pad workspace list per Codex review (round 1) Codex P2: classifyExecError's WorkspaceLister side channel calls `pad workspace list --format json` to populate available_workspaces in no_workspace / unknown_workspace error envelopes (TASK-973). The CLI command silently ignored formatFlag and always printed the human-readable shape, so parseWorkspaceListJSON would fail and the hint was effectively never populated. Add JSON branch to workspacesCmd that emits a {slug, name, updated_at, default} array. The `default: true` flag marks the CWD-linked workspace so agents can prefer it without a separate DetectWorkspace call. Manual verification: $ pad workspace list --format json | jq '.[0]' { "slug": "docapp", "name": "pad", "updated_at": "2026-04-14T13:24:51Z", "default": true } Parent: TASK-973 → PLAN-969. * fix(mcp): tighten unknown-workspace slug regex per Codex review (round 2) Codex finding: extractUnknownWorkspaceSlug's bare-word regex captured stop-words like "not" out of generic "Workspace not found" messages. The server emits exactly this generic body in middleware_auth.go and handlers_workspaces.go, so the resulting envelope would say `Workspace "not" is not visible to this session.` — pushing agents toward retrying with a bogus slug. Tighten the regex to only match QUOTED slug forms ("workspace 'foo'" or "workspace \"bar\""). Bare-word phrasings yield empty slug, and unknownWorkspaceResult now emits a generic "Workspace not visible to this session." instead of the misleading empty-string `Workspace ""`. Test cases updated: - "workspace 'foo' does not exist" → "foo" (still works) - "workspace \"bar\" not found" → "bar" (still works) - "unknown workspace baz" → "" (was "baz", now intentionally empty) - "workspace docapp not visible" → "" (was "docapp", now empty) - "Workspace not found" → "" (the actual server response) The other taxonomy / hint behavior is unchanged: ErrUnknownWorkspace still classifies correctly, available_workspaces still populates from ListWorkspaces, and the body text still appears in Hint via the classifyHTTPStatus 404 branch's body-append logic. Parent: TASK-973 → PLAN-969. --- cmd/pad/main.go | 31 +- cmd/pad/mcp.go | 14 +- internal/mcp/dispatch.go | 81 ++++- internal/mcp/dispatch_http.go | 27 +- internal/mcp/dispatch_http_advanced.go | 2 +- internal/mcp/dispatch_http_test.go | 4 +- internal/mcp/errors.go | 441 +++++++++++++++++++++++++ internal/mcp/errors_test.go | 298 +++++++++++++++++ 8 files changed, 870 insertions(+), 28 deletions(-) create mode 100644 internal/mcp/errors.go create mode 100644 internal/mcp/errors_test.go diff --git a/cmd/pad/main.go b/cmd/pad/main.go index 1990e611..4858a908 100644 --- a/cmd/pad/main.go +++ b/cmd/pad/main.go @@ -2086,13 +2086,38 @@ func workspacesCmd() *cobra.Command { if err != nil { return err } + + current, _ := cli.DetectWorkspace(workspaceFlag) + + // JSON output: machine-readable shape consumed by the MCP + // server's structured-error side channel (TASK-973's + // classifyExecError populates available_workspaces from + // this output). Each entry includes `default: true` for the + // CWD-linked workspace so agents can prefer it without a + // separate lookup. + if formatFlag == "json" { + type entry struct { + Slug string `json:"slug"` + Name string `json:"name"` + UpdatedAt string `json:"updated_at,omitempty"` + Default bool `json:"default,omitempty"` + } + out := make([]entry, 0, len(workspaces)) + for _, ws := range workspaces { + out = append(out, entry{ + Slug: ws.Slug, + Name: ws.Name, + UpdatedAt: ws.UpdatedAt.Format(time.RFC3339), + Default: ws.Slug == current, + }) + } + return cli.PrintJSON(out) + } + if len(workspaces) == 0 { fmt.Println("No workspaces. Run 'pad workspace init' to create one.") return nil } - - current, _ := cli.DetectWorkspace(workspaceFlag) - for _, ws := range workspaces { marker := " " if ws.Slug == current { diff --git a/cmd/pad/mcp.go b/cmd/pad/mcp.go index 64ea9baf..3c340c5f 100644 --- a/cmd/pad/mcp.go +++ b/cmd/pad/mcp.go @@ -279,7 +279,19 @@ Shuts down cleanly on EOF, SIGINT, or SIGTERM.`, // still consumed at dispatch time (BuildCLIArgs reads // individual command schemas) but no longer drives the tool // surface shape. - dispatcher := &mcpserver.ExecDispatcher{Binary: bin} + // Pre-flatten rootFlags into the token list ExecDispatcher + // reuses for its WorkspaceLister side channel — when + // classifyExecError needs to populate available_workspaces, + // it spawns `pad workspace list` and must hit the same + // server endpoint (e.g. --url for non-default servers). + rootArgs := []string{} + for k, v := range rootFlags { + if v == "" { + continue + } + rootArgs = append(rootArgs, "--"+k, v) + } + dispatcher := &mcpserver.ExecDispatcher{Binary: bin, RootArgs: rootArgs} if _, err := mcpserver.Register(srv.MCP(), mcpserver.RegistryOptions{ Doc: doc, Workspace: state, diff --git a/internal/mcp/dispatch.go b/internal/mcp/dispatch.go index 90843437..d0c6141a 100644 --- a/internal/mcp/dispatch.go +++ b/internal/mcp/dispatch.go @@ -3,6 +3,7 @@ package mcp import ( "context" "encoding/json" + "errors" "fmt" "os/exec" "sort" @@ -102,11 +103,23 @@ func mergeDispatchInput(input map[string]any, sessionWorkspace string, rootFlags // ExecDispatcher shells out to the pad binary at Binary. stdout is // returned as Text content; if it parses as JSON the result is also // surfaced via StructuredContent so MCP clients can consume it -// natively. Non-zero exit returns an IsError-flagged result with -// stderr as the message. +// natively. Non-zero exit returns an IsError-flagged result with a +// structured ErrorEnvelope (TASK-973) — stderr is classified into a +// closed-set ErrorCode so agents can branch on the code rather than +// parsing free-form text. type ExecDispatcher struct { // Binary is the path to the pad executable. Required. Binary string + + // RootArgs are pre-formatted root-flag tokens (e.g. ["--url", X]) + // to forward to every spawned subprocess. Same data as the + // rootFlags map carried by RegistryOptions but pre-flattened so + // the dispatcher doesn't re-iterate it on every Dispatch call. + // Used by the WorkspaceLister side-channel — when classifyExecError + // needs to populate available_workspaces, it spawns + // `pad workspace list` with these flags so it hits the same + // endpoint as the original failed call. + RootArgs []string } // Dispatch runs ` ` and packages the @@ -121,13 +134,11 @@ func (d *ExecDispatcher) Dispatch(ctx context.Context, cmdPath []string, cliArgs cmd.Stdout = &stdout cmd.Stderr = &stderr if err := cmd.Run(); err != nil { - msg := strings.TrimSpace(stderr.String()) - if msg == "" { - msg = err.Error() - } - return mcp.NewToolResultErrorf( - "pad %s failed: %s", strings.Join(cmdPath, " "), msg, - ), nil + // Classify into the structured envelope. WorkspaceLister + // uses the same dispatcher (recursive in spirit but a fresh + // subprocess) to enrich no_workspace / unknown_workspace + // errors with available_workspaces. + return classifyExecError(ctx, cmdPath, err, stderr.String(), d), nil } out := stdout.String() // If stdout is JSON, surface it as structured content alongside @@ -143,6 +154,58 @@ func (d *ExecDispatcher) Dispatch(ctx context.Context, cmdPath []string, cliArgs return mcp.NewToolResultText(out), nil } +// ListWorkspaces satisfies WorkspaceLister so error helpers can +// populate available_workspaces hints. Best-effort: if listing fails +// (no auth, network down, etc.) the caller treats an error as "no +// listing available" and surfaces the bare error envelope. +// +// Implementation note: spawns a fresh `pad workspace list --format +// json` subprocess. Adds one extra exec per error path that needs the +// hint — acceptable cost given how rare these errors are. Honors +// RootArgs so the listing hits the same server endpoint as the +// originally-failed call (e.g. --url for non-default servers). +func (d *ExecDispatcher) ListWorkspaces(ctx context.Context) ([]WorkspaceHint, error) { + if d.Binary == "" { + return nil, errors.New("dispatcher: binary path not configured") + } + args := []string{"workspace", "list", "--format", "json"} + args = append(args, d.RootArgs...) + cmd := exec.CommandContext(ctx, d.Binary, args...) + var stdout, stderr strings.Builder + cmd.Stdout = &stdout + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + return nil, fmt.Errorf("workspace list: %s", strings.TrimSpace(stderr.String())) + } + return parseWorkspaceListJSON(stdout.String()) +} + +// parseWorkspaceListJSON decodes the JSON shape `pad workspace list +// --format json` emits into the WorkspaceHint slice. Lenient about +// optional fields — only `slug` is required; everything else gets +// zero values when absent. +func parseWorkspaceListJSON(body string) ([]WorkspaceHint, error) { + body = strings.TrimSpace(body) + if body == "" || body == "null" { + return nil, nil + } + var raw []map[string]any + if err := json.Unmarshal([]byte(body), &raw); err != nil { + return nil, fmt.Errorf("decode workspace list: %w", err) + } + out := make([]WorkspaceHint, 0, len(raw)) + for _, w := range raw { + slug, _ := w["slug"].(string) + if slug == "" { + continue + } + name, _ := w["name"].(string) + isDefault, _ := w["default"].(bool) + out = append(out, WorkspaceHint{Slug: slug, Name: name, Default: isDefault}) + } + return out, nil +} + // BuildCLIArgs translates an MCP tool call's JSON arguments into the // CLI argument list that should be appended after the command path. // Pure function — no side effects, no subprocess — so it tests cleanly. diff --git a/internal/mcp/dispatch_http.go b/internal/mcp/dispatch_http.go index 8d0fa016..0a052f3f 100644 --- a/internal/mcp/dispatch_http.go +++ b/internal/mcp/dispatch_http.go @@ -353,7 +353,7 @@ func (d *HTTPHandlerDispatcher) executeRequest( rec := httptest.NewRecorder() d.Handler.ServeHTTP(rec, req) - return packageHTTPResponse(cmdKey, rec.Result()) + return packageHTTPResponse(ctx, cmdKey, rec.Result()) } // buildAuthedRequest constructs an in-process HTTP request against @@ -415,9 +415,12 @@ func buildHTTPRequest(ctx context.Context, method, urlPath string, body []byte, // packageHTTPResponse turns the recorded handler response into an MCP // CallToolResult, mirroring ExecDispatcher's "JSON → structured + text -// fallback" behaviour. 4xx/5xx surface as IsError-flagged results so -// MCP clients distinguish protocol vs. tool failures. -func packageHTTPResponse(cmdKey string, resp *http.Response) (*mcp.CallToolResult, error) { +// fallback" behaviour. 4xx/5xx surface as structured ErrorEnvelopes +// (TASK-973) so MCP clients see the same closed-set error codes +// regardless of transport. TASK-977 (PLAN-943) extends this with +// privacy-preserving available_workspaces filtering once the OAuth +// allow-list is in place. +func packageHTTPResponse(ctx context.Context, cmdKey string, resp *http.Response) (*mcp.CallToolResult, error) { defer resp.Body.Close() bodyBytes, err := io.ReadAll(resp.Body) if err != nil { @@ -426,14 +429,14 @@ func packageHTTPResponse(cmdKey string, resp *http.Response) (*mcp.CallToolResul body := string(bodyBytes) if resp.StatusCode >= 400 { - // Match the CLI's `pad ` error format from ExecDispatcher - // so MCP clients see a consistent shape regardless of - // transport. - msg := strings.TrimSpace(body) - if msg == "" { - msg = http.StatusText(resp.StatusCode) - } - return mcp.NewToolResultErrorf("pad %s failed: %s", cmdKey, msg), nil + // classifyHTTPStatus does the status → ErrorCode mapping. The + // `lookup` parameter is nil here because TASK-977 owns the + // remote-side workspace listing (it must filter by OAuth + // allow-list to avoid leaking workspaces the agent didn't + // consent to). For now, available_workspaces stays empty on + // the HTTP transport; the rest of the envelope still gives + // the agent enough to branch on `code`. + return classifyHTTPStatus(ctx, cmdKey, resp.StatusCode, bodyBytes, nil), nil } if trimmed := strings.TrimSpace(body); strings.HasPrefix(trimmed, "{") || strings.HasPrefix(trimmed, "[") { diff --git a/internal/mcp/dispatch_http_advanced.go b/internal/mcp/dispatch_http_advanced.go index 9eef08a9..69112006 100644 --- a/internal/mcp/dispatch_http_advanced.go +++ b/internal/mcp/dispatch_http_advanced.go @@ -297,7 +297,7 @@ func (d *HTTPHandlerDispatcher) dispatchItemUpdate( // Mirror the CLI's "not found" UX — the handler's 404 body // already contains a clear message; package it the same way // any other tool error would be packaged. - return packageHTTPResponse(cmdKey, prefetchRec.Result()) + return packageHTTPResponse(ctx, cmdKey, prefetchRec.Result()) } var existing struct { Fields string `json:"fields"` diff --git a/internal/mcp/dispatch_http_test.go b/internal/mcp/dispatch_http_test.go index a1d6a4b6..148fbeb2 100644 --- a/internal/mcp/dispatch_http_test.go +++ b/internal/mcp/dispatch_http_test.go @@ -499,7 +499,7 @@ func TestPackageHTTPResponse_StructuredJSONOnSuccess(t *testing.T) { StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader(`{"ref":"TASK-1"}`)), } - res, err := packageHTTPResponse("item create", resp) + res, err := packageHTTPResponse(context.Background(), "item create", resp) if err != nil { t.Fatalf("packageHTTPResponse: %v", err) } @@ -516,7 +516,7 @@ func TestPackageHTTPResponse_TextFallbackOnNonJSON(t *testing.T) { StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader("hello world")), } - res, err := packageHTTPResponse("item create", resp) + res, err := packageHTTPResponse(context.Background(), "item create", resp) if err != nil { t.Fatalf("packageHTTPResponse: %v", err) } diff --git a/internal/mcp/errors.go b/internal/mcp/errors.go new file mode 100644 index 00000000..4f051247 --- /dev/null +++ b/internal/mcp/errors.go @@ -0,0 +1,441 @@ +package mcp + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "regexp" + "strings" + + "github.com/mark3labs/mcp-go/mcp" +) + +// ───────────────────────────────────────────────────────────────────── +// Structured MCP error envelopes (TASK-973) +// +// Replaces raw CLI stderr passthrough with a closed taxonomy of error +// codes the model can branch on. The agent receives a JSON envelope +// like: +// +// { +// "error": { +// "code": "no_workspace", +// "message": "No workspace context — pass workspace= or call pad_set_workspace first.", +// "hint": "Available workspaces: docapp, pad-web. Or run pad workspace init.", +// "available_workspaces": [{"slug": "docapp", "default": true}, ...] +// } +// } +// +// instead of an opaque string like "no workspace linked. Run 'pad +// workspace init'". Closed code set means the model can implement +// recovery logic per-code rather than parsing free-form text. +// +// Both ExecDispatcher (stderr classification) and HTTPHandlerDispatcher +// (HTTP status mapping) feed into the same taxonomy. Mirror impl on +// the remote side is TASK-977 — it inherits the type definitions from +// here and adds a privacy-preserving available_workspaces filter. +// ───────────────────────────────────────────────────────────────────── + +// ErrorCode is one of the closed-set MCP error codes. Enumerated +// constants below; do not introduce a new code without updating the +// docs (server instructions block + getpad.dev/mcp/local). +type ErrorCode string + +const ( + // ErrNoWorkspace fires when no workspace context resolves + // (no explicit param, no session default, no CWD .pad.toml). + // Populates available_workspaces from `pad workspace list`. + ErrNoWorkspace ErrorCode = "no_workspace" + + // ErrUnknownWorkspace fires when a slug is supplied but doesn't + // match any workspace the user can read. Same available_workspaces + // hint as ErrNoWorkspace. + ErrUnknownWorkspace ErrorCode = "unknown_workspace" + + // ErrAuthRequired fires when no valid credentials are present — + // CLI: ~/.pad/credentials.json missing or expired; HTTP: 401. + ErrAuthRequired ErrorCode = "auth_required" + + // ErrPermissionDenied fires when authentication succeeds but role + // is insufficient for the operation. HTTP: 403. + ErrPermissionDenied ErrorCode = "permission_denied" + + // ErrItemNotFound fires when an item ref / slug doesn't resolve. + // Future enhancement: populate `available_collections` or recent + // items as a hint (deferred to TASK-977). + ErrItemNotFound ErrorCode = "item_not_found" + + // ErrValidationFailed fires on bad input — required field missing, + // enum value out of range, malformed JSON. HTTP: 422. + ErrValidationFailed ErrorCode = "validation_failed" + + // ErrConflict fires when an operation collides with concurrent + // state (e.g. version mismatch on update). HTTP: 409. + ErrConflict ErrorCode = "conflict" + + // ErrServerError is the catch-all for unexpected failures — + // 5xx from HTTP, unknown stderr patterns from exec. The wrapped + // message preserves the underlying detail for debugging without + // promising any structured shape. + ErrServerError ErrorCode = "server_error" +) + +// ErrorEnvelope is the wire shape returned to MCP clients on tool +// failures. The outer key is `error` so the JSON unambiguously signals +// "this is the error path"; clients that want to switch on code can do +// so without inspecting IsError separately. +type ErrorEnvelope struct { + Error ErrorPayload `json:"error"` +} + +// ErrorPayload is the structured error body. Optional fields use +// pointers / `omitempty` so they only appear when populated, keeping +// success-case-shaped clients happy. Per-code fields documented inline. +type ErrorPayload struct { + // Code is one of the ErrorCode constants. Stable across versions. + Code ErrorCode `json:"code"` + + // Message is a short, human-readable summary suitable for direct + // display. Avoid PII / token values here — the message may end up + // in logs. + Message string `json:"message"` + + // Hint is a longer suggestion for self-recovery. May reference + // commands, alternate values, or follow-up reads. Optional. + Hint string `json:"hint,omitempty"` + + // AvailableWorkspaces is populated for ErrNoWorkspace / + // ErrUnknownWorkspace so the agent can pick a valid slug without + // a human round-trip. Empty array means lookup failed (e.g. no + // auth) — agents should treat this as "no workspace listing + // available" rather than "no workspaces exist." + AvailableWorkspaces []WorkspaceHint `json:"available_workspaces,omitempty"` + + // Field / Expected / Got populate ErrValidationFailed when the + // underlying error pinpoints a specific input. + Field string `json:"field,omitempty"` + Expected string `json:"expected,omitempty"` + Got string `json:"got,omitempty"` + + // RequiredRole / CurrentRole populate ErrPermissionDenied so + // agents see why the call was rejected. + RequiredRole string `json:"required_role,omitempty"` + CurrentRole string `json:"current_role,omitempty"` +} + +// WorkspaceHint is a minimal workspace summary surfaced in the +// no_workspace / unknown_workspace envelopes. +type WorkspaceHint struct { + Slug string `json:"slug"` + Name string `json:"name,omitempty"` + Default bool `json:"default,omitempty"` +} + +// NewErrorResult packages an ErrorPayload as an MCP CallToolResult +// with IsError=true. Both the JSON envelope and a human-readable +// summary are returned: the envelope as structured content for clients +// that parse it (Claude Desktop, Cursor), the summary as text fallback. +func NewErrorResult(p ErrorPayload) *mcp.CallToolResult { + envelope := ErrorEnvelope{Error: p} + body, err := json.Marshal(envelope) + if err != nil { + // Marshal of a struct with only string + bool + slice fields + // can't realistically fail; defensive fallback returns a plain + // errorf so the agent at least sees something. + return mcp.NewToolResultErrorf("%s: %s", p.Code, p.Message) + } + // NewToolResultStructured returns a result with content blocks + // PLUS structured content. The IsError flag has to be set after + // because the structured constructor doesn't accept it as a + // parameter — set it here so MCP clients see both. + res := mcp.NewToolResultStructured(envelope, string(body)) + res.IsError = true + return res +} + +// noWorkspaceResult builds the standard ErrNoWorkspace envelope with +// available_workspaces populated by the supplied lookup. Lookup is +// best-effort: failures (e.g. no auth) yield an envelope with empty +// AvailableWorkspaces rather than dropping the whole error. +func noWorkspaceResult(ctx context.Context, lookup WorkspaceLister) *mcp.CallToolResult { + hints := bestEffortWorkspaceHints(ctx, lookup) + return NewErrorResult(ErrorPayload{ + Code: ErrNoWorkspace, + Message: "No workspace context. Pass `workspace` explicitly, call pad_set_workspace first, or run from a directory with .pad.toml.", + Hint: workspaceHintLine(hints), + AvailableWorkspaces: hints, + }) +} + +// unknownWorkspaceResult wraps a "workspace X doesn't exist" failure. +// Same available_workspaces enrichment as no_workspace. Empty slug +// emits a generic message rather than a misleading `Workspace ""` +// — happens when the source error doesn't explicitly name the slug. +func unknownWorkspaceResult(ctx context.Context, slug string, lookup WorkspaceLister) *mcp.CallToolResult { + hints := bestEffortWorkspaceHints(ctx, lookup) + message := "Workspace not visible to this session." + if slug != "" { + message = fmt.Sprintf("Workspace %q is not visible to this session.", slug) + } + return NewErrorResult(ErrorPayload{ + Code: ErrUnknownWorkspace, + Message: message, + Hint: workspaceHintLine(hints), + AvailableWorkspaces: hints, + }) +} + +// workspaceHintLine returns a concise comma-joined slug list, or an +// empty string when no hints were resolved (avoids "Available +// workspaces: " trailing nothing). +func workspaceHintLine(hints []WorkspaceHint) string { + if len(hints) == 0 { + return "" + } + slugs := make([]string, 0, len(hints)) + for _, h := range hints { + slugs = append(slugs, h.Slug) + } + return "Available workspaces: " + strings.Join(slugs, ", ") +} + +// WorkspaceLister is the side-channel a dispatcher exposes so error +// helpers can populate available_workspaces hints. Returning an empty +// slice (rather than an error) when lookup fails is fine — callers +// already treat empty as "no listing available." +type WorkspaceLister interface { + ListWorkspaces(ctx context.Context) ([]WorkspaceHint, error) +} + +// bestEffortWorkspaceHints calls lookup.ListWorkspaces and swallows +// errors. The error envelope is more valuable than nothing even when +// the listing failed. +func bestEffortWorkspaceHints(ctx context.Context, lookup WorkspaceLister) []WorkspaceHint { + if lookup == nil { + return nil + } + hints, err := lookup.ListWorkspaces(ctx) + if err != nil { + return nil + } + return hints +} + +// envelopeFrom reads the ErrorEnvelope back out of an MCP error +// result's structured content. Used by classifyHTTPStatus's 404 +// workspace path so we can layer additional Hint detail on top of the +// envelope unknownWorkspaceResult already built. Returns a zero +// envelope when the structured content is missing or malformed — +// callers fall back gracefully. +func envelopeFrom(res *mcp.CallToolResult) ErrorEnvelope { + if res == nil { + return ErrorEnvelope{} + } + if env, ok := res.StructuredContent.(ErrorEnvelope); ok { + return env + } + return ErrorEnvelope{} +} + +// ───────────────────────────────────────────────────────────────────── +// ExecDispatcher classification +// +// The local subprocess emits stderr strings that we pattern-match +// against known cases. Unmatched output falls through to ErrServerError +// with the raw stderr preserved in Message. +// ───────────────────────────────────────────────────────────────────── + +// classifyExecError turns an exec.Cmd failure (err + stderr) into a +// structured envelope. lookup is optional — when supplied, no_workspace +// errors get available_workspaces enrichment. +func classifyExecError(ctx context.Context, cmdPath []string, runErr error, stderr string, lookup WorkspaceLister) *mcp.CallToolResult { + stderr = strings.TrimSpace(stderr) + lower := strings.ToLower(stderr) + + switch { + case execStderrMatchesNoWorkspace(lower): + return noWorkspaceResult(ctx, lookup) + case execStderrMatchesUnknownWorkspace(lower): + // Stderr typically embeds the slug — try to extract it. If + // extraction fails, the envelope still carries the message; + // the slug just won't appear in the hint line. + slug := extractUnknownWorkspaceSlug(stderr) + return unknownWorkspaceResult(ctx, slug, lookup) + case execStderrMatchesAuthRequired(lower): + return NewErrorResult(ErrorPayload{ + Code: ErrAuthRequired, + Message: "Authentication required. Run `pad auth login` to sign in.", + Hint: stderr, + }) + case execStderrMatchesPermissionDenied(lower): + return NewErrorResult(ErrorPayload{ + Code: ErrPermissionDenied, + Message: "Permission denied for this operation.", + Hint: stderr, + }) + case execStderrMatchesItemNotFound(lower): + return NewErrorResult(ErrorPayload{ + Code: ErrItemNotFound, + Message: "Item not found.", + Hint: stderr, + }) + case execStderrMatchesValidation(lower): + return NewErrorResult(ErrorPayload{ + Code: ErrValidationFailed, + Message: "Validation failed.", + Hint: stderr, + }) + } + + // Fallback: unstructured server error. Preserve the original + // "pad failed: " shape so any agent that special- + // cased the old text still has something to read. + msg := stderr + if msg == "" && runErr != nil { + msg = runErr.Error() + } + if msg == "" { + msg = "unknown error" + } + cmd := strings.Join(cmdPath, " ") + return NewErrorResult(ErrorPayload{ + Code: ErrServerError, + Message: fmt.Sprintf("pad %s failed: %s", cmd, msg), + }) +} + +// Stderr-pattern matchers. Compiled at init for cost-free +// classification. Patterns are case-insensitive against `lower` +// (the caller pre-lowercases for performance). +var ( + reNoWorkspace = regexp.MustCompile(`no workspace.*(linked|configured)`) + reUnknownWorkspaceA = regexp.MustCompile(`workspace .* (does not exist|not found)`) + reUnknownWorkspaceB = regexp.MustCompile(`unknown workspace`) + reAuthRequired = regexp.MustCompile(`(not authenticated|authentication required|please log in|run pad auth login|invalid token|expired token)`) + rePermissionDenied = regexp.MustCompile(`(permission denied|forbidden|insufficient (permissions|role))`) + reItemNotFound = regexp.MustCompile(`(item.*not found|no such item|unknown ref)`) + reValidationFailed = regexp.MustCompile(`(invalid|missing required|must be one of|validation)`) + // Only match QUOTED slugs to avoid capturing stop-words like "not" + // in generic "Workspace not found" / "workspace not visible" + // messages. Quoted forms come from CLI stderr ("workspace 'foo' + // does not exist") and from JSON error bodies that explicitly name + // the slug. Unquoted phrasings yield an empty slug — the + // unknownWorkspaceResult message handles that gracefully without + // emitting a misleading `Workspace "not"` line. + reUnknownWorkspaceID = regexp.MustCompile(`workspace ['"]([a-z0-9][a-z0-9-]*)['"]`) +) + +func execStderrMatchesNoWorkspace(lower string) bool { return reNoWorkspace.MatchString(lower) } +func execStderrMatchesUnknownWorkspace(lower string) bool { + return reUnknownWorkspaceA.MatchString(lower) || reUnknownWorkspaceB.MatchString(lower) +} +func execStderrMatchesAuthRequired(lower string) bool { return reAuthRequired.MatchString(lower) } +func execStderrMatchesPermissionDenied(lower string) bool { + return rePermissionDenied.MatchString(lower) +} +func execStderrMatchesItemNotFound(lower string) bool { return reItemNotFound.MatchString(lower) } +func execStderrMatchesValidation(lower string) bool { return reValidationFailed.MatchString(lower) } + +// extractUnknownWorkspaceSlug pulls the slug from a CLI stderr like +// "workspace 'foo' does not exist" so the envelope can name it. +// Returns empty string if nothing matches; callers fall back to a +// generic "this slug" phrasing. +func extractUnknownWorkspaceSlug(stderr string) string { + m := reUnknownWorkspaceID.FindStringSubmatch(strings.ToLower(stderr)) + if len(m) < 2 { + return "" + } + return m[1] +} + +// ───────────────────────────────────────────────────────────────────── +// HTTPHandlerDispatcher classification +// +// HTTP status codes map cleanly to the taxonomy. Body parsing is +// best-effort: when the handler returns a structured payload we +// surface its message; otherwise the status text serves as the +// envelope's Message. +// ───────────────────────────────────────────────────────────────────── + +// classifyHTTPStatus turns an HTTP error response into a structured +// envelope. body is the raw response body (may be empty); cmdKey is +// the dotted command path for debugging context. lookup is optional +// — supply the dispatcher's WorkspaceLister so 404 (workspace) / +// 403 / etc. can populate available_workspaces when relevant. +func classifyHTTPStatus(ctx context.Context, cmdKey string, status int, body []byte, lookup WorkspaceLister) *mcp.CallToolResult { + bodyText := strings.TrimSpace(string(body)) + if bodyText == "" { + bodyText = http.StatusText(status) + } + + switch status { + case http.StatusUnauthorized: + return NewErrorResult(ErrorPayload{ + Code: ErrAuthRequired, + Message: "Authentication required.", + Hint: bodyText, + }) + case http.StatusForbidden: + return NewErrorResult(ErrorPayload{ + Code: ErrPermissionDenied, + Message: "Permission denied for this operation.", + Hint: bodyText, + }) + case http.StatusNotFound: + // Without inspecting the URL we can't tell workspace-404 vs + // item-404 with certainty; the body usually says. Default to + // item_not_found and let TASK-977 refine when the remote + // transport can provide URL-aware classification. + if strings.Contains(strings.ToLower(bodyText), "workspace") { + // Try to pull the slug out of the body so the message + // names it (e.g. body="workspace 'foo' not visible" → + // "Workspace \"foo\" is not visible..."). Falls back to + // the body in Hint when no slug parses. + slug := extractUnknownWorkspaceSlug(bodyText) + res := unknownWorkspaceResult(ctx, slug, lookup) + // unknownWorkspaceResult uses the available_workspaces + // hint line; preserve the original handler body in Hint + // so debug detail isn't lost. Concatenate when both + // exist so neither hides the other. + env := envelopeFrom(res) + if env.Error.Hint == "" { + env.Error.Hint = bodyText + } else { + env.Error.Hint = bodyText + " — " + env.Error.Hint + } + return NewErrorResult(env.Error) + } + return NewErrorResult(ErrorPayload{ + Code: ErrItemNotFound, + Message: "Item not found.", + Hint: bodyText, + }) + case http.StatusConflict: + return NewErrorResult(ErrorPayload{ + Code: ErrConflict, + Message: "Conflict — current state changed beneath this update.", + Hint: bodyText, + }) + case http.StatusUnprocessableEntity, http.StatusBadRequest: + return NewErrorResult(ErrorPayload{ + Code: ErrValidationFailed, + Message: "Validation failed.", + Hint: bodyText, + }) + } + + if status >= 500 { + return NewErrorResult(ErrorPayload{ + Code: ErrServerError, + Message: fmt.Sprintf("pad %s failed: %s", cmdKey, bodyText), + }) + } + // Other 4xx without a specific mapping — surface as server_error + // with the raw body so debugging is still possible. Avoid silently + // promoting them to validation_failed; that would mislead callers. + return NewErrorResult(ErrorPayload{ + Code: ErrServerError, + Message: fmt.Sprintf("pad %s failed (HTTP %d): %s", cmdKey, status, bodyText), + }) +} diff --git a/internal/mcp/errors_test.go b/internal/mcp/errors_test.go new file mode 100644 index 00000000..a5e22c26 --- /dev/null +++ b/internal/mcp/errors_test.go @@ -0,0 +1,298 @@ +package mcp + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "strings" + "testing" + + "github.com/mark3labs/mcp-go/mcp" +) + +// TestNewErrorResult_Envelope confirms the wire shape every error +// path produces. The test inspects: +// +// - IsError flag set on the result. +// - StructuredContent contains the envelope as a Go value. +// - Text fallback parses back to the same envelope. +// +// MCP clients pick whichever surface they understand (Claude Desktop +// uses StructuredContent; older clients fall back to text). The +// invariant: both must agree. +func TestNewErrorResult_Envelope(t *testing.T) { + payload := ErrorPayload{ + Code: ErrNoWorkspace, + Message: "No workspace context.", + Hint: "Available workspaces: docapp", + AvailableWorkspaces: []WorkspaceHint{ + {Slug: "docapp", Name: "Pad", Default: true}, + }, + } + res := NewErrorResult(payload) + + if !res.IsError { + t.Errorf("IsError = false, want true") + } + if res.StructuredContent == nil { + t.Errorf("StructuredContent missing — clients won't see the typed envelope") + } + + // Round-trip via the text fallback. + body := textOf(res) + if body == "" { + t.Fatalf("text fallback missing") + } + var got ErrorEnvelope + if err := json.Unmarshal([]byte(body), &got); err != nil { + t.Fatalf("unmarshal envelope: %v\nbody=%s", err, body) + } + if got.Error.Code != ErrNoWorkspace { + t.Errorf("Code = %q, want %q", got.Error.Code, ErrNoWorkspace) + } + if got.Error.Message != payload.Message { + t.Errorf("Message mismatch: got %q, want %q", got.Error.Message, payload.Message) + } + if len(got.Error.AvailableWorkspaces) != 1 { + t.Fatalf("AvailableWorkspaces length = %d, want 1", len(got.Error.AvailableWorkspaces)) + } + if got.Error.AvailableWorkspaces[0].Slug != "docapp" { + t.Errorf("workspace slug = %q, want docapp", got.Error.AvailableWorkspaces[0].Slug) + } +} + +// fakeWorkspaceLister is a controllable WorkspaceLister for testing. +// Returns the configured slice unless ErrOut is set, in which case it +// errors (best-effort path test). +type fakeWorkspaceLister struct { + hints []WorkspaceHint + errOut error +} + +func (f *fakeWorkspaceLister) ListWorkspaces(_ context.Context) ([]WorkspaceHint, error) { + if f.errOut != nil { + return nil, f.errOut + } + return f.hints, nil +} + +// TestClassifyExecError covers the full taxonomy via the ExecDispatcher +// classifier. Each subtest pairs a stderr pattern with the expected +// ErrorCode. Hits all 6 patterns the classifier recognizes plus the +// server_error fallback for unmatched output. +func TestClassifyExecError(t *testing.T) { + lookup := &fakeWorkspaceLister{hints: []WorkspaceHint{ + {Slug: "docapp"}, {Slug: "pad-web"}, + }} + cases := []struct { + name string + stderr string + wantCode ErrorCode + wantHints bool // true if this code populates available_workspaces + }{ + {"no_workspace", "Error: no workspace linked. Run 'pad workspace init'.", ErrNoWorkspace, true}, + {"unknown_workspace_does_not_exist", "Error: workspace 'foo' does not exist", ErrUnknownWorkspace, true}, + {"unknown_workspace_explicit", "Error: unknown workspace bar", ErrUnknownWorkspace, true}, + {"auth_required_login", "Error: not authenticated. please run pad auth login.", ErrAuthRequired, false}, + {"auth_required_expired", "Error: expired token", ErrAuthRequired, false}, + {"permission_denied", "Error: permission denied for this resource", ErrPermissionDenied, false}, + {"item_not_found", "Error: item TASK-99 not found", ErrItemNotFound, false}, + {"validation_failed_required", "Error: missing required field 'title'", ErrValidationFailed, false}, + {"validation_failed_enum", "Error: status must be one of: open, in-progress, done", ErrValidationFailed, false}, + {"server_error_unknown", "Error: something went wrong in unexpected ways", ErrServerError, false}, + {"server_error_empty", "", ErrServerError, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + runErr := errors.New("exit status 1") + res := classifyExecError(context.Background(), + []string{"item", "list"}, runErr, tc.stderr, lookup) + if !res.IsError { + t.Errorf("expected IsError, got success") + } + env := decodeEnvelope(t, res) + if env.Error.Code != tc.wantCode { + t.Errorf("Code = %q, want %q", env.Error.Code, tc.wantCode) + } + if tc.wantHints { + if len(env.Error.AvailableWorkspaces) == 0 { + t.Errorf("expected available_workspaces populated for %s", tc.wantCode) + } + } else { + if len(env.Error.AvailableWorkspaces) > 0 { + t.Errorf("did not expect available_workspaces for %s; got %v", + tc.wantCode, env.Error.AvailableWorkspaces) + } + } + }) + } +} + +// TestClassifyExecError_LookupFailureStillReturnsEnvelope ensures +// best-effort listing — when ListWorkspaces fails, we still surface +// the no_workspace error (just with empty available_workspaces). +// Without this guarantee, a transient lookup failure would degrade +// every workspace-context error into a confusing "couldn't even +// produce an error envelope" path. +func TestClassifyExecError_LookupFailureStillReturnsEnvelope(t *testing.T) { + lookup := &fakeWorkspaceLister{errOut: errors.New("listing exploded")} + res := classifyExecError(context.Background(), + []string{"item", "list"}, + errors.New("exit"), + "Error: no workspace linked", + lookup, + ) + env := decodeEnvelope(t, res) + if env.Error.Code != ErrNoWorkspace { + t.Errorf("Code = %q, want no_workspace", env.Error.Code) + } + if len(env.Error.AvailableWorkspaces) != 0 { + t.Errorf("expected empty AvailableWorkspaces on lookup failure; got %v", + env.Error.AvailableWorkspaces) + } +} + +// TestClassifyHTTPStatus covers the HTTP status → ErrorCode mapping. +// Each documented status maps to the expected code; unmapped statuses +// fall through to server_error. +func TestClassifyHTTPStatus(t *testing.T) { + cases := []struct { + name string + status int + body string + wantCode ErrorCode + }{ + {"401_unauthorized", http.StatusUnauthorized, "token invalid", ErrAuthRequired}, + {"403_forbidden", http.StatusForbidden, "role insufficient", ErrPermissionDenied}, + {"404_item", http.StatusNotFound, "item TASK-99 not found", ErrItemNotFound}, + {"404_workspace", http.StatusNotFound, "workspace foo not visible", ErrUnknownWorkspace}, + {"409_conflict", http.StatusConflict, "version mismatch", ErrConflict}, + {"422_validation", http.StatusUnprocessableEntity, "title required", ErrValidationFailed}, + {"400_validation", http.StatusBadRequest, "bad input", ErrValidationFailed}, + {"500_server", http.StatusInternalServerError, "boom", ErrServerError}, + {"503_server", http.StatusServiceUnavailable, "down", ErrServerError}, + {"418_other", http.StatusTeapot, "weird", ErrServerError}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + res := classifyHTTPStatus(context.Background(), + "pad item list", tc.status, []byte(tc.body), nil) + if !res.IsError { + t.Errorf("expected IsError") + } + env := decodeEnvelope(t, res) + if env.Error.Code != tc.wantCode { + t.Errorf("Code = %q, want %q", env.Error.Code, tc.wantCode) + } + // Body fragment should appear somewhere in the envelope — + // either in Hint (typical) or Message (server_error path). + combined := env.Error.Hint + " " + env.Error.Message + if tc.body != "" && !strings.Contains(combined, tc.body) { + t.Errorf("envelope should preserve body %q somewhere; got message=%q hint=%q", + tc.body, env.Error.Message, env.Error.Hint) + } + }) + } +} + +// TestParseWorkspaceListJSON locks the JSON shape ExecDispatcher's +// ListWorkspaces consumes. Emits the same fields `pad workspace list +// --format json` would so the parser stays aligned with the CLI as +// the latter evolves. +func TestParseWorkspaceListJSON(t *testing.T) { + t.Run("happy path", func(t *testing.T) { + body := `[{"slug":"docapp","name":"Pad","default":true},{"slug":"pad-web"}]` + got, err := parseWorkspaceListJSON(body) + if err != nil { + t.Fatalf("parse: %v", err) + } + if len(got) != 2 { + t.Fatalf("len = %d, want 2", len(got)) + } + if got[0].Slug != "docapp" || !got[0].Default || got[0].Name != "Pad" { + t.Errorf("first hint mismatch: %+v", got[0]) + } + if got[1].Slug != "pad-web" { + t.Errorf("second hint slug = %q, want pad-web", got[1].Slug) + } + }) + + t.Run("empty body", func(t *testing.T) { + got, err := parseWorkspaceListJSON("") + if err != nil || got != nil { + t.Errorf("empty body should return (nil, nil); got (%v, %v)", got, err) + } + }) + + t.Run("null body", func(t *testing.T) { + got, err := parseWorkspaceListJSON("null") + if err != nil || got != nil { + t.Errorf("null body should return (nil, nil); got (%v, %v)", got, err) + } + }) + + t.Run("entry without slug skipped", func(t *testing.T) { + body := `[{"name":"orphan"},{"slug":"valid"}]` + got, err := parseWorkspaceListJSON(body) + if err != nil { + t.Fatalf("parse: %v", err) + } + if len(got) != 1 || got[0].Slug != "valid" { + t.Errorf("expected [valid], got %+v", got) + } + }) + + t.Run("malformed JSON", func(t *testing.T) { + _, err := parseWorkspaceListJSON("not json {") + if err == nil { + t.Errorf("expected error on malformed JSON") + } + }) +} + +// TestExtractUnknownWorkspaceSlug covers the small regex helper used +// to pull a slug name out of CLI stderr like "workspace 'foo' does +// not exist". Only QUOTED slugs match — Codex review on PR #357 +// caught that bare-word matching captured stop-words like "not" out +// of generic "Workspace not found" responses, which would push agents +// toward retrying with a bogus slug. The regex now requires single or +// double quotes around the slug; bare-word phrasings yield empty +// (the caller's message handles that gracefully). +func TestExtractUnknownWorkspaceSlug(t *testing.T) { + cases := map[string]string{ + "Error: workspace 'foo' does not exist": "foo", + "Error: workspace \"bar\" not found": "bar", + "workspace 'docapp' is gone": "docapp", + // Bare-word phrasings — slug NOT extractable; empty result. + "unknown workspace baz": "", + "workspace docapp not visible": "", + "Workspace not found": "", + // Unrelated input. + "completely unrelated message": "", + "": "", + } + for stderr, want := range cases { + t.Run(stderr, func(t *testing.T) { + if got := extractUnknownWorkspaceSlug(stderr); got != want { + t.Errorf("got %q, want %q", got, want) + } + }) + } +} + +// decodeEnvelope helper extracts the ErrorEnvelope from an MCP error +// result. Uses textOf (the shared helper in catalog_test.go) to +// retrieve the JSON body from the result's content blocks. +func decodeEnvelope(t *testing.T, res *mcp.CallToolResult) ErrorEnvelope { + t.Helper() + body := textOf(res) + if body == "" { + t.Fatalf("error result has empty text body") + } + var env ErrorEnvelope + if err := json.Unmarshal([]byte(body), &env); err != nil { + t.Fatalf("decode envelope: %v\nbody=%s", err, body) + } + return env +}