From e5e56a519ac15b10d9f30f84f89cf49f78793d8d Mon Sep 17 00:00:00 2001 From: Ryan Lewis Date: Sat, 30 May 2026 21:32:09 +0100 Subject: [PATCH 1/2] feat(mcp): add opt-in write tools gated by --toolsets Expose the CLI's write surface (add, edit, complete, cancel, the project variants, log, import) as MCP tools, grouped into GitHub-style toolsets mounted with --toolsets (tasks, projects, areas, tags, bulk; "all" and THINGS_TOOLSETS supported). Writes are off by default; --read-only=false enables the write tools of the mounted toolsets, so a bare `things mcp` stays read-only and byte-identical to before. - A Writer interface (default forwards to internal/things) keeps the handlers unit-testable without shelling out to open/osascript. - edit/edit_project/import read the URL-scheme auth token from the DB; complete handles the project cascade; cancel points projects at edit. - add/edit are submitted-not-confirmed (URL scheme); complete/cancel are synchronous (AppleScript). Tool descriptions say so. Refs #82 --- README.md | 68 +++-- cmd/things/main.go | 22 +- cmd/things/mcp_integration_test.go | 132 ++++++--- internal/mcpserver/server.go | 131 ++++++++- internal/mcpserver/server_test.go | 12 +- internal/mcpserver/tools.go | 384 ++++++++++++++++++++++++-- internal/mcpserver/writes_test.go | 424 +++++++++++++++++++++++++++++ 7 files changed, 1078 insertions(+), 95 deletions(-) create mode 100644 internal/mcpserver/writes_test.go diff --git a/README.md b/README.md index eb15a03..a280f50 100644 --- a/README.md +++ b/README.md @@ -414,26 +414,50 @@ embedded in the binary — so a plain `things` upgrade refreshes it; re-run ## MCP server `things mcp` runs a [Model Context Protocol](https://modelcontextprotocol.io) -server over stdio, exposing the **read-only** side of the CLI as typed tools. -It's aimed at MCP hosts that can't shell out — chiefly **Claude Desktop** — but -hosts that can (Cursor, Claude Code) get a typed alternative to driving the CLI -via Bash. The agent skill above and the MCP server are independent; use whichever -your host supports. +server over stdio, exposing the CLI as typed tools. It's aimed at MCP hosts that +can't shell out — chiefly **Claude Desktop** — but hosts that can (Cursor, Claude +Code) get a typed alternative to driving the CLI via Bash. The agent skill above +and the MCP server are independent; use whichever your host supports. -Tools (results are the same JSON the CLI emits with `--json`): +The server is **read-only by default** and **fails fast** at startup with a clear +error if the Things3 database can't be found or opened. It requires Things3 on +macOS, like the rest of the CLI. -| Tool | Mirrors | Arguments | -| --- | --- | --- | -| `things_list` | `things ` | `view`, `project`, `area`, `tag`, `on`, `from`, `to` (all optional) | -| `things_show` | `things show` | `task` (UUID or title) | -| `things_search` | `things search` | `query` | -| `things_projects` | `things projects` | `area`, `completed` (optional) | -| `things_areas` | `things areas` | — | -| `things_tags` | `things tags` | — | +### Toolsets + +Tools are grouped into **toolsets** you mount with `--toolsets` (comma-separated; +`all` is shorthand; defaults to all). Mount only what you use to keep the tool +list short. The read tools of a mounted toolset are always available; its write +tools appear only when you pass `--read-only=false` (see [Writes](#writes)). -The server is **read-only** (no add/complete/cancel/edit) and **fails fast** at -startup with a clear error if the Things3 database can't be found or opened. It -requires Things3 on macOS, like the rest of the CLI. +| Toolset | Read tools | Write tools (need `--read-only=false`) | +| --- | --- | --- | +| `tasks` | `things_list`, `things_show`, `things_search` | `things_add`, `things_edit`, `things_complete`, `things_cancel` | +| `projects` | `things_projects` | `things_add_project`, `things_edit_project` | +| `areas` | `things_areas` | — | +| `tags` | `things_tags` | — | +| `bulk` | — | `things_log`, `things_import` | + +Each tool mirrors the CLI command of the same name (run `things --help` for +its arguments); read results are the same JSON the CLI emits with `--json`. +`--toolsets` also reads from the `THINGS_TOOLSETS` environment variable. For +example, `--toolsets=tasks` mounts just `things_list`/`things_show`/`things_search`. + +### Writes + +Write tools are **off by default**; enable them by adding `--read-only=false` +(or `--no-read-only`) to the server's `args`. A few things to know: + +- **`things_add` / `things_edit` (and the project and import variants) are + fire-and-forget**: they hand the change to the Things URL scheme and return + *before* Things has applied it, so the result reports *submitted*, not + *confirmed*, and a bad payload surfaces only as an in-app Things notification. + `things_complete` / `things_cancel` go through AppleScript and are synchronous. +- **`things_edit`, `things_edit_project`, and `things_import` need the Things URL + auth token.** Enable it once in **Things → Settings → General → Enable Things + URLs**; the server reads it from your database automatically. +- Most MCP hosts ask you to approve each tool call, so writes stay under your + control. ### Add it to Claude Desktop @@ -462,6 +486,10 @@ in-app config editor keeps setup to a copy-paste: 6. Click the **+ / tools (🔨)** icon in the chat box; **things** should be listed. Try *"what's on my list today?"* +To let Claude **create and change** to-dos, add `--read-only=false` to `args` +(`"args": ["mcp", "--read-only=false"]`) and read [Writes](#writes) first — it +enables the add/edit/complete/cancel tools. + **Not working?** ① the `command` must be the full absolute path from `which things`; ② a JSON typo makes Claude ignore the whole file silently — recheck the braces and commas; ③ errors are logged to @@ -477,8 +505,10 @@ like this. - **Generic** — `command: things` (use the absolute path if the host doesn't inherit your `PATH`), `args: ["mcp"]`. -To pin a specific database, add `"--db", "/path/to/main.sqlite"` before `"mcp"` -in `args`; otherwise it auto-discovers your Things3 database. +All of these accept the same flags in `args`: append `"--read-only=false"` to +enable writes, `"--toolsets=tasks,projects"` to mount a subset, or +`"--db", "/path/to/main.sqlite"` (before `"mcp"`) to pin a database — otherwise +it auto-discovers your Things3 database. ## How it works diff --git a/cmd/things/main.go b/cmd/things/main.go index 1fa833a..bc2c9f5 100644 --- a/cmd/things/main.go +++ b/cmd/things/main.go @@ -54,7 +54,7 @@ type CLI struct { Open OpenCmd `cmd:"" help:"Reveal a task, project, area, tag, or built-in list in Things3."` Import ImportCmd `cmd:"" help:"Batch create/update via the Things JSON URL scheme. Reads JSON from stdin or --file."` Skill SkillCmd `cmd:"" help:"Manage the bundled agent skill (Claude Code, etc.)."` - MCP MCPCmd `cmd:"" name:"mcp" help:"Run a read-only MCP server over stdio (for Claude Desktop, Cursor, etc.)."` + MCP MCPCmd `cmd:"" name:"mcp" help:"Run an MCP server over stdio (read-only by default; --read-only=false enables write tools). For Claude Desktop, Cursor, etc."` Ver VersionCmd `cmd:"" name:"version" help:"Print version and exit."` Completions CompletionsCmd `cmd:"" help:"Print a shell completion script (bash|zsh|fish)."` @@ -499,12 +499,20 @@ func (c *LogCmd) Run(_ *Deps) error { return things.LogCompleted() } -// MCPCmd runs a read-only Model Context Protocol server over stdio, exposing -// the CLI's read commands as MCP tools (see internal/mcpserver). Transport is -// stdio only; the global --db flag selects the database. -type MCPCmd struct{} +// MCPCmd runs a Model Context Protocol server over stdio, exposing the CLI's +// commands as MCP tools (see internal/mcpserver). Tools are grouped into +// toolsets the operator mounts à la carte; the server is read-only unless +// --read-only=false. Transport is stdio only; the global --db flag selects the +// database. +type MCPCmd struct { + Toolsets []string `help:"Toolsets to expose (comma-separated): tasks, projects, areas, tags, bulk, or all. Defaults to all." env:"THINGS_TOOLSETS"` + ReadOnly bool `help:"Expose only read tools. Pass --read-only=false (or --no-read-only) to enable write tools." default:"true" negatable:""` +} func (c *MCPCmd) Run(d *Deps) error { + if err := mcpserver.ValidateToolsets(c.Toolsets); err != nil { + return err + } path := d.DBPath if path == "" { p, err := db.FindDBPath() @@ -526,7 +534,9 @@ func (c *MCPCmd) Run(d *Deps) error { defer stop() return mcpserver.Serve(ctx, mcpserver.Config{ - Version: version, + Version: version, + Toolsets: c.Toolsets, + EnableWrites: !c.ReadOnly, Open: func() (mcpserver.Backend, error) { return db.Open(path) }, diff --git a/cmd/things/mcp_integration_test.go b/cmd/things/mcp_integration_test.go index 7ff1f76..504253f 100644 --- a/cmd/things/mcp_integration_test.go +++ b/cmd/things/mcp_integration_test.go @@ -16,10 +16,12 @@ import ( "github.com/ryanlewis/things-cli/internal/model" ) -// TestMCPStdioRoundTrip builds the binary, runs `things --db mcp`, and -// drives a full MCP session over its stdio (spawn → initialize → tools/list → -// tools/call). It is hermetic: no real Things3 database is required. Gated -// behind the `integration` build tag (run via `make test-integration`). +// TestMCPStdioRoundTrip builds the binary and drives full MCP sessions over its +// stdio (spawn → initialize → tools/list → tools/call) under several flag +// combinations. It is hermetic: a seeded temp DB stands in for real Things3, and +// write tools are only listed, never called (calling them would shell out to +// open/osascript). Gated behind the `integration` build tag (run via +// `make test-integration`). func TestMCPStdioRoundTrip(t *testing.T) { dbPath, sqlDB := dbtest.NewFileSQL(t) if _, err := sqlDB.Exec( @@ -34,53 +36,101 @@ func TestMCPStdioRoundTrip(t *testing.T) { t.Fatalf("build binary: %v\n%s", err, out) } - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) defer cancel() - transport := &mcp.CommandTransport{Command: exec.Command(bin, "--db", dbPath, "mcp")} + t.Run("default is read-only with six tools and serves a read", func(t *testing.T) { + cs := connectSpawned(t, ctx, bin, dbPath) + if names := listToolNames(t, ctx, cs); len(names) != 6 { + t.Fatalf("default tools/list = %d tools %v, want 6", len(names), names) + } + + res, err := cs.CallTool(ctx, &mcp.CallToolParams{ + Name: "things_search", + Arguments: map[string]any{"query": "milk"}, + }) + if err != nil { + t.Fatalf("tools/call: %v", err) + } + if res.IsError { + t.Fatalf("things_search reported a tool error: %+v", res.Content) + } + var text string + for _, c := range res.Content { + if tc, ok := c.(*mcp.TextContent); ok { + text += tc.Text + } + } + var tasks []model.Task + if err := json.Unmarshal([]byte(text), &tasks); err != nil { + t.Fatalf("result is not the CLI's JSON: %v\n%s", err, text) + } + found := false + for _, task := range tasks { + if task.Title == "Buy milk" { + found = true + } + } + if !found { + t.Errorf("round-trip did not return the seeded task: %s", text) + } + }) + + t.Run("toolsets trims the read tool list", func(t *testing.T) { + cs := connectSpawned(t, ctx, bin, dbPath, "--toolsets=tasks") + names := listToolNames(t, ctx, cs) + if len(names) != 3 { + t.Fatalf("--toolsets=tasks = %d tools %v, want 3", len(names), names) + } + for _, want := range []string{"things_list", "things_show", "things_search"} { + if !names[want] { + t.Errorf("missing tool %q (got %v)", want, names) + } + } + }) + + t.Run("read-only=false exposes the write tools", func(t *testing.T) { + cs := connectSpawned(t, ctx, bin, dbPath, "--read-only=false") + names := listToolNames(t, ctx, cs) + if len(names) != 14 { + t.Fatalf("--read-only=false = %d tools %v, want 14", len(names), names) + } + for _, want := range []string{ + "things_add", "things_edit", "things_complete", "things_cancel", + "things_add_project", "things_edit_project", "things_log", "things_import", + } { + if !names[want] { + t.Errorf("missing write tool %q (got %v)", want, names) + } + } + // Deliberately do not CALL any write tool here: that would shell out to + // open/osascript and mutate real Things. The recordingWriter unit tests + // cover write-handler behavior. + }) +} + +func connectSpawned(t *testing.T, ctx context.Context, bin, dbPath string, extraArgs ...string) *mcp.ClientSession { + t.Helper() + args := append([]string{"--db", dbPath, "mcp"}, extraArgs...) + transport := &mcp.CommandTransport{Command: exec.Command(bin, args...)} client := mcp.NewClient(&mcp.Implementation{Name: "test", Version: "test"}, nil) cs, err := client.Connect(ctx, transport, nil) if err != nil { - t.Fatalf("connect to spawned server: %v", err) + t.Fatalf("connect to spawned server (%v): %v", extraArgs, err) } - defer func() { _ = cs.Close() }() + t.Cleanup(func() { _ = cs.Close() }) + return cs +} +func listToolNames(t *testing.T, ctx context.Context, cs *mcp.ClientSession) map[string]bool { + t.Helper() tools, err := cs.ListTools(ctx, nil) if err != nil { t.Fatalf("tools/list: %v", err) } - if len(tools.Tools) != 6 { - t.Fatalf("tools/list returned %d tools, want 6", len(tools.Tools)) - } - - res, err := cs.CallTool(ctx, &mcp.CallToolParams{ - Name: "things_search", - Arguments: map[string]any{"query": "milk"}, - }) - if err != nil { - t.Fatalf("tools/call: %v", err) - } - if res.IsError { - t.Fatalf("things_search reported a tool error: %+v", res.Content) - } - - var text string - for _, c := range res.Content { - if tc, ok := c.(*mcp.TextContent); ok { - text += tc.Text - } - } - var tasks []model.Task - if err := json.Unmarshal([]byte(text), &tasks); err != nil { - t.Fatalf("result is not the CLI's JSON: %v\n%s", err, text) - } - found := false - for _, task := range tasks { - if task.Title == "Buy milk" { - found = true - } - } - if !found { - t.Errorf("round-trip did not return the seeded task: %s", text) + names := make(map[string]bool, len(tools.Tools)) + for _, tool := range tools.Tools { + names[tool.Name] = true } + return names } diff --git a/internal/mcpserver/server.go b/internal/mcpserver/server.go index 6c59744..373a87f 100644 --- a/internal/mcpserver/server.go +++ b/internal/mcpserver/server.go @@ -1,25 +1,31 @@ -// Package mcpserver exposes the read-only side of things-cli as a Model Context -// Protocol (MCP) server over stdio. +// Package mcpserver exposes things-cli as a Model Context Protocol (MCP) server +// over stdio. // -// It mirrors the CLI's read commands 1:1 and renders every result as the same -// JSON the CLI emits with --json, so MCP hosts that cannot shell out (Claude -// Desktop) — and those that can (Cursor, Claude Code) — get a typed alternative -// to driving the binary via Bash. Writes are intentionally out of scope. +// It mirrors the CLI's commands and renders read results as the same JSON the +// CLI emits with --json, so MCP hosts that cannot shell out (Claude Desktop) — +// and those that can (Cursor, Claude Code) — get a typed alternative to driving +// the binary via Bash. +// +// Tools are grouped into named toolsets (see Toolsets) that the operator mounts +// à la carte, GitHub-MCP style. Write tools are registered only when ReadOnly is +// false, so the default server is read-only. package mcpserver import ( "context" "fmt" + "strings" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/ryanlewis/things-cli/internal/db" "github.com/ryanlewis/things-cli/internal/model" + "github.com/ryanlewis/things-cli/internal/things" ) -// Backend is the read-only slice of the Things3 database the tools need. -// *db.DB satisfies it. Config.Open hands each tool call a fresh Backend -// (open-per-call, mirroring the CLI) which the handler closes when done. +// Backend is the read slice of the Things3 database the tools need. *db.DB +// satisfies it. Config.Open hands each tool call a fresh Backend (open-per-call, +// mirroring the CLI) which the handler closes when done. type Backend interface { ListTasks(view string, opts db.TaskFilter) ([]model.Task, error) GetTask(ref string) (*model.Task, error) @@ -28,9 +34,94 @@ type Backend interface { ListProjects(area string, completed bool) ([]model.Project, error) ListAreas() ([]model.Area, error) ListTags() ([]model.Tag, error) + // GetAuthToken returns the Things URL-scheme auth token (empty if unset). + // Required by the update-based write tools (edit / import). + GetAuthToken() (string, error) Close() error } +// Writer is the write surface the mutating tools need. The default +// implementation forwards to the package-level functions in internal/things +// (URL scheme for add/edit, AppleScript for complete/cancel). It is an interface +// so tests can record calls without shelling out to open/osascript. +type Writer interface { + AddTask(things.AddParams) error + AddProject(things.AddProjectParams) error + UpdateTask(things.UpdateParams) error + UpdateProject(things.UpdateProjectParams) error + CompleteTask(uuid string) error + CompleteProject(uuid string) error + CancelTask(uuid string) error + LogCompleted() error + ImportJSON(data, authToken string, reveal bool) error +} + +// thingsWriter is the production Writer: a thin pass-through to internal/things. +type thingsWriter struct{} + +func (thingsWriter) AddTask(p things.AddParams) error { return things.AddTask(p) } +func (thingsWriter) AddProject(p things.AddProjectParams) error { return things.AddProject(p) } +func (thingsWriter) UpdateTask(p things.UpdateParams) error { return things.UpdateTask(p) } +func (thingsWriter) UpdateProject(p things.UpdateProjectParams) error { + return things.UpdateProject(p) +} +func (thingsWriter) CompleteTask(uuid string) error { return things.CompleteTask(uuid) } +func (thingsWriter) CompleteProject(uuid string) error { return things.CompleteProject(uuid) } +func (thingsWriter) CancelTask(uuid string) error { return things.CancelTask(uuid) } +func (thingsWriter) LogCompleted() error { return things.LogCompleted() } +func (thingsWriter) ImportJSON(data, authToken string, reveal bool) error { + return things.ImportJSON(data, authToken, reveal) +} + +// AllToolsets lists every toolset name in registration order. Each is a domain +// that owns its read tools (always registered) and write tools (registered only +// when ReadOnly is false). +var AllToolsets = []string{"tasks", "projects", "areas", "tags", "bulk"} + +// ValidateToolsets reports the first unrecognized name in names, treating the +// special value "all" as valid. Empty input is valid (defaults to all). +func ValidateToolsets(names []string) error { + for _, n := range names { + n = strings.ToLower(strings.TrimSpace(n)) + if n == "all" { + continue + } + valid := false + for _, a := range AllToolsets { + if n == a { + valid = true + break + } + } + if !valid { + return fmt.Errorf("unknown toolset %q (valid: %s, all)", n, strings.Join(AllToolsets, ", ")) + } + } + return nil +} + +// resolveToolsets turns the configured names into a lookup set, expanding "all" +// and defaulting to every toolset when none are given. +func resolveToolsets(names []string) map[string]bool { + if len(names) == 0 { + names = []string{"all"} + } + set := make(map[string]bool, len(AllToolsets)) + for _, n := range names { + switch n = strings.ToLower(strings.TrimSpace(n)); n { + case "": + continue + case "all": + for _, a := range AllToolsets { + set[a] = true + } + default: + set[n] = true + } + } + return set +} + // Config configures the MCP server. type Config struct { // Open returns a fresh Backend for a single tool call. The handler that @@ -38,13 +129,28 @@ type Config struct { Open func() (Backend, error) // Version is reported to clients as the server implementation version. Version string + // Toolsets selects which toolsets to mount. Empty means all; "all" is an + // accepted alias. Unknown names are ignored here — validate up front with + // ValidateToolsets for a clear startup error. + Toolsets []string + // EnableWrites registers the write tools of the enabled toolsets. The zero + // value is read-only, so a default Config is always safe; the CLI flips this + // on via --read-only=false. + EnableWrites bool + // Writer backs the write tools. Defaults to the production internal/things + // pass-through when nil; tests inject a recording fake. + Writer Writer } -// NewServer builds an *mcp.Server with the read-only tools registered. It is +// NewServer builds an *mcp.Server with the configured tools registered. It is // exported so tests can drive it over an in-memory transport. func NewServer(cfg Config) *mcp.Server { + w := cfg.Writer + if w == nil { + w = thingsWriter{} + } s := mcp.NewServer(&mcp.Implementation{Name: "things", Version: cfg.Version}, nil) - registerTools(s, toolset{open: cfg.Open}) + registerTools(s, toolset{open: cfg.Open, write: w}, resolveToolsets(cfg.Toolsets), !cfg.EnableWrites) return s } @@ -54,5 +160,8 @@ func Serve(ctx context.Context, cfg Config) error { if cfg.Open == nil { return fmt.Errorf("mcpserver: Config.Open is required") } + if err := ValidateToolsets(cfg.Toolsets); err != nil { + return err + } return NewServer(cfg).Run(ctx, &mcp.StdioTransport{}) } diff --git a/internal/mcpserver/server_test.go b/internal/mcpserver/server_test.go index a80ca48..8707289 100644 --- a/internal/mcpserver/server_test.go +++ b/internal/mcpserver/server_test.go @@ -37,7 +37,17 @@ func connect(t *testing.T) *mcp.ClientSession { // in-memory transport and returns a connected client session. func session(t *testing.T, open func() (mcpserver.Backend, error)) *mcp.ClientSession { t.Helper() - srv := mcpserver.NewServer(mcpserver.Config{Version: "test", Open: open}) + return sessionCfg(t, mcpserver.Config{Open: open}) +} + +// sessionCfg is session with a caller-supplied Config (for toolset/write +// coverage). Version defaults to "test" when unset. +func sessionCfg(t *testing.T, cfg mcpserver.Config) *mcp.ClientSession { + t.Helper() + if cfg.Version == "" { + cfg.Version = "test" + } + srv := mcpserver.NewServer(cfg) ctx := context.Background() serverT, clientT := mcp.NewInMemoryTransports() diff --git a/internal/mcpserver/tools.go b/internal/mcpserver/tools.go index f8f759b..9b690e7 100644 --- a/internal/mcpserver/tools.go +++ b/internal/mcpserver/tools.go @@ -3,6 +3,7 @@ package mcpserver import ( "bytes" "context" + "encoding/json" "errors" "fmt" "strings" @@ -15,13 +16,18 @@ import ( "github.com/ryanlewis/things-cli/internal/things" ) -// toolset binds the per-call backend opener that every tool handler shares. +// toolset binds the per-call backend opener and the write surface that the tool +// handlers share. open is used by read tools and by write tools that must +// resolve a task reference or read the auth token; write is the mutation surface. type toolset struct { - open func() (Backend, error) + open func() (Backend, error) + write Writer } // Tool descriptions are the entire UX surface for hosts without Bash or the // agent skill (Claude Desktop), so they spell out behavior and return shape. +// Write tools shout that they MUTATE the database and whether the result is +// confirmed (synchronous AppleScript) or merely submitted (async URL scheme). const ( listDesc = "List Things3 to-dos from a built-in list/view as JSON. " + "Views: today, inbox, upcoming, anytime, someday, logbook, trash, deadlines (default: today). " + @@ -35,18 +41,71 @@ const ( projectsDesc = "List Things3 projects as JSON, optionally filtered by area and optionally including completed projects. Read-only." areasDesc = "List Things3 areas as JSON. Read-only." tagsDesc = "List Things3 tags as JSON. Read-only." + + addDesc = "Create a new Things3 to-do. MUTATES your Things database. " + + "Only title is required; optionally set notes, a schedule (when), deadline, tags, checklist items, " + + "a target list (project or area name), and a heading. Submitted via the Things URL scheme: creation is " + + "asynchronous and NOT confirmed by this tool, and any payload error surfaces only as an in-app Things notification." + addProjectDesc = "Create a new Things3 project. MUTATES your Things database. " + + "Only title is required; optionally set notes, schedule, deadline, tags, an area, and initial to-dos. " + + "Submitted asynchronously via the URL scheme and not confirmed by this tool." + editDesc = "Edit an existing Things3 to-do. MUTATES your Things database. " + + "Identify it by UUID (preferred) or title; an ambiguous title returns candidates so you can retry with a UUID. " + + "Any field you set is changed, omitted fields are left as-is, and an explicit empty string clears that field. " + + "Can also complete, cancel, or duplicate the to-do. Requires the Things URL auth token " + + "(Things → Settings → General → Enable Things URLs). Submitted asynchronously; not confirmed by this tool." + editProjectDesc = "Edit an existing Things3 project. MUTATES your Things database. " + + "Identify it by UUID (preferred) or title. Set fields to change, omit to leave as-is, empty string to clear. " + + "Requires the Things URL auth token (Things → Settings → General → Enable Things URLs). " + + "Submitted asynchronously; not confirmed by this tool." + completeDesc = "Mark a Things3 to-do as completed. MUTATES your Things database. " + + "Identify it by UUID (preferred) or title. If the target is a project, the project AND all of its to-dos are " + + "completed. Applied synchronously via AppleScript (requires Things3 to be running)." + cancelDesc = "Cancel a Things3 to-do. MUTATES your Things database. " + + "Identify it by UUID (preferred) or title. Applies to to-dos only — cancel a project with things_edit_project " + + "(cancel=true). Applied synchronously via AppleScript (requires Things3 to be running)." + logDesc = "Move completed and cancelled items from Today to the Logbook (Things → Items → Log Completed). " + + "MUTATES your Things database; applied synchronously via AppleScript (requires Things3 to be running). Takes no arguments." + importDesc = "Batch create or update Things3 items from a Things JSON array payload. MUTATES your Things database. " + + "`data` is the JSON array accepted by the Things JSON URL scheme; items with operation \"update\" use the auth " + + "token (sent automatically when configured). Submitted asynchronously and not confirmed by this tool. " + + "Advanced — prefer things_add for a single to-do." ) -// registerTools wires the six read-only tools onto the server. Input schemas -// are inferred from the struct types (jsonschema tags supply field -// descriptions; fields without `omitempty` are required). -func registerTools(s *mcp.Server, t toolset) { - mcp.AddTool(s, &mcp.Tool{Name: "things_list", Description: listDesc}, t.list) - mcp.AddTool(s, &mcp.Tool{Name: "things_show", Description: showDesc}, t.show) - mcp.AddTool(s, &mcp.Tool{Name: "things_search", Description: searchDesc}, t.search) - mcp.AddTool(s, &mcp.Tool{Name: "things_projects", Description: projectsDesc}, t.projects) - mcp.AddTool(s, &mcp.Tool{Name: "things_areas", Description: areasDesc}, t.areas) - mcp.AddTool(s, &mcp.Tool{Name: "things_tags", Description: tagsDesc}, t.tags) +// registerTools wires the tools for the enabled toolsets onto the server. Read +// tools are always registered for an enabled toolset; write tools are added only +// when readOnly is false. Input schemas are inferred from the struct types +// (jsonschema tags supply field descriptions; fields without `omitempty` are +// required). +func registerTools(s *mcp.Server, t toolset, sets map[string]bool, readOnly bool) { + if sets["tasks"] { + mcp.AddTool(s, &mcp.Tool{Name: "things_list", Description: listDesc}, t.list) + mcp.AddTool(s, &mcp.Tool{Name: "things_show", Description: showDesc}, t.show) + mcp.AddTool(s, &mcp.Tool{Name: "things_search", Description: searchDesc}, t.search) + if !readOnly { + mcp.AddTool(s, &mcp.Tool{Name: "things_add", Description: addDesc}, t.add) + mcp.AddTool(s, &mcp.Tool{Name: "things_edit", Description: editDesc}, t.edit) + mcp.AddTool(s, &mcp.Tool{Name: "things_complete", Description: completeDesc}, t.complete) + mcp.AddTool(s, &mcp.Tool{Name: "things_cancel", Description: cancelDesc}, t.cancel) + } + } + if sets["projects"] { + mcp.AddTool(s, &mcp.Tool{Name: "things_projects", Description: projectsDesc}, t.projects) + if !readOnly { + mcp.AddTool(s, &mcp.Tool{Name: "things_add_project", Description: addProjectDesc}, t.addProject) + mcp.AddTool(s, &mcp.Tool{Name: "things_edit_project", Description: editProjectDesc}, t.editProject) + } + } + if sets["areas"] { + mcp.AddTool(s, &mcp.Tool{Name: "things_areas", Description: areasDesc}, t.areas) + } + if sets["tags"] { + mcp.AddTool(s, &mcp.Tool{Name: "things_tags", Description: tagsDesc}, t.tags) + } + if sets["bulk"] && !readOnly { + mcp.AddTool(s, &mcp.Tool{Name: "things_log", Description: logDesc}, t.log) + mcp.AddTool(s, &mcp.Tool{Name: "things_import", Description: importDesc}, t.importJSON) + } } type listInput struct { @@ -75,6 +134,88 @@ type projectsInput struct { // emptyInput is the object schema for tools that take no arguments. type emptyInput struct{} +// --- write tool inputs --------------------------------------------------- +// +// add/add_project take list-like fields as typed arrays (more natural for an +// agent than the CLI's delimited strings); they are joined before handing off +// to internal/things. edit/edit_project keep pointer string fields so the +// "omit = leave unchanged, empty = clear" semantics of the CLI survive exactly. + +type addInput struct { + Title string `json:"title" jsonschema:"Title of the new to-do (required)."` + Notes string `json:"notes,omitempty" jsonschema:"Note body for the to-do."` + When string `json:"when,omitempty" jsonschema:"Schedule: today, tomorrow, evening, anytime, someday, a YYYY-MM-DD date, HH:MM, YYYY-MM-DD@HH:MM, or RFC3339."` + Deadline string `json:"deadline,omitempty" jsonschema:"Deadline date (YYYY-MM-DD)."` + Tags []string `json:"tags,omitempty" jsonschema:"Tag names to apply (must already exist in Things)."` + Checklist []string `json:"checklist,omitempty" jsonschema:"Checklist item titles, in order."` + List string `json:"list,omitempty" jsonschema:"Target list: a project or area name (or UUID)."` + Heading string `json:"heading,omitempty" jsonschema:"Heading within the target project to file the to-do under."` +} + +type addProjectInput struct { + Title string `json:"title" jsonschema:"Title of the new project (required)."` + Notes string `json:"notes,omitempty" jsonschema:"Note body for the project."` + When string `json:"when,omitempty" jsonschema:"Schedule: today, tomorrow, evening, anytime, someday, a YYYY-MM-DD date, HH:MM, YYYY-MM-DD@HH:MM, or RFC3339."` + Deadline string `json:"deadline,omitempty" jsonschema:"Deadline date (YYYY-MM-DD)."` + Tags []string `json:"tags,omitempty" jsonschema:"Tag names to apply (must already exist in Things)."` + Area string `json:"area,omitempty" jsonschema:"Area name or UUID to file the project under."` + Todos []string `json:"todos,omitempty" jsonschema:"Initial to-do titles to create inside the project, in order."` +} + +type editInput struct { + Task string `json:"task" jsonschema:"To-do to edit: a UUID (preferred) or title. An ambiguous title returns candidates to retry with a UUID."` + Title *string `json:"title,omitempty" jsonschema:"Replace the title."` + Notes *string `json:"notes,omitempty" jsonschema:"Replace the notes (empty string clears them)."` + PrependNotes *string `json:"prepend_notes,omitempty" jsonschema:"Text to insert before the existing notes."` + AppendNotes *string `json:"append_notes,omitempty" jsonschema:"Text to append after the existing notes."` + When *string `json:"when,omitempty" jsonschema:"Reschedule (today/tomorrow/evening/anytime/someday, YYYY-MM-DD, HH:MM, YYYY-MM-DD@HH:MM, RFC3339); empty string clears the schedule."` + Deadline *string `json:"deadline,omitempty" jsonschema:"Set the deadline (YYYY-MM-DD); empty string clears it."` + Tags *string `json:"tags,omitempty" jsonschema:"Replace all tags with this comma-separated list; empty string clears tags."` + AddTags *string `json:"add_tags,omitempty" jsonschema:"Add these comma-separated tags to the existing tags."` + Checklist *string `json:"checklist,omitempty" jsonschema:"Replace checklist items (newline-separated)."` + PrependChecklist *string `json:"prepend_checklist,omitempty" jsonschema:"Checklist items (newline-separated) to insert before the existing ones."` + AppendChecklist *string `json:"append_checklist,omitempty" jsonschema:"Checklist items (newline-separated) to append after the existing ones."` + List *string `json:"list,omitempty" jsonschema:"Move to a list/project by name."` + ListID *string `json:"list_id,omitempty" jsonschema:"Move to a list/project by UUID."` + Heading *string `json:"heading,omitempty" jsonschema:"Set the heading within the project by name."` + HeadingID *string `json:"heading_id,omitempty" jsonschema:"Set the heading by UUID."` + Complete bool `json:"complete,omitempty" jsonschema:"Mark the to-do completed."` + Cancel bool `json:"cancel,omitempty" jsonschema:"Mark the to-do canceled."` + Duplicate bool `json:"duplicate,omitempty" jsonschema:"Duplicate the to-do before applying edits."` + Reveal bool `json:"reveal,omitempty" jsonschema:"Reveal the to-do in Things after editing."` +} + +type editProjectInput struct { + Project string `json:"project" jsonschema:"Project to edit: a UUID (preferred) or title."` + Title *string `json:"title,omitempty" jsonschema:"Replace the title."` + Notes *string `json:"notes,omitempty" jsonschema:"Replace the notes (empty string clears them)."` + PrependNotes *string `json:"prepend_notes,omitempty" jsonschema:"Text to insert before the existing notes."` + AppendNotes *string `json:"append_notes,omitempty" jsonschema:"Text to append after the existing notes."` + When *string `json:"when,omitempty" jsonschema:"Reschedule (today/tomorrow/evening/anytime/someday, YYYY-MM-DD, HH:MM, YYYY-MM-DD@HH:MM, RFC3339); empty string clears the schedule."` + Deadline *string `json:"deadline,omitempty" jsonschema:"Set the deadline (YYYY-MM-DD); empty string clears it."` + Tags *string `json:"tags,omitempty" jsonschema:"Replace all tags with this comma-separated list; empty string clears tags."` + AddTags *string `json:"add_tags,omitempty" jsonschema:"Add these comma-separated tags to the existing tags."` + Area *string `json:"area,omitempty" jsonschema:"Move to an area by name."` + AreaID *string `json:"area_id,omitempty" jsonschema:"Move to an area by UUID."` + Complete bool `json:"complete,omitempty" jsonschema:"Mark the project completed."` + Cancel bool `json:"cancel,omitempty" jsonschema:"Mark the project canceled."` + Duplicate bool `json:"duplicate,omitempty" jsonschema:"Duplicate the project before applying edits."` + Reveal bool `json:"reveal,omitempty" jsonschema:"Reveal the project in Things after editing."` +} + +type completeInput struct { + Task string `json:"task" jsonschema:"To-do (or project) to complete: a UUID (preferred) or title."` +} + +type cancelInput struct { + Task string `json:"task" jsonschema:"To-do to cancel: a UUID (preferred) or title."` +} + +type importInput struct { + Data string `json:"data" jsonschema:"A Things JSON array payload (the format accepted by the things:///json URL scheme)."` + Reveal bool `json:"reveal,omitempty" jsonschema:"Reveal the imported items in Things afterwards."` +} + func (t toolset) list(_ context.Context, _ *mcp.CallToolRequest, in listInput) (*mcp.CallToolResult, any, error) { view, filter, err := resolveListQuery(in) if err != nil { @@ -105,12 +246,8 @@ func (t toolset) show(_ context.Context, _ *mcp.CallToolRequest, in showInput) ( return nil, nil, err } defer func() { _ = b.Close() }() - task, err := b.GetTask(in.Task) + task, err := resolveRef(b, in.Task) if err != nil { - var ambig *db.AmbiguousTaskError - if errors.As(err, &ambig) { - return nil, nil, ambiguousError(ambig) - } return nil, nil, err } items, err := b.GetChecklistItems(task.UUID) @@ -124,6 +261,186 @@ func (t toolset) show(_ context.Context, _ *mcp.CallToolRequest, in showInput) ( return textResult(buf.String()), nil, nil } +func (t toolset) add(_ context.Context, _ *mcp.CallToolRequest, in addInput) (*mcp.CallToolResult, any, error) { + if err := t.write.AddTask(things.AddParams{ + Title: in.Title, + Notes: in.Notes, + When: in.When, + Deadline: in.Deadline, + Tags: strings.Join(in.Tags, ","), + Checklist: strings.Join(in.Checklist, "\n"), + Heading: in.Heading, + List: in.List, + }); err != nil { + return nil, nil, err + } + return textResult(fmt.Sprintf("Submitted new to-do %q to Things. Creation is asynchronous and not confirmed by this tool; check Things to verify.", in.Title)), nil, nil +} + +func (t toolset) addProject(_ context.Context, _ *mcp.CallToolRequest, in addProjectInput) (*mcp.CallToolResult, any, error) { + if err := t.write.AddProject(things.AddProjectParams{ + Title: in.Title, + Notes: in.Notes, + When: in.When, + Deadline: in.Deadline, + Tags: strings.Join(in.Tags, ","), + Area: in.Area, + Todos: strings.Join(in.Todos, "\n"), + }); err != nil { + return nil, nil, err + } + return textResult(fmt.Sprintf("Submitted new project %q to Things. Creation is asynchronous and not confirmed by this tool; check Things to verify.", in.Title)), nil, nil +} + +func (t toolset) edit(_ context.Context, _ *mcp.CallToolRequest, in editInput) (*mcp.CallToolResult, any, error) { + b, err := t.open() + if err != nil { + return nil, nil, err + } + defer func() { _ = b.Close() }() + task, err := resolveRef(b, in.Task) + if err != nil { + return nil, nil, err + } + token, err := b.GetAuthToken() + if err != nil { + return nil, nil, err + } + if err := t.write.UpdateTask(things.UpdateParams{ + ID: task.UUID, + AuthToken: token, + Title: in.Title, + Notes: in.Notes, + PrependNotes: in.PrependNotes, + AppendNotes: in.AppendNotes, + When: in.When, + Deadline: in.Deadline, + Tags: in.Tags, + AddTags: in.AddTags, + Checklist: in.Checklist, + PrependChecklist: in.PrependChecklist, + AppendChecklist: in.AppendChecklist, + List: in.List, + ListID: in.ListID, + Heading: in.Heading, + HeadingID: in.HeadingID, + Completed: in.Complete, + Canceled: in.Cancel, + Duplicate: in.Duplicate, + Reveal: in.Reveal, + }); err != nil { + return nil, nil, err + } + return textResult(fmt.Sprintf("Submitted edit to to-do %q (%s). Changes are applied asynchronously and not confirmed by this tool.", task.Title, task.UUID)), nil, nil +} + +func (t toolset) editProject(_ context.Context, _ *mcp.CallToolRequest, in editProjectInput) (*mcp.CallToolResult, any, error) { + b, err := t.open() + if err != nil { + return nil, nil, err + } + defer func() { _ = b.Close() }() + project, err := resolveRef(b, in.Project) + if err != nil { + return nil, nil, err + } + if project.Type != model.TypeProject { + return nil, nil, fmt.Errorf("not a project: %s", project.Title) + } + token, err := b.GetAuthToken() + if err != nil { + return nil, nil, err + } + if err := t.write.UpdateProject(things.UpdateProjectParams{ + ID: project.UUID, + AuthToken: token, + Title: in.Title, + Notes: in.Notes, + PrependNotes: in.PrependNotes, + AppendNotes: in.AppendNotes, + When: in.When, + Deadline: in.Deadline, + Tags: in.Tags, + AddTags: in.AddTags, + Area: in.Area, + AreaID: in.AreaID, + Completed: in.Complete, + Canceled: in.Cancel, + Duplicate: in.Duplicate, + Reveal: in.Reveal, + }); err != nil { + return nil, nil, err + } + return textResult(fmt.Sprintf("Submitted edit to project %q (%s). Changes are applied asynchronously and not confirmed by this tool.", project.Title, project.UUID)), nil, nil +} + +func (t toolset) complete(_ context.Context, _ *mcp.CallToolRequest, in completeInput) (*mcp.CallToolResult, any, error) { + b, err := t.open() + if err != nil { + return nil, nil, err + } + defer func() { _ = b.Close() }() + task, err := resolveRef(b, in.Task) + if err != nil { + return nil, nil, err + } + if task.Type == model.TypeProject { + if err := t.write.CompleteProject(task.UUID); err != nil { + return nil, nil, err + } + return textResult(fmt.Sprintf("Completed project %q (%s) and all of its to-dos.", task.Title, task.UUID)), nil, nil + } + if err := t.write.CompleteTask(task.UUID); err != nil { + return nil, nil, err + } + return textResult(fmt.Sprintf("Completed to-do %q (%s).", task.Title, task.UUID)), nil, nil +} + +func (t toolset) cancel(_ context.Context, _ *mcp.CallToolRequest, in cancelInput) (*mcp.CallToolResult, any, error) { + b, err := t.open() + if err != nil { + return nil, nil, err + } + defer func() { _ = b.Close() }() + task, err := resolveRef(b, in.Task) + if err != nil { + return nil, nil, err + } + if task.Type == model.TypeProject { + return nil, nil, fmt.Errorf("%q is a project; cancel it with things_edit_project (cancel=true)", task.Title) + } + if err := t.write.CancelTask(task.UUID); err != nil { + return nil, nil, err + } + return textResult(fmt.Sprintf("Cancelled to-do %q (%s).", task.Title, task.UUID)), nil, nil +} + +func (t toolset) log(_ context.Context, _ *mcp.CallToolRequest, _ emptyInput) (*mcp.CallToolResult, any, error) { + if err := t.write.LogCompleted(); err != nil { + return nil, nil, err + } + return textResult("Logged completed and cancelled items to the Logbook."), nil, nil +} + +func (t toolset) importJSON(_ context.Context, _ *mcp.CallToolRequest, in importInput) (*mcp.CallToolResult, any, error) { + if err := validateImportPayload(in.Data); err != nil { + return nil, nil, err + } + b, err := t.open() + if err != nil { + return nil, nil, err + } + defer func() { _ = b.Close() }() + token, err := b.GetAuthToken() + if err != nil { + return nil, nil, err + } + if err := t.write.ImportJSON(in.Data, token, in.Reveal); err != nil { + return nil, nil, err + } + return textResult("Submitted import payload to Things. Processing is asynchronous and not confirmed by this tool; check Things for any error notification."), nil, nil +} + // query opens a fresh backend (open-per-call, mirroring the CLI), runs fetch, // renders the result as the CLI's --json output, and closes the backend. func (t toolset) query(fetch func(Backend) (any, error)) (*mcp.CallToolResult, any, error) { @@ -139,6 +456,21 @@ func (t toolset) query(fetch func(Backend) (any, error)) (*mcp.CallToolResult, a return jsonResult(v) } +// resolveRef resolves a UUID-or-title reference to a task, mapping an ambiguous +// title to a candidate-listing tool error. It is the non-interactive subset of +// the CLI's resolveTask (no last-list cache index, no stdin prompt). +func resolveRef(b Backend, ref string) (*model.Task, error) { + task, err := b.GetTask(ref) + if err != nil { + var ambig *db.AmbiguousTaskError + if errors.As(err, &ambig) { + return nil, ambiguousError(ambig) + } + return nil, err + } + return task, nil +} + // resolveListQuery mirrors ListCmd.Run: default the view (to the project view // when a project is given, else today), validate it, and apply date filters. func resolveListQuery(in listInput) (string, db.TaskFilter, error) { @@ -206,6 +538,24 @@ func applyDateFilters(filter *db.TaskFilter, view, on, from, to string) error { return nil } +// validateImportPayload rejects obvious garbage before handing the payload to +// the URL scheme (which reports its own errors only as an in-app notification). +// It is a lighter cousin of the CLI's validateImportJSON: non-empty, valid JSON, +// shaped as an array. +func validateImportPayload(data string) error { + trimmed := strings.TrimSpace(data) + if trimmed == "" { + return fmt.Errorf("import: data is empty") + } + if !json.Valid([]byte(data)) { + return fmt.Errorf("import: data is not valid JSON") + } + if trimmed[0] != '[' { + return fmt.Errorf("import: data must be a JSON array of items") + } + return nil +} + // jsonResult renders v as the CLI's --json output and wraps it as tool content. func jsonResult(v any) (*mcp.CallToolResult, any, error) { var buf bytes.Buffer diff --git a/internal/mcpserver/writes_test.go b/internal/mcpserver/writes_test.go new file mode 100644 index 0000000..e4256f1 --- /dev/null +++ b/internal/mcpserver/writes_test.go @@ -0,0 +1,424 @@ +package mcpserver_test + +import ( + "context" + "strings" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + + "github.com/ryanlewis/things-cli/internal/db" + "github.com/ryanlewis/things-cli/internal/mcpserver" + "github.com/ryanlewis/things-cli/internal/model" + "github.com/ryanlewis/things-cli/internal/things" +) + +// recordingWriter is a fake mcpserver.Writer that captures the last call to each +// method instead of shelling out to open/osascript, so write handlers can be +// tested for the params they forward. +type recordingWriter struct { + addTask *things.AddParams + addProject *things.AddProjectParams + updateTask *things.UpdateParams + updateProject *things.UpdateProjectParams + completeTask string + completeProject string + cancelTask string + logged bool + importData string + importToken string + importReveal bool + err error // returned by every method when set +} + +func (w *recordingWriter) AddTask(p things.AddParams) error { w.addTask = &p; return w.err } +func (w *recordingWriter) AddProject(p things.AddProjectParams) error { + w.addProject = &p + return w.err +} +func (w *recordingWriter) UpdateTask(p things.UpdateParams) error { w.updateTask = &p; return w.err } +func (w *recordingWriter) UpdateProject(p things.UpdateProjectParams) error { + w.updateProject = &p + return w.err +} +func (w *recordingWriter) CompleteTask(uuid string) error { w.completeTask = uuid; return w.err } +func (w *recordingWriter) CompleteProject(uuid string) error { w.completeProject = uuid; return w.err } +func (w *recordingWriter) CancelTask(uuid string) error { w.cancelTask = uuid; return w.err } +func (w *recordingWriter) LogCompleted() error { w.logged = true; return w.err } +func (w *recordingWriter) ImportJSON(data, token string, reveal bool) error { + w.importData, w.importToken, w.importReveal = data, token, reveal + return w.err +} + +// fakeBackend serves a fixed task and auth token to the resolve+token path of +// the write handlers. The embedded nil Backend panics if any other method is +// reached, which keeps the handlers honest about what they touch. +type fakeBackend struct { + mcpserver.Backend + task *model.Task + getErr error + token string +} + +func (fakeBackend) Close() error { return nil } +func (f fakeBackend) GetTask(string) (*model.Task, error) { + if f.getErr != nil { + return nil, f.getErr + } + return f.task, nil +} +func (f fakeBackend) GetAuthToken() (string, error) { return f.token, nil } + +func openFake(b fakeBackend) func() (mcpserver.Backend, error) { + return func() (mcpserver.Backend, error) { return b, nil } +} + +// mustNotOpen fails the test if a handler opens the database — used for tools +// (add, add_project, log) that mutate without needing the DB. +func mustNotOpen(t *testing.T) func() (mcpserver.Backend, error) { + t.Helper() + return func() (mcpserver.Backend, error) { + t.Error("backend opened, but this tool should not need the database") + return fakeBackend{}, nil + } +} + +func writeSession(t *testing.T, w mcpserver.Writer, open func() (mcpserver.Backend, error)) *mcp.ClientSession { + t.Helper() + return sessionCfg(t, mcpserver.Config{EnableWrites: true, Writer: w, Open: open}) +} + +func toolNames(t *testing.T, cs *mcp.ClientSession) map[string]bool { + t.Helper() + res, err := cs.ListTools(context.Background(), nil) + if err != nil { + t.Fatalf("ListTools: %v", err) + } + names := make(map[string]bool, len(res.Tools)) + for _, tool := range res.Tools { + names[tool.Name] = true + } + return names +} + +func TestToolsetGating(t *testing.T) { + noOpen := func() (mcpserver.Backend, error) { return fakeBackend{}, nil } + cases := []struct { + name string + cfg mcpserver.Config + want []string + }{ + {"default is read-only, all toolsets", mcpserver.Config{Open: noOpen}, []string{ + "things_list", "things_show", "things_search", "things_projects", "things_areas", "things_tags", + }}, + {"tasks read-only", mcpserver.Config{Open: noOpen, Toolsets: []string{"tasks"}}, []string{ + "things_list", "things_show", "things_search", + }}, + {"tasks with writes", mcpserver.Config{Open: noOpen, Toolsets: []string{"tasks"}, EnableWrites: true}, []string{ + "things_list", "things_show", "things_search", "things_add", "things_edit", "things_complete", "things_cancel", + }}, + {"areas has no writes", mcpserver.Config{Open: noOpen, Toolsets: []string{"areas"}, EnableWrites: true}, []string{ + "things_areas", + }}, + {"bulk is empty when read-only", mcpserver.Config{Open: noOpen, Toolsets: []string{"bulk"}}, nil}, + {"bulk with writes", mcpserver.Config{Open: noOpen, Toolsets: []string{"bulk"}, EnableWrites: true}, []string{ + "things_log", "things_import", + }}, + {"all with writes is full parity", mcpserver.Config{Open: noOpen, Toolsets: []string{"all"}, EnableWrites: true}, []string{ + "things_list", "things_show", "things_search", "things_add", "things_edit", "things_complete", "things_cancel", + "things_projects", "things_add_project", "things_edit_project", + "things_areas", "things_tags", "things_log", "things_import", + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := toolNames(t, sessionCfg(t, tc.cfg)) + if len(got) != len(tc.want) { + t.Errorf("got %d tools %v, want %d %v", len(got), got, len(tc.want), tc.want) + } + for _, w := range tc.want { + if !got[w] { + t.Errorf("missing tool %q (got %v)", w, got) + } + } + }) + } +} + +func TestAddForwarding(t *testing.T) { + w := &recordingWriter{} + cs := writeSession(t, w, mustNotOpen(t)) + text, isErr := call(t, cs, "things_add", map[string]any{ + "title": "Buy milk", + "notes": "2%", + "when": "today", + "tags": []any{"urgent", "home"}, + "checklist": []any{"semi-skimmed", "lactose free"}, + "list": "Chores", + "heading": "Errands", + }) + if isErr { + t.Fatalf("unexpected tool error: %s", text) + } + if w.addTask == nil { + t.Fatal("AddTask was not called") + } + if w.addTask.Title != "Buy milk" || w.addTask.Notes != "2%" || w.addTask.When != "today" { + t.Errorf("scalar fields not forwarded: %+v", *w.addTask) + } + if w.addTask.Tags != "urgent,home" { + t.Errorf("tags = %q, want comma-joined", w.addTask.Tags) + } + if w.addTask.Checklist != "semi-skimmed\nlactose free" { + t.Errorf("checklist = %q, want newline-joined", w.addTask.Checklist) + } + if w.addTask.List != "Chores" || w.addTask.Heading != "Errands" { + t.Errorf("list/heading not forwarded: %+v", *w.addTask) + } + if !strings.Contains(text, "Submitted new to-do") { + t.Errorf("result should note async submission: %s", text) + } +} + +func TestAddProjectForwarding(t *testing.T) { + w := &recordingWriter{} + cs := writeSession(t, w, mustNotOpen(t)) + text, isErr := call(t, cs, "things_add_project", map[string]any{ + "title": "House move", + "area": "Home", + "tags": []any{"big"}, + "todos": []any{"pack", "hire van"}, + }) + if isErr { + t.Fatalf("unexpected tool error: %s", text) + } + if w.addProject == nil { + t.Fatal("AddProject was not called") + } + if w.addProject.Title != "House move" || w.addProject.Area != "Home" { + t.Errorf("title/area not forwarded: %+v", *w.addProject) + } + if w.addProject.Tags != "big" || w.addProject.Todos != "pack\nhire van" { + t.Errorf("tags/todos not joined: %+v", *w.addProject) + } +} + +func TestEditForwarding(t *testing.T) { + w := &recordingWriter{} + task := &model.Task{UUID: "task-1", Title: "Buy milk", Type: model.TypeTask} + cs := writeSession(t, w, openFake(fakeBackend{task: task, token: "secret"})) + text, isErr := call(t, cs, "things_edit", map[string]any{ + "task": "task-1", + "title": "Buy oat milk", + "complete": true, + }) + if isErr { + t.Fatalf("unexpected tool error: %s", text) + } + if w.updateTask == nil { + t.Fatal("UpdateTask was not called") + } + if w.updateTask.ID != "task-1" { + t.Errorf("ID = %q, want resolved UUID task-1", w.updateTask.ID) + } + if w.updateTask.AuthToken != "secret" { + t.Errorf("AuthToken = %q, want the backend's token", w.updateTask.AuthToken) + } + if w.updateTask.Title == nil || *w.updateTask.Title != "Buy oat milk" { + t.Errorf("Title pointer not forwarded: %+v", w.updateTask.Title) + } + if !w.updateTask.Completed { + t.Errorf("Completed flag not forwarded") + } +} + +func TestEditProjectForwarding(t *testing.T) { + w := &recordingWriter{} + proj := &model.Task{UUID: "proj-1", Title: "Chores", Type: model.TypeProject} + cs := writeSession(t, w, openFake(fakeBackend{task: proj, token: "tok"})) + text, isErr := call(t, cs, "things_edit_project", map[string]any{ + "project": "proj-1", + "title": "Household", + }) + if isErr { + t.Fatalf("unexpected tool error: %s", text) + } + if w.updateProject == nil { + t.Fatal("UpdateProject was not called") + } + if w.updateProject.ID != "proj-1" || w.updateProject.AuthToken != "tok" { + t.Errorf("ID/token not forwarded: %+v", *w.updateProject) + } + if w.updateProject.Title == nil || *w.updateProject.Title != "Household" { + t.Errorf("Title not forwarded: %+v", w.updateProject.Title) + } +} + +func TestEditProjectRejectsNonProject(t *testing.T) { + w := &recordingWriter{} + todo := &model.Task{UUID: "task-1", Title: "Buy milk", Type: model.TypeTask} + cs := writeSession(t, w, openFake(fakeBackend{task: todo, token: "tok"})) + text, isErr := call(t, cs, "things_edit_project", map[string]any{"project": "task-1", "title": "x"}) + if !isErr { + t.Fatalf("expected error editing a non-project, got: %s", text) + } + if !strings.Contains(text, "not a project") { + t.Errorf("error should say not a project: %s", text) + } + if w.updateProject != nil { + t.Errorf("UpdateProject should not be called for a to-do") + } +} + +func TestCompleteForwarding(t *testing.T) { + t.Run("to-do", func(t *testing.T) { + w := &recordingWriter{} + cs := writeSession(t, w, openFake(fakeBackend{task: &model.Task{UUID: "task-1", Title: "Buy milk", Type: model.TypeTask}})) + text, isErr := call(t, cs, "things_complete", map[string]any{"task": "task-1"}) + if isErr { + t.Fatalf("unexpected tool error: %s", text) + } + if w.completeTask != "task-1" { + t.Errorf("CompleteTask = %q, want task-1", w.completeTask) + } + if w.completeProject != "" { + t.Errorf("CompleteProject should not be called for a to-do") + } + if !strings.Contains(text, "Completed to-do") { + t.Errorf("result text = %q", text) + } + }) + + t.Run("project completes cascade", func(t *testing.T) { + w := &recordingWriter{} + cs := writeSession(t, w, openFake(fakeBackend{task: &model.Task{UUID: "proj-1", Title: "Chores", Type: model.TypeProject}})) + text, isErr := call(t, cs, "things_complete", map[string]any{"task": "proj-1"}) + if isErr { + t.Fatalf("unexpected tool error: %s", text) + } + if w.completeProject != "proj-1" { + t.Errorf("CompleteProject = %q, want proj-1", w.completeProject) + } + if !strings.Contains(text, "Completed project") { + t.Errorf("result text = %q", text) + } + }) +} + +func TestCancelForwarding(t *testing.T) { + t.Run("to-do", func(t *testing.T) { + w := &recordingWriter{} + cs := writeSession(t, w, openFake(fakeBackend{task: &model.Task{UUID: "task-1", Title: "Buy milk", Type: model.TypeTask}})) + text, isErr := call(t, cs, "things_cancel", map[string]any{"task": "task-1"}) + if isErr { + t.Fatalf("unexpected tool error: %s", text) + } + if w.cancelTask != "task-1" { + t.Errorf("CancelTask = %q, want task-1", w.cancelTask) + } + }) + + t.Run("project rejected", func(t *testing.T) { + w := &recordingWriter{} + cs := writeSession(t, w, openFake(fakeBackend{task: &model.Task{UUID: "proj-1", Title: "Chores", Type: model.TypeProject}})) + text, isErr := call(t, cs, "things_cancel", map[string]any{"task": "proj-1"}) + if !isErr { + t.Fatalf("expected error cancelling a project, got: %s", text) + } + if !strings.Contains(text, "is a project") { + t.Errorf("error should point at edit_project: %s", text) + } + if w.cancelTask != "" { + t.Errorf("CancelTask should not be called for a project") + } + }) +} + +func TestLogForwarding(t *testing.T) { + w := &recordingWriter{} + cs := writeSession(t, w, mustNotOpen(t)) + text, isErr := call(t, cs, "things_log", nil) + if isErr { + t.Fatalf("unexpected tool error: %s", text) + } + if !w.logged { + t.Errorf("LogCompleted was not called") + } +} + +func TestImportForwarding(t *testing.T) { + w := &recordingWriter{} + cs := writeSession(t, w, openFake(fakeBackend{token: "tok"})) + payload := `[{"type":"to-do","attributes":{"title":"x"}}]` + text, isErr := call(t, cs, "things_import", map[string]any{"data": payload, "reveal": true}) + if isErr { + t.Fatalf("unexpected tool error: %s", text) + } + if w.importData != payload { + t.Errorf("import data = %q, want passthrough", w.importData) + } + if w.importToken != "tok" { + t.Errorf("import token = %q, want the backend's token", w.importToken) + } + if !w.importReveal { + t.Errorf("reveal flag not forwarded") + } +} + +func TestImportValidation(t *testing.T) { + w := &recordingWriter{} + cs := writeSession(t, w, openFake(fakeBackend{})) + for _, bad := range []struct{ name, data string }{ + {"empty", " "}, + {"not json", "not json"}, + {"not an array", `{"a":1}`}, + } { + t.Run(bad.name, func(t *testing.T) { + text, isErr := call(t, cs, "things_import", map[string]any{"data": bad.data}) + if !isErr { + t.Fatalf("expected validation error, got: %s", text) + } + }) + } + if w.importData != "" { + t.Errorf("ImportJSON should not be called for an invalid payload") + } +} + +func TestWriteAmbiguousRef(t *testing.T) { + ambig := &db.AmbiguousTaskError{Query: "Review PR", Matches: []model.Task{ + {UUID: "task-3", Title: "Review PR alpha"}, + {UUID: "task-4", Title: "Review PR beta"}, + }} + for _, tool := range []string{"things_edit", "things_complete", "things_cancel"} { + t.Run(tool, func(t *testing.T) { + w := &recordingWriter{} + cs := writeSession(t, w, openFake(fakeBackend{getErr: ambig})) + text, isErr := call(t, cs, tool, map[string]any{"task": "Review PR"}) + if !isErr { + t.Fatalf("expected ambiguous tool error, got: %s", text) + } + if !strings.Contains(text, "ambiguous") || !strings.Contains(text, "task-3") || !strings.Contains(text, "task-4") { + t.Errorf("error should list candidate UUIDs: %s", text) + } + }) + } +} + +// TestEditMissingTokenError uses the real Writer (no fake) so the actual +// things.UpdateTask validates the empty auth token and the helpful "enable +// Things URLs" message reaches the caller — without shelling out (the token +// check precedes any URL-scheme call). +func TestEditMissingTokenError(t *testing.T) { + cs := sessionCfg(t, mcpserver.Config{ + EnableWrites: true, + Open: openFake(fakeBackend{task: &model.Task{UUID: "task-1", Title: "Buy milk", Type: model.TypeTask}, token: ""}), + }) + text, isErr := call(t, cs, "things_edit", map[string]any{"task": "task-1", "title": "x"}) + if !isErr { + t.Fatalf("expected auth-token error, got: %s", text) + } + if !strings.Contains(text, "auth token is required") { + t.Errorf("want the auth-token guidance: %s", text) + } +} From 78e457e633c81e1c1559b30ab9db06798dd34022 Mon Sep 17 00:00:00 2001 From: Ryan Lewis Date: Sat, 30 May 2026 22:02:53 +0100 Subject: [PATCH 2/2] =?UTF-8?q?fix(mcp):=20address=20review=20=E2=80=94=20?= =?UTF-8?q?refuse=20project=20complete,=20tolerate=20token-read=20errors,?= =?UTF-8?q?=20reject=20empty=20import?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extra-high-effort review fixes: - things_complete now refuses a project (which would cascade-complete every to-do inside it) and points at things_edit_project (complete=true). The CLI guards that cascade behind an interactive y/N and refuses it non-interactively; MCP has no prompt, so don't do it silently. Drops the now unused Writer.CompleteProject. - things_edit / things_edit_project / things_import no longer treat a GetAuthToken read error as fatal (`token, _ :=`, like the CLI): a missing token yields the actionable "enable Things URLs" guidance, and a create-only import proceeds without one. - validateImportPayload rejects an empty array "[]" (parity with the CLI's validateImportJSON) so a no-op import isn't reported as submitted. - Add TestEveryToolsetRegistersTools so AllToolsets can't drift from registerTools (a declared toolset registering nothing now fails the build). --- internal/mcpserver/server.go | 8 ++-- internal/mcpserver/tools.go | 39 ++++++++++--------- internal/mcpserver/writes_test.go | 64 ++++++++++++++++++------------- 3 files changed, 62 insertions(+), 49 deletions(-) diff --git a/internal/mcpserver/server.go b/internal/mcpserver/server.go index 373a87f..4748a31 100644 --- a/internal/mcpserver/server.go +++ b/internal/mcpserver/server.go @@ -50,7 +50,6 @@ type Writer interface { UpdateTask(things.UpdateParams) error UpdateProject(things.UpdateProjectParams) error CompleteTask(uuid string) error - CompleteProject(uuid string) error CancelTask(uuid string) error LogCompleted() error ImportJSON(data, authToken string, reveal bool) error @@ -65,10 +64,9 @@ func (thingsWriter) UpdateTask(p things.UpdateParams) error { return things. func (thingsWriter) UpdateProject(p things.UpdateProjectParams) error { return things.UpdateProject(p) } -func (thingsWriter) CompleteTask(uuid string) error { return things.CompleteTask(uuid) } -func (thingsWriter) CompleteProject(uuid string) error { return things.CompleteProject(uuid) } -func (thingsWriter) CancelTask(uuid string) error { return things.CancelTask(uuid) } -func (thingsWriter) LogCompleted() error { return things.LogCompleted() } +func (thingsWriter) CompleteTask(uuid string) error { return things.CompleteTask(uuid) } +func (thingsWriter) CancelTask(uuid string) error { return things.CancelTask(uuid) } +func (thingsWriter) LogCompleted() error { return things.LogCompleted() } func (thingsWriter) ImportJSON(data, authToken string, reveal bool) error { return things.ImportJSON(data, authToken, reveal) } diff --git a/internal/mcpserver/tools.go b/internal/mcpserver/tools.go index 9b690e7..d869da5 100644 --- a/internal/mcpserver/tools.go +++ b/internal/mcpserver/tools.go @@ -59,8 +59,8 @@ const ( "Requires the Things URL auth token (Things → Settings → General → Enable Things URLs). " + "Submitted asynchronously; not confirmed by this tool." completeDesc = "Mark a Things3 to-do as completed. MUTATES your Things database. " + - "Identify it by UUID (preferred) or title. If the target is a project, the project AND all of its to-dos are " + - "completed. Applied synchronously via AppleScript (requires Things3 to be running)." + "Identify it by UUID (preferred) or title. Applies to to-dos only — complete a project (which also completes " + + "all its to-dos) with things_edit_project (complete=true). Applied synchronously via AppleScript (requires Things3 to be running)." cancelDesc = "Cancel a Things3 to-do. MUTATES your Things database. " + "Identify it by UUID (preferred) or title. Applies to to-dos only — cancel a project with things_edit_project " + "(cancel=true). Applied synchronously via AppleScript (requires Things3 to be running)." @@ -302,10 +302,10 @@ func (t toolset) edit(_ context.Context, _ *mcp.CallToolRequest, in editInput) ( if err != nil { return nil, nil, err } - token, err := b.GetAuthToken() - if err != nil { - return nil, nil, err - } + // Ignore a token-read error like the CLI's EditCmd (`token, _ :=`): a missing + // token yields "", and things.UpdateTask then surfaces the actionable + // "enable Things URLs" guidance rather than a raw DB error. + token, _ := b.GetAuthToken() if err := t.write.UpdateTask(things.UpdateParams{ ID: task.UUID, AuthToken: token, @@ -347,10 +347,7 @@ func (t toolset) editProject(_ context.Context, _ *mcp.CallToolRequest, in editP if project.Type != model.TypeProject { return nil, nil, fmt.Errorf("not a project: %s", project.Title) } - token, err := b.GetAuthToken() - if err != nil { - return nil, nil, err - } + token, _ := b.GetAuthToken() // see edit(): a token-read error is non-fatal, mirroring the CLI if err := t.write.UpdateProject(things.UpdateProjectParams{ ID: project.UUID, AuthToken: token, @@ -384,11 +381,12 @@ func (t toolset) complete(_ context.Context, _ *mcp.CallToolRequest, in complete if err != nil { return nil, nil, err } + // Completing a project cascades to every to-do inside it. The CLI guards + // that behind an interactive y/N and refuses it non-interactively; MCP has + // no prompt, so route project completion through the explicit, project-named + // tool rather than doing the cascade silently here. if task.Type == model.TypeProject { - if err := t.write.CompleteProject(task.UUID); err != nil { - return nil, nil, err - } - return textResult(fmt.Sprintf("Completed project %q (%s) and all of its to-dos.", task.Title, task.UUID)), nil, nil + return nil, nil, fmt.Errorf("%q is a project; complete it (and all its to-dos) with things_edit_project (complete=true)", task.Title) } if err := t.write.CompleteTask(task.UUID); err != nil { return nil, nil, err @@ -431,10 +429,9 @@ func (t toolset) importJSON(_ context.Context, _ *mcp.CallToolRequest, in import return nil, nil, err } defer func() { _ = b.Close() }() - token, err := b.GetAuthToken() - if err != nil { - return nil, nil, err - } + // Send the token when present but don't fail on a read error: a create-only + // payload needs no token, and the CLI's ImportCmd warns-and-continues too. + token, _ := b.GetAuthToken() if err := t.write.ImportJSON(in.Data, token, in.Reveal); err != nil { return nil, nil, err } @@ -553,6 +550,12 @@ func validateImportPayload(data string) error { if trimmed[0] != '[' { return fmt.Errorf("import: data must be a JSON array of items") } + // A valid JSON array starting with '[' is at minimum "[]"; reject an empty + // one (as the CLI's validateImportJSON does) so a no-op import isn't reported + // back as submitted. + if len(strings.TrimSpace(trimmed[1:len(trimmed)-1])) == 0 { + return fmt.Errorf("import: data array is empty") + } return nil } diff --git a/internal/mcpserver/writes_test.go b/internal/mcpserver/writes_test.go index e4256f1..af2e477 100644 --- a/internal/mcpserver/writes_test.go +++ b/internal/mcpserver/writes_test.go @@ -17,18 +17,17 @@ import ( // method instead of shelling out to open/osascript, so write handlers can be // tested for the params they forward. type recordingWriter struct { - addTask *things.AddParams - addProject *things.AddProjectParams - updateTask *things.UpdateParams - updateProject *things.UpdateProjectParams - completeTask string - completeProject string - cancelTask string - logged bool - importData string - importToken string - importReveal bool - err error // returned by every method when set + addTask *things.AddParams + addProject *things.AddProjectParams + updateTask *things.UpdateParams + updateProject *things.UpdateProjectParams + completeTask string + cancelTask string + logged bool + importData string + importToken string + importReveal bool + err error // returned by every method when set } func (w *recordingWriter) AddTask(p things.AddParams) error { w.addTask = &p; return w.err } @@ -41,10 +40,9 @@ func (w *recordingWriter) UpdateProject(p things.UpdateProjectParams) error { w.updateProject = &p return w.err } -func (w *recordingWriter) CompleteTask(uuid string) error { w.completeTask = uuid; return w.err } -func (w *recordingWriter) CompleteProject(uuid string) error { w.completeProject = uuid; return w.err } -func (w *recordingWriter) CancelTask(uuid string) error { w.cancelTask = uuid; return w.err } -func (w *recordingWriter) LogCompleted() error { w.logged = true; return w.err } +func (w *recordingWriter) CompleteTask(uuid string) error { w.completeTask = uuid; return w.err } +func (w *recordingWriter) CancelTask(uuid string) error { w.cancelTask = uuid; return w.err } +func (w *recordingWriter) LogCompleted() error { w.logged = true; return w.err } func (w *recordingWriter) ImportJSON(data, token string, reveal bool) error { w.importData, w.importToken, w.importReveal = data, token, reveal return w.err @@ -145,6 +143,23 @@ func TestToolsetGating(t *testing.T) { } } +// TestEveryToolsetRegistersTools guards against AllToolsets drifting from +// registerTools: every declared toolset must contribute at least one tool when +// writes are enabled. A name added to AllToolsets without a matching +// registration branch (or a typo in either) would otherwise pass validation yet +// expose nothing. +func TestEveryToolsetRegistersTools(t *testing.T) { + noOpen := func() (mcpserver.Backend, error) { return fakeBackend{}, nil } + for _, ts := range mcpserver.AllToolsets { + t.Run(ts, func(t *testing.T) { + cs := sessionCfg(t, mcpserver.Config{Open: noOpen, Toolsets: []string{ts}, EnableWrites: true}) + if got := toolNames(t, cs); len(got) == 0 { + t.Errorf("toolset %q registered no tools", ts) + } + }) + } +} + func TestAddForwarding(t *testing.T) { w := &recordingWriter{} cs := writeSession(t, w, mustNotOpen(t)) @@ -281,26 +296,23 @@ func TestCompleteForwarding(t *testing.T) { if w.completeTask != "task-1" { t.Errorf("CompleteTask = %q, want task-1", w.completeTask) } - if w.completeProject != "" { - t.Errorf("CompleteProject should not be called for a to-do") - } if !strings.Contains(text, "Completed to-do") { t.Errorf("result text = %q", text) } }) - t.Run("project completes cascade", func(t *testing.T) { + t.Run("project is rejected (route via edit_project)", func(t *testing.T) { w := &recordingWriter{} cs := writeSession(t, w, openFake(fakeBackend{task: &model.Task{UUID: "proj-1", Title: "Chores", Type: model.TypeProject}})) text, isErr := call(t, cs, "things_complete", map[string]any{"task": "proj-1"}) - if isErr { - t.Fatalf("unexpected tool error: %s", text) + if !isErr { + t.Fatalf("expected error completing a project, got: %s", text) } - if w.completeProject != "proj-1" { - t.Errorf("CompleteProject = %q, want proj-1", w.completeProject) + if !strings.Contains(text, "is a project") { + t.Errorf("error should point at edit_project: %s", text) } - if !strings.Contains(text, "Completed project") { - t.Errorf("result text = %q", text) + if w.completeTask != "" { + t.Errorf("CompleteTask should not be called for a project") } }) }