Skip to content

feat(mcp): opt-in write tools gated by --toolsets - #99

Open
ryanlewis wants to merge 2 commits into
feat/issue-82-mcpfrom
feat/mcp-writes
Open

ryanlewis wants to merge 2 commits into
feat/issue-82-mcpfrom
feat/mcp-writes

Conversation

@ryanlewis

Copy link
Copy Markdown
Owner

Stacked on #98 (base feat/issue-82-mcp; GitHub will retarget this to main once #98 merges).

Adds the write half of the MCP server, gated GitHub-MCP style.

What

  • --toolsets (comma-separated; all shorthand; THINGS_TOOLSETS env) mounts only the domain groups you use: tasks, projects, areas, tags, bulk. Each toolset owns its read tools (always on) and write tools (opt-in).
  • --read-only defaults on. Write tools register only with --read-only=false (or --no-read-only), so a bare things mcp stays read-only and byte-identical to feat(mcp): read-only MCP server baked into the CLI (things mcp) #98 (still exactly 6 read tools).
  • Full write parity across 8 tools: things_add, things_edit, things_complete, things_cancel, things_add_project, things_edit_project, things_log, things_import.

How

  • A Writer interface (default forwards to internal/things; all ops already exist and self-validate) keeps handlers unit-testable without shelling out to open/osascript.
  • UUID resolution reuses GetTask + the existing ambiguousError helper (the non-interactive subset of the CLI's resolveTask); no stdin prompts, no last-list cache.
  • edit/edit_project/import read the URL-scheme auth token from the DB (Backend.GetAuthToken). complete handles the project cascade; cancel points projects at edit_project.
  • Honest results: add/edit are submitted, not confirmed (URL scheme, async); complete/cancel are synchronous (AppleScript). Tool descriptions shout that they MUTATE.
  • Library API uses a positive Config.EnableWrites so the zero value is read-only (safe default); the CLI maps --read-only onto it.

Tests

  • internal/mcpserver/writes_test.go: toolset/read-only gating matrix, each write handler forwards correct params to a recording Writer fake, ambiguous-ref + missing-token error paths.
  • cmd/things/mcp_integration_test.go: spawns the real binary and asserts tools/list under default / --toolsets=tasks / --read-only=false (write tools are listed, never called).
  • make build, make test (race), make lint, make test-integration all green.

internal/skill/SKILL.md is intentionally unchanged (same standing exception as #98: the MCP server is a long-running server, not a Bash one-shot the skill drives).

Refs #82

ryanlewis added 2 commits May 30, 2026 21:32
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
…ad errors, reject empty import

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).
ryanlewis added a commit that referenced this pull request Sep 10, 2026
Closes #208

## What changed

`type` in JSON output is now the string `todo`, `project` or `heading`
instead of Things' raw TMTask integer. `status` already rendered as a
string, so the two fields were in two different styles on the same
object; they now share one.

The codec is `model.TaskType`, a named int that mirrors `model.Status`:
a `typeNames` map as the single source of truth for `String`,
`MarshalJSON` and `UnmarshalJSON`; an unrecognized raw code preserved as
its integer rather than collapsing to a lossy `"unknown"`;
`UnmarshalJSON` accepting the name or the legacy integer, and treating a
JSON null as a no-op. `Task.Type` changes from `int` to
`model.TaskType`, so comparisons against `model.TypeTask` /
`model.TypeProject` / `model.TypeHeading` compile unchanged.

Every JSON surface picks this up without its own change, because they
all marshal `model.Task` through the single encoder in
`internal/output`.

Plain text is unchanged, including the `(project)` marker on a project
row and the `Type: project` line in a project's detail block.
`model.Project` still has no `type` field.

One non-obvious knock-on: `notHeading` in `internal/db/tasks.go` builds
a SQL fragment with `fmt.Sprintf("... != %d", model.TypeHeading)`. That
constant is now a `fmt.Stringer`, so `%v` or `%s` would splice the word
`heading` into the SQL where `2` belongs. The constant is wrapped in
`int()` with a comment, so the fragment no longer depends on the format
verb.

## Why

A bare integer is an internal Things code. It reads badly for a human
and forces an agent or a jq filter to carry a lookup table that appears
nowhere in the output.

## How tested

`make test` and `make lint` both clean, on top of #206.

- `TestTaskTypeMarshalJSON`, `TestTaskTypeUnmarshalJSON`,
`TestTaskTypeRoundTripJSON` and `TestTaskTypeString` lock the three wire
names, the integer fallback, the null no-op and the rejection of unknown
names.
- `TestRunJSONRendersTypeAsString` walks the JSON surfaces end to end
and asserts the raw JSON text, not an unmarshalled `model.Task`. That
distinction matters: `UnmarshalJSON` still accepts the legacy integer,
so a decode-and-compare test would keep passing even if the encoder
regressed to emitting ints. It also fails on any `type` field holding a
bare number, matched by regex so it is independent of the encoder's
indentation and catches an unmapped code rather than only 0, 1 and 2.
- Both new tests were mutation-checked: reverting `MarshalJSON` to emit
the raw int fails them.
- Verified against the real Things database as well as the seeded one,
uuids not titles. `list today` returns `Vf6GPsvdnJYqTdD5f41H3A` as
`todo` and `X28bim63xxvGvWLncbfd7f` as `project`; `show` returns the
same two individually; `search` returns `BEFn6McBh2DFfU577KBaq7` as
`project`; `logbook` returns `L16wNJD3D84AEJwr14QWni` as `project`
across 1190 rows. `someday` currently holds no project, so only `todo`
appears there. `things projects` carries no `type` field at all, across
23 rows.

## Breaking change

| | Old | New |
|---|---|---|
| to-do | `"type": 0` | `"type": "todo"` |
| project | `"type": 1` | `"type": "project"` |
| heading | `"type": 2` | `"type": "heading"` |

Any caller matching on `.type==1` has to become `.type=="project"`.

Docs updated in `internal/skill/SKILL.md`, `docs/content/commands.md`,
`docs/content/agents.md` and `README.md`, including the worked `jq`
example in `agents.md` that filtered on `select(.type==0)`. Each page
describing JSON output carries a note naming the old integers, anchored
to v0.7.0 as the last release that emitted them.

Two things the docs now say that they did not before, both found in
review:

- `type` rides on task rows only, and headings are never returned by any
command, so `"heading"` never actually appears in output. It exists in
the codec because the codec has to be total over the three codes the
database uses.
- The value is not the vocabulary a `things import` payload takes. That
format is Things' own JSON URL scheme, which spells a to-do `"to-do"`.
An agent that copies `.type` from a listing into an import item gets an
item Things drops silently, since `validateImportJSON` only checks array
shape.

## For whoever cuts the release

The release body is read from `.github/releases/<Tag>.md` and
`.github/release.yml` has no breaking-change category, so a `feat!`
commit files under "Features" with no compatibility warning. The
`status` break in 8656bd8 hit the same gap. The release note wants the
table above, and a reminder to run `things skill install` so agents pick
up the new SKILL.md.

## Note for the open MCP branches

#98 and #99 serialise the same `model.Task` and will need a rebase onto
this. They pick up the string automatically; what needs checking is any
assertion or tool schema in those branches that describes `type` as an
integer.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant