Files
xarmian 727cd80927 fix(mcp): explicit tool annotations from catalog write-shape knowledge (BUG-2302) (#1121)
mcp-go's NewTool injects default annotations on every tool —
ReadOnlyHint:false, DestructiveHint:true, OpenWorldHint:true — and
buildToolFromDef never overrode them, so every Pad tool advertised
itself as destructive, including pure reads like pad_search and
pad_project. Hosts use destructiveHint to decide whether to prompt;
mislabeling reads trains users to click through prompts.

Derive the block in buildToolFromDef from the catalog's own knowledge
(readOnlyActions — the same single source the tool-surface serializer
uses — plus a new sibling additiveWriteActions allowlist):

- every action read-only → ReadOnlyHint:true, DestructiveHint:false,
  IdempotentHint:true (pad_search, pad_project, pad_attachment,
  pad_meta, pad_playbook);
- writes all purely ADDITIVE → ReadOnlyHint:false,
  DestructiveHint:false (pad_workspace: invite/create/claim/restore;
  pad_library: activate — codex round 1: marking additive writes
  destructive reintroduces the prompt-training harm at tool level);
- any overwrite/delete-capable action → the conservative
  ReadOnlyHint:false, DestructiveHint:true (pad_item, pad_collection,
  pad_role) — unchanged on the wire from the old defaults;
- OpenWorldHint:false everywhere (pad tools are closed-world).

pad_set_workspace gets a hand-written block (write, non-destructive,
idempotent, closed-world).

Also adds the missing pad_item.history entry to readOnlyActions —
documented read-only since v0.14 but reported read_only:false on the
tool-surface descriptor.

ToolSurfaceVersion 0.19 → 0.20 (behavior bump, v0.9/v0.16 precedent):
no tool names, action enums, or param shapes changed. instructions.md
and README headings retitled per the drift tests. The changelog entry
describes only this change; BUG-2305 appends to it if it ships in the
same window (one bump total).

Tests: TestCatalogTools_AnnotationsExplicit pins a literal per-tool
read/additive/destructive table (deliberate second enumeration — a new
tool, or a write action added to an all-read or all-additive tool,
fails loudly until someone decides its class);
TestAdditiveWriteActions_NoStaleEntries guards the new allowlist
(real catalog pairs only, never overlapping readOnlyActions);
TestSetWorkspaceTool_AnnotationsExplicit covers both deployment
variants; pad_item.history joins the read spot-checks.
Mutation-verified both directions: destructive-polarity flip fails 10
tools; always-destructive fails the two additive rows.

Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM
2026-08-16 13:49:00 -04:00

281 lines
10 KiB
Go

package mcp
import "encoding/json"
// tool_surface.go — the cycle-free catalog→JSON serializer (PLAN-1888 /
// TASK-1891, DR-3). Builds the same tool-surface descriptor blob that
// pad_meta.action: tool-surface emits, PLUS a per-action `read_only`
// bool, directly from the package-global Catalog. No server types, no
// ActionEnv — so cmd/pad/main.go can build an http.Handler from this
// and inject it into *server.Server without internal/server having to
// import internal/mcp (which would close the import cycle —
// internal/mcp already imports internal/server via dispatch_http.go).
//
// Why a per-action read_only flag (DR-2): WebMCP's readOnlyHint is
// per-TOOL, but the fat catalog tools straddle read+write (pad_item
// mixes show/list AND create/update/delete). The browser layer
// (Phase 3) derives readOnlyHint=true only when EVERY action a tool
// exposes is a read. Emitting the per-action bool here keeps that
// derivation a lookup rather than a re-derivation of the read set in
// TypeScript. The read set lives in Go (readOnlyActions below) as the
// single source of truth.
// toolSurfaceActionSummary is one action entry in the serialized
// surface. Adds `read_only` to the existing {name} shape — additive,
// so any consumer of pad_meta.action: tool-surface that ignores unknown
// keys is unaffected (DR-7: no ToolSurfaceVersion bump).
type toolSurfaceActionSummary struct {
Name string `json:"name"`
ReadOnly bool `json:"read_only"`
}
type toolSurfaceParamSummary struct {
Name string `json:"name"`
Type string `json:"type"`
Description string `json:"description,omitempty"`
Enum []string `json:"enum,omitempty"`
}
type toolSurfaceToolSummary struct {
Name string `json:"name"`
Description string `json:"description"`
Workspace bool `json:"workspace"`
Actions []toolSurfaceActionSummary `json:"actions"`
Params []toolSurfaceParamSummary `json:"params"`
}
// readOnlyActions is the allowlist of (tool, action) pairs that perform
// no mutation — pure reads / introspection. Co-located with the catalog
// (DR-2) and consulted by the serializer to stamp each action's
// `read_only` bool. Keyed by tool name → set of read-only action names.
//
// Anything NOT listed here is treated as a write (the safe default —
// a missing entry produces read_only=false, so a forgotten mutating
// action never silently advertises as read-only). Confirmed against the
// catalog_*.go ToolDefs:
//
// - pad_item: get/list/deps/starred/list-comments/backlinks/export read;
// create/update/delete/move/restore/link/unlink/star/unstar/
// comment/bulk-update/note/decide/import write.
// - pad_workspace: list/members/storage/audit-log/deleted read;
// invite/create/claim/restore write.
// - pad_collection: list read; create/update/delete write.
// - pad_project: all read (dashboard/next/ready/stale/standup/
// changelog/report/activity).
// - pad_role: list read; create/update/delete write.
// - pad_search: query read.
// - pad_playbook: list/get read; run is side-effect-free (returns the
// body + bound args for the agent to execute) → read.
// - pad_library: list/get read; activate mutates workspace state → write.
// - pad_attachment: list/show read (metadata only; no upload/download).
// - pad_meta: server-info/version/tool-surface/bootstrap all read.
var readOnlyActions = map[string]map[string]bool{
"pad_item": {
"get": true,
"list": true,
"deps": true,
"starred": true,
"list-comments": true,
"backlinks": true,
"export": true,
// BUG-2302: was missing since v0.14 added it — history is
// documented read-only in catalog_item.go (version metadata,
// no content body, no mutation).
"history": true,
},
"pad_workspace": {
"list": true,
"members": true,
"storage": true,
"audit-log": true,
// TASK-1973: restore is a write; deleted is read-only.
"deleted": true,
},
"pad_collection": {
"list": true,
},
"pad_project": {
"dashboard": true,
"next": true,
"ready": true,
"stale": true,
"standup": true,
"changelog": true,
"report": true,
"activity": true,
},
"pad_role": {
"list": true,
},
"pad_search": {
"query": true,
},
"pad_playbook": {
"list": true,
"get": true,
"run": true,
},
"pad_library": {
"list": true,
"get": true,
},
"pad_attachment": {
"list": true,
"show": true,
},
"pad_meta": {
"server-info": true,
"version": true,
"tool-surface": true,
"bootstrap": true,
},
}
// additiveWriteActions is the allowlist of WRITE actions that are
// purely ADDITIVE — they create or attach state, never overwrite,
// remove, or destroy it (BUG-2302, codex round 1). Used by
// annotationForDef: a tool advertises DestructiveHint:false only when
// every action is either read-only or listed here. Absence ⇒ assume
// destructive — the conservative direction, same shape as
// readOnlyActions above.
//
// Only tools whose ENTIRE write set is additive need complete entries
// (that's what flips their tool-level hint); tools with any
// overwrite/delete action (pad_item.update/delete,
// pad_collection.update/delete, pad_role.update/delete) stay
// destructive at tool level regardless, so their additive writes are
// deliberately not enumerated — one judgment per load-bearing line.
//
// - pad_workspace: create (new workspace), invite (adds an
// invitation/member), claim (redeems an invite code — joins),
// restore (un-soft-delete; the catalog itself documents it
// "mutating but non-destructive", TASK-1973).
// - pad_library: activate copies a library entry into the workspace
// as a new item.
var additiveWriteActions = map[string]map[string]bool{
"pad_workspace": {
"create": true,
"invite": true,
"claim": true,
"restore": true,
},
"pad_library": {
"activate": true,
},
}
// isAdditiveWriteAction reports whether (toolName, action) is a
// purely-additive write. Defaults to false (assume destructive) for
// any pair not in the allowlist.
func isAdditiveWriteAction(toolName, action string) bool {
actions, ok := additiveWriteActions[toolName]
if !ok {
return false
}
return actions[action]
}
// isReadOnlyAction reports whether (toolName, action) performs no
// mutation. Defaults to false (write) for any pair not in the
// allowlist — the conservative direction.
func isReadOnlyAction(toolName, action string) bool {
actions, ok := readOnlyActions[toolName]
if !ok {
return false
}
return actions[action]
}
// buildToolSurfaceTools projects a catalog slice into the serialized
// per-tool summaries. Shared by ToolSurfaceJSON (the REST/browser path)
// and actionMetaToolSurface (the MCP path) so the two surfaces can't
// drift. The synthesized param list mirrors buildToolFromDef's
// implicit-param logic (action enum first, optional workspace, then the
// declared ParamDefs) so the dump is self-contained for consumers.
func buildToolSurfaceTools(catalog []ToolDef) []toolSurfaceToolSummary {
tools := make([]toolSurfaceToolSummary, 0, len(catalog))
for _, def := range catalog {
actionNames := sortedActionNames(def)
actions := make([]toolSurfaceActionSummary, 0, len(actionNames))
for _, name := range actionNames {
actions = append(actions, toolSurfaceActionSummary{
Name: name,
ReadOnly: isReadOnlyAction(def.Name, name),
})
}
// `action` is always present and always required — buildToolFromDef
// injects it into the schema. Synthesize it here so consumers get
// the full param picture. Enum mirrors sortedActionNames.
params := []toolSurfaceParamSummary{{
Name: "action",
Type: "string",
Description: "The action to perform. Required.",
Enum: actionNames,
}}
if def.Schema.Workspace {
params = append(params, toolSurfaceParamSummary{
Name: "workspace",
Type: "string",
Description: "Workspace slug. An explicit value always wins. A " +
"single-user local server else uses the pad_set_workspace " +
"session default, then the .pad.toml workspace; a multi-user/" +
"remote server requires an explicit workspace per call.",
})
}
for _, p := range def.Schema.Params {
params = append(params, toolSurfaceParamSummary{
Name: p.Name,
Type: p.Type,
Description: p.Description,
Enum: p.Enum,
})
}
tools = append(tools, toolSurfaceToolSummary{
Name: def.Name,
Description: def.Description,
Workspace: def.Schema.Workspace,
Actions: actions,
Params: params,
})
}
return tools
}
// buildToolSurfacePayload assembles the full tool-surface document
// (version + rollout status + tools) from a catalog slice. The payload
// shape is identical to what actionMetaToolSurface emitted pre-TASK-1891
// except every action now carries `read_only`.
func buildToolSurfacePayload(catalog []ToolDef) map[string]any {
// rollout_status mirrors the historical actionMetaToolSurface logic:
// "complete" once the cmdhelp leaf walker retired (any version past
// the initial 0.1). Kept for backwards compatibility with consumers
// that branched on it during the v0.1→v0.2 retirement.
rolloutStatus := "in-progress"
if ToolSurfaceVersion != "0.1" {
rolloutStatus = "complete"
}
return map[string]any{
"tool_surface_version": ToolSurfaceVersion,
"rollout_status": rolloutStatus,
"tools": buildToolSurfaceTools(catalog),
}
}
// ToolSurfaceJSON serializes the package-global Catalog into the
// tool-surface descriptor JSON — the same blob pad_meta.action:
// tool-surface emits, plus a per-action `read_only` bool. Cycle-free:
// reads only the in-package Catalog + readOnlyActions, no server types.
//
// cmd/pad/main.go builds an http.Handler from this and injects it into
// *server.Server (the SetMCPTransport pattern) so the
// GET /api/v1/mcp/tool-surface endpoint can serve it without
// internal/server importing internal/mcp.
//
// Scope is the nine env.Catalog tools (pad_item, pad_workspace,
// pad_collection, pad_project, pad_role, pad_search, pad_playbook,
// pad_meta, pad_library). pad_set_workspace is registered separately
// (registry.go) and is NOT in Catalog, so it's naturally excluded.
func ToolSurfaceJSON() ([]byte, error) {
return json.Marshal(buildToolSurfacePayload(Catalog))
}